Skip to content

fix(runtime): fail-closed Claude resolution and runtime selection (R2, R7) - #31

Merged
aurascoper merged 3 commits into
docs/eeg-methods-scopefrom
fix/runtime-selection-failclosed
Jul 25, 2026
Merged

fix(runtime): fail-closed Claude resolution and runtime selection (R2, R7)#31
aurascoper merged 3 commits into
docs/eeg-methods-scopefrom
fix/runtime-selection-failclosed

Conversation

@aurascoper

@aurascoper aurascoper commented Jul 25, 2026

Copy link
Copy Markdown
Owner
head: 6b8d1e4
commits: 3

R2:
  harness_call_site_fixed: true
  mutation_tested: true
  app_default_claude_path_unified: false

R7:
  behavior_fixed_by_PR_29: true
  catch_path_regression_coverage: true
  mutation_tested: true

R18:
  status: open
  ollama_model_readiness_probe: not_implemented

real_provider_requests_executed: false   # by the new tests; see caveat below
new_runtime_authority: false

R2 — the malformed invocation

resolveClaudeCLI() returned the first executable file from
["/usr/bin/env", "/usr/local/bin/claude", "/opt/homebrew/bin/claude"].
/usr/bin/env always exists and is always executable, so it always won. That
value reached ClaudeCLITransport as executablePath, whose override branch
passes argv unmodified — producing:

/usr/bin/env -p --model claude-sonnet-5 --system-prompt … 

Reproduced directly: /usr/bin/env: illegal option -- p. The default harness
runtime 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

explicit path given?
  → non-empty, must exist, be a regular file, be executable,
    invocation basename "claude"
  → used exclusively; no PATH fallback, so misconfiguration is a hard error
otherwise
  → PATH split on ":", order preserved, empty entries dropped
  → first entry with a valid "claude" wins
  → an entry with an unusable "claude" is skipped, search continues
  → none → typed .notFoundOnPath(searched:)

The load-bearing rule: the resolved file must itself be named claude. A
wrapper 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 the
link, 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 ResolutionError raised before any Process exists.

RuntimeFactory.makeClaude is the fixed call site

Sources/DialecticSession/RuntimeFactory.swift:113 now calls
ClaudeExecutableResolver.resolve(environment:). DialecticSessionTests
exercises 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.

ClaudeCLITransport and ClaudeCLIGenerator each have two branches. The
override branch (executablePath: supplied) now calls
ClaudeExecutableResolver.validate before launch. The default branch
(executablePath: nil) still launches /usr/bin/env with
arguments = ["claude"] + args — which is env's correct contract, since
claude arrives as the command name rather than as an env flag. env can
therefore never receive -p as its own option on this path.

But that branch performs no pre-launch resolution, and it is what the app uses:
AppViewModelLiveRuntimeFactoryClaudeCLIGenerator(executablePath: nil).
LiveRuntimeFactory never calls resolve().

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 than fix(app):.

The first attempt tested disableHypnagogicLoop(reason:) directly, which only
proves 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(_:), taking
the resolving closure. Production passes the real LiveRuntimeFactory.make
calls; tests pass one that throws — bypassing the permission gate without
bypassing the control flow under test. disableHypnagogicLoop stays private.

Asserted on failure: the resolver runs exactly once · nil is returned
rather than a substituted provider · the typed reason survives into lastError
unflattened · startupWarning is 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 == false was likewise asserted on a property
that defaults to false and was never set true in the test — deleting the
production assignment left it green.

Proving the toggle transition, and proving the call-site guard
short-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:

// in makeClaude, replacing the do/catch around ClaudeExecutableResolver.resolve:
let cliPath = resolveClaudeCLI()

// and restore:
private static func resolveClaudeCLI() -> String? {
    let candidates = ["/usr/bin/env", "/usr/local/bin/claude", "/opt/homebrew/bin/claude"]
    for c in candidates where FileManager.default.isExecutableFile(atPath: c) {
        return c
    }
    return nil
}
swift test --filter DialecticSessionTests

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(...) from
disableHypnagogicLoop in Sources/NeuralComposeApp/AppViewModel.swift.

swift test --filter AppViewModelRuntimeFailClosedTests

2 of 3 fail.

M3 — reintroduce provider substitution. In resolveHypnagogicRuntime,
change the catch to return a generator instead of nil:

} catch {
    disableHypnagogicLoop(reason: error)
    return (try! ClaudeCLIGenerator(), LiveRuntimeFactory.Resolved(
        name: "claude-cli", model: "claude-sonnet-5", systemPromptSource: "legacy-static"))
}
swift test --filter AppViewModelRuntimeFailClosedTests

2 of 3 fail.

