fix(brains): resolve the capture credential from the client's MCP sign-in [BRNS-CORE-085] - #29
Merged
Merged
Conversation
…n-in Conversation capture and the inbox have been off for anyone who never pasted a token, since the plugin's first commit, with no request, no log line and no error to show for it. Three install paths reach that state and one of them is the command the README tells people to type. The credential now falls back to the MCP OAuth token the client already stored, which is the same credential class the hook endpoints accept, so any user whose brains tools work at all has one. Both hooks share one resolver: they used to build separate chains, and a Codex user with header auth had capture on and the inbox off. A resolved credential is a record — source, binding, origin, locator — and the token itself never occupies a shell variable at all. That covers the discovered credential; the three explicitly configured tokens arrive as environment variables and cannot, so the credential path also shields the caller's xtrace and restores it afterwards. Four disclosure channels were closed one at a time (xtrace, curl argv, the user's ~/.curlrc, and `set -a`, which exports locals too) before it became clear that shielding channels one by one was the wrong shape. The value now goes from the store through jq into a private 0600 curl config and from there into curl, so there is nothing to export, trace or inherit. Only brains_request presents it. The one cleanup primitive that removes these files refuses any path that is not a direct child of the temp root it created, so it cannot be aimed at a directory the library does not own. Endpoint URLs are env-overridable, so a discovered credential is bound to the origin that issued it and refused anywhere else; explicitly configured tokens still go wherever the user points them. Selection requires BOTH the canonical origin and a server name that identifies brains — another MCP server sharing the origin would otherwise have its token sent to the brains endpoints — and refuses when more than one distinct candidate matches, because neither store records which account a token belongs to and capturing into the wrong brain is worse than a 401. curl runs with -q so the user's own ~/.curlrc cannot route the header into a trace file, which is a disclosure channel neither argv nor xtrace covers. Every network call is bounded from inside brains_request, so no call site can be unbounded by omission. Store reads share one budget across the whole resolution rather than one deadline each, and candidates are narrowed by server name before any keychain read so that budget effectively never binds. The resolver starts at indeterminate and only a provably complete enumeration may downgrade that to no-credential, so an incomplete scan for any reason — the cap, the budget, an unreadable account, or every account failing at once — reports what it actually knows rather than what it happened to find. A keychain that answers "item not found" is a conclusive absence and stays no-credential, so a fresh install is still told to sign in; that distinction travels as an exit status, because every call site is a command substitution and a variable set in one never reaches the caller. An unread account matters for the same reason a truncated scan does: it may hold a different credential for the same server, and the readable one would otherwise be handed back as uniquely valid. A record that parses cleanly and simply holds no token is a conclusive answer about that account and does not count as incomplete, so one junk keychain entry cannot disable capture permanently. Reads clean up their temporary directory on INT, TERM and HUP as well as on the normal path, and sweep the reader's process group so a forked descendant cannot outlive them. Backgrounded requests take their own lease on the credential and clean it up on the same signals, so neither the hook's exit nor a terminated child leaves the bearer on disk; SIGKILL is untrappable and is swept by a prune at session start and session end. The off-state is now observable. Every request records a health state keyed by capability and by the origin of the URL it actually contacts — the same key the credential binding uses, since every endpoint is independently overridable — ordered by logical generation, so a stale completion cannot overwrite a newer one, a working inbox cannot clear a broken capture, and a healthy production session cannot clear a self-hosted one's rejection. Any non-zero curl exit is a transport failure whatever the status says: curl writes the code when headers arrive, so a transfer that dies mid-body still reports 200 and its truncated body can be valid JSON. Session start announces the state once, naming the one command that fixes it, and only an observed success re-arms it. The suite mutates the shipped code and requires the damage to show: each registered mechanism is neutered in a copy of the plugin tree and a probe must change its answer. Two guards had already decayed into tautologies that passed with their mechanism deleted, and three mechanisms shipped with no guard at all; this makes both states fail the suite rather than pass it. Codex reads its sign-in from the macOS keychain, and Codex on Linux is not a supported configuration, so there the note says that and names nothing rather than pointing at a token workaround for a platform the product does not support. Also: the current-time injection moves ahead of the credential gate, where it always belonged; per-session marker files are pruned; and the token option and README no longer describe a gate that no longer exists. Codex on Linux is called out explicitly as still needing a token, because where it stores its sign-in there has not been verified. BRNS-CORE-085
olegshmuelov
force-pushed
the
fix/capture-credential-resolver
branch
from
August 17, 2026 08:02
4dffaa6 to
25467ee
Compare
olegshmuelov
marked this pull request as ready for review
August 17, 2026 08:13
Ravid-SSVLabs
previously approved these changes
Aug 17, 2026
Ravid-SSVLabs
left a comment
Ravid-SSVLabs
left a comment
There was a problem hiding this comment.
Review at 25467ee0 — approving
First review on this PR. Full pass over the diff: the resolver library line by line (it is the security surface), the three hook integrations, the docs/config sync, and the test harness structure. All 5 CI checks are green.
What I verified holds:
- I1 (token never in a shell variable) is real in the code, not just the header: the discovered credential travels store → bounded-read file → jq → 0600 curl config →
curl --config, and the three explicit-token branches (the only place a secret is expanded) are wrapped in the xtrace shield. The response body and allBRAINS_CRED_*metadata are correctly treated as non-secret. The-q-first curl invocation closes the~/.curlrctrace channel, and the leak battery covers argv (pspolling),bash -x, and inheritedSHELLOPTS=xtracefor both the discovered and explicit paths. - I2 (indeterminate by default) is consistently enforced: every truncation, unreadable account, unparseable document, and budget expiry lands on
indeterminate, and only two provably-complete outcomes downgrade tono-credential(keychain error 44 + absent fallback file; a complete scan with zero entries). Theblockedstate for "store has credentials, none for this origin" gives the self-hosted-endpoint user the right remedy instead of a sign-in loop. - Binding —
brains_requestis genuinely the only call site presenting the credential (all four legacy curl call sites are gone), the origin check runs on every request for bound credentials, and explicit tokens crossing origins is documented as the user's choice. - The fire-and-forget lease solves a real bug the old code didn't have (the old code kept the token in argv, so backgrounding was free): the hook's EXIT trap would delete the config out from under an in-flight curl. Lease-before-background with self-cleanup in the subshell is the right shape, and the mutation gate proves the probe notices when the lease mechanism is neutered.
- Health generations — begin-under-lock, apply-if-highest correctly closes the lost-update race that atomic rename alone leaves open; per-capability × per-origin scoping matches the stated 200/403-same-turn scenario, and an
okreleases only the claims that success actually disproves. - Transport truth — non-zero curl exit discards the body even on a 200, pinned by the truncator stub.
- bash 3.2 compatibility is clearly deliberate (
trinstead of${var,,}, no associative arrays,PIPESTATUSonly) — this matters since macOS ships 3.2 and that's the primary Codex platform. - Docs stay honest: plugin.json option copy, README (both clients), and core.md v7 all describe the token as an override now, and the plugin-contract test pins the new copy so it can't drift back.
The test suite deserves its own mention: mutation-gating the six mechanisms in a copied tree, with a requires guard so a probe that can't observe its mechanism fails loudly instead of passing vacuously, is exactly the countermeasure for the tautology-decay it describes.
Non-blocking findings (3)
-
Latent undercount in the Codex keychain distinct-token collapse —
brains-credential.sh, thexargs -0 -n 200 … jq -s … | tail -1line. If more than 200 matched records ever reached this point, xargs would split into multiple jq invocations andtail -1would keep only the last batch's unique-count — two distinct tokens split across batches would read as one and resolve an ambiguity that should refuse. Unreachable today:BRAINS_CRED_MAX_CANDIDATES=8marks anything larger TRUNCATED → indeterminate well before this line. But nothing at this line records that it depends on the cap; a one-line comment (or a single-invocation construct) would keep a future cap raise from silently arming it. -
_brains_write_configreuses a deterministic work dir withmkdir -p(r.$$.cfg). Every other work dir gets a$RANDOMsuffix and a baremkdirthat fails on collision; this one silently adopts a pre-existing directory after PID reuse against a SIGKILL orphan. Same user, 0700, pruned at the next session boundary — so no exposure, just an inconsistency with the file's own discipline. Anrm -rfbefore themkdirwould close it. -
brains_originmis-parses a pathless URL carrying a query (https://host?x=1→ the query string lands in the authority → no match → bound credential blocked). Only reachable through a pathlessBRAINS_INBOX_URL-style override, since every built-in URL has a path before the?, and it fails in the safe direction — refuse, never missend. Splitting the authority at[/?#]rather than/alone would make the parse exact.
None of these block: the first two are unreachable or harmless today and the third fails safe. Approving.
|
🟢 Nit: |
…ound in review [BRNS-CORE-085] Three reviewers read the resolver. Between them they found that the guards were thinner than the thing they guarded, in five distinct ways. Cleanup ran on INT/TERM/HUP as well as EXIT. A cleanup-only handler does not re-raise, so bash runs it and RESUMES — the process stops dying on that signal, and any handler the caller had is replaced outright. Once a credential resolved, the turn hook stopped responding to Ctrl-C for the rest of its run, on the path every working user takes on every turn. The existing test could not see it: its store read fails, so it never reached the config writer where the trap lived. That was the fourth instance of this mistake on one change, so it is now stated once as a rule — I3, cleanup on EXIT only, never on a signal — and enforced by a scan over every shipped file. The rule found two more instances in the hooks that nobody had reported. Raising the document ceiling had quietly traded a fast reject for a slow accept. The selection loop spent two forks on every entry, one recomputing the field separator and one canonicalising an origin, so a store the wider ceiling exists to admit cost seconds per hook run, twice a turn. The separator is hoisted, the fork-free predicate is tested first, and the loop now consults the total budget this file documents as bounding the whole resolution but only ever applied to one path. Measured on the full hook: 3.97s -> 0.63s at 1450 entries, 2.89s -> 0.57s at 1000, identical answers at both. The Codex array backend deleted the snapshot every row shares on its first non-matching row, because the guard was keyed on the client rather than on shared-ness; selection then found its one credential and the config writer read a deleted file. Keyed on shared-ness directly, both orders of a mixed array resolve. The Codex-on-Linux note claimed there was nothing to change, which is measurably false — an explicit token resolves and captures there — so a note with no action now has its own shape instead of borrowing the offer protocol and telling the agent to offer something that does not exist. The suite itself was non-deterministic under load, and that turned out to be one bug rather than the flakiness and the containment gap it looked like: fixed stub ports meant a second run talked to the first run's server and its receipts landed in a log nobody was reading, which let an assertion that no credential crossed origins pass while one did. Ports are chosen by the kernel now. Three concurrent runs went from two to four failures each to none. Coverage was the larger problem. Behavioural claims about the shipped shell were pinned as substrings, which a comment can satisfy — three already were, including both guards against the two hooks regrowing separate credential chains, carried by a `# shellcheck source=` directive. Those are code-only region pins now, with a self-scan that fails if the weaker form reappears. And the mutation gate proved a hand-picked list of mechanisms load-bearing while being incomplete by construction: the server-identity guard could be replaced with `return 0` with every suite green while another MCP server's bearer was selected and posted to the ingest endpoint, and the capture log line — the observability this whole change is justified by — was deletable without a single test noticing. scripts/mutation-coverage.sh derives the candidate set from the diff instead of from a list: it neuters each changed line and requires something to fail. Of 485 independently mutable lines it found 227 survivors, which is how the session-start housekeeping, the `now:` injection, the off-state health note and the Codex synchronous ingest turned out to have no coverage at all. It is not part of the per-commit gate; one suite run per line is an hour, not seconds. BRNS-CORE-085
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Conversation capture and the inbox have been silently off since the plugin's first commit for anyone who never pasted a token — no request, no log line, no error. Three install paths reach that state, reproduced on a fresh
CLAUDE_CONFIG_DIR; one is the command this README publishes. BRNS-CORE-015 moved tool auth to OAuth and deliberately left the hooks on a separate credential, which this completes.Capture and the inbox now use the MCP sign-in that already authenticates the tools, which is the same credential class the ingest and inbox endpoints accept. An explicitly configured token stays available as an override, for a self-hosted endpoint or a different account. One shared resolver replaces two divergent chains, so a Codex user with header auth no longer has capture on and the inbox off. A resolved credential never occupies a shell variable: it flows store → jq → a private 0600 config →
curl --config, and is bound to the origin that issued it, refused for any other target. Selection is by canonical origin plus server identity, and refuses when more than one distinct candidate matches or when enumeration is incomplete — neither store records which account a token belongs to, and capturing into the wrong brain is worse than a 401.The off-state is now observable. Every request records per-capability health ordered by logical generation, so a stale completion cannot overwrite a newer one and a working inbox cannot clear a broken capture — a read-scoped token returns 200 and 403 on the same turn. Any non-zero curl exit is a transport failure whatever the status says: curl writes the code when headers arrive, so a transfer that dies mid-body still reports 200 and its truncated body can be valid JSON. Session start announces the state once, naming the one step that fixes it, and only an observed success re-arms it.
Also: current-time injection moves ahead of the credential gate, where it always belonged; per-session marker files are pruned; and the token option and README no longer describe a gate that no longer exists.
Deliberate trade-offs, not oversights
curl -qmeans a self-hosted user's custom~/.curlrcCA or proxy is ignored. That is what closes the trace-to-disk channel for the bearer.Consolidating onto a single credential, which removes the remaining token inputs and the
endpointoption, is tracked in BRNS-CORE-086. It is gated on verifying Codex-on-macOS sign-in against a real store.Testing
New
tests/credentialsuite (109 checks) plus additions toplugin-contractandinbox-v2. The suite mutates the shipped code and requires the damage to show: six mechanisms are registered in one table, each neutered in a copy of the plugin tree with a probe that must change its answer. That exists because two earlier tests had decayed into tautologies while staying green, and it caught three of its own probes failing to observe their mechanism on first run.Verified live on macOS and in a Debian container: a tokenless session captures with
source=claude-oauth, an absent store reportsno-credentialand emits one session-start signal, and no credential files remain on disk after any run.