~bigbes/agents-dev · parade

main · last commit 13 days ago · 7g0stsfu

← Back to the parade

ah-nyl.10 fix: reconcile review findings 1-6 (FIX-FIRST verdict) Past Stand

status: closed P0 bug
bd reopen ah-nyl.10
Created byEugene Blikh
Ownerbigbes@gmail.com
Created2026-07-13T00:36:33Z
Started2026-07-13T00:36:34Z
Updated2026-07-13T00:56:49Z
Closed2026-07-13T00:56:49Z
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/.

Depends on

  • ah-nyl — Stage 1 MVP: poll-driven happy path parent-child closed

Depended on by

Nothing depends on this issue.

No comments.

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
  • Eugene Blikh created the issue · 2026-07-13T03:36:32Z
  • Eugene Blikh added under epic ah-nyl · 2026-07-13T03:36:33Z
  • Eugene Blikh changed status to in_progress · 2026-07-13T03:36:33Z
  • Eugene Blikh closed the issue · 2026-07-13T03:56:49Z
    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
  • Eugene Blikh added label milestone:stage-1 · 2026-07-20T03:08:46Z
  • Eugene Blikh removed label milestone:stage-1 · 2026-07-20T03:10:19Z
Stored rows — what this pane was built from, as read
issues 1 row
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
dependencies 1 row
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
events 5 rows
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