fix(broker): report the real login failure, not aeval's artifacts banner

[guohai]

Found while diagnosing job 31072 in production.

What happened

With the BROKER_ADVERTISE_URL fix in place, Core reached the broker and a mint finally ran end-to-end. It failed, and this is what Core recorded:

[SessionBroker] Mint failed for platform agora: broker mint failed: 502
[Broker] Mint failed for platform agora: aeval exited 1: 2026-08-30 17:49:51.109 | INFO | Artifacts saved to: output/mint/20260830_174839_2119

A directory path. The actual cause was two lines earlier in the same stream, and only visible by exec-ing into the container:

ERROR | Error waiting for URL pattern: https://conversational-ai.agora.io/, current URL: https://sso2.agora.io/en/login?...
ERROR | Step 1 failed: platform.setup - Timeout 60000ms exceeded.

i.e. the login was rejected and the browser never left the SSO page.

Cause

mintWithAeval took stderr.trim().split("\n").pop(). aeval's last line is an INFO banner printed after the diagnosis, so the one line kept was structurally guaranteed to be the least useful one — the same defect fixed for the daemon in #128, here in the one path where a credential is the thing under test.

Fix

• Reuse summarizeAevalFailure (prefers loguru ERROR lines, takes the last ones).
• Keep scrubCredentials after it: the summarizer's redaction has a 4-char floor, scrubCredentials has none, so a very short password still gets redacted.
• Add the broker stage's missing COPY vox_eval_agentd/aeval-output.ts — the module was daemon-only. Verified with a real docker build --target broker, not just tsc; this is the exact missing-COPY class of bug that broke the agentd image in fix: surface the real cause of a missing-secret run instead of a PyInstaller banner #128.

Tests

Two new cases in tests/auth-session-broker-service.test.ts pinned to the verbatim stderr tail from the real job-31072 failure: the reported message must name the login failure and must not be the artifacts banner, and the credential must not survive into it.

Full local gate green: 1611/1611 across 88 files. (Getting there required clearing ~244 leaked test workflows from the dev DB — the suite creates workflows without cleanup and trips the 200-workflow principal cap after ~22 runs. Filed separately.)

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

No findings.

The PR addresses the broker’s misleading error selection without introducing obvious security, logic, performance, or maintainability regressions. The additional broker COPY looks necessary for the new import, and keeping scrubCredentials after summarizeAevalFailure preserves defense-in-depth for the response/log path.

I did not run the test suite because this environment is read-only.

[github-actions]

🤖 Claude Code Review

Reviewed the merge ref (d1053c6) against main.

The core fix is right: summarizeAevalFailure already existed and is battle-tested for the daemon path (vox-agentd.ts:870), so reusing it in the broker rather than re-implementing is the correct call, and the Dockerfile COPY of aeval-output.ts is required for the esbuild bundle to resolve ./aeval-output (the dev-local-run.sh:579 bundle check and docker.yml:220 node --check both cover this). The test fixture is a real captured failure, which is the right kind of regression test.

Credential scrub is narrower here than in the daemon — vox_eval_agentd/auth-session-broker.ts:115

The broker embeds the password into the scenario YAML as JSON.stringify(req.password) (line 74), i.e. backslash/quote-escaped — but scrubs only the raw value. The daemon deliberately passes both forms for exactly this reason:

// vox-agentd.ts:1888
this.activeSecretValues = Object.values(jobSecrets).flatMap((v) => [v, yamlEscape(v)]);

A password like pa"ss\word appears in aeval's output as pa\"ss\\word, which neither summarizeAevalFailure's scrub nor the follow-up scrubCredentials will match. The resulting string is logged (line 161) and returned in the 502 body to Core, where it lands in a user-visible job error. The gap pre-dates this PR, but the PR widens the blast radius: the old code kept one line, this keeps up to 500 chars of ERROR lines — precisely the lines most likely to echo step params. Suggested fix at both call sites:

const esc = (v: string) => v.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
const needles = [req.email, req.password, esc(req.email), esc(req.password)];

