joshtronic/igor

Watch

fix: let a starved cascade stage jump the queue #448

Merged

joshtronic merged 2 commits from fix/441-cascade-starvation into master

2026-07-28 17:11:59 +00:00

igor commented

2026-07-28 16:31:10 +00:00

Collaborator

What this PR does

fix: let a starved cascade stage jump the queue

The second half of #441. The timer half shipped in #447.

The problem

Measured 2026-07-27: zero ceo/seo/sports/maintenance lines across a 2.5-hour window. No tick reached them — automerge, deploy watching, PR review and issue work consumed every one.

Not abstract: #435 had just shipped a same-tick path for acting on board steering left on a CEO digest, and a fast path behind a stage nothing reaches for hours is not fast.

The approach

Reordering is not the fix — it moves the starvation to whatever ends up last. Instead each gate records the tick at which its stage was reached, and a stage that goes CASCADE_STARVE_TICKS (20) unreached runs first on the next tick that gets here. Normal order is untouched whenever nothing is starving, which is the common case.

Every if do_x_tick; then became if cascade_run x; then, which stamps the stage and then calls do_x_tick, returning its status — so the gate semantics are unchanged.

Four decisions worth reviewing

The counter advances per tick that REACHES the cascade, not per tick overall. A tick exiting earlier (deploy barrier, health gate) gave no stage an opportunity, and a rescue placed here could not have helped it either. Age therefore counts opportunities missed, not wall time.

The rescue is one-shot. Running the stage stamps it, so it stops being starved immediately and cannot pin the cascade.

State writes use the same per-operation jq read-modify-write as weekly_mark_done, not a whole-document write. Other passes in the same tick write their own keys, and writing the whole file from a stale in-memory copy would silently drop them.

cascade_starved_stage fails CLOSED on unparseable state. Without that guard every lookup falls back to "never reached", the entire cascade looks starved at once, and it reorders on the strength of a corrupt file. The test suite caught this — it returned review before the guard went in.

Ties break toward the earlier stage in cascade order, so existing priority still decides between two equally starved stages.

Test plan

bash bin/test-cascade.sh — 18 assertions: counter monotonicity, fresh state starves nothing, a stale stage is rescued then stops being starved, most-starved wins, ties break by order, threshold is honoured, malformed state and non-numeric values degrade safely
Verified all 9 stage names resolve to real do_<stage>_tick functions — feedback and deferred live in lib/, not tick.sh, so a naive grep says they are missing. A typo here would call a nonexistent function on the live harness.
bash -n bin/tick.sh clean, shellcheck clean on both changed files
make test and make lint pass
End-to-end against a real state file: a 40-tick-stale stage is identified with its age, and after being reached reports as no longer starved

Diff is 77 net lines in tick.sh plus the new lib and suite — inside the per-issue budget.

Closes #441

