Repository navigation
Keep goal progress writable until the real turn settles - #3832
Merged
Merged
Conversation
Fixes #3830. Preserve the original reporting-turn identity and existing state/input checks; native terminal settlement stays with the attempt ledger.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
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
2 commits;3 files,+68/-40. Delete1 incorrect per-turn write ban, retain1 original reporting-turn stamp, add0 stores/flags/timers/turn identities. Three old rejection-pinning test sites now cover the actual permitted workflow, including a four-case action-family selection. Correct1 stopped-recovery fixture to retain the original released process and strengthen2 wrong-identity refusals; production recovery is unchanged.
Fixes #3830. Assigned by pi-commander52037; source author pi-ui-colleague. Merged as e87abcc; not installed or live-accepted. Forge merge identity and ancestry on origin/main independently read back, rc0. Author ran source checks only, no pytest/provider/native/UI. Commander's initial-head selection ran46passed/1failed; the corrected recovery control at0244662a94 then passed1/1 in1.16s, rc0. Both actual logs read. These are two selections at their recorded heads, not a claimed fresh47-case run. Installed build.json still names Cored4, so its old reporting veto remains live until the existing installer succeeds.
Why it failed
GoalReportAction.changeusedGoal.reported_turnboth to retain the original reporting identity and to forbid every later report in that turn. Active progress, standby, block and completion all inherit this owner. Thus a valid progress observation prevented a later dependency wait before its existing state/input-review publication could run.The guard did not own terminal settlement.
TurnGoalAccount.finishandVerifiedGoalSettlementsettle the actual producer terminal;LaunchPermit.advance_ready/record_verified_completionuse the existing SQLite claimed-attempt/generation fences. A status update is not a new launch or settlement.Before / after structure
The class graph is unchanged:
GoalAction → TransitionGoalAction → GoalReportAction → active / standby / completion / blocked actions.Before: the shared report owner stamped a turn, then rejected later actions solely because the stamp matched.
After: that same owner retains the real reporting turn and delegates every update to the existing precondition/order/state/wait checks. No action-name switch, special active/standby bypass, new reporting state or reset of the marker.
Unchanged owners:
CommsGoalToolchecks evidence and binds the active original GoalId before ModelInvocable publication.GoalPrecondition,GoalStateand the original generation lifecycle preserve CAS, actor, pause/block/retry and terminal-state rules.Deleted / corrected consumers
No compatibility shim, alternate codec, migration or wire-shape change.
Source ownership evidence
Retained source map:
/home/ts/.cache/agent-scratch/pi-ui-colleague/goal-report-source-map.jsonSHA256
e8d70ccec25a7cdd9a5b3b3b2e939692ee1ffd26b7881914ba5229f1ba74810e.Baseline Corea150; Core src/tests/tools plus installed Toadd4 production. Existing NRA parser, ModuleSyntaxIndex and canonical class index:1,556 modules;8,609 original ClassDefs;8,334 projections;275 unprojected OPEN;0 parse omissions;2,131 lexical sites. One worker, peak753,484KiB RSS. No local NRA gate or custom parser/resolver.
The source trace follows GoalAction/GoalState, Goal/ReportedTurnState, public model action creation, turn declarations, prepared waits, native terminal settlement and the SQL attempt owner. Dynamic/aliased/external bindings remain OPEN; JavaScript/C/native execution is not a Python projection or behavioral proof. A preliminary nonexistent
native/path was not treated as zero coverage; extensions/stack produced no reported-turn references in the later bounded text query.Open PR file inspection completed before edits. #3820 has no shared hunk (salvage52047); #3824 has no goal-owner file overlap. Existing #3767 changes context annotations/GoalRecords extraction, and #3609 changes retained Todo-review publication; their exact goal-owner hunks contain no repeated-report repair. No peer worktree edited or superseded.
Recovery fixture correction after the combined run
Commander's leased selection on initial head3d777a43d: 46passed/1failed,14.86s,rc1, actual log read at
/home/ts/.cache/agent-scratch/pi-commander/heartbeat-publication/goal-repeat-controls-1.log. The only failure wastests/test_goal_attempts.py:473, after the real stop already advanced exactly one ready generation. New progress/standby/block/completion/reopen cases passed; this is not an all-green report.The test passed the post-stop registry record to repeated recovery. That record is not the release's original process:
ReleasedOwner.matches,owner_lifecycle.py:209, compares both original incarnation and process identity.OwnerReleaseStore.record:343–355writesoriginal.process_identity;_finish_stopped_owners:630–645unregisters the registry record and records those retained originals.RegistryDocument.unregister:955–958replaces the current process binding withNoProcess.OwnerLifecycle.stop:563–568passes thefence_stopselection;FencedOwnerBatch.complete,owner_restart.py:241–242, passesself.captured.RegistryEdit.fence_restart_owners:241–252retains the selected Thread while advancing its fence.test_stop_grant.py:131also uses the original owner. Whole src/tests/tools retrieval plus installed Toad named-reference search found no other call sites; aliases/dynamic construction remain outside the binding proof.GenerationState.stopped_permit/ReadyGeneration,goal_generation.py:87–88/97, andNoLaunchPermit.claims,goal_turn_permit.py:68–69: an already-ready successor has no stopped claim to advance.LaunchPermit.advance_ready,goal_attempts.py:551, retains its claimed-attempt/generation SQL fences.Correction: repeated recovery receives
claimed_owner, matching both production callers and the release. The following negative selection now rejects both the post-stop cleared process and the foreign incarnation, asserting the successor generation is unchanged after each refusal. No release matcher, recovery guard, attempt/generation owner, process observation or UNKNOWN-input path was changed. The blank badge does not become an identity just because the test brought it twice.Bounded NRA extension before editing:11modules,111original ClassDefs/111local projections,0parse omissions,5call sites (2production/3test).
SourceModule/ModuleSyntaxIndex/build_class_family_indexat NRA9242a6b; selected-family extension, not a new full scan or runtime binding proof. Evidencegoal-recovery-source-trace.json, SHA256236b6fbb56de69ece64914400b896da18b0bb4edb7aa003c9b1c51b408195f0a. Command:PYTHONPATH=/home/ts/code/projects/NominalRefactorAdvisor timeout -s INT 60 /home/ts/.local/share/agent-comms/current/bin/python .goal-recovery-trace-once.py, rc0. One-shot removed after output/exit/no-running-borrower checks. Whole-tree retrieval initially named2nonexistent speculative modules; the subsequent actual-tree query covered src/tests/tools without treating those missing paths as zero coverage.Fresh open-PR file comparison found20open PRs and no other
tests/test_goal_attempts.pyedit. Older3767/3614/3607 touch production recovery sources, which this correction leaves unchanged. Fixture compiles andgit diff --checkpasses; no behavior rerun by this author. The integration owner'sgoal-repeat-controls-2.log/.rcsubsequently records the corrected0244662a94 case passing1/1 in1.16s, rc0; read independently. Its earlier46unchanged passing cases remain evidence from the original3d777a43d selection.Checks and remaining delivery
Done: both changed files compile from source;
git diff --check; whole-source marker/actor search; retained NRA evidence and direct semantic source review. These establish syntax/source relationships only.Integration owner completed the necessary combined selection and one failed-case correction rerun, not per-head reassurance. The original selection was:
tests/test_goal_resume.pytests/test_goal_waits.pytests/test_goal_attempts.pytests/test_turn_goal_account.pyExercise the real private Core tool journey with original turn identity: repeated progress, then a valid named-dependency wait, persistence/reopen, later permitted continuation; retain duplicate native-terminal/settlement refusal and failed/UNKNOWN input protection. No original input replay and no paid provider needed for the controlled owner path. Actual installed acceptance follows the existing single installer after its independent failure is repaired.
Assumed: the newly authorized workflow supersedes the former per-turn report limit; external/dynamic consumers are not certified by a lexical map. No runtime or installation success is inferred from these source checks.