fix(mint): thread traceWriter into phase forks so they emit lifecycle events#696
fix(mint): thread traceWriter into phase forks so they emit lifecycle events#696griffinwork40 wants to merge 1 commit into
Conversation
… events Each of /mint's 8 phase functions constructs its own SubagentManager WITHOUT the session's trace writer, so their forked subagents emitted NO subagent_lifecycle events — the last dispatch path still invisible in the witness trace (the agent-tool and compose paths were fixed in #466). This is why no trace in the corpus ever carried a `mint-*` agentType. Thread ctx.traceWriter (already available on SkillExecutionContext — see skills/index.ts) from the mint handler through runPhasesAfterSpec into every phase (spec, research, plan, parallelize-dispatch, build, verify, heal, ship) and into their fork managers, mirroring the root-manager threading (#466) and user-skills.ts. Additive optional param; resume/test paths without a ctx degrade to the prior (no-writer) behavior, unchanged. Test: mint.test.ts asserts every phase's fork manager is constructed WITH the writer when ctx.traceWriter is set, and WITHOUT it when absent.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
When a TS parallelize skill is registered, this branch wins over the plugin-body fallback below, but the newly threaded traceWriter is still not passed into the handler. SkillMetadata.handler accepts the execution context as its third argument, so a registered override that forks subagents with its own SubagentManager remains invisible in the durable witness trace even though the plugin fallback now receives the writer. Pass a context containing the writer (and the same model/callId fields as applicable) instead of calling the handler with only { plan }.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Code Review — PR #696
fix(mint): thread traceWriter into phase forks so they emit lifecycle events
Regime: light (10 files, +124/−9 = 133 lines) · Reviewed ref: fe2f3dc
Stated intent: PR #696 title + body — spec-compliance assessed.
Merge decision: ✅ MERGE
The fix does exactly what it claims. All 8 phase construction sites are threaded, the positional argument plumbing is correct at every call site, and the change is genuinely additive. One medium test-coverage finding below — the new regression test is weaker than the PR body claims, but it does not block the fix itself.
Findings
1. medium · confidence: high · test-coverage
src/skills/mint.test.ts:1031-1034 · ref:fe2f3dc · citation-type: diff-context
finding: The new test's loop asserts traceWriter only over whichever managers the mock happened to construct and asserts no exact count, so it never exercises the parallelize-dispatch.ts or ship.ts construction sites — both are structurally unreachable under this mock — meaning a regression that drops the spread from either file still passes this test green.
evidence:
const ctorCalls = vi.mocked(SubagentManager).mock.calls;
expect(ctorCalls.length, 'at least one phase fork manager constructed').toBeGreaterThan(0);
for (const [opts] of ctorCalls) {
expect((opts as { traceWriter?: unknown } | undefined)?.traceWriter).toBe(traceWriter);
}Two independent short-circuits make those two sites unreachable in this test:
- parallelize skipped.
plan.ts:75returnsplanResult.message.contentverbatim; the mock sets that to the literal`Mocked ${idPrefix} output`(mint.test.ts:76), which contains no path-like token.countFileReferencestherefore returns 0, andparallelize-dispatch.ts:73-75returns{kind:'skipped'}before thenew SubagentManager(at line 111 is ever reached. - ship never runs.
forkVerifyModederivespassedsolely fromoutput.status === 'PASS'(verify.ts:94), and the mock hard-codesstatus:'FAIL'for everymint-verify-*fork (mint.test.ts:65-70). The heal loop exhaustsHEAL_ITERATION_CAP = 2(index.ts:303) andindex.ts:325-334returns{paused:true, phase:'heal-failed'}beforerunShipPhaseatindex.ts:337.
So the assertion covers 6 of 8 sites while the PR body states it covers every phase.
suggestion: Assert an exact ctorCalls.length (or assert the set of idPrefix values seen), and make the mocked plan content carry ≥3 path tokens plus a passing verify mock so the run actually reaches parallelize and ship.
Note — the mock-leakage failure mode was checked and does not apply: vi.clearAllMocks() runs in the file's beforeEach (mint.test.ts:101), so the second test does not observe the first test's constructor calls.
Dimension summary
| Dimension | Verdict | Severity |
|---|---|---|
| Correctness | clean | — |
| Spec-compliance | clean | — |
| Test-coverage | 1 finding | medium |
| API-compat | clean | — |
| Security | clean | — |
| Perf-observability | clean | — |
Correctness — no issues found; read all 9 source files at fe2f3dc. Every new trailing param lands in the right positional slot. runSpecPhase is declared with 7 params (spec.ts), and index.ts:407 passes ..., defaultSubagentModel, undefined, ctx?.traceWriter — undefined correctly occupies the 6th parentReadRoots slot. runPhasesAfterSpec (7 params, index.ts:168) is called at index.ts:386 and index.ts:426, both with 7 in-order args. heal.ts:164-173 → runVerifyPhase (8 params, verify.ts:101) matches position-for-position. verify.ts:127-129's three forkVerifyMode calls each pass 10 args identically. In parallelize-dispatch.ts:111-116 the conditional spreads use distinct keys from the unconditional parentAbortSignal, so no key is clobbered. No argument shift anywhere.
Spec-compliance — no issues found. git ls-tree at the ref shows exactly 8 files in _phases/, and git grep 'new SubagentManager(' finds exactly 8 construction sites, one per file — every one carrying ...(traceWriter !== undefined ? { traceWriter } : {}). The "all 8 phases" claim holds at the source level. No scope creep: the diff is import + trailing param + spread, repeated, plus the two tests.
API-compat — no issues found (reachability pre-check performed). git grep across src/ for all 8 exported phase functions found zero production callers outside src/skills/mint/; the only other references are three mint test files. The "additive & backward-compatible" claim is corroborated by pre-existing call sites such as runSpecPhase('idea','sess',undefined,undefined,MODEL) (mint-phase-incomplete-guard.test.ts:166) remaining valid.
Security — no issues found; read src/agent/subagent.ts and src/agent/trace/{emit,types}.ts at the ref. traceWriter is a declared field on SubagentManagerOptions (no index signature), emitSubagentLifecycle no-ops on if (!writer) return; (emit.ts:64 — confirming the PR's stated premise), and SubagentLifecyclePayload is a narrow shape carrying systemPromptHash rather than raw prompt text. No credential or writer object enters the payload. This diff adds no new exposure surface; it wires an already-shipped mechanism (#466) into more call sites.
Perf-observability — no issues found. Each spread is a single O(1) conditional key add at manager-construction time, bounded to ~10 per mint run by HEAL_ITERATION_CAP = 2. Not per-token, not in a hot loop.
What was not checked
- Ref read: all Wave 1 citations were independently re-verified against branch HEAD
fe2f3dc. Verification manifest: 0 fabricated, 0 false-absent, 0 diff-only — every cited line exists at the reviewed ref. - Absence claims grounded by grep: the claim that no other test covers
traceWriterthreading forparallelize-dispatch.ts/ship.tswas checked withgit grep -c 'traceWriter' fe2f3dc -- src/skills/mint-parallelize-dispatch.test.ts src/skills/mint-failed-parallelize.test.ts→ zero matches in both. - Not executed:
pnpm test/pnpm lintwere not run. The PR's "55 mint tests pass, lint clean" claim is unverified here; all conclusions are static analysis against the reviewed ref. CI on this head is green except theTest (windows-latest)job, which is red across sibling PRs #698/#700/#701/#702 on this repo and appears pre-existing rather than introduced by this change — not independently confirmed. - Not reviewed: the runtime shape of emitted
subagent_lifecycleevents during a real mint run (no live trace corpus inspected), and the "mirrors #466 /user-skills.ts" claim beyond confirming the spread idiom matches. - The deferred
audit:trace-writerCI gate named in the PR body is explicitly out of scope and was not evaluated. It would mechanically prevent the recurrence class the PR describes.
Read-only review — no files, branches, or PR state were modified.
Why
/mint's 8 phase functions (src/skills/mint/_phases/*.ts) each construct their ownSubagentManagerwithout the session's trace writer:emitSubagentLifecycleno-ops without a writer (trace/emit.ts), so mint phase forks emitted zerosubagent_lifecycleevents — the last dispatch path still invisible in the witness trace after #466 fixed the agent-tool and compose paths. Confirmed by absence: no trace in the corpus ever carried amint-*agentType, even though the phases set one (e.g.agentType: 'mint-research').What
Thread
ctx.traceWriter— already available onSkillExecutionContext(skills/index.ts) and used identically byuser-skills.ts— from the mint handler throughrunPhasesAfterSpecinto every phase and into their fork managers:mint/index.ts— resolvectx.traceWriteronce, thread it torunSpecPhaseandrunPhasesAfterSpec, and from there to all 7 downstream phase calls._phases/*.ts(all 8) — accept an optional trailingtraceWriter?: TraceWriterand spread it into theSubagentManageroptions (mirrors the existingparentReadRootsthreading).heal.tsalso forwards it to its re-runrunVerifyPhase.Scope / notes
ctxdegrade to the prior no-writer behavior, unchanged.user-skills.ts— same pattern, samectx.traceWritersource.resolvedAgentTypeto the trace/routing telemetry: once both land, a mint run's phase forks are both visible AND type-attributed.audit:trace-writergate (mirroringaudit:env:check) asserting everynew SubagentManager(insrc/skills/src/agent/toolsthreads a writer would prevent this class of gap from recurring (it recurred once — roots fixed in fix(trace): make agent-tool and compose forks visible in the witness trace #466, mint missed).Test
mint.test.ts(the mocked-SubagentManagersuite) asserts every phase's fork manager is constructed with the writer whenctx.traceWriteris set, and without it when absent.pnpm lintclean; all 55 mint tests pass (53 existing + 2 new).