From 9f480165dec3cbe13116abefe61cb8dff561771c Mon Sep 17 00:00:00 2001 From: Christopher McKay Date: Tue, 29 Sep 2026 03:22:00 -0400 Subject: [PATCH 1/3] fix(bin): converge every open owner onto a known terminal contribution settle_final only cleared a stale error on retry, so an owner whose saved row still said open kept projecting a merged or closed pull request as open after another owner's row had already recorded the terminal observation. Copy the known terminal observation to every owner whose saved row is not itself terminal, keeping that owner's own pending and notified state, and clear its error. --- bin/fm-contributions.sh | 8 +++++--- tests/fm-contributions.test.sh | 35 +++++++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/bin/fm-contributions.sh b/bin/fm-contributions.sh index 39cc1072d22..09da0f1db2e 100755 --- a/bin/fm-contributions.sh +++ b/bin/fm-contributions.sh @@ -56,7 +56,8 @@ # API failure leaves error evidence; an expired or absent observation is not # silence. FM_CONTRIBUTIONS_MAX_AGE (default 900 seconds) bounds freshness. # A URL whose last good observation is merged or closed is final: it is -# never re-read, stays fresh, and a stale error beside it is cleared once. +# never re-read, stays fresh, and every owner's saved row converges on that +# observation, with a stale error beside it cleared. # A genuine failure prints its unavailable line only when it starts an episode # (no prior owner has an error); a successful read ends the episode. # FM_CONTRIBUTIONS_NOW supplies an ISO UTC clock for tests, otherwise UTC now. @@ -338,8 +339,9 @@ settle_final() { # canonical-url task... : copy the URL's final observation to e jq -n --slurpfile final "$TMP/final.json" ' $final[0] + {error:null,pending:[],notified:[]}' > "$TMP/row.json" write_record "$task" "$TMP/row.json" - elif jq -e '.error != null' "$TMP/old.json" >/dev/null; then - jq '.error = null' "$TMP/old.json" > "$TMP/row.json" + elif jq -e '(.observation.state | IN("merged","closed") | not) or .error != null' "$TMP/old.json" >/dev/null; then + jq -n --slurpfile final "$TMP/final.json" --slurpfile old "$TMP/old.json" ' + $old[0] + {observation:$final[0].observation,error:null}' > "$TMP/row.json" write_record "$task" "$TMP/row.json" fi done diff --git a/tests/fm-contributions.test.sh b/tests/fm-contributions.test.sh index 8cf07a5f7bb..80d16b42379 100755 --- a/tests/fm-contributions.test.sh +++ b/tests/fm-contributions.test.sh @@ -766,6 +766,39 @@ test_late_owner_inherits_terminal_observation() { pass 'a late owner inherits a terminal observation without a forge read or wake' } +test_interrupted_multi_owner_poll_settles_every_owner() { + local home later=2026-09-17T08:00:00Z + home=$(new_home multi-owner-open) + forge_home "$home" + wrap_forge "$home" + record "$home" duplicate 8 open mergeable + mutate_record "$home" duplicate '.records[0].pending=[{token:"evt-1"}] | .records[0].notified=["evt-0"]' + mutate_record "$home" delivery '.records[0].observation.state="merged"' + printf 'down\n' > "$home/forge/fault" + with_home "$home" env FM_CONTRIBUTIONS_NOW="$later" "$ROOT/bin/fm-contributions.sh" poll >/dev/null \ + || fail 'interrupted multi-owner poll failed' + [ ! -s "$home/forge/calls" ] || fail 'a known terminal URL triggered a forge read' + jq -e '.records[0] | .observation.state == "merged" and .error == null + and .pending == [{token:"evt-1"}] and .notified == ["evt-0"]' \ + "$home/data/duplicate/contributions.json" >/dev/null \ + || fail "an owner whose saved row stayed open did not converge on the known terminal observation: $(cat "$home/data/duplicate/contributions.json")" + + home=$(new_home multi-owner-errored) + forge_home "$home" + wrap_forge "$home" + record "$home" duplicate 8 open mergeable + mutate_record "$home" duplicate '.records[0].error="forge observation unavailable or changed during read"' + mutate_record "$home" delivery '.records[0].observation.state="merged"' + printf 'down\n' > "$home/forge/fault" + with_home "$home" env FM_CONTRIBUTIONS_NOW="$later" "$ROOT/bin/fm-contributions.sh" poll >/dev/null \ + || fail 'interrupted multi-owner poll (errored owner) failed' + [ ! -s "$home/forge/calls" ] || fail 'a known terminal URL triggered a forge read (errored owner)' + jq -e '.records[0].observation.state == "merged" and .records[0].error == null' \ + "$home/data/duplicate/contributions.json" >/dev/null \ + || fail "an errored owner did not converge on the known terminal observation: $(cat "$home/data/duplicate/contributions.json")" + pass 'a retry converges every owner whose saved row is not terminal, keeping its own acknowledgement state' +} + test_done_task_open_pr_still_observed() { local home later=2026-09-17T08:00:00Z home=$(new_home done-open) @@ -1029,7 +1062,7 @@ test_late_owner_keeps_failure_episode_suppressed() { } failures=0 -for test_name in test_actor_coverage test_stale_verdict test_unchecked_is_not_silence test_newest_check_has_no_verdict test_comment_wake test_review_wake test_inline_wake test_ready_issue_wake test_fresh_issue_requires_maintainer test_missing_lane_remains_missing test_partial_freshness_keeps_measured_rows test_malformed_record_cannot_prove_silence test_issue_timeline_and_exact_ack test_verdict_retains_judged_head test_observed_replacement_refreshes_verdict test_unobserved_head_leaves_verdict_unknown test_away_yolo_is_fleet_work test_away_yolo_cross_home_is_fleet_work test_retired_and_unsupported_coverage test_unsupported_forge_is_not_fleet_work test_held_unsupported_forge_is_not_captain_work test_shared_contribution_signal_wakes_once test_watcher_keeps_diagnostics_separate_from_contribution_wakes test_expired_child_unsupported_forge_stays_unmeasured test_watcher_surfaces_new_contribution_once test_home_summary_coverage test_unreadable_pending_is_not_empty test_record_task_identity_matches_dirname_basename test_read_only_views_create_no_state test_budget_refusal_between_calls test_budget_bounded_call_timeout test_genuine_failure_near_deadline_is_unavailable test_shared_url_observed_once test_terminal_contribution_settles test_late_owner_inherits_terminal_observation test_done_task_open_pr_still_observed test_reservation_defers_later_url_when_fifteen_seconds_do_not_remain test_three_second_pr_reads_complete_fresh_in_one_cycle test_slow_read_deadline_kill_is_budget_refusal test_unmeasured_url_does_not_starve_the_tail test_budget_is_cut_down_to_the_watcher_check_bound test_arm_plumbs_a_configured_budget_into_the_check_shim test_unavailable_forge_records_error_and_wakes_once_per_episode test_late_owner_keeps_failure_episode_suppressed; do +for test_name in test_actor_coverage test_stale_verdict test_unchecked_is_not_silence test_newest_check_has_no_verdict test_comment_wake test_review_wake test_inline_wake test_ready_issue_wake test_fresh_issue_requires_maintainer test_missing_lane_remains_missing test_partial_freshness_keeps_measured_rows test_malformed_record_cannot_prove_silence test_issue_timeline_and_exact_ack test_verdict_retains_judged_head test_observed_replacement_refreshes_verdict test_unobserved_head_leaves_verdict_unknown test_away_yolo_is_fleet_work test_away_yolo_cross_home_is_fleet_work test_retired_and_unsupported_coverage test_unsupported_forge_is_not_fleet_work test_held_unsupported_forge_is_not_captain_work test_shared_contribution_signal_wakes_once test_watcher_keeps_diagnostics_separate_from_contribution_wakes test_expired_child_unsupported_forge_stays_unmeasured test_watcher_surfaces_new_contribution_once test_home_summary_coverage test_unreadable_pending_is_not_empty test_record_task_identity_matches_dirname_basename test_read_only_views_create_no_state test_budget_refusal_between_calls test_budget_bounded_call_timeout test_genuine_failure_near_deadline_is_unavailable test_shared_url_observed_once test_terminal_contribution_settles test_late_owner_inherits_terminal_observation test_interrupted_multi_owner_poll_settles_every_owner test_done_task_open_pr_still_observed test_reservation_defers_later_url_when_fifteen_seconds_do_not_remain test_three_second_pr_reads_complete_fresh_in_one_cycle test_slow_read_deadline_kill_is_budget_refusal test_unmeasured_url_does_not_starve_the_tail test_budget_is_cut_down_to_the_watcher_check_bound test_arm_plumbs_a_configured_budget_into_the_check_shim test_unavailable_forge_records_error_and_wakes_once_per_episode test_late_owner_keeps_failure_episode_suppressed; do ( "$test_name" ) || failures=$((failures + 1)) done [ "$failures" -eq 0 ] || fail "$failures contribution regressions" From eb6ebd88c401f8b8d00530fcbbca3914fe621a1f Mon Sep 17 00:00:00 2001 From: Christopher McKay Date: Tue, 29 Sep 2026 05:12:33 -0400 Subject: [PATCH 2/3] no-mistakes(review): Carry terminal checked_at when converging existing owner rows --- bin/fm-contributions.sh | 2 +- tests/fm-contributions.test.sh | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/bin/fm-contributions.sh b/bin/fm-contributions.sh index 09da0f1db2e..4e27e33e8a1 100755 --- a/bin/fm-contributions.sh +++ b/bin/fm-contributions.sh @@ -341,7 +341,7 @@ settle_final() { # canonical-url task... : copy the URL's final observation to e write_record "$task" "$TMP/row.json" elif jq -e '(.observation.state | IN("merged","closed") | not) or .error != null' "$TMP/old.json" >/dev/null; then jq -n --slurpfile final "$TMP/final.json" --slurpfile old "$TMP/old.json" ' - $old[0] + {observation:$final[0].observation,error:null}' > "$TMP/row.json" + $old[0] + {observation:$final[0].observation,checked_at:$final[0].checked_at,error:null}' > "$TMP/row.json" write_record "$task" "$TMP/row.json" fi done diff --git a/tests/fm-contributions.test.sh b/tests/fm-contributions.test.sh index 80d16b42379..b55cfecaf47 100755 --- a/tests/fm-contributions.test.sh +++ b/tests/fm-contributions.test.sh @@ -772,13 +772,15 @@ test_interrupted_multi_owner_poll_settles_every_owner() { forge_home "$home" wrap_forge "$home" record "$home" duplicate 8 open mergeable - mutate_record "$home" duplicate '.records[0].pending=[{token:"evt-1"}] | .records[0].notified=["evt-0"]' + mutate_record "$home" duplicate '.records[0].pending=[{token:"evt-1"}] | .records[0].notified=["evt-0"] + | .records[0].checked_at="2026-09-15T08:00:00Z"' mutate_record "$home" delivery '.records[0].observation.state="merged"' printf 'down\n' > "$home/forge/fault" with_home "$home" env FM_CONTRIBUTIONS_NOW="$later" "$ROOT/bin/fm-contributions.sh" poll >/dev/null \ || fail 'interrupted multi-owner poll failed' [ ! -s "$home/forge/calls" ] || fail 'a known terminal URL triggered a forge read' - jq -e '.records[0] | .observation.state == "merged" and .error == null + jq -e --slurpfile terminal "$home/data/delivery/contributions.json" '.records[0] | .observation.state == "merged" + and .error == null and .checked_at == $terminal[0].records[0].checked_at and .pending == [{token:"evt-1"}] and .notified == ["evt-0"]' \ "$home/data/duplicate/contributions.json" >/dev/null \ || fail "an owner whose saved row stayed open did not converge on the known terminal observation: $(cat "$home/data/duplicate/contributions.json")" From 1b410dc7cf31594e24e3da9d042ea734cdb8e48f Mon Sep 17 00:00:00 2001 From: Christopher McKay Date: Tue, 29 Sep 2026 06:04:11 -0400 Subject: [PATCH 3/3] no-mistakes(ci): I fixed Greptile finding ci-2 as you asked, with a change to tests/fm-contributions.test.sh only. The rule it enforces: when a retry converges an owner onto a URL that is already merged or closed, that owner gets the terminal owner's whole observation, not just its state. The same weak check appeared twice in test_interrupted_multi_owner_poll_settles_every_owner, so I fixed both: - **Open owner (line 784):** the check now also requires `.observation == $terminal[0].records[0].observation`. The existing checks for error, checked_at, pending and notified are unchanged. - **Errored owner (just below):** it only checked state and error before. It now reads the terminal owner's file and makes the same full-observation comparison. Adding the comparison alone would not have caught anything. The test fixtures gave both owners identical observations apart from `state`, so copying only the state would still have passed. In both cases I also set the terminal owner's observation head to HEAD_B, so the two observations now really differ. Verification: - The focused test passes against the current bin/fm-contributions.sh. - I temporarily changed `settle_final` so it copied only the state. The test then failed, reporting the owner still on the old head (HEAD_A). I restored the file afterwards, and `git status` shows only the test file modified. - The full tests/fm-contributions.test.sh suite exits 0. No product code changed. The other CI finding (ci-1, "Behavior portable serial 9") was left alone because you chose to ignore it --- tests/fm-contributions.test.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/fm-contributions.test.sh b/tests/fm-contributions.test.sh index b55cfecaf47..aa14fe28a0e 100755 --- a/tests/fm-contributions.test.sh +++ b/tests/fm-contributions.test.sh @@ -774,12 +774,13 @@ test_interrupted_multi_owner_poll_settles_every_owner() { record "$home" duplicate 8 open mergeable mutate_record "$home" duplicate '.records[0].pending=[{token:"evt-1"}] | .records[0].notified=["evt-0"] | .records[0].checked_at="2026-09-15T08:00:00Z"' - mutate_record "$home" delivery '.records[0].observation.state="merged"' + mutate_record "$home" delivery ".records[0].observation.state=\"merged\" | .records[0].observation.head=\"$HEAD_B\"" printf 'down\n' > "$home/forge/fault" with_home "$home" env FM_CONTRIBUTIONS_NOW="$later" "$ROOT/bin/fm-contributions.sh" poll >/dev/null \ || fail 'interrupted multi-owner poll failed' [ ! -s "$home/forge/calls" ] || fail 'a known terminal URL triggered a forge read' jq -e --slurpfile terminal "$home/data/delivery/contributions.json" '.records[0] | .observation.state == "merged" + and .observation == $terminal[0].records[0].observation and .error == null and .checked_at == $terminal[0].records[0].checked_at and .pending == [{token:"evt-1"}] and .notified == ["evt-0"]' \ "$home/data/duplicate/contributions.json" >/dev/null \ @@ -790,12 +791,13 @@ test_interrupted_multi_owner_poll_settles_every_owner() { wrap_forge "$home" record "$home" duplicate 8 open mergeable mutate_record "$home" duplicate '.records[0].error="forge observation unavailable or changed during read"' - mutate_record "$home" delivery '.records[0].observation.state="merged"' + mutate_record "$home" delivery ".records[0].observation.state=\"merged\" | .records[0].observation.head=\"$HEAD_B\"" printf 'down\n' > "$home/forge/fault" with_home "$home" env FM_CONTRIBUTIONS_NOW="$later" "$ROOT/bin/fm-contributions.sh" poll >/dev/null \ || fail 'interrupted multi-owner poll (errored owner) failed' [ ! -s "$home/forge/calls" ] || fail 'a known terminal URL triggered a forge read (errored owner)' - jq -e '.records[0].observation.state == "merged" and .records[0].error == null' \ + jq -e --slurpfile terminal "$home/data/delivery/contributions.json" '.records[0] | .observation.state == "merged" + and .observation == $terminal[0].records[0].observation and .error == null' \ "$home/data/duplicate/contributions.json" >/dev/null \ || fail "an errored owner did not converge on the known terminal observation: $(cat "$home/data/duplicate/contributions.json")" pass 'a retry converges every owner whose saved row is not terminal, keeping its own acknowledgement state'