Uh oh!
There was an error while loading. Please reload this page.
[Spec 1470] Automatic builder context refresh at porch phase boundaries - #1528
Merged
Conversation
…lidate context-refresh boundaries Protocols declare refresh boundaries via a context_refresh block keyed by porch's own transition points (on_enter, on_plan_phase_advance) rather than protocol-shaped literal names, so no mapping layer can go stale. Validation rejects at load rather than ignoring, because porch has no runtime schema validation: protocol-schema.json is editor tooling only. An unresolvable boundary would otherwise produce a feature that is configured, reports no error, and silently never fires - and a context refresh that does not happen is invisible by nature. Unknown keys are rejected for the same reason: a typo'd on_entry is indistinguishable from declaring nothing if skipped. SPIR and ASPIR opt in with the same four boundaries in both trees; every other protocol declares nothing and is unchanged. All three protocol-schema.json copies gain the same block. Those copies are not byte-identical to each other and never were (draft 2020-12 vs draft-07, different required sets); that divergence pre-dates this project, so parity is asserted on the context_refresh block alone.
… null and duplicate on_enter entries Review found context_refresh: null was accepted as equivalent to an omitted key, contradicting this validator's own stated principle (reject rather than ignore, because a silent no-op is the failure mode) and all three schemas, which type the key as an object. Only undefined now means omitted; null fails with a message naming the key and saying to omit it instead. Duplicates in on_enter are rejected at runtime and pinned by uniqueItems in the three schemas. Fixing only the schema would leave editor and runtime disagreeing about the same input, and since the schema validates nothing at run time a schema-only rule is advice rather than a rule. Skeleton protocols are now load-tested. The resolver hits codev/ first, so loading by name never parsed codev-skeleton/protocols/*/protocol.json - yet for an adopter those are the shipped protocols, so a broken one would have shipped past a green suite.
… constraints from review
…tasks at declared boundaries
Porch emits a single refresh task at each declared boundary and records it in
status.yaml. Four transition sites are wired, five call sites in total:
pre-approval skip enter:plan / enter:implement for artifacts carrying
approved: frontmatter - the path CLAUDE.md documents as
normal, and the one the first plan draft missed
gate-approved SPIR's human-gated transitions
moveToReview enter:review, the quality boundary
plan-phase advance plan-phase:<id>
no-gate direct ASPIR's ungated transitions
At-most-once comes from control flow, not a guard: each site mutates the phase
fields AND appends the boundary record in ONE writeStateAndCommit, then returns
the refresh task instead of recursing into next(). There is no moment where the
state says transitioned but not refreshed-here. A firing boundary returns rather
than recursing, so a single porch next no longer chains several transitions -
each refresh gets its own turn.
The plan-phase boundary fires on advance BETWEEN plan phases, which excludes the
first by construction: entering implement IS entering plan phase 1, so no dedup
rule is needed to stop two refreshes firing back to back.
Boundary declaration is read from the FIELDS, never the truthiness of the
context_refresh object, since {} is valid config that declares nothing.
Also settles the entry-phase question raised in Phase 1 review: no site
transitions INTO the protocol's first phase (porch init sets it directly), so
on_enter naming it could never fire and is now rejected.… the ungated advance, and make the trigger tests real PRE-EXISTING BUG (not introduced by this spec, fixed because it blocks it): only two extractPlanPhases call sites existed - pre-approval and gate-approved. The ungated direct-advance path had none. ASPIR has no spec/plan gates, so plan -> implement ALWAYS comes through there, meaning ASPIR entered implement with an empty plan_phases and never reached the per-plan-phase advance branch at all. That silently cost ASPIR its per-phase iteration long before this project; it surfaces now because ASPIR's declared plan-phase:* boundaries could never fire without it. This is a behavior change beyond Spec 1470 - ASPIR's implement loop goes from single-shot to a genuine per-plan-phase cycle - so it is called out rather than folded in silently. In-flight ASPIR projects are deliberately NOT repaired: the fix runs on transition, and retroactively populating plan_phases mid-implement would reset phase statuses to pending and rewind recorded progress, which is the #1408 class of harm this spec's idempotency design exists to prevent. Tests were passing for the wrong reason in nine places. isBuildVerify is !!(phase.build && phase.verify), and the fixtures omitted both on implement, so everything fell through to handleOncePhase - four new tests failed and five existing negatives were vacuous, asserting no refresh fired about a path where none could. Fixtures now carry build+verify, all four transition sites have real end-to-end tests, and every assertion checks an observable effect of the transition rather than only the response shape. Also adopts next.test.ts's loadConfig/fetchIssue mocks: resolveConsultationModels reads workspace config rather than the phase's verify.models, so a temp root fell back to three models and porch waited for a gemini review the fixture never writes. Mocking fetchIssue also drops per-test time from ~350ms to ~2ms. Drops the unused state parameter from buildRefreshTask.
…ng on the ASPIR fix (#1503)
…eal, add transition matrix and atomicity assertion The #1408 test was still vacuous: it left build_complete false, so next() returned a build task and never re-entered a transition. It asserted that no refresh fired about a call that could not have produced one. It now drives a genuine approving verify round that advances INTO a boundary already present in context_refreshes, and asserts both that the transition still happens (suppressing the refresh must not suppress protocol progress) and that the record is neither re-appended nor overwritten. Transition matrix: 8 parameterized cases naming the ROUTE as well as the boundary, because the same boundary reached by a different route is a different code path - which is exactly how the first plan draft shipped 3 of 4 routes with a boundary list that looked complete. It caught a real gap on its first run: the pre-approved plan-to-implement case, whose fixture wrote the approval frontmatter over the file carrying the phases JSON, so extractPlanPhases fell back to inventing a phase_1. writePlan now emits frontmatter and phases in one file. Atomicity: a writeStateAndCommit spy asserts exactly one write per boundary transition. Inspecting final state cannot distinguish one atomic write from a transition write followed by a separate boundary write, and that distinction is the whole at-most-once mechanism - a crash between two writes would leave a project transitioned but unmarked, and the next porch next would clear the builder again.
…he $schema bug it produced
Iteration-1 review fixes (Codex REQUEST_CHANGES, Claude APPROVE + 3 minor).
My Phase 7 fix treated the symptom. copyProtocols copies codev-skeleton/
protocols/* into a project's codev/protocols/ but NOT the skeleton's
root-level schema, so ../../ resolved in the skeleton and broke the instant
it was scaffolded. The nine broken files in our codev/ are not an analogous
bug — they are what the skeleton produces, and every adopter has been getting
them the same way. The skeleton's nine now use ../, which resolves in both
layouts.
The test drives the real copyProtocols into a temp dir, because asserting
resolution inside the skeleton passes with either path — it carries a schema
at both levels. Reverting the skeleton fails exactly one test, the scaffold
one, with in-tree resolution still green.
Codex: the corrected --delay help traded a false claim for an over-broad one
in the same string, so it now names the one thing a restart does drop. The
test I first wrote for that was vacuous — toContain('--interrupt') passed on
the unrelated --interrupt option defined a few lines above, and deleting the
caveat changed nothing. Scoped to the --delay line and re-verified.
Also: stale-phrase scan narrowed to each file's delay-describing region with
the anchor asserted, and a header comment recording that spec test 39 is
covered jointly with the phase 1 boundary-config test.…tead of shrink Iteration-2 review fixes (both APPROVE, 3 minor). The $schema enumeration filtered on existence, so deleting a protocol.json would have quietly removed its own test rather than failing one — coverage evaporating while the run stayed green. The enumeration guard I had already written for this (>= 18) did not help: a floor catches "matched nothing", not "matched one fewer". release is now excluded by name and every remaining directory is required to carry a protocol.json. delayRegion fell back to whole-file scanning for SKILL.md, so the false-positive protection added in the same iteration did not extend to whatever skill enters LIVE_DOCS next. Scoping is now a property of the helper rather than of today's file list. Both mutation-checked. codev-skeleton/protocol-schema.json being unreferenced is recorded as a follow-up, not acted on: the two schemas differ in content, so deleting the unreferenced one may be backwards.
…all transition sites Spec test 36. Drives a project through the real next(), acknowledging each refresh the way a returning builder does, and asserts the refresh history as a whole — which isolated per-site tests structurally cannot do. The Phase 2 harness is extracted into a shared fixture rather than copied: a simulation asserting "all four boundaries" against a near-copy of the protocol could pass while testing a shape nobody ships. Phase 2's 40 tests still pass against the extracted version. Three defects in the test code itself, all caught by mutation or by pinning a positive expectation: - hardcoded plan_phase: where the code emits plan-phase:, so the test asserted a remembered format string. It now builds expectations from the functions under test. - the gated scenario ran EMPTY: the driver matched gate status 'requested' where porch writes 'pending', so it broke at the first gate and every gated assertion passed on a run that never left specify. Non-vacuity floors added throughout, written before the property they guard. - the simulation did not test its own name: disabling hasRefreshed outright left every test green, because a healthy sequence never revisits a transition. Renamed to what it checks, and a replay arm added that drives a real run then applies the #1408 reset — that arm catches the mutation, and unlike Phase 2's version it exercises the guard against history a real run wrote itself. Also confirms end to end that a pre-approval skip fires no refresh while an approved gate does — the human's SUPPRESS ruling, asserted as a pair.
…ents, live-run runbook Spec tests 34 and 35 against a real file-backed SQLite database seeded from the production schema. File-backed is load-bearing: a restart is simulated by closing and reopening the connection, which an in-memory database cannot express — it would vanish on close and prove the opposite of the claim. Boundary-save measurement, the evidence Phase 3's retained 1000-byte floor promised. Saves do not cluster at the floor, they straddle it: a real save for this project at enter:review is 2952 bytes (3x, unpadded), while a plausible terse save for a small project is 634 and would be rejected. The floor is NOT lowered — Baked Decision 4 requires the auto path to be more conservative than the manual one, and the failure is safe (a rejected save means no clear, so a builder loses the refresh rather than its memory). Recorded as a decision for the architect, with the caveat that the terse sample is constructed rather than observed. The runbook caught a defect in itself: it named the challenge file .builder-challenge.json where the constant is .builder-refresh-challenge, so the cleanup step would have left a stale challenge in place — the exact state the boundary binding defends against — one step before a clear. Its facts are now pinned to the constants, because this is the only document here that is RUN by hand rather than read, against a builder whose context the procedure destroys on purpose.
…erified invocation Architect ruled Option B — drive the CLI by hand, no Tower restart — because Option A kills every builder across 14 workspaces including another project's fleet. Recorded with the reasoning, since the trade-off looks like a shortcut later and is not: tests 37/38 are about the harness, and porch's emission is covered by the full-protocol simulation, so each half gets the cheaper instrument that genuinely covers it. Invocation verified rather than recalled: the entry is bin/afx.js (which imports dist/agent-farm/cli.js), dist is newer than every src .ts, --help lists the feature flags 3.3.0 lacks, and identity is cwd-derived — the same binary run from /tmp refuses and exits 1. So the architect runs my build with the subject's cwd. Updating the runbook broke its own --boundary guard, and the fix took three passes, each caught by mutation. The worst: filtering out prose by "has arguments" also skipped a bare '<AFX> self-refresh', so the check that exists to catch a missing --boundary skipped the command missing everything. Now two passes — code blocks with no argument filter, inline steps keyed on the <AFX> placeholder, with that convention asserted rather than assumed. The template was fixed rather than exempted; one that omits the guard teaches the omission.
Walks all 23 items accumulated in the builder thread across eight phases. Live-run sections are explicit stubs rather than prose written around the gap: spec tests 37 and 38 are marked PENDING in the compliance table, the evidence table carries empty rows for the preflight and both runs, and DEFAULT_REENTRY_DELAY_SECONDS is named as the one number still resting on inheritance rather than measurement. A cold reader can see what is unproven. Records the boundary-save straddle measurement with the constructed-sample caveat and the architect's revisit trigger, the Option B ruling and its reasoning, the force-advance that actually occurred, and the correction that the skeleton was not 'correct' about $schema — it resolved while emitting the bug into every scaffolded project.
… 38 PASS Architect-driven live runs on a disposable subject, Option B. Raw evidence preserved verbatim in measurements/live-run-evidence.md. Test 37: the re-entry was delivered 15.9s after the clear and was NOT consumed by it — the property no unit test can reach, now demonstrated rather than argued. The post-clear probe had the subject state it sourced its answer from .builder-state.md and the worktree, 'not from memory of doing the work, which is gone'. Test 38 used variant 2c, the echo-the-request bypass — the attack that once passed every gate AND every test. Rejected before any clear, no re-entry scheduled, subject unaffected. Unplanned bonus: the stale-challenge guard fired live on a challenge aged 18320s against a 3600s limit, refusing with an empty step log and an intact context — the replay guard demonstrated on the destructive path without anything being destroyed. DEFAULT_REENTRY_DELAY_SECONDS stays 15, now measured rather than inherited. The value is unchanged and that is the honest outcome. What the measurement adds is the timing's shape: delivery lags the due time by ~1s because a drainer pass rather than a timer delivers, so the constant is a floor on the wait and never an exact interval; and the clear lands ~0.1s after scheduling, so nearly the whole window is post-clear margin rather than time spent waiting for the clear. Save measured at 4506 bytes, 4.5x the floor unpadded — third real data point. Every observed save clears the floor; the only sample below it remains the constructed one.
…the busy gate for real Both reviewers, independently: the full-protocol simulation captured finalStatus and never asserted it. drive() breaks on status === 'error' WITHOUT hitting the ceiling, so an errored run satisfied the progress test and read as healthy. Captured-but-unasserted is worse than not captured — it looks like the check exists. All three full-run arms now assert 'complete' (verified: the fixture reaches implement → review → verified). Codex on spec test 35: the busy-terminal test enqueued a row already marked reason: 'busy' and read it back, which proves enqueue stores what it is given and would have passed even if production delivered onto busy terminals. The claim is about the DECISION, so the decision is now exercised — deliverAgentMail driven through its real ports with the gate verdict as the only variable, asserting nothing is WRITTEN (a re-entry written onto a busy prompt fuses with whatever the builder was typing), the row stays held, and the paired clean-gate positive delivers. A pre-due row is not delivered even onto a clean prompt. Mutation-checked both directions: forcing every verdict clean fails the busy test, forcing every verdict busy fails the clean one.
… pass-3 evidence Third live pass closed spec test 37's two remaining clauses on a real ASPIR porch project: emission at a real boundary (plan-phase:phase_2_index) and post-clear resumption via porch next — with porch recovering the consultation that died with the clear, which is the planner repairing in-flight state the refresh disturbed. Second independent measurements: 15.8s clear-to-re-entry (vs 15.9s) and a 5751-byte save. The subject also found the one place the core promise is weaker than it reads. 'Never clear on an unverified save' means authentic, substantive, settled and recent — nothing checks the save is still TRUE. A refusal-and-retry cycle left a save saying 'phase 2 not started' when phase 2 was done; a cold reader following it would have re-implemented finished work, which is the harm the refresh exists to prevent, produced by the refresh. Shipped: the request now tells the builder to rewrite a save that work has made stale, and says why. NOT shipped: the cheap HEAD-moved guard. It catches only committed drift, so it would advertise a staleness check that misses uncommitted work — false confidence is worse than a known gap. It would also add a gate to the destructive path in the final phase, after the review cycle that would normally scrutinise it, and 'what counts as stale' is a design question that belongs in a spec. Filed as the top follow-up instead.
All findings accepted. The third sent the phase back for another live run rather than being fixed in code: an architect ruling recorded in a runbook is not an amendment to an approved artifact, and two of spec test 37's four clauses were unproven. Resolved by running the missing pass, so the spec still describes what was actually verified.
…tor; pin every arm Codex: the simulation drove only next(). The plan requires next() and the orchestrator together, and without that it would have passed unchanged if every refresh failed verification, never scheduled a re-entry, or never cleared. New integration test composes them against the Phase 3 fakes — extracted and imported, not re-declared — and fails against all three of those scenarios plus a dropped byte floor. Codex: the gated and ASPIR arms asserted 'at least one, no duplicates', which would pass with enter:implement or a plan-phase advance missing. Both now pin the exact five-boundary set. Doing so exposed that the ASPIR arm pre-approved its artifacts and so took the skip path, never exercising the no-gate direct advance it exists to cover. Claude: the pass-3 timeline shows acknowledged_at set four minutes BEFORE the clear, because the refusal/escalation round-trip put a porch next inside the --begin -> execute window. A lost re-entry would then have left porch status showing health while a cleared builder sat idle — the invisible-stall case the marker exists for. Recorded with save staleness as one follow-up with one root: that window is where the model assumes nothing happens, and both live runs put real events in it. Claude: the runbook-accuracy test was pinning my own false framing that the subject's porch cannot emit refresh tasks. A test that enforces its author's mistake makes the error durable. Corrected by addendum, not rewrite.
Three of the four findings were places where I had already written down the right thing — the plan said 'and the orchestrator', the doc comment said what the acknowledgment really means, the runbook framing was mine to check — and had not acted on my own words. Recording a constraint is not satisfying it.
Both reviewers APPROVE; phase 8 converged at iteration 3 with no force-advance. The /tmp write survived from diagnosing the step-shape bugs. A test that writes outside the repo as a side effect is not merely untidy — it makes the suite depend on a path that works on one machine and can fail in CI. Swept the test tree for the same pattern; no others. Claude's second item — filing the false-acknowledgment gap as a GitHub issue — is routed to the architect rather than done here: issue creation has run through them on this project, and a follow-up that lives only in a review artifact is found by nobody once the PR merges.
#1529 is the false-acknowledgment gap and #1530 the task-lane reply loss — both routed to the architect rather than filed by me, since issue creation has run through them on this project. Referenced here so the issues point forward and the review points back. A follow-up that lives only in a review artifact is found by nobody once the PR merges, and #1529 in particular is unmitigated and weakens a signal shipping in this same PR.
Uh oh!
There was an error while loading. Please reload this page.
waleedkadous added a commit
that referenced
this pull request
Aug 19, 2026
waleedkadous added a commit
to pseudoseed/codev
that referenced
this pull request
Sep 4, 2026
(cherry picked from commit 7236cc1)
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 freeto 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.
Closes#1470. Also closes#1503 (see Pre-existing fix below).
Porch now emits a context-refresh step at boundaries a protocol declares, so a builder gets the
/arch-saveloop automatically instead of relying on someone noticing it is running long. For SPIRthat is after spec approval, after plan approval, on each plan-phase advance, and before review.
How it works
A two-step challenge handshake.
afx self-refresh --beginmints a nonce; the builder writes acold-reader save carrying it on the first line;
afx self-refreshverifies that save beforeclearing anything.
The re-entry is scheduled before the clear — the inverse of
/arch-save— because the twoorderings fail asymmetrically. Schedule-then-clear can deliver early, which is recoverable.
Clear-then-schedule can leave a builder cleared with no route back: idle, and indistinguishable
from working.
Live verification
The one property no unit test can reach is whether a queued
/clearconsumes a re-entry deliveredjust after turn-end. Three architect-driven live passes on disposable subjects, 2026-08-19:
Resumption was proven rather than inferred. A post-clear probe had the subject say it sourced its
state "from
.builder-state.md… then confirmed against the worktree — not from memory of doingthe work, which is gone."
The negative run used the echo-the-request bypass (
cp <request> .builder-state.md) — the exactattack that once passed every gate and every test here. Rejected before any clear, no re-entry
scheduled, subject unaffected.
A third pass ran on a real ASPIR porch project, because the first pass proved only two of spec
test 37's four clauses — review caught that an architect ruling recorded in a runbook is not an
amendment to an approved spec. There, the feature porch emitted the task at a real boundary and the
fresh context resumed via
porch next, with porch recovering the consultation that had died withthe clear. Clear→re-entry measured 15.9s and 15.8s across two independent passes.
Two guards fired live without being asked to: the stale-challenge guard refused a challenge aged
18320s against a 3600s limit, and the dirty-worktree guard refused an execute and produced the
right behaviour rather than merely permitting it — the subject escalated instead of reaching for
--allow-dirtyitself.Safety properties
status.yaml—/clearhas no undo, so a boundaryfiring twice destroys a builder's context with nobody watching. Verified against the porch: SPIR verify-approval triggers phase-transition that resets all plan phases to pending (state loop) #1408 replay
loop using history a real run wrote itself.
gate leaves the context intact and says so.
(human ruling: a skip is not work).
Pre-existing fix — #1503
ASPIR's ungated direct-advance path never extracted
plan_phases, so implement ran single-shot.Found while wiring the fourth transition site; fixed here by architect ruling rather than split
out, and #1503 closes with this PR.
In-flight ASPIR projects are deliberately not retroactively repaired: populating
plan_phasesmid-implement would reset phase statuses to
pendingand rewind progress — the #1408 harm class.Also fixed along the way
$schemabug in the generator, not just its output. All ninecodev/protocols/*/protocol.jsondeclared a path that does not resolve — and so did the skeleton, which
copyProtocolscopies intoevery scaffolded project without the root-level schema those paths point at. Existing adopter
projects keep their broken copies (noted as a follow-up); the generator no longer produces them.
--delaydocumentation that caused a factual error in this project's own spec. Aplain
--delayis persisted and survives a Tower restart; only a delayed--interrupt's ^Cnudge is dropped. Corrected in all six live locations.
Known gaps, stated plainly
The
--begin→ execute window is where the design assumes nothing happens, and both live runs putreal events in it. Two consequences, recorded as the top follow-up:
checks it is still accurate. Mitigated here by instruction only.
porch nextinside that window marks the boundary acknowledged beforethe clear, so a lost re-entry would leave
porch statusshowing health over an idle clearedbuilder. Unmitigated.
Both need design rather than a last-phase patch, and the review says so rather than implying the
feature is airtight.
Testing
~370 new tests across 17 files; full suite 5289 passing, 0 failing. Every guard here was
mutation-checked — the defect injected, the test watched to fail — which caught nine vacuous tests,
three wiring gaps, and two holes in guards written specifically to close those holes.
Review
codev/reviews/1470-automatic-builder-context-refr.md— spec compliance, five plan deviations,the boundary-save measurements, and ten follow-up items, all out of scope and none blocking.