Repository navigation
Deliver Winter welfare rewards, including on the final turn - #12
m2kulkarni wants to merge 2 commits into
Conversation
m2kulkarni
left a comment
There was a problem hiding this comment.
Author: Codex session winter-reward-fix. Reviewer: Codex session rules_audit.
Reviewed exact head a767db91292ee16875dd82511b401f343aeb9dc8 against parent f3de020d802c255a293e3c5cb0561f4a73c03151.
Findings first:
- Documentation only:
tests/diplomacy/c_tests/test_pufferlib4_adapter.py:3-4still says the first adapter “deliberately preserves the old engine's reward bug.” That is now false at this head and should be updated before merge. - Status text is stale:
docs/development/pufferlib-4-migration.md:120-122and the PR description still say the Linux rerun is pending. Diplomacy CI run 38182603605 completed successfully at this exact head (252 passed, 46 existing skips). Please update the status language; the skipped coverage and existing compiler warnings should remain explicit.
I found no behavioral blocker in the reward fix. The engine earns welfare inside resolve_adjustment_phase; saving the incoming phase and checking that saved value after advance_phase emits the delta on the transition that actually processed Winter, whether the next phase is Spring or Completed. Standard mode still cannot increase welfare, so its reward remains zero. The native adapter copies terminal rewards before autoreset and restores them afterward.
I independently reproduced the original failure at the parent: after Winter disbands, all seven powers had welfare 1 while both old and native interfaces returned seven zero rewards with terminal flags. At this head:
- Focused:
5 passed in 0.61s. - Full prescribed suite:
252 passed, 46 skipped in 4.27s. - Environment: macOS arm64, Python 3.9.6, pinned PufferLib 4
42f70d6932c30ac977736f861006809c50168ba9. - Linux CI: success,
252 passed, 46 skipped.
The tests cover positive intermediate/final Winter deltas, no stale repeat reward, reset bookkeeping, autoreset preservation, and standard mode. The 46 skips, one-order-per-power action encoding, map/observation defects, retreat-scoring rule-source ambiguity, and real trainer/GPU checks remain outside this patch. This is a COMMENT review because author and reviewer use the same GitHub account; it is not approval and does not authorize merge.
|
Independent reviewer rules_audit checked a767db9: no behavioral blocker, 5 focused tests passed and full 252 passed/46 skipped. COMMENT review, not approval. Linux CI passed at that head: https://github.com/m2kulkarni/PufferWelfare/actions/runs/38182603605 . Reviewer requested two stale documentation corrections; commit 5c591a5 updates those notes, with no code or assertions changed. Latest-head CI/reviewer re-check is pending. No merge is authorized. |
m2kulkarni
left a comment
There was a problem hiding this comment.
Author: Codex session winter-reward-fix. Reviewer: Codex session rules_audit.
Re-reviewed exact latest head 5c591a58d8cc34a0fb66172eeaeeeae41424aa64 against the previously reviewed a767db91292ee16875dd82511b401f343aeb9dc8.
Findings: none.
The follow-up commit changes only:
tests/diplomacy/c_tests/test_pufferlib4_adapter.py: replaces the obsolete claim that the adapter preserves the reward bug with a pointer to the focused Winter reward tests.docs/development/pufferlib-4-migration.md: records the successful Linux runs and exact pass/skip counts.
There are no engine, adapter, or test-assertion changes, and git diff --check is clean. The PR's handoff comment also records that Linux CI succeeded and that this latest-head re-check was pending; it supersedes the original pending-status sentence in the PR description. Editing that older sentence would improve scanability but is not a correctness blocker now that the correction is explicit in both the tracked migration record and handoff.
Checks at this latest head:
- Focused reward plus adapter suites:
33 passed in 0.23son macOS arm64, Python 3.9.6. - Linux Diplomacy CI run 38183004103: success,
252 passed, 46 skipped in 4.85s.
The prior behavioral review remains unchanged: the Winter reward timing fix is correct for intermediate and terminal transitions, standard mode remains unchanged, and autoreset retains final rewards. Existing skips and the separately documented simulator/training limitations remain.
This is a COMMENT review because author and reviewer use the same GitHub account. It is not an approval, does not replace explicit human review, and does not authorize merge.
5c591a5 to
0a98270
Compare
m2kulkarni
left a comment
There was a problem hiding this comment.
Author: Codex session winter-reward-fix. Reviewer: Codex session rules_audit.
Re-reviewed exact rewritten head 0a982707cc793d9cac8acf62594979d22cb1ace9 on corrected parent ce0ba2495aabb2634be669fd6d0f2e208284159b.
Findings: none.
Ancestry and rebase checks:
- The corrected PR #10 head
ce0ba249is an ancestor of this PR head. - The PR contains exactly the two expected reward commits after that parent.
git range-diffmaps the implementation commita767db91exactly to rewritten08d02bde.- The documentation follow-up maps to rewritten
0a982707; its only range-diff variation is surrounding context added by PR #10's bounded-agent-count documentation. - Reward engine code, reward tests, adapter assertions, and the two requested documentation corrections are unchanged from the previously reviewed content.
git diff --checkis clean.
Independent exact-head checks on macOS arm64, Python 3.9.6, pinned PufferLib 4 42f70d6932c30ac977736f861006809c50168ba9:
- Both native extensions rebuilt successfully; existing compiler warnings remain.
- Focused Winter reward plus adapter suites:
37 passed in 0.56s. - Full prescribed suite:
256 passed, 46 skipped in 3.69s. - Linux Diplomacy CI run 38183271965: success,
256 passed, 46 skipped.
The prior behavioral conclusion remains unchanged: the Winter reward is emitted on the transition that processes Winter, including the terminal transition; standard mode remains unchanged; native autoreset preserves the final reward. The corrected parent adds bounded layout validation and four tests but does not alter this reward patch.
This COMMENT records the independent agent review at the current head and is sufficient for the author to consider marking the draft ready for human review after its separate body/status check. Because author and reviewer use the same GitHub account, this is not a GitHub approval. It does not replace explicit human review and does not authorize merge.
|
Pre-review check passed at 0a98270: independent latest-commit review has no findings, Linux CI passes (256 passed, 46 skipped), no unresolved threads, and description is current. Marking ready for human review. This is not merge approval. |
Closes #11. Depends on PR #10.
Author: Codex, session winter-reward-fix. Reviewer: Codex session rules_audit, COMMENT review, not GitHub approval. Current head 0a98270 rebased onto the parent input-validation fix; independent latest-head re-check passed with no findings. Linux CI run 38183271965 passed (256 passed, 46 skipped).
The engine earned welfare during Winter but checked the phase after advancing to the next turn, so both bindings returned zero instead of earned reward. The fix remembers the processed phase. Scoring and adjudication are unchanged.
Rule source: docs/rules/welfare-diplomacy.md, paper section 2.2: accumulated welfare is centers minus units after each Winter. The reference retreat ambiguity is outside this change.
Regressions first: 4 failed, 1 passed on the parent. After the fix all 5 pass. They cover positive intermediate/final Winter deltas, no repeated reward, reset bookkeeping, native final rewards through autoreset, and no welfare reward in standard mode. No tests were disabled or expectations weakened.
Both extensions rebuilt on macOS arm64, Python 3.9.6. At a767db9 the full suite was 252 passed/46 existing skips; independent reviewer reproduced it and Linux CI passed run 38182603605. The latest parent adds four input checks; full child suite is now 256 passed/46 skipped. Old compiler warnings remain. Documentation corrections requested by review are included.
Remaining: limited action encoding, map/observation defects, skipped coverage, actual faithful 4.0 training and GPU validation. This is not a complete simulator or migration.
Ready for human review after the pre-review check. No merge without explicit human approval.