Repository navigation
M2 Track T: project API keys and scheduled submission - #22
Merged
Merged
Conversation
Submission logic lived inline in the Hono handler, closed over the request context for the db, the session user, the queue, and audit. That is fine for the browser path and impossible for every other caller Track T introduces: a cron schedule and an inbound webhook have no `Context`, and an API-key request has a principal that is not a session user. So the body moves to `lib/submit.ts` as `submitTask(db, queue, args)` and the route becomes a thin adapter. Nothing about the sequence changes — quota headroom, repo-ref verification, resource authorization, the single transaction that writes task + run + latestRunId + the audit row together, and the post-commit enqueue all keep their order and their comments. Two supporting moves make that possible: - `audit()` grew `auditAs(db, actor, entry)` underneath it, taking the actor explicitly instead of reading `c.var.user`. audit_logs already has a column per actor kind (user, api key, runtime); only the helper insisted on a session. The daemon surface had to route around this with a private copy, and the API-key path would have been the third — so the generalization is overdue rather than speculative. - `resolveRunPlan()` moves to `lib/run-plan.ts`, since submit and retry both call it and it can no longer live in the route module. Attribution is worth stating because it constrains Track T: tasks.created_by and runs.created_by are NOT NULL → users.id, so automated submissions cannot be authorless. `actorUserId` is separate from the audit `actor` for exactly that reason — a schedule's runs are attributed to the schedule's owner while the audit row can still name the key or trigger that fired it. Pure extraction, no behavior change: the existing api suite (117 tests) passes untouched.
…ng (M2 Track T2) The `api_keys` table has existed since 0000_init and nothing ever read it; the design doc has described `Bearer agr_<key>` since M1. This implements it, and the shape of the implementation is mostly a set of decisions about what a credential should *not* be able to do. Reach is an allow-list, not a denylist. A table of (method, path) → required scope names every endpoint a key may touch; anything unlisted is 403. The alternative — "a key can do whatever its owner can, minus a denylist" — fails open on every route added afterwards, which is the failure mode that matters here: a key issued today would silently gain tomorrow's admin endpoint. So administration (registries, template publish, members, quota, settings, notification endpoints, checkpoint decisions, and key management itself) is unreachable by construction rather than by omission, and the org role is pinned to org_member on key requests so an admin's key still administers nothing. A key acts as its creating user and is bound to its project. That keeps project membership the single source of access — revoking someone's membership stops their keys with it, which is also how you disable a departing teammate's automation. The binding is enforced inside assertProjectRole, which now takes the whole RequestPrincipal instead of a bare user id. That signature change is the point: handlers reach a project through a :projectId param, a loaded run, or a loaded task, and all of them land in that function, so the check cannot be bypassed by forgetting it at one call site. The compiler enforces the rest. Audit-on-use needed no new machinery once the actor was generalized in T1: mutations already wrote an audit row, and they now carry actor_api_key_id alongside actor_user_id, so a leaked key is distinguishable from its owner working in the browser. `last_used_at` advances at most once a minute — it is a liveness signal for the UI, not the audit trail, and an UPDATE in front of every read would be a poor trade for a timestamp nobody reads to the second. Verification reuses the daemon-token scheme rather than inventing a second one: prefix-indexed lookup then constant-time compare, with unknown, malformed, revoked, and expired all answering the same 401 so a probe learns nothing from the difference. The generate/hash/compare trio existed in three near-identical copies (runtime tokens, invitation tokens, and this would have been the third); it is now one `bearer-tokens` module they all call. Migration 0023 adds the unique index on api_keys.prefix that the lookup needs — unique so a prefix collision can never make resolution ambiguous, mirroring runtimes_token_prefix_uq. Covered by apps/api/src/test/api-keys.integration.test.ts: one-time plaintext, hash-only at rest, admin gating, uniform 401 across unknown/revoked/expired, per-route scope enforcement, blanket 403 across eleven admin surfaces, cross-project denial, audit attribution, and a key dying with its owner's membership. Docs updated in design/05 and both manual locales.
`project.archive` has always written `projects.status = 'archived'` and nothing has ever read it — grepping `apps/api/src` and `packages/orchestration/src` for the value turns up exactly two hits, both the write itself. Meanwhile both manual locales promise that archiving "stops accepting submissions". In practice the project only dropped out of the switcher's active list; a submit or a retry against it still went through and consumed quota. That gap was tolerable while every submission required a human to click a button in a UI that hid the project. It stops being tolerable now that work can arrive without one: an archived project is by definition the one nobody is looking at, so unattended runs would accumulate there unseen, which is the worst place for them to accumulate. Submit and retry now both call `assertProjectAcceptsWork` and answer `409 project_archived`. Reads are deliberately untouched — archiving preserves history rather than hiding it, and the manual's promise is about new work. Landing this before schedules rather than with them: it is the fire-time check's most important case, and it is a bug on its own terms today.
…rchestration T1 put `submitTask()` in `apps/api/src/lib/` and noted it should move to `packages/orchestration` only if the worker ever needed it. Cron schedules need it: the fire handler runs in the worker, and `scripts/check-deps.ts` rightly forbids the worker importing the api. Moved, unchanged: `submit.ts`, `run-plan.ts`, `usage.ts` (both the reporting query and the quota gate — `assertQuotaHeadroom` calls `projectUsage`, so splitting them would have put one month-window definition in two packages), and `executors.ts` → `worker-executors.ts` (renamed on arrival; `executors` next to core's `EXECUTOR_CATALOG` would have read as the catalog rather than as which workers are currently alive). `audit.ts` splits along the seam T1 opened: the db-writing `auditAs` goes to orchestration, the Hono wrappers `audit`/ `requestActor` stay in the api. None of these ever had an HTTP dependency — they import only core, db, and drizzle. They were in `apps/api/src/lib/` because that is where the first caller happened to live, and this corrects that rather than accommodating the worker: "is a project over quota" and "which executors are live" are facts about runs, not about requests. Pure move. Behavior, signatures, and comments are unchanged; the full suite (522 pass, 5 pre-existing skips) and the dependency-direction check confirm it.
A task type, its parameters, and a five-field cron in an IANA timezone. pg-boss owns the calendar, so DST edges are the queue's problem rather than ours; the worker re-registers every enabled schedule at boot, because the row and the calendar are written separately and a crash between them would leave a schedule that never fires — invisible by nature. The design turns on one distinction: conditions that will never heal on their own disable the schedule and announce it, everything else leaves it enabled and records the error. An owner who left the project, an archived project, and a withdrawn task type are permanent — the schedule stops, says why on its row, audits, and emits schedule.disabled. An exhausted quota, a revoked grant, or no worker for the required executor are fixable — the schedule stays on, surfaces last_error, emits schedule.failed, and tries again next time. Disabling is deliberately loud. The obvious alternative was to copy the API-key rule and let a schedule die with its owner's access, but the two have opposite failure signatures: a dead key is loud (the script gets a 401 within minutes), while a dead schedule is silent, and silence is indistinguishable from "no schedule was ever created". That is the worst property available to the one feature whose entire job is to run while nobody is watching — you would find out three Mondays later. Authority is checked at fire time rather than revoked eagerly, using the same member gate the manual submit path applies, so a schedule does exactly what its owner could do right now and never more. Eager revocation means hooking every path that can remove authority — membership removal, role downgrade, archive, org-role change — and missing one leaves a schedule firing with authority its owner no longer has. One check at the point of use cannot be forgotten. Runs are attributed to that owner because runs.created_by is NOT NULL: unattended work still names a human, which is what makes the check meaningful in audit. Two details worth recording: - `replace` cancels through a new `requestRunCancellation` helper rather than finalizing directly. Cancellation is cooperative for a RUNNING run — the engine checks the flag at its next step boundary — so a hard finalize would race an executor mid-invocation and strand its usage accounting. The helper is the API cancel handler's semantics, extracted rather than reimplemented. - Schedule notifications have no run behind them, so they reuse the run-less delivery shape test sends already established (`run_id`/`event_id` null) instead of widening the schema or inventing a fictional run. Covered by apps/api/src/test/schedules.integration.test.ts (11 tests: CRUD, cron/timezone rejection, admin gating, all three concurrency policies, each disable reason, and the transient-failure case proving the schedule survives an exhausted quota) plus a unit suite for the cron validator. Docs in design/04 and both manual locales.
…e tokens
T3 shipped the machinery but not a schedule you could usefully create. Three
gaps, in order of how much they mattered.
**Parameters were frozen, and the flagship scheduled task needs them not to
be.** `pm.weekly-report` takes a required free-text `dateRange` that is
interpolated into both the prompt and the artifact title, and the template
expression language is pure path resolution — no `now`, no date arithmetic, by
design. So a weekly schedule would have produced "Weekly Report —
2026-07-27..2026-08-02" every Monday forever, telling the agent to report on
the same fixed week each time: the M2 acceptance criterion literally true and
practically worthless.
Nine fire-time tokens fix that ({{today}}, {{lastWeekStart}}, {{lastMonthEnd}},
…), substituted into string parameters when the schedule fires and resolved as
calendar days in the schedule's own timezone, from one clock reading shared
with lastFiredAt so a report cannot be stamped with a different day than it
covers. The set is closed on purpose — no arithmetic, no formats, no nesting.
The failure mode here is quietly growing a second, more capable expression
language beside the one ADR-0006 deliberately made non-Turing-complete, so the
list lives in core as a const and adding to it is a deliberate edit.
**Bad parameters surfaced a week late.** Creation now validates the *resolved*
parameters against the same compiled schema the submit form renders from, and
rejects unknown tokens outright — a `{{lastWeek}}` typo would otherwise reach
the agent's prompt verbatim and read as a broken report rather than a broken
schedule. Editing re-validates for the same reason. "Loud but late" is barely
better than silent when the whole point is unattended operation.
**Creation lived in the wrong place.** The settings form could not render a
task's parameter form, so it could only ever create schedules that failed. That
form, the preflight check, and the agent-slot picker all already exist on the
submit page, and "this task, repeatedly" is the honest mental model — so
"Run on a schedule" goes there, gated on the project-admin role the API
requires, and Settings → Schedules becomes purely the management view it is
good at: pause, resume, delete, see why one stopped. The create form is gone
rather than left half-working.
Verified in a real browser, not just in tests: filled the weekly-report form
with a token, opened the dialog (all copy resolved, no leaked i18n keys),
created the schedule, landed on the management view, then sent a schedule.fire
job through the running worker — the resulting run's paramsSnapshot carried
`dateRange: "2026-07-27..2026-08-02"`, last Mon-Sun relative to a Monday
firing, while the schedule row kept the token.
Date math is covered directly (packages/core/src/schedules.test.ts): Monday-based
weeks, Sunday as a week end, timezone-not-server resolution, a DST crossing, and
month/leap-year boundaries.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded project-scoped API keys, recurring schedules, signed inbound webhook triggers, shared task submission orchestration, archive enforcement, worker processing, persistence, web controls, tests, documentation, and translations. ChangesAutomation platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hutusi
marked this pull request as ready for review
August 3, 2026 09:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (21)
apps/api/src/test/api-keys.integration.test.ts (1)
255-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not delete shared membership in the last test.
This test deletes the admin's
project_membersrow forprojectId. All other tests in the file depend on that membership. The suite passes only because this test runs last. Any test added after it fails. Restore the membership afterwards, or run this case against a dedicated project.♻️ Proposed change
it("stops working when its owner loses project membership", async () => { + const scopedProjectId = ( + await jsonOf<{ id: string }>( + await admin.request("/api/v1/projects", { + method: "POST", + json: { slug: "keys-membership", name: "keys-membership" }, + }), + ) + ).id; - const key = await issueKey(["runs:read"]); - expect((await withKey(key.key as string, `/projects/${projectId}/tasks`)).status).toBe(200); + const key = await issueKey(["runs:read"], scopedProjectId); + expect((await withKey(key.key as string, `/projects/${scopedProjectId}/tasks`)).status).toBe( + 200, + ); const [owner] = await db.select().from(apiKeys).where(eq(apiKeys.id, key.id)); await db .delete(projectMembers) .where( and( - eq(projectMembers.projectId, projectId), + eq(projectMembers.projectId, scopedProjectId), eq(projectMembers.userId, owner?.createdBy as string), ), ); - expect((await withKey(key.key as string, `/projects/${projectId}/tasks`)).status).toBe(403); + expect((await withKey(key.key as string, `/projects/${scopedProjectId}/tasks`)).status).toBe( + 403, + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/api-keys.integration.test.ts` around lines 255 - 270, Update the test “stops working when its owner loses project membership” to avoid permanently deleting the shared project_members row: either create and use a dedicated project for this scenario or restore the owner’s membership after asserting the 403 response. Keep the membership state required by other tests intact regardless of test execution order.apps/api/src/middleware/auth.ts (1)
52-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the two API-key denials distinct stable codes.
Both denials use
AppError.forbidden, so both answer with codeforbidden. A client cannot distinguish "this endpoint is closed to API keys" from "this key lacks a scope", and the localized message cannot differ by case. Line 57 also interpolates the scope into the message, which a locale resource cannot reproduce from the code alone. Use distinct codes and move the scope name intodetails.♻️ Proposed change
const needed = requiredScopeFor(c.req.method, c.req.path); if (!needed) { - throw AppError.forbidden("This endpoint is not available to API keys"); + throw new AppError( + "api_key_route_forbidden", + 403, + "This endpoint is not available to API keys", + ); } if (!row.scopes.includes(needed)) { - throw AppError.forbidden(`This API key lacks the ${needed} scope`); + throw new AppError("api_key_scope_missing", 403, `This API key lacks the ${needed} scope`, { + scope: needed, + }); }Add the matching
enandzh-CNentries in theerrorsnamespace for the new codes.As per coding guidelines: "API error
messagevalues must localize from theerrorsnamespace using a stable errorcode."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/middleware/auth.ts` around lines 52 - 58, Update the API-key denial branches in the authentication middleware to use distinct stable error codes for endpoint-unavailable and missing-scope cases. Move the required scope from the interpolated message into the error details so each message can localize solely through its code, and add matching English and zh-CN entries in the errors namespace for both codes.Source: Coding guidelines
apps/api/src/routes/api-keys.ts (1)
57-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail explicitly when the insert returns no row.
rowis typed as possiblyundefined. The handler then writes an audit row withresourceId: undefinedand answers 201 with{ key }and no key metadata. The sibling schedule route guards the same case with an explicit throw. Add the same guard so the response shape cannot degrade silently.♻️ Proposed change
.returning(apiKeyView); + if (!row) throw new Error("api key insert failed"); await audit(c, { action: "project.apikey.create", resourceType: "api_key", - resourceId: row?.id, + resourceId: row.id,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/api-keys.ts` around lines 57 - 78, In the API key creation handler, add an explicit guard immediately after the insert and before audit creation, using the returned row from the insert operation. Throw the same established error used by the sibling schedule route when no row is returned, and preserve the existing audit and 201 response flow for successful inserts.apps/api/src/routes/catalog.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DEFAULT_EXECUTORis redefined in two routes.packages/orchestration/src/run-plan.tsexportsDEFAULT_EXECUTORasprocess.env.AGRIPPA_EXECUTOR ?? "claude-agent-sdk". Two API routes declare the same expression locally, and both carry comments stating they must resolve the executor exactly as submit does. Three definitions can drift.
apps/api/src/routes/catalog.ts#L3-L3: addDEFAULT_EXECUTORto the@agrippa/orchestrationimport and delete the local declaration at line 8.apps/api/src/routes/projects.ts#L38-L43: addDEFAULT_EXECUTORto the same import and delete the localdefaultExecutorat line 638.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/catalog.ts` at line 3, Reuse the centralized DEFAULT_EXECUTOR from `@agrippa/orchestration` instead of redefining it locally. In apps/api/src/routes/catalog.ts at lines 3-3, import DEFAULT_EXECUTOR and remove the local declaration at line 8; in apps/api/src/routes/projects.ts at lines 38-43, add the same import and remove the local defaultExecutor declaration at line 638, updating references as needed.apps/api/src/test/api.integration.test.ts (1)
194-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd retry coverage for the archived project.
apps/api/src/routes/execution.tsnow callsassertProjectAcceptsWorkonPOST /tasks/:id/retry. This test covers submission only. A retry of the pre-existing task should also return 409 withproject_archived. Add that assertion so the new retry guard cannot regress unnoticed.As per coding guidelines: "API request mutations must use shared schemas and produce audit records; endpoint changes require integration coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/api.integration.test.ts` around lines 194 - 206, Extend the archived-project scenario around the existing submit assertion to call POST /tasks/:id/retry for the pre-existing task, then assert a 409 response with error code project_archived. Reuse the existing project and task identifiers and request helpers so retry coverage verifies the assertProjectAcceptsWork guard without changing the submission or history checks.Source: Coding guidelines
packages/orchestration/src/submit.ts (1)
106-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive
tasks.orgIdfrom the verified project row.
submitTasktrustsactor.orgId, butassertProjectAcceptsWorkonly checksprojects.statusandassertProjectRoleonly checks project membership and API-key binding. Useprojects.orgIdreturned or selected at that check so org-scoped audit and usage rows cannot diverge from the target project.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/submit.ts` around lines 106 - 118, Update submitTask and the project verification flow around assertProjectAcceptsWork to retain the verified project row, then set the tasks insert’s orgId from that row’s projects.orgId instead of actor.orgId. Preserve the existing project-role and work-acceptance checks while ensuring downstream org-scoped records use the target project’s organization.packages/orchestration/src/usage.ts (1)
95-109: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the full usage aggregate in the quota gate.
assertQuotaHeadroomreads onlyusage.tokens, butprojectUsageruns five queries: totals,byModel,byTaskType(three joins), the clock row, andbyDay. Four of them are discarded. This gate runs on every task submission (submitTask) and on every schedule firing, so the extra round trips and the three-table join are paid on the hot path.Query the month total directly and keep
projectUsagefor the display endpoint.♻️ Proposed refactor to query only the total
+/** Month-window token total only — what the quota gate needs. */ +async function periodTokens(db: Db, projectId: string): Promise<number> { + const [totals] = await db + .select({ + tokens: sql<string>`coalesce(sum(${tokenUsage.inputTokens} + ${tokenUsage.outputTokens}), 0)`, + }) + .from(tokenUsage) + .where(and(eq(tokenUsage.projectId, projectId), gte(tokenUsage.occurredAt, periodStart))); + return Number(totals?.tokens ?? 0); +} + export async function assertQuotaHeadroom(db: Db, projectId: string): Promise<void> { const [quota] = await db .select() .from(projectQuotas) .where(eq(projectQuotas.projectId, projectId)); if (!quota?.hardStop || quota.tokenLimit === null) return; - const usage = await projectUsage(db, projectId); - if (usage.tokens >= quota.tokenLimit) { + const tokens = await periodTokens(db, projectId); + if (tokens >= quota.tokenLimit) { throw new AppError("quota_exhausted", 400, "The project's token quota is exhausted", { - tokens: usage.tokens, + tokens, tokenLimit: quota.tokenLimit, }); } }
projectUsagecan then callperiodTokensfor its owntokensfield.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/usage.ts` around lines 95 - 109, Update assertQuotaHeadroom to query the current month’s token total directly instead of calling projectUsage, preserving the existing quota limit comparison and error behavior. Reuse the existing periodTokens helper or equivalent total-query symbol, and update projectUsage to obtain its tokens field through periodTokens while retaining its other display metrics.apps/api/src/lib/bearer-tokens.ts (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the prefix-length assumption in
issueToken.
issueTokenderivestokenPrefixfrom the concatenated token, so the stored prefix containsDISPLAY_PREFIX_LENGTHcharacters counted from the scheme tag. The current tagsagr_andagrd_are shorter than 12 characters, so each stored prefix stays unique. If a future caller passes a tag of 12 or more characters, every issued token produces the same prefix, and the unique prefix indexes reject the second issuance. Add an invariant check so the failure appears at the call site.🛡️ Proposed guard
export function issueToken(prefix: string): IssuedToken { + if (prefix.length >= DISPLAY_PREFIX_LENGTH) { + throw new Error( + `token prefix "${prefix}" must be shorter than DISPLAY_PREFIX_LENGTH (${DISPLAY_PREFIX_LENGTH}) so stored prefixes stay unique`, + ); + } const token = prefix + generateSecret(); return { token, tokenPrefix: tokenPrefixOf(token), tokenHash: hashToken(token) }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/lib/bearer-tokens.ts` around lines 47 - 51, Update issueToken to validate that the supplied prefix is shorter than DISPLAY_PREFIX_LENGTH before concatenating and deriving tokenPrefix. Throw an invariant error at the call site when the prefix is too long, while preserving the existing token generation and hashing behavior for valid prefixes.apps/web/src/lib/types.ts (1)
377-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
ScheduleConcurrencyPolicytype.
packages/core/src/schedules.tsalready derivesScheduleConcurrencyPolicyfromSCHEDULE_CONCURRENCY_POLICIES.SubmitTaskPage.tsximports it. This local union duplicates that contract and will drift if a policy is added to core.♻️ Proposed refactor
- concurrencyPolicy: "skip" | "queue" | "replace"; + concurrencyPolicy: ScheduleConcurrencyPolicy;Add the import at the top of the file:
import type { ScheduleConcurrencyPolicy } from "`@agrippa/core`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/types.ts` at line 377, Update the type containing concurrencyPolicy in types.ts to use the shared ScheduleConcurrencyPolicy imported from `@agrippa/core` instead of the duplicated string union, preserving the existing property name and contract.apps/web/src/pages/SettingsPage.tsx (1)
1290-1294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the switch accessible name identify the schedule.
Every row uses the same
aria-labelvalue, "Enabled". A screen reader announces identical switches for all schedules, so the user cannot tell which schedule a switch controls.♿ Proposed fix
<Switch checked={row.enabled} - aria-label={t("settings:schedules.toggle")} + aria-label={`${t("settings:schedules.toggle")} — ${row.name}`} onCheckedChange={(on) => update.mutate({ id: row.id, body: { enabled: on } })} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/pages/SettingsPage.tsx` around lines 1290 - 1294, Update the Switch in the schedule row to include the specific schedule’s identifying name in its accessible label, rather than using the shared settings:schedules.toggle translation alone. Preserve the existing checked state and update.mutate behavior while ensuring each row’s control is distinguishable to screen readers.docs/design/04-execution-runtime.md (1)
142-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the scheduled-submission material under its own heading, and mention edit-time validation.
Two points.
These three paragraphs sit under the
### Notifications (outbound)heading on line 140, and line 148 returns to the notifications topic. Scheduled submission is a separate subject. A reader scanning headings will not find it here. A### Scheduled submissionheading, placed near theschedule.firebullet on line 51, would match the document's structure.Line 144 says creation rejects unknown tokens and validates the resolved parameters. The PATCH handler applies the same check on edit (
apps/api/src/routes/schedules.tslines 157-170). State that both creation and edit validate, so a reader does not assume an edit can bypass the check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/04-execution-runtime.md` around lines 142 - 147, Move the three scheduled-submission paragraphs out from under “### Notifications (outbound)” into a new “### Scheduled submission” section near the existing schedule.fire bullet, preserving their content and order. In the token-validation paragraph, explicitly state that both schedule creation and PATCH edits reject unknown tokens and validate resolved parameters against the compiled schema.packages/core/src/schedules.test.ts (1)
153-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a parity test between
SCHEDULE_TOKENSandscheduleTokenValues.
findUnknownScheduleTokensclassifies a token as known usingSCHEDULE_TOKENS.applyScheduleTokenssubstitutes using the keys ofscheduleTokenValues. Nothing in this suite ties the two together. If a token is added toSCHEDULE_TOKENSwithout a matching value, the API accepts it atapps/api/src/routes/schedules.tsline 44 and the substitution then writesundefinedinto the agent prompt. The same list is printed to the user as the known set, so drift also produces a misleading error message.💚 Proposed test
+ it("keeps SCHEDULE_TOKENS and the resolved values in step", () => { + const values = scheduleTokenValues(at, "UTC"); + expect([...SCHEDULE_TOKENS].sort()).toEqual(Object.keys(values).sort()); + for (const token of SCHEDULE_TOKENS) { + expect(applyScheduleTokens({ v: `{{${token}}}` }, at, "UTC").v).toMatch(/^\d{4}-\d{2}-\d{2}$/); + } + });Add
SCHEDULE_TOKENSto the import on lines 2-8.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/schedules.test.ts` around lines 153 - 162, Add a parity test in schedules.test.ts that imports SCHEDULE_TOKENS and verifies every token classified as known by findUnknownScheduleTokens has a corresponding key in scheduleTokenValues used by applyScheduleTokens. Keep the assertion focused on detecting drift between these two token sets.apps/api/src/test/schedules.integration.test.ts (6)
407-423: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGive the re-enable test an error to clear.
The test name says the re-enable clears the reason. The schedule created on line 408 never failed, so
disabledReasonandlastErrorare already null before the PATCH on line 415. The assertions on lines 421-422 would pass even if the PATCH handler dropped the clearing logic on lines 186-188 ofapps/api/src/routes/schedules.ts. Set the columns first so the test measures the clearing.💚 Proposed change
expect(await fireSchedule(db, queue, row.id)).toEqual({ kind: "skipped", reason: "disabled" }); + await db + .update(taskSchedules) + .set({ disabledReason: "owner_lost_access", lastError: "boom", lastErrorAt: new Date() }) + .where(eq(taskSchedules.id, row.id)); await admin.request(`/api/v1/projects/${projectId}/schedules/${row.id}`, { method: "PATCH", json: { enabled: true }, }); const after = await rowOf(row.id); expect(after?.enabled).toBe(true); expect(after?.disabledReason).toBeNull(); expect(after?.lastError).toBeNull(); + expect(after?.lastErrorAt).toBeNull();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 407 - 423, Update the test “does not fire while disabled, and clears its reason when re-enabled” to seed non-null disabledReason and lastError values on the created schedule before disabling it. Keep the existing disable, re-enable, and null assertions so the test verifies the PATCH handler actually clears previously stored error state.
274-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify how the endpoint's
createdByis obtained.Line 280 creates a schedule through the API, reads its row, and takes
createdBy, only to supply a user id for the notification endpoint. The call leaves an extra enabled schedule in the project as a side effect, and the nesting is hard to follow. The admin's user id is available directly.♻️ Proposed change
+ const [adminUser] = await db + .select({ id: users.id }) + .from(users) + .where(eq(users.email, "root@example.com")); await db.insert(notificationEndpoints).values({ projectId, kind: "generic", name: "hook", url: "https://example.com/hook", events: [], - createdBy: (await rowOf((await createSchedule()).id))?.createdBy as string, + createdBy: adminUser?.id as string, });Add
usersto the@agrippa/dbimport on lines 3-13.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 274 - 281, Update the notificationEndpoints insert to obtain createdBy directly from the admin user record instead of creating a schedule and querying its row. Add the users symbol to the existing `@agrippa/db` import and use the established admin user lookup, removing the nested createSchedule/rowOf expression and its extra schedule side effect.
141-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the audit rows for
schedule.updateandschedule.delete.Line 108 verifies the audit row for
schedule.create. This test exercises PATCH and DELETE but does not check that either wrote an audit row. The coding guidelines require an audit row for every API mutation, so the two uncovered mutations are exactly the ones a regression would silently drop.💚 Proposed addition
expect(unregistered).toContain(deleted.id); expect(await rowOf(deleted.id)).toBeUndefined(); + + for (const [action, id] of [ + ["schedule.update", row.id], + ["schedule.delete", deleted.id], + ] as const) { + const [logged] = await db + .select() + .from(auditLogs) + .where(and(eq(auditLogs.action, action), eq(auditLogs.resourceId, id))); + expect({ action, logged: logged !== undefined }).toEqual({ action, logged: true }); + }As per coding guidelines: "Every API mutation must write an audit row via
apps/api/src/lib/audit.ts".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 141 - 155, Extend the “unregisters the calendar when paused or deleted” test to assert audit rows for both mutations: verify the PATCH produces a schedule.update audit entry and the DELETE produces a schedule.delete entry, using the existing audit-row query/assertion pattern from the schedule.create check near line 108.Source: Coding guidelines
229-268: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the ownership assertion, and cover a finished previous run.
Two gaps.
Line 243 asserts only that
run?.createdByis truthy. The test is named "submits a run attributed to its owner", anddocs/design/04-execution-runtime.mdline 146 makes owner attribution a design invariant. Compare against the schedule'screatedByso a regression that attributes the run to the wrong user fails the test.The
skipcase covers an unfinished previous run. Nothing covers a finished one. That is the normal path: a weekly schedule whose previous run reached a terminal status must fire again. A bug inisTerminalRunStatushandling would make everyskipschedule fire exactly once, and this suite would still pass.💚 Proposed additions
const [run] = await db .select() .from(runs) .where(eq(runs.id, after?.lastRunId as string)); - // runs.created_by is NOT NULL — an unattended submission still names a human - expect(run?.createdBy).toBeTruthy(); + // runs.created_by is NOT NULL — an unattended submission still names a human + expect(run?.createdBy).toBe(after?.createdBy as string); }); + + it("fires again once the previous run has finished", async () => { + const row = await createSchedule({ concurrencyPolicy: "skip" }); + await fireSchedule(db, queue, row.id); + const first = (await rowOf(row.id))?.lastRunId as string; + await db.update(runs).set({ status: "succeeded" }).where(eq(runs.id, first)); + expect((await fireSchedule(db, queue, row.id)).kind).toBe("submitted"); + expect((await rowOf(row.id))?.lastRunId).not.toBe(first); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 229 - 268, Strengthen the ownership assertion in the “submits a run attributed to its owner” test by comparing run.createdBy with the schedule row’s createdBy, not merely checking truthiness. Extend the skip-policy coverage in “honors each concurrency policy against an unfinished previous run” with a terminal previous run, then verify firing submits a new run and updates lastRunId instead of skipping.
49-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset the spy arrays between tests.
registeredandunregisteredaccumulate for the whole file. Line 106 asserts onregistered.at(-1), which depends on the create under test being the most recent registration, and everytoContainassertion passes for any earlier push. The ids are unique, so the current assertions stay specific, but a test inserted between two others can silently change whatat(-1)refers to. AbeforeEachthat clears both arrays removes the ordering dependency.♻️ Proposed change
+ beforeEach(() => { + registered.length = 0; + unregistered.length = 0; + });Import
beforeEachfrombun:teston line 1, and change line 106 to assert onregistered[0].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 49 - 58, Reset the shared registered and unregistered arrays before each test by importing and registering beforeEach from bun:test, so each test starts with empty spy state. In the assertion around registered, use registered[0] rather than registered.at(-1) after the reset.
175-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the token-in-a-non-string-field case.
The route doc comment in
apps/api/src/routes/schedules.tslines 27-28 states that resolving tokens before validation "also catches a token placed in a field that is not a string". No test exercises that path. A case that puts"{{today}}"into a numeric or boolean input and expects 400 would pin the behavior the comment claims.Also, the comment on line 219 says the range ends the day before this Monday, but lines 221-224 only assert that the range is ordered and in the past. Either tighten the assertion or reword the comment to match what it checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 175 - 199, Extend the schedule integration tests near “rejects an unknown token...” with a case placing "{{today}}" in a numeric or boolean parameter and assert the POST returns 400. Update the range test around its existing date-range assertions so the implementation and comment agree: either assert the range ends the day before Monday or reword the comment to describe only ordering and pastness.apps/api/src/routes/schedules.ts (2)
49-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared compiled-template lookup.
Lines 49-66 repeat the task-type → template → published version →
upgradeCompiledTemplatechain thatsubmitTaskperforms inpackages/orchestration/src/submit.ts(lines 70-88), including the sametemplate_unpublishederror. If the resolution rules change, both sites must change together. A shared helper in@agrippa/orchestration(for exampleloadCompiledTemplateForTaskType(db, taskTypeId)) would keep the contract in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/schedules.ts` around lines 49 - 66, Extract the repeated task-type → template → published-version resolution and compilation logic from the schedule route and submitTask into a shared `@agrippa/orchestration` helper, such as loadCompiledTemplateForTaskType(db, taskTypeId). Preserve the existing enabled-task-type, template_unpublished, missing-version, and upgradeCompiledTemplate behaviors, then update both callers to use the helper.
157-193: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueValidation and update are not atomic.
Line 159 reads
current, then line 172 writes. Two concurrent PATCH requests can each validate against a state the other replaces, so the storedparams/timezonepair can differ from the pair that was validated. The window is small and the route is admin-only, so this is a hardening note rather than a defect. Wrapping the read and the write in one transaction closes it, and the same transaction can carry the audit row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/schedules.ts` around lines 157 - 193, Make the schedule validation and update in the PATCH handler atomic by wrapping the current-row read, assertSchedulableParams call, and taskSchedules update in a single database transaction. Use the transaction handle consistently for all operations in this flow, preserve the existing not-found behavior, and ensure any audit-row write associated with the update uses the same transaction.apps/worker/src/index.ts (1)
285-294: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne failed registration stops the remaining ones.
Line 290 registers every schedule inside a single
try. IfregisterSchedulerejects for one row, the loop aborts and every schedule after it stays unregistered until the next boot. The warning at line 293 does not name the row that failed. Isolating each registration keeps the rest of the reconciliation useful.♻️ Proposed change
- for (const row of enabled) await queue.registerSchedule(row.id, row.cron, row.timezone); - deps.logger.info(`reconciled ${enabled.length} schedule(s)`); + let ok = 0; + for (const row of enabled) { + try { + await queue.registerSchedule(row.id, row.cron, row.timezone); + ok++; + } catch (err) { + deps.logger.warn("schedule registration failed", { scheduleId: row.id, err: String(err) }); + } + } + deps.logger.info(`reconciled ${ok}/${enabled.length} schedule(s)`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/index.ts` around lines 285 - 294, Update the schedule reconciliation loop around queue.registerSchedule so each row’s registration is handled independently: catch failures per schedule, log a warning that includes the affected row.id and error details, and continue registering subsequent rows. Keep the outer try/catch for query-level reconciliation failures and the final reconciled count log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/lib/bearer-tokens.ts`:
- Around line 53-58: Update bearerToken to recognize the Bearer authentication
scheme case insensitively while preserving the existing whitespace trimming and
empty-token behavior. Change only the scheme check in bearerToken; continue
returning the extracted token for valid credentials and null for absent,
unrelated, or empty values.
In `@apps/api/src/routes/execution.ts`:
- Around line 333-336: Update the retry run insertion associated with the
authorization in this route to set createdBy from c.var.principal.userId instead
of c.var.user.id, keeping the row attribution consistent with assertProjectRole
and the authorized principal; leave assertProjectAcceptsWork unchanged.
In `@apps/api/src/routes/schedules.ts`:
- Around line 119-142: Wrap each schedule mutation and its corresponding audit
call in the POST, PATCH, and DELETE handlers using c.var.db.transaction(...).
Pass the transaction object as the third argument to audit(c, { ... }, tx), and
ensure the mutation result and existing validation behavior are preserved within
the transaction callback.
In `@apps/api/src/test/schedules.integration.test.ts`:
- Around line 370-374: Remove the unused orgId alias from the select projection
in the query that assigns anyRun, retaining only runs.id because downstream
logic uses anyRun?.id exclusively.
- Around line 352-360: Wrap the shared task type disable/assertion flow in the
test “disables when its task type is gone” with a try/finally so
taskTypes.enabled is restored even when fireSchedule or its assertion fails.
Apply the same try/finally cleanup to the quota mutation in the quota test,
ensuring the project quota is restored regardless of assertion or execution
failures.
In `@apps/web/src/pages/SettingsPage.tsx`:
- Around line 4-7: Remove the unused SCHEDULE_CONCURRENCY_POLICIES,
ScheduleConcurrencyPolicy, and useQueries imports from SettingsPage.tsx at lines
4-7, and remove the unused TaskTypeSummary import at line 63. Apply Biome
formatting and ensure bun run check passes.
- Around line 1174-1178: Update the clipboard click handler around lastKey.key
to safely handle navigator.clipboard being unavailable and writeText failures,
showing a translated failure toast via settings:apiKeys.copyFailed when copying
cannot complete. Add the apiKeys.copyFailed translation to both English and
zh-CN settings locale resources while preserving existing success behavior.
In `@apps/worker/src/deps/notify.ts`:
- Line 94: Update the reason handling in the notification payload around
ctx.eventPayload.reason to resolve the ScheduleDisabledReason through the
notifications namespace’s scheduleDisabledReasons map instead of passing the raw
enum value. Add matching scheduleDisabledReasons entries for every supported
reason to both the English and zh-CN notifications locale files, and use the
configured locale so rendered messages remain natural.
- Around line 95-97: Update the error formatting expression in the notification
handler around the error variable to branch on its runtime type rather than
relying on the cast. Preserve structured `{ code, message }` formatting for
object errors, while returning the plain string directly when
`ctx.eventPayload.error` is a string so schedule.failed notifications do not
render an empty message.
In `@apps/worker/src/index.ts`:
- Around line 270-278: Update the logging in the queue worker’s schedule-fire
handler after fireSchedule to include the outcome detail alongside outcome.kind:
use outcome.error for failed outcomes and outcome.reason for disabled or skipped
outcomes. Preserve the existing success logging and avoid changing fireSchedule
behavior.
In `@docs/design/04-execution-runtime.md`:
- Line 146: The documentation incorrectly names the nonexistent
assertProjectRole helper. Update the execution-runtime passage to reference
fireSchedule’s actual projectMembers lookup and
projectRoleAtLeast(membership.role, "member") check, or describe that fire-time
membership check without naming a helper.
In `@docs/design/05-api-and-auth.md`:
- Line 13: Update the Governance endpoint list to remove the stale org-level
CRUD /api-keys entry, then add the API-key endpoints under Projects using
POST/GET /api/v1/projects/:projectId/api-keys and POST
/api/v1/projects/:projectId/api-keys/:id/revoke, matching the documented
project-admin surface.
In `@packages/core/src/schemas.ts`:
- Around line 302-304: Update cronField to use superRefine so the submitted cron
value v is directly available when constructing validation issues. Derive the
error message with validateCron(v), preserving the existing generic fallback
while avoiding reliance on issue.input.
In `@packages/i18n/locales/en/notifications.json`:
- Around line 30-37: Update the schedule-disabled notification flow around
schedule-fire and notify so the raw reason identifier is translated via the
settings:schedules.reasons.<reason> key before constructing the notification
payload. Apply this localization for both supported locales, while preserving
the existing scheduleDisabled template and fallback behavior for unknown
reasons.
In `@packages/orchestration/src/notifications.ts`:
- Around line 104-150: Make run-less project notifications idempotent in the
schedule firing flow: update the code around fireSchedule so notifyProjectEvent
runs only after unregisterSchedule succeeds, preventing a retried worker job
from creating duplicate deliveries. Preserve the existing error propagation and
transition behavior, and do not rely on the event_id-based
notification_deliveries_dedupe_uq for this run-less path.
In `@packages/orchestration/src/schedule-fire.ts`:
- Line 54: Update the schedule checks in the relevant flow, including the guard
near line 54 and the corresponding check near line 100, to use optional chaining
as required by Biome’s useOptionalChain rule while preserving their existing
behavior and return values.
- Around line 56-79: Update the disable function so the enabled=false state
change and auditAs record are committed atomically in one database transaction.
Keep notifyProjectEvent and queue?.unregisterSchedule as best-effort operations
inside a try/catch after the transaction, ensuring their failures do not
propagate from fireSchedule while preserving the disabled outcome.
- Around line 181-193: Update the catch in fireSchedule so only AppError and the
intended recoverable SubmitError classification are converted through
fail(message); rethrow all other errors, including plain infrastructure Error
instances from submitTask, so the scheduled worker job can retry. Preserve the
existing AppError message formatting and verify SubmitError is included in the
recoverable submit-path classification.
---
Nitpick comments:
In `@apps/api/src/lib/bearer-tokens.ts`:
- Around line 47-51: Update issueToken to validate that the supplied prefix is
shorter than DISPLAY_PREFIX_LENGTH before concatenating and deriving
tokenPrefix. Throw an invariant error at the call site when the prefix is too
long, while preserving the existing token generation and hashing behavior for
valid prefixes.
In `@apps/api/src/middleware/auth.ts`:
- Around line 52-58: Update the API-key denial branches in the authentication
middleware to use distinct stable error codes for endpoint-unavailable and
missing-scope cases. Move the required scope from the interpolated message into
the error details so each message can localize solely through its code, and add
matching English and zh-CN entries in the errors namespace for both codes.
In `@apps/api/src/routes/api-keys.ts`:
- Around line 57-78: In the API key creation handler, add an explicit guard
immediately after the insert and before audit creation, using the returned row
from the insert operation. Throw the same established error used by the sibling
schedule route when no row is returned, and preserve the existing audit and 201
response flow for successful inserts.
In `@apps/api/src/routes/catalog.ts`:
- Line 3: Reuse the centralized DEFAULT_EXECUTOR from `@agrippa/orchestration`
instead of redefining it locally. In apps/api/src/routes/catalog.ts at lines
3-3, import DEFAULT_EXECUTOR and remove the local declaration at line 8; in
apps/api/src/routes/projects.ts at lines 38-43, add the same import and remove
the local defaultExecutor declaration at line 638, updating references as
needed.
In `@apps/api/src/routes/schedules.ts`:
- Around line 49-66: Extract the repeated task-type → template →
published-version resolution and compilation logic from the schedule route and
submitTask into a shared `@agrippa/orchestration` helper, such as
loadCompiledTemplateForTaskType(db, taskTypeId). Preserve the existing
enabled-task-type, template_unpublished, missing-version, and
upgradeCompiledTemplate behaviors, then update both callers to use the helper.
- Around line 157-193: Make the schedule validation and update in the PATCH
handler atomic by wrapping the current-row read, assertSchedulableParams call,
and taskSchedules update in a single database transaction. Use the transaction
handle consistently for all operations in this flow, preserve the existing
not-found behavior, and ensure any audit-row write associated with the update
uses the same transaction.
In `@apps/api/src/test/api-keys.integration.test.ts`:
- Around line 255-270: Update the test “stops working when its owner loses
project membership” to avoid permanently deleting the shared project_members
row: either create and use a dedicated project for this scenario or restore the
owner’s membership after asserting the 403 response. Keep the membership state
required by other tests intact regardless of test execution order.
In `@apps/api/src/test/api.integration.test.ts`:
- Around line 194-206: Extend the archived-project scenario around the existing
submit assertion to call POST /tasks/:id/retry for the pre-existing task, then
assert a 409 response with error code project_archived. Reuse the existing
project and task identifiers and request helpers so retry coverage verifies the
assertProjectAcceptsWork guard without changing the submission or history
checks.
In `@apps/api/src/test/schedules.integration.test.ts`:
- Around line 407-423: Update the test “does not fire while disabled, and clears
its reason when re-enabled” to seed non-null disabledReason and lastError values
on the created schedule before disabling it. Keep the existing disable,
re-enable, and null assertions so the test verifies the PATCH handler actually
clears previously stored error state.
- Around line 274-281: Update the notificationEndpoints insert to obtain
createdBy directly from the admin user record instead of creating a schedule and
querying its row. Add the users symbol to the existing `@agrippa/db` import and
use the established admin user lookup, removing the nested createSchedule/rowOf
expression and its extra schedule side effect.
- Around line 141-155: Extend the “unregisters the calendar when paused or
deleted” test to assert audit rows for both mutations: verify the PATCH produces
a schedule.update audit entry and the DELETE produces a schedule.delete entry,
using the existing audit-row query/assertion pattern from the schedule.create
check near line 108.
- Around line 229-268: Strengthen the ownership assertion in the “submits a run
attributed to its owner” test by comparing run.createdBy with the schedule row’s
createdBy, not merely checking truthiness. Extend the skip-policy coverage in
“honors each concurrency policy against an unfinished previous run” with a
terminal previous run, then verify firing submits a new run and updates
lastRunId instead of skipping.
- Around line 49-58: Reset the shared registered and unregistered arrays before
each test by importing and registering beforeEach from bun:test, so each test
starts with empty spy state. In the assertion around registered, use
registered[0] rather than registered.at(-1) after the reset.
- Around line 175-199: Extend the schedule integration tests near “rejects an
unknown token...” with a case placing "{{today}}" in a numeric or boolean
parameter and assert the POST returns 400. Update the range test around its
existing date-range assertions so the implementation and comment agree: either
assert the range ends the day before Monday or reword the comment to describe
only ordering and pastness.
In `@apps/web/src/lib/types.ts`:
- Line 377: Update the type containing concurrencyPolicy in types.ts to use the
shared ScheduleConcurrencyPolicy imported from `@agrippa/core` instead of the
duplicated string union, preserving the existing property name and contract.
In `@apps/web/src/pages/SettingsPage.tsx`:
- Around line 1290-1294: Update the Switch in the schedule row to include the
specific schedule’s identifying name in its accessible label, rather than using
the shared settings:schedules.toggle translation alone. Preserve the existing
checked state and update.mutate behavior while ensuring each row’s control is
distinguishable to screen readers.
In `@apps/worker/src/index.ts`:
- Around line 285-294: Update the schedule reconciliation loop around
queue.registerSchedule so each row’s registration is handled independently:
catch failures per schedule, log a warning that includes the affected row.id and
error details, and continue registering subsequent rows. Keep the outer
try/catch for query-level reconciliation failures and the final reconciled count
log.
In `@docs/design/04-execution-runtime.md`:
- Around line 142-147: Move the three scheduled-submission paragraphs out from
under “### Notifications (outbound)” into a new “### Scheduled submission”
section near the existing schedule.fire bullet, preserving their content and
order. In the token-validation paragraph, explicitly state that both schedule
creation and PATCH edits reject unknown tokens and validate resolved parameters
against the compiled schema.
In `@packages/core/src/schedules.test.ts`:
- Around line 153-162: Add a parity test in schedules.test.ts that imports
SCHEDULE_TOKENS and verifies every token classified as known by
findUnknownScheduleTokens has a corresponding key in scheduleTokenValues used by
applyScheduleTokens. Keep the assertion focused on detecting drift between these
two token sets.
In `@packages/orchestration/src/submit.ts`:
- Around line 106-118: Update submitTask and the project verification flow
around assertProjectAcceptsWork to retain the verified project row, then set the
tasks insert’s orgId from that row’s projects.orgId instead of actor.orgId.
Preserve the existing project-role and work-acceptance checks while ensuring
downstream org-scoped records use the target project’s organization.
In `@packages/orchestration/src/usage.ts`:
- Around line 95-109: Update assertQuotaHeadroom to query the current month’s
token total directly instead of calling projectUsage, preserving the existing
quota limit comparison and error behavior. Reuse the existing periodTokens
helper or equivalent total-query symbol, and update projectUsage to obtain its
tokens field through periodTokens while retaining its other display metrics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 750df31b-f068-41e5-877b-fc2fffedf472
📒 Files selected for processing (66)
CHANGELOG.mdapps/api/src/app.tsapps/api/src/context.tsapps/api/src/lib/api-key-routes.tsapps/api/src/lib/audit.tsapps/api/src/lib/bearer-tokens.tsapps/api/src/lib/runtime-tokens.tsapps/api/src/middleware/auth.tsapps/api/src/middleware/rbac.tsapps/api/src/routes/api-keys.tsapps/api/src/routes/catalog.tsapps/api/src/routes/execution.tsapps/api/src/routes/invitations.tsapps/api/src/routes/projects.tsapps/api/src/routes/schedules.tsapps/api/src/test/api-keys.integration.test.tsapps/api/src/test/api.integration.test.tsapps/api/src/test/checkpoints.integration.test.tsapps/api/src/test/execution.integration.test.tsapps/api/src/test/fleet.integration.test.tsapps/api/src/test/helpers.tsapps/api/src/test/notifications.integration.test.tsapps/api/src/test/runtimes.integration.test.tsapps/api/src/test/schedules.integration.test.tsapps/web/src/lib/types.tsapps/web/src/pages/SettingsPage.tsxapps/web/src/pages/SubmitTaskPage.tsxapps/worker/src/deps/notify.tsapps/worker/src/index.tsdocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/manual/en/04-administration.mddocs/manual/zh-CN/04-administration.mdpackages/core/src/api-keys.tspackages/core/src/index.tspackages/core/src/notifications.tspackages/core/src/queue.tspackages/core/src/schedules.test.tspackages/core/src/schedules.tspackages/core/src/schemas.tspackages/db/drizzle/0023_api_keys_prefix_uq.sqlpackages/db/drizzle/0024_task_schedules.sqlpackages/db/drizzle/meta/0023_snapshot.jsonpackages/db/drizzle/meta/0024_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/api-keys.tspackages/db/src/schema/index.tspackages/db/src/schema/schedules.tspackages/i18n/locales/en/catalog.jsonpackages/i18n/locales/en/errors.jsonpackages/i18n/locales/en/notifications.jsonpackages/i18n/locales/en/settings.jsonpackages/i18n/locales/zh-CN/catalog.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/i18n/locales/zh-CN/notifications.jsonpackages/i18n/locales/zh-CN/settings.jsonpackages/orchestration/src/audit.tspackages/orchestration/src/engine/run-lifecycle.tspackages/orchestration/src/index.tspackages/orchestration/src/notifications.tspackages/orchestration/src/queue.tspackages/orchestration/src/run-plan.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/submit.tspackages/orchestration/src/usage.tspackages/orchestration/src/worker-executors.ts
Comment on lines
+333
to
+336
| await assertProjectRole(db, c.var.principal, task.projectId, "member"); | ||
| // a retry is new work and consumes tokens like any run — an archived | ||
| // project refuses it for the same reason it refuses a submission | ||
| await assertProjectAcceptsWork(db, task.projectId); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retry authorization now uses the principal, but the new run row is still attributed to c.var.user.id.
Line 333 authorizes with c.var.principal. The run insert at line 407 sets createdBy: c.var.user.id. This is the same identity mismatch flagged on the submit route. Use c.var.principal.userId for createdBy so the row attribution matches the authorized principal.
The added assertProjectAcceptsWork call is correct and matches the submit path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/execution.ts` around lines 333 - 336, Update the retry
run insertion associated with the authorization in this route to set createdBy
from c.var.principal.userId instead of c.var.user.id, keeping the row
attribution consistent with assertProjectRole and the authorized principal;
leave assertProjectAcceptsWork unchanged.
Comment on lines
+352
to
+360
| it("disables when its task type is gone", async () => { | ||
| const row = await createSchedule(); | ||
| await db.update(taskTypes).set({ enabled: false }).where(eq(taskTypes.id, taskTypeId)); | ||
| expect(await fireSchedule(db, queue, row.id)).toEqual({ | ||
| kind: "disabled", | ||
| reason: "task_type_gone", | ||
| }); | ||
| await db.update(taskTypes).set({ enabled: true }).where(eq(taskTypes.id, taskTypeId)); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the shared task type in a finally block.
Line 354 disables the taskTypes row that every other test in this file uses, and line 359 restores it in the test body. If the assertion on lines 355-358 fails, the restore never runs. The next test at line 362 then fires a schedule against a disabled task type, so it receives { kind: "disabled", reason: "task_type_gone" } instead of the expected failed outcome, and the quota assertions fail for an unrelated reason. One real failure becomes a cascade that hides its own cause.
💚 Proposed fix
it("disables when its task type is gone", async () => {
const row = await createSchedule();
await db.update(taskTypes).set({ enabled: false }).where(eq(taskTypes.id, taskTypeId));
- expect(await fireSchedule(db, queue, row.id)).toEqual({
- kind: "disabled",
- reason: "task_type_gone",
- });
- await db.update(taskTypes).set({ enabled: true }).where(eq(taskTypes.id, taskTypeId));
+ try {
+ expect(await fireSchedule(db, queue, row.id)).toEqual({
+ kind: "disabled",
+ reason: "task_type_gone",
+ });
+ } finally {
+ await db.update(taskTypes).set({ enabled: true }).where(eq(taskTypes.id, taskTypeId));
+ }
});The quota test at lines 362-405 has the same shape: it restores the project quota on line 401 in the test body. Apply the same treatment there.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("disables when its task type is gone", async () => { | |
| const row = await createSchedule(); | |
| await db.update(taskTypes).set({ enabled: false }).where(eq(taskTypes.id, taskTypeId)); | |
| expect(await fireSchedule(db, queue, row.id)).toEqual({ | |
| kind: "disabled", | |
| reason: "task_type_gone", | |
| }); | |
| await db.update(taskTypes).set({ enabled: true }).where(eq(taskTypes.id, taskTypeId)); | |
| }); | |
| it("disables when its task type is gone", async () => { | |
| const row = await createSchedule(); | |
| await db.update(taskTypes).set({ enabled: false }).where(eq(taskTypes.id, taskTypeId)); | |
| try { | |
| expect(await fireSchedule(db, queue, row.id)).toEqual({ | |
| kind: "disabled", | |
| reason: "task_type_gone", | |
| }); | |
| } finally { | |
| await db.update(taskTypes).set({ enabled: true }).where(eq(taskTypes.id, taskTypeId)); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/test/schedules.integration.test.ts` around lines 352 - 360, Wrap
the shared task type disable/assertion flow in the test “disables when its task
type is gone” with a try/finally so taskTypes.enabled is restored even when
fireSchedule or its assertion fails. Apply the same try/finally cleanup to the
quota mutation in the quota test, ensuring the project quota is restored
regardless of assertion or execution failures.
Comment on lines
+370
to
+374
| const [anyRun] = await db | ||
| .select({ id: runs.id, orgId: runs.projectId }) | ||
| .from(runs) | ||
| .where(eq(runs.projectId, projectId)) | ||
| .limit(1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the mislabeled orgId alias.
Line 371 selects orgId: runs.projectId. The alias names an org id but reads a project id. Only anyRun?.id is used on line 382, so nothing breaks, but the field misstates what it holds and invites a wrong copy.
♻️ Proposed change
const [anyRun] = await db
- .select({ id: runs.id, orgId: runs.projectId })
+ .select({ id: runs.id })
.from(runs)
.where(eq(runs.projectId, projectId))
.limit(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [anyRun] = await db | |
| .select({ id: runs.id, orgId: runs.projectId }) | |
| .from(runs) | |
| .where(eq(runs.projectId, projectId)) | |
| .limit(1); | |
| const [anyRun] = await db | |
| .select({ id: runs.id }) | |
| .from(runs) | |
| .where(eq(runs.projectId, projectId)) | |
| .limit(1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/test/schedules.integration.test.ts` around lines 370 - 374,
Remove the unused orgId alias from the select projection in the query that
assigns anyRun, retaining only runs.id because downstream logic uses anyRun?.id
exclusively.
| scheduleId: string, | ||
| ): Promise<ScheduleFireOutcome> { | ||
| const [schedule] = await db.select().from(taskSchedules).where(eq(taskSchedules.id, scheduleId)); | ||
| if (!schedule || !schedule.enabled) return { kind: "skipped", reason: "disabled" }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply the Biome useOptionalChain fix at lines 54 and 100.
CI reports lint/complexity/useOptionalChain at both lines. The coding guidelines require bun run check to pass.
🧹 Proposed fix
- if (!schedule || !schedule.enabled) return { kind: "skipped", reason: "disabled" };
+ if (!schedule?.enabled) return { kind: "skipped", reason: "disabled" };- if (!project || project.status !== "active") return disable("project_archived");
+ if (project?.status !== "active") return disable("project_archived");As per coding guidelines: "Use Biome formatting and linting, and keep bun run check passing".
🧰 Tools
🪛 GitHub Check: ci
[warning] 54-54: lint/complexity/useOptionalChain
Change to an optional chain.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/orchestration/src/schedule-fire.ts` at line 54, Update the schedule
checks in the relevant flow, including the guard near line 54 and the
corresponding check near line 100, to use optional chaining as required by
Biome’s useOptionalChain rule while preserving their existing behavior and
return values.
Sources: Coding guidelines, Linters/SAST tools
A signed URL an external system POSTs to in order to start a task. Track N's
outbound pipeline inverted — a token plus a signing secret, a durable delivery
log with payload inspection, replay — with the HMAC verified rather than
generated, using deliberately the same x-agrippa-timestamp / x-agrippa-signature
convention the platform sends. One dialect in both directions beats two.
Three decisions worth recording.
The token travels in the URL path, not a header, because the senders that
matter — CI runners, IM bots, hosted git providers — can all POST to a URL and
many cannot set Authorization. That makes the URL a capability, which is
exactly why the signing secret is *mandatory* here although it is optional on
outbound IM endpoints: an unsigned inbound trigger is an open "spend this
project's tokens" endpoint for anyone who ever sees the URL in a proxy log or a
CI transcript. The signature covers the raw request bytes, never a
re-serialization, and timestamps outside ±5 minutes are refused in both
directions so a fast sender clock cannot buy a longer replay window.
The request records and acknowledges; the worker submits. The delivery row is
written before the 202, so a submission that fails later stays visible and
replayable instead of vanishing with the sender's connection, and a slow submit
can never become a timeout the sender retries into a duplicate run. An optional
x-agrippa-delivery-id makes that idempotent outright via a partial unique
index, and fireTrigger refuses a delivery already marked succeeded — so
at-least-once job delivery and an operator hitting replay converge on one run.
Because the sender is acknowledged before the run exists, it can never learn
that the run did not happen. trigger.disabled and trigger.failed notifications
are therefore not a nicety but the only channel that reaches a human — the same
argument that made schedules disable loudly rather than skip. Both features now
share checkWorkAuthority for the three permanent conditions, since both needed
exactly that check at exactly the same moment.
The received payload is stored for inspection and never interpolated into a
prompt: a valid signature proves who sent the bytes, not that the bytes are
safe. Mapping payload fields into task parameters is deliberately out of scope
— it is a prompt-injection surface that deserves its own design.
One bug the browser smoke caught that the unit tests did not: creation
validates the *resolved* parameters (sharing the schedule validator), so it
accepted a {{lastWeekStart}} token — but firing passed endpoint.params
straight through, and the token reached the agent's prompt verbatim. Triggers
now carry a timezone and resolve tokens at fire time exactly as schedules do,
because the same token typed into the same form field has to mean the same
thing in both places. Covered by a regression test asserting the run's snapshot
holds dates while the stored parameter keeps the token.
Verified end to end against a live stack, not only in tests: created a trigger
through the browser, POSTed a correctly signed request (202) plus a
wrong-signature and a stale-timestamp one (both the same uniform 401), repeated
the request with the same delivery id (deduplicated, still one delivery row),
and confirmed the worker produced a run whose dateRange resolved to
2026-07-27..2026-08-02. 14 integration tests cover the rest.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orchestration/src/schedule-fire.ts (1)
135-161: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftA failed bookkeeping update after a successful submit is misreported as a submission failure.
The
tryblock wraps bothsubmitTask(line 136) and the follow-updb.update(taskSchedules)call (lines 151-160). IfsubmitTasksucceeds but the update throws, thecatchat line 162 treats it the same as a submit failure: it callsfail(), which writes a misleadinglastErrorand emitsschedule.failed, even though a task and run were already created and enqueued.lastRunId/lastFiredAtstay stale, so the overlap check at lines 105-127 evaluates the wrong prior run on the next firing.Separate the bookkeeping update from the submission call so an update failure does not overwrite a genuine "submitted" outcome.
🛠️ Proposed fix
try { - const { taskId, runId } = await submitTask(db, queue, { + var submitResult; + submitResult = await submitTask(db, queue, { projectId: schedule.projectId, actorUserId: schedule.createdBy, actor: { orgId: schedule.orgId, userId: schedule.createdBy }, input: { taskTypeId: schedule.taskTypeId, title: schedule.name, params: applyScheduleTokens(schedule.params, firedAt, schedule.timezone), agents: schedule.agentOverrides, }, }); - await db - .update(taskSchedules) - .set({ - lastFiredAt: firedAt, - lastRunId: runId, - lastError: null, - lastErrorAt: null, - updatedAt: new Date(), - }) - .where(eq(taskSchedules.id, scheduleId)); - return { kind: "submitted", taskId, runId }; } catch (err) { const message = err instanceof AppError ? `${err.code}: ${err.message}` : err instanceof Error ? err.message : String(err); return fail(message); } + // bookkeeping happens outside the submit try/catch: a failure here must + // never be reported as a submission failure, since the run already exists + await db + .update(taskSchedules) + .set({ + lastFiredAt: firedAt, + lastRunId: submitResult.runId, + lastError: null, + lastErrorAt: null, + updatedAt: new Date(), + }) + .where(eq(taskSchedules.id, scheduleId)); + return { kind: "submitted", taskId: submitResult.taskId, runId: submitResult.runId };Note this restructuring lets the bookkeeping-update exception propagate out of
fireScheduleon failure; confirm that is acceptable givensubmitTaskalready committed, or wrap it in its owntry/catchthat logs rather than reclassifies the outcome, since a plain rethrow here would let pg-boss retry the whole firing and resubmit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/schedule-fire.ts` around lines 135 - 161, Separate submitTask from the taskSchedules bookkeeping update in fireSchedule: only submission errors should invoke fail() and produce a failed outcome. After a successful submission, update lastFiredAt, lastRunId, and related fields in a distinct error-handling path that logs bookkeeping failures without reclassifying or retrying the already-submitted task; preserve the submitted result.
♻️ Duplicate comments (4)
packages/orchestration/src/schedule-fire.ts (2)
162-174: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPlain
Errors fromsubmitTaskare still recorded as recoverable schedule failures instead of propagating for a pg-boss retry.Every thrown value that is not an
AppErroris converted into afail()outcome viaerr.message. An infrastructure-levelError(a DB error insidesubmitTask, a queue-name derivation throw) is indistinguishable here from a legitimate configuration problem likeusage_limit_exceeded. BecausefireSchedulereturns normally afterfail(), pg-boss does not retry the underlying job for an infrastructure fault.This is the same finding raised on a prior revision of this file and it is still present unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/schedule-fire.ts` around lines 162 - 174, Update the catch handling in fireSchedule so only AppError instances are converted to the recoverable fail(message) outcome; rethrow plain Error and other unexpected thrown values so pg-boss can retry infrastructure failures from submitTask or queue-name derivation.
56-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
disablecan fail silently between the state flip and the announcement.
disablewritesenabled = falsefirst, then the audit row, then the notification, thenunregisterSchedule. If any of the latter three throws, the exception propagates out offireSchedule. On the pg-boss retry, line 54's!schedule?.enabledcheck returnsskippedimmediately, so the audit row, theschedule.disablednotification, and the calendar unregistration never happen. The schedule stops firing with no record of why.This is the same finding raised on a prior revision of this file and it is still present unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/schedule-fire.ts` around lines 56 - 79, Update the disable function so marking the schedule disabled and completing its audit, notification, and queue unregistration are resilient to failures after the database state change. Ensure those follow-up actions are retried or otherwise completed independently of the early !schedule?.enabled path, while preserving the disabled outcome and reason.docs/design/04-execution-runtime.md (1)
147-147: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc still names a helper that does not exist in the implementation.
The text states the handler applies
assertProjectRole(owner, project, "member"). The current implementation callscheckWorkAuthority(packages/orchestration/src/schedule-fire.tslines 96-101,packages/orchestration/src/authority.tslines 27-52), which has no function namedassertProjectRole. A reader who greps forassertProjectRolefinds nothing in the schedule path.Name the helper that is actually used, or describe the fire-time membership check without naming a specific symbol.
This is the same finding raised on a prior revision of this file and it is still present unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/04-execution-runtime.md` at line 147, Update the schedule authority paragraph to reference the implemented checkWorkAuthority helper instead of assertProjectRole, or describe the fire-time membership validation without naming a helper. Keep the existing explanation of owner-based authorization and attribution unchanged.apps/worker/src/index.ts (1)
283-291: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWorker job consumers drop failure detail from structured logs.
fireScheduleandfireTriggerboth return outcome unions that carryerror(forfailed) orreason(fordisabled/skipped) alongsidekind, but both consumers log onlyoutcome.kind. This is the shared root cause behind both sites; a schedule or trigger that goes silent in production shows no reason in the worker log.
apps/worker/src/index.ts#L283-L291: includeoutcome.error/outcome.reasonalongsideoutcome.kindwhen logging the trigger delivery outcome.apps/worker/src/index.ts#L273-L281: includeoutcome.error/outcome.reasonalongsideoutcome.kindwhen logging the schedule-fire outcome, as requested on a prior revision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/index.ts` around lines 283 - 291, Update both the schedule-fire consumer at apps/worker/src/index.ts lines 273-281 and the trigger delivery consumer at apps/worker/src/index.ts lines 283-291 to include the outcome’s error or reason alongside outcome.kind in their structured logger calls. Preserve the existing outcome handling and use the appropriate detail for failed, disabled, and skipped outcomes.
🧹 Nitpick comments (10)
packages/orchestration/src/authority.ts (1)
15-15: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive
WorkAuthorityFailurefrom the shared reason list.
WorkAuthorityFailurelists the same three values asTRIGGER_DISABLED_REASONSinpackages/core/src/triggers.ts. The two declarations are independent.fireTriggerwrites the result ofcheckWorkAuthoritystraight intotrigger_endpoints.disabled_reason, so if one list gains a member and the other does not, the persisted value falls outside the vocabulary the API and SPA expect, and no compiler error reports it.Declare one union and derive the other, or export a single shared list from
@agrippa/core.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/authority.ts` at line 15, Update WorkAuthorityFailure in authority.ts to derive its union from the shared TRIGGER_DISABLED_REASONS definition in `@agrippa/core`, or centralize both declarations on one exported shared list. Ensure checkWorkAuthority and trigger_endpoints.disabled_reason use the same source of truth so any future reason additions are compiler-checked.packages/core/src/schemas.ts (1)
358-369: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd secret rotation to the trigger update contract.
triggerUpdateSchemaomitssecret. The PATCH handler inapps/api/src/routes/triggers.tstherefore never writessecretRef. If a signing secret leaks, an admin cannot rotate it. The only remedy is delete plus recreate, which also invalidates the URL token and forces every sender to be reconfigured.Consider adding an optional
secretfield with the same length bounds astriggerCreateSchema, and re-encrypt the stored secret when it is present.♻️ Proposed contract change
export const triggerUpdateSchema = z .object({ name: z.string().min(1).max(200), timezone: timezoneField, params: z.record(z.string(), z.unknown()), agents: z.record( z.string(), z.object({ executorId: z.string().min(1).optional(), faberId: z.uuid().optional() }), ), enabled: z.boolean(), + /** Write-only; rotates the stored signing secret. */ + secret: z.string().min(16).max(200), }) .partial();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/schemas.ts` around lines 358 - 369, Extend triggerUpdateSchema with an optional secret field using the same validation and length bounds as triggerCreateSchema, then update the PATCH handler to detect a provided secret, re-encrypt it, and persist the resulting secretRef while preserving the existing secret when omitted.packages/db/src/schema/triggers.ts (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
disabledReasonwith the shared vocabulary.
statusontriggerDeliveriesnarrows its values throughtext("status", { enum: [...] }), butdisabledReasonstays untypedtext.packages/core/src/triggers.tsalready exportsTriggerDisabledReasonas a closed set of three values. Narrowing the column makes the writer inpackages/orchestration/src/trigger-fire.tstype-checked against that set, so the column and the exported union cannot drift apart.♻️ Proposed typing change
- disabledReason: text("disabled_reason"), + disabledReason: text("disabled_reason").$type<TriggerDisabledReason>(),Add the import:
+import type { TriggerDisabledReason } from "`@agrippa/core`";Note: confirm the dependency direction is allowed before adding this import. If
packages/dbmust not import@agrippa/core, keep the column untyped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/schema/triggers.ts` at line 72, Update the disabledReason column in the triggerDeliveries schema to use the shared TriggerDisabledReason vocabulary from packages/core, matching the enum-style typing used by status. First verify that packages/db may depend on `@agrippa/core`; if that dependency is disallowed, leave disabledReason untyped rather than introducing an invalid import.apps/web/src/pages/SettingsPage.tsx (2)
1077-1080: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider polling trigger deliveries.
A delivery row starts in
pendingand the worker updates it asynchronously. This query has norefetchInterval, so the list stays stale until the user remounts the section.NotificationsSectionpolls its deliveries every 10 seconds at line 806.♻️ Proposed change
const deliveries = useQuery({ queryKey: ["trigger-deliveries", projectId], queryFn: () => api<TriggerDeliveryRow[]>(`/projects/${projectId}/triggers/deliveries?limit=30`), + refetchInterval: 10_000, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/pages/SettingsPage.tsx` around lines 1077 - 1080, Update the trigger deliveries useQuery in SettingsPage to poll for refreshed results by adding a 10-second refetch interval, matching NotificationsSection’s existing delivery polling behavior while preserving the current query key and query function.
1236-1236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the default scopes against
API_KEY_SCOPES.The default scope list uses bare strings. If a scope identifier in
@agrippa/corechanges, the default no longer matches the allow-list and the API rejects the create request. The compiler cannot detect this.♻️ Proposed change
- const [scopes, setScopes] = useState<string[]>(["tasks:write", "runs:read"]); + type ApiKeyScope = (typeof API_KEY_SCOPES)[number]; + const [scopes, setScopes] = useState<ApiKeyScope[]>(["tasks:write", "runs:read"]);Adjust
toggleScopeto acceptApiKeyScope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/pages/SettingsPage.tsx` at line 1236, Type the default scopes and toggleScope state updates against the API_KEY_SCOPES allow-list by using the ApiKeyScope type instead of bare string identifiers. Update toggleScope to accept ApiKeyScope so renamed or removed scope values are caught by the compiler while preserving the existing defaults.apps/web/src/pages/SubmitTaskPage.tsx (1)
277-282: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMask the signing secret input.
This field holds a shared HMAC secret, but it renders as plain text. The notification secret field in
apps/web/src/pages/SettingsPage.tsxusestype="password"withautoComplete="off". Match that behavior.♻️ Proposed change
<Input id="trigger-secret" + type="password" + autoComplete="off" value={secret} onChange={(e) => setSecret(e.target.value)} placeholder={t("catalog:trigger.secretPlaceholder")} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/pages/SubmitTaskPage.tsx` around lines 277 - 282, Update the signing secret Input identified by id "trigger-secret" in SubmitTaskPage to use password masking and disable browser autocomplete, matching the notification secret field’s behavior in SettingsPage while preserving its existing value and change handling.apps/api/src/test/triggers.integration.test.ts (2)
103-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the seeded task type in
beforeAll.Line 106 casts a possibly missing id with
as string. If theweekly-reportseed changes,taskTypeIdbecomesundefinedand every later test fails with an unrelated validation error. Addexpect(taskTypeId).toBeTruthy()so the setup failure is reported at its source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/triggers.integration.test.ts` around lines 103 - 107, Add an explicit expect(taskTypeId).toBeTruthy() assertion in the beforeAll setup immediately after locating the "weekly-report" task type, before later tests use taskTypeId, so missing seed data fails during setup rather than through unrelated validation errors.
251-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the oversized body from
TRIGGER_PAYLOAD_MAX_BYTES.The literal
70_000assumes the current cap. If the constant is raised, this test no longer exceeds the limit and the 413 assertion fails for an unrelated reason. Import the constant and size the body from it.♻️ Proposed change
- const res = await send(t.token as string, { blob: "x".repeat(70_000) }); + const res = await send(t.token as string, { + blob: "x".repeat(TRIGGER_PAYLOAD_MAX_BYTES + 1_000), + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/triggers.integration.test.ts` around lines 251 - 255, Update the “caps the payload size” test to import and use TRIGGER_PAYLOAD_MAX_BYTES, constructing a payload larger than that configured limit instead of relying on the hardcoded 70_000 value while preserving the expected 413 response.packages/orchestration/src/trigger-fire.ts (1)
77-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite the disable mutation and its audit row in one transaction.
Lines 78-93 disable the endpoint, mark the attempt, and then audit, each as a separate statement. If the worker crashes between them, the endpoint stays disabled with no audit row.
submitTaskinpackages/orchestration/src/submit.tskeepsauditAsinside the mutation transaction for this reason. Wrap the endpoint update, the attempt mark, andauditAsin a singledb.transaction, and keepnotifyProjectEventafter the commit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/trigger-fire.ts` around lines 77 - 99, Update the denied branch in the trigger-fire flow to wrap the triggerEndpoints update, markAttempt, and auditAs calls in a single db.transaction, preserving their current operations and data. Keep notifyProjectEvent outside and after the transaction commits, then return the existing disabled result.apps/api/src/routes/trigger-inbound.ts (1)
87-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalize array payloads instead of casting with
as never.
typeof [] === "object", so a JSON array is stored inpayload, which the schema types asRecord<string, unknown>. Theas nevercast hides that mismatch from the type checker. Wrap non-object JSON values explicitly.♻️ Proposed normalization
- payload = - parsed !== null && typeof parsed === "object" ? (parsed as never) : { value: parsed }; + payload = + parsed !== null && typeof parsed === "object" && !Array.isArray(parsed) + ? (parsed as Record<string, unknown>) + : { value: parsed };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/trigger-inbound.ts` around lines 87 - 96, Update payload normalization in the raw JSON parsing block to distinguish plain object payloads from arrays before assigning to the Record<string, unknown> payload. Remove the `as never` cast, preserve scalar wrapping via `{ value: parsed }`, and explicitly normalize array values into an object-compatible representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/trigger-inbound.ts`:
- Around line 60-65: Update the inbound trigger handler around the raw-body read
to reject oversized requests before calling c.req.text(), using the request
Content-Length against TRIGGER_PAYLOAD_MAX_BYTES, and apply Hono bodyLimit
middleware to stop oversized streams during reading. Preserve the existing
exact-byte signature verification and AppError response for payloads that exceed
the configured limit.
In `@apps/api/src/routes/triggers.ts`:
- Around line 177-199: Update the limit calculation in the deliveries route
handler to convert invalid, zero, and negative query values to the default
positive limit, while capping valid values at 100. Ensure the value passed to
the Drizzle .limit call is always a finite positive number.
- Around line 157-173: Update the delete trigger handler around the returning
clause and audit flow to also return secretRef, then delete the corresponding
row from secrets in the same database transaction as the trigger endpoint
deletion. Preserve the existing not-found behavior and audit payload, and ensure
cleanup occurs only after a trigger row is successfully deleted.
In `@apps/api/src/test/triggers.integration.test.ts`:
- Around line 132-140: Add member to the project with the member role before the
request in the “is project-admin gated and rejects params that could never run”
test. Keep the existing request and 403 assertion so the test verifies an
authenticated project member is denied by the admin gate, not merely rejected
for lacking membership.
In `@apps/web/src/pages/SubmitTaskPage.tsx`:
- Around line 251-254: Update the clipboard handlers in
apps/web/src/pages/SubmitTaskPage.tsx lines 251-254 and
apps/web/src/pages/SettingsPage.tsx lines 1335-1339 to guard navigator.clipboard
before calling writeText, handle synchronous access failures, and catch rejected
promises with localized failure toasts for the trigger URL and plaintext API key
respectively. Add matching failure-message translation keys to the relevant en
and zh-CN JSON locale files while preserving locale key parity.
In `@docs/manual/zh-CN/04-administration.md`:
- Line 55: Update the intercepted-request sentence in the administration
documentation to explicitly state that even if a request is intercepted, it will
automatically expire, using the wording “即使请求被截获，也会自动失效” while leaving the
surrounding guidance unchanged.
In `@packages/orchestration/src/queue.ts`:
- Line 76: Update packages/orchestration/src/queue.ts lines 76-76 to create
QUEUE_TRIGGER_FIRE with policy: "exclusive" and apply the same
converge-by-recreate handling used by QUEUE_NOTIFICATION_DELIVER. At lines
116-125, retain singletonKey: deliveryId and revise the comment to attribute
deduplication to the exclusive queue policy rather than the key alone.
---
Outside diff comments:
In `@packages/orchestration/src/schedule-fire.ts`:
- Around line 135-161: Separate submitTask from the taskSchedules bookkeeping
update in fireSchedule: only submission errors should invoke fail() and produce
a failed outcome. After a successful submission, update lastFiredAt, lastRunId,
and related fields in a distinct error-handling path that logs bookkeeping
failures without reclassifying or retrying the already-submitted task; preserve
the submitted result.
---
Duplicate comments:
In `@apps/worker/src/index.ts`:
- Around line 283-291: Update both the schedule-fire consumer at
apps/worker/src/index.ts lines 273-281 and the trigger delivery consumer at
apps/worker/src/index.ts lines 283-291 to include the outcome’s error or reason
alongside outcome.kind in their structured logger calls. Preserve the existing
outcome handling and use the appropriate detail for failed, disabled, and
skipped outcomes.
In `@docs/design/04-execution-runtime.md`:
- Line 147: Update the schedule authority paragraph to reference the implemented
checkWorkAuthority helper instead of assertProjectRole, or describe the
fire-time membership validation without naming a helper. Keep the existing
explanation of owner-based authorization and attribution unchanged.
In `@packages/orchestration/src/schedule-fire.ts`:
- Around line 162-174: Update the catch handling in fireSchedule so only
AppError instances are converted to the recoverable fail(message) outcome;
rethrow plain Error and other unexpected thrown values so pg-boss can retry
infrastructure failures from submitTask or queue-name derivation.
- Around line 56-79: Update the disable function so marking the schedule
disabled and completing its audit, notification, and queue unregistration are
resilient to failures after the database state change. Ensure those follow-up
actions are retried or otherwise completed independently of the early
!schedule?.enabled path, while preserving the disabled outcome and reason.
---
Nitpick comments:
In `@apps/api/src/routes/trigger-inbound.ts`:
- Around line 87-96: Update payload normalization in the raw JSON parsing block
to distinguish plain object payloads from arrays before assigning to the
Record<string, unknown> payload. Remove the `as never` cast, preserve scalar
wrapping via `{ value: parsed }`, and explicitly normalize array values into an
object-compatible representation.
In `@apps/api/src/test/triggers.integration.test.ts`:
- Around line 103-107: Add an explicit expect(taskTypeId).toBeTruthy() assertion
in the beforeAll setup immediately after locating the "weekly-report" task type,
before later tests use taskTypeId, so missing seed data fails during setup
rather than through unrelated validation errors.
- Around line 251-255: Update the “caps the payload size” test to import and use
TRIGGER_PAYLOAD_MAX_BYTES, constructing a payload larger than that configured
limit instead of relying on the hardcoded 70_000 value while preserving the
expected 413 response.
In `@apps/web/src/pages/SettingsPage.tsx`:
- Around line 1077-1080: Update the trigger deliveries useQuery in SettingsPage
to poll for refreshed results by adding a 10-second refetch interval, matching
NotificationsSection’s existing delivery polling behavior while preserving the
current query key and query function.
- Line 1236: Type the default scopes and toggleScope state updates against the
API_KEY_SCOPES allow-list by using the ApiKeyScope type instead of bare string
identifiers. Update toggleScope to accept ApiKeyScope so renamed or removed
scope values are caught by the compiler while preserving the existing defaults.
In `@apps/web/src/pages/SubmitTaskPage.tsx`:
- Around line 277-282: Update the signing secret Input identified by id
"trigger-secret" in SubmitTaskPage to use password masking and disable browser
autocomplete, matching the notification secret field’s behavior in SettingsPage
while preserving its existing value and change handling.
In `@packages/core/src/schemas.ts`:
- Around line 358-369: Extend triggerUpdateSchema with an optional secret field
using the same validation and length bounds as triggerCreateSchema, then update
the PATCH handler to detect a provided secret, re-encrypt it, and persist the
resulting secretRef while preserving the existing secret when omitted.
In `@packages/db/src/schema/triggers.ts`:
- Line 72: Update the disabledReason column in the triggerDeliveries schema to
use the shared TriggerDisabledReason vocabulary from packages/core, matching the
enum-style typing used by status. First verify that packages/db may depend on
`@agrippa/core`; if that dependency is disallowed, leave disabledReason untyped
rather than introducing an invalid import.
In `@packages/orchestration/src/authority.ts`:
- Line 15: Update WorkAuthorityFailure in authority.ts to derive its union from
the shared TRIGGER_DISABLED_REASONS definition in `@agrippa/core`, or centralize
both declarations on one exported shared list. Ensure checkWorkAuthority and
trigger_endpoints.disabled_reason use the same source of truth so any future
reason additions are compiler-checked.
In `@packages/orchestration/src/trigger-fire.ts`:
- Around line 77-99: Update the denied branch in the trigger-fire flow to wrap
the triggerEndpoints update, markAttempt, and auditAs calls in a single
db.transaction, preserving their current operations and data. Keep
notifyProjectEvent outside and after the transaction commits, then return the
existing disabled result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a27615ad-09ca-4286-8ab6-43502212a672
📒 Files selected for processing (40)
CHANGELOG.mdapps/api/src/app.tsapps/api/src/lib/task-params.tsapps/api/src/routes/schedules.tsapps/api/src/routes/trigger-inbound.tsapps/api/src/routes/triggers.tsapps/api/src/test/helpers.tsapps/api/src/test/triggers.integration.test.tsapps/web/src/lib/types.tsapps/web/src/pages/SettingsPage.tsxapps/web/src/pages/SubmitTaskPage.tsxapps/worker/src/deps/notify.tsapps/worker/src/index.tsdocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/manual/en/04-administration.mddocs/manual/zh-CN/04-administration.mdpackages/core/src/index.tspackages/core/src/notifications.tspackages/core/src/queue.tspackages/core/src/schemas.tspackages/core/src/triggers.tspackages/db/drizzle/0025_trigger_endpoints.sqlpackages/db/drizzle/meta/0025_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/index.tspackages/db/src/schema/triggers.tspackages/i18n/locales/en/catalog.jsonpackages/i18n/locales/en/errors.jsonpackages/i18n/locales/en/notifications.jsonpackages/i18n/locales/en/settings.jsonpackages/i18n/locales/zh-CN/catalog.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/i18n/locales/zh-CN/notifications.jsonpackages/i18n/locales/zh-CN/settings.jsonpackages/orchestration/src/authority.tspackages/orchestration/src/index.tspackages/orchestration/src/queue.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/trigger-fire.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/worker/src/deps/notify.ts
- packages/orchestration/src/index.ts
- packages/i18n/locales/zh-CN/errors.json
- apps/api/src/app.ts
- apps/api/src/routes/schedules.ts
- apps/api/src/test/helpers.ts
- packages/core/src/index.ts
- packages/core/src/notifications.ts
Comment on lines
+132
to
+140
| it("is project-admin gated and rejects params that could never run", async () => { | ||
| expect( | ||
| ( | ||
| await member.request(`/api/v1/projects/${projectId}/triggers`, { | ||
| method: "POST", | ||
| json: { name: "nope", taskTypeId, params, secret: SECRET }, | ||
| }) | ||
| ).status, | ||
| ).toBe(403); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add member to the project so the admin gate is actually tested.
member never joins the project, so line 140 only proves that a non-member is rejected. requireProjectRole("member") would reject that user too. If the route is ever weakened from admin to member, this test still passes. Add the user as a project member with role member first.
💚 Proposed strengthening
it("is project-admin gated and rejects params that could never run", async () => {
+ await admin.request(`/api/v1/projects/${projectId}/members`, {
+ method: "POST",
+ json: { email: member.email, role: "member" },
+ });
expect(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("is project-admin gated and rejects params that could never run", async () => { | |
| expect( | |
| ( | |
| await member.request(`/api/v1/projects/${projectId}/triggers`, { | |
| method: "POST", | |
| json: { name: "nope", taskTypeId, params, secret: SECRET }, | |
| }) | |
| ).status, | |
| ).toBe(403); | |
| it("is project-admin gated and rejects params that could never run", async () => { | |
| await admin.request(`/api/v1/projects/${projectId}/members`, { | |
| method: "POST", | |
| json: { email: member.email, role: "member" }, | |
| }); | |
| expect( | |
| ( | |
| await member.request(`/api/v1/projects/${projectId}/triggers`, { | |
| method: "POST", | |
| json: { name: "nope", taskTypeId, params, secret: SECRET }, | |
| }) | |
| ).status, | |
| ).toBe(403); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/test/triggers.integration.test.ts` around lines 132 - 140, Add
member to the project with the member role before the request in the “is
project-admin gated and rejects params that could never run” test. Keep the
existing request and 403 assertion so the test verifies an authenticated project
member is denied by the admin gate, not merely rejected for lacking membership.
Comment on lines
+251
to
+254
| void navigator.clipboard | ||
| .writeText(url) | ||
| .then(() => toast.success(t("catalog:trigger.copied"))); | ||
| }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle clipboard failures for one-time secrets. Both copy handlers call navigator.clipboard.writeText with no failure path. navigator.clipboard is undefined outside a secure context, so the property access throws inside the click handler. If the browser denies permission, the promise rejects unhandled. Each copied value appears exactly once, so a silent failure loses it.
apps/web/src/pages/SubmitTaskPage.tsx#L251-L254: wrap thewriteTextcall so the property access cannot throw, then add acatchthat shows a localized failure toast for the trigger URL.apps/web/src/pages/SettingsPage.tsx#L1335-L1339: apply the same guard and failure toast for the plaintext API key.
Add the new failure message to packages/i18n/locales/en/*.json and packages/i18n/locales/zh-CN/*.json.
As per coding guidelines: "Every user-facing string must have both en and zh-CN translations; locale resources must maintain key parity."
🛡️ Proposed fix for the trigger URL site
onClick={() => {
- void navigator.clipboard
- .writeText(url)
- .then(() => toast.success(t("catalog:trigger.copied")));
+ void Promise.resolve()
+ .then(() => navigator.clipboard.writeText(url))
+ .then(() => toast.success(t("catalog:trigger.copied")))
+ .catch(() => toast.error(t("catalog:trigger.copyFailed")));
}}📍 Affects 2 files
apps/web/src/pages/SubmitTaskPage.tsx#L251-L254(this comment)apps/web/src/pages/SettingsPage.tsx#L1335-L1339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/pages/SubmitTaskPage.tsx` around lines 251 - 254, Update the
clipboard handlers in apps/web/src/pages/SubmitTaskPage.tsx lines 251-254 and
apps/web/src/pages/SettingsPage.tsx lines 1335-1339 to guard navigator.clipboard
before calling writeText, handle synchronous access failures, and catch rejected
promises with localized failure toasts for the trigger URL and plaintext API key
respectively. Add matching failure-message translation keys to the relevant en
and zh-CN JSON locale files while preserving locale key parity.
Source: Coding guidelines
| 计划**以创建者的身份执行**，且该权限在每次触发时重新校验，而非在创建时固化：它能做的事与创建者当下能做的完全一致，绝不更多。当某些条件永久性地不再成立时——创建者失去本项目权限、项目被归档、任务类型被下线——计划会被**停用并主动通知**，同时写入通知端点与审计日志，而不是悄悄跳过。这是有意为之：静默停止的计划与从未创建过的计划无法区分，等你发现时可能已过去数周。可修复的问题（配额耗尽、资源未授权、没有具备所需执行器的工作节点）则不同——计划保持启用，错误显示在其条目上并以 `schedule.failed` 发出，下次触发会再次尝试。重新启用已停用的计划会清除其停用原因；底层问题是否真的修好，留待下次触发时重新校验。 | ||
| - **Webhook 触发器** —— 一个供外部系统 POST 的 URL，用于启动任务：CI 完成、部署上线、表单提交等。与定时计划一样从任务创建（任务目录 → 填写表单 → **Webhook 触发**），在此管理。 | ||
|
|
||
| 接入前有两点需要了解。该 URL 是一种**能力凭证**——任何持有它的人都能触达你的触发器——因此**必须设置签名密钥**，每个请求都要带上用它计算的 `x-agrippa-timestamp` 与 `x-agrippa-signature: v1=<对 "时间戳.正文" 的 HMAC-SHA256>`。超过五分钟的请求会被拒绝，因此被截获的请求会自动失效。创建时对话框会给出可直接粘贴的 `curl` 示例；URL 只显示这一次。另外，平台在**记录请求的那一刻就返回 202**，此时执行尚未产生——发送方只会知道请求被接受，永远不会知道执行是否成功。这正是触发器失效时会主动通知你（`trigger.disabled`、`trigger.failed`）而不是等发送方反馈的原因：发送方早已被告知一切正常。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the intercepted-request sentence.
被截获的请求会自动失效 is awkward in this sentence. Use 即使请求被截获，也会自动失效 so the condition is explicit.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: 能愿动词不能成为‘把’字句、‘被’字句的谓语动词。应该是:"会被……截获"。
Context: ..."时间戳.正文" 的 HMAC-SHA256>。超过五分钟的请求会被拒绝，因此被截获的请求会自动失效。创建时对话框会给出可直接粘贴的 curl` 示例；URL 只显示这一...
(wa3)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/manual/zh-CN/04-administration.md` at line 55, Update the
intercepted-request sentence in the administration documentation to explicitly
state that even if a request is intercepted, it will automatically expire, using
the wording “即使请求被截获，也会自动失效” while leaving the surrounding guidance unchanged.
Source: Linters/SAST tools
…e key binding lives Two things found auditing my own branch before review rather than after. The inbound trigger checked payload size *after* `c.req.text()`, so an oversized send was already resident in memory by the time we objected to it. The daemon event batch does this correctly — bounding by Content-Length before parsing, with a comment saying why — and design/05 records that as the standard, so this was a gap against a rule the codebase already had. Both checks now run: the header check keeps the common case from being paid for at all, and the post-read check stays as the backstop, because Content-Length is absent under chunked encoding and only the second one cannot be evaded. Separately, `assertProjectRole` is where an API key's project binding is enforced, and every id-based route in execution.ts does reach project data through it — I checked all twelve. But `GET /checkpoints/pending` scopes by joining `project_members` directly, so a bound key would see rows from every project its owner belongs to. That route is unreachable by keys today, which means the endpoint's safety is a property of the allow-list rather than of the handler. Since the mistake would be made by adding a line to the allow-list, the warning belongs there, which is where it now is.
…e, disable atomicity
All seven findings verified against source before changing anything; none was
refuted, and four turned out worse than reported. Six of the seven are places
where the code contradicted a rule this repo had already written down.
**Post-commit enqueue is best-effort (P1).** submitTask commits the task and
run, then enqueues — and the comment two lines above that send says the
worker's straggler sweep makes it loss-proof. But fireSchedule and fireTrigger
treated any throw as a submission failure, so a transient queue error produced
a run the sweeper executed AND a second one on retry, while the delivery row
said failed with no run id and a schedule's lastRunId never advanced, blinding
its own skip/replace policy. The duplicate needed no operator: the catch path
itself enqueues a notification, fails identically, escapes the handler, and
pg-boss retries the whole firing. Fixed at all five post-commit sites, each of
which already had a sweeper behind it.
Found while verifying, not in the report: the 60s sweep was one try block, so a
single straggler with an underivable executor set aborted lease expiry,
offline-runtime detection, checkpoint recovery and both delivery sweeps — every
tick, forever, behind one log line. A recovery mechanism one bad row can
disable is not a recovery mechanism. Each stage is now isolated.
**trigger.fire dedupe never worked (P1).** It was created with pg-boss's
default `standard` policy while its sends passed singletonKey. The comment ten
lines above says "'standard' has no singleton index at all"; an existing test
asserts it; design/04 states it — and my own new sentence in the same file
claimed the opposite. Now `exclusive`, with fireTrigger claiming a delivery by
CAS on its attempts counter (the pattern deliverNotification already used)
rather than reading its status and hoping.
**Accepted deliveries could strand forever (P1).** No sweeper touched
trigger_deliveries — the partial index one would need was already there,
unused. Worse, a stranded pending row was unreachable through the product:
Retry only moves failed → pending, and the UI offers it only on failed. Added
sweepTriggerDeliveries; a duplicate delivery-id whose original never got a job
now re-enqueues instead of silently answering 200.
**Disable could stop silently (P2).** The enabled flag was flipped first and
the audit, notification and cron cleanup came after — and that flag is exactly
what makes the retry skip. A throw in between lost all three permanently, the
run-less notification being unreconstructible by any sweeper. So the feature
whose whole premise is "a schedule that stops always says so" could stop
without a word. Flag, audit row and delivery rows now commit together; boot
reconciliation also unregisters disabled schedules, which it never did.
**CRUD 500'd after committing (P2).** scheduleRoutes' comment claimed
registration was "best-effort at request time on purpose" while the code
awaited it unguarded — so a queue hiccup returned 500 for a schedule that
existed, was enabled, and would start firing at the next worker boot, with the
audit log recording success. The comment is now true.
**The body cap was not a cap (P2).** My own previous commit claimed to fix
this. It did not: Number("abc") is NaN and NaN > limit is false, so a junk
Content-Length walked straight past it, and chunked encoding omits the header
entirely — leaving the runtime's 128 MiB as the real ceiling, ~2048x the
documented 64 KiB, reachable with the URL token alone since the signature
necessarily comes after the read. The body is now bounded while being read, and
rejects rather than truncates: truncation would change the bytes the signature
covers and surface as a wrong-secret error.
**A deleted trigger kept its secret (P2).** No reverse cascade, and the row
that pointed at the ciphertext was the only thing that could find it — so every
deleted trigger left live key material the operator believed was destroyed, and
unreachable by key rotation. Now deleted in the same transaction, matching what
notification endpoints and provider credentials have always done.
Ten regression tests, one per finding that can carry one, including a
concurrent double-fire (the existing "never twice" test fired sequentially and
could not have caught the race) and a chunked body with no Content-Length
(fetch always sets one, so the old test only exercised the header path).
Docs close Track T's four gaps and the drift they inherited: 01-domain-model
gains the three tables and a schema-file list that was missing six of sixteen;
06-frontend gains the four Settings sections it lacked — including Track N's
notifications — the two Submit dialogs, and Phase B's Workers page;
ARCHITECTURE's orchestration codemap was eight modules stale and its apps/api
row still claimed submission logic that moved out of that package on this
branch, which would send a contributor to the wrong package; m2-plan ticks
Track T. design/04's self-contradiction about singleton keys is corrected, and
a new invariant records that unattended work names a human whose authority is
re-checked at the point of use.
Verified: check, build, templates, and 572 pass / 5 skip / 0 fail with
Postgres connections reclaimed first. Browser smoke re-run — trigger created,
signed POST accepted, worker produced a run with its date tokens resolved.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/orchestration/src/submit.ts (1)
70-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSerialize the archive check with task creation.
assertProjectAcceptsWorkcompletes before the transaction starts at Line 107. If an admin archives the project after Line 70, this function can still commit a new task and run. Re-check and lock the project row inside the insert transaction, or use a conditional insert tied to active project status. Add an integration test that races archival with submission.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/submit.ts` at line 70, Move the project acceptance validation into the task-creation transaction in the submit flow, using a row lock or conditional insert that prevents creating a task once the project is archived; do not rely on the earlier assertProjectAcceptsWork call alone. Add an integration test that races project archival against submission and verifies no task is committed after archival.apps/api/src/routes/trigger-inbound.ts (1)
143-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAn empty delivery-ID header deduplicates every later event.
c.req.header()returns""when the header is present but empty.??does not replace"", soexternalIdbecomes"".""is not NULL, so the partial indextrigger_deliveries_dedupe_uqapplies to it. The first such delivery inserts, and every later delivery from the same endpoint with an empty header conflicts, returns 200deduplicated, and never runs. Treat blank as absent.🐛 Proposed fix
- const externalId = c.req.header(TRIGGER_DELIVERY_ID_HEADER)?.slice(0, 200) ?? null; + const externalId = c.req.header(TRIGGER_DELIVERY_ID_HEADER)?.trim().slice(0, 200) || null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/trigger-inbound.ts` at line 143, Update the externalId extraction in the inbound trigger route to treat an empty or whitespace-only TRIGGER_DELIVERY_ID_HEADER value as absent, producing null before the value is persisted or used for deduplication. Preserve the existing 200-character limit for non-blank header values.
🧹 Nitpick comments (2)
apps/api/src/test/schedules.integration.test.ts (1)
425-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the shared fixtures that this test mutates.
The test deletes the owner's
projectMembersrow and inserts anotificationEndpointsrow namedblocker. Neither is restored. The endpoint usesevents: [], which the schema treats as every notifiable event, so it receives deliveries for every later test in this process. Any test added after this one that countsnotificationDeliveriesrows gets extra rows fromblocker. Delete the endpoint in afinallyblock or anafterEachhook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/schedules.integration.test.ts` around lines 425 - 440, The test setup mutates shared fixtures without restoring them: update the cleanup around the test’s projectMembers deletion and blocker notificationEndpoints insertion to run in a finally block or afterEach hook, deleting the inserted blocker endpoint and restoring the owner membership so later tests see the original state.apps/api/src/routes/trigger-inbound.ts (1)
144-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite an audit row for accepted inbound trigger deliveries.
This route inserts
triggerDeliveriesand calls queue enqueue, but never writes the required API audit entry. Record the accepted delivery withauditAs, using a public/endpoint actor; if the public delivery row is intended as the audit trail for this unauthenticated ingress surface, document that explicit exception at this route.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/trigger-inbound.ts` around lines 144 - 155, After the successful triggerDeliveries insert in the inbound delivery route, record the accepted delivery through auditAs using a public/endpoint actor, preserving the existing duplicate-conflict behavior and queue flow. If the public delivery record is intentionally serving as the audit trail for this unauthenticated ingress path, document that exception directly in the route.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/test/schedules.integration.test.ts`:
- Around line 465-469: Update the deliveries query in the schedule-disabled
retry test to filter by both eventType and the payload condition for this
schedule’s scheduleId, reusing the existing scheduleId payload expression. Keep
the assertion unchanged so it only passes when this retry’s notification
delivery was committed.
In `@apps/api/src/test/triggers.integration.test.ts`:
- Around line 519-529: Update the junk Content-Length case in the trigger
integration test so it reliably reaches the handler with a non-numeric declared
length, avoiding the forbidden-header stripping behavior. Ensure the test
directly exercises and asserts rejection of the bogus value through the
handler’s Number.isFinite(declared) guard rather than relying on the body-stream
limit response.
In `@apps/worker/src/index.ts`:
- Around line 309-317: Update the schedule reconciliation loop in the startup
handler around registerSchedule and unregisterSchedule so each row is processed
inside its own try/catch, logging the row-specific failure and continuing with
subsequent rows. Track successfully reconciled rows separately and use that
count in the final deps.logger.info message instead of rows.length.
In `@packages/orchestration/src/trigger-fire.ts`:
- Around line 78-90: Update the trigger delivery retry route to set
lastAttemptAt to null alongside status: "pending" and clearing lastError,
matching the existing notification retry path. Ensure fireTrigger can
immediately claim the retried delivery without being blocked by the 20-second
timestamp guard.
- Around line 197-223: Update both the exhaustion update and stale-delivery
selection in the trigger sweep to require an attempt-staleness condition based
on lastAttemptAt, rather than relying only on pending status and, for the batch
query, createdAt. Ensure in-flight attempts are excluded from failure marking
and re-enqueueing while preserving the existing retry limits and stale batch
ordering.
---
Outside diff comments:
In `@apps/api/src/routes/trigger-inbound.ts`:
- Line 143: Update the externalId extraction in the inbound trigger route to
treat an empty or whitespace-only TRIGGER_DELIVERY_ID_HEADER value as absent,
producing null before the value is persisted or used for deduplication. Preserve
the existing 200-character limit for non-blank header values.
In `@packages/orchestration/src/submit.ts`:
- Line 70: Move the project acceptance validation into the task-creation
transaction in the submit flow, using a row lock or conditional insert that
prevents creating a task once the project is archived; do not rely on the
earlier assertProjectAcceptsWork call alone. Add an integration test that races
project archival against submission and verifies no task is committed after
archival.
---
Nitpick comments:
In `@apps/api/src/routes/trigger-inbound.ts`:
- Around line 144-155: After the successful triggerDeliveries insert in the
inbound delivery route, record the accepted delivery through auditAs using a
public/endpoint actor, preserving the existing duplicate-conflict behavior and
queue flow. If the public delivery record is intentionally serving as the audit
trail for this unauthenticated ingress path, document that exception directly in
the route.
In `@apps/api/src/test/schedules.integration.test.ts`:
- Around line 425-440: The test setup mutates shared fixtures without restoring
them: update the cleanup around the test’s projectMembers deletion and blocker
notificationEndpoints insertion to run in a finally block or afterEach hook,
deleting the inserted blocker endpoint and restoring the owner membership so
later tests see the original state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0176687a-54db-4c3a-a85b-929e519411d6
📒 Files selected for processing (20)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/lib/api-key-routes.tsapps/api/src/routes/execution.tsapps/api/src/routes/schedules.tsapps/api/src/routes/trigger-inbound.tsapps/api/src/routes/triggers.tsapps/api/src/test/schedules.integration.test.tsapps/api/src/test/triggers.integration.test.tsapps/worker/src/index.tsdocs/design/01-domain-model.mddocs/design/04-execution-runtime.mddocs/design/06-frontend.mddocs/plan/m2-plan.mdpackages/orchestration/src/notifications.tspackages/orchestration/src/queue.integration.test.tspackages/orchestration/src/queue.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/submit.tspackages/orchestration/src/trigger-fire.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/orchestration/src/schedule-fire.ts
- docs/design/04-execution-runtime.md
- apps/api/src/routes/schedules.ts
- apps/api/src/routes/execution.ts
- apps/api/src/routes/triggers.ts
- apps/api/src/lib/api-key-routes.ts
Comment on lines
+465
to
+469
| const deliveries = await db | ||
| .select() | ||
| .from(notificationDeliveries) | ||
| .where(eq(notificationDeliveries.eventType, "schedule.disabled")); | ||
| expect(deliveries.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== imports in the test file ==="
fd -t f 'schedules.integration.test.ts' apps --exec sed -n '1,40p'
echo "=== insertProjectEventDeliveries definition ==="
ast-grep run --pattern 'export async function insertProjectEventDeliveries($$$) { $$$ }' --lang typescript packages/orchestration/srcRepository: ainaive/agrippa
Length of output: 1097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate schedules integration file and insertProjectEventDeliveries ==="
fd -t f 'schedules.integration.test.ts|.*schedule.*|deliveri' . | sed -n '1,120p'
echo
echo "=== file path ==="
fd -t f 'schedules.integration.test.ts' . -x echo "{}"
echo
echo "=== search insertProjectEventDeliveries ==="
rg -n "insertProjectEventDeliveries|scheduleId|notifications?Deliveries|eventType|schedule.disabled" . -g '*.ts' | sed -n '1,220p'Repository: ainaive/agrippa
Length of output: 20796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== test file imports and focused tests ==="
cat -n apps/api/src/test/schedules.integration.test.ts | sed -n '1,525p'
echo
echo "=== notification delivery schema ==="
cat -n packages/db/src/schema/notifications.ts | sed -n '50,85p'
echo
echo "=== schedule disabled code path ==="
cat -n packages/orchestration/src/schedule-fire.ts | sed -n '70,92p'Repository: ainaive/agrippa
Length of output: 24625
Scope the delivery assertion to this schedule.
The suite creates a schedule.disabled delivery earlier, so the current eventType filter can pass even if the retry does not commit this retry’s notification. Add the payload condition used for scheduleId.
💚 Proposed fix
const deliveries = await db
.select()
.from(notificationDeliveries)
- .where(eq(notificationDeliveries.eventType, "schedule.disabled"));
+ .where(
+ and(
+ eq(notificationDeliveries.eventType, "schedule.disabled"),
+ sql`${notificationDeliveries.payload} ->> 'scheduleId' = ${row.id}`,
+ ),
+ );
expect(deliveries.length).toBeGreaterThan(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const deliveries = await db | |
| .select() | |
| .from(notificationDeliveries) | |
| .where(eq(notificationDeliveries.eventType, "schedule.disabled")); | |
| expect(deliveries.length).toBeGreaterThan(0); | |
| const deliveries = await db | |
| .select() | |
| .from(notificationDeliveries) | |
| .where( | |
| and( | |
| eq(notificationDeliveries.eventType, "schedule.disabled"), | |
| sql`${notificationDeliveries.payload} ->> 'scheduleId' = ${row.id}`, | |
| ), | |
| ); | |
| expect(deliveries.length).toBeGreaterThan(0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/test/schedules.integration.test.ts` around lines 465 - 469,
Update the deliveries query in the schedule-disabled retry test to filter by
both eventType and the payload condition for this schedule’s scheduleId, reusing
the existing scheduleId payload expression. Keep the assertion unchanged so it
only passes when this retry’s notification delivery was committed.
Comment on lines
+519
to
+529
| const junk = await app.request(`/api/triggers/${t.token}`, { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| "content-length": "not-a-number", | ||
| [TRIGGER_TIMESTAMP_HEADER]: ts, | ||
| [TRIGGER_SIGNATURE_HEADER]: sig, | ||
| }, | ||
| body, | ||
| }); | ||
| expect(junk.status).toBe(413); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is Content-Length a forbidden request header name in the Fetch standard, and does the Request constructor drop it?
💡 Result:
Yes, Content-Length is classified as a forbidden request header name in the Fetch standard [1][2]. Headers are marked as forbidden to ensure that the user agent (e.g., the browser) maintains full control over them, preventing developers from manually overriding headers that are critical to the underlying HTTP protocol's operation [2][3]. Regarding the Request constructor, the Fetch specification generally does not require it to drop the Content-Length header simply because it is forbidden; rather, the restriction applies to the programmatic modification of these headers [3]. However, in practice, because Content-Length is a forbidden request header, you cannot set it via the Headers object when constructing a Request [3]. If you attempt to include it in a Headers object passed to the Request constructor, it will be ignored or rejected depending on the implementation, as the user agent is responsible for calculating and setting the correct Content-Length based on the request body [4]. Consequently, while the header is forbidden to be manually set by developers, the user agent will automatically calculate and provide the appropriate Content-Length header for the request [4]. If an implementation drops it or sets it to zero unexpectedly, it is often due to the specific runtime environment's handling of the request body stream or its internal limitations rather than a direct mandate to drop the header during construction [5][6].
Citations:
- 1: https://fetch.spec.whatwg.org/
- 2: https://github.com/whatwg/fetch/blob/09e95249f3669a335ca4551c7702888c46f7cce4/fetch.bs
- 3: https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_header
- 4: https://developers.cloudflare.com/workers/runtime-apis/request/index.md
- 5:
Requestobject behaves differently in fetch vercel/next.js#66840 - 6: Passing a Request into fetch results in
Content-Lengthalways set to 0 even when explicitly set denoland/deno#18956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files:"
fd -a 'triggers.integration.test.ts|audit.ts' . | sed 's#^\./##'
echo
echo "Candidate test snippet:"
if [ -f apps/api/src/test/triggers.integration.test.ts ]; then
sed -n '450,550p' apps/api/src/test/triggers.integration.test.ts | cat -n
fi
echo
echo "Search triggers route/header handlers:"
rg -n "TRIGGER_TIMESTAMP_HEADER|TRIGGER_SIGNATURE_HEADER|content-length|Content-Length|number of bytes|body size|Request|app.request|handler" apps/api/src -g '*.ts' | head -200Repository: ainaive/agrippa
Length of output: 13954
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "app.request helpers and type definitions:"
sed -n '1,150p' apps/api/src/test/helpers.ts | cat -n
rg -n "function request|const request|request:|app\.request|RequestInit|WorkerRuntime|Hono|app\.handle|c\.req\.header|header\\(" apps/api/src -g '*.ts' | head -200
echo
echo "trigger-inbound route:"
sed -n '80,145p' apps/api/src/routes/trigger-inbound.ts | cat -n
echo
echo "package/runtime hints:"
for f in package.json apps/api/package.json bunfig.toml wrangler.toml; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
doneRepository: ainaive/agrippa
Length of output: 19991
🌐 Web query:
Bun app.request Request headers forbidden headers Content-Length Request constructor
💡 Result:
In Bun, request headers are governed by standard Fetch API behaviors, with specific implementation nuances in the Bun runtime [1][2]. Regarding the Content-Length header and forbidden headers: 1. Forbidden Headers: Consistent with the Fetch API specification, certain headers are considered "forbidden" or guarded to prevent them from being tampered with by client-side code [3]. These headers are automatically managed by the user agent (Bun) and cannot be modified or removed by manual assignment in a Request object or fetch call [3]. This ensures the integrity of the HTTP protocol [3]. 2. Content-Length Behavior: - In standard fetch, the Content-Length header is typically managed automatically by the runtime [1]. If you provide a stream as a request body, Bun will often send the request with Transfer-Encoding: chunked because the total length may not be known upfront [4][5]. - There have been reported issues where using the node:http compatibility layer specifically ignores explicitly set Content-Length headers in favor of chunked encoding [4][5]. While fixes have been tracked in the Bun repository (e.g., issues #26976 and #24612), behavior in the node:http module may vary from standard fetch [4][5]. 3. Other Default Headers: Bun automatically adds certain headers (like User-Agent, Accept, and Accept-Encoding) to requests [6]. Users have reported that these cannot be fully removed even when explicitly cleared in the headers configuration, as the runtime re-applies them [6]. 4. Using app.request: When using app.request (often used in frameworks like Hono for testing), you are essentially invoking the application's internal request handling [7][8]. If you encounter issues (e.g., middleware failing), it is often because the headers object provided in the app.request options is being misinterpreted or colliding with middleware expectations, rather than a "forbidden header" restriction in the runtime itself [7][8]. Always ensure that the headers object provided to app.request is properly structured, preferably using a Headers object instance if standard object literals behave unexpectedly [8]. For the most reliable control over headers, use the standard Request and Headers APIs [2][3], and be aware that the runtime maintains ultimate control over mandatory protocol headers [3][6].
Citations:
- 1: https://bun.com/docs/runtime/networking/fetch
- 2: https://bun.com/reference/globals/Request
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Headers
- 4:
node:httprequest.write()strips explicitContent-Lengthheader oven-sh/bun#26976 - 5: node:http requests ignoring explicitly set content-length header oven-sh/bun#24612
- 6: Unable to Remove Default Headers (User-Agent, Accept, Accept-Encoding) in Bun oven-sh/bun#17012
- 7: Solution : Hono CSRF middleware blocking valid formData requests. honojs/hono#4052
- 8: Add Authorization header when testing using request honojs/hono#3431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Hono test mode and app.request implementation:"
sed -n '45,75p' apps/api/src/app.ts | cat -n
echo
echo "Inspect `@hono/bun` and hono runtime/test helper files in lockfile/node_modules/cache if present:"
fd -a 'test-request|adapter|request|index.ts|hono' apps/api/node_modules 2>/dev/null | head -20 || true
rg -n "app\.request|new Request|headers:|content-length|contentLength|contentLength" apps/api/node_modules/@hono apps/api/node_modules/hono 2>/dev/null | head -200 || true
echo
echo "Read package-lock entries for hono:"
python3 - <<'PY'
import json, pathlib
for p in [pathlib.Path("bun.lock"), pathlib.Path("bun.lockb")]
PYRepository: ainaive/agrippa
Length of output: 2166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Exact trigger-inbound size guard:"
nl -ba apps/api/src/routes/trigger-inbound.ts | sed -n '15,35p'
echo
echo "Hono app request construction in installed runtime:"
python3 - <<'PY'
import json, pathlib
for name in ["bun.lock", "bun.lockb", "package-lock.json", "pnpm-lock.yaml"]:
p = pathlib.Path(name)
if p.exists():
print(f"lockfile: {name}")
PY
if [ -d apps/api/node_modules/hono ]; then
rg -n "app\.request|new Request|setHeaders|content-length|contentLength|forbidden|forb" apps/api/node_modules/hono apps/api/node_modules/@hono 2>/dev/null | head -200
else
echo "no apps/api/node_modules/hono"
fi
echo
echo "Relevant installed Hono files:"
git ls-files 'apps/api/node_modules/hono/**/*.ts' 'apps/api/node_modules/hono/**/*.js' | rg 'test|request|adapter' | head -100 || true
echo
echo "If bun is available, behavioral probe for Headers/Request Content-Length header:"
tmp="$(mktemp -d)"
cat > "$tmp/probe.js" <<'JS'
const h1 = new Headers({ "content-length": "not-a-number"]);
try {
const req = new Request("http://x/", { method: "POST", body: "x" });
console.log("Request empty headers contain content-length:", req.headers.get("content-length") !== null);
console.log("Request empty headers content-length:", req.headers.get("content-length"));
} catch (e) {
console.log("Request empty error:", e && e.message);
}
try {
const req2 = new Request("http://x/", { method: "POST", body: "x", headers: h1 });
console.log("Manual Content-Length header value:", req2.headers.get("content-length"));
} catch (e) {
console.log("Manual Content-Length error:", e && e.message);
}
JS
if command -v bun >/dev/null 2>&1; then
bun "$tmp/probe.js" || true
else
echo "bun not available"
fi
node "$tmp/probe.js" || trueRepository: ainaive/agrippa
Length of output: 227
Use a bypass-proof way to send junk Content-Length.
Content-Length is a forbidden request header, so this request may strip "not-a-number" before the route sees it. In that case, the body-stream counter returns 413, and the test does not catch a regression in Number.isFinite(declared). Make this case carry a junk value the handler can read, or assert that the guard rejects bogus declared lengths directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/test/triggers.integration.test.ts` around lines 519 - 529,
Update the junk Content-Length case in the trigger integration test so it
reliably reaches the handler with a non-numeric declared length, avoiding the
forbidden-header stripping behavior. Ensure the test directly exercises and
asserts rejection of the bogus value through the handler’s
Number.isFinite(declared) guard rather than relying on the body-stream limit
response.
… sweeping Two Major findings against the round-1 commit itself, both verified. Boot reconciliation awaited register/unregister with no per-row guard, inside one try. Reconciliation runs only at startup, so one failing schedule left an arbitrary remainder unreconciled for the life of the process with nothing to retry it — and row order is unspecified, so which ones was a lottery. This is precisely the failure mode fixed for the sweeper two functions away in the same commit, missed in the adjacent loop. Now per row, and the count logs what actually converged rather than what was considered. `sweepTriggerDeliveries` measured row age instead of attempt age. A delivery created long ago and re-attempted a moment ago read as stale, so it was re-enqueued every tick, and — the real defect — the exhaustion update could stamp `failed` on a row whose submit was still running, which the success path then overwrote. Both statements now key off `coalesce(last_attempt_at, created_at)`, the coalesce covering a never-claimed row whose creation genuinely is its clock. One correction to the report, recorded because it changes the severity rather than the fix: it argued this permits a second submission and a second charge once a submit exceeds the 20s CAS window. It does not. `trigger.fire` became an `exclusive` queue in the same commit, and that policy's unique index spans created/retry/active — so while the original job is still active a sweeper re-enqueue cannot mint a second job at all. What remained was the spurious `failed` stamp and pointless re-enqueue work, which is what this fixes. 573 pass / 5 skip / 0 fail, with a test pinning the distinction: an old row with a current attempt is neither re-enqueued nor failed.
…y, audited mutations All seven verified against source; none refuted. Six fully valid, one (HMAC) valid in mechanism but not a regression and not a forgery risk. Three were consequences of round 1, each because the fix went to the site the review named rather than to the class of sites sharing the defect. **Submission is atomic with its bookkeeping (P1).** submitTask committed the run, then the caller recorded that it had happened. A crash in that gap left the delivery pending or lastRunId stale, and the retry — whose claim window is deliberately SHORTER than the retry delay, so legitimate retries get through — submitted a second charged run. The exclusive queue policy is no help: the duplicate is the same job re-executing, not a second job. submitTaskIn now takes the caller's transaction; the enqueue stays outside as best-effort. The read chain widened to DbOrTx, which the codebase's other tx-aware helpers already took. Schedules were worse: their queue retries immediately with no claim at all, and the lost lastRunId is exactly what the skip/replace check reads, so it stopped skipping and "replaced" a run that had already finished. Two more the report did not name. The try spanned the bookkeeping as well as the submission, so a failure after the run committed marked the delivery failed and fired trigger.failed for a run that was alive and billing; the try now covers only the submission. And Retry gated on status alone, so clicking it on such a delivery minted yet another run — it now refuses any delivery with a run_id. **The rest.** Both inbound enqueues were still bare, so a queue outage 500'd an accepted delivery and the sender's retry — lacking the *optional* delivery-id header — inserted a second one. The trigger.failed path wrote its terminal status separately from its announcement, the same shape fixed for the disable path one branch above, and `failed` is what makes the retry's claim reject, so the notification was lost for good. Retry left attempts and lastAttemptAt untouched, so an exhausted delivery was re-failed the moment it aged and a click within 20s silently no-opped — both hazards the notification retry's own comment names. The HMAC hashed decoded text, so a body with invalid UTF-8 could never verify and was reported as a wrong secret; it now hashes wire bytes and decodes after. **Schedule registration got the recovery half it was missing.** Round 1 made it best-effort so a hiccup would stop 500-ing a schedule that existed — but reconciliation ran only at boot, which traded a loud wrong answer for a silent one: the schedule never fired until a restart, and an edited one kept its old cron while the UI showed the new. A schedule-calendar sweeper stage now diffs the rows against boss.getSchedules each tick, writing nothing when converged. It earned its keep immediately: the boot pass removed an orphaned entry left by a schedule deleted in an earlier smoke, which nothing could previously reap. enqueueAfterCommit's docstring is corrected too — it asserted that every post-commit send has a sweeper behind it, and this was the call site where that was false while its warning told operators otherwise. **Audits ride their mutation's transaction at the nine new sites** (ADR-0014 decision 3). Scoped deliberately: all 50 route call sites in the codebase are non-transactional, so this is a long-standing pattern rather than a Track T regression, and the other 41 are recorded as their own change rather than enlarging a PR under review. The API-key case was worth not deferring — a failed audit after the key row committed left a live credential whose only plaintext died with the request and whose creation the audit log never knew. 581 pass / 5 skip / 0 fail (was 573), with eight regressions including a rollback proof for the atomic submission, an invalid-UTF-8 signature, a retried-then-swept delivery, and calendar drift repaired without a restart. Browser smoke re-run: trigger created, signed POST accepted, worker produced the run.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/orchestration/src/schedule-fire.ts (1)
193-205: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRethrow errors that are not attributable submission failures.
The catch converts every thrown value into
fail(message)and then returns normally. The comment on lines 194-197 claims that only fixable configuration or a transient shortage reaches this point. That does not hold for the whole call.submitTaskInthrows a plainErrorfor"task insert failed"and"run insert failed", and the surroundingdb.transactionpropagates driver faults such as a lost connection, a deadlock, or a serialization failure.For those faults the schedule records a firing failure, announces it, and returns
{ kind: "failed" }. The worker consumer sees a normal return, so pg-boss does not retry the job. The firing is lost until the next cron occurrence. For a weekly schedule that is a week of silence, and the recorded error names an infrastructure fault as a configuration problem.Classify the error: convert
AppErrorandSubmitErrorthroughfail, and rethrow everything else so pg-boss retries the firing.🛡️ Proposed fix
} catch (err) { - // Everything reaching here is fixable configuration or a transient - // shortage — quota exhausted, a revoked grant, no live worker for the - // executor set. The schedule stays on so next week's firing can succeed; - // the error is recorded and announced so the fix happens before then. - const message = - err instanceof AppError - ? `${err.code}: ${err.message}` - : err instanceof Error - ? err.message - : String(err); - return fail(message); + // Only an attributable submission failure — fixable configuration or a + // transient shortage — is recorded and announced. The schedule stays on so + // the next firing can succeed. Anything else is an infrastructure fault: + // rethrow it so pg-boss retries this firing instead of skipping it. + if (err instanceof AppError) return fail(`${err.code}: ${err.message}`); + if (err instanceof SubmitError) return fail(`${err.code}: ${err.message}`); + throw err; }Import
SubmitErrorfrom the module that declares it, and confirm its shape carriescode.As per coding guidelines: "Preserve engine error semantics:
RunFailureandBudgetExceededErrorfinalize the run, while unexpected errors must rethrow so pg-boss retries and the engine resumes at step granularity".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/schedule-fire.ts` around lines 193 - 205, Update the catch block in the schedule-fire flow to pass only AppError and SubmitError instances to fail(message), preserving their code-aware message formatting and importing SubmitError from its declaring module. Rethrow all other errors, including task/run insert failures and database driver faults, so pg-boss retry semantics remain intact.Source: Coding guidelines
🧹 Nitpick comments (3)
packages/orchestration/src/schedule-fire.ts (1)
252-270: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReport the per-row repair failure.
The catch on lines 267-269 discards the error.
reconcileScheduleCalendarreturns only aggregate counts, so a schedule whoseregistercall fails on every tick produces no signal. The worker then logs+0/-0, which reads as a converged calendar while that schedule never fires.Accept an optional reporter and call it in the catch, or return the failed ids so the worker can log them.
♻️ Proposed change
export async function reconcileScheduleCalendar( db: Db, calendar: { list(): Promise<Array<{ key: string; cron: string; timezone: string }>>; register(scheduleId: string, cron: string, timezone: string): Promise<void>; unregister(scheduleId: string): Promise<void>; }, -): Promise<{ registered: number; unregistered: number }> { + onError?: (scheduleId: string, err: unknown) => void, +): Promise<{ registered: number; unregistered: number }> { @@ - } catch { + } catch (err) { + onError?.(row.id, err); live.delete(row.id); // not an orphan; it has a row, just an unhappy one }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/schedule-fire.ts` around lines 252 - 270, Update reconcileScheduleCalendar’s per-row catch to report the failed schedule instead of silently discarding the error: accept an optional reporter and invoke it with the row identifier and caught error, or return failed IDs for the worker to log. Preserve the existing per-row isolation and live.delete behavior while ensuring repeated register/unregister failures produce an observable signal.apps/api/src/test/triggers.integration.test.ts (1)
649-654: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op task-type disable and fix the comment.
Line 650 disables the task type, and line 654 re-enables it. Only
sendruns between them.sendrecords and enqueues the delivery; it does not submit.fireTriggerruns on line 665, after the task type is enabled again, so the disable never affects the outcome. The catch path is reached by the invalidparamswritten on lines 661-664. The comment on line 649 states a mechanism the test does not use, so a later edit could remove theparamsmutation and still expect a failure.♻️ Proposed change
const t = await createTrigger({ params: { dateRange: "x", rawNotes: "y" } }); - // make submission fail so we reach the catch path - await db.update(taskTypes).set({ enabled: false }).where(eq(taskTypes.id, taskTypeId)); const { deliveryId } = await jsonOf<{ deliveryId: string }>( await send(t.token as string, { event: "x" }), ); - await db.update(taskTypes).set({ enabled: true }).where(eq(taskTypes.id, taskTypeId));The params rewrite on lines 661-664 already forces submission to fail, so keep that as the single mechanism.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/triggers.integration.test.ts` around lines 649 - 654, Remove the taskTypes updates that disable and re-enable the task type around send, and update the preceding comment to state that the invalid params mutation is what forces submission to fail and reaches the catch path. Keep the existing params rewrite as the sole failure mechanism.packages/orchestration/src/submit.ts (1)
102-103: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider serializing the quota check against concurrent submissions.
assertQuotaHeadroomnow runs inside the caller transaction, and the inserts follow it. Two concurrent submissions in separate transactions both read the pre-commit run count, so both can pass the check and both can commit. A project can exceed its hard-stop quota by the number of concurrent submissions. Scheduled and trigger firings make concurrent submission more likely than the browser path did.This shape predates the refactor, so it is not a regression. If the quota is a hard stop, enforce it with a row lock on the project quota row inside the transaction, or with a database constraint that the insert violates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/submit.ts` around lines 102 - 103, Serialize the quota validation in the submit transaction by locking the project’s quota row before assertQuotaHeadroom evaluates usage, ensuring concurrent submissions for the same project cannot pass against the same pre-commit count. Update the flow around assertQuotaHeadroom in submit.ts while preserving the existing hard-stop rejection and subsequent inserts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/05-api-and-auth.md`:
- Line 21: Qualify the idempotency statement in the API/auth design text:
clarify that duplicate prevention via the unique (endpoint, external_id) key
applies only when callers provide a stable x-agrippa-delivery-id. State that
retries without this optional header may create another delivery and run,
without changing the header’s optional status.
In `@packages/db/src/catalog.ts`:
- Line 3: Remove the unused Db symbol from the type import in catalog.ts,
leaving DbOrTx available for loadProviderCatalog().
---
Duplicate comments:
In `@packages/orchestration/src/schedule-fire.ts`:
- Around line 193-205: Update the catch block in the schedule-fire flow to pass
only AppError and SubmitError instances to fail(message), preserving their
code-aware message formatting and importing SubmitError from its declaring
module. Rethrow all other errors, including task/run insert failures and
database driver faults, so pg-boss retry semantics remain intact.
---
Nitpick comments:
In `@apps/api/src/test/triggers.integration.test.ts`:
- Around line 649-654: Remove the taskTypes updates that disable and re-enable
the task type around send, and update the preceding comment to state that the
invalid params mutation is what forces submission to fail and reaches the catch
path. Keep the existing params rewrite as the sole failure mechanism.
In `@packages/orchestration/src/schedule-fire.ts`:
- Around line 252-270: Update reconcileScheduleCalendar’s per-row catch to
report the failed schedule instead of silently discarding the error: accept an
optional reporter and invoke it with the row identifier and caught error, or
return failed IDs for the worker to log. Preserve the existing per-row isolation
and live.delete behavior while ensuring repeated register/unregister failures
produce an observable signal.
In `@packages/orchestration/src/submit.ts`:
- Around line 102-103: Serialize the quota validation in the submit transaction
by locking the project’s quota row before assertQuotaHeadroom evaluates usage,
ensuring concurrent submissions for the same project cannot pass against the
same pre-commit count. Update the flow around assertQuotaHeadroom in submit.ts
while preserving the existing hard-stop rejection and subsequent inserts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b84bc123-0852-45c1-bf2f-53dc6fbd5831
📒 Files selected for processing (19)
CHANGELOG.mdapps/api/src/routes/api-keys.tsapps/api/src/routes/schedules.tsapps/api/src/routes/trigger-inbound.tsapps/api/src/routes/triggers.tsapps/api/src/test/schedules.integration.test.tsapps/api/src/test/triggers.integration.test.tsapps/worker/src/index.tsdocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mdpackages/db/src/catalog.tspackages/orchestration/src/queue.tspackages/orchestration/src/resolve.tspackages/orchestration/src/run-plan.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/submit.tspackages/orchestration/src/trigger-fire.tspackages/orchestration/src/usage.tspackages/orchestration/src/worker-executors.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/api/src/routes/api-keys.ts
- packages/orchestration/src/usage.ts
- packages/orchestration/src/worker-executors.ts
- apps/api/src/routes/triggers.ts
- docs/design/04-execution-runtime.md
- apps/api/src/routes/schedules.ts
- packages/orchestration/src/run-plan.ts
- apps/api/src/routes/trigger-inbound.ts
- packages/orchestration/src/queue.ts
- packages/orchestration/src/trigger-fire.ts
- CHANGELOG.md
CodeRabbit was right about my own text. design/05 said an optional `x-agrippa-delivery-id` "makes that idempotent outright" directly after asserting a sender "can never" retry into a duplicate run — which reads as an unconditional platform guarantee. It is not one. The dedupe key is `(endpoint, external_id)` on a partial index, `external_id` comes from that optional header, and a sender that omits it has nothing to match on: any retry it makes, for any reason, inserts a second delivery and produces a second run. What the platform guarantees without the sender's help is narrower and worth stating separately: it acknowledges before submitting, so a slow submit cannot itself be the timeout that provokes the retry. Both manual locales get the same correction, since an integrator reads those rather than the design docs. Also drops a `Db` import in packages/db/src/catalog.ts left unused when `loadProviderCatalog` widened to `DbOrTx` in the round-2 commit. 581 pass / 5 skip / 0 fail; check and build green.
… loud repairs
Round 2's finding 1 offered two mechanisms — a unique firing key, or
committing the submission with its bookkeeping — and I implemented only
the second, treating the choice as settled. They close different
windows. The transaction covers the gap between the run insert and the
bookkeeping write; it says nothing about pg-boss re-delivering a job
whose transaction already committed, which after a worker dies mid-ack
arrives ~15 minutes later on the queue's defaults. `queue` then
submitted unconditionally, `replace` cancelled the run that same firing
had just created before submitting another, and `skip` only held while
the previous run was still running — which by then it usually is not.
Runs now carry `origin_key`, nullable with a partial unique index,
written inside the submission transaction: `schedule:<pg-boss job id>`
(stable across a retry, fresh per cron tick — the worker had the id in
hand and discarded it), `trigger:<delivery id>`, NULL for anything a
human submitted. `submitTaskIn` raises DuplicateFiringError, which
callers report as a skip, because fireSchedule routes anything else it
catches into a `schedule.failed` notification — paging a project
because the system refused to charge it twice is its own kind of
broken. Triggers already had this property through their delivery row's
`succeeded` flip; schedules had no per-firing record at all.
Calendar reconciliation, added in round 2, read the schedule rows
*before* the live calendar. Every writer commits its row first and
touches the calendar after, so that ordering raced the wrong way round:
a schedule created in the window was absent from the rows snapshot but
present in the calendar, and was torn out as an orphan seconds after
being created — enabled, no error, healthy in the UI, never firing. An
edit was worse: the reconciler compared the new entry against the old
row and re-registered the OLD cron, and nothing downstream re-validates
it, so the schedule genuinely fired at a time the UI said it did not.
The calendar is read first now, which makes the rows snapshot the
fresher of the two and every interleaving benign, and each unregister
re-validates against the row immediately before tearing an entry out.
The same function swallowed per-schedule failures into a bare `catch {}`
— no log, no counter, no rethrow — and the counters incremented after
the await, so a failure did not even show as a shortfall. If the only
drift was the schedule that kept failing, the sweeper logged nothing at
all, every tick, while the boot pass printed `+0/-0`: the exact line a
converged fleet prints. Both signals existed before the round-2 refactor
moved the loop into a package that takes no logger. Failures are now
returned, logged with the schedule id, counted against a restored `X/N`
denominator, and written to the amber `last_error` an admin already
reads on the row.
The webhook acknowledgement still awaited its enqueue.
`enqueueAfterCommit` bounds failure but never latency, and pg-boss
builds its own pool that waits indefinitely for a connection — so a
stalled database held the response open until Bun's 60s idleTimeout
killed the socket, and the sender saw a connection reset rather than a
status code. That is the most retry-triggering answer there is, and an
unkeyed retry inserts a second delivery and a second run while the
sweeper revives the first. It is no longer awaited; the durable row and
sweepTriggerDeliveries are the guarantee.
Delivery ids were truncated to 200 characters. The reachable half was
not the prefix collision but the empty string: Headers.get returns ""
for a present-but-empty header, "" is not null, and the dedupe index is
partial on IS NOT NULL — so every request carrying a blank
x-agrippa-delivery-id collapsed onto one delivery row forever, and once
that row succeeded the endpoint accepted webhooks and ran nothing,
answering 200 every time. Blank is now absent, and an over-long id is a
400: rewriting a sender's idempotency key into a shorter one turns two
distinct events into a duplicate. The test helper could not express the
failing case — `if (opts.deliveryId)` dropped an empty string — which
is why it survived two rounds.
Also clears the four noUnusedImports warnings left by the round-2
Db → DbOrTx widening, which `check` prints and then exits 0 on.
Verified against a real pg-boss cron tick as well as the suite: three
successive firings of a minutely schedule produced three runs with
three distinct job-id keys, and the restored boot line reads
"+0/-0 of N".
… place Four defects found reviewing 41f8c7c, the one commit on this branch nobody else had read. Three are mine, from that commit. **A redelivery destroyed the occurrence's run (P1).** The origin-key check lived inside the submission, which is downstream of the concurrency policy — so `replace` reached its cancellation first, killed the run THIS firing had created, and only then discovered the duplicate and returned without putting anything back. The occurrence ended with a destroyed live run and no replacement: strictly worse than the double charge the key exists to prevent, and the opposite of what the docstring and the changelog claimed. The key is now resolved at the top of fireSchedule, before anything observable happens; the in-transaction check stays as the atomicity guard. The new test missed it because it used `queue`, the one policy that never reaches the cancellation. **The lastError meant to make repairs loud could never be cleared (P1).** Nothing else clears that column for a schedule that is not firing — a *disabled* one had no route at all, and PATCH only clears it when the body happens to carry `enabled` — so the first transient calendar blip marked a schedule broken in the UI permanently. Loud-forever is the silence it replaced, reached from the other side. It now clears when the repair lands, scoped by prefix so a real firing error is never cleared by a calendar pass. Three more in the same seven lines: it reported unregister failures as registration failures, issued an UPDATE per tick for orphan keys that have no row, and re-stamped an unchanged message every minute on every replica — dead tuples on a read-mostly table at exactly the moment an outage has made every schedule fail at once. **A real constraint violation still paged (P2).** DuplicateFiringError was raised only by the pre-check, so a genuine 23505 — which two concurrent invocations of one firing can still reach, since pg-boss expiry re-delivers a job a live worker still holds — fell through to the failure path: on schedules the raw constraint name in a `schedule.failed` notification, on triggers a delivery flipped to `failed` behind a run that had succeeded, unclearable because Retry refuses a delivery that already has a run id. Now classified at the insert. Verified the field names against the driver rather than assuming: bun-sql reports SQLSTATE in `errno` while `code` is its own tag, so checking `code` alone would have made the whole branch dead. **And the meta-pattern twice more.** Round 3 fixed the awaited enqueue at the two inbound sites and left it on the front door: submitTask backs `POST /projects/:id/tasks`, the first entry on the API-key allow-list, and the one API-key-reachable mutation with no idempotency guard at all — same stall, same connection reset, same retry, but a second task, run and charge with nothing to dedupe against. Separately, sweepNotificationDeliveries — the function sweepTriggerDeliveries was copied FROM — still lacks both guards the copy was given in round 1, with the comment explaining why written into the copy. A delivery whose POST is in flight could be stamped `failed`; the success path overwrote it back, but in that window the UI offered Retry and a click re-sent a duplicate webhook to the customer. Smaller: origin_key no longer rides a `...run` spread into GET /runs/:id (a queue identity is not an API surface), the 400 carries the header name and the limit in `details` since the localized message wins over the constructed one, the trigger duplicate branch writes a terminal status instead of leaving the sweeper to exhaust it, `?limit=abc` no longer 500s, two ARCHITECTURE invariants no longer share a number, and the schedule queue carries a note that a dead-letter queue would break the invariant by minting a new job id. Verified against a real pg-boss cron under `replace`: two ticks produced two runs with two distinct job-id keys and the first correctly cancelled, while the unit test proves a redelivery of one key leaves the run alive.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/api/src/test/triggers.integration.test.ts (1)
844-857: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClear the race timer after the request settles.
The 2-second timer stays armed after
Promise.raceresolves. Bun keeps the event loop alive until it fires, so the suite pays the full delay. Store the handle and clear it in afinally.♻️ Proposed change
- const res = await Promise.race([ - stalled.request(`/api/triggers/${trigger.token}`, { - method: "POST", - headers: { - "content-type": "application/json", - [TRIGGER_TIMESTAMP_HEADER]: ts, - [TRIGGER_SIGNATURE_HEADER]: `v1=${createHmac("sha256", SECRET) - .update(`${ts}.${raw}`) - .digest("hex")}`, - }, - body: raw, - }), - new Promise<never>((_, reject) => setTimeout(() => reject(new Error("ack blocked")), 2_000)), - ]); + let timer: ReturnType<typeof setTimeout> | undefined; + let res: Response; + try { + res = await Promise.race([ + stalled.request(`/api/triggers/${trigger.token}`, { + method: "POST", + headers: { + "content-type": "application/json", + [TRIGGER_TIMESTAMP_HEADER]: ts, + [TRIGGER_SIGNATURE_HEADER]: `v1=${createHmac("sha256", SECRET) + .update(`${ts}.${raw}`) + .digest("hex")}`, + }, + body: raw, + }), + new Promise<never>((_, reject) => { + timer = setTimeout(() => reject(new Error("ack blocked")), 2_000); + }), + ]); + } finally { + clearTimeout(timer); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/test/triggers.integration.test.ts` around lines 844 - 857, Update the Promise.race in the trigger request test to store the 2-second setTimeout handle, then clear it in a finally block after the race settles, preserving the existing timeout rejection and request behavior.packages/orchestration/src/trigger-fire.ts (1)
187-190: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse
err.runIdwhen the pre-check supplied it.
DuplicateFiringErrorcarriesrunIdwhenever the sequential pre-check raised it. Only the index path leaves itnull. The re-read is therefore redundant in the common case. Read only whenerr.runIdis null.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/orchestration/src/trigger-fire.ts` around lines 187 - 190, Update the DuplicateFiringError handling around the existing runs query to reuse err.runId when it is already populated by the sequential pre-check. Only execute the database re-read for the index path when err.runId is null, preserving the existing lookup and result handling for that case.packages/db/drizzle/0026_run_origin_key.sql (1)
2-2: 🩺 Stability & Availability | 🔵 TrivialPlan for the index build lock on large
runstables.
CREATE UNIQUE INDEXwithoutCONCURRENTLYtakes an ACCESS EXCLUSIVE lock onrunsand blocks reads and writes until the build finishes. The predicate limits the index to non-nullorigin_key, but Postgres still scans the whole table. If a deployment has a largerunstable, schedule this migration in a maintenance window, or build the index out-of-band withCONCURRENTLYbefore running the migration.CONCURRENTLYcannot run inside a transaction, so it does not fit the transactional migration runner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/drizzle/0026_run_origin_key.sql` at line 2, Address the runs_origin_key_uq index migration by scheduling its non-concurrent build in a maintenance window or prebuilding it out-of-band with PostgreSQL’s concurrent index operation before running the transactional migration; do not add CONCURRENTLY to this migration, since the migration runner requires transactional execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/04-execution-runtime.md`:
- Line 151: Update the inline code span on the affected documentation line to
remove any leading or trailing whitespace inside its backticks, while preserving
the surrounding wording and code content.
In `@docs/design/05-api-and-auth.md`:
- Line 21: Update the delivery guarantee wording around “exactly-once delivery”
to qualify it: a stable x-agrippa-delivery-id deduplicates sender retries into
one durable delivery and pg-boss job, but that delivery may still be retried or
fail. Preserve the existing at-least-once job-delivery and replay behavior
without claiming the delivery itself is guaranteed exactly once.
In `@docs/manual/zh-CN/04-administration.md`:
- Line 57: 在文档中定位包含 `x-agrippa-delivery-id` 和“超过 200
个字符”说明的句子，改写末尾截断规则，明确表示超长值会直接返回 `400`，不会被截断为可能产生标识冲突的值；保持其余去重和重试说明不变。
In `@packages/orchestration/src/schedule-fire.ts`:
- Around line 427-436: Update the clearing UPDATE in the healed-row flow to add
a lastError prefix predicate alongside the taskSchedules.id filter, using
CALENDAR_ERROR_PREFIX. Keep the existing healed calculation and null-field
updates unchanged so only rows whose current error still belongs to the
reconciler are cleared.
---
Nitpick comments:
In `@apps/api/src/test/triggers.integration.test.ts`:
- Around line 844-857: Update the Promise.race in the trigger request test to
store the 2-second setTimeout handle, then clear it in a finally block after the
race settles, preserving the existing timeout rejection and request behavior.
In `@packages/db/drizzle/0026_run_origin_key.sql`:
- Line 2: Address the runs_origin_key_uq index migration by scheduling its
non-concurrent build in a maintenance window or prebuilding it out-of-band with
PostgreSQL’s concurrent index operation before running the transactional
migration; do not add CONCURRENTLY to this migration, since the migration runner
requires transactional execution.
In `@packages/orchestration/src/trigger-fire.ts`:
- Around line 187-190: Update the DuplicateFiringError handling around the
existing runs query to reuse err.runId when it is already populated by the
sequential pre-check. Only execute the database re-read for the index path when
err.runId is null, preserving the existing lookup and result handling for that
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d816e8b-5a85-4410-8760-1f3b80a19800
📒 Files selected for processing (30)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/routes/execution.tsapps/api/src/routes/trigger-inbound.tsapps/api/src/routes/triggers.tsapps/api/src/test/notifications.integration.test.tsapps/api/src/test/schedules.integration.test.tsapps/api/src/test/triggers.integration.test.tsapps/worker/src/index.tsdocs/design/01-domain-model.mddocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/manual/en/04-administration.mddocs/manual/zh-CN/04-administration.mdpackages/core/src/triggers.tspackages/db/drizzle/0026_run_origin_key.sqlpackages/db/drizzle/meta/0026_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/catalog.tspackages/db/src/schema/runs.tspackages/i18n/locales/en/errors.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/orchestration/src/notifications.tspackages/orchestration/src/queue.tspackages/orchestration/src/run-plan.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/submit.tspackages/orchestration/src/trigger-fire.tspackages/orchestration/src/usage.tspackages/orchestration/src/worker-executors.ts
💤 Files with no reviewable changes (1)
- packages/orchestration/src/usage.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/i18n/locales/en/errors.json
- packages/db/drizzle/meta/_journal.json
- packages/i18n/locales/zh-CN/errors.json
- packages/db/src/catalog.ts
- ARCHITECTURE.md
- apps/api/src/test/notifications.integration.test.ts
- apps/api/src/routes/triggers.ts
- packages/core/src/triggers.ts
- packages/orchestration/src/run-plan.ts
- CHANGELOG.md
- packages/orchestration/src/worker-executors.ts
- packages/orchestration/src/queue.ts
- apps/worker/src/index.ts
- apps/api/src/routes/trigger-inbound.ts
- apps/api/src/routes/execution.ts
|
|
||
| The transaction closes the gap *between the two writes*; it says nothing about the queue re-delivering a job whose transaction already committed, which is a second and independent way to charge twice. **Every unattended firing therefore carries an idempotency key** — `runs.origin_key`, nullable with a partial unique index, set inside the submission transaction: schedules pass `schedule:<pg-boss job id>`, triggers pass `trigger:<delivery id>`, and the human paths pass nothing (a second click is a second intent). Both values are stable across a redelivery and distinct between firings — pg-boss preserves a job's id when it re-inserts a retried job and mints a new one per cron tick — so "this run exists" and "this firing already happened" become one committed fact. The key is resolved **before the concurrency policy**, not at the submission: everything below that point has side effects and `replace`'s is destructive, so a redelivery discovered late would cancel the run its own firing created and then decline to replace it. `submitTaskIn` raises `DuplicateFiringError`, which callers report as a skip rather than a failure: `fireSchedule` routes anything else it catches into a `schedule.failed` notification, and paging a project because the system correctly refused to charge it twice is its own kind of broken. Schedules needed this most — a trigger already had a per-firing row whose `succeeded` flip commits with the run, while a schedule's `lastRunId` is a pointer overwritten each firing, and the liveness check that read it only held while the previous run was still running, which at pg-boss's ~15-minute redelivery latency it usually is not. | ||
|
|
||
| The calendar has its own convergence: a `schedule-calendar` sweeper stage diffs `task_schedules` against `boss.getSchedules`, so a registration dropped at request time is repaired within a tick rather than at the next worker restart, and an entry whose row was deleted is removed. **The calendar is read before the rows**, and the order is load-bearing: the two stores have no transaction spanning them, and every writer commits its row before touching the calendar, so reading the calendar first makes the rows snapshot the fresher of the two and turns every interleaving benign — a schedule created mid-reconcile is registered rather than mistaken for an orphan and torn out, and an edit converges onto the new cron instead of having the old one restored under it. What that buys is convergence within a tick, not mutual exclusion, which no read order can give across two stores; the remaining window is closed for the one repair that can silence a live schedule by re-validating the row immediately before each unregister. A schedule the reconciler cannot repair is **named** — returned to the worker, logged with its id, and written to the same `last_error` an admin already reads on the row — because a repair loop that tolerates failure silently is indistinguishable from one that succeeded, and the schedule stays unregistered while the drift line reads `+0/-0`. That message is prefixed `calendar `, and **the reconciler clears it once the repair lands**: nothing else can, for a schedule that is not firing, so without it one transient blip marks a schedule broken in the UI permanently — loud-forever being the same failure as silence, reached from the other side. The write is guarded against re-stamping an unchanged message, since it runs every minute on every replica. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove whitespace inside inline code spans.
markdownlint reports MD038 on Line 151. Remove leading or trailing spaces inside the affected backtick span.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 151-151: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/04-execution-runtime.md` at line 151, Update the inline code span
on the affected documentation line to remove any leading or trailing whitespace
inside its backticks, while preserving the surrounding wording and code content.
Source: Linters/SAST tools
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/design/05-api-and-auth.md (1)
21-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not claim exactly-once delivery.
A stable
x-agrippa-delivery-iddeduplicates repeat submissions into one durable delivery and job. The delivery can still be retried or fail because queue processing is at least once. Replace “gets exactly-once delivery” with “gets retry-safe deduplication” or equivalent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/05-api-and-auth.md` at line 21, Update the delivery guarantee wording in the API/auth design text: replace the claim that a stable x-agrippa-delivery-id provides “exactly-once delivery” with wording that accurately describes retry-safe deduplication, while preserving the existing explanation of durable delivery records and at-least-once queue processing.docs/manual/zh-CN/04-administration.md (1)
57-57: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win请明确说明超长标识不会被截断。
不会被截断成可能撞车的另一个值表述不够准确。建议改为而不会被截断为可能与其他事件冲突的值，以明确说明超长值会直接返回400，不会被截断为冲突标识。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manual/zh-CN/04-administration.md` at line 57, 修改中文文档中关于超过 200 个字符的 x-agrippa-delivery-id 的表述，明确说明该值会直接返回 400，且不会被截断为可能与其他事件冲突的标识；保留其余去重和重试说明不变。Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/design/05-api-and-auth.md`:
- Line 21: Update the delivery guarantee wording in the API/auth design text:
replace the claim that a stable x-agrippa-delivery-id provides “exactly-once
delivery” with wording that accurately describes retry-safe deduplication, while
preserving the existing explanation of durable delivery records and
at-least-once queue processing.
In `@docs/manual/zh-CN/04-administration.md`:
- Line 57: 修改中文文档中关于超过 200 个字符的 x-agrippa-delivery-id 的表述，明确说明该值会直接返回
400，且不会被截断为可能与其他事件冲突的标识；保留其余去重和重试说明不变。
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b94c77b4-23ff-494e-a622-9df9aa262203
📒 Files selected for processing (30)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/routes/execution.tsapps/api/src/routes/trigger-inbound.tsapps/api/src/routes/triggers.tsapps/api/src/test/notifications.integration.test.tsapps/api/src/test/schedules.integration.test.tsapps/api/src/test/triggers.integration.test.tsapps/worker/src/index.tsdocs/design/01-domain-model.mddocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/manual/en/04-administration.mddocs/manual/zh-CN/04-administration.mdpackages/core/src/triggers.tspackages/db/drizzle/0026_run_origin_key.sqlpackages/db/drizzle/meta/0026_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/catalog.tspackages/db/src/schema/runs.tspackages/i18n/locales/en/errors.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/orchestration/src/notifications.tspackages/orchestration/src/queue.tspackages/orchestration/src/run-plan.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/submit.tspackages/orchestration/src/trigger-fire.tspackages/orchestration/src/usage.tspackages/orchestration/src/worker-executors.ts
💤 Files with no reviewable changes (1)
- packages/orchestration/src/usage.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/db/src/schema/runs.ts
- apps/api/src/test/notifications.integration.test.ts
- packages/db/drizzle/0026_run_origin_key.sql
- packages/db/drizzle/meta/_journal.json
- packages/orchestration/src/run-plan.ts
- packages/orchestration/src/trigger-fire.ts
- packages/core/src/triggers.ts
- packages/i18n/locales/en/errors.json
- apps/api/src/routes/triggers.ts
- ARCHITECTURE.md
- packages/orchestration/src/queue.ts
- apps/api/src/routes/execution.ts
- packages/orchestration/src/worker-executors.ts
- CHANGELOG.md
- packages/orchestration/src/notifications.ts
- apps/api/src/test/triggers.integration.test.ts
- apps/worker/src/index.ts
- packages/i18n/locales/zh-CN/errors.json
- packages/orchestration/src/schedule-fire.ts
- docs/design/01-domain-model.md
- packages/db/drizzle/meta/0026_snapshot.json
- packages/orchestration/src/submit.ts
- apps/api/src/test/schedules.integration.test.ts
- apps/api/src/routes/trigger-inbound.ts
- packages/db/src/catalog.ts
CI was green and both reviewers had signed off on everything except two
concurrency findings. Triaging CodeRabbit's older unresolved threads while
verifying those turned up something neither report led with.
**Every schedule and trigger notification shipped with an empty body.**
deliverNotification sourced its template variables from `run_events`, but a
project-scoped event — a schedule or trigger that stopped — has no run to hang
an event row on, so insertProjectEventDeliveries puts the payload on the
delivery row instead, and nothing ever read it. The card an operator received
read, verbatim: `The schedule "" in Acme produced no run this time: .` That is
the only channel that reaches a human when unattended work breaks, and it is
the argument this branch has made in three rounds of docs, comments and both
manuals. It reached them with nothing in it. Two bugs in one path: the payload
never arrived, and `error` is emitted as a formatted string while the renderer
cast it to {code, message}, so a truthy string took the object branch and
rendered empty even when the payload was there. Disabled reasons interpolated
as raw enums into prose; they now resolve through the same catalog the
settings page uses.
**An infrastructure fault spent the occurrence.** fireSchedule and fireTrigger
classified only DuplicateFiringError and funnelled everything else — deadlocks,
statement timeouts, plain bugs — into the record-and-announce path, which
returns rather than throws. The pg-boss job then completed, so a weekly
schedule lost a week to a transient error and the project was told it was at
fault. Only AppError and SubmitError are the project's problem now; the rest
escapes so pg-boss retries, which is what AGENTS.md says the engine does and
what the worker's comment at the call site already claimed happened here. An
existing test pinned the old behaviour and changes with it.
**The two races codex found, one of which CodeRabbit found independently with
the same fix.** The healing clear tested ownership in JavaScript against the
rows snapshot but filtered the UPDATE by id alone — so a genuine firing error
committed during the reconcile was wiped, and the row rendered healthy until
its next failure. Ownership is now a SQL predicate evaluated under the row
lock, with the id list kept: a bare prefix match would erase the failures the
same tick just recorded. And the register leg wrote cron/timezone straight
from that snapshot while both unregister legs re-validated first — but the
snapshot is taken once and the loop is serial with a round trip per repair, so
a row late in the list acted on a reading many seconds old, and an edit landing
in the gap had its old cron restored under it, which nothing downstream
re-validates. Every repair now re-reads before writing. Neither mistake is
durably worse than the other and both self-heal within a tick, so the
docstring's justification for guarding one leg was wrong and is rewritten —
along with the sentence claiming the clear was already scoped by prefix, which
was the sharper defect, being what a reader would trust instead of re-deriving
the race.
The 60s sweeper also gained a re-entrancy guard: setInterval does not await its
callback, so a slow sweep could overlap itself and run two reconciles that
invalidate each other's snapshots. The worker logs the outcome's error/reason
rather than just its discriminant — a lost firing used to print "failed" with
no cause.
Also: `Bearer` is matched case-insensitively per RFC 9110 (a lowercase scheme
fell through to the session branch and answered a bare `unauthorized`), with
the daemon router's duplicate parse folded into the shared helper that calls
itself the one implementation; the exactly-once claim in design/05 qualified to
the at-most-once it actually is; the stale Governance api-keys block corrected;
MD038 and a colloquial zh-CN phrase fixed.
Verified against a real pg-boss cron as well as the suite: a minutely schedule
registered, fired on a genuine job id, and left no spurious calendar error.
Dismissed as verified non-defects: the retry attribution (c.var.user is
populated for API-key callers from the key's owner), the cron error message,
and the junk Content-Length test.
Six findings reviewing the previous commit, which nobody else had read.
Four are mine, from that commit. Two of them matter.
**Making unexpected errors propagate left the persistent case silent.**
Retrying beats recording only while retries remain. Past the limit the job
rests in `failed` — and pg-boss does not emit on handler failure, and no
dead-letter queue is configured (deliberately: redrive would mint a new job
id and therefore a second origin_key). So a permanent error produced no
lastError, no schedule.failed, no log line, and a UI showing the schedule
healthy. I traded a wrong-but-visible outcome for an invisible one, which is
the exact failure this feature exists to prevent, while fixing its opposite.
Both firings now take the last attempt as a signal to record instead — the
shape the notification handler thirty lines above already used. That also
covers the permanent-but-untyped cases the AppError/SubmitError split misses:
a template row that cannot be upgraded, params that fail a bare property read.
The commit body's own list conflated transient faults with plain bugs; only
the first deserved to escape.
**The localized reason doubled the sentence.** It resolved through the
settings catalog, whose strings stand alone as a status line, so the card read
"was disabled and will not run again: Stopped: its owner no longer has access
to this project.." — doubled clause, doubled terminator, in all four
renderings and both locales. The notifications namespace gets its own
clause-shaped wording; the settings copy is left alone. Two surfaces, two
grammars.
**And my "exhaustive" audit of the payload column missed a statement.** The
dual-purpose read is safe because only the success path writes the snapshot
and a succeeded delivery is never re-delivered — but that rests on a succeeded
row never becoming `failed`, and the endpoint-disabled write had no
`status = 'pending'` guard and ran before the claim. A duplicate job could
stamp `failed` over a delivery that had just succeeded, and the retry would
re-render from the request snapshot. Guarded now, like its sibling.
**Two tests proved less than they appeared to.** The cron-race test committed
its edit *before* calling the reconciler, so the snapshot already held the new
value — it passed against the unfixed code. It now commits from inside a
previous row's repair, which is the only way into that window; verified by
reverting the fix and watching it fail. And the notification end-to-end
asserted on the raw request body, which the generic formatter fills with the
payload verbatim — so it passed on the echo even if the rendering were empty.
It now asserts the rendered message, with an exact string, which is what
catches the doubling above. Nothing pinned the reason catalog against the
enums either; a fourth reason would have shipped as a raw code with a green
suite.
Smaller: the wrong-verb defect fixed in round 4 came back narrower, because
the refactor moved the re-read ahead of the branch that sets the label —
seeded from the snapshot now. The sweeper's re-entrancy guard was unbounded,
and nothing on that path has a timeout, so one hung call would have disabled
every recovery mechanism until restart — the same argument the per-stage
isolation makes, one level up; it now force-releases past a deadline and says
so at error level. outcomeDetail dropped the run id on the success branch and
is typed as the two unions so the next omission fails to compile. Three
docstrings that described retired rules: the notifications schema (retries
"re-render from the durable event" — project-scoped ones have none),
currentState ("before an unregister" — it now guards every repair), and
design/04's two-way split, which is three-way.
Verified: 601 pass, 5 skip, 0 fail.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/i18n/src/index.ts (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
notificationMessagesand drop the structural cast.Line 94 repeats the locale resolution already implemented at lines 71-73 and casts the catalog to a hand-written shape. The cast removes type checking on
reasons, so a locale file that loses the key compiles and renders the raw enum code. CallingnotificationMessages(locale)keeps one locale rule and lets the JSON-derivedNotificationCatalogtype describe the field.♻️ Proposed refactor
export function disabledReasonText(reason: string, locale: string): string { - const table = (locale.startsWith("zh") ? resources["zh-CN"] : resources.en).notifications as { - reasons?: Record<string, string>; - }; - return table.reasons?.[reason] ?? reason; + const { reasons } = notificationMessages(locale) as NotificationCatalog & { + reasons?: Record<string, string>; + }; + return reasons?.[reason] ?? reason; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/i18n/src/index.ts` around lines 93 - 98, Update disabledReasonText to obtain the notification catalog through notificationMessages(locale) instead of resolving resources directly. Remove the handwritten structural cast and read reasons from the typed NotificationCatalog result, preserving the existing raw reason fallback when no translation exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/i18n/src/index.ts`:
- Around line 93-98: Update disabledReasonText to obtain the notification
catalog through notificationMessages(locale) instead of resolving resources
directly. Remove the handwritten structural cast and read reasons from the typed
NotificationCatalog result, preserving the existing raw reason fallback when no
translation exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1229960-a3c8-4147-b078-919b253e9a84
📒 Files selected for processing (18)
CHANGELOG.mdapps/api/src/lib/bearer-tokens.tsapps/api/src/routes/daemon.tsapps/api/src/test/api-keys.integration.test.tsapps/api/src/test/schedules.integration.test.tsapps/api/src/test/triggers.integration.test.tsapps/worker/src/deps/notify.test.tsapps/worker/src/deps/notify.tsapps/worker/src/index.tsdocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/manual/zh-CN/04-administration.mdpackages/db/src/schema/notifications.tspackages/i18n/locales/en/notifications.jsonpackages/i18n/locales/zh-CN/notifications.jsonpackages/i18n/src/index.tspackages/orchestration/src/schedule-fire.tspackages/orchestration/src/trigger-fire.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/i18n/locales/zh-CN/notifications.json
- packages/i18n/locales/en/notifications.json
- apps/api/src/lib/bearer-tokens.ts
- CHANGELOG.md
- docs/design/04-execution-runtime.md
- apps/api/src/test/api-keys.integration.test.ts
- apps/api/src/test/triggers.integration.test.ts
- apps/api/src/test/schedules.integration.test.ts
- packages/orchestration/src/trigger-fire.ts
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.
M2 Track T, complete: the parts of the platform that let it run when nobody is watching. Three ways to start work without a person — a key, a clock, and an inbound event — over one shared submission path.
What's here
Project API keys (
Authorization: Bearer agr_<key>) — issued and revoked by project admins under Settings → API keys, plaintext shown exactly once, verification reusing the daemon-token scheme (prefix-indexed lookup, constant-time compare) now extracted into onebearer-tokensmodule that theagrd_and invitation tokens call too, replacing three near-identical copies.Two decisions shape what a key can do:
403. A denylist fails open on every route added afterwards — a key issued today would silently gain tomorrow's admin endpoint. Administration is unreachable by construction, and the org role is pinned toorg_memberon key requests so an admin's key still administers nothing.assertProjectRole, which now takes the whole request principal instead of a bare user id. That signature change is the enforcement: handlers reach a project through a:projectIdparam, a loaded run, or a loaded task, and all of them land in that function.Scheduled task submission — a task type, its parameters, and a five-field cron in an IANA timezone. pg-boss owns the calendar (and its DST edges); the worker re-registers every enabled schedule at boot, since the row and the calendar are written separately and a crash between them would leave a schedule that silently never fires.
The design turns on one distinction: conditions that will never heal on their own disable the schedule and announce it; everything else leaves it enabled and records the error. Owner lost access / project archived / task type withdrawn → stop, say why, audit,
schedule.disabled. Quota exhausted / grant revoked / no capable worker → stay on, surfacelast_error,schedule.failed, try again.The obvious alternative was to copy the API-key rule and let a schedule die with its owner's access. Rejected deliberately: a dead key is loud (a
401within minutes), a dead schedule is silent, and silence is indistinguishable from "no schedule was ever created" — the worst property available to the one feature whose job is to run unattended. Authority is checked at fire time using the samemembergate a human submit uses, rather than revoked eagerly, because eager revocation means hooking every path that can remove authority and missing one leaves a schedule firing with authority its owner no longer has.Fire-time date tokens —
pm.weekly-reporttakes a required free-textdateRangethat reaches both the prompt and the artifact title, and the template expression language has no date support by design. Without tokens a weekly schedule reports on the same fixed week forever: the M2 acceptance criterion literally true and practically worthless. Nine tokens ({{today}},{{lastWeekStart}},{{lastMonthEnd}}, …) resolve in the schedule's timezone from one clock reading shared withlastFiredAt. The set is closed on purpose — the failure mode is quietly growing a second expression language beside the deliberately non-Turing-complete one in ADR-0006. Parameters are validated at creation against the compiled template, so a schedule that could never produce a run is a400rather than aschedule.faileda week later.Schedules are created from a task (fill the normal submit form → "Run on a schedule"), so they carry exactly the parameters a real submission would; Settings → Schedules is the management view.
Inbound webhook triggers — a signed URL an external system POSTs to in order to start a task. Track N's outbound pipeline inverted, with the HMAC verified rather than generated, using the same
x-agrippa-timestamp/x-agrippa-signature: v1=…convention the platform sends.Three decisions shape it:
Authorization. That makes the URL a capability, which is exactly why the signing secret is mandatory here although it is optional on outbound IM endpoints: an unsigned inbound trigger is an open "spend this project's tokens" endpoint for anyone who ever sees the URL in a proxy log or a CI transcript. The signature covers the raw request bytes, and timestamps outside ±5 minutes are refused in both directions.202, so a failed submission stays visible and replayable instead of vanishing with the sender's connection, and a slow submit never becomes a timeout the sender retries into a duplicate run. An optionalx-agrippa-delivery-idmakes that idempotent outright, andfireTriggerrefuses an already-succeeded delivery — at-least-once job delivery and an operator hitting replay converge on one run.trigger.disabled/trigger.failednotifications are therefore the only channel that reaches a human — the same argument that made schedules disable loudly rather than skip. Both now sharecheckWorkAuthority.The received payload is stored for inspection and never interpolated into a prompt: a valid signature proves who sent the bytes, not that the bytes are safe. Mapping payload fields into parameters is deliberately out of scope — it is a prompt-injection surface deserving its own design.
Also in here
project.archivehas always writtenprojects.statusand nothing read it — the only two references to the value were the write itself — while both manuals promised it "stops accepting submissions". Survivable while a human had to click Submit; not once work arrives unattended, since an archived project is precisely the one nobody is watching.submitTask, quota, and worker-liveness move to@agrippa/orchestration. The schedule fire handler runs in the worker, which cannot import the api. None of these ever had an HTTP dependency; they were inapps/api/src/lib/because that is where the first caller happened to live.Verification
bun run check,bun test,bun run templates:validate,bun run buildgreen at every commit. 549 pass / 5 skip / 0 fail — the 5 are the pre-existing compliance cases that don't run through the remote transport. (An earlier run showed 116 skips because the dev stack was holding Postgres connections; that is the "0 fail with skips is not a pass" trap, re-run clean with the stack stopped.)New coverage: 14 trigger integration tests (one-time token, uniform 401 across unknown token / wrong secret / malformed signature / stale + future timestamp / disabled trigger, raw-byte signing, delivery-id dedupe, payload cap, owner-lost-access and archived-project disable, no-double-submit on redelivery, delivery log and failed-only replay), 15 schedule integration tests (CRUD, cron/timezone rejection, admin gating, all three concurrency policies, each disable reason, transient-failure-does-not-disable, token resolution), 10 API-key integration tests (one-time plaintext, uniform 401 across unknown/revoked/expired, per-route scopes, blanket 403 across eleven admin surfaces, cross-project denial, audit attribution, key dies with its owner's membership), and unit tests for the cron validator and the token date math (DST crossing, leap year, Monday-based weeks, timezone-not-server).
Driven in a real browser against a live stack, not only in tests. For schedules: filled the weekly-report form with a token, created the schedule, sent a
schedule.firejob through the running worker — the run'sparamsSnapshotcarrieddateRange: "2026-07-27..2026-08-02"while the schedule row kept the token. For triggers: created one through the browser, POSTed a correctly signed request (202) plus wrong-signature and stale-timestamp ones (both the same uniform401), repeated with the same delivery id (deduplicated, one delivery row), and confirmed the worker produced a run.That browser smoke earned its keep: it caught a bug the unit tests missed. Trigger creation validates the resolved parameters (sharing the schedule validator), so it accepted a
{{lastWeekStart}}token — but firing passedendpoint.paramsstraight through and the token reached the agent's prompt verbatim. Triggers now carry a timezone and resolve tokens at fire time exactly as schedules do, with a regression test asserting the run's snapshot holds dates while the stored parameter keeps the token.Noted, not fixed here
runs.params_snapshotandtasks.paramsare stored as jsonb strings, not objects —jsonb_typeofreturnsstringfor every existing row, including ones predating this branch. Runtime is unaffected (drizzle round-trips it), butparams_snapshot->>'key'returns null, so SQL over params silently yields nothing. Pre-existing; needs a migration plus backfill, so it wants its own change rather than being buried here.Summary by CodeRabbit