## What this PR does - [x] fix: let a starved cascade stage jump the queue The second half of #441. The timer half shipped in #447. ## The problem Measured 2026-07-27: **zero** `ceo`/`seo`/`sports`/`maintenance` lines across a 2.5-hour window. No tick reached them — automerge, deploy watching, PR review and issue work consumed every one. Not abstract: #435 had just shipped a same-tick path for acting on board steering left on a CEO digest, and a fast path behind a stage nothing reaches for hours is not fast. ## The approach **Reordering is not the fix** — it moves the starvation to whatever ends up last. Instead each gate records the tick at which its stage was *reached*, and a stage that goes `CASCADE_STARVE_TICKS` (20) unreached runs **first** on the next tick that gets here. Normal order is untouched whenever nothing is starving, which is the common case. Every `if do_x_tick; then` became `if cascade_run x; then`, which stamps the stage and then calls `do_x_tick`, returning its status — so the gate semantics are unchanged. ## Four decisions worth reviewing **The counter advances per tick that REACHES the cascade**, not per tick overall. A tick exiting earlier (deploy barrier, health gate) gave no stage an opportunity, and a rescue placed here could not have helped it either. Age therefore counts *opportunities missed*, not wall time. **The rescue is one-shot.** Running the stage stamps it, so it stops being starved immediately and cannot pin the cascade. **State writes use the same per-operation `jq` read-modify-write as `weekly_mark_done`**, not a whole-document write. Other passes in the same tick write their own keys, and writing the whole file from a stale in-memory copy would silently drop them. **`cascade_starved_stage` fails CLOSED on unparseable state.** Without that guard every lookup falls back to "never reached", the entire cascade looks starved at once, and it reorders on the strength of a corrupt file. The test suite caught this — it returned `review` before the guard went in. Ties break toward the earlier stage in cascade order, so existing priority still decides between two equally starved stages. ## Test plan - [x] `bash bin/test-cascade.sh` — 18 assertions: counter monotonicity, fresh state starves nothing, a stale stage is rescued then stops being starved, most-starved wins, ties break by order, threshold is honoured, malformed state and non-numeric values degrade safely - [x] **Verified all 9 stage names resolve to real `do_<stage>_tick` functions** — `feedback` and `deferred` live in `lib/`, not `tick.sh`, so a naive grep says they are missing. A typo here would call a nonexistent function on the live harness. - [x] `bash -n bin/tick.sh` clean, `shellcheck` clean on both changed files - [x] `make test` and `make lint` pass - [x] End-to-end against a real state file: a 40-tick-stale stage is identified with its age, and after being reached reports as no longer starved Diff is 77 net lines in `tick.sh` plus the new lib and suite — inside the per-issue budget. Closes #441

igor added 1 commit

2026-07-28 16:31:11 +00:00

fix: let a starved cascade stage jump the queue

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

df7347d96c

Measured 2026-07-27: ZERO ceo/seo/sports/maintenance lines across a
2.5-hour window. No tick reached them -- automerge, deploy watching, PR
review and issue work consumed every one. That is not an abstract fairness
concern: igor#435 had just shipped a same-tick path for acting on board
steering, and a fast path behind a stage nothing reaches for hours is not
fast.

Reordering the cascade is not the fix -- it only moves the starvation to
whatever ends up last. Instead every gate now records the tick at which its
stage was REACHED, and a stage that goes CASCADE_STARVE_TICKS (20) without
being reached runs FIRST on the next tick that gets here. Normal order is
untouched whenever nothing is starving, which is the common case.

Details worth knowing:

- The counter advances per tick that REACHES the cascade, not per tick
  overall. A tick that exits earlier (deploy barrier, health gate) gave no
  stage an opportunity, and a rescue placed here could not have helped it
  either -- so age counts opportunities missed, not wall time.

- The rescue is one-shot: running the stage stamps it, so it stops being
  starved immediately. It cannot pin the cascade.

- State writes use the same per-operation jq read-modify-write as
  weekly_mark_done rather than holding the document in memory. Other passes
  in the same tick write their own keys, and a whole-file write from a
  stale copy would silently drop them.

- cascade_starved_stage fails CLOSED on unparseable state. Without that,
  every lookup falls back to "never reached", the whole cascade looks
  starved at once, and it reorders on the strength of a corrupt file.
  The test suite caught exactly that -- it returned "review" before the
  guard went in.

Ties break toward the earlier stage in cascade order, so the existing
priority still decides between two equally starved stages.

Verified: all 9 stage names resolve to real do_<stage>_tick functions
(feedback and deferred live in lib/, not tick.sh), bash -n clean,
shellcheck clean, make test and make lint pass, and an end-to-end run
against a real state file shows a 40-tick-stale stage identified, rescued,
and then no longer starved.

Closes #441

igor commented

2026-07-28 16:34:23 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for df7347d9: success

Adds a starvation-fairness gate to the tick cascade (lib/cascade.sh + pure-function suite) and wraps each do_*_tick gate in cascade_run. The design (age = opportunities missed, one-shot rescue, per-key jq RMW, fail-closed on corrupt state) is sound and well-argued, CI is green, scope matches #441, and the log line satisfies the observability criterion. Blocking on three concrete defects, all in the new code.

Blocking

