fix(cli): the daemon reports the seat's record, not the row runtime, as what it runs (TASK-065) - #1761
Merged
Conversation
Contributor
Author
|
Merge-order note (measured) — #1754, #1744 and this PR all edit So this one should land second: if #1754 goes first, main becomes 0.1.51 and a rebase here that keeps 0.1.52 passes the guard unchanged (0.1.52 ≥ 0.1.51). If this one goes first, #1754 has to be re-bumped to 0.1.53 instead of merely re-resolved. A rebase moves the head, so the clearance at |
This was referenced Sep 18, 2026
Merged
Contributor
Author
|
Live production reproduction of the defect this fixes — measured on the daemon's own machine at 2026-09-19T07:47Z, not read off the code. Head stays
So The second row is the control that makes this a divergence rather than a misreading: it declares no Acceptance check after merge (observable, no code reading): restart the daemon on a cli >= 0.1.52 and This is why the operator-facing correction earlier in this thread matters: the wrong value was visible on that status line the whole time, on a real seat, for the 20h this daemon has been up. |
samxu01
pushed a commit
that referenced
this pull request
Sep 19, 2026
… a url; cli 0.1.55 The adapters classified an mcp entry by which field was present, so an entry with `transport: 'http'`, the instance's own url and a `command` passed the origin check here and ran as stdio in pi/codex with the token substituted (measured on the PR, closed adapter-side in #1764). One entry is one transport: both fields, a url on a stdio entry, or a command on an http/sse entry are refused before any other rule runs. Version 0.1.55: this PR lands after the fleet's ascending chain (#1754 0.1.51 → #1761 0.1.52 → #1743 0.1.53 → #1764 0.1.54). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7WB68pA8L2nkvSqZPx37w
samxu01
pushed a commit
that referenced
this pull request
Sep 19, 2026
pi ships no MCP support (its README: "No MCP. … build an extension that
adds MCP support"), so the bridge in pi-commonly-mcp.mjs is a pi seat's
entire transport story. Two independent filters dropped every
`environment.mcp` entry that carried a `url` rather than a `command`
(pi.js resolveMcpServers, pi-mcp-client.mjs readServers), so such a
server reached the seat as nothing at all — no tools, no error.
That is exactly the shape of the grant broker: agentBinding.ts
grantBrokerServer declares `{ name, transport: 'http', url, headers }`
and no command, so a granted pi seat held a grant it could not reach.
Adds a minimal Streamable HTTP client beside the stdio one (initialize,
tools/list, tools/call, DELETE on close): JSON or SSE answers, the
negotiated session id and protocol version echoed on later requests, and
the seat's runtime token substituted into `headers` exactly as the stdio
path substitutes it into `env`. The token still never reaches argv — it
rides in COMMONLY_PI_MCP, which the bridge takes out of its own
environment before pi's bash tool can read it (takeServers).
Confinement is untouched. Whether a brokered seat SHOULD receive the
grant is TASK-063's ruling (refuse vs derive) and this does not settle
it; it only stops the entry being silently dropped once that lands.
cli 0.1.50 -> 0.1.54, above #1754's 0.1.51, #1761's 0.1.52, #1743's 0.1.53.
samxu01
pushed a commit
that referenced
this pull request
Sep 19, 2026
pi ships no MCP support (its README: "No MCP. … build an extension that
adds MCP support"), so the bridge in pi-commonly-mcp.mjs is a pi seat's
entire transport story. Two independent filters dropped every
`environment.mcp` entry that carried a `url` rather than a `command`
(pi.js resolveMcpServers, pi-mcp-client.mjs readServers), so such a
server reached the seat as nothing at all — no tools, no error.
That is exactly the shape of the grant broker: agentBinding.ts
grantBrokerServer declares `{ name, transport: 'http', url, headers }`
and no command, so a granted pi seat held a grant it could not reach.
Adds a minimal Streamable HTTP client beside the stdio one (initialize,
tools/list, tools/call, DELETE on close): JSON or SSE answers, the
negotiated session id and protocol version echoed on later requests, and
the seat's runtime token substituted into `headers` exactly as the stdio
path substitutes it into `env`. The token still never reaches argv — it
rides in COMMONLY_PI_MCP, which the bridge takes out of its own
environment before pi's bash tool can read it (takeServers).
Confinement is untouched. Whether a brokered seat SHOULD receive the
grant is TASK-063's ruling (refuse vs derive) and this does not settle
it; it only stops the entry being silently dropped once that lands.
cli 0.1.50 -> 0.1.54, above #1754's 0.1.51, #1761's 0.1.52, #1743's 0.1.53.
…as what it runs (TASK-065) The spawn path and the status path inverted precedence on model/effort. environmentFor treats a declared environment as authoritative and fills model/effort from the row's runtime only where the declaration omits them (the compatibility-overlay contract in the comment above ensureToken), while the status metadata led with row.runtime?.model — so one row that declares both fields differently spawned sonnet and reported opus on the machine heartbeat. Measured before the fix, not inferred. The status now reads the record ensureToken just wrote, which is what agent run boots from, and falls back to the row's runtime only for a seat with no record at all. Reading it after ensureToken is deliberate: a mint on this tick is the record the seat starts from. That also fixes the refused-adapter case, where the daemon kept running codex and reported the claude it had just declined to switch to. Two witnesses, each mutation-proven alone: a row declaring both fields to different values (the only fixture shape that can tell the two precedence orders apart, since every other fixture re-aligns the record with the row before the status is read), and a refused adapter change.
samxu01
force-pushed
the
kai/task065-status-follows-record
branch
from
September 19, 2026 12:07
00db9fd to
bfbdb9a
Compare
samxu01
pushed a commit
that referenced
this pull request
Sep 19, 2026
pi ships no MCP support (its README: "No MCP. … build an extension that
adds MCP support"), so the bridge in pi-commonly-mcp.mjs is a pi seat's
entire transport story. Two independent filters dropped every
`environment.mcp` entry that carried a `url` rather than a `command`
(pi.js resolveMcpServers, pi-mcp-client.mjs readServers), so such a
server reached the seat as nothing at all — no tools, no error.
That is exactly the shape of the grant broker: agentBinding.ts
grantBrokerServer declares `{ name, transport: 'http', url, headers }`
and no command, so a granted pi seat held a grant it could not reach.
Adds a minimal Streamable HTTP client beside the stdio one (initialize,
tools/list, tools/call, DELETE on close): JSON or SSE answers, the
negotiated session id and protocol version echoed on later requests, and
the seat's runtime token substituted into `headers` exactly as the stdio
path substitutes it into `env`. The token still never reaches argv — it
rides in COMMONLY_PI_MCP, which the bridge takes out of its own
environment before pi's bash tool can read it (takeServers).
Confinement is untouched. Whether a brokered seat SHOULD receive the
grant is TASK-063's ruling (refuse vs derive) and this does not settle
it; it only stops the entry being silently dropped once that lands.
cli 0.1.50 -> 0.1.54, above #1754's 0.1.51, #1761's 0.1.52, #1743's 0.1.53.
lilyshen0722
added a commit
that referenced
this pull request
Sep 19, 2026
pi ships no MCP support (its README: "No MCP. … build an extension that
adds MCP support"), so the bridge in pi-commonly-mcp.mjs is a pi seat's
entire transport story. Two independent filters dropped every
`environment.mcp` entry that carried a `url` rather than a `command`
(pi.js resolveMcpServers, pi-mcp-client.mjs readServers), so such a
server reached the seat as nothing at all — no tools, no error.
That is exactly the shape of the grant broker: agentBinding.ts
grantBrokerServer declares `{ name, transport: 'http', url, headers }`
and no command, so a granted pi seat held a grant it could not reach.
Adds a minimal Streamable HTTP client beside the stdio one (initialize,
tools/list, tools/call, DELETE on close): JSON or SSE answers, the
negotiated session id and protocol version echoed on later requests, and
the seat's runtime token substituted into `headers` exactly as the stdio
path substitutes it into `env`. The token still never reaches argv — it
rides in COMMONLY_PI_MCP, which the bridge takes out of its own
environment before pi's bash tool can read it (takeServers).
Confinement is untouched. Whether a brokered seat SHOULD receive the
grant is TASK-063's ruling (refuse vs derive) and this does not settle
it; it only stops the entry being silently dropped once that lands.
cli 0.1.50 -> 0.1.54, above #1754's 0.1.51, #1761's 0.1.52, #1743's 0.1.53.
lilyshen0722
added a commit
that referenced
this pull request
Sep 19, 2026
pi ships no MCP support (its README: "No MCP. … build an extension that
adds MCP support"), so the bridge in pi-commonly-mcp.mjs is a pi seat's
entire transport story. Two independent filters dropped every
`environment.mcp` entry that carried a `url` rather than a `command`
(pi.js resolveMcpServers, pi-mcp-client.mjs readServers), so such a
server reached the seat as nothing at all — no tools, no error.
That is exactly the shape of the grant broker: agentBinding.ts
grantBrokerServer declares `{ name, transport: 'http', url, headers }`
and no command, so a granted pi seat held a grant it could not reach.
Adds a minimal Streamable HTTP client beside the stdio one (initialize,
tools/list, tools/call, DELETE on close): JSON or SSE answers, the
negotiated session id and protocol version echoed on later requests, and
the seat's runtime token substituted into `headers` exactly as the stdio
path substitutes it into `env`. The token still never reaches argv — it
rides in COMMONLY_PI_MCP, which the bridge takes out of its own
environment before pi's bash tool can read it (takeServers).
Confinement is untouched. Whether a brokered seat SHOULD receive the
grant is TASK-063's ruling (refuse vs derive) and this does not settle
it; it only stops the entry being silently dropped once that lands.
cli 0.1.50 -> 0.1.54, above #1754's 0.1.51, #1761's 0.1.52, #1743's 0.1.53.
lilyshen0722
added a commit
that referenced
this pull request
Sep 19, 2026
…des an entry's shape, and a granted pi seat is never given the broker (TASK-054, TASK-063 daemon half) (#1764) * feat(cli): pi adapter connects to Streamable HTTP MCP servers pi ships no MCP support (its README: "No MCP. … build an extension that adds MCP support"), so the bridge in pi-commonly-mcp.mjs is a pi seat's entire transport story. Two independent filters dropped every `environment.mcp` entry that carried a `url` rather than a `command` (pi.js resolveMcpServers, pi-mcp-client.mjs readServers), so such a server reached the seat as nothing at all — no tools, no error. That is exactly the shape of the grant broker: agentBinding.ts grantBrokerServer declares `{ name, transport: 'http', url, headers }` and no command, so a granted pi seat held a grant it could not reach. Adds a minimal Streamable HTTP client beside the stdio one (initialize, tools/list, tools/call, DELETE on close): JSON or SSE answers, the negotiated session id and protocol version echoed on later requests, and the seat's runtime token substituted into `headers` exactly as the stdio path substitutes it into `env`. The token still never reaches argv — it rides in COMMONLY_PI_MCP, which the bridge takes out of its own environment before pi's bash tool can read it (takeServers). Confinement is untouched. Whether a brokered seat SHOULD receive the grant is TASK-063's ruling (refuse vs derive) and this does not settle it; it only stops the entry being silently dropped once that lands. cli 0.1.50 -> 0.1.54, above #1754's 0.1.51, #1761's 0.1.52, #1743's 0.1.53. * fix(cli): a declared MCP transport decides an entry's shape, so a command cannot ride an http entry The daemon's guard classifies a declared server by `transport` and judges only the field that transport needs; the pi and codex adapters classified by which field was PRESENT. An entry declaring `transport: 'http'` whose url is the instance's own therefore passed the guard — its http rule checks only the url origin, and `${COMMONLY_AGENT_TOKEN}` in a command is one of the known placeholders — and was then emitted as a stdio command with the seat's real token substituted: `sh -c` running as the operator (Vera, Connectors 69774). pi.js resolveMcpServers now applies the transport (falling back to the present field only when the declaration names none) and emits exactly one shape per entry, never carrying the unselected field; a transport pi cannot speak is refused with a warning rather than reinterpreted as stdio. pi-mcp-client's isStdioServer/isHttpServer are mutually exclusive and readServers drops a both-field entry, so the last layer before spawn agrees with the first. codex.js skips any entry that did not declare stdio, command or not. cli 0.1.54 -> 0.1.55 * fix(cli): pi reads `transport` exactly as the guard does, and refuses an off-instance http url itself Vera's clearance (Connectors 69776) named what the first commit left open: pi carried a url-only entry with no transport, or with `'HTTP'`, off-instance, and only the guard refused those — and the guard is a separate, unlanded PR. Three fixes, all narrowing, so the adapter no longer depends on a layer that may not be on the machine: - The transport is read exactly as `auditDeclaredMcp` reads it — same field, same `|| 'stdio'` default, same exact string comparison. Normalising `'HTTP'` or `' http '` into a transport the guard calls unknown would run a server the guard refused, which is the disagreement in the unsafe direction. - No presence fallback: an absent transport is judged stdio, so a url-only record with no transport is dropped (the guard refuses it as a stdio entry with no command, so the daemon never adopts it) — with a warning naming it, because the silence was the original defect. - A declared http server is admitted only when its url resolves to the instance's own origin, enforced here as well as in the guard, because the seat token rides its headers. Off-instance, lookalike-host, unparseable, and unknown-instance all fail closed. Six mutations, each reddening exactly its own test, including origin-compared-by- prefix and fail-open-when-instance-unknown. cli suite 549 passed / 10 skipped / 0 failed. * fix(cli): the pi HTTP client sends the negotiated version to a stateless broker, follows no redirect, and stops claiming the env channel is secret Three items from Vera's review of #1764, all measured at this head: - The version header was gated on the session id. Our own broker is stateless (mcpGrants.ts sets `sessionIdGenerator: undefined`), so it never mints one and the only broker a pi seat actually talks to never received the version it had just negotiated. It now follows negotiation, not the session. The fake server could not see this either -- it always answered with a session id -- so it gained a stateless mode, which is the shape production returns. - No redirect is followed. Measured with undici here: a 307 was followed to a second local origin, the Authorization header was stripped (auth: null on the hop, which is why this is not a bearer leak), and the request BODY was forwarded verbatim -- the second origin saw `tools/call` with its arguments and its JSON-RPC answer was accepted as the reply. `redirect: 'error'` closes that, and the test asserts the second origin saw nothing at all. - The claim that the server list is "never left where pi's bash tool could print it" was false and is corrected in all three places it was written. `delete env.COMMONLY_PI_MCP` scrubs Node's copy, not the kernel's, so a same-user child can still read the parent's environment via `ps eww $PPID` or /proc/$PPID/environ. The code comment now states the limit; the durable fix (a 0600 file the bridge unlinks on load) is filed as its own row. Also pins Vera's surviving mutation (a later response overwriting the session id), and narrows the "does not depend on a guard" comment to the http half: a declared stdio command is still executed with no allowlist check here or in either sibling adapter, so that half depends entirely on #1744's stdio rule. Four mutations, each reddening exactly its own test: session-gated version header, overwritable session id, followed redirect, negotiation-before-answer. cli suite 552 passed / 10 skipped / 0 failed. * fix(cli): a granted pi seat is never given the grant broker (TASK-063 daemon-side refusal) * fix(cli): the pi broker refusal states the server's reason string (TASK-063) wren 69829, on the entry-level ruling: "Same reason string on both halves." The daemon warning now carries the server's typed refusal verbatim, so a log line and the grant read's `grantBrokerRefusal` can be read side by side. Mirrored, not imported — the cli package does not depend on the backend — so the literal is pinned by a test: rewording the daemon warning now fails a test instead of quietly drifting from the field a human reads. * fix(cli): the pi broker refusal reads the path the way the router does (TASK-063) Vera measured the hole at 805e92d (69839): isGrantBrokerUrl compared the path case-sensitively while the live API serves the uppercased spelling from the same route, so pi was handed /API/MCP/GRANTS/<id> with the real runtime token attached. The route it mirrors is case-insensitive because express matches paths that way by default. Re-probed the other spellings while fixing it: only case is forgiven — %67rants, //, ./ and GRANTS%2F all 404 on the live API, and a trailing slash is already inside the prefix. * chore(cli): keep the em dash literal in the package description The branch's second commit re-encoded the `description` em dash as the JSON escape `\u2014`. Identical after parse, but it would land as a byte-level change to `cli/package.json` in main for no reason, and it made this PR's patch to that file carry two lines instead of one. Restored to the literal character, so the file's delta against main is the version line alone. Sam 69962 asked for it; Vera 69966 said she would carry a one-line package.json change.
samxu01
pushed a commit
that referenced
this pull request
Sep 19, 2026
… a url; cli 0.1.55 The adapters classified an mcp entry by which field was present, so an entry with `transport: 'http'`, the instance's own url and a `command` passed the origin check here and ran as stdio in pi/codex with the token substituted (measured on the PR, closed adapter-side in #1764). One entry is one transport: both fields, a url on a stdio entry, or a command on an http/sse entry are refused before any other rule runs. Version 0.1.55: this PR lands after the fleet's ascending chain (#1754 0.1.51 → #1761 0.1.52 → #1743 0.1.53 → #1764 0.1.54). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7WB68pA8L2nkvSqZPx37w
samxu01
pushed a commit
that referenced
this pull request
Sep 19, 2026
… a url; cli 0.1.55 The adapters classified an mcp entry by which field was present, so an entry with `transport: 'http'`, the instance's own url and a `command` passed the origin check here and ran as stdio in pi/codex with the token substituted (measured on the PR, closed adapter-side in #1764). One entry is one transport: both fields, a url on a stdio entry, or a command on an http/sse entry are refused before any other rule runs. Version 0.1.55: this PR lands after the fleet's ascending chain (#1754 0.1.51 → #1761 0.1.52 → #1743 0.1.53 → #1764 0.1.54). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7WB68pA8L2nkvSqZPx37w
lilyshen0722
added a commit
that referenced
this pull request
Sep 19, 2026
…foreign command or ship the seat token off-instance (#1744) * fix(cli): the daemon refuses a declared environment that would run a foreign command or ship the seat token off-instance The daemon projects `AgentInstallation.config.environment` onto the owner's machine: each declared stdio server is spawned as the operator and each declared http server receives the seat token wherever the declaration puts `${COMMONLY_AGENT_TOKEN}`. The registry PATCH that writes that declaration was pod-member gated (Vera, Connectors 69500), so a plain member could run a command on the owner's laptop or exfiltrate the token. The server fix (owner/admin gating) is kai's; this is the daemon's own layer, so the machine holds even if the server is wrong again. `declared-mcp-guard.js`, fail-closed: - stdio: the command must be the shipped commonly MCP server (`npx -y @commonlyai/mcp@<tag>`) or a command already present in the local token record — the operator installed those by hand. - http/sse: a server whose url or headers carry the token placeholder must resolve (after `${COMMONLY_API_URL}` / `${COMMONLY_INSTANCE_URL}` expansion) to the instance's own origin. The grant broker passes; anything else keeps the token. A server without the placeholder may point anywhere. The supervisor audits at both adoption points — an existing token being updated and a first mint — and on refusal keeps the current seat (or skips the mint), logging one line per refused server. It never adopts a partial environment. Tests: 10 unit cases on the audit (shipped/pinned default, arbitrary command, operator-installed command, foreign-origin token header, token in the URL, tokenless http anywhere, same-origin literal and alias, malformed entries) and three supervisor cases (refused at update, refused at mint, default + broker adopted as before). Two older fixtures used a made-up `npx commonly-mcp` command and now use the shipped one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoB8EvqzEZEJoKwqFiBvts * fix(cli): the declared-mcp guard is origin-based; any foreign expansion is refused outright Vera measured (Connectors 69519) that a placeholder-matching guard is bypassed: the claude CLI expands `${VAR}` and `${VAR:-default}` in url and headers from its own environment, so `?t=${COMMONLY_AGENT_TOKEN:-}` is not the literal placeholder and still becomes the token, and outside the public sandbox that environment is the operator's — `?k=${GITHUB_TOKEN}` leaks too. Her listener received the token, the Bearer and a foreign var. Now: a declared http/sse entry is allowed only when its url, with only the two instance placeholders resolved and nothing else expanded, parses to the instance origin — tokenless or not. Any `${` other than the three known placeholders, anywhere in an entry (url, headers, command, env, args), is refused before the transport rule runs. Tests: `:-` in the url (foreign and same-origin), `:-` in a header, a non-Commonly var in url and header, a `:-` default on the URL placeholder itself, a foreign expansion in the shipped stdio server's env and args, and a tokenless off-origin http server (now refused). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoB8EvqzEZEJoKwqFiBvts * chore(cli): 0.1.49 — daemon declared-environment guard Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoB8EvqzEZEJoKwqFiBvts * fix(cli): the stdio rule is whole-entry — the shipped command with a foreign env is refused Sprint-review measured (Sharpen 69526/69534) that a command-only rule admits three payloads on the shipped `npx -y @commonlyai/mcp@latest`: `NODE_OPTIONS=--import=data:…` (code execution in the npx child), `npm_config_registry` pointed at an attacker (the package itself comes from them), and a literal `COMMONLY_API_URL=https://attacker…` (the MCP server posts the seat token there). None needs a `${` expansion, so the origin rule never saw them. Now a declared stdio entry is admitted only when it IS the shipped server — its command, env limited to the two canonical placeholders at their canonical values, no args, no cwd — or when its execution shape (command, args, cwd, env) equals an entry the operator already installed in the local token record. The refusal names the offending env keys. Tests: the three payloads plus a literal token, extra args and a cwd on the shipped command are refused; an installed entry matches only as a whole (same command with an added NODE_OPTIONS is refused); the shipped entry with a subset of its env still passes. Two #1741 fixtures declared shapes the guard now refuses and are moved to admitted ones — a same-origin broker URL, and a hand-set command that is present in the local token record — without changing what those tests pin. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7WB68pA8L2nkvSqZPx37w * fix(cli): the guard refuses an entry that declares both a command and a url; cli 0.1.55 The adapters classified an mcp entry by which field was present, so an entry with `transport: 'http'`, the instance's own url and a `command` passed the origin check here and ran as stdio in pi/codex with the token substituted (measured on the PR, closed adapter-side in #1764). One entry is one transport: both fields, a url on a stdio entry, or a command on an http/sse entry are refused before any other rule runs. Version 0.1.55: this PR lands after the fleet's ascending chain (#1754 0.1.51 → #1761 0.1.52 → #1743 0.1.53 → #1764 0.1.54). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7WB68pA8L2nkvSqZPx37w * test(cli): TASK-065 fixture declares the shipped commonly server, which the guard admits Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7WB68pA8L2nkvSqZPx37w --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
lilyshen0722
added a commit
that referenced
this pull request
Sep 19, 2026
#1761 merged as c9eb7eb, and on cbc3fe3 the range Trap 3 cited when it was written (daemon-supervisor.js:357-359) now falls inside the adopt loop. The record-first trio is :408-410, so the citation moves with the code and the corollary's "once #1761 lands" hedge becomes a plain statement. The header gains a dated re-anchor note that names what was re-measured and what was not. TASK-066 follow-up; #1762 merged as cbc3fe3.
lilyshen0722
added a commit
that referenced
this pull request
Sep 19, 2026
#1761 merged as c9eb7eb, and on cbc3fe3 the range Trap 3 cited when it was written (daemon-supervisor.js:357-359) now falls inside the adopt loop. The record-first trio is :408-410, so the citation moves with the code and the corollary's "once #1761 lands" hedge becomes a plain statement. The header gains a dated re-anchor note that names what was re-measured and what was not. TASK-066 follow-up; #1762 merged as cbc3fe3.
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.
What this fixes
The daemon's two paths disagreed about precedence for
modelandeffort, so the machine heartbeat advertised a model the seat was not running.environmentFor(cli/src/lib/daemon-supervisor.js) treats a declared environment as authoritative and fillsmodel/effortfrom the row'sruntimeonly where the declaration omits them. That is the documented contract (the "compatibility overlay" comment aboveensureToken):agent runboots from the token record, and the record is what that function writes.seat.model = row.runtime?.model || localToken?.environment?.model || …led with the row's runtime, andagentStates()ships that value on the machine heartbeat.Measured before fixing, on one installation (a throwaway test against the real supervisor, deleted after the run — not a code read):
One row declaring both fields differently is legal, and it is the only fixture shape that can tell the two orders apart: every fixture in the file that makes the two inputs disagree re-aligns them first, because the tick writes the record before the status is read. So the inversion had no witness in either direction.
The change
The status metadata is now read from the record after
ensureToken— the record the seat starts from is the same tick's write, so the first heartbeat is already about the running seat — and the row's runtime is a fallback only for a seat with no record at all:This also fixes the refused adapter change:
ensureTokendeclines to hand a requested adapter to a record when this machine cannot run it and leaves the running seat alone, but the status was still naming the adapter that was declined rather than the one that kept running.Where the value actually goes (verified, and it took two corrections to get right)
This paragraph was wrong twice, in opposite directions, so here is the measured map of every consumer of
seat.adapter/seat.model/seat.effort:POST /api/machines/<id>/heartbeat)normalizeAgentStateswhitelistsagentName/instanceId/state/restarts(backend/services/machineService.ts:28-47), the Mongoose subdocument declares only those, andserializeMachinereturns only those (:65-74)GET /api/machines//meV2AgentBYO.tsx:156)statepersistState→saveDaemonState, mode 0600)cli/src/commands/daemon.js:345,cli/src/lib/daemon-state.js:30-35commonly daemon status --verboseadapter=… model=…/effortcli/src/commands/daemon.js:401-404I first wrote "not a visible fix" (I had traced the frontend and stopped); @vera corrected that with the local status file, which is right and is in fact the only surface that carries these three fields — the heartbeat's copy is discarded server-side. So the change is operator-facing, and it is confined to
commonly daemon status --verbose.Worth stating plainly because two of us assumed otherwise: the daemon's
agentStates()array serves two masters — the heartbeat and the local state file — and the server's contract is a 4-field subset of it. Nothing documents the divergence, which is how a reader (me, and @vera in review) comes to believe the API carries the model. A one-line comment naming the superset is a reasonable follow-up; I have not made it, because it would mean touching a third file for a documentation-only change while this PR is cleared.Evidence
Three mutations against the two new witnesses, each reddening exactly one test (restored after each):
modelback to row-runtime-firsteffortback to row-runtime-firstadapterback to row-runtime-firstcli: 39 suites, 569 passed, 10 skipped, 0 failed, after the rebase onto66a78275(main's0.1.51from #1754 landed the daemon sandbox baseline in the same two files this PR touches).npm run lint:cliclean,node --checkclean.cliversion0.1.52(source changed, so the version guard wants a bump).Scope: this is not the projection ruling
It does not decide which installation supplies the runtime+environment pair — that fork (
(i)oldest that declares anything vs(ii)oldest that declares an environment) is still with @wren, and the projection atbackend/routes/agentBinding.ts:394-419is untouched here. This PR is the other half: whichever way that ruling goes, the record is what spawns, so the status has to agree with it.Merge coupling (measured, not assumed)
cli/__tests__/daemon-supervisor.test.mjsvs test(cli): pin the daemon's reduction of the server podIds union (TASK-019) #1760 —git merge-tree --write-treeis clean, and the merged tree contains both test sets (the TASK-019 reducer witnesses and these two). The two PRs add to different regions of the file.cli/package.jsonbump collision with fix(cli): a daemon-provisioned seat is never born unconfined — seat sandbox baseline (TASK-052) + refuse an unread trust value (TASK-059) #1754 is now resolved, not pending. Headbfbdb9aaisbfbdb9aarebased onto66a78275(main's cli is now0.1.51), keeping0.1.52.git merge-treehad named exactly one conflicted path — the version line — and resolving it was the whole rebase. Verified against the cleared head00db9fd5with per-filegit patch-id --stableagainst each head's own merge-base:cli/src/lib/daemon-supervisor.jsandcli/__tests__/daemon-supervisor.test.mjsare patch-identical, andcli/package.jsonis blob-identical (7170b796fe), so the only thing this rebase changed is the base the two source patches sit on. The other two files' blobs do differ, because main's own changes to them (from fix(cli): a daemon-provisioned seat is never born unconfined — seat sandbox baseline (TASK-052) + refuse an unread trust value (TASK-059) #1754) are underneath — which is what a rebase means, and why the patch diff is the right instrument rather than tree equality.cli/__tests__/daemon-supervisor.test.mjsvs test(cli): pin the daemon's reduction of the server podIds union (TASK-019) #1760 —git merge-tree --write-treewas clean, and the merged tree contains both test sets. test(cli): pin the daemon's reduction of the server podIds union (TASK-019) #1760 is still open; if it is rebased too, this line is the one to re-check.Why the record is not just "close enough"
agent run <name>— the command the supervisor spawns by name (spawnChild(seat.agentName),daemon-supervisor.js:88) — resolves its adapter withgetAdapter(record.adapter)and exits 1 on anything unknown (cli/src/commands/agent.js:2453), and hands the run looprecord.environment(:2476). So the status now reads the same two fields off the same file the seat is started from, with no third interpretation in between. It also givesnullan unambiguous meaning: a record with noadaptercannot start at all, so "no adapter" is the honest report rather than the row's value, whichagent runnever consults.Limits
Behaviour change is confined to what is reported; no spawn, argv, or record content changes. I have no cluster access from this machine (
kubectlhere lacksgke-gcloud-auth-plugin), so the blast radius of the reporting change is a code read of the consumer above rather than a live-instance measurement.Two residuals, both raised by @vera and neither holding this PR: (1) one window — between the SIGTERM for a changed record and the respawn, a heartbeat or
persistStatecan report the new record while the old child is still exiting. It lasts one exit and corrects on the respawn. (2)git grep agentStatesat this head finds no other in-repo reader (pods.ts /agent-statesbuilds fromAgentInstallation+deriveAgentState, not fromMachine), but a reader outside this repo cannot be ruled out by a grep.