fix: surface the real cause of a missing-secret run instead of a PyInstaller banner

[guohai]

The problem

A workflow referencing an unconfigured ${secrets.X} produced this on the job record:

aeval exited with code 1: [PYI-3739:ERROR] Failed to execute script 'pyi_entrypoint' due to unhandled exception!

A Python packaging artifact. Meanwhile the agent's own log said exactly what was wrong:

ERROR | Unknown variable source: secrets
ERROR | Step 1 failed: platform.setup - Unknown variable source: secrets

Reported from prod job #31006 (a cloned workflow whose secrets didn't come with it). The truth existed at four points and each one dropped it:

1. Dispatch — Core computed referencedSecrets with present: false and ignored it; server and client both only ever filtered on present === true.
2. Substitution — resolveSecrets logged Secret placeholder ${secrets.X} not found to the agent's local console and returned the placeholder verbatim. That line never reaches the job or the UI.
3. aeval — received a literal ${secrets.X} (hence "Unknown variable source", not "secret missing"), aborted, exit 1.
4. Daemon — kept stderr.split('\n').pop(). For a PyInstaller binary the banner is printed after the traceback, so the one line retained was structurally guaranteed to be the least informative.

The fix

A — reject at dispatch, naming the secrets. New missingSecretNames() helper, enforced at all four job-minting entry points: run route, v1 run, schedule create, schedule run-now. Scope is the workflow owner's, matching what the job-secrets endpoint resolves at claim time. Schedules are included deliberately — otherwise they emit a doomed job every tick.

B — daemon fails fast on an unresolved placeholder, before aeval is spawned. Deliberately a post-substitution scan rather than instrumentation inside resolveSecrets, because substitution is skipped entirely when the secrets map is empty (if (Object.keys(jobSecrets).length > 0)) — exactly the reported case. Also catches a secret deleted between dispatch and claim, which A can't.

C — summarizeAevalFailure() replaces the last-line heuristic: prefers aeval's own loguru ERROR lines (scanning both streams, since its sink may be either), strips the timestamp | LEVEL | prefix, falls back to a noise-filtered tail, and caps length so one bad run can't bloat the error column. Lives in shared/ so tests can import it without pulling in the daemon entrypoint (which runs main() and would call process.exit).

D — client shows it and disables Start. This also fixes a latent bug the work uncovered: both dialogs typed referencedSecrets with class: "runtime" | "protected" while the server sends brokerType (renamed when the class split became the broker-type column). So s.class === "runtime" was undefined === "runtime" — always false. Consequence: the runtime-secret exposure warning and its consent checkbox have never rendered, and runtimeSecretConsent was never sent — making a shared-tier dispatch of any workflow with runtime secrets unsatisfiable from the UI, since the server hard-requires that flag. Latent today only because the marketplace plugin isn't live.

Known behavior change

collectSecretRefs JSON-stringifies the entire config, while the daemon only substitutes into scenario/app/stepsPrefix/stepsSuffix. A stray ${secrets.X} in a non-substituted field will now block a run that previously succeeded. Accepted: the same superset already gates the existing Brokered-misuse 400, and the message stays actionable either way.

Test plan

• Dispatch gate: run route and schedule create return 400 naming the missing secret, no job created. Fixture mirrors the real failure — the reference lives in the workflow's stepsPrefix (i.e. platform.setup), which is where #31006's was.
• summarizeAevalFailure: recovers Unknown variable source: secrets from the reported log verbatim, never returns the pyi_entrypoint banner, works when the ERROR lines are on either stream, falls back to a filtered tail, never returns empty, and respects the length cap. These would fail against the old heuristic by construction.
• npm run check clean; 455 passing across tier-pool-dispatch, eval-agent-daemon, session-dispatch, api.test, secrets, secrets-class-api.

Not covered (deliberately)

The origin of the confusion — cloning copies ${secrets.X} references but not values, since secrets are per-owner — is untouched. Surfacing required-vs-missing secrets on the workflow itself (badge, or a note at clone time) is the higher-leverage UX fix and belongs in its own change. Separately, C is a heuristic over log text; the durable answer is aeval emitting a structured failure reason, which is an upstream ask.

Generated with SMT smt@agora.io

[github-actions]

🤖 Codex Code Review

Findings

• server/index.ts:431 — The automatic scheduler path still does not re-check missingSecretNames before stamping and creating the scheduled job. Schedule create and “run now” now reject missing secrets, but if a secret is deleted after a recurring schedule is created, processScheduledJobs() will continue minting jobs every tick; the daemon will fail-fast each one, but the PR’s intended “don’t emit doomed jobs every tick” behavior is not achieved. Add the same owner-scope missing-secret check here before stampOwnerSession() / createEvalJob() and disable or skip the schedule with a clear log message.

Notes

• I didn’t see security regressions around secret values being exposed; the new server/client messages name secret references only.
• The aeval stderr/stdout summarizer looks reasonable and covered by focused tests.

[github-actions]

🤖 Claude Code Review

Summary

The core idea is right — the PyInstaller-banner heuristic really was structurally guaranteed to pick the least useful line, and gating at dispatch beats burning an agent run. summarizeAevalFailure is clean and well-tested, and the server-side gates are placed correctly (before job creation / escrow hold, and reusing the already-computed classified in the run route rather than re-querying). One blocker below.

Blocker

vox_eval_agentd/vox-agentd.ts:1874 — the fail-fast check breaks every session-injected (brokered-login) job.

For a workflow that needs a Core-minted session, stepsPrefix must contain params.email: ${secrets.X} / params.password: ${secrets.Y} — that's exactly how parsePlatformSetup (server/auth-session.ts:44) detects the need. Those secrets are brokerType != null, so they are structurally withheld from /jobs/:id/secrets (server/storage.ts:2154, :2622). Core never rewrites the job's stepsPrefix; stampOwnerSession only adds sessionInjection. So after resolveSecrets, those two placeholders survive by design — and the strip that removes them (injectStorageSession, which deletes params.email / params.password) runs at line ~1897, after the new throw at 1874.

Net effect: every auth-broker workflow now fails at claim time with Unresolved secret placeholder(s): LOGIN_EMAIL, LOGIN_PASSWORD ... create it under Console → Secrets — for secrets that exist and are correctly configured. Core's dispatch gate won't catch it either (those rows are present: true), so it's a hard runtime regression on the whole session-injection path.

