docs: record the testing traps the loop learned - #323
Conversation
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: dsv4f@https://litellm.jory.dev/v1 (openai) — primary route
Recommendation: Approve. This is a documentation-only change that adds practical GDScript testing guidance to AGENTS.md. The claims are consistent with the repository's actual test harness, test helpers, and reward-tick implementation. CI is green on the PR commit. No blockers.
Change-by-change findings
AGENTS.md(+15 lines): adds a## Testing Traps (learned by the loop)section with three items. Verification against repo evidence:assert_eqtakes(actual, expected, name)intests/test_case.gd, and all existing call sites in the grep results use the required 3-argument form. The warning about 2-arg calls causing parse errors is consistent with the codebase's test harness documentation.GoalReward.tick_rewardsinscripts/goal_reward.gdchecksreward["remaining"] <= 0before decrementing, so a duration-N reward survives N ticks and expires on tick N+1. The documented off-by-one is accurate.- The headless invocation matches the CI
script-testsjob in.github/workflows/test.yml(godot --headless --path . --script res://tests/<suite>.gd). One minor accuracy nit is noted below.
- No source, scene, or manifest files changed; the PR has no runtime impact.
Standards Compliance
The change is an addition to AGENTS.md itself and does not conflict with any documented conventions in CLAUDE.md or the existing AGENTS.md content. It reinforces the existing Validation guidance about headless Godot runs and the repo's "GDScript warnings are treated as errors in practice" stance. No standards violations found.
Unknowns or Needs Verification
The original coder-godot agent prompt and the full PR PR 321 diff were not available in the corpus, so the "suite lost coverage without a single red assertion" anecdote could not be replayed independently. Repository history confirms PR 321 exists and the tick_rewards behavior described matches the current implementation, so this is non-blocking for a docs-only PR.
Sources
- PR metadata and diff for PR 323
AGENTS.md.github/workflows/test.ymlscripts/goal_reward.gdtests/test_case.gd- Repository history / commit log for PR 321
| suite lost its pre-existing coverage without a single red assertion.) | ||
| - Run suites headless the way CI does: | ||
| `godot --headless --path . --script res://tests/<suite>.gd` with | ||
| `HOME=/tmp XDG_DATA_HOME=/tmp/.local/share` in constrained environments. |
There was a problem hiding this comment.
Minor (docs): The documented headless invocation adds HOME=/tmp and XDG_DATA_HOME=/tmp/.local/share as part of 'the way CI does', but the CI script-tests job in .github/workflows/test.yml runs Godot headless without setting those environment variables; consider clarifying they are only for constrained local environments.
Automated finding from AI PR review.
Moves the GDScript testing traps out of the
coder-godotagent prompt and into this repo's AGENTS.md, where every coder and reviewer sees them — the per-language agents are being collapsed into one (joryirving/home-ops#8944 follow-up), and repo knowledge belongs with the repo. Content: theassert_eqarity/parse-error trap from #321, headless suite invocation, and thetick_rewardsoff-by-one from the same PR.