diff --git a/.claude/skills/arch-init/SKILL.md b/.claude/skills/arch-init/SKILL.md index b69b0b751..fb6a11cc7 100644 --- a/.claude/skills/arch-init/SKILL.md +++ b/.claude/skills/arch-init/SKILL.md @@ -63,7 +63,11 @@ not choose; a state save happens at a boundary **you** pick, with a summary **you** curate. That is strictly better, so use it: ``` -/arch-init (recover) → work → save at a checkpoint → suggest /clear → human /clears → /arch-init → … +/arch-init (recover) → work → save at a checkpoint → refresh → /arch-init (recover) → … + │ + packaged: /arch-save ─────────────────┤ stops monitors, saves, clears, + │ schedules /arch-init + manual: suggest /clear → human clears ┘ then human runs /arch-init ``` **When to save.** Save at a *resumable boundary* — a point a fresh session @@ -99,17 +103,26 @@ plus the most recent dated section*, so a save must leave exactly that behind: dumps or raw tool output. Include only: current focus, open loops, and the instructions a fresh session needs to resume. -**Then — and only then — suggest `/clear`.** Save first, *then* tell the human -it is a good time to clear. You cannot clear your own context and must never -decide unilaterally to lose it; keeping the irreversible step behind a human -keystroke means accepting the suggestion can never lose anything, because the -save already happened. Make the suggestion **advisory, never nagging**, and -only right after a save — e.g.: +**Then — and only then — suggest the refresh.** Save first, *then* tell the +human it is a good time to clear. You must never decide unilaterally to lose +your context; keeping the irreversible step behind a human decision means +accepting the suggestion can never lose anything, because the save already +happened. Make the suggestion **advisory, never nagging**, and only right +after a save — e.g.: -> State saved to `codev/state/.md` — good time to `/clear` if this +> State saved to `codev/state/.md` — good time to refresh if this > session is feeling heavy. -Do not repeat it, and do not prompt to `/clear` at any other time. +Do not repeat it, and do not prompt for it at any other time. + +**`/arch-save` packages this whole loop**, and is the preferred path when the +owner directs a refresh: it stops your monitors, writes the pruned state file, +clears, and schedules `/arch-init` to bring you back — in that order, which is +the part that matters. The save discipline above is what it performs at its +step 3, so this section remains the source of truth for *how to write the +file*; `/arch-save` is the source of truth for *the sequence*. The manual path +(save → human clears → `/arch-init`) stays valid and is the fallback when +Tower is unavailable. ## Guardrails (architect-wide; the state file may add more) diff --git a/.claude/skills/arch-save/SKILL.md b/.claude/skills/arch-save/SKILL.md new file mode 100644 index 000000000..dce69a773 --- /dev/null +++ b/.claude/skills/arch-save/SKILL.md @@ -0,0 +1,183 @@ +--- +name: arch-save +description: Save an architect's state, clear its context, and re-init automatically — the packaged save→clear→re-init refresh cycle. Use when the owner directs a context refresh, or says "/arch-save", "save and clear", "refresh your context". Runs on the owner's direction; an architect does not invoke it autonomously mid-task. Counterpart to /arch-init, which recovers the state this writes. +argument-hint: "[name] (e.g. main; omit to auto-detect via afx whoami)" +--- + +# /arch-save — save state, clear, and come back as yourself + +Long sessions accumulate stale context. This is the deliberate cure: you choose the +moment, you choose what survives, and a fresh session resumes from what you wrote. + +`$ARGUMENTS` is the architect name (e.g. `main`). Omit it to auto-detect. + +## When NOT to run this + +**On the owner's direction, or when the owner runs it themselves.** Do not invoke this +autonomously mid-task on your own judgement — the irreversible step is a human decision, +relocated from "press `/clear`" to "invoke `/arch-save`", not removed. If the owner tells +you to run it, run it; if you think it is time, *suggest* it and wait. + +**Only at a resumable boundary** — a gate approval, a PR merge, a completed investigation, +the end of a long tool-heavy stretch. **Never mid-task.** Nothing here can check that; the +state file must describe a point a fresh session can resume *from*, not a half-finished +action. A mid-task snapshot resumes into confusion. + +## The procedure + +Do these in order. **The order is the feature** — step 3 must precede step 4, because the +context that knows what to write is the one about to be destroyed. + +### 1. Resolve your name + +If `$ARGUMENTS` is non-empty, that is your name. Otherwise run `afx whoami` and use the +reported `name` when `type: architect`. + +- `type: builder` → **STOP.** This terminal is a builder. Report the mismatch. +- Non-zero exit → **STOP** and ask which architect you are. Do **not** guess, and do not + default to `main` — writing another architect's state file is the exact failure + `/arch-init` exists to prevent (#1094). + +**Validate the name before building any path**: `[a-z][a-z0-9-]*`, at most 64 characters. +Reject slashes, `..`, uppercase, spaces. Never interpolate an unvalidated name into +`codev/state/.md`. + +### 2. Stop your own monitors + +Enumerate every monitor, watcher or background task you armed, and stop it. + +This is the half that only *you* can do. Monitors are **session-bound, not +context-bound**: they survive `/clear` and keep firing into a context that cannot evaluate +their alerts. `pgrep` cannot see them — they are harness background tasks, not shell +processes — so the instance after the clear has no handle on them. You do. Use it. + +### 3. Write the pruned state file + +Rewrite `codev/state/.md`. **Pruning is part of the save, not polish afterwards — a +save that only appends has not done its job.** + +- **Rewrite the current-state / open-loops section in place.** Do not accumulate stale + "current state" blocks. Never leave two sections with the same heading — a duplicated + "How to resume" means you appended where you should have overwritten. +- **Delete resolved loops outright.** A closed item's record is the log entry, not a + lingering line in current state. +- **Append one short dated entry** for what changed this stretch. +- **Collapse older entries into one-line summaries that point at durable artifacts** — the + merged PRs, closed issues and reviews where the detail actually lives. +- **Aim for one screen.** If the file has grown past easy reading, prune as part of *this* + save rather than leaving it for next time. + +**Prune by pointer, never by deletion.** These files are gitignored (`.gitignore:15`), so +there is no history to recover from — pruned prose is gone for good. Replace detail with a +pointer to something durable; never delete the only record of something. Copying the file +first (`cp codev/state/.md codev/state/..bak.md`) is cheap insurance. + +**Content guardrails.** No secrets — tokens, keys, credentials. No transcript dumps, no +raw tool output. Only: current focus, open loops, and what a fresh session needs to resume. + +Use the template at the end of this document. + +### 4. Clear + +```bash +afx send architect: --raw '/clear' +``` + +**`architect:`, never bare `architect`.** For a non-builder sender the bare form +resolves to `main`, or to the first registered architect — so a *sibling* architect +running this would clear **main's** terminal instead of its own. That destroys the context +of someone who never asked for anything, and it is one word away from correct. + +**`--raw`, never the escape channel.** The escape route writes a bare ESC and discards the +message body, so `/clear` sent that way delivers an interrupt: the command appears to +succeed and nothing is cleared. + +### 5. Schedule the re-init + +```bash +afx send architect: --delay 15 --raw '/arch-init ' +``` + +Tower holds this for 15 seconds and then delivers it. It has to come from outside the +session, because the clear destroys the context that would otherwise send it. + +**Tower does not know whether the clear landed** — it waits out a delay, it does not +observe the result. 15 seconds is a value chosen because it works in practice, not a +guarantee about the clear's completion. If the timing is wrong the re-init arrives at the +wrong moment, which costs one manual message (see below) and nothing else. That is the +whole reason this cycle can be built on a delay rather than on machinery. + +Delayed sends are **not persisted** — if Tower restarts inside the window, the message is +dropped. That is recoverable; see below. + +**If this send fails, do not end your turn.** Step 4 queued the `/clear`, but it does not +take effect until your turn ends — so at this moment you still have your full context and +the failure is recoverable. Retry the send; if it keeps failing (Tower down, for example), +tell the owner that the clear is queued with no re-init scheduled, and give them the +command to send by hand once Tower is back. Ending the turn here is the one way to turn a +recoverable failure into a cleared session nobody is coming back for. + +### 6. Stop + +Do not start new work. End your turn so the clear can take effect. + +## After the clear + +`/arch-init` will read your state file and resume. Two things it should do first, in this +order: + +1. **Reconcile monitors.** You stopped yours in step 2, but treat any alert you cannot + account for from the state block's monitor list as **stale** — disregard it, and stop + it if you can. An alert from a decommissioned target is indistinguishable from a live + one. +2. **Then re-arm** the monitors the block lists, and let each one **self-test once** before + trusting its alerts. A freshly-armed monitor's first alert has been a false positive in + practice. + +## If `/arch-init` never arrives + +Nothing is lost. The state file is on disk, the terminal is alive. Send it by hand: + +```bash +afx send architect: --raw '/arch-init ' +``` + +This is the recovery the whole design leans on, which is why the cycle can accept imprecise +timing rather than needing machinery to guarantee it. + +**If the clear did not take effect** — you still have your full context and a stray +`/arch-init` arrived — nothing was destroyed. Check whether `/clear` was submitted as its +own message rather than merged into another one, and report it; a `/clear` that arrives as +literal text on the front of the next message never executes. + +## State block template + +The structure below comes from a live run of this cycle. Every element earns its place; +keep them all, including a `MONITORS:` line even when the answer is "none armed" — an +omitted monitor list is indistinguishable from a forgotten one. + +``` +# architect — state (vNN, ~HH:MM UTC — , DELIBERATE /clear cycle) +# ⭐ THIS /clear IS INTENTIONAL (owner-directed context refresh). On re-init: normal +# /arch-init flow, then: +# 1. MONITORS: — watch target, +# cadence, alert pattern. Self-test once before trusting alerts. ("none armed" is a +# valid and complete answer.) +# 2. DONE pre-clear, with receipts: MERGED (, verified on origin/); +# PUSH-VERIFIED ( local==origin). Distinguish "written" from "verified" — +# a cold reader cannot tell. +# 3. ACTIVE LANES: = (; ). +# Name the file, so no instruction lives only in the context being destroyed. +# 4. LATEST RESULTS: , so the first post-resume decision +# needs no archaeology. +# 5. QUEUED, with ordering: — WAITS for ; — . +# 6. ENVELOPE: ; . +``` + +## Guardrails (architect-wide) + +- **Never auto-approve porch gates.** A gate notification is for the human, not you. +- **Touch only your own builders / spawns / filings.** +- **Never `cd` into a builder worktree**; use `git -C` and absolute paths. +- **Stay on the default branch at the workspace root.** diff --git a/.codex/skills/arch-init/SKILL.md b/.codex/skills/arch-init/SKILL.md index b69b0b751..fb6a11cc7 100644 --- a/.codex/skills/arch-init/SKILL.md +++ b/.codex/skills/arch-init/SKILL.md @@ -63,7 +63,11 @@ not choose; a state save happens at a boundary **you** pick, with a summary **you** curate. That is strictly better, so use it: ``` -/arch-init (recover) → work → save at a checkpoint → suggest /clear → human /clears → /arch-init → … +/arch-init (recover) → work → save at a checkpoint → refresh → /arch-init (recover) → … + │ + packaged: /arch-save ─────────────────┤ stops monitors, saves, clears, + │ schedules /arch-init + manual: suggest /clear → human clears ┘ then human runs /arch-init ``` **When to save.** Save at a *resumable boundary* — a point a fresh session @@ -99,17 +103,26 @@ plus the most recent dated section*, so a save must leave exactly that behind: dumps or raw tool output. Include only: current focus, open loops, and the instructions a fresh session needs to resume. -**Then — and only then — suggest `/clear`.** Save first, *then* tell the human -it is a good time to clear. You cannot clear your own context and must never -decide unilaterally to lose it; keeping the irreversible step behind a human -keystroke means accepting the suggestion can never lose anything, because the -save already happened. Make the suggestion **advisory, never nagging**, and -only right after a save — e.g.: +**Then — and only then — suggest the refresh.** Save first, *then* tell the +human it is a good time to clear. You must never decide unilaterally to lose +your context; keeping the irreversible step behind a human decision means +accepting the suggestion can never lose anything, because the save already +happened. Make the suggestion **advisory, never nagging**, and only right +after a save — e.g.: -> State saved to `codev/state/.md` — good time to `/clear` if this +> State saved to `codev/state/.md` — good time to refresh if this > session is feeling heavy. -Do not repeat it, and do not prompt to `/clear` at any other time. +Do not repeat it, and do not prompt for it at any other time. + +**`/arch-save` packages this whole loop**, and is the preferred path when the +owner directs a refresh: it stops your monitors, writes the pruned state file, +clears, and schedules `/arch-init` to bring you back — in that order, which is +the part that matters. The save discipline above is what it performs at its +step 3, so this section remains the source of truth for *how to write the +file*; `/arch-save` is the source of truth for *the sequence*. The manual path +(save → human clears → `/arch-init`) stays valid and is the fallback when +Tower is unavailable. ## Guardrails (architect-wide; the state file may add more) diff --git a/.codex/skills/arch-save/SKILL.md b/.codex/skills/arch-save/SKILL.md new file mode 100644 index 000000000..dce69a773 --- /dev/null +++ b/.codex/skills/arch-save/SKILL.md @@ -0,0 +1,183 @@ +--- +name: arch-save +description: Save an architect's state, clear its context, and re-init automatically — the packaged save→clear→re-init refresh cycle. Use when the owner directs a context refresh, or says "/arch-save", "save and clear", "refresh your context". Runs on the owner's direction; an architect does not invoke it autonomously mid-task. Counterpart to /arch-init, which recovers the state this writes. +argument-hint: "[name] (e.g. main; omit to auto-detect via afx whoami)" +--- + +# /arch-save — save state, clear, and come back as yourself + +Long sessions accumulate stale context. This is the deliberate cure: you choose the +moment, you choose what survives, and a fresh session resumes from what you wrote. + +`$ARGUMENTS` is the architect name (e.g. `main`). Omit it to auto-detect. + +## When NOT to run this + +**On the owner's direction, or when the owner runs it themselves.** Do not invoke this +autonomously mid-task on your own judgement — the irreversible step is a human decision, +relocated from "press `/clear`" to "invoke `/arch-save`", not removed. If the owner tells +you to run it, run it; if you think it is time, *suggest* it and wait. + +**Only at a resumable boundary** — a gate approval, a PR merge, a completed investigation, +the end of a long tool-heavy stretch. **Never mid-task.** Nothing here can check that; the +state file must describe a point a fresh session can resume *from*, not a half-finished +action. A mid-task snapshot resumes into confusion. + +## The procedure + +Do these in order. **The order is the feature** — step 3 must precede step 4, because the +context that knows what to write is the one about to be destroyed. + +### 1. Resolve your name + +If `$ARGUMENTS` is non-empty, that is your name. Otherwise run `afx whoami` and use the +reported `name` when `type: architect`. + +- `type: builder` → **STOP.** This terminal is a builder. Report the mismatch. +- Non-zero exit → **STOP** and ask which architect you are. Do **not** guess, and do not + default to `main` — writing another architect's state file is the exact failure + `/arch-init` exists to prevent (#1094). + +**Validate the name before building any path**: `[a-z][a-z0-9-]*`, at most 64 characters. +Reject slashes, `..`, uppercase, spaces. Never interpolate an unvalidated name into +`codev/state/.md`. + +### 2. Stop your own monitors + +Enumerate every monitor, watcher or background task you armed, and stop it. + +This is the half that only *you* can do. Monitors are **session-bound, not +context-bound**: they survive `/clear` and keep firing into a context that cannot evaluate +their alerts. `pgrep` cannot see them — they are harness background tasks, not shell +processes — so the instance after the clear has no handle on them. You do. Use it. + +### 3. Write the pruned state file + +Rewrite `codev/state/.md`. **Pruning is part of the save, not polish afterwards — a +save that only appends has not done its job.** + +- **Rewrite the current-state / open-loops section in place.** Do not accumulate stale + "current state" blocks. Never leave two sections with the same heading — a duplicated + "How to resume" means you appended where you should have overwritten. +- **Delete resolved loops outright.** A closed item's record is the log entry, not a + lingering line in current state. +- **Append one short dated entry** for what changed this stretch. +- **Collapse older entries into one-line summaries that point at durable artifacts** — the + merged PRs, closed issues and reviews where the detail actually lives. +- **Aim for one screen.** If the file has grown past easy reading, prune as part of *this* + save rather than leaving it for next time. + +**Prune by pointer, never by deletion.** These files are gitignored (`.gitignore:15`), so +there is no history to recover from — pruned prose is gone for good. Replace detail with a +pointer to something durable; never delete the only record of something. Copying the file +first (`cp codev/state/.md codev/state/..bak.md`) is cheap insurance. + +**Content guardrails.** No secrets — tokens, keys, credentials. No transcript dumps, no +raw tool output. Only: current focus, open loops, and what a fresh session needs to resume. + +Use the template at the end of this document. + +### 4. Clear + +```bash +afx send architect: --raw '/clear' +``` + +**`architect:`, never bare `architect`.** For a non-builder sender the bare form +resolves to `main`, or to the first registered architect — so a *sibling* architect +running this would clear **main's** terminal instead of its own. That destroys the context +of someone who never asked for anything, and it is one word away from correct. + +**`--raw`, never the escape channel.** The escape route writes a bare ESC and discards the +message body, so `/clear` sent that way delivers an interrupt: the command appears to +succeed and nothing is cleared. + +### 5. Schedule the re-init + +```bash +afx send architect: --delay 15 --raw '/arch-init ' +``` + +Tower holds this for 15 seconds and then delivers it. It has to come from outside the +session, because the clear destroys the context that would otherwise send it. + +**Tower does not know whether the clear landed** — it waits out a delay, it does not +observe the result. 15 seconds is a value chosen because it works in practice, not a +guarantee about the clear's completion. If the timing is wrong the re-init arrives at the +wrong moment, which costs one manual message (see below) and nothing else. That is the +whole reason this cycle can be built on a delay rather than on machinery. + +Delayed sends are **not persisted** — if Tower restarts inside the window, the message is +dropped. That is recoverable; see below. + +**If this send fails, do not end your turn.** Step 4 queued the `/clear`, but it does not +take effect until your turn ends — so at this moment you still have your full context and +the failure is recoverable. Retry the send; if it keeps failing (Tower down, for example), +tell the owner that the clear is queued with no re-init scheduled, and give them the +command to send by hand once Tower is back. Ending the turn here is the one way to turn a +recoverable failure into a cleared session nobody is coming back for. + +### 6. Stop + +Do not start new work. End your turn so the clear can take effect. + +## After the clear + +`/arch-init` will read your state file and resume. Two things it should do first, in this +order: + +1. **Reconcile monitors.** You stopped yours in step 2, but treat any alert you cannot + account for from the state block's monitor list as **stale** — disregard it, and stop + it if you can. An alert from a decommissioned target is indistinguishable from a live + one. +2. **Then re-arm** the monitors the block lists, and let each one **self-test once** before + trusting its alerts. A freshly-armed monitor's first alert has been a false positive in + practice. + +## If `/arch-init` never arrives + +Nothing is lost. The state file is on disk, the terminal is alive. Send it by hand: + +```bash +afx send architect: --raw '/arch-init ' +``` + +This is the recovery the whole design leans on, which is why the cycle can accept imprecise +timing rather than needing machinery to guarantee it. + +**If the clear did not take effect** — you still have your full context and a stray +`/arch-init` arrived — nothing was destroyed. Check whether `/clear` was submitted as its +own message rather than merged into another one, and report it; a `/clear` that arrives as +literal text on the front of the next message never executes. + +## State block template + +The structure below comes from a live run of this cycle. Every element earns its place; +keep them all, including a `MONITORS:` line even when the answer is "none armed" — an +omitted monitor list is indistinguishable from a forgotten one. + +``` +# architect — state (vNN, ~HH:MM UTC — , DELIBERATE /clear cycle) +# ⭐ THIS /clear IS INTENTIONAL (owner-directed context refresh). On re-init: normal +# /arch-init flow, then: +# 1. MONITORS: — watch target, +# cadence, alert pattern. Self-test once before trusting alerts. ("none armed" is a +# valid and complete answer.) +# 2. DONE pre-clear, with receipts: MERGED (, verified on origin/); +# PUSH-VERIFIED ( local==origin). Distinguish "written" from "verified" — +# a cold reader cannot tell. +# 3. ACTIVE LANES: = (; ). +# Name the file, so no instruction lives only in the context being destroyed. +# 4. LATEST RESULTS: , so the first post-resume decision +# needs no archaeology. +# 5. QUEUED, with ordering: — WAITS for ; — . +# 6. ENVELOPE: ; . +``` + +## Guardrails (architect-wide) + +- **Never auto-approve porch gates.** A gate notification is for the human, not you. +- **Touch only your own builders / spawns / filings.** +- **Never `cd` into a builder worktree**; use `git -C` and absolute paths. +- **Stay on the default branch at the workspace root.** diff --git a/codev-skeleton/.claude/skills/arch-init/SKILL.md b/codev-skeleton/.claude/skills/arch-init/SKILL.md index b69b0b751..fb6a11cc7 100644 --- a/codev-skeleton/.claude/skills/arch-init/SKILL.md +++ b/codev-skeleton/.claude/skills/arch-init/SKILL.md @@ -63,7 +63,11 @@ not choose; a state save happens at a boundary **you** pick, with a summary **you** curate. That is strictly better, so use it: ``` -/arch-init (recover) → work → save at a checkpoint → suggest /clear → human /clears → /arch-init → … +/arch-init (recover) → work → save at a checkpoint → refresh → /arch-init (recover) → … + │ + packaged: /arch-save ─────────────────┤ stops monitors, saves, clears, + │ schedules /arch-init + manual: suggest /clear → human clears ┘ then human runs /arch-init ``` **When to save.** Save at a *resumable boundary* — a point a fresh session @@ -99,17 +103,26 @@ plus the most recent dated section*, so a save must leave exactly that behind: dumps or raw tool output. Include only: current focus, open loops, and the instructions a fresh session needs to resume. -**Then — and only then — suggest `/clear`.** Save first, *then* tell the human -it is a good time to clear. You cannot clear your own context and must never -decide unilaterally to lose it; keeping the irreversible step behind a human -keystroke means accepting the suggestion can never lose anything, because the -save already happened. Make the suggestion **advisory, never nagging**, and -only right after a save — e.g.: +**Then — and only then — suggest the refresh.** Save first, *then* tell the +human it is a good time to clear. You must never decide unilaterally to lose +your context; keeping the irreversible step behind a human decision means +accepting the suggestion can never lose anything, because the save already +happened. Make the suggestion **advisory, never nagging**, and only right +after a save — e.g.: -> State saved to `codev/state/.md` — good time to `/clear` if this +> State saved to `codev/state/.md` — good time to refresh if this > session is feeling heavy. -Do not repeat it, and do not prompt to `/clear` at any other time. +Do not repeat it, and do not prompt for it at any other time. + +**`/arch-save` packages this whole loop**, and is the preferred path when the +owner directs a refresh: it stops your monitors, writes the pruned state file, +clears, and schedules `/arch-init` to bring you back — in that order, which is +the part that matters. The save discipline above is what it performs at its +step 3, so this section remains the source of truth for *how to write the +file*; `/arch-save` is the source of truth for *the sequence*. The manual path +(save → human clears → `/arch-init`) stays valid and is the fallback when +Tower is unavailable. ## Guardrails (architect-wide; the state file may add more) diff --git a/codev-skeleton/.claude/skills/arch-save/SKILL.md b/codev-skeleton/.claude/skills/arch-save/SKILL.md new file mode 100644 index 000000000..dce69a773 --- /dev/null +++ b/codev-skeleton/.claude/skills/arch-save/SKILL.md @@ -0,0 +1,183 @@ +--- +name: arch-save +description: Save an architect's state, clear its context, and re-init automatically — the packaged save→clear→re-init refresh cycle. Use when the owner directs a context refresh, or says "/arch-save", "save and clear", "refresh your context". Runs on the owner's direction; an architect does not invoke it autonomously mid-task. Counterpart to /arch-init, which recovers the state this writes. +argument-hint: "[name] (e.g. main; omit to auto-detect via afx whoami)" +--- + +# /arch-save — save state, clear, and come back as yourself + +Long sessions accumulate stale context. This is the deliberate cure: you choose the +moment, you choose what survives, and a fresh session resumes from what you wrote. + +`$ARGUMENTS` is the architect name (e.g. `main`). Omit it to auto-detect. + +## When NOT to run this + +**On the owner's direction, or when the owner runs it themselves.** Do not invoke this +autonomously mid-task on your own judgement — the irreversible step is a human decision, +relocated from "press `/clear`" to "invoke `/arch-save`", not removed. If the owner tells +you to run it, run it; if you think it is time, *suggest* it and wait. + +**Only at a resumable boundary** — a gate approval, a PR merge, a completed investigation, +the end of a long tool-heavy stretch. **Never mid-task.** Nothing here can check that; the +state file must describe a point a fresh session can resume *from*, not a half-finished +action. A mid-task snapshot resumes into confusion. + +## The procedure + +Do these in order. **The order is the feature** — step 3 must precede step 4, because the +context that knows what to write is the one about to be destroyed. + +### 1. Resolve your name + +If `$ARGUMENTS` is non-empty, that is your name. Otherwise run `afx whoami` and use the +reported `name` when `type: architect`. + +- `type: builder` → **STOP.** This terminal is a builder. Report the mismatch. +- Non-zero exit → **STOP** and ask which architect you are. Do **not** guess, and do not + default to `main` — writing another architect's state file is the exact failure + `/arch-init` exists to prevent (#1094). + +**Validate the name before building any path**: `[a-z][a-z0-9-]*`, at most 64 characters. +Reject slashes, `..`, uppercase, spaces. Never interpolate an unvalidated name into +`codev/state/.md`. + +### 2. Stop your own monitors + +Enumerate every monitor, watcher or background task you armed, and stop it. + +This is the half that only *you* can do. Monitors are **session-bound, not +context-bound**: they survive `/clear` and keep firing into a context that cannot evaluate +their alerts. `pgrep` cannot see them — they are harness background tasks, not shell +processes — so the instance after the clear has no handle on them. You do. Use it. + +### 3. Write the pruned state file + +Rewrite `codev/state/.md`. **Pruning is part of the save, not polish afterwards — a +save that only appends has not done its job.** + +- **Rewrite the current-state / open-loops section in place.** Do not accumulate stale + "current state" blocks. Never leave two sections with the same heading — a duplicated + "How to resume" means you appended where you should have overwritten. +- **Delete resolved loops outright.** A closed item's record is the log entry, not a + lingering line in current state. +- **Append one short dated entry** for what changed this stretch. +- **Collapse older entries into one-line summaries that point at durable artifacts** — the + merged PRs, closed issues and reviews where the detail actually lives. +- **Aim for one screen.** If the file has grown past easy reading, prune as part of *this* + save rather than leaving it for next time. + +**Prune by pointer, never by deletion.** These files are gitignored (`.gitignore:15`), so +there is no history to recover from — pruned prose is gone for good. Replace detail with a +pointer to something durable; never delete the only record of something. Copying the file +first (`cp codev/state/.md codev/state/..bak.md`) is cheap insurance. + +**Content guardrails.** No secrets — tokens, keys, credentials. No transcript dumps, no +raw tool output. Only: current focus, open loops, and what a fresh session needs to resume. + +Use the template at the end of this document. + +### 4. Clear + +```bash +afx send architect: --raw '/clear' +``` + +**`architect:`, never bare `architect`.** For a non-builder sender the bare form +resolves to `main`, or to the first registered architect — so a *sibling* architect +running this would clear **main's** terminal instead of its own. That destroys the context +of someone who never asked for anything, and it is one word away from correct. + +**`--raw`, never the escape channel.** The escape route writes a bare ESC and discards the +message body, so `/clear` sent that way delivers an interrupt: the command appears to +succeed and nothing is cleared. + +### 5. Schedule the re-init + +```bash +afx send architect: --delay 15 --raw '/arch-init ' +``` + +Tower holds this for 15 seconds and then delivers it. It has to come from outside the +session, because the clear destroys the context that would otherwise send it. + +**Tower does not know whether the clear landed** — it waits out a delay, it does not +observe the result. 15 seconds is a value chosen because it works in practice, not a +guarantee about the clear's completion. If the timing is wrong the re-init arrives at the +wrong moment, which costs one manual message (see below) and nothing else. That is the +whole reason this cycle can be built on a delay rather than on machinery. + +Delayed sends are **not persisted** — if Tower restarts inside the window, the message is +dropped. That is recoverable; see below. + +**If this send fails, do not end your turn.** Step 4 queued the `/clear`, but it does not +take effect until your turn ends — so at this moment you still have your full context and +the failure is recoverable. Retry the send; if it keeps failing (Tower down, for example), +tell the owner that the clear is queued with no re-init scheduled, and give them the +command to send by hand once Tower is back. Ending the turn here is the one way to turn a +recoverable failure into a cleared session nobody is coming back for. + +### 6. Stop + +Do not start new work. End your turn so the clear can take effect. + +## After the clear + +`/arch-init` will read your state file and resume. Two things it should do first, in this +order: + +1. **Reconcile monitors.** You stopped yours in step 2, but treat any alert you cannot + account for from the state block's monitor list as **stale** — disregard it, and stop + it if you can. An alert from a decommissioned target is indistinguishable from a live + one. +2. **Then re-arm** the monitors the block lists, and let each one **self-test once** before + trusting its alerts. A freshly-armed monitor's first alert has been a false positive in + practice. + +## If `/arch-init` never arrives + +Nothing is lost. The state file is on disk, the terminal is alive. Send it by hand: + +```bash +afx send architect: --raw '/arch-init ' +``` + +This is the recovery the whole design leans on, which is why the cycle can accept imprecise +timing rather than needing machinery to guarantee it. + +**If the clear did not take effect** — you still have your full context and a stray +`/arch-init` arrived — nothing was destroyed. Check whether `/clear` was submitted as its +own message rather than merged into another one, and report it; a `/clear` that arrives as +literal text on the front of the next message never executes. + +## State block template + +The structure below comes from a live run of this cycle. Every element earns its place; +keep them all, including a `MONITORS:` line even when the answer is "none armed" — an +omitted monitor list is indistinguishable from a forgotten one. + +``` +# architect — state (vNN, ~HH:MM UTC — , DELIBERATE /clear cycle) +# ⭐ THIS /clear IS INTENTIONAL (owner-directed context refresh). On re-init: normal +# /arch-init flow, then: +# 1. MONITORS: — watch target, +# cadence, alert pattern. Self-test once before trusting alerts. ("none armed" is a +# valid and complete answer.) +# 2. DONE pre-clear, with receipts: MERGED (, verified on origin/); +# PUSH-VERIFIED ( local==origin). Distinguish "written" from "verified" — +# a cold reader cannot tell. +# 3. ACTIVE LANES: = (; ). +# Name the file, so no instruction lives only in the context being destroyed. +# 4. LATEST RESULTS: , so the first post-resume decision +# needs no archaeology. +# 5. QUEUED, with ordering: — WAITS for ; — . +# 6. ENVELOPE: ; . +``` + +## Guardrails (architect-wide) + +- **Never auto-approve porch gates.** A gate notification is for the human, not you. +- **Touch only your own builders / spawns / filings.** +- **Never `cd` into a builder worktree**; use `git -C` and absolute paths. +- **Stay on the default branch at the workspace root.** diff --git a/codev-skeleton/.codex/skills/arch-init/SKILL.md b/codev-skeleton/.codex/skills/arch-init/SKILL.md index b69b0b751..fb6a11cc7 100644 --- a/codev-skeleton/.codex/skills/arch-init/SKILL.md +++ b/codev-skeleton/.codex/skills/arch-init/SKILL.md @@ -63,7 +63,11 @@ not choose; a state save happens at a boundary **you** pick, with a summary **you** curate. That is strictly better, so use it: ``` -/arch-init (recover) → work → save at a checkpoint → suggest /clear → human /clears → /arch-init → … +/arch-init (recover) → work → save at a checkpoint → refresh → /arch-init (recover) → … + │ + packaged: /arch-save ─────────────────┤ stops monitors, saves, clears, + │ schedules /arch-init + manual: suggest /clear → human clears ┘ then human runs /arch-init ``` **When to save.** Save at a *resumable boundary* — a point a fresh session @@ -99,17 +103,26 @@ plus the most recent dated section*, so a save must leave exactly that behind: dumps or raw tool output. Include only: current focus, open loops, and the instructions a fresh session needs to resume. -**Then — and only then — suggest `/clear`.** Save first, *then* tell the human -it is a good time to clear. You cannot clear your own context and must never -decide unilaterally to lose it; keeping the irreversible step behind a human -keystroke means accepting the suggestion can never lose anything, because the -save already happened. Make the suggestion **advisory, never nagging**, and -only right after a save — e.g.: +**Then — and only then — suggest the refresh.** Save first, *then* tell the +human it is a good time to clear. You must never decide unilaterally to lose +your context; keeping the irreversible step behind a human decision means +accepting the suggestion can never lose anything, because the save already +happened. Make the suggestion **advisory, never nagging**, and only right +after a save — e.g.: -> State saved to `codev/state/.md` — good time to `/clear` if this +> State saved to `codev/state/.md` — good time to refresh if this > session is feeling heavy. -Do not repeat it, and do not prompt to `/clear` at any other time. +Do not repeat it, and do not prompt for it at any other time. + +**`/arch-save` packages this whole loop**, and is the preferred path when the +owner directs a refresh: it stops your monitors, writes the pruned state file, +clears, and schedules `/arch-init` to bring you back — in that order, which is +the part that matters. The save discipline above is what it performs at its +step 3, so this section remains the source of truth for *how to write the +file*; `/arch-save` is the source of truth for *the sequence*. The manual path +(save → human clears → `/arch-init`) stays valid and is the fallback when +Tower is unavailable. ## Guardrails (architect-wide; the state file may add more) diff --git a/codev-skeleton/.codex/skills/arch-save/SKILL.md b/codev-skeleton/.codex/skills/arch-save/SKILL.md new file mode 100644 index 000000000..dce69a773 --- /dev/null +++ b/codev-skeleton/.codex/skills/arch-save/SKILL.md @@ -0,0 +1,183 @@ +--- +name: arch-save +description: Save an architect's state, clear its context, and re-init automatically — the packaged save→clear→re-init refresh cycle. Use when the owner directs a context refresh, or says "/arch-save", "save and clear", "refresh your context". Runs on the owner's direction; an architect does not invoke it autonomously mid-task. Counterpart to /arch-init, which recovers the state this writes. +argument-hint: "[name] (e.g. main; omit to auto-detect via afx whoami)" +--- + +# /arch-save — save state, clear, and come back as yourself + +Long sessions accumulate stale context. This is the deliberate cure: you choose the +moment, you choose what survives, and a fresh session resumes from what you wrote. + +`$ARGUMENTS` is the architect name (e.g. `main`). Omit it to auto-detect. + +## When NOT to run this + +**On the owner's direction, or when the owner runs it themselves.** Do not invoke this +autonomously mid-task on your own judgement — the irreversible step is a human decision, +relocated from "press `/clear`" to "invoke `/arch-save`", not removed. If the owner tells +you to run it, run it; if you think it is time, *suggest* it and wait. + +**Only at a resumable boundary** — a gate approval, a PR merge, a completed investigation, +the end of a long tool-heavy stretch. **Never mid-task.** Nothing here can check that; the +state file must describe a point a fresh session can resume *from*, not a half-finished +action. A mid-task snapshot resumes into confusion. + +## The procedure + +Do these in order. **The order is the feature** — step 3 must precede step 4, because the +context that knows what to write is the one about to be destroyed. + +### 1. Resolve your name + +If `$ARGUMENTS` is non-empty, that is your name. Otherwise run `afx whoami` and use the +reported `name` when `type: architect`. + +- `type: builder` → **STOP.** This terminal is a builder. Report the mismatch. +- Non-zero exit → **STOP** and ask which architect you are. Do **not** guess, and do not + default to `main` — writing another architect's state file is the exact failure + `/arch-init` exists to prevent (#1094). + +**Validate the name before building any path**: `[a-z][a-z0-9-]*`, at most 64 characters. +Reject slashes, `..`, uppercase, spaces. Never interpolate an unvalidated name into +`codev/state/.md`. + +### 2. Stop your own monitors + +Enumerate every monitor, watcher or background task you armed, and stop it. + +This is the half that only *you* can do. Monitors are **session-bound, not +context-bound**: they survive `/clear` and keep firing into a context that cannot evaluate +their alerts. `pgrep` cannot see them — they are harness background tasks, not shell +processes — so the instance after the clear has no handle on them. You do. Use it. + +### 3. Write the pruned state file + +Rewrite `codev/state/.md`. **Pruning is part of the save, not polish afterwards — a +save that only appends has not done its job.** + +- **Rewrite the current-state / open-loops section in place.** Do not accumulate stale + "current state" blocks. Never leave two sections with the same heading — a duplicated + "How to resume" means you appended where you should have overwritten. +- **Delete resolved loops outright.** A closed item's record is the log entry, not a + lingering line in current state. +- **Append one short dated entry** for what changed this stretch. +- **Collapse older entries into one-line summaries that point at durable artifacts** — the + merged PRs, closed issues and reviews where the detail actually lives. +- **Aim for one screen.** If the file has grown past easy reading, prune as part of *this* + save rather than leaving it for next time. + +**Prune by pointer, never by deletion.** These files are gitignored (`.gitignore:15`), so +there is no history to recover from — pruned prose is gone for good. Replace detail with a +pointer to something durable; never delete the only record of something. Copying the file +first (`cp codev/state/.md codev/state/..bak.md`) is cheap insurance. + +**Content guardrails.** No secrets — tokens, keys, credentials. No transcript dumps, no +raw tool output. Only: current focus, open loops, and what a fresh session needs to resume. + +Use the template at the end of this document. + +### 4. Clear + +```bash +afx send architect: --raw '/clear' +``` + +**`architect:`, never bare `architect`.** For a non-builder sender the bare form +resolves to `main`, or to the first registered architect — so a *sibling* architect +running this would clear **main's** terminal instead of its own. That destroys the context +of someone who never asked for anything, and it is one word away from correct. + +**`--raw`, never the escape channel.** The escape route writes a bare ESC and discards the +message body, so `/clear` sent that way delivers an interrupt: the command appears to +succeed and nothing is cleared. + +### 5. Schedule the re-init + +```bash +afx send architect: --delay 15 --raw '/arch-init ' +``` + +Tower holds this for 15 seconds and then delivers it. It has to come from outside the +session, because the clear destroys the context that would otherwise send it. + +**Tower does not know whether the clear landed** — it waits out a delay, it does not +observe the result. 15 seconds is a value chosen because it works in practice, not a +guarantee about the clear's completion. If the timing is wrong the re-init arrives at the +wrong moment, which costs one manual message (see below) and nothing else. That is the +whole reason this cycle can be built on a delay rather than on machinery. + +Delayed sends are **not persisted** — if Tower restarts inside the window, the message is +dropped. That is recoverable; see below. + +**If this send fails, do not end your turn.** Step 4 queued the `/clear`, but it does not +take effect until your turn ends — so at this moment you still have your full context and +the failure is recoverable. Retry the send; if it keeps failing (Tower down, for example), +tell the owner that the clear is queued with no re-init scheduled, and give them the +command to send by hand once Tower is back. Ending the turn here is the one way to turn a +recoverable failure into a cleared session nobody is coming back for. + +### 6. Stop + +Do not start new work. End your turn so the clear can take effect. + +## After the clear + +`/arch-init` will read your state file and resume. Two things it should do first, in this +order: + +1. **Reconcile monitors.** You stopped yours in step 2, but treat any alert you cannot + account for from the state block's monitor list as **stale** — disregard it, and stop + it if you can. An alert from a decommissioned target is indistinguishable from a live + one. +2. **Then re-arm** the monitors the block lists, and let each one **self-test once** before + trusting its alerts. A freshly-armed monitor's first alert has been a false positive in + practice. + +## If `/arch-init` never arrives + +Nothing is lost. The state file is on disk, the terminal is alive. Send it by hand: + +```bash +afx send architect: --raw '/arch-init ' +``` + +This is the recovery the whole design leans on, which is why the cycle can accept imprecise +timing rather than needing machinery to guarantee it. + +**If the clear did not take effect** — you still have your full context and a stray +`/arch-init` arrived — nothing was destroyed. Check whether `/clear` was submitted as its +own message rather than merged into another one, and report it; a `/clear` that arrives as +literal text on the front of the next message never executes. + +## State block template + +The structure below comes from a live run of this cycle. Every element earns its place; +keep them all, including a `MONITORS:` line even when the answer is "none armed" — an +omitted monitor list is indistinguishable from a forgotten one. + +``` +# architect — state (vNN, ~HH:MM UTC — , DELIBERATE /clear cycle) +# ⭐ THIS /clear IS INTENTIONAL (owner-directed context refresh). On re-init: normal +# /arch-init flow, then: +# 1. MONITORS: — watch target, +# cadence, alert pattern. Self-test once before trusting alerts. ("none armed" is a +# valid and complete answer.) +# 2. DONE pre-clear, with receipts: MERGED (, verified on origin/); +# PUSH-VERIFIED ( local==origin). Distinguish "written" from "verified" — +# a cold reader cannot tell. +# 3. ACTIVE LANES: = (; ). +# Name the file, so no instruction lives only in the context being destroyed. +# 4. LATEST RESULTS: , so the first post-resume decision +# needs no archaeology. +# 5. QUEUED, with ordering: — WAITS for ; — . +# 6. ENVELOPE: ; . +``` + +## Guardrails (architect-wide) + +- **Never auto-approve porch gates.** A gate notification is for the human, not you. +- **Touch only your own builders / spawns / filings.** +- **Never `cd` into a builder worktree**; use `git -C` and absolute paths. +- **Stay on the default branch at the workspace root.** diff --git a/codev-skeleton/resources/commands/agent-farm.md b/codev-skeleton/resources/commands/agent-farm.md index 5a0bb566a..2d48deabd 100644 --- a/codev-skeleton/resources/commands/agent-farm.md +++ b/codev-skeleton/resources/commands/agent-farm.md @@ -342,6 +342,35 @@ afx send [builder] [message] [options] - `--interrupt` - Send Ctrl+C first - `--raw` - Skip structured message formatting - `--no-enter` - Do not send Enter after message +- `--delay ` - Deliver after N seconds instead of immediately + +**Delayed delivery (`--delay`):** + +Tower holds the message and delivers it after the stated delay, so the sending process is +free to exit in the meantime. That is the point: a session can schedule a message to +*itself* for after something that destroys it. + +- **Authorised at request time, delivered later.** Target resolution and the + builder-spoofing check run when the command is issued, exactly as for an immediate send. + A delayed send cannot defer a check past the conditions that would fail it. +- **Bounds:** a whole number of seconds, 1–3600, rejected at both the CLI and server + boundaries — a bad value silently changes *when* (or whether) a message arrives rather + than failing loudly. +- **Not persisted.** A pending message is a Tower-side timer; a Tower restart drops it by + design, since a delayed message's timing was chosen against a world the restart has + already invalidated. Re-send by hand if it matters. +- **Ordering:** a delayed message never overtakes one already queued for that session, and + concurrent deliveries to one session do not interleave. Request order across *differing* + delays is **not** preserved — `--delay 30` then `--delay 5` delivers the 5-second one + first, because that is what `--delay` means. +- **Reporting:** the CLI says "scheduled", not "sent". +- `--interrupt` is combinable (the Ctrl+C defers *with* the message); the API's `escape` + option is not (an ESC bypasses buffering precisely so it interrupts the *current* turn). + +```bash +# Deliver in 15 seconds; this shell can exit immediately +afx send architect:main --delay 15 --raw '/arch-init main' +``` **Description:** diff --git a/codev/plans/1307-arch-save-packaged-save-clear-.md b/codev/plans/1307-arch-save-packaged-save-clear-.md new file mode 100644 index 000000000..1c87428d9 --- /dev/null +++ b/codev/plans/1307-arch-save-packaged-save-clear-.md @@ -0,0 +1,662 @@ +# Implementation Plan: `/arch-save` — packaged save→clear→re-init for architects + +## Metadata +- **ID**: plan-2026-07-31-arch-save +- **Status**: draft +- **Specification**: [codev/specs/1307-arch-save-packaged-save-clear-.md](../specs/1307-arch-save-packaged-save-clear-.md) +- **Created**: 2026-07-31 + +## Executive Summary + +Implements the spec's Approach 1: **one Tower-side send parameter plus one skill.** + +`afx send --delay ` lets Tower hold a message and deliver it later, which is the +only genuinely missing capability — the third leg of the refresh cycle cannot be sent by +the session that is about to be cleared, so something that outlives the clear has to send +it. Tower already mediates every send, so this is a parameter on an existing path rather +than new machinery. + +`/arch-save` is then a document: stop monitors → write the pruned state file → `--raw +'/clear'` → `--delay 15 --raw '/arch-init '`. + +Three phases, ordered so the mechanism is proven before the skill depends on it, and so +the live run lands before the documented default delay is fixed. + +**This plan replaces an earlier seven-phase version** that built a Tower job runner, +verification gates and a handshake protocol. That was descoped by owner directive; the +reasoning is in the spec's Notes. Nothing from the deleted phases is smuggled back in +here. + +## Success Metrics + +From the specification: +- [ ] `afx send --delay` delivers Tower-side, sender free to exit immediately. +- [ ] Composes with `--raw`, formatted messages, and every addressing form; undelayed + behaviour unchanged. +- [ ] Invalid delays rejected at the CLI boundary. +- [ ] `/arch-save` ships in all four skill trees with the write-then-clear ordering and the + pruning requirement. +- [ ] A real architect completes save → clear → resume in a live workspace. +- [ ] `CLAUDE.md`/`AGENTS.md` byte-identical; `--delay` documented. + +Implementation-specific: +- [ ] >90% coverage of the new delivery path. +- [ ] No leaked timers on delivery, failure, or shutdown. +- [ ] A delayed send is subject to the same spoofing check as an immediate one. + +## Phases (Machine Readable) + + + +```json +{ + "phases": [ + {"id": "phase_1", "title": "afx send --delay (Tower-side deferred delivery)"}, + {"id": "phase_2", "title": "/arch-save skill in four trees + state-block template"}, + {"id": "phase_3", "title": "Live end-to-end run and documentation"} + ] +} +``` + +## Phase Breakdown + +### Phase 1: `afx send --delay` + +**Dependencies**: None + +#### Objectives +- Add Tower-side deferred delivery to the existing send pipeline, without altering + undelayed behaviour. + +#### Deliverables +- [ ] `--delay ` on the send command in + `packages/codev/src/agent-farm/cli.ts:448-454`, with boundary validation. +- [ ] `deliverAfter` plumbed through `SendOptions` + (`packages/codev/src/agent-farm/types.ts`), `commands/send.ts`, and **the core + client `packages/core/src/tower-client.ts` (`sendMessage`, line 655)** — note + `agent-farm/lib/tower-client.ts` is only a re-export shim, so this is a + cross-package change with core-first build ordering. +- [ ] Tower-side scheduling in the send route (`servers/tower-routes.ts`, around the + existing `shouldDefer` branch at :1570). +- [ ] A delayed-send registry with a shutdown function wired into `tower-server.ts`'s + graceful-shutdown sequence (~:151). +- [ ] `deferred`/`scheduled` surfaced in the CLI result (currently discarded in + `commands/send.ts`). +- [ ] `packages/codev/src/agent-farm/__tests__/spec-1307-send-delay.test.ts` +- [ ] Core-side test coverage for the `sendMessage` parameter. + +#### Implementation Details + +**Authorise now, deliver later.** Target resolution and the builder-spoofing check +(`servers/tower-messages.ts:225-234`) run at request time, exactly as today, so a delayed +send cannot dodge a check by deferring it. Only delivery is scheduled. Note the spoofing +check fires on the `architect:` path specifically — the bare `architect` path has +separate affinity logic — so the request-time authorisation test must use +`architect:` to exercise it. + +**Due messages re-enter the normal delivery path — this is the critical rule.** `/api/send` +already defers messages through `SendBuffer` (Spec 403) when the user is typing: +`shouldDefer = !interrupt && !session.isUserIdle(3000)` (`tower-routes.ts:1570`), holding +for up to 60 seconds. If a delayed message wrote directly to the session, this sequence +would invert the one ordering the whole feature promises: + +``` +T+0 /clear sent → user typing → BUFFERED (up to 60s) +T+15 /arch-init due → direct write → LANDS FIRST +T+40 buffer flushes → /clear lands → wipes the recovered context +``` + +So a due message re-enters the same path — buffering included — rather than writing to the +session. The existing per-session queue then does the work, and ordering stops depending +on timing luck. + +The guarantee is deliberately narrow: a delayed message never overtakes one **already +queued** for that session. It is NOT request-order across differing delays — `--delay 30` +followed by `--delay 5` delivers the 5s one first, because that is what `--delay` means. +Separately, concurrent deliveries to one session must not interleave, which requires +waiting out each other's *paced writes*, not merely their scheduling. + +**Delivery must re-resolve, not close over a session.** Retain the *authorised terminal +id*; at delivery, re-fetch that exact session and re-check it is writable. Holding a +`PtySession` reference across a 15-second gap risks writing into a session that has since +died or been replaced. + +**Validation** at the CLI boundary, matching `reset`'s pattern (`cli.ts:513-522`): +positive integer with a maximum (one hour) so a typo cannot park a message indefinitely. +Reject NaN explicitly — `NaN > 0` and `NaN <= 0` are both false, so a single comparison +written the obvious way lets it through. + +**Composition** is decided rather than left open: `--raw`, `--file` and `--no-enter` are +payload/formatting concerns and simply travel with the delayed message. `--all` fans out +and each delivery is scheduled independently. **`--interrupt` currently writes Ctrl+C at +request time** — with `--delay` it must be deferred *with* the message, or the interrupt +lands now and the message 15 seconds later. There is no `--escape` CLI flag +(`cli.ts:450-454`); interrupts are `afx interrupt`, so the spec's mention is recorded N/A. + +**Not persisted.** A pending message is a Tower-side timer. Shutdown **drops** delayed +sends rather than flushing them — unlike `SendBuffer`, whose flush-on-shutdown is correct +for messages already accepted for immediate delivery. A dropped `/arch-init` is recovered +by a manual re-send; a flushed-on-shutdown one could land in a session that has moved on. + +**Why not `tower-cron.ts`**: its 60-second tick is too coarse, and `CronDeps.resolveTarget` +takes no `sender`, so routing through it would drop affinity and the spoofing check. +Stated here so reviewers do not re-litigate it. + +#### Acceptance Criteria +- [ ] `afx send --delay N` returns immediately; the message lands after ~N seconds. +- [ ] **Ordering holds under buffering**: a `/clear` held by `SendBuffer` is delivered + before a `/arch-init` whose delay expires while the first is still buffered. Tested + with the buffer deliberately engaged, not just with an idle session. +- [ ] Works with `--raw`, `--file`, `--no-enter`, `--all`, `--interrupt`, formatted + messages, and `` / `architect` / `architect:` addressing. +- [ ] `--interrupt` with `--delay` defers the Ctrl+C **with** the message. +- [ ] Sends without `--delay` are unchanged in behaviour and timing. +- [ ] Zero, negative, non-integer, NaN and over-maximum delays rejected before scheduling. +- [ ] A delayed `architect:` send from a builder that does not own that architect is + refused **at request time**, not at delivery time. +- [ ] Delivery re-fetches the session by terminal id and re-checks writability; a target + that vanished fails gracefully with no unhandled rejection. +- [ ] Shutdown **drops** pending delayed sends (does not flush them) and leaks no timers. +- [ ] The CLI reports "scheduled", not "sent", and surfaces the `deferred` flag. +- [ ] All tests pass. Code review completed. + +#### Test Plan +- **Unit Tests**: delay validation; scheduling with a fake clock; registry cleanup on + delivery, failure and shutdown; request-time spoofing refusal via `architect:`; + `--interrupt` deferral. +- **Integration Tests**: real route handler with a fake session — the buffered-ordering + scenario above, delayed vs undelayed, and the vanished-target case. +- **Manual Testing**: `afx send --delay 10 "ping"` from a shell that exits + immediately; then repeat while typing into the target terminal, to see the buffer and + the delay interact. + +#### Rollback Strategy +Remove the flag and the `deliverAfter` branch. The change is additive — the undelayed path +is untouched — so reverting cannot strand callers. + +#### Risks +- **Risk**: a delayed message overtakes a buffered one, inverting `/clear` and + `/arch-init` so the clear destroys the recovered context. + - **Mitigation**: due messages re-enter the normal delivery path including + `SendBuffer`; the inversion scenario is an explicit acceptance test with the buffer + engaged. **This is the one hazard here that a manual re-send cannot repair**, so it + is designed out rather than accepted. +- **Risk**: authorisation is accidentally deferred along with delivery, letting a delayed + send bypass the spoofing check. + - **Mitigation**: resolve-and-authorise-now, deliver-later is the phase's central rule, + with the request-time refusal as an explicit criterion and test — not left implicit + in "it reuses the existing path." +- **Risk**: a stale `PtySession` captured at request time is written to 15 seconds later. + - **Mitigation**: retain the terminal id, re-fetch and re-check writability at delivery. +- **Risk**: the cross-package edit is made only in the `agent-farm` shim, so nothing + actually changes. + - **Mitigation**: `packages/core/src/tower-client.ts:655` named explicitly, with + core-first build ordering called out. + +--- + +### Phase 2: `/arch-save` skill and state-block template + +**Dependencies**: Phase 1 + +#### Objectives +- Ship the architect-facing procedure and the resume-block format the live run validated. + +#### Deliverables +- [ ] `.claude/skills/arch-save/SKILL.md` +- [ ] `.codex/skills/arch-save/SKILL.md` +- [ ] `codev-skeleton/.claude/skills/arch-save/SKILL.md` +- [ ] `codev-skeleton/.codex/skills/arch-save/SKILL.md` +- [ ] Scaffolding assertions in `packages/codev/src/__tests__/scaffold.test.ts` (:302), + `init.test.ts` (:68), `update.test.ts` (:105) **and `adopt.test.ts` (:92)**, + mirroring `arch-init`'s existing coverage. +- [ ] **Updates to the four existing `arch-init` SKILL.md copies**, whose "Saving your + state" section still documents the manual save→suggest-`/clear`→human-clears loop. + Leaving it unchanged ships two contradictory procedures for the same task. + +#### Implementation Details + +Skills are discovered by directory (`lib/scaffold.ts:copySkills` iterates entries), so no +manifest edit is needed — but all four trees must carry it or adopters silently lack the +command. + +**The procedure, in order** (the ordering is the feature): +1. Resolve identity — `afx whoami`, or an explicit name argument. Never guess; no implicit + fallback to `main` (#1094). Validate against `[a-z][a-z0-9-]*`, ≤64 chars, before + building any path. +2. **Stop your own monitors.** This is the enforceable half of the monitor problem: this + context holds the handles and the post-clear one does not. +3. **Write the pruned state file** to `codev/state/.md` — rewrite current state in + place, append one dated entry, **and compact**: resolved loops deleted, older entries + collapsed into pointers at durable artifacts, one-screen order of magnitude. Optionally + `cp` the previous version first; these files are gitignored, so a bad save has no undo. +4. `afx send architect: --raw '/clear'` +5. `afx send architect: --delay 15 --raw '/arch-init '` +6. Stop. Do not start new work. + +**Submission atomicity comes from Spec 1273's per-session submission lock — adopt it, +do not build a rival.** (Architect ruling, 2026-08-01; aspir-1273 owns the primitive.) + +Ordering and atomicity are different layers, and this plan only solved the first. +`writeMessageToSession` writes the text and schedules its Enter via `setTimeout` +(`message-write.ts:16-19`: 50ms short, 80ms paced), and `/api/send` responds once the +write is *scheduled*, not once it is *submitted*. So two correctly-ordered sends can still +coalesce into one user turn if the second is written before the first's Enter fires — which +is exactly what happened to `afx reset` in production: its `/clear` arrived as literal text +welded to the front of the next message, never executed, context fully intact. + +`--delay 15` puts ~15 seconds between this skill's two sends, so it does not sit in the +50ms coalescing window. That is a property of the delay, not a guarantee of the send path: +if the delay is ever shortened, or a caller sequences two undelayed sends, the hazard is +live. When 1273's lock lands, this sequence adopts it unchanged and phase 1's narrower +`writeCompletesInMs` wait on the delayed path should be **deleted** rather than kept +alongside it — one mechanism, not two. + +**The address must be `architect:`, never bare `architect`.** For a non-builder +sender the bare form resolves to `main` or the first registered architect +(`servers/tower-messages.ts:371-372`), so a sibling architect's `/arch-save` would clear +**main's** terminal. That is the worst outcome this feature can produce, it lands on +someone who never invoked anything, and it is one word away from correct. The skill uses +the resolved name explicitly and says why. + +**Why step 3 precedes step 4** must be stated in the doc, not just implied by ordering: the +context that knows what to write is the one about to be destroyed. + +**`--raw`, never `--escape`** — with the reason, because the failure is silent: Tower's +escape route discards the message body, so a `/clear` sent as an escape delivers a bare +interrupt and nothing is cleared. + +**Content the skill must state plainly**: +- The **owner-direction rule** with the standard override carve-out: "don't autonomously + invoke this mid-task on your own judgment," not "this is forbidden." +- **Prune by pointer, never by deletion** — gitignored files have no history to recover. +- Content guardrails from `/arch-init`: no secrets, no transcript dumps, no raw tool output. +- **Post-clear monitor order**: reconcile against the state block's list, disregard any + alert you cannot account for as stale, *then* re-arm — self-testing once before trusting + a re-armed monitor's alerts. +- **What to do when `/arch-init` does not arrive**: re-send it by hand. This is the + recovery the whole design leans on, so it belongs in the doc rather than in tribal + knowledge. + +**The state-block template** carries the seven elements the live run validated: intent +stamp, monitor list, DONE-with-receipts, active lanes with brief pointers, latest results, +queued-with-ordering, authorization envelope. + +#### Acceptance Criteria +- [ ] `codev init` into a clean directory produces the skill in both provider trees; + `codev adopt` and `codev update` backfill it without touching a customised copy. +- [ ] All four `arch-save` copies identical. (Note: this means *this skill* across the + four trees — the skeleton trees deliberately carry a subset of skills overall, so + full tree parity is not the claim. `skill-parity.test.ts` already checks + provider-tree byte parity dynamically and should pick this up for free.) +- [ ] The doc states the write-before-clear reason, the `architect:` reason, the + `--raw` reason, the pruning requirement, the owner-direction carve-out, and the + manual-re-send recovery. +- [ ] The four `arch-init` copies no longer document a manual loop that contradicts + `/arch-save`. + +#### Test Plan +- **Unit Tests**: scaffold/init/update assertions mirroring `arch-init`'s. +- **Integration Tests**: none — this phase ships documents. +- **Manual Testing**: walk the procedure in a scratch architect session through step 3, + stopping before the clear. + +#### Rollback Strategy +Delete the four directories; no code depends on them. + +#### Risks +- **Risk**: the skill ships in one tree and not the others. + - **Mitigation**: four-tree assertion is an acceptance criterion, plus a repo-wide grep + across `codev/` and `codev-skeleton/`. +- **Risk**: the procedure is followed but the pruning step is skipped, since nothing + enforces it. + - **Mitigation**: stated as a requirement with its rationale. Accepted as unenforced — + the spec is explicit that nothing verifies it, and adding a gate was the descoped + design. + +--- + +### Phase 3: Live end-to-end run and documentation + +**Dependencies**: Phase 2 + +#### Queued merge actions — all three in one place + +Three separate pending merges is exactly where one gets forgotten, so they are listed +together and each says what "done" looks like. + +| # | Waiting on | Action | Verified? | +|---|---|---|---| +| 1 | PR #1320 (1273 submission lock) | Merge; resolve `tower-routes.ts` keeping BOTH sides; **add** two `submitToSession` call sites (delayed delivery, `flush()` drain); *then* delete `writeCompletesInMs`, `busyUntil` + its flush busy-gate, and `delayed-send.ts`'s chain | Conflict surface measured (`git merge-tree`): one file, `tower-routes.ts` | +| 2 | PR #1327 (1280 invariant tests) | **Drop this project's baseline bump** to `spec-1280-measurement-instrument.test.ts` — take theirs wholesale for that file | **Yes.** Ran their new file against this branch's docs: 24/24 pass unchanged, so the drop is safe | +| 3 | #1320 merged + installed | Live e2e, batched with 1273's probe retest (architect ruling) | Runbook below | + +On (2): the bump exists only because 1280's original form pinned live absolute counts, which +fire on any always-on edit. #1327 replaces that with invariants plus a synthetic fixture, so +a +62-word real-repo change keeps all 24 green by design. Their new file comments on this +case by name. Carrying the bump past that merge would be a conflicting edit with no purpose. + +#### Adopting Spec 1273's submission lock (added 2026-08-01) + +PR #1320 (`builder/1273-submission-lock`) adds +`submitToSession(sessionId, write, clock?)` in `servers/session-submit.ts` — a per-session +promise chain where each submission waits out its own Enter. It is wired into `/api/send`'s +escape and immediate paths, deliberately **not** the buffered path (awaiting a message that +can sit 60s would hang callers). + +**Merge state, measured not assumed** (`git merge-tree`, 2026-08-01): merge-base +`57c51a6e`; their branch has none of this project's 36 phase-1 commits. Exactly one +conflicting file — `servers/tower-routes.ts`, where both sides edited `handleSend` and +`deliverBufferedMessage`. `tower-routes.test.ts` auto-merges. **This project merges second +and therefore resolves.** Both sides must survive: their `submitToSession` wiring, and this +project's `--delay` parsing/validation, `escape`+`delay` rejection, and `interruptFirst`. +Verify by running both mutation-verified suites, not by inspecting the resolution. + +**Then delete the three narrower mechanisms** built here before the primitive existed — +`deliverOrBuffer`'s `writeCompletesInMs` wait, `SendBuffer.busyUntil` (and its `flush()` +busy-gate), and the per-terminal chain in `delayed-send.ts`. One mechanism, not two. + +**The deletion is not a deletion — it is a replacement, and 1273 corrected my framing +here.** #1320 wires `submitToSession` into the escape and immediate paths only; the +buffered and delayed paths are deliberately left to whoever owns them. So this project must +**add two `submitToSession` call sites** — one in the delayed delivery path, one wrapping +`flush()`'s drain — *before* removing anything. `write: () => number` may perform many +writes and return the final offset, so a whole flush batch is one reservation with the +existing offset threading intact (1273 pinned that with a batch test; mutation-verified on +their side). + +**Verify the replacement with this project's own test, not theirs.** Their test proves the +*primitive* supports the pattern; it cannot prove this project's *wiring* of it is correct. +Those are different claims. Concretely: re-run the mid-flush ordering test after wiring and +before deleting `busyUntil`. If the property does not survive, take the specific failing +case back to 1273 rather than reinventing a local guard. + +Sequence: (1) #1320 lands; (2) merge main, resolve `tower-routes.ts` keeping both sides; +(3) send the resolved `handleSend` to 1273 for a diff against their intent — the failure +mode of a bad resolution is silent; (4) run both mutation-verified suites; (5) wire the two +call sites, re-run the mid-flush test, *then* delete `writeCompletesInMs`, `busyUntil` + +its `flush()` busy-gate, and `delayed-send.ts`'s chain; (6) report back whether any +`ORDERING:` test broke, either way. + +If #1320 has not landed when this project is ready to open its PR, ship as-is and do the +adoption as a follow-up — but say so explicitly in the PR rather than leaving two +mechanisms unremarked. + +#### Objectives +- Run the real cycle, fix the documented default delay from observation, and document the + command. + +#### Deliverables + +**AMENDED 2026-08-02** (architect ruling; recorded in place rather than left as +misleading checkboxes, the way the spec's byte-identical criterion was). The live run and +delay calibration are **moved to the verify phase** — `/arch-save` is architect-only and a +builder must refuse it, so its live cycle cannot run during implement; it runs post-merge as +a throwaway-sibling probe. The `CLAUDE.md`/`AGENTS.md` item is **superseded**: Spec 1280's +Phase 1 restructured `CLAUDE.md` so per-flag CLI detail no longer belongs there, so the +correct outcome is *no* `--delay` content in the always-on surface and the reference in +`agent-farm.md` (both trees) instead. + +- [→ verify] A completed live run: a real architect saves, clears, and resumes. +- [→ verify] Confirmed or corrected default delay in the skill (needs the live + send→session-ready measurement). +- [x] `codev/resources/commands/agent-farm.md` — `--delay` reference (both `codev/` and + `codev-skeleton/`). +- [x] `CLAUDE.md`/`AGENTS.md` remain byte-identical with **no** `--delay` content + (superseded, per above). +- [x] `codev/reviews/1307-*.md`. + +#### Implementation Details + +Three questions the live run answers, none of which unit tests can: + +1. **Does `/clear` take effect when typed over the raw channel?** Never verified + end-to-end — Spec 1273's live run was never done. Manual practice in the proposing + workspace is the existing evidence. +2. **Does raw-typed `/arch-init ` land, or does slash-command autocomplete + intercept the Enter?** Manual runs succeed, but not over this delivery path. If it + bites, the fallback is a plain-text message naming identity and state-file path, which + has no completion surface — a skill edit, not a code change. +0. **Does the `/clear` actually get SUBMITTED, not just written?** Spec 1273's production + e2e found its `/clear` welded to the front of the next message as literal text, never + executed — the coalescing failure described in phase 2. The live run must confirm the + clear *executed* (a harness clear announcement, context genuinely gone), not merely + that the text arrived. "It was written" is the exact thing that looked like success in + 1273's run and was not. If 1273's submission lock has landed by then, verify through + it; if not, this is the check that would catch the same failure here. +3. **Is 15 seconds right — and 15 seconds from *when*?** The delay budget starts when the + send is issued, but `/clear` cannot execute until the architect's turn ends, and the + turn continues for as long as the skill takes to finish. So the interval that actually + matters is **send → session-ready-after-clear**, not send → clear-sent. Measure that, + and set the documented default from it. A default calibrated against the wrong + interval would look right in testing and misfire whenever a turn runs long. + +**Exercise the recovery path too**, deliberately: drop the delayed message and re-send +`/arch-init ` by hand. The design's central claim is that this recovers everything, +and a claim the whole risk posture rests on should be run at least once rather than +assumed. + +#### Live-run runbook + +Written ahead of the window so execution is mechanical. The run is batched with Spec +1273's probe retest after #1320 merges (architect ruling, 2026-08-01) — running before that +would test the pre-fix world, in which a `/clear` can arrive without executing. + +**Precondition**: #1320 on main, merged into this branch, `pnpm install`, clean build. + +1. **Plant a canary.** Before anything, have the architect commit a distinctive fact to + memory (a secret word). The post-clear check is whether it can still recite it — the + only observation that distinguishes "context cleared" from "looks cleared". 1273's probe + used exactly this and it is what caught their silent failure. +2. **Record the pre-state**: `codev/state/.md` size and its last dated entry; + `afx status` for the architect's terminal id. +3. **Run `/arch-save`** on the architect, on the owner's direction. +4. **Q1 — did the state file get written AND pruned?** Compare against step 2: new dated + entry present, resolved loops gone, `MONITORS:` line present, not merely longer. +5. **Q2 — did the `/clear` EXECUTE?** The decisive question, and the one that looked green + in 1273's run while failing. Check all three: + - a harness clear announcement / `` block in the terminal output; + - the canary from step 1 is **gone**; + - the `/clear` did **not** appear as literal text welded to the front of another + message. + "The send returned 200" is not evidence. It was 200 in the failing run too. +6. **Q3 — did `/arch-init` arrive, and at the right moment?** Measure **send → + session-ready-after-clear**, not send → clear-sent. The delay budget starts at the send + while the clear cannot execute until the turn ends, so the interval that matters is the + one that spans both. +7. **Q4 — did the fresh session recover?** Reports its identity, resumes from the state + file, and performs the monitor steps in order (reconcile/disregard, then re-arm with a + self-test). +8. **Exercise the recovery path deliberately**: drop the delayed message (or let it expire) + and re-send `/arch-init ` by hand. The whole risk posture rests on this working, + so it gets run once rather than assumed. +9. **Set the documented default** from step 6's measurement, in all four skill copies, and + re-run the drift guard. + +Record the answers in the review even if the run is clean — a live run with no findings is +still the evidence that the headline path works, and its absence is what let 1273 ship a +`/clear` that never executed. + +#### Acceptance Criteria +- [ ] A real architect completes the cycle and reports its identity from the state file. +- [ ] Default delay set from observation. +- [ ] Manual re-send recovery exercised and confirmed. +- [ ] `diff CLAUDE.md AGENTS.md` is empty. +- [ ] Command reference documents `--delay`, its maximum, and the not-persisted behaviour. + +#### Test Plan +- **Unit Tests**: none new. +- **Integration Tests**: none new. +- **Manual Testing**: this phase is the manual test — the full cycle, the autocomplete + question, the delay calibration, and the recovery path. + +#### Rollback Strategy +Documentation-only. If the live run shows the cycle does not work, the skill stays +unshipped; `--delay` is independently useful and can stand alone. + +#### Risks +- **Risk**: `/clear` does not take effect over the raw channel, making the cycle inert. + - **Mitigation**: manual field evidence says it does. If it fails, the failure is loud + and harmless — the architect keeps its context and receives a stray `/arch-init`. +- **Risk**: the live run is skipped under time pressure. + - **Mitigation**: it is the phase's only deliverable; there is nothing else to ship here + that could stand in for it. + +## Dependency Map + +``` +Phase 1 (--delay) ──→ Phase 2 (skill) ──→ Phase 3 (live run + docs) +``` + +Strictly sequential. Phase 2's procedure calls the flag phase 1 adds; phase 3 calibrates +the value phase 2 documents. + +## Resource Requirements + +### Development Resources +- **Engineers**: one builder. +- **Environment**: local Tower; phase 3 needs a live workspace with a real architect + terminal. + +### Infrastructure +- **Database changes**: none. +- **New services**: none — delivery is a timer inside the existing Tower process. +- **Configuration updates**: none; the delay is a per-invocation flag. +- **Monitoring additions**: none. + +## Integration Points + +### External Systems +None. + +### Internal Systems +- **Tower send pipeline** (`servers/tower-messages.ts`, `servers/message-write.ts`) — + phase 1. *Fallback*: Tower down is an ordinary send failure, as today. +- **`lib/scaffold.ts` / `codev init|adopt|update`** — phase 2. *Fallback*: none needed; + discovery is directory-based. +- **`/arch-init` skill** — the recovery entry point the delayed message invokes. Phases + 2–3. *Fallback*: a human re-sends it. + +## Risk Analysis + +### Technical Risks +| Risk | Probability | Impact | Mitigation | Owner | +|------|------------|--------|------------|-------| +| **Delayed `/arch-init` overtakes a buffered `/clear`; the clear then wipes the recovered context** | M | **H — manual re-send does not repair it** | Due messages re-enter the normal delivery path including `SendBuffer`; inversion tested with the buffer engaged | Builder | +| **`/arch-save` clears the wrong architect (bare `architect` → main)** | M | **H — hits an uninvolved session** | Skill addresses `architect:` explicitly, with an acceptance criterion | Builder | +| A delayed send defers its authorisation check too | L | H | Resolve-and-authorise at request time, schedule only delivery; asserted by test via `architect:` | Builder | +| A stale `PtySession` is written to at delivery | M | M | Retain terminal id; re-fetch and re-check writability at delivery | Builder | +| The cross-package edit lands only in the re-export shim | M | M | `packages/core/src/tower-client.ts:655` named; core-first build ordering called out | Builder | +| Delay calibrated against send→clear-sent instead of send→session-ready | M | M | Phase 3 measures the interval that matters and says which one it is | Builder | +| Leaked timers in Tower | M | L | Cleanup asserted on delivery, failure and shutdown; shutdown drops rather than flushes | Builder | +| `/clear` does not take effect over `--raw` | L | H | Manual field evidence; loud and harmless if it fails | Builder | +| Autocomplete intercepts raw-typed `/arch-init ` | L | M | Confirmed in phase 3; fallback is a plain-text payload (skill edit only) | Builder | +| 15s default is wrong | M | L | Calibrated in phase 3; tunable per invocation | Builder | +| Skill ships in fewer than four trees | M | L | Four-tree acceptance criterion + repo-wide grep | Builder | +| Pruning requirement ignored in practice | M | M | Documented with rationale; accepted as unenforced by design | Builder/Architect | + +### Schedule Risks +| Risk | Probability | Impact | Mitigation | Owner | +|------|------------|--------|------------|-------| +| Phase 3 blocked on architect-terminal availability | M | M | Phases 1–2 are fully testable without one | Builder | +| Scope creep back toward the descoped architecture | M | H | Out-of-scope list restated in the spec and this plan; any "we should also verify…" belongs to a future spec | Builder/Architect | + +## Validation Checkpoints + +1. **After Phase 1**: a delayed send arrives after its delay, from a process that has + already exited; undelayed sends unchanged; authorisation still happens at request time. +2. **After Phase 2**: `codev init`/`update` place the skill in all four trees; the doc + states each of its five required points. +3. **Before Production (Phase 3)**: a real architect completes the cycle; the recovery path + is exercised; docs match observed behaviour. + +## Monitoring and Observability + +### Metrics to Track +None new. A delayed send either arrives or does not, and the person who invoked it is +present to see which. + +### Logging Requirements +- Log a delayed send at schedule time (target, delay) and at delivery time (target, + outcome) — enough to tell "never scheduled" from "scheduled and dropped," which are the + two failures worth distinguishing. +- **Never log message bodies or state-file contents.** +- Retention: whatever Tower already does. + +### Alerting +None. This is a human-initiated operation reporting synchronously to the person who ran it. + +## Documentation Updates Required +- [ ] `codev/resources/commands/agent-farm.md` — `--delay` +- [ ] `CLAUDE.md` and `AGENTS.md` (byte-identical) +- [ ] The four `SKILL.md` copies +- [ ] `codev/reviews/1307-*.md` +- [ ] Architecture diagrams: not required — no new subsystem +- [ ] Runbooks / user guides / configuration guides: not required + +## Post-Implementation Tasks +- [ ] Security audit: authorisation timing on delayed sends; path validation on `` +- [ ] Performance validation: **N/A** — one timer per pending send +- [ ] Load testing: **N/A** +- [ ] User acceptance testing: the phase-3 live run +- [ ] Monitoring validation: **N/A** — no new metrics + +## Expert Review +**Date**: pending +**Model**: Codex and Claude — run by porch at the end of this phase. +**Key Feedback**: +- (to be recorded) + +**Plan Adjustments**: +- (to be recorded) + +## Approval +- [ ] Technical Lead Review +- [ ] Engineering Manager Approval +- [ ] Resource Allocation Confirmed +- [ ] Expert AI Consultation Complete + +## Change Log +| Date | Change | Reason | Author | +|------|--------|--------|--------| +| 2026-07-31 | Initial plan (7 phases, Tower job architecture) | Spec 1307 entered plan phase | Builder aspir-1307 | +| 2026-07-31 | Rewritten to 3 phases | Owner descope directive: `afx send --delay` + a skill replaces the Tower-owned job, handshake, and intent-record machinery | Builder aspir-1307 | +| 2026-07-31 | Plan CMAP iteration 1 | Both reviewers independently found the `SendBuffer` ordering inversion and the bare-`architect` addressing bug; plus core-vs-shim file targeting, delivery re-resolution, shutdown wiring, flag composition, `adopt` coverage, `arch-init` doc contradiction, and the delay-budget interval | Builder aspir-1307 | + +## Notes + +**On the rewrite.** The first version of this plan had seven phases: a shared extraction +from `commands/reset/`, a validation module with a compaction predicate, a clear-job state +machine with six ordering invariants, a Tower job surface with status/cancel and durable +intent records, a CLI with a `--begin`/`--boundary` handshake, then the skill and a +bake-off. It was a competent plan for the wrong feature. The owner's descope removed the +question it answered, and almost all of it went away — correctly. + +**What phase 1 must get right, since it is now most of the code.** The temptation is to +treat `--delay` as "the same send, later." It is, for delivery — but *not* for +authorisation. Target resolution and the builder-spoofing check must happen at request +time, or a delayed send becomes a way to defer a check past the conditions that would fail +it. That is the single security-relevant decision in this plan, which is why it has its own +acceptance criterion and its own test rather than living inside "reuses the existing path." + +**On the two hazards that are NOT accepted risk.** The plan review surfaced two failures +that the manual-re-send posture does not cover, because in both the damage lands on a +context that is not the one being refreshed: a delayed `/arch-init` overtaking a buffered +`/clear` (the clear then wipes the recovered session, and re-sending re-runs the race), and +bare-`architect` addressing clearing main instead of the sibling that invoked it (the +victim never invoked anything). Both are designed out — FIFO re-entry into the delivery +path, and explicit `architect:` addressing — not accepted. The recoverability +argument is load-bearing for this whole design, so its boundary has to be as precise as its +claim. + +**On accepted risk.** The remaining hazards this design does not close — mistimed delivery, a dropped +message on restart, work started between save and clear — are all recoverable by re-sending +one message by hand. That recovery is exercised in phase 3 rather than assumed, because the +entire risk posture rests on it. Issue #1310 is the primitive that would let a future +version replace the timing assumption with observation, if evidence ever shows these bite +in practice. It is **not** a dependency of this work. + +**Out of scope**, restated so the plan cannot re-absorb it: Tower-side quiescence detection, +clear confirmation, verification gates on the state file, job status/cancellation surfaces, +listing or cancelling pending delayed sends, persisting delayed sends across restarts, +cross-workspace or sibling-architect targeting, UI surfaces, and building #1310. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter1-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter1-rebuttals.md new file mode 100644 index 000000000..d10028768 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter1-rebuttals.md @@ -0,0 +1,119 @@ +# Phase 1 (`afx send --delay`) — Rebuttals, iteration 1 + +Both reviewers returned `REQUEST_CHANGES` (HIGH confidence). **All findings accepted and +fixed.** Nothing defended. + +They converged on four issues independently — the third round running where independent +convergence has picked out the items that actually mattered. + +Fix commit: `413e4261`. Build clean, 4059 tests passing. + +--- + +## 1. Ordering tests asserted against a *copy* of the predicate — both reviewers + +**Accepted, and this was the most serious finding in the round.** + +`spec-1307-send-delay.test.ts` re-implemented `shouldDefer` inside the test file. So the +tests guarding the one hazard that a manual re-send *cannot* repair — a delayed +`/arch-init` overtaking a buffered `/clear`, after which the clear wipes the recovered +context — would have kept passing if the shipped predicate in `tower-routes.ts` regressed. +A guard bolted to a replica of the thing it guards. + +Worse, the plan's own Test Plan called for "a route-level test with the buffer engaged." +I wrote route-level tests for eight *other* behaviours and left the one that mattered as a +local copy. + +**Fixed**: two route-level tests exercising the real `handleRequest` and the real +module-level `SendBuffer`, structured so the session is *idle* at delivery and the +earlier message is still queued — which isolates the `hasPending` term specifically rather +than passing via the pre-existing "user is typing" term. + +**Verified by mutation.** With `queueAhead` forced to `false`, both new tests fail; the +guard restored, both pass. I ran this because a regression guard that has never been +observed failing is a guess about its own value. The predicate-copy tests are retained +(they document the rule readably) but they are no longer the only thing standing between +the codebase and that inversion. + +## 2. Delayed `--interrupt` bypassed FIFO entirely — both reviewers + +**Accepted.** `shouldDefer = !interrupt && (...)`, so `--interrupt --delay` wrote directly +and could overtake queued messages — reintroducing the exact inversion through a side +door, in the same function that argues against it at length. + +Codex called it a violation of the phase's ordering requirement; Claude judged it a +documented gap, being off the `/arch-save` path. **Codex's reading is the right one.** The +existing justification for interrupts bypassing the buffer — "an interrupt that can be +deferred is not an interrupt" — is sound for an *immediate* interrupt and does not survive +being applied to one already deferred by N seconds. + +**Fixed properly rather than documented.** A delayed interrupt now queues, carrying its +Ctrl+C on the message itself (`BufferedMessage.interruptFirst`), written 100ms ahead of +its own payload at flush time. The queue drains in order *and* the interrupt still +interrupts. + +I considered refusing `--interrupt` with `--delay` (the way `escape` + `delay` is refused). +Rejected: `afx send X --delay 15 --interrupt "msg"` has a clear, legitimate meaning — "in +15s, interrupt and deliver this" — and removing a capability is not a fix for an ordering +bug. Refusal was the cheap option, not the correct one. + +## 3. `--all --delay` reported as "Sent" — both reviewers + +**Accepted.** `sendToAll` pushed every target into `results.sent` and printed "Sent to N +builder(s)" even when Tower had merely *scheduled* them — precisely the misreport the +single-target path had been fixed to avoid, one function away. + +**Fixed**: `sendToAll` returns `scheduled` alongside `sent` and `failed`, and reports them +separately. Covered by tests for delayed-only, immediate-only, and mixed outcomes. + +## 4. `deferred` never surfaced to the CLI — both reviewers + +**Accepted.** The route returns `deferred`, `TowerClient.sendMessage` dropped it, and the +plan's deliverable explicitly listed "`deferred`/`scheduled` surfaced in the CLI result." +I implemented half of it. + +**Fixed**: `deferred` is threaded through the client and reported — a send buffered because +someone is typing now says so, instead of looking like a completed send. + +## 5. Duplicated delay ceiling — Claude + +**Accepted.** `cli.ts` hardcoded `3600` and its error string repeated "between 1 and 3600", +while `delayed-send.ts` exported `MAX_DELAY_SECONDS`. Two bounds that can drift, and the +drift is silent until the CLI accepts something Tower rejects. + +**Fixed**: the CLI imports `validateDelaySeconds`, so there is one bound and one error +message. Claude's observation that `delayed-send.ts` has zero imports made this free. + +## 6. No `--all` + `--delay` coverage — Claude (nit) + +**Accepted.** It was the only composition flag in the spec with no test. Added, at the +reporting layer where the actual risk lives (misreporting scheduled as sent). + +--- + +## Note on the review environment + +Codex reported it could not execute tests (`EPERM` — Vitest writing its generated config +under a read-only filesystem), so its findings came from source inspection. Worth +recording: every one of its findings was still correct, and it independently found the +same four issues Claude found with a working test run. The environment limitation cost +nothing this round, but a reviewer that cannot run tests cannot catch a test that passes +for the wrong reason — which is exactly finding #1. + +## Summary + +| # | Finding | Source | Fix | +|---|---|---|---| +| 1 | Ordering tests asserted against a predicate copy | Both | Route-level tests; mutation-verified | +| 2 | Delayed `--interrupt` bypassed FIFO | Both | Queues, carrying its Ctrl+C | +| 3 | `--all --delay` reported as "Sent" | Both | `scheduled` tracked and reported | +| 4 | `deferred` dropped by the client | Both | Threaded through and reported | +| 5 | Duplicated `3600` ceiling | Claude | Imports `validateDelaySeconds` | +| 6 | No `--all --delay` test | Claude | Added | + +**What I take from this round.** Findings 1 and 4 are the same mistake in two places: I +wrote the *shape* of what the plan asked for and skipped the part that made it load-bearing +— route-level tests for everything except the hazard, and half of a two-field deliverable. +Both passed a self-review because the artifact existed. Existence is not the criterion; +"would this fail if the thing it protects broke?" is, and it is a question I can ask +myself with a two-minute mutation run. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter2-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter2-rebuttals.md new file mode 100644 index 000000000..f49b00c39 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter2-rebuttals.md @@ -0,0 +1,151 @@ +# Phase 1 (`afx send --delay`) — Rebuttals, iteration 2 + +Both reviewers `REQUEST_CHANGES` again. **All findings accepted**, with one framing I +narrowed rather than adopted wholesale — explained below, because the narrowing is the +substantive part. + +Fix commit: `bf4040b5`. core 48 tests, codev 4065 tests, both builds clean. + +--- + +## 1. Codex: independent timers let same-terminal deliveries interleave + +**Accepted — a real bug I had not considered, and the most valuable finding of the round.** + +Each scheduled message owned its own `setTimeout`. Two due for the same terminal at the +same instant both begin delivering concurrently, and delivery is *not atomic*: +`writeMessageToSession` paces multi-line output across several timeouts. So two concurrent +deliveries to one PTY interleave **lines** — producing two mangled messages rather than +two messages. + +**Fixed**: a per-terminal promise chain in `delayed-send.ts`. Each due delivery waits for +the previous one to that terminal. Chains are dropped once drained, so the map tracks +active chains only, and a throwing delivery cannot strand later messages (tested). + +### The framing I narrowed + +Codex described this as violating "the explicit per-session FIFO requirement," implying +delayed messages should be delivered in *request* order. **I did not adopt that**, and the +disagreement is worth stating precisely rather than quietly implementing the smaller fix. + +Two sends with different delays are meant to arrive at different times. `--delay 30` +followed by `--delay 5` should deliver the 5-second one first — that is what the caller +asked for, and enforcing request-order would make `--delay` not mean what it says. + +The guarantee this feature actually makes is narrower: **a delayed message never overtakes +one already QUEUED for that session.** That is the `/arch-save` hazard (a delayed +`/arch-init` jumping ahead of a buffered `/clear`), and it is what `hasPending` closes. +Serialising concurrent deliveries is a *separate* correctness property — no interleaving — +and Codex was right that it was missing. + +Both are now implemented, tested, and documented as distinct. A test pins the +deliver-by-due-time behaviour explicitly, so a future reader does not "fix" it into +request-order. + +My earlier commit messages claimed "per-session order is preserved" broadly, which was +sloppier than the code. Corrected. + +## 2. Both: no coverage of the CLI → client → wire chain + +**Accepted, and blocking was the right severity.** + +`deliverAfter` travels CLI → `SendOptions` → `TowerClient` → HTTP body → Tower. Every hop +except the client was covered, and that one **cannot** be covered from `packages/codev` — +the agent-farm `tower-client.ts` is a re-export shim resolving to core's built `dist`, so a +codev-side test exercises compiled output rather than this source. + +The consequence Claude spelled out: deleting `deliverAfter` from the request body left all +4059 tests green while `--delay` silently degraded to an immediate send. For a feature +whose whole failure mode is "arrives at the wrong time," that is the coverage that matters +most, and the plan had listed it as a deliverable ("Core-side test coverage for the +`sendMessage` parameter") which I did not do. + +**Fixed**: `packages/core/src/__tests__/tower-client-send.test.ts` — 7 tests covering the +field on the wire, its absence when unset, `scheduled`/`deferred` surfaced, and +back-compat when an older Tower omits them. Plus `--delay` cases in `send.test.ts` for the +CLI→client hop, including `--all`. + +**Mutation-verified**: removing `deliverAfter` from the request body now fails two core +tests. Same check I ran on the ordering guards last round — a coverage test that has never +been observed failing is a guess about its own value. + +## 3. Both: `--all` classified buffered messages as "sent" + +**Accepted.** `sendToAll` ignored `result.deferred`, so a message Tower had merely buffered +was reported as sent. I had fixed exactly this for `scheduled` in the previous round and +left `deferred` — the same half-a-deliverable pattern review caught last time. + +**Fixed**: `sent` / `scheduled` / `deferred` / `failed` tracked and reported distinctly. + +## 4. Codex: the `--all` reporting tests asserted against a replica + +**Accepted, and it is the same class of mistake as last round's ordering tests.** I tested a +local `summarise()` helper rather than the shipped `send()`, so a regression in the real +reporting path would not have failed anything. I introduced that replica in the *fix* for a +finding about replicas. + +**Fixed**: replaced with real `send()` tests through the existing mocked-`TowerClient` +harness, asserting the actual log output. + +## 5. Claude: stale `queueAhead` across the 100ms interrupt `await` + +**Accepted** (flagged non-blocking; fixed anyway — it is two lines). `queueAhead` was +computed, then `await`ed across for 100ms, then used. A concurrent enqueue in that window +would be overtaken. Now re-checked after the await: a decision taken before an await is a +decision about a world that may have moved on. + +## 6. Claude: comment claimed identity it did not have + +**Accepted.** The local predicate in `spec-1307-send-delay.test.ts` omits the shipped +`!interrupt` term while the comment claimed the rule was "stated identically in both +places." Rewritten to say plainly that it is a *simplification for readability*, that it is +**not** the regression guard, and to point at the route-level tests that are. + +## 7. Claude: `delay` vs `deliverAfter` naming drift + +**Accepted as a documentation gap rather than renamed.** The two names are deliberate: +`delay` matches the user-facing `--delay` flag ("how long the caller asked to wait"); +`deliverAfter` is the wire/client name ("when to deliver"). Documented as intentional in +`types.ts` so the next reader does not have to guess. + +## 8. Claude: undelayed-but-buffered CLI message changed wording + +**Noted, keeping the change.** "Message sent" → "Message queued for X (target is being +typed in)" is a user-visible change on the undelayed path, which brushes against the +spec's "undelayed sends unchanged." But the spec's constraint is about *delivery +behaviour*, and reporting a buffered message as sent is the misreport this phase exists to +stop. Flagging rather than hiding it. + +## 9. Claude: `deliverAfter: null` treated as absent + +**Accepted as correct as-is** — consistent with `undefined`, and the reviewer agreed. + +--- + +## Note on the review environment (carried from round 1) + +Codex again could not execute tests. Its findings were again all correct, and this round it +found the interleaving bug from source inspection alone — a defect no existing test would +have surfaced. Worth recording as a counterweight to my round-1 note: a reviewer that +cannot run tests reads the code more carefully, and that has now paid off twice. + +## Summary + +| # | Finding | Source | Disposition | +|---|---|---|---| +| 1 | Independent timers interleave same-terminal deliveries | Codex | **Fixed** (per-terminal chain); FIFO framing narrowed | +| 2 | No CLI→client→wire coverage | Both | **Fixed** (7 core tests + send tests), mutation-verified | +| 3 | `--all` reported buffered as sent | Both | **Fixed** (4 distinct buckets) | +| 4 | `--all` tests asserted against a replica | Codex | **Fixed** (real `send()` coverage) | +| 5 | Stale `queueAhead` across await | Claude | **Fixed** | +| 6 | Comment claimed false identity | Claude | **Fixed** | +| 7 | `delay` vs `deliverAfter` naming | Claude | Documented as deliberate | +| 8 | Buffered-send wording change | Claude | Kept, flagged | +| 9 | `deliverAfter: null` | Claude | No change (correct) | + +**What I take from this round.** Findings 3 and 4 are both *repeats of last round's lesson +inside last round's fix*: I fixed `scheduled` and left `deferred`; I removed one predicate +replica and introduced another. Fixing a finding is not the same as internalising it, and +the tell is that both regressions live in code I wrote *while addressing the original*. The +check that would have caught both is the one I already know to run — "would this fail if +the thing it protects broke?" — applied to the fix, not just the original. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter3-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter3-rebuttals.md new file mode 100644 index 000000000..670e6296f --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter3-rebuttals.md @@ -0,0 +1,79 @@ +# Phase 1 — Rebuttals, iteration 3 + +Codex `REQUEST_CHANGES`, Claude `APPROVE`. **Codex's finding was accepted and fixed**, and +the work continued through several further review rounds beyond this iteration. + +A note on numbering, since it matters for reading this file: porch's iteration counter and +my consultation filenames drifted apart. I ran review rounds named `iter3` … `iter8` while +porch's counter stayed at 3. Everything below covers that whole sequence, so this rebuttal +answers more than just the two `iter3` files. The later review files are on disk under the +same directory and are the record of the rounds after this one. + +--- + +## Codex (iter3): the per-terminal chain did not actually serialise + +**Accepted, and it was correct about something my own test had disguised.** + +The chain `await`ed `deliverOrBuffer`, but that returns as soon as +`writeMessageToSession` has *scheduled* its paced writes and trailing Enter. Two +same-terminal delayed sends due together therefore still interleaved — short ones +producing `firstsecond\r\r` instead of two messages. My round-2 fix had serialised the +*callback*, not the writes. + +Codex also named why the test missed it: the chain test used an artificially async +callback, so it proved the chain waits for the callback rather than for the writes. + +**Fixed** in `29abc16c`: `deliverOrBuffer` returns `writeCompletesInMs` (a value +`writeMessageToSession` already computed) and the scheduled callback holds the terminal's +chain open for that long. Added a route-level test whose decisive assertion is that the +first message's trailing Enter lands before the second payload begins. Mutation-verified. + +## What the subsequent rounds found (iter4 – iter8) + +Recorded here because they are part of the same phase and the same thread of reasoning: + +- **iter4 — the spec contradicted the code.** My success criterion said a delayed message + "never overtakes an earlier message," which reads as request-order FIFO; the + implementation deliberately lets `--delay 5` overtake `--delay 30`. Codex was right that + the artifacts disagreed. I revised the **spec** rather than the code: `--delay N` is a + statement about *when* to deliver, and enforcing request-order would make the flag + silently not mean what it says. The spec now states the narrow guarantee, the + no-interleave property, and the deliberate exclusion separately (`e0ff15ad`). +- **iter5 — the mid-flush window.** `flush()` drops a session's queue as soon as it has + scheduled its writes, so `hasPending()` went false while `/clear` was still + mid-delivery; a delayed `/arch-init` due in that window wrote *into* it. I had chosen to + document this window; Codex correctly pushed back, and it was worse than I had assessed + (the clear never executes at all). Fixed with `SendBuffer.busyUntil` **and** a busy-gate + in `flush()` — both needed, and I only found the second because the new test failed with + the first applied (`17db2e9e`). +- **iter6 — shutdown did not cancel due-but-not-started deliveries.** Clearing the `chains` + map cannot cancel a callback already attached with `.then()`. Fixed with a generation + guard checked inside the chain callback (`0d8ee648`). +- **iter7 — the guarantee comment had gone stale**, describing a limitation `busyUntil` had + closed while omitting the residual that remains. Rewritten as COVERED / NOT COVERED / + NOT GUARANTEED (`0eaf6689`). +- **iter8 — both reviewers APPROVE.** One papercut fixed: the `--delay` error echoed + `NaN` instead of the user's raw input (`093f6781`). + +## Nothing disputed + +Every finding across these rounds was accepted. There are no false positives to rebut. + +## The pattern, since it recurred + +Three findings were the same mistake in different materials: a test asserting against a +copied predicate, a test asserting against a replica helper, a test asserting against a +synthetic callback — and then a *spec* asserting a guarantee the code did not make. Each +artifact described something *adjacent* to the real thing and passed self-review because +the artifact existed. + +Three others were the same mistake in the code: correct about the mechanism, incomplete +about its lifetime. Serialising the callback but not its writes; guarding `hasPending` but +not `flush`'s own drain; clearing the registry but not the already-attached continuations. +Each worked for the case I was picturing and left the adjacent case open. + +The check that catches both is the same one, and it is cheap: mutate the guard and confirm +the test fails. By the end of the phase I was running it before claiming a fix rather than +after being told — which is how the mid-flush test's vacuous first version got caught by +me instead of by a reviewer. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_2-iter1-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_2-iter1-rebuttals.md new file mode 100644 index 000000000..fb4f35774 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_2-iter1-rebuttals.md @@ -0,0 +1,92 @@ +# Phase 2 (`/arch-save` skill) — Rebuttals, iteration 1 + +Codex `REQUEST_CHANGES`, Claude `COMMENT`. **All findings accepted and fixed**; nothing +disputed. Iteration 2 returned `APPROVE` from both. + +Fix commits: `b4c5d08c`, `45f946e1`. + +--- + +## 1. No instance↔skeleton drift guard (both reviewers) + +**Accepted, and it was the same mistake in a new material.** + +Phase 2's acceptance criterion was "all four copies identical". I verified it by hand with +`md5` and guarded it with nothing. `skill-parity.test.ts` compares Claude against Codex +*within* a tree — it never compares our instance against the shipped skeleton, so the +classic "edited `codev/` and forgot `codev-skeleton/`" drift passes it silently while +shipping a stale skill to every adopter. + +A one-time check is not a guard. This is the fifth instance in this project of an artifact +that exists without doing anything, and the reviewers were right to treat it as the +blocking one. + +**Fixed**: `spec-1307-arch-save-skill.test.ts`, mirroring `spec-1134-arch-init-skill.test.ts` +— four-way byte identity, an explicit instance-vs-skeleton assertion, and content +assertions pinning the statements the plan required the document to make. A skill is a +*document*, so "identical everywhere" is only half of correct; identical copies of a doc +missing its load-bearing warning are still wrong. + +**Mutation-verified twice**: appending one line to the skeleton copy fails both drift +guards; restoring the old overclaim fails the content guard. + +## 2. The Tower timing claim was false (Codex) + +**Accepted — a real accuracy defect, not a wording preference.** + +The skill said Tower "delivers it after the clear has landed". Tower waits out 15 seconds; +it never observes the clear. That promises an observation the system does not make, and it +contradicted this project's own spec, which is explicit that clear completion is not +guaranteed. + +**Fixed**: the skill now states plainly that Tower does not know whether the clear landed, +that 15s is a value that works in practice rather than a guarantee, and that a mistimed +re-init costs one manual message. A content assertion prevents the old phrasing returning. + +Worth naming: this is the same failure as the spec claiming a request-order FIFO guarantee +the code did not make — **prose asserting something adjacent to what the system does**. +Code review catches code drift; nothing automatically catches prose drift, which is why the +content assertions matter more than they look. + +## 3. `arch-init`'s loop diagram still showed only the manual path (Claude) + +**Accepted.** The diagram contradicted the prose two paragraphs below it, and the diagram is +what a reader skims. Now shows both routes, with `/arch-save` as the packaged path and the +manual one as the Tower-unavailable fallback. Pinned by a test. + +## 4. `init.test.ts` assertions are inert (Claude, informational) + +**Confirmed and already documented in place.** `init.test.ts` is excluded at +`vitest.config.ts` ("Flaky: codev doctor timeout in worktree context"), so the assertion I +added there guards nothing. I found this by noticing only four of the five files I named +actually executed, kept the assertion (correct if the exclusion lifts), labelled it +in-place as not counting as coverage, and confirmed the real guard lives in +`scaffold`/`update`/`adopt`, which do run. + +## 5. Step-5 failure after a successful step-4 clear (Claude, iteration 2) + +**Accepted; fixed in `45f946e1`.** Raised as minor but it is a real gap. Step 4 queues the +`/clear`, which only takes effect when the turn *ends* — so a step-5 failure still leaves +the architect holding its full context, and the failure is recoverable. Unless it ends the +turn anyway, converting a recoverable failure into a cleared session with no re-init +scheduled and nobody told. The skill now says so explicitly, pinned by an assertion. + +--- + +## Nothing disputed + +Every finding across both iterations was accepted. There are no false positives to rebut. + +## Post-approval changes (recorded for completeness) + +After both `APPROVE`s, two further changes touched phase-2 files under architect +authorization: + +- **`--delay` documentation relocated out of the always-on surface** (`5bcf52be`). Spec + 1280's Phase 1 restructured `CLAUDE.md` so CLI detail lives in skills and reference docs; + a per-flag pointer there is a regression to the pattern 1280 just deleted. `CLAUDE.md` + and `AGENTS.md` now gain nothing, and the detail lives in + `codev/resources/commands/agent-farm.md` **and its skeleton mirror**. The spec criterion + was amended in place with a dated note rather than silently changed. +- **The `afx` skill is deliberately NOT updated** with `--delay`. Its drift is #1318's to + reconcile, per the same ruling 1280 received. Flagged for the review. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter1-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter1-rebuttals.md new file mode 100644 index 000000000..77c3ecfc2 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter1-rebuttals.md @@ -0,0 +1,82 @@ +# Phase 3 — Rebuttals, iteration 1 + +Both `REQUEST_CHANGES`. **All actionable findings accepted and fixed** in `ddf02abf`; the +remainder are the verify-phase items the architect explicitly scheduled, disclosed here +rather than disputed. + +Fix commit: `ddf02abf`. Suite 4180 passing, build clean. + +--- + +## Code cleanup — accepted and done + +### `writeCompletesInMs` never actually deleted (both reviewers) + +**Accepted, and this was a real miss.** The plan named deleting it after `submitToSession` +integration. I made it a permanent `0` with an unreachable consumer instead of removing it +— the deletion in name, not in fact. Now gone entirely: the field, the settling-wait block, +the `@returns` clause, and the object return type. `deliverOrBuffer` returns a plain +boolean; the delayed scheduler just calls it, because the lock owns serialisation and there +is nothing left to wait out. + +### Four comments credited deleted mechanisms (Claude) + +**Accepted — the project's recurring failure, once more: an artifact describing a system +that no longer exists.** + +- The `WHAT THIS GUARANTEES` block credited `SendBuffer.busyUntil` and the deleted + per-terminal chain. Rewritten to the real split: `enforceFifo` decides *order*, + `submitToSession` provides *atomicity*. +- Its `NOT COVERED — an IMMEDIATE direct write sets no busyUntil` caveat was **true under + busyUntil and false under the lock** — the immediate path now takes the lock on the same + key. Removed, because a stale caveat that understates a guarantee invites a redundant + future guard (Claude's exact concern). +- The `@returns` doc crediting "the per-terminal chain" — gone with the return-type change. +- `delayed-send.ts`'s `generation` rationale, written entirely around the deleted `chains` + map — rewritten onto the submission lock. + +### Shutdown-drop promise was narrower than stated (Claude, non-blocking) + +**Accepted and corrected in the docs rather than the code.** A delivery already *writing* +when shutdown fires still completes — the lock does not interrupt a write in progress. So +"drops on shutdown" means "starts nothing new," not "aborts what is mid-flight." Both the +`generation` note and the `shutdownDelayedSends` doc now say so. No behaviour change: the +window is sub-second and the outcome (a fully-delivered message) is harmless; the fix is +telling the truth about the bound. + +--- + +## Verify-phase items — scheduled, not disputed (Codex) + +Codex is correct that these are absent. They are absent *by architect ruling* (2026-08-02, +modified option c), not by oversight, and each is disclosed at the PR gate rather than +discovered later — the explicit 1273 lesson applied forward. + +### The live e2e has not run + +**Correct, and deferred to the verify phase by design.** `/arch-save` is an +architect-session skill; its step 1 makes a *builder* refuse. The builder implementing this +feature therefore cannot run its own live cycle, and running it would clear a real +architect's context. The run is a throwaway-sibling-architect probe executed in verify +(spec Test Scenarios, "Why the live e2e has the shape it does"). Until then, `/clear` +execution, canary loss, identity recovery, monitor restoration and manual re-send remain +recorded as **unverified**, stated plainly in the review's Known Gaps. + +### The 15-second default is uncalibrated + +**Correct.** It is the value the proposing workspace uses in manual practice, and the skill +now says exactly that rather than implying it was measured. Calibration needs the live +run's send→session-ready-after-clear measurement, which is a verify-phase deliverable. The +skill flags it as a starting default pending that measurement. + +### The `codev/reviews/1307-*.md` artifact is absent + +**Being written now**, as the Review-phase deliverable, with the verify plan and the +unrun-e2e disclosure in it. It was not expected during implement phase_3. + +--- + +## Nothing disputed + +Every finding is either fixed (`ddf02abf`) or a correctly-identified verify-phase item +whose deferral the architect authorized and which the review discloses. No false positives. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter2-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter2-rebuttals.md new file mode 100644 index 000000000..9e07169a6 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter2-rebuttals.md @@ -0,0 +1,60 @@ +# Phase 3 — Rebuttals, iteration 2 + +Both `REQUEST_CHANGES`. **Both blocking findings accepted and fixed** in `27029541`; the +review file is written (`a0c1709b`). Nothing disputed — both reviewers were right, and both +findings were regressions I introduced adopting the submission lock. + +--- + +## 1. Delayed `--interrupt` wrote Ctrl+C outside the lock (both reviewers) + +**Accepted; a real regression from my `busyUntil` deletion, empirically reproduced by both.** + +`deliverOrBuffer` wrote the Ctrl+C *directly* — before the `submitToSession` reservation — +then awaited 100ms, then submitted the payload. That was safe under `busyUntil`, which kept a +mid-flush session "pending". With `busyUntil` gone and `hasPending` back to queue-only, a +delayed `--interrupt` due mid-flush put its Ctrl+C into the middle of the flush's stream, +split from its own payload (Claude: `ctrlC=50, lastClear=150, arch=152`; Codex: same site). + +**Fixed** by folding the whole delivery into one reservation: the Ctrl+C, its 100ms pause, +and the payload+Enter now run inside a single `submitToSession` thunk, mirroring +`deliverBufferedMessage`'s `interruptFirst`. An interrupt due mid-flush therefore queues +behind the flush's own reservation as a unit. This let me delete the pre-lock write, the +`await`, the `wroteInterrupt` flag, and the `queueAhead` re-check — machinery that existed +only to compensate for writing before the lock. New route test +`ORDERING: a delayed --interrupt due MID-FLUSH does not split into the flush`, and +**mutation-verified**: moving the Ctrl+C back outside the lock fails it. + +## 2. Generation checked before the lock, not at the write (Codex) + +**Accepted.** The timer-time generation check in `delayed-send.ts` fires before delivery +enters `submitToSession`. A delivery that then blocks on the lock behind an in-flight write +could have shutdown land in that wait and still write afterward — contradicting the "shutdown +starts nothing new" bound I had just written into the comments. + +**Fixed** by threading an `isStillLive()` predicate from `scheduleDelayedSend` through to the +write site, re-checked *inside* the reservation immediately before writing. Unit test added +for the timer-fired-but-lock-blocked case. The immediate path passes no predicate and is +unaffected. + +## 3. Review file absent (both, non-blocking) + +**Written** (`a0c1709b`): `codev/reviews/1307-arch-save-packaged-save-clear-.md`, with the +verify-phase live-run plan and the unrun-e2e disclosure in it, as required before the PR +gate. It was a Review-phase deliverable, not expected during implement, but both reviewers +were right that it must exist before the gate — so it does now. + +--- + +## The verify-phase items remain verify-phase items + +The live e2e and the 15s calibration are still unrun. That is by architect ruling (modified +option c, 2026-08-02), not oversight, and is now disclosed in the review's Known Gaps and +laid out in its Verify-phase plan. Not disputed — correctly identified, deliberately +deferred. + +## Nothing disputed + +Every finding is fixed or a correctly-identified verify-phase item. No false positives. Both +blocking items this round were mine — regressions from the lock adoption — and both are the +project's recurring shape: a guarantee stated in a comment before the code fully backed it. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter3-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter3-rebuttals.md new file mode 100644 index 000000000..694345db3 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter3-rebuttals.md @@ -0,0 +1,60 @@ +# Phase 3 — Rebuttals, iteration 3 + +Both `REQUEST_CHANGES`. **Both accepted and fixed** in `905bc9f4`. Nothing disputed — both +were real, both were regressions I introduced adopting the submission lock, and one was a +false claim in my own review. + +--- + +## 1. Shutdown-flush regression (Codex) + +**Accepted — I made an existing guarantee weaker and did not notice.** + +Before the lock adoption, `SendBuffer.stop()`'s final `flush(true)` at least *scheduled* its +writes synchronously. Routing the drain through `submitToSession` means a batch can now be +*queued behind an in-flight write* and not yet delivered when `stop()` returns — after which +graceful shutdown tears down terminals and exits, losing a buffered message that was accepted +for delivery. Plus the voided submission promise could surface as an unhandled rejection. + +**Fixed** by restoring what I broke: `SubmitFn` returns its promise; `flush(forceAll)` awaits +its submissions; `stop()` and `stopSendBuffer()` are async; `gracefulShutdown` awaits +`stopSendBuffer()` *before* terminal teardown. The injected submit `.catch()`es, covering the +unhandled-rejection note too. New `send-buffer.ts` test: `stop()` does not resolve until the +injected submission settles. + +This is in scope precisely because it is a *restoration*, not a new guarantee — I weakened +shutdown-flush by adopting the lock, so fixing it is finishing the adoption. + +## 2. Route-site `stillLive` guard untested — and I claimed it verified (Claude) + +**Accepted, and this is the project's own lesson committed one more time.** + +The iteration-2 `stillLive` guard at the production call site was untested: deleting it kept +the whole suite green. My unit test checked only `delayed-send.ts`'s predicate *return value* +via a synthetic callback — the replica-test pattern, the exact thing I have now hit five +times — not that `deliverOrBuffer` actually skips the write. Worse, I wrote "every +cancellation guard is mutation-verified" into the review, which was **false for this guard**. + +**Fixed** with a route-level test that occupies the session's submission lock, fires a delayed +send that queues behind it, calls `shutdownDelayedSends()` during the wait, and asserts the +message never reaches the session. **Mutation-verified**: disabling the guard fails it. The +review's overclaim is corrected in place, and I named it there as an instance of the very +failure mode the lessons section is about — because a review that hides its own gap is worse +than one that admits it. + +--- + +## The pattern, stated plainly + +Every blocking finding in phase 3's three review rounds was a regression I introduced while +adopting Spec 1273's lock, and each was the same shape: **a guarantee asserted (in a comment, +or a review, or a too-weak test) before the code fully backed it.** The lock adoption touched +the seam between the new delayed path and the existing buffer/shutdown machinery, and — as the +review's own lesson 1 predicts — that seam is where every defect lived. The mutation check is +what finally closed each one; I am now running it *before* claiming a fix, which is how the +last two were caught by me rather than by a third review round. + +## Nothing disputed + +No false positives. The live e2e and 15s calibration remain the architect-scheduled verify +items, disclosed in the review. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-plan-iter1-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-plan-iter1-rebuttals.md new file mode 100644 index 000000000..cb3b6d65c --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-plan-iter1-rebuttals.md @@ -0,0 +1,157 @@ +# Plan 1307 — Rebuttals, Plan iteration 1 + +Both reviewers returned `REQUEST_CHANGES` (both HIGH confidence). **All findings +accepted**; nothing defended. Both reviewed the *descoped* plan, not the earlier +seven-phase version. + +The headline: **the two reviewers independently found the same two defects**, and both are +outside the design's recoverability posture — that is, they are the failures a manual +re-send does *not* repair. Independent convergence on the same two items, out of ~14 total +findings, is the strongest signal either review produced. + +--- + +## The two that matter + +### P1. `SendBuffer` can invert `/clear` and `/arch-init` — both reviewers + +**Accepted; verified against source.** `/api/send` already defers messages when the user +is typing: `shouldDefer = !interrupt && !session.isUserIdle(3000)` +(`servers/tower-routes.ts:1570`), buffering for up to 60s (`send-buffer.ts:32-33`). +`isUserIdle` reads `_lastInputAt` (`terminal/pty-session.ts:554`) — *user input*, not +output. The `/arch-save` flow is precisely the case that trips it: the owner has just +typed a direction, so input is recent and the `/clear` is buffered. + +``` +T+0 /clear sent → user typing → BUFFERED (up to 60s) +T+15 /arch-init due → direct write → LANDS FIRST +T+40 buffer flushes → /clear lands → wipes the recovered context +``` + +This inverts the single ordering the whole feature promises. **Worse, it is outside the +recoverability posture**: the damage is a clear landing *after* recovery, so re-sending +`/arch-init` just re-runs the race. My plan said the design "schedules only the terminal +write" — which is exactly the bypass that causes this. + +**Changed**: a due message **re-enters the normal delivery path, buffering included**, so +per-session FIFO does the work and ordering stops depending on timing luck. Stated as the +phase's critical rule, with the inversion sequence written out, an acceptance criterion +that exercises it **with the buffer engaged**, and a risk row marking it +designed-out-not-accepted. + +### P2. `afx send ` is unspecified and can clear the wrong architect — both reviewers + +**Accepted; verified against source.** I wrote `` as a placeholder and never +resolved it. For a non-builder sender, bare `architect` resolves to `main` or the +first-registered architect (`servers/tower-messages.ts:371-372`). So a **sibling +architect** running `/arch-save` would clear **main's** terminal. + +This is the worst thing this feature could do: it destroys the context of a session whose +owner never invoked anything, and it is one word away from correct. + +**Changed**: the skill addresses `architect:` explicitly, with the reason stated in +both spec and plan, plus an acceptance criterion. The explicit form is safe for architect +senders — the spoofing check constrains builders, while architects have an open address +grammar. + +--- + +## Codex + +### X1. Phase 1 targets a re-export shim, not the implementation +**Accepted; verified.** `sendMessage` lives at `packages/core/src/tower-client.ts:655`; +`agent-farm/lib/tower-client.ts` only re-exports. **Changed**: core file named explicitly, +flagged as a cross-package change with core-first build ordering, and core-side test +coverage added to deliverables. + +### X2. Delayed-target lifecycle undefined +**Accepted.** **Changed**: retain the *authorised terminal id*, re-fetch that exact session +at delivery, re-check writability, drop gracefully if gone. Explicitly do not close over a +`PtySession` — a 15-second-old reference may point at a dead or replaced session. + +### X3. Shutdown wiring missing +**Accepted.** **Changed**: a delayed-send registry with a shutdown function wired into +`tower-server.ts`'s graceful-shutdown sequence. Codex's sharper point is that shutdown must +**drop** delayed sends rather than flush them — unlike `SendBuffer`, whose flush-on-shutdown +is right for messages already accepted for immediate delivery. A flushed delayed message +could land in a session that has moved on. Now an acceptance criterion. + +### X4. `--escape` composition is unsatisfiable +**Accepted.** My spec required composition with `--escape`; `afx send` has no such flag +(`cli.ts:450-454`) — interrupts are `afx interrupt`, and `escape` exists only as a +client/route option. **Changed**: recorded **N/A** in the spec rather than silently +dropped, and the real flag set (`--all`, `--file`, `--interrupt`, `--raw`, `--no-enter`) +enumerated with a decision for each. `--interrupt` needed a real decision: it currently +writes Ctrl+C at request time, so with `--delay` it must be deferred *with* the message. + +### X5. `adopt` coverage missing; `skill-parity.test.ts` exists +**Accepted; verified** (`adopt.test.ts:92`, `skill-parity.test.ts`). **Changed**: `adopt` +added to phase 2's deliverables, and the existing parity test acknowledged so it is not +duplicated. + +--- + +## Claude + +### C1. `arch-init`'s SKILL.md still documents the manual loop +**Accepted, and this one I would have shipped.** The four `arch-init` copies describe +save→suggest-`/clear`→human-clears in prose. Adding `/arch-save` without touching them +ships two contradictory procedures for the same task. **Changed**: updating the four +`arch-init` copies is now a phase-2 deliverable and acceptance criterion. + +### C2. The delay budget starts at the wrong moment +**Accepted, and it reframes the calibration.** The delay begins when the send is issued, +but `/clear` cannot execute until the architect's turn ends — and the turn runs as long as +the skill takes. The interval that matters is **send → session-ready-after-clear**, not +send → clear-sent. **Changed**: phase 3 measures that interval and says which one it is. A +default calibrated against the wrong interval looks right in testing and misfires whenever +a turn runs long. + +### C3. Line drift on the spoofing check; it only fires on `architect:` +**Accepted; verified.** The check is at `tower-messages.ts:225-234` (213-218 is the +signature). The operationally useful half: it fires on the `architect:` path — the +bare `architect` path has separate affinity logic — so the request-time authorisation test +must use `architect:` or it proves nothing. **Changed** in both the implementation +notes and the acceptance criterion. + +### C4. Say why `tower-cron.ts` is not reused +**Accepted.** `CronDeps.resolveTarget` takes no `sender`, so routing through it would drop +affinity and the spoofing check — a better reason than the tick interval I had given. +**Changed**: stated in phase 1 so reviewers do not re-litigate it. + +### C5. "All four copies identical" needs precision +**Accepted.** The skeleton trees carry a *subset* of skills (no `forge`/`team`/ +`skill-creator`), so the claim is parity for *this skill*, not tree parity. **Changed**, +with `skill-parity.test.ts` noted as already covering provider-tree byte parity. + +### C6. CLI should say "scheduled", not "sent"; surface `deferred` +**Accepted.** The route already returns a `deferred` flag that `commands/send.ts` +discards. **Changed**: both are phase-1 deliverables. Reporting "sent" for a message that +has not been sent is the kind of small dishonesty that costs someone a debugging session. + +--- + +## Summary + +| # | Finding | Source | Disposition | +|---|---|---|---| +| P1 | `SendBuffer` inverts clear/re-init ordering | Both | **Designed out** — FIFO re-entry | +| P2 | Bare `architect` clears the wrong terminal | Both | **Designed out** — `architect:` | +| X1 | Phase 1 targeted the re-export shim | Codex | Core file named; cross-package flagged | +| X2 | Delayed-target lifecycle undefined | Codex | Re-fetch by id; re-check writable | +| X3 | Shutdown wiring missing | Codex | Registry + shutdown; drops, not flushes | +| X4 | `--escape` composition unsatisfiable | Codex | Recorded N/A; real flag set decided | +| X5 | `adopt` coverage; parity test exists | Codex | Added; acknowledged | +| C1 | `arch-init` docs contradict `/arch-save` | Claude | Four copies updated | +| C2 | Delay budget measured from the wrong point | Claude | Phase 3 measures send→ready | +| C3 | Spoofing check line + `architect:`-only | Claude | Corrected; test uses that form | +| C4 | Say why not `tower-cron` | Claude | Stated (no `sender` in `resolveTarget`) | +| C5 | "Four copies identical" imprecise | Claude | Scoped to this skill | +| C6 | "scheduled" not "sent"; surface `deferred` | Claude | Both added | + +**What I take from this round.** The descope removed a great deal of machinery, and my +plan for the small design was correspondingly thin in the one place that still had real +risk: the interaction between a *new* delivery path and the *existing* one. Both defects +live in that seam. Making something smaller does not make it simpler to get right — it +concentrates the remaining risk into fewer places, and the review found both of them +sitting in the same seam. diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/1307-specify-iter1-rebuttals.md b/codev/projects/1307-arch-save-packaged-save-clear-/1307-specify-iter1-rebuttals.md new file mode 100644 index 000000000..fd64d558b --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/1307-specify-iter1-rebuttals.md @@ -0,0 +1,220 @@ +# Spec 1307 — Rebuttals, Specify iteration 1 + +Both reviewers returned `REQUEST_CHANGES`. **I accepted all fourteen findings.** There +are no disagreements to defend — but "accepted" is doing different work in different +places, so each entry says what actually changed and, where a finding invalidated +something I had asserted, says so plainly. + +Sequencing note: Codex's lane was down when Claude reviewed (vendored `@openai/codex-sdk` +binary rejected for `gpt-5.6-sol`; PR #1309 bumped it). Per architect ruling, Codex +reviewed the *revised* spec. That worked in the spec's favour — its findings are all +distinct from Claude's, and several are consequences of the redesign Claude prompted. + +--- + +## Claude (REQUEST_CHANGES, HIGH confidence) + +### C1. Post-clear "stop stale monitors" is unimplementable as written + +**Accepted — the criterion was wishful.** I required the *resumed* instance to stop +monitors that survived the clear, but issue comment 2 says these are harness background +tasks that `pgrep` cannot see, and the fresh context has no handles for them. I had +written an acceptance criterion and a test for something no one could implement. + +**Changed**: split into the enforceable half and the best-effort half. The **pre-clear** +architect stops its own monitors — it is the only party holding the handles — and the +skill sequences that before the state write. The resumed instance's obligation is +reconciliation: treat any alert it cannot account for from the state block as stale, and +disregard rather than act on it. Success criteria and Test 18 rewritten. Whether a +harness task-listing surface exists is now an open question that the spec deliberately +does *not* depend on. + +### C2. The `## Monitors` gate contradicts the template it validates + +**Accepted, and this one was self-inflicted twice over**: I mandated a machine-checked +`## Monitors` heading while simultaneously listing its placement as an *unresolved* open +question, against a template (v67) that carries the list as numbered lines inside a +`#`-comment intent stamp. The shipped validator would have rejected the shipped template. + +**Changed**: the gate is now a literal `MONITORS:` token that the template carries +verbatim, checkable without constraining the block's shape. Placement question closed +rather than left open under a mandate. + +### C3. No protection against a new turn between receipt and clear + +**Accepted.** Absent from risks, questions and tests. See Codex X2 — its follow-up showed +my first fix still overclaimed, and the final position is a *bounded window*, not a +guarantee. + +### C4. `--boundary` overclaimed as a recorded human decision + +**Accepted.** In the self path the agent types the flag; nothing about it establishes +human provenance. **Changed**: Security now states what it does and does not prove, and +the audit record captures **invocation mode** (self vs external) so a reader can tell +which kind of cycle they are looking at. + +### C5. The unrun 1273 e2e leaves quiescence unvalidated too, not just `/clear` + +**Accepted.** I had elevated the `/clear` question to Critical and missed that the same +unrun test leaves quiescence-against-a-live-TUI equally unknown. **Changed**: added as a +second Critical open question, with its distinguishing property called out — the failure +is *safe but total* (if an idle TUI repaints, every run aborts and the feature never +works). The live run is now scoped to both. + +### C6. Raw-injecting a slash command with an argument; `sendMessage` vs `sendRaw` + +**Accepted.** **Changed** twice, and the second change matters: I first adopted +plain-text injection as settled. The owner then directed that the delivery mechanism be +carried as an **explicitly open decision** — correctly, since I had settled it twice in +opposite directions on reasoning alone. It is now a named decision with three candidates +(raw-typed, plain-text, 1273's file+inline shape), to be resolved empirically against a +real terminal with the reason recorded. The channel-distinctness constraint survives +independently. + +### C7. Write-then-verify was not considered + +**Accepted, and it changed the recommendation.** This was the most valuable finding in +either review. Having the architect write the state file *before* invoking the CLI lets +the CLI validate synchronously and arm only `quiesce → clear → reorient`. It removes +receipt polling from Tower, makes "no clear without a verified save" true **by +construction** in the self path, and shrinks the post-save-work window from minutes to a +quiet window. Now Approach 1; the original nonce/Tower-armed design is retained as +Approach 1b with its rejection reasons rather than deleted. + +**Self-caught consequence**: write-then-verify breaks the state-file snapshot, since the +CLI no longer runs before the overwrite. Flagged as its own risk — and Codex then showed +my first fix for it was inadequate (X3). + +### C8. Scope note (not a defect) + +**Accepted as guidance.** Added a Notes paragraph telling the plan to phase this honestly +rather than compress it. + +--- + +## Codex (REQUEST_CHANGES, HIGH confidence) + +Two of Codex's findings were factual claims about the codebase. I verified both against +the source before acting, per the standing lesson that reviewer claims are evidence and +not ground truth. **Both were correct, and both invalidated a premise of mine.** + +### X1. The Tower scheduling premise is wrong — VERIFIED + +**Accepted.** I claimed the armed job could ride "an existing Tower tick." Checked +`packages/codev/src/agent-farm/servers/tower-cron.ts:70`: the interval is **60 seconds**, +over filesystem-backed cron definitions. It is not a generic job runner, and 60s is two +orders of magnitude too coarse to observe a 1.5s quiet window. + +**Changed**: the clear-job runs its own bounded poll loop started at arm time, at the +reset poll interval; Performance's resource model corrected; the erroneous claim +explicitly retracted in the spec text so the next reader does not re-derive it. + +### X2. The post-save-work guarantee is not implementable from `lastDataAt` — VERIFIED + +**Accepted, and this is the most important correction in the round.** Checked +`packages/codev/src/terminal/shellper-client.ts`: `lastDataAt` is a last-output +timestamp. Tower exposes no turn identifier, no input-generation counter, no handoff +token. Therefore "the original turn ended" and "a follow-up turn ended" are +**observationally identical**, and my criterion — "the clear can never destroy work +created after the verified save" — could not be implemented or tested. Notably this +survived *my own* fix for C3: I closed the hazard with a mechanism that cannot observe +what it needs to observe. + +**Changed**: downgraded from guarantee to **bounded window**, stated as such. What +remains enforceable: fire on the first quiescence transition after arming, cap the armed +lifetime, and refuse if the terminal's output total has grown beyond tolerance since +arming — the last being an explicit *heuristic* (it catches a full follow-up turn, not a +one-line exchange) and labelled as one everywhere it appears. Adding a proper Tower +observable is raised as an open question rather than quietly pulled into scope. + +### X3. The self-path snapshot is not machine-gated + +**Accepted.** The skill took the snapshot before the CLI started, so nothing verified it +existed or predated the new file — while Security claimed a clear was unreachable without +it. A guarantee resting on a convention. + +**Changed — this produced a real design improvement.** Introduced a `--begin` / +`--boundary` handshake: `--begin` takes the snapshot under machine control and issues a +one-time token; `--boundary` requires the state file to carry it. Missing or stale token +is refused. This closes the snapshot hole **and** restores a machine-proven freshness +token to the self path — which the previous draft had traded away on the argument that +self-attestation was equivalent. It was not, precisely because it left the snapshot +ordering unproven. + +### X4. Cancellation, status and dropped-job reporting have no specified surface + +**Accepted, including the contradiction underneath it**: I required that a job dropped by +a Tower restart be "reported rather than silent" while also specifying purely in-memory +jobs. A purely in-memory job that dies with Tower leaves nothing to report. + +**Changed**: split execution from intent. The **running job** stays in memory, preserving +the fail-safe restart property (a dropped job can never clear). A small **durable intent +record** is written at arm time and removed on completion or cancellation, so a leftover +record is unambiguous evidence of an unfinished cycle. Status and cancel are specified as +user-visible surfaces; tests 15e/15f added. + +### X5. The self-invocation flow contradicts itself + +**Accepted.** Test 2 still described the CLI returning "the nonce and instructions" +*before* the write — a leftover from the superseded design that survived the redesign +because I revised the prose and did not re-read the tests against it. + +**Changed**: Test 2 rewritten to the `--begin` → write → `--boundary` sequence, with a +parenthetical recording what it used to say and why that was wrong. Tests 4 and 5 scoped +to the external path, since the self path has no receipt wait. Tests 2a/2b added for the +missing- and stale-token cases. + +### X6. Compaction validation needs exact rules + +**Accepted.** "Growth comparison" and "one-screen order of magnitude" are not testable +boundaries. + +**Changed**: exact predicate — reject if the `--begin` snapshot survives in the new file +as an **unmodified leading section** (trailing whitespace normalised). Genuine compaction +always edits content above the new entry, so a byte-identical prefix is precisely the +append-only signature. Chosen over a size ratio deliberately: it admits the +compact-and-grow case (old material collapsed to pointers, substantial new material +added, net larger) that a ratio rule would wrongly reject — now Test 15c. Behaviour with +no predecessor defined: check skipped, not failed (Test 15d), or no architect could ever +write a first save. Size ceiling kept as an independent bound, with its value an open +question to be derived from real state files rather than guessed. + +### X7. Failure guarantees are overstated + +**Accepted.** "Every gate that fails … leaves … a saved state file" is false for +missing-boundary, invalid-name, Tower-down, missing-file, and external receipt-timeout +failures — in several of those, the save is exactly what did not happen. + +**Changed**: split into preflight failures (nothing touched; no fresh state file implied) +and post-verification aborts (context intact **and** a verified state file on disk), with +the universally-true guarantee stated narrowly: **no failure path clears context.** + +--- + +## Summary of changes + +| Finding | Disposition | Substance of the change | +|---|---|---| +| C1 monitor enumeration | Accepted | Pre-clear stop enforceable; post-clear best-effort | +| C2 `## Monitors` contradiction | Accepted | `MONITORS:` token; open question closed | +| C3 turn-after-save hazard | Accepted | Added; then corrected by X2 | +| C4 `--boundary` overclaim | Accepted | Records invocation mode; states the limit | +| C5 quiescence unvalidated | Accepted | Second Critical question; safe-but-total | +| C6 slash-command delivery | Accepted | Now an explicitly open decision, 3 candidates | +| C7 write-then-verify | Accepted | **Changed the recommended approach** | +| C8 scope note | Accepted | Phasing guidance for the plan | +| X1 Tower tick (verified) | Accepted | Own bounded loop; wrong claim retracted | +| X2 turn observability (verified) | Accepted | **Guarantee → bounded window** | +| X3 snapshot not gated | Accepted | **`--begin`/`--boundary` handshake** | +| X4 status/cancel/reporting | Accepted | In-memory execution + durable intent record | +| X5 flow contradiction | Accepted | Test 2 rewritten; tests scoped by path | +| X6 compaction rules | Accepted | Exact prefix predicate; no-predecessor case | +| X7 overstated guarantees | Accepted | Preflight vs post-verification split | + +Also incorporated this round, from the owner via the architect: **pruning is a +requirement, not guidance** (a save that only appends fails — X6's predicate is how that +is enforced), and **the re-orientation delivery mechanism is explicitly undecided** +(C6's final disposition). + +Commits: `4150edb7` (Claude round), `1f11f794` (delivery-mechanism iteration), +`de043dfd` (owner directives), `93bd2a9d` (Codex round). diff --git a/codev/projects/1307-arch-save-packaged-save-clear-/status.yaml b/codev/projects/1307-arch-save-packaged-save-clear-/status.yaml new file mode 100644 index 000000000..d1f48d065 --- /dev/null +++ b/codev/projects/1307-arch-save-packaged-save-clear-/status.yaml @@ -0,0 +1,122 @@ +id: '1307' +title: arch-save-packaged-save-clear- +protocol: aspir +phase: review +plan_phases: + - id: phase_1 + title: afx send --delay (Tower-side deferred delivery) + status: complete + - id: phase_2 + title: /arch-save skill in four trees + state-block template + status: complete + - id: phase_3 + title: Live end-to-end run and documentation + status: complete +current_plan_phase: null +gates: + pr: + status: pending + requested_at: '2026-08-02T05:46:32.100Z' + verify-approval: + status: pending +iteration: 1 +build_complete: true +history: + - iteration: 1 + plan_phase: phase_1 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter1-codex.txt + - model: claude + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter1-claude.txt + - iteration: 2 + plan_phase: phase_1 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter2-codex.txt + - model: claude + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter2-claude.txt + - iteration: 3 + plan_phase: phase_1 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter3-codex.txt + - model: claude + verdict: APPROVE + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_1-iter3-claude.txt + - iteration: 1 + plan_phase: phase_2 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_2-iter1-codex.txt + - model: claude + verdict: COMMENT + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_2-iter1-claude.txt + - iteration: 1 + plan_phase: phase_3 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter1-codex.txt + - model: claude + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter1-claude.txt + - iteration: 2 + plan_phase: phase_3 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter2-codex.txt + - model: claude + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter2-claude.txt + - iteration: 3 + plan_phase: phase_3 + build_output: '' + reviews: + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter3-codex.txt + - model: claude + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/aspir-1307/codev/projects/1307-arch-save-packaged-save-clear-/1307-phase_3-iter3-claude.txt +started_at: '2026-07-31T21:42:49.601Z' +updated_at: '2026-08-02T05:46:32.100Z' +force_advanced: + phase: phase_3 + iteration: 3 + max_iterations: 3 + rebuttal_file: 1307-phase_3-iter3-rebuttals.md + at: '2026-08-02T05:03:27.880Z' +pr_history: + - phase: review + pr_number: 1335 + branch: builder/aspir-1307 + created_at: '2026-08-02T05:22:40.839Z' +pr_ready_for_human: true diff --git a/codev/resources/commands/agent-farm.md b/codev/resources/commands/agent-farm.md index 48a5c5c3e..cf1ef017d 100644 --- a/codev/resources/commands/agent-farm.md +++ b/codev/resources/commands/agent-farm.md @@ -505,6 +505,42 @@ afx send [builder] [message] [options] - `--interrupt` - Send Ctrl+C first - `--raw` - Skip structured message formatting - `--no-enter` - Do not send Enter after message +- `--delay ` - Deliver after N seconds instead of immediately (Spec 1307) + +**Delayed delivery (`--delay`):** + +Tower holds the message and delivers it after the stated delay, so the sending process is +free to exit in the meantime. That is the point: a session can schedule a message to +*itself* for after something that destroys it — which is what `/arch-save` uses to send +`/arch-init` after a `/clear`. + +- **Authorised at request time, delivered later.** Target resolution and the + builder-spoofing check run when the command is issued, exactly as for an immediate send. + A delayed send cannot defer a check past the conditions that would fail it. +- **Bounds:** a whole number of seconds, 1–3600. Rejected at the CLI *and* server + boundaries, because a bad value silently changes *when* (or whether) the message arrives + rather than failing loudly. +- **Not persisted.** A pending message is a Tower-side timer. A Tower restart drops it, by + design — a delayed message's timing was chosen against a world the restart has already + invalidated, so delivering it late could be worse than not delivering it. Re-send by hand + if it matters. +- **Ordering:** a delayed message never overtakes one already queued for that session + (including one held by the typing-aware send buffer), and concurrent deliveries to one + session do not interleave. Request order across *differing* delays is **not** preserved — + `--delay 30` followed by `--delay 5` delivers the 5-second one first, because that is + what `--delay` means. +- **Reporting:** the CLI says "scheduled", not "sent". A message Tower is merely holding + has not been delivered, and saying otherwise costs someone a debugging session. +- **Not combinable with the API's `escape` option** — an ESC bypasses buffering precisely + so that it interrupts the *current* turn, which a delay contradicts. Refused rather than + silently dropping one of the two. (`afx send` has no `--escape` flag; use `afx interrupt`.) +- `--interrupt` **is** combinable: the Ctrl+C is deferred *with* the message rather than + firing immediately. + +```bash +# Deliver in 15 seconds; this shell can exit immediately +afx send architect:main --delay 15 --raw '/arch-init main' +``` **Description:** diff --git a/codev/reviews/1307-arch-save-packaged-save-clear-.md b/codev/reviews/1307-arch-save-packaged-save-clear-.md new file mode 100644 index 000000000..18c12c693 --- /dev/null +++ b/codev/reviews/1307-arch-save-packaged-save-clear-.md @@ -0,0 +1,177 @@ +# Review: `/arch-save` — packaged save→clear→re-init for architects (Spec 1307) + +## Summary + +`/arch-save` packages the manual architect-refresh recipe — save state → clear → re-init — +into a single skill, backed by one new primitive: `afx send --delay`, a Tower-side deferred +send. The skill sequences: stop your own monitors → write a pruned state file → `--raw +'/clear'` → `--delay 15 --raw '/arch-init '` → stop. Tower holds the last message past +the clear that would otherwise have destroyed the sender, and `/arch-init` recovers from the +state file. + +**What this project is not, by the time it shipped:** the Tower-owned job orchestrator, +`--begin`/`--boundary` handshake, verification gates and bounded-window machinery that the +first two CMAP rounds hardened. The owner descoped all of it. The feature is one send +parameter and a document, and the descope is the single most important design decision in +the record — see Lessons. + +## What shipped + +- **`afx send --delay `** (Tower-side deferred delivery): authorised at request + time, delivered later, so the sending process can exit — the capability that makes the + cycle's third leg possible from inside a session about to be cleared. Bounds 1–3600s, + validated at CLI and server; not persisted (a restart drops pending sends, by design); + reports "scheduled", not "sent". +- **`/arch-save` skill** in all four trees (`.claude`, `.codex`, and both `codev-skeleton` + mirrors), guarded against drift and content-regression by + `spec-1307-arch-save-skill.test.ts`. +- **`/arch-init` updated** so it no longer documents a competing manual-only loop; the + manual path remains as the Tower-unavailable fallback. +- **Adoption of Spec 1273's submission lock** (`submitToSession`): every write from + `deliverOrBuffer` — immediate and delayed — goes through it, so a message is *submitted* + (Enter included) before the next write to that session begins. This project's three + narrower mechanisms (`writeCompletesInMs` wait, `SendBuffer.busyUntil`, the per-terminal + chain in `delayed-send.ts`) were deleted in favour of it. One mechanism, not two. +- **Ordering guarantees**, tested at the route level and mutation-verified: a delayed + message never overtakes one already queued for a session; concurrent deliveries do not + interleave; a delayed message (including a `--interrupt`) due mid-flush queues behind the + flush rather than writing into it. Request-order across *differing* delays is explicitly + not guaranteed. + +## Architecture Updates + +Nothing in `arch.md`/`arch-critical.md` needs changing: no new subsystem, no new invariant. +`--delay` is a parameter on the existing send pipeline; `/arch-save` is a skill resolved +through the existing four-tier chain. The one cross-cutting fact worth carrying forward — +that submission atomicity is now a shared Tower primitive (`submitToSession`) rather than +per-caller — belongs to Spec 1273's review, which owns the primitive. + +## Lessons Learned Updates + +1. **Descoping concentrates risk into the seams; review the seams hardest.** The feature + shrank from a job orchestrator to a send flag, and every genuine defect across eight + review rounds lived where the *new* delivery path met the *existing* one — `SendBuffer`, + the paced-write window, the submission lock's boundaries. Smaller did not mean simpler to + get right; it meant the remaining risk pooled at the integration points. (Pairs with + 1273's own lesson: proportionate machinery.) + +2. **An artifact can assert something adjacent to the truth, and pass self-review because + it exists.** This recurred in six materials this project: a test asserting against a + copied predicate; a test against a replica helper; a test against a synthetic callback; a + test whose *timing* missed the window it was named for; a shutdown-flush test given too + few ticks to actually exercise the wait; and a *spec* claiming a request-order guarantee + the code did not make. Plus comments — and one review claim — crediting a guarantee the + code did not yet back. The cheap check that catches all of them: **mutate the guard, + confirm the test fails.** The lesson is not that I learned it once; it is that I had to + apply it repeatedly, and the times it caught the defect before a reviewer did were the + times I ran it *before* claiming the fix rather than after. + +3. **A stale CI green is the same failure one level up.** A merge landed on main whose + July-6 green predated the parity guards the repo had since grown — true when produced, + false when used. The standing rule that came out of it (re-validate a stale green against + current main's guards before merge) is the CI-level version of the mutation check. + +4. **Verify a reviewer's factual claim against source before acting on it.** Codex made + several claims about the codebase (`tower-cron`'s tick, `lastDataAt`'s semantics, the + `session-submit` API); checking each before acting confirmed them fast enough to act with + confidence, and separately let me catch a *sibling's* false datum (`_lastInputAt` bumped + by Tower's own writes) before either of us built on it. + +## Deviations from the plan + +- The spec's Approach 2 (Tower-owned job) was rejected wholesale by owner directive before + implementation; the plan was rewritten to match. Recorded in the spec's Notes. +- `--delay` documentation was removed from `CLAUDE.md`/`AGENTS.md` and placed only in the + command reference, per architect ruling — Spec 1280's Phase 1 restructured `CLAUDE.md` so + per-flag CLI detail no longer belongs in the always-on surface. The spec's byte-identical + criterion was amended in place with a dated supersession note. +- The `afx` skill does **not** gain `--delay`; that drift is #1318's to reconcile, per the + same ruling Spec 1273 received. + +## Known gaps + +- **The live end-to-end run has not happened.** It is scheduled for the verify phase (see + below), and this is disclosed here rather than discovered later — the explicit lesson from + 1273, which shipped a non-functional `/clear` because no one ran the headline path. + Unverified until the verify run: that `/clear` actually *executes* (not merely arrives), + canary loss, identity recovery from the state file, monitor reconciliation, and the manual + re-send recovery path. +- **The 15-second default is uncalibrated** — it is the value the proposing workspace uses in + manual practice, not one measured against the send→session-ready-after-clear interval. The + verify run calibrates it. + +## Verify-phase plan (the live e2e) + +`/arch-save` is an architect-session skill: its step 1 makes a *builder* refuse, so the +builder that implemented this feature cannot run its own live cycle, and running it clears a +real architect's context. The run is therefore an architect action in verify, shaped exactly +as Spec 1273's successful probe retest: + +1. After merge, the next batched install lands (`submitToSession` and this project's code in + one running Tower). +2. Architect creates a throwaway sibling: `afx workspace add-architect --name probe-1307` + (architect-only, from the main root). +3. Plant a canary — a distinctive fact — in the sibling's context. +4. The sibling invokes `/arch-save`. Verify, in order: the state file was written and pruned; + `/clear` *executed* (harness clear announcement; canary gone; `/clear` not welded to the + front of another message); `/arch-init` arrived and recovered identity from the state + file; monitors reconciled. +5. Exercise the recovery path deliberately: drop the delayed `/arch-init`, re-send it by + hand, confirm recovery. +6. Set the documented default delay from the measured send→session-ready interval. +7. `afx workspace remove-architect probe-1307`. + +The runbook with the exact checks is in the plan's phase 3. + +## Per-phase review history (including phase_3's force-advance) + +Stated explicitly so the gate reader needs no `status.yaml` archaeology. + +| Phase | Rounds | Outcome | +|---|---|---| +| phase_1 (`--delay`) | 8 iterations | Clean: iter-3 recorded double-review resolution; six of the eight found real defects | +| phase_2 (skill) | 2 iterations | Clean double-approve | +| phase_3 (adoption + docs) | 3 iterations **+ confirming round** | **Force-advanced at porch's 3-iteration cap** | + +**phase_3 did not reach a clean double-APPROVE within the cap.** Its three iterations each +returned `REQUEST_CHANGES` from both lanes; every finding was a real regression introduced +adopting Spec 1273's submission lock, each fixed with mutation-verification, but porch's +`max_iterations: 3` was reached before a fourth consult could confirm the iter-3 fixes. +Porch force-advanced (recorded in `status.yaml` as `force_advanced`), which is its designed +behaviour at the cap — it hands adjudication to the human gate rather than looping. + +Because force-advance is not approval, a **confirming review round** was run after the cap +(architect ruling, 2026-08-02): both lanes returned `REQUEST_CHANGES` on the iter-3 state — +a vacuous shutdown-flush test and a shutdown-ordering race — which were treated as a real +fourth iteration, fixed (`await`-drain of all in-flight submissions; `shutdownDelayedSends` +ordered before the buffer flush; both fixes mutation-verified), and re-confirmed clean +before this PR was prepared. The full round-by-round record and rebuttals are in +`codev/projects/1307-arch-save-packaged-save-clear-/`. + +The honest read: the lock adoption was a leaky seam and took more than three rounds to +settle. Nothing here shipped on the strength of a force-advance alone. + +## Flaky Tests + +None introduced. One self-inflicted test-isolation issue found and fixed: delayed-send tests +sharing a session id poisoned each other once `submitToSession` serialised per session (a +chain abandoned under fake timers never drains). Each test now uses its own id — correct +hygiene, and reported to 1273 as a note about their primitive (benign in production, where +writes complete). + +## Testing + +- Full suite green: 4187 passed, 0 failed, 48 skipped. +- New coverage: `spec-1307-send-delay.test.ts` (validation, scheduling, shutdown-drop, + shutdown-during-lock-wait, FIFO), `spec-1307-arch-save-skill.test.ts` (four-tree drift + + content), and route-level `ORDERING:` tests (buffered-inversion, two-simultaneous-delayed, + mid-flush, mid-flush-interrupt), plus core-side `tower-client-send.test.ts` for the wire + contract. +- Every ordering and cancellation guard is mutation-verified: the fix is confirmed to be the + thing the test depends on, not incidental. This includes the route-site `stillLive` + cancellation guard, which an earlier draft of this review claimed was mutation-verified + when it was not — the test then covered only the predicate's return value, not that the + write was skipped. That gap (Claude, phase-3 iter 3) is now closed by a route-level test + that drives `deliverOrBuffer` with a shutdown landing during the `submitToSession` wait, + and deleting the guard fails it. Recorded because claiming a check that did not exist is + precisely the failure mode lesson 2 is about. diff --git a/codev/specs/1307-arch-save-packaged-save-clear-.md b/codev/specs/1307-arch-save-packaged-save-clear-.md new file mode 100644 index 000000000..4f2618a30 --- /dev/null +++ b/codev/specs/1307-arch-save-packaged-save-clear-.md @@ -0,0 +1,543 @@ +# Specification: `/arch-save` — packaged save→clear→re-init for architect context refresh + +## Metadata +- **ID**: spec-2026-07-31-arch-save +- **Status**: draft (rewritten 2026-07-31 to a descoped target shape — see Notes) +- **Created**: 2026-07-31 + +## Clarifying Questions Asked + +Strict-mode ASPIR against a fully-specified issue (#1307), so no clarifying round was +needed. The questions a spec author would have asked were answered by the issue, its two +comments, and two rounds of owner direction. Recorded as pairs so the reasoning is +auditable. + +**Q: What is the actual mechanism?** +A (owner, descope directive): one small extension — `afx send --delay `, +delivered Tower-side — plus a skill that sequences three steps. Tower already mediates +delivery, so a delayed send is one parameter on an existing path. Not a client process +that sleeps; not a job orchestrator. + +**Q: Why is a delay sufficient, when the clear's timing is not precisely observable?** +A: because the failure is cheap. The state file survives the clear, the terminal stays +alive, and re-sending `/arch-init ` by hand recovers everything. A mistimed +re-orientation costs one manual message. That is the whole reason heuristics suffice here +and guarantee-machinery is not worth its weight. + +**Q: Is the monitor list a re-arm list or a kill-list?** +A (issue comment 2, from the live run): **both.** Monitors are session-bound, *not* +context-bound — a watcher armed pre-clear **survives** `/clear` and fired a stale alert 8 +minutes into a fresh context, against a target decommissioned before the clear. `pgrep` +cannot see them; they are harness background tasks. The pre-clear architect stops them +(it holds the handles); the state block lists them so the resumed instance can recognise +a stale alert and re-arm deliberately. + +**Q: Who pulls the trigger?** +A (issue design note 2): the human decision is *relocated*, not removed — from "press +`/clear`" to "invoke `/arch-save`". Either the owner runs it or the architect runs it **on +the owner's direction**, with the standard override carve-out ("don't autonomously X"). + +**Q: Must the save prune?** +A (owner directive): yes, as a requirement. The write must remove cruft, not merely +append. + +## Problem Statement + +Long architect sessions accumulate stale context. The cure exists and is proven — the +proposing workspace runs it by hand today — but it is unpackaged: three manual steps a +human has to remember and sequence, with one step that cannot be done from inside the +session that needs it. + +`/arch-init`'s skill doc already describes the loop as prose: + +``` +/arch-init (recover) → work → save at a checkpoint → suggest /clear → human /clears → /arch-init → … +``` + +Two things make this worse than it looks. **Ordering**: the state write must happen +strictly before the clear, or the context that knew what to write is already gone. +**Monitors**: session-bound watchers survive the clear and fire into a context that cannot +evaluate their alerts. + +And one structural gap: an architect told "go ahead and refresh" cannot complete the +cycle, because the clear destroys the very context that would have sent `/arch-init` +afterwards. Something outside the session has to deliver that last message. + +## Current State + +**The manual recipe**, from `/arch-init`'s SKILL.md: the architect judges it has reached a +resumable boundary, rewrites its current-state section, appends a dated log entry, +compacts, then advises the human to `/clear`; the human clears and types `/arch-init +`. + +**What exists in code:** +- `.claude/skills/arch-init/` and `.codex/skills/arch-init/` (plus both skeleton mirrors) + — identity resolution, state-file read, save discipline, `/clear` suggestion rule. +- `afx send` with `--raw` (types literal text into a PTY) and `--escape`. Tower mediates + every send: `servers/tower-messages.ts` resolves the target, + `servers/message-write.ts` writes to the session. +- `afx whoami` — architect identity from `CODEV_ARCHITECT_NAME`, failing loud rather than + defaulting to `main` (#1094). +- `codev/state/*.md` gitignored (`.gitignore:15`), `*_thread.md` re-included (line 16). + +**The limitation that blocks packaging**: `afx send` delivers immediately. There is no way +to say "deliver this after the clear has landed," so the third leg of the cycle has no +mechanism — which is exactly why it is still a human keystroke today. + +## Desired State + +**One new capability**: `afx send --delay `, held and delivered by Tower. + +**One new skill**, `/arch-save`, whose entire procedure is: + +1. **Stop your own monitors** — the pre-clear context is the only one holding the handles. +2. **Write the pruned state file** to `codev/state/.md`: rewrite current state in + place, append one dated entry, **and compact** — resolved loops deleted, older entries + collapsed into pointers at durable artifacts, one-screen order of magnitude. +3. `afx send architect: --raw '/clear'` +4. `afx send architect: --delay 15 --raw '/arch-init '` + +**The address must be `architect:`, never bare `architect`.** For a non-builder +sender the bare form resolves to `main`, or to the first registered architect +(`servers/tower-messages.ts:371-372`) — so a *sibling* architect running `/arch-save` +would clear **main's** terminal instead of its own. Clearing the wrong architect's context +is the single worst outcome this feature could produce, and it is one word away from the +correct behaviour. The explicit form is safe for architect senders: the spoofing check +constrains builders, while architects have an open address grammar. + +That is the whole feature. Tower holds the fourth message while the clear takes effect, +then delivers it into the fresh session, which re-adopts its identity and resumes from the +state file. + +**Why this is enough.** The expensive failure would be clearing without a good save — and +that is prevented by ordering the skill's own steps, since step 2 precedes step 3. Every +*other* failure is cheap: the state file is on disk, the terminal is alive, and a human +re-sends one message. The design buys ordering where it matters and accepts recoverable +imprecision everywhere else. + +## Stakeholders + +- **Primary Users**: architect agents and the owners who direct them. The proposing + workspace runs this cycle manually today and is the first consumer. +- **Secondary Users**: builders — a refreshed architect gives clearer direction, and a + phantom monitor firing into a stale context produces spurious messages to them. +- **Technical Team**: codev maintainers. Lands in `packages/codev` (one send parameter) + and four skill trees. +- **Business Owners**: the codev project owner, who approves at the PR gate. + +## Success Criteria + +- [ ] `afx send --delay --raw ''` delivers the message after the + stated delay, Tower-side, with the sender's process free to exit immediately. +- [ ] `--delay` composes with existing send flags (`--raw`, `--file`, `--no-enter`, + `--all`, `--interrupt`) and with every addressing form, without changing undelayed + behaviour. (`--escape` is **N/A**: `afx send` has no such flag — interrupts are + `afx interrupt`, and `escape` exists only as a client/route option.) +- [ ] **A delayed message never overtakes a message already QUEUED for that session** — + including one held by the existing typing-aware send buffer. This is the ordering + the whole feature depends on: if `/arch-init` overtakes `/clear`, the clear wipes + the re-orientation that already landed. In `/arch-save` the `/clear` is sent with + no delay and the `/arch-init` with one, so this is exactly the case that matters. +- [ ] **Concurrent deliveries to one session do not interleave.** Two delayed messages + coming due together are written one after the other, waiting out each other's + paced writes — not just each other's scheduling. +- [ ] **Deliberately NOT guaranteed: request-order across differing delays.** + `--delay 30` followed by `--delay 5` delivers the 5-second one first, because that + is what the caller asked for. `--delay N` is a statement about *when* to deliver; + forcing request-order would make the flag silently not mean what it says. This + exclusion is stated explicitly because an earlier draft of this criterion said + "never overtakes an earlier message," which reads as request-order FIFO and + contradicted the implementation — a review caught the disagreement between the two + artifacts. The narrow guarantee above is the one the feature needs and the one it + makes. +- [ ] `/arch-save` addresses its own terminal as `architect:`, never bare + `architect`, so a sibling architect cannot clear main's session. +- [ ] Invalid delays (zero, negative, non-integer, NaN, absurdly large) are rejected at the + CLI boundary. +- [ ] `/arch-save` ships as a skill in all four trees (`.claude/skills/`, `.codex/skills/`, + and both `codev-skeleton/` mirrors), picked up by `codev init`/`adopt`/`update`, and + covered by the same scaffolding tests as `arch-init`. +- [ ] The skill's procedure is ordered **write-then-clear**, and says why that ordering is + load-bearing. +- [ ] **The save prunes.** The skill requires resolved loops deleted, older entries + collapsed to pointers at durable artifacts, and a one-screen order of magnitude. A + save that only appends does not satisfy the skill's own instructions. +- [ ] The state-block template documents the seven elements the live run validated, and + documents the monitor list as both a pre-clear kill-list and a post-clear re-arm + list. +- [ ] The skill states the owner-direction rule with a standard override carve-out. +- [ ] A real architect completes save → clear → resume end-to-end in a live workspace. +- [ ] `--delay` is documented in the command reference — `codev/resources/commands/agent-farm.md` + **and its `codev-skeleton/` mirror**. `CLAUDE.md` and `AGENTS.md` remain byte-identical + and gain **no** `--delay` content. + + **AMENDED 2026-08-01, per architect authorization.** As originally written this + criterion required a `--delay` note *in* `CLAUDE.md`/`AGENTS.md`. It was authored + against the pre-rewrite world. Spec 1280's Phase 1 has since restructured `CLAUDE.md` + so CLI detail lives in skills and reference docs — its Tooling section now says + "check the skill, don't guess" and carries no per-flag content. Under that + architecture a per-flag pointer in `CLAUDE.md` is a regression to the pattern 1280 + just deleted, so the detail is relocated to `agent-farm.md` and the always-on surface + gains nothing. Recorded rather than silently changed because it moves a success + criterion. +- [ ] Tests pass with >90% coverage of the new delivery path. +- [ ] Documentation updated. + +## Constraints + +### Technical Constraints + +- **A delayed message must not overtake an earlier one to the same session.** Tower + already holds messages for reasons of its own: `SendBuffer` (Spec 403) defers delivery + while the user is typing — `shouldDefer = !interrupt && !session.isUserIdle(3000)` + (`servers/tower-routes.ts:1570`) — for up to 60 seconds. So a `/clear` sent while + someone is at the keyboard can sit buffered while the `/arch-init` timer expires behind + it. If the delayed write bypassed the buffer, `/arch-init` would land **first** and the + `/clear` would then destroy the freshly-recovered context. The rule that prevents this: + a due message **re-enters the normal delivery path**, buffering included, rather than + writing directly to the session. Ordering then follows from the existing per-session + FIFO rather than from timing luck. +- **Submission atomicity is Spec 1273's per-session submission lock, adopted unchanged.** + Architect ruling, 2026-08-01. Ordering and atomicity are separate layers: this spec's + FIFO guarantee decides *which message goes first*, the lock guarantees *each one is + submitted alone*. `writeMessageToSession` schedules its Enter via `setTimeout` + (`message-write.ts:16-19`) and `/api/send` returns once the write is scheduled, so two + correctly-ordered sends can still merge into a single user turn. Spec 1273 hit this in + production — its `/clear` arrived as literal text on the front of the next message and + never executed. `--delay 15` keeps this skill's two sends far outside that window, but + that is a property of the delay rather than of the send path, and this spec must not + grow a second mechanism to cover it. +- **`--delay` is Tower-side, not client-side.** The sending process must be free to exit — + in the self-invoked case it is a Bash call inside the very session about to be cleared. + A client that sleeps would die with the clear, which is the failure the whole design + avoids. Tower already mediates delivery, so this is one parameter on an existing path. +- **`/clear` must travel over `--raw`, never `--escape`.** Tower's escape route + (`servers/message-write.ts:writeEscapeToSession`) writes a hardcoded ESC and **discards + the message body**, so a `/clear` sent as an escape delivers a bare interrupt: the + command appears to succeed and nothing is cleared. +- **Architect names are path components.** `codev/state/.md` is built from the name, + so `/arch-init`'s existing rule applies: `[a-z][a-z0-9-]*`, ≤64 characters, validated + before any path is constructed. +- **State files are gitignored**, so pruned prose is unrecoverable. Compaction must + proceed by *replacing detail with pointers*, never by deleting the only record of + something — the rule `/arch-init` already states. +- **Both provider trees, both repos.** Skills ship in `.claude/` and `.codex/`, mirrored in + `codev/` and `codev-skeleton/`. +- **Delayed sends are not persisted.** A Tower restart drops them. This is fail-safe in + the direction that matters: the worst case is a `/arch-init` that never arrives, which a + human re-sends. + +### Business Constraints + +None. No timeline, budget, or compliance requirements. + +## Assumptions + +- Tower is running and the target terminal is registered. Both are already preconditions + for any `afx send`. +- The architect's harness supports `/clear` (Claude Code does). +- **A ~15s delay is long enough for the clear to take effect.** This is the value the + proposing workspace uses in its manual runs. It is a starting default, tunable per + invocation, not a claim about worst-case timing. +- The architect writes an honest, substantive, pruned resume block. The skill can + prescribe this; nothing verifies it, and the spec does not pretend otherwise. +- `/arch-init` remains the recovery entry point and keeps reading the role banner plus the + most recent dated section. + +## Solution Approaches + +### Approach 1: `afx send --delay` + a skill (recommended, and the owner's directive) + +**Description**: exactly the Desired State above. One Tower-side parameter; one skill. + +**Pros**: +- Minimal new surface: a delivery parameter on a path that already exists, and a document. +- The sending process is free to die — which is the actual constraint that made the third + leg impossible before. +- Nothing new to reason about at review time: no state machine, no job lifecycle, no + ordering invariants beyond "the skill's steps are in order." +- `--delay` is independently useful beyond this feature. +- Matches what the proposing workspace already does by hand, so the mechanism has field + evidence rather than only a design argument. + +**Cons**: +- Timing is open-loop. If a turn runs long, the delayed message can land at the wrong + moment. Accepted: recoverable by one manual re-send. +- A Tower restart during the window drops the message. Accepted: same recovery. +- Nothing enforces the write-before-clear ordering except the skill's own step order. + Accepted: the architect executing the skill is the same party that would have to be + trusted anyway. + +**Estimated Complexity**: Low +**Risk Level**: Low + +### Approach 2: Tower-owned quiesce → clear → re-orient job (rejected — descoped) + +**Description**: the shape this spec carried through two CMAP rounds. Tower arms a job +that waits for genuine terminal quiescence, delivers `/clear`, confirms it, then injects +the re-orientation. Verification gates (nonce receipt, size floor, compaction predicate, +stability) before anything is armed; a `--begin`/`--boundary` handshake to machine-own the +snapshot; a durable intent record so a dropped job is reportable. + +**Pros**: +- Closes ordering hazards by construction rather than by convention. +- Refuses to clear mid-turn, on an unverified save, or on a stub. + +**Cons**: +- **Disproportionate to the failure it prevents.** Every hazard it closes costs, at worst, + one manual re-send. The machinery to close them is a job runner, a durable record, a + handshake protocol, and a set of ordering invariants — permanently, in Tower. +- Two CMAP rounds went into hardening it, and the findings were sound; they were answers + to a question not worth asking at this price. +- It could not fully deliver its headline guarantee anyway: Tower exposes no turn + identifier, so "clear never destroys post-save work" degraded to a bounded window with a + heuristic regardless. + +**Estimated Complexity**: Medium-High +**Risk Level**: Medium + +*Rejected by owner directive.* Recorded because the rejection is informative: the review +rounds improved the design without ever questioning its scale, and the descope came from +outside that loop. + +### Approach 3: Detached client process (rejected) + +**Description**: the issue's original leg-3 design — a detached process that sleeps ~45s, +then sends `/arch-init`. + +**Cons**: an orphan process holding a scheduled action is invisible to `afx status`, +survives Tower restarts so it can fire into a world nobody expects, and has no +cancellation path. `--delay` puts the same wait inside the component that already owns +delivery and already has a lifecycle. + +**Estimated Complexity**: Low +**Risk Level**: Medium-High + +## Open Questions + +### Critical (Blocks Progress) + +*None.* The mechanism has field evidence: the proposing workspace runs this cycle +manually, including raw-typed `/arch-init `, successfully. + +### Important (Affects Design) + +- [ ] **Does raw-typed `/arch-init ` land reliably when delivered by Tower?** + Manual runs in the proposing workspace succeed, which is real evidence but not + evidence about *this* delivery path. The theoretical concern is slash-command + autocomplete accepting a highlighted completion instead of submitting. Verified + empirically in the live run; if it bites, the fallback is a plain-text message + naming identity and state-file path, which has no completion surface. +- [ ] **Is 15 seconds the right default?** Taken from manual practice. Tunable per + invocation; confirm against a real clear and adjust the skill's documented value. +- [ ] **Should `--delay` have a maximum?** A bound (say, one hour) prevents a typo from + parking a message indefinitely. Assumed yes. + +### Nice-to-Know (Optimization) + +- [ ] Should pending delayed sends be listable or cancellable? Not required for this + feature; worth it only if delayed sends find other uses. +- [ ] Should the skill snapshot the previous state file before overwriting? These files + are gitignored, so a bad save is unrecoverable. A one-line `cp` in the skill is + nearly free insurance — but it is the architect's discipline, not a gate. + +## Performance Requirements + +- **Response Time**: `afx send --delay` returns immediately, like any send. This is + functional, not cosmetic — the calling session must be free to end. +- **Delivery accuracy**: best-effort, order-of-seconds. Precision is explicitly not + required; the recovery for a mistimed delivery is one manual message. +- **Throughput / Resource Usage**: a pending delayed send is one timer in Tower. Negligible. +- **Availability**: N/A. Tower down means no delivery, recovered manually. + +## Security Considerations + +- **Authentication / authorization**: unchanged. `--delay` adds no new addressing or + privilege; a delayed send is subject to exactly the same target resolution and + builder-spoofing checks as an immediate one (`servers/tower-messages.ts:213-218`). +- **Path traversal**: `` is interpolated into `codev/state/.md` by the skill. + The existing `/arch-init` validation rule applies. +- **Data privacy**: state files are per-person and gitignored. The skill must repeat + `/arch-init`'s content guardrails — no secrets, no transcript dumps, no raw tool output. +- **Destructive action**: `/clear` is irreversible, and the human decision is relocated to + invoking `/arch-save`. The skill documents that architects do not invoke it autonomously + mid-task. Nothing verifies this — it is a documented norm, and the spec says so rather + than implying a check. +- **Delayed delivery is not a privilege escalation**: it cannot target anything an + immediate send could not, and it carries no elevated rights while pending. + +## Test Scenarios + +### Functional Tests + +1. **Delayed delivery.** `afx send --delay N` returns immediately; the message arrives + after ~N seconds; the sender's process has already exited. +2. **Composition.** `--delay` works with `--raw`, `--file`, `--no-enter`, `--all`, + `--interrupt`, with normal formatted messages, and across addressing forms + (``, `architect`, `architect:`). +2a. **Ordering under buffering.** An earlier message held by `SendBuffer` (user typing) + is delivered **before** a later delayed message to the same session, even when the + delay expires while the first is still buffered. This is the ordering the feature + depends on, so it is tested directly rather than inferred from FIFO. +2b. **Self-addressing.** `/arch-save` targets `architect:`; a sibling architect + invoking it does not touch main's terminal. +3. **Undelayed behaviour unchanged.** Sends without `--delay` are byte-identical in + behaviour and timing to today. +4. **Invalid delays rejected**: zero, negative, non-integer, NaN, and above the maximum — + each at the CLI boundary, before anything is scheduled. +5. **Tower restart during the window.** The pending message is dropped; nothing is + delivered; no crash, no leaked timer. +6. **Target disappears before delivery.** Delivery fails gracefully; no unhandled + rejection. +7. **Skill scaffolding.** `codev init` into a clean directory produces + `.claude/skills/arch-save/SKILL.md` and `.codex/skills/arch-save/SKILL.md`; `codev + update` backfills without touching a customised copy — mirroring `arch-init`'s coverage. +8. **All four skill copies identical.** +9. **Full cycle, live.** A real architect runs `/arch-save`: state written and pruned, + monitors stopped, `/clear` lands, `/arch-init ` arrives after the delay, the fresh + session reports its identity and resumes from the state file. +10. **Recovery path.** With the delayed message deliberately dropped, a human re-sends + `/arch-init ` and the session recovers fully — the property the whole design + leans on, so it is exercised rather than assumed. + +### Why the live e2e has the shape it does (architect-only, sibling probe) + +`/arch-save` is an **architect-session skill**: its step 1 requires a *builder* to stop and +report the mismatch rather than proceed. This is deliberate — the cycle clears its own +session's context, and only an architect has a context worth clearing this way. Two +consequences fix the shape of scenario 9's live run, and both are constraints, not choices: + +- **The builder implementing this feature cannot run its own live e2e.** By design it must + refuse. So the run is executed by an architect, in the **verify phase**, not by the + builder during implement. +- **Running it destroys the invoking architect's context.** Using the workspace's `main` + architect as the fixture would wipe the coordinating context mid-project. The run + therefore uses a *throwaway sibling architect* (`afx workspace add-architect --name + probe-1307`, an architect-only action from the main root): plant a canary, have the + sibling invoke `/arch-save`, verify the canary is gone and the state file recovered, then + `remove-architect`. Non-destructive to `main`, and it exercises the `architect:` + addressing the skill's own worst-case warning is about. + +This is modelled on Spec 1273's probe retest, which proved the same shape works. The e2e is +scheduled for verify and **disclosed as unrun at the PR gate** rather than discovered +missing later — the 1273 lesson applied forward. + +### Non-Functional Tests + +1. **Timer hygiene**: no leaked timers after delivery, after failure, and after shutdown. +2. **Sender independence**: delivery still happens when the sending process exits + immediately after the call. +3. **Security parity**: a delayed send is subject to the same spoofing check as an + immediate one — asserted directly, since a bypass here would be a real privilege gap. + +## Dependencies + +- **External Services**: none. +- **Internal Systems**: Tower's send pipeline (`servers/tower-messages.ts`, + `servers/message-write.ts`); the `afx send` CLI; `lib/scaffold.ts` and the `codev + init/adopt/update` path for skill distribution; the `/arch-init` skill as the recovery + entry point. +- **Libraries/Frameworks**: none new. + +## References + +- Issue #1307 — proposal, design notes, the v67 state-block template (comment 1), and the + monitor-lifecycle correction (comment 2). +- Issue #1310 — monotonic per-session input-generation counter. **Not a dependency.** The + primitive that would let a future version replace timing assumptions with observation, + if evidence ever shows the tail hazards below actually bite. +- `.claude/skills/arch-init/SKILL.md` — the save discipline this packages. +- `codev/specs/1273-builder-context-reset-should-b.md` — the builder-flavoured cycle; + source of the raw-vs-escape channel constraint. +- `codev/specs/1134-afx-whoami-ship-arch-init-comm.md` — `afx whoami`, `/arch-init`. + +## Risks and Mitigation + +**The posture, stated once and applied throughout**: the state file survives the clear, +the terminal stays alive, and re-sending `/arch-init ` by hand recovers everything. +So *most* hazards below cost at most one manual message, and for those the mitigation is +**accepted recoverability** rather than more mechanism. That is why timing heuristics +suffice here. + +**Two hazards fall outside that posture and must be designed out, not accepted** — both +surfaced by the plan review, and both share a signature worth naming: the damage lands on +a context that is *not* the one being refreshed, so "re-send `/arch-init`" does not repair +it. + +1. **A clear that arrives *after* recovery.** If the delayed `/arch-init` overtakes a + buffered `/clear`, the clear destroys the context that just recovered. Re-sending + produces the same race. +2. **A clear aimed at the wrong architect.** Bare `architect` addressing resolves to + `main`, so a sibling architect's refresh would wipe an uninvolved session whose owner + never asked for anything. + +The recoverability argument is load-bearing for this design, so where it does not apply +has to be stated as precisely as where it does. + +| Risk | Probability | Impact | Mitigation | +|------|------------|--------|------------| +| The 15s delay is mistimed — `/arch-init` lands before the clear completes, or long after | Medium | Low | **Accepted as recoverable**: re-send by hand. Delay is tunable; default confirmed against a live run. | +| A Tower restart drops the pending `/arch-init` | Low | Low | **Accepted as recoverable**: re-send by hand. Not persisting is deliberate — a persisted message could fire into a session that has moved on. | +| Raw-typed `/arch-init ` is intercepted by slash-command autocomplete | Low | Medium | Field evidence from manual runs says it works; confirmed in the live run. Fallback is a plain-text message naming identity and state path (no completion surface). | +| The architect starts new work between the save and the clear | Low | Medium | **Accepted**: the skill instructs stopping after step 4. Closing this properly needs the observable in #1310; not worth building for a recoverable loss. | +| Phantom monitors survive the clear and fire stale alerts | High (observed live) | Medium | Skill sequences the pre-clear stop (the enforceable half — that context holds the handles); the state block lists them so the resumed instance recognises an unaccountable alert as stale and re-arms deliberately, self-testing before trusting alerts. | +| A save that only appends, or over-prunes an irreplaceable file | Medium | Medium | Pruning is a stated requirement of the skill, with the prune-by-pointer rule repeated because these files are gitignored. Optionally a one-line `cp` snapshot before the write. | +| An architect invokes `/arch-save` autonomously mid-task | Low | Medium | Documented owner-direction norm with an override carve-out. Not machine-checked, and the spec says so. | +| `/clear` sent over the escape route instead of `--raw` delivers a bare interrupt | Low | High | The escape route discards the message body. Skill uses `--raw` explicitly and says why; asserted in the live run. | +| **The delayed `/arch-init` overtakes a buffered `/clear`, so the clear wipes the recovered context** | Medium | **High — not recoverable by re-send** | Due messages re-enter the normal delivery path including `SendBuffer`, so a delayed message queues behind anything already pending for that session. This is the one hazard here that the manual-re-send posture does **not** cover: the damage is a *second* clear after recovery, so it must be designed out rather than accepted. | +| **`/arch-save` clears the wrong architect's terminal** | Medium if bare `architect` is used | **High — destroys an uninvolved session** | The skill addresses `architect:` explicitly. Bare `architect` resolves to `main`/first-registered for non-builder senders (`tower-messages.ts:371-372`), so a sibling architect would clear main. Also outside the recoverable posture — the victim never invoked anything. | +| Skill ships in fewer than four trees, so adopters silently lack it | Medium | Low | Four-tree coverage is a success criterion, using `arch-init`'s existing scaffolding test pattern. | + +## Expert Consultation + +**Date**: 2026-07-31 +**Models Consulted**: Claude (`REQUEST_CHANGES`) and Codex (`REQUEST_CHANGES`), against +the **previous, larger architecture** (Approach 2). + +**Disposition**: all 14 findings were accepted and incorporated, and the resulting design +was then **descoped out of existence** by owner directive. The findings were not wrong — +they were sound answers to a question that should not have been asked at that cost. What +survives from those rounds: + +- **The monitor-lifecycle correction** (Claude): the post-clear "stop stale monitors" + obligation was unimplementable — no enumeration mechanism exists, `pgrep` cannot see + harness tasks. The enforceable half is the *pre-clear* stop. Carried into the skill. +- **Two verified factual corrections** (Codex): `tower-cron.ts:70` ticks every 60 seconds, + and `lastDataAt` (`terminal/shellper-client.ts`) is a last-output timestamp with no turn + identity. The second one killed a guarantee the previous design advertised, and is why + #1310 exists. +- **The failure-containment analysis**: worked out while hardening Approach 2, and it is + what makes Approach 1 defensible. Knowing precisely how cheap the failures are is what + licensed removing the machinery. + +Full record: `codev/projects/1307-*/1307-specify-iter1-rebuttals.md`. + +## Approval +- [ ] Technical Lead Review +- [ ] Product Owner Review +- [ ] Stakeholder Sign-off +- [ ] Expert AI Consultation Complete + +## Notes + +**On the rewrite.** This spec previously specified Approach 2 — a Tower-owned job with +verification gates, a `--begin`/`--boundary` handshake, durable intent records, and +bounded-window machinery around the clear. The owner descoped it: *"this is +overcomplicated way more than it needs to be."* That is correct, and the diagnosis is +worth recording, because the failure mode was invisible from inside the review loop: two +CMAP rounds and several owner exchanges all worked on making the design *sound* without +anyone asking whether it was *proportionate*. Each round added rigour to machinery that +should not have existed. Reviews optimise the design in front of them. + +The measure of this rewrite is that it is much shorter. That is the result, not a loss. + +**Explicitly out of scope**: Tower-side quiescence detection, clear confirmation, +verification gates on the state file, job status/cancellation surfaces, persisting delayed +sends across restarts, cross-workspace or sibling-architect targeting, UI surfaces, and +building #1310. + +**What this feature promises**: that the ordering which matters — save before clear — is +built into the procedure, and that the last leg of the cycle can be delivered by something +that outlives the clear. **What it does not promise**: precise timing, or that the +re-orientation always lands. When it does not, one manual message fixes it, and the design +is shaped around that being true rather than around preventing it. diff --git a/codev/state/aspir-1307_thread.md b/codev/state/aspir-1307_thread.md new file mode 100644 index 000000000..8eaee884f --- /dev/null +++ b/codev/state/aspir-1307_thread.md @@ -0,0 +1,566 @@ +# aspir-1307 — `/arch-save`: packaged save→clear→re-init for architects + +## 2026-07-31 — Specify phase, iteration 1 + +Spawned strict-mode ASPIR on issue #1307. No spec existed on disk, so `porch next` +put me straight into Specify with a blank sheet. + +### What I read before writing + +- **Issue #1307 + both comments.** The second comment is a *correction* that changes + the design and is easy to miss: monitors are **session-bound, not context-bound** — + a watcher armed pre-clear SURVIVES `/clear` and fires stale alerts into the fresh + context. So the state block's monitor list is a **kill-list AND a re-arm list**, in + that order. The original "monitors die at the clear" framing in the issue body is + superseded. +- **`.claude/skills/arch-init/SKILL.md`** — the save discipline (§"Saving your state") + is the prose this issue asks to package. Its human-keystroke rule is the invariant + that must survive packaging. +- **`packages/codev/src/agent-farm/commands/reset/`** (Spec 1273, PR #1305) — the + machinery to reuse: `receipt.ts` (nonce-in-file freshness gate, R2), `index.ts` + (ordering state machine + step log, R1/R3/R4), `reset.ts` (port bindings). +- **`servers/message-write.ts`** — confirmed `escape: true` discards the body, so + `/clear` must go over `raw: true`. Already handled in reset's port split. +- **`commands/whoami.ts`** — architect identity comes from `CODEV_ARCHITECT_NAME`, + builders from worktree cwd. This is how self-invocation is detected. +- **`servers/tower-cron.ts`** — precedent that Tower already runs deferred work. + +### The crux I had to design around + +`afx reset` works because the invoker is a *different* terminal from the target. For +`/arch-save` the architect is often the invoker AND the target, and that breaks two +things at once: + +1. The quiescence gate (R4) can never pass — the CLI's own output is the noise it is + waiting to stop. +2. The CLI process dies with the clear, so it cannot deliver the re-orientation. + +The issue's own design note calls the answer: Tower survives the clear, so **Tower +owns the post-arm sequence**. The CLI arms an in-memory Tower job and exits so the +architect's turn can end. External (owner-run) invocation arms the same job and just +tails it. One state machine, two front doors. + +### Decisions baked into the spec + +- Dedicated `afx arch-save`, not `afx reset --state` — reset resolves + targets via `findBuilderById`, architects are not builders, and arch-critical says + add a dedicated concept rather than bolting a flag onto a shared one. Machinery is + factored out and shared, not duplicated. +- Re-orientation payload is **exactly `/arch-init `**, delivered raw. Every + resume instruction lives in the state file, because that is what `/arch-init` reads. + Appending a checklist to the injected line would corrupt the slash-command argument. +- Nonce round-trip kept from #1273 (arm → nonce → architect writes → Tower verifies), + so both invocation modes share one freshness proof. +- `--boundary` is required and is where the relocated human decision is recorded. +- New: a required `## Monitors` section in the state file, so "none armed" has to be + written consciously rather than omitted silently. +- New: the CLI snapshots the *previous* state file at arm time. These files are + gitignored (`.gitignore:15`), so a bad save is otherwise unrecoverable. + +### Known dependency, flagged not blocking + +The #1273 live e2e ("does `/clear` actually take effect over the raw channel") has not +run. Spec records it as a Critical open question with an explicit mitigation: every +gate aborts *without* clearing, and a no-op `/clear` degrades to "architect keeps its +context and also gets `/arch-init` re-injected" — which loses nothing. Proceeding +rather than blocking; called out for the architect at the PR gate. + +Next: write `codev/specs/1307-arch-save-packaged-save-clear-.md`, then `porch done`. + +## 2026-07-31 — Specify iter 1 review: REQUEST_CHANGES, and it changed the design + +Claude returned `REQUEST_CHANGES` (HIGH confidence) after verifying my factual claims +against the code — all held. Codex could not run (see below). Seven issues raised; I +agreed with all seven and one of them moved the recommended architecture. + +**The one that mattered — write-then-verify.** The review asked why I hadn't considered +having the architect write the state file *before* invoking the CLI, so the CLI could +validate synchronously and arm only `quiesce → clear → reorient`. I hadn't, and it wins: + +- Removes receipt polling from Tower entirely — the new server-side surface drops to one + small job. +- Makes "no clear without a verified save" **true by construction** in the self path, + rather than a gate that could be misordered. +- Collapses the clear-after-new-work window from up to 300s to a single quiet window. + +Kept the original nonce/Tower-armed shape as Approach 1b with explicit rejection reasons +rather than deleting it. The external (owner-run) path still uses the nonce receipt gate, +because there a *remote* party is being asked to comply — and that gate already exists +and works in the CLI process. Two proof-of-save paths, one shared destructive job. + +**A consequence I had to chase down myself**: write-then-verify breaks the state-file +snapshot. The CLI used to run before the overwrite; now it runs after. So in the self +path the snapshot has to be the *skill's first step*, and the test has to compare +snapshot content (not just existence) or a snapshot-taken-too-late passes silently. +These files are gitignored — there is no second chance to notice. Logged as its own risk +row. + +**Other six, all incorporated in place:** +1. Post-clear "stop stale monitors" was unimplementable as I wrote it — no enumeration + mechanism, and comment 2 says `pgrep` can't see harness tasks. Restated: pre-clear + stop is the enforceable half (that context holds the handles); post-clear is + best-effort reconciliation + disregard-what-you-can't-account-for. +2. My `## Monitors` heading gate contradicted the v67 template I claimed to adopt + (its monitor list lives in a `#`-comment intent stamp). Worse, I mandated a gate + while leaving its placement an open question. Now a `MONITORS:` token the template + carries verbatim; open question closed. +3. Clear-after-new-work hazard — absent from risks, questions and tests. Added to all + three; mitigated structurally by write-then-verify + first-quiescence-only firing + + bounded armed lifetime. +4. `--boundary` overclaimed as "a recorded human decision" — in the self path the agent + types it. Now records invocation mode and states the limit plainly. +5. Quiescence-against-a-live-TUI is a *second* unrun-e2e unknown, not just `/clear`. + Safe but total failure (feature never fires). Live run scoped to both. +6. `sendRaw` vs `sendMessage` divergence: reset wraps its reorientation in a + `[MESSAGE FROM …]` envelope, which would stop `/arch-init` being a slash command at + all. Recorded as a constraint the shared extraction must not collapse, plus a test + asserting the exact channel and payload. + +Also flagged: raw-typing a slash command *with an argument* may hit autocomplete and +have Enter accept a completion instead of submitting. This is the one step with no safe +degradation — a swallowed reorientation leaves a *cleared* architect with no identity — +so it's a risk row demanding live confirmation, not an assumption. + +### Codex lane down — escalated, ruled on + +`consult -m codex` failed: server rejects `gpt-5.6-sol` as needing a newer Codex. I +checked the global CLI (0.146.0), judged a global upgrade out of a builder's scope, and +escalated rather than working around it. Architect's ruling: correct call, but wrong +culprit — consult's codex lane runs `@openai/codex-sdk` with its own **vendored** binary +(0.142.5), a different client from the global CLI entirely. PR #1309 bumps it; needs the +owner's merge word plus a global reinstall. + +Directed path: neither "proceed claude-only" nor "wait idle" — keep revising now, and +re-run codex against the **revised** spec once the reinstall lands. Strictly better than +having codex review a draft Claude already marked up. Not re-running consult until the +architect pings. + +## 2026-07-31 — Architect design input: the autocomplete hazard, designed out + +Architect offered a design input (explicitly "not a directive") on the one risk I'd +flagged as having no safe degradation: raw-typing `/arch-init ` into a TUI, where +autocomplete may eat the Enter and leave a *cleared* architect with no identity. Their +point: the payload doesn't have to be a typed slash command at all. + +Evaluated and **adopted**, with one addition of my own. + +**What it costs**: deterministic harness-level skill loading, traded for model-side +invocation. That would be a bad trade if the payload *depended* on the skill firing. + +**So I required it not to.** The message must be self-sufficient — identity and +state-file path stated inline — so a session that never invokes the skill still knows +who it is and where its state lives, and recovers by reading the file directly. Skill +invocation becomes an upgrade (whoami validation, architect guardrails) rather than the +load-bearing step. A step with no safe degradation now has two. + +Bonus simplification: with a plain-text payload this command's delivery matches reset's, +so the `sendRaw`-vs-`sendMessage` divergence I'd recorded as a constraint mostly +dissolves. The raw/escape split still has to survive extraction — `/clear` itself is +still raw-typed, and Tower's escape route discards the body — so that constraint stays, +narrowed to where it actually bites. + +Also added, per the same input: an explicit worst-case statement under Notes. State file, +terminal, and Tower's record all survive every failure mode, so the worst realistic +outcome is a live terminal with no identity *yet* and its full state one message away — +recoverable manual re-entry, not data loss. Worth writing down because it reframes how +the whole risk table should be weighed: the only expensive failure is clearing without a +good save, and that one is now true by construction. + +Swept the spec for stale references afterwards — nine places still described the payload +as a raw-typed `/arch-init ` (Desired State, success criteria, Approach 1, the +Critical open question's mitigation, Security, test 1, Dependencies). All updated. + +Still holding on codex; #1309 queued for the owner's merge word. + +## 2026-07-31 — Owner directives, then codex round. SPECIFY COMPLETE. + +Two owner directives arrived (via architect), then the codex lane unblocked (#1309 +merged, reinstall done) and codex reviewed the revised spec. + +**Owner directive 1 — pruning is a REQUIREMENT.** The save must remove cruft, not just +append. Resolved loops deleted, older entries collapsed to pointers at durable artifacts, +one-screen order of magnitude. A save that only appends FAILS acceptance. + +**Owner directive 2 — the reorientation delivery mechanism is explicitly UNDECIDED.** +Owner: "I'm not sure the best way to send the /arch-init again." This *reversed* what I'd +settled one message earlier. Correctly so: I had settled that question twice, in opposite +directions, both times on reasoning alone. Now a named open decision with three +candidates (raw-typed slash command / plain-text instruction / 1273's file+inline shape), +to be resolved empirically against a real terminal with the reason recorded. Noted +honestly that candidate (c) is proven in tests and design only — 1273's live e2e never +ran — so it doesn't get credit it hasn't earned. + +Lesson worth keeping: "settled by argument" kept *looking* like progress. Two reviewers +and an owner all had to push back before it became an explicit open decision. + +### Codex round — 7 findings, all accepted, two of my premises were false + +I verified codex's two factual claims against source before acting (standing lesson: +reviewer claims are evidence, not ground truth). Both correct, both fatal to something +I'd asserted: + +- **`tower-cron.ts:70` ticks every 60 SECONDS**, over filesystem-backed definitions. My + "the job rides an existing Tower tick" claim was wrong, and 60s cannot observe a 1.5s + quiet window. Clear-job now runs its own bounded loop; retracted the claim in-text so + the next reader doesn't re-derive it. +- **`lastDataAt` is a last-output timestamp** (`terminal/shellper-client.ts`); Tower has + no turn id or input-generation counter. So "original turn ended" and "follow-up turn + ended" are *observationally identical* — my criterion "the clear can never destroy work + created after the verified save" was UNIMPLEMENTABLE. Worse: that criterion was my own + fix for Claude's C3. I closed a hazard with a mechanism that can't observe what it + needs to. Downgraded to a bounded window + an output-total heuristic labelled as a + heuristic, with the residual gap named and a Tower observable raised as an open + question rather than pulled into scope. + +Two findings produced genuine design improvements, not just wording: + +- **`--begin`/`--boundary` handshake.** Codex caught that the self-path snapshot was + convention-owned — the skill took it, nothing verified it. Fix: `--begin` snapshots + under machine control and issues a token `--boundary` requires. Closes the snapshot + hole AND restores a machine-proven freshness token to the self path, which I'd traded + away arguing self-attestation was equivalent. It wasn't — precisely because it left + snapshot ordering unproven. +- **In-memory execution vs durable intent record.** I'd required a dropped job be + "reported rather than silent" while specifying purely in-memory jobs — those can't both + hold. Split: execution in memory (fail-safe, a restart can never clear), intent record + durable and inert (makes status/cancel/dropped-job reporting implementable). + +Also: exact compaction predicate (reject if snapshot survives as an unmodified leading +section) replacing a vague size comparison — admits the compact-and-grow case a size +ratio would wrongly reject; no-predecessor case defined; preflight vs post-verification +failure guarantees split, since "every gate leaves a saved state file" was false for +preflight; and Test 2 fixed, which still described the superseded nonce-before-write +sequence because I revised prose without re-reading tests against it. + +Rebuttal written (all 14 findings accepted, no disagreements defended). `porch done` +passed checks. **SPECIFY COMPLETE — advanced to PLAN.** No spec gate in ASPIR. + +Commits: 4150edb7, 1f11f794, de043dfd, 93bd2a9d. + +### LESSONS — carry these verbatim into codev/reviews/1307-*.md + +Architect asked that the first one be recorded verbatim. Both are review-file material, +staged here so they survive the phase boundary. + +1. **"I closed a hazard with a mechanism that cannot observe what it needs to."** + A fix's *implementability against real observables* is part of the fix, not a + downstream implementation detail. I answered Claude's clear-after-new-work finding + with "fire on the first quiescence transition" — which reads as a real mitigation and + is not one, because `lastDataAt` cannot distinguish which turn just ended. The fix + survived a full review cycle before Codex caught it. When proposing a mitigation, + name the observable it reads and confirm that observable exists. + +2. **Verifying reviewer factual claims against source paid off twice in one round.** + Codex made two claims about the codebase (`tower-cron`'s tick interval, `lastDataAt`'s + semantics). I checked both before acting. Both were correct — and each invalidated a + premise I had written into the spec. The habit is usually framed as protection against + *wrong* reviewer claims; its larger value here was confirming *right* ones fast enough + to act on them with confidence instead of hedging. + +3. **"Settled by argument" kept looking like progress.** The reorientation delivery + mechanism was settled twice, in opposite directions, before the owner made it an + explicit open decision. Neither settlement had an empirical check behind it. A + decision with a plausible rationale and no evidence should be *labelled* undecided, + not recorded as decided-with-reasons. + +Follow-up filed by the architect out of this round: **issue #1310** (monotonic +per-session input-generation counter). It is the observable that upgrades this spec's +bounded window to a guarantee, and it fixes the same blind spot in `afx reset`'s R4. +This spec ships without it and references it where the gap is named. + +## 2026-07-31 — OWNER DESCOPE. Spec and plan rewritten; 1164 → 438 lines. + +I had just finished a seven-phase plan when the owner's descope landed: *"this is +overcomplicated way more than it needs to be."* He's right, and the whole architecture is +gone. + +**New target shape — the entire feature:** +1. `afx send --delay ` — Tower-side deferred delivery, one parameter on the + existing send path. Not a client that sleeps, not a job orchestrator. +2. `/arch-save` as a SKILL: stop monitors → write the pruned state file → `--raw '/clear'` + → `--delay 15 --raw '/arch-init '`. + +**Dropped:** Tower-armed quiesce/clear/reorient job, `--begin`/`--boundary` handshake, +durable intent records, bounded-window machinery, the validation module, the shared +extraction from `commands/reset/`. + +**Kept:** pruning-as-requirement; the empirical check (narrowed to "does raw-typed +`/arch-init ` land," with the production workspace's successful manual runs as +existing evidence); and the failure-containment posture stated plainly as the *reason* +heuristics suffice — state file survives, terminal alive, manual re-send recovers +everything. Tail hazards get one honest RISKS section marking them accepted-as-recoverable, +with #1310 referenced as the future primitive if evidence ever shows they bite. + +### The lesson, and it is the biggest one of this project + +**Two CMAP rounds and several owner exchanges all worked on making the design *sound* +without anyone asking whether it was *proportionate*.** Every round added rigour to +machinery that should not have existed. The reviewers weren't wrong — their findings were +sound answers to a question we shouldn't have been asking at that cost. But reviews +optimise the design *in front of them*; none of them is structurally positioned to ask +"why is this here at all?" + +I was the worst offender: I had the failure-containment analysis in hand — I wrote "the +worst case is manual re-entry, not data loss" into the spec myself — and did not draw the +obvious conclusion, which is that machinery to prevent a one-message loss is not worth its +weight. I treated that analysis as *reassurance about* the design instead of *evidence +against* it. The descope came from outside the review loop because it could only have come +from outside. + +Concretely worth carrying to the review file: **when you find yourself proving a design is +safe, check whether the thing it protects is expensive.** A cheap failure plus elaborate +prevention is the signature of over-design, and it is visible in the spec's own text well +before anyone says so. + +Also: the descope vindicates the earlier "settled by argument kept looking like progress" +lesson at a larger scale. Same failure mode, one level up — local rigour masking a global +question nobody asked. + +### One thing I did carry forward deliberately + +The failure-containment analysis itself. Knowing *precisely* how cheap the failures are is +what makes the small design defensible rather than merely smaller. That analysis was +produced by the hardening rounds, so those rounds weren't wasted — they just produced a +different deliverable than the one they thought they were producing. + +### Plan rewritten: 3 phases + +phase_1 `afx send --delay` (Tower-side) → phase_2 skill in four trees + template → +phase_3 live e2e + docs. + +**The one security-relevant call in phase 1**, flagged there with its own acceptance +criterion and test: `--delay` defers *delivery*, never *authorisation*. Target resolution +and the builder-spoofing check must run at request time, or a delayed send becomes a way +to defer a check past the conditions that would fail it. Easy to get wrong by treating +`--delay` as "the same send, later." + +## 2026-08-01 — Phases 1 and 2 complete; coordination with aspir-1273 + +**Phase 1 (`afx send --delay`) took EIGHT review rounds**, six finding real defects. Two +patterns, each repeated three times: + +*Artifacts asserting something adjacent to the real thing* — a test against a copied +predicate, against a replica helper, against a synthetic callback, and a SPEC claiming a +request-order FIFO guarantee the code deliberately did not make. Each passed self-review +because the artifact existed. + +*Fixes correct about the mechanism, incomplete about its lifetime* — serialising the +callback but not its writes; guarding `hasPending` but not `flush()`'s own drain; clearing +the registry but not the already-attached `.then()` continuations. Each worked for the case +I was picturing and left the adjacent one open. + +One cheap check catches both classes: **mutate the guard, confirm the test fails.** By the +end I ran it before claiming a fix rather than after being told — which is how the +mid-flush test's vacuous first version (4-line message, writes completing in ~110ms, so the +delayed send never entered the window it was named for) got caught by me instead of a +reviewer. Same again in phase 2: I noticed only 4 of 5 test files executed and found +`init.test.ts` is excluded in `vitest.config.ts`, so an assertion I had just added guarded +nothing. + +**Phase 2 (skill in four trees)** approved in two rounds. Both reviewers caught that my +"all four copies identical" was verified by hand with md5 and not guarded — `skill-parity` +only compares providers *within* a tree, never instance vs skeleton. Codex separately caught +a real overclaim: the skill said Tower "delivers it after the clear has landed" when Tower +only waits out 15s and never observes the result. + +### Coordination with aspir-1273 (submission lock, PR #1320) + +Their production e2e found `afx reset`'s `/clear` arriving as literal text welded to the +next message — never executed, context intact, every layer reporting success. Root cause: +`writeMessageToSession` schedules its Enter 50–80ms later and `/api/send` responds once the +write is *scheduled*, so an awaited send resolves before its own submission. + +**Ordering is not atomicity.** My FIFO work decides which message goes first; it does not +make a delivery atomic. Architect ruled 1273 owns the primitive and I adopt it unchanged. + +Two things I contributed by checking rather than accepting: +- Their datum that "reset's own writes bump `_lastInputAt`, so the flow trips its own + buffering" is **false** — `recordUserInput()` is called only at `pty-manager.ts:310/:317`, + both in the websocket handler. They verified independently and retracted it. +- Measured the merge surface with `git merge-tree` rather than guessing: merge-base + `57c51a6e`, exactly one conflicting file (`tower-routes.ts`), test file auto-merges. + +And one correction I received, which changed the work: adopting #1320 is a **replacement, +not a deletion**. It wires only the escape and immediate paths, so I must ADD two +`submitToSession` call sites (delayed delivery, `flush()`'s drain) before removing +`writeCompletesInMs`, `busyUntil`, and `delayed-send.ts`'s chain. Recorded as a six-step +sequence in the plan. + +The line 1273 drew and I am keeping: their test proves the *primitive* supports the +pattern; it cannot prove *my wiring* of it is correct. Different claims. + +### 2026-08-01 — merged origin/main (architect-directed) + +PR #1324 landed, skipping `agy-integration.e2e.test.ts` which was opening OAuth browser +windows on the human's machine during suite runs. Previewed with `git merge-tree` first: +clean, no conflicts. #1320 had **not** landed, so no adoption work triggered. + +Post-merge: install/build clean, 4149 tests passing (up from 4090 — incoming Spec 1280 +tests). Re-ran my own suites explicitly rather than trusting the aggregate: 186 Spec 1307 +codev tests + 48 core, all green, all four `ORDERING:` guards intact. + +Note for later: the agy binary was briefly disabled machine-wide (gemini lane skipping); +that was reverted the same day and agy is back on PATH. + +## 2026-08-01 — Phase 3 docs; a cross-project collision with Spec 1280 + +Documented `--delay` in `codev/resources/commands/agent-farm.md` (full reference) and +`CLAUDE.md`/`AGENTS.md` (pointer, byte-identical). + +**The collision.** Spec 1280 landed on main mid-flight. Its measurement instrument asserts +*exact* word counts of the always-on prompt surface, and CLAUDE.md/AGENTS.md are part of +that surface — which is exactly where my deliverable lives. My first draft (+125 words) +broke two of its assertions. + +**Checked the cause instead of assuming it**: backed the two files up, restored them from +HEAD, re-ran the instrument, got exactly 34231 — their asserted value. So their instrument +was unchanged and correct; its *input* had grown. Then restored my edits. Worth the two +minutes: the alternative reading ("their new test is broken") would have sent me editing +the wrong thing. + +**Two responses.** Shrank the always-on addition 125 → 62 words, because Spec 1280 exists +to *measure and reduce* that surface and spending 125 words of it on a CLI flag with an +on-demand full reference is disproportionate — that judgement stands independent of the +test failure. Then updated their baselines (34231 → 34293, 8599 → 8661) with their +derivation comment preserved and my causal note appended. + +**Flagged rather than decided**: an absolute pinned count breaks on *every* future edit to +*any* always-on doc, by any project. I hit it on day one of 1280 being merged, and the +natural reaction for the next person is to bump the number without checking whether the +instrument itself regressed — the exact failure the test exists to prevent. Routed to 1280 +as their call. Architect approved the handling in full and confirmed the routing. + +### Live run: HELD, deliberately + +`#1320` (1273's submission lock) is not on main. My live run's first question — *does the +`/clear` actually EXECUTE* — is precisely what that PR fixes, so running now would test the +pre-fix world. Architect confirmed the hold: my e2e batches with 1273's probe retest in one +window after #1320 merges and installs. + +Wrote the **live-run runbook** into plan phase 3 while waiting, so the window is mechanical +rather than improvised. The load-bearing step is the canary: plant a secret word before the +cycle, and check it is gone afterwards. "The send returned 200" is not evidence of a clear — +it returned 200 in 1273's failing run too. + +Everything else in phase 3 is done. Phases 1 and 2 approved by both reviewers. + +## 2026-08-02 — phase 2 closed; `--delay` relocated; main found red + +**A drift I caused myself, worth recording.** Asked "what are you waiting for?", I checked +instead of restating — and found I was only *partly* blocked. #1327 had merged (my queued +baseline-bump drop was actionable), and porch had been sitting on phase_2 waiting for *my* +verification consults while I had wandered into phase-3 work during a run of interleaved +instructions. Phase 2 is now properly closed and porch is on phase 3. + +Lesson: when several instructions arrive mid-turn, the orchestrator's own state is the +thread most likely to be dropped, because nothing prompts for it. `porch status` is cheap. + +**`--delay` documentation relocated** (ruling, ratified). Zero words in +`CLAUDE.md`/`AGENTS.md`; full reference in `codev/resources/commands/agent-farm.md` **and +its skeleton mirror**. I had only edited the `codev/` copy — checked whether the skeleton +was a mirror rather than blind-copying, found it legitimately differs on main, and wrote +the equivalent content in its own shape. Spec criterion amended in place with a dated +supersession note: it was authored against the pre-1280-rewrite world, where CLI detail +still lived in `CLAUDE.md`. + +One self-inflicted detour: after reverting the two files the guard still failed, because +`origin/main...HEAD` compares **committed** state and my revert was uncommitted. Reads as +"the fix didn't work" when it is "the fix isn't in the commit yet." + +**Merged #1143 proactively** — it touches both copies of `agent-farm.md`, which I had just +edited. Previewed with `merge-tree` (clean), merged, verified both my `--delay` section and +their cron content survived. So the conflict 1273 hit between #1320 and #1143 does not +repeat here. + +### main was red, and it was not mine + +That merge turned the suite red on three parity tests. Checked `origin/main` **directly** +rather than assuming my merge caused it: + + git show origin/main:.claude/skills/afx/SKILL.md | md5 -q -> 667efc64… + git show origin/main:.codex/skills/afx/SKILL.md | md5 -q -> 32c9692c… + +#1143 updated the two `.claude` copies of the afx skill and neither `.codex` copy. Main was +already broken; my branch inherited it, as would every builder merging next. + +**Did not fix it.** The file is the one I had been told not to touch (`#1318`'s), and I +would have been guessing whether #1318 had a fix in flight that mine would conflict with. +Escalated with the md5 evidence and three options instead. Architect took it, fixed it +themselves (#1332), and confirmed the hold was right on both layers. + +**Their root cause, recorded because it generalises:** #1143's green CI was from July 6, +predating the parity guards the repo has grown since. The gate check confirmed no drift in +the files #1143 *touched*, but not against invariants added *after* its run. New standing +rule: a stale CI green gets re-validated against current main's guards before merge. + +That is the same shape as this project's recurring lesson, one level up — **an artifact +(a CI result) asserting something adjacent to the truth**. It was true when produced and +false when used. + +Currently blocked on #1332 landing. #1320 also still open, its own conflict with #1143 +being resolved by 1273, so the live-run window has moved but is still coming. + +## 2026-07-31 — Plan CMAP iter 1: both reviewers found the SAME two defects + +Both REQUEST_CHANGES, both HIGH. All ~14 findings accepted, none defended. The signal +worth noting: **codex and claude independently converged on the same two items**, and both +are outside this design's recoverability posture — the failures a manual re-send does NOT +repair. + +**P1 — `SendBuffer` can invert `/clear` and `/arch-init`.** Verified: `/api/send` already +buffers when the user is typing (`tower-routes.ts:1570`, `!session.isUserIdle(3000)`, up to +60s; `isUserIdle` reads `_lastInputAt`, i.e. *input*). The `/arch-save` flow is exactly the +trip case — the owner just typed a direction, so `/clear` gets buffered: + +``` +T+0 /clear → BUFFERED (user typing, up to 60s) +T+15 /arch-init due → direct write → LANDS FIRST +T+40 buffer flushes → /clear → wipes the recovered context +``` + +My plan literally said it "schedules only the terminal write" — that bypass IS the bug. +Fix: due messages re-enter the normal delivery path, buffering included, so per-session +FIFO does the work. Not accepted risk: re-sending `/arch-init` just re-runs the race. + +**P2 — `afx send ` was a placeholder I never resolved.** Bare `architect` resolves to +`main`/first-registered for non-builder senders (`tower-messages.ts:371-372`), so a SIBLING +architect's `/arch-save` would clear MAIN's terminal. Worst possible outcome — destroys a +session whose owner never invoked anything — and one word from correct. Fix: +`architect:` explicitly, everywhere, with the reason stated. + +Other findings, all real: phase 1 pointed at `agent-farm/lib/tower-client.ts`, a re-export +shim — the implementation is `packages/core/src/tower-client.ts:655` (cross-package, +core-first build); delivery must re-fetch by terminal id rather than close over a +`PtySession`; shutdown needs a registry and must DROP delayed sends, not flush them like +`SendBuffer` does; `--escape` composition was unsatisfiable (`afx send` has no such flag) +so it is recorded N/A; `--interrupt` writes Ctrl+C at request time and must be deferred +WITH the message; `adopt.test.ts` coverage was missing; the spoofing check is at +`tower-messages.ts:225-234` and only fires on the `architect:` path, so the +authorisation test must use that form or it proves nothing; `tower-cron` is unsuitable +because `CronDeps.resolveTarget` takes no `sender` (better reason than the tick interval I +gave); and the delay budget is measured from send, while the clear only runs after the +turn ends — so phase 3 must calibrate send→session-ready, not send→clear-sent. + +**One I would have shipped**: the four `arch-init` SKILL.md copies still document the +manual save→suggest-`/clear`→human-clears loop. Adding `/arch-save` without touching them +ships two contradictory procedures for the same task. Now a phase-2 deliverable. + +### Lesson for the review file + +**Making a design smaller does not make it easier to get right — it concentrates the +remaining risk.** After the descope I had ~40 lines of real behaviour change, and both +genuine defects were in the same seam: where the NEW delivery path meets the EXISTING one +(`SendBuffer`, address resolution). I wrote the small plan as though small meant safe, and +under-specified precisely the interaction surface. When scope drops sharply, the remaining +risk does not spread out — it pools at the integration points with what was already there, +and that is where the next review should be pointed. + +## 2026-08-02 — At the `pr` gate. Waiting. + +All plan phases complete, review file written, PR #1335 open against `main`. All six CI +checks green (unit, CLI ubuntu + macos, CLI integration, Tower integration, package +install). `porch status` reports `pr` gate pending since 05:46Z. + +Nothing further for me to do autonomously: the gate is a human decision and I do not call +`porch approve`. Architect notified. Stopping until approval arrives, then I merge and +enter verify. diff --git a/packages/codev/src/__tests__/adopt.test.ts b/packages/codev/src/__tests__/adopt.test.ts index 8ae80b7a5..075b9a991 100644 --- a/packages/codev/src/__tests__/adopt.test.ts +++ b/packages/codev/src/__tests__/adopt.test.ts @@ -106,6 +106,10 @@ describe('adopt command', () => { expect( fs.existsSync(path.join(projectDir, '.codex', 'skills', 'afx', 'SKILL.md')) ).toBe(true); + // Spec 1307 + expect( + fs.existsSync(path.join(projectDir, '.codex', 'skills', 'arch-save', 'SKILL.md')) + ).toBe(true); }); it('should throw error if codev directory already exists', async () => { diff --git a/packages/codev/src/__tests__/init.test.ts b/packages/codev/src/__tests__/init.test.ts index 302ae8199..7c2dae1aa 100644 --- a/packages/codev/src/__tests__/init.test.ts +++ b/packages/codev/src/__tests__/init.test.ts @@ -70,6 +70,19 @@ describe('init command', () => { expect( fs.existsSync(path.join(projectDir, '.codex', 'skills', 'arch-init', 'SKILL.md')) ).toBe(true); + // Spec 1307. NOTE: this file is excluded from the default run + // (vitest.config.ts — "Flaky: codev doctor timeout in worktree + // context"), so this assertion does NOT currently guard anything. The + // real arch-save scaffolding coverage lives in scaffold.test.ts + // (copySkills against the actual skeleton), update.test.ts and + // adopt.test.ts, all of which do run. Kept so it is correct if the + // exclusion is ever lifted — not counted as coverage. + expect( + fs.existsSync(path.join(projectDir, '.claude', 'skills', 'arch-save', 'SKILL.md')) + ).toBe(true); + expect( + fs.existsSync(path.join(projectDir, '.codex', 'skills', 'arch-save', 'SKILL.md')) + ).toBe(true); // Verify user data directories (minimal structure) expect(fs.existsSync(path.join(projectDir, 'codev', 'specs'))).toBe(true); diff --git a/packages/codev/src/__tests__/scaffold.test.ts b/packages/codev/src/__tests__/scaffold.test.ts index 9fc4b39dc..e45570d05 100644 --- a/packages/codev/src/__tests__/scaffold.test.ts +++ b/packages/codev/src/__tests__/scaffold.test.ts @@ -309,6 +309,21 @@ describe('Scaffold Utilities', () => { ).toBe(true); }); + // Spec 1307: /arch-save is useless to an adopter if it ships in some trees + // and not others — the failure is silent, since nothing errors, the command + // simply is not there. + it('installs the arch-save skill for both providers', () => { + const result = copySkills(tempDir, realSkeletonDir); + expect(result.copied).toContain('.claude/skills/arch-save/'); + expect(result.copied).toContain('.codex/skills/arch-save/'); + expect( + fs.existsSync(path.join(tempDir, '.claude', 'skills', 'arch-save', 'SKILL.md')) + ).toBe(true); + expect( + fs.existsSync(path.join(tempDir, '.codex', 'skills', 'arch-save', 'SKILL.md')) + ).toBe(true); + }); + it('enumerates every skill directory dynamically for both providers', () => { const result = copySkills(tempDir, realSkeletonDir); const expected = ['claude', 'codex'].flatMap((provider) => diff --git a/packages/codev/src/__tests__/update.test.ts b/packages/codev/src/__tests__/update.test.ts index 7a0ecd7dc..6e54d3af5 100644 --- a/packages/codev/src/__tests__/update.test.ts +++ b/packages/codev/src/__tests__/update.test.ts @@ -120,6 +120,12 @@ describe('update command', () => { ).toBe(true); expect(result.newFiles).toContain('.codex/skills/afx/'); expect(result.newFiles).not.toContain('.codex/skills/arch-init/'); + // Spec 1307: existing projects get /arch-save via `codev update`, which is + // the path every current adopter takes — init only covers new ones. + expect(result.newFiles).toContain('.codex/skills/arch-save/'); + expect( + fs.existsSync(path.join(projectDir, '.codex', 'skills', 'arch-save', 'SKILL.md')) + ).toBe(true); }); it('should return UpdateResult from update()', async () => { diff --git a/packages/codev/src/agent-farm/__tests__/send-buffer.test.ts b/packages/codev/src/agent-farm/__tests__/send-buffer.test.ts index e3c174d86..1072bf0c1 100644 --- a/packages/codev/src/agent-farm/__tests__/send-buffer.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-buffer.test.ts @@ -44,8 +44,8 @@ describe('SendBuffer', () => { buf = new SendBuffer({ idleThresholdMs: 3000, maxBufferAgeMs: 10_000 }); }); - afterEach(() => { - buf.stop(); + afterEach(async () => { + await buf.stop(); // stop() is async (Spec 1307); await before real timers vi.useRealTimers(); }); @@ -180,7 +180,7 @@ describe('SendBuffer', () => { expect(log).toHaveBeenCalledWith('WARN', expect.stringContaining('Discarding')); }); - it('stop() delivers all remaining messages (force flush)', () => { + it('stop() delivers all remaining messages (force flush)', async () => { const session = makeSession(false); // not idle — normally wouldn't deliver const deliver = vi.fn().mockReturnValue(0); const log = vi.fn(); @@ -190,7 +190,7 @@ describe('SendBuffer', () => { buf.enqueue(makeMsg('sess-1')); // Stop forces delivery of everything - buf.stop(); + await buf.stop(); expect(deliver).toHaveBeenCalledTimes(2); expect(buf.pendingCount).toBe(0); @@ -279,4 +279,82 @@ describe('SendBuffer', () => { expect(buf.pendingCount).toBe(0); }); }); + + describe('stop() awaits outstanding flush submissions (Spec 1307)', () => { + it('does not resolve until the injected submit settles', async () => { + // Codex regression: once the drain goes through submitToSession, a flush + // batch can be queued behind an in-flight write. If stop() returns before + // that submission settles, graceful shutdown tears down terminals and the + // buffered message — accepted for delivery — is lost. stop() must await. + const session = makeSession(/* idle */ true); + const deliver = vi.fn(() => 0); + const log = vi.fn(); + + // An injected submit that runs the batch but only settles when released. + let release!: () => void; + const gate = new Promise(resolve => { release = resolve; }); + const submit = vi.fn((_id: string, write: () => number) => { + write(); + return gate; + }); + + buf.start(() => session, deliver, log, submit); + buf.enqueue(makeMsg('sess-1')); + + let stopped = false; + const stopping = buf.stop().then(() => { stopped = true; }); + + // The batch has been written but the submission has not settled. + expect(submit).toHaveBeenCalledTimes(1); + expect(deliver).toHaveBeenCalledTimes(1); + // Flush enough microtasks/timers that stop()'s drain WOULD resolve if it + // were not actually waiting. `await Promise.resolve()` gave only one tick + // — too few for the chain — so the test passed even with the fix reverted + // (Claude, phase-3 confirm). advanceTimersByTimeAsync drains the queue. + await vi.advanceTimersByTimeAsync(0); + await vi.advanceTimersByTimeAsync(1000); + expect(stopped).toBe(false); // stop() must still be waiting + + release(); + await stopping; + expect(stopped).toBe(true); // and resolves once the submission does + }); + + it('awaits a periodic-flush submission still queued at stop (Codex)', async () => { + // The deeper case: a periodic flush(false) hands a batch to submit and + // deletes its buffer entry immediately. If that submission is still queued + // behind the lock when stop() runs, stop() finds an EMPTY buffer — so it + // must await instance-tracked outstanding submissions, not just the ones + // its own final flush(true) starts. + const session = makeSession(/* idle */ true); + const deliver = vi.fn(() => 0); + const log = vi.fn(); + let release!: () => void; + const gate = new Promise(resolve => { release = resolve; }); + const submit = vi.fn((_id: string, write: () => number) => { write(); return gate; }); + + buf.start(() => session, deliver, log, submit); + buf.enqueue(makeMsg('sess-1')); + + // Periodic flush drives the submission and clears the buffer. + await vi.advanceTimersByTimeAsync(600); + expect(submit).toHaveBeenCalledTimes(1); + expect(buf.pendingCount).toBe(0); // buffer already empty + + // stop() must still block on the un-settled periodic submission. + let stopped = false; + const stopping = buf.stop().then(() => { stopped = true; }); + await vi.advanceTimersByTimeAsync(1000); + expect(stopped).toBe(false); + + release(); + await stopping; + expect(stopped).toBe(true); + }); + + it('resolves promptly when nothing is buffered', async () => { + buf.start(() => makeSession(true), vi.fn(() => 0), vi.fn(), (_id, w) => { w(); return Promise.resolve(); }); + await expect(buf.stop()).resolves.toBeUndefined(); + }); + }); }); diff --git a/packages/codev/src/agent-farm/__tests__/send.test.ts b/packages/codev/src/agent-farm/__tests__/send.test.ts index fd5072919..a31a0fa5d 100644 --- a/packages/codev/src/agent-farm/__tests__/send.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send.test.ts @@ -68,7 +68,7 @@ vi.mock('node:fs', async () => { import { tmpdir } from 'node:os'; import { send, detectWorkspaceRoot } from '../commands/send.js'; -import { fatal } from '../utils/logger.js'; +import { fatal, logger } from '../utils/logger.js'; // ============================================================================ // Helpers @@ -268,6 +268,67 @@ describe('send command', () => { }); }); + // ========================================================================= + // --delay (Spec 1307) + // ========================================================================= + + describe('--delay', () => { + it('passes deliverAfter through to the client', async () => { + // The CLI->client hop for --delay. Without this assertion, dropping + // `deliverAfter: options.delay` from send.ts leaves every other test + // green while --delay silently degrades to an immediate send. + await send({ builder: 'builder-spir-109', message: 'later', delay: 15 }); + + expect(mockSendMessage).toHaveBeenCalledWith( + 'builder-spir-109', + 'later', + expect.objectContaining({ deliverAfter: 15 }), + ); + }); + + it('omits deliverAfter when no delay is given', async () => { + await send({ builder: 'builder-spir-109', message: 'now' }); + + expect(mockSendMessage).toHaveBeenCalledWith( + 'builder-spir-109', + 'now', + expect.objectContaining({ deliverAfter: undefined }), + ); + }); + + it('passes deliverAfter for every target under --all', async () => { + await send({ all: true, builder: 'broadcast later', delay: 20 }); + + for (const call of mockSendMessage.mock.calls) { + expect(call[2]).toEqual(expect.objectContaining({ deliverAfter: 20 })); + } + expect(mockSendMessage.mock.calls.length).toBeGreaterThan(0); + }); + + it('reports a scheduled send as scheduled, not sent', async () => { + mockSendMessage.mockResolvedValue({ + ok: true, resolvedTo: 'builder-spir-109', scheduled: true, + }); + + await send({ builder: 'builder-spir-109', message: 'later', delay: 15 }); + + const messages = vi.mocked(logger.success).mock.calls.map(c => String(c[0])); + expect(messages.some(m => /scheduled/i.test(m))).toBe(true); + expect(messages.some(m => /^Message sent to/.test(m))).toBe(false); + }); + + it('reports a buffered send as queued, not sent', async () => { + mockSendMessage.mockResolvedValue({ + ok: true, resolvedTo: 'builder-spir-109', deferred: true, + }); + + await send({ builder: 'builder-spir-109', message: 'hi' }); + + const messages = vi.mocked(logger.success).mock.calls.map(c => String(c[0])); + expect(messages.some(m => /queued/i.test(m))).toBe(true); + }); + }); + describe('error handling', () => { it('throws when Tower is not running', async () => { mockIsRunning.mockResolvedValue(false); diff --git a/packages/codev/src/agent-farm/__tests__/spec-1307-arch-save-skill.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1307-arch-save-skill.test.ts new file mode 100644 index 000000000..564264168 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/spec-1307-arch-save-skill.test.ts @@ -0,0 +1,146 @@ +/** + * `/arch-save` skill drift + content guard (Spec 1307, phase 2). + * + * Mirrors `spec-1134-arch-init-skill.test.ts`. Two distinct guards, and the + * distinction is the reason this file exists: + * + * - `skill-parity.test.ts` compares Claude against Codex *within* a tree. It + * does NOT compare our instance (`.claude/`) against the shipped skeleton + * (`codev-skeleton/.claude/`), so the classic "edited codev/ and forgot + * codev-skeleton/" drift passes it silently. That is exactly the failure the + * repo's own arch-critical rules warn about, and it ships a stale skill to + * every adopter while looking green here. + * - The content assertions pin the statements the plan required the doc to + * make. A skill is a document, so "it exists and is identical everywhere" is + * only half of correct — identical copies of a doc missing its load-bearing + * warning are still wrong. + * + * Phase 2's acceptance criterion was "all four copies identical". It was + * verified by hand with md5 and NOT guarded by a test until review pointed that + * out — a one-time check is not a guard. + */ + +import { describe, expect, it } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +const repoRoot = path.resolve(__dirname, '..', '..', '..', '..', '..'); + +const COPIES = { + 'instance/.claude': path.join(repoRoot, '.claude', 'skills', 'arch-save', 'SKILL.md'), + 'instance/.codex': path.join(repoRoot, '.codex', 'skills', 'arch-save', 'SKILL.md'), + 'skeleton/.claude': path.join(repoRoot, 'codev-skeleton', '.claude', 'skills', 'arch-save', 'SKILL.md'), + 'skeleton/.codex': path.join(repoRoot, 'codev-skeleton', '.codex', 'skills', 'arch-save', 'SKILL.md'), +} as const; + +describe('Spec 1307 — /arch-save ships in all four trees', () => { + it.each(Object.entries(COPIES))('exists: %s', (_label, file) => { + expect(fs.existsSync(file)).toBe(true); + }); + + it('is byte-identical across all four copies (drift guard)', () => { + const [first, ...rest] = Object.values(COPIES).map(f => fs.readFileSync(f, 'utf-8')); + for (const other of rest) { + expect(other).toBe(first); + } + }); + + it('guards instance-vs-skeleton drift specifically', () => { + // Called out separately because skill-parity.test.ts cannot catch it: an + // edit applied to both providers in the instance but to neither in the + // skeleton passes provider parity in both contexts and still ships stale. + expect(fs.readFileSync(COPIES['instance/.claude'], 'utf-8')).toBe( + fs.readFileSync(COPIES['skeleton/.claude'], 'utf-8'), + ); + }); +}); + +describe('Spec 1307 — required content', () => { + const text = () => fs.readFileSync(COPIES['skeleton/.claude'], 'utf-8'); + + it('addresses architect:, never bare architect', () => { + expect(text()).toContain('architect:'); + // The reason must travel with the rule — a sibling architect clearing + // main's terminal is the worst outcome this skill can produce. + expect(text()).toMatch(/never bare `architect`/); + }); + + it('uses --raw and explains why not the escape channel', () => { + expect(text()).toContain("--raw '/clear'"); + expect(text()).toMatch(/escape route writes a bare ESC and discards the/); + }); + + it('states why the state write must precede the clear', () => { + expect(text()).toMatch(/context that knows what to write is the one about to be destroyed/); + }); + + it('requires pruning, not just appending', () => { + expect(text()).toMatch(/save that only appends has not done its job/); + expect(text()).toMatch(/Prune by pointer, never by deletion/); + }); + + it('carries the owner-direction rule with an override carve-out', () => { + expect(text()).toMatch(/Do not invoke this\s+autonomously mid-task/); + expect(text()).toMatch(/If the owner tells\s+you to run it, run it/); + }); + + it('documents the manual re-send recovery', () => { + expect(text()).toContain("--raw '/arch-init '"); + expect(text()).toMatch(/Nothing is lost/); + }); + + it('does NOT claim Tower waits for the clear to land', () => { + // Tower waits out a delay; it does not observe the result. An earlier draft + // said "delivers it after the clear has landed", which promises an + // observation the system never makes — the exact kind of overclaim that + // sends a reader looking for a guarantee that is not there. + expect(text()).not.toMatch(/after the clear has landed/); + expect(text()).toMatch(/Tower does not know whether the clear landed/); + }); + + it('tells the architect not to end its turn if scheduling the re-init fails', () => { + // The gap review found: step 4 queues the /clear but it only takes effect + // when the turn ends, so a step-5 failure is still recoverable — unless the + // architect ends its turn anyway, which converts it into a cleared session + // with no re-init scheduled and nobody informed. + expect(text()).toMatch(/If this send fails, do not end your turn/); + }); + + it('tells the reader what a non-executing /clear looks like', () => { + expect(text()).toMatch(/literal text on the front of the next message/); + }); + + it('requires a MONITORS line even when nothing is armed', () => { + expect(text()).toContain('MONITORS:'); + expect(text()).toMatch(/none armed/); + }); + + it('orders the post-clear monitor steps reconcile-then-rearm', () => { + const body = text(); + expect(body.indexOf('Reconcile monitors')).toBeGreaterThanOrEqual(0); + expect(body.indexOf('Then re-arm')).toBeGreaterThan(body.indexOf('Reconcile monitors')); + }); +}); + +describe('Spec 1307 — /arch-init no longer documents a competing procedure', () => { + const archInit = () => + fs.readFileSync( + path.join(repoRoot, 'codev-skeleton', '.claude', 'skills', 'arch-init', 'SKILL.md'), + 'utf-8', + ); + + it('points at /arch-save as the packaged path', () => { + expect(archInit()).toContain('/arch-save'); + }); + + it('keeps the manual path documented as the Tower-unavailable fallback', () => { + expect(archInit()).toMatch(/fallback when\s+Tower is unavailable/); + }); + + it('shows /arch-save in the refresh loop diagram', () => { + // The diagram is what a reader skims; leaving it manual-only contradicts + // the prose two paragraphs below it. + const loop = archInit().slice(archInit().indexOf('/arch-init (recover)')); + expect(loop.slice(0, 400)).toContain('/arch-save'); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/spec-1307-send-delay.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1307-send-delay.test.ts new file mode 100644 index 000000000..8d159b20d --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/spec-1307-send-delay.test.ts @@ -0,0 +1,545 @@ +/** + * `afx send --delay` — Tower-side deferred delivery (Spec 1307, phase 1). + * + * The tests that matter here are the ORDERING ones. `--delay` is otherwise a + * thin scheduling parameter, but it introduces a second delivery path alongside + * the existing typing-aware `SendBuffer`, and the seam between them is where + * this feature can silently destroy work: + * + * T+0 /clear sent → user typing → BUFFERED (up to 60s) + * T+15 /arch-init due → written directly → LANDS FIRST + * T+40 buffer flushes → /clear lands → wipes the recovered context + * + * That inversion is not recoverable by re-sending (the re-send re-runs the + * race), so it is the one hazard in Spec 1307's design that had to be designed + * out rather than accepted. `hasPending` is what closes it. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { SendBuffer, type BufferedMessage } from '../servers/send-buffer.js'; +import { + scheduleDelayedSend, + shutdownDelayedSends, + pendingDelayedSendCount, + validateDelaySeconds, + MAX_DELAY_SECONDS, +} from '../servers/delayed-send.js'; + +// ============================================================================ +// Fakes +// ============================================================================ + +/** Minimal stand-in for PtySession: only what the delivery path touches. */ +class FakeSession { + writes: string[] = []; + writable = true; + private lastInputAt: number; + + constructor(opts?: { lastInputAt?: number }) { + this.lastInputAt = opts?.lastInputAt ?? 0; + } + + write(data: string): void { + this.writes.push(data); + } + + isUserIdle(thresholdMs: number): boolean { + return Date.now() - this.lastInputAt >= thresholdMs; + } + + /** Simulate the user typing right now. */ + type(): void { + this.lastInputAt = Date.now(); + } + + /** Simulate the user having stopped typing long enough to count as idle. */ + goIdle(): void { + this.lastInputAt = 0; + } +} + +function bufferedMessage(sessionId: string, text: string): BufferedMessage { + return { + sessionId, + formattedMessage: text, + noEnter: false, + timestamp: Date.now(), + broadcastPayload: { + type: 'message', + from: { project: 'p', agent: 'architect' }, + to: { project: 'p', agent: 'architect' }, + content: text, + metadata: {}, + timestamp: new Date().toISOString(), + }, + logMessage: `sent ${text}`, + }; +} + +// ============================================================================ +// Delay validation +// ============================================================================ + +describe('validateDelaySeconds', () => { + it('accepts a whole number of seconds inside the bound', () => { + expect(validateDelaySeconds(1)).toBeNull(); + expect(validateDelaySeconds(15)).toBeNull(); + expect(validateDelaySeconds(MAX_DELAY_SECONDS)).toBeNull(); + }); + + it('rejects zero and negatives', () => { + expect(validateDelaySeconds(0)).toMatch(/greater than zero/); + expect(validateDelaySeconds(-5)).toMatch(/greater than zero/); + }); + + it('rejects non-integers', () => { + expect(validateDelaySeconds(1.5)).toMatch(/whole number/); + }); + + it('rejects NaN', () => { + // The case a naive `value > 0` check lets through: NaN fails every + // comparison, so it would reach setTimeout and fire IMMEDIATELY — silently + // converting a delayed send into an instant one. + expect(validateDelaySeconds(NaN)).toMatch(/whole number/); + }); + + it('rejects Infinity', () => { + expect(validateDelaySeconds(Infinity)).toMatch(/whole number/); + }); + + it('rejects values above the maximum', () => { + expect(validateDelaySeconds(MAX_DELAY_SECONDS + 1)).toMatch(/at most/); + }); + + it('rejects non-numbers', () => { + expect(validateDelaySeconds('15')).toMatch(/whole number/); + expect(validateDelaySeconds(null)).toMatch(/whole number/); + expect(validateDelaySeconds(undefined)).toMatch(/whole number/); + }); +}); + +// ============================================================================ +// Scheduling and shutdown +// ============================================================================ + +describe('scheduleDelayedSend', () => { + beforeEach(() => { + vi.useFakeTimers(); + shutdownDelayedSends(); + }); + + afterEach(() => { + shutdownDelayedSends(); + vi.useRealTimers(); + }); + + it('does not deliver before the delay elapses', () => { + const deliver = vi.fn(); + scheduleDelayedSend(15, 'term-1', deliver); + + vi.advanceTimersByTime(14_000); + expect(deliver).not.toHaveBeenCalled(); + }); + + it('delivers once the delay elapses', async () => { + const deliver = vi.fn(); + scheduleDelayedSend(15, 'term-1', deliver); + + // Async advance: delivery runs through the per-terminal chain, so the + // callback fires in a microtask rather than synchronously in the timer. + await vi.advanceTimersByTimeAsync(15_000); + expect(deliver).toHaveBeenCalledTimes(1); + }); + + it('delivers exactly once', async () => { + const deliver = vi.fn(); + scheduleDelayedSend(5, 'term-1', deliver); + + await vi.advanceTimersByTimeAsync(60_000); + expect(deliver).toHaveBeenCalledTimes(1); + }); + + it('deregisters after delivery, leaving no phantom pending send', async () => { + scheduleDelayedSend(5, 'term-1', () => {}); + expect(pendingDelayedSendCount()).toBe(1); + + await vi.advanceTimersByTimeAsync(5_000); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('deregisters even when delivery throws', async () => { + scheduleDelayedSend(5, 'term-1', () => { + throw new Error('delivery blew up'); + }); + + await expect(vi.advanceTimersByTimeAsync(5_000)).resolves.not.toThrow(); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('survives a rejected async delivery without an unhandled rejection', async () => { + // One undeliverable message must not be able to take Tower down. + scheduleDelayedSend(5, 'term-1', async () => { + throw new Error('async delivery blew up'); + }); + + await vi.advanceTimersByTimeAsync(5_000); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('tracks several pending sends independently', async () => { + scheduleDelayedSend(5, 'term-1', () => {}); + scheduleDelayedSend(10, 'term-2', () => {}); + expect(pendingDelayedSendCount()).toBe(2); + + await vi.advanceTimersByTimeAsync(5_000); + expect(pendingDelayedSendCount()).toBe(1); + + await vi.advanceTimersByTimeAsync(5_000); + expect(pendingDelayedSendCount()).toBe(0); + }); +}); + +describe('shutdownDelayedSends', () => { + beforeEach(() => { + vi.useFakeTimers(); + shutdownDelayedSends(); + }); + + afterEach(() => { + shutdownDelayedSends(); + vi.useRealTimers(); + }); + + it('DROPS pending sends rather than flushing them', async () => { + // The deliberate disagreement with SendBuffer.stop(), which flushes. A + // delayed message's timing was chosen against a world a restart has already + // invalidated — flushing would land `/arch-init` in a session that was + // never cleared. Dropping is recoverable by re-sending. + const deliver = vi.fn(); + scheduleDelayedSend(15, 'term-1', deliver); + + const dropped = shutdownDelayedSends(); + + expect(dropped).toBe(1); + await vi.advanceTimersByTimeAsync(60_000); + expect(deliver).not.toHaveBeenCalled(); + }); + + it('reports how many were dropped so shutdown can log it', () => { + scheduleDelayedSend(5, 'a', () => {}); + scheduleDelayedSend(5, 'b', () => {}); + scheduleDelayedSend(5, 'c', () => {}); + + expect(shutdownDelayedSends()).toBe(3); + }); + + it('leaves no timers behind', () => { + scheduleDelayedSend(5, 'term-1', () => {}); + shutdownDelayedSends(); + + expect(pendingDelayedSendCount()).toBe(0); + expect(vi.getTimerCount()).toBe(0); + }); + + it('is safe to call with nothing pending', () => { + expect(shutdownDelayedSends()).toBe(0); + }); + + it('cancels a delivery whose timer has not fired yet', async () => { + // The generation guard still matters after the chain was removed: a + // delivery can now be waiting on the SUBMISSION LOCK rather than on a + // predecessor in this module, and shutdown must still stop it. The + // observable case that remains here is the simpler one — a scheduled send + // whose due time arrives after shutdown must not deliver. + const ran: string[] = []; + scheduleDelayedSend(5, 'term-1', () => { ran.push('early'); }); + scheduleDelayedSend(30, 'term-1', () => { ran.push('late'); }); + + await vi.advanceTimersByTimeAsync(5_000); + expect(ran).toEqual(['early']); + + shutdownDelayedSends(); + await vi.advanceTimersByTimeAsync(60_000); + + expect(ran).toEqual(['early']); + }); + + it('cancels a delivery whose lock wait outlasts a shutdown (isStillLive)', async () => { + // Codex's finding: the timer-time generation check passes, delivery is + // handed to deliverOrBuffer, and THERE it can block on submitToSession + // behind an in-flight write to the same session. If shutdown fires during + // that block, the write must still be cancelled — the timer check already + // passed, so only the write-time `isStillLive()` re-check catches it. + let liveWhenWritten: boolean | undefined; + scheduleDelayedSend(5, 'term-1', (isStillLive) => { + // Simulate reaching the write site (as deliverOrBuffer does inside the + // lock) only after shutdown has run. + shutdownDelayedSends(); + liveWhenWritten = isStillLive(); + }); + + await vi.advanceTimersByTimeAsync(5_000); + + // The predicate the write site consults reports "not live", so + // deliverOrBuffer's `if (!stillLive()) return 0` skips the write. + expect(liveWhenWritten).toBe(false); + }); + + it('does not cancel deliveries scheduled AFTER a shutdown', async () => { + // The generation guard must not poison the next Tower lifetime. + shutdownDelayedSends(); + + const ran: string[] = []; + scheduleDelayedSend(5, 'term-1', () => { ran.push('after'); }); + + await vi.advanceTimersByTimeAsync(5_000); + expect(ran).toEqual(['after']); + }); +}); + +// ============================================================================ +// FIFO — the ordering guarantee +// ============================================================================ + +describe('per-terminal delivery chain', () => { + beforeEach(() => { + vi.useFakeTimers(); + shutdownDelayedSends(); + }); + + afterEach(() => { + shutdownDelayedSends(); + vi.useRealTimers(); + }); + + it('does NOT serialise on its own — that is the submission lock\'s job now', () => { + // This module used to hold a per-terminal promise chain. Spec 1273's + // `submitToSession` now owns serialisation, and every due message re-enters + // `deliverOrBuffer`, which submits under the lock. One mechanism, not two. + // + // So scheduling alone is deliberately concurrent here. The property that + // two same-terminal deliveries do not interleave is REAL but lives at the + // route level, where the real writes happen — see tower-routes.test.ts + // "ORDERING: two simultaneous delayed sends do not interleave their + // writes", which runs against the actual handler and is mutation-verified. + // Asserting it here again would re-create the replica-test mistake this + // project hit four times. + const started: string[] = []; + scheduleDelayedSend(5, 'term-1', () => { started.push('a'); }); + scheduleDelayedSend(5, 'term-1', () => { started.push('b'); }); + + vi.advanceTimersByTime(5_000); + + // Both timers fired; ordering of the WRITES is the lock's guarantee. + expect(started.sort()).toEqual(['a', 'b']); + }); + + it('does not serialise across different terminals', async () => { + // Chaining is per-terminal; an unrelated session must not be held up. + const order: string[] = []; + const slowDeliver = (label: string) => async () => { + order.push(`start:${label}`); + await new Promise(resolve => setTimeout(resolve, 50)); + order.push(`end:${label}`); + }; + + scheduleDelayedSend(5, 'term-1', slowDeliver('a')); + scheduleDelayedSend(5, 'term-2', slowDeliver('b')); + + await vi.advanceTimersByTimeAsync(5_000); + await vi.advanceTimersByTimeAsync(200); + + // Both started before either finished. + expect(order.slice(0, 2).sort()).toEqual(['start:a', 'start:b']); + }); + + it('delivers by DUE time, not request order, when delays differ', async () => { + // Deliberate and worth pinning: `--delay 30` then `--delay 5` delivers the + // 5s one first, because that is what the caller asked for. The ordering + // guarantee this feature makes is narrower — a delayed message never + // overtakes one already QUEUED for the session — not "request order wins". + const order: string[] = []; + scheduleDelayedSend(30, 'term-1', () => { order.push('long'); }); + scheduleDelayedSend(5, 'term-1', () => { order.push('short'); }); + + await vi.advanceTimersByTimeAsync(30_000); + + expect(order).toEqual(['short', 'long']); + }); + + it('a failing delivery does not strand later messages on the same terminal', async () => { + const order: string[] = []; + scheduleDelayedSend(5, 'term-1', () => { throw new Error('boom'); }); + scheduleDelayedSend(5, 'term-1', () => { order.push('second'); }); + + await vi.advanceTimersByTimeAsync(5_000); + await vi.advanceTimersByTimeAsync(100); + + expect(order).toEqual(['second']); + }); +}); + +describe('SendBuffer.hasPending (per-session FIFO for delayed sends)', () => { + let buffer: SendBuffer; + + beforeEach(() => { + buffer = new SendBuffer(); + }); + + it('reports nothing pending for an untouched session', () => { + expect(buffer.hasPending('term-1')).toBe(false); + }); + + it('reports pending once a message is queued', () => { + buffer.enqueue(bufferedMessage('term-1', '/clear')); + expect(buffer.hasPending('term-1')).toBe(true); + }); + + it('scopes pending state per session', () => { + buffer.enqueue(bufferedMessage('term-1', '/clear')); + expect(buffer.hasPending('term-2')).toBe(false); + }); + + it('reports nothing pending after the queue is flushed', () => { + const session = new FakeSession({ lastInputAt: 0 }); + buffer.enqueue(bufferedMessage('term-1', '/clear')); + buffer.start( + () => session as never, + (s, msg) => { + (s as unknown as FakeSession).write(msg.formattedMessage); + return 0; + }, + () => {}, + ); + + buffer.flush(); + + expect(buffer.hasPending('term-1')).toBe(false); + buffer.stop(); + }); +}); + +describe('delivery ordering under buffering (the inversion this design prevents)', () => { + let buffer: SendBuffer; + let session: FakeSession; + + /** + * The delivery decision as `deliverOrBuffer` makes it: buffer when the user + * is typing, or — for DELAYED deliveries only — when this session already has + * something queued. Otherwise write straight through. + * + * Reproduced here rather than imported because the real function is bound to + * the route's module-level terminal manager and logger. + * + * IMPORTANT — this is a SIMPLIFICATION, not a copy. It omits the shipped + * predicate's interrupt handling entirely. These tests document the FIFO rule + * readably; they are NOT the regression guard for it. That guard lives in + * `tower-routes.test.ts` ("ORDERING: ..."), runs against the real route and + * the real SendBuffer, and is mutation-verified. Review caught this file + * standing in for that one. + * + * `enforceFifo` is scoped to delayed sends on purpose: Spec 1307 requires + * undelayed sends to behave exactly as before, and applying the FIFO term to + * every send changes immediate-path behaviour (it did — three existing + * tower-routes tests caught it). + */ + function deliver(text: string, enforceFifo = false): 'buffered' | 'written' { + const shouldDefer = !session.isUserIdle(3000) + || (enforceFifo && buffer.hasPending('term-1')); + if (shouldDefer) { + buffer.enqueue(bufferedMessage('term-1', text)); + return 'buffered'; + } + session.write(text); + return 'written'; + } + + /** A delayed delivery coming due. */ + function deliverDelayed(text: string): 'buffered' | 'written' { + return deliver(text, true); + } + + beforeEach(() => { + buffer = new SendBuffer(); + session = new FakeSession({ lastInputAt: 0 }); + buffer.start( + () => session as never, + (s, msg) => { + (s as unknown as FakeSession).write(msg.formattedMessage); + return 0; + }, + () => {}, + ); + }); + + afterEach(() => { + buffer.stop(); + }); + + it('writes straight through when the session is idle and nothing is queued', () => { + expect(deliver('hello')).toBe('written'); + expect(session.writes).toEqual(['hello']); + }); + + it('buffers when the user is typing', () => { + session.type(); + expect(deliver('/clear')).toBe('buffered'); + expect(session.writes).toEqual([]); + }); + + it('does NOT let a DELAYED message overtake an earlier buffered one', () => { + // The regression this whole mechanism exists for — the /arch-save sequence. + session.type(); + expect(deliver('/clear')).toBe('buffered'); + + // The user stops typing; 15s later the delayed /arch-init comes due. Without + // the FIFO term it would find the session idle and write directly — landing + // BEFORE the /clear still sitting in the buffer, after which the clear wipes + // the context that just recovered. + session.goIdle(); + expect(deliverDelayed('/arch-init main')).toBe('buffered'); + + // Nothing written yet; both are queued in order. + expect(session.writes).toEqual([]); + + buffer.flush(); + expect(session.writes).toEqual(['/clear', '/arch-init main']); + }); + + it('leaves the IMMEDIATE path unchanged: an idle session is written directly even with a queue', () => { + // The other half of the contract. Spec 1307 requires undelayed sends to + // behave exactly as before; applying the FIFO term to every send changed + // immediate-path behaviour and broke three existing tower-routes tests. + session.type(); + expect(deliver('queued-earlier')).toBe('buffered'); + + session.goIdle(); + expect(deliver('immediate')).toBe('written'); + }); + + it('preserves order across three delayed messages with mixed idle states', () => { + session.type(); + deliverDelayed('first'); + session.goIdle(); + deliverDelayed('second'); + deliverDelayed('third'); + + buffer.flush(); + expect(session.writes).toEqual(['first', 'second', 'third']); + }); + + it('resumes direct writes once the queue has drained', () => { + session.type(); + deliverDelayed('queued'); + + // The buffer only releases once the user is idle — flushing while they are + // still typing correctly holds the message, which is the behaviour the + // inversion test above depends on. + session.goIdle(); + buffer.flush(); + expect(session.writes).toEqual(['queued']); + + expect(deliverDelayed('direct')).toBe('written'); + expect(session.writes).toEqual(['queued', 'direct']); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts index 805953a91..a36c50236 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts @@ -6,11 +6,13 @@ * workspace path decoding, and 404 fallback. */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import http from 'node:http'; import { EventEmitter } from 'node:events'; -import { handleRequest } from '../servers/tower-routes.js'; +import { handleRequest, startSendBuffer, stopSendBuffer } from '../servers/tower-routes.js'; import type { RouteContext } from '../servers/tower-routes.js'; +import { shutdownDelayedSends, pendingDelayedSendCount } from '../servers/delayed-send.js'; +import { submitToSession, resetSubmissionChains } from '../servers/session-submit.js'; // ============================================================================ // Mocks @@ -1629,6 +1631,579 @@ describe('tower-routes', () => { }); }); + // ========================================================================= + // POST /api/send — delayed delivery (Spec 1307) + // ========================================================================= + + // These use their own terminal id. The SendBuffer in tower-routes.ts is + // module-level state shared across this file, and earlier tests deliberately + // leave messages queued for `term-001` — which a delayed send would then + // correctly queue behind, masking what these tests are checking. + describe('POST /api/send with deliverAfter', () => { + beforeEach(() => { + shutdownDelayedSends(); + }); + + afterEach(() => { + shutdownDelayedSends(); + // Drains anything these tests left queued, so the module-level SendBuffer + // does not leak state into later describes. + stopSendBuffer(); + vi.useRealTimers(); + }); + + function idleSession(write: ReturnType) { + return { write, pid: 1234, writable: true, isUserIdle: () => true, composing: false }; + } + + it('responds scheduled:true and writes nothing yet', async () => { + vi.useFakeTimers(); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: '/arch-init main', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 15 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-083', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => idleSession(mockWrite), listSessions: () => [], + }); + const { res, statusCode, body } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + + expect(statusCode()).toBe(200); + const parsed = JSON.parse(body()); + expect(parsed.scheduled).toBe(true); + expect(parsed.deliverAfter).toBe(15); + expect(mockWrite).not.toHaveBeenCalled(); + }); + + it('delivers once the delay elapses', async () => { + vi.useFakeTimers(); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'later', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 15 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-084', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => idleSession(mockWrite), listSessions: () => [], + }); + const { res } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + expect(mockWrite).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(15_000); + expect(mockWrite).toHaveBeenCalled(); + }); + + it('re-fetches the session at delivery and drops gracefully when it is gone', async () => { + // The reason delivery must not close over a PtySession: between scheduling + // and delivery the session can die, and writes to a stale reference go + // nowhere silently. + vi.useFakeTimers(); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'later', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 5 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-085', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + let alive = true; + mockGetTerminalManager.mockReturnValue({ + getSession: () => (alive ? idleSession(mockWrite) : undefined), + listSessions: () => [], + }); + const { res, statusCode } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + expect(statusCode()).toBe(200); + + alive = false; + await expect(vi.advanceTimersByTimeAsync(5_000)).resolves.not.toThrow(); + expect(mockWrite).not.toHaveBeenCalled(); + }); + + it('does not write to a session that became unwritable during the wait', async () => { + vi.useFakeTimers(); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'later', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 5 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-086', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + let writable = true; + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ ...idleSession(mockWrite), writable }), + listSessions: () => [], + }); + const { res } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + writable = false; + await vi.advanceTimersByTimeAsync(5_000); + + expect(mockWrite).not.toHaveBeenCalled(); + }); + + it('rejects an invalid delay before scheduling anything', async () => { + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'x', workspace: '/tmp/ws', + options: { deliverAfter: 0 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-087', workspacePath: '/tmp/ws', agent: 'architect', + }); + const { res, statusCode, body } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + + expect(statusCode()).toBe(400); + expect(JSON.parse(body()).error).toBe('INVALID_PARAMS'); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('rejects NaN delays', async () => { + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'x', workspace: '/tmp/ws', + options: { deliverAfter: NaN }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-088', workspacePath: '/tmp/ws', agent: 'architect', + }); + const { res, statusCode } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + + expect(statusCode()).toBe(400); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('refuses escape combined with a delay rather than silently ignoring one', async () => { + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'x', workspace: '/tmp/ws', + options: { escape: true, deliverAfter: 5 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-089', workspacePath: '/tmp/ws', agent: 'architect', + }); + const { res, statusCode, body } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + + expect(statusCode()).toBe(400); + expect(JSON.parse(body()).message).toMatch(/escape cannot be combined with a delay/); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('AUTHORISES at request time: a refused target never schedules', async () => { + // The security-relevant property. A delayed send must not be able to defer + // an authorization check past the conditions that would fail it — so a + // resolveTarget refusal (e.g. the builder-spoofing check on + // `architect:`) must stop the request before anything is scheduled. + mockParseJsonBody.mockResolvedValue({ + to: 'architect:other', message: 'x', workspace: '/tmp/ws', from: 'aspir-1307', + options: { deliverAfter: 15 }, + }); + // Mirrors what the real resolver returns for this refusal + // (tower-messages.ts:229) — 'NOT_FOUND', not a 'FORBIDDEN' code that does + // not exist. `isResolveError` only checks for `code`, so the assertion + // held either way, but a mock that does not match production is a + // half-truth waiting to mislead the next reader. + mockResolveTarget.mockReturnValue({ + code: 'NOT_FOUND', + message: 'builder aspir-1307 may only address its own spawning architect', + }); + const { res, statusCode } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + + expect(statusCode()).not.toBe(200); + expect(pendingDelayedSendCount()).toBe(0); + }); + + it('defers the interrupt WITH the message rather than firing it now', async () => { + // Otherwise the Ctrl+C lands immediately — interrupting the sender's own + // turn — and the message arrives alone N seconds later. + vi.useFakeTimers(); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'later', workspace: '/tmp/ws', + options: { raw: true, interrupt: true, deliverAfter: 5 }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-091', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => idleSession(mockWrite), listSessions: () => [], + }); + const { res } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + expect(mockWrite).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(5_000); + expect(mockWrite.mock.calls[0][0]).toBe('\x03'); + }); + + it('ORDERING: a delayed message never overtakes an earlier buffered one', async () => { + // The regression guard for the one hazard in Spec 1307 that a manual + // re-send cannot repair. Exercised against the REAL route and the REAL + // module-level SendBuffer — an equivalent test that re-implements the + // shouldDefer predicate locally would keep passing if the shipped + // predicate regressed, which is exactly what review caught. + vi.useFakeTimers(); + const mockWrite = vi.fn(); + let typing = true; + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ + write: mockWrite, pid: 1234, writable: true, + isUserIdle: () => !typing, composing: false, + }), + listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-fifo-001', workspacePath: '/tmp/ws', agent: 'architect', + }); + + // 1. /clear is sent while the user is typing → buffered by Spec 403. + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: '/clear', workspace: '/tmp/ws', options: { raw: true }, + }); + const first = makeRes(); + await handleRequest(makeReq('POST', '/api/send'), first.res, makeCtx()); + expect(JSON.parse(first.body()).deferred).toBe(true); + expect(mockWrite).not.toHaveBeenCalled(); + + // 2. /arch-init is scheduled for +15s. + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: '/arch-init main', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 15 }, + }); + const second = makeRes(); + await handleRequest(makeReq('POST', '/api/send'), second.res, makeCtx()); + expect(JSON.parse(second.body()).scheduled).toBe(true); + + // 3. The user stops typing BEFORE the delayed message comes due. The + // buffer's flush timer is not running yet, so /clear is still queued. + // This isolates the `hasPending` term specifically: the session is + // idle, so only that term can prevent a direct write. + typing = false; + await vi.advanceTimersByTimeAsync(15_000); + + // Nothing has bypassed the queue. + const writesBeforeFlush = mockWrite.mock.calls.map(c => String(c[0])).join(''); + expect(writesBeforeFlush).not.toContain('/arch-init'); + + // 4. Draining the buffer delivers them in the order they were sent. + startSendBuffer(() => {}); + await vi.advanceTimersByTimeAsync(600); + const order = mockWrite.mock.calls.map(c => String(c[0])).join('|'); + expect(order.indexOf('/clear')).toBeGreaterThanOrEqual(0); + expect(order.indexOf('/arch-init')).toBeGreaterThan(order.indexOf('/clear')); + }); + + it('ORDERING: a delayed --interrupt also queues, carrying its Ctrl+C', async () => { + // An immediate --interrupt deliberately bypasses buffering. A DELAYED one + // must not, or it reintroduces the same inversion through a side door. + vi.useFakeTimers(); + const mockWrite = vi.fn(); + let typing = true; + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ + write: mockWrite, pid: 1234, writable: true, + isUserIdle: () => !typing, composing: false, + }), + listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-fifo-002', workspacePath: '/tmp/ws', agent: 'architect', + }); + + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'first', workspace: '/tmp/ws', options: { raw: true }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'urgent', workspace: '/tmp/ws', + options: { raw: true, interrupt: true, deliverAfter: 5 }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + typing = false; + await vi.advanceTimersByTimeAsync(5_000); + + // The Ctrl+C has NOT jumped the queue. + expect(mockWrite.mock.calls.map(c => c[0])).not.toContain('\x03'); + + startSendBuffer(() => {}); + await vi.advanceTimersByTimeAsync(1_000); + const writes = mockWrite.mock.calls.map(c => c[0]); + const ctrlC = writes.indexOf('\x03'); + const firstIdx = writes.findIndex(w => String(w).includes('first')); + const urgentIdx = writes.findIndex(w => String(w).includes('urgent')); + // Order: first → Ctrl+C → urgent. The interrupt lands directly ahead of + // its own payload, not ahead of the whole queue. + expect(firstIdx).toBeGreaterThanOrEqual(0); + expect(ctrlC).toBeGreaterThan(firstIdx); + expect(urgentIdx).toBeGreaterThan(ctrlC); + }); + + it('ORDERING: two simultaneous delayed sends do not interleave their writes', async () => { + // Against the REAL route and the REAL paced writer. The unit-level chain + // test used an artificially async callback, so it proved the chain waits + // for the CALLBACK — not for the writes the callback schedules. + // writeMessageToSession returns after SCHEDULING its pacing and trailing + // Enter, so without waiting out that window two due messages produce + // "firstsecond\r\r" rather than two messages. + vi.useFakeTimers(); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => idleSession(mockWrite), listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-serial-001', workspacePath: '/tmp/ws', agent: 'architect', + }); + + for (const text of ['first', 'second']) { + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: text, workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 5 }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + } + + await vi.advanceTimersByTimeAsync(5_000); + await vi.advanceTimersByTimeAsync(2_000); + + const writes = mockWrite.mock.calls.map(c => String(c[0])); + const firstIdx = writes.findIndex(w => w.includes('first')); + const secondIdx = writes.findIndex(w => w.includes('second')); + + expect(firstIdx).toBeGreaterThanOrEqual(0); + expect(secondIdx).toBeGreaterThan(firstIdx); + + // The decisive assertion: everything belonging to the FIRST message — + // including its trailing Enter — lands before the second begins. An + // Enter appearing after 'second' would mean the writes interleaved. + const enterAfterFirst = writes.findIndex((w, i) => i > firstIdx && w === '\r'); + expect(enterAfterFirst).toBeGreaterThan(firstIdx); + expect(enterAfterFirst).toBeLessThan(secondIdx); + }); + + it('ORDERING: a delayed send due MID-FLUSH does not write into the flush', async () => { + // The window `hasPending` used to miss. flush() drops a session's queue as + // soon as it has SCHEDULED its paced writes, so between that moment and + // the trailing Enter landing, the queue looks empty. A delayed /arch-init + // due in that window used to write into the middle of the /clear being + // delivered — yielding "/clear/arch-init main" on one line, so the clear + // never executes at all. + vi.useFakeTimers(); + const mockWrite = vi.fn(); + let typing = true; + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ + write: mockWrite, pid: 1234, writable: true, + isUserIdle: () => !typing, composing: false, + }), + listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-midflush-001', workspacePath: '/tmp/ws', agent: 'architect', + }); + + // The /clear must be long enough that its paced writes span a real + // window: writeMessageToSession spaces lines 10ms apart and adds the + // Enter 80ms after the last one, so 150 lines ≈ 1.57s of writing. A + // short message completes in ~0.1s and the delayed send lands cleanly + // after it — which is why an earlier version of this test passed with + // the guard removed. Mutation testing caught that. + const clearBody = Array.from({ length: 150 }, (_, i) => `CLEAR-${i}`).join('\n'); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: clearBody, + workspace: '/tmp/ws', options: { raw: true }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + // Due at ~1s: after the flush starts (~0.5s), well before it finishes. + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'ARCHINIT', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 1 }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + // User goes idle; the buffer flush starts writing the /clear. + typing = false; + startSendBuffer(() => {}); + await vi.advanceTimersByTimeAsync(600); // flush fires, schedules writes + await vi.advanceTimersByTimeAsync(1_000); // /arch-init comes due MID-write + await vi.advanceTimersByTimeAsync(5_000); // everything settles + + const writes = mockWrite.mock.calls.map(c => String(c[0])); + const joined = writes.join(''); + const archIdx = joined.indexOf('ARCHINIT'); + const lastClearIdx = joined.lastIndexOf('CLEAR-149'); + + expect(archIdx).toBeGreaterThanOrEqual(0); + expect(lastClearIdx).toBeGreaterThanOrEqual(0); + // Every part of the clear lands before the re-orientation begins. + expect(archIdx).toBeGreaterThan(lastClearIdx); + }); + + it('ORDERING: a delayed --interrupt due MID-FLUSH does not split into the flush', async () => { + // Review regression: deleting busyUntil made hasPending queue-only, and a + // delayed --interrupt wrote its Ctrl+C DIRECTLY (outside the lock) before + // its payload. Due mid-flush, that Ctrl+C landed inside the flush's + // stream, separated from its own payload. The fix folds the Ctrl+C into + // the payload's submitToSession reservation, so the whole interrupt+ + // message queues behind the flush as a unit. + vi.useFakeTimers(); + const mockWrite = vi.fn(); + let typing = true; + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ + write: mockWrite, pid: 1234, writable: true, + isUserIdle: () => !typing, composing: false, + }), + listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-midflush-int', workspacePath: '/tmp/ws', agent: 'architect', + }); + + const clearBody = Array.from({ length: 150 }, (_, i) => `CLEAR-${i}`).join('\n'); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: clearBody, workspace: '/tmp/ws', options: { raw: true }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + // A delayed INTERRUPT due mid-flush. + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'URGENT', workspace: '/tmp/ws', + options: { raw: true, interrupt: true, deliverAfter: 1 }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + typing = false; + startSendBuffer(() => {}); + await vi.advanceTimersByTimeAsync(600); + await vi.advanceTimersByTimeAsync(1_000); + await vi.advanceTimersByTimeAsync(5_000); + + const writes = mockWrite.mock.calls.map(c => String(c[0])); + const ctrlCIdx = writes.indexOf('\x03'); + const lastClear = writes.map((w, i) => w.includes('CLEAR-149') ? i : -1).filter(i => i >= 0).pop() ?? -1; + const urgentIdx = writes.findIndex(w => w.includes('URGENT')); + + // The Ctrl+C did not jump into the flush: it lands after the whole clear, + // and directly ahead of its own payload. + expect(lastClear).toBeGreaterThanOrEqual(0); + expect(ctrlCIdx).toBeGreaterThan(lastClear); + expect(urgentIdx).toBeGreaterThan(ctrlCIdx); + }); + + it('CANCELLATION: a delayed send whose lock wait outlasts shutdown does not write', async () => { + // The route-site `stillLive` guard, exercised where it lives. The + // delayed-send unit test only checks the predicate's value; this drives + // the real deliverOrBuffer and asserts the WRITE is skipped. + // + // Window: the delayed timer fires (generation check passes), delivery + // enters deliverOrBuffer and calls submitToSession, which QUEUES behind an + // occupier already holding this session's lock. Shutdown then bumps the + // generation. When the lock frees, the guard inside the reservation sees + // stillLive() === false and returns without writing. + vi.useFakeTimers(); + resetSubmissionChains(); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ + write: mockWrite, pid: 1234, writable: true, + isUserIdle: () => true, composing: false, + }), + listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-cancel-lock', workspacePath: '/tmp/ws', agent: 'architect', + }); + + // Occupy the session's lock for 10s so any later submission queues behind it. + void submitToSession('term-cancel-lock', () => 10_000); + + // A delayed send due at 1s — it will queue behind the occupier. + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'CANARYMSG', workspace: '/tmp/ws', + options: { raw: true, deliverAfter: 1 }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx()); + + await vi.advanceTimersByTimeAsync(1_000); // delayed timer fires, queues on the lock + shutdownDelayedSends(); // shutdown while it waits + await vi.advanceTimersByTimeAsync(15_000); // occupier frees; queued delivery runs its guard + + // The guard skipped the write: CANARYMSG never reached the session. + const wrote = mockWrite.mock.calls.map(c => String(c[0])).join(''); + expect(wrote).not.toContain('CANARYMSG'); + }); + + it('logs a write that throws instead of swallowing it (Codex/Claude PR review)', async () => { + // A torn-down session can make the write throw. The submission lock's + // callers only .catch to keep Tower alive; without logging here the drop + // is silent, which is exactly the delivery-outcome the log must record. + vi.useFakeTimers(); + const ctxLog = vi.fn(); + const throwingWrite = vi.fn(() => { throw new Error('session gone'); }); + mockGetTerminalManager.mockReturnValue({ + getSession: () => ({ + write: throwingWrite, pid: 1234, writable: true, + isUserIdle: () => true, composing: false, + }), + listSessions: () => [], + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-throw', workspacePath: '/tmp/ws', agent: 'architect', + }); + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'x', workspace: '/tmp/ws', options: { raw: true }, + }); + await handleRequest(makeReq('POST', '/api/send'), makeRes().res, makeCtx({ log: ctxLog })); + await vi.advanceTimersByTimeAsync(200); + + const errorLogged = ctxLog.mock.calls.some( + c => c[0] === 'ERROR' && String(c[1]).includes('write threw'), + ); + expect(errorLogged).toBe(true); + }); + + it('leaves undelayed sends on the immediate path', async () => { + mockParseJsonBody.mockResolvedValue({ + to: 'architect:main', message: 'now', workspace: '/tmp/ws', options: { raw: true }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-delay-096', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => idleSession(mockWrite), listSessions: () => [], + }); + const { res, body } = makeRes(); + + await handleRequest(makeReq('POST', '/api/send'), res, makeCtx()); + + const parsed = JSON.parse(body()); + expect(parsed.scheduled).toBe(false); + expect(mockWrite).toHaveBeenCalled(); + }); + }); + // ========================================================================= // GET /api/analytics (Spec 456) // ========================================================================= diff --git a/packages/codev/src/agent-farm/cli.ts b/packages/codev/src/agent-farm/cli.ts index 38b871406..9d09b0ff5 100644 --- a/packages/codev/src/agent-farm/cli.ts +++ b/packages/codev/src/agent-farm/cli.ts @@ -452,9 +452,31 @@ export async function runAgentFarm(args: string[]): Promise { .option('--interrupt', 'Send Ctrl+C first') .option('--raw', 'Skip structured message formatting') .option('--no-enter', 'Do not send Enter after message') + .option('--delay ', 'Deliver after N seconds (Tower-side; dropped if Tower restarts)') .action(async (builder, message, options) => { const { send } = await import('./commands/send.js'); try { + // Spec 1307: validated here AND server-side. A bad value does not + // degrade the send — it silently changes when (or whether) the message + // arrives. NaN in particular yields a timer that fires immediately, + // turning a delayed send into an immediate one with no error. + let delay: number | undefined; + if (options.delay !== undefined) { + // Bound imported rather than repeated: a second hardcoded ceiling + // drifts from the server's, and the two disagreeing means the CLI + // accepts a value Tower then rejects. + const { validateDelaySeconds } = await import('./servers/delayed-send.js'); + const parsed = Number(options.delay); + const delayError = validateDelaySeconds(parsed); + if (delayError) { + // Echo what the USER typed, not the parse result. `--delay abc` + // becoming "got 'NaN'" tells them about an intermediate value they + // never entered and cannot search for. + logger.error(`--delay '${options.delay}': ${delayError.replace(/, got .*$/, '')}`); + process.exit(1); + } + delay = parsed; + } await send({ builder, message, @@ -463,6 +485,7 @@ export async function runAgentFarm(args: string[]): Promise { interrupt: options.interrupt, raw: options.raw, noEnter: !options.enter, + delay, }); } catch (error) { logger.error(error instanceof Error ? error.message : String(error)); diff --git a/packages/codev/src/agent-farm/commands/send.ts b/packages/codev/src/agent-farm/commands/send.ts index 624fdb2de..e4f7e5a6d 100644 --- a/packages/codev/src/agent-farm/commands/send.ts +++ b/packages/codev/src/agent-farm/commands/send.ts @@ -203,13 +203,21 @@ async function sendToAll( workspace: string | undefined, from: string, options: SendOptions, -): Promise<{ sent: string[]; failed: string[] }> { +): Promise<{ sent: string[]; scheduled: string[]; deferred: string[]; failed: string[] }> { // Bugfix #826: loadState is workspace-scoped (for the architect read). // Builders are global per state.db; use the detected workspace root as // scope. `process.cwd()` is a safe fallback when detection fails — the // architect read returns [] and `--all` only uses `state.builders`. const state = loadState(detectWorkspaceRoot() ?? process.cwd()); - const results = { sent: [] as string[], failed: [] as string[] }; + // Spec 1307: `scheduled` is tracked separately from `sent`. Reporting a + // delayed fan-out as "Sent" would claim delivery that has not happened — the + // same misreport the single-target path below deliberately avoids. + const results = { + sent: [] as string[], + scheduled: [] as string[], + deferred: [] as string[], + failed: [] as string[], + }; if (state.builders.length === 0) { logger.warn('No active builders found.'); @@ -225,11 +233,21 @@ async function sendToAll( raw: options.raw, noEnter: options.noEnter, interrupt: options.interrupt, + // Spec 1307: each target's delivery is scheduled independently. + deliverAfter: options.delay, }); if (!result.ok) { throw new Error(result.error || 'Unknown error'); } - results.sent.push(builder.id); + // Three distinct outcomes, kept distinct. Classifying a buffered or + // scheduled message as "sent" claims a delivery that has not happened. + if (result.scheduled) { + results.scheduled.push(builder.id); + } else if (result.deferred) { + results.deferred.push(builder.id); + } else { + results.sent.push(builder.id); + } } catch (error) { logger.error(`Failed to send to ${builder.id}: ${error instanceof Error ? error.message : String(error)}`); results.failed.push(builder.id); @@ -310,6 +328,17 @@ export async function send(options: SendOptions): Promise { if (results.sent.length > 0) { logger.success(`Sent to ${results.sent.length} builder(s): ${results.sent.join(', ')}`); } + if (results.scheduled.length > 0) { + logger.success( + `Scheduled for ${results.scheduled.length} builder(s) (+${options.delay}s): ${results.scheduled.join(', ')}`, + ); + logger.info('Pending delayed sends are dropped if Tower restarts.'); + } + if (results.deferred.length > 0) { + logger.success( + `Queued for ${results.deferred.length} builder(s) being typed in: ${results.deferred.join(', ')}`, + ); + } if (results.failed.length > 0) { logger.error(`Failed for ${results.failed.length} builder(s): ${results.failed.join(', ')}`); } @@ -323,13 +352,26 @@ export async function send(options: SendOptions): Promise { raw: options.raw, noEnter: options.noEnter, interrupt: options.interrupt, + deliverAfter: options.delay, }); if (!result.ok) { throw new Error(result.error || 'Unknown error'); } - logger.success(`Message sent to ${result.resolvedTo ?? target}`); + // Report what actually happened. A delayed message has NOT been sent, and + // saying so would hide the one detail that matters when it never arrives. + if (result.scheduled) { + logger.success(`Message scheduled for ${result.resolvedTo ?? target} (+${options.delay}s)`); + logger.info('Pending delayed sends are dropped if Tower restarts.'); + } else if (result.deferred) { + // Buffered because someone is typing in the target terminal (Spec 403). + // Worth saying: the message is accepted but not on screen yet, which + // otherwise looks like a lost send. + logger.success(`Message queued for ${result.resolvedTo ?? target} (target is being typed in)`); + } else { + logger.success(`Message sent to ${result.resolvedTo ?? target}`); + } } catch (error) { fatal(error instanceof Error ? error.message : String(error)); } diff --git a/packages/codev/src/agent-farm/servers/delayed-send.ts b/packages/codev/src/agent-farm/servers/delayed-send.ts new file mode 100644 index 000000000..fbbc5e229 --- /dev/null +++ b/packages/codev/src/agent-farm/servers/delayed-send.ts @@ -0,0 +1,180 @@ +/** + * Delayed message delivery for `afx send --delay` (Spec 1307). + * + * Holds a due-time timer per scheduled message and nothing else. The *decision* + * of how to deliver — write now, or hand to the typing-aware send buffer — is + * deliberately NOT made here: it is re-made at delivery time by the same code + * the immediate path uses. See `deliverOrBuffer` in tower-routes.ts. + * + * ## Why the registry exists at all + * + * A bare `setTimeout` would work until Tower shuts down, at which point the + * process would either hang on a pending timer or exit with a message + * half-scheduled and no record of it. The registry makes shutdown explicit. + * + * ## Shutdown DROPS, it does not flush + * + * This is the one place this module deliberately disagrees with `SendBuffer`, + * whose `stop()` performs a final flush. That is right for the buffer: those + * messages were accepted for *immediate* delivery and merely held back because + * someone was typing, so delivering them late is better than losing them. + * + * A delayed message is the opposite. Its whole content is "deliver this at a + * moment that has not arrived yet", and the moment is chosen relative to a + * world (a session mid-clear, a turn about to end) that a Tower restart has + * already invalidated. Flushing on shutdown would fire `/arch-init` into a + * session that never got cleared, or into one that has moved on to other work. + * Dropping is recoverable — a human re-sends one message — and Spec 1307's + * design explicitly accepts that trade. + */ + +/** A scheduled delivery, retained so shutdown can cancel it. */ +interface PendingDelayedSend { + timer: ReturnType; + /** Terminal this message is bound for. Diagnostics only. */ + terminalId: string; + /** Epoch ms the message becomes due. Diagnostics only. */ + dueAt: number; +} + +const pending = new Set(); + +/* + * NOTE: this module no longer serialises deliveries. It used to hold a + * per-terminal promise chain; Spec 1273's `submitToSession` now owns that, and + * every due message re-enters `deliverOrBuffer`, which submits under the lock. + * One mechanism, not two — per the architect's ruling that this project adopts + * the primitive rather than keeping a rival. + */ + +/** + * Incremented by every shutdown. Each scheduled send captures the value current + * when it was scheduled and re-checks it at delivery. + * + * Clearing the pending timers is not sufficient on its own. A message whose + * timer has already fired is out of `pending` but its delivery may not have + * started yet — it can be waiting on the session's `submitToSession` lock + * behind an in-flight write. That queued delivery would otherwise run AFTER + * shutdown, which is exactly what "shutdown drops pending delayed sends" + * promises it will not. The generation check re-read at delivery time makes + * such an already-scheduled delivery a no-op. + * + * Honest bound: a delivery that has ALREADY begun its write when shutdown fires + * still completes — the lock does not interrupt a write in progress. "Drops on + * shutdown" therefore means "does not START anything new," not "aborts what is + * mid-flight." See the shutdown function. + */ +let generation = 0; + +/** + * Upper bound on `--delay`, in seconds. + * + * One hour. Not a meaningful workflow limit — it exists so a typo (`--delay + * 1500` when 15 was meant) cannot park a message for 25 minutes with no way to + * see or cancel it. Listing and cancelling pending sends are deliberately out + * of scope for Spec 1307, which is exactly why the ceiling matters. + */ +export const MAX_DELAY_SECONDS = 3600; + +/** + * Validate a delay in seconds, returning null when acceptable or an error + * string naming the problem. + * + * `Number.isInteger` rather than a bare comparison chain: `NaN > 0` and + * `NaN <= 0` are both false, so a NaN slips through any single comparison + * written the obvious way and yields a `setTimeout` that fires immediately — + * silently converting a delayed send into an immediate one. Infinity is + * rejected for the same class of reason. + */ +export function validateDelaySeconds(value: unknown): string | null { + if (typeof value !== 'number' || !Number.isInteger(value)) { + return `delay must be a whole number of seconds, got '${String(value)}'`; + } + if (value <= 0) { + return `delay must be greater than zero, got ${value}`; + } + if (value > MAX_DELAY_SECONDS) { + return `delay must be at most ${MAX_DELAY_SECONDS} seconds (1 hour), got ${value}`; + } + return null; +} + +/** + * Schedule `deliver` to run after `delaySeconds`. + * + * The callback is responsible for re-resolving the session and re-deciding how + * to deliver; this module guarantees only *when* it is invoked, and that it is + * invoked at most once. + */ +export function scheduleDelayedSend( + delaySeconds: number, + terminalId: string, + /** + * Invoked when the send comes due. Receives `isStillLive`, which it must + * re-check at the moment it actually writes (inside the submission lock): a + * delivery can acquire the lock only AFTER a shutdown that fired while it + * queued, and the generation check below only guards the moment BEFORE it + * enters the lock. Return value is ignored. + */ + deliver: (isStillLive: () => boolean) => unknown, +): void { + const entry: PendingDelayedSend = { + terminalId, + dueAt: Date.now() + delaySeconds * 1000, + // Assigned below; the object must exist first so the callback can + // deregister itself by identity. + timer: undefined as unknown as ReturnType, + }; + + const scheduledGeneration = generation; + + entry.timer = setTimeout(() => { + // Deregister BEFORE delivering. If delivery throws, the entry must not be + // left behind as a phantom pending send that shutdown would then report. + pending.delete(entry); + + void (async () => { + // Generation re-checked at DELIVERY time, not timer time: delivery now + // queues behind the session's submission lock, so the wait to actually + // write can outlast a shutdown. + if (generation !== scheduledGeneration) return; + try { + // Passed through to the write site, where it is re-checked while the + // lock is held — closing the shutdown-during-lock-wait window. + await deliver(() => generation === scheduledGeneration); + } catch { + // deliverOrBuffer logs a write failure at its own site with terminal + // context; this catch is a last-resort guard so an unexpected throw + // cannot become an unhandled rejection that takes Tower down over one + // undeliverable message. + } + })(); + }, delaySeconds * 1000); + + pending.add(entry); +} + +/** + * Cancel every pending delayed send without delivering. Returns the count of + * still-timing sends dropped, so shutdown can log it rather than losing + * messages silently. + * + * Covers two states: sends still on their timer (cleared here) and sends whose + * timer has fired but whose delivery has not started — invalidated by bumping + * `generation`, which the delivery callback re-checks. A delivery already + * writing when this runs is NOT interrupted; see `generation`'s note. + */ +export function shutdownDelayedSends(): number { + const count = pending.size; + for (const entry of pending) { + clearTimeout(entry.timer); + } + pending.clear(); + generation++; + return count; +} + +/** Number of pending delayed sends. Diagnostics and tests. */ +export function pendingDelayedSendCount(): number { + return pending.size; +} diff --git a/packages/codev/src/agent-farm/servers/send-buffer.ts b/packages/codev/src/agent-farm/servers/send-buffer.ts index cba3c3d61..1d8ceacde 100644 --- a/packages/codev/src/agent-farm/servers/send-buffer.ts +++ b/packages/codev/src/agent-farm/servers/send-buffer.ts @@ -12,6 +12,17 @@ export interface BufferedMessage { sessionId: string; formattedMessage: string; noEnter: boolean; + /** + * Write Ctrl+C immediately before THIS message's payload (Spec 1307). + * + * Only set for a delayed `--interrupt` send that had to queue behind earlier + * buffered messages. Without it such a send would have to choose between + * interrupting (write directly, overtaking the queue) and preserving order + * (queue, losing the interrupt). Carrying the Ctrl+C on the message keeps + * both: the queue drains in order, and the interrupt still lands directly + * ahead of the payload it belongs to. + */ + interruptFirst?: boolean; timestamp: number; broadcastPayload: { type: string; @@ -29,8 +40,25 @@ export type GetSessionFn = (id: string) => PtySession | undefined; export type DeliverFn = (session: PtySession, msg: BufferedMessage, delayOffset?: number) => number; export type LogFn = (level: 'INFO' | 'ERROR' | 'WARN', message: string) => void; +/** + * Reserves a session for the duration of one batch (Spec 1273's submission + * lock, adopted per Spec 1307). + * + * `write` may perform MANY writes and returns the FINAL completion offset, so a + * whole flush drains as ONE reservation with the existing `delayOffset` + * threading intact. That is what stops a direct or delayed send writing into a + * flush that has scheduled its paced writes but not finished them — the + * `busyUntil` bookkeeping this replaces. + * + * Injected rather than imported so this module keeps no dependency on the + * server layer, and so tests can drive it without Tower. + */ +export type SubmitFn = (sessionId: string, write: () => number) => Promise; + const DEFAULT_IDLE_THRESHOLD_MS = 3000; const DEFAULT_MAX_BUFFER_AGE_MS = 60_000; +/** Cap on how long stop() waits for in-flight submissions to drain (Spec 1307). */ +const DEFAULT_DRAIN_TIMEOUT_MS = 5_000; const FLUSH_INTERVAL_MS = 500; export class SendBuffer { @@ -39,12 +67,23 @@ export class SendBuffer { private getSession: GetSessionFn | null = null; private deliver: DeliverFn | null = null; private log: LogFn | null = null; + private submit: SubmitFn = (_id, write) => { write(); return Promise.resolve(); }; + /** + * Every submission started by a flush and not yet settled — periodic AND + * final (Spec 1307). `stop()` awaits these so a periodic `flush(false)` whose + * submission is still queued behind the lock is not lost when the buffer is + * already empty (its buffered entry was deleted the moment the batch was + * handed to `submit`). A per-`flush()`-call list could not see it. + */ + private outstanding = new Set>(); readonly idleThresholdMs: number; readonly maxBufferAgeMs: number; + private readonly drainTimeoutMs: number; - constructor(opts?: { idleThresholdMs?: number; maxBufferAgeMs?: number }) { + constructor(opts?: { idleThresholdMs?: number; maxBufferAgeMs?: number; drainTimeoutMs?: number }) { this.idleThresholdMs = opts?.idleThresholdMs ?? DEFAULT_IDLE_THRESHOLD_MS; this.maxBufferAgeMs = opts?.maxBufferAgeMs ?? DEFAULT_MAX_BUFFER_AGE_MS; + this.drainTimeoutMs = opts?.drainTimeoutMs ?? DEFAULT_DRAIN_TIMEOUT_MS; } /** Buffer a message for deferred delivery. */ @@ -58,28 +97,52 @@ export class SendBuffer { } /** Start the periodic flush timer. Clears any existing timer first. */ - start(getSession: GetSessionFn, deliver: DeliverFn, log: LogFn): void { + start(getSession: GetSessionFn, deliver: DeliverFn, log: LogFn, submit?: SubmitFn): void { if (this.flushTimer) clearInterval(this.flushTimer); this.getSession = getSession; this.deliver = deliver; this.log = log; + // Default runs the batch inline — used by tests that drive flush() directly + // and do not care about cross-path serialisation. + this.submit = submit ?? ((_id, write) => { write(); return Promise.resolve(); }); this.flushTimer = setInterval(() => this.flush(), FLUSH_INTERVAL_MS); } - /** Stop the flush timer and deliver all remaining messages. */ - stop(): void { + /** + * Stop the flush timer and deliver all remaining messages. + * + * Awaits the final flush's submissions (Spec 1307): once the drain goes + * through `submitToSession`, a batch can be queued behind an in-flight write + * and NOT yet delivered when this returns. Graceful shutdown must await this + * before tearing down terminals, or a buffered message accepted for delivery + * is silently lost — the guarantee that held before the lock adoption and had + * to be restored after it. + */ + async stop(): Promise { if (this.flushTimer) { clearInterval(this.flushTimer); this.flushTimer = null; } - // Final flush — deliver everything remaining + // Final flush — deliver everything remaining — then wait for EVERY in-flight + // submission (this flush's and any periodic one still queued behind the + // lock) to land before returning. this.flush(true); + await this.drainOutstanding(); } - /** Check and deliver messages for sessions that are idle or aged out. */ + /** + * Check and deliver messages for sessions that are idle or aged out. + * + * Delivery is fire-and-forget from flush()'s perspective: each batch is + * handed to `submit` and tracked in `outstanding` (Spec 1307). Callers that + * need to wait for delivery — only `stop()` does — drain `outstanding`; flush + * itself does not return a promise, because a returned-but-ignored one is the + * kind of latent trap this project kept tripping over. + */ flush(forceAll = false): void { if (!this.getSession || !this.deliver) return; + for (const [sessionId, messages] of this.buffers) { const session = this.getSession(sessionId); @@ -117,13 +180,24 @@ export class SendBuffer { // Deliver all messages in order, serializing paced writes (Bugfix #584). // Each delivery returns the ms when its writes complete; the next message // starts after that to prevent interleaved lines. - let offset = 0; - for (const msg of messages) { - offset = this.deliver(session, msg, offset); - if (this.log && msg.logMessage) { - this.log('INFO', msg.logMessage); + // Spec 1307: the whole drain is ONE reservation. `write` may perform + // many writes and returns the final offset, so the existing offset + // threading is untouched while nothing else can write into this + // session mid-batch. + const submitted = this.submit(sessionId, () => { + let offset = 0; + for (const msg of messages) { + offset = this.deliver!(session, msg, offset); + if (this.log && msg.logMessage) { + this.log('INFO', msg.logMessage); + } } - } + return offset; + }); + // Track instance-wide so stop() awaits it even if this flush() call has + // long returned (the periodic-flush case). Self-removes on settle. + this.outstanding.add(submitted); + void submitted.catch(() => undefined).finally(() => this.outstanding.delete(submitted)); if (this.log && !forceAll) { const reason = maxAgeExceeded ? 'max age exceeded' : 'user idle'; this.log('INFO', `Delivered ${messages.length} deferred message(s) to session ${sessionId.slice(0, 8)}... (${reason})`); @@ -133,6 +207,39 @@ export class SendBuffer { } } + /** Await every in-flight flush submission (Spec 1307 — used by stop()). */ + private async drainOutstanding(): Promise { + // Snapshot: a submission settling during the await removes itself, and new + // ones cannot appear once the flush timer is stopped. + const drained = Promise.all([...this.outstanding].map(p => p.catch(() => undefined))); + // Bounded: graceful shutdown must not hang if a submission never settles + // (a wedged PTY, a lost shellper). Better to exit having delivered what + // landed in time than to block teardown forever. The paced writes complete + // in well under a second, so this cap is generous. + const timeout = new Promise(resolve => { + const t = setTimeout(resolve, this.drainTimeoutMs); + if (typeof t.unref === 'function') t.unref(); + }); + await Promise.race([drained.then(() => undefined), timeout]); + } + + /** + * Whether this session already has messages waiting (Spec 1307). + * + * Used by the delayed-send path to preserve per-session FIFO. A delayed + * message that finds the session idle would otherwise write straight to the + * PTY and overtake an earlier message still sitting in this buffer — which + * for `/arch-save` means `/arch-init` landing before the `/clear` that was + * sent first, after which the clear destroys the freshly recovered context. + * + * Consulting this makes ordering a property of the queue rather than of + * flush timing. + */ + hasPending(sessionId: string): boolean { + const queue = this.buffers.get(sessionId); + return queue !== undefined && queue.length > 0; + } + /** Number of buffered messages across all sessions (for testing). */ get pendingCount(): number { let count = 0; diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index cb469f753..66d4ccdb5 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -41,14 +41,16 @@ * * ## Exactly what it covers — this is NOT blanket per-session atomicity * - * A lock only serialises writers that take it. Currently that is the `escape` - * and immediate-delivery paths of `/api/send`. Every other PTY writer still + * A lock only serialises writers that take it. That is the `escape` and + * immediate-delivery paths of `/api/send`, and — since Spec 1307 (#1335) — the + * buffer flush and delayed-delivery paths too. Every other PTY writer still * writes directly, and it is worth being precise about why: * - * - `tower-routes.ts` `deliverBufferedMessage` (buffer flush) — NOT covered. - * Adopting it is Spec 1307's work; the batch form - * (`write` performing the whole drain and returning the final offset) is - * supported and tested, so no API change is needed when they wire it. + * - `tower-routes.ts` `deliverBufferedMessage` (buffer flush) — COVERED as of + * Spec 1307: the whole drain is one reservation via the batch form (`write` + * performing the drain and returning the final offset), which needed no API + * change here. (This bullet previously said "NOT covered; adopting it is + * Spec 1307's work" — that work is #1335.) * - `tower-cron.ts` cron delivery — NOT covered, and RE-VERIFIED against * #1143's rewrite of that region rather than assumed. `deliverMessage` * still calls `writeMessageToSession` directly (`tower-cron.ts:338`), so a diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index a5fad7a21..7831e8fdf 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -52,6 +52,7 @@ import { SendBuffer } from './send-buffer.js'; import type { BufferedMessage } from './send-buffer.js'; import type { PtySession } from '../../terminal/pty-session.js'; import { writeMessageToSession, writeEscapeToSession } from './message-write.js'; +import { scheduleDelayedSend, validateDelaySeconds } from './delayed-send.js'; import { submitToSession } from './session-submit.js'; import { getKnownWorkspacePaths, @@ -119,7 +120,20 @@ const sendBuffer = new SendBuffer(); /** Deliver a buffered message to a session (write + broadcast + log). * Returns the ms timestamp when all writes complete (for serialization). */ function deliverBufferedMessage(session: PtySession, msg: BufferedMessage, delayOffset = 0): number { - const endTime = writeMessageToSession(session, msg.formattedMessage, msg.noEnter, delayOffset); + let offset = delayOffset; + // Spec 1307: a queued delayed `--interrupt` carries its Ctrl+C, written just + // ahead of its own payload rather than ahead of the whole queue. The 100ms + // gap mirrors the immediate path's pause between the interrupt and the text. + if (msg.interruptFirst) { + if (offset === 0) { + session.write('\x03'); + } else { + const at = offset; + setTimeout(() => session.write('\x03'), at); + } + offset += 100; + } + const endTime = writeMessageToSession(session, msg.formattedMessage, msg.noEnter, offset); broadcastMessage(msg.broadcastPayload as Parameters[0]); return endTime; } @@ -130,12 +144,24 @@ export function startSendBuffer(log: (level: 'INFO' | 'ERROR' | 'WARN', message: (id) => getTerminalManager().getSession(id), deliverBufferedMessage, log, + // Spec 1307: drain each session's batch under Spec 1273's submission lock, + // so a direct or delayed send cannot write into a flush that has scheduled + // its paced writes but not finished them. Returns the promise so the + // shutdown flush can be awaited; the catch keeps a throwing batch from + // becoming an unhandled rejection (the periodic flush ignores the return). + (sessionId, write) => + submitToSession(sessionId, write).catch((err) => { + // deliverBufferedMessage's own writes do not throw synchronously, but a + // torn-down session could; log rather than swallow silently, and never + // crash Tower over one batch. + log('ERROR', `Buffered flush submission failed for ${sessionId.slice(0, 8)}...: ${err instanceof Error ? err.message : String(err)}`); + }), ); } /** Stop the send buffer and deliver remaining messages (called from tower-server during shutdown). */ -export function stopSendBuffer(): void { - sendBuffer.stop(); +export async function stopSendBuffer(): Promise { + await sendBuffer.stop(); } // ============================================================================ @@ -1458,6 +1484,35 @@ async function handleSend( const interrupt = options.interrupt === true; const escape = options.escape === true; + // Spec 1307: optional delayed delivery. Validated here as well as at the CLI + // boundary — this is a public HTTP route, so the CLI is not the only caller, + // and an unvalidated value becomes a setTimeout that either fires instantly + // (NaN) or never (Infinity). + let deliverAfter: number | undefined; + if (options.deliverAfter !== undefined && options.deliverAfter !== null) { + const delayError = validateDelaySeconds(options.deliverAfter); + if (delayError) { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'INVALID_PARAMS', message: delayError })); + return; + } + deliverAfter = options.deliverAfter as number; + } + + // `escape` short-circuits before formatting and before the send buffer, by + // design (an interrupt that can be deferred is not an interrupt). Combining it + // with a delay is therefore contradictory rather than merely unsupported, and + // is refused instead of silently ignoring one of the two — a delay that is + // quietly dropped would look like it worked. + if (escape && deliverAfter !== undefined) { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + error: 'INVALID_PARAMS', + message: 'escape cannot be combined with a delay: an ESC keystroke bypasses buffering by design so that it interrupts the CURRENT turn. Send the ESC now, or send a delayed message without escape.', + })); + return; + } + // Resolve the target address to a terminal ID. // Spec 755: pass `from` so architect resolution is sender-affinity-aware // when the sender is a builder. Non-builder senders see unchanged behavior. @@ -1560,57 +1615,245 @@ async function handleSend( }; const logMessage = `Message sent: ${from ?? 'unknown'} → ${result.agent} (terminal ${result.terminalId.slice(0, 8)}...)`; - // Optionally interrupt first — bypass buffering entirely - if (interrupt) { - session.write('\x03'); // Ctrl+C - await new Promise(resolve => setTimeout(resolve, 100)); + // Spec 1307: `--delay` schedules DELIVERY only. Everything above this point — + // target resolution, the builder-spoofing check inside resolveTarget, + // writability, formatting — has already happened at REQUEST time, which is the + // security-relevant half of the design: a delayed send must not be able to + // defer an authorization check past the conditions that would fail it. + if (deliverAfter !== undefined) { + const deliveryContext: DeliveryContext = { + terminalId: result.terminalId, + agent: result.agent, + from, + formattedMessage, + noEnter, + interrupt, + broadcastPayload, + logMessage, + ctx, + // Delayed deliveries queue behind anything already buffered. + enforceFifo: true, + }; + // A due message re-enters deliverOrBuffer, which submits under Spec 1273's + // per-session lock — so serialisation against other writes to this session + // is the lock's job, and this scheduler only owns WHEN delivery starts. + // `stillLive` is re-checked inside the lock so a shutdown during the wait + // for it cancels the write (delayed-send.ts passes the generation check). + scheduleDelayedSend(deliverAfter, result.terminalId, (stillLive) => + deliverOrBuffer({ ...deliveryContext, stillLive })); + ctx.log('INFO', `Message scheduled (+${deliverAfter}s): ${from ?? 'unknown'} → ${result.agent} (terminal ${result.terminalId.slice(0, 8)}...)`); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + ok: true, + terminalId: result.terminalId, + resolvedTo: result.agent, + deferred: false, + scheduled: true, + deliverAfter, + })); + return; + } + + const deferred = await deliverOrBuffer({ + terminalId: result.terminalId, + agent: result.agent, + from, + formattedMessage, + noEnter, + interrupt, + broadcastPayload, + logMessage, + ctx, + // Immediate sends keep their existing behaviour exactly (Spec 1307). + enforceFifo: false, + }); + + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + ok: true, + terminalId: result.terminalId, + resolvedTo: result.agent, + deferred, + scheduled: false, + })); +} + +/** Everything `deliverOrBuffer` needs, captured at request time. */ +interface DeliveryContext { + terminalId: string; + agent: string; + from?: string; + formattedMessage: string; + noEnter: boolean; + interrupt: boolean; + broadcastPayload: Parameters[0]; + logMessage: string; + ctx: RouteContext; + /** + * Whether to queue behind messages already buffered for this session even + * when it looks idle. True only for DELAYED deliveries. + * + * Scoped deliberately rather than applied to every send. An immediate send + * races the 500ms buffer flush at worst, which is existing behaviour and not + * this spec's to change — Spec 1307 requires undelayed sends to be unchanged. + * A delayed send is different in kind: it can come due arbitrarily long after + * a message that is still queued, so "the session is idle right now" says + * nothing about whether it would overtake something. + */ + enforceFifo: boolean; + /** + * Re-checked at the moment of the write, INSIDE the submission reservation — + * for DELAYED deliveries only (Spec 1307). + * + * A delayed delivery can sit behind an in-flight write to this session while + * it waits for the submission lock, and a shutdown can land in that wait. The + * generation check in `delayed-send.ts` fires before the delivery enters the + * lock, so without this second check a message that acquired the lock AFTER + * shutdown would still write — contradicting "shutdown starts nothing new". + * Undefined on the immediate path, which has no shutdown-cancellation notion. + */ + stillLive?: () => boolean; +} + +/** + * Deliver a formatted message: write it now, or hand it to the typing-aware + * send buffer (Spec 403). + * + * Extracted from `handleSend` so the immediate and delayed paths make this + * decision through the SAME code (Spec 1307). A delayed message that wrote + * straight to the PTY would be deciding "is the user typing?" against a world + * observed 15 seconds ago, and — worse — could overtake an earlier message + * still sitting in the buffer. + * + * The session is re-fetched by id rather than captured: between scheduling and + * delivery the session can die, be replaced, or lose its shellper connection, + * and a retained `PtySession` reference would happily absorb writes that go + * nowhere. + * + * @returns whether the message was buffered rather than written now. + * + * The write itself goes through Spec 1273's `submitToSession`, so it is + * submitted — Enter included — before the session's next write begins. Callers + * therefore need no settling wait of their own; "delivered" means delivered. + */ +async function deliverOrBuffer( + delivery: DeliveryContext, +): Promise { + const { + terminalId, agent, from, formattedMessage, noEnter, interrupt, + broadcastPayload, logMessage, ctx, enforceFifo, stillLive, + } = delivery; + + // Re-resolve. For the immediate path this is the same session that was just + // validated; for the delayed path it is the whole point. + const session = getTerminalManager().getSession(terminalId); + if (!session) { + ctx.log('WARN', `Message DROPPED: ${from ?? 'unknown'} → ${agent} (terminal ${terminalId.slice(0, 8)}...): session gone before delivery`); + return false; } + if (!session.writable) { + ctx.log('ERROR', `Message DROPPED: ${from ?? 'unknown'} → ${agent} (terminal ${terminalId.slice(0, 8)}...): terminal not writable (shellper connection down)`); + return false; + } + + // Spec 1307: a DELAYED interrupt must still respect per-session order. An + // immediate `--interrupt` deliberately bypasses buffering ("an interrupt that + // can be deferred is not an interrupt"), but that reasoning does not carry to + // one that was already deferred by N seconds — writing it directly would let + // it overtake messages queued ahead of it. When there IS a queue ahead, the + // Ctrl+C rides along with the message (`interruptFirst`); otherwise it is + // written INSIDE the payload's submission reservation below, never before it. + const queueAhead = enforceFifo && sendBuffer.hasPending(terminalId); // Check if user is idle — deliver immediately or buffer (Spec 403, Bugfix #450) // Defer only when user has typed recently (within idle threshold). // Bugfix #492: removed session.composing check — composing gets stuck true // after non-Enter keystrokes (Ctrl+C, arrows, Tab), causing 60s delays. - const shouldDefer = !interrupt && !session.isUserIdle(sendBuffer.idleThresholdMs); + // + // Spec 1307 adds the `enforceFifo` term, for DELAYED deliveries only: an idle + // session must not be written to directly while earlier messages are still + // queued for it, or the delayed message overtakes them. For `/arch-save` that + // inversion means `/arch-init` landing before its `/clear`, after which the + // clear wipes the context that just recovered — a failure no re-send repairs. + // + // WHAT THIS GUARANTEES, and what it does not: + // `enforceFifo` (this predicate) decides ORDER: a delayed message never + // bypasses one already queued for the session. ATOMICITY — that each + // delivery, Enter included, completes before the next write to that + // session begins — is Spec 1273's `submitToSession`, which every write + // from here goes through, immediate and delayed alike. Order and + // atomicity are separate layers; this term is the first, the lock is the + // second. Together they close the mid-flush interleave (route test + // "ORDERING: ... MID-FLUSH", mutation-verified against the flush's + // submitToSession reservation) and the two-simultaneous-delayed case. + // NOT GUARANTEED — request-order across differing delays: `--delay 5` after + // `--delay 30` lands first, because that is what `--delay` means. + const shouldDefer = queueAhead + || (!interrupt && !session.isUserIdle(sendBuffer.idleThresholdMs)); if (shouldDefer) { - // User is actively typing — buffer for deferred delivery sendBuffer.enqueue({ - sessionId: result.terminalId, + sessionId: terminalId, formattedMessage, noEnter, timestamp: Date.now(), broadcastPayload, logMessage, + // A deferred interrupt carries its Ctrl+C on the message, written just + // ahead of its own payload at flush time rather than ahead of the whole + // queue. Nothing is pre-written, so there is no double-Ctrl+C to guard. + interruptFirst: interrupt ? true : undefined, }); - ctx.log('INFO', `Message deferred (user typing): ${from ?? 'unknown'} → ${result.agent} (terminal ${result.terminalId.slice(0, 8)}...)`); - } else { - // User is idle (or interrupt) — deliver immediately. - // Bugfix #584: paces multi-line output to avoid paste detection. - // - // AWAITED (Spec 1273 verify). `writeMessageToSession` schedules the Enter - // 50–80ms out and returns immediately; responding on that meant a caller's - // `await send(...)` resolved BEFORE its message was submitted. Two sends in - // quick succession then landed in the same composer and were submitted as - // one message — which is how `afx reset` sent - // `/clear### [ARCHITECT INSTRUCTION...` and never cleared anything. - // - // Only the immediate path is awaited. The buffered path above must NOT be: - // a deferred message can sit up to 60s, and awaiting that would hang the - // caller instead of returning `deferred: true`. - await submitToSession(result.terminalId, () => - writeMessageToSession(session, formattedMessage, noEnter), - ); + ctx.log('INFO', `Message deferred (user typing): ${from ?? 'unknown'} → ${agent} (terminal ${terminalId.slice(0, 8)}...)`); + return true; + } + + // Direct delivery, through Spec 1273's submission lock. Everything this + // message writes — an optional Ctrl+C, the payload, its Enter — happens in + // ONE reservation, so nothing else can write to this session mid-delivery and + // the interrupt cannot be separated from the payload it belongs to. + // + // AWAITED: `writeMessageToSession` schedules its Enter 50-80ms out and returns + // immediately, so responding on that return meant an awaited send resolved + // BEFORE its message was submitted — two sends in quick succession landed in + // one composer and were submitted as one. That is how `afx reset` sent + // `/clear### [ARCHITECT INSTRUCTION...` and cleared nothing. Both of Spec + // 1307's paths route through here, so both inherit the guarantee. + let wrote = false; + await submitToSession(terminalId, () => { + // Cancellation is re-checked HERE, holding the lock, not before the wait for + // it: a delayed delivery can acquire the lock only after a shutdown that + // fired while it queued. `stillLive` is undefined on the immediate path. + if (stillLive && !stillLive()) { + // Cancelled by a shutdown that landed while this delayed delivery waited + // for the lock. Logged like every other drop path — a silent return here + // was the one drop this feature did not record (Claude, PR review). + ctx.log('INFO', `Delayed send cancelled at shutdown: ${from ?? 'unknown'} → ${agent} (terminal ${terminalId.slice(0, 8)}...)`); + return 0; + } + try { + let offset = 0; + if (interrupt) { + session.write('\x03'); // Ctrl+C, inside the reservation + offset = 100; // same pause the buffered interruptFirst path uses + } + const endTime = writeMessageToSession(session, formattedMessage, noEnter, offset); + wrote = true; + return endTime; + } catch (err) { + // A write can throw if the session is torn down between the writability + // check and here. Log it — the caller's catch (delayed-send, or the flush + // submit) only swallows to keep Tower alive, and a silently-dropped + // scheduled message is exactly the failure the delivery log must record. + ctx.log('ERROR', `Message DROPPED: ${from ?? 'unknown'} → ${agent} (terminal ${terminalId.slice(0, 8)}...): write threw: ${err instanceof Error ? err.message : String(err)}`); + return 0; + } + }); + if (wrote) { broadcastMessage(broadcastPayload); ctx.log('INFO', logMessage); } - - res.writeHead(200, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify({ - ok: true, - terminalId: result.terminalId, - resolvedTo: result.agent, - deferred: shouldDefer, - })); + return false; } async function handleBrowse(res: http.ServerResponse, url: URL): Promise { diff --git a/packages/codev/src/agent-farm/servers/tower-server.ts b/packages/codev/src/agent-farm/servers/tower-server.ts index a567ea3e7..fe88dd598 100644 --- a/packages/codev/src/agent-farm/servers/tower-server.ts +++ b/packages/codev/src/agent-farm/servers/tower-server.ts @@ -57,6 +57,7 @@ import { setupUpgradeHandler, } from './tower-websocket.js'; import { handleRequest, startSendBuffer, stopSendBuffer } from './tower-routes.js'; +import { shutdownDelayedSends } from './delayed-send.js'; import type { RouteContext } from './tower-routes.js'; import { setCodevConfigNotifier, stopAllCodevConfigWatchers } from './codev-config-watcher.js'; import { getGlobalDb } from '../db/index.js'; @@ -181,8 +182,25 @@ async function gracefulShutdown(signal: string): Promise { if (sessionLogSweepInterval) clearInterval(sessionLogSweepInterval); clearInterval(sseHeartbeatInterval); - // 4b. Flush and stop send buffer (Spec 403) — delivers any deferred messages - stopSendBuffer(); + // 4b. Drop pending delayed sends FIRST (Spec 1307). Ordering is load-bearing: + // this runs before the awaited buffer flush below, not after. If it ran after, + // a delayed timer could fire DURING that await, pass the generation guard + // (not yet bumped), and write or enqueue after shutdown had begun. Dropping + // first bumps the generation up front, so any timer that fires during the + // flush is cancelled at its write site. A delayed message that had ALREADY + // re-entered the buffer before now is a buffered message and is still flushed + // by 4c — this cancels only sends still waiting on their timer. + const droppedDelayed = shutdownDelayedSends(); + if (droppedDelayed > 0) { + log('INFO', `Dropped ${droppedDelayed} pending delayed send(s) — re-send them if still wanted`); + } + + // 4c. Flush and stop the send buffer (Spec 403) — deliver deferred messages. + // Awaited (Spec 1307): the flush drains under the submission lock, so a batch + // can be queued behind an in-flight write. Awaiting here — before the terminal + // teardown below — is what keeps a buffered message accepted for delivery from + // being lost when the process exits. + await stopSendBuffer(); // 5. Stop cron scheduler (Spec 399) shutdownCron(); diff --git a/packages/codev/src/agent-farm/types.ts b/packages/codev/src/agent-farm/types.ts index a32ac1812..b6d1ba6c3 100644 --- a/packages/codev/src/agent-farm/types.ts +++ b/packages/codev/src/agent-farm/types.ts @@ -164,6 +164,17 @@ export interface SendOptions { interrupt?: boolean; // Send Ctrl+C first to ensure prompt is ready raw?: boolean; // Skip structured formatting noEnter?: boolean; // Don't send Enter after message + /** + * Spec 1307: hold in Tower and deliver after this many seconds. Resolution + * and authorization still happen at request time; only delivery is deferred. + * Not persisted — a Tower restart drops pending sends. + * + * Named `delay` here to match the user-facing `--delay` flag; it becomes + * `deliverAfter` at the client and wire layers, where the question is *when + * to deliver* rather than *how long the caller asked to wait*. The two names + * are deliberate, not drift. + */ + delay?: number; } /** diff --git a/packages/core/src/__tests__/tower-client-send.test.ts b/packages/core/src/__tests__/tower-client-send.test.ts new file mode 100644 index 000000000..3a272a0f2 --- /dev/null +++ b/packages/core/src/__tests__/tower-client-send.test.ts @@ -0,0 +1,138 @@ +/** + * `TowerClient.sendMessage` wire contract (Spec 1307). + * + * This suite exists because of a specific gap: `--delay` travels + * CLI → SendOptions → TowerClient → HTTP body → Tower. Every hop except this + * one is covered from `packages/codev`, and this one *cannot* be — the + * agent-farm `tower-client.ts` is a re-export shim that resolves to core's + * built `dist`, so a codev-side test exercises compiled output, not this + * source. + * + * The consequence, before this file existed: deleting `deliverAfter` from the + * request body left all 4059 codev tests green while `--delay` silently + * degraded to an immediate send. A feature whose failure mode is "arrives at + * the wrong time" needs a test that fails when the field stops being sent. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { TowerClient } from '../tower-client.js'; + +/** Captured fetch calls, so assertions can read the actual request body. */ +interface CapturedRequest { + url: string; + body: Record; +} + +let captured: CapturedRequest[] = []; + +function mockFetchReturning(payload: Record, ok = true) { + return vi.fn(async (url: string, init?: { body?: string }) => { + captured.push({ + url: String(url), + body: init?.body ? JSON.parse(init.body) : {}, + }); + return { + ok, + status: ok ? 200 : 500, + json: async () => payload, + text: async () => JSON.stringify(payload), + } as unknown as Response; + }); +} + +describe('TowerClient.sendMessage — delayed delivery wire contract', () => { + beforeEach(() => { + captured = []; + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('puts deliverAfter on the wire when a delay is given', async () => { + vi.stubGlobal('fetch', mockFetchReturning({ ok: true, resolvedTo: 'architect' })); + + const client = new TowerClient(); + await client.sendMessage('architect:main', '/arch-init main', { + raw: true, + deliverAfter: 15, + }); + + expect(captured).toHaveLength(1); + const options = captured[0].body.options as Record; + expect(options.deliverAfter).toBe(15); + expect(options.raw).toBe(true); + }); + + it('omits deliverAfter when no delay is given', async () => { + vi.stubGlobal('fetch', mockFetchReturning({ ok: true, resolvedTo: 'architect' })); + + const client = new TowerClient(); + await client.sendMessage('architect:main', 'now', { raw: true }); + + const options = captured[0].body.options as Record; + expect(options.deliverAfter).toBeUndefined(); + }); + + it('surfaces scheduled from the response', async () => { + vi.stubGlobal('fetch', mockFetchReturning({ + ok: true, resolvedTo: 'architect', scheduled: true, deferred: false, + })); + + const client = new TowerClient(); + const result = await client.sendMessage('architect:main', 'later', { deliverAfter: 15 }); + + expect(result.ok).toBe(true); + expect(result.scheduled).toBe(true); + expect(result.deferred).toBe(false); + }); + + it('surfaces deferred from the response', async () => { + // Tower buffered it because someone is typing in the target terminal. + vi.stubGlobal('fetch', mockFetchReturning({ + ok: true, resolvedTo: 'architect', scheduled: false, deferred: true, + })); + + const client = new TowerClient(); + const result = await client.sendMessage('architect:main', 'hello', {}); + + expect(result.deferred).toBe(true); + expect(result.scheduled).toBe(false); + }); + + it('reports scheduled/deferred as false when the response omits them', async () => { + // An older Tower does not send these fields. They must read as "no", not + // as undefined leaking into a truthiness check downstream. + vi.stubGlobal('fetch', mockFetchReturning({ ok: true, resolvedTo: 'architect' })); + + const client = new TowerClient(); + const result = await client.sendMessage('architect:main', 'hello', {}); + + expect(result.scheduled).toBe(false); + expect(result.deferred).toBe(false); + }); + + it('still carries the other send options alongside a delay', async () => { + vi.stubGlobal('fetch', mockFetchReturning({ ok: true, resolvedTo: 'b1' })); + + const client = new TowerClient(); + await client.sendMessage('b1', 'msg', { + raw: true, noEnter: true, interrupt: true, deliverAfter: 30, + }); + + const options = captured[0].body.options as Record; + expect(options).toMatchObject({ + raw: true, noEnter: true, interrupt: true, deliverAfter: 30, + }); + }); + + it('addresses the send endpoint', async () => { + vi.stubGlobal('fetch', mockFetchReturning({ ok: true, resolvedTo: 'architect' })); + + const client = new TowerClient(); + await client.sendMessage('architect:main', 'x', { deliverAfter: 5 }); + + expect(captured[0].url).toContain('/api/send'); + expect(captured[0].body.to).toBe('architect:main'); + }); +}); diff --git a/packages/core/src/tower-client.ts b/packages/core/src/tower-client.ts index dcc1c6196..ce33154ec 100644 --- a/packages/core/src/tower-client.ts +++ b/packages/core/src/tower-client.ts @@ -669,9 +669,30 @@ export class TowerClient { * can process. Distinct from `interrupt`, which sends Ctrl+C (`\x03`). */ escape?: boolean; + /** + * Spec 1307: hold the message in Tower and deliver it after this many + * seconds. Resolution and authorization still happen at request time — + * only delivery is deferred. + * + * Tower-side rather than a sleeping client because the caller may be the + * session being written to: `/arch-save` sends its own `/clear` and then a + * delayed `/arch-init`, and the process issuing them does not survive the + * clear. Not persisted; a Tower restart drops pending sends. + */ + deliverAfter?: number; }, - ): Promise<{ ok: boolean; resolvedTo?: string; error?: string }> { - const result = await this.request<{ ok: boolean; resolvedTo: string }>( + ): Promise<{ + ok: boolean; + resolvedTo?: string; + /** Tower is holding this for later delivery (`deliverAfter`). */ + scheduled?: boolean; + /** Tower buffered this because the user was typing (Spec 403). */ + deferred?: boolean; + error?: string; + }> { + const result = await this.request<{ + ok: boolean; resolvedTo: string; scheduled?: boolean; deferred?: boolean; + }>( '/api/send', { method: 'POST', @@ -686,6 +707,7 @@ export class TowerClient { noEnter: options?.noEnter, interrupt: options?.interrupt, escape: options?.escape, + deliverAfter: options?.deliverAfter, }, }), }, @@ -695,7 +717,12 @@ export class TowerClient { return { ok: false, error: result.error }; } - return { ok: true, resolvedTo: result.data!.resolvedTo }; + return { + ok: true, + resolvedTo: result.data!.resolvedTo, + scheduled: result.data!.scheduled === true, + deferred: result.data!.deferred === true, + }; } async signalTunnel(action: 'connect' | 'disconnect'): Promise {