Repository navigation
fix(action,http,core)!: one caller's idempotent response went to another, and ten codes paged the on-call for a caller's mistake - #112
Conversation
…her, and ten codes paged the on-call for a caller's mistake The http and action remainder of audit slices 02 and 06, plus the gate step slice 02 asks for by name. auth and entity are absent because they are already closed — every finding naming them landed in #104 and #106, verified before this wave was scoped. `idempotencyKeyFor(actionName, key)` namespaced by action name only, with no actor anywhere, so alice's stored `charge` response was returned to bob whenever bob sent the same key; a differing payload gave bob X_IDEMPOTENCY_CONFLICT instead, which is a cross-actor denial of service against any key. And `Headers.get()` answers '' rather than null for `Idempotency-Key:`, so a blank header was a live key every blank sender shared. The status table is closed, so a code with no row falls to 500 — and stages.ts reports every status >= 500 to the error monitor. Ten caller-caused codes were in that state: a reused key, an expired cursor, a weak password, a duplicate signup. The sharpest was the framework contradicting its own published contract, action/http.ts:151 declaring '409' for X_IDEMPOTENCY_CONFLICT while the runtime answered 500. Nothing would have caught the eleventh, so the errors step gained a fourth host rule. The specified predicate — every code owned by a tier <= 4 package needs a row — flags 237 of 394, which is a step an agent disables in week one; and whether a code can reach a request is not derivable, since X_MIGRATION_DESTRUCTIVE and X_TENANCY_CROSS_DENIED are the same tier and the same shape and blanket re-exports collapse import reachability to the whole package. So it is a ratchet on the expectedRed idiom: 226 undecided codes pinned with a reason, the list may only shrink, and a pin says "nobody has decided yet" rather than "this can never reach a request". It caught two codes on its first run, both added by its own teammates in this PR. Also: a second server after a drain bound a port it could never serve from and kept accepting connections after its own stop() returned; ?__proto__= replaced the parsed query object's prototype, which also let a schema coerce an inherited function through `key in record`; an empty issues array read as validation success and reached a handler as an impossible undefined; the third copy of FNV-1a/32 over client-chosen input, here backing both the idempotency requestHash and the job dedupe key; and an unfenced settle overwriting a record already replaced. BREAKING CHANGE: `idempotencyKeyFor` takes a required third Actor argument; the stored key's shape changed, so on the shared Postgres store a retry crossing the deploy boundary re-runs the handler inside the 24h window (`truncate x_idempotency` makes that state honest); `Idempotency-Key` is now enforced at the 255 characters OpenAPI already published; action's `fingerprint` is SHA-256/16, changing job-handle.ts's dedupe key; `markReady()` throws X_LIFECYCLE_DRAINED on a drained lifecycle instead of declining silently. New codes: X_IDEMPOTENCY_KEY_INVALID, X_LIFECYCLE_DRAINED, X_ERROR_STATUS_MISSING, X_ERROR_STATUS_BACKLOG_STALE, X_ERROR_STATUS_UNKNOWN_CODE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 78 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (44)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
auth and entity needed nothing — every finding naming them was already closed by #104 and #106, verified file-by-file before #112 was scoped. That is what took the PR from an estimated ~70 files to 43. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBwWKBJkiogA4mDaJiJf3D
1 participant
The
httpandactionremainder of slices 02 (tier 2–3 bugs) and 06 (concurrency &lifecycle), plus the gate step slice 02 asks for by name.
Four agents on disjoint file sets in one checkout:
packages/action·packages/http(error-map +input) ·
packages/http/server.ts+packages/core/lifecycle.ts·scripts/.authandentityare not here because they are already done. Every finding in these twoslices naming those packages was closed by #104 and #106 — verified file-by-file before this wave
was scoped, which is why this PR is 43 files rather than the ~70 estimated.
The two that matter
One caller's idempotent response was replayed to another.
idempotencyKeyFor(actionName, key)namespaced by action name only — no actor — so alice's stored
chargeresponse went to bob wheneverbob sent the same key; with a differing payload bob got
X_IDEMPOTENCY_CONFLICTinstead, which isa cross-actor denial of service against any key. And
Headers.get()answers''— nevernull—for
Idempotency-Key:, so a blank header was a live key every blank sender shared.Ten codes answered 500 and paged the on-call for a caller's mistake. The status table is closed:
no row means
DEFAULT_STATUS, andstages.tsreports everystatus >= 500to the error monitor. Areused key, an expired cursor, a weak password, a duplicate signup — each woke somebody. The sharpest
case was the framework contradicting its own published contract:
action/http.ts:151declares'409'forX_IDEMPOTENCY_CONFLICTin OpenAPI while the runtime answered 500.Findings
actionactionIdempotency-Keyis a live shared keyX_IDEMPOTENCY_KEY_INVALIDhttphttp+coreX_LIFECYCLE_DRAINEDhttp?__proto__=replaces the parsed object's prototypeschema(via 5)key in recordlet a schema coerce an inherited functionhttpissuesarray reads as validation success → 500 + pageactionactionSETTLE/FAILcarry no status fence; a late settle overwritesdocs04-error-contract.mddocuments a status, an owner and a code that do not existThe gate step, and why it is a ratchet
Slice 02 asks for this in three places — "the error-map completeness check is a verify step, not a
convention". Built, with one deliberate departure from the specified rule.
The rule as written — every code owned by a tier ≤ 4 package needs a row — flags 237 of 394
codes, including
X_MIGRATION_DESTRUCTIVEandX_CRON_INVALID. That is a step an agent disablesin week one. Whether a code can reach a request turns out not to be derivable:
X_MIGRATION_DESTRUCTIVEandX_TENANCY_CROSS_DENIEDare the same tier and the same shape, andblanket
index.tsre-exports collapse import reachability to "the whole package" at every boundary.Every declaration-derived signal available (
OAUTH_ROUTE_STATUS, OpenAPIproblemResponse— onecall site,
HTTP_ERROR_TITLES) would together have caught 0 of the 10.So: a ratchet, the same
expectedRedidiom the tracked-app gate already uses. 226 undecidedcodes pinned with a reason per group; the list may only shrink. A pin means "nobody has decided
yet", never "this can never reach a request".
Three codes —
X_ERROR_STATUS_MISSING,X_ERROR_STATUS_BACKLOG_STALE,X_ERROR_STATUS_UNKNOWN_CODE(a mistyped row reads as enforced and maps nothing). It reads the exported
ERROR_STATUSobjectrather than parsing source, so
Object.hasOwndistinguishes "no row" from "row = 500" — whichstatusForstructurally cannot.It caught two codes on its first run against live work — both added by its own teammates in this
same PR, which is the best evidence available that it is aimed correctly.
Premises the hive falsified
X_QUERY_NOT_PAGEABLEis a developer bug by its ownfix:— an edit to the read's own SQL, nothing the caller sends changes it — andX_IDEMPOTENCY_REPLAYED_FAILUREsurfaces as itself only when the first attempt carried no code.Both got explicit 500 rows so the check sees a decision, not an omission.
scheduler.ts:153uses." That line is a colon-joined string overframework-controlled values with no actor in it — copying it would have produced exactly the
ambiguous encoding the brief warned against.
SQL_ACK/SQL_NACKare in the same file." They are injobs/driver-pg-sql.ts, and theyfence on id AND state because jobs' settle API carries the job id. Idempotency's does not, so
only the state half was available.
oauth-route-status.test.ts's error-map parser." It has none — it importsstatusFor.No parser was written at all; the check imports the live table.
30_000." Eleven. All now point at oneREPO_SCAN_TIMEOUT_MS.out['__proto__']on a plain object reads a valuethat is not
undefined, so the first occurrence takes the repeated-key branch and assigns anarray.
Object.prototypeis never mutated — it is per-object prototype replacement.Deliberately not fixed
action'sstableStringifystill folds-0,NaNandInfinity. It cannot takequery'sfix: it is also the OpenAPI document serializer, and emitting the bare token
NaNwould make apublished spec invalid JSON. Reachable —
JSON.parse('{"n":-0}')yields-0, so{n:-0}and{n:0}share onerequestHashand one job dedupe key. Needs the spec serializer split from thehash canonicalizer.
Known-Gapsrow landed.invoke()'s cache bust is not transaction-aware. A real design change and adbseam tier-3actiondeliberately does not import; no tracked app callstransaction(inapps/.Known-Gapsrow landed.admin(tier 5) is out of the completeness check's scope, thoughX_ADMIN_DENIEDreaches abrowser. The dashboard is dev-only and refused in production, so its 500 pages nobody; widening
costs ~80 more pins for no signal.
flight — needs the reservation id in
settle, i.e. a publicIdempotencyStoresignature change.Disproportionate for a LOW finding.
Breaking
idempotencyKeyFortakes a required thirdActorargument · the stored key's shape changed, so aretry crossing the deploy boundary re-runs the handler on the shared Postgres store (
truncate x_idempotencymakes that honest) ·Idempotency-Keyis enforced at the 255 chars OpenAPI alreadypublished ·
action'sfingerprintis SHA-256/16, changingjob-handle.ts's dedupe key ·markReady()throws on a drained lifecycle instead of declining silently.Codes
X_IDEMPOTENCY_KEY_INVALID,X_LIFECYCLE_DRAINED,X_ERROR_STATUS_MISSING,X_ERROR_STATUS_BACKLOG_STALE,X_ERROR_STATUS_UNKNOWN_CODE— all inwiki/Error-Codes.md,manifest regenerated (394 codes, 29 packages).
Gate
bun run verifygreen first run.bun run scripts/reference-app-gate.ts— both tracked apps ontheir ratchet.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.