1. lib/cascade.sh — the ${1:-\{\}} defaults expand to a literal \{\}, not {}.
Inside double quotes, bash only strips a backslash before $ \ " \—{survives verbatim. Socascade_tick_number/cascade_bump_tick/cascade_mark_reached/cascade_stage_agewith a missing or empty first argument feed jq the invalid document{}. It happens to degrade safely today because every one of those has a || echo 0/|| printf '%s' "$1"fallback — but that safety is accidental, not designed, andcascade_bump_tickwould emit{}as its "state" for a caller to consume. This path is reachable in production:cascade_state_file_readreturns an empty string (not{}) if the state file exists but is zero-length or whitespace, and the empty string triggers the :-default. Fix:local state=${1:-}; [ -n "$state" ] || state='{}'` (or equivalent). No test covers the no-arg/empty-arg path, which is why this slipped — please add one.

2. bin/tick.sh — nothing asserts that every name in CASCADE_STAGES resolves to a real function.
cascade_run does "do_${stage}_tick"; a typo yields exit 127, which the gate reads as "this stage did no work", so the stage is silently never run and silently never reachable again — the exact failure class the PR description says it hand-verified ("feedback and deferred live in lib/, so a naive grep says they are missing"). Hand-verification doesn't survive the next stage rename. This repo already has bin/check-sync.sh for precisely this kind of contract; either add a check there or an assertion in bin/test-cascade.sh that each of the 9 names has a defined do_<stage>_tick. Note the list is also duplicated — once in CASCADE_STAGES, once in the nine literal cascade_run x call sites — so drift between them is a second silent-death mode.

3. bin/tick.sh:4130ish — a rescued stage runs twice in the same tick.
The rescue calls cascade_run "$CASCADE_STARVED"; if it returns non-zero the cascade falls through and reaches that same stage again at its normal position, invoking do_<stage>_tick a second time. For stages whose "no work" decision is a cheap guard this is just wasted time; for sports/ceo/feedback/deferred (model calls) it's potentially a duplicated model call, and for any stage that performs a side effect before deciding to return non-zero it's a duplicated side effect. I can't rule that out from the diff since the do_*_tick bodies aren't shown. Either skip the starved stage in the normal pass on the tick it was rescued, or state explicitly (with a comment) that every do_*_tick is side-effect-free on the false path.

Non-blocking

cascade_bump_tick_file / cascade_mark_reached_file handle a missing state file ([ -f "$f" ] || echo '{}' > "$f") but not an empty or corrupt one: jq fails, rm -f "$tmp", and the write is dropped silently. If discretionary-state.json is ever left unparseable, the tick counter stops advancing and the entire fairness feature is permanently and invisibly dead. A log on the failure branch is one line and makes that diagnosable.
Neither file-level wrapper (cascade_bump_tick_file, cascade_mark_reached_file, cascade_run) has any test coverage — the suite is pure-function only, as its header admits. That's the layer holding the mktemp/mv and the function-name dispatch, i.e. the risky part.
mktemp defaults to $TMPDIR; if that's a different filesystem from the state dir, mv degrades to copy+unlink and is no longer atomic. The description says this mirrors weekly_mark_done, so it's a pre-existing pattern, not a regression — just noting it, since #441 explicitly called out the non-atomic jq+mv hazard.
Description says "18 assertions"; I count 16 eq calls in bin/test-cascade.sh. Also "shellcheck clean on both changed files" — three files changed. Cosmetic, but the checklist is what a human trusts.
AGENTS.md isn't touched. A tick can now execute stages out of the documented cascade order; check-sync.sh only enforces OUTCOME-sentinel parity so CI is happy, but the behavioural contract doc arguably should mention the rescue path.
The agent.timer acceptance criteria from #441 are claimed as shipped in #447 — plausible split, not verifiable from here, flagged only so the human knows to confirm #441 can actually close.

What I liked

