fix(runtime): fail-closed Claude resolution and runtime selection (R2, R7) - #31
Conversation
The default harness runtime was non-functional. `resolveClaudeCLI()` returned
the first *executable file* from
["/usr/bin/env", "/usr/local/bin/claude", "/opt/homebrew/bin/claude"]
and /usr/bin/env always exists and is always executable, so it always won.
That value was passed as `executablePath` to `ClaudeCLITransport`, whose
override branch runs the binary with the Claude argv *unmodified* — so the
process became
/usr/bin/env -p --model claude-sonnet-5 --system-prompt …
and env rejected `-p` as one of its own flags. Reproduced directly:
`/usr/bin/env: illegal option -- p`.
The old doc comment shows the intent: that candidate list was meant to answer
"is Claude installed?" for a dry-run report, not to name the executable.
Conflating those two questions is the whole defect.
ClaudeExecutableResolver replaces it. An explicit configured path is used
exclusively — no PATH fallback, so a misconfiguration is a hard error rather
than a silent substitution. Otherwise PATH is searched in order, empty entries
dropped, and an entry holding an unusable `claude` is skipped so the search
continues, which is how PATH resolution is expected to behave.
The load-bearing rule is that the resolved file must itself be named `claude`.
A wrapper needs a subcommand argument the argv contract does not carry, so
accepting one can only produce a malformed invocation. Symlinks are accepted,
because Homebrew and npm expose commands that way: the *invocation* basename
must be `claude`, file and executable checks follow the link, and the
invocation path — not the link target — is what reaches Process.executableURL.
Broken symlinks, symlinks to directories, and symlinks to non-executable
targets are all rejected.
Every failure is a typed ResolutionError raised before any Process is created.
The same validation now guards both override branches (ClaudeCLITransport and
the legacy ClaudeCLIGenerator), since both pass argv unmodified and so share
the defect. The argv contract itself is unchanged once a real Claude resolves.
The egress test stub is renamed from `claude-stub-….sh` to a file named
`claude` in a unique directory. That is required by the new guard and is the
more faithful fixture anyway — a real installation is named `claude`.
No live Claude request is made by any test.
Verified: 19 resolver tests, 3 transport-guard tests, 3 egress tests; 81
BCICloudBridge tests pass; ./Scripts/build.sh clean; git diff --check clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not a behavioural fix. PR #29 already removed the cross-provider fallback — `(try? LiveRuntimeFactory.make(...)) ?? (ClaudeCLIGenerator(...), …)` became a do/catch that disables the loop. This adds the regression coverage that was missing, and the seam needed to write it honestly. The first attempt tested `disableHypnagogicLoop(reason:)` directly. That was not good enough: it proves the helper works when called, not that a resolution failure ever reaches it. The catch itself — the thing that regressed once — went unexercised. The production call sites sit behind a mic/speech authorization gate, so a toggle-driven test returns early at that gate and passes without executing any resolution code at all. So the resolution step is extracted into `resolveHypnagogicRuntime(_:)`, an internal method taking the resolving closure. Production passes the real `LiveRuntimeFactory.make` calls; tests pass one that throws. That bypasses the permission gate without bypassing the control flow under test. `disableHypnagogicLoop` returns to private. The dialectic path now short-circuits through a single `guard let … else { return }` over both resolutions, and the mirror path likewise, which is also less code than the two do/catch blocks it replaces. Asserted on failure: the resolver runs exactly once; no generator is constructed; no subprocess is launched; the returned runtime is nil; the loop is disabled; the typed reason is preserved in `lastError` rather than flattened; and `startupWarning` is not overwritten, since a runtime failure is not a startup substitution notice. A separate test asserts exactly one resolution attempt — if the old `?? (ClaudeCLIGenerator(...))` returned, a second attempt against a different provider would make it fail. The success path is covered too: a resolved runtime is returned unchanged, records no error, and does not by itself enable the loop. Also renames the Ollama characterization test to `testAppPathCharacterizationDoesNotProbeOllamaModelAvailability_R18` and marks it explicitly as recording a known gap. `LiveRuntimeFactory` performs no model-readiness probe, so an unpulled model resolves and fails only at generation. That is R18, tracked separately: it does not violate the no-cross-provider-fallback invariant, but it does leave the loop enabled longer than the readiness contract intends. That test does not satisfy the missing-model acceptance criterion and is not counted toward it. Verified: 4 app fail-closed tests, 6 LiveRuntimeFactory tests; 93 BCICloudBridge and 117 NeuralComposeApp tests pass; ./Scripts/build.sh clean; no runtime-resolution catch substitutes a provider; git diff --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adversarial review found that the previous two commits' tests did not prove
what they claimed. Confirmed by mutation, not argument:
- Reverting makeClaude to the old resolveClaudeCLI() helper — the one that
returned /usr/bin/env — left all 93 BCICloudBridge tests GREEN. The tests
exercised ClaudeExecutableResolver in isolation and never touched the
defective caller. The resolver was pinned; the fix was not.
- Deleting `hypnagogicLoopEnabled = false` from disableHypnagogicLoop left
all 4 R7 tests GREEN. That property defaults to false and the test never
set it true, so the assertion could not fail.
Both are the same false-green pattern these commits exist to eliminate.
R2 call site is now covered. DialecticSession is an executable target with
top-level code, but SwiftPM 6 tests it cleanly, so DialecticSessionTests is
added and `environment:` is threaded through RuntimeFactory.make/makeClaude.
The headline test drives a PATH containing /usr/bin — hence env — but no
claude, which is exactly the original defect, and asserts resolution throws.
Re-running the same mutation now fails 2 of 5 tests.
The vacuous R7 assertions are deleted rather than patched. `generatorsBuilt`
and `processesLaunched` counted call sites that exist only inside the test's
own closures and could never be non-zero. `testFailureDoesNotTriggerAnAlternate
ProviderRequest` was removed outright: the old fallback lived at the call site
and constructed a generator directly rather than re-invoking the injected
closure, so the resolver-call count would have stayed at 1 and the test would
have stayed green — it could not detect the regression its comment described.
What remains is mutation-sensitive: removing setLastError fails 2 of 3, and
making resolveHypnagogicRuntime substitute a provider instead of returning nil
also fails 2 of 3. The test file now states plainly which properties are NOT
covered — the toggle transition and the call-site guard both need the
mic/speech authorization gate stubbed, which is A2 work.
Also from the review:
- An empty configured executable path silently fell back to PATH, the same
substitution class as the original defect. Now a typed error, with a test.
- RuntimeFactoryError.claudeCLINotFound had become dead code, losing its
actionable "install Claude Code and run claude login" message in favour of
a raw PATH dump. makeClaude now translates notFoundOnPath back into it.
- The --dry-run doc claimed the factory is a no-op so dry runs succeed
without Claude installed. That was only true because the old resolver
always returned /usr/bin/env; the doc now records the behaviour change.
- The egress stub leaked an empty temp directory per run; teardown added.
- NeuralComposeAppTests imported BCICloudBridge transitively; now declared.
Deliberately not addressed here: the witness runtime is resolved even for
profiles with witnessEnabled == false (pre-existing, belongs with R3), and
NeuralComposeAppTests is excluded from CI (pre-existing policy, R9).
Verified: 351 tests pass across BCICore 202, BCICloudBridge 94,
NeuralComposeApp 116, DialecticSession 5, BCIClassifier 13, BCIVoice 21;
./Scripts/build.sh clean; git diff --check clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adversarial review — findings and correctionsThe review found that the first two commits' tests did not prove what they claimed. Confirmed by mutation, not argument. Two false greens, both mine
The first was the serious one: the tests pinned FixesR2 call site now covered. Vacuous R7 assertions deleted rather than patched. The test file now states plainly what is not covered: the toggle transition and the call-site Also fixed
Accepted, not fixed here
351 tests pass: BCICore 202 · BCICloudBridge 94 · NeuralComposeApp 116 · DialecticSession 5 · BCIClassifier 13 · BCIVoice 21. Build clean, |
R2 — the malformed invocation
resolveClaudeCLI()returned the first executable file from["/usr/bin/env", "/usr/local/bin/claude", "/opt/homebrew/bin/claude"]./usr/bin/envalways exists and is always executable, so it always won. Thatvalue reached
ClaudeCLITransportasexecutablePath, whose override branchpasses argv unmodified — producing:
Reproduced directly:
/usr/bin/env: illegal option -- p. The default harnessruntime was non-functional.
The old doc comment shows the intent — that list was meant to answer "is Claude
installed?" for a dry-run report, not to name the executable. Conflating those
two questions is the defect.
Resolution algorithm
The load-bearing rule: the resolved file must itself be named
claude. Awrapper needs a subcommand argument the argv contract does not carry.
Symlinks are accepted — Homebrew and npm expose commands that way. The
invocation basename must be
claude, file and executable checks follow thelink, and the invocation path (not the target) reaches
Process.executableURL.Broken symlinks, symlinks to directories, and symlinks to non-executable
targets are rejected.
Every failure is a typed
ResolutionErrorraised before anyProcessexists.RuntimeFactory.makeClaudeis the fixed call siteSources/DialecticSession/RuntimeFactory.swift:113now callsClaudeExecutableResolver.resolve(environment:).DialecticSessionTestsexercises that caller directly — not the resolver in isolation — so reverting
the call site fails the suite. See the mutation record below.
Scope limit: the app's default Claude path is still outside the resolver
Stated plainly because the first version of this description implied otherwise.
ClaudeCLITransportandClaudeCLIGeneratoreach have two branches. Theoverride branch (
executablePath:supplied) now callsClaudeExecutableResolver.validatebefore launch. The default branch(
executablePath: nil) still launches/usr/bin/envwitharguments = ["claude"] + args— which is env's correct contract, sinceclaudearrives as the command name rather than as an env flag. env cantherefore never receive
-pas its own option on this path.But that branch performs no pre-launch resolution, and it is what the app uses:
AppViewModel→LiveRuntimeFactory→ClaudeCLIGenerator(executablePath: nil).LiveRuntimeFactorynever callsresolve().R2 as fixed here covers the DialecticSession harness. Unifying the app's
default path onto the same resolver is follow-up work, tracked with A2.
R7 — already fixed; this adds coverage
The cross-provider fallback was removed in PR #29. This PR does not claim
to fix it, hence
refactor(app):rather thanfix(app):.The first attempt tested
disableHypnagogicLoop(reason:)directly, which onlyproves the helper works when called — not that a resolution failure reaches it.
The catch itself went unexercised. Since the production call sites sit behind a
mic/speech authorization gate, a toggle-driven test returns early there and
passes without executing any resolution code.
So the resolution step is extracted into
resolveHypnagogicRuntime(_:), takingthe resolving closure. Production passes the real
LiveRuntimeFactory.makecalls; tests pass one that throws — bypassing the permission gate without
bypassing the control flow under test.
disableHypnagogicLoopstays private.Asserted on failure: the resolver runs exactly once ·
nilis returnedrather than a substituted provider · the typed reason survives into
lastErrorunflattened ·
startupWarningis not overwritten.Not asserted, and previously claimed in error: "no generator constructed"
and "no subprocess launched" were measured by counters that lived only inside
the tests' own closures and that no production code path ever incremented. They
were structurally incapable of failing and have been deleted rather than
rewritten.
hypnagogicLoopEnabled == falsewas likewise asserted on a propertythat defaults to
falseand was never settruein the test — deleting theproduction assignment left it green.
Proving the toggle transition, and proving the call-site
guardshort-circuits, both require stubbing the mic/speech authorization gate. That is
A2 work; until then those two properties are unverified and are not claimed.
Mutation record
The claim is not "these lines have tests" but "reintroducing the defect fails
the suite." Each mutation below was applied to a clean tree at
6b8d1e4,verified, then reverted.
M1 — revert the R2 call site. In
Sources/DialecticSession/RuntimeFactory.swift,replace the resolver call with the original helper:
Before this PR's third commit: 93 BCICloudBridge tests green — the defect
was fully reintroducible without failing anything. Now: 2 of 5 fail.
M2 — remove the error surface. Delete
setLastError(...)fromdisableHypnagogicLoopinSources/NeuralComposeApp/AppViewModel.swift.2 of 3 fail.
M3 — reintroduce provider substitution. In
resolveHypnagogicRuntime,change the catch to return a generator instead of
nil:2 of 3 fail.
M4 — delete the toggle assignment. Remove
hypnagogicLoopEnabled = falsefrom
disableHypnagogicLoop. All tests stay green — this is recorded as aknown coverage gap, not as covered behaviour, and the assertion that falsely
implied otherwise has been removed.
The mutations are documented rather than automated; there is no re-runnable
mutation harness in the tree.
R18 — open, tracked separately
LiveRuntimeFactoryperforms no model-readiness probe, so an unpulled Ollamamodel resolves and fails only at generation. This does not violate the
no-cross-provider-fallback invariant, but it leaves the loop enabled longer than
the readiness contract intends.
testAppPathCharacterizationDoesNotProbeOllamaModelAvailability_R18records thecurrent behaviour and is written to fail once probing is added. It explicitly
does not satisfy the missing-model acceptance criterion and is not counted
toward it.
Verification at
6b8d1e4BCICoreTestsBCICloudBridgeTestsNeuralComposeAppTestsDialecticSessionTests./Scripts/build.shclean ·git diff --checkclean.swift testwith no filter aborts insideBCIEEGTests.MindMonitorOSCStreamTests.testTruncatedSampleAddressCountsAsDroppedNotIgnored.That crash reproduces identically on
main, and this PR touches noBCIEEGfile. It is pre-existing and is not counted as a pass or a failure here.
Static search finds no
try? LiveRuntimeFactory,?? (ClaudeCLIGenerator, orclaude-cli-fallbackinSources/— the only match is a doc comment describingthe removed defect.
Provider requests. No test added by this PR makes a real provider request.
This is not blanket-true of the suite: the pre-existing
testLiveRuntimeHitsOllamadoes contact a localhost Ollama daemon when one isrunning.
Packaging.
Package.swiftgains one.testTargetand declares analready-transitive
BCICloudBridgedependency explicitly. No product,executable target, resource, or build setting changed.
DialecticSessionis anexecutable target with top-level
main.swift; SwiftPM 6 tests it withoutaltering the shipped binary, confirmed by running
./.build/debug/dialectic-session --help(exit 0, usage printed).PATH exposure.
.notFoundOnPath(searched:)is the only error embedding theenvironment
PATH. It is thrown only byresolve(), called only fromRuntimeFactory.makeClaude, where it is caught and translated back intoRuntimeFactoryError.claudeCLINotFound— which carries the actionable installhint instead.
LiveRuntimeFactorynever callsresolve(), so the app'spublished
lastErrorcannot carry PATH today. Named follow-up: if theshared resolver enters the UI path — likely when readiness probing lands in
A2 — that string would reach
lastErrorand must be redacted first.Untouched: R3, R8, R9, R16, R1, EEG acquisition/preprocessing, model contracts,
experiment/promotion status, packaging, private data.
🤖 Generated with Claude Code