Fix: move the scan below the session-injection block (after injectStorageSession has stripped the credential params), or when job.config.sessionInjection is set, exclude the snapshot's emailSecret/passwordSecret names from unresolved. Worth an added test: session-injected job with unresolvable login placeholders must still run.

Other findings

Scheduler tick isn't gated (server/index.ts:~396-443). The comment on the schedule-create gate says an unconfigured secret "would emit a doomed job on every tick" — but only creation (routes.ts:2186) and run-now (:2440) check. A schedule created while the secret existed keeps emitting doomed jobs after the secret is deleted, which is precisely the case the run-now re-check was added for. The tick already re-derives canScheduleWorkflow and detectSessionNeed per iteration, so adding missingSecretNames there is consistent (and could auto-disable, like the orphaned-schedule path).

Message is wrong for the public-org-workflow / non-member runner case. missingSecretNames resolves against the workflow-owner scope, but getOrgSecretsForJob fences on the job creator's org membership and deliberately returns {} for a non-member. That run passes the Core gate, then hits the daemon throw claiming the secret is "not configured for the workflow owner" — it is; the runner just isn't entitled to it. Consider softening the daemon-side wording to cover both causes (not configured or not available to this run).

Nit — server/auth-session.ts:258-285: missingSecretNames was inserted between classifyReferencedSecrets's JSDoc and its declaration, so it now carries two stacked doc blocks and classifyReferencedSecrets is left undocumented. Move the "Join referenced secret NAMES..." block back down.

Low — error-string surface. summarizeAevalFailure now stores up to 3 aeval lines (or a traceback tail) in the job's error field, where previously effectively only the PYI banner landed. A YAML/parse error tail can echo a line that already had a secret substituted into it. The blast radius is small (the secrets belong to the workflow owner, and the agent operator sees the raw streams anyway), but if job errors are ever surfaced on a public/community job view it's worth a redaction pass over the resolved secret values before storing.

The client changes (class → brokerType == null rename, missing-secret alert, run-button disable) match the server response shape at routes.ts:4279 and look correct.

[github-actions]

🤖 Codex Code Review

Findings

• High: vox_eval_agentd/vox-agentd.ts:1874 runs the unresolved-secret scan before session injection rewrites stepsPrefix. For session-injected workflows, brokered login secrets are intentionally withheld from /jobs/:id/secrets, so ${secrets.EMAIL} / ${secrets.PASSWORD} remain in stepsPrefix until injectStorageSession() strips them at vox_eval_agentd/vox-agentd.ts:1897. This means valid brokered/session workflows now fail before the injection path runs. Move this scan after session injection, or exclude the stamped login placeholders that will be stripped.
• Medium: server/index.ts:431 still lets the background scheduler mint jobs after a scheduled workflow/eval set starts referencing a missing secret, or after a secret is deleted. The PR adds checks for schedule creation and run-now, but the recurring tick path goes straight from session checks/stamping to createEvalJob at server/index.ts:443. That leaves recurring schedules emitting a failed job every tick; the daemon error is clearer now, but the schedule is not stopped. Add the same missingSecretNames(sessionScopeForWorkflow(workflow), [workflow.config, evalSet.config]) gate here, likely disabling the schedule as with other runtime-invalid schedule conditions.

No other real issues found in the reviewed diff.

[github-actions]

🤖 Claude Code Review

Findings

1. Blocker — the daemon's unresolved-placeholder check breaks every session-injected (brokered login) job

vox_eval_agentd/vox-agentd.ts:1874 runs collectSecretRefs([scenario, app, stepsPrefix, stepsSuffix]) before the session-injection block at :1888.

For a brokered workflow, stepsPrefix legitimately contains platform.setup with email: ${secrets.LOGIN_EMAIL} / password: ${secrets.LOGIN_PASSWORD} — and those secrets are deliberately withheld from /jobs/:id/secrets (server/storage.ts:2154 filter(s => s.brokerType == null), :2622 if (s.brokerType != null) continue). So jobSecrets never contains them, the placeholders survive substitution by design, and injectStorageSession() is what strips those params a few lines later — its own module doc says exactly that ("this transform also strips any email/password params so leftover placeholders can't reach temp YAML or logs").

Net effect: the new check throws for every auth-session job, failJob() records it, and the message is actively wrong — it tells the owner the secret "is not configured" when it is configured and Core intentionally withheld it. Core's dispatch-side gates are unaffected (a brokered secret has present: true), so this only shows up at claim time, after the job is running.

Fix: move the scan below the injectStorageSession rewrite, or exclude the snapshot's sessionInjection login pair from unresolved. Worth a daemon test with a platform.setup + sessionInjection job.

