From a5d700d3bd4cba94ecdc2412ce679c6c9dadac2c Mon Sep 17 00:00:00 2001 From: Samuel Reed Date: Mon, 19 Oct 2026 12:49:46 -0400 Subject: [PATCH] refactor: drop the round-1 review launch-marker migration Round-1 markers are date-keyed files at most 14 days old; the reap deletes them anyway. Removes the migration block and its four tests. --- CLAUDE.md | 2 +- bin/review.sh | 32 ---------------- tests/review-cmux.sh | 88 -------------------------------------------- 3 files changed, 1 insertion(+), 121 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 445850e..74b2cc6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -473,7 +473,7 @@ The suite pins `AUTODREAM_CONFIG` into its sandbox now that `run.sh` sources the `tests/review-skip.sh` covers `bin/review.sh`'s skip/launch decision against fixture reports, with an inline mock claude that just touches a marker file — if the marker exists, review.sh reached `exec claude`. It pins `AUTODREAM_CONFIG` to a nonexistent path so the host's own config (`AUTODREAM_TRIAGE_SURFACE=cmux`) can't leak in and spawn a real workspace mid-test. Run it after any review.sh change, and after changing PROMPT.md's Open-questions marker contract. -`tests/review-cmux.sh` covers `bin/review.sh`'s `AUTODREAM_TRIAGE_SURFACE=cmux` launch path (the morning review job's workspace popup) with a mock cmux binary — no real workspace ever spawns. It pins the same-day dedup marker contract against the report: first trigger opens and stamps a confirmed token (claim dir + `.confirmed` sibling), a same-day same-digest trigger is deduped, `--force` bypasses the marker, a failed create releases the claim and exits non-zero, the marker binds to the report content digest (a rebuilt report opens again), stale claims and tokens >14 days get reaped, a confirmed marker survives the reclaim grace window while an abandoned (unconfirmed, old) claim is reclaimed and a young unconfirmed claim suppresses concurrent triggers, the round-1 legacy `review-launched-$DATE` marker is migrated, a headless (non-TTY) run with missing cmux fails non-zero instead of falling back to a headless inline claude, and a logs dir that can't be created fails loudly. Run it after any change to the cmux branch of review.sh. +`tests/review-cmux.sh` covers `bin/review.sh`'s `AUTODREAM_TRIAGE_SURFACE=cmux` launch path (the morning review job's workspace popup) with a mock cmux binary — no real workspace ever spawns. It pins the same-day dedup marker contract against the report: first trigger opens and stamps a confirmed token (claim dir + `.confirmed` sibling), a same-day same-digest trigger is deduped, `--force` bypasses the marker, a failed create releases the claim and exits non-zero, the marker binds to the report content digest (a rebuilt report opens again), stale claims and tokens >14 days get reaped, a confirmed marker survives the reclaim grace window while an abandoned (unconfirmed, old) claim is reclaimed and a young unconfirmed claim suppresses concurrent triggers, a headless (non-TTY) run with missing cmux fails non-zero instead of falling back to a headless inline claude, and a logs dir that can't be created fails loudly. Run it after any change to the cmux branch of review.sh. ## The OMP adapter (`adapters/omp`) diff --git a/bin/review.sh b/bin/review.sh index dac175f..cf9bfeb 100755 --- a/bin/review.sh +++ b/bin/review.sh @@ -204,42 +204,10 @@ if [ "$AUTODREAM_TRIAGE_SURFACE" = "cmux" ]; then # fails on a non-empty dir. Kept separate, the claim dir stays empty and # both names still match the review-launched-* prune glob. LAUNCH_CONFIRMED="$LAUNCH_MARKER.confirmed" - # Users upgrading from the round-1 code carry a touch-created FILE - # review-launched-$DATE (date-bound, no content key). Honor it as a real - # launch and migrate it to the confirmed token so the new key owns state - # from here on — otherwise the same report opens a second workspace - # post-upgrade on the very day it was already triaged. - # if [ ! -f "$CLAUDE_BIN" ] || [ ! -x "$CLAUDE_BIN" ]; then echo "review.sh: claude binary not found or not executable ($CLAUDE_BIN); not launching triage workspace" >&2 exit 1 fi - # Legacy round-1 marker migration, gated on the SAME preflight as a real - # launch: stamping a confirmed token for a workspace whose claude cannot run - # would make every later trigger dedupe a report that was never triaged - # (deepseek 7.4). The mv PRESERVES the legacy marker's mtime, which on a - # late upgrade (>14 days) would be pruned immediately by the reap below — - # touch rebinds the token's mtime to now so the just-created migration - # survives (deepseek 7.3). - # - # But only when the marker is strictly NEWER than the current report: a - # date was triaged, then the report rebuilt for that date (new content, new - # digest), the legacy date-only marker must NOT be migrated onto the new - # report's key — that would mark unreviewed content as confirmed and - # swallow its triage. Compare mtimes; same-second ties (a rebuild landing in - # the same wall-clock second as the legacy touch) are treated as - # not-newer — migrate only when the legacy launch is unambiguously after the - # current report, letting a tied/older report open for triage (executor 6.1). - LEGACY_MARKER="$LOGS_DIR/review-launched-$DATE" - if [ -f "$LEGACY_MARKER" ]; then - LEGACY_MTIME=$(pstat_mtime "$LEGACY_MARKER" || echo 0) - if [ "$LEGACY_MTIME" -gt "$(pstat_mtime "$REPORT" || echo 0)" ]; then - echo "review.sh: migrating legacy marker $LEGACY_MARKER -> $LAUNCH_CONFIRMED" - mv "$LEGACY_MARKER" "$LAUNCH_CONFIRMED" 2>/dev/null && touch "$LAUNCH_CONFIRMED" - else - echo "review.sh: legacy marker $LEGACY_MARKER not newer than the current report; not migrated (may be rebuilt content)" - fi - fi CLAIM_GRACE=900 CLAIMED_OWN=0 # Reap stale state so it cannot grow unbounded: claim dirs and confirmed diff --git a/tests/review-cmux.sh b/tests/review-cmux.sh index 203e049..a704db1 100755 --- a/tests/review-cmux.sh +++ b/tests/review-cmux.sh @@ -12,8 +12,6 @@ # confirmed $LOGS_DIR/review-launched-$DATE-$DIGEST.confirmed (written # only after cmux returns success — dedup keys on THIS, so an # unconfirmed claim can age out and be reclaimed) -# legacy file $LOGS_DIR/review-launched-$DATE (round-1 touch-marker, -# migrated into a confirmed token on first run) # # The mock cmux (like the mock claude in review-skip.sh) records that it was # invoked and can be told to fail. review.sh runs cmux as @@ -281,31 +279,6 @@ test_in_progress_claim_suppresses(){ assert_grep "$root/out" 'already in progress' "in-progress notice is emitted" } -# Users upgrading from the round-1 code have a touch-created FILE -# review-launched-$DATE (date-only key). It must be honored as a real launch -# AND migrated into a confirmed token, so the same report doesn't pop up a -# second workspace right after upgrade. -test_legacy_marker_is_migrated(){ - local root; root=$(setup_env) - mk_report "$root" 2020-01-02 "$REPORT_OPEN" - mkdir -p "$root/autodream/logs" - # The migration base is the report mtime; force the report to an OLD stamp so - # the legacy marker (touched after) is unambiguously STRICTLY newer — BSD - # seconds-resolution would otherwise tie them and (correctly) refuse to - # migrate, which is not what this test is about. - touch -t 202006010000 "$root/dreams/2020-01-02.md" - touch "$root/autodream/logs/review-launched-2020-01-02" - run_review "$root" 2020-01-02 - assert_eq "$(cmux_calls "$root")" 0 "legacy marker suppresses a duplicate launch" - assert_grep "$root/out" 'migrating legacy marker' "migration is reported" - [ -f "$(marker_confirmed "$root" 2020-01-02)" ] \ - && ok "legacy marker becomes a confirmed token" \ - || no "legacy marker was not migrated" - [ ! -e "$root/autodream/logs/review-launched-2020-01-02" ] \ - && ok "legacy marker file no longer lingers" \ - || no "legacy marker file still present" -} - # The scheduled review job forces AUTODREAM_TRIAGE_SURFACE=cmux. If cmux is # missing, review.sh must fail non-zero (no headless inline claude from a # launchd job) when stdin is not a TTY — a manual terminal still gets the @@ -385,43 +358,6 @@ test_unwritable_claim_dir_fails(){ assert_eq "$(cmux_calls "$root")" 0 "no cmux launch when the claim could not be written" } -# A legacy date-only marker that predates the current report (the date was -# triaged, then the report rebuilt with new content before upgrade) must NOT be -# migrated onto the new digest — that would mark unreviewed content confirmed -# and swallow its triage. The report must open again. -test_legacy_marker_stale_is_not_migrated(){ - local root; root=$(setup_env) - mkdir -p "$root/autodream/logs" - # Legacy marker OLDER than the report: touch a stale marker at an ancient - # mtime, then write the report after it. - touch -t 202006010000 "$root/autodream/logs/review-launched-2020-01-02" - mk_report "$root" 2020-01-02 "$REPORT_OPEN" - run_review "$root" 2020-01-02 - assert_eq "$(cmux_calls "$root")" 1 "stale legacy marker does not suppress a rebuilt report" - assert_grep "$root/out" 'not newer than the current report' "stale legacy marker is reported as not migrated" - [ -f "$(marker_confirmed "$root" 2020-01-02)" ] \ - && ok "rebuilt report gets its own confirmed token" \ - || no "rebuilt report has no confirmed token" -} - -# A legacy marker and a rebuilt report sharing the same wall-clock second (BSD -# seconds-resolution mtime) must NOT migrate: the legacy launch is not -# unambiguously newer, so the rebuilt report opens for triage instead of being -# marked confirmed (executor 6.1). -test_legacy_marker_same_second_tie_not_migrated(){ - local root; root=$(setup_env) - mkdir -p "$root/autodream/logs" - mk_report "$root" 2020-01-02 "$REPORT_OPEN" - touch -t 202006011200 "$root/dreams/2020-01-02.md" - touch -t 202006011200 "$root/autodream/logs/review-launched-2020-01-02" # tie - run_review "$root" 2020-01-02 - assert_eq "$(cmux_calls "$root")" 1 "same-second legacy tie opens the report (no migration)" - assert_grep "$root/out" 'not newer than the current report' "tie is reported as not migrated" - [ -f "$(marker_confirmed "$root" 2020-01-02)" ] \ - && ok "tied report gets its own confirmed token" \ - || no "tied report has no confirmed token" -} - # A workspace whose embedded claude cannot start is a confirmed dead end: the # token would be written on cmux success and every later trigger dedupes on it. # Preflight the exact CLAUDE_BIN path we pass into the workspace env. @@ -507,26 +443,6 @@ MOCK assert_grep "$root/out2" 'already launched for this report' "dedup notice fired" } -# A late upgrade (>14 days after round-1's launch) still migrates: the legacy -# marker carries an old mtime, and mv would preserve it into the confirmed -# token — which the unconditional 14-day reap would then delete immediately, -# reopening the workspace (deepseek 7.3). The post-migration touch must rebind -# the token's mtime to now so it survives the prune. -test_legacy_marker_late_upgrade_survives_prune(){ - local root; root=$(setup_env) - mkdir -p "$root/autodream/logs" - # Report older still: legacy launch is AFTER the report (migrates) but both - # are >14 days old (would be pruned). - touch -t 202001011200 "$root/dreams/2020-01-02.md" - touch -t 202001021200 "$root/autodream/logs/review-launched-2020-01-02" - run_review "$root" 2020-01-02 - assert_eq "$(cmux_calls "$root")" 0 "late-upgrade legacy marker suppresses a duplicate launch" - assert_grep "$root/out" 'migrating legacy marker' "late-upgrade migration is reported" - [ -f "$(marker_confirmed "$root" 2020-01-02)" ] \ - && ok "late-upgrade migrated token survives the 14-day reap" \ - || no "late-upgrade migrated token was pruned (duplicate popup)" -} - # --------------------------------------------------------------------------- [ -x "$REVIEW" ] || { echo "FATAL: $REVIEW not executable"; exit 1; } @@ -542,10 +458,6 @@ test_stale_markers_are_pruned test_confirmed_marker_survives_grace test_abandoned_claim_is_reclaimed test_in_progress_claim_suppresses -test_legacy_marker_is_migrated -test_legacy_marker_stale_is_not_migrated -test_legacy_marker_same_second_tie_not_migrated -test_legacy_marker_late_upgrade_survives_prune test_headless_missing_cmux_fails test_interactive_missing_cmux_falls_back_inline test_logs_dir_failure_exits_nonzero