Skip to content

🔌 feat: Add Decision Credentials, a Breaker, and a Readiness Probe - #625

Merged
danny-avila merged 18 commits into
mainfrom
feat/decision-credentials-breaker
Oct 22, 2026
Merged

danny-avila merged 18 commits into
mainfrom
feat/decision-credentials-breaker

Conversation

@danny-avila

@danny-avila danny-avila commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

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 ./decisions subpath (#623). Its copy has four things the SDK lacks, and it can't switch until the SDK has them:

  • a credential minted by a shell command,
  • a breaker that stops calling once the credential is refused or out of credits,
  • a readiness probe that never throws,
  • a redaction helper for log lines and status rows.

LibreChat backends get the same pieces through the light subpaths without loading LangChain.

What is missing today

  • Short-lived tokens. DecisionCredential already 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 example dataplanectl chai auth token for the ClickHouse gateway).
  • Fatal failures. After a 401 that survives a refresh (and a recheck for a token minted since), or a 402 (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.
  • Readiness checks. A daemon has to wrap decide() in its own try/catch and invent a status shape.
  • Errors in log lines. Error text bound for a log can carry a bearer value or a token.

What changes

withBreaker(model, breaker?) and createBreaker() (src/decisions/breaker.ts). The first unauthorized or quota failure opens the breaker. Every later decide() then rejects at once, without a request, with the same failure and status and circuitOpen: true, until breaker.reset() is called.

  • Rate limits, server errors, timeouts and network failures never trip it. The transport retries those.
  • The breaker counts resets. A fatal failure from a call that started before a reset() does not reopen it, so an in-flight 402 that arrives after the operator tops up the quota is dropped.
  • Models that share a credential can share one breaker.
  • DecisionError gains a circuitOpen flag.

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 }. The detail is 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 as network, and its message is dropped. So does a message the probe cannot redact because the secrets it was handed cannot be read (a JavaScript host passing null): the probe still resolves, with a fixed detail. Through withBreaker, 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:

  • each listed secret,
  • Bearer values,
  • JWTs,
  • runs of 40 or more opaque characters.

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 Bearer keyword, 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.

  • The last non-empty stdout line is the token.
  • The token is reused for ttlMs (default 50 minutes, under the gateway's hourly token), counted from when the command finished. Concurrent callers share one run of the command. A refresh from the transport stops serving the cached token and runs the command again, unless the token the 401 refused has 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.
  • Given the model's breaker, a call made after breaker.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.
  • A run ends when its shell exits. A process the command leaves behind holding stdout or stderr (a lazily started agent, 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.
  • A caller whose signal aborts, including at the decision deadline, rejects at once with aborted. The run is killed only when every caller sharing it has aborted, and the next call starts a fresh run. A kill sends SIGTERM, then SIGKILL once killGraceMs (default two seconds) passes with the run still alive, so a command that ignores SIGTERM cannot 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 not exec it), not only the shell; on Windows it ends only the shell. The command reads no stdin.
  • Off Windows the command runs in its own session with no controlling terminal, so a signal sent to the host's process group (a terminal's Ctrl-C) does not reach it. When the host process exits, each run still going is sent SIGKILL. A host killed by a signal it does not handle never reaches its exit event and leaves such a run going.
  • Failures are classified so that only a real refusal trips a breaker:
    • a non-zero exit or empty output is unauthorized, except that off Windows an exit status of 129–192 (how a surviving shell reports a forked command killed by a signal, 137 for SIGKILL) is network;
    • with refusalExitCodes, only a listed exit status is unauthorized and any other is network, 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;
    • a run killed at timeoutMs is timeout;
    • a spawn error (including Node's synchronous one, whose message quotes the command), output overflow or outside signal is network.
  • The error message names the exit status and gives a redacted stderr excerpt. It never contains a line of stdout, any of the last eight tokens the credential minted or was told were refused, the command, or any word of the command (split at whitespace, quotes, =, 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/sh echoes the offending command line into stderr on a syntax error, and a CLI may quote a bad argument back.

./decisions stays platform-neutral. commandToken needs node:child_process, so it lives on its own subpath, and the root entry re-exports it. config/light-entries.mjs now also checks that ./decisions and ./rerank reach no Node built-in. ./decisions/node is allowed child_process only.

import { commandToken } from '@librechat/agents/decisions/node';
import { createBreaker, createDecisionModel, decisionPreset, probe, withBreaker } from '@librechat/agents/decisions';

const breaker = createBreaker();
const model = withBreaker(
  createDecisionModel(
    decisionPreset('clickhouse')!,
    commandToken('dataplanectl chai auth token', { breaker })
  ),
  breaker
);
const ready = await probe(model, { timeoutMs: 15_000 }); // never throws

How a call flows:

withBreaker.decide(request)
├─ breaker open?  → reject { failure, status, circuitOpen: true }, no mint, no HTTP
├─ generation = breaker.generation
└─ model.decide(request)
   └─ transport.send
      ├─ resolveKey(refresh=false) → commandToken: cached, or one shared run of the command
      ├─ POST → 401 → resolveKey(refresh=true, refused=key) → newer cached token, or run the command again → POST once more
      ├─ POST → 401 again → resolveKey(refresh=false) → a key not yet refused in this call? POST it once; else the 401 stands
      └─ fatal failure (unauthorized | quota) → breaker.trip(error, generation)
                                               (dropped if reset() happened since)

Measured

These are loaded from the built package in a fresh Node process, through its own exports map, in both ESM and CJS:

Subpath Modules Third-party packages Node built-ins
./decisions 11 0 0
./decisions/node 3 0 child_process

How it was tested

  • npx jest src/decisions src/rerank at the current head: 8 suites, 196 tests pass, with no "worker process has failed to exit gracefully" warning across three runs. An earlier head of node.test.ts also passed while four busy-loop processes loaded the CPU.
  • breaker.test.ts covers:
    • a 401 with a string key opens the breaker, and later calls make no request;
    • a credential function is refreshed once and rechecked once (refresh: false), then the breaker opens when every key is refused;
    • when the recheck has nothing newer, the call opens the breaker without re-sending the refused key;
    • a 402 opens it after exactly one request with no backoff;
    • a 429 and a 500 retry maxRetries times and leave it closed;
    • an explicit reset lets calls through again;
    • a breaker can be shared between models;
    • the first error is kept;
    • a call that started before reset() and then gets a 402 leaves the breaker closed, and the next call goes out;
    • the probe returns a request shape, a typed failure, network for a foreign exception, and a redacted detail, and still resolves with a fixed detail when its secrets cannot 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);
    • redact handles each pattern.
  • redact.test.ts: 200 copies of } against 1 MiB of } redact to one marker (the previous redact aborts 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). redact on 800 KB of eyJ- 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 M a\t pairs in under 100 ms (870 ms collapsing the whole text) and hides a listed secret the raw prefix cuts through (x hun leaked with the raw-cut check removed).
  • node.test.ts runs real shell commands (a counting Node script) and covers:
    • the token is the last stdout line and is cached;
    • concurrent callers share a single run;
    • a refresh re-mints, and ttl expiry re-mints (driven by a skewed Date.now, so a slow spawn cannot expire a fresh token);
    • a non-zero exit is unauthorized with a redacted stderr;
    • a failed run is not cached;
    • empty output is rejected;
    • a run killed at timeoutMs is timeout, sends no HTTP request and leaves a withBreaker model closed;
    • 2 MB of output is network;
    • stderr that echoes the command (--key=sk_short123) is redacted;
    • construction validates its options;
    • end to end through the transport: re-mint after a 401, then the breaker opens on a second 401;
    • a refresh that refused an older token gets the token minted since, and a delayed 401 on the old token in a concurrent decide() retries with the newer one: two runs, breaker closed;
    • a cached token shorter than eight characters is redacted when its refresh fails;
    • a stdout token shorter than eight characters is redacted when its first run fails, and so is a short inline secret (--pw=abc12) the command quotes back;
    • a forked command the shell saw killed by SIGKILL (exit 137) is network and leaves a withBreaker model closed, while exit 3 stays unauthorized;
    • a refreshed token refused late, after another call replaced it and succeeded, is rechecked and the call succeeds with the newer token: three runs, breaker closed;
    • a host (a separate Node process) whose mint of a forked node command timed out exits within a second, not after killGraceMs (5 s);
    • inside Jest's vm context, a killed run's escalation timer is cleared once its group is gone;
    • a host that calls process.exit() with a mint pending takes the command down with it;
    • sleep 3 & echo tok-1 resolves tok-1 well before timeoutMs and leaves the sleep running, and sleep 3 & exit 3 is unauthorized, not timeout;
    • with refusalExitCodes: [1, 2], exit 7 is network, the breaker stays closed and the next call succeeds, while exit 2 is unauthorized; invalid lists are rejected at construction;
    • a call made after breaker.reset() while a pre-reset run is still failing starts a fresh run, succeeds, and the breaker stays closed;
    • a short token minted by a run detached by a breaker reset, never cached, is redacted when a later run's stderr repeats it;
    • stderr is withheld after a backslash escape, an escaped space, an escape inside double quotes, $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} redacts ab12 (leaked with the old word split); an argument ended without whitespace by ;, &&, ||, |, >, < or a closing ) redacts abc12 (each leaked whole with the old word split);
    • a failing command printing 20,000 } 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).
    • after a 403 opens the breaker on a cached token and the operator logs in and resets, the next call mints afresh, succeeds, and the breaker stays closed (2 runs);
    • a run from before a reset that settles with no caller arriving meanwhile does not feed the post-reset cache;
    • both fail with the cache-hit generation check removed (2 failed).
  • The detached-token and unreadable-secrets tests were each run with only their fix removed and failed (the stderr kept Ab3dE6; the probe rejected with TypeError: secrets is not iterable), then passed with it restored.
  • Each of the six tests before the detached-token one was run with only its fix removed and failed (escalation poll, cross-realm ESRCH check, exit hook, drain, refusal list, reset detach); all six also fail against the previous node.ts.
  • Fix removal: with the previous breaker.ts and node.ts restored, the stale-reset, generation, timeout, overflow and command-echo tests fail (5 failed, 22 passed). With the fix restored, all pass.
  • npx tsc --noEmit passes.
  • npm run build passes.
  • npm run test:light-entries passes (16 of 16).
  • eslint and prettier are clean on the touched files.

