[minsky-ai]

Summary

Two mt#3377 tests in src/cockpit/driven-session-host.test.ts — "production default provisions the minsky MCP server" and "a resume re-provisions the MCP config rather than dropping it" — asserted Object.keys(parsed.mcpServers) equals exactly ["minsky"]. Since mt#4239 the payload also INHERITS entries from the daemon checkout's real .mcp.json (readOperatorMcpServers(process.cwd()/.mcp.json), default set ["minsky", "github"]), so the full key list is a function of the machine running the tests: CI has no .mcp.json and passes; any operator machine with a github server fails both with received ["github", "minsky"]. Found by mt#5097's local full-suite run — the only two failures in 18,369 — and reproduced identically under bun 1.3.14 and 1.4.2, so this is not a runtime change.

Key Changes

• src/cockpit/driven-session-host.test.ts (only file): both sites now assert the HOST's own contract, which holds on every machine — minsky is present, it is the LAST key (mt#4239 synthesizes it after the inherited entries so nothing inherited can shadow it), and its args carry the session cwd as --repo (unchanged line). A comment at each site explains why the exact set is not asserted.
• No production change. The inherited set is the resolver's contract and is already covered with an injected reader in driven-session-mcp-config.test.ts (AT1/AT2, "an inherited minsky entry can never shadow the synthesized one").

