Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces version 2 of the balatrobot API with breaking changes (!). The main focus is on restructuring how tags are represented in the game state and improving error messages across all endpoints to be more actionable and helpful.
Changes:
- Restructured tag representation from flat
tag_name/tag_effectfields to nestedtagobjects withkey,name, andeffectfields - Added
tagsarray to gamestate for tracking accumulated player-owned tags - Enhanced error messages across all endpoints with actionable guidance (e.g., suggesting
reroll,sell, etc.) - Added support for selling jokers when Buffoon packs are open (SMODS_BOOSTER_OPENED state)
- Implemented voucher effect extraction using game's localize function
- Added comprehensive Tag enum definitions and test coverage
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lua/utils/types.lua | Updated Blind type to use nested Tag object instead of flat tag_name/tag_effect fields; added Tag class definition |
| src/lua/utils/openrpc.json | Updated OpenRPC schema to reflect Tag object structure and enhanced sell endpoint description |
| src/lua/utils/gamestate.lua | Implemented voucher effect extraction, tag ownership tracking, and updated blind tag structure |
| src/lua/utils/enums.lua | Added comprehensive Tag.Key enum definitions for all Balatro tag types |
| src/lua/endpoints/sell.lua | Added support for SMODS_BOOSTER_OPENED state with Buffoon pack validation |
| src/lua/endpoints/skip.lua | Enhanced error message with actionable guidance |
| src/lua/endpoints/buy.lua | Enhanced error messages with actionable guidance |
| src/lua/endpoints/add.lua | Updated to support pack additions and refactored voucher handling to use dedicated SMODS function |
| src/lua/endpoints/use.lua | Enhanced error messages with actionable guidance |
| src/lua/endpoints/play.lua | Enhanced error message with actionable guidance |
| src/lua/endpoints/discard.lua | Enhanced error messages with actionable guidance |
| src/lua/endpoints/pack.lua | Enhanced error messages with actionable guidance |
| tests/lua/endpoints/test_skip.py | Added tests for tag accumulation after skipping blinds |
| tests/lua/endpoints/test_pack.py | Added tests for selling jokers during Buffoon pack selection |
| tests/lua/endpoints/test_gamestate.py | Added comprehensive test coverage for voucher effects and tag structure |
| tests/lua/endpoints/test_buy.py | Updated error message expectations |
| tests/lua/endpoints/test_add.py | Updated error message expectations |
| docs/api.md | Updated documentation to reflect new Tag structure and enhanced endpoint descriptions |
Comments suppressed due to low confidence (4)
src/lua/endpoints/add.lua:409
- The comment says "For jokers and consumables" but this else branch will also execute for vouchers and packs, creating unnecessary params that won't be used. Consider adding an explicit check:
elseif card_type == "joker" or card_type == "consumable" thento match the comment and avoid creating unused params for vouchers and packs.
else
-- For jokers and consumables - just pass the key
params = {
key = args.key,
skip_materialize = true,
stickers = {},
force_stickers = true,
}
-- Add edition if provided
if edition_value then
params.edition = edition_value
end
-- Add eternal if provided (jokers only - validation already done)
if args.eternal then
params.stickers[#params.stickers + 1] = "eternal"
end
-- Add perishable if provided (jokers only - validation already done)
if args.perishable then
params.stickers[#params.stickers + 1] = "perishable"
end
-- Add rental if provided (jokers only - validation already done)
if args.rental then
params.stickers[#params.stickers + 1] = "rental"
end
end
tests/lua/endpoints/test_skip.py:43
- Grammar issue in comment: "because it used immediately" should be "because it is used immediately"
assert "tag_investment" not in gamestate["tags"] # because it used immediately
tests/lua/endpoints/test_skip.py:53
- Grammar issue in comment: "because it used immediately" should be "because it is used immediately"
assert "tag_investment" not in gamestate["tags"] # because it used immediately
src/lua/utils/types.lua:58
- Typo: "bilnd" should be "blind"
---@field status Blind.Status Status of the bilnd
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert gamestate["tags"][0]["key"] == "tag_polychrome" | ||
| assert "tag_investment" not in gamestate["tags"] # because it used immediately |
There was a problem hiding this comment.
This test file has a bug that will cause test_skip_big_boss to fail. The test at line 54-58 (not shown in diff) expects the error message "Cannot skip Boss blind" but skip.lua line 39 now returns "Cannot skip Boss blind. Use select to select and play the boss blind." The expected error message in the test needs to be updated to match the new implementation.
| assert gamestate["blinds"]["big"]["status"] == "SKIPPED" | ||
| assert gamestate["blinds"]["boss"]["status"] == "SELECT" | ||
| assert gamestate["tags"][0]["key"] == "tag_polychrome" | ||
| assert "tag_investment" not in gamestate["tags"] # because it used immediately |
There was a problem hiding this comment.
This assertion is checking if the string "tag_investment" is in a list of tag objects. Since gamestate["tags"] is a list of objects (each with "key", "name", "effect" fields), the in operator will never find a string match. This should likely be checking if any tag in the list has key == "tag_investment", such as: assert not any(tag["key"] == "tag_investment" for tag in gamestate["tags"])
| assert gamestate["state"] == "BLIND_SELECT" | ||
| assert gamestate["blinds"]["boss"]["status"] == "SELECT" | ||
| assert gamestate["tags"][0]["key"] == "tag_polychrome" | ||
| assert "tag_investment" not in gamestate["tags"] # because it used immediately |
There was a problem hiding this comment.
This assertion is checking if the string "tag_investment" is in a list of tag objects. Since gamestate["tags"] is a list of objects (each with "key", "name", "effect" fields), the in operator will never find a string match. This should likely be checking if any tag in the list has key == "tag_investment", such as: assert not any(tag["key"] == "tag_investment" for tag in gamestate["tags"])
Previously, used_vouchers extracted descriptions from static voucher_data.description which was unreliable. Now uses get_voucher_effect() that fetches effect text via the game's localize() function with proper loc_vars for each voucher type. Also adds strip_color_codes() helper and comprehensive parametrized tests covering all 32 voucher types. Closes #154.
Improve error messages across 6 endpoint files by adding actionable guidance to help bots self-heal from failed tool calls. Changes: - buy.lua: Add endpoint suggestions for empty shop/slot errors - use.lua: Add card parameter guidance for consumable errors - discard.lua/play.lua: Add card limit suggestions - pack.lua: Add pack buying and target selection hints - skip.lua: Add boss blind selection suggestion - Update test_buy.py to match new error messages Closes #148.
Closes #143.
- Remove .claude/ directory (settings.json, skills/balatrobot/SKILL.md) - Remove CLAUDE.md in favor of AGENTS.md - Remove .mux/ directory (init, mcp.jsonc, tool_env, tool_post) - Remove .mdformat.toml (flags moved to Makefile) - Add AGENTS.md with project structure and rules - Add CONTEXT.md with glossary of domain terms - Add .agents/skills/balatrobot/SKILL.md for pi skill
Replace verbose boilerplate with minimal, curated entries covering macOS, Python, Lua, and project-specific ignores.
Inline --number and --exclude flags since the config file was removed.
Remove integration marker from pyproject.toml markers config.
The integration marker is no longer used. Remove auto-marking hooks from conftest files and the @pytest.mark.integration decorator.
Rename BalatroInstance module to match its primary export. Update import paths in tests.
Introduce BalatroPool with start/stop lifecycle, automatic port allocation, fail-fast cleanup, and async context-manager support. Includes InstanceInfo frozen dataclass for connection metadata.
StateFile wraps BalatroPool with a JSON state file (Jupyter pattern). Atomic write on pool start, delete on stop. Supports PID-based liveness checks, stale-file cleanup, and resolve-by-host:port or index. Add platformdirs dependency for cross-platform state directory.
Replace single BalatroInstance with pool-based serve. Adds -n / --num-instances flag for launching multiple instances. State file is written on start and cleaned up on exit.
New `balatrobot list` command reads the state file and displays running instances. Supports --json for machine-readable output. Register in CLI app.
Host/port are now optional — when omitted, the api command resolves the target from the state file. Supports --index flag to select an instance (default: 0). Falls back gracefully when no state file exists.
Replace separate host/port fixtures with unified InstanceInfo. Update Lua conftest, server tests, and CLI conftest imports.
Update __init__.py to re-export the new modules so users can import directly from the balatrobot package.
Drop uuid-based session IDs in favor of human-readable timestamps for log directory names. Rename session_id to session_name throughout BalatroPool and BalatroInstance.
Remove --host and --port from serve command. Ports are now always ephemeral and discovered via state file. Add validation that --num-instances is >= 1. Print session name and logs directory instead of misleading PID label.
Error if only one of --host or --port is provided. Both must be given for direct connection, or neither for state file discovery.
Wrap pool.start() and _write_state() in try/except in StateFile.__aenter__. If writing the state file fails after the pool starts, stop the pool before re-raising to prevent orphaned processes.
Replace misleading PID output with Started timestamp in list command.
Update tests for session_id -> session_name rename, remove unused imports flagged by ruff, and add regression tests for: - serve -n 0 and negative values - api --host without --port (and vice versa) - StateFile cleanup on write failure - serve --help no longer shows --host/--port
Add optional log_path field to InstanceInfo, propagate it through BalatroPool and StateFile, and display it in No running instances. output.
Restructure runbook around serve/list/api commands with examples, auto-discovery notes, and a logs section.
Add InstanceDiedError exception for subprocess death detection and check_alive() sync method that polls the subprocess and raises when the process has exited. Purely additive — no existing behavior changed.
Add check_alive() that delegates to each instance's check_alive(). Remove __aenter__/__aexit__ — lifecycle is now managed by the caller (Server) rather than the pool itself.
Replace instance-based context manager with static write()/delete() methods. Remove __init__, __aenter__/__aexit__, _write_state, _delete_state, and all instance properties (path, instances, is_started). Lifecycle ownership moves to the new Server class.
Add Server async context manager that owns pool start/stop, state file write/delete, and a run() supervision loop (SIGTERM + check_alive). Rewrite _serve() and serve() to use Server, replacing the old StateFile context manager. Add InstanceDiedError and StateFileBusy exception handling in serve().
- test_instance: add check_alive tests (healthy, dead, not-started) - test_pool: replace context manager tests with check_alive tests - test_state: replace context manager tests with write/delete tests - test_server: new file with 5 tests for Server lifecycle and run loop
Remove BalatroPool, InstanceInfo, and StateFile from __all__. These are CLI internals, not part of the public bot-author API. Keep APIError, BalatroClient, BalatroInstance, Config, __version__.
Add sys.platform != 'win32' guard around add_signal_handler/ remove_signal_handler. Wrap the supervision loop in try/finally to ensure the signal handler is always removed, even on InstanceDiedError.
Replace 'async context-manager protocol' with check_alive(). The pool no longer has __aenter__/__aexit__.
Verify that add_signal_handler is never called on win32 and that the supervision loop still exits cleanly.
Sends SIGTERM to the server PID from the state file, polls for process death (100ms intervals, 5s timeout). Idempotent — calling twice or on an already-dead process is safe.
Verify that Server.run() registers a SIGTERM signal handler on non-Windows platforms and that the full SIGTERM flow triggers clean shutdown (state file deleted, pool stopped).
- Move InstanceInfo from pool.py to instance.py, change log_path type to Path | None (stringify only at JSON boundary) - Rename _default_state_path to default_state_path (public API) - Fix Server.__aexit__ ordering: stop pool before deleting state file - Document BalatroPool as non-restartable after stop() - Fix stale docstring in test_instance.py - Drop redundant host/port args in api.py else branch
Uh oh!
There was an error while loading. Please reload this page.