Repository navigation
🔌 feat: Add Decision Credentials, a Breaker, and a Readiness Probe - #625
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59e661e9e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| }); | ||
|
|
||
| return ({ refresh }): Promise<string> => { |
There was a problem hiding this comment.
Honor the credential abort signal
When an HTTP decision is cancelled or reaches its deadline while the token command is running, the transport passes that combined AbortSignal to the minter, but this implementation discards it by destructuring only refresh. The caller therefore returns while the child process continues for up to the independent 60-second default, and pending makes subsequent calls reuse that abandoned execution and potentially stall as well. Coordinate the supplied signal with the shared subprocess so an unneeded mint is terminated and cleaned up.
AGENTS.md reference: AGENTS.md:L39-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7f862be. The minter now honors signal. An already-aborted caller rejects with aborted without spawning. Each caller joins the shared run and counts as a waiter; its abort rejects only that caller. When the last waiter aborts, pending is cleared at once and the run is killed, so the next call starts fresh instead of joining an abandoned run. A run that finishes after it was abandoned does not write the cache.
Fixing this exposed a second gap: exec's kill only reached /bin/sh. dash forks the command rather than exec-ing it, so the real command outlived both an abort and the existing timeoutMs kill. The run now uses spawn with detached off Windows and signals the whole process group. A synchronous spawn throw, whose Node message quotes the command, now becomes a plain network failure.
Tests in node.test.ts:
- "kills a run once its only caller aborts and starts fresh for the next call": rejects
aborted, mintstoken-2on a second run, and the command's pid has exited. - "keeps a shared run alive while another caller still waits on it"
- "rejects an already-aborted caller without running the command"
- "reports a run killed at timeoutMs as a timeout…" now also asserts that the forked command exited.
- "drops the message of a spawn that throws, which quotes the command"
All of them fail against 59e661e. The two kill assertions also fail with the process-group kill disabled. Tradeoff: a token command slower than the decision deadline is now killed rather than warming the cache in the background. The README says so and points to probe with a longer timeoutMs to warm it.
| if (refresh) { | ||
| /** The cached token was refused; never hand it out again. */ | ||
| cached = null; |
There was a problem hiding this comment.
Retain refused tokens for failure redaction
When a cached token receives a 401, this clears the only saved copy before running the refresh command. If that command then fails and its stderr repeats the refused token, the error path cannot add the old value to secrets; a medium-length token that is not prefixed with Bearer and is shorter than the opaque-token threshold is consequently returned verbatim in DecisionError.message. Preserve the invalidated token separately until refresh diagnostics have been redacted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7f862be. A refresh now sets expiresAt = 0 instead of nulling cached, so the refused token is never served again but stays in the failure path's secrets until a successful run replaces it. This also covers a refresh that arrives while a run is pending. Test: node.test.ts › "redacts a refused token when the refresh that replaces it fails" (mints tok-medium-secret-1234, then a refresh that writes it to stderr and exits 1, and expects refresh failed for [REDACTED]). It fails when expiresAt = 0 is reverted to cached = null.
| cached = token; | ||
| expiresAt = started + ttlMs; |
There was a problem hiding this comment.
Measure the cache TTL from a successful mint
For configurations where the command runtime approaches or exceeds ttlMs, calculating expiry from started means the newly returned token is already expired and the very next non-refresh call executes the command again. This contradicts the option's documented behavior of reusing a minted token for ttlMs and can turn every request into a shell invocation; set the expiry from the successful completion time instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7f862be. expiresAt = Date.now() + ttlMs is now set when the run completes, and the ttlMs JSDoc says it counts from completion and should leave headroom for the command's runtime. Test: node.test.ts › "counts the ttl from when the command finished, not when it started". On a mocked clock the command takes 10 s with ttlMs: 5_000. A call 4 s after completion reuses token-1 (runs=1), and one 5.001 s after completion mints token-2. It fails against 59e661e and with the anchor moved back to the start time.
| result = result.split(secret).join(REDACTED); | ||
| } |
There was a problem hiding this comment.
Detect shaped credentials before replacing listed secrets
When a listed secret is also part of the syntax used by a later recognizer, the literal replacement can disable that recognizer and leak a different credential. For example, redact('Bearer live-token', ['ear']) produces B[REDACTED]er live-token; the subsequent bearer regex no longer matches, and the bearer value is too short for the opaque-token fallback. Apply bearer/JWT recognition before literal substitutions, or otherwise prevent listed replacements from destroying credential prefixes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9f8b4a9. Running the shape patterns first would only reverse the problem: a listed secret spanning into a replaced bearer value would then leak its remainder. So redact now collects spans from every recognizer (listed literals, the bearer value, JWT, OPAQUE) against the original text and replaces their union in one pass. Test: breaker.test.ts › "still recognizes shaped credentials that a listed secret overlaps". It covers ('Bearer live-token', ['ear']) → B[REDACTED]er [REDACTED], 'Authorization: Bearer' listed, a JWT whose header segment is listed, and a 62-character opaque token split by a listed secret. It fails against the sequential implementation at 59e661e.
7f862be to
ed2e58c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed2e58c08a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| try { | ||
| if (GROUP_KILL) { | ||
| process.kill(-child.pid, KILL_SIGNAL); |
There was a problem hiding this comment.
Escalate termination when token commands ignore SIGTERM
When the token command traps or ignores SIGTERM, this one-shot process-group signal does not terminate it; the promise nevertheless rejects and clears pending, so later calls can start replacement commands while the detached original continues running indefinitely. Add a bounded grace period followed by a forceful kill so timeouts and last-waiter aborts cannot accumulate orphaned token processes.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb6f2b5. A kill now sends SIGTERM to the group and arms a SIGKILL for it after killGraceMs (new option, default 2 s, 0 escalates at once). On the shell's exit the escalation is cleared only when process.kill(-pid, 0) reports the group gone (ESRCH), so a forked command that ignores SIGTERM after the shell itself died is still reached. The caller still rejects immediately; a killed run can overlap its replacement for at most the grace period instead of indefinitely. JSDoc, README and PR body now state the escalation and that on Windows a kill ends only the shell.
Proving tests in src/decisions/node.test.ts:
force-kills a timed-out command that ignores SIGTERM after the shell that forked it dies(<cmd>; true, so dash forks)force-kills an abandoned command that ignores SIGTERM
Both fail with the SIGKILL timer turned into a no-op, and both also fail when the escalation is cleared on shell exit without the group probe (the TERM-ignoring node process survived in each case). Checks: jest src/decisions src/rerank 179/179, tsc, eslint, prettier, build + test:light-entries 16/0, check:circular-deps.
fb6f2b5 to
83533ae
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83533ae9f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...echoed, | ||
| cached ?? '', | ||
| ...stdout().split(/\r?\n/), | ||
| ].filter((line) => line.trim().length >= MIN_SECRET_LENGTH); |
There was a problem hiding this comment.
Redact cached tokens regardless of length
When a successfully minted token is shorter than MIN_SECRET_LENGTH, a later refresh failure can expose it if stderr repeats the refused token: lastLine accepts tokens of any nonzero length, but this filter removes the cached value before redact sees it, and the shape recognizers do not catch short opaque values. Fresh evidence since the prior finding is that the token is now retained in cached, but this length filter still discards it from the failure-path secrets; always retain the cached token even if other short stdout lines are excluded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98758fa. The premise holds: the length floor of 8 characters is meant for guessed secrets, but it also dropped the cached token, which is a known credential. fail() now applies MIN_SECRET_LENGTH only to the guessed lists (the command words in echoed and the lines of a failing run's partial stdout) and always lists cached (redact already skips null and empty entries). The JSDoc and README now say "a minted token of any length".
The test is node.test.ts "redacts a cached token shorter than eight characters when its refresh fails": a 7-character token Zq7xK2p, then a refresh whose stderr echoes it. The message must be token command exited with code 1: refresh failed: token [REDACTED] was refused. With the old filter restored, the test fails and receives the token in the clear.
Short lines of a failing run's partial stdout are still filtered on purpose. That run produced no token, and listing short words like ok would garble the excerpt. The comment on that line states this trade-off.
| if (refresh) { | ||
| /** The cached token was refused; never hand it out again. */ | ||
| expiresAt = 0; |
There was a problem hiding this comment.
Reuse a newer token when a stale request refreshes
When concurrent requests use cached token A and receive staggered 401 responses, the first refresh may already have cached token B by the time a later A request reaches this branch. That stale request nevertheless expires B and starts another command, so a burst at token expiry can mint once per delayed response; if minting revokes the previous token or a later command fails, otherwise valid retries can be rejected and the breaker can open. Track which cached generation each request used, or pass the refused token into the refresh operation, so a refresh caused by A can reuse B rather than invalidating it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5dbc12a. The premise holds: the minter contract had no way to say which token was refused, so a 401 that arrived late on token A cleared the cache that already held B and ran the command again. I fixed it at the contract instead of only in commandToken. DecisionTokenMinter now receives refused on a refresh: the key the 401 refused, as it was sent (trimmed). The transport records it when it refreshes (refused = key). commandToken now invalidates only when refused === cached, or when refused is absent, which keeps the old behavior for callers that don't pass it. In every other case it returns the newer cached token, or joins the run that is minting it.
Tests:
node.test.ts"retries a stale 401 with the token another call already refreshed, without minting again": two concurrentdecide()calls throughwithBreaker(createHttpDecisionModel(...commandToken)). The second call's 401 on token-1 is held until the first call has refreshed and succeeded, and run 3 of the command exits 1. Result: both calls fulfilled,runs() === 2, breaker closed.node.test.ts"hands a refresh that refused an older token the one minted since" covers commandToken directly.decisions.test.ts"refreshes a minted key once for a 401" now asserts the minter sees{ refresh: true, refused: 'old' }.
I proved the fix both ways. With the commandToken check reverted, both node tests fail. With the transport reverted to not pass refused, the stale-401 test and the decisions test fail.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98758faa39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...stdout() | ||
| .split(/\r?\n/) | ||
| .filter((line) => line.trim().length >= MIN_SECRET_LENGTH), |
There was a problem hiding this comment.
Redact short stdout token candidates on failure
When an initial token command prints a credential shorter than eight characters and then exits nonzero, times out, or overflows, this filter omits that stdout line from secrets. If stderr repeats the value (for example, token abc1234 failed), none of the shape recognizers catches it, so describeFailure exposes the credential in DecisionError.message, where it may be logged. Since successful command tokens may have any nonzero length, retain all non-empty stdout token candidates for failure redaction rather than applying MIN_SECRET_LENGTH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 97a73fb. fail() now lists every non-empty stdout line whatever its length (a successful run accepts a token of any length), and commandSecrets() had the same length cutoff, so it now lists every command word too; a short word may garble the excerpt instead of leaking. Tests in src/decisions/node.test.ts: "redacts a stdout token shorter than eight characters when its first run fails" (stdout Zq7xK2p, stderr repeats it, exit 1) and "redacts a short inline secret that the command quotes back" (--pw=abc12). Both fail with the previous node.ts restored and pass with the fix.
| if ( | ||
| lastError.status === 401 && | ||
| typeof credential === 'function' && | ||
| !refreshed | ||
| refused === undefined && | ||
| typeof key === 'string' |
There was a problem hiding this comment.
Recheck the token before treating a second 401 as fatal
When a call refreshes token A to B and its request with B is delayed, another call can receive a 401 for B, refresh successfully to C, and finish before the delayed B response arrives. Because refused still records A here, the delayed call cannot consult the minter again, returns unauthorized, and causes withBreaker to open even though C is now valid; all subsequent calls are then rejected until a manual reset. Before treating this post-refresh 401 as fatal, allow the minter to determine whether key has already been replaced and reuse that newer generation without minting again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6992826. A call still refreshes at most once, but after a second 401 it now asks the minter once more with refresh: false and re-sends only a key it has not had refused. With commandToken, that recheck returns the cached newer token (C) without running the command; when nothing newer exists, it returns the refused B and the 401 stands without a re-send. So a call sends at most three keys after 401s, and the unbounded re-mint a second refresh would allow cannot happen. Asking the minter (rather than keeping a transport-level latest key) also covers models that share one minter and one breaker. Tests: node.test.ts "rechecks a refreshed token refused late and sends the one another call minted since" (your sequence: X holds B, Y refreshes to C and succeeds, X 401s on B then succeeds with C; 3 runs, breaker closed). breaker.test.ts "opens without re-sending when the recheck after a second 401 has nothing newer" and "refreshes a credential function once, rechecks once, then opens when every key is refused". All three fail with the previous transport.ts restored.
| if (end.kind === 'exit') { | ||
| return { | ||
| failure: 'unauthorized', | ||
| reason: `token command exited with code ${end.code}`, | ||
| }; |
There was a problem hiding this comment.
Do not treat shell-encoded signals as credential refusals
When the actual token command is killed externally while the shell: true wrapper survives—for example by an OOM killer or an operator targeting the command—POSIX shells report this as an ordinary exit status such as 137 (128 + SIGKILL), so Node supplies a numeric code and a null signal. This branch classifies that execution failure as unauthorized, causing withBreaker to open permanently even though no credential was refused; recognize shell-encoded signal statuses as a nonfatal execution/network failure rather than treating every nonzero code as authentication failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5159308. Off Windows, a close with a numeric exit status of 129–192 (128 + N for signals up to 64) is now a shellSignal end. It classifies as network, with the reason "token command exited with code 137 (killed by signal 9)", so it never trips a breaker. 126/127 and every other code still read as unauthorized. The JSDoc and README also note that cmd | tail -1 hides a killed cmd behind exit 0 with no output, which no exit-code rule can detect. Test: node.test.ts "reports a forked command the shell saw killed by a signal as network, leaving a breaker closed" (sh -c "kill -KILL \$\$" && echo token-1 through withBreaker: network, breaker closed, no HTTP request; exit 3 stays unauthorized). It fails with the previous node.ts restored.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 763b80a876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pending === null && cached !== null && Date.now() < expiresAt) { | ||
| return Promise.resolve(cached); |
There was a problem hiding this comment.
Invalidate tokens minted before a breaker reset
When a pre-reset command finishes before the next caller arrives, settle has already cleared pending and the run was never marked detached, so its token is cached and returned here despite belonging to the old breaker generation. A post-reset request can therefore use the stale credential; if the server rejects it with 403, which the transport classifies as unauthorized without attempting a refresh, the breaker immediately reopens instead of running the repaired token command. Store the generation with the cached token and bypass it after a reset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3979477. The cache now records the breaker generation of the run that minted it (cachedGeneration, set alongside cached in ended), and a cache hit requires cachedGeneration === breaker?.generation. cached itself is kept, so redaction and the refused === cached refresh check still see it. The option JSDoc, the withBreaker JSDoc and the README now say a pre-reset token, cached or in flight, never serves a post-reset caller.
Proving tests in src/decisions/node.test.ts:
mints afresh after a reset instead of reusing a token cached before it, even one a 403 refused: a 403 opens the breaker onbad-1, the operator logs in and resets, and the next call sendsgood-2. The breaker stays closed and the command ran 2 times.mints afresh after a reset that a run from before it outlived with no caller arriving meanwhile: the post-reset call getstoken-2, nottoken-1.
With only the generation check removed, both fail (one receives token-1 where token-2 is expected). With it restored, all 35 node tests pass, as do 198 tests across src/decisions and src/rerank, tsc, eslint, prettier, the build, light entries, and circular-deps.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39794770df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| at !== -1; | ||
| at = text.indexOf(secret, at + 1) | ||
| ) { | ||
| spans.push([at, at + secret.length]); |
There was a problem hiding this comment.
Bound span collection for short secrets
When commandToken redacts near-limit stderr and the command contains a short word that occurs frequently in that output, this loop allocates one tuple per occurrence and later sorts the entire array. A one-character word repeated across the permitted 1 MiB output creates over a million arrays and roughly 100 MiB of transient heap, so a failing credential command can stall or exhaust a daemon while merely formatting its error; merge matches incrementally or otherwise bound/coalesce spans before storing them.
AGENTS.md reference: AGENTS.md:L39-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 873e81a. The cost was worse than one short word: fail() lists every stdout line without deduplicating, so the span count was (copies of a secret) × (its matches). A failing command that printed 20,000 } lines to stdout and 3,000 JSON lines to stderr aborted the host with a fatal heap error (uncatchable) while it formatted the error.
What changed in src/decisions/redact.ts:
- Secrets are deduplicated. They are still iterated with
for...of, so the probe's unreadable-secretscontract still throws and is caught. - Each secret's overlapping matches are merged into runs as they are found.
- Coverage is an
Int32Arraydifference array sized by the text, replacing one tuple per match and the sort. - The search time still grows with distinct secrets × text length (
a,aa,aaa… against a run ofas). So the token command's excerpt now goes throughredactExcerpt, which collapses whitespace (in the secrets too), reads only the first 4,096 characters, and keeps 200 of them. When stderr runs past that window, the tail is redacted back as far as the longest secret reaches and through any token-shaped run the cut splits, so a credential the cut splits does not show its first characters.
Proving tests:
redact.test.ts"keeps memory proportional to the text when a short secret repeats throughout it" (200 ×}against 1 MiB) andnode.test.ts"formats the failure promptly when stdout repeats short lines that stderr is full of" (the 20,000-line case). Both abort withFATAL ERROR: ... JavaScript heap out of memoryon the previousredact.ts.- "formats the failure promptly against many distinct stdout lines that stderr matches everywhere" and the
redactExcerptbound test took 39 s and 40 s with the window removed, and fail their time bound. - The two cut-off tests leak
hunteand a run ofBs with the tail cover removed. - "redacts a listed secret whose whitespace the excerpt collapses" fails without collapsing the secrets.
| return [command, ...command.split(/[\s'"`=]+/)].filter( | ||
| (word) => word.trim() !== '' |
There was a problem hiding this comment.
Redact shell-decoded command arguments
When an inline CLI secret contains a shell-escaped character, splitting the raw command does not recover the argument that the shell passes to the program. For example, with mint --password=abc\$def, this list contains abc\$def, but the CLI receives abc$def; if it reports bad password abc$def, the short value matches none of the shape recognizers and is returned verbatim in the DecisionError. Fresh evidence beyond the earlier short-command-word fix is that shell escape removal changes the value before stderr is produced; redact decoded literal arguments as well, or suppress diagnostics that cannot be scrubbed safely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 873e81a. I reproduced it first on 3979477: a stub CLI run as mint --password=abc\$def produced bad password abc$def.
The root problem is wider than escapes. Redaction matches the command text, but the program echoes its argv after the shell has rewritten it. Backslash escapes, escaped spaces, and $/backtick expansions ($HOME, $(...)) all produce values that no word of the text contains. Escapes could be decoded, but expansions can't be known in advance, so there is a single rule for all of them. rewritesWords checks for a backslash, $ or backtick outside single quotes (on Windows: %, ^, or \"). When the command has one, describeFailure withholds stderr and the message reads token command exited with code N (stderr withheld: the command uses shell escapes or expansion). Single-quoted text is literal, so jq -r '.token' and awk '{print $2}' keep their excerpt. Words now also split at {, } and ,, because bash brace expansion (--pw={ab12,cd} becomes --pw=ab12 --pw=cd) would otherwise hand the program a piece that no word matches.
Proving tests in src/decisions/node.test.ts ("when the shell rewrites an argument the command text does not contain"):
withholds stderr after %scovers--pw=abc\$def,--pw=abc\ def,"--pw=abc\$def","--pw=$HOME",--pw=$(printf ab)cdand a backtick substitution. WithrewritesWordsforced tofalse, all 6 fail, and the received messages includedbad password abc$def,bad password /home/dannyandbad password abcd.redacts each piece of a word that brace expansion splitsfails with the old word split (receivedbad password ab12).keeps the excerpt when $ and \ appear only inside single quotesguards against over-withholding.
JSDoc, README and the PR body now describe the withholding rule.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 873e81ab1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function commandSecrets(command: string): string[] { | ||
| return [command, ...command.split(/[\s'"`=,{}]+/)].filter( | ||
| (word) => word.trim() !== '' | ||
| ); |
There was a problem hiding this comment.
Redact arguments terminated by shell operators
On POSIX, when a short inline credential is immediately followed by a shell control operator, the shell strips the operator but commandSecrets retains it because this delimiter regex omits ;, &, |, and parentheses. For example, mint --password=abc12; exit 1 passes abc12 to the program, while the redaction list contains only abc12;; if stderr reports bad password abc12, neither literal nor shape redaction catches it and the credential leaks through DecisionError.message. Fresh evidence beyond the previously addressed escape/expansion case is that shell tokenization itself changes arguments without making rewritesWords withhold stderr; split on shell operators as well or treat such commands as unscrubbable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9f5d655. commandSecrets now also splits at ;, &, |, (, ), < and >, the operators that end an unquoted word without whitespace in a POSIX shell (and in cmd.exe), so --pw=abc12; lists abc12. Splitting quoted text there too only redacts more. Other shell rewrites from the class sweep don't put the secret in stderr: a # comment drops text the program never sees, and tilde or glob expansion substitutes a path rather than the secret. Escapes and expansions are already withheld via rewritesWords.
Proving tests in src/decisions/node.test.ts: 'redacts an argument the shell ends at %s without whitespace' covers ;exit 7, &&true, ||exit 3, |cat;exit 5, >/dev/null and </dev/null, and 'redacts an argument the shell ends at a closing parenthesis' covers (cmd --pw=abc12). With the old split regex restored, all 7 fail with bad password abc12 in the message. With the fix, src/decisions + src/rerank pass 224/224. The JSDoc, README and PR body now list the operators.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f5d6552c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const [segment] of command.matchAll(SHELL_SEGMENT)) { | ||
| if (!SINGLE_QUOTED.test(segment) && /[\\$`]/.test(segment)) { | ||
| return true; |
There was a problem hiding this comment.
Withhold stderr when pathname expansion rewrites arguments
On POSIX, an unquoted *, ?, or bracket expression can be expanded by the shell into an argument that does not occur in the command text, but rewritesWords still marks such commands as scrubbable. For example, if /tmp/sec7 exists, mint --password=/tmp/sec* can pass /tmp/sec7; when the program reports that short value on stderr, neither commandSecrets nor the shape recognizers redact it, exposing it in the DecisionError. Fresh evidence beyond the earlier shell-rewrite finding is that the current predicate checks only backslashes, $, and backticks and therefore misses pathname expansion; detect unquoted glob syntax or withhold stderr for these commands too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing the check; wording narrowed in e79820a.
The example does not expand. The shell globs the whole word --password=/tmp/sec*, so it only matches a directory literally named --password= in the cwd. With a real sec7 file next to the pattern, dash (/bin/sh here), bash and bash --posix all passed the pattern through literally, and the real commandToken reported token command exited with code 7: [REDACTED] [REDACTED] [REDACTED]=[REDACTED]. commandSecrets splits at =, so the literal /tmp/sec* is a redacted word.
When a glob or ~ does expand (as its own word), the program gets a path read from the filesystem or HOME, not text the command carried. rewritesWords withholds stderr for the expansions whose output can be a secret the text does not show: $VAR, $(...), backticks, and escapes that decode an inline secret into a different string. A secret could only leak through a glob if it contained an unquoted *, ? or [...] and a file in the cwd matched the whole word. In that case the secret's characters are already a filename on disk, and the program receives that filename. This matches the earlier ruling on 4346240050 (9f5d655): "tilde or glob expansion substitutes a path rather than the secret."
The docs did overclaim, though. They said stderr is withheld whenever the shell may pass an argument the text does not contain, and glob and tilde do that. The rewritesWords doc, the commandToken JSDoc, src/decisions/README.md and the PR body now say "an argument built from text the command does not show (a variable, command output, or a decoded escape)". They also state that pathname and tilde expansion are not withheld and can show existing paths in the excerpt.
| try { | ||
| return redact(message, secrets).slice(0, DETAIL_LIMIT); |
There was a problem hiding this comment.
Bound probe details before allocating redaction coverage
When a custom DecisionModel throws a DecisionError with a very large diagnostic message, safeDetail redacts the entire message before slicing it to 200 characters. cover consequently allocates an Int32Array proportional to the full message and scans it once per distinct secret, so a readiness probe can consume hundreds of megabytes or stall despite returning only a short status detail. Apply a bounded excerpt-redaction path before the full-size coverage allocation.
AGENTS.md reference: AGENTS.md:L41-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e79820a, along with the rest of this class.
- Probe:
safeDetailnow callsredactExcerpt(message, secrets, 200), so only the start of the message is read. An unreadablesecretsstill returns the fixed detail. - Full-text
redact: the cost was worse than memory. The backtracking JWT pattern\beyJ[\w-]{8,}\.[\w-]{8,}\.[\w-]{8,}was quadratic on runs like repeatedeyJ-, and it blocked the event loop afterdecidesettled, where the probe's timeout cannot reach.jwtSpansnow measures each segment once and, after a failed start, resumes past that start's first segment. Any later start inside that segment shares the same tail and has a shorter first segment, so it fails too. redactExcerpt: it used to collapse whitespace across the whole text first. It now collapses at most the first 16,384 characters and treats a longer text as cut, so the existing tail cover applies.
Proving tests. Each fails with its fix removed:
breaker.test.ts"reads only the start of an oversized failure message": an 800 KB message givestoken [REDACTED] was refused [REDACTED]in under 500 ms. It took 136 s against the previousprobe.ts/redact.ts.redact.test.ts"finds JWT shapes in time linear in the text": 800 KB ofeyJ-in under 500 ms (120 s with the old pattern). "matches the backtracking JWT pattern on generated text" checks the new scan against the old regex on 5,000 generated strings.redact.test.ts"reads a bounded prefix of a long text with whitespace to collapse": under 100 ms (872 ms before). "hides a listed secret that the raw prefix cuts through" leakedx hunwith the raw-cut check removed.
probe.ts was the only full-text redact( call in src. node.ts already used redactExcerpt.
eac8f5f to
40620d3
Compare
An unauthorized or quota failure belongs to the credential, not the request, so every later call would be refused the same way. withBreaker(model, breaker?) opens on the first one: later decide() calls reject at once, without a request, with the same failure and status and circuitOpen: true, until breaker.reset(). Rate limits, server errors, timeouts, and network failures never trip it. DecisionError gains a circuitOpen flag, false by default. probe(model) asks one tiny boolean question and never throws: it resolves ok with the answering model, or the typed failure with a redacted detail; a non-SDK exception reads as network with its message dropped. redact(text, secrets) removes listed secrets, bearer values, JWTs, and long opaque tokens. All three ship on the platform-neutral decisions entry.
Gateways such as ClickHouse's issue hourly bearer tokens from a CLI, so a host had to wrap the command itself. commandToken(command, { ttlMs, timeoutMs }) returns a DecisionCredential that runs the command, takes the last non-empty stdout line as the token, reuses it for ttlMs (default 50 minutes), shares one run between concurrent callers, and runs it again when the transport refreshes after a 401, dropping the refused token. A failed run is an unauthorized DecisionError naming the exit status with a redacted stderr excerpt; it never carries stdout, the command, or a token.
It needs node:child_process, so it ships on a new @librechat/agents/decisions/node entry (and the root entry) while @librechat/agents/decisions stays platform-neutral. The light-entry guard now loads the new subpath, records Node built-ins, and fails if ./decisions or ./rerank reaches any, or ./decisions/node reaches anything beyond child_process.
createBreaker counts resets, and withBreaker trips only when the generation it read before the request is still current, so a 402 or 401 from a call that started before reset() cannot reopen the circuit.
… Refusals commandToken now rejects with 'timeout' when exec kills the command at timeoutMs and 'network' for spawn errors, output overflow, or an outside signal; only a non-zero exit or empty output is 'unauthorized', so a slow token command no longer trips withBreaker. The failure message also redacts the command text and each credential-length word of it. The ttl test drives expiry with a skewed Date.now instead of a 40 ms real ttl that a slow spawn could outlive.
…Pass Every recognizer (listed literals, bearer values, JWTs, long opaque runs) now reads the original text and the union of their matches is replaced at once. Replacing listed secrets first could break the 'Bearer' keyword, a JWT's header segment, or a 40-character run, and leak the credential the shape check would have caught.
…ycle - A refresh stops serving the refused token but keeps it, so a failed refresh whose stderr repeats it still redacts it. - ttlMs counts from when the command finished, so a command slower than the ttl no longer runs on every call. - The minter honors the caller's signal: an aborted caller rejects at once, and a run is killed only when every caller sharing it has left, so later calls start fresh instead of joining an abandoned run. - Off Windows the command runs in its own process group and a kill (at timeoutMs or on abandonment) reaches the command the shell forked, not only the shell. - A synchronous spawn error, whose message quotes the command, becomes a plain network failure.
A timed-out or abandoned token command got one SIGTERM and its run was released at once, so a command that traps or ignores SIGTERM kept running detached while later calls started replacements. A kill now arms a SIGKILL for the run after killGraceMs (default 2 s), cleared only when the shell has exited and its process group is empty, so a forked command that survives the shell is still reached. Windows kills reach only the shell; the docs now say so.
…r One A 401 refresh now carries the key it refused. commandToken invalidates its cache only when the refused token is still the cached one, so a delayed 401 on an older token reuses the token minted since (or joins the run minting it) instead of discarding it and running the command again.
The length floor that keeps short stdout lines and command words out of the redaction list also dropped the cached token, so a token under eight characters echoed by a failed refresh reached the error message. The cached token is a known credential and is now always listed.
A failed token command's stderr excerpt listed only stdout lines and command words of eight or more characters as secrets. A shorter token printed to stdout before a non-zero exit, or a short inline secret the command quoted back (for example --pw=abc12), matched none of the shape recognizers and reached DecisionError.message. A successful run accepts a token of any length, so every non-empty stdout line and every command word is now listed; a short word may garble the excerpt instead of leaking.
commandToken spawns its command through a shell. When the command the shell forked is killed (an OOM kill, an operator, a crash) and the shell survives, the shell exits 128 + N, so Node reports a numeric code with no signal and the run was classified as unauthorized, opening a breaker until reset. Off Windows an exit status of 129-192 is now a network failure that leaves the breaker closed; 126/127 and other codes still read as a refusal. The docs note that a pipeline such as `cmd | tail -1` hides a killed cmd behind a clean exit with no output.
A call refreshed token A to B; while its B request was in flight another call had B refused, minted C, and succeeded. The delayed 401 on B was then fatal because a call refreshed at most once, so withBreaker opened with a valid C cached. After a second 401 the call now asks the minter once more with refresh: false and re-sends only a key it has not had refused; when the minter has nothing newer (commandToken returns the cached B without running the command) the 401 stands. A call sends at most three keys after 401s, and never re-sends one that was refused.
…nd Breaker Reset Process cleanup: a killed run now polls its process group (unref'd, every 25 ms) and drops the SIGKILL escalation once the group is empty, so the escalation never holds the host open for killGraceMs or signals an emptied pgid. runAlive no longer checks `instanceof Error`, which failed for the ESRCH thrown in Jest's vm realm and kept every escalation armed (the "worker failed to exit gracefully" warning). Runs still going when the host exits are sent SIGKILL from one process 'exit' hook. Run end: a run settles when its shell exits; a process the command left holding stdout or stderr gets 100 ms, then the run settles from what it printed instead of timing out and killing that process. Classification: refusalExitCodes lists the exit statuses that mean a refusal; any other non-zero status is network, so a token CLI's transient failure cannot open the breaker. The default keeps today's behaviour. Breaker reset: commandToken takes the model's breaker; a call made after breaker.reset() starts a fresh run instead of joining one from before the reset, whose failure would otherwise reopen the breaker.
…rom Throwing A run detached by a breaker reset hands out a token it never caches, and the command's own store can still hold it when a later run fails; that run's stderr now redacts the last eight tokens minted or refused, not only the cached one. A probe handed secrets it cannot read resolves with a fixed detail instead of rejecting. The README example passes the breaker to commandToken as the text recommends.
A run that settled before any post-reset caller arrived was never detached, so its token stayed cached and was served after the reset; a 403 never refreshes, so the breaker reopened on that stale token. The cache now records the breaker generation its run started under and a hit requires the current generation.
…Redaction Cost Redaction matched the command's words, but the program echoes its argv after the shell rewrote it: `--password=abc\$def` reaches the CLI as `abc$def`, which no word of the text matches. When a POSIX command uses a backslash escape or a `$` or backtick expansion outside single quotes (on Windows, `%`, `^`, or `\"`), the failure message now withholds stderr and says so. Words also split at braces and commas, so bash's brace expansion cannot hand the program a piece no word matches. redact kept one span tuple per match and sorted them all, and the token command lists every stdout line, duplicates included: a failing command printing 20,000 ` }` lines aborted the host with a fatal out-of-memory error while formatting its error. Secrets are now deduplicated, each secret's overlapping matches merge into runs as they are found, and coverage is a difference array sized by the text. The stderr excerpt reads only the first 4,096 characters (whitespace collapsed first, in the secrets too), and when stderr runs past that window the tail is redacted back as far as the longest secret reaches and through any token-shaped run the cut splits.
commandSecrets split the command at whitespace, quotes, =, braces and commas, but a POSIX shell (and cmd.exe) also ends an unquoted word at ; & | ( ) < >, so --pw=abc12; listed only 'abc12;' while the program received abc12 and could echo it into stderr. Split at those operators too; over-splitting quoted text only redacts more.
probe() redacted a host model's whole DecisionError message before keeping 200 characters, so an oversized message cost memory in proportion to it, and the backtracking JWT pattern made redact() quadratic on runs such as repeated 'eyJ-' (about 2 minutes for 800 KB, blocking the event loop after decide() settled). probe() now uses redactExcerpt; JWT shapes are measured segment by segment and the scan resumes past a failed first segment; redactExcerpt collapses at most the first 16384 characters, treating a longer text as cut. Also narrows the commandToken stderr-withholding wording to arguments built from text the command does not show; pathname and tilde expansion substitute existing paths and are not withheld.
e79820a to
93933aa
Compare
What this unblocks
codegraph is a separate code-intelligence service. It is moving its System One decision client off its own copy and onto this SDK's
./decisionssubpath (#623). Its copy has four things the SDK lacks, and it can't switch until the SDK has them:LibreChat backends get the same pieces through the light subpaths without loading LangChain.
What is missing today
DecisionCredentialalready accepts({ refresh, signal }) => Promise<string>, and the transport already asks for a fresh token once after a 401. That matches codegraph's call shape. This PR adds one optional field to the refresh call:refused, the key the 401 refused, so a minter that caches can tell a stale refusal from a current one. But the SDK has no minter for the common case, which is a CLI that prints an hourly bearer token (for exampledataplanectl chai auth tokenfor the ClickHouse gateway).quota, from 🎯 fix: Accept Two-Decimal Rounding and Quota Failures in System One Decisions #624), every later call is refused the same way. Without a breaker, each one still pays for an HTTP round trip and a token mint.decide()in its own try/catch and invent a status shape.What changes
withBreaker(model, breaker?)andcreateBreaker()(src/decisions/breaker.ts). The firstunauthorizedorquotafailure opens the breaker. Every laterdecide()then rejects at once, without a request, with the samefailureandstatusandcircuitOpen: true, untilbreaker.reset()is called.reset()does not reopen it, so an in-flight 402 that arrives after the operator tops up the quota is dropped.DecisionErrorgains acircuitOpenflag.probe(model, { timeoutMs?, signal?, secrets? })(src/decisions/probe.ts). Asks one tiny boolean question. It resolves{ ok: true, model }with the model that answered, or{ ok: false, failure, status?, circuitOpen, detail }. Thedetailis the redacted start of the failure message with whitespace collapsed, capped at 200 characters; only the first 16,384 characters are read, so a host model's oversized error message cannot make the probe slow or large. A non-SDK exception reads asnetwork, and its message is dropped. So does a message the probe cannot redact because thesecretsit was handed cannot be read (a JavaScript host passingnull): the probe still resolves, with a fixed detail. ThroughwithBreaker, a fatal probe trips the breaker, and an open breaker answers without a request.redact(text, secrets?)(src/decisions/redact.ts). Removes these from text:Bearervalues,Listed secrets and shaped credentials are all matched against the original text, and the union of their matches is replaced in one pass, so no recognizer runs first. A listed secret that overlaps a
Bearerkeyword, a JWT segment, or part of a long token therefore cannot hide that credential from the shape checks. Secrets are deduplicated and coverage is kept as one counter per character, so memory stays proportional to the text however often a secret repeats in it. JWT shapes are measured segment by segment in one pass instead of with a backtracking pattern, so no text makes a recognizer quadratic in its length.commandToken(command, { ttlMs, timeoutMs, killGraceMs, refusalExitCodes, breaker })on a new Node-only subpath,@librechat/agents/decisions/node.ttlMs(default 50 minutes, under the gateway's hourly token), counted from when the command finished. Concurrent callers share one run of the command. Arefreshfrom the transport stops serving the cached token and runs the command again, unless the token the 401refusedhas already been replaced; then it reuses the newer token (or joins the run minting it), so a burst of delayed 401s on an old token mints once. The refused token is still redacted, whatever its length, if that refresh fails.breaker, a call made afterbreaker.reset()never joins a run that started before the reset. Without this, a run started before the operator logged in could fail after the reset and reopen the breaker on stale evidence. That run's own callers keep it, and its token is not cached. Nor is a token cached before the reset served after it, even one whose run settled before anyone arrived to detach it: the cache records the breaker generation its run started under, so a 403-refused token (a 403 never refreshes) cannot reopen the breaker after the operator logs in as the right account.cmd &) gets 100 ms to let go. The run then settles from what it printed, and that process is left running instead of the run timing out and killing it.aborted. The run is killed only when every caller sharing it has aborted, and the next call starts a fresh run. A kill sendsSIGTERM, thenSIGKILLoncekillGraceMs(default two seconds) passes with the run still alive, so a command that ignoresSIGTERMcannot outlive its run. A killed run polls its process group and drops the escalation once nothing is left in it, so the escalation never holds the host open for the grace period or signals an emptied group. Off Windows the command runs in its own process group, so a kill reaches the command the shell forked (dash does notexecit), not only the shell; on Windows it ends only the shell. The command reads no stdin.SIGKILL. A host killed by a signal it does not handle never reaches itsexitevent and leaves such a run going.unauthorized, except that off Windows an exit status of 129–192 (how a surviving shell reports a forked command killed by a signal, 137 forSIGKILL) isnetwork;refusalExitCodes, only a listed exit status isunauthorizedand any other isnetwork, which the transport retries and which never trips the breaker, so a CLI that exits 7 when its auth server is unreachable cannot open it until a manual reset. Without the option, every non-zero status outside 129–192 is a refusal, as before;timeoutMsistimeout;network.=, braces, commas and the shell operators;,&,|,(,),<and>, which end an unquoted word without whitespace), whatever their length; a short word may garble the excerpt instead. The excerpt reads only the first 4,096 characters of stderr (whitespace collapsed), redacting the tail a cut could split, so formatting a failure stays cheap however much the command printed. When the shell may hand the program an argument built from text the command does not show, such as a variable, command output or a decoded escape (off Windows, a backslash escape or a$or backtick expansion outside single quotes; on Windows,%,^or\"), no word can match what the program echoes, so stderr is withheld and the message says so. Pathname (*,?,[) and tilde expansion do not withhold it: they substitute paths that already exist, which can appear in the excerpt. The recent tokens matter beyond the cached one because a run detached by a breaker reset hands out a token it never caches, and the command's own store may still hold that token when a later run fails. Command words matter because bash as/bin/shechoes the offending command line into stderr on a syntax error, and a CLI may quote a bad argument back../decisionsstays platform-neutral.commandTokenneedsnode:child_process, so it lives on its own subpath, and the root entry re-exports it.config/light-entries.mjsnow also checks that./decisionsand./rerankreach no Node built-in../decisions/nodeis allowedchild_processonly.How a call flows:
Measured
These are loaded from the built package in a fresh Node process, through its own exports map, in both ESM and CJS:
./decisions./decisions/nodechild_processHow it was tested
npx jest src/decisions src/rerankat the current head: 8 suites, 196 tests pass, with no "worker process has failed to exit gracefully" warning across three runs. An earlier head ofnode.test.tsalso passed while four busy-loop processes loaded the CPU.breaker.test.tscovers:refresh: false), then the breaker opens when every key is refused;maxRetriestimes and leave it closed;reset()and then gets a 402 leaves the breaker closed, and the next call goes out;networkfor a foreign exception, and a redacted detail, and still resolves with a fixed detail when itssecretscannot be read; a host model's 800 KB failure message yields a 200-character detail in under 500 ms (136 s before the probe read only the start);redacthandles each pattern.redact.test.ts: 200 copies of}against 1 MiB of}redact to one marker (the previousredactaborts with a fatal heap error); the excerpt collapses whitespace in secrets too, stays under 2 s against 600 distinct secrets matching 900 KB everywhere (40 s without the window), and hides the first characters of a listed secret or an unlisted token the window cuts off (both leak with the tail cover removed).redacton 800 KB ofeyJ-returns in under 500 ms (120 s with the backtracking JWT pattern), and matches that pattern on 5,000 generated texts. The excerpt reads 10 Ma\tpairs in under 100 ms (870 ms collapsing the whole text) and hides a listed secret the raw prefix cuts through (x hunleaked with the raw-cut check removed).node.test.tsruns real shell commands (a counting Node script) and covers:Date.now, so a slow spawn cannot expire a fresh token);unauthorizedwith a redacted stderr;timeoutMsistimeout, sends no HTTP request and leaves awithBreakermodel closed;network;--key=sk_short123) is redacted;decide()retries with the newer one: two runs, breaker closed;--pw=abc12) the command quotes back;SIGKILL(exit 137) isnetworkand leaves awithBreakermodel closed, whileexit 3staysunauthorized;nodecommand timed out exits within a second, not afterkillGraceMs(5 s);process.exit()with a mint pending takes the command down with it;sleep 3 & echo tok-1resolvestok-1well beforetimeoutMsand leaves thesleeprunning, andsleep 3 & exit 3isunauthorized, nottimeout;refusalExitCodes: [1, 2], exit 7 isnetwork, the breaker stays closed and the next call succeeds, while exit 2 isunauthorized; invalid lists are rejected at construction;breaker.reset()while a pre-reset run is still failing starts a fresh run, succeeds, and the breaker stays closed;$HOME,$(...)and a backtick substitution (each leaked or mismatched with the check removed:abc$def,/home/...,abcd), while$and\inside single quotes keep the excerpt; a bash brace expansion{ab12,cd}redactsab12(leaked with the old word split); an argument ended without whitespace by;,&&,||,|,>,<or a closing)redactsabc12(each leaked whole with the old word split);}stdout lines and 3,000 JSON stderr lines, or 600 distinct stdout lines matching 900 KB of stderr, formats its failure promptly (the previous code aborted on the heap limit; without the excerpt window the second took 39 s).secretstests were each run with only their fix removed and failed (the stderr keptAb3dE6; the probe rejected withTypeError: secrets is not iterable), then passed with it restored.ESRCHcheck, exit hook, drain, refusal list, reset detach); all six also fail against the previousnode.ts.breaker.tsandnode.tsrestored, the stale-reset, generation, timeout, overflow and command-echo tests fail (5 failed, 22 passed). With the fix restored, all pass.npx tsc --noEmitpasses.npm run buildpasses.npm run test:light-entriespasses (16 of 16).Stack
This PR is 3 of 4. Together they let codegraph depend on the SDK's decisions and rerank modules:
./decisionsand./reranksubpaths (basemain).quotafailure. This PR's base.Merge #623, then #624, then this PR.