The fail-closed guard in cascade_starved_stage (with the test that would have caught its absence), the strict -gt giving deterministic tie-breaks toward cascade order, and the ramp behaviour on a fresh state file (ticks 21-29 sweep each unreached stage once, then settle) are all correct and well covered.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `df7347d9`: **success** Adds a starvation-fairness gate to the tick cascade (`lib/cascade.sh` + pure-function suite) and wraps each `do_*_tick` gate in `cascade_run`. The design (age = opportunities missed, one-shot rescue, per-key jq RMW, fail-closed on corrupt state) is sound and well-argued, CI is green, scope matches #441, and the log line satisfies the observability criterion. Blocking on three concrete defects, all in the new code. ## Blocking **1. `lib/cascade.sh` — the `${1:-\{\}}` defaults expand to a literal `\{\}`, not `{}`.** Inside double quotes, bash only strips a backslash before `$ \` " \\` — `\{` survives verbatim. So `cascade_tick_number` / `cascade_bump_tick` / `cascade_mark_reached` / `cascade_stage_age` with a missing or empty first argument feed jq the invalid document `\{\}`. It happens to degrade safely today because every one of those has a `|| echo 0` / `|| printf '%s' "$1"` fallback — but that safety is accidental, not designed, and `cascade_bump_tick` would emit `\{\}` as its "state" for a caller to consume. This path is reachable in production: `cascade_state_file_read` returns an empty string (not `{}`) if the state file exists but is zero-length or whitespace, and the empty string triggers the `:-` default. Fix: `local state=${1:-}; [ -n "$state" ] || state='{}'` (or equivalent). No test covers the no-arg/empty-arg path, which is why this slipped — please add one. **2. `bin/tick.sh` — nothing asserts that every name in `CASCADE_STAGES` resolves to a real function.** `cascade_run` does `"do_${stage}_tick"`; a typo yields exit 127, which the gate reads as "this stage did no work", so the stage is silently never run *and* silently never reachable again — the exact failure class the PR description says it hand-verified ("`feedback` and `deferred` live in `lib/`, so a naive grep says they are missing"). Hand-verification doesn't survive the next stage rename. This repo already has `bin/check-sync.sh` for precisely this kind of contract; either add a check there or an assertion in `bin/test-cascade.sh` that each of the 9 names has a defined `do_<stage>_tick`. Note the list is also duplicated — once in `CASCADE_STAGES`, once in the nine literal `cascade_run x` call sites — so drift between them is a second silent-death mode. **3. `bin/tick.sh:4130ish` — a rescued stage runs twice in the same tick.** The rescue calls `cascade_run "$CASCADE_STARVED"`; if it returns non-zero the cascade falls through and reaches that same stage again at its normal position, invoking `do_<stage>_tick` a second time. For stages whose "no work" decision is a cheap guard this is just wasted time; for `sports`/`ceo`/`feedback`/`deferred` (model calls) it's potentially a duplicated model call, and for any stage that performs a side effect *before* deciding to return non-zero it's a duplicated side effect. I can't rule that out from the diff since the `do_*_tick` bodies aren't shown. Either skip the starved stage in the normal pass on the tick it was rescued, or state explicitly (with a comment) that every `do_*_tick` is side-effect-free on the false path. ## Non-blocking - `cascade_bump_tick_file` / `cascade_mark_reached_file` handle a *missing* state file (`[ -f "$f" ] || echo '{}' > "$f"`) but not an empty or corrupt one: jq fails, `rm -f "$tmp"`, and the write is dropped **silently**. If `discretionary-state.json` is ever left unparseable, the tick counter stops advancing and the entire fairness feature is permanently and invisibly dead. A `log` on the failure branch is one line and makes that diagnosable. - Neither file-level wrapper (`cascade_bump_tick_file`, `cascade_mark_reached_file`, `cascade_run`) has any test coverage — the suite is pure-function only, as its header admits. That's the layer holding the mktemp/mv and the function-name dispatch, i.e. the risky part. - `mktemp` defaults to `$TMPDIR`; if that's a different filesystem from the state dir, `mv` degrades to copy+unlink and is no longer atomic. The description says this mirrors `weekly_mark_done`, so it's a pre-existing pattern, not a regression — just noting it, since #441 explicitly called out the non-atomic jq+mv hazard. - Description says "18 assertions"; I count 16 `eq` calls in `bin/test-cascade.sh`. Also "shellcheck clean on both changed files" — three files changed. Cosmetic, but the checklist is what a human trusts. - `AGENTS.md` isn't touched. A tick can now execute stages out of the documented cascade order; `check-sync.sh` only enforces OUTCOME-sentinel parity so CI is happy, but the behavioural contract doc arguably should mention the rescue path. - The `agent.timer` acceptance criteria from #441 are claimed as shipped in #447 — plausible split, not verifiable from here, flagged only so the human knows to confirm #441 can actually close. ## What I liked The fail-closed guard in `cascade_starved_stage` (with the test that would have caught its absence), the strict `-gt` giving deterministic tie-breaks toward cascade order, and the ramp behaviour on a fresh state file (ticks 21-29 sweep each unreached stage once, then settle) are all correct and well covered. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-28 16:34:24 +00:00

