Add error-code parity contract test (#297 finding #12)#338
Merged
Conversation
Plugin handlers send `{"error": {"code": "<NAME>", ...}}` over the wire
and `godot_client.client.GodotCommandError` forwards `error.code`
verbatim. If a GDScript handler ever adds a code Python's `ErrorCode`
StrEnum doesn't define, the forwarded string still works at runtime but
agents and tests that match on `ErrorCode.X` silently miss it. The audit
roadmap (#297 finding #12) flagged this as a P2 drift hazard.
The new contract test parses `plugin/addons/godot_ai/utils/error_codes.gd`
and asserts:
1. The parser actually finds constants (guards a regex regression that
would otherwise let the next two tests pass vacuously).
2. Every GDScript code name appears in Python's `ErrorCode`.
3. String values match exactly for shared names.
The contract is one-way: GDScript ⊆ Python. Python carries server-only
codes (`SESSION_NOT_FOUND`, `COMMAND_TIMEOUT`, `PLUGIN_DISCONNECTED`)
the plugin never emits; the asymmetry is intentional and documented in
the test module docstring.
Pure-Python static-source contract test, no plugin/runtime code touched
— per the same-day precedent set by PR #317 ("No GDScript edits → live
test_run skipped per CLAUDE.md 'Pre-commit smoke test' rules"), live
editor smoke is N/A here.
https://claude.ai/code/session_01A7TqXrKqbriAf1fHaF97R8
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new unit-level contract test to enforce error code parity between the Godot plugin’s GDScript McpErrorCodes constants and Python’s ErrorCode StrEnum, closing audit finding #12 from issue #297. This ensures that any error code emitted by the plugin is representable as ErrorCode.<NAME> in Python, preventing silent mismatches in agents/tests that pattern-match on the enum.
Changes:
- Introduces
tests/unit/test_error_code_parity.py, which parsesplugin/.../utils/error_codes.gdvia regex and asserts it yields at least one constant (parser self-test). - Asserts GDScript ⊆ Python for error-code names (no plugin-emitted codes missing from Python’s
ErrorCode). - Asserts string-value equality for codes defined on both sides (no drift between constant values).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4
to
+7
| `godot_client.client.GodotCommandError` forwards `error.code` verbatim. If a | ||
| GDScript handler ever emits a code Python's `ErrorCode` enum doesn't know, the | ||
| forwarded string still works at runtime but agents and tests that match on | ||
| `ErrorCode.X` silently miss it. Tracked as #297 audit finding #12. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Copilot review on #338 — class lives at godot_ai.godot_client.client.GodotCommandError, not godot_client.client.GodotCommandError. Updating the path so readers and grep-based searches land on the right module. https://claude.ai/code/session_01DwmN9ouQq88KnabiW42y2C
Contributor
Author
Copilot review triage1 finding, agreed and applied in 25dd86e:
Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Base:
beta. Closes audit finding #12 from #297 — the lone "deferred outside audit" P2 item that's a self-contained, additive contract test (the kind of fix that PR #317 explicitly listed as still-open). Picks up where the audit-cleanup batch left off.What it does
Plugin handlers send
{"error": {"code": "<NAME>", ...}}over the wire andgodot_client.client.GodotCommandErrorforwardserror.codeverbatim (src/godot_ai/godot_client/client.py:86). If a GDScript handler ever emits a code Python'sErrorCodeStrEnum doesn't define, the forwarded string still works at runtime but agents and tests that match onErrorCode.Xsilently miss it. There was no contract test enforcing parity until now.The new file
tests/unit/test_error_code_parity.pyparsesplugin/addons/godot_ai/utils/error_codes.gdand asserts three invariants:_parse_gdscript_codes()returns at least one entry. Guards against a regex regression that would otherwise let the two parity assertions pass vacuously.McpErrorCodes.X) appears in Python'sErrorCodeenum.Direction is one-way on purpose
GDScript ⊆ Python. Python carries server-only codes (
SESSION_NOT_FOUND,COMMAND_TIMEOUT,PLUGIN_DISCONNECTED) the plugin never emits; that asymmetry is intentional (Python emits them inclient.py,_readiness.py,_meta_tool.py) and is documented in the test module docstring. The reverse-direction guard (Python codes the plugin should also know about) isn't a contract — it would force plugin churn for server-only codes — so it's deliberately not asserted.Why the parser is regex-based, not import-based
Importing the GDScript module from Python isn't possible without a Godot runtime. Re-declaring the codes in a Python list would defeat the purpose (the contract is between the two sources of truth). The regex
^\s*const\s+([A-Z_]+)\s*:=\s*"([A-Z_]+)"\s*$matches the existingerror_codes.gddeclaration shape exactly; if a future change uses a different declaration form the parser-self-test fires and someone has to touch this test file.Verification
ruff check src/ tests/— clean.venv/bin/python -m pytest tests/unit/test_error_code_parity.py -v— 3 passed.venv/bin/python -m pytest -q— 728 passed (725 pre-existing + 3 new)HYPOTHETICALcode → test 2 fires; mutated a string value → test 3 fires).No GDScript / runtime / plugin code touched → live
test_runskipped per CLAUDE.md "Pre-commit smoke test" rules and the same-day precedent set by PR #317. No self-update edits →script/local-self-update-smokeskipped per CLAUDE.md "Self-update" trigger list./simplify pass
Three review agents (reuse, quality, efficiency) reviewed the diff:
@functools.cacheon_parse_gdscript_codes()(efficiency win, eliminates 2 redundant file reads + regex passes per test run).PLUGIN_ROOThelper used by ~6 test files intotests/unit/_gdscript_text.py. That's a separate, broader refactor.Out of scope
This is the #12 contract-test slice only. Other deferred-outside-audit items in #297 (animation_handler split #13, debugger timer leak, CLI finder cache invalidation, structured
ci-check-gdscriptexit,setup-dev.ps1uv parity, broader self-update preload-alias depth-2+ work) remain open under #297 for any future agent. Each is a separate PR — they don't bundle naturally because they touch different layers and concerns.Closes audit finding #12 from #297.
https://claude.ai/code/session_01A7TqXrKqbriAf1fHaF97R8
Generated by Claude Code