Fix stockpile_food milestone to read current resources, not lifetime harvest - #341
Merged
Merged
Conversation
…harvest
The stockpile_food milestone ("Stockpile 10 food" / "The larder is filling
up") was reading game_state.harvested — lifetime food gathered, which never
decreases — so a colony that harvested 10 food and then ate it all would
still complete the milestone while holding 0 food. That contradicts the
name/description and the STOCKPILE type, and let milestone chains advance
past stockpile_food on the tick build_hut completed when starting food was
already at the stockpile target.
scripts/milestone_manager.gd: evaluate_milestone's MILESTONE_TYPE_STOCKPILE
branch now reads from game_state.resources (current stock) clamped to the
target, so progress regresses when food is spent.
tests/test_milestone_goals.gd:
- test_evaluate_stockpile_milestone now drives the milestone from a
`resources` dict and includes a harvested-then-spent scenario that proves
progress reflects the larder, not harvest history (10 harvested, 3 in
stock → progress 3, milestone not complete).
- test_is_milestone_complete and the save/load compatibility test seed
`resources` instead of `harvested` for the stockpile case.
tests/test_tick_integration.gd: build_completion test now starts at 5 food
(rather than 10) and runs 3 ticks (the tick the build actually completes on
at full food), so the chain ends on stockpile_food instead of racing past
it to build_workshop once the build completion bonus tips food over the
stockpile target.
Other milestone types are unchanged; the targeted suites stay green
(test_milestone_goals 107/107, test_tick_integration 32/32,
test_food_upkeep 28/28, test_e2e ok).
Fixes #334
Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
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 PR directly implements issue PR 334: the stockpile_food milestone now evaluates against current resources rather than lifetime harvested, with added regression coverage for the harvested-then-spent case.
Change-by-change findings
scripts/milestone_manager.gd: theMILESTONE_TYPE_STOCKPILEbranch now readsgame_state["resources"]and clamps progress to the target. This matches the milestone's name/description and the acceptance criterion that spending food regresses progress.tests/test_milestone_goals.gd: updated stockpile progress tests to useresources, added the regression scenario (resources.food = 3,harvested.food = 10) asserting progress 3 < total 10 and that the milestone is not complete, and updatedtest_is_milestone_completeandtest_save_load_compatibilityto useresources.tests/test_tick_integration.gd: adjusted the build-completion test to start with food 5 and run 3 ticks so the milestone chain stops atstockpile_foodafter the build bonus and food upkeep does not consume the asserted amount. This file is outside the two paths named in the issue, but the change is necessary to keep the suite green under the corrected semantics and does not alter gameplay behavior.
Sources
- Linked issue PR 334 (body and acceptance criteria)
- PR PR 341 diff
- Repository standards (AGENTS.md / CLAUDE.md)
- CI status for commit 716ba26: all checks passed (script test suite, export validations, smoke test, macOS validation)
Standards Compliance
- AGENTS.md/CLAUDE.md conventions are respected: all
assert_eqcalls use the required 3-argument form(actual, expected, name), avoiding the GDScript parse-error trap documented in CLAUDE.md. - No release-process or version-convention changes are introduced.
- No must_check items were present in the PR classifier output, so there are no mandatory checklist items to resolve beyond the linked issue.
Linked Issue Fit
- Issue ask: evaluate
stockpile_foodagainst current stockpiled food instead of lifetime harvested food — implemented inscripts/milestone_manager.gd. - Acceptance 1: progress derives from current
resources.food(clamped) and spending regresses progress — satisfied, with a dedicated regression test. - Acceptance 2:
test_evaluate_stockpile_milestoneupdated to useresourcesand includes a harvested-then-spent scenario proving progress < total — satisfied. - Acceptance 3: other milestone types unchanged — the diff only touches the
MILESTONE_TYPE_STOCKPILEbranch. - Acceptance 4: suites stay green — confirmed by CI script test suite success.
Tool Harness Findings
The tool harness section reports only "planning pending"; no concrete harness findings or failures were supplied.
Unknowns / Needs Verification
I did not independently rerun the Godot test suite in this review; CI results for commit 716ba26 report the script test suite and all validations as successful.
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
2 participants
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.
Review of branch foreman/wl-misospace-windowstead-334/issue-334: Milestone stockpile evaluation now reads
game_state["resources"]instead ofharvested, fixing the regression where spending food after harvesting made the stockpile milestone appear complete. …Fixes #334
Opened by foreman on review GO (workload wl-misospace-windowstead-334).