2 minutes 26 seconds

igor self-assigned this

2026-07-28 16:34:24 +00:00

igor commented

2026-07-28 16:43:45 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 1 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 1 new commit(s).

igor removed their assignment

2026-07-28 16:44:11 +00:00

igor added 1 commit

2026-07-28 16:44:11 +00:00

fix: address review on the cascade starvation gate

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

5a9aa0c3eb

Three blocking defects from the shadow review, all in the new code.

1. lib/cascade.sh's `${1:-\{\}}` defaults expanded to a literal
   backslash-brace, not `{}` -- bash strips a backslash inside double
   quotes only before $ ` " \. Every call site happened to have a
   fallback behind it, so it degraded safely by accident, but
   cascade_bump_tick would hand a caller `\{\}` as its "state". The
   path is reachable: cascade_state_file_read returns "" (not "{}")
   for a zero-length state file. A `_cascade_state` helper now
   normalises, and the suite covers the no-arg and empty-arg paths.

2. Nothing asserted that each CASCADE_STAGES name resolves to a real
   do_<stage>_tick. A typo exits 127, which the gate reads as "did no
   work" -- silently never run, and silently never starved either
   because cascade_run already stamped it. check-sync.sh now checks
   both the function and the drift between the list and the nine
   literal `cascade_run x` gates. Verified it fires on a typo'd stage.

3. A rescued stage that returned non-zero fell through and ran again
   at its normal position -- a duplicated model call for
   sports/ceo/feedback/deferred. cascade_run now skips the stage the
   rescue already ran this tick.

Also from the non-blocking list: log when a jq read-modify-write is
dropped (an unparseable state file would otherwise kill the fairness
gate permanently and invisibly), cover cascade_run itself by lifting
it out of tick.sh, and note the one documented exception to cascade
order in CLAUDE.md.

Not addressed: the mktemp/mv cross-filesystem note (pre-existing
weekly_mark_done pattern, out of scope here). The PR body's assertion
count is now stale in the other direction -- the suite is 16 -> 25.

igor added spent time

2026-07-28 16:44:11 +00:00

8 minutes 32 seconds

igor commented

2026-07-28 16:48:33 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 5a9aa0c3: success

Adds a starvation-fairness gate to the tick cascade: each stage stamps the tick at which it was reached, and a stage unreached for 20 cascade-reaching ticks runs once at the top of the next cascade. CI is green, the new lib is well-factored and genuinely tested, and the cascade_run wrapper preserves the old if …; then exit 0; fi gate semantics. I found no blocking defect, but three things I'd want a human to weigh before merge.

Things worth a human's eye

Write amplification on a file the linked issue itself calls hazardous. cascade_mark_reached_file (bin/tick.sh ~4110) does a full jq read-modify-write + mv of discretionary-state.json for every stage reached, so a tick that falls through to the issue grind now rewrites that file 10× (1 bump + 9 stamps) where it previously wrote it ~0×. #441 explicitly flags the "non-atomic jq+mv, 36 call sites" pattern as a hazard it declined to make worse. Two sub-points:
- tmp=$(mktemp) puts the temp file in $TMPDIR, so the mv is likely cross-filesystem (copy+unlink, not an atomic rename) relative to ~/.local/state/agent/. The PR says this mirrors weekly_mark_done; I can't see that function in the diff to confirm it uses the same TMPDIR-based mktemp rather than a same-directory temp. If weekly_mark_done writes its temp beside the target, this is a regression in durability, not parity.
- Cheap mitigation if you want it: stamp only the stages that actually changed, or batch the whole cascade's stamps into one write at the point the cascade is entered.
Does this actually fix the measured 2.5h blackout? The counter only advances on ticks that reach the cascade (cascade_bump_tick_file sits at the top of the block, bin/tick.sh ~4142). #441's measurement attributes the blackout partly to automerge and deploy-watching, which exit above this point and therefore never advance the counter. The design note in the PR argues a rescue here couldn't have helped those ticks — true — but the comment shipped in lib/cascade.sh goes further and claims 20 ticks is "short enough that the 2.5-hour blackout above becomes impossible." That's only true if cascade-reaching ticks are frequent; if automerge/deploy-watch dominate, 20 cascade-reaching ticks can still span hours and ceo still never runs. I'd soften that comment or justify the 20 with the observed cascade-reaching tick rate, not the wall-clock tick rate.
_cascade_state / non-numeric handling is fail-open, contrary to the description. cascade_starved_stage fails closed on unparseable JSON (jq -e . || return 0, lib/cascade.sh) — good — but a parseable state with a garbage value ({"reached":{"ceo":"banana"}}) is normalized to last=0, i.e. "never reached", which makes the stage look maximally starved. The test asserts this as intended behavior, so it's a deliberate choice, but it's the opposite polarity from the "fails CLOSED on corrupt state" framing in the description. Worth a sentence in the comment saying why a corrupt value is treated differently from a corrupt document.

Smaller notes

bin/tick.sh ~4083: [ -f "$f" ] || echo '{}' > "$f" doesn't mkdir -p the parent. If tick.sh runs under set -e, a missing state dir makes this top-level call abort the whole tick rather than degrade. Low likelihood (36 existing call sites already write there), but the two other failure modes in these functions are handled with a log line and this one isn't.
bin/tick.sh ~4148: cascade_state_file_read is invoked 3–4× in the fairness block (bump, tick number, starved, age) — each a fresh cat + jq. Reading once into a local would be cheaper and race-free within the block.
Description says the suite has 18 assertions; I count ~30 eq calls in bin/test-cascade.sh. Under-count, not over-count, so no honesty concern — just stale.
bin/check-sync.sh: the new stage/gate drift check is a good defensive addition and is arguably in scope as a guard for the new dispatch. The diff <(…) | sed failure path inherits the existing script's set -euo pipefail abort-before-FAIL=1 behavior; consistent with the outcome check above it, so not new.
I could not verify from the diff whether AGENTS.md documents the cascade order (only CLAUDE.md was updated, and check-sync.sh only enforces OUTCOME sentinels). Worth a glance.

What I checked and found fine

All 9 CASCADE_STAGES names map to gates in the diff, and the rescue block is placed at the top of the contiguous cascade run — the reordering stays entirely inside the cascade, so it can't lift a model-call stage above the health gate.
The one-shot property holds: cascade_run returns 1 for $CASCADE_RESCUED without re-invoking do_<stage>_tick or re-stamping, which is directly tested (no duplicated model call on the fall-through path).
Tie-break (-gt worst_age, strictly greater) correctly favors the earlier stage in cascade order; the age > thresh boundary is tested at exactly CASCADE_STARVE_TICKS.
Fresh state doesn't declare the whole cascade starved on tick 1, and the never-recorded-⇒-tick-0 default resolves one stage per tick after the threshold rather than pinning.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `5a9aa0c3`: **success** Adds a starvation-fairness gate to the tick cascade: each stage stamps the tick at which it was *reached*, and a stage unreached for 20 cascade-reaching ticks runs once at the top of the next cascade. CI is green, the new lib is well-factored and genuinely tested, and the `cascade_run` wrapper preserves the old `if …; then exit 0; fi` gate semantics. I found no blocking defect, but three things I'd want a human to weigh before merge. ## Things worth a human's eye - **Write amplification on a file the linked issue itself calls hazardous.** `cascade_mark_reached_file` (bin/tick.sh ~4110) does a full jq read-modify-write + `mv` of `discretionary-state.json` for *every stage reached*, so a tick that falls through to the issue grind now rewrites that file 10× (1 bump + 9 stamps) where it previously wrote it ~0×. #441 explicitly flags the "non-atomic jq+mv, 36 call sites" pattern as a hazard it declined to make worse. Two sub-points: - `tmp=$(mktemp)` puts the temp file in `$TMPDIR`, so the `mv` is likely cross-filesystem (copy+unlink, not an atomic rename) relative to `~/.local/state/agent/`. The PR says this mirrors `weekly_mark_done`; I can't see that function in the diff to confirm it uses the same TMPDIR-based `mktemp` rather than a same-directory temp. If `weekly_mark_done` writes its temp beside the target, this is a regression in durability, not parity. - Cheap mitigation if you want it: stamp only the stages that actually changed, or batch the whole cascade's stamps into one write at the point the cascade is entered. - **Does this actually fix the measured 2.5h blackout?** The counter only advances on ticks that *reach* the cascade (`cascade_bump_tick_file` sits at the top of the block, bin/tick.sh ~4142). #441's measurement attributes the blackout partly to **automerge and deploy-watching**, which exit *above* this point and therefore never advance the counter. The design note in the PR argues a rescue here couldn't have helped those ticks — true — but the comment shipped in `lib/cascade.sh` goes further and claims 20 ticks is "short enough that the 2.5-hour blackout above becomes impossible." That's only true if cascade-reaching ticks are frequent; if automerge/deploy-watch dominate, 20 cascade-reaching ticks can still span hours and `ceo` still never runs. I'd soften that comment or justify the 20 with the observed *cascade-reaching* tick rate, not the wall-clock tick rate. - **`_cascade_state` / non-numeric handling is fail-open, contrary to the description.** `cascade_starved_stage` fails *closed* on unparseable JSON (`jq -e . || return 0`, lib/cascade.sh) — good — but a parseable state with a garbage value (`{"reached":{"ceo":"banana"}}`) is normalized to `last=0`, i.e. "never reached", which makes the stage look maximally starved. The test asserts this as intended behavior, so it's a deliberate choice, but it's the opposite polarity from the "fails CLOSED on corrupt state" framing in the description. Worth a sentence in the comment saying why a corrupt *value* is treated differently from a corrupt *document*. ## Smaller notes - bin/tick.sh ~4083: `[ -f "$f" ] || echo '{}' > "$f"` doesn't `mkdir -p` the parent. If `tick.sh` runs under `set -e`, a missing state dir makes this top-level call abort the whole tick rather than degrade. Low likelihood (36 existing call sites already write there), but the two other failure modes in these functions are handled with a `log` line and this one isn't. - bin/tick.sh ~4148: `cascade_state_file_read` is invoked 3–4× in the fairness block (bump, tick number, starved, age) — each a fresh `cat` + `jq`. Reading once into a local would be cheaper and race-free within the block. - Description says the suite has **18 assertions**; I count ~30 `eq` calls in `bin/test-cascade.sh`. Under-count, not over-count, so no honesty concern — just stale. - `bin/check-sync.sh`: the new stage/gate drift check is a good defensive addition and is arguably in scope as a guard for the new dispatch. The `diff <(…) | sed` failure path inherits the existing script's `set -euo pipefail` abort-before-`FAIL=1` behavior; consistent with the outcome check above it, so not new. - I could not verify from the diff whether `AGENTS.md` documents the cascade order (only `CLAUDE.md` was updated, and `check-sync.sh` only enforces OUTCOME sentinels). Worth a glance. ## What I checked and found fine - All 9 `CASCADE_STAGES` names map to gates in the diff, and the rescue block is placed at the top of the contiguous cascade run — the reordering stays entirely *inside* the cascade, so it can't lift a model-call stage above the health gate. - The one-shot property holds: `cascade_run` returns 1 for `$CASCADE_RESCUED` without re-invoking `do_<stage>_tick` or re-stamping, which is directly tested (no duplicated model call on the fall-through path). - Tie-break (`-gt worst_age`, strictly greater) correctly favors the earlier stage in cascade order; the `age > thresh` boundary is tested at exactly `CASCADE_STARVE_TICKS`. - Fresh state doesn't declare the whole cascade starved on tick 1, and the never-recorded-⇒-tick-0 default resolves one stage per tick after the threshold rather than pinning. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-28 16:48:33 +00:00

3 minutes 14 seconds

igor requested review from joshtronic

2026-07-28 16:48:33 +00:00