[minsky-ai]

Summary

Two defects in the cold-agent onboarding harness (mt#5012) and the .claude.json resolvers it depends on.

Defect 1 — a hardcoded $HOME/.claude.json at every resolver. Claude Code relocates .claude.json under CLAUDE_CONFIG_DIR when that variable is set (measured on 2.1.258 during planning; the vendor docs describe the variable but are silent on this file). Every Minsky resolver ignored the variable: ClaudeCodeRegistrar.configPath (packages/domain/src/mcp/registration.ts), local-http discovery (src/mcp/setup/local-http-config.ts) and revert candidates (local-http-apply.ts). Under a relocated config dir — which is exactly what the sandbox sets — the registrar wrote the operator's real file and the discovery/revert paths read it. The harness inherited the gap from the other side: its channel-5 probe enumerated only the customizations directory, so a sandbox whose .claude.json carried minsky-server read ISOLATED (the 2026-09-10 run's shape).

Defect 2 — the scrubbed transcript was written inside the temp sandbox root, so two existing run records (run-1788898295224, run-1789019061415) pointed into /var/folders/…/T/, which is not durable.

Changes

• New packages/domain/src/mcp/claude-code-paths.ts — resolveClaudeJsonPath(env = process.env, home = homedir()), the single domain resolver, extending the resolveClaudeSessionsDir pattern. Registrar, discovery (DiscoveryOptions.env) and revert (revertCandidates(projectRoot, home, env)) consume it.
• scripts/cold-agent-onboarding-run.ts — channel 5 is customizations dir empty AND .claude.json carries no user-scope MCP servers; userScopeMcpServerNamesIn(file) reads NAMES only (never values), absent → [], unparseable → UNPARSED:<basename> (opens the channel). On --execute, the operator's .claude.json server names are snapshotted before spawn and a failure is pushed if any were added (RunRecord.operatorClaudeJsonNewServerNames). The transcript lands at transcriptPathFor(outDir, nowMs) beside its record; the scrub step stays ahead of the write. The harness resolves the file with its own claudeJsonPathUnder(env, home), pinned to the domain resolver by test (R1, below).
• Comment-only: harness-detection.ts, driven-session-mcp-config.ts mention the relocated path.

Deploy verification

Deploy surface: registration.ts, local-http-*.ts, harness-detection.ts, driven-session-mcp-config.ts are all isDeploySurfaceFile = true (checked over the full changed set). Affected services: minsky-mcp, reviewer. After merge: mcp__minsky__deployment_wait-for-latest for both services with notBefore = merge time and expectCommitSha = the merge commit, workflow-run correlation, and a /health body read confirming the running build's identity — SUCCESS plus runtime started, recorded on the task spec. A tool/auth flake is a blocker to reconnect and retry, not a deferral.

Execution evidence

Per success criterion (mt#5066 ## Success Criteria):

• SC1 (channel 5 observes the relocated .claude.json; names only; write-side snapshot on --execute) — live assert-mode run in the session (re-run after R1): ISOLATED 5/e user-level Claude Code customizations — config dir carries no customizations; .claude.json carries no user-scope MCP servers. Run records run-1789157442103.json (R0) and run-1789158429588.json (R1) under ~/.local/state/minsky/cold-agent-runs/; key set enumerated with jq keys — operatorClaudeJsonNewServerNames present and null (assert mode; populated only on --execute). Names-only: scripts/cold-agent-onboarding-run.test.ts asserts a fixture value s3cret never appears in userScopeMcpServerNamesIn's output.
• SC2 (the widened probe can FAIL — negative control OPEN) — same runs: OPEN 5/e … .claude.json user-scope MCP servers: minsky, minsky-server.
• SC3 (transcript durable, beside its record, scrub ahead of write) — transcriptPathFor test: path is <outDir>/transcript-run-<nowMs>.txt, not under mt5012-cold-; the --execute path scrubs, then mkdirSync(outDir), then writes.
• SC4 (existing records backfilled) — done on the authoring machine before this PR: transcript-run-1788898295224.txt copied from the still-present temp file (264,963 bytes), transcript-run-1789019061415.txt confirmed byte-identical to its temp source (cmp, 588,179 bytes), both records' transcriptPath rewritten and confirmed to resolve; the newly copied transcript passes minsky security check-credentials --quiet (exit 0). Recorded on the task spec (## Implementation record). The harness docblock names an out-of---out-dir transcriptPath as a pre-mt#5066 record.
• SC5 (Minsky's writers resolve like Claude Code) — claude-code-paths.test.ts (3), registration.test.ts new case expecting /sandbox/claude-config/.claude.json, local-http-config.test.ts relocated-file discovery case, local-http-apply.test.ts revert with { CLAUDE_CONFIG_DIR: "/sandbox/cfg" }: 156 pass / 0 fail across the four files.

Negative control (test-first, mt#3244):

• Harness channel-5 cases ("channel 5 is OPEN on a user-scope MCP registration even when the customizations dir is clean", "an unreadable .claude.json opens the channel") observed FAILING pre-fix: Expected: false Received: true on verdict.isolated.
• Resolver cases observed FAILING pre-fix: registrar returned /Users/edobry/.claude.json under CLAUDE_CONFIG_DIR=/sandbox/claude-config; discovery read the HOME-root file — 2 fail before the fix, 156 pass after.

Other verification in the session workspace (after R1): validate_typecheck 0 errors across 8 projects (infra/ skipped, not installed locally); validate_lint 0 errors / 0 warnings on the changed files; prettier unchanged; harness + registrar test files 175 pass / 0 fail; R0 bun scripts/run-related-tests.ts over the non-test changed files: 1186 + 39 + 34 pass, 0 fail.

R1 (review round 1)

• BLOCKING — harness imports domain code. Fixed by the harness's own mechanism for this constraint (PR fix(mt#5016): Point onboarding at a Postgres that can actually run the migrations #3678 R1, DISPOSABLE_POSTGRES_IMAGE): a local claudeJsonPathUnder(env, home) in the harness, and a five-case equality test in cold-agent-onboarding-run.test.ts (unset / set / blank / empty / default-home) against the domain's resolveClaudeJsonPath, which the test file is free to import. The @minsky/domain/mcp/claude-code-paths import is gone. Note for the record: the "nothing but Node/Bun builtins" docblock was already contradicted on main by the maskConnectionString import (PR fix(mt#5018): Bill the cold-agent run to the subscription, with every channel still closed #3680 R1); it now names that as the one standing exception (a regex, which equality cannot pin) rather than leaving the constraint text and the import set disagreeing.
• Comment parentheses (harness-detection.ts) — reflowed.
• revertCandidates usage sweep — one non-test caller, src/adapters/shared/commands/setup-local-http.ts:116, two-arg form; the env = process.env default applies, matching the discoverMinskyEntries call beside it (:225), which also falls through to process.env. The CLI runs in the operator's env, so that is the intended resolution. No change.
• UNPARSED:<basename> sentinel — kept, with basis: it is the file's pre-existing convention (parseMcpServerNames on main emits UNPARSED:<line-prefix> for the same failure direction), and both consumers read the array as a presence set (length === 0 closes the channel), never as identities; the docblock now says so. A structured result would be a second shape for one convention.
• Registrar default-location test — inline note added that CLAUDE_CONFIG_DIR is deliberately unset by the describe's beforeEach.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UG8FZC1RHk7otPDDfs2zrr

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 758K prompt, 11K completion | Duration: 145s
Mode: normal

Commands

• /review — request a fresh review

[github-actions]

Cockpit Preview

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

Deployed from commit 5a34acb86.
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

---

REQUEST_CHANGES — The PR correctly honors CLAUDE_CONFIG_DIR for .claude.json across registrar/discovery/revert and widens channel 5 with good tests; it also fixes the transcript durability path. However, it introduces a blocking design regression: the harness script now imports domain code, contradicting its own "no domain imports" constraint and risking runtime resolution failures in environments where only scripts run. Non-blocking nits: a mismatched parenthesis in a comment, a signature-change adoption note for revertCandidates, a sentinel overloading risk in userScopeMcpServerNamesIn, and a minor test clarity suggestion. Address the harness coupling (decouple or explicitly document and guarantee availability) to proceed; otherwise the rest looks solid and criteria appear met.

Findings

• [BLOCKING] scripts/cold-agent-onboarding-run.ts:121 — Harness now imports domain code, contradicting its own isolation constraint and risking coupling/runtime resolution failures
  The harness script now imports resolveClaudeJsonPath from @minsky/domain/mcp/claude-code-paths (scripts/cold-agent-onboarding-run.ts:121). This contradicts the harness' previously stated constraint to "deliberately import nothing from the domain" (see scripts/cold-agent-onboarding-run.test.ts:66-73 comment explaining why the harness stands in for a machine with no Minsky installed). Beyond the comment, this introduces a real risk: running the harness now requires that the monorepo package alias @minsky/domain is resolvable in the execution environment. If the script is used in contexts where only scripts/ are run (without building/packaging the domain workspace), it will fail to resolve, undermining the instrument's independence and portability. Suggested fix: copy the minimal resolver logic into scripts/ (or inject a tiny env→path function via parameter) to keep the harness self-contained, or refactor to avoid importing across packages (e.g., a small shared util published to a thin, dependency-light location explicitly approved for scripts). If keeping the import is intentional, the harness docs and test comment should be updated and CI/runtime guarantees added that @minsky/domain is always available when invoking this script.
• [NON-BLOCKING] packages/domain/src/runtime/harness-detection.ts:22 — Comment text has mismatched/awkward parentheses around the CLAUDE_CONFIG_DIR aside
  In packages/domain/src/runtime/harness-detection.ts:22-28, the parenthetical aside reads: "~/.claude.json's top-level mcpServers key — or $CLAUDE_CONFIG_DIR/.claude.json when that variable is set, mt#5066), not per-project — ..." There appears to be an extra closing parenthesis after mt#5066), making the sentence hard to parse. Please fix the punctuation to avoid confusion (e.g., move the closing parenthesis to encompass the entire aside or remove the stray one).
• [NON-BLOCKING] src/mcp/setup/local-http-apply.ts:126 — Public function signature change warrants a usage sweep beyond immediate tests
  revertCandidates changed signature to accept an env parameter (src/mcp/setup/local-http-apply.ts:126-134), and direct call sites in tests were updated. While TypeScript will catch most consumers, this is a deploy-surface module; please confirm (or document) that no other runtime callers (e.g., CLI path or scripts) rely on the old arity. If any exist, update them to pass the appropriate env (or rely on the default) and add a note in the changelog or docs for the setup command.
• [NON-BLOCKING] scripts/cold-agent-onboarding-run.ts:545 — Sentinel overloading risk in userScopeMcpServerNamesIn: ‘UNPARSED:’ can collide with real names
  userScopeMcpServerNamesIn(file) reports parse failures as "UNPARSED:<basename>". This overloads the returned name channel with a sentinel value that could theoretically collide with a legitimate server name beginning with UNPARSED:. Consider returning a structured result (e.g., {kind:"ok", names:[...]} | {kind:"unparsed", file:basename}) or a second boolean/array to indicate parse failures distinctly. Tests already assert the sentinel behavior; this is advisory design feedback to reduce ambiguity in future consumers.
• [NON-BLOCKING] packages/domain/src/mcp/registration.test.ts:223 — Test still asserts HOME-root path; recommend adding an explicit ‘unset var’ note to avoid misread under CLAUDE_CONFIG_DIR
  The ClaudeCodeRegistrar tests now include a CLAUDE_CONFIG_DIR-set case, good. A later test still asserts ~/.claude.json (HOME root). It depends on the beforeEach cleanup of the env var. Consider adding an inline comment in that assertion that the variable is intentionally unset, to prevent future refactors from accidentally inheriting a set env and producing a false failure.

Documentation impact

• no-update-needed — Behavioral changes are internal (path resolution honoring CLAUDE_CONFIG_DIR and harness transcript location) with inline code comments and tests updated. No docs in docs/ reference the harness script or the specific .claude.json resolution; spot-check: docs/mcp-migration-guide.md covers discovery/apply behavior generically and remains accurate. No CLI flags or user-facing contract changed. If the project has a user-facing doc about minsky setup local-http revert behavior and CLAUDE_CONFIG_DIR, consider a future doc mention, but this PR does not ship or require doc changes.

[minsky-reviewer]

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

---

Re-verification of the fix commit shows the prior BLOCKING issue (harness and writers ignoring CLAUDE_CONFIG_DIR for .claude.json) is addressed. A single resolver resolveClaudeJsonPath is added and used by both ClaudeCodeRegistrar and local-http discovery; the harness keeps a builtins-only twin pinned by tests. Channel 5 now considers both the customizations dir and user-scope .claude.json names-only, and --execute snapshots operator names to detect write-side changes. Transcripts are written beside the run record under --out-dir after scrubbing. Tests cover names-only semantics, UNPARSED handling, widened probe negative control, and durable transcript path. I see no new critical defects introduced by these changes; outstanding backfill notes are out-of-repo and non-blocking. Verdict: APPROVE.

Spec verification

Criterion	Status	Evidence
SC1 — channel 5 observes the .claude.json Claude Code will actually read, and the write path it guards. Decision: widen; names-only; --execute snapshots operator ~/.claude.json names and asserts unchanged.	Met	scripts/cold-agent-onboarding-run.ts:541-611 adds claudeJsonPathUnder and userScopeMcpServerNamesIn (names-only, UNPARSED on failure); observe() now reads it (606-611). The --execute path snapshots before/after and computes operatorClaudeJsonNewServerNames (lines 1212-1243 and 1274-1286), recording it in RunRecord (652-668, 1319-1333). Tests in scripts/cold-agent-onboarding-run.test.ts assert names-only and UNPARSED behavior, and that channel 5 opens when only the file carries a server (lines ~24-66, 197-220).
SC2 — if the probe is widened, it must be able to FAIL (negative control OPEN).	Met	main() performs an unsandboxed observe with claudeJsonPathUnder(process.env) (scripts/cold-agent-onboarding-run.ts:1169-1178) and reports negative-control gaps (1179-1188). Tests assert the widened channel opens in the negative control (scripts/cold-agent-onboarding-run.test.ts:197-220) and that the set of open channels matches NEGATIVE_CONTROL_CHANNELS (155-171).
SC3 — the transcript lands somewhere durable, beside its record, scrub ahead of write.	Met	scripts/cold-agent-onboarding-run.ts: transcriptPathFor returns <outDir>/transcript-run-<ms>.txt (1057-1064). On --execute, scrub occurs first (1228-1234), then mkdirSync(opts.outDir) and write to the durable path (1236-1244). RunRecord.transcriptPath points to this path (615-668). Test asserts durable path shape and not under tmp (scripts/cold-agent-onboarding-run.test.ts:44-56).
SC4 — existing run records are not silently invalidated; docblock guidance; two author-machine records backfilled as a one-off.	Unverifiable	This criterion depends on out-of-repo state/backfill performed on the author’s machine. The code adds a RunRecord docblock explaining pre-mt#5066 records may point to temp and were backfilled (scripts/cold-agent-onboarding-run.ts:615-668), but the actual backfill action cannot be verified from the diff. Per the Constitution, out-of-repo paths/actions are not verifiable here.
SC5 — Minsky's own writers resolve .claude.json the way Claude Code does via a single resolveClaudeJsonPath(env, home) used by ClaudeCodeRegistrar.configPath and local-http-config.ts discovery/revert.	Met	New packages/domain/src/mcp/claude-code-paths.ts exports resolveClaudeJsonPath (entire file). packages/domain/src/mcp/registration.ts now uses it in ClaudeCodeRegistrar.configPath() (lines ~130-143). src/mcp/setup/local-http-config.ts imports and uses it via claudeJsonPath(home, env) (around 46-70). Tests updated to assert CLAUDE_CONFIG_DIR is honored (packages/domain/src/mcp/registration.test.ts:200-226) and harness tests pin harness resolver to domain’s (scripts/cold-agent-onboarding-run.test.ts:113-136).
Does NOT cover — Redirecting HOME is out of scope.	Met	No change redirects HOME; buildSandboxEnv explicitly preserves HOME (scripts/cold-agent-onboarding-run.test.ts:413-420 asserts, implementation at scripts/cold-agent-onboarding-run.ts:252-303).
Does NOT cover — The other five channels are unchanged.	Met	No logic changes to other channels beyond added detail strings; evaluateIsolation changes are confined to channel claudeCustomizations (scripts/cold-agent-onboarding-run.ts:203-238). Tests for other channels remain and pass shape (e.g., daemon/database tests in scripts/cold-agent-onboarding-run.test.ts).
Does NOT cover — Whether any past run actually wrote to ~/.claude.json.	Met	The implementation adds a names-only write-side check for future runs but does not attempt to assert past writes; no retroactive mutation beyond the documented backfill note (scripts/cold-agent-onboarding-run.ts:1212-1286, 615-668).

Documentation impact

• no-update-needed — This PR is a bugfix: it aligns .claude.json path resolution with Claude Code via a new internal resolver and makes the cold-agent harness write its transcript beside the run record. It does not add or change any public CLI flags, routes, or user-facing configuration contracts. I did not sweep the full docs; I spot-checked docs/mcp-migration-guide.md and general MCP/daemon docs by filename only during review context and did not find prose asserting a different .claude.json resolution. Given the scope (internal path resolution and harness internals), no documentation updates are required.

[minsky-reviewer]

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

---

Re-verification confirms the prior BLOCKING issues are addressed. A single canonical resolver now honors CLAUDE_CONFIG_DIR for .claude.json and is used consistently by both the registrar and local-http discovery/revert; the harness keeps a builtins-only twin pinned to the domain resolver via tests. Channel 5 is widened to consider both customizations dir and user-scope .claude.json (names-only), and --execute snapshots operator server names to detect write-side changes. Transcripts are written beside the run record after scrubbing. Tests cover the new paths, and I see no new critical defects introduced. One minor doc completeness suggestion (mentioning CLAUDE_CONFIG_DIR in a doc table) is non-blocking. Verdict: APPROVE.

Findings

• [NON-BLOCKING] docs/local-mcp-daemon.md:1 — Docs mention only ~/.claude.json for user-scope config; consider noting CLAUDE_CONFIG_DIR relocation (behavior now honored by code)
  The updated code resolves the user-scope Claude Code config file via CLAUDE_CONFIG_DIR when set (mt#5066). See packages/domain/src/mcp/claude-code-paths.ts and its consumers packages/domain/src/mcp/registration.ts and src/mcp/setup/local-http-config.ts. The operator-facing doc docs/local-mcp-daemon.md's “What it scans” table lists the user scope as ~/.claude.json without mentioning the relocation under $CLAUDE_CONFIG_DIR/.claude.json. The existing text is not strictly wrong, but it is incomplete for users who set CLAUDE_CONFIG_DIR. Consider adding a parenthetical to clarify the relocation to avoid confusion.

Spec verification

Criterion	Status	Evidence
SC1 — channel 5 observes the .claude.json Claude Code will actually read, and the write path it guards. Decision: WIDEN; names-only; --execute snapshots operator's real ~/.claude.json before and after.	Met	scripts/cold-agent-onboarding-run.ts:203-222 adds userScopeMcpServerNames to observations; :526-575 implements claudeJsonPathUnder and userScopeMcpServerNamesIn (names-only, UNPARSED handling); :1042-1078 uses claudeJsonPathUnder(env); :1116-1147 snapshots operator .claude.json before dispatch and asserts additions, writing operatorClaudeJsonNewServerNames into the record at :1199-1231. Tests in scripts/cold-agent-onboarding-run.test.ts cover names-only and sentinel behavior; packages/domain/src/mcp/claude-code-paths.ts centralizes resolver and is pinned by a harness test.
SC2 — if the probe is widened, it must be able to FAIL. Negative control OPEN.	Met	scripts/cold-agent-onboarding-run.ts:1150-1168 constructs the unsandboxed observation; :1171-1183 computes negative-control gaps and fails when any expected-open channel reads isolated. Tests in scripts/cold-agent-onboarding-run.test.ts lines ~340-371 assert channel 5 is OPEN when .claude.json shows a user-scope server, and an unreadable file opens the channel.
SC3 — the transcript lands somewhere durable, beside its record, scrub ahead of write.	Met	scripts/cold-agent-onboarding-run.ts:977-985 introduces transcriptPathFor(outDir, nowMs); :1160-1168 creates outDir and writes scrubbed transcript to the durable path; :1234-1255 records the durable path in RunRecord.transcriptPath. Test in scripts/cold-agent-onboarding-run.test.ts validates the computed durable path and that it is not under the sandbox temp root.
SC4 — existing run records are not silently invalidated; docblock names out-of---out-dir transcriptPath as pre-mt#5066; backfill done on authoring machine.	Met	scripts/cold-agent-onboarding-run.ts:589-620 extends RunRecord docblock noting pre-mt#5066 transcripts may have expired and that two such records were backfilled. The implementation does not attempt automated migration (out-of-repo), but records the guidance as specified.
SC5 — Minsky's writers resolve .claude.json the way Claude Code does (new resolver consumed by registrar and local-http discovery/revert).	Met	packages/domain/src/mcp/claude-code-paths.ts implements resolveClaudeJsonPath; packages/domain/src/mcp/registration.ts:169-176 switches configPath to use it; src/mcp/setup/local-http-config.ts:205-216 updates claudeJsonPath to delegate to the resolver and discoverMinskyEntries to pass env; src/mcp/setup/local-http-apply.ts:116-124 updates revertCandidates to accept env and pass it through. Tests added in packages/domain/src/mcp/claude-code-paths.test.ts, packages/domain/src/mcp/registration.test.ts, src/mcp/setup/local-http-config.test.ts, and src/mcp/setup/local-http-apply.test.ts verify behavior under CLAUDE_CONFIG_DIR.

Documentation impact

• no-update-needed — This PR is primarily an internal bugfix: honoring CLAUDE_CONFIG_DIR for .claude.json resolution and adjusting the cold-agent harness behavior. It updates code paths and tests without introducing new public commands or flags. I spot one minor doc completeness opportunity: docs/local-mcp-daemon.md’s table names ~/.claude.json for user scope but doesn’t mention $CLAUDE_CONFIG_DIR/.claude.json; I filed this as a NON-BLOCKING suggestion. No existing doc appears invalidated by the change. I did not sweep all docs; I checked docs/local-mcp-daemon.md, which remains largely accurate aside from the noted parenthetical.