Stack

This PR is 3 of 4. Together they let codegraph depend on the SDK's decisions and rerank modules:

  1. 🪶 feat: Add Light Decisions and Rerank Subpath Exports #623: light ./decisions and ./rerank subpaths (base main).
  2. 🎯 fix: Accept Two-Decimal Rounding and Quota Failures in System One Decisions #624: wire fidelity: two-decimal rounding bounds and the quota failure. This PR's base.
  3. 🔌 feat: Add Decision Credentials, a Breaker, and a Readiness Probe #625: credential, breaker, probe and redaction hooks. This PR.
  4. 🎚️ feat: Add Never-Throwing Rerank Outcomes With Per-Document System One, Cross-Encoder, BM25, and Fallback Chains #630: rerank parity: per-document System One, cross-encoder, BM25 and a never-throwing fallback chain.

Merge #623, then #624, then this PR.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-22T09:00:00.311010Z 9f5d655 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts Outdated
);
});

return ({ refresh }): Promise<string> => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, mints token-2 on 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.

Comment thread src/decisions/node.ts Outdated
Comment on lines +177 to +179
if (refresh) {
/** The cached token was refused; never hand it out again. */
cached = null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/decisions/node.ts Outdated
Comment on lines +169 to +170
cached = token;
expiresAt = started + ttlMs;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/decisions/redact.ts Outdated
Comment on lines +23 to +24
result = result.split(secret).join(REDACTED);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danny-avila
danny-avila force-pushed the feat/decision-credentials-breaker branch from 7f862be to ed2e58c Compare October 21, 2026 15:34
@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts Outdated
}
try {
if (GROUP_KILL) {
process.kill(-child.pid, KILL_SIGNAL);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danny-avila
danny-avila force-pushed the feat/decision-credentials-breaker branch from fb6f2b5 to 83533ae Compare October 21, 2026 16:13
@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts Outdated
...echoed,
cached ?? '',
...stdout().split(/\r?\n/),
].filter((line) => line.trim().length >= MIN_SECRET_LENGTH);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/decisions/node.ts Outdated
Comment on lines +393 to +395
if (refresh) {
/** The cached token was refused; never hand it out again. */
expiresAt = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 concurrent decide() calls through withBreaker(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.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts Outdated
Comment on lines +299 to +301
...stdout()
.split(/\r?\n/)
.filter((line) => line.trim().length >= MIN_SECRET_LENGTH),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/decisions/transport.ts Outdated
Comment on lines +297 to +301
if (
lastError.status === 401 &&
typeof credential === 'function' &&
!refreshed
refused === undefined &&
typeof key === 'string'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/decisions/node.ts
Comment on lines +84 to +88
if (end.kind === 'exit') {
return {
failure: 'unauthorized',
reason: `token command exited with code ${end.code}`,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts Outdated
Comment on lines +599 to +600
if (pending === null && cached !== null && Date.now() < expiresAt) {
return Promise.resolve(cached);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 on bad-1, the operator logs in and resets, and the next call sends good-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 gets token-2, not token-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.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/redact.ts Outdated
at !== -1;
at = text.indexOf(secret, at + 1)
) {
spans.push([at, at + secret.length]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-secrets contract still throws and is caught.
  • Each secret's overlapping matches are merged into runs as they are found.
  • Coverage is an Int32Array difference 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 of as). So the token command's excerpt now goes through redactExcerpt, 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) and node.test.ts "formats the failure promptly when stdout repeats short lines that stderr is full of" (the 20,000-line case). Both abort with FATAL ERROR: ... JavaScript heap out of memory on the previous redact.ts.
  • "formats the failure promptly against many distinct stdout lines that stderr matches everywhere" and the redactExcerpt bound test took 39 s and 40 s with the window removed, and fail their time bound.
  • The two cut-off tests leak hunte and a run of Bs with the tail cover removed.
  • "redacts a listed secret whose whitespace the excerpt collapses" fails without collapsing the secrets.

Comment thread src/decisions/node.ts Outdated
Comment on lines +248 to +249
return [command, ...command.split(/[\s'"`=]+/)].filter(
(word) => word.trim() !== ''

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 %s covers --pw=abc\$def, --pw=abc\ def, "--pw=abc\$def", "--pw=$HOME", --pw=$(printf ab)cd and a backtick substitution. With rewritesWords forced to false, all 6 fail, and the received messages included bad password abc$def, bad password /home/danny and bad password abcd.
  • redacts each piece of a word that brace expansion splits fails with the old word split (received bad password ab12).
  • keeps the excerpt when $ and \ appear only inside single quotes guards against over-withholding.

JSDoc, README and the PR body now describe the withholding rule.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts
Comment on lines +249 to +252
function commandSecrets(command: string): string[] {
return [command, ...command.split(/[\s'"`=,{}]+/)].filter(
(word) => word.trim() !== ''
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/decisions/node.ts
Comment on lines +273 to +275
for (const [segment] of command.matchAll(SHELL_SEGMENT)) {
if (!SINGLE_QUOTED.test(segment) && /[\\$`]/.test(segment)) {
return true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/decisions/probe.ts Outdated
Comment on lines +86 to +87
try {
return redact(message, secrets).slice(0, DETAIL_LIMIT);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e79820a, along with the rest of this class.

  • Probe: safeDetail now calls redactExcerpt(message, secrets, 200), so only the start of the message is read. An unreadable secrets still 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 repeated eyJ-, and it blocked the event loop after decide settled, where the probe's timeout cannot reach. jwtSpans now 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 gives token [REDACTED] was refused [REDACTED] in under 500 ms. It took 136 s against the previous probe.ts/redact.ts.
  • redact.test.ts "finds JWT shapes in time linear in the text": 800 KB of eyJ- 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" leaked x hun with the raw-cut check removed.

probe.ts was the only full-text redact( call in src. node.ts already used redactExcerpt.

@danny-avila
danny-avila force-pushed the fix/decision-wire-fidelity branch from eac8f5f to 40620d3 Compare October 22, 2026 09:54
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.
@danny-avila
danny-avila force-pushed the feat/decision-credentials-breaker branch from e79820a to 93933aa Compare October 22, 2026 09:59
@danny-avila
danny-avila changed the base branch from fix/decision-wire-fidelity to main October 22, 2026 09:59
@danny-avila
danny-avila marked this pull request as ready for review October 22, 2026 10:03
@danny-avila
danny-avila merged commit 16d8327 into main Oct 22, 2026
13 checks passed
@lia-by-librechat lia-by-librechat Bot mentioned this pull request Oct 22, 2026
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