[Spec 1307] /arch-save: packaged save→clear→re-init for architect context refresh - #1335
Open
waleedkadous wants to merge 81 commits into
Open
[Spec 1307] /arch-save: packaged save→clear→re-init for architect context refresh#1335waleedkadous wants to merge 81 commits into
waleedkadous wants to merge 81 commits into
Conversation
Claude REQUEST_CHANGES incorporated. Recommended approach changed to write-then-verify: architect writes the state file before invoking the CLI, which validates synchronously and arms only quiesce -> clear -> reorient. Removes receipt polling from Tower, makes 'no clear without a verified save' true by construction in the self path, and shrinks the clear-after-new-work window from 300s to one quiet window. Original nonce/Tower-armed design kept as Approach 1b with rejection reasons. Also: restated the post-clear monitor-stop as pre-clear (enforceable) plus best-effort reconciliation; replaced the '## Monitors' heading gate with a MONITORS: token the adopted template carries verbatim; added the clear-after-new-work hazard to risks/questions/tests; stopped overclaiming --boundary as a recorded human decision in the self path; added quiescence-against-a-live-TUI as a second inherited unknown; recorded the sendRaw vs sendMessage divergence the shared extraction must preserve; fixed the snapshot ordering that write-then-verify would otherwise have broken. Codex consult pending - its lane is down (vendored codex-sdk binary, PR #1309).
Architect design input, evaluated and adopted: the re-orientation payload need not be a typed slash command. It is now plain text with no completion surface, which eliminates the autocomplete hazard rather than mitigating it. The trade is deterministic harness-level skill loading for model-side invocation. Bought back by requiring the payload to be self-sufficient (identity + state-file path inline), so a session that never invokes the arch-init skill still recovers by reading the state file directly. The step that previously had no safe degradation now has two. Consequently the sendRaw-vs-sendMessage divergence largely dissolves: this command's delivery now matches reset's. The raw/escape split still must survive extraction, since /clear is raw-typed and Tower's escape route discards the message body — constraint narrowed to where it bites. Added an explicit worst-case statement: state file, terminal and Tower's record survive every failure mode, so the worst realistic outcome is a live terminal with no identity yet and its state one message away — recoverable manual re-entry, not data loss. Swept nine stale references to the raw-typed payload across Desired State, success criteria, Approach 1, the Critical open question, Security, test 1 and Dependencies.
…pened Owner directives, both incorporated. (1) Pruning is part of the save, as a requirement not guidance. The write step must remove cruft, not merely append: resolved loops deleted, older entries collapsed to pointers at durable artifacts, one-screen order of magnitude — matching the compaction discipline /arch-init already prescribes for manual saves. A save that only appends FAILS its acceptance criteria. Because the save is snapshotted first, growth against the immediate predecessor is computable for free, which makes the append-only failure mode machine-detectable; added as a proxy alongside a size ceiling, with the false-rejection direction (healthy pruning shrinks the file) tested explicitly. Prune-by-pointer guardrail repeated — these files are gitignored, so over-pruning is as unrecoverable as a bad save. (2) The reorientation delivery mechanism is explicitly UNDECIDED. Carried as a named open design decision with three candidates — raw-typed slash command, plain-text injected instruction, and 1273's two-part file+inline shape — to be resolved during plan/implementation against a real terminal with the reason recorded. This reverses the previous commit's disposition, correctly: the spec had settled this question twice in opposite directions, each time on reasoning alone. Both settlements are now demoted to candidates. Recorded honestly that 1273's own live e2e never ran, so candidate (c) is proven in tests and design, not in production. The self-sufficiency constraint and the failure-containment note are mechanism-independent and survive whichever candidate wins.
…ary handshake Codex REQUEST_CHANGES, all seven issues incorporated. Two were factual claims I verified against the code before acting; both were correct and both invalidated a premise of mine: - 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. The clear-job now runs its own bounded loop. - lastDataAt is a last-output timestamp; Tower exposes no turn id or input-generation counter. So 'the original turn ended' and 'a follow-up turn ended' are observationally identical, making my 'can never destroy work created after the verified save' criterion UNIMPLEMENTABLE. Downgraded to a bounded window with the residual gap named, plus an output-total heuristic labelled as such. A Tower observable that would close it properly is raised as an open question, not smuggled into scope. Two findings produced real design improvements rather than wording fixes: - --begin/--boundary handshake. --begin snapshots the predecessor and issues a token under machine control; --boundary requires it. This closes the ungated-snapshot hole AND restores a machine-proven freshness token to the self path, which the previous draft had traded away on a reasoning argument. - In-memory execution vs durable intent record. Resolves the contradiction between 'jobs are in-memory (fail-safe)' and 'a dropped job is reported' — execution stays in memory so a restart can never clear, while an inert intent record makes status, cancel and dropped-job reporting implementable. Also: replaced the vague compaction 'growth comparison' with an exact predicate (reject if the snapshot survives as an unmodified leading section), which admits the compact-and-grow case a size ratio would wrongly reject; skip the check when no predecessor exists; split preflight failures from post-verification aborts, since 'every gate leaves a saved state file' was false for the former; and fixed Test 2, which still described the superseded nonce-before-write sequence.
All 14 findings across both reviewers accepted; no disagreements defended. Codex's two factual claims verified against source before acting — both correct, both invalidated a premise (tower-cron's 60s tick, and lastDataAt having no turn identifier).
Architect filed #1310 (monotonic per-session input-generation counter) out of this spec's review. Referenced at all four points where the spec names the gap: the turn-observability constraint, the bounded-window success criterion, the risk row, and References. The open question is marked resolved rather than deleted, so the dependency stays legible from both issues. The spec does NOT depend on #1310 — it ships with the bound and the labelled heuristic and strengthens to a guarantee once the primitive exists. Also staged three lessons in the thread for the review file, per architect request that the first be recorded verbatim: a fix's implementability against real observables is part of the fix; verifying reviewer factual claims against source paid off twice in one round; and 'settled by argument' kept looking like progress.
…b architecture Owner directive: 'this is overcomplicated way more than it needs to be.' Correct. Spec 1164 -> 438 lines; plan 7 phases -> 3. Target shape is the whole feature: 1. afx send --delay <seconds>, Tower-side deferred delivery — one parameter on the existing send path. Not a client that sleeps, not a job runner. 2. /arch-save as a skill: stop monitors, write the pruned state file, --raw '/clear', then --delay 15 --raw '/arch-init <name>'. 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 whether raw-typed '/arch-init <name>' lands (the production workspace's successful manual runs are 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 shows they bite. Recorded in the spec's Notes and the thread: two CMAP rounds and several owner exchanges all worked on making the design sound without anyone asking whether it was proportionate. The findings were not wrong; they answered a question that should not have been asked at that cost. The failure-containment analysis was in the spec in my own words and I treated it as reassurance about the design rather than evidence against it. Plan's one security-relevant call, flagged with its own criterion and test: --delay defers delivery, never authorisation. Target resolution and the builder-spoofing check run at request time, or a delayed send becomes a way to defer a check past the conditions that would fail it.
…iewers found Codex and Claude independently converged on the same two defects, and both fall OUTSIDE this design's recoverability posture — the failures a manual re-send does not repair. Both are now designed out, not accepted. 1. SendBuffer can invert /clear and /arch-init. /api/send already buffers when the user is typing (tower-routes.ts:1570, !isUserIdle(3000), up to 60s), and the /arch-save flow is exactly the trip case since the owner has just typed a direction. A buffered /clear plus a direct-write /arch-init at +15s lands the re-init FIRST, after which the clear wipes the recovered context. My plan's 'schedules only the terminal write' was the bypass that causes it. Due messages now re-enter the normal delivery path including buffering, so per-session FIFO holds. 2. 'afx send <self>' was an unresolved placeholder. 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 — destroying a session whose owner never invoked anything. Now architect:<name> explicitly, with the reason stated. Also: phase 1 pointed at a re-export shim (implementation is packages/core/src/tower-client.ts:655, cross-package, core-first build); delivery re-fetches by terminal id rather than closing over a PtySession; shutdown drops delayed sends rather than flushing them; --escape composition recorded N/A (afx send has no such flag); --interrupt must defer with the message; adopt.test.ts coverage added; the authorisation test must use architect:<name> since the spoofing check only fires on that path; tower-cron ruled out because CronDeps.resolveTarget takes no sender; phase 3 calibrates send->session-ready rather than send->clear-sent; and the four arch-init SKILL.md copies get updated so they stop documenting a contradictory manual loop. Spec's risk preamble corrected: it claimed every hazard costs at most one manual message, which is no longer true for the two above.
…ed delivery) Adds --delay <seconds> to afx send. Tower holds the message and delivers it later, so the sending process is free to exit — the constraint that made /arch-save's third leg impossible, since the session issuing /clear does not survive it. Authorise now, deliver later. Target resolution and the builder-spoofing check run at request time exactly as before; only delivery is scheduled. A delayed send must not be able to defer an authorisation check past the conditions that would fail it. Ordering is the load-bearing part. /api/send already defers messages through SendBuffer while the user is typing (up to 60s), so a direct-writing delayed message could overtake a buffered one: T+0 /clear -> user typing -> BUFFERED T+15 /arch-init due -> direct write -> LANDS FIRST T+40 buffer flushes -> /clear -> wipes the recovered context Due messages therefore re-enter the normal delivery path via a shared deliverOrBuffer(), and delayed deliveries additionally queue behind anything already buffered for that session (SendBuffer.hasPending). The FIFO term is scoped to delayed deliveries only. Applying it to every send changed immediate-path behaviour and broke three existing tower-routes tests, which violated this spec's own criterion that undelayed sends are unchanged. Both halves of that contract are now tested. Also: delivery re-fetches the session by terminal id and re-checks writability rather than closing over a PtySession that may have died during the wait; shutdown DROPS pending delayed sends rather than flushing them (the deliberate opposite of SendBuffer.stop, since a delayed message's timing was chosen against a world a restart has invalidated); escape+delay is refused rather than silently ignoring the delay; --interrupt defers with the message instead of firing at request time; and the CLI reports 'scheduled', not 'sent'. Validation rejects NaN explicitly at both boundaries — NaN fails every comparison, so a naive check yields a timer that fires immediately, silently converting a delayed send into an instant one. Tests: 27 new (validation, scheduling, shutdown-drops, FIFO ordering incl. the inversion scenario and the immediate-path-unchanged counterpart). Full suite green: 4044 passed, 0 failed.
…livery Covers the phase's acceptance criteria that unit tests could not reach, against the real route handler: - scheduled:true with nothing written yet; delivery once the delay elapses - delivery re-fetches the session by terminal id: a session that died, or became unwritable, during the wait is dropped gracefully with no write and no unhandled rejection - invalid and NaN delays rejected before anything is scheduled - escape+delay refused rather than silently dropping the delay - AUTHORISATION AT REQUEST TIME: a refused target never schedules, so a delayed send cannot defer a check past the conditions that would fail it - --interrupt defers the Ctrl+C with the message instead of firing now - undelayed sends stay on the immediate path These use their own terminal id. The SendBuffer in tower-routes.ts is module-level state shared across the file, and earlier tests leave messages queued for term-001 — which a delayed send then correctly queued behind, masking what the test was checking. That was the FIFO rule working, caught by its own test. Full suite: 4054 passed, 0 failed. Build clean.
…nterrupt ordering, honest reporting
Both reviewers converged on four issues; all fixed.
1. ORDERING TESTS ASSERTED AGAINST A COPY OF THE PREDICATE. The FIFO tests
re-implemented shouldDefer locally, so a regression in tower-routes.ts
would not have failed anything — leaving the one unrecoverable hazard
unguarded. Added two route-level tests exercising the real handler and the
real module-level SendBuffer, structured to isolate the hasPending term
(session idle, message still queued).
Verified by mutation: with the guard removed, both tests fail. A guard test
that cannot fail is not a guard.
2. DELAYED --interrupt BYPASSED FIFO. shouldDefer was "!interrupt && (...)",
so a delayed interrupt wrote directly and could overtake queued messages —
reintroducing the inversion through a side door. An IMMEDIATE interrupt
should bypass buffering ("an interrupt that can be deferred is not an
interrupt"), but that reasoning does not carry to one already deferred by N
seconds. It now queues, carrying its Ctrl+C on the message
(BufferedMessage.interruptFirst) so the queue drains in order AND the
interrupt still lands directly ahead of its own payload. Chosen over
refusing --interrupt with --delay, which would have removed a legitimate
capability rather than fixing it.
3. --all --delay MISREPORTED AS SENT. sendToAll now tracks scheduled
separately and reports it, matching the single-target path.
4. DUPLICATED CEILING. cli.ts hardcoded 3600 alongside the exported
MAX_DELAY_SECONDS; it now imports validateDelaySeconds, so CLI and server
cannot drift into the CLI accepting what Tower rejects.
Also surfaced "deferred" through TowerClient (the route returned it, the
client dropped it) so a typing-buffered send reports as queued rather than
looking like a silent success.
Note for the next builder: this needed a core rebuild before codev would
compile — the cross-package build ordering the plan called out, hit in
practice.
Build clean. Tests 4059 passed, 0 failed.
…isation, wire coverage Both reviewers REQUEST_CHANGES again. All findings accepted. CODEX FOUND A REAL BUG I HAD NOT CONSIDERED. Each delayed message owned an independent timer, so two due for the same terminal at the same instant both began delivering concurrently. Delivery is not atomic — writeMessageToSession paces multi-line output across several timeouts — so concurrent deliveries to one PTY interleave LINES, producing two mangled messages instead of two messages. Fixed with a per-terminal promise chain in delayed-send.ts. I did NOT adopt Codex's framing that this "violates per-session FIFO" in the broad sense. Two sends with different delays are meant to arrive at different times: --delay 30 followed by --delay 5 delivers the 5s one first, because that is what the caller asked for. Forcing request-order would make --delay not mean what it says. The guarantee is narrower and is now stated precisely in both the code and a test: a delayed message never overtakes one already QUEUED for that session. Chaining serialises concurrent deliveries; it does not reorder by request time. BLOCKING COVERAGE GAP (both reviewers). The CLI -> client -> wire chain that produces deliverAfter had zero coverage, and it cannot be covered from packages/codev — the agent-farm tower-client is a re-export shim resolving to core's dist, so a codev-side test exercises compiled output. Deleting deliverAfter from the request body left all 4059 tests green while --delay silently degraded to an immediate send. Added packages/core/src/__tests__/tower-client-send.test.ts (7 tests) and --delay cases to send.test.ts. Mutation-verified: removing the field now fails two core tests. Also: --all classified buffered messages as "sent" (result.deferred ignored) — sendToAll now tracks sent/scheduled/deferred/failed distinctly; replaced the --all reporting tests, which asserted against a local summarise() replica rather than the shipped send(), with real send() coverage; re-check queueAhead after the 100ms interrupt await, since a decision taken before an await is a decision about a world that may have moved on; corrected a test comment that claimed the local predicate was "stated identically" to the shipped one when it omits the interrupt term; and documented the delay/deliverAfter naming as deliberate (user-facing flag vs wire semantics) rather than drift. Six older delayed-send tests moved to advanceTimersByTimeAsync — a real consequence of delivery now running through the chain. core: 48 tests passing. codev: 4065 passing. Both builds clean.
…eir scheduling Claude APPROVE; Codex REQUEST_CHANGES with one finding, which was correct. THE PER-TERMINAL CHAIN DID NOT ACTUALLY SERIALISE. It awaited deliverOrBuffer, but that returns as soon as writeMessageToSession SCHEDULES its paced writes and trailing Enter. So two same-terminal delayed sends due together still interleaved — short ones producing "firstsecond\r\r" rather than two messages. The round-2 fix serialised the callback, not the writes. Codex also named why my test missed it: the chain test used an artificially async callback, so it proved the chain waits for the CALLBACK. That is the third round running where a test I wrote asserted against something adjacent to the real path rather than the real path. Fixed: deliverOrBuffer returns writeCompletesInMs (writeMessageToSession already computed it), and the scheduled callback holds the terminal's chain open for that long. Added a real-route test with two simultaneous delayed sends whose decisive assertion is that the FIRST message's trailing Enter lands before the second payload begins — an Enter after "second" would mean interleaving. Mutation-verified: removing the wait fails it. Also fixed Claude's two minor notes: - DOUBLE Ctrl+C, introduced by my own round-2 re-check. An interrupt that found the queue empty, wrote its Ctrl+C, then discovered a new arrival during its 100ms pause would enqueue with interruptFirst and send a SECOND one at flush. Now guarded by a wroteInterrupt flag. - The spoofing-refusal test mocked code 'FORBIDDEN'; the real resolver returns 'NOT_FOUND' (tower-messages.ts:229). isResolveError only checks for the presence of code, so the assertion held — but a mock that does not match production is a half-truth waiting to mislead. Left as-is, both agreed non-blocking: Number() accepting hex/exponent forms (bounded and integer-checked downstream), and hasPending not seeing an in-flight paced flush on the IMMEDIATE path (pre-existing, not introduced here; the delayed-vs-delayed case is what this phase owns and is now covered). Build clean. Tests 4066 passed, 0 failed.
…ely in spec, plan and code Claude APPROVE. Codex REQUEST_CHANGES on one point, and it was right about the substance even though I disagree with its proposed remedy. Codex found my ARTIFACTS DISAGREEING. The spec's success criterion said a delayed message "never overtakes an earlier message to the same session", which reads as request-order FIFO. The implementation deliberately lets --delay 5 overtake --delay 30. Spec and code contradicted each other, and that is a real defect regardless of which one is wrong. Codex offered both remedies: implement request-order FIFO, or revise the spec. I took the second, and the reasoning is now written down rather than left implicit in a rebuttal: --delay N is a statement about WHEN to deliver. Two sends with different delays specify different times. Enforcing request-order would make --delay 5 wait 30 seconds behind an earlier --delay 30 — the flag would silently not mean what it says. And the hazard the criterion actually cares about, named in its own sentence, is /arch-init overtaking /clear: in /arch-save the /clear carries NO delay and the /arch-init does, so the case that matters is delayed-vs-queued, which IS enforced. The spec now states three things separately instead of one over-broad claim: a delayed message never overtakes one already QUEUED for the session; concurrent deliveries do not interleave (waiting out paced writes, not just scheduling); and request-order across differing delays is explicitly NOT guaranteed, with the reason. Plan and risk-table wording reconciled to match. Also documented, per Claude's remaining minor note, exactly where the guarantee stops — in deliverOrBuffer, next to the predicate: hasPending sees queued messages but not ones mid-flush, so a delayed message arriving within ~50ms after a flush can still interleave with its pending paced writes. Recoverable (a garbled unsubmitted line, never a destroyed context) and inside the spec's accepted posture, but better stated than discovered. No behaviour change. Build clean. Tests 4066 passed, 0 failed.
Claude APPROVE. Codex REQUEST_CHANGES, and it was right — I had documented
this window rather than closing it, and my own newly-written spec sentence
("concurrent deliveries do not interleave") already promised it closed.
THE WINDOW. SendBuffer.flush() deletes a session's queue as soon as it has
SCHEDULED its paced writes, but writeMessageToSession spreads lines 10ms apart
and adds the trailing Enter 80ms after the last. Between those two moments
hasPending() reported false. A delayed /arch-init coming due in that window
wrote into the middle of the /clear being delivered — yielding
"/clear/arch-init main" on a single line, so the clear never executed at all.
That is worse than the garbled-line severity I assigned it when I chose to
document it.
Two changes, and BOTH are required:
1. SendBuffer tracks busyUntil per session: the epoch-ms at which the last
flush's writes finish. hasPending() consults it, so a delayed message
cannot bypass a delivery in progress.
2. flush() itself skips a session that is still busy. Without this the fix is
incomplete in a way that looks complete: the delayed message correctly
queues, then the next 500ms tick delivers it straight into the same
in-flight write. forceAll (shutdown) overrides — delivering late beats
losing the message.
I only found (2) because the new test failed WITH the fix applied.
TEST HONESTY. My first version of the mid-flush test passed with the guard
mutated out — it used a 4-line message whose writes complete in ~110ms, so
the delayed send landed cleanly after them and never entered the window. It
was a test of the happy path wearing the name of a regression guard. Rewritten
with a 150-line message (~1.57s of paced writing) so the delayed send comes due
genuinely mid-write. Now mutation-verified against BOTH halves independently:
removing busy tracking fails it, and making flush ignore busy fails it.
That is the fourth time this phase an artifact of mine asserted something
adjacent to the real thing. The difference here is that mutation testing
caught it before review did.
Build clean. Tests 4067 passed, 0 failed.
…s at shutdown Claude APPROVE. Codex REQUEST_CHANGES with one finding, correct again. shutdownDelayedSends() cleared the chains map, but clearing a Map cannot cancel a callback already attached with .then(). A message whose timer had fired — so it was already removed from `pending` — but which was waiting behind a slow delivery to the same terminal would still execute AFTER shutdown. That is exactly what "shutdown drops pending delayed sends" promises cannot happen, and the count shutdown reported (0, since both timers had fired) actively concealed it. Fixed with a generation counter: each send captures the generation current when scheduled and re-checks it immediately before delivering — inside the chain callback, not at timer time, because the wait to get there can be long and shutdown may have happened during it. shutdownDelayedSends() increments. Test per Codex's description: two same-terminal sends, first blocked, shut down while the second is queued behind it, release, assert the second never ran. Mutation-verified. Also a companion test that sends scheduled AFTER a shutdown still deliver, so the guard cannot poison the next Tower lifetime. Also, both from Claude's minor notes: - The write-settling timer is now unref'd. It runs after a completed write and must never be the reason Tower's event loop stays alive at shutdown. - SendBuffer.busyUntil entries are dropped when a session dies, rather than waiting for a later hasPending() to reclaim them lazily. Recorded for the phase review, per Claude: the flush() busy-gate does alter buffered-path drain timing, which brushes against "undelayed behaviour unchanged" — it is a fix for a real interleave, not a behaviour change made for its own sake. And rejecting escape + deliverAfter is a deliberate addition beyond the spec, correct for an HTTP route with non-CLI callers. Build clean. Tests 4069 passed, 0 failed.
…; reclaim busy markers Claude APPROVE, Codex COMMENT. Both flagged the same thing, non-blocking. STALE COMMENT. The "where the guarantee stops" block still said hasPending() cannot see mid-flush writes — a limitation busyUntil closed two commits ago — while omitting the one that actually remains. A comment that describes a fixed bug and hides a live one is worse than no comment: it is the artifact a future reader trusts instead of reading the code. Rewritten as COVERED / NOT COVERED / NOT GUARANTEED, naming the real residual: an IMMEDIATE direct write sets no busyUntil, so a delayed message due inside that write's ~100ms pacing window can still interleave. Left open deliberately — pre-existing property of the immediate path, damage is a garbled unsubmitted line rather than a destroyed context, and /arch-save is nowhere near it (its /clear and /arch-init are ~15s apart). SHUTDOWN ORDERING, per Claude: 4b (flush buffer) runs before 4c (drop delayed), so a delayed message that had already come due and re-entered the buffer is flushed rather than dropped. That is correct rather than a hole in the rule — once a delayed message re-enters the buffer it has been accepted for delivery and IS a buffered message; "drop, don't flush" governs sends still waiting on their timer, which is exactly what 4c cancels. Stated in a clause so nobody has to re-derive it. BUSY-MARKER RECLAMATION, per both: entries were only reclaimed lazily, so a session that flushed once and never received another message kept a stale numeric entry. flush() now purges expired markers — bounded work, since the map only holds recently-flushed sessions. No behaviour change beyond the reclamation. Build clean. Tests 4069 passed.
Records three things worth carrying to the review: 1. My own porch drift — I had wandered into phase-3 work while porch sat on phase_2 waiting for my consults. When several instructions arrive mid-turn, the orchestrator's state is the thread most likely to drop, because nothing prompts for it. 2. The --delay relocation and why the spec criterion was amended rather than silently changed: it was authored against the pre-1280-rewrite world. 3. main was red and it was NOT mine — #1143 updated both .claude copies of the afx skill and neither .codex copy. Verified against origin/main directly rather than assuming my merge caused it, then escalated instead of fixing a file I had been told not to touch. The architect's root cause on that generalises and is recorded: #1143's green CI predated the parity guards the repo has since grown, so it was true when produced and false when used — the same shape as this project's recurring lesson (an artifact asserting something adjacent to the truth), one level up.
# Conflicts: # packages/codev/src/agent-farm/servers/tower-routes.ts
…delete the three rival mechanisms Completes the adoption the architect ruled on. One mechanism, not two. WIRED submitToSession at the two sites #1320 deliberately left to whoever owns them: - deliverOrBuffer's direct write. Both of this project's paths route through that function — immediate AND delayed, since due messages re-enter it rather than writing directly — so both inherit the guarantee from one call site. - SendBuffer's flush drain, via an injected SubmitFn port so the module keeps no dependency on the server layer. The whole batch is ONE reservation: write() performs many writes and returns the final offset, so the existing delayOffset threading is untouched. DELETED, now redundant: - deliverOrBuffer's writeCompletesInMs settling wait (the lock already waits out the Enter; the field became 0 on its own once wired, rather than by deletion) - SendBuffer.busyUntil, its flush busy-gate, and its lazy reclamation - delayed-send.ts's per-terminal promise chain VERIFIED MY WIRING, not their assurance. 1273's batch test proves the PRIMITIVE supports the pattern; it cannot prove this project's wiring of it is correct. Those are different claims and only this side can make the second: - All four route-level ORDERING guards still pass, including MID-FLUSH and two-simultaneous-delayed — the two properties busyUntil and the chain used to hold. - Mutation-verified: replacing the flush's submitToSession with an inline write fails the MID-FLUSH test. So that property now rests on the primitive, not on leftovers. TWO UNIT TESTS REWRITTEN rather than deleted. Both asserted the chain that is now gone: - 'serialises two same-terminal messages' now states plainly that this module does NOT serialise and points at the route-level test that does. Re-asserting it here would recreate the replica-test mistake this project hit four times. - the shutdown-cancellation test now exercises the case that still exists — a scheduled send whose due time arrives after shutdown — since 'waiting behind a slow predecessor' was a property of the deleted chain. Also: gave each delayed route test its own session id. Chains key by session and only drain when the write settles, so a chain abandoned under fake timers poisons that id and the next submission waits forever. Cost one 5s timeout; reported to 1273 as a testing note (benign in production, where writes complete). Build clean. Tests 4180 passed, 0 failed.
…t-only Architect directive: the structural catch (arch-save is architect-only by design, a builder must refuse) belongs in the spec's test-scenarios as the reason the e2e has the shape it does. Documents that the builder cannot run its own live e2e (step 1 makes it refuse), so the run is executed by a throwaway sibling architect in the verify phase — plant canary, sibling invokes /arch-save, verify canary gone + state recovered, remove-architect. Non-destructive to main, exercises the architect:<name> path the skill warns about. Modelled on 1273's probe retest; disclosed as unrun at the PR gate rather than discovered later.
…mments that credit deleted mechanisms Both reviewers caught that my #1320 adoption left the plan's writeCompletesInMs deletion undone (I made it a no-op instead) and four comment blocks still crediting mechanisms I had removed. Same failure class as the whole project: an artifact describing a system that no longer exists. - writeCompletesInMs: field, settling-wait block, @returns clause, and the boolean-in-object return type all removed. deliverOrBuffer now returns a plain boolean; the delayed scheduler just calls it (submitToSession owns serialisation, so there is nothing to wait out). - The 'WHAT THIS GUARANTEES' block no longer credits busyUntil or the deleted chain; it now states the real split — enforceFifo decides order, submitToSession provides atomicity — and drops the 'NOT COVERED' immediate- path caveat, which was true under busyUntil and false under the lock (the immediate path takes the lock on the same key). - delayed-send.ts's generation rationale rewritten off the deleted chains map and onto the submission lock, with the honest bound stated: 'drops on shutdown' means 'starts nothing new', not 'aborts a write in progress'. Build clean. Tests 4180 passed, 0 failed.
All actionable findings fixed in ddf02ab (writeCompletesInMs deletion finished; four stale comments corrected; shutdown-drop bound stated honestly). The remaining findings — live e2e unrun, 15s uncalibrated, review file absent — are the verify-phase items the architect scheduled, disclosed rather than disputed.
…lock; cancel at write time Both reviewers, same root cause: my busyUntil deletion left two writes OUTSIDE Spec 1273's submission lock. Both regressions I introduced adopting the lock. 1. DELAYED --interrupt WROTE Ctrl+C OUTSIDE THE LOCK (both reviewers, empirically reproduced). deliverOrBuffer used to write the Ctrl+C directly, await 100ms, then submit the payload. With busyUntil gone and hasPending now 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. Fixed by folding the whole thing into ONE reservation: the Ctrl+C, its 100ms pause, and the payload+Enter now all run inside a single submitToSession thunk (mirroring deliverBufferedMessage's interruptFirst). So an interrupt due mid-flush queues behind the flush's own reservation as a unit. Removed the pre-lock write, the await, the wroteInterrupt flag, and the queueAhead re-check — all of which existed only to compensate for writing before the lock. Mutation-verified: moving the Ctrl+C back outside the lock fails the new 'delayed --interrupt due MID-FLUSH' route test. 2. GENERATION CHECKED BEFORE THE LOCK, NOT AT THE WRITE (Codex). The timer-time generation check passes, then delivery can block on submitToSession behind an in-flight write; a shutdown in that wait was not caught, so the write ran after shutdown — contradicting 'shutdown starts nothing new'. Fixed by threading an isStillLive() predicate from scheduleDelayedSend through to the write site, re-checked INSIDE the reservation. Unit test added for the timer-fired-but-lock-blocked case. The immediate path is unaffected: stillLive is undefined there, and its interrupt (queueAhead always false) takes the same in-lock write. Build clean. Tests 4182 passed, 0 failed.
…run-e2e disclosure The Review-phase deliverable both reviewers flagged as absent. States what shipped (and what was descoped), the lessons, deviations, known gaps, and the verify-phase live-run plan — with the unrun e2e disclosed at the gate rather than discovered later (the 1273 lesson applied forward).
Both blocking findings (delayed-interrupt Ctrl+C outside the lock; generation checked before the lock not at the write) fixed in 2702954 and mutation-verified. Review file written (a0c1709). Both were regressions I introduced adopting the submission lock — the project's recurring shape, a guarantee stated in a comment before the code fully backed it.
…-site cancellation guard Two findings, both mine, both from the lock adoption. CODEX — shutdown-flush regression. Routing the buffer drain through submitToSession made SendBuffer.stop() weaker than before: the batch can queue behind an in-flight write, and stop() returned before it landed, so graceful shutdown tore down terminals and lost a buffered message accepted for delivery. Restored the guarantee I broke: SubmitFn returns its promise, flush(forceAll) awaits its submissions, stop() and stopSendBuffer() are async, and gracefulShutdown awaits stopSendBuffer() before terminal teardown. The injected submit .catch()es so a throwing batch is not an unhandled rejection (also Codex's non-blocking note). New send-buffer test: stop() does not resolve until the injected submission settles. CLAUDE — the route-site stillLive guard was untested and I falsely claimed it mutation-verified in the review. My unit test checked only the predicate's RETURN VALUE via a synthetic callback — the replica pattern, again — not that the write is skipped. Added a route-level test that occupies the session lock, fires a delayed send that queues behind it, shuts down during the wait, and asserts the message never writes. Mutation-verified: deleting the guard fails it. Corrected the review's overclaim in place, naming it as exactly the failure lesson 2 is about. Build clean. Tests 4185 passed, 0 failed.
Both findings fixed in 905bc9f: the shutdown-flush regression (stop() now awaits its submissions) and the untested route-site stillLive guard (now mutation-verified, review overclaim corrected). Both were regressions from the lock adoption, both the same shape — a guarantee asserted before the code backed it — and both closed by the mutation check, now run before claiming a fix.
… coverage (confirming round) The post-cap confirming CMAP returned REQUEST_CHANGES from both lanes; treated as a real iteration 4 per architect ruling rather than riding the force-advance. Both findings were, again, mine from the lock adoption. - SHUTDOWN ORDERING (both lanes): shutdownDelayedSends ran AFTER the awaited buffer flush, so a delayed timer firing during that await passed the generation guard and could write/enqueue post-shutdown. Reversed: drop delayed sends BEFORE awaiting the flush, so the generation is bumped up front and any timer firing during the flush is cancelled at its write site. - VACUOUS stop-awaits TEST (Claude): my iter-3 test used a single microtask tick, too few for stop's chain, so it passed with the fix reverted. The sixth instance this project of an artifact asserting something adjacent to the truth, and in the very test meant to prove a fix. Now uses advanceTimersByTimeAsync and is mutation-verified. - PERIODIC-FLUSH SUBMISSIONS UNTRACKED (Codex): stop only awaited its own final forced flush's submissions. A periodic flush whose submission was still queued behind the lock -- its buffer entry already deleted -- was invisible to stop, so it could be lost at teardown. SendBuffer now tracks ALL in-flight submissions instance-level and stop drains them; regression test added. - Docs: review corrected (spec's Approach 2, not 1, for the rejected job; 4186 tests; lesson-2 count corrected to six and told straight). Added an explicit per-phase review-history section incl. the phase_3 force-advance, so the gate needs no status.yaml archaeology. Both new/fixed stop-awaits tests mutation-verified. Build clean. Tests 4186 passed, 0 failed.
…dead submissions array Claude's non-blocking note from the clean confirming round. Once stop() moved to draining instance-level outstanding submissions, flush()'s local submissions array was never populated, so its returned promise always resolved immediately and its comment described the deleted design — a latent trap for a future caller awaiting flush(true). flush() now returns void; stop() drains via outstanding, which is the single source of truth. Both lanes APPROVED the confirming round before this cleanup. Build clean. Tests 4186 passed, 0 failed.
…earned Updates' for the porch check
…t stale docs; amend plan phase 3 Final PR-review round (#1335): Codex REQUEST_CHANGES, Claude COMMENT. All actionable findings addressed; the deferred live e2e is unchanged and disclosed. - SWALLOWED WRITE FAILURE (both lanes). deliverOrBuffer's write, and the flush submit's .catch, both swallowed a thrown write with no log — while comments claimed "the route logs it." Now: deliverOrBuffer wraps the write in try/catch and logs ERROR with terminal context; the flush .catch logs; the delayed-send catch comment is corrected to describe itself as a last-resort guard. A torn-down-session write that throws is now recorded, not lost silently. - STALE session-submit.ts INVENTORY (Claude). 1273's coverage comment said the buffer flush was NOT lock-covered and that adopting it was "Spec 1307's work" — which THIS PR did. Corrected to COVERED-as-of-1307, since the comment named 1307 as the thing that would flip it; this completes that handoff rather than rewriting 1273's design. - PLAN PHASE 3 marked complete with undone deliverables (both). Amended in place with a dated note (as the spec criterion was): the live run and calibration are moved to verify (architect ruling; arch-save is architect-only), and the CLAUDE.md/AGENTS.md item is superseded by 1280's Phase-1 restructure. Checkbox states now reflect reality (verify / done / superseded). - BOUNDED SHUTDOWN (Claude non-blocking). stop()'s drain of outstanding submissions is now capped (drainTimeoutMs, default 5s, unref'd) so a wedged PTY cannot hang graceful shutdown forever. Test afterEach and the force-flush test now await the async stop(). Build clean. Tests 4186 passed, 0 failed.
Codex asked for coverage of the swallowed-write-failure fix. Route test: a session whose write throws produces an ERROR log with 'write threw', rather than a silent drop. Review test count updated to 4187.
… send (post-approval nit) Both PR-review lanes APPROVED. This closes Claude's one-line observability note: the shutdown-cancel branch was the only drop path that returned silently while every other logs. Now logged INFO like the rest — consistency with the observability fix made earlier this round. Review test count synced to 4187.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/arch-save: packaged save→clear→re-init for architect context refresh (#1307)Packages the manual architect-refresh recipe into one skill, backed by one new primitive.
What this does
afx send --delay <seconds>— Tower-side deferred delivery. Authorised at requesttime, delivered later, so the sending process can exit in between. That is the capability
the cycle needs: a session can schedule a message to itself for after something that
destroys it.
/arch-saveskill (all four trees): stop your own monitors → write a pruned statefile →
--raw '/clear'→--delay 15 --raw '/arch-init <name>'→ stop. Tower holds thelast message past the clear;
/arch-initrecovers from the state file./arch-initupdated so it no longer documents a competing manual-only loop (kept asthe Tower-unavailable fallback).
submitToSession, [Spec 1273] Per-session submission lock: an awaited send now means submitted #1320): every write goesthrough it, so a message is submitted — Enter included — before the next write to that
session. This project's three interim mechanisms (
writeCompletesInMs,SendBuffer.busyUntil, a per-terminal chain) were deleted in favour of it. Onemechanism, not two.
What was descoped
The first two review rounds hardened a Tower-owned job orchestrator with a
--begin/--boundaryhandshake and verification gates. The owner descoped all of it: thefeature is one send parameter and a document. The reasoning is in the spec's Notes — the
recoverability of the failure modes (state file survives, terminal survives, one manual
re-send recovers) is what makes the small design correct.
The live end-to-end run has NOT happened. It is scheduled for the verify phase, by
architect ruling, because
/arch-saveis an architect-session skill that a builder mustrefuse and that clears the invoking architect's context. The verify run is a throwaway
sibling-architect probe (plan phase 3 has the runbook). Disclosed here rather than
discovered later — the explicit lesson from Builder context reset should be a first-class flow: save-state → /clear → re-orient #1273, which shipped a non-functional
/clearbecause the headline path was never run. Unverified until then: that/clearactually executes, canary loss, identity recovery, monitor reconciliation, andthe manual re-send path.
The 15-second default is uncalibrated — it is the value the proposing workspace uses
manually, calibrated against the send→session-ready interval in the verify run.
phase_3 force-advanced at porch's 3-iteration cap — then was confirmed clean out of
band. Phases 1 and 2 reached clean double-APPROVE; phase_3 did not within the cap. Each
of its three rounds found a real regression from the lock adoption, each was fixed with
mutation-verification, but
max_iterations: 3was hit before a 4th consult could confirmthe iter-3 fixes, so porch force-advanced (its designed behaviour — it hands adjudication
to this gate). Because force-advance is not approval, a confirming CMAP was run after the
cap: it returned
REQUEST_CHANGESfrom both lanes (a vacuous shutdown-flush test; ashutdown-ordering race), which were treated as a real 4th iteration, fixed and
mutation-verified, and re-confirmed APPROVE by both lanes before this PR. Nothing
shipped on a force-advance alone. Full round-by-round history is in the review file's
"Per-phase review history" section and in
codev/projects/1307-*/.Testing
spec-1307-send-delay.test.ts,spec-1307-arch-save-skill.test.ts(four-tree driftORDERING:/CANCELLATION:tests,send-bufferstop-awaits, andcore-side
tower-client-send.test.tswire contract.confirmed to fail its test.
Review, verify plan, lessons, and known gaps:
codev/reviews/1307-arch-save-packaged-save-clear-.md.🤖 Generated with Claude Code