feat(spawn): honor the persona file's agent: harness pin - #911
Conversation
A persona declaring 'agent: jcode' was silently launched on whatever the env or product default chose: the key swept into the verbatim meta bag (no code path read it) and both launch paths resolved the connector before loadAgentFile ran. The harness now resolves once on every spawn path as: --agent flag > persona agent: > COTAL_DEFAULT_AGENT > default, matching model/variant precedence. - core: AgentDef.agent modelled in loadAgentFile/saveAgentFile - cli: foreground consults def.agent; --detach threads only an explicit flag across the control plane (flag and env collapsed into one field before, making file precedence unreachable manager-side) - manager: startAgentActive loads the persona before resolving the connector (single load, consumed downstream) - spawnRequiredExtensions consults the pin for pre-materialization - a pin naming an unregistered connector fails loud (no fallback) - smokes: loader round-trip, manager precedence, and a reachability suite driving the real CLI/manager over a throwaway broker (#869)
Six mutations against the #869 fix, each naming the assertion it must redden: the manager precedence line (restores the shipped env-beats-file defect), the CLI control-plane collapse (pre-#869 flag/env merge), the foreground drop, the requiredExtensions drop, the loader meta-sweep, and the manager unit cell. Commands build the workspace packages first and afterRestore rebuilds, because every graded suite resolves dist/.
…arker The loader mutation's expectRed was a substring of the GREEN line, so the grader correctly refused to count it (WRONG-RED). The manager-unit mutation inherited the reachability suite's completion marker while running start-overrides, which prints a different trailer; it now names its own.
With agent: swept back into meta, the parse cell still passes (meta is unread by it) while the meta-purity cell fails, so the proof names that one. The unit suite's completion marker is its failure banner: under the mutation it stops at the red cell and never prints the green trailer.
The agent-file smoke prints its summary on both pass and fail (with the count), so the marker is the summary line, not a success-only trailer.
…frozen The gate requires every ungated suite to carry a reason. Gating this one means inserting into bin/smoke/ci-suites.txt, which is frozen by position until PR #880: the shard walk is round-robin by index, so a mid-file insert re-shards every later suite across CI runners. Declared in UNGATED with that reason instead; one-line reversal when the freeze lifts.
The known-set mutation alone still parses def.agent (the return line is independent), so a revert of the field to unmodelled status has two layers: stop parsing it into the modelled field (kills the parse cell), and drop it from the known-set so meta re-captures it (kills the meta-purity cell). Plus the writer mutation: a saveAgentFile that stops emitting the pin makes every redefine silently drop it, which is the runtime redefine path (cotal_persona), not just first spawn.
check:docsbundle regenerates the bundle and diffs, so the docs edits ride with their generated copy.
|
CI note: the two red smoke shards fail in |
The repo rule bans them in text landing on GitHub; the changeset rides into CHANGELOG.md verbatim, so it is public writing like the PR body. Punctuation chosen per sentence, not blind-replaced; pre-existing style elsewhere in these files is untouched.
Re-applies the changeset, agent-files, and connectors fixes lost from the previous punctuation commit (only the cli.md edit survived it). Chosen per sentence; pre-existing style elsewhere untouched.
The edit tool's write to this file did not persist twice; applied and verified byte-level before committing.
BLOCKGraded head Model line: GPT-5.5. I am independent of the authoring model line for this PR. Blocking finding
This is foreground-only because The reachability smoke covers pre-materialization only while running from the workspace root. It does not cover a foreground spawn from outside the target root with What would change my mindMake pre-materialization resolve the same target root and persona reference as the spawn body, failing loudly on resolution errors, or defer extension materialization until after the authoritative persona load. Add a foreground test from outside the target root using Checked
Not checkedI did not run mutation-proof, aggregate suites, live suites, or any shared stack command. CI conclusions were not used as execution evidence. |
|
APPROVE at 14b6ce4 Reviewer model: Grok 4.6 I re-resolved this head independently. What I checked, from git objects at this SHA (no checkout of this tip, and I did not run the docs-bundle generator in any working tree):
What I did not:
Open finding: personas that already declare Verdict: APPROVE. |
|
Superseded by a fuller two-part version below, which separates my independent verdict from my BLOCK Head graded: Model: claude (opus). Named gap up front: my worktree sits at a commit adjacent to PR #908's head, not at this PR's head, and I could not check out this head to run its suite (checkout is outside what my seat may do). I read the full diff, the full mutation fixture, and the full new smoke test source ( What checks out. The blocking finding. Why this is not academic. The PR's own doc comment on What would change my mind: making I did run |
|
Posting in two parts per request: what I concluded independently before any disclosure, and my position now, including whether I re-derived or accepted the finding that changed my verdict. Model: claude (opus). PART 1: INDEPENDENT VERDICT, before disclosure Head graded: Verdict at that head: APPROVE. I found no defect on my own pass. What I checked. I read the new reachability suite ( I ran PART 2: POSITION NOW Head re-resolved just now: Position: REVERSED, from APPROVE to BLOCK. I was told the shape of a finding by another reviewer's report: a pre-materialization path that can abort a spawn before the correct persona loads. I did not accept that report on its authority. I opened What I re-derived and confirmed directly: I then independently confirmed the consequence, which was not handed to me: I also checked the existing reachability suite against this specific gap myself, which was not part of what I was told: its four cells spawn from a single scratch workspace root with no registry entries and no So: I was told where to look, and I re-derived the finding at the artifact rather than restating a description of it. The parts of part 1 that already checked out (schema, backward compatibility, precedence parallelism, the manifest path) still hold on this pass and are not affected by the block, which is scoped to one function. What would change my mind: |
spawnRequiredExtensions read the persona via the cwd walk pre-parse to pre-materialize the pinned connector. That introduced a root divergence the pre-#869 hook never had: resolveMeshTarget ranks --space, --server, and the selected current ABOVE the cwd walk, so a spawn issued from a directory outside the target could pre-materialize the WRONG connector, and materializeExtension throws on an uninstalled extension, hard- aborting the command before the body loads the correct persona. The registry then lacks the pinned connector and the operator is told the harness they wanted is unregistered. The hook is root-free again, exactly as before #869; the spawn body materializes the chosen connector immediately after the authoritative persona load and before registry.resolve. One resolver, one root. Reachability cell E makes the roots actually differ (registry-named space vs foreign cwd) and asserts the target persona's pin builds the launch with no confusing abort.
Cell E enables installed-extensions mode to reproduce the published- binary abort path. Manifest resolution reads globalConfigDir(), which follows XDG_CONFIG_HOME rather than COTAL_HOME, so the cell points it at a scratch dir with an empty manifest: the operator's real extensions.json is never consulted, and an unknown connector throws from the scratch manifest. Read-only in practice before, but a suite that names the live path at all is the contamination class.
|
Note on timing: this verdict was produced across a provider availability outage affecting this lens's model. Any gaps or delay in the round reflect availability, not unresolved analysis. Verdict: BLOCK (tests and evidence lens). My position reversed after being told of a defect on the pre-materialization path; the finding was then re-derived first-hand rather than accepted, and the reversal is on the re-derived mechanics, which are worse than the reported framing. This comment states both positions plainly. PART 1. What I concluded independently, before any disclosure, at head 6c938dc (the brief's originally claimed head was stale; I re-resolved and graded the current one at that time):
PART 2. My position now, and how I got there:
Heads: graded at 6c938dc, re-resolved at the current head 14b6ce4 (negative control on an invented tail for both). The block applies to 14b6ce4; the intervening commits change only docs punctuation. Model line: glm-5.3, the same family that authored this PR, so treat this verdict accordingly where independence matters: Part 1 was independent in process (no disclosure), Part 2 was disclosure-triggered but re-derived at the artifact before I changed position. Named gaps: the live detached spawn path against a running shared manager was not exercised (forbidden here); CI was not read, so no CI result is cited. |
Cell E must drive the real runCli dispatcher with installed-mode extension resolution on, an empty scratch manifest, and no pre-registered connectors, so the refusal it grades is produced by the same path the published binary takes. The recorders are unregistered for the dispatch and restored right after; the exit the refusal path takes is intercepted so the suite survives to grade it.
The runCli boot reconciles seeded connectors before dispatch, and in a fresh XDG root a first run stages and installs the default connectors. That erases the not-installed world cell E grades and made the cell flaky. Borrow the same skip the published binary uses for its internal children.
…emption The position freeze that justified the UNGATED entry was repealed on main (tail appends are the established pattern and move zero shards), so the exemption asserted a constraint that no longer exists. Appended at the file tail (round-robin shard walk: zero existing suites re-sharded) and removed the obsolete entry.
1 participant
What
A persona file declaring
agent:in its frontmatter is now honored when the spawn harness is chosen. The precedence is explicit--agent> personaagent:> invoking CLICOTAL_DEFAULT_AGENT> managerCOTAL_DEFAULT_AGENT> product default. Fixes #869.Why it was broken
The manager resolved its connector before loading the persona file, so the file could not participate in harness selection.
AgentDefalso had no modelledagentfield, and the detached CLI collapsed an explicit flag and the environment default into one control-plane value.The first fix separated the explicit flag but dropped the caller's default. That changed detached behavior when the invoking CLI set
COTAL_DEFAULT_AGENTand the already-running manager did not. An unpinned persona then used the manager or product default instead of the caller's default.The fix
AgentDef.agentis modelled.loadAgentFileparses it as a known field, andsaveAgentFileround-trips it so persona redefinition preserves the pin.spawnDetachedsendsagentanddefaultAgentseparately. The caller default keeps its default semantics without becoming an explicit override.startAgentActiveloads the authoritative persona once before connector resolution. Harness precedence isopts.agent ?? def?.agent ?? opts.defaultAgent ?? defaultAgentType(DEFAULT_CONNECTOR).defaultAgent, and shared validation refuses an empty value.values.agent ?? def.agent ?? defaultAgentType("claude").spawnRequiredExtensionsremains root-free. Foreground connector materialization happens only after the authoritative target persona is loaded.Reachability proof
pnpm smoke:persona-agentexercises the real entry paths with a throwaway broker, a real manager, the kernel-parsed CLI, and a separate child CLI process.All 14 assertions pass. They cover:
COTAL_DEFAULT_AGENTwhile the running manager lacks itThe caller-default cell is discriminating. On the previous public head it failed with
no connector registered for "claude"while the caller requestedother.Mutation proof kills all 10 mutations in
bin/smoke/mutations/persona-agent.jsonon their named assertions. The two caller-default mutations kill removal of the dedicated field and manager-before-caller mis-precedence on the same real detached cell.Also green on the current-main descendant:
pnpm smoke:agent-file: 41 assertionspnpm smoke:start-overrides: 34 assertionspnpm smoke:manager-service-ops: 62 assertionspnpm smoke:launch-paritypnpm check:docsbundlepnpm changeset statuspnpm check:shard-stability origin/main HEAD: the new smoke is appended at the true suite tail and moves zero pre-existing suitesThe feature and gate surfaces were independently reviewed at exact head
b46f09ffby four model families: cold DeepSeek APPROVE, contract GLM PASS, implementation GPT PASS, and tests Grok APPROVE. Final merge commit972411c3then integrates current main81b4e1f; its only tree delta from the reviewed head is the.internalsubmodule pointer already present on main. Every reviewed product, test, documentation, package, and generated file remains byte-identical tob46f09ff.Compatibility
agent:now use that pin. This is the intended behavior change.COTAL_DEFAULT_AGENTeven when the manager environment differs.defaultAgentis an imperative manager control field. It does not enter@cotal-ai/core,AgentCard, SPEC.md, orspec/cotal.schema.json.AgentCard.meta.connectorremains derived from the live session and cannot be spoofed by the file.agent-files.md,connectors.md,cli.md, andconfig.md.Closes #869