fix(schedules): consolidate pool gate onto sessionPoolViolation; 403 flapping re-enable - #123
Merged
Merged
Conversation
…flapping re-enable The two cheapest follow-ups from PR #122's final review: - Schedule POST and PATCH now CALL sessionPoolViolation instead of carrying byte-identical open-coded copies of the public/team session checks — the drift the helper exists to prevent (run route, scheduler tick, run-now, and both schedule writes now share one rule). - The PATCH re-check fires on re-enable as well as tier change: a schedule the scheduler auto-disabled for a pool violation previously re-enabled fine and was silently disabled again next tick (log-only flap); it now returns an actionable 403 telling the user to change the tier first. 🤖 Built with SMT <smt@agora.build>
🤖 Codex Code ReviewNo findings.
Not run: tests/tsc, due read-only review context. |
🤖 Claude Code ReviewThe change consolidates four copies of the session-pool composition rule onto One real issue and two minor ones: 1. The re-enable check uses the requesting user, but the scheduler uses the schedule's creator — the flap can survive (
const creator = schedule.createdBy ? await storage.getUser(schedule.createdBy) : undefined;
...
const violation = sessionPoolViolation(schedule.targetTier, workflow, creator); // server/index.ts:427Concretely: workflow Fix: resolve the schedule's creator and check against them, matching the tick: const scheduleCreator = schedule.createdBy ? await storage.getUser(schedule.createdBy) : undefined;
const violation = sessionPoolViolation(effectiveTier, wf, scheduleCreator);(An argument for the current form is that on a tier change the caller is choosing the new tier — but the schedule still dispatches as its creator at tick time, so the creator's org is the one that decides claimability. Using the creator for both arms is the consistent choice.) 2. Message wording is off for the tier-change-only arm (
3. No test coverage for the new arm
Not a regression, just noting: an already-enabled schedule edited on cron/region only still skips the session check ( |
guohai
added a commit
that referenced
this pull request
Aug 25, 2026
…ion; perf + UX improvements (#124) Closes the remaining improvement backlog from the tier-targeting review rounds (PRs #121-#123): - NEW auth-session.detectSessionNeed(workflow): one pure detector (parse setup → owner scope → evaluate against brokered secrets) consumed by the run route, run-targets, schedule create/PATCH, run-now, and the scheduler tick — the "does this workflow need a session?" answer can no longer drift between routes. stampOwnerSession accepts the precomputed requirement, eliminating the duplicate getBrokeredSecretNames query per scheduler tick and per run-now. - run-targets derives needsSession from that detector (not "any brokered secret anywhere"), and gates agents.mine on session trust: a dispatcher who is neither owner nor workflow-org member no longer sees their own tokens offered for a credential-injected workflow the targeted branch would 403 (shared listings stay — attestation+consent is the sanctioned cross-user path). - Region job filter gains an indexable fast-path: eq(target_region) covers pooled rows (stamped at creation, never cleared), regex only for site-pinned targeted/legacy rows. - api.test's Organization Roles suite cleans up the admin-owner org it creates (owners can't leave via API), closing the cross-suite "admin has no org" pollution race that bit three gate runs. - console-workflow-detail Job History reads the {data,total} response shape (was typed as a bare array — table silently broke). - The tier auto-hop announces itself with a toast instead of silently moving the user's selection. 🤖 Built with SMT <smt@agora.build>
guohai
added a commit
that referenced
this pull request
Aug 30, 2026
…chedule re-enable Round 3. #1 Client/server gate divergence — my own doing. I narrowed the SERVER gate to the four fields the daemon resolves, but both dialogs kept filtering the full-config `referencedSecrets`, so a placeholder in an unresolved config key permanently disabled Run (no override) while POST /run would have accepted it. run-targets now tags each entry `resolvable`, and the client filters on it — the two agree by construction. #2 Schedule PATCH re-enable had no gate: it 200'd and the next tick silently disabled the schedule again, reason visible only in server logs. Same flap PR #123 closed for pool violations; now returns the 400 with the names. #4 nit: the missingSecretNames doc block had re-attached itself to resolvableSecretSources during the earlier reorder. Test updated for the additive `resolvable` field (an exact-match assertion, correctly failing on a real shape change). New test: re-enabling a schedule whose secret is missing 400s instead of flapping. Deferred with reasons: ${config.*} indirection can still smuggle a secret ref past the gate (falls through to the daemon's now-clear error, and widening the scan is what risked disabling schedules); 8-char redaction floor; one extra secrets read per due schedule per tick. 435 passing across tier-pool-dispatch, session-dispatch, api.test, eval-agent-daemon, session-inject. 🤖 Built with SMT <smt@agora.build>
guohai
added a commit
that referenced
this pull request
Aug 30, 2026
…staller banner (#128) * fix: make a missing-secret run fail with its actual cause, not a PyInstaller banner A run whose workflow references an unconfigured ${secrets.X} failed with "aeval exited with code 1: [PYI-3739:ERROR] Failed to execute script 'pyi_entrypoint'" — a Python packaging artifact. The real diagnosis existed at four points and every one dropped it (reported as job #31006). A. Reject at dispatch, naming the secrets. Core already computed `present` for every referenced placeholder and ignored it. New shared helper missingSecretNames(); enforced at ALL four job-minting entry points — run route, v1 run, schedule create, schedule run-now — so schedules can't keep emitting a doomed job every tick. B. Daemon fails fast on an unresolved placeholder, before spawning aeval. Scans post-substitution rather than instrumenting resolveSecrets, because substitution is SKIPPED entirely when the secrets map is empty — precisely the reported case. Catches a secret deleted between dispatch and claim. C. Replace "last line of stderr" with summarizeAevalFailure(). That heuristic is structurally wrong for a PyInstaller binary: its generic banner is printed AFTER the traceback, so the one line kept was guaranteed to be the least useful. Now prefers aeval's own loguru ERROR lines (scanning BOTH streams, since its sink may be either), else the noise-filtered tail; length-capped. Lives in shared/ so tests don't import the daemon entrypoint (which runs main()). D. Client: show the missing secrets and disable Start. Also fixes a latent bug this uncovered — both dialogs typed referencedSecrets with `class` while the server sends `brokerType`, so `s.class === "runtime"` was always false: the runtime-secret exposure warning and its consent checkbox have never rendered, making a shared-tier dispatch of any workflow with runtime secrets unsatisfiable from the UI (server requires the consent flag). Tests: dispatch gate 400s and names the secret on both the run route and schedule create (fixture mirrors the real failure — the ref lives in the workflow's stepsPrefix / platform.setup); summarizeAevalFailure recovers "Unknown variable source: secrets" from the reported log verbatim and never returns the pyi_entrypoint banner, on either stream, with a length cap. 🤖 Built with SMT <smt@agora.build> * fix(build): keep the aeval-output helper daemon-local so the image build resolves it CI's build-vox-eval-agentd failed with `Could not resolve "../shared/aeval-output"`: the Dockerfile copies shared/ files INDIVIDUALLY (secrets.ts, metrics.ts), so a new shared module is invisible to the esbuild bundle step. tsc can't catch this — only a real image build can. The placement was wrong anyway: shared/ should mean "used by more than one deployable", and nothing outside the daemon uses summarizeAevalFailure. Moved to vox_eval_agentd/aeval-output.ts (still its own module, so tests import it without pulling in the entrypoint that runs main()) and added the matching COPY line. Verified with a real local build (repo-root context, --target daemon): succeeds, and the helper is present in the bundled /app/vox-agentd.js. 🤖 Built with SMT <smt@agora.build> * fix(review): run the unresolved-secret scan AFTER session injection; gate the scheduler tick Both review bots caught a blocker I introduced. BLOCKER — the daemon's unresolved-placeholder scan ran BEFORE the session injection block, so it would have failed every session-injected (brokered login) job. Those jobs are exactly the case where ${secrets.EMAIL} / ${secrets.PASSWORD} are still literal at substitution time: Core withholds brokered login secrets from /jobs/:id/secrets by design (the agent must never hold durable credentials), and injectStorageSession only rewrites them away afterwards, forcing setup:storage. Moved the scan to just before framework dispatch, where the strings are final. Added a test in session-inject pinning the invariant the ordering depends on: post-injection, no ${secrets.*} survives a credential-bearing platform.setup. Scheduler tick was a FIFTH job-minting path I missed (run route, v1 run, schedule create, run-now were covered). A secret deleted after a recurring schedule was created would emit a doomed job every tick; it now disables the schedule with the secret names, matching the existing misconfigured/pool handling. 313 passing across session-dispatch, tier-pool-dispatch, session-capability-gate, api.test; session-inject + eval-agent-daemon 99. 🤖 Built with SMT <smt@agora.build> * fix(review): redact secret values from job errors; prefer the fatal error; honest copy Security (finding #1) — summarizeAevalFailure now captures up to 3 ERROR lines into the job's persisted, user-visible error, and resolveSecrets substitutes DECRYPTED values into the YAML aeval sees. An ERROR line echoing step params could therefore have leaked a live credential into the console. The summarizer now scrubs every known value of the active job's secrets (values under 4 chars skipped so a short value can't shred the message); the daemon holds them only for the job's duration and clears them in finally. #2 — take the LAST error lines, not the first. In a long run an early recoverable ERROR would otherwise bury the fatal one, which is the very "wrong line wins" failure this helper exists to fix. Identical for short traces like the reported one. #3/#6 — the daemon cannot distinguish "no such secret" from "the server withheld it for this job" (an org secret is additionally fenced on the job creator's membership), so its message no longer tells the user to go create one. Server and client copy likewise stop saying "you have not configured": secrets resolve in the WORKFLOW OWNER's scope, so someone running another user's public workflow cannot fix it themselves. Nits: missingSecretNames no longer orphans classifyReferencedSecrets' doc comment; api-v1 reuses the already-bound scope; stray blank lines. Tests: redaction (value scrubbed, surrounding diagnosis kept, short values ignored) and last-error preference. 🤖 Built with SMT <smt@agora.build> * fix(review): multi-line redaction, clear-on-throw, and narrow the gate to what the daemon resolves Round 2 of review findings. #1 Multi-line secret values escaped redaction. The summary is built from already split+trimmed lines, so no single line contains a PEM key or JSON blob and the whole-value match never fired — fragments reached the persisted error. Now redacts each LINE of every value as well, longest-first so whole values win over fragments. Floor raised 4 → 8 chars (#4): a value like "8080" or "prod" would otherwise shred unrelated text. #2 activeSecretValues was assigned OUTSIDE the try whose finally clears it, so a throw in between (e.g. from resolveSecrets) left decrypted values resident until the next job overwrote them — the opposite of what the comment promised. Moved inside the try. #3 Narrowed the Core gate to the four fields the daemon actually resolves (scenario, app, stepsPrefix, stepsSuffix) instead of the whole config. I had accepted this over-approximation in the PR body, but the review surfaced a consequence I had underweighted: a stray placeholder in an unresolved field would now PERMANENTLY DISABLE a recurring schedule on its next tick, with only a log line to show for it. Gate and daemon now agree by construction. Picked per-config rather than via mergeEvalConfig, which throws on conflicting keys and would turn a clean 400 into a 500. The run route filters its existing `classified` array, so no extra query. Deferred: one extra full-scope secrets read per due schedule per tick (#5) — noted, not urgent at current schedule counts. 362 passing across tier-pool-dispatch, session-dispatch, api.test, secrets, session-inject; daemon suite 97 incl. new multi-line redaction coverage. 🤖 Built with SMT <smt@agora.build> * fix(review): stop the client blocking runs the server accepts; gate schedule re-enable Round 3. #1 Client/server gate divergence — my own doing. I narrowed the SERVER gate to the four fields the daemon resolves, but both dialogs kept filtering the full-config `referencedSecrets`, so a placeholder in an unresolved config key permanently disabled Run (no override) while POST /run would have accepted it. run-targets now tags each entry `resolvable`, and the client filters on it — the two agree by construction. #2 Schedule PATCH re-enable had no gate: it 200'd and the next tick silently disabled the schedule again, reason visible only in server logs. Same flap PR #123 closed for pool violations; now returns the 400 with the names. #4 nit: the missingSecretNames doc block had re-attached itself to resolvableSecretSources during the earlier reorder. Test updated for the additive `resolvable` field (an exact-match assertion, correctly failing on a real shape change). New test: re-enabling a schedule whose secret is missing 400s instead of flapping. Deferred with reasons: ${config.*} indirection can still smuggle a secret ref past the gate (falls through to the daemon's now-clear error, and widening the scan is what risked disabling schedules); 8-char redaction floor; one extra secrets read per due schedule per tick. 435 passing across tier-pool-dispatch, session-dispatch, api.test, eval-agent-daemon, session-inject. 🤖 Built with SMT <smt@agora.build> * fix(review): re-enable gate covers the eval set; framework-aware sources; lower redaction floor Round 4. #1 The re-enable gate I added last round passed only [wf.config], omitting the eval-set config — so a schedule whose EVAL SET holds the missing secret still flapped, which is exactly the bug the gate exists to close. The scheduler checks both; now so does PATCH. #4 Both the gate and the daemon scan were framework-blind. aeval never reads `app`; voice-agent-tester never reads stepsPrefix/stepsSuffix. A stale placeholder in a field the selected framework ignores ran fine before and would have started hard-blocking. Both sides now pick sources by framework (defaulting to aeval, mirroring the daemon's own fallback). #2 Redaction floor 8 → 4. The failure modes are asymmetric: over-redacting garbles a word, under-redacting leaves a live credential in a persisted, user-visible job error — and short PINs/account IDs are precisely what a high floor leaks. Reasoning recorded in the comment so it stops oscillating. #3 Corrected a false claim in my own comment: the gate and daemon do NOT agree "by construction", because executeJob expands ${config.X} before ${secrets.X}, so a secret reached via config indirection is invisible to both. That direction is fail-safe (run accepted, daemon reports it clearly); widening the scan is what risks disabling schedules, so it stays deferred — now documented rather than misstated. 460 passing across tier-pool-dispatch, session-dispatch, api.test, secrets, eval-agent-daemon, session-inject. 🤖 Built with SMT <smt@agora.build> * fix(review): redact the YAML-escaped form; narrow to the framework intersection when unknown Round 5. HIGH (Codex) — resolveSecrets embeds values double-quoted and YAML-ESCAPED, so a secret containing a quote or backslash reaches aeval's output in escaped form and slipped past a raw-value-only scrub, landing in the persisted, user-visible job error. The escape logic is now a named helper and BOTH forms go into the redaction set. Test covers a value with a quote and a backslash. Framework fallback — the server guessed "aeval" when the config omits it, but the daemon actually falls back to its OWN env default (EVAL_FRAMEWORK, a per-agent knob Core cannot see). For a workflow dispatched to an agent running voice-agent-tester, that guess would 400 on stepsPrefix the daemon never reads — and permanently disable the schedule on the next tick, the exact harm this function's docblock says to avoid. Unknown framework now narrows to the INTERSECTION (scenario only), consistent with the module's stated asymmetry: over-blocking is destructive, under-blocking just defers to the daemon's clear error. missingSecretNames returns early when nothing is referenced, skipping a full-scope secrets query on the common path. Deferred with reasons: scheduler disable is permanent on a more transient condition than the cases it mirrors (matches existing precedent; a lastError-style surface is the real fix); v1 scans an eval set whose access check is pre-existing; the agent's own container logs echo raw output unredacted (pre-existing, and a separate boundary from the persisted field). 381 passing across tier-pool-dispatch, session-dispatch, api.test, eval-agent-daemon. 🤖 Built with SMT <smt@agora.build> * fix(review): gate the reported shape (no explicit framework); PATCH keys on re-enable alone Round 6. The intersection fallback from round 5 was too conservative in one important case: a workflow that OMITS `framework` with its secret in stepsPrefix — the exact reported job #31006 shape — slipped the gate entirely, so the "don't burn a run / don't flap the schedule" half of the fix never fired for the case that motivated it. (The existing test only passed because it pins framework: "aeval" explicitly.) Safe inference restores it: voice-agent-tester hard-fails without `app`, so a config with no `app` but with steps can only run under aeval — gating those fields cannot reject anything that would otherwise have worked. When `app` IS present and framework is unset, stay conservative. New test covers a workflow with neither `framework` nor `app`. Also: `framework` is workflow-exclusive, so find it rather than merge-spreading every config for one key. And the schedule PATCH gate now keys on wantsEnable ALONE — secret resolution has nothing to do with region or tier, and gating those stopped an owner from repointing an already-broken, already-disabled schedule, i.e. from fixing the thing they came for. Deferred with reasons: the agent's container logs still echo raw aeval output unredacted (pre-existing; a separate boundary from the persisted error, now cheap to close given activeSecretValues); scheduler disable remains permanent, matching existing precedent. 456 passing across tier-pool-dispatch, session-dispatch, api.test, eval-agent-daemon, secrets. 🤖 Built with SMT <smt@agora.build> * fix(review): track unsupplied secret NAMES instead of rescanning substituted text Codex medium: the post-substitution regex scan treated a secret whose VALUE happens to contain "${secrets.X}" as an unresolved placeholder, failing a perfectly good job. The unsupplied names are now captured BEFORE substitution (referenced minus supplied), and after session injection we check only whether those specific names still appear in the strings the selected framework reads. A substituted value can no longer introduce a phantom "missing" secret, and brokered refs stripped by injectStorageSession still drop out correctly. 462 passing across eval-agent-daemon, tier-pool-dispatch, session-dispatch, session-inject, api.test, secrets. 🤖 Built with SMT <smt@agora.build>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The two cheapest follow-ups from PR #122's final review round:
sessionPoolViolation()instead of carrying byte-identical open-coded copies of the public-pool / team-org session checks. All five session×pool enforcement points (run route, v1 run, scheduler tick, run-now, schedule writes) now share one rule.isEnabled: true, returning an actionable 403 ("change the schedule's tier before re-enabling").Behavior is unchanged for non-session workflows (the pure detector short-circuits).
Test plan
tscclean; tier-pool-dispatch (8) + session-dispatch (19) + api.test (283) = 310/310 passing.Generated with SMT smt@agora.build