[valthon]

Record before-create, before-update and before-delete hooks now keep the status and message of ctx.fail, and the field details of ctx.invalid, in the standard API error envelope, for any status the HTTP transport supports. A rejection still rolls back the primary write and the hook's database side-writes. Unsupported statuses and arbitrary hook errors keep the generic 400.

Ctx copies the message and every validation string before the hook returns, so hooks may pass stack buffers. Generated dispatch clears a recovered failure before each handler.

Hook 5xx responses, including allocation-failure fallbacks, reach onError and the reporter once, with phase before_hook, after rollback and writer release, so the callback can write to the database. Each report's message is <collection>: <status> <hook message>, so reporter dedup keeps distinct failures apart. Its err is OutOfMemory for the allocation fallback and Handled otherwise.

Unmapped 4xx statuses use bad_request across record hooks, typed and custom routes, and auth methods. Auth-method messages on a named 4xx status are intentionally client-facing. Messages on 5xx, non-error and transport-unnamed statuses (such as 432) stay operator-only: logged and redacted. Shared http validity helpers keep hook mapping and response normalization aligned. The framework guide, recipes, method contract and changelog describe these semantics.

The series is two commits: the shared status mapping, then the record-hook behaviour built on it.

Fixes #477.

Validation on Zig 0.16.0, against the tree of this head:

• zig build test --summary all returned 0 at both commits; 22/22 steps and 2269 of 2321 tests passed, 52 skipped.
• tests/admin with -n 4: 364 passed, 154 skipped. test_docs_parity.py and test_two_factor_golfsim.py were re-run in a checkout with git metadata and golfsim built: 11 passed.
• check-gating, allocator contracts, disabled agent CLI, error-code ledger, doctor ledger and changelog assembly passed.
• Restoring the constant Handled report text fails exactly the two reporter tests.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS

[Copilot]

🟡 Changes recommended

Accepted but transport-unsupported 4xx statuses are converted to generic 500 responses and lose the hook’s message, so status handling needs correction and regression coverage through response normalization.

1 open issue

• Reject unsupported 4xx statuses or preserve them in response normalization · New

What changed in this PR

This PR addresses #477 by preserving explicit client errors from record before-hooks, allowing API callers to receive meaningful rejection messages while retaining atomic rollback.

Changes:

• Preserve ctx.fail client statuses and messages and ctx.invalid field details without changing the public dispatcher signature.
• Expand regression coverage across create, update, and delete hooks, including rollback and response ownership.
• Document supported hook failures and generic fallback behavior.

File	Description
src/​api/​records.zig	Preserve hook rejection details and add response, rollback, and ownership tests.
docs/​recipes.md	Explain explicit client errors in the before-create recipe.
docs/​framework.md	Document before-hook error responses and rollback guarantees.
changelog.d/​record-hook-client-errors.md	Record the consumer-visible hook error fix.

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

No active review issues block the change to preserve explicit client-error responses from record before-hooks.

No open issues 🎉

1 resolved since last review

• Reject unsupported 4xx statuses or preserve them in response normalization

_{Review effort: Balanced}

[valthon]

