main · last commit
13 days ago ·
7g0stsfu
ah-nyl.10 fix: reconcile review findings 1-6 (FIX-FIRST verdict)
Past Stand
bd reopen ah-nyl.10
| Created by | Eugene Blikh |
| Owner | bigbes@gmail.com |
| Created | 2026-07-13T00:36:33Z |
| Started | 2026-07-13T00:36:34Z |
| Updated | 2026-07-13T00:56:49Z |
| Closed | 2026-07-13T00:56:49Z |
Hostile review of internal/reconcile @ 2acf7c4 returned FIX-FIRST with findings below. Fix ALL six (1-3 blocking, 4-5 behavior, 6 test infrastructure that would have caught 1). Line numbers refer to internal/reconcile/reconcile.go @ 2acf7c4. F1 MAJOR (:294-304,:311-330): a FINISHED latest-run row is fed to check(); runner.Status ignores attempt, so a live re-attempt gets killed on the old row's ancient StartedAt (timeout) and/or the new attempt's result is written onto the previous attempt's row. FIX: check() only when run.State == RunStateRunning; when rec.State==in_progress and latest run is finished, route to adoptOrFail and there create the missing run row as attempt = latestRun.Attempt+1 when adopting a live runtime (instead of skipping CreateRun). F2 MAJOR (:250-262): UpsertTask/CreateRun failure AFTER successful runner.Start leaves the claim retryable -> next tick re-claims same attempt: Start wipes the live attempt's exit.json/events and opens a SECOND pi pane on the same worktree (duplicate paid agents). Same divergence from a daemon crash between Start and persist. FIX: (a) on persist failure after Start, compensate with best-effort runner.Kill before returning (log both errors); (b) before Start in claim, probe runner.Status for the computed attempt — if there is evidence of a live/completed runtime for it, adopt instead of double-starting. F3 MAJOR (:411-412,:216-220): comment is posted BEFORE MoveToBucket in the adoptOrFail fail-branch and the claim render-failure path -> persistent move failure = a new comment every 20s forever; the claim_failed event is also appended even when the move failed. FIX: reorder both sites to move-then-comment (finalize already does this); gate the claim_failed event on move success (mirror adopt_failed). F4 MINOR (:334-347): finalize unconditionally rewrites an already-finished run row on retry (e.g. timeout kill recorded, then UpsertTask failed; next tick Status says crashed -> outcome falsified timeout->crashed, FinishedAt smeared). FIX: when run.State == RunStateFinished, skip UpdateRun and derive the target task state from the stored run.Outcome (success->in_review, else failed). F5 MINOR (:135-136): an in_progress record whose card sits in an UNMAPPED bucket is skipped entirely -> no check(), no timeout; the run burns tokens unobserved. DECISION (fix this way): run check() for any record with State==in_progress regardless of the card's bucket (including unmapped and ready-awaiting-heal); card moves still only happen per the §10 bucket rules — i.e. decouple runtime supervision from card position. F6 MINOR (test honesty, fakes_test.go:290-299): (a) fakeBoard.MoveToBucket must update the task's bucket so consecutive iterations see the moved card; (b) add double-iterate zero-delta assertions (run iterate twice with unchanged inputs after claim, finalize, adopt-fail: second pass produces ZERO new side effects) — this is the level-triggered core property; (c) key fakeRunner status by (taskID, attempt) so per-attempt misattribution (F1) is expressible; add regression tests for F1 (finished run + live re-attempt: no kill, no overwrite; adoption creates attempt+1 row), F2 (persist-failure -> Kill called; probe-adopt on retry), F3 (move fails -> no comment posted, no claim_failed event; comment appears after move succeeds), F4 (finalize retry preserves outcome), F5 (unmapped bucket + in_progress -> check still enforces timeout). Constraints: scope = internal/reconcile/ ONLY (the reviewer's suggested fixes deliberately keep runner untouched). SPEC §10 stays authoritative — where these fixes refine it (F1 routing, F3 ordering, F5 supervision decoupling), implement per THIS bead; the orchestrator will sync SPEC afterwards. Conventions: testify + culpa (SPEC §13). Build/test ONLY ./internal/reconcile/... (never ./...). Verify with -race. Commit 'reconcile: fix review findings (check routing, claim compensation, move-then-comment, finalize guard, unmapped supervision)' staging only internal/reconcile; hooks bypassed (git -c core.hooksPath=/tmp/nohooks commit); retry on index.lock; do NOT push; do NOT touch .beads/.
ah-nyl
— Stage 1 MVP: poll-driven happy path
parent-child
closed
Nothing depends on this issue.
| id | ah-nyl.10 |
| content_hash | 867e80be5ab1f94c6768afe0ee66011466dec5fca7e42479d958babdf51ea649 |
| title | fix: reconcile review findings 1-6 (FIX-FIRST verdict) |
| description | Hostile review of internal/reconcile @ 2acf7c4 returned FIX-FIRST with findings below. Fix ALL six (1-3 blocking, 4-5 behavior, 6 test infrastructure that would have caught 1). Line numbers refer to internal/reconcile/reconcile.go @ 2acf7c4. F1 MAJOR (:294-304,:311-330): a FINISHED latest-run row is fed to check(); runner.Status ignores attempt, so a live re-attempt gets killed on the old row's ancient StartedAt (timeout) and/or the new attempt's result is written onto the previous attempt's row. FIX: check() only when run.State == RunStateRunning; when rec.State==in_progress and latest run is finished, route to adoptOrFail and there create the missing run row as attempt = latestRun.Attempt+1 when adopting a live runtime (instead of skipping CreateRun). F2 MAJOR (:250-262): UpsertTask/CreateRun failure AFTER successful runner.Start leaves the claim retryable -> next tick re-claims same attempt: Start wipes the live attempt's exit.json/events and opens a SECOND pi pane on the same worktree (duplicate paid agents). Same divergence from a daemon crash between Start and persist. FIX: (a) on persist failure after Start, compensate with best-effort runner.Kill before returning (log both errors); (b) before Start in claim, probe runner.Status for the computed attempt — if there is evidence of a live/completed runtime for it, adopt instead of double-starting. F3 MAJOR (:411-412,:216-220): comment is posted BEFORE MoveToBucket in the adoptOrFail fail-branch and the claim render-failure path -> persistent move failure = a new comment every 20s forever; the claim_failed event is also appended even when the move failed. FIX: reorder both sites to move-then-comment (finalize already does this); gate the claim_failed event on move success (mirror adopt_failed). F4 MINOR (:334-347): finalize unconditionally rewrites an already-finished run row on retry (e.g. timeout kill recorded, then UpsertTask failed; next tick Status says crashed -> outcome falsified timeout->crashed, FinishedAt smeared). FIX: when run.State == RunStateFinished, skip UpdateRun and derive the target task state from the stored run.Outcome (success->in_review, else failed). F5 MINOR (:135-136): an in_progress record whose card sits in an UNMAPPED bucket is skipped entirely -> no check(), no timeout; the run burns tokens unobserved. DECISION (fix this way): run check() for any record with State==in_progress regardless of the card's bucket (including unmapped and ready-awaiting-heal); card moves still only happen per the §10 bucket rules — i.e. decouple runtime supervision from card position. F6 MINOR (test honesty, fakes_test.go:290-299): (a) fakeBoard.MoveToBucket must update the task's bucket so consecutive iterations see the moved card; (b) add double-iterate zero-delta assertions (run iterate twice with unchanged inputs after claim, finalize, adopt-fail: second pass produces ZERO new side effects) — this is the level-triggered core property; (c) key fakeRunner status by (taskID, attempt) so per-attempt misattribution (F1) is expressible; add regression tests for F1 (finished run + live re-attempt: no kill, no overwrite; adoption creates attempt+1 row), F2 (persist-failure -> Kill called; probe-adopt on retry), F3 (move fails -> no comment posted, no claim_failed event; comment appears after move succeeds), F4 (finalize retry preserves outcome), F5 (unmapped bucket + in_progress -> check still enforces timeout). Constraints: scope = internal/reconcile/ ONLY (the reviewer's suggested fixes deliberately keep runner untouched). SPEC §10 stays authoritative — where these fixes refine it (F1 routing, F3 ordering, F5 supervision decoupling), implement per THIS bead; the orchestrator will sync SPEC afterwards. Conventions: testify + culpa (SPEC §13). Build/test ONLY ./internal/reconcile/... (never ./...). Verify with -race. Commit 'reconcile: fix review findings (check routing, claim compensation, move-then-comment, finalize guard, unmapped supervision)' staging only internal/reconcile; hooks bypassed (git -c core.hooksPath=/tmp/nohooks commit); retry on index.lock; do NOT push; do NOT touch .beads/. |
| design | |
| acceptance_criteria | |
| notes | |
| status | closed |
| priority | 0 |
| issue_type | bug |
| assignee | NULL |
| estimated_minutes | NULL |
| created_at | 2026-07-13T00:36:33Z |
| created_by | Eugene Blikh |
| owner | bigbes@gmail.com |
| updated_at | 2026-07-13T00:56:49Z |
| closed_at | 2026-07-13T00:56:49Z |
| closed_by_session | |
| external_ref | NULL |
| spec_id | |
| compaction_level | 0 |
| compacted_at | NULL |
| compacted_at_commit | NULL |
| original_size | NULL |
| sender | |
| ephemeral | 0 |
| wisp_type | |
| pinned | 0 |
| is_template | 0 |
| mol_type | |
| work_type | |
| source_system | |
| metadata | �{} |
| source_repo | |
| close_reason | landed e863502: check routing via run.State, adopt creates attempt+1, claim probe-adopt + compensation kill, move-then-comment, finalize finished-row guard, bucket-decoupled supervision, fakes keyed by (task,attempt) + zero-delta assertions; 34 tests -race green |
| event_kind | |
| actor | |
| target | |
| payload | |
| await_type | |
| await_id | |
| timeout_ns | 0 |
| waiters | |
| hook_bead | |
| role_bead | |
| agent_state | |
| last_activity | NULL |
| role_type | |
| rig | |
| due_at | NULL |
| defer_until | NULL |
| no_history | 0 |
| started_at | 2026-07-13T00:36:34Z |
| is_blocked | 0 |
| id | 15728877-b96c-5de5-b91e-3beadeddb18f |
| issue_id | ah-nyl.10 |
| type | parent-child |
| created_at | 2026-07-13T03:36:33Z |
| created_by | Eugene Blikh |
| metadata | �{} |
| thread_id | |
| depends_on_issue_id | ah-nyl |
| depends_on_wisp_id | NULL |
| depends_on_external | NULL |
| id | 019f58e7-7a45-7e67-9d93-ec7a3f5d777d |
| issue_id | ah-nyl.10 |
| event_type | created |
| actor | Eugene Blikh |
| old_value | |
| new_value | |
| comment | NULL |
| created_at | 2026-07-13T03:36:32Z |
| id | 019f58e7-7ca4-7f26-baa3-845f8f8e4709 |
| issue_id | ah-nyl.10 |
| event_type | status_changed |
| actor | Eugene Blikh |
| old_value | {"id":"ah-nyl.10","title":"fix: reconcile review findings 1-6 (FIX-FIRST verdict)","description":"Hostile review of internal/reconcile @ 2acf7c4 returned FIX-FIRST with findings below. Fix ALL six (1-3 blocking, 4-5 behavior, 6 test infrastructure that would have caught 1). Line numbers refer to internal/reconcile/reconcile.go @ 2acf7c4.\n\nF1 MAJOR (:294-304,:311-330): a FINISHED latest-run row is fed to check(); runner.Status ignores attempt, so a live re-attempt gets killed on the old row's ancient StartedAt (timeout) and/or the new attempt's result is written onto the previous attempt's row. FIX: check() only when run.State == RunStateRunning; when rec.State==in_progress and latest run is finished, route to adoptOrFail and there create the missing run row as attempt = latestRun.Attempt+1 when adopting a live runtime (instead of skipping CreateRun).\n\nF2 MAJOR (:250-262): UpsertTask/CreateRun failure AFTER successful runner.Start leaves the claim retryable -\u003e next tick re-claims same attempt: Start wipes the live attempt's exit.json/events and opens a SECOND pi pane on the same worktree (duplicate paid agents). Same divergence from a daemon crash between Start and persist. FIX: (a) on persist failure after Start, compensate with best-effort runner.Kill before returning (log both errors); (b) before Start in claim, probe runner.Status for the computed attempt — if there is evidence of a live/completed runtime for it, adopt instead of double-starting.\n\nF3 MAJOR (:411-412,:216-220): comment is posted BEFORE MoveToBucket in the adoptOrFail fail-branch and the claim render-failure path -\u003e persistent move failure = a new comment every 20s forever; the claim_failed event is also appended even when the move failed. FIX: reorder both sites to move-then-comment (finalize already does this); gate the claim_failed event on move success (mirror adopt_failed).\n\nF4 MINOR (:334-347): finalize unconditionally rewrites an already-finished run row on retry (e.g. timeout kill recorded, then UpsertTask failed; next tick Status says crashed -\u003e outcome falsified timeout-\u003ecrashed, FinishedAt smeared). FIX: when run.State == RunStateFinished, skip UpdateRun and derive the target task state from the stored run.Outcome (success-\u003ein_review, else failed).\n\nF5 MINOR (:135-136): an in_progress record whose card sits in an UNMAPPED bucket is skipped entirely -\u003e no check(), no timeout; the run burns tokens unobserved. DECISION (fix this way): run check() for any record with State==in_progress regardless of the card's bucket (including unmapped and ready-awaiting-heal); card moves still only happen per the §10 bucket rules — i.e. decouple runtime supervision from card position.\n\nF6 MINOR (test honesty, fakes_test.go:290-299): (a) fakeBoard.MoveToBucket must update the task's bucket so consecutive iterations see the moved card; (b) add double-iterate zero-delta assertions (run iterate twice with unchanged inputs after claim, finalize, adopt-fail: second pass produces ZERO new side effects) — this is the level-triggered core property; (c) key fakeRunner status by (taskID, attempt) so per-attempt misattribution (F1) is expressible; add regression tests for F1 (finished run + live re-attempt: no kill, no overwrite; adoption creates attempt+1 row), F2 (persist-failure -\u003e Kill called; probe-adopt on retry), F3 (move fails -\u003e no comment posted, no claim_failed event; comment appears after move succeeds), F4 (finalize retry preserves outcome), F5 (unmapped bucket + in_progress -\u003e check still enforces timeout).\n\nConstraints: scope = internal/reconcile/ ONLY (the reviewer's suggested fixes deliberately keep runner untouched). SPEC §10 stays authoritative — where these fixes refine it (F1 routing, F3 ordering, F5 supervision decoupling), implement per THIS bead; the orchestrator will sync SPEC afterwards. Conventions: testify + culpa (SPEC §13). Build/test ONLY ./internal/reconcile/... (never ./...). Verify with -race. Commit 'reconcile: fix review findings (check routing, claim compensation, move-then-comment, finalize guard, unmapped supervision)' staging only internal/reconcile; hooks bypassed (git -c core.hooksPath=/tmp/nohooks commit); retry on index.lock; do NOT push; do NOT touch .beads/.\n","status":"open","priority":0,"issue_type":"bug","owner":"bigbes@gmail.com","created_at":"2026-07-13T00:36:33Z","created_by":"Eugene Blikh","updated_at":"2026-07-13T00:36:33Z"} |
| new_value | {"status":"in_progress"} |
| comment | NULL |
| created_at | 2026-07-13T03:36:33Z |
| id | 019f58fa-0a56-76e1-a023-9ae14e1605d4 |
| issue_id | ah-nyl.10 |
| event_type | closed |
| actor | Eugene Blikh |
| old_value | |
| new_value | landed e863502: check routing via run.State, adopt creates attempt+1, claim probe-adopt + compensation kill, move-then-comment, finalize finished-row guard, bucket-decoupled supervision, fakes keyed by (task,attempt) + zero-delta assertions; 34 tests -race green |
| comment | NULL |
| created_at | 2026-07-13T03:56:49Z |
| id | 019f7cda-9084-75c4-a27a-976f23a200ad |
| issue_id | ah-nyl.10 |
| event_type | label_added |
| actor | Eugene Blikh |
| old_value | NULL |
| new_value | NULL |
| comment | Added label: milestone:stage-1 |
| created_at | 2026-07-20T03:08:46Z |
| id | 019f7cdb-fb4d-7a93-8601-13dcd1efb5e4 |
| issue_id | ah-nyl.10 |
| event_type | label_removed |
| actor | Eugene Blikh |
| old_value | NULL |
| new_value | NULL |
| comment | Removed label: milestone:stage-1 |
| created_at | 2026-07-20T03:10:19Z |
No comments.
Close reason