(JSON.stringify also escapes \n/\t/control chars, so mirroring the daemon's yamlEscape fully would be more precise than the two-replace version above.)

Minor

• summary || 'login failed' (line 116) is dead — summarizeAevalFailure returns 'unknown error' on empty input, never ''. Not a bug, but on a silent aeval exit the operator now sees aeval exited 1: unknown error rather than the more specific login failed. Either drop the fallback or invert it (summary === 'unknown error' ? 'login failed' : summary).
• The helper's docstring advertises that it scans both streams because "loguru's sink may be either," but the broker deliberately discards stdout (line 90) and passes ''. That's the correct security tradeoff, just worth knowing the fallback property doesn't apply here — if a future aeval build routes loguru to stdout, the broker silently regresses to the artifacts-banner tail. A one-line comment noting the '' is intentional would keep the next reader from "fixing" it by wiring stdout through.
• Very marginal: the internal .slice(0, 500) runs before the outer scrubCredentials, so a sub-4-char secret straddling the 500-char cut is truncated to a fragment the outer scrub can't match. Only matters for passwords the summarizer's floor already skips; noting for completeness, not asking for a change.

Nothing blocking beyond the escaped-value scrub, which I'd fix before merge given the message is persisted and user-visible.

[guohai]

Addressed the review in e56a0b3.

Blocking item — escaped-form credential leak. Fixed, and taken a step further than suggested: rather than hand-rolling an escaper that could drift from the emitter, credentialForms() derives the escaped form from the same JSON.stringify that writes the scenario YAML:

export function credentialForms(values: string[]): string[] {
  return Array.from(new Set(
    values.filter((v) => v.length > 0).flatMap((v) => [v, JSON.stringify(v).slice(1, -1)]),
  ));
}

By construction, whatever the scenario emits is what gets redacted — including \n/\t/\uXXXX control-char escapes, which the two-replace version would have missed. Both the summarizer and the follow-up scrubCredentials now take these forms.

New test pins it with a password that actually differs when escaped (pa"ss\word → pa\"ss\\word), plus a guard asserting the fixture is not accidentally escape-invariant.

Minor 1 — dead summary || "login failed". Correct, summarizeAevalFailure returns "unknown error" and never "". Rather than compare against another module's sentinel, the empty case is now branched explicitly:

const summary = stderr.trim() ? scrubCredentials(summarizeAevalFailure("", stderr, forms), forms)
                              : "login failed with no output";

Minor 2 — the "" stdout argument. Added a comment at the call site saying the discard is deliberate and not to wire stdout through, exactly to stop the silent regression you describe if a future aeval routes loguru to stdout.

Minor 3 — .slice(0, 500) running before the outer scrub. Left as-is, as you suggested. Worth noting the fix above shrinks it: the escaped forms are now in the summarizer's own needle list, so they are redacted before the cap. The residue is only a sub-4-char secret straddling the 500-char boundary.

Verified: tsc clean, 20/20 in auth-session-broker-service.test.ts, and a real docker build --target broker with node --check on the bundle. Full gate: audio 29/29, E2E 134/134, unit 1610/1611 — the one failure is in practical-shared-agents-credits.test.ts, which passes 3/3 in isolation and is untouched by this diff.

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

No findings.

Residual risk: I didn’t run the suite in this read-only environment, so I’m relying on static review. The change looks aligned with the described failure mode: broker failures now prefer aeval ERROR lines, credentials are scrubbed again after summarization, and the broker Docker stage includes the newly imported module.

[github-actions]

🤖 Claude Code Review

Reviewed the merge ref (commits d1053c6 + e56a0b3) against vox_eval_agentd/auth-session-broker.ts, vox_eval_agentd/aeval-output.ts, the Dockerfile, and the tests.

The core change is correct. Reusing summarizeAevalFailure fixes a real failure-reporting bug (aeval's trailing INFO | Artifacts saved to: banner was structurally guaranteed to win the old last-line heuristic), and the helper's ERROR-line preference plus .slice(-3) matches the real fixture. The COPY vox_eval_agentd/aeval-output.ts addition is required and sufficient — esbuild --bundle inlines it, and scripts/dev-local-run.sh:579 already bundles the broker the same way, so the local-dev path doesn't drift. Passing '' for stdout is right given stdout is discarded (not buffered) at line 110, and the inline comment saying so will save the next reader a "fix."

One real gap:

vox_eval_agentd/auth-session-broker.ts:190 — the outer defense-in-depth scrub still uses raw values:

const msg = scrubCredentials(raw, [body.email ?? '', body.password ?? '']);

Its own comment promises it holds "even if a future mint implementation forgets to scrub" — but that's exactly the case where the escaped form (pa\"ss\\word) reaches it, and this call can't match it. Since msg is both logged and returned in the 502 body (which Core persists as a user-visible job error), the second commit's fix doesn't cover the layer that exists to be the backstop. Should be credentialForms([body.email ?? '', body.password ?? '']) — a strict superset, no behavior change on the happy path.

Smaller notes (not blocking):

• credentialForms correctly derives the escaped form from the same JSON.stringify that writes the scenario YAML, and the Set dedup makes the no-op case ("plain") clean. Deriving rather than hand-rolling the escaper is the right call.
• summarizeAevalFailure scrubs before .slice(0, 500), so long credentials can't survive truncation; only a sub-4-char value straddling the boundary could leave a fragment, and the outer scrubCredentials (no length floor) closes that on the mint path. Fine as-is.
• The stderr.trim() ? … : 'login failed with no output' branch is cosmetic — the helper already returns 'unknown error' for empty input — but the broker-specific wording is more actionable for an operator, so keep it.
• Tests are well-targeted: the verbatim job-31072 fixture pins the actual regression, and the expect(escaped).not.toBe(password) guard keeps the escaping test from silently degrading into a tautology. I could not execute the suite here (command approval declined), so the assertions are reviewed statically only.

[guohai]

Good catch — fixed in 5c6a168.

You are right that the backstop was the wrong layer to leave on raw values: it exists for "a future mint implementation that forgets to scrub," and a mint that forgot to scrub is precisely the path on which the escaped form arrives. Raw-only made it blind to the one case it was there for.

const msg = scrubCredentials(raw, credentialForms([body.email ?? , body.password ?? ]));

Strict superset, no happy-path change, and the comment now says why it uses the forms rather than the pair.

Added an HTTP-level test that drives /mint with a mint throwing an unscrubbed escaped password and asserts the 502 body contains neither form. I confirmed it is not tautological — reverting just that one line makes it fail, restoring it makes it pass:

FAIL  the defense-in-depth scrub catches the ESCAPED credential a forgetful mint would leak   (raw-only)
PASS  (credentialForms)

tsc clean, 21/21 in the broker suite.

On your three non-blocking notes: agreed on all, and no further change made — the .slice(0, 500) residue is now only a sub-4-char value straddling the boundary, and the login failed with no output wording stays for operator legibility.

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

Findings

• vox_eval_agentd/auth-session-broker.ts:48 — Credential scrubbing is order-dependent, so overlapping credentials can still leak. credentialForms([email, password]) preserves email before password; if a password contains the email/user value, e.g. password = "brent@agora.op-2026!", scrubCredentials redacts the email first and the full password no longer matches, leaving -2026! in logs and the 502 response. This matters most for the defense-in-depth path at vox_eval_agentd/auth-session-broker.ts:193, where a future/alternate mint may return an unsanitized error directly. Sort redaction needles longest-first, as summarizeAevalFailure already does, before reducing.

Notes

• The main fix looks correct: using summarizeAevalFailure avoids the trailing artifacts banner and the broker Docker target now copies the needed module.
• I didn’t see other security, logic, performance, or maintainability issues in the changed files.

[github-actions]

🤖 Claude Code Review

The fix is well-targeted and the propagation chain checks out: broker 502 body → mintViaBroker throw → web_sessions.last_error → returned to the eval agent at server/routes.ts:3939. That path is agent-visible (including a shared-tier marketplace agent), so scrubbing before the message escapes mintWithAeval is the right layer, and doing it again in createBrokerServer is correct. credentialForms deriving the escaped form from the same JSON.stringify that writes the scenario YAML (auth-session-broker.ts:71 vs. :93-94) is the right call — it can't drift. The yamlEscape/activeSecretValues parallel cited in the comment is accurate (vox-agentd.ts:551, :1888), and the Dockerfile now copies aeval-output.ts into the broker stage, which still keeps the bundle builtins-only.

The fix is conditional on aeval logging to stderr, and the broker only buffers stderr (auth-session-broker.ts:111-112, :144-146). summarizeAevalFailure's own contract says it "Scans BOTH streams — loguru's sink may be either," and the daemon honors that (vox-agentd.ts:870 passes both). If aeval routes loguru to stdout — different version, --log-file/LOGURU_* config, or a TTY-vs-pipe difference — the broker falls into the stderr.trim() false branch and reports login failed with no output, which is the same class of uninformative error this PR exists to remove, just a different string.

The stated reason for not wiring stdout through ("may echo step params verbatim") doesn't hold up asymmetrically: stderr echoes step params too — the second new test (bad params: password="${escaped}") is precisely that case on stderr, and it's handled by passing forms into the summarizer plus the outer scrubCredentials. The same two layers would cover stdout. Suggest buffering stdout into a variable (still never console.log-ing it) and passing it as the first argument, with the guard becoming (stdout + stderr).trim(). If you'd rather not, the comment should say the broker has verified aeval's loguru sink is stderr, rather than implying stdout carries a hazard stderr doesn't.

Minor, non-blocking:

• scrubCredentials reduces in array order while summarizeAevalFailure deliberately sorts needles longest-first. With credentialForms output the escaped form is always ≥ the raw form, so raw-first can in principle chew a prefix of an escaped occurrence. I worked through the cases and the residue is cosmetic (a stray backslash) rather than a leak, since escaping only inserts backslashes — but a .sort((a, b) => b.length - a.length) in credentialForms would make that a property instead of a coincidence, and would cost nothing.
• summarizeAevalFailure slices to 500 chars before the outer scrubCredentials sees the string, so a sub-4-char credential straddling the cut can't be matched by either pass. Genuinely negligible; noting only for completeness.

The tests are good — the real-job stderr fixture with the trailing INFO banner is the right regression, and both escaped-form tests include the expect(escaped).not.toBe(password) guard so they can't silently degrade into tautologies.

[github-actions]

🤖 Codex Code Review

Findings

• Medium: vox_eval_agentd/auth-session-broker.ts:124 now buffers all of stdout for the full mint run with no cap. Since aeval/browser output is external to this service and the timeout can be minutes, a noisy or stuck run can grow broker memory until OOM; this is especially risky because /mint is a long-running authenticated endpoint. The failure summarizer only needs a small tail plus ERROR lines, so consider a bounded/ring buffer per stream, or cap retained output while preserving recent ERROR/CRITICAL lines.

Notes

• I didn’t see a credential-exposure regression in the changed reporting path; credentialForms plus the second scrubCredentials pass addresses the escaped-password case.
• I did not run tests because this environment is read-only.

[github-actions]

🤖 Claude Code Review

The change is well-reasoned and the fix is correct: summarizeAevalFailure already existed and was already used by the daemon for exactly this failure mode, so reusing it here is the right call rather than a second heuristic. The credentialForms derivation from the same JSON.stringify that writes the YAML is a genuinely good design choice — it can't drift from the scenario writer the way vox-agentd.ts's hand-rolled yamlEscape can. The longest-first sort in scrubCredentials fixes a real substring-shredding leak, and the Dockerfile COPY of aeval-output.ts into the broker stage is required for the esbuild bundle to resolve the new import.

Findings, in order of significance:

1. stdout is now a leak surface in a persisted, user-visible string (auth-session-broker.ts:126, :154)

The removed line said "stdout intentionally discarded — may echo step params. Never log it." Now stdout is concatenated into the message that is logged, returned in the 502 body, and persisted by Core as the job's error. The scrub covers exactly two encodings — raw and JSON-escaped. Anything else aeval or Playwright might emit on stdout passes through unredacted: a URL-encoded form body (pa%22ss), an HTML dump of the login page, or a Python repr of a dict containing \uXXXX escapes for a non-ASCII password.

The stated justification is hedging against a future aeval that routes loguru to stdout. You can keep that hedge without paying for it in the normal case:

const hasStderrDiagnosis = /\|\s*(ERROR|CRITICAL)\s*\|/.test(stderr);
const summary = hasStderrDiagnosis
  ? scrubCredentials(summarizeAevalFailure('', stderr, forms), forms)
  : scrubCredentials(summarizeAevalFailure(stdout, stderr, forms), forms);

stdout is consulted only when stderr yielded no diagnosis — which is precisely the "loguru moved to stdout" scenario — and is otherwise never in the reported text.

2. Multi-byte split can defeat the scrub (auth-session-broker.ts:126-127)

d.toString() per chunk decodes each Buffer independently. A UTF-8 sequence straddling a pipe read boundary becomes U+FFFD, so a password containing a non-ASCII character will no longer match its needle and can survive scrubbing in fragments. string_decoder's StringDecoder holds the partial sequence across chunks:

const outDec = new StringDecoder('utf8'), errDec = new StringDecoder('utf8');
proc.stdout!.on('data', (d) => { stdout += outDec.write(d); });

Pre-existing for stderr, but this PR doubles the exposure and it's cheap to fix in the same place.

3. Unbounded buffering (auth-session-broker.ts:124-127)

Both streams accumulate without a cap for up to WEB_SESSION_MINT_TIMEOUT_SECONDS (default 180s), in a long-lived sidecar, and Chromium children inherit these pipes. Only ~500 chars are ever consumed. A ring buffer (keep the last ~64KB per stream) would bound it. This matches the existing daemon pattern, so it's consistency rather than a regression — worth doing here since you're already touching these handlers.

4. Minor: truncation ordering vs. the summarizer's 4-char floor

summarizeAevalFailure scrubs and then .slice(0, 500). For a credential shorter than 4 chars the summarizer's floor skips it, the slice can cut it mid-value, and the outer scrubCredentials (no floor) then can't match the surviving fragment. mintWithAeval has no minimum password length, so this is reachable in principle, though a 1–2 character residue is close to harmless. Not worth restructuring for.

Tests — good coverage of the actual regression. The fixture is a real captured failure rather than a synthetic one, the expect(escaped).not.toBe(password) guards keep the escaping tests from silently degenerating into no-ops, and the containment-order test (brent@agora.op inside brent@agora.op-2026!) pins the sort that makes the fix correct. The defense-in-depth HTTP test correctly exercises the escaped form, which is the case that layer actually exists for. Note I was not able to execute the suite in this environment, so this is a read of the tests, not a run of them.

[guohai]

All three addressed in 557da60.

1. Unbounded capture (Codex). Agreed, and it applied to stderr before this PR too — I just doubled the surface. appendBounded caps each stream at 64 KiB, keeps the tail (where the diagnosis is, and what summarizeAevalFailure reads, since it takes the last ERROR lines), and cuts forward to a line boundary so a partial line cannot strand a credential fragment the scrub would miss. Exported rather than left as a closure so the boundary behavior is unit-tested, including the single-line-longer-than-limit case.

2. Multi-byte split defeating the scrub (Claude). This one I would not have caught — thank you. StringDecoder per stream, flushed with .end() before the text is scrubbed, so a trailing partial character is not dropped either.

3. stdout as a leak surface (Claude). Took your suggested shape. The predicate is exported from aeval-output rather than restated in the broker, so the two cannot drift:

const diagnosisSource = hasAevalDiagnosis(stderr) ? "" : stdout;

stdout is consulted only when stderr yielded no diagnosis — exactly the "future aeval moved loguru to stdout" case the hedge exists for — so in the normal case it contributes nothing to the logged/persisted string.

Verification. tsc clean, 24/24 in the broker suite. Rebuilt the image with --target broker and did a runtime import() inside the container, not just node --check, to confirm esbuild keeps string_decoder external under --platform=node:

auth-session-broker.js  9.5kb
module loads OK

Full local gate: 1613/1617 unit, and the 4 failures are agora-e2e moderator-lifecycle, untouched by this diff — that suite hardcodes channel clash-event-99999 and leaks a live ConvoAI agent whenever start fails, so the next run gets 409 TaskConflict. Filed under #134; clearing the orphan makes it green.

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

Findings

• High: vox_eval_agentd/auth-session-broker.ts:98 truncates captured output before redaction, and the no-newline fallback slices mid-line. If aeval emits a single overlong ERROR line that includes password=<credential> or the escaped credential, the retained tail can start inside the credential. summarizeAevalFailure and the final scrubCredentials only redact complete credential forms, so the suffix can survive into the broker log, the 502 response, and Core’s persisted job error. This is especially relevant because the new tests explicitly allow the no-newline truncation path. Consider dropping the entire overlong line when no newline exists after the cut, or redacting before/while bounding with enough overlap to avoid partial-secret leakage.

Testing

• Not run; review only, sandbox is read-only.

[github-actions]

🤖 Claude Code Review

I couldn't run the test suite (command approval denied), so this is a static review.

Overall

The core fix is right and well-targeted: preferring loguru ERROR/CRITICAL lines over the trailing Artifacts saved to: banner directly addresses the reported bug, and the supporting hardening (escaped-form redaction, longest-needle-first ordering, StringDecoder across chunk boundaries, bounded capture) each closes a real gap. The Dockerfile change is correct — --bundle with --platform=node resolves ./aeval-output and auto-externalizes string_decoder, so no runtime node_modules is needed.

Three things worth addressing.

---

1. stdout reaches the persisted error in more cases than the comment claims

vox_eval_agentd/auth-session-broker.ts:196

const diagnosisSource = hasAevalDiagnosis(stderr) ? '' : stdout;

The comment says stdout is consulted "exactly the 'a future aeval moved loguru to stdout' case," but the actual condition is stderr has no diagnosis — a strict superset. When neither stream has an ERROR/CRITICAL line (segfault, PyInstaller bootstrap failure, Chromium crash), summarizeAevalFailure falls through to its tail path over ${stdout}\n${stderr}, and if stderr's meaningful tail is short or empty, raw stdout lines land in the message.

That matters because the pre-change code was explicit that this must never happen (stdout intentionally discarded — may echo step params. Never log it.), and the resulting string is logged, returned in the 502 body, and persisted by Core into web_sessions.last_error (server/storage.ts:2778), then surfaced to the user (server/routes.ts:3939). Email/password are double-scrubbed so those are covered, but anything else aeval writes to stdout — Playwright page dumps, storage-state fragments, cookie values — is not modeled by any needle.

Gating on the condition the comment actually describes keeps the behavior you intended and restores the old guarantee everywhere else:

const diagnosisSource = !hasAevalDiagnosis(stderr) && hasAevalDiagnosis(stdout) ? stdout : '';

2. appendBounded can cut mid-line, which is the case its doc says it prevents

vox_eval_agentd/auth-session-broker.ts:100-106

The docstring states "the retained text never begins mid-line. A partial line could otherwise strand a fragment of a credential that no longer matches its redaction needle." But when indexOf('\n', cut) returns -1 — a single line longer than the 64 KB limit, e.g. a base64 blob or a JSON dump — it falls back to next.slice(cut), which is precisely a mid-line cut. The test at tests/auth-session-broker-service.test.ts ("A single line longer than the limit still gets bounded") asserts this branch, so it's known behavior; the comment just overstates it.

Narrow, but it is a genuine new under-redaction path: before this change the full credential was present and fully scrubbed, now a truncated tail fragment can survive both scrub layers. Either drop the leading partial segment when no newline is found, or soften the comment to say the line-boundary trim is best-effort.

3. Minor: emptiness check allocates both buffers

vox_eval_agentd/auth-session-broker.ts:200

const summary = (diagnosisSource + stderr).trim() ? ...

Concatenates up to 128 KB and trims it purely to test for emptiness, on a path that already has the two strings in hand. diagnosisSource.trim() || stderr.trim() is equivalent and short-circuits.

---

Notes (no action needed)

• The longest-first sort in scrubCredentials operates on the array returned by .filter(), so the caller's array isn't mutated — correct.
• credentialForms deriving the escaped form from the same JSON.stringify that writes the YAML is the right call; a hand-rolled escaper would drift.
• Decoder .end() flush inside the close handler is correctly ordered (stdio streams are closed by then) and correctly skipped on the timeout path, where buffers are discarded anyway.
• Tests cover the pure helpers thoroughly, including a verbatim real-failure fixture. Nothing exercises the close-handler branch in mintWithAeval itself — the stdout/stderr selection and the 'login failed with no output' fallback are untested. Extracting that selection into a small pure function would make finding Improve initialization gating and routing #1 testable and is probably worth it given it's the security-relevant line.

[guohai]

Both fixed in d257cf1.

Codex (High) — mid-line cut can start inside a credential. Correct, and the fix needed more than a smarter slice. Enforcing line alignment requires state: abandoning the overlong line is not enough, because the very next chunk is that line's continuation and would be treated as line-start content, reintroducing the fragment. So appendBounded became createBoundedCapture, which drops the overlong line and keeps dropping until the next newline:

cap.push("x".repeat(200) + "password=SUPERSECRETVALUE"); // → ""
cap.push("STILL_THE_SAME_LINE_SECRET");                  // → "" (continuation)
cap.push("_tail\nERROR | recovered");                    // → "ERROR | recovered"

All three assertions are in the new test.

Claude #1 — the stdout gate was a strict superset of its own comment. Also correct, and the case you named is the dangerous one: when neither stream has a diagnosis, summarizeAevalFailure falls through to a tail over ${stdout}\n${stderr}. Now gated on what the comment actually described:

return !hasAevalDiagnosis(stderr) && hasAevalDiagnosis(stdout) ? stdout : "";

Claude #2 — (diagnosisSource + stderr).trim() allocating up to 128 KB to test emptiness. Dropped, short-circuits now.

Claude closing note — the close-handler branch was untested. Taken. The selection and message pipeline are extracted into pure selectDiagnosisSource / describeMintFailure, so the security-relevant line is now covered directly, including the both-streams-silent case and the login failed with no output fallback that previously had no test at all.

Verification: tsc clean, 26/26 in the broker suite, full local gate 1619/1619 across 88 files, image rebuilt with --target broker and imported at runtime inside the container (10.1 kb bundle, module loads OK).

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

Findings

• P2 Security: stdout can still be persisted on false-positive “diagnosis” matches. hasAevalDiagnosis uses an unanchored /\|\s*(ERROR|CRITICAL)\s*\|/, so any stdout content containing | ERROR | is treated as trusted aeval/loguru output when stderr lacks a diagnosis. That bypasses the intended stdout quarantine in selectDiagnosisSource, and summarizeAevalFailure will return that stdout line, potentially leaking page dumps/cookies/storage fragments the scrubber does not model. Consider anchoring this to the actual loguru prefix/line shape before allowing stdout into the user-visible error. vox_eval_agentd/aeval-output.ts:25, vox_eval_agentd/auth-session-broker.ts:150

• P3 Security: short credentials can be partially leaked when the 500-char summary cap cuts through them before the broker’s floorless scrub runs. summarizeAevalFailure ignores redaction needles shorter than 4 chars and slices the result to 500 chars; only after that does describeMintFailure call scrubCredentials. If an aeval ERROR line contains a 1-3 char password at the truncation boundary, the returned string can include a prefix/suffix that no longer matches the full credential, despite the intended no-floor scrub. Consider applying the broker’s floorless scrub before the 500-char truncation or making the truncation credential-boundary-safe. vox_eval_agentd/aeval-output.ts:73, vox_eval_agentd/auth-session-broker.ts:164

Notes

• I did not run the test suite because this environment is read-only.

[github-actions]

🤖 Claude Code Review

Review

The change is well-reasoned and the streaming/redaction hardening is solid: StringDecoder per stream, tail-keeping bounded capture, longest-needle-first scrub ordering, and deriving the escaped form from the same JSON.stringify that writes the YAML. Test coverage matches the claims. The Dockerfile COPY for the new import is present in the broker stage (and dev-local-run.sh:579 would have caught it otherwise).

Three things worth addressing.

1. hasAevalDiagnosis can match across a newline, defeating the stdout gate — vox_eval_agentd/aeval-output.ts:23

DIAGNOSIS_LINE contains \s*, which matches \n. hasAevalDiagnosis runs it against the whole buffer, but summarizeAevalFailure runs it against individual trimmed lines. Those can disagree:

const s = "dump: foo |\nERROR |x| y";
hasAevalDiagnosis(s);                       // true  — "|" + "\n" + "ERROR" + " " + "|"
s.split('\n').filter(l => DIAGNOSIS_LINE.test(l))  // []  — no single line matches

Reachable path: stdout contains such a sequence (page dumps / target-site text are exactly what stdout may carry), stderr has no diagnosis. Then selectDiagnosisSource returns stdout, summarizeAevalFailure finds zero ERROR lines, and falls through to its tail path over `${stdout}\n${stderr}` — putting raw stdout into the message that gets logged, returned in the 502, and persisted by Core as a user-visible job error. That is precisely the leak the selectDiagnosisSource comment says it exists to prevent.

Make the predicate line-scoped so the two agree by construction, e.g.:

const DIAGNOSIS_LINE = /^[^\n]*\|[^\S\n]*(ERROR|CRITICAL)[^\S\n]*\|/m;

(or have hasAevalDiagnosis split and test per line). Same regex object is used in both places, so tightening it fixes both.

2. createBoundedCapture discards already-captured diagnosis on any overlong line — vox_eval_agentd/auth-session-broker.ts:139

When the retained region has no newline, buf = '' throws away every complete line captured so far, not just the unsafe partial one. With input "ERROR | real cause\n" + "X".repeat(100_000), the ERROR line is dropped and describeMintFailure reports "login failed with no output" — the exact uninformative failure this PR removes. Keeping the last complete line when it fits under the limit is safe (it's line-aligned by definition) and preserves the diagnosis:

if (nl === -1) {
  const lastNl = buf.lastIndexOf('\n');
  const keepFrom = buf.lastIndexOf('\n', lastNl - 1) + 1;   // start of last complete line
  buf = lastNl !== -1 && lastNl + 1 - keepFrom <= limit ? buf.slice(keepFrom, lastNl + 1) : '';
  dropping = true;
  return;
}

Low likelihood (needs a >64 KB single line), but the failure mode is silent loss of the whole diagnosis.

3. credentialForms models JSON escaping, not Python repr — vox_eval_agentd/auth-session-broker.ts:80

aeval parses the double-quoted YAML scalar and holds the raw password, so the form most likely to appear in a stderr ERROR line is a Python repr/f-string echo. For pa"ss\word that's pa"ss\\word (backslash doubled, quote not escaped) — which matches neither the raw needle nor the JSON needle pa\"ss\\word, so it would survive into the persisted error. Only affects passwords containing a backslash. One extra form covers it:

[v, JSON.stringify(v).slice(1, -1), v.replace(/\\/g, '\\\\')]

Minor

• describeMintFailure returns "login failed with no output" whenever stderr is blank, even if stdout had (non-diagnosis) content. Correct security-wise, but an operator debugging a run where aeval wrote only to stdout gets a message that is factually wrong. "login failed with no usable diagnostic output" would be more honest.
• The --external:crypto/string_decoder additions aren't needed in the esbuild flags — --platform=node externalizes builtins already — so no Dockerfile change was required there. Nothing broken; just noting the existing --external list is decorative.

Nothing blocking on security in the normal (stderr-has-diagnosis) path; item 1 is the one I'd fix before merge.

[guohai]

All four addressed in b31991d.

Claude #1 — \s* matching across a newline. Genuinely subtle, and correct. hasAevalDiagnosis ran the regex over a whole buffer while summarizeAevalFailure ran it per line, and \s matches \n, so the two could disagree exactly as you showed. Both now use a per-line-anchored [^\S\n] form, so they agree by construction rather than by coincidence. Pinned with your "dump: foo |\nERROR |x| y" case.

Codex P2 — unanchored match lets stdout in. Same root, different consequence. Rather than tighten one predicate and risk regressing the primary fix if aeval's log format shifts, I split them by trust level:

• hasAevalDiagnosis (lenient, per-line) — for stderr, the stream we already trust.
• hasLoguruDiagnosis (full TIMESTAMP | LEVEL | shape) — required to admit untrusted stdout.

A page dump containing | ERROR | no longer buys its way into a persisted error; forging a loguru timestamp prefix by accident is a much higher bar.

Codex P3 — 500-char truncation cutting a sub-4-char credential. Fixed at the root instead of deferred again: describeMintFailure now scrubs the inputs before the summarizer sees them, so no credential survives to the truncation boundary at all. The outbound scrub stays as defense in depth. Test uses a 3-char password.

Claude #2 — bounded capture discarding a captured diagnosis. Correct, and it produced precisely the regression this PR exists to prevent: "ERROR | real cause\n" + "X".repeat(100_000) reported login failed with no output. It now abandons only the unsafe partial line and keeps the line-aligned prefix. Test asserts the ERROR survives the 100 KB blob.

---

One correction to my earlier comments. I reported "tsc clean" several times on this PR. That claim was empty: tsconfig.json's include does not list vox_eval_agentd, so npm run check has never type-checked the daemon or the broker — esbuild strips types without checking them. It surfaced when I called hasLoguruDiagnosis without importing it, npm run check passed, and only the unit test caught what would have been a ReferenceError inside the mint failure path. What actually validated this PR was the test suite and the in-container runtime import, not tsc.

npx tsc --strict over vox_eval_agentd/*.ts reports zero errors today, so closing that gap is free — but it is a repo-wide gate change and does not belong buried in this PR, so it is going up separately.

Verification here: 29/29 broker suite, image rebuilt --target broker with a runtime import() in-container (10.6 kb, module loads OK), full gate 1621/1622 with the single failure in agora-e2e (the hardcoded-channel ConvoAI leak, #134, untouched by this diff).

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

Findings

• vox_eval_agentd/auth-session-broker.ts:181 — describeMintFailure scrubs with floorless credential matching before summarizeAevalFailure classifies loguru ERROR lines. For very short passwords, which this PR explicitly tries to support, a password like E, R, |, or even a space can rewrite the timestamp/level separators or the word ERROR, causing DIAGNOSIS_LINE to stop matching. The summarizer then falls back to the generic tail path and can again report trailing INFO/artifacts instead of the real failure if enough non-error lines follow. Consider selecting/classifying on the original text, then scrubbing the chosen lines before truncation, or making the pre-scrub avoid mutating the log prefix used for classification.

Notes

• I did not find credential exposure in the main broker failure path for the real job-31072 shape.
• The Docker COPY vox_eval_agentd/aeval-output.ts addition looks correct for the broker bundle.

[github-actions]

🤖 Claude Code Review

Summary

The change is well-constructed and the security reasoning in the comments matches what the code actually does. I verified the three moving parts by hand:

• createBoundedCapture — the "retained text never begins mid-line" invariant holds in all four exit paths (early return, cut-at-newline, abandon-overlong-line, second trim). The dropping latch correctly swallows the continuation of an abandoned line, and nl !== -1 can only be false when the tail is a genuinely unterminated line (if buf ended in \n, indexOf('\n', cut) would find it for any limit >= 1). Growth is bounded at limit + chunkSize, and chunks are pipe-read-sized.
• DIAGNOSIS_LINE per-line vs. whole-buffer agreement — [^\S\n] + /m genuinely fixes the cross-newline disagreement; the per-line semantics are otherwise equivalent to the old regex, so vox-agentd.ts's use of summarizeAevalFailure doesn't regress. No /g, so .test() is stateless.
• Scrub ordering / escaped forms — longest-needle-first plus pre-scrubbing the inputs (no length floor) before the summarizer's 4-char floor and 500-char truncation closes both the substring-shredding and short-credential gaps. credentialForms deriving the escaped form from the same JSON.stringify that writes the YAML is the right call, and it matches the daemon's parallel yamlEscape list (vox-agentd.ts:1888).

The Dockerfile addition is required and correct — aeval-output.ts imports no non-builtins, so the broker's "no node_modules at runtime" property is preserved, and esbuild resolves the extensionless ./aeval-output.

Findings

Admitted stdout is line-selected leniently (aeval-output.ts:95, auth-session-broker.ts:selectDiagnosisSource) — low severity, hedge path only. hasLoguruDiagnosis gates whether stdout is admitted, but once admitted, summarizeAevalFailure picks lines with the lenient DIAGNOSIS_LINE and takes the last three. Untrusted page-dump text containing a bare | ERROR | therefore both reaches the persisted error and can bury the real loguru line that earned admission. Filtering the admitted stream to loguru-shaped lines before handing it to the summarizer would make selection as strict as admission:

const source = selectDiagnosisSource(stdout, stderr);
// e.g. keep only lines the strict predicate accepts when source === stdout

Not a new leak class (admission already grants the stream), so treat as a precision improvement rather than a blocker.

Detached doc comment (aeval-output.ts:8-23) — the block comment describing summarizeAevalFailure's order of preference now sits directly above DIAGNOSIS_LINE, which has its own doc comment below it. The function itself (line 64) is left undocumented for IDE hover. Move the original comment back down onto the function.

hasLoguruDiagnosis is narrower than its comment implies (aeval-output.ts:45) — it requires an unprefixed line start and fractional seconds. If aeval ever emits ANSI color codes (loguru colorizes on a TTY; a pipe is fine today) or drops .SSS, the hedge silently stops admitting stdout. It fails closed, which is the right direction, so this is a note rather than a defect — worth a line in the comment so a future reader doesn't assume the hedge is live.

Nothing else: no auth, injection, or credential-exposure issues introduced, and the test file covers each of the claimed invariants with fixtures that actually exercise them (I traced the capture, regex, and scrub assertions manually — they hold). I was unable to execute the suite in this environment, so the pass/fail claim is from static tracing, not a run.

[guohai]

Both fixed in 09866d7.

Codex — pre-scrubbing the input can destroy classification. You are right, and this was a bug I introduced one commit earlier while fixing the truncation gap. Scrubbing the input rewrites the tokens the classifier reads: with a password of E, every ERROR becomes [redacted]RROR, DIAGNOSIS_LINE matches nothing, and the summarizer falls back to the tail path — reporting the artifacts banner again, i.e. exactly the bug this PR exists to fix.

The right lever was the floor, not the input. summarizeAevalFailure already redacts after choosing lines and before truncating, so it now takes an optional minNeedleLength (default 4, unchanged for the daemon; the broker passes 0). That closes the short-credential gap without touching the text classification reads.

One thing your suggestion surfaced indirectly: a floor of 0 admits the empty string, and "".split("") splits between every character, which would replace the whole message with [redacted] separators. Guarded with an independent v.length >= 1, with a test.

Regression test runs passwords E, R, |, ERROR against the real job-31072 fixture and asserts the diagnosis still wins. I verified it is not vacuous — restoring the pre-scrub makes it fail:

FAIL  still finds the ERROR line when the password is a substring of ERROR   (pre-scrub)
PASS  (minNeedleLength 0)

Claude — admitted stdout was line-selected leniently. Correct, and worth closing even as a precision issue: the summarizer keeps the last three matching lines, so trailing page-dump text with a bare | ERROR | would not merely leak, it would bury the loguru line that earned admission. selectDiagnosisSource now hands over only loguru-shaped lines, so selection is as strict as admission. Test asserts a cookie=... page-dump line is neither reported nor able to displace the real one.

Verification: 32/32 broker suite, tsc --strict over vox_eval_agentd/*.ts clean, image rebuilt --target broker with a runtime in-container import() (10.7 kb, module loads OK), full gate 1624/1625 — the single failure is practical-shared-agents-credits, which passes in isolation and is the cross-suite contention tracked in #134.

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

No findings.

• The broker now reports loguru ERROR/CRITICAL lines instead of the trailing artifacts banner.
• Credential scrubbing order and escaped-form handling look sound for the changed path.
• Stdout quarantine is appropriately strict and avoids falling back to raw stdout when neither stream has a diagnosis.
• Broker Dockerfile now copies the newly imported helper module.

Testing note: I could not run the targeted test locally because dependencies are not installed in this checkout: vitest: not found.

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref: aeval-output.ts, auth-session-broker.ts, the Dockerfile broker stage, and the new tests.

Verdict

The change is correct and the security reasoning holds up under tracing. I could not find a way to leak a credential or smuggle untrusted stdout into the persisted error. Findings below are minor.

Things I specifically verified:

• Predicate consistency. hasAevalDiagnosis (whole-buffer, /m) and the per-line filter in summarizeAevalFailure genuinely agree: DIAGNOSIS_LINE is prefix-anchored, and trim() can neither create nor destroy a match for a prefix-anchored pattern. The \s → [^\S\n] fix is the right one.
• stdout quarantine. describeMintFailure passes selectDiagnosisSource(...) — never raw stdout — as the summarizer's first argument, so the tail fallback (aeval-output.ts:125-127) can only ever see stderr. That closes the hole the commit message describes.
• createBoundedCapture invariant. buf always starts at a line boundary: every trim slices at nl + 1, and the dropping path resumes after a newline. In the no-newline-at-or-after-cut branch, lastNl < cut necessarily, so buf in that branch cannot end in \n — the "abandon the partial line" reasoning is sound, and the second trim (aeval-output-style nl2) always finds a newline because buf ends in one. Peak memory is limit + one pipe chunk.
• Ordering. Scrub-then-slice(0, 500) in the summarizer means truncation can't expose a half-redacted credential, and the minNeedleLength: 0 + v.length >= 1 guard correctly avoids "".split("").
• Longest-first sort in scrubCredentials operates on the array returned by .filter(), so it does not mutate the caller's array.

Minor findings

1. Orphaned JSDoc — vox_eval_agentd/aeval-output.ts:8-23

The "Pick the most informative line… Order of preference: 1. … 2. …" block that documented summarizeAevalFailure now sits directly above the DIAGNOSIS_LINE comment, so it attaches to nothing. Two consecutive doc blocks with the function ~55 lines further down. Worth moving back onto summarizeAevalFailure at line 77 — it's the only place the two-tier preference order is written down.

2. Timeout path reports no captured output — auth-session-broker.ts:254-258

finish(() => reject(new Error(`login timed out after ${timeoutMs}ms`)));

errCap.text is populated and in scope here, but discarded. A broker-level timeout (WEB_SESSION_MINT_TIMEOUT_SECONDS, default 180s) is a plausible outcome for a hung login, and it's the one remaining path that gives the operator nothing — the same class of problem this PR fixes for the non-zero-exit path. Note the decoders would need flushing here too. Not a blocker; the fixture's "Timeout 60000ms exceeded" is aeval's internal step timeout, which does exit non-zero and does get the good treatment.

3. LOGURU_DIAGNOSIS_LINE requires fractional seconds — aeval-output.ts:45-46

[.,]\d+ is mandatory. If aeval ever configures loguru as {time:YYYY-MM-DD HH:mm:ss} (no .SSS), the stdout hedge silently never admits anything and you're back to "login failed with no output" for the exact scenario the hedge exists for. This fails closed, which is the right direction, and the tests only exercise a synthetic fixture — so the hedge is effectively unvalidated against the real binary. Fine to ship; just be aware the fallback is load-bearing-but-unproven.

4. selectDiagnosisSource overloads ''

Empty return means both "stdout is empty" and "stdout is rejected". describeMintFailure treats them identically so there's no bug, but a { admitted: boolean, text: string } (or just a comment on the sentinel) would make the security-relevant distinction legible to the next reader.

Non-issues I checked and dismissed

• v.length >= 1 && v.length >= minNeedleLength is redundant-looking but the comment is right; Math.max(1, minNeedleLength) would be equivalent and slightly clearer, purely stylistic.
• Dockerfile: esbuild --bundle --platform=node treats builtins as external by default, so string_decoder needs no new --external flag, and ./aeval-output resolves from the newly-copied file. Correct as written.
• Admitted-stdout loguru lines still carry attacker-influenceable message bodies, but they're scrubbed and capped at 500 chars, and only reachable when stderr carried no diagnosis at all. Acceptable residual.

The test additions are unusually good — the "password is a substring of ERROR" case and the overlong-line-continuation case are exactly the two things a reviewer would want to see pinned down. I was not able to run the suite (npx vitest required approval in this environment), so I reviewed the tests by reading rather than execution.

[github-actions]

🤖 Claude Code Review

Summary

Solid, well-reasoned change. The core fix (prefer loguru ERROR lines over aeval's trailing artifacts banner, redact before truncation, bound and line-align the capture, and gate failure detail to owner-operated agents while still serving the session to consented marketplace agents) is correct and the isSessionServable → isOwnerOperatedAgent split is behavior-preserving.

I verified the pieces that looked most likely to be subtly wrong and they hold up:

• DIAGNOSIS_LINE / hasAevalDiagnosis / the per-line filter genuinely agree, because /m's ^ anchors on exactly the set LINE_TERMINATORS splits on and the char class can't cross a terminator.
• selectDiagnosisSource correctly keeps rejected stdout out of the tail fallback path (the neither stream has a diagnosis case), which was the actual hole.
• createBoundedCapture's dropping state machine is right, and the 2× hysteresis really does amortize eviction (during growth complete is an un-flattened ConsString and evictOldest early-returns without an exec; the trim then flattens once and subsequent slices are O(1) SlicedStrings).
• strip's widened \S+(?:\s+\S+)? does not backtrack pathologically — \S+ cannot cross whitespace, so the search space is len(tok1) × len(tok2), not n².

Findings below, in severity order.

The daemon path did not get the StringDecoder fix

vox_eval_agentd/vox-agentd.ts:840,845

proc.stdout.on('data', (data) => { stdout += data.toString(); ... });
proc.stderr.on('data', (data) => { stderr += data.toString(); ... });

This is the exact scrub bypass the broker's new StringDecoder pair exists to close, and the comment there states the reasoning: a UTF-8 sequence split across two pipe reads decodes to U+FFFD on each side, and a mangled non-ASCII secret no longer matches its needle in activeSecretValues.

Failure scenario: a job secret containing a non-ASCII character (pässwort, a CJK passphrase, an em-dash in a webhook URL) is echoed by aeval in an ERROR line that lands on a 64 KiB pipe-read boundary mid-character. summarizeAevalFailure redacts pässwort but the buffer holds p\uFFFD\uFFFDsswort, so nothing matches, and the fragment is persisted to the job's user-visible error. The daemon path is the one where secret values actually reach the agent, so it has strictly more secret material flowing through it than the broker's single login pair.

This PR touches line 878 (the summarize call) and 1899 (activeSecretValues) — the two lines immediately around the gap — so it reads as an oversight rather than a deliberate scope boundary. Same two-line fix as the broker.

Related and lower priority: those buffers are still unbounded while the broker got CAPTURE_LIMIT, so a noisy aeval run is an OOM on the agent, and reduceUrlsSafely now does two extra full-buffer regex passes plus a split/join per URL-shaped needle over that unbounded string on the failure path.

Core's re-redaction backstop uses raw values only

server/broker-registry.ts — the new detail re-redaction:

detail = [req.email, req.password]
  .filter(...)
  .sort((a, b) => b.length - a.length)
  .reduce((acc, v) => acc.split(v).join("[redacted]"), detail);

The comment says this exists so "a stale or buggy broker echoing a credential should not become a durable leak here." But this PR makes precisely the opposite argument two files over, when upgrading the broker's own backstop:

Uses credentialForms, not the raw pair: a mint that forgot to scrub is precisely the case where the ESCAPED form arrives here, so a raw-only backstop would miss the one thing it exists to catch.

A stale broker image (the realistic case — Core and sidecar deploy independently) is one that predates credentialForms entirely, so the JSON-escaped and percent-encoded spellings are exactly what it would fail to scrub, and they are exactly what this backstop cannot catch. detail lands in webSessions.lastError, which is now served to owner-operated agents.

Core can't import vox_eval_agentd/aeval-output.ts; the clean fix is to move urlForms and the JSON.stringify(v).slice(1, -1) derivation into shared/ (next to shared/secrets.ts, which both sides already import) and have the broker, the daemon, and Core share one definition. That also removes the drift risk the PR is otherwise careful about.

mintTimeoutSeconds() was left unvalidated, so the two reads now genuinely diverge

server/broker-registry.ts adds a validated read and comments "The env read is duplicated rather than imported because auth-session.ts imports this module; keep the two in step." But server/auth-session.ts:129 is still:

return parseInt(process.env.WEB_SESSION_MINT_TIMEOUT_SECONDS || "180", 10);

With WEB_SESSION_MINT_TIMEOUT_SECONDS=abc, the new code falls back to 180 (abort at 195 s) while staleMintThresholdSeconds() returns NaN, which reaches claimWebSessionMint and is interpolated into make_interval(secs => NaN) — Postgres rejects that with interval out of range, so every mint claim throws. The latent bug predates this PR, but the PR is what makes the two paths disagree, and the fix is to apply the same Number.isFinite(...) && > 0 guard in mintTimeoutSeconds().

Minor

• Daemon timeout branch still reports nothing. vox-agentd.ts:869 — aeval timed out after ${AEVAL_RUN_TIMEOUT_MS}ms with no captured detail. The broker's timeout path was upgraded in this PR with the reasoning that "a hung login was the one path still telling the operator nothing"; the daemon has the identical branch and the buffers are right there. Not a defect, just the same asymmetry as the first finding.
• NO_OUTPUT_MESSAGE identity check. summary === NO_OUTPUT_MESSAGE in the timeout path would false-positive if aeval ever emitted the literal string login failed with no output. Harmless (worst case: a dropped : ) and the coupling is documented — noting only so it isn't rediscovered.
• reduceUrlsToHost appends /… to bare-authority URLs, so https://example.com becomes https://example.com/…. Cosmetic.

The Dockerfile change is correct — --platform=node does externalize all builtins, and aeval-output.ts is genuinely import-free so the broker bundle still needs no node_modules at runtime.

[guohai]

All three fixed in bc488fb.

1. URL-valued secret as a PREFIX (Codex). Correct, and it defeated the guard I had just added: wss://h/rtc echoed as wss://h/rtc?access_token=JWT became [redacted]?access_token=JWT, which reduceUrlsToHost no longer sees as a URL, so the query survived. The pre-redaction consumes the rest of the URL run now, not just the needle.

2. The daemon never got the StringDecoder fix (Claude). You are right that it reads as an oversight — this PR touches the two lines around it. And your severity ordering is right too: that path carries every job secret, not just the broker's single login pair. Both streams now use a decoder and the broker's bounded capture, which moves into aeval-output.ts alongside the summarizer so there is one implementation.

3. Core's backstop used raw values only. The sharpest observation in this review, because it names the shape of the whole problem: a backstop covering fewer encodings than the layer it backstops is weakest in exactly the case that calls for it. The broker failing to scrub is precisely when an escaped or URL-encoded spelling arrives.

Rather than widen Core's copy, I removed the third copy. shared/credentials.ts now owns urlForms, credentialForms and redactValues; Core imports it via @shared/credentials, the daemon and broker via ../shared/credentials with a matching Dockerfile COPY — the arrangement shared/secrets.ts already uses, so it is a pattern the repo has rather than a new one. scrubCredentials survives as a named alias because it reads better at its call sites. The daemon keeps yamlEscape alongside the shared forms, since that is what its own resolveSecrets substitutes — same rule as deriving the JSON form from the emitter.

That is the same drift you and Codex have flagged in four different guises across this review (client/server name heuristic in #135, broker/daemon URL guard, broker/daemon capture, Core/broker needles). Consolidating was overdue.

Verification: tsc clean, both images rebuilt from the new layout (broker 13.3 kb broker loads OK; daemon 185.2 kb, node --check passes), server and daemon restarted so integration suites hit the new code.

Local gate 1646/1651, and all five are accounted for: four are agora-e2e's ConvoAI orphan, a production bug fixed in #136 and not on this branch — clearing the leaked session makes that file 13/13, which I verified. The fifth moves between files run to run (#134 contention) and passes in isolation.

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

Reviewed the full diff against the merge base (3523859...HEAD), including the surrounding auth-session.ts, storage.ts, and daemon call sites.

Overall this is careful, well-reasoned work. The redaction consolidation into shared/credentials.ts, the strict/lenient predicate split for admitting the untrusted stdout stream, the StringDecoder fix for split UTF-8 sequences, and the isOwnerOperatedAgent disclosure split in permissions.ts/routes.ts are all correct as written. I traced createBoundedCapture by hand through the overlong-line, cross-chunk-continuation, CRLF-split, and eviction paths and did not find a case where retained text can begin mid-line.

Findings below.

1. server/broker-registry.ts — truncates the third-party body before redacting it

if (typeof body?.error === "string") detail = body.error.slice(0, 500);
...
detail = redactValues(detail, credentialForms([req.email, req.password]));

This is the exact ordering the PR argues against elsewhere. aeval-output.ts documents that scrubbing must happen before truncation, and summarizeAevalFailure returns scrub(...).slice(0, 500) for that reason. Here it's inverted.

Failure scenario: the broker fails to scrub (which is the only case this backstop exists for — the code comment says so explicitly), and the credential straddles offset 500 in the body. slice leaves a partial value, redactValues matches no whole needle, and the surviving prefix lands in web_sessions.last_error — a durable field now served to owner-operated agents by the new routes.ts branch.

Fix is a reorder:

detail = redactValues(detail, credentialForms([req.email, req.password])).slice(0, 500);

Note tests/broker-registry.test.ts:134 ("caps third-party detail before it reaches a durable field") pins the current order with /^broker mint failed: 502: x{500}$/; it passes either way since the payload has no credential, but a boundary-straddling case isn't covered.

2. server/broker-registry.ts — the "keep the two in step" invariant isn't actually held

The new comment says the env read is duplicated from auth-session.ts and the two should be kept in step, but only this side got validation. auth-session.ts:127 is still:

return parseInt(process.env.WEB_SESSION_MINT_TIMEOUT_SECONDS || "180", 10);

With a non-numeric value, staleMintThresholdSeconds() is NaN, which reaches claimWebSessionMint and is interpolated into make_interval(secs => $n) (storage.ts:2731) — Postgres rejects NaN there with interval out of range, so ensureSession throws into its outer catch and every mint silently stops happening while the abort deadline quietly uses 180s. Pre-existing, but the new comment asserts an invariant that isn't enforced. Cheapest fix is to export the validated parse from one place and have both call it (broker-registry doesn't import auth-session, so the helper can live in broker-registry and be imported the other direction, matching the existing dependency edge).

3. vox_eval_agentd/aeval-output.ts:1 — dead import

import { urlForms, redactValues } from '../shared/credentials';
export { urlForms, credentialForms, redactValues } from '../shared/credentials';

Neither imported binding is used in the file — line 2 re-exports independently. Line 1 should be deleted (npm run lint with the TS unused-vars rule should catch this).

Relatedly, and in tension with the module's own "one definition" premise: summarizeAevalFailure still hand-rolls scrub (needles.reduce((acc, v) => acc.split(v).join('[redacted]'), text)) rather than delegating to redactValues after applying minNeedleLength. Same logic, two copies — the drift risk the PR set out to remove.

4. Minor — bounded capture can evict aeval's Session directory: line

resolveAevalOutputDir (vox-agentd.ts:914) greps the captured output for Session directory: ..., which aeval prints early. createBoundedCapture keeps the tail, so on a run exceeding ~128 KiB per stream that line is evicted and the daemon falls through to the "most recent directory by name" heuristic. That fallback is almost always right for a single in-flight run, so this is a graceful degradation rather than a break — but it is a behavior change from the previously-unbounded buffer and isn't noted anywhere. Worth a comment at the resolveAevalOutputDir call site at minimum.

Things I checked and found correct

• isSessionServable refactor preserves the original three-arm semantics exactly (owner → org → consented target token).
• The 503 branch's ||-not-?? fallback is right, and lastError has no other exposure path (grepped server/, client/, shared/).
• selectDiagnosisSource admission (strict, whole-buffer) and selection (strict, per-line, post-trim) agree, and LOGURU_DIAGNOSIS_LINE ⊂ DIAGNOSIS_LINE, so an admitted stdout line always survives the summarizer's lenient filter. hasAevalDiagnosis(stderr) and the per-line filter use the same pattern, so there's no stream where one says "no diagnosis" and the other picks lines up.
• urlForms correctly returns [] on a lone surrogate instead of throwing inside a child-process handler.
• describeMintFailure's NO_OUTPUT_MESSAGE early return does precede any scrub, so the timeout gate's stated coupling holds.
• Dockerfile: ../shared/credentials resolves to /shared/credentials.ts from /app/ for both auth-session-broker.ts and aeval-output.ts; dropping the builtin --external flags is correct under --platform=node, and string_decoder is externalized without being listed.
• AbortSignal.timeout is available on Node 22 (repo's runtime), and it bounds the response-body read too, so the new res.json() on the error path can't hang.

[guohai]

All four fixed in c8e7fa8.

1. Truncate-before-redact in Core. Caught cleanly — it was the exact inversion this change argues for elsewhere, in the one place that only executes when the broker has already failed to scrub. Reordered, and I added the boundary case you noted was missing: the credential is positioned across offset 500, and the test fails under the old order.

2. "Keep the two in step" was not held. Right — only the new copy validated. Rather than validate both, mintTimeoutSeconds() now lives in broker-registry.ts, which auth-session.ts already imports, and auth-session.ts re-exports it. So staleMintThresholdSeconds() and the AbortSignal derive from one validated number instead of two that a comment asked someone to keep synchronized.

3. Dead import, and the last hand-rolled scrub. Both gone. The second half is the better catch: summarizeAevalFailure was still open-coding the reduce after this PR consolidated everything else, which is precisely the drift the module argues against.

4. Session directory: eviction. Documented at the call site. Agreed it degrades rather than breaks — resolveAevalOutputDir's newest-by-name fallback is right for a single in-flight run — but it is a behaviour change from the unbounded buffer and deserved saying.

Verification: tsc clean, npm run lint introduces no new warnings in the touched files, broker image rebuilt with a runtime in-container import() (13.2 kb), and both server and daemon restarted so the integration suites exercised the new code.

Local gate 1648/1652, and all four failures are agora-e2e's ConvoAI orphan — the production bug fixed in #136, not on this branch. I verified separately that clearing the leaked session takes that file to 13/13.

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

Summary

Solid, well-reasoned change. The core fix — preferring loguru ERROR lines over aeval's trailing artifacts banner, and folding the broker's diagnosis into webSessions.lastError — is correct, and the security work around it is careful: redaction happens before truncation everywhere, credentialForms is one shared definition, the StringDecoder fix genuinely closes a scrub-bypass (split UTF‑8 → U+FFFD → unmatchable needle), AbortSignal.timeout closes a real hang, and splitting isOwnerOperatedAgent out of isSessionServable is the right call for withholding page-state detail from marketplace agents. createBoundedCapture's line-alignment and dropping state machine hold up under the edge cases I traced (\r-only terminators, overlong line spanning chunks, eviction hysteresis termination).

Four things worth acting on.

---

1. Bounding the daemon's capture silently degrades the stdout metrics fallback (medium)

vox-agentd.ts:900 — parseAevalResults(outputDir, allOutput) now receives a tail-truncated buffer. Its priority‑3 fallback parseAevalStdout (vox-agentd.ts:1325) is not a failure-path summarizer; it walks the entire event log with a phase state machine:

let currentPhase = 'response'; // default phase until we see a phase marker

On a run whose combined output exceeds CAPTURE_LIMIT, the early Phase N (...) completed markers are evicted, so the walk resumes mid-run under the default response phase — interrupt-phase turns get counted as response latencies, and early turns are lost outright. This happens on the exit-code-0 path, so nothing fails; it just reports wrong numbers. That contradicts the policy stated two lines above at vox-agentd.ts:879-883 ("we deliberately don't salvage partial metrics… statistically unreliable").

The resolveAevalOutputDir degradation is documented at vox-agentd.ts:840-845 and is genuinely benign; this one isn't, and isn't mentioned. Suggest exposing a truncated flag from createBoundedCapture and failing the job (rather than emitting metrics) when metrics.json is absent and the capture overflowed. That keeps the OOM bound without inventing latency numbers.

2. mintTimeoutSeconds() doesn't do what its doc says (low)

broker-registry.ts:64-67 — the JSDoc argues the validation exists because AbortSignal.timeout takes [EnforceRange] unsigned long long, but only NaN and <= 0 are rejected. An out-of-range value (WEB_SESSION_MINT_TIMEOUT_SECONDS=99999999999999999999 → 1e20, above 2^64‑1) passes Number.isFinite && > 0 and still throws TypeError at AbortSignal.timeout. It's caught by ensureSession's inner catch so it isn't process-fatal — but every mint fails with an opaque message. An upper clamp (Math.min(configured, 3600)) closes the gap the comment claims is closed. Separately, Number.isFinite is exactly a NaN check here, since parseInt never returns Infinity.

3. Daemon's timeout path still reports nothing (low)

vox-agentd.ts:884-885 still yields a bare aeval timed out after ${AEVAL_RUN_TIMEOUT_MS}ms. The broker's identical hang path got the "say what we captured before it hung" treatment (auth-session-broker.ts:203-217), and outCap/errCap are already in scope here. Same operator complaint, one file over — cheap symmetry.

4. reduceUrlsSafely continuation class doesn't exclude | (nit)

aeval-output.ts:207 — escapeForRegExp(v) + '[^\\s"\'<>]*' will swallow |-delimited text following a URL-valued needle up to the next whitespace. Over-redaction only, and it can't eat a loguru prefix (which always precedes the message), so it's cosmetic — but adding | to the excluded set costs nothing.

---

Verified, no issue found

• LINE_TERMINATORS / DIAGNOSIS_LINE / LOGURU_DIAGNOSIS_LINE are all non-global, so .test/.exec are stateless; the whole-buffer and per-line predicates do agree across \r, \u2028, \u2029.
• selectDiagnosisSource correctly excludes quarantined stdout from summarizeAevalFailure's tail fallback (passing '' for the stdout arg), which was the stated superset bug.
• redactValues / reduceUrlsSafely sort on filter() copies — no caller-array mutation.
• reduceUrlsSafely pre-redaction can't disturb line classification: needles require ://, which can't appear in a loguru timestamp/level prefix.
• The describeMintFailure ↔ NO_OUTPUT_MESSAGE coupling at auth-session-broker.ts:211-216 holds — the constant is returned before any scrub can rewrite it.
• Dockerfile: dropping --external: for builtins is correct (--platform=node externalizes all builtins, including the newly-imported string_decoder); @aws-sdk/client-s3 correctly retained. COPY shared/credentials.ts /shared/credentials.ts resolves ../shared/credentials from both /app/auth-session-broker.ts and /app/aeval-output.ts.
• webSessions.lastError is text (shared/schema.ts:806), so the ~525-char message won't truncate mid-[redacted].
• isSessionServable is behaviorally identical after the extraction; the routes.ts:3951 ||-not-?? choice is right.

I could not run tsc or the test suite (sandbox declined both), so the review is static only. Test coverage in the diff looks genuinely targeted at the tricky cases (bare-CR quarantine bypass, lone surrogate, short-needle floor, password-contains-email ordering, percent-encoded SSO redirect).

[guohai]

All four fixed in 091c339, and #1 was a regression this PR introduced — thank you for tracing it to the success path.

1. Bounding the daemon capture corrupted the stdout METRICS fallback. You are right that this is a different class from the resolveAevalOutputDir degradation I documented: parseAevalStdout is not a failure summarizer, it walks the whole event log with a phase state machine defaulting to response, so a tail-truncated buffer makes it resume mid-run in the wrong phase — interrupt turns counted as response latencies, early turns lost. Reported as success, with no signal. That directly contradicts the no-partial-metrics policy two lines above it.

Took your suggested shape: createBoundedCapture exposes truncated, and the daemon refuses the fallback when metrics.json is absent and the capture overflowed, failing the job rather than inventing latencies. metrics.json is read from disk so the common path is untouched.

2. mintTimeoutSeconds() did not do what its doc said. Correct on both counts — 1e20 passes "finite and positive" and still throws at AbortSignal.timeout, and Number.isFinite here is exactly a NaN check since parseInt never returns Infinity. Clamped at both ends, so the comment now describes the code.

3. The daemon timeout path still reported nothing. Fixed, mirroring the broker — including reading completeText rather than text, since the child is mid-write and a half-written line can strand a credential fragment.

4. | in the continuation class. Added.

Verification: tsc clean both sides, both images rebuilt (daemon 186.3 kb node --check passes; broker 13.4 kb broker loads OK), server and daemon restarted so the integration suites hit the new code.

Local gate 1652/1653 — including agora-e2e green. The single failure is tier-pool-claim's reaper case racing the live daemon for a pooled job, which passes in isolation (#134).

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

I reviewed the merge ref statically (the sandbox blocked vitest/tsc, so I did not execute the suite).

Overall this is solid, security-conscious work. The redaction centralization in shared/credentials.ts, the redact-before-truncate ordering, the StringDecoder fix (a genuine scrub bypass — split UTF‑8 sequences decoding to U+FFFD broke needle matching), the isOwnerOperatedAgent split so marketplace agents get status but not page-quoting detail, and refusing to derive metrics from a truncated log are all correct and well covered by the new tests.

Findings below, most severe first.

1. The broker's own mint timeout is still unvalidated — vox_eval_agentd/auth-session-broker.ts:350

The PR's stated rationale for centralizing and validating mintTimeoutSeconds() (server/broker-registry.ts:63) is that a bad WEB_SESSION_MINT_TIMEOUT_SECONDS takes the mint path down. The sidecar reads the same env var with a bare parseInt:

const timeoutMs = parseInt(process.env.WEB_SESSION_MINT_TIMEOUT_SECONDS || '180', 10) * 1000;

WEB_SESSION_MINT_TIMEOUT_SECONDS=abc → NaN * 1000 → setTimeout(fn, NaN) is coerced to 0, so the timer fires on the next tick: every mint SIGTERMs its child immediately and rejects with login timed out after NaNms. A 0 or negative value does the same. Core would clamp to 180 and wait ~195s for a broker that fails instantly.

shared/credentials.ts already proves the shared-module + Dockerfile-COPY pattern works here — moving the clamp helper into shared/ and calling it from both sides would make the guarantee actually hold end to end.

2. createBoundedCapture evicts captured lines for a line it then discards — vox_eval_agentd/aeval-output.ts:293-307

The comment at :318-320 states the design intent: an overlong line is dropped rather than evicting from the front, "so one ERROR line followed by a 100 KB blob would report nothing useful." The streaming (no-terminator) path violates that.

While a huge single line accumulates across chunks, each push hits the m === null branch with partial.length <= limit, so it calls evictOldest(). Once complete + partial > 2*limit, eviction trims complete until complete + partial <= limit — with partial near limit, complete is emptied. The next chunk pushes partial past limit and it's discarded anyway. Net effect: the diagnosis already captured is destroyed by a blob that never makes it into the buffer.

Simplest fix: don't call evictOldest() from the no-terminator branch. partial is independently capped at limit, so retained text stays bounded by 3*limit instead of 2*limit — still bounded, which is the property the comment says matters.

3. Stale comments that now contradict the code

• server/broker-registry.ts:110-115 — "The env read is duplicated rather than imported because auth-session.ts imports this module; keep the two in step." There is no duplication anymore; mintTimeoutSeconds is defined in this file and imported by auth-session.ts. It directly contradicts the doc block at :55-62 and will send the next reader looking for a second copy.
• vox_eval_agentd/Dockerfile:106 — the broker stage header still says "no shared/", but shared/credentials.ts is now COPYed in (and the .ts source remains in the final image after the bundle; harmless, just no longer what the comment claims).

Given how much of this change's value is in its comments, these two are worth fixing.

4. Minor

• server/broker-registry.ts:126 — await res.json() buffers a failing broker's response with no size bound before the 500-char cap is applied. The peer is internal and authenticated and AbortSignal.timeout bounds the read, so this is low risk, but a res.text() + slice before parsing would remove it entirely.
• vox_eval_agentd/vox-agentd.ts:1939 — activeSecretValues isn't deduped, so an alphanumeric secret contributes ~6 identical needles (v, yamlEscape(v), and four urlForms that all equal v). summarizeAevalFailure dedupes via Set, but reduceUrlsSafely and any direct redactValues caller don't. Wasted passes only, not a correctness issue — a Set here would match what credentialForms already does.

Verified as correct

• isSessionServable is behaviorally identical after the split (owner → org → consent, same predicates); the excess targetTokenId on serveJob is fine since it's a variable, not a fresh literal.
• || "session mint failed" rather than ?? is right — an empty lastError should fall back.
• DIAGNOSIS_LINE / LOGURU_DIAGNOSIS_LINE are non-global, so .test() per line has no lastIndex hazard, and the [^\r\n\u2028\u2029]* anchoring does make hasAevalDiagnosis agree with the per-line filter.
• selectDiagnosisSource returning '' when stderr carries a diagnosis correctly keeps quarantined stdout out of the summarizer's tail fallback.
• The Dockerfile builtin-external removal is correct — --platform=node externalizes every builtin, and esbuild resolves ../shared/credentials to /shared/credentials.ts from /app by default extension order.

[github-actions]

🤖 Claude Code Review

Overall this is careful, well-reasoned work. The AbortSignal fix in mintViaBroker, the clamped shared mintTimeoutSeconds(), the StringDecoder-per-stream change, redact-before-truncate ordering, and the isOwnerOperatedAgent split (serving a session ≠ serving the failure detail) are all correct and address real gaps. The Dockerfile --external cleanup is right — --platform=node already externalizes builtins. A few issues:

1. Truncation guard rejects runs whose metrics are on disk under analysis/ — vox_eval_agentd/vox-agentd.ts:919

const metricsOnDisk = fs.existsSync(path.join(outputDir, 'metrics.json'));
if (!metricsOnDisk && (outCap.truncated || errCap.truncated)) { fail(...); }

parseAevalResults (vox-agentd.ts:1012-1014) treats two paths as priority-1 sources:

path.join(outputDir, 'metrics.json'),
path.join(outputDir, 'analysis', 'metrics.json'),

and report.json as priority 2 — also read from disk, also unaffected by truncation. The guard only checks the first. So an exit-0 run whose analysis output landed in analysis/metrics.json (or that would have been served by report.json) and whose console output exceeded the cap is failed with "refusing to derive latencies from a truncated log" even though the stdout fallback would never have been reached. That's a spurious job failure on a run that actually measured the product.

The guard should mirror the source list — e.g. gate on "none of the disk sources are usable", or at minimum test both metrics paths and report.json.

2. res.text() does not bound the body it claims to bound — server/broker-registry.ts:113

// text() + slice before parsing, so a failing broker cannot make Core
// buffer an unbounded body.
const raw = (await res.text()).slice(0, 8192);

await res.text() fully buffers the response before .slice runs. The slice bounds what is retained, not what is allocated — a broker returning a multi-GB 502 body still OOMs Core. Impact is low (internal-only sidecar, authenticated), but the comment asserts a protection that isn't there. Either read the stream incrementally with an explicit byte cap, or reword the comment to say it bounds retention only.

3. CAPTURE_LIMIT = 64 KiB is small relative to a real run — aeval-output.ts:220

A multi-minute voice eval emits well past 64 KiB on stdout. Two consequences worth confirming against a real run before merge:

• resolveAevalOutputDir parses "Session directory: …", printed early, so on most production runs it will be evicted and resolution silently falls back to newest-by-name. The code comments this at vox-agentd.ts:840-846, and it degrades safely for a single in-flight run — but it means the primary resolution path is effectively dead in production, not a rare edge.
• It makes issue Improve initialization gating and routing #1 fire routinely rather than rarely.

If the cap exists purely as an OOM bound, 1 MiB would still bound memory while keeping these paths working.

4. Minor — vox_eval_agentd/vox-agentd.ts:887-895

timeoutDetail is computed unconditionally on every non-zero exit, then discarded on the !timedOut branch which recomputes summarizeAevalFailure over outCap.text. Two full summarize passes over up to ~400 KiB × N needles per failure. Also, (outCap.completeText + errCap.completeText).trim() concatenates both buffers just to test emptiness — the broker's describeMintFailure explicitly avoids exactly this. Move the computation inside the timedOut branch.

Notes, not blockers

• mintViaBroker uses redactValues directly with credentialForms(...), which — unlike summarizeAevalFailure's needle construction — does not split multi-line values into per-line fragments. Irrelevant for a login pair in practice, but the two redaction entry points differ in a way the shared-module rationale suggests they shouldn't.
• The NO_OUTPUT_MESSAGE sentinel comparison in the broker's timeout path is a real coupling (a genuine aeval line equal to that string would suppress the detail). The comment flags it; fine as-is.
• selectDiagnosisSource's strict/lenient predicate split holds: LOGURU_DIAGNOSIS_LINE implies DIAGNOSIS_LINE, so an admitted stdout stream always reaches the errors path rather than the tail path. Good.
• createBoundedCapture's dropping state machine, eviction hysteresis, and sticky-index scan all look correct; the CRLF-split-across-chunks case yields one spurious empty line, which is harmless.

[guohai]

All four fixed in 6389605.

1. The truncation guard checked one path where parseAevalResults accepts three. The important catch — my guard would have failed perfectly good exit-0 runs whose analysis landed under analysis/metrics.json, which is a worse failure than the one it was added for. It now mirrors the full disk list (metrics.json, analysis/metrics.json, report.json), all of which are read from files and so are untouched by a truncated console capture.

2. CAPTURE_LIMIT too small. Agreed, and this is the one that made #1 matter: at 64 KiB a multi-minute voice eval evicts the early Session directory: line on ordinary runs, so resolveAevalOutputDir's primary path would be effectively dead in production rather than a rare fallback, and the guard would fire routinely. Raised to 1 MiB — the cap only ever existed as an OOM bound, and 1 MiB still is one.

3. The res.text() comment overclaimed. Correct: text() buffers first, so the slice bounds retention, not allocation. Reworded to say exactly that and why the residual is accepted (internal authenticated peer, AbortSignal.timeout bounds the read) rather than asserting a protection that is not there.

4. Unconditional timeoutDetail. Moved inside the branch, and the emptiness test no longer concatenates both buffers — the same thing describeMintFailure avoids one file over.

Verification: tsc clean both sides, both images rebuilt (broker 13.8 kb broker loads OK; daemon 186.6 kb node --check passes). Local gate 1650/1654, all four failures agora-e2e's ConvoAI orphan, which is #136's production bug and not on this branch.

---

On CI: codex-review has hung on every run since 18:15 — the run on c8e7fa8 sat on its "Run Codex Code Review" step for 53 minutes (19:06:19 → 19:59:20) before failing, with the step still in_progress. It is not reporting findings; it is not finishing. I cancelled the two hung runs. Every other check — build-vox-service, build-vox-eval-agentd, build-vox-clash-runner, changes, review — is green, and the Claude reviewer has completed in ~3 minutes each round throughout.

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

Reviewed the full diff against 35238596. The change is coherent and unusually well-tested — the redaction ordering (redact → truncate), the credentialForms/mintTimeoutSeconds deduplication, the AbortSignal that staleMintThresholdSeconds() was already documented against, and the owner-tier gate on session.lastError all hold up. A few things worth addressing:

Findings

1. The truncation guard checks file existence, not usability — vox_eval_agentd/vox-agentd.ts:930-943

const metricsOnDisk = diskSources.some((f) => fs.existsSync(f));
if (!metricsOnDisk && (outCap.truncated || errCap.truncated)) { fail(...) }
const results = this.parseAevalResults(outputDir, allOutput);

parseAevalResults (:1029-1069) does not commit to a disk source just because the file exists — a metrics.json that fails hasLatency() || isAnalysisOutput(), and a report.json that fails hasLatency(), both fall through to parseAevalStdout(allOutput). So on a run with a present-but-structureless metrics.json and a truncated capture, the exact failure mode the guard was added to prevent (phase state machine resuming mid-run in the default 'response' phase, miscounting interrupt turns as response latencies) still reaches evalResults on the exit-code-0 path.

Gating on the parse result rather than existsSync closes it — e.g. hoist the parseAevalResults call and refuse only when it fell through to the stdout path:

const results = this.parseAevalResults(outputDir, allOutput);
if (results.source === 'stdout' && (outCap.truncated || errCap.truncated)) { fail(...); return; }

(needs a source marker or equivalent on the return; the current diskSources list is otherwise correct and matches :1030-1057).

2. Stale comment in the broker stage — vox_eval_agentd/Dockerfile:120-121

auth-session-broker.ts imports only Node builtins plus ./aeval-output (COPYed above, itself import-free)

aeval-output.ts:1 imports ../shared/credentials. It happens to work because the broker stage also COPYs shared/credentials.ts, but the comment is the thing a future editor reads before deciding whether that COPY is still needed. Since the same paragraph is arguing that a partial --external list "reads as authoritative," this one has the same hazard.

Notes (no action needed)

• createBoundedCapture retains up to 3 × limit per stream (2× hysteresis in evictOldest + an independent partial cap) — 3 MiB × 2 streams per concurrent mint/job. Bounded and documented; just worth knowing the effective figure is 6 MiB, not 1 MiB.
• The NO_OUTPUT_MESSAGE identity check at auth-session-broker.ts:229 couples the timeout gate to describeMintFailure early-returning the constant before any scrub. The comment flags it and there's a test pinning it, so it's held — but it is a string-equality coupling across two functions.
• urlForms covers all-upper and all-lower percent-hex but not mixed casing within one value (%2Fa%2f). Documented as best-effort, and the outer scrub plus reduceUrlsSafely cover the realistic cases.
• Auth on /mint correctly runs before the request body is read; the unbounded body accumulation there is pre-existing and internal-only.

Everything else — the isOwnerOperatedAgent split, the ||-not-?? fallback at routes.ts:3951, the StringDecoder per stream, LINE_TERMINATORS unifying split and regex semantics, and the clamping in shared/mint-timeout.ts — reads correct.

[guohai]

Both fixed.

1. The guard tested existence, not usability. Correct, and it is the right distinction: parseAevalResults does not commit to a disk source just because the file is there — a structureless metrics.json or a report.json without latency both fall through to parseAevalStdout, so my existsSync check waved through precisely the case the guard exists for. It now records whether the parse fell through to stdout and gates on that. Existence was never the question.

2. The Dockerfile "itself import-free" comment. Fixed, and the hazard you name is exactly right — that COPY is load-bearing, and the comment would have told a future editor it was not, in the same paragraph that argues a partial --external list "reads as authoritative."

Verification: both images rebuilt (broker broker loads OK; daemon 186.8 kb, node --check passes), tsc clean. Local gate 1653/1654, the single failure being the known practical-shared-agents-credits contention that passes in isolation (#134).

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

Reviewed the full diff (3523859...b65ea98) across Core, the broker sidecar, the daemon, and the Dockerfile. Static review only — tsc and vitest both needed approval I don't have, so I read rather than ran.

Overall this is solid, and unusually well-reasoned. The security-relevant parts hold up under adversarial reading:

• The stdout quarantine actually closes. selectDiagnosisSource returns '' unless stderr lacks a diagnosis and stdout carries a strict loguru-shaped one, and it hands the summarizer only the matching lines. When stdout is admitted, stderr is diagnosis-free by construction, so errors in summarizeAevalFailure can only come from the filtered lines — the tail path can never fall back onto raw stdout.
• Redact-before-truncate is consistent everywhere it matters (describeMintFailure, mintViaBroker, summarizeAevalFailure).
• minNeedleLength: 0 is genuinely safe here because scrubbing happens after line classification — the "password is E" reasoning is right.
• The isOwnerOperatedAgent split preserves isSessionServable's semantics exactly, and gating lastError on it (marketplace agents get the status only) is the correct boundary.
• createBoundedCapture never yields text beginning mid-line; complete only ever grows by terminator-ended segments, and evictOldest cuts at terminators. LINE_TERMINATORS and the /m regexes agree, and the dropping state correctly suppresses an abandoned line's continuation across chunks.
• Dockerfile: dropping the builtin --external flags is correct (--platform=node externalizes all builtins), and the /shared/*.ts COPY + extensionless import resolves the same way shared/secrets.ts already does.

Real findings, in order:

1. The mint timeout has a fourth reader that wasn't unified — vox-agentd.ts:531

shared/mint-timeout.ts claims "one definition for all three readers," but fetchSession has its own hard-coded Date.now() + 240_000 poll deadline. With the defaults (broker 180s, Core abort 195s, daemon 240s) the ordering is fine. But MAX_MINT_TIMEOUT_SECONDS is 3600, so any WEB_SESSION_MINT_TIMEOUT_SECONDS above ~225 inverts it: the daemon gives up at 240s with target login failed: timed out waiting for session mint while Core and the broker are still legitimately working, and the diagnosis this PR exists to surface never reaches the job error. Deriving the daemon's deadline from the same value (it's a config the agent can't read, so it'd need to come down on the 202 response, or just be documented as a ceiling) would close the loop.

2. CAPTURE_LIMIT's docstring understates the actual bound by 3× — aeval-output.ts

The comment says "Max characters retained per captured stream," but the eviction hysteresis lets complete reach 2 * limit before trimming, plus up to limit in partial — so ~3 MiB per stream, ~6 MiB per in-flight mint or run. The evictOldest and no-match-branch comments both state this correctly; only the constant's own docstring disagrees. Worth fixing, since /mint has no concurrency cap and this is the number someone will size the sidecar against.

3. Comment inaccuracy in staleMintThresholdSeconds — auth-session.ts:140

"+30s of headroom over the abort" — the abort is mintTimeoutSeconds() + 15, so the headroom over the abort is 15s, not 30. The value is still correct (30 > 15); only the justification is off. In a change whose whole thesis is that these numbers must not drift, this is the comment most likely to be trusted verbatim later.

Nits, take or leave:

• vox-agentd.ts:840,845 — the raw console.log(\[aeval] ${data.toString().trim()}`)lines still write unredacted child output to the agent's container log.resolveSecrets` substitutes decrypted values into the scenario YAML, so this is the one place on the daemon path where a live credential can still land in a log. Pre-existing and out of scope, but it's the same stream the PR now carefully scrubs before persisting; worth a follow-up issue alongside Daemon's aeval failure summary lacks the URL-encoded credential forms the broker now redacts #138.
• vox-agentd.ts:938 — the guard message says "console output exceeded the capture limit," but truncated is also set when a single overlong line is dropped, even on otherwise small output. Slightly misleading in the job error.
• vox-agentd.ts:869 — the daemon never calls outDec.end() / errDec.end() on the close path (the broker's capturedFailure does). Effect is dropping a trailing incomplete UTF-8 sequence, which is the safe direction, but it's an asymmetry with the broker for no stated reason.
• credentialForms output is fed to redactValues, which sorts a .filter() result — no caller-array mutation. Confirmed clean; noting only because the longest-first ordering is load-bearing and easy to break later.

[guohai]

All three fixed in 152aba5.

1. A fourth reader of the mint deadline. The best catch of this round — shared/mint-timeout.ts literally claimed "one definition for all three readers" while the daemon's 240s poll sat outside it, and my own MAX_MINT_TIMEOUT_SECONDS = 3600 is what made the inversion reachable.

Since the daemon runs on another host and cannot read Core's env, I made the ordering structural instead of documented: the ceiling is now 200, so the chain is 200 (broker child) < 215 (Core abort) < 230 (stale reclaim) < 240 (agent poll) for every permitted configuration. Both ends carry the constraint, including that raising one means raising the other. An operator can no longer produce the failure you describe.

2. CAPTURE_LIMIT understated 3×. Right, and it matters because it is the number someone sizes a sidecar against. It is documented as a retention target now, with the ~3x/stream and ~6x/run worst case spelled out.

3. staleMintThresholdSeconds arithmetic. Correct — 15s of headroom beyond the abort, not 30. The value was fine; the justification was wrong, which in this change is the more dangerous of the two.

Nits: guard message now says output "was discarded (capture limit, or a single overlong line)" since truncated covers both; the daemon flushes its decoders on the close path, matching the broker. Your first nit — the raw console.log of child output, the one place a live credential can still hit the agent's container log — is real and I filed it on #138 rather than widening this further; the needles are already in scope there, so it is a redactValues call at each log site.

Verification: both images rebuilt (broker 13.8 kb broker loads OK; daemon 186.9 kb node --check passes), tsc clean. Local gate 1650/1654, all four agora-e2e's ConvoAI orphan (#136, not on this branch).

Generated with SMT smt@agora.io

[github-actions]

🤖 Claude Code Review

I reviewed the diff against the merge base (3523859..HEAD). Note: npm run check and vitest were blocked by the sandbox in this session, so this is a static review — the type-check/test gate still needs to run in CI.

Overall this is careful, well-reasoned work. The core fix (prefer loguru ERROR lines over aeval's trailing artifacts banner, redact-before-truncate, bound the previously-unbounded mintViaBroker fetch) is correct, and the permissions split is behavior-preserving for isSessionServable — isOwnerOperatedAgent only gates the new failure detail, which is the right boundary. I traced createBoundedCapture through the edge cases that matter (overlong line spanning chunks, \r\n split across a pipe read, bare-\r writers, eviction hysteresis, dropping reset) and the state machine holds; the LINE_TERMINATORS / /m-anchor alignment between the split and the regexes is a real bug class that's now closed. Dockerfile --external removal is correct (--platform=node externalizes builtins), and the new shared/ COPYs resolve for both the broker and eval-agent stages.

Findings, roughly by significance:

1. MAX_MINT_TIMEOUT_SECONDS = 200 silently clamps operator config (shared/mint-timeout.ts:47)

An operator setting WEB_SESSION_MINT_TIMEOUT_SECONDS=600 gets 200 with no signal anywhere — the config just doesn't do what it says. The clamp itself is well-argued (it keeps the four deadlines ordered), but it deserves a one-time console.warn on the clamp path, and CLAUDE.md still documents the var as "default 180" with no mention of a ceiling. Both env-var entries in CLAUDE.md should say ≤ 200.

2. Pre-flush snapshot, and inconsistent text vs completeText within one handler (vox_eval_agentd/vox-agentd.ts:876-887, 896-910)

allOutput is captured at line 876, before outDec.end()/errDec.end() are flushed at 886-887, and that stale snapshot is what's later handed to parseAevalResults. Moving the two flush lines above line 876 is free and makes it consistent.

Separately, timeoutReason() reads completeText on the grounds that "the child is still mid-write" — but this is the close handler, which fires after both stdio streams have ended, so nothing is mid-write. The non-timeout branch one line below correctly uses outCap.text. As written the timeout branch can silently drop a legitimately-final unterminated diagnosis line, for a hazard that doesn't exist on this path. (The rationale is correct on the broker's setTimeout path — that one really is concurrent with the child.)

3. Core's re-redaction is credential-only, not URL-reduced (server/broker-registry.ts:~110)

The comment argues the case for not trusting the broker's scrub ("a stale or buggy broker echoing a credential must not become a leak here") and then applies only half of the broker's sanitization. reduceUrlsSafely/reduceUrlsToHost — the half that strips ?code=, ?access_token=, magic-link path tokens — isn't repeated. If the premise is a buggy broker, that's exactly the material a buggy broker would forward. A reduceUrlsToHost(detail) before the credential scrub would make the backstop match its stated rationale, and the helper is already importable from the daemon module the broker shares.

4. Behavior change worth validating before merge: bounded capture can newly fail jobs that used to complete

The lastResultsFromStdout && truncated guard (vox-agentd.ts:~940) is the right call in principle — deriving latencies from a phase state machine that resumed mid-run is silently wrong data. But the trade only lands well if 1 MiB is comfortably above real run volumes; CAPTURE_LIMIT's own comment notes a multi-minute voice eval "emits well past 64 KiB", and the ceiling isn't obviously an order of magnitude clear of that. Two consequences to confirm against production output sizes: (a) any run over the cap now hard-fails when metrics.json is unusable, where it previously reported numbers; (b) resolveAevalOutputDir's primary "Session directory:" path is head-of-stream while the capture keeps the tail, so on those runs it's permanently the fallback. Both are acknowledged in comments — I'd just want one real chatty run measured rather than reasoned about.

5. Low: JSON.parse of the storage file is outside the summarization pipeline (auth-session-broker.ts:261)

JSON.parse(fs.readFileSync(storageFile, 'utf8')) throws a V8 SyntaxError that embeds a snippet of the input (Unexpected token 'x', "..." is not valid JSON). On a truncated or corrupt storageState that puts a cookie fragment into the 502 body, Core's log, and web_sessions.last_error. The recipient is owner-tier only (who'd receive the storageState anyway), so impact is small — but wrapping it to throw a fixed message costs nothing and keeps the log clean.

6. Nit: urlForms models percent-encoding but not HTML entity encoding. Given the boundary note explicitly accepts that DOM fragments reach the message, an email quoted out of a page dump is a plausible @/attribute-escaped spelling that no needle covers. Low likelihood, but the module's stated goal is "every spelling a credential can take", so it's worth either adding or explicitly listing as out of scope alongside the #138 note.

One process note: the comment density in aeval-output.ts and auth-session-broker.ts is unusually high even for this codebase — several blocks are longer than the code they explain and restate the same argument (redact-before-truncate, strict-vs-lenient admission) in three or four places. That's a judgment call, not a defect, but the duplicated rationale is itself a drift risk of the kind the PR is trying to eliminate; consolidating each argument to one canonical spot with pointers would age better.