Changes requested: please fix all 8 inline comments before merge. (Submitted as a comment because GitHub doesn't allow "request changes" on your own PR.)

Correctness (highest priority)

1. Use-after-free: ctx.fail/ctx.invalid slices are rendered after the hook frame is gone.
2. 5xx ctx.fail statuses become a generic 400, unlike custom routes and auth lifecycle hooks.
3. Supported 4xx statuses with no dedicated code (418, 422, …) go out with code: "internal". Fix this in error_codes.forStatus.
4. An error.Handled from a later hook can reuse an earlier ctx.handled the hook recovered from.

Docs
5. The §4 Semantics bullet in docs/framework.md still says before-hook rejections are always 400.

Design and cleanup
6. Reuse ctx.errorResponse(err) inside the hook scope (as fireAuthLifecycleBefore does) instead of the new failure out-parameter. This also fixes 1 and 2.
7. Move the inline 4xx/transport-status check into a named http helper shared with normalizeResponse.
8. The rollback test creates 60 TestEnvs. Cut it to a few representative cases per phase.

Note: this review is from reading the code. I did not build the branch or run its tests.

---

Generated by Claude Code

[Copilot]

🟢 Approval recommended

No active review issues are identified for the hook-error response changes.

No open issues 🎉

_{Review effort: Balanced}

[valthon]

Re-review of d90acf1: all 8 earlier findings are addressed. There are 3 new comments, 2 of which should be fixed before merge.

#	Earlier finding	Status
1	Use-after-free on hook-stack slices	✅ ctx.fail/ctx.invalid copy into Ctx-owned memory (copyHandled); tests now overwrite the original buffers after the call
2	5xx turned into 400	✅ preserved through http.isErrorStatus
3	Unmapped 4xx sent as internal	✅ fixed in forStatus (but see the new comment there)
4	Old ctx.handled reused	✅ clearHandled() runs before each handler; new dispatcher test
5	Contradictory Semantics docs	✅
6	Second error-mapping path	✅ renders with ctx.errorResponse(err) while the Ctx is alive
7	Inline status check	✅ http.isFinalStatus/isErrorStatus, shared with normalizeResponse
8	60 TestEnvs	✅ cut to 3 cases × 3 phases (nit: selected by index)

New

• Fix before merge: the forStatus change also affects typed routes, route ctx.fail and auth-method failures. Unmapped 4xx auth-method failures now send their message to clients where they used to become a generic 500. Please record this in the changelog, confirm it is intended, and test it.
• Fix before merge: a before-hook 5xx skips onError and the Sentry backstop, unlike custom routes.
• Nit: pick the reduced rollback-test cases by mode, not by array index.

Note: this re-review is from reading the code. I did not build the branch or run its tests.

---

Generated by Claude Code

[Copilot]

🔵 Needs a closer look

The transport update changes native polling and connection locking across Linux and macOS, requiring specialist concurrency review and still-unverified exact-head hosted ZigBase CI validation before approval.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The zap/facil.io repin changes concurrency-sensitive behavior outside the repository diff, requiring specialist confirmation of descriptor-generation and locking invariants across Linux and macOS.

No open issues 🎉

_{Review effort: Balanced}

[valthon]

Branch untangled and history rewritten; 1 finding left open (inline), and the Zig changes need a compile/test pass.

What changed on this PR

• The branch had merged in commits from #481/#482: the zap transport re-pins, tests/admin fixture/port-reservation fixes, and an HTTP connection-reuse changelog fragment. Those are already in #481 and #482 and have been removed here. The previous head 611eed2 is preserved on branch codex-does-too-much.
• History rewritten into 2 commits (authored by @valthon):
  1. 4e5b546 Map unmapped 4xx statuses to bad_request: forStatus, the auth-method redaction rule, the http status helpers + normalizeResponse refactor, the registry explanations, docs and changelog.
  2. 9c17484 Preserve record before-hook error responses: Ctx-owned failure copies, per-handler clearing, record-hook rendering, onError with phase before_hook after rollback and writer release, docs and changelog.
• Fixes from three adversarial review rounds are folded in:
  • registry explanations (internal and bad_request) and the guard comment updated;
  • changelog wording clarified;
  • auth-method failures with a transport-unnamed status (e.g. 432) are now logged and redacted, where before their message was lost;
  • before-hook incidents use the documented before_hook phase instead of request.

Not verified: these commits have NOT been compiled or tested. The session that rewrote them had no Zig toolchain. The PR description's validation section refers to the old head (611eed2) and no longer applies. Before merge:

• Run zig build test --summary all on each of the 2 commits, plus the relevant tests/admin cases.
• Specifically confirm that the onError-after-release test's inspection of the writer mutex internals (.state/.unlocked) compiles on Zig 0.16.
• The auth-method unnamed-status redaction branch has no test, because it logs via std.log.err, which the test runner treats as a failure.
• Update the PR description: it still describes the removed transport/fixture changes.

Open: see the inline comment on before-hook incident reports.

---

Generated by Claude Code

[Copilot]

🟡 Changes recommended

Before-hook 5xx failures share a deduplication key that suppresses distinct error reports during the default 60-second window, while the surviving report omits the status and rejection message.

1 open issue

• Before-hook 5xx deduplication suppresses distinct failures · New

_{Review effort: Balanced}

[valthon]

Follow-up on the pre-merge list

Answers to the pre-merge list in the 16:12 review, at c8f4a4a0 (2 commits on main):

• Compile and test each commit: zig build test --summary all returned 0 at a581480f and c8f4a4a0. The suite reports 22/22 steps and 2269 of 2321 tests passed, 52 skipped. tests/admin passed on this tree (364 passed, 154 skipped; test_docs_parity.py and test_two_factor_golfsim.py re-run separately after fixing my scratch setup, 11 passed).
• Writer mutex inspection on Zig 0.16: it compiles and the onError-after-release test passes in the default build.
• Auth-method unnamed-status redaction test: added. 432, 500, 503 and 200 go through response normalization and are checked for the generic internal response with no method message. The log uses the repo's warn-under-test seam, as webauthn.zig does.
• PR description: rewritten to describe this series only.
• Incident reports: see the two resolved threads.

Commit c8f4a4a0 has the same content as 32c8a6cf, which a separate adversarial review read in full for the delta from 9c17484 and found no bugs. That review noted one judgement call left as is: the report text does not name create, update or delete, so the same failure across those operations is one incident per dedup window.

[Copilot]

🟢 Approval recommended

No active review issues were identified for the record-hook error-handling changes.

No open issues 🎉

1 resolved since last review

• Before-hook 5xx deduplication suppresses distinct failures

_{Review effort: Balanced}