The same conflation hits a second, milder case: an org workflow run by a non-member gets no org secrets by design (getOrgSecretsForJob fences on the creator's membership). That run should still fail, but the message "not configured for the workflow owner" misattributes the cause.

2. The recurring scheduler tick isn't gated

The create-time check (server/routes.ts:2186) and the run-now re-check (:2440, correctly noting secrets can be deleted afterwards) don't cover the actual tick in server/index.ts:~437. A secret deleted after schedule creation → a doomed job emitted every tick, indefinitely. Every other guaranteed-failure condition in that loop (deleted workflow/eval-set, lost schedule authorization, pool violation, split-class pair) disables the schedule; missing secrets should do the same, and it's the loop where "doomed job on every tick" — the create-route comment's own justification — actually applies.

3. Low — summarizeAevalFailure widens what gets persisted into the job error

It now scans stdout as well as stderr and keeps up to 3 lines / 500 chars, versus one stderr line before. aeval's ERROR lines can echo resolved config. That string is stored on the job and visible to the runner, who for a public workflow is not the secret owner. Cheap mitigation: redact the values in jobSecrets from the summary before recording it (the daemon has them in hand at that point).

4. Nits

• server/routes-api-v1.ts:370 duplicates the error copy instead of reusing MISSING_SECRETS_MSG, and recomputes sessionScopeForWorkflow(workflow) though scope is already bound at :351.

Looks right

• The client class → brokerType type fix is a real correctness fix, not just typing: s.class === "runtime" never matched the server's response shape, so runtimeExposed was always empty and the shared-tier runtime-secret consent warning was dead client-side (the server-side gate still held).
• Gate placement in the run route (server/routes.ts:4028) is before the targeting branch and therefore before authorizeDispatch's credit hold — correct, no escrow burned on a rejected run.
• missingSecretNames resolving against sessionScopeForWorkflow matches both the run-targets endpoint and the claim-time secrets endpoint, so client, dispatch, and execution agree on scope.
• Splitting aeval-output.ts out of the daemon entrypoint for testability, and the COPY line in the Dockerfile, are both right.

[github-actions]

🤖 Claude Code Review

Summary

Solid, well-motivated fix. The diagnosis (PyInstaller's banner is structurally always the last stderr line, so "last line of stderr" was guaranteed to be the least useful one) is correct, the daemon-side scan is placed correctly relative to session injection (and the new session-inject.test.ts case pins exactly the invariant that makes the ordering safe), the Core-side gates land before any escrow/authorizeDispatch on the run route, and the aeval-output.ts COPY is in the right Dockerfile stage (daemon, not broker).

Worth calling out: the class: "runtime" | "protected" → brokerType client type fix isn't cosmetic. The server's classifyReferencedSecrets has returned brokerType for a while, so s.class was always undefined — meaning runtimeExposed was always empty and showRuntimeWarning (the shared-dispatch runtime-secret consent alert + ackRuntime gate) was dead code on both pages. This PR silently revives it. That's good, but it's an untested behavior change riding along in a "better error message" PR — worth confirming the ack flow still works end-to-end before merge, since nobody has exercised it.

Findings

1. summarizeAevalFailure widens the credential-exposure surface of job error messages (medium)

vox_eval_agentd/aeval-output.ts:29-42 now returns up to 3 ERROR lines (500 chars) scanned across both streams, and this string becomes the job's stored errorMessage. resolveSecrets (vox-agentd.ts:549) substitutes decrypted secret values into the YAML handed to aeval — so any aeval ERROR line that echoes step params/config carries a live credential into a persisted, user-visible field. The old path had the same theoretical risk but in practice never fired (the banner was always last). The daemon has jobSecrets in hand at executeJob; thread the values down and redact them from the summary before fail().

2. "First 3 ERROR lines" can report a transient error instead of the fatal one (medium)

aeval-output.ts:36-39 assumes the first ERROR is the diagnosis and later ones are cascades. That holds for the #31006 trace, but if aeval logs a retried/recoverable failure at ERROR earlier in a longer run, the summary reports that and buries the actual cause — the same failure mode this PR is fixing, mirrored. Consider anchoring to the last error block (or lines after the final Test N marker) rather than the first.

3. The Core gate and the runtime secrets path disagree for public org workflows (medium)

missingSecretNames resolves against sessionScopeForWorkflow(workflow) (org scope), but getOrgSecretsForJob (server/storage.ts:2616-2618) additionally fences on the job creator's org membership. A non-member running a public org workflow passes the new gate (rows exist) and then trips the daemon's scan, which tells them the secret "is not configured for the workflow owner — create it under Console → Secrets." That's actively wrong advice for a case the daemon can't distinguish. Either mirror the membership fence in the gate, or make the daemon message scope-agnostic ("the server did not supply this secret for this job").

4. Brokered-secret misuse produces the same wrong message on non-console paths (low-medium)

findBrokeredMisuse only runs on the console run route. On routes-api-v1.ts and the scheduler, a brokered secret referenced outside platform.setup has present: true (gate passes), is structurally withheld at claim time, and lands on the daemon's new scan with "not configured … create it under Console → Secrets" — for a secret that exists. Lifting findBrokeredMisuse into the shared path alongside missingSecretNames would close this.

5. Scheduler disables permanently on a transient window (low-medium)

server/index.ts:431-445 disables the schedule outright. Secret rotation done as delete-then-recreate has a window where a tick lands in between and silently kills a recurring schedule, with only a log() line — no user-visible reason. It matches the existing misconfigured/pool precedent, so it's defensible, but those conditions aren't self-healing and this one is. Consider a tolerance (skip N ticks) or persisting the disable reason where the user can see it.

6. Copy assumes the runner is the workflow owner (low)

MISSING_SECRETS_MSG ("secret(s) … that you have not configured") and both client alerts ("Create them under Console → Secrets") are wrong when running someone else's public workflow — the scope is the owner's, and the runner cannot fix it. The Run button is now hard-disabled in that case (missingSecrets.length > 0 in the disabled expression), which is a real behavior change for community workflows; make sure the copy matches.

Nits

• server/auth-session.ts:264-283: missingSecretNames was inserted between classifyReferencedSecrets's doc comment and its declaration. missingSecretNames now carries two stacked doc blocks and classifyReferencedSecrets has none.
• server/routes-api-v1.ts:370: recomputes sessionScopeForWorkflow(workflow) when scope is already bound a few lines above.
• missingSecretNames issues its own full-scope secrets read; routes.ts reuses the existing classified (good), but api-v1 / schedule-create / run-now / scheduler each add one redundant read. Small, but easy to pass classified through.
• vox_eval_agentd/vox-agentd.ts:213-215: stray double blank line.
• docs/openapi.yaml isn't updated for the new 400 on POST /api/v1/workflows/:id/run.

Test coverage

The regression tests are good but only cover the console run route and schedule-create. The api-v1 gate, the run-now re-check, and the scheduler-tick disable are all untested. The scheduler one is the highest-value gap given finding #5.

[github-actions]

🤖 Claude Code Review

The change is well-structured: the dispatch gate lands before authorizeDispatch/escrow on the run path, the daemon's post-substitution scan is correctly ordered after session injection (with tests/session-inject.test.ts pinning the invariant that makes that ordering safe), the Dockerfile COPY for the new module is present and esbuild bundles it, and test coverage is genuinely targeted at the reported regression. A few real issues:

1. Multi-line secret values escape redaction (vox_eval_agentd/aeval-output.ts:31-47)

scrub runs on text that has already been split('\n') and trim()ed. A decrypted secret whose value contains a newline — a PEM key, a service-account JSON blob, a multi-line token — is never matched by acc.split(value).join('[redacted]'), because no single line contains the whole value. Fragments of it then land in the job's persisted, user-visible error string. The whole point of scrub is that this string is persisted and rendered in the console.

Scrub before splitting, or normalize each redact value:

const scrub = (text: string) =>
  redact.flatMap((v) => (v ?? '').split('\n'))
    .map((v) => v.trim())
    .reduce((acc, v) => (v.length >= 4 ? acc.split(v).join('[redacted]') : acc), text);

2. activeSecretValues is assigned outside the try whose finally clears it (vox_eval_agentd/vox-agentd.ts:1858)

const jobSecrets = await this.fetchSecrets(job.id);
this.activeSecretValues = Object.values(jobSecrets);   // <- outside
if (...) { scenario = this.resolveSecrets(...); ... }  // can throw
const tempFiles = [];
try { ... } finally { this.activeSecretValues = []; }

If resolveSecrets throws (or anything else between the assignment and the try), decrypted values stay resident on the daemon instance indefinitely — until the next job overwrites them. The comment says "don't retain decrypted values past the job," but the placement doesn't deliver that. Move the assignment inside the try, or wrap from fetchSecrets onward.

3. Server gate over-approximates what the daemon actually resolves

missingSecretNames scans JSON.stringify(workflow.config) + the eval-set config — the entire config object. The daemon only substitutes into scenario, app, stepsPrefix, stepsSuffix. A ${secrets.X} sitting in any other key (a description, a legacy/unused field, a commented-out region of a YAML string) previously ran fine and now:

• hard-400s the run and schedule-create routes,
• disables the Run button in both dialogs with no override (unlike the runtime-secret warning, which has an ack checkbox),
• and most consequentially, permanently disables an existing schedule on the next tick (server/index.ts:432-441), silently — evalSchedules has no lastError column, so the only trace is a server log.

findBrokeredMisuse already uses the same over-approximate collection, so there's precedent, but it only fires on brokered secrets, whereas this fires on any dangling name. Consider narrowing the scan to the four fields the daemon resolves (the daemon's own scan already uses exactly those), which would also make the client gate and the server gate agree by construction.

4. Minor: 4-char redaction floor is low

value.length >= 4 means a secret whose value happens to be 8080, prod, or true will shred every occurrence of that substring in the diagnosis. 8 would be safer with essentially no loss of coverage.

5. Minor: extra full-secrets fetch per due schedule per tick

missingSecretNames → classifyReferencedSecrets → getSecretsByUserId/getOrgSecrets fetches all rows in scope. The run route correctly reuses its existing classified array (no extra query), but the scheduler tick, schedule-create, run-now and v1 each add one. In the scheduler that's one more round-trip per due schedule per tick, on top of the several detectSessionNeed/stampOwnerSession already does. Not urgent at current scale; worth a shared per-tick cache if schedule counts grow.

Nits (no action needed)

• summarizeAevalFailure takes the last 3 | ERROR | lines and falls back to the tail; a fatal Python traceback with no loguru prefix loses to an earlier recoverable loguru ERROR. Acknowledged in the comment and an acceptable tradeoff.
• routes-api-v1.ts:373 uses its own copy ("Create them…") rather than MISSING_SECRETS_MSG; correct there since that route is owner-only, but the divergence is easy to miss if ownership rules loosen.

[github-actions]

🤖 Codex Code Review

Findings

• server/auth-session.ts:277 misses secrets introduced through ${config.*} indirection. The daemon expands config vars before resolving secrets (vox_eval_agentd/vox-agentd.ts:1849), so a config like stepsPrefix: "...${config.LOGIN}..." plus LOGIN: "${secrets.MISSING}" will still create a job/schedule because resolvableSecretSources() only scans the raw stepsPrefix/app/scenario strings. The daemon then fails fast at claim time, and recurring schedules can keep emitting doomed jobs. The dispatch gate should evaluate the same effective strings the daemon will resolve, or share that expansion logic.

• server/routes.ts:4288 returns referencedSecrets for the entire config, while the new client blocker treats every missing entry as fatal (client/src/pages/console-workflow-detail.tsx:140, client/src/pages/run-your-own.tsx:235). Since the server-side run gate was narrowed to only daemon-resolved fields, a missing placeholder in a non-resolved custom/metadata config field can now disable Start in the UI even though the run route would allow and the daemon would not substitute it. The run-targets payload should either expose fatal/resolvable missing refs separately or the client should filter with the same effective-resolvable set as dispatch.

No other security or logic issues stood out in the reviewed diff.

[github-actions]

🤖 Claude Code Review

Summary

Solid, well-reasoned change. The core fix — replacing "last line of stderr" with a loguru-ERROR-preferring summarizer, plus a fail-fast unresolved-placeholder check in the daemon and matching dispatch gates in Core — is correct, and the placement decisions (scan after injectStorageSession; narrow the server gate to the four fields the daemon actually resolves) are right and well-documented. Test coverage for the new helper is good, including the redaction and last-error-wins cases. aeval-output.ts is correctly added to the Dockerfile (that's the only build manifest that lists daemon modules).

Findings below.

---

1. Client hard-blocks runs the server would accept (client/server gate divergence)

client/src/pages/console-workflow-detail.tsx:139 and client/src/pages/run-your-own.tsx:234 compute:

const missingSecrets = (runTargets?.referencedSecrets ?? []).filter((s) => !s.present).map((s) => s.name);

…and then disabled={... || missingSecrets.length > 0 ...}.

But referencedSecrets from /run-targets (server/routes.ts:4288) is built from collectSecretRefs([workflow.config, evalSet.config]) — the full configs. The server-side gate deliberately narrows to resolvableSecretSources() (scenario/app/stepsPrefix/stepsSuffix), for exactly the reason stated in server/auth-session.ts: "Gating on anything wider would reject runs that work today."

So a ${secrets.X} sitting in any other config key (e.g. an unused config.url variant, a notes field) that the daemon never resolves now produces a permanently disabled Run button with no override — while POST /run would happily accept it. Either narrow the endpoint's referencedSecrets (or add a resolvable flag per entry) so the UI filters on the same set, or downgrade the client to a warning rather than a hard disable.

Related, smaller: on the console page the eval-set config is only included in referencedSecrets when evalSetIdRaw is supplied and visible, so an eval-set-only missing secret won't show the banner — the user still gets the 400. Fine, but the banner is then inconsistent with the button's outcome.

2. Re-enabling a schedule silently flips back to disabled

PATCH /api/eval-schedules/:id (enable / re-cron) didn't get the gate that create and run-now got. An owner who re-enables a schedule with a missing secret gets a 200, and the next scheduler tick (server/index.ts:431) disables it again — the reason only lands in server logs. Adding the same missingSecretNames check to the enable path would close the loop cheaply.

3. Redaction floor leaves short secret values in persisted job errors

vox_eval_agentd/aeval-output.ts:44 — .filter((v) => v.length >= 8). The tradeoff is documented and reasonable, but the consequence is that a user-configured secret shorter than 8 characters is never scrubbed from the job error string that Core persists and renders in the console. A tighter rule (e.g. drop the floor to ~4 and require a word-boundary match, or skip only values that are purely numeric/common words) would keep the "don't shred the message" property without leaving short credentials in the clear.

Also note redaction is scoped to the aeval-failure path only; the timeout branch and other thrown errors aren't scrubbed. That's fine today (they don't echo config), but it's an easy invariant to break later — worth scrubbing at the point the error is reported to Core instead.

4. Minor

• server/auth-session.ts:258-275 — two stacked JSDoc blocks: the first (Names of ${secrets.X} placeholders…) documents missingSecretNames but is attached to resolvableSecretSources, and missingSecretNames itself is undocumented. Move it down.
• server/routes-api-v1.ts:373 uses an inline message ("…not configured. Create them…") rather than MISSING_SECRETS_MSG. That route is owner-only so the wording is accurate, but two copies of the same string will drift; consider exporting the constant with an owner/non-owner variant.
• The scheduler tick now does one extra secrets query per schedule per tick (on top of the existing detectSessionNeed → getBrokeredSecretNames). Both resolve against the same scope; passing the already-fetched rows through would halve it. Low priority at current scale.

No auth-bypass, injection, or credential-exposure issues found in the new code paths — the brokered-secret withholding invariant is preserved, and the daemon's scan placement relative to injectStorageSession is correct (pinned by the new session-inject.test.ts case).

[github-actions]

🤖 Codex Code Review

Findings

• High: vox_eval_agentd/aeval-output.ts:45 leaves short secret values unredacted. Secret values have no minimum length, and this PR now persists selected ERROR lines from both streams into the user-visible job error. If aeval echoes a short password/PIN/token such as hunter2 or a 6-digit code, it will be stored and shown because values under 8 chars are filtered out. This is a credential exposure regression from the new summarizer path; redact all known secret values or avoid returning lines that can contain substituted params.

• Medium: server/auth-session.ts:267 does not mirror the daemon’s actual resolution order. The daemon first expands ${config.*} inside scenario/app/stepsPrefix/stepsSuffix at vox_eval_agentd/vox-agentd.ts:1851, then resolves/scans secrets. A missing secret hidden behind a config indirection, e.g. stepsPrefix: "${config.loginSetup}" with loginSetup: "${secrets.MISSING}", is resolvable by the daemon but invisible to resolvableSecretSources(). Dispatch/UI will allow the run, and the daemon will still fail it later. The server-side gate should apply the same config interpolation before collecting secret refs, or otherwise include config fields referenced by the resolvable fields.

• Medium: server/routes.ts:2313 re-checks only wf.config when re-enabling/changing a schedule. If a schedule’s eval-set config contains the missing secret and that secret is deleted after schedule creation, PATCH /api/eval-schedules/:id with isEnabled: true will still return 200, then the scheduler will disable it on the next tick. This undercuts the intended “no silent flap” behavior; include the schedule’s eval set config in this check, like create/run-now/scheduler do.

Notes

• The main run-route and run-now missing-secret gates otherwise look consistent with the daemon-side fast failure.
• Tests cover the workflow-config case well, but miss the config-indirection and eval-set re-enable edge cases above.

[github-actions]

🤖 Claude Code Review

Overall this is a well-constructed change — the daemon-side scan is correctly ordered after session injection (and that invariant is pinned by a test), the server gates sit ahead of authorizeDispatch/escrow so no credit hold is taken on a doomed run, and the class → brokerType client fix quietly revives showRuntimeWarning, which was dead code (s.class was always undefined against the server's payload). A few real issues:

1. server/routes.ts:2313 — the re-enable gate skips the eval-set config, so the flap it's meant to close still happens

Every other call site passes both configs ([workflow.config, evalSet?.config] — create at :2191, run-now at :2454, scheduler tick at server/index.ts:439). The PATCH path passes [wf.config] only. A secret referenced solely from the eval set's config therefore passes re-enable with a 200 and is silently disabled by the scheduler on the next tick — exactly the behavior the comment above it says it fixes. schedule.evalSetId is in scope; fetch it and include evalSet?.config. The new test happens to put GHOST in the workflow config, so it doesn't catch this.

2. vox_eval_agentd/aeval-output.ts:45 — the 8-char redaction floor lets short secret values through into a persisted, user-visible error

The floor is a reasonable instinct (a value of prod would shred unrelated text), but the failure mode is asymmetric: shredding is cosmetic, a leaked credential in evalJobs.error is not. Short passwords/PINs/account IDs under 8 chars pass through verbatim. Consider dropping the floor to ~4 with a word-boundary-aware replace, or keeping the floor only for values matching ^[a-z]+$-style dictionary words.

Related, and pre-existing but worth knowing given this PR's framing: vox-agentd.ts:834 and :839 console.log/console.error the raw aeval stdout/stderr unredacted, so the container logs retain the credential regardless of what scrub does to the persisted string.

3. server/auth-session.ts:265 — the gate and the daemon don't actually "agree by construction"

executeJob resolves ${config.X} placeholders before ${secrets.X} (vox-agentd.ts:1843-1854), splicing any string config key into scenario/app/steps*. So config.url = "${secrets.API_KEY}" referenced as ${config.url} in the scenario is a reference the daemon really does resolve, but resolvableSecretSources never sees it. The result is fail-safe (the run is accepted, and the daemon's new scan produces a clear error rather than the PyInstaller banner), so this isn't a blocker — but the comment's claim is wrong and the UI will report resolvable: false for a secret that genuinely is. Either fold sibling string keys into the sources or soften the comment.

4. Both the gate and the daemon scan are framework-blind, which can hard-block workflows that run today

aeval never consumes config.app; voice-agent-tester never consumes stepsPrefix/stepsSuffix (vox-agentd.ts:1932-1950). A stale ${secrets.X} sitting in a field the selected framework ignores previously ran fine; it now 400s at dispatch and, if it somehow gets past, throws in the daemon. Narrow, but it's a behavior change for existing configs — worth gating the source list on config.framework.

5. Nit — redundant secret fetch on the scheduler tick

missingSecretNames re-runs getSecretsByUserId/getOrgSecrets, which detectSessionNeed → getBrokeredSecretNames fetched moments earlier for the same scope, once per due schedule per tick. The run route already avoids this by filtering the existing classified; the scheduler could thread the rows through the same way.

[github-actions]

🤖 Codex Code Review

Findings

• High: vox_eval_agentd/aeval-output.ts:47 redacts only the raw secret values, but the daemon writes YAML-escaped values into the aeval input at vox_eval_agentd/vox-agentd.ts:554. If aeval logs the source/config form, values containing quotes/backslashes/control chars can be persisted unredacted, e.g. secret pa"ssword may appear as pa\"ssword, which the current split(rawValue) scrub misses. Since this field is user-visible, add the exact escaped variants produced by resolveSecrets (and ideally JSON/YAML-rendered forms) to the redaction needles.

• Medium: vox_eval_agentd/vox-agentd.ts:1918 scans the post-substitution strings for ${secrets.X}, so a valid secret value that literally contains that pattern is treated as an unresolved placeholder and the job fails. Example: ${secrets.MSG} with value ${secrets.NOT_A_REF} becomes "${secrets.NOT_A_REF}" after substitution and is reported as missing. This should track unresolved placeholders from the pre-substitution/config-expanded input and subtract supplied names, rather than regex-scanning arbitrary substituted secret values.

Notes

• I did not run tests because the environment is read-only with approvals disabled.
• The server-side dispatch gates and UI typing changes otherwise look aligned with the stated fix.

[github-actions]

🤖 Claude Code Review

Reviewed the full diff on the merge ref (0930b17), plus the surrounding code in vox-agentd.ts, session-inject.ts, auth-session.ts, and the three dispatch paths.

Overall this is careful work. The core direction is right, and two things in particular are correct in ways that are easy to get wrong: placing the daemon's unresolved-placeholder scan after injectStorageSession (and pinning that ordering with the new session-inject.test.ts invariant), and scrubbing before the .slice(0, 500) truncation so a cut can't re-expose a fragment. The dispatch gates are all placed after their authz checks. Findings below.

1. resolvableSecretSources assumes aeval when the daemon may not — false-positive blocks and schedule disables

server/auth-session.ts:151

const framework = typeof merged.framework === "string" ? merged.framework : "aeval";

The comment says this "mirrors the daemon's own fallback (config.framework || daemon default aeval)", but the daemon's fallback is config.framework || this.config.framework, where this.config.framework is EVAL_FRAMEWORK || 'aeval' — a documented per-agent env knob (vox_eval_agentd/README.md:163, passed through by scripts/vox-upgrade.sh:203).

For a workflow whose config omits framework, dispatched to an agent running EVAL_FRAMEWORK=voice-agent-tester:

• the server gates on stepsPrefix/stepsSuffix, which that daemon never reads. A stale ${secrets.X} there now 400s the run — and on the next scheduler tick permanently disables the schedule (server/index.ts:409). That is precisely the harm the function's own docblock says must be avoided.
• conversely app isn't gated, but that direction is fail-safe (the daemon's own scan catches it).

Given the module's stated asymmetry, the consistent fix is to narrow to the intersection when framework is absent — push only c.scenario — rather than guessing aeval.

2. missingSecretNames does a full secrets query even when nothing is referenced

classifyReferencedSecrets (server/auth-session.ts:317) unconditionally runs getSecretsByUserId/getOrgSecrets before mapping over names. The new callers make this hot: the scheduler now issues one extra secrets query per due schedule, per tick, including for the common case of a workflow with zero ${secrets.*} placeholders. A if (names.size === 0) return [] short-circuit in classifyReferencedSecrets (or in missingSecretNames) removes it entirely.

3. Scheduler disable is permanent, on a much more transient condition than the cases it mirrors

The existing auto-disables (deleted workflow, lost authorization, pool violation) are structural — they can't self-heal. "Secret row is absent" can be a several-second window if a user rotates a credential by delete-then-create. One tick landing in that window silently and permanently disables a recurring schedule, with only a server log as the signal; the user finds out later. Worth either tolerating N consecutive failures before disabling, or at least confirming this is the intended product tradeoff. (The 400 on re-enable is right and helpful — it's the silent disable that's the sharp edge.)

4. v1 route now scans an eval set the caller may not be able to access

server/routes-api-v1.ts:322 fetches evalSet with no canAccessResource check (pre-existing gap — the console route at server/routes.ts:4006 does check). The new gate scans evalSet.config and echoes the referenced secret names back in the 400. That lets an authenticated caller enumerate secret names referenced by another user's private eval set by pointing their own workflow at an arbitrary evalSetId. Names only, not values, so low severity — but adding the same canAccessResource(user, evalSet) guard the console route has would close both this and the underlying hole.

5. Redaction covers the persisted error but not the agent's own logs

summarizeAevalFailure's scrub is sound (longest-needle-first, multi-line split, redact-then-truncate). But vox-agentd.ts:835,841 still console.log/console.error every raw aeval stdout/stderr chunk verbatim — the same credential-bearing lines the new scrub exists to catch, written unredacted to the agent's container logs. Pre-existing and out of this PR's stated scope, but the docblock's framing ("any ERROR line echoing step params can carry a live credential") applies equally there, so it's worth not treating this as solved.

The 4-char floor is a reasonable call and well-argued in the comment.

Minor

• server/routes.ts:2310 — the comment cites "the same flap PR fix(schedules): consolidate pool gate onto sessionPoolViolation; 403 flapping re-enable #123 closed"; that looks like a placeholder reference rather than a real PR.
• tests/tier-pool-dispatch.test.ts — createdBy: 1 hardcodes the admin's user id in a direct DB insert. Fine today, brittle if seeding order ever changes; deriving it from the logged-in session would be safer.
• I wasn't able to run npm run check or the suites here (command approval), so the type-level and runtime verification of the new resolvable field and the daemon import is by inspection only. Per CLAUDE.md, ./scripts/full-tests-run.sh should gate the merge.

[github-actions]

🤖 Claude Code Review

Overall this is a well-constructed fix: the two halves (server-side pre-dispatch gate + daemon fail-fast with a redacted, informative summary) are consistent, the gates are placed before the side-effecting steps (stampOwnerSession/ensureSession/marketplace hold) on every path, and the secret-scrubbing on the persisted error is a genuine improvement. A few things worth addressing:

1. The gate misses the exact failure shape it was written for when framework isn't pinned (medium)

resolvableSecretSources (server/auth-session.ts:288-294) only includes stepsPrefix/stepsSuffix when merged.framework === "aeval". But framework is optional — validateWorkflowConfig (server/storage.ts:233) only checks it if defined — and the daemon falls back to config.framework || this.config.framework (vox-agentd.ts:1833), i.e. EVAL_FRAMEWORK || 'aeval'.

So for a workflow that omits framework and puts ${secrets.X} in stepsPrefix — precisely the reported job #31006 shape, and the shape the new test pins only because it explicitly sets framework: "aeval" — the run route accepts, the scheduler queues, and the console Run button stays enabled (resolvable: false). The daemon's new check still produces a good error, but the "don't burn an agent run / don't flap the schedule" half of the fix doesn't fire.

The intersection choice is defensible in general, but there's a stronger signal available here that doesn't over-block: stepsPrefix/stepsSuffix/app are WORKFLOW_ONLY_KEYS and role-disjoint, and voice-agent-tester hard-fails with job.config.app is required when app is absent (vox-agentd.ts:1955). So when framework is unset:

• app absent + stepsPrefix/stepsSuffix present → the run can only succeed under aeval, so gating on those fields cannot reject anything that would otherwise have worked.
• app present → keep the current conservative intersection.

That closes the gap without reintroducing the over-blocking risk the comment is guarding against.

2. resolvableSecretSources computes framework from a merge but reads fields per-config

Minor, but the reduce-spread merged is only used for framework, which is workflow-exclusive and can never be set by an eval set (validateEvalSetConfig rejects WORKFLOW_ONLY_KEYS). The merge is dead weight and slightly misleading — configs.find(c => typeof c?.framework === "string") would say what's actually meant.

3. Schedule PATCH gate keys on regionChanged/tierChanged (minor)

server/routes.ts:2307 runs the missing-secret check on wantsEnable || regionChanged || tierChanged. Secret resolution has nothing to do with region or tier, so this 400s a user who only wants to repoint an already-broken (and already disabled) schedule to a different region — they can't fix the thing they came to fix. Conversely cronChanged/capChanged on an enabled broken schedule pass silently. Gating on wantsEnable alone matches the stated intent (close the re-enable flap) without the collateral.

4. Redaction covers the persisted error but not the agent's own logs (note, pre-existing)

summarizeAevalFailure correctly scrubs both the raw and YAML-escaped forms before the string is persisted, and truncates after scrubbing — good ordering. But vox-agentd.ts:838-846 still does console.log('[aeval] ' + data) on every stdout/stderr chunk unscrubbed, so decrypted credentials echoed by aeval land in the agent container's logs verbatim. That's pre-existing and out of this PR's scope, but the new activeSecretValues field makes it a cheap follow-up (scrub in the stream handlers, not just at summary time).

5. Smaller observations

• strip()'s /^\S+\s+\S+\s*\|\s*\w+\s*\|\s*/ will mangle a non-loguru line that happens to start with two tokens followed by | WORD |. Cosmetic only.
• The 4-char redaction floor and the asymmetry argument behind it are sound; a 3-char secret leaking is a real but acceptable residual, and the comment says so honestly.
• activeSecretValues assignment inside the try whose finally clears it is correct — no window where values outlive the job.
• The session-inject.test.ts addition pins the right invariant (post-injection no ${secrets.*} survives), which is what makes the daemon's scan placement safe. Good call adding it rather than just relying on the comment.

6. Verification note

I could not run npm run check or the test suite in this environment (sandbox blocked the commands), so type-checking and the new tests are unverified here. Per CLAUDE.md, ./scripts/full-tests-run.sh should gate the merge — the new tier-pool-dispatch.test.ts case inserts directly into evalSchedules with createdBy: 1, which assumes the admin user is id 1; worth confirming that holds on a fresh DB.

[github-actions]

🤖 Claude Code Review

Reviewed the full diff against the merge base, plus the surrounding call sites (executeJob, runAeval, the job-secrets endpoint, classifyReferencedSecrets, the config validators, and the scheduler tick).

Overall

This is careful, well-scoped work. The core fix — replacing "last line of stderr" with an ERROR-line-preferring summarizer, plus a pre-dispatch gate on unconfigured secrets — is correct, and the layering is right: Core rejects up front, the daemon still fails loud post-substitution (catching a secret deleted between dispatch and claim), and the UI mirrors the server gate exactly rather than inventing its own predicate. The scrubbing of decrypted values out of a persisted, user-visible job error is a real security improvement, not just cosmetics. Test coverage matches the claims made in the comments.

I verified the two things most likely to be subtly wrong, and both hold:

• Scope agreement. sessionScopeForWorkflow (org → org secrets, else owner personal) is byte-for-byte the same branch the job-secrets endpoint takes (server/routes.ts:3833), so the new gate cannot reject a run that would have resolved at claim time. The known under-block (org secrets fenced on the job creator's membership) falls on the fail-safe side and now produces the daemon's clear message.
• Redaction lifetime. activeSecretValues is cleared in executeJob's finally, and summarizeAevalFailure is called from runAeval's close handler — which is awaited through executeAevalWithChunking before that finally runs. The forced-timeout path can't race it either: proc.on('close') returns early on settled, so the un-redacted branch is unreachable. Scrub-then-slice(0, 500) is also the right order.

Findings

1. vox_eval_agentd/aeval-output.ts:44 — redaction covers only the raw and YAML-escaped forms.
activeSecretValues carries v and yamlEscape(v). That covers the two forms the value actually takes on the way in. But loguru/Python can echo it back re-encoded — a repr() of a dict, a URL-encoded query param, a value nested inside a JSON string. Those survive into the persisted error. This is a mitigation rather than a guarantee, which is fine, but a one-line widening is nearly free:

Object.values(jobSecrets).flatMap((v) => [v, VoxEvalAgentDaemon.yamlEscape(v), JSON.stringify(v).slice(1, -1), encodeURIComponent(v)])

Worth doing given the output is stored and shown in the console; not a blocker.

2. server/auth-session.ts:145 — the hasApp ? null : "aeval" inference disagrees with the daemon when a fleet sets EVAL_FRAMEWORK=voice-agent-tester.
The daemon resolves config.framework || this.config.framework (a per-agent env knob Core can't see). For a config with no framework and no app, Core assumes aeval and gates stepsPrefix/stepsSuffix. On a voice-agent-tester-default fleet that's an over-block — but the run was doomed anyway (job.config.app is required), so the only change is which 400 the user gets, and the new one is more actionable. Correct as written; noting it because the comment argues the inference "cannot reject anything that would otherwise have worked," and that holds only because of the app-required hard-fail, which is a property of a different file. A pointer to that coupling would keep the reasoning checkable if VAT ever gains an app default.

3. server/index.ts:428 — the scheduler's new auto-disable is discoverable only via server logs.
Consistent with the existing misconfigured/pool-violation handling, so no objection to the shape. Combined with the PATCH re-enable gate the loop does close properly: the owner hits PATCH isEnabled:true, gets a 400 naming the exact secrets, fixes it, re-enables. Just flagging that a schedule can now go dark for a reason that leaves no trace on the row itself — if the schedules table ever grows a lastDisabledReason, this is a good first customer.

4. Duplicated alert block in console-workflow-detail.tsx:316 and run-your-own.tsx:609 (identical JSX plus identical missingSecrets derivation). Follows the existing showRuntimeWarning precedent, so it's consistent — but that's now two copies of a gate the comments explicitly say must stay in lockstep with the server. A shared hook/component would make drift impossible rather than merely unlikely.

Nothing here blocks merge. Note I could not run npm run check or the suite (both need approval in this sandbox), so the type-check and ./scripts/full-tests-run.sh pre-merge gate still need to be run.

[github-actions]

🤖 Codex Code Review

No findings.

• I reviewed the dispatch gates, secret classification/resolution paths, daemon unresolved-placeholder handling, and failure summarization/redaction logic.
• I didn’t see security regressions, auth bypasses, credential exposure in persisted errors, or meaningful edge-case/logic issues in the changed code.
• I did not run the test suite because the environment is read-only, but the added tests cover the main regression paths and the aeval-output heuristic well.

[github-actions]

🤖 Claude Code Review

Overall this is careful, well-reasoned work. The core change is sound: gate at every dispatch entry point (run, API v1 run, schedule create, schedule PATCH re-enable, run-now, scheduler tick), fail fast in the daemon, and surface aeval's own diagnosis instead of the PyInstaller banner. The framework-narrowing rationale in resolvableSecretSources is correct — framework/app/stepsPrefix/stepsSuffix are all WORKFLOW_ONLY_KEYS, so the inference can't diverge between the run-targets endpoint (which may omit the eval set) and the run route (which always includes it).

A few notes:

Incidental bug fix worth calling out. The client filter changed from s.class === "runtime" to s.brokerType == null. classifyReferencedSecrets returns {name, brokerType, present} and has no class field, so runtimeExposed was always empty client-side — meaning showRuntimeWarning never fired, the ack checkbox never rendered, and any shared-tier run against a workflow with present runtime secrets 400'd at routes.ts:4088 with no UI path to satisfy it. This PR repairs that as a side effect. Fine, but it's a user-visible behavior change (shared-tier runs now require ticking a box that previously never appeared) that isn't in the PR description or covered by a test.

Redaction has a residual gap (vox_eval_agentd/aeval-output.ts:443). activeSecretValues covers the raw and YAML-escaped forms, which is the right pair for what resolveSecrets writes. But two things escape it:

• The raw aeval stdout/stderr is still console.log/console.error'd unredacted at vox-agentd.ts:839,844, so the container logs carry every echoed credential. The persisted job error is now clean while the log is not — worth noting since the redaction comment implies the value is contained.
• Session-injected jobs: the minted storageState isn't in the redact list. Only the file path is injected into the YAML, so in practice aeval shouldn't echo cookie values — but if it ever dumps the loaded state on a parse failure, that goes into a persisted, user-visible error.

Neither blocks merge; the second is speculative.

Daemon hard-fail is slightly stricter than before for voice-agent-tester. active = [scenario, app] fails the job on any surviving placeholder anywhere in the app YAML, including a field that framework never reads. The comment justifies the field-level narrowing but not the within-field strictness. In practice the server gate now rejects these at dispatch anyway, so it's consistent — just noting that a previously-working job with a stale placeholder in an unused app key will now fail.

Scheduler disable is silent to the user (server/index.ts:428). Consistent with the existing pool-violation/misconfigured handling, and the PATCH gate closes the flap. But an owner whose secret was deleted gets a disabled schedule with only a server log to explain it — the MISSING_SECRETS_MSG text exists but is never persisted anywhere they can read. Same pre-existing gap as the neighbouring branches, so not a regression; a lastError-style column would close all four at once.

Minor:

• MISSING_SECRETS_MSG's "otherwise ask its owner to" clause is dead text on the schedule-create and run-now routes, which are owner-only (canScheduleWorkflow). Harmless, just slightly confusing.
• tests/tier-pool-dispatch.test.ts hardcodes createdBy: 1 and relies on the admin login being user 1 for canEditResource to pass. Fragile if seed ordering ever changes; reading the id off /api/auth/status would be safer.
• The missing-secrets alert block is duplicated verbatim across console-workflow-detail.tsx and run-your-own.tsx, along with the missingSecrets/runtimeExposed derivation. The file already had this duplication, so it's consistent — but this is the third copy of the same logic and a shared hook would be cheap now.

Security-wise I found no injection, auth-bypass, or new credential-exposure path: the gates run before marketplace.authorizeDispatch (so no escrow is held on a doomed run), the daemon's error message is deliberately scope-agnostic and leaks no names the caller couldn't already see, and the ordering of the unresolved-placeholder scan after injectStorageSession is correct and now pinned by a test.