Test coverage: mailbox owner-resolution wiring (escalateHeldToOwner) and cleanupBuilder() held-row dismissal (#1477)

[mohidmakhdoomi]

Closes #1477.

Test-only. No production file is modified — git diff against main touches nothing under
src/, and all three reviewers verified that independently.

Issue #1477 named two Spec 1313 round-3 glue points that were verified by inspection only. Both
are now covered directly.

What was actually uncovered

Glue point	Why the existing tests missed it
escalateHeldToOwner (servers/mailbox-wiring.ts:405)	send-delivery.test.ts only ever stubs ports.escalateHeldToOwner (:150, :1189, :1196), so it tests the drainer's decision to escalate, never the binding that resolves the owner.
cleanupBuilder's held-row dismissal (commands/cleanup.ts:389)	spec-1313-cleanup-dismiss.test.ts says in its own header that it re-implements the seam (mailbox.dismissHeldForAgent called directly) and that "the full cleanupBuilder … is out of scope here".

The resolvers underneath (resolveAgentInRegistry) and formatOwnerNoticeBody are already tested
in isolation. These two files are the missing third leg: the binding between them.

Approach — substitute the DB singleton, nothing else

vi.mock('../db/index.js') returns one in-memory GLOBAL_SCHEMA database from both getDb() and
getGlobalDb() — which is the Issue #1118 production shape. That leaves state.ts,
servers/tower-messages.ts, db/mailbox.ts and utils/workspace-path.ts all running for real, so
the tests observe rows the production code path actually transitioned.

Precedent: spec-755-lookup-builder.test.ts, improved on by importing the real GLOBAL_SCHEMA
rather than hand-copying a CREATE TABLE.

The cleanup test drives the real exported cleanup() end to end, stubbing only side-effecting
collaborators (git, Tower, forge, ps, state writes).

Coverage

air-1477-owner-escalation-wiring.test.ts (9 tests) — architect-skip; spawning-architect
affinity; main fallback when the spawner is deregistered; the no-main tie-break; the no-op that
returns false so the drainer's once-per-episode guard stays unarmed and retries; supersede-keyed
coalescing; drain scheduled for the resolved owner; cross-workspace owner resolution;
clearHeldOwnerNotice key specificity.

air-1477-cleanup-dismiss-invocation.test.ts (4 tests) — dismissal reached from cleanup(),
siblings untouched; the normalized-workspace round-trip; cross-workspace isolation of the same
builder id (#1118); ephemeral builders; non-fatal on a DB failure.

The fixture hands getConfig() a symlink to the workspace directory while the mailbox keys
rows under the canonical path, so every test in that file traverses normalizeWorkspacePath rather
than just one, and a guard assertion fails loudly if the two paths ever collapse to the same
string.

Verification

Every assertion was mutation-checked against the real source, then the source restored:

Mutation	Result
Delete dismissHeldForAgent(...) from cleanup.ts:389	3 failures
Strip normalizeWorkspacePath from that call	3 failures
Drop the workspace scope from that call	3 failures
Drop the sender arg from resolveAgentInRegistry('architect', ws, info.toAgent)	2 failures
Delete the getArchitectByName architect-skip guard	1 failure
Point scheduleDrain at the starving agent instead of the owner	1 failure
Make clearHeldOwnerNotice ignore the agent key	1 failure
Switch getArchitects to ORDER BY started_at	1 failure

13 tests pass; tsc --noEmit clean; the full agent-farm suite is green (163 files, 3442 tests).

On size, against AIR's "<300 LOC" framing:

$ git diff --stat main...HEAD -- packages/codev/src
 air-1477-cleanup-dismiss-invocation.test.ts | 252 +
 air-1477-owner-escalation-wiring.test.ts    | 298 +
 spec-1313-cleanup-dismiss.test.ts           |   6 +
 3 files changed, 556 insertions(+)

This number has been wrong three times now — 511 went stale when the round-2 fixes grew the files,
544 when the rmSync lines landed, 554 when the #1633 cross-link did. Each time I retyped a
snapshot instead of re-running the command. Pasting the output so the next reader can see what it
counts, and re-checked against the final commit. Comment density is high, but flagging the number rather than letting you
discover it. Happy to trim; the cross-workspace cases are the ones I'd cut last.

Note for anyone running the suite in a fresh worktree: 12 unrelated suites fail until
pnpm bundle-assets and tsc have run (they need packages/codev/skeleton/ and dist/). All 12
pass afterwards.

Correction to an earlier revision of this description

The first push of this PR claimed the normalization test proved "a raw config.workspaceRoot would
key a different string and dismiss nothing." That was false, and the review caught it. The
fixture pre-realpathSync'd its root (making normalizeWorkspacePath the identity) and its
"non-canonical" override join(root, 'nested', '..') collapsed lexically inside path.join
before normalization was ever reached — so the two strings were already equal and the suite stayed
green with normalizeWorkspacePath deleted from cleanup.ts. I reproduced that before fixing it.

Round 1 said the tests were "mutation-checked", and they were — but I mutated the call and never
its argument, so the one thing that test existed to pin was the one thing I never tried to break.
The symlink fixture fixes it, and stripping normalizeWorkspacePath now fails 3 tests.

Reviewed 3 ways

Gemini APPROVE. Codex and Claude each found real defects in the first draft, all fixed here — most
importantly that scheduleDrain was uncovered: the drainer is never started in a unit test, so
the call no-ops and deleting it left every test green. It now has a spy asserting the drain targets
the resolved owner.

Two other corrections worth naming, because both were cases of the tests claiming more than they
did:

• The suite implied it covered the owner.agent === info.toAgent self-notify guard. It does not —
  that branch is unreachable under the real resolver, since resolving to an architect requires that
  architect to be registered, which the architect-skip already caught. Comment corrected rather than
  mocking a path to dead code.
• The TowerClient stub was backwards: it implemented killTerminal (never called) and omitted
  refreshOverview (called at cleanup.ts:422), so every test silently ran the Tower-unreachable
  branch.

Round 2 (on this PR) fixed the vacuous test above, plus a scheduleDrain spy whose mockRestore
sat at the end of the it body (a failed assertion above it would leak the spy into later tests),
and an architect fixture where both records shared new Date() and could tie — startedAt is now
explicit, with zeta registered earlier, so id order and registration order genuinely disagree.

Two review items are deliberately not addressed, rather than silently skipped: scheduleDrain's
workspace argument cannot be pinned from outside (resolveRegistryArchitect returns the workspace
it was asked about, so owner and starving workspace are equal by construction — noted in a comment
instead of faked), and the loadConfig(homedir()) hermeticity smell is inherited from production
and would need a production change.

One production observation — now filed as #1633

resolveRegistryArchitect takes architects[0] from getArchitects, which is ORDER BY id —
alphabetical by architect name. The doc comments there and in mailbox-wiring.ts say
"first-registered", and both loadState (ORDER BY (id != 'main'), started_at) and the live
resolver (registry insertion order) implement it that way. So an offline hold can name a different
architect than a live send to the same workspace would.

Deterministic and low-impact, and changing it is a production decision outside a test-only AIR — so
the test pins current behaviour and says plainly in a comment that it diverges from the documented
contract, rather than either blessing the wording or quietly asserting the aspiration.

Filed as #1633. Worth noting there: getArchitect (singular) was already fixed for exactly this
bug — spec-755-migration.test.ts:279 pins it as "started_at order, not lexicographic" — but
getArchitects (plural), which the offline resolver reads, never got the same treatment. The fix
landed on one of the two functions.

PR-stage CMAP (AIR pr phase)

Distinct from the architect's integration review above — different stage, different prompt.

gemini APPROVE (HIGH) · codex COMMENT (HIGH) · claude APPROVE (HIGH). No blockers.

Claude independently re-ran three mutations against real source (restoring afterwards) and
reproduced the claimed counts exactly: stripping normalizeWorkspacePath → 3 failures, retargeting
scheduleDrain → 1, dropping the sender arg → 2. It also confirmed CI runs ubuntu + macos only,
so the symlinkSync fixture has no Windows exposure.

Two review items acted on:

• Codex — the size disclosure was stale (511 vs the actual 544). Corrected above; it was my
  round-1 number left unrevised after round 2 grew the files.
• Claude — the module-scope symlinkSync used a PID-derived name, so a hard-killed run
  (SIGKILL/OOM) could strand the link and make a later run drawing the same PID fail with EEXIST
  at collection, losing the whole file rather than one test. Now rmSync(…, { force: true })
  first, which I verified reproduces EEXIST without it and succeeds with it.

Claude also noted the ORDER BY id divergence had no filed issue and could die with this thread.
The human's call was that I file it: #1633. The test that pins the current behaviour now names
that issue in its comment, and the issue names the test — including the fact that fixing #1633
should break that assertion, and that flipping it to zeta is the intended response rather than
deleting it.

Refs #1313 (PR #1330), maintainer optional-2 in the Follow-up section of
codev/reviews/1313-afx-send-mailbox-first-delivery.md.

🤖 Generated with Claude Code

[mohidmakhdoomi]

Architect integration review — 3-way CMAP

Verdicts: gemini APPROVE (HIGH) · codex COMMENT (MEDIUM) · claude REQUEST_CHANGES (HIGH).

Reviewed at high depth: the diff is +615 lines, which crosses the >500 line threshold in codev/resources/risk-triage.md, and precedence there is "highest factor wins" even though every other factor (test-only, 2 code files, zero production change) reads low.

All three agree the PR is well-integrated and that both #1477 premises were real. I verified the substantive findings against the source myself rather than relaying them.

Blocking — one vacuous test (claude, verified)

air-1477-cleanup-dismiss-invocation.test.ts:190 — "normalizes the configured workspaceRoot to the path the mailbox keyed rows under" — pins nothing.

:101  workspaceRoot = realpathSync(mkdtempSync(...))   ← already real
:103  WS            = normalizeWorkspacePath(workspaceRoot)   ← identity
:194  override      = join(workspaceRoot, 'nested', '..')     ← collapses LEXICALLY to workspaceRoot

path.join resolves nested/.. before normalizeWorkspacePath is ever reached (no filesystem access, and nested need not exist), and normalizeWorkspacePath is just realpathSync — the identity on an already-realpath'd path. So config.workspaceRoot === WS in both tests, and stripping normalizeWorkspacePath from cleanup.ts:389 leaves the suite green. Claude confirmed this empirically by mutating the source in the worktree (restored afterwards); I confirmed the two halves independently.

Two consequences:

1. The inline comment at :192 — "A raw config.workspaceRoot would key a different string and dismiss nothing" — is untrue as written, as is the matching line in the PR description. Fix the prose alongside the test so the git record isn't wrong.
2. Note the irony: the pre-existing spec-1313-cleanup-dismiss.test.ts does not pre-realpath its mkdtemp, so on macOS (/var → /private/var) its round-trip assertion is discriminating. The new test realpaths that difference away and is strictly weaker on this axis.

Fix: make the two paths genuinely differ, e.g. a symlinked root:

const realRoot = realpathSync(mkdtempSync(join(tmpdir(), 'air-1477-cleanup-')));
const linkRoot = join(realpathSync(tmpdir()), `air-1477-link-${process.pid}`);
symlinkSync(realRoot, linkRoot);            // rmSync in afterAll
// config({ workspaceRoot: linkRoot }) → a raw key ≠ WS → dismisses nothing

The mockRemoveBuilder assertion in that test then expects linkRoot. Re-run the mutation afterwards to prove the test now fails without normalizeWorkspacePath.

Also fix (codex, both confirmed by reading)

• Spy leak. scheduleDrain.mockRestore() is the last statement of its it body, so a failed assertion above it skips the restore and leaks the spy into later tests. Move it to afterEach or a finally.
• Tied startedAt. The first-architect fallback fixture builds both architects with new Date().toISOString(), which can produce identical timestamps. Give them explicit, distinct values so the test actually discriminates id-order from registration-order — otherwise a future resolver fix would be sorting on a tie and the assertion becomes arbitrary.

Non-blocking

• scheduleDrain's workspace argument is unpinned (claude). Production passes owner.workspacePath; every fixture makes owner and starving workspaces equal, so swapping them is undetectable. The cross-workspace test doesn't extend to the drain target.
• ensureDrainer() → loadConfig(homedir()) reads the developer's real ~/.codev/config.json from a unit test. Inherited from production and try/catch-guarded — a hermeticity smell, not a bug.
• Cross-reference spec-1313-cleanup-dismiss.test.ts's header to the new invocation test so the seam-vs-invocation split is discoverable from either file.
• 511 LOC vs AIR's "<300" — self-flagged; I'd keep it. The cross-workspace cases are the highest-value assertions here.
• Follow-up for MAINTAIN, not this PR (claude): propagate the GLOBAL_SCHEMA-import improvement back to spec-755-lookup-builder.test.ts, which still hand-copies a CREATE TABLE that can drift from the real schema.

The production observation is real — architect verified

Confirmed against source, independently of the builder's report:

• getArchitects (state.ts:511) is ORDER BY id — lexicographic.
• resolveRegistryArchitect (tower-messages.ts:539) takes architects[0].name off that.
• The live resolver (tower-messages.ts:372) uses entry.architects.values().next().value — Map insertion order, i.e. registration order.
• state.ts:494 documents the intent as "first-registered", and spec-755-migration.test.ts:279 pins it as "started_at order, not lexicographic".

So an offline hold can name a different architect than a live send to the same workspace, and the doc comments at tower-messages.ts:539 / mailbox-wiring.ts:396 are currently wrong. Deterministic and low blast radius. Pinning current behaviour with a comment that flags the divergence was the right call for a test-only AIR — a separate issue is being raised rather than folding a production fix in here.

What was verified as accurate

Test-only is real (no src/ changes; the status.yaml + thread-log commits match porch convention). Both #1477 premises hold — escalateHeldToOwner at mailbox-wiring.ts:405 reachable only via the exported makeDeliveryPorts (:292), and the dismissal at cleanup.ts:389 left untested by the existing seam test, which says so in its own header. The scheduleDrain spy is the right instrument: mailbox-delivery.ts:1222-1225 returns Promise.resolve() when ports/db are unset, so the call genuinely no-ops in a unit test and deleting it would otherwise leave everything green. 13/13 pass.

[mohidmakhdoomi]

Architect follow-up — blocker resolved, verified independently

a2255ed5a clears the blocking finding. I re-verified rather than accepting the report.

The vacuous test is now discriminating. The fixture makes WS a real directory and workspaceRoot a symlink to it, and the symlink is what getConfig() returns — so all four tests traverse normalization, not just the one that claimed to. I confirmed the mechanism empirically: raw strings differ, path.resolve does not collapse a symlink (unlike the old join(root,'nested','..'), which collapsed lexically before normalization was ever reached), and only realpathSync gets from one path to the other. So keying on the raw config.workspaceRoot matches no row. The builder re-ran the mutation and reports 3 tests now fail with normalizeWorkspacePath stripped from cleanup.ts:389.

Better than what was asked for: expect(workspaceRoot).not.toBe(WS) guards the fixture against silently degenerating back to vacuity, which is the failure mode that produced this finding in the first place.

Both codex items fixed and checked. vi.restoreAllMocks() moved into afterEach (with a comment at the call site explaining why it is not the last statement of the it body). architect() now takes an explicit startedAt, and the id-order test registers zeta at 2026-01-01 and alpha at 2026-06-01 — so id order yields alpha, registration order yields zeta, and the assertion genuinely discriminates the two. It would not have before.

The false prose is corrected in both the inline comment and the PR description, the latter carrying an explicit correction paragraph rather than a quiet rewrite. That matters for the git record.

Three non-blocking items were declined with reasons I accept: scheduleDrain's workspace argument is unpinnable from outside (resolveRegistryArchitect returns the workspace it was asked about, so the two are equal by construction — documented rather than faked); loadConfig(homedir()) needs a production change; and the spec-755-lookup-builder GLOBAL_SCHEMA propagation is MAINTAIN work, not this PR.

Mutation sweep is now 7 points, all caught. The builder's own retrospective is the most useful line in this round and worth recording for the cohort: "I mutated the CALL and never its ARGUMENT, so the one thing that test existed to pin was the one thing I never tried to break. A mutation sweep is only as good as the set of mutations you think to try."

No further changes requested from me. The resolveRegistryArchitect ORDER BY id divergence stays a separate follow-up; this PR correctly pins current behaviour with a comment flagging it.