Why not inject the server set (the spec's original SC1): startDrivenSession reaches the resolver via ClaudeStreamJsonTransport.spawn → resolveMcpConfigForSpawn → resolveDrivenSessionMcpConfig(repoPath, { names }), which passes no readServers/sourceDir. The host-level seams are mcpConfig (bypasses synthesis — the thing under test) and mcpServerNames. Threading a reader seam through driven-session-host.ts → driver-transport.ts → claude-transport.ts on both the start and resume paths for two assertions is disproportionate, and the file already tolerates the analogous machine-dependence of invocation.command (the bun binary path) by not asserting on it. Recorded in the spec's ## Planning Audit (READY); SC1 was amended at planning to the contract-assertion form.

Alternative weighed and rejected: mcpServerNames: ["minsky"] pins the set deterministically but changes what the test proves (an explicit set rather than the production default) and still reads the real file.

Testing

Execution evidence:

• AT1 — on the operator machine (grep -c '"github"' .mcp.json → 1), in the session workspace:

  bun test --preload ./tests/setup.ts --timeout=15000 src/cockpit/driven-session-host.test.ts
  94 pass
   0 fail
  Ran 94 tests across 1 file. [155.00ms]
  

• AT2 — CI on this PR (no .mcp.json on the runner; the assertion is invariant across both cases).
• SC1 — the two assertions at the former :206 and :901 now read expect(serverNames).toContain("minsky"); expect(serverNames.at(-1)).toBe("minsky"); with the existing toContain(SCRATCH_CWD) line kept.
• SC2 — full gated suite on this machine (MINSKY_PREPUSH_FULL_SUITE=1 bun scripts/run-tests-gated.ts, session workspace):

  → MINSKY_PREPUSH_FULL_SUITE=1 — running the full suite.
  → Main suite (scripts/run-tests-main.ts, src/mcp excluded)...
   18359 pass
   10 skip
   0 fail
  Ran 18369 tests across 1175 files. [317.36s]
  → src/mcp (scripts/run-tests-mcp-isolated.ts, per-file isolation)...
  (49 files, every tally 0 fail)
  run-tests-gated.ts: all test steps passed.
  EXIT=0
  

  Before this change the same invocation on this machine was 18,357 pass / 2 fail (mt#5097 ## Outcome); the 10 skips are pre-existing (18,357 + 2 + 10 = 18,369).
• SC3 — see the negative control below.
• SC4 — comment present at both sites (start-path site carries the full explanation; resume-path site points at it).

Negative control: the un-fixed assertions on the same machine (main checkout at 1c4eba401, pre-change tree) — bun test --preload ./tests/setup.ts --timeout=15000 src/cockpit/driven-session-host.test.ts → 92 pass / 2 fail, both expect(received).toEqual(expected) at driven-session-host.test.ts:206 and :901 with diff + "github" above "minsky". That is the full pre-fix state (the change is the test itself), not a partial revert.

Defect class: a test asserting an exact value derived from real machine state through a production default. Other members checked: every other mcpServers key assertion under src/ is either fixture-backed (driven-session-mcp-config.test.ts injects readServers) or in an unrelated module (src/mcp/setup/local-http-*); the --strict-mcp-config test in this file asserts flag presence only. No other site reaches the real .mcp.json.

Typecheck: pass (8 projects, session workspace). Lint: pass (4,460 files, session workspace).

Deploy verification: src/** is a push trigger for deploy-minsky-mcp.yml (the image COPYs the whole src tree, tests included), so this merge redeploys minsky-mcp with no behavioural change. Post-merge: deployment_wait-for-latest(service: "minsky-mcp", notBefore: <mergeDate>, expectCommitSha: <mergeSha>) → SUCCESS with buildIdentity read, then /health asserting service: minsky-mcp. Recorded on the task spec.

🤖 Generated with Claude Code

https://claude.ai/code/session_0147JJ3SVgqRz9JyUD4NZBz9

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 123K prompt, 4K completion | Duration: 57s
Mode: test-only

Commands

• /review — request a fresh review

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Scoped, test-only PR that replaces environment-dependent exact-set assertions with host-contract assertions looks correct and matches the task spec. I verified the two modified sites now assert minsky presence, last-key position, and preserved --repo arg; explanatory comments are included. One non-blocking suggestion: relying on key order via Object.keys().at(-1) can be brittle — consider strengthening the check by asserting the synthesized minsky entry’s distinguishing shape in addition to presence. No production behavior changed; no docs impact. Approving.

Findings

• [NON-BLOCKING] src/cockpit/driven-session-host.test.ts:210 — Order-based assertion on Object.keys can be brittle; consider asserting synthesized minsky shape instead of relying on key order
  The tests now assert serverNames.at(-1) === "minsky" (src/cockpit/driven-session-host.test.ts:210 and :909). While JS preserves string-key insertion order for Object.keys, this couples the test to the serialization/build order of the payload. A future refactor that builds the object via a sorted entries path (or merges maps in a different order) would fail this test even if the host contract still holds (i.e., inherited minsky cannot shadow synthesized minsky). Consider also asserting on the synthesized minsky entry's distinguishing properties (e.g., that parsed.mcpServers.minsky.args includes --repo with SCRATCH_CWD, and optionally that its type/command matches the synthesized local-command shape) to reduce reliance on key order. This is advisory; current assertions are valid given today's builder guarantees.

Spec verification

Criterion	Status	Evidence
Both tests stop asserting the exact mcpServers key set and assert the HOST's own contract instead — which holds on every machine: the synthesized payload carries a minsky entry; it is the LAST key (mt#4239 writes it after the inherited entries so it always wins); its args carry the session cwd as --repo. The inherited set is NOT asserted here: it is the resolver's contract, covered with an injected reader in src/cockpit/driven-session-mcp-config.test.ts (AT1, AT2, "an inherited minsky entry can never shadow the synthesized one"). (Amended at planning, 2026-09-11 — the original "inject the operator server set" form is not reachable from these tests; see ## Planning Audit (READY) → gate (b).)	Met	src/cockpit/driven-session-host.test.ts:206-216 and :901-911 replace toEqual(["minsky"]) with toContain("minsky") and expect(serverNames.at(-1)).toBe("minsky"), and retain expect(parsed.mcpServers.minsky.args).toContain(SCRATCH_CWD).
The full gated suite (MINSKY_PREPUSH_FULL_SUITE=1 bun scripts/run-tests-gated.ts) passes on a machine whose .mcp.json declares a github server — the condition that fails today.	Unverifiable	Execution outcome is not present in the diff and cannot be verified from repository contents alone. Criterion depends on a local machine state and CI run outside this PR.
Negative control: the un-fixed assertion, run on such a machine, observed failing with received ["github", "minsky"]. (Observed at planning, 2026-09-11 18:30Z: 92 pass / 2 fail at driven-session-host.test.ts:206 and :901, both + "github"; grep -c '"github"' .mcp.json → 1. The implementer restates this in the PR body's test-first evidence — mt#3244 requires the negative control there.)	Unverifiable	This is an execution observation, not encoded in the repo. The PR description claims it, but it cannot be verified from the diff.
A one-line note in the test explains why the exact key set is not asserted (mt#4239's inheritance makes it a function of the daemon machine's .mcp.json).	Met	src/cockpit/driven-session-host.test.ts:206-213 and :903-906 include explanatory comments stating inheritance from the daemon checkout’s .mcp.json and why the exact set is not asserted.

Documentation impact

• no-update-needed — Test-only change in src/cockpit/driven-session-host.test.ts adjusting assertions; no production behavior or public API changed. No docs updated in this PR and none appear necessary.

[github-actions]

Cockpit Preview

Preview URL: https://cockpit-preview-production.up.railway.app

Deployed from commit 083788b32.
Preview runs in read-only mode — mutations are disabled.

Updated by Cockpit Preview.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verification round: the prior non-blocking concern stands (order-based key assertion), but the test-only changes meet the spec. Both modified sites now assert the host contract (minsky present, last key, and --repo arg contains the session cwd) and include explanatory comments. No production code touched; no critical new defects introduced. I recommend optionally strengthening the assertion by relying less on key order, but this is advisory. Approving.

Findings

• [NON-BLOCKING] src/cockpit/driven-session-host.test.ts:214 — Order-based assertion on Object.keys().at(-1) may be brittle; consider asserting synthesized minsky shape too
  The revised tests assert serverNames.at(-1) === "minsky" (start path at src/cockpit/driven-session-host.test.ts:214 and resume path at :909). While this matches today’s builder guarantee (synthesized minsky appended after inherited entries), it couples the tests to key order. A future refactor that constructs mcpServers via sorted entries or a different merge order would fail these tests even if the host contract ("inherited minsky cannot shadow synthesized minsky") still holds. Consider additionally asserting on the synthesized minsky entry’s distinguishing properties — e.g., that parsed.mcpServers.minsky.args includes --repo with SCRATCH_CWD (already present) and, optionally, its type/command shape — and relaxing the last-key assertion to presence + shape if order is not essential.

Spec verification

Criterion	Status	Evidence
Both tests stop asserting the exact mcpServers key set and assert the HOST's own contract instead — which holds on every machine: the synthesized payload carries a minsky entry; it is the LAST key (mt#4239 writes it after the inherited entries so it always wins); its args carry the session cwd as --repo. The inherited set is NOT asserted here: it is the resolver's contract, covered with an injected reader in src/cockpit/driven-session-mcp-config.test.ts (AT1, AT2, "an inherited minsky entry can never shadow the synthesized one"). (Amended at planning, 2026-09-11 — the original "inject the operator server set" form is not reachable from these tests; see ## Planning Audit (READY) → gate (b).)	Met	src/cockpit/driven-session-host.test.ts:206-216 and :901-911 replace toEqual(["minsky"]) with expect(serverNames).toContain("minsky") and expect(serverNames.at(-1)).toBe("minsky"), and retain expect(parsed.mcpServers.minsky.args).toContain(SCRATCH_CWD); comments explain the contract-based assertion.
The full gated suite (MINSKY_PREPUSH_FULL_SUITE=1 bun scripts/run-tests-gated.ts) passes on a machine whose .mcp.json declares a github server — the condition that fails today.	Unverifiable	Execution outcome depends on local machine state and CI; not present in the diff. Cannot be verified from repo contents.
Negative control: the un-fixed assertion, run on such a machine, observed failing with received ["github", "minsky"]. (Observed at planning, 2026-09-11 18:30Z: 92 pass / 2 fail at driven-session-host.test.ts:206 and :901, both + "github"; grep -c '"github"' .mcp.json → 1. The implementer restates this in the PR body's test-first evidence — mt#3244 requires the negative control there.)	Unverifiable	Pre-change execution evidence is not encoded in the repo; cannot be verified from this diff.
A one-line note in the test explains why the exact key set is not asserted (mt#4239's inheritance makes it a function of the daemon machine's .mcp.json).	Met	src/cockpit/driven-session-host.test.ts:206-213 and :903-906 include explanatory comments about inherited entries from the daemon checkout’s .mcp.json and why the exact set is not asserted.

Documentation impact

• no-update-needed — This PR modifies only tests (src/cockpit/driven-session-host.test.ts) to assert contract-based behavior; no production code or public API changed. No relevant docs are impacted or invalidated.