M4 — delete the toggle assignment. Remove hypnagogicLoopEnabled = false
from disableHypnagogicLoop. All tests stay green — this is recorded as a
known 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

LiveRuntimeFactory performs no model-readiness probe, so an unpulled Ollama
model 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_R18 records the
current 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 6b8d1e4

target result
BCICoreTests 202 (2 skipped)
BCICloudBridgeTests 94 (2 skipped)
NeuralComposeAppTests 116
DialecticSessionTests 5

./Scripts/build.sh clean · git diff --check clean.

swift test with no filter aborts inside
BCIEEGTests.MindMonitorOSCStreamTests.testTruncatedSampleAddressCountsAsDroppedNotIgnored.
That crash reproduces identically on main, and this PR touches no BCIEEG
file. It is pre-existing and is not counted as a pass or a failure here.

Static search finds no try? LiveRuntimeFactory, ?? (ClaudeCLIGenerator, or
claude-cli-fallback in Sources/ — the only match is a doc comment describing
the 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
testLiveRuntimeHitsOllama does contact a localhost Ollama daemon when one is
running.

Packaging. Package.swift gains one .testTarget and declares an
already-transitive BCICloudBridge dependency explicitly. No product,
executable target, resource, or build setting changed. DialecticSession is an
executable target with top-level main.swift; SwiftPM 6 tests it without
altering the shipped binary, confirmed by running
./.build/debug/dialectic-session --help (exit 0, usage printed).

PATH exposure. .notFoundOnPath(searched:) is the only error embedding the
environment PATH. It is thrown only by resolve(), called only from
RuntimeFactory.makeClaude, where it is caught and translated back into
RuntimeFactoryError.claudeCLINotFound — which carries the actionable install
hint instead. LiveRuntimeFactory never calls resolve(), so the app's
published lastError cannot carry PATH today. Named follow-up: if the
shared resolver enters the UI path — likely when readiness probing lands in
A2 — that string would reach lastError and 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

aurascoper and others added 3 commits July 24, 2026 23:38
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>
@aurascoper

Copy link
Copy Markdown
Owner Author

Adversarial review — findings and corrections

The review found that the first two commits' tests did not prove what they claimed. Confirmed by mutation, not argument.

Two false greens, both mine

Mutation Before After 6b8d1e4
Revert makeClaude to the old resolveClaudeCLI() (the one returning /usr/bin/env) 93 tests green 2 of 5 fail
Delete hypnagogicLoopEnabled = false from disableHypnagogicLoop 4 tests green assertion removed as unprovable
Remove setLastError from disableHypnagogicLoop 2 of 3 fail
Make resolveHypnagogicRuntime substitute a provider instead of returning nil 2 of 3 fail

The first was the serious one: the tests pinned ClaudeExecutableResolver in isolation and never touched the defective caller, so the R2 production fix could have been reverted silently.

Fixes

R2 call site 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.

Vacuous R7 assertions deleted rather than patched. generatorsBuilt/processesLaunched counted call sites existing only inside the test's own closures and could never be non-zero. testFailureDoesNotTriggerAnAlternateProviderRequest 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 regardless.

The test file now states plainly what is not covered: the toggle transition and the call-site guard both need the mic/speech authorization gate stubbed, which is A2 work.

Also fixed

  • 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 hint in favour of a raw PATH dump. makeClaude translates notFoundOnPath back into it.
  • The --dry-run doc claimed dry runs succeed without Claude installed — true only because the old resolver always returned /usr/bin/env. Doc now records the behaviour change.
  • Egress stub leaked a temp directory per run; teardown added.
  • NeuralComposeAppTests imported BCICloudBridge transitively; now declared.

Accepted, not fixed here

  • Scope caveat worth stating plainly: only the override branch is validated. The app's production path (AppViewModelLiveRuntimeFactoryClaudeCLIGenerator(executablePath: nil)) uses the PATH branch and never calls the resolver. R2 fixes the DialecticSession harness only.
  • Witness runtime is resolved even when witnessEnabled == false — pre-existing, belongs with R3.
  • NeuralComposeAppTests is excluded from CI — pre-existing policy, R9.
  • TOCTOU between validate and launch — irreducible with Process; an attacker who can replace a file on PATH already owns the next invocation.
  • .notFoundOnPath embeds the full PATH. Unreachable from the UI today because LiveRuntimeFactory never calls resolve(). If it ever does, that string would land in the published lastError.

351 tests pass: BCICore 202 · BCICloudBridge 94 · NeuralComposeApp 116 · DialecticSession 5 · BCIClassifier 13 · BCIVoice 21. Build clean, git diff --check clean.

@aurascoper
aurascoper marked this pull request as ready for review July 25, 2026 07:03
@aurascoper
aurascoper merged commit 23c56ea into docs/eeg-methods-scope Jul 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant