[tbrownio]

Agents no longer meet a missing MCP sign-in mid-session. Before each launch, Agent Farm asks Claude or Codex whether it holds a sign-in for every auth: native connection the agent would get (a hosted MCP server whose browser sign-in the harness handles itself). A connection without one is left out of that launch. The agent is told it is unavailable and not to sign in, and the person gets one line with the command that fixes it. agent-farm mcp login <connection> now signs in, for a workspace connection, with every installed harness that needs it, and agent-farm doctor lists the gaps.

Visual Overview

PR artifact: walkthrough page (private to the maintainers' Grain organization)

Plan: package P3 of the setup-fixes plan (brief, cover sheet; private to the maintainers' Grain organization), journeys J7 and J8, decision Q-92B. The other packages of the same plan ship as separate PRs: P1 telemetry export (#202), P2 Codex update screen (#199), P4 skill text (#201) and P6 status page (#200), all merged, and P5 workspace text in another repository.

Why this change exists

Codex launches in one repository lacked sign-ins for three hosted MCP services (Linear, Notion, Sentry). Agent Farm still registered them, so Codex agents found those tools missing, logged credential warnings mid-session (credential_kind=mcp outcome=not_found), and tried to sort out the logins themselves. An agent can't finish a browser sign-in, so that effort is wasted and the session runs with less than it thinks.

Planning checked where the sign-ins live. Codex keeps MCP tokens in the macOS keychain, keyed by server name and URL, not by Codex home. So the per-profile Codex homes Agent Farm creates already see the same sign-ins. Nothing was lost; those three services were simply never signed in.

The person chose (Q-92B): Agent Farm checks before launch, leaves out services that aren't signed in, and tells both the person and the agent. One agent-farm mcp login per service covers every harness, and doctor lists the gaps. The rejected alternative was an Agent Farm relay that holds tokens for every harness, which is a large new component.

Concepts and approach

• auth: native connection. A workspace MCP connection with a URL whose login the harness (Claude or Codex) handles itself, through OAuth in the browser. Agent Farm registers it as orchestra_<name>. Only these connections are checked; local command servers and auth: none connections are unaffected.
• Sign-in check. Agent Farm asks the harness, and never reads or prints a token itself. A connection counts as "not signed in" only when the harness says so. Every other outcome, including a harness that is missing, crashes, times out or answers in an unexpected format, keeps the connection: when in doubt the tool stays available (it fails open).

Harness	How Agent Farm asks	Cost
Codex	One codex mcp list --json with the connections passed as -c mcp_servers.orchestra_<name>=…, against the original Codex home, 5 s timeout. o_auth = signed in; not_logged_in or unknown = not signed in. Codex reads its credential store before the network, so a signed-in server reads o_auth even offline, and unknown appeared offline only when nothing was stored.	about 0.2–0.4 s
Claude	No token-free local read exists (the keychain item holds the token itself), so one claude mcp get orchestra_<name> per connection, in parallel, from the private folder mcp login already uses (timeouts: 6 s per call, and 8 s for the whole parallel batch; observed cost about 1–2.5 s per call, see the next column). "Needs authentication" = not signed in; "Connected" = signed in; "Failed to connect" keeps the connection.	each call connects to the server, about 1–2.5 s

A choice beyond the plan, for the reviewer: because the Claude check connects to each server, a confirmed Claude sign-in is cached for one hour in ~/.cache/agent-farm/claude-mcp-signins.json (a hash of name and URL plus a timestamp, no token). Dropping the cache means paying that 1–2.5 s on every Claude launch with auth: native connections. A "Needs authentication" answer clears the entry.

A launch with one unsigned connection, linear, under Codex:

stderr:  agent-farm: linear is not signed in for codex, so this session starts without it; sign in with: agent-farm mcp login linear --directory /path/to/repo (for a connection set in an agent definition rather than the workspace, use codex's own MCP login instead)
argv:    no -c mcp_servers.orchestra_linear=…
agent:   Unavailable connections: linear (orchestra_linear). Codex holds no sign-in for it, so its tools are not loaded in this session. Do not try to sign in; the person has been told how. Work without it and say so where it matters.

--config-root is added to the login hint only when it is not the default. Each path in the hint is one literal shell argument: on macOS and Linux a path that needs quoting is single-quoted, so pasting the command never runs a $(…) or backtick inside it. On Windows, where cmd and PowerShell share no literal quoting, a plain path is double-quoted and a path containing " % ! $ or a backtick is shown as <this repository> to fill in. AGENT_FARM_SIGNIN_CHECK=off skips the check, for example on an offline machine.

Behavior and diff walkthrough

Changed area	Before → after	Why it matters
src/runtime.ts: nativeSignIns, nativeCodexHome, claudeProbeDirectory, loginHint, shellArgument (new)	The check described above, plus the shared helpers: the original Codex home (also from inside a session's generated home), the private Claude folder holding only that connection's .mcp.json, and the login command line.	One check used by launches, doctor and mcp login. The runtime may import only node: builtins, so the parallel Claude calls run in a short inline node -e script under spawnSync.
resolveLaunchFor (and childPrompt, codexChildToml, moved from the compiler)	Prepared launches (run, --exec, --print-launch, --print-acp) run the check for the agent's harness; --explain never does. Unsigned connections are removed from agent.connections, the note is appended to the instructions, and the result carries unavailable.	Everything that builds argv or ACP config reads agent.connections, so leaving a connection out there covers every output. The project-bridge check (projectCodexDuplicates) still reads the agent's full connection list, so a left-out connection still turns off the repository's own mcp-remote bridge to the same server.
command() and acpLaunch()	Codex argv and CODEX_CONFIG omit the connection. Claude gets its servers inline in --mcp-config when any are left out, because the bundled mcp.json lists all of them. Both return unavailable.	Same behavior in terminals, headless runs and ACP clients.
run()	Prints one stderr line per left-out connection (reportUnavailable).	The person learns about it at launch, with the exact command.
src/mcp-auth.ts: loginPlan, signInGaps (new); loginCommand	mcp login without --harness checks each installed harness and runs the native login, one after the other, only where no sign-in is stored. The Claude login folder is now keyed by connection name + URL (shared with the check). signInGaps feeds doctor.	One command per service instead of one per service per harness.
src/cli.ts	--harness is optional for mcp login. Without it: "already signed in", "signing in", or "could not confirm a sign-in; signing in" per harness; a failed login sets exit code 1. With it: unchanged, no check.	Keeps the old form working.
src/interactive.ts doctorCommand	New "MCP sign-ins" section for the current directory's workspace: ✗ <conn>: not signed in for <harness>. Run: <login command>, ? <conn>: could not check …; launches keep it, ✓ <conn>: signed in, or ✓ No missing sign-ins.	The person can find every gap before any launch.
Docs: docs/operations.md (new "MCP sign-ins before launch"), CONFIGURATION.md, README.md, src/commands.ts help	Describe the check, the cache, AGENT_FARM_SIGNIN_CHECK=off, the optional --harness, and the doctor section.	One home per topic; the old "inspect does not check authentication" note moves into the new section.
Tests: tests/mcp-signin.test.mjs (new), tests/workspaces.test.mjs	J7 for Codex (argv and ACP) and Claude (inline --mcp-config); fail-open on unknown and on a missing harness; no check for --explain; --print-launch through a fake codex printing the exact stderr line; J8 doctor lists the gap, then none, and never prints a planted token; mcp login without --harness signs in only where needed. The workspace tests set AGENT_FARM_SIGNIN_CHECK=off so they never reach a real harness.	Covers both journeys and the fail-open paths.

Validation and limitations

• Head 3923897 is dda0b0d rebased onto main after Agent Farm 0.2.36, with no change in behavior. The one conflict was in src/runtime.ts, where main's telemetry launch ID (Telemetry export keeps what happened, and an orchestrator exports a finished worker by its launch ID #202) and this PR's sign-in check both added a launch option and both changed the command() return; both are kept, so --launch export and left-out connections work together. The diff against main is the same 11 files as before.
• Head dda0b0d fixes review round 1 (review): F-1, a path like repo-$(printf INJECTED) in the printed login command was double-quoted, so pasting it ran the substitution; it is now single-quoted, with a test that has /bin/sh split the printed command and get each hostile path back byte for byte. F-2, a left-out connection no longer re-enables the project's mcp-remote bridge, with a test that failed before the fix. F-3 (added to this round by the person, D-37), native Codex subagents are now checked with the launch, child-only connections included; a subagent missing a sign-in gets this launch's copy of its config without that connection and with the note, under ~/.cache/agent-farm/native-children/, while the shared bundle stays as built. Native Claude subagents share the parent's servers and now get the same note. The child-config renderer moved from the compiler into runtime.ts so both use it. F-4, the stderr line now says what to do for a connection set in an agent definition.
• Head 5a932ec added one fix after opening: on Windows the printed login command quoted the path with JSON escaping, doubling every backslash. A path is now left bare when it needs no quoting, otherwise wrapped in plain double quotes. CI caught it (Windows tests J7 and J8); a test now covers it.
• Local on 3923897: pnpm typecheck passed; pnpm test: 366 tests, 357 passed, 9 skipped, 0 failed. (On dda0b0d: 353 tests, 344 passed, 9 skipped; bigbrain sync check passed.)
• CI: green on 3923897 (check; tests on macOS, Ubuntu and Windows; Windows link fallback; Socket). Also green earlier on dda0b0d and 5a932ec.
• Validation level: full. QA and review below ran on dda0b0d and earlier heads; the follow-up review on dda0b0d found it mergeable. They were not rerun after the rebase, which changed no behavior.
• Independent QA on 5a932ec (before the round-1 fixes) (a separate agent, temp HOME and config root): J7 for Codex (unreachable URL left out, stderr login line, instructions note) and for Claude against a local server that demands OAuth (left out, inline --mcp-config without it) PASS; J8 doctor PASS, and a planted token appeared 0 times; fail-open with no harness, AGENT_FARM_SIGNIN_CHECK=off, --explain (no check) and --print-acp PASS; a harness that never exits costs at most about 5 s (Codex) or 8 s (Claude) and leaves no processes. --print-launch timings: Codex 0.11–0.12 s; Claude 1.5–4 s while connections are unsigned, 0.1–0.2 s with the check off.
• QA of the round-1 fixes on dda0b0d (temp HOME and config root, fake codex reporting not_logged_in): run main --print-launch from a project folder named repo-$(printf INJECTED) with a checked-in mcp-remote bridge exited 0, left out orchestra_remote, passed mcp_servers.remote_bridge.enabled=false, and the printed hint, pasted into bash with agent-farm as an inert function, delivered the directory unchanged with nothing expanded. F-3: a Codex parent with a native helper that declares its own child_only connection printed one login line each for remote and child_only; the config_file passed for helper was the per-launch copy, holding neither server and the note naming both, while the bundle's helper.toml still listed both. A Claude parent's --agents prompt for helper carried the note, and --mcp-config held no orchestra_remote.
• Review: Codex review round 1 found F-1 to F-5; F-1 to F-4 are fixed here. The follow-up review of dda0b0d said mergeable as is; its remaining Should-Fix note is not addressed in this PR. F-5 (a fake-claude fixture test of the probe parser) was a suggestion and is not done.
• Quick check (temp HOME and temp config root, real codex on PATH, a connection example → https://mcp.example.invalid/mcp): run worker --print-launch exited 0; mcp_servers.orchestra_example was absent from argv; stderr held agent-farm: example is not signed in for codex, so this session starts without it; sign in with: agent-farm mcp login example --directory <scratch>/project; developer_instructions began with the Unavailable note. doctor showed ✗ example: not signed in for codex. Run: agent-farm mcp login example --directory <scratch>/project.

Not verified, or known limits:

• Claude keeps a connection whose server can't be reached (Failed to connect), by design: that is not a missing sign-in. Only a server that answers "needs authentication" is left out.

• Claude launches pay 1.5–4 s (up to 8 s for a slow server) on every launch while a connection is not signed in, because only confirmed sign-ins are cached. Signing in ends it.

• doctor prints one line per connection, so when one harness reports "missing" and the other "unknown", the unknown isn't shown.

• Platforms: the check asks the harness rather than reading a credential store, so it doesn't depend on the OS, but it was only observed on macOS; CI runs the stubbed tests on macOS, Ubuntu and Windows.

• doctor reporting "No missing sign-ins" against real sign-ins: a temp HOME can't see the macOS keychain, so that case is covered only by a test with a stubbed harness.

• A live Codex session confirming no not_found warnings for a left-out connection.

• Process children run their own check when they launch.

• Profile-level connections are checked and left out too, but mcp login resolves only workspace connections. The stderr line says so and points to the harness's own MCP login; it doesn't know which connections came from an agent definition, so it says this every time.

• Per-launch native-child configs are small files named by content, so they are reused rather than rewritten, and are not cleaned up.

• The claude mcp get probe folders get recorded in the user's ~/.claude.json, as mcp login already did.

• Old Claude login folders under ~/.cache/agent-farm/mcp-login/ are left unused, because the folder key changed to connection name + URL.

After merge

Once CI, QA and review are done and this is merged and released. Ask the person first; Agent Farm never signs in for them. In <the repository> that lacked the three sign-ins, run once per service (each opens a browser sign-in for each harness that needs it):

agent-farm mcp login linear --directory <the repository>
agent-farm mcp login notion --directory <the repository>
agent-farm mcp login sentry --directory <the repository>

Then agent-farm doctor in that repository should list no missing sign-ins.

🤖 Generated with Claude Code

[tbrownio]

PR Review

Size: large because 10 files, 366 lines; risk trigger auth (src/mcp-auth.ts).
Budget: 3 fix rounds.
Issue context: no linked issue; approved setup-fixes cover sheet, package P3, Q-92B, journeys J7/J8. Reviewed origin/main...5a932ec using the updated review criteria from main (#198).

Quality Gates

• pnpm install --frozen-lockfile: PASS in an isolated snapshot of the reviewed head; includes the build.
• pnpm typecheck: PASS.
• node --test tests/*.test.mjs: PASS, 341 tests, 332 passed, 9 skipped, 0 failed. This is the test phase of pnpm test, run with scratch HOME/config after the build, as required by docs/development.md. The initial archive lacked a Git index for the docs-links test; creating an index in the scratch snapshot resolved that environment failure.
• node scripts/sync-bigbrain-from-greenfield.mjs --check: PASS; pinned copies/checksums current, informational drift expected.
• CI: PASS at 5a932ec, including check and macOS/Ubuntu/Windows tests, install smoke and Windows link fallback. Linux and Windows logs were opened.
• Cross-OS Workspaces: local macOS checks executed; additional Linux/Windows manual checks BLOCKED because no machines with those OSes were available. Their CI results prove the automated paths, including stubbed sign-in tests; live native authentication remains unverified there.

Must-Fix (1)

F-1 Quote recovery-command paths as literal shell arguments · src/runtime.ts:571 — double quotes leave command substitution active in the printed login command. Failure scenario: a repository directory named repo-$(printf INJECTED) produces a hint containing that literal text inside double quotes; pasting the hint executes the substitution and changes the directory argument. Other substitutions could execute arbitrary commands. Fix: use literal escaping for an explicitly supported shell, or decline to print an executable hint when its arguments cannot be formatted safely. Handle both directory and custom config-root; retain the Windows backslash fix.
Source: repository directory/custom config-root → Sink: the recovery command copied into a shell → Missing control: literal shell argument escaping.
Reached by: resolveLaunchFor (src/runtime.ts:701) and signInGaps (src/mcp-auth.ts:38) generate commands printed by launch/doctor; the trigger requires an unusual path and someone copying the command.
Harm: high (operator shell command execution) · Reach: low (metacharacters in paths plus copying the hint) · Recovery: silent (substitution runs before login) → Must-Fix (security).
Fix size: small (approximately 30–45 lines with harmless round-trip tests; no new dependency or option).
Options: 1. Format a literal command for a named shell, with a safe explanatory fallback for unsupported characters. 2. Correctly format separate commands for every supported shell.
Decision: fix (option 1).
Class: shell-active text in recovery-command arguments.
Scope: in (new login-hint formatter) · Approach/scope: no (same recovery flow).
Checked by: repro — independently invoked the target formatter and executed its hint with an inert Bash function; repo-$(printf INJECTED) became repo-INJECTED. No real login or credentials were used.

Should-Fix (3)

F-2 Preserve project-bridge suppression when omitting a connection · src/runtime.ts:702 — filtering agent.connections also removes the endpoints used by projectCodexDuplicates in both terminal and ACP preparation. Failure scenario: the workspace connection is unsigned and the project already has an equivalent mcp-remote bridge; Agent Farm announces that the service was omitted, but no longer disables that project bridge, so Codex still starts it and can enter its browser-login flow. Fix: use the original selected connections for duplicate detection while using the filtered set for registration.
Reached by: the supported project-config coexistence path documented in CONFIGURATION.md:1123, with consumers at src/runtime.ts:783 and src/runtime.ts:849.
Harm: medium (the announced omission is ineffective and restores the unwanted login flow) · Reach: medium (supported existing project bridges) · Recovery: visible (native login failure; signing in is a workaround) → Should-Fix.
Fix size: small (approximately 20–35 lines including a combined missing-sign-in/project-bridge regression case; adds no abstraction or state).
Options: 1. Pass the unfiltered selected connections to duplicate detection in both renderings. 2. None needed.
Decision: fix (option 1).
Scope: in (launch connection handling) · Approach/scope: no (preserves existing duplicate suppression).
Checked by: repro — signed-in output disabled the project bridge; missing-sign-in output had zero MCP overrides. Independently verified the same omission in ACP output.

F-3 Apply the omission to native Codex children too · src/runtime.ts:700 — the check/filter applies only to the selected entry agent; its native children still use the prebuilt TOMLs containing the unsigned servers and instructions without the unavailable note. Failure scenario: a normal Codex reviewer delegates to its native codebase explorer after the parent omitted an unsigned workspace service; the child receives that service from its static config and discovers the missing sign-in mid-session, contrary to P3/J7. Fix: prepare the native-child configurations and tool notes from the launch's sign-in results, including child-only connections, without rewriting a shared cached bundle.
Reached by: shipped plugins/greenfield/agents/codex-pr-reviewer.md:11; connection inheritance/config generation in src/compiler.ts:62 and src/compiler.ts:93; static config consumers in src/runtime.ts:777 and src/runtime.ts:846.
Harm: medium (delegated agents still meet the unsupported login flow) · Reach: medium (normal supported native Codex delegation with an unsigned service) · Recovery: visible (sign in before delegation) → Should-Fix.
Fix size: large (launch-specific child configuration preparation and coverage; more than approximately 50 lines or additional launch state).
Options: 1. Reuse the existing sign-in result to filter inherited servers and annotate launch-specific child configs, checking child-only native servers in the same preparation. 2. Broader launch-configuration redesign is unnecessary.
Decision: fix (option 1; this is a shipped delegation path). The PR documents the limitation, but it remains a gap against the approved all-agent outcome.
Scope: in (P3's agents-before-launch promise) · Approach/scope: no (extends the chosen prelaunch check).
Checked by: repro and reading — independently prepared a missing-sign-in launch and opened the exact child config named in argv: it retained [mcp_servers.orchestra_remote] and had no unavailable note. Traced both native-child consumers; no live child session was launched.

F-4 Give profile-only connections an honest recovery path · src/runtime.ts:701 — every missing connection gets a workspace-only mcp login hint, including connections declared only in an agent definition. Failure scenario: a profile-only native service is omitted, the person follows the printed hint, and nativeConnection rejects it with Unknown workspace connection; the advertised command cannot restore the tool. Fix: provide safe guidance that explains the workspace-only limitation, or retain the existing behavior for unsupported profile-only recovery instead of promising that command works.
Reached by: supported agent-level connections (src/compiler.ts:62, tests/compiler.test.mjs:153); no shipped profile-only OAuth service was found, so demonstrated reach is low.
Harm: medium (an omitted tool has an unusable recovery command) · Reach: low (supported profile-only OAuth setup) · Recovery: visible (explicit unknown-connection error; native harness login is available) → Should-Fix.
Fix size: small for mitigation (approximately 10–25 lines of honest guidance/coverage; adds no selector, parser or state); full profile-aware login would be large.
Options: 1. Explain that this command resolves workspace connections and provide native-login guidance for profile-only services. 2. Add a profile-aware login selection mechanism in separately agreed scope.
Decision: mitigate (option 1; keep the limitation in the PR notes).
Scope: in (new omission/recovery message) · Approach/scope: no (guidance only; full profile-aware login would require a scope decision).
Checked by: repro — independently built a profile-only service, observed its omission/hint, and called the same login resolver, which threw Unknown workspace connection: extra.

Suggestions (1)

F-5 Cover the Claude executable boundary with one focused fixture · tests/mcp-signin.test.mjs:47 — the Claude test injects final states, so it bypasses the new probe/parser/cache implementation. Failure scenario: a change to parsing Status: Needs authentication or recognizing an unreadable response would leave these tests green. Fix: use the existing fake-executable helper for representative connected, needs-authentication and unknown responses, plus positive-cache reuse; keep it token-free.
Reached by: normal Claude preparation (src/runtime.ts:700); current fake-executable CLI/doctor tests exercise Codex.
Harm: low (coverage gap, no demonstrated supported-runtime failure) · Reach: high (normal Claude path) · Recovery: visible (inspectable coverage gap) → Suggestion.
Checked by: reading — traced all changed assertions against the production probe boundary. Optional coverage improvement, outside review fix rounds.

Principles

Ran: 1 Reuse, 2 Clarity, 3 Scope, 4 Anti-patterns, 5 Single Way, 8 Documentation, 9 Query/Data/Startup Cost, 12 Spec Fidelity, 13 Security/PHI, 14 Test Integrity, 15 Test Strategy. Each dimension had an independent gpt-6.1-sol finder with low reasoning on the same range. Skipped: 6 Backend Conventions (no server/backend layer), 7 Frontend Conventions (no frontend), 10 Self-contained Components (no temporary/optional product component), 11 Data Layer Consistency (no application data-fetching/ORM layer). Database-specific cost checks N/A; local startup cost was measured.

Earlier findings

None. No earlier greenfield reviews were present; review submissions were checked again before posting.

Dropped

• Initial docs-links failure: the archive snapshot had no Git index; the unchanged test passed once the isolated snapshot had an index. This was a review-environment issue.
• Claude nonzero-exit parsing candidate: a synthetic process can emit an authoritative status and exit nonzero; without a real harness contract showing that status must be ignored, that input does not establish a bug.
• Unbounded startup-cost candidate: for three silent signal-responsive fake servers, checks returned unknown in approximately 5.0 s (Codex) and 6.1 s total (parallel Claude). No demonstrated production-scale cost supports a finding. SIGTERM-refusing executables were not established as a supported harness behavior.
• One-hour Claude positive cache as scope creep: it serves the plan's speed constraint and stores hashes/timestamps only. Its stale-positive window after logout/revocation is a documented tradeoff, not a separate finding.

Claims

• Local typecheck and 341 tests / 332 pass / 0 fail on 5a932ec: checked by independently running the target snapshot's gates; 9 skipped explicitly recorded.
• CI green, including the Windows hint fix: checked via run/job metadata and Linux/Windows logs at the reviewed head. CI tests the merge result; local gates above test the head snapshot.
• Independent QA: opened the implementer's scratchpad/qa/j7-pr203.txt, j7b-pr203-stub.txt, j8-pr203-doctor.txt, j7c-pr203-failopen-off.txt, j7d-pr203-hangs.txt and j7e-pr203-explain-acp.txt. They support the stated ordinary launch, local HTTPS OAuth-stub, doctor, opt-out, missing-harness, timeout, explain and ACP checks. These are CLI outputs, not evidence of a live agent session.
• J7: ordinary Codex omission/hint/note and Claude OAuth-required-server omission are supported by those transcripts and automated tests. The all-agent outcome is partial (F-2/F-3). Live Codex absence of credential_kind=mcp outcome=not_found warnings is explicitly not claimed as verified.
• J8: missing-sign-in/login-command reporting and zero planted-token occurrences are supported by j8-pr203-doctor.txt; no-gaps reporting is supported by the stubbed test. The person's real three-service sign-ins and subsequent no-gaps doctor run are explicitly deferred, not claimed.
• Claimed normal startup timing and bounded ordinary hangs: supported by QA timings and the independent startup-cost measurements. Absolute process-tree cleanup in every failure mode was not established; do not treat those fixtures as that proof.
• Claude's one-hour positive cache: confirmed in code and fake-harness measurements. It trades a possible one-hour stale-positive result for avoiding repeat network probes; no token is cached.
• Whole-feature typecheck/test and pinned-copy checks: supported as above. P1/P2/P4/P5/P6 journeys are outside this P3 review.

Completeness

P3 delivers ordinary launch checks, workspace login across installed harnesses, doctor reporting and matching docs. J7 remains partial for existing Codex project bridges and native children. Profile-only recovery has the documented limitation in F-4. The post-release human sign-ins remain deferred. No fixes were applied; the shared checkout and branch were left unchanged.

Summary

Reviewed head 5a932ecb61177c36263cfffcbda7b6c2364ce831, unchanged from the assignment. Shared probe/login helpers and ordinary failure-open behavior are coherent, and required checks pass. One independently reproduced security finding blocks merge; three Should-Fixes and one optional test suggestion are recorded above.
Mergeable as is: no — F-1 permits shell substitution in the copied recovery command.

[tbrownio]

F-1 Quote recovery-command paths as literal shell arguments · src/runtime.ts:571 — double quotes leave command substitution active in the printed login command. Failure scenario: a repository directory named repo-$(printf INJECTED) produces a hint containing that literal text inside double quotes; pasting the hint executes the substitution and changes the directory argument. Other substitutions could execute arbitrary commands. Fix: use literal escaping for an explicitly supported shell, or decline to print an executable hint when its arguments cannot be formatted safely. Handle both directory and custom config-root; retain the Windows backslash fix.
Source: repository directory/custom config-root → Sink: the recovery command copied into a shell → Missing control: literal shell argument escaping.
Reached by: resolveLaunchFor (src/runtime.ts:701) and signInGaps (src/mcp-auth.ts:38) generate commands printed by launch/doctor; the trigger requires an unusual path and someone copying the command.
Harm: high (operator shell command execution) · Reach: low (metacharacters in paths plus copying the hint) · Recovery: silent (substitution runs before login) → Must-Fix (security).
Fix size: small (approximately 30–45 lines with harmless round-trip tests; no new dependency or option).
Options: 1. Format a literal command for a named shell, with a safe explanatory fallback for unsupported characters. 2. Correctly format separate commands for every supported shell.
Decision: fix (option 1).
Class: shell-active text in recovery-command arguments.
Scope: in (new login-hint formatter) · Approach/scope: no (same recovery flow).
Checked by: repro — independently invoked the target formatter and executed its hint with an inert Bash function; repo-$(printf INJECTED) became repo-INJECTED. No real login or credentials were used.

[tbrownio]

F-2 Preserve project-bridge suppression when omitting a connection · src/runtime.ts:702 — filtering agent.connections also removes the endpoints used by projectCodexDuplicates in both terminal and ACP preparation. Failure scenario: the workspace connection is unsigned and the project already has an equivalent mcp-remote bridge; Agent Farm announces that the service was omitted, but no longer disables that project bridge, so Codex still starts it and can enter its browser-login flow. Fix: use the original selected connections for duplicate detection while using the filtered set for registration.
Reached by: the supported project-config coexistence path documented in CONFIGURATION.md:1123, with consumers at src/runtime.ts:783 and src/runtime.ts:849.
Harm: medium (the announced omission is ineffective and restores the unwanted login flow) · Reach: medium (supported existing project bridges) · Recovery: visible (native login failure; signing in is a workaround) → Should-Fix.
Fix size: small (approximately 20–35 lines including a combined missing-sign-in/project-bridge regression case; adds no abstraction or state).
Options: 1. Pass the unfiltered selected connections to duplicate detection in both renderings. 2. None needed.
Decision: fix (option 1).
Scope: in (launch connection handling) · Approach/scope: no (preserves existing duplicate suppression).
Checked by: repro — signed-in output disabled the project bridge; missing-sign-in output had zero MCP overrides. Independently verified the same omission in ACP output.

[tbrownio]

F-3 Apply the omission to native Codex children too · src/runtime.ts:700 — the check/filter applies only to the selected entry agent; its native children still use the prebuilt TOMLs containing the unsigned servers and instructions without the unavailable note. Failure scenario: a normal Codex reviewer delegates to its native codebase explorer after the parent omitted an unsigned workspace service; the child receives that service from its static config and discovers the missing sign-in mid-session, contrary to P3/J7. Fix: prepare the native-child configurations and tool notes from the launch's sign-in results, including child-only connections, without rewriting a shared cached bundle.
Reached by: shipped plugins/greenfield/agents/codex-pr-reviewer.md:11; connection inheritance/config generation in src/compiler.ts:62 and src/compiler.ts:93; static config consumers in src/runtime.ts:777 and src/runtime.ts:846.
Harm: medium (delegated agents still meet the unsupported login flow) · Reach: medium (normal supported native Codex delegation with an unsigned service) · Recovery: visible (sign in before delegation) → Should-Fix.
Fix size: large (launch-specific child configuration preparation and coverage; more than approximately 50 lines or additional launch state).
Options: 1. Reuse the existing sign-in result to filter inherited servers and annotate launch-specific child configs, checking child-only native servers in the same preparation. 2. Broader launch-configuration redesign is unnecessary.
Decision: fix (option 1; this is a shipped delegation path). The PR documents the limitation, but it remains a gap against the approved all-agent outcome.
Scope: in (P3's agents-before-launch promise) · Approach/scope: no (extends the chosen prelaunch check).
Checked by: repro and reading — independently prepared a missing-sign-in launch and opened the exact child config named in argv: it retained [mcp_servers.orchestra_remote] and had no unavailable note. Traced both native-child consumers; no live child session was launched.

[tbrownio]

F-4 Give profile-only connections an honest recovery path · src/runtime.ts:701 — every missing connection gets a workspace-only mcp login hint, including connections declared only in an agent definition. Failure scenario: a profile-only native service is omitted, the person follows the printed hint, and nativeConnection rejects it with Unknown workspace connection; the advertised command cannot restore the tool. Fix: provide safe guidance that explains the workspace-only limitation, or retain the existing behavior for unsupported profile-only recovery instead of promising that command works.
Reached by: supported agent-level connections (src/compiler.ts:62, tests/compiler.test.mjs:153); no shipped profile-only OAuth service was found, so demonstrated reach is low.
Harm: medium (an omitted tool has an unusable recovery command) · Reach: low (supported profile-only OAuth setup) · Recovery: visible (explicit unknown-connection error; native harness login is available) → Should-Fix.
Fix size: small for mitigation (approximately 10–25 lines of honest guidance/coverage; adds no selector, parser or state); full profile-aware login would be large.
Options: 1. Explain that this command resolves workspace connections and provide native-login guidance for profile-only services. 2. Add a profile-aware login selection mechanism in separately agreed scope.
Decision: mitigate (option 1; keep the limitation in the PR notes).
Scope: in (new omission/recovery message) · Approach/scope: no (guidance only; full profile-aware login would require a scope decision).
Checked by: repro — independently built a profile-only service, observed its omission/hint, and called the same login resolver, which threw Unknown workspace connection: extra.

[tbrownio]

PR Review follow-up (round 1 fixes)

Size: large because 41 files, 957 lines; over 30 files; risk trigger auth (src/mcp-auth.ts).
Budget: 3 fix rounds.
Issue context: no linked issue; P3, Q-92B, J7/J8. Scoped to F-1–F-4 in 5a932ec...dda0b0d; rebased main changes excluded from reassessment. Applied the review criteria on main.

Quality Gates

• Isolated pnpm install --frozen-lockfile (includes build), pnpm typecheck, and node scripts/sync-bigbrain-from-greenfield.mjs --check: PASS.
• node --test tests/*.test.mjs, with scratch HOME/config after that build: PASS, 353 tests, 344 passed, 9 skipped, 0 failed.
• Current-head CI: PASS on macOS, Ubuntu, Windows and Windows link fallback; opened the logs. Windows has 21 skipped tests, including the POSIX-shell test.
• Cross-OS Workspaces: macOS checks executed. Live Linux/Windows checks BLOCKED: discovery exposes only macOS machines. Their automated CI paths pass; native Windows shell behavior remains unverified.

Must-Fix (0)

None. No new fix-caused Must-Fix found.

Should-Fix (1)

F-3 Native-child omission remains partial for colliding sibling connection names · src/runtime.ts:724 — Object.assign keeps only the last definition of a child-only connection name. Failure scenario: sibling one declares unsigned extra at URL A, while sibling two declares signed-in extra at URL B; only B is checked, so one keeps its unsigned server and receives no unavailable note. Fix: document that child-only names must be unique across native siblings and recommend renaming; complete support would retain results per connection definition/child.
Reached by: invented only — a custom sibling configuration accepted by src/compiler.ts:62–64; no shipped collision found.
Harm: medium (a child still encounters the unsupported sign-in flow) · Reach: low (custom siblings reuse a child-only name for different endpoints) · Recovery: visible (use distinct names or sign in) → Should-Fix.
Fix size: small for mitigation (approximately 5–10 documentation lines; adds no runtime abstraction or state); full per-definition probing is large.
Options: 1. Carry the unique-name requirement as a known limitation and explain the rename workaround. 2. Track sign-in results per definition/child.
Decision: mitigate (option 1; retain the limitation in PR notes).
Scope: in (F-3 native-child checking) · Approach/scope: no (documents the existing approach).
Checked by: repro — independently built both children; the check received only URL B, unavailable was empty, and one retained its original unsigned TOML without the note.

Suggestions (0)

None added in this follow-up.

Principles

Ran: 1 Reuse, 2 Clarity, 3 Scope, 4 Anti-patterns, 5 Single Way, 8 Documentation, 9 Query/Data/Startup Cost, 12 Spec Fidelity, 13 Security/PHI, 14 Test Integrity, 15 Test Strategy; independent gpt-6.1-sol finders, low reasoning, all using 5a932ec...dda0b0d within this follow-up's scope. Skipped: 6 Backend, 7 Frontend, 10 Self-contained Components, 11 Data Layer (those layers/components did not change in the scoped fixes).

Earlier findings

From <!-- greenfield-review head=5a932ecb61177c36263cfffcbda7b6c2364ce831 -->:

• F-1 resolved (42b937f, src/runtime.ts:569–578): independently round-tripped directory/config-root substitutions, backticks, apostrophes and newline through /bin/sh; bytes remained literal. Windows formatter rejects expansion characters.
• F-2 resolved (42b937f, src/runtime.ts:830, :896): terminal and ACP retain project-bridge suppression after omitting the connection.
• F-3 partially resolved (dda0b0d): ordinary inherited and uniquely named child-only Codex connections are filtered in launch-specific files; the bundle stays unchanged. Independently checked Claude's native-child note too. The rare remainder is recorded above under F-3.
• F-4 resolved by the selected mitigation (42b937f, src/runtime.ts:959): a profile-only CLI fixture now explicitly directs the person to the harness's own MCP login; workspace-only resolution remains unchanged.
• F-5 remains an optional suggestion from the earlier review; outside this follow-up and not reassessed.

Dropped

None.

Claims

• Current-head typecheck, 353/344/9 test counts, sync check and green CI: independently checked as above.
• Round-1 fixes/QA: F-1, F-2, F-4 and ordinary Codex/Claude native-child behavior are supported by independent executions and the regression tests. The claim that F-3 is fully fixed remains partial for the case above.
• Cover-sheet J7: launch/config evidence supports the ordinary omission, notice and recovery paths; the remaining child case is F-3. A live Codex session proving no credential warnings remains explicitly unclaimed.
• J8: doctor regression tests pass. The person's three real-service sign-ins and subsequent doctor check remain explicitly deferred. Other package journeys and unchanged earlier QA claims were not reassessed.

Completeness

F-1, F-2 and F-4 are resolved. F-3 covers the shipped/ordinary delegation path, with the rare custom-name limitation above. No fixes applied.

Summary

Reviewed dda0b0dee2a4bc8af64427b900ebfebafaa079d7, unchanged from the assignment. The original security blocker is resolved; one narrowed Should-Fix remains as a proposed documented limitation.
Mergeable as is: yes — no Must-Fix remains.

[tbrownio]

F-3 Native-child omission remains partial for colliding sibling connection names · src/runtime.ts:724 — Object.assign keeps only the last definition of a child-only connection name. Failure scenario: sibling one declares unsigned extra at URL A, while sibling two declares signed-in extra at URL B; only B is checked, so one keeps its unsigned server and receives no unavailable note. Fix: document that child-only names must be unique across native siblings and recommend renaming; complete support would retain results per connection definition/child.
Reached by: invented only — a custom sibling configuration accepted by src/compiler.ts:62–64; no shipped collision found.
Harm: medium (a child still encounters the unsupported sign-in flow) · Reach: low (custom siblings reuse a child-only name for different endpoints) · Recovery: visible (use distinct names or sign in) → Should-Fix.
Fix size: small for mitigation (approximately 5–10 documentation lines; adds no runtime abstraction or state); full per-definition probing is large.
Options: 1. Carry the unique-name requirement as a known limitation and explain the rename workaround. 2. Track sign-in results per definition/child.
Decision: mitigate (option 1; retain the limitation in PR notes).
Scope: in (F-3 native-child checking) · Approach/scope: no (documents the existing approach).
Checked by: repro — independently built both children; the check received only URL B, unavailable was empty, and one retained its original unsigned TOML without the note.