diff --git a/.agent-loop/LOOP_STATE.md b/.agent-loop/LOOP_STATE.md index 31f9f768..4b35c46c 100644 --- a/.agent-loop/LOOP_STATE.md +++ b/.agent-loop/LOOP_STATE.md @@ -4,22 +4,23 @@ - Active initiative: `WS-AUTH-001` - Workstream Authorization Service - Active planning chunk: none -- Active implementation chunk: none -- Branch: `codex/ws-auth-001-03-post-merge-memory` -- Worktree: `/home/abiorh/flow/workstream-authorization-service` -- Status: AUTH-03 merged through PR #109 as `f06532e` after Backend, Agent - Gates, CodeRabbit, internal review, and explicit human approval passed. +- Active implementation chunk: `WS-AUTH-001-04A` - Request And Error Context +- Branch: `codex/ws-auth-001-04-request-api-controls` +- Worktree: `/home/abiorh/flow/workstream-auth-001-04` +- Status: AUTH-04A implementation and required internal repair review pass on + production SHA `cdcaf77`; final reviewed candidate `4fd6db9` includes only + additive scalar-context and logging-state behavior-test repairs plus evidence. - Prior `WS-AUTH-001-01` reviewed implementation SHA: `be0b836` - Prior `WS-AUTH-001-01` final merged branch head: `b5217e1` -- Latest integrated `main` merge commit: `f06532e` -- Current gate: publish and merge the AUTH-03 post-merge memory update, then - stop. -- Next chunk: none; `WS-AUTH-001-04` remains proposed and inactive pending a - separate explicit user start after this memory update merges. -- Parallel initiative: `WS-QUAL-001-01B1B-R10` merged through PR #108 as - `5c47aba`, and this convergence records its post-merge memory. B2 remains - inactive pending a separate explicit user start; coverage chunk 02 remains - inactive pending B2 merge/memory and its separate explicit start. +- Latest integrated `main` merge commit: `1864867` +- Current gate: publish the ready PR, then require GitHub Backend, Agent Gates, + CodeRabbit, and explicit human review before merge. +- Next chunk: none; do not start `WS-AUTH-001-05` automatically. +- `WS-AUTH-001-04B` is inactive until AUTH-04A merges, memory is updated, and + the user gives its separate explicit start signal. +- Parallel initiative: `WS-QUAL-001-01B2` is paused at the user's direction so + AUTH receives the laptop's test capacity. Its last official whole-app result + remains `6466/8159` statements (`79.249908%`); no replacement evidence exists. - Parallel initiative: `WS-ART-001-01` merged through PR #101 as `050eb15`. `WS-ART-001-02` remains proposed and inactive pending a separate explicit user start. diff --git a/.agent-loop/REVIEW_LOG.md b/.agent-loop/REVIEW_LOG.md index 04b761e2..44ead3d1 100644 --- a/.agent-loop/REVIEW_LOG.md +++ b/.agent-loop/REVIEW_LOG.md @@ -1,5 +1,82 @@ # Review Log +## 2026-07-13 - WS-AUTH-001-04A Internal Review Passed + +All required implementation tracks pass on production SHA `cdcaf77`. +Test-validation head `47241cf` added one genuine real-ASGI scalar +validation-context branch assertion. A later test-only repair establishes and +restores logging capture after in-process Alembic configuration disables an +existing logger. Exact-head test-delta review confirmed identical production +blobs and retained every earlier result; the final reviewed test revision is +bound in the internal review evidence. Valid OpenAPI, response-header, +pre-response logging, real-ASGI behavior, inventory, compatibility, and memory +findings were repaired without rate controls, schema, grants, roles, routes, or +product-authority changes. + +Focused evidence is 235 passing tests. Changed-file statement coverage is +98.08 percent for API controls, 90.82 percent for the application factory, +90.70 percent for auth dependencies, and 92.36 percent for the task router. The +API drill, isolated-runner lifecycle suite, Ruff, docstring threshold, stale +scans, Markdown links, Agent Gates, additive test-delta scan, and diff hygiene +pass. A complete isolated backend regression run exposed an Alembic logging +state leak after 114 passing tests; the affected real-ASGI test now establishes +and restores its logging capture precondition, and the exact failing order +passes. GitHub Backend owns the complete-suite rerun before merge. The official +separate whole-app coverage baseline remains 79.249908 percent; no replacement +is claimed. + +## 2026-07-13 - WS-AUTH-001-04A Implementation Review Repair + +The final repaired 04A contract passed both required preimplementation review +tracks at exact commit `f98bbfc` before runtime implementation began. The +implementation crossed the 350 non-comment production-line checkpoint; scope +was inspected and frozen to the approved request/error boundary, with no rate +control, schema migration, grant, role, or product-authority work. The durable +checkpoint record was omitted at that time and is repaired here. The candidate +remains below the 500-line hard stop; 04B and later chunks are inactive. + +Required implementation review of exact head `2a129f4` passed security, +product authority, test-delta, CI-integrity, ASGI architecture, and shared +adapter-boundary checks, but rejected publication pending truthful per-route +OpenAPI, required response-header metadata, bounded pre-response failure +logging, complete real-ASGI error/schema/inventory proof, and synchronized loop +state. Those valid findings are in the first bounded repair cycle. A full-suite +run against `2a129f4` was stopped after findings made that exact-head evidence +obsolete; its partial output contained one unidentified failure and is not +completion evidence. + +## 2026-07-13 - WS-AUTH-001-04 Split Required Before Implementation + +Required L1 plan review rejected the combined request/error/rate-control parent +contract before any runtime edit. The valid findings required exact ASGI header +behavior and OpenAPI compatibility for request/error context, plus an +independent committed PostgreSQL transaction, database-time atomic upsert, +privacy-key framing, and migration proof for rate controls. + +The parent is split into `WS-AUTH-001-04A` Request And Error Context and +`WS-AUTH-001-04B` PostgreSQL Rate Controls. Only 04A contract repair and +preimplementation re-review are active. 04B requires 04A merge/memory and a +separate explicit user start. No runtime implementation was written under the +failed combined plan. + +The first split re-review passed senior/architecture/security/product/docs/reuse +at `f01427a` but QA/CI/test-delta required one more 04A contract repair: exact +per-branch errors and invalid-ID behavior, bounded validation shapes, +CI-equivalent isolated-runner/docstring proof, per-file coverage, and additive +test-delta evidence. Runtime implementation remains gated on that re-review. + +## 2026-07-13 - WS-AUTH-001-04 Started + +PR #110 merged AUTH-03 post-merge memory to `main` as `1864867` after Backend, +Agent Gates, CodeRabbit, internal review, and explicit human approval passed. +The user then explicitly started `WS-AUTH-001-04` in the isolated worktree +`/home/abiorh/flow/workstream-auth-001-04` on branch +`codex/ws-auth-001-04-request-api-controls`. + +AUTH-04 is L1/P1 authentication infrastructure. Discovery and required +preimplementation plan review must pass before runtime edits. AUTH-05, +POL-002-04, and all QA implementation chunks remain inactive. + ## 2026-07-13 - WS-AUTH-001-03 Merged PR #109 merged `WS-AUTH-001-03` to `main` as `f06532e` after final branch head diff --git a/.agent-loop/WORK_QUEUE.md b/.agent-loop/WORK_QUEUE.md index 8fca6331..e815bcb2 100644 --- a/.agent-loop/WORK_QUEUE.md +++ b/.agent-loop/WORK_QUEUE.md @@ -4,14 +4,14 @@ | Chunk | Title | Risk | Status | |---|---|---:|---| -| - | No active implementation chunk | - | AUTH-03 post-merge memory in progress; no product implementation active | +| `WS-AUTH-001-04A` | Request And Error Context | L1 | Internal review passed; ready PR publication active, complete GitHub checks required before merge | ## Planned Next | Chunk | Title | Risk | Status | |---|---|---:|---| -| `WS-AUTH-001-04` | Request, Error, And API Control Foundation | L1 | Proposed; inactive pending AUTH-03 memory merge and explicit user start | -| `WS-QUAL-001-01B2` | Baseline Evidence And CI Ratchet | L1 | Inactive pending explicit user start | +| `WS-AUTH-001-04B` | PostgreSQL Rate Controls | L1 | Inactive until 04A merge/memory and a separate explicit user start | +| `WS-QUAL-001-01B2` | Baseline Evidence And CI Ratchet | L1 | Paused for AUTH priority; no valid replacement baseline yet | | `WS-QUAL-001-02` | Project Service Coverage | L1 | Inactive until 01B2 merge/memory plus explicit user start | | `WS-POL-002-04` | Locked Runtime Execution And Routing Hardening | L1 | Inactive pending relevant authorization proof and a separate explicit user start | | `WS-ART-001-02` | Flow Node Adapter And Reconciliation | L1 | Proposed; inactive pending separate explicit user start | @@ -56,10 +56,11 @@ ## Proposed Next -`WS-AUTH-001-03` merged through PR #109 as `f06532e` after all required checks -and reviews passed. Its post-merge memory update is active; no AUTH product -implementation is active. Do not start `WS-AUTH-001-04` or `WS-POL-002-04` -automatically. +`WS-AUTH-001-03` post-merge memory merged through PR #110 as `1864867`. The user +explicitly started parent `WS-AUTH-001-04`. Required plan review split it before +runtime implementation. Only child `WS-AUTH-001-04A` contract repair and +re-review are active in its isolated worktree. Do not start 04B, AUTH-05, or +POL-002-04 automatically. Coverage R10 merged through PR #108. Do not start 01B2, chunk 02, or another coverage implementation chunk from this worktree. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md index f7fe1635..d57a57ba 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md @@ -15,7 +15,9 @@ stopped. | `WS-AUTH-001-01` | Adopt Authorization Baseline And Repository Contracts | L1 | Merged through PR #93 as `772af1d` | | `WS-AUTH-001-02` | Verified Issuer Token And JWKS Boundary | L1 | Merged through PR #107 as `060b780` | | `WS-AUTH-001-03` | Legacy Actor Classification Preflight | L1 | Merged through PR #109 as `f06532e` | -| `WS-AUTH-001-04` | Request, Error, And API Control Foundation | L1 | Proposed | +| `WS-AUTH-001-04` | Request, Error, And API Control Foundation | L1 | Split before implementation into 04A and 04B | +| `WS-AUTH-001-04A` | Request And Error Context | L1 | Internally reviewed; ready PR publication active and complete GitHub checks required | +| `WS-AUTH-001-04B` | PostgreSQL Rate Controls | L1 | Inactive pending 04A merge/memory and separate explicit start | | `WS-AUTH-001-05` | Authority Evidence And Idempotency Foundation | L1 | Proposed | | `WS-AUTH-001-06` | Canonical Actor Profile And Identity Link | L1 | Proposed | | `WS-AUTH-001-07` | Authorization Kernel And Permission Registry | L1 | Proposed | @@ -36,7 +38,8 @@ WS-AUTH-001-PLAN -> WS-AUTH-001-01 -> WS-AUTH-001-02 -> WS-AUTH-001-03 --> WS-AUTH-001-04 +-> WS-AUTH-001-04A +-> WS-AUTH-001-04B -> WS-AUTH-001-05 -> WS-AUTH-001-06 -> WS-AUTH-001-07 @@ -55,7 +58,9 @@ WS-AUTH-001-PLAN - Chunk 02 authenticates tokens but grants no product authority. - Chunk 03 provides a supported classification gate before schema migration. -- Chunk 04 establishes request/error/rate controls. +- Parent chunk 04 was split before implementation. Chunk 04A establishes + request/correlation and additive error compatibility; chunk 04B later owns + durable PostgreSQL rate controls and its migration. - Chunk 05 evolves the shared audit path and idempotency before mutations. - Chunk 06 establishes canonical actor resolution while preserving only the enumerated non-authoritative legacy workflow-eligibility consumers required @@ -72,8 +77,11 @@ WS-AUTH-001-PLAN ## Stop condition -`WS-AUTH-001-03` merged through PR #109 as `f06532e` after deterministic proof, -all required internal reviewers, Backend, Agent Gates, CodeRabbit, and explicit -human approval passed. Its post-merge memory update is the current gate. Do not -start `WS-AUTH-001-04` or `WS-POL-002-04`; both retain separate explicit start -signals and prerequisites. +AUTH-03 post-merge memory merged through PR #110 as `1864867`. The user +explicitly started parent AUTH-04. Required plan review split it before runtime +implementation. AUTH-04A's repaired-contract review passed at `f98bbfc`; +production review passed at `cdcaf77`, and the final test-only revision is +bound in internal review evidence after exact-head confirmation. Ready PR +publication and complete GitHub checks are the current gate. +Do not start AUTH-04B, AUTH-05, or POL-002-04; each retains its separate start +signal and prerequisites. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md index 4193174f..5a0e4141 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md @@ -158,3 +158,32 @@ start does not imply dependency approval. No production dependency or runtime implementation may change until the user explicitly accepts D12 and that acceptance is recorded here. The user explicitly approved D12 by replying "ok apporeved" after the final preimplementation plan review passed. + +## D13: Identity issuer is the stable verification boundary + +Status: accepted by the user on 2026-07-13. + +Workstream depends on one provider-neutral `IdentityIssuerVerifier` capability +port. It extends the repository-wide `ExternalServiceAdapter` convention and +is constructed through the typed +`ExternalServiceAdapterFactory[IdentityIssuerVerifier]` foundation being +prepared by `WS-ART-001-01C`. AUTH must adopt that merged shared foundation; it +must not create a second generic adapter registry or factory. + +The `/auth` dependency and product authorization layers must not change when an +additional external identity issuer is configured. Issuer-specific discovery, +JWKS, signature, claim, and introspection behavior remains behind explicit +adapter registration, construction, and configuration; it does not become a +second internal auth system or grant Workstream product authority. + +The current `AuthVerifier` port, `FlowAuthVerifier` adapter, and `flow_auth_*` +configuration names are migration inputs rather than the target vocabulary. +Their rename must be one bounded, atomic specification and implementation +chunk covering the port, adapter, factory, configuration compatibility, +tests, operator documentation, and stale-name proof. AUTH-04A does not absorb +that cross-cutting rename because its reviewed request/error scope is already +at the production size circuit breaker. The owning follow-up chunk is blocked +until the shared convention merges, and must then remove the old AUTH factory +entry point and migrate all callers in one clean cut. It must be planned and +reviewed before implementation; no compatibility factory alias, duplicate +verifier, service locator, or public login/session API may be introduced. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md index 67894ea9..b4454949 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md @@ -156,3 +156,74 @@ Production deployment inputs remain externally supplied: - Append-only audit facts for lifecycle and authority changes. - Fixed internal system actors for server-owned automation. - One PR-sized chunk at a time with deterministic evidence and internal review. + +## AUTH-04 Delta Discovery (2026-07-13) + +### Current behavior + +- `backend/app/main.py` only customizes FastAPI request-validation errors. It + has no request/correlation context, HTTP/unhandled exception envelope, or + response ID headers. +- Existing HTTP errors primarily expose FastAPI `detail`. Two bounded task + domain errors expose top-level `code/details`; existing tests rely on these + fields, so AUTH-04 must add the canonical nested envelope without deleting + compatibility fields. +- `backend/app/api/deps/auth.py` already distinguishes missing, invalid, and + unavailable authentication conditions internally, but its public responses + do not carry stable machine codes or correlation evidence. +- No rate-control model, query, service, or dependency exists. Redis is Celery + delivery infrastructure and is not an authority or API-control store. +- No first-access or admin-mutation endpoint exists yet. Their control + dependencies must be prepared here and attached only by the later owning + chunks; unrelated legacy reads must not consume those limits. + +### Current migration and ownership boundary + +- Current `main` migration head is `0016_artifact_domain`; the original AUTH-04 + `0016_*` filename allowance is stale after parallel artifact integration. +- AUTH-04 therefore owns `0017_api_controls` with down revision + `0016_artifact_domain`. +- Existing feature modules use `models -> repository -> service`, with + `backend/app/db/models.py` importing feature models for Alembic metadata. + `backend/app/modules/api_controls` must follow that pattern and own SQL and + transaction behavior. + +### Required compatibility and security properties + +- Request/correlation headers require bounded canonical UUID parsing, duplicate + rejection, safe generation, and no reflection of invalid bytes. +- The adopted fallback envelope is + `error.code/message/details/correlation_id/retryable`; raw exceptions, token + material, claims, provider responses, SQL, secrets, and PII are excluded. +- Existing `detail` and coded-domain fields remain additive compatibility + surfaces in this chunk; current intake assertions may not be weakened. +- Privacy-safe rate keys require keyed HMAC rather than persisted raw issuer, + subject, actor, token, network, or email values. +- Cross-replica fixed-window increments require one PostgreSQL atomic upsert + driven by database time. Missing configuration or database failure must fail + closed for the protected dependency. + +### Proof gaps owned by AUTH-04 + +- Header absence/propagation, malformed/duplicate values, success and every + error class, unhandled exception redaction, and legacy-field compatibility. +- Allowed, exact-limit, exceeded, expiry-reset, concurrent, cross-session, + distinct-scope/key, missing-secret, and database-unavailable rate behavior. +- Prior-head upgrade, downgrade, re-upgrade, constraints, unique key, indexes, + and database-time fields for migration `0017`. +- Full backend and API-contract regression proof without modifying product + authorization or attaching future controls to unrelated routes. + +### Required plan-review split + +The first combined AUTH-04 plan failed required preimplementation review before +any runtime edit. Request/error compatibility and durable rate consumption each +need an independently reviewable contract and production-code budget. + +- `WS-AUTH-001-04A` owns pure-ASGI request/correlation context, bounded additive + error envelopes, legacy response/header compatibility, and OpenAPI proof. +- `WS-AUTH-001-04B` later owns HMAC-keyed PostgreSQL counters, a dedicated + committed transaction, atomic database-time windows, `0017_api_controls`, + configuration failure behavior, and migration/concurrency proof. +- 04B is not activated by this split. It requires 04A merge/memory and a + separate explicit user start. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md index 50d09df7..50d2b7cd 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md @@ -23,7 +23,16 @@ to `main` as `5c47aba`; that main revision is integrated into AUTH, whose prior lifecycle reconciliation was reviewed at `a70b89c`. Backend, Agent Gates, and CodeRabbit passed on final branch head `43ffbfe`, then explicit human approval merged PR #109 to `main` as `f06532e` on 2026-07-13. AUTH-03 post-merge memory -is active; AUTH-04 remains inactive. +merged through PR #110 as `1864867`. The user then explicitly started AUTH-04. +Required L1 plan review rejected the combined request/error/rate-control scope +before runtime edits and required a split. AUTH-04A's final repaired contract +passed both required preimplementation reviews at `f98bbfc`. Implementation +candidate `2a129f4` then entered required internal review. Valid OpenAPI, +logging, behavior-proof, and lifecycle-evidence findings were repaired; all +required tracks pass on production SHA `cdcaf77`, and final test-only head +`4fd6db9` passed exact-head confirmation for the additive scalar-context and +logging-state behavior-test repairs plus lifecycle evidence. +AUTH-04B remains inactive. ## Active planning chunk @@ -31,12 +40,13 @@ None. ## Active implementation chunk -None. +`WS-AUTH-001-04A` - Request And Error Context. Internal review passed and ready +PR publication is active. Complete GitHub checks remain required before merge. +No later chunk is active. ## Current implementation branch -None. Post-merge memory uses `codex/ws-auth-001-03-post-merge-memory` and makes -no product implementation change. +`codex/ws-auth-001-04-request-api-controls`. ## Chunk status @@ -46,7 +56,9 @@ no product implementation change. | `WS-AUTH-001-01` | Merged | `codex/ws-auth-001-01-adopt-authorization-baseline` | #93 | Authorization baseline, Contributor terminology boundary, scanner, and repository contracts; merged as `772af1d`. | | `WS-AUTH-001-02` | Merged | `codex/ws-auth-001-02-verified-issuer-token` | #107 | Merged as `060b780`; reviewed code SHA `47dd5a7`. | | `WS-AUTH-001-03` | Merged | `codex/ws-auth-001-03-legacy-actor-classification` | #109 | Merged as `f06532e`; reviewed code `8c5334c`; final branch head `43ffbfe`. | -| `WS-AUTH-001-04` | Proposed | - | - | Request, error, and API control foundation. | +| `WS-AUTH-001-04` | Split | `codex/ws-auth-001-04-request-api-controls` | - | Parent split before runtime implementation. | +| `WS-AUTH-001-04A` | Internally reviewed | `codex/ws-auth-001-04-request-api-controls` | - | Production review passed at `cdcaf77`; final reviewed candidate `4fd6db9` confirmed. | +| `WS-AUTH-001-04B` | Inactive | - | - | PostgreSQL rate controls; requires 04A merge/memory and separate explicit start. | | `WS-AUTH-001-05` | Proposed | - | - | Authority evidence and idempotency foundation. | | `WS-AUTH-001-06` | Proposed | - | - | Canonical actor profile and identity link. | | `WS-AUTH-001-07` | Proposed | - | - | Authorization kernel and permissions. | @@ -62,11 +74,15 @@ no product implementation change. ## Blockers -No active implementation blocker because no AUTH product chunk is active. -AUTH-04 remains proposed and may start only after this memory update merges and -the user gives a separate explicit start signal. Non-test operators must later -supply explicit classification evidence rather than inferred kinds before the -owning canonical actor migration. +No external blocker. The combined AUTH-04 plan failed its internal gate and was +split before runtime edits. AUTH-04A's repaired contract passed at `f98bbfc`; +valid implementation-review findings passed repair review. A local complete +run exposed and reproduced an Alembic logging-state test interaction; its exact +order now passes, and GitHub Backend owns complete-suite proof before merge. +AUTH-04B later owns the migration corrected from the now-owned `0016` prefix to +`0017` on current main, but remains inactive. Non-test +operators must later supply explicit classification evidence rather than +inferred kinds before the owning canonical actor migration. AUTH-03 review evidence and its PR trust bundle are recorded at `reviews/WS-AUTH-001-03-internal-review-evidence.md` and @@ -77,3 +93,10 @@ AUTH-03 review evidence and its PR trust bundle are recorded at Production issuer configuration and legacy non-test actor classification are future implementation/live-proof inputs and are tracked explicitly in `DISCOVERY.md` and chunk stop conditions. + +The user accepted D13 on 2026-07-13: the target provider-neutral boundary is +`IdentityIssuerVerifier`, extending the shared `ExternalServiceAdapter` +convention and using its typed factory. The current verifier/configuration +names remain functional migration inputs. The atomic AUTH adoption requires +its own reviewed chunk after `WS-ART-001-01C` installs the shared foundation; +it is not part of the size-capped AUTH-04A request/error implementation. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04-request-api-controls.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04-request-api-controls.md index f304934b..cda98b57 100644 --- a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04-request-api-controls.md +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04-request-api-controls.md @@ -1,5 +1,11 @@ # Chunk Contract: WS-AUTH-001-04 - Request, Error, And API Control Foundation +## Status + +Split before implementation after required plan review. This parent is not an +implementation contract. Its authorized children are `WS-AUTH-001-04A` and +`WS-AUTH-001-04B`; only 04A is currently active. + ## Parent initiative `WS-AUTH-001` - Workstream Authorization Service @@ -38,7 +44,7 @@ backend/app/core/api_controls.py backend/app/core/config.py backend/app/modules/api_controls/** backend/app/db/models.py -backend/alembic/versions/0016_*.py +backend/alembic/versions/0017_*.py backend/tests/test_app.py backend/tests/test_auth.py backend/tests/test_alembic.py @@ -63,7 +69,16 @@ dependency overrides that bypass the real request path - Every request has validated request/correlation IDs with safe generation and propagation rules. +- `X-Request-ID` and `X-Correlation-ID` accept one canonical UUID value each; + absent request IDs are generated, absent correlation IDs reuse the effective + request ID, and malformed/duplicate values fail as `invalid_request` without + reflecting the supplied bytes. +- Successful and error responses return the effective request/correlation IDs + in headers. The structured error object contains the correlation ID only. - Errors use a stable envelope without raw exception, token, claims, or PII. +- The envelope is `error.code/message/details/correlation_id/retryable`. + Existing `detail`, `code`, and `details` fields remain additively compatible + during the bounded migration so current intake clients and tests do not break. - Postgres-backed rate controls work across replicas, use privacy-safe keys and database time, and are configurable for first access/admin mutations. - `backend/app/modules/api_controls` owns feature models, persistence queries, @@ -73,6 +88,15 @@ dependency overrides that bypass the real request path controls fail closed without bypassing protected operations. - Migration owns counter constraints/indexes/database-time fields and proves prior-head upgrade, downgrade, and re-upgrade. +- The rate-control table has one atomic fixed-window counter per control scope + and HMAC-SHA256 subject key. The repository owns PostgreSQL upsert/database + time; the service owns key derivation and limit decisions. +- Rate-control invocation without its configured secret or with unavailable + persistence returns a retryable safe 503. Exceeded controls return a stable + retryable 429 with a bounded `Retry-After` value. +- AUTH-04 exposes named first-access and admin-mutation control dependencies for + their later owning endpoints but does not attach them to unrelated legacy + reads or implement those future endpoints. - Full backend suite and the intentionally updated API contract drill preserve the existing intake lifecycle and error compatibility. @@ -111,3 +135,19 @@ database-time semantics, and unchanged product authority. Stop if rate controls require a new production dependency without explicit human approval or if existing intake assertions must be weakened. + +## Activation + +AUTH-03 post-merge memory merged through PR #110 as `1864867`. The user +explicitly started AUTH-04 on 2026-07-13. Discovery found that parallel artifact +work already owns migration `0016`, so this chunk uses additive migration +`0017_api_controls` from `0016_artifact_domain`; this is a sequence correction, +not a scope expansion. + +## Split implementation plan + +1. Implement and merge 04A request/error context under its repaired contract. +2. Update post-merge memory and stop. +3. Start 04B only after a separate explicit user signal; implement its durable + rate controls and migration under its own reviewed contract. +4. Do not start AUTH-05 automatically after either child. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04A-request-error-context.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04A-request-error-context.md new file mode 100644 index 00000000..127fea51 --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04A-request-error-context.md @@ -0,0 +1,220 @@ +# Chunk Contract: WS-AUTH-001-04A - Request And Error Context + +## Parent initiative + +`WS-AUTH-001` - Workstream Authorization Service + +## Goal + +Add canonical request/correlation context and a stable privacy-bounded error +contract while preserving existing response fields, headers, routes, and +product authorization behavior. + +## Risk class and SLA + +L1 / P1 + +## Size circuit breaker + +- Stop at 350 changed non-comment production lines for a scope checkpoint. +- Stop and replan above 500 changed non-comment production lines. +- Test, documentation, generated schema, and durable loop evidence lines do not + count toward the production cap. + +## Allowed files + +```text +backend/app/main.py +backend/app/core/api_controls.py +backend/app/api/deps/auth.py +backend/app/modules/tasks/router.py +backend/tests/test_api_controls.py +backend/tests/test_app.py +backend/tests/test_auth.py +backend/tests/test_tasks.py +backend/scripts/api_contract_e2e.py +docs/operations_authorization_service.md +.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/** +.agent-loop/LOOP_STATE.md +.agent-loop/WORK_QUEUE.md +.agent-loop/REVIEW_LOG.md +``` + +## Not allowed + +```text +database models, migrations, rate counters, or rate-control settings +new routes or rate-control dependency attachments +product permission, role, actor-state, or identity behavior +AUTH-04B, AUTH-05, or later chunk implementation +weakened tests, coverage thresholds, or OpenAPI constraints +BaseHTTPMiddleware, response-body buffering, or response JSON rewriting +``` + +## Request-context contract + +- Inspect raw ASGI header pairs so duplicate `X-Request-ID` or + `X-Correlation-ID` fields cannot be collapsed by framework parsing. +- Accept zero or one value for each ID. Reject repeated fields, including + identical duplicates, and reject comma-joined values. +- An accepted ID is exactly 36 ASCII characters and the canonical lowercase, + hyphenated, non-nil RFC 9562 UUID rendering with RFC variant and version 1 + through 8. +- Generate UUIDv4 for a missing request ID. A missing correlation ID reuses the + effective request ID. +- Invalid ID input short-circuits before any route or dependency runs and + returns HTTP 400 `invalid_request`. Generate one fresh UUIDv4 and use it as + both the safe request ID and safe correlation ID; supplied bytes are never + reflected. +- Store effective IDs only on the request's ASGI scope state. Concurrent + requests must not share context. +- Every response, including malformed-ID, 404, 405, validation, authentication, + unhandled 500, streaming, and background-task responses, returns canonical + `X-Request-ID` and `X-Correlation-ID` headers. Application-supplied ID response + headers are overwritten with the effective pair. +- Use pure ASGI middleware that edits only `http.response.start` headers. It + must preserve status, unrelated duplicate headers, `WWW-Authenticate`, + `Retry-After`, content length, body chunks, streaming, and background work. + +## Error contract + +- The canonical nested object has required fields + `error.code/message/details/correlation_id/retryable`. +- Add the object for explicit API errors, `HTTPException`, request validation, + coded task-domain JSON errors, auth missing/invalid/unavailable conditions, + 404, 405, 409, 422, future 429/503 errors, and unhandled 500 responses. +- Use the following exact branch mappings. Every unspecified `details` value is + `{}`. Compatibility `detail`, status, and headers remain as stated below. + +| Condition | HTTP | Code | Canonical message | Details | Retryable | +|---|---:|---|---|---|---| +| invalid request/correlation ID | 400 | `invalid_request` | `Invalid request identifier` | `{}` | false | +| request validation | 422 | `invalid_request` | `Request validation failed` | bounded validation summary | false | +| missing bearer token | 401 | `missing_token` | `Missing bearer token` | `{}` | false | +| invalid bearer token | 401 | `invalid_token` | `Invalid bearer token` | `{}` | false | +| verifier unavailable | 503 | `identity_verification_unavailable` | `Identity verification unavailable` | `{}` | true | +| unsupported verified subject kind | 403 | `unsupported_subject_kind` | `Unsupported subject kind` | `{}` | false | +| actor registry unavailable | 503 | `service_unavailable` | `Service unavailable` | `{}` | true | +| coded pre-submit failure | 422 | `pre_submission_checker_failed` | `Pre-submission checks failed` | exact legacy domain details | false | +| coded locked-context failure | 422 | `task_locked_context_invalid` | `Task locked context is invalid` | exact legacy domain details | false | +| other HTTP 400 | 400 | `invalid_request` | `Invalid request` | `{}` | false | +| other HTTP 401 | 401 | `invalid_token` | `Invalid bearer token` | `{}` | false | +| other HTTP 403 | 403 | `permission_not_granted` | `Permission not granted` | `{}` | false | +| other HTTP 404 | 404 | `resource_not_found` | `Resource not found` | `{}` | false | +| other HTTP 405 | 405 | `method_not_allowed` | `Method not allowed` | `{}` | false | +| other HTTP 409 | 409 | `conflict` | `Request conflict` | `{}` | false | +| other HTTP 422 | 422 | `invalid_request` | `Invalid request` | `{}` | false | +| future HTTP 429 | 429 | `rate_limit_exceeded` | `Rate limit exceeded` | `{}` | true | +| other HTTP 503 | 503 | `service_unavailable` | `Service unavailable` | `{}` | true | +| unhandled exception | 500 | `internal_error` | `Internal server error` | `{}` | false | + +- Actor-registry domain exceptions retain their existing status and top-level + `detail`; their nested code/message/retryable values use the matching generic + status row unless the exact unavailable branch above applies. +- Existing top-level `detail`, `code`, and `details` fields remain present with + their current types and values. Coded task responses mirror their top-level + `code/details` into the nested object. +- Preserve status codes and compatibility headers, including exact + `WWW-Authenticate` and future `Retry-After` values. +- Validation returns at most 20 errors. Each nested item contains only `type` + and `loc`: `type` is capped at 64 ASCII code characters; `loc` contains at + most eight integer or string components, with strings capped at 64 ASCII + characters and unsupported components replaced by `redacted`. The nested + details shape is `{"errors": [...], "truncated": }`. +- The compatibility `detail` remains the existing sanitized list in original + order for the first 20 errors. Preserve every currently emitted safe + field/value and validator message, including tested `loc`, constant-redacted + `input`, and class-name-only exception context. Only the 20-item list cap is + new; do not truncate locations/messages or remove fields from these retained + compatibility entries. Submitted input, exception values, and non-finite + numbers continue to use the existing redaction/JSON-safety helpers. The strict + eight-location/64-character/type-only privacy shape applies only to nested + `error.details`. +- Unhandled errors use a constant message, empty details, and + `retryable=false` in both debug and non-debug modes. No exception, SQL, + token, claim, provider response, secret, or PII enters a response or log. +- Shared explicit response builders and exception handlers own JSON shapes. + The request-context middleware must not parse or rewrite response bodies. + +## OpenAPI and compatibility contract + +- Document the required canonical error object and only the optional legacy + compatibility fields that routes can actually return. +- Document request/correlation request headers, response ID headers, + `WWW-Authenticate`, and future `Retry-After` behavior where applicable. +- Existing task coded-error schemas retain `additionalProperties: false`, their + required top-level fields, and add required canonical `error` through an + explicit compatible schema or `oneOf` composition. +- No route path, method, response status, or dependency consumer is added, + removed, or attached by this chunk. Tests inventory the route set and relevant + dependency consumers before and after the change. + +## Behavior proof + +- Missing, valid, malformed, non-ASCII, nil, uppercase, non-hyphenated, + unsupported-version, repeated-identical, repeated-different, and comma-joined + ID inputs. +- Request-only, correlation-only, both-ID, and concurrent-request isolation. +- Success, validation, 400, 401, 403, 404, 405, 409, 422, representative coded + task errors, 500, streaming, background task, and duplicate unrelated header + preservation. +- Exact legacy field/status/header preservation and bounded redaction. +- OpenAPI response-schema validation plus unchanged path/method and dependency + consumer inventories. +- Test-delta evidence proves existing test files only gain assertions/fixtures: + no assertion, `pytest.raises`, skip/xfail marker, or `skipTest` line is removed + or relaxed. New behavior runs through real FastAPI routes and ASGI transport, + not direct handler-only tests or dependency overrides that bypass middleware. + +## Verification commands + +```bash +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q tests/test_api_controls.py tests/test_app.py tests/test_auth.py tests/test_tasks.py) +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q --cov=app --cov-report=term-missing tests/test_api_controls.py tests/test_app.py tests/test_auth.py tests/test_tasks.py) +(cd backend && for file in app/core/api_controls.py app/main.py app/api/deps/auth.py app/modules/tasks/router.py; do .venv/bin/coverage report --include="$file" --precision=2 --fail-under=90; done) +(cd backend && WORKSTREAM_TEST_ADMIN_DATABASE_URL= .venv/bin/python -m pytest -q tests/test_isolated_database_runner.py) +(cd backend && .venv/bin/python scripts/run_isolated_tests.py --metadata-json --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py --cov=app --cov-report=term-missing --cov-fail-under=78) +(cd backend && .venv/bin/python scripts/run_isolated_tests.py --metadata-json --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py) +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python scripts/api_contract_e2e.py) +(cd backend && .venv/bin/python -m ruff check app tests scripts) +(cd backend && .venv/bin/docstr-coverage --config .docstr.yaml) +python3 scripts/check_loop_memory_state.py +python3 scripts/check_stale_workstream_wording.py +python3 scripts/check_stale_authorization_docs.py +python3 scripts/check_stale_artifact_contracts.py +python3 scripts/check_markdown_links.py +python3 scripts/check_internal_review_evidence.py +python3 scripts/test_agent_gates.py +git diff --unified=0 origin/main...HEAD -- backend/tests | (! rg '^-(.*assert|.*pytest\.raises|.*pytest\.mark\.(skip|xfail)|.*skipTest)') +git diff --check +``` + +## Required reviewers + +- senior engineering +- QA/test +- security/auth +- product/ops +- architecture +- CI integrity +- docs +- reuse/dedup +- test delta + +## Human review focus + +Review duplicate-ID rejection, ASGI response preservation, error privacy, +legacy compatibility, OpenAPI truth, and unchanged product authority. + +## Stop conditions + +- Stop if a response body must be parsed or buffered in middleware. +- Stop if an existing response field/header/assertion would need weakening. +- Stop if production changes exceed the size circuit breaker. +- Stop after the reviewed 04A PR; do not start 04B or AUTH-05 automatically. + +## Activation + +The user explicitly started parent AUTH-04 on 2026-07-13. Required plan review +split it before implementation. Only this repaired 04A contract is active; +runtime edits remain gated on its required preimplementation re-review. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md new file mode 100644 index 00000000..8b95deb0 --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-04B-postgres-rate-controls.md @@ -0,0 +1,151 @@ +# Chunk Contract: WS-AUTH-001-04B - PostgreSQL Rate Controls + +## Status + +Inactive. This contract requires AUTH-04A merge/memory, a separate explicit +user start, and required preimplementation review before any runtime edit. + +## Parent initiative + +`WS-AUTH-001` - Workstream Authorization Service + +## Goal + +Provide privacy-keyed, cross-replica PostgreSQL fixed-window rate controls for +future first-access and authority-management mutations without attaching them +to routes owned by later chunks. + +## Risk class and SLA + +L1 / P1 + +## Size circuit breaker + +- Stop at 350 changed non-comment production lines for a scope checkpoint. +- Stop and replan above 500 changed non-comment production lines. + +## Allowed files + +```text +backend/app/core/config.py +backend/app/api/deps/api_controls.py +backend/app/modules/api_controls/__init__.py +backend/app/modules/api_controls/models.py +backend/app/modules/api_controls/repository.py +backend/app/modules/api_controls/service.py +backend/app/db/models.py +backend/alembic/versions/0017_api_controls.py +backend/tests/test_api_rate_controls.py +backend/tests/test_config.py +backend/tests/test_alembic.py +backend/scripts/api_contract_e2e.py +docs/operations_authorization_service.md +.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/** +.agent-loop/LOOP_STATE.md +.agent-loop/WORK_QUEUE.md +.agent-loop/REVIEW_LOG.md +``` + +## Not allowed + +```text +request/error-context implementation owned by AUTH-04A +new routes or attachment to unrelated existing routes +first-access, grant, actor-state, or product authorization behavior +Redis/in-memory counters or persisted raw identity/network/token material +AUTH-05 or later chunk implementation +``` + +## Persistence and transaction contract + +- Persist one row per `(control_scope, key_digest)` with bounded scope, fixed + HMAC-SHA256 digest length, `window_started_at`, `window_expires_at`, + `request_count`, and `updated_at` using timezone-aware database timestamps. +- Enforce nonempty bounded scope, exact digest length, positive bounded count, + ordered window timestamps, uniqueness, and the lookup/expiry indexes required + by the executed query. +- One PostgreSQL upsert uses one `statement_timestamp()` value to reset an + expired window or atomically increment an active window and returns database + now, window start/end, and count. Saturate at the database integer maximum. +- Counts `1..limit` allow. Count `limit+1` and later deny and remain durably + consumed. `Retry-After` is the ceiling of remaining database-time seconds, + clamped to `1..window_seconds`. +- Invoke the upsert in a dedicated short-lived session/transaction and commit + before returning either allow or 429. A downstream request rollback cannot + undo rate consumption. + +## Privacy and configuration contract + +- Derive `key_digest` with HMAC-SHA256 using domain + `workstream-api-rate/v1`, the bounded control scope, and exact issuer plus + opaque subject bytes framed by four-byte big-endian lengths and UTF-8 values. +- Persist only scope and digest. Never persist or log raw issuer, subject, + actor ID, email, role, token, claims, or network address. +- Add `api_rate_limit_key_secret` with no default and minimum 32-byte secret + material. Never render or log it. Present malformed values fail settings + construction. +- Defaults are first-access `10/60s` and admin-mutation `30/60s`, with limit + bounds `1..10000` and window bounds `1..3600`. +- Missing secret remains startup-compatible while no route is attached, but + invoking a protected dependency without it returns a safe retryable 503. + Database unavailability also fails closed as safe retryable 503. +- Secret rotation intentionally resets effective counters. The runbook requires + quiescing protected writes for at most the largest configured window before + replacing the secret. + +## Route boundary and behavior proof + +- Export named first-access and admin-mutation dependencies for later owning + chunks, but attach neither dependency here. +- Inventory path/method and dependency consumers to prove no route or consumer + changed. +- Prove allow, exact limit, repeated exceed, expiry reset, distinct key/scope, + concurrent increments, cross-session visibility, downstream rollback + independence, missing/malformed secret, database unavailable, bounded + `Retry-After`, and privacy-safe persistence. + +## Migration proof + +- `0017_api_controls` revises `0016_artifact_domain`. +- Prove `0016 -> 0017 -> 0016 -> 0017`, seeded-row preservation across the + supported direction, unique/check/index enforcement, invalid insert + rejection, and the explicit nonempty-table downgrade policy. + +## Verification commands + +```bash +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q tests/test_api_rate_controls.py tests/test_config.py tests/test_alembic.py) +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python -m pytest -q --cov=app.modules.api_controls --cov=app.api.deps.api_controls --cov-report=term-missing --cov-fail-under=90 tests/test_api_rate_controls.py tests/test_config.py tests/test_alembic.py) +(cd backend && .venv/bin/python scripts/run_isolated_tests.py --metadata-json --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py --cov=app --cov-report=term-missing --cov-fail-under=78) +(cd backend && .venv/bin/python scripts/run_isolated_tests.py --metadata-json --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py) +(cd backend && WORKSTREAM_DATABASE_URL= .venv/bin/python scripts/api_contract_e2e.py) +(cd backend && .venv/bin/python -m ruff check app tests scripts) +python3 scripts/check_loop_memory_state.py +python3 scripts/check_stale_workstream_wording.py +python3 scripts/check_stale_authorization_docs.py +python3 scripts/check_stale_artifact_contracts.py +python3 scripts/check_markdown_links.py +python3 scripts/check_internal_review_evidence.py +python3 scripts/test_agent_gates.py +git diff --check +``` + +## Required reviewers + +- senior engineering +- QA/test +- security/auth +- product/ops +- architecture +- CI integrity +- docs +- reuse/dedup +- test delta + +## Stop conditions + +- Stop if a rate count can be rolled back with downstream work. +- Stop if atomic database-time semantics cannot be proven under concurrency. +- Stop if a raw identity or token-derived value would be persisted or logged. +- Stop if production changes exceed the size circuit breaker. +- Stop after 04B; do not start AUTH-05 automatically. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04A-internal-review-evidence.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04A-internal-review-evidence.md new file mode 100644 index 00000000..cc38c9b4 --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04A-internal-review-evidence.md @@ -0,0 +1,130 @@ +# Internal Review Evidence: WS-AUTH-001-04A + +## Chunk + +`WS-AUTH-001-04A` - Request And Error Context + +open sub-agent sessions: none + +valid findings addressed: yes + +## Reviewed Revision + +Reviewed code SHA: `4fd6db9ebe9911d30ec85657903b71707fc3bbfb` + +Reviewed production SHA: `cdcaf77f4a73d091afd54232f378f9a2831376c5` + +Reviewed at: 2026-07-13T19:34:02Z + +Reviewer run IDs: engineering-architecture=AUTH-04A-4FD6DB9-ENG-ARCH-20260713; +qa-ci-test-delta=AUTH-04A-4FD6DB9-QA-CI-20260713; +security-product-docs-reuse=AUTH-04A-4FD6DB9-SEC-PROD-20260713 + +The final revision differs from the reviewed production SHA only by additive +real-ASGI test changes: scalar validation context is exercised, and logging +tests establish and restore capture after in-process Alembic configuration +disables an existing logger. Final test-delta review confirmed identical +production blobs and retained the complete prior review results. + +## Reviewed Change + +- Added pure ASGI request context that validates or generates canonical request + and correlation UUIDs and overwrites only the two response ID headers. +- Added stable privacy-bounded nested error objects while retaining existing + top-level compatibility fields, status codes, and response headers. +- Added truthful per-route OpenAPI error and response-header contracts without + changing the route or dependency-consumer inventories. +- Added bounded correlation-only request failure logging before and after + response start. +- Added real-ASGI behavior, compatibility, privacy, concurrency, OpenAPI, + inventory, streaming, background-task, and API-drill proof. + +## Reviewer Results + +| Reviewer | Result | Blocking findings | Notes | +|---|---:|---|---| +| senior engineering | PASS | None | The bounded ASGI and error helpers are maintainable and preserve compatibility. | +| QA/test | PASS | None | The behavior matrix covers success, failure, privacy, headers, schemas, and inventory through real ASGI paths. | +| security/auth | PASS | None | Invalid identifiers, validation context, unhandled failures, and logs remain privacy bounded and fail closed. | +| product/ops | PASS | None | No role, grant, lifecycle, or product-authority behavior changed. | +| architecture | PASS | None | Pure ASGI middleware changes response-start headers only; rate controls and adapter migration remain separate. | +| CI integrity | PASS | None | No workflow, dependency, threshold, exclusion, or bypass changed. | +| docs | PASS | None | The authorization runbook and durable loop state describe the implemented boundary and inactive follow-up. | +| reuse/dedup | PASS | None | Shared builders centralize error and OpenAPI behavior without duplicating provider or factory boundaries. | +| test delta | PASS | None | No assertion, raises block, skip, xfail, or skipTest line was removed or weakened. | + +## Valid Findings Addressed + +- Replaced broad global OpenAPI error claims with per-route applicable statuses + and explicit response ID headers. +- Added bounded pre-response failure logging and proved post-response-start + failure behavior without reflecting exceptions or identity data. +- Completed real-ASGI proof for malformed headers, every stable error branch, + concurrent isolation, streaming, background tasks, and compatibility headers. +- Added exact route and protected-consumer inventory digests and additive schema + assertions for task domain errors. +- Reconciled lifecycle memory and recorded the 350-line scope checkpoint. +- Added the final scalar validation-context branch test; production code did not + change after the final production review. + +## Commands Run + +```bash +(cd backend && isolated-runner pytest -q tests/test_api_controls.py tests/test_app.py tests/test_auth.py tests/test_tasks.py) +(cd backend && isolated-runner pytest -q --cov=app tests/test_api_controls.py tests/test_app.py tests/test_auth.py tests/test_tasks.py) +(cd backend && for file in app/core/api_controls.py app/main.py app/api/deps/auth.py app/modules/tasks/router.py; do coverage report --include="$file" --precision=2 --fail-under=90; done) +(cd backend && pytest -q tests/test_isolated_database_runner.py) +(cd backend && isolated-runner pytest -q -x --ignore=tests/test_isolated_database_runner.py) +(cd backend && WORKSTREAM_DATABASE_URL= python scripts/api_contract_e2e.py) +(cd backend && ruff check app tests scripts) +(cd backend && docstr-coverage --config .docstr.yaml) +python3 scripts/check_loop_memory_state.py +python3 scripts/check_stale_workstream_wording.py +python3 scripts/check_stale_authorization_docs.py +python3 scripts/check_stale_artifact_contracts.py +python3 scripts/check_markdown_links.py +python3 scripts/test_agent_gates.py +git diff --unified=0 origin/main...HEAD -- backend/tests | (! rg '^-(.*assert|.*pytest\.raises|.*pytest\.mark\.(skip|xfail)|.*skipTest)') +git diff --check +``` + +Results: + +- focused real-ASGI behavior matrix: 235 passed; +- changed-file statement coverage: API controls 98.08 percent, application + factory 90.82 percent, auth dependencies 90.70 percent, and task router 92.36 + percent; +- isolated-runner lifecycle suite: 16 passed; +- API contract drill: passed; +- Ruff, docstring threshold, stale wording, authorization docs, artifact + contracts, Markdown links, loop memory, Agent Gates, additive test-delta, and + diff hygiene: passed; +- complete isolated backend regression run: exposed one order-dependent logging + capture failure after 114 passes; the failing Alembic-before-ASGI order and + both logging branches pass after the test-only repair. + +## Evidence Gate + +Evidence gate: PASS WITH EXTERNAL COMPLETE-SUITE PROOF PENDING + +The reviewed implementation stays inside the approved request/error contract +and below the 500 non-comment production-line hard stop. It adds no migration, +rate counter, grant, role, identity state, product permission, route, dependency +consumer, or later AUTH behavior. GitHub Backend must supply complete-suite +proof before merge; the exact locally failing order now passes. + +## Remaining Risks + +- GitHub Backend must complete the full regression suite, and Agent Gates must + repeat repository checks on the published branch before human merge. +- The repository-wide coverage improvement initiative remains paused at its + last official 79.249908 percent result; this AUTH chunk proves each changed + production file at or above 90 percent and does not claim a new whole-app + baseline. +- PostgreSQL rate controls and provider-neutral `IdentityIssuerVerifier` + adoption remain separate reviewed chunks with unmet prerequisites. + +## Stop Condition + +After ready PR publication, stop for complete GitHub checks and explicit human +review. Do not merge or start AUTH-04B or AUTH-05 automatically. diff --git a/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04A-pr-trust-bundle.md b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04A-pr-trust-bundle.md new file mode 100644 index 00000000..2f7f771a --- /dev/null +++ b/.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-04A-pr-trust-bundle.md @@ -0,0 +1,132 @@ +# PR Trust Bundle: WS-AUTH-001-04A + +## Chunk + +`WS-AUTH-001-04A` - Request And Error Context + +## Goal + +Give every API response canonical request and correlation identifiers and a +stable privacy-bounded error object without changing product authority or +breaking existing clients. + +## Human-Approved Intent + +The user explicitly started AUTH-04 after AUTH-03 memory merged through PR +#110. Required L1 review split request/error context from PostgreSQL rate +controls before implementation. This PR contains only the reviewed 04A child; +04B and later authorization chunks remain inactive. + +## What Changed + +- Added strict canonical request/correlation UUID handling in pure ASGI + middleware. +- Added stable nested error objects while preserving legacy response fields, + status codes, and headers. +- Added per-route OpenAPI error and response-header documentation. +- Added privacy-bounded correlation-only failure logs. +- Added real-ASGI behavior, inventory, schema, and API drill coverage. + +## Why It Changed + +Workstream needs traceable API failures and a dependable error contract before +authority enforcement and rate controls expand, but existing consumers cannot +be forced through a breaking response migration. + +## Design Chosen + +Pure ASGI middleware reads raw header pairs, stores effective IDs on request +scope state, and edits only `http.response.start` headers. Shared response +builders and exception handlers own JSON shapes. Existing compatibility fields +remain additive beside the canonical nested object. + +## Alternatives Rejected + +- `BaseHTTPMiddleware`, response buffering, or response-body rewriting. +- Collapsed framework header parsing that cannot detect duplicate fields. +- Replacing existing top-level error fields in one breaking migration. +- Global OpenAPI statuses that routes cannot actually emit. +- Adding PostgreSQL rate controls or provider-adapter migration to this chunk. + +## Scope Control + +The production change remains below the contract's 500 non-comment line hard +stop. No route, dependency consumer, database model, migration, setting, +permission, role, grant, actor state, rate counter, or product lifecycle changed. + +## Product Behavior + +Clients receive canonical `X-Request-ID` and `X-Correlation-ID` headers on +success and failure. Errors gain a required nested object with stable code, +message, bounded details, correlation ID, and retryability while retaining +existing compatible fields and headers. Product authorization is unchanged. + +## Acceptance Criteria Proof + +Real-ASGI tests cover missing, valid, malformed, duplicate, non-ASCII, nil, +uppercase, non-hyphenated, unsupported-version, and comma-joined identifiers; +concurrency; success and every required error class; validation privacy; +streaming; background tasks; duplicate unrelated headers; task compatibility; +OpenAPI schemas; and exact route/dependency inventories. + +## Tests And Checks + +- 235 focused behavior tests passed. +- Every changed production file exceeds 90 percent statement coverage: 98.08, + 90.82, 90.70, and 92.36 percent respectively. +- The isolated-runner lifecycle suite passed 16/16 and the live API contract + drill passed. +- Ruff, docstring threshold, stale scans, Markdown links, loop memory, Agent + Gates, additive test delta, and diff hygiene passed. +- A complete local run found one Alembic logging-state test interaction after + 114 passes; its exact order passes after repair. GitHub Backend must provide + the complete-suite rerun before merge. + +## Test Delta + +No assertion, `pytest.raises`, skip/xfail marker, or `skipTest` line was removed +or weakened. New proof exercises actual FastAPI routes through ASGI transport. + +## CI Integrity + +No workflow, package, dependency, threshold, exclusion, or bypass changed. +GitHub Backend and Agent Gates remain mandatory before merge. The separate +whole-app coverage initiative remains paused at its official 79.249908 percent +baseline; this chunk's changed-file threshold is at least 90 percent. + +## Reviewer Results + +Senior engineering, QA/test, security/auth, product/ops, architecture, CI +integrity, docs, reuse/dedup, and test-delta review pass. Production was reviewed +at `cdcaf77`; final reviewed candidate `4fd6db9` received exact-head confirmation +for both additive test repairs with identical production blobs and no weakened +tests. + +## External Review + +GitHub Backend, Agent Gates, CodeRabbit, and explicit human review begin after +the ready PR is published. None substitutes for the completed internal review. + +## Remaining Risks + +- Rate controls are intentionally absent and remain owned by inactive 04B. +- Provider-neutral `IdentityIssuerVerifier` adoption depends on the shared + external-service adapter foundation and a separate reviewed AUTH chunk. +- The canonical nested error is additive; compatibility fields remain until a + separately reviewed removal is safe. + +## Follow-Up Work + +After this PR merges and memory is updated, AUTH-04B still requires a separate +explicit user start. AUTH-05 and later cutover chunks remain proposed. + +## Human Review Focus + +Inspect duplicate-ID rejection, response-start-only header mutation, error and +log privacy, exact legacy compatibility, per-route OpenAPI truth, and unchanged +route/dependency inventories. + +## Human Merge Ownership + +Only the user may approve and merge this PR. Publication is not merge approval, +and no later AUTH chunk starts automatically. diff --git a/backend/app/api/deps/auth.py b/backend/app/api/deps/auth.py index 82c13f7b..1901b325 100644 --- a/backend/app/api/deps/auth.py +++ b/backend/app/api/deps/auth.py @@ -9,6 +9,7 @@ from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.ext.asyncio import AsyncSession +from app.core.api_controls import StructuredHTTPException from app.core.auth import get_auth_verifier from app.db.session import get_db_session from app.interfaces.auth import ( @@ -29,7 +30,7 @@ def get_application_auth_verifier(request: Request) -> AuthVerifier: return request.app.state.auth_verifier -def unauthorized(detail: str) -> HTTPException: +def unauthorized(detail: str, *, error_code: str) -> HTTPException: """Build a bearer-auth unauthorized response. Args: @@ -38,9 +39,11 @@ def unauthorized(detail: str) -> HTTPException: Returns: HTTP exception with the bearer challenge header. """ - return HTTPException( + return StructuredHTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail=detail, + error_code=error_code, + error_message=detail, headers={"WWW-Authenticate": "Bearer"}, ) @@ -62,17 +65,20 @@ async def get_auth_verification_result( HTTPException: If the bearer token is missing or invalid. """ if credentials is None: - raise unauthorized("Missing bearer token") + raise unauthorized("Missing bearer token", error_code="missing_token") try: return await verifier.verify(credentials.credentials) except AuthVerificationUnavailableError as exc: - raise HTTPException( + raise StructuredHTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Identity verification unavailable", + error_code="identity_verification_unavailable", + error_message="Identity verification unavailable", + retryable=True, ) from exc except AuthVerificationError as exc: - raise unauthorized("Invalid bearer token") from exc + raise unauthorized("Invalid bearer token", error_code="invalid_token") from exc async def get_current_actor( @@ -97,9 +103,11 @@ async def get_registered_actor( this token-derived context, not persisted profile rows. """ if result.token.subject_kind != "human" or result.legacy is None: - raise HTTPException( + raise StructuredHTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Unsupported subject kind", + error_code="unsupported_subject_kind", + error_message="Unsupported subject kind", ) actor = result.legacy_actor() try: @@ -109,8 +117,11 @@ async def get_registered_actor( raise HTTPException(status_code=exc.status_code, detail=str(exc)) from exc except SQLAlchemyError as exc: await session.rollback() - raise HTTPException( + raise StructuredHTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Actor registry unavailable", + error_code="service_unavailable", + error_message="Service unavailable", + retryable=True, ) from exc return actor diff --git a/backend/app/core/api_controls.py b/backend/app/core/api_controls.py new file mode 100644 index 00000000..35880c0b --- /dev/null +++ b/backend/app/core/api_controls.py @@ -0,0 +1,340 @@ +"""Request context and stable API error primitives.""" + +from __future__ import annotations + +from collections.abc import Mapping, Sequence +import logging +from typing import Any +from uuid import RFC_4122, UUID, uuid4 + +from fastapi import FastAPI, HTTPException, Request +from fastapi.responses import JSONResponse +from pydantic import BaseModel, ConfigDict +from starlette.types import ASGIApp, Message, Receive, Scope, Send + +REQUEST_ID_HEADER = b"x-request-id" +CORRELATION_ID_HEADER = b"x-correlation-id" +REQUEST_ID_STATE_KEY = "request_id" +CORRELATION_ID_STATE_KEY = "correlation_id" +LOGGER = logging.getLogger(__name__) + + +class ApiError(BaseModel): + """Canonical machine-readable API error.""" + + model_config = ConfigDict(extra="forbid") + + code: str + message: str + details: dict[str, Any] + correlation_id: str + retryable: bool + + +class ApiErrorResponse(BaseModel): + """Canonical error plus additive legacy compatibility fields.""" + + model_config = ConfigDict(extra="forbid") + + error: ApiError + detail: Any | None = None + + +class StructuredHTTPException(HTTPException): + """HTTP exception carrying an explicit canonical error classification.""" + + def __init__( + self, + *, + status_code: int, + detail: Any, + error_code: str, + error_message: str, + retryable: bool = False, + headers: Mapping[str, str] | None = None, + ) -> None: + super().__init__(status_code=status_code, detail=detail, headers=headers) + self.error_code = error_code + self.error_message = error_message + self.retryable = retryable + + +def request_ids(request: Request) -> tuple[str, str]: + """Return the request-scoped canonical request and correlation IDs.""" + state = request.scope.setdefault("state", {}) + request_id = state.get(REQUEST_ID_STATE_KEY) + correlation_id = state.get(CORRELATION_ID_STATE_KEY) + if not isinstance(request_id, str) or not isinstance(correlation_id, str): + fallback = str(uuid4()) + state[REQUEST_ID_STATE_KEY] = fallback + state[CORRELATION_ID_STATE_KEY] = fallback + return fallback, fallback + return request_id, correlation_id + + +def error_payload( + request: Request, + *, + code: str, + message: str, + details: dict[str, Any] | None = None, + retryable: bool = False, + compatibility: Mapping[str, Any] | None = None, +) -> dict[str, Any]: + """Build one canonical error body with optional legacy fields.""" + _, correlation_id = request_ids(request) + payload: dict[str, Any] = dict(compatibility or {}) + payload["error"] = { + "code": code, + "message": message, + "details": details or {}, + "correlation_id": correlation_id, + "retryable": retryable, + } + return payload + + +def error_response( + request: Request, + *, + status_code: int, + code: str, + message: str, + details: dict[str, Any] | None = None, + retryable: bool = False, + compatibility: Mapping[str, Any] | None = None, + headers: Mapping[str, str] | None = None, +) -> JSONResponse: + """Return a structured JSON error with canonical response IDs.""" + request_id, correlation_id = request_ids(request) + response_headers = dict(headers or {}) + response_headers["X-Request-ID"] = request_id + response_headers["X-Correlation-ID"] = correlation_id + return JSONResponse( + status_code=status_code, + content=error_payload( + request, + code=code, + message=message, + details=details, + retryable=retryable, + compatibility=compatibility, + ), + headers=response_headers, + ) + + +def _header_value(scope: Scope, name: bytes) -> bytes | None: + values = [value for key, value in scope.get("headers", ()) if key.lower() == name] + if len(values) > 1: + raise ValueError("duplicate request context header") + if not values: + return None + if b"," in values[0]: + raise ValueError("combined request context header") + return values[0] + + +def _canonical_uuid(value: bytes) -> str: + if len(value) != 36: + raise ValueError("invalid request context header") + try: + text = value.decode("ascii") + parsed = UUID(text) + except (UnicodeDecodeError, ValueError) as exc: + raise ValueError("invalid request context header") from exc + if ( + parsed.int == 0 + or parsed.variant != RFC_4122 + or parsed.version not in range(1, 9) + or str(parsed) != text + ): + raise ValueError("invalid request context header") + return text + + +def _resolve_request_ids(scope: Scope) -> tuple[str, str]: + raw_request_id = _header_value(scope, REQUEST_ID_HEADER) + raw_correlation_id = _header_value(scope, CORRELATION_ID_HEADER) + request_id = str(uuid4()) if raw_request_id is None else _canonical_uuid(raw_request_id) + correlation_id = ( + request_id if raw_correlation_id is None else _canonical_uuid(raw_correlation_id) + ) + return request_id, correlation_id + + +def _response_headers( + headers: Sequence[tuple[bytes, bytes]], request_id: str, correlation_id: str +) -> list[tuple[bytes, bytes]]: + retained = [ + (name, value) + for name, value in headers + if name.lower() not in {REQUEST_ID_HEADER, CORRELATION_ID_HEADER} + ] + retained.extend( + [ + (REQUEST_ID_HEADER, request_id.encode("ascii")), + (CORRELATION_ID_HEADER, correlation_id.encode("ascii")), + ] + ) + return retained + + +class RequestContextMiddleware: + """Validate request context and append canonical IDs without buffering.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + state = scope.setdefault("state", {}) + try: + request_id, correlation_id = _resolve_request_ids(scope) + except ValueError: + request_id = correlation_id = str(uuid4()) + state[REQUEST_ID_STATE_KEY] = request_id + state[CORRELATION_ID_STATE_KEY] = correlation_id + response = error_response( + Request(scope, receive), + status_code=400, + code="invalid_request", + message="Invalid request identifier", + compatibility={"detail": "Invalid request identifier"}, + ) + await response(scope, receive, send) + return + + state[REQUEST_ID_STATE_KEY] = request_id + state[CORRELATION_ID_STATE_KEY] = correlation_id + response_started = False + + async def send_with_context(message: Message) -> None: + nonlocal response_started + if message["type"] == "http.response.start": + response_started = True + message["headers"] = _response_headers( + message.get("headers", ()), request_id, correlation_id + ) + await send(message) + + try: + await self.app(scope, receive, send_with_context) + except Exception: + if response_started: + LOGGER.error( + "request_failed_after_response_start", + extra={"correlation_id": correlation_id}, + ) + return + LOGGER.error( + "request_failed_before_response_start", + extra={"correlation_id": correlation_id}, + ) + response = error_response( + Request(scope, receive), + status_code=500, + code="internal_error", + message="Internal server error", + compatibility={"detail": "Internal server error"}, + ) + await response(scope, receive, send) + + +def install_api_control_openapi(app: FastAPI) -> None: + """Document request context headers across the existing route inventory.""" + original_openapi = app.openapi + + def openapi() -> dict[str, Any]: + if app.openapi_schema is not None: + return app.openapi_schema + schema = original_openapi() + schemas = schema.setdefault("components", {}).setdefault("schemas", {}) + schemas["HTTPValidationError"] = { + "type": "object", + "required": ["detail", "error"], + "properties": { + "detail": {"type": "array", "items": {"type": "object"}}, + "error": {"$ref": "#/components/schemas/ApiError"}, + }, + "additionalProperties": False, + } + request_parameters = tuple( + { + "name": header, + "in": "header", + "required": False, + "schema": { + "type": "string", + "format": "uuid", + "minLength": 36, + "maxLength": 36, + }, + } + for header in ("X-Request-ID", "X-Correlation-ID") + ) + response_headers = { + "X-Request-ID": { + "required": True, + "schema": {"type": "string", "format": "uuid"}, + }, + "X-Correlation-ID": { + "required": True, + "schema": {"type": "string", "format": "uuid"}, + }, + } + methods = {"get", "put", "post", "delete", "options", "head", "patch", "trace"} + for path, path_item in schema.get("paths", {}).items(): + for method, operation in path_item.items(): + if method not in methods: + continue + parameters = operation.setdefault("parameters", []) + present = { + (parameter.get("in"), parameter.get("name", "").lower()) + for parameter in parameters + if isinstance(parameter, dict) + } + for parameter in request_parameters: + identity = (parameter["in"], parameter["name"].lower()) + if identity not in present: + parameters.append(parameter) + responses = operation.get("responses", {}) + if path not in {"/health", "/api/v1/health"}: + applicable_errors = { + "401": "Authentication failed.", + "403": "Permission denied.", + "503": "Identity verification unavailable.", + } + if "{" in path: + applicable_errors["404"] = "Resource not found." + if "{" in path and method in {"post", "put", "patch", "delete"}: + applicable_errors["409"] = "Request conflict." + for status, description in applicable_errors.items(): + headers = dict(response_headers) + if status == "401": + headers["WWW-Authenticate"] = {"schema": {"type": "string"}} + responses.setdefault( + status, + { + "description": description, + "headers": headers, + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ApiErrorResponse" + } + } + }, + }, + ) + for response in responses.values(): + headers = response.setdefault("headers", {}) + for name, header_schema in response_headers.items(): + headers.setdefault(name, header_schema) + app.openapi_schema = schema + return schema + + app.openapi = openapi diff --git a/backend/app/main.py b/backend/app/main.py index 15adb0dc..a95c4f82 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -11,12 +11,44 @@ from fastapi.encoders import jsonable_encoder from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse +from starlette.exceptions import HTTPException as StarletteHTTPException from app.api.router import api_router +from app.core.api_controls import ( + ApiErrorResponse, + RequestContextMiddleware, + StructuredHTTPException, + error_response, + install_api_control_openapi, +) from app.core.auth import build_auth_verifier, cache_auth_verifier, prepare_auth_verifier from app.core.config import Settings, get_settings PRODUCTION_LIKE_ENVIRONMENTS = {"staging", "preview", "prod", "production"} +MAX_VALIDATION_ERRORS = 20 +MAX_VALIDATION_LOCATION_PARTS = 8 +MAX_VALIDATION_CODE_LENGTH = 64 +ERROR_RESPONSE_HEADERS = { + "X-Request-ID": { + "required": True, + "schema": {"type": "string", "format": "uuid"}, + }, + "X-Correlation-ID": { + "required": True, + "schema": {"type": "string", "format": "uuid"}, + }, +} +DEFAULT_ERROR_RESPONSES = { + status_code: { + "model": ApiErrorResponse, + "description": description, + "headers": dict(ERROR_RESPONSE_HEADERS), + } + for status_code, description in { + 400: "Invalid request.", + 500: "Internal server error.", + }.items() +} @asynccontextmanager @@ -73,18 +105,86 @@ def _json_safe_validation_value(value: Any) -> Any: return value +def _validation_summary(error: dict[str, Any]) -> dict[str, Any]: + """Return bounded type/location evidence for the canonical error object.""" + raw_type = error.get("type") + error_type = raw_type if isinstance(raw_type, str) and raw_type.isascii() else "validation_error" + error_type = error_type[:MAX_VALIDATION_CODE_LENGTH] + location: list[int | str] = [] + raw_location = error.get("loc") + if isinstance(raw_location, (list, tuple)): + for part in raw_location[:MAX_VALIDATION_LOCATION_PARTS]: + if isinstance(part, int): + location.append(part) + elif isinstance(part, str) and part.isascii(): + location.append(part[:MAX_VALIDATION_CODE_LENGTH]) + else: + location.append("redacted") + return {"type": error_type, "loc": location} + + async def request_validation_exception_handler( - _request: Request, + request: Request, exc: RequestValidationError, ) -> JSONResponse: """Return validation errors without echoing non-finite JSON values.""" - return JSONResponse( + errors = exc.errors() + compatibility_detail = [ + _validation_error_detail(error) for error in errors[:MAX_VALIDATION_ERRORS] + ] + return error_response( + request, status_code=422, - content=jsonable_encoder( - { - "detail": [_validation_error_detail(error) for error in exc.errors()], - } - ), + code="invalid_request", + message="Request validation failed", + details={ + "errors": [_validation_summary(error) for error in errors[:MAX_VALIDATION_ERRORS]], + "truncated": len(errors) > MAX_VALIDATION_ERRORS, + }, + compatibility={"detail": jsonable_encoder(compatibility_detail)}, + ) + + +def _generic_http_error(status_code: int) -> tuple[str, str, bool]: + """Return the canonical fallback classification for an HTTP status.""" + return { + 400: ("invalid_request", "Invalid request", False), + 401: ("invalid_token", "Invalid bearer token", False), + 403: ("permission_not_granted", "Permission not granted", False), + 404: ("resource_not_found", "Resource not found", False), + 405: ("method_not_allowed", "Method not allowed", False), + 409: ("conflict", "Request conflict", False), + 422: ("invalid_request", "Invalid request", False), + 429: ("rate_limit_exceeded", "Rate limit exceeded", True), + 503: ("service_unavailable", "Service unavailable", True), + }.get(status_code, ("http_error", "Request failed", False)) + + +async def http_exception_handler(request: Request, exc: StarletteHTTPException) -> JSONResponse: + """Add the canonical envelope while retaining FastAPI compatibility fields.""" + if isinstance(exc, StructuredHTTPException): + code, message, retryable = exc.error_code, exc.error_message, exc.retryable + else: + code, message, retryable = _generic_http_error(exc.status_code) + return error_response( + request, + status_code=exc.status_code, + code=code, + message=message, + retryable=retryable, + compatibility={"detail": exc.detail}, + headers=exc.headers, + ) + + +async def unhandled_exception_handler(request: Request, _exc: Exception) -> JSONResponse: + """Return a constant private response for an unhandled application error.""" + return error_response( + request, + status_code=500, + code="internal_error", + message="Internal server error", + compatibility={"detail": "Internal server error"}, ) @@ -103,6 +203,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: version=settings.app_version, debug=settings.debug, lifespan=_application_lifespan, + responses=DEFAULT_ERROR_RESPONSES, ) app.state.settings = settings app.state.auth_verifier, app.state.auth_configuration_valid = prepare_auth_verifier(settings) @@ -111,7 +212,11 @@ def create_app(settings: Settings | None = None) -> FastAPI: RequestValidationError, request_validation_exception_handler, ) + app.add_exception_handler(StarletteHTTPException, http_exception_handler) + app.add_exception_handler(Exception, unhandled_exception_handler) + app.add_middleware(RequestContextMiddleware) app.include_router(api_router) + install_api_control_openapi(app) return app diff --git a/backend/app/modules/tasks/router.py b/backend/app/modules/tasks/router.py index 03adbab1..f157c4c1 100644 --- a/backend/app/modules/tasks/router.py +++ b/backend/app/modules/tasks/router.py @@ -4,11 +4,12 @@ from typing import Annotated -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends, HTTPException, Request from fastapi.responses import JSONResponse from sqlalchemy.ext.asyncio import AsyncSession from app.api.deps.auth import get_registered_actor +from app.core.api_controls import error_response from app.core.permissions import PermissionDenied from app.db.session import get_db_session from app.modules.actors.schemas import ActorProfileActivationRequest, ActorProfileResponse @@ -30,17 +31,19 @@ router = APIRouter(tags=["tasks"]) +CANONICAL_ERROR_OBJECT_SCHEMA = {"$ref": "#/components/schemas/ApiError"} PRE_SUBMIT_DOMAIN_ERROR_RESPONSE_SCHEMA = { "oneOf": [ { "type": "object", - "required": ["code", "details"], + "required": ["code", "details", "error"], "properties": { "code": { "type": "string", "enum": ["pre_submission_checker_failed"], }, "details": {"type": "object"}, + "error": CANONICAL_ERROR_OBJECT_SCHEMA, }, "additionalProperties": False, }, @@ -51,13 +54,14 @@ "oneOf": [ { "type": "object", - "required": ["code", "details"], + "required": ["code", "details", "error"], "properties": { "code": { "type": "string", "enum": ["task_locked_context_invalid"], }, "details": {"type": "object"}, + "error": CANONICAL_ERROR_OBJECT_SCHEMA, }, "additionalProperties": False, }, @@ -78,14 +82,21 @@ def task_http_error(exc: TaskServiceError) -> HTTPException: return HTTPException(status_code=exc.status_code, detail=str(exc)) -def task_domain_error_response(exc: TaskServiceError) -> JSONResponse: +def task_domain_error_response(request: Request, exc: TaskServiceError) -> JSONResponse: """Convert a coded domain error into the public API error body.""" - return JSONResponse( + code = getattr(exc, "code") + details = getattr(exc, "details", None) or {} + message = { + "pre_submission_checker_failed": "Pre-submission checks failed", + "task_locked_context_invalid": "Task locked context is invalid", + }[code] + return error_response( + request, status_code=exc.status_code, - content={ - "code": getattr(exc, "code"), - "details": getattr(exc, "details", None) or {}, - }, + code=code, + message=message, + details=details, + compatibility={"code": code, "details": details}, ) @@ -180,6 +191,7 @@ async def get_task( }, ) async def get_task_work_context( + request: Request, task_id: str, actor: Annotated[ActorContext, Depends(get_registered_actor)], session: Annotated[AsyncSession, Depends(get_db_session)], @@ -191,7 +203,7 @@ async def get_task_work_context( raise permission_http_error(exc) from exc except TaskServiceError as exc: if getattr(exc, "code", None) is not None: - return task_domain_error_response(exc) + return task_domain_error_response(request, exc) raise task_http_error(exc) from exc @@ -211,6 +223,7 @@ async def get_task_work_context( }, ) async def get_task_submission_requirements( + request: Request, task_id: str, actor: Annotated[ActorContext, Depends(get_registered_actor)], session: Annotated[AsyncSession, Depends(get_db_session)], @@ -222,7 +235,7 @@ async def get_task_submission_requirements( raise permission_http_error(exc) from exc except TaskServiceError as exc: if getattr(exc, "code", None) is not None: - return task_domain_error_response(exc) + return task_domain_error_response(request, exc) raise task_http_error(exc) from exc @@ -242,6 +255,7 @@ async def get_task_submission_requirements( }, ) async def get_task_locked_context( + request: Request, task_id: str, actor: Annotated[ActorContext, Depends(get_registered_actor)], session: Annotated[AsyncSession, Depends(get_db_session)], @@ -253,7 +267,7 @@ async def get_task_locked_context( raise permission_http_error(exc) from exc except TaskServiceError as exc: if getattr(exc, "code", None) is not None: - return task_domain_error_response(exc) + return task_domain_error_response(request, exc) raise task_http_error(exc) from exc @@ -370,6 +384,7 @@ async def start_task( }, ) async def create_submission( + request: Request, task_id: str, payload: SubmissionCreate, actor: Annotated[ActorContext, Depends(get_registered_actor)], @@ -382,7 +397,7 @@ async def create_submission( raise permission_http_error(exc) from exc except TaskServiceError as exc: if getattr(exc, "code", None) is not None: - return task_domain_error_response(exc) + return task_domain_error_response(request, exc) raise task_http_error(exc) from exc diff --git a/backend/scripts/api_contract_e2e.py b/backend/scripts/api_contract_e2e.py index 5e95ea40..87919274 100644 --- a/backend/scripts/api_contract_e2e.py +++ b/backend/scripts/api_contract_e2e.py @@ -300,10 +300,16 @@ async def request_json( Raises: AssertionError: If the response status does not match. """ + request_id = str(uuid4()) + correlation_id = str(uuid4()) + headers = {} if token is None else auth_headers(token) + headers.update( + {"X-Request-ID": request_id, "X-Correlation-ID": correlation_id} + ) response = await client.request( method, path, - headers={} if token is None else auth_headers(token), + headers=headers, json=payload, ) if response.status_code != expected_status: @@ -322,6 +328,15 @@ async def request_json( ) from exc if not isinstance(body, dict | list): raise AssertionError(f"{method} {path} returned non-JSON payload: {body!r}") + if response.headers.get("x-request-id") != request_id: + raise AssertionError(f"{method} {path} did not preserve the request ID") + if response.headers.get("x-correlation-id") != correlation_id: + raise AssertionError(f"{method} {path} did not preserve the correlation ID") + if expected_status >= 400: + if not isinstance(body, dict) or body.get("error", {}).get( + "correlation_id" + ) != correlation_id: + raise AssertionError(f"{method} {path} returned invalid error context") print(f"PASS {method} {path} -> {response.status_code}") return body diff --git a/backend/tests/test_api_controls.py b/backend/tests/test_api_controls.py new file mode 100644 index 00000000..f80697dd --- /dev/null +++ b/backend/tests/test_api_controls.py @@ -0,0 +1,476 @@ +from __future__ import annotations + +import asyncio +from hashlib import sha256 +import logging +from typing import Annotated +from uuid import UUID, uuid4 + +from fastapi import BackgroundTasks, HTTPException +from fastapi.exceptions import RequestValidationError +from fastapi.responses import Response, StreamingResponse +from httpx import ASGITransport, AsyncClient +from pydantic import BaseModel, Field, field_validator +import pytest +from starlette.requests import Request + +from app.core.api_controls import ApiErrorResponse, error_response +from app.core.config import Settings +from app.main import create_app + + +def _assert_uuid(value: str) -> None: + parsed = UUID(value) + assert str(parsed) == value + assert parsed.int != 0 + assert parsed.version in range(1, 9) + + +def test_error_response_fallback_ids_remain_consistent() -> None: + request = Request({"type": "http", "method": "GET", "path": "/", "headers": []}) + + response = error_response( + request, + status_code=500, + code="internal_error", + message="Internal server error", + ) + + correlation_id = response.headers["x-correlation-id"] + assert response.headers["x-request-id"] == correlation_id + assert f'"correlation_id":"{correlation_id}"'.encode() in response.body + + +async def _get(app, path: str = "/health", **kwargs): + async with AsyncClient( + transport=ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://testserver", + ) as client: + return await client.get(path, **kwargs) + + +async def test_missing_and_supplied_request_context_propagates() -> None: + app = create_app() + generated = await _get(app) + request_id = str(uuid4()) + correlation_id = str(uuid4()) + supplied = await _get( + app, + headers={"X-Request-ID": request_id, "X-Correlation-ID": correlation_id}, + ) + request_only = await _get(app, headers={"X-Request-ID": request_id}) + correlation_only = await _get(app, headers={"X-Correlation-ID": correlation_id}) + + _assert_uuid(generated.headers["x-request-id"]) + assert generated.headers["x-correlation-id"] == generated.headers["x-request-id"] + assert supplied.headers["x-request-id"] == request_id + assert supplied.headers["x-correlation-id"] == correlation_id + assert request_only.headers["x-request-id"] == request_id + assert request_only.headers["x-correlation-id"] == request_id + _assert_uuid(correlation_only.headers["x-request-id"]) + assert correlation_only.headers["x-correlation-id"] == correlation_id + + +@pytest.mark.parametrize( + "invalid_id", + [ + str(uuid4()).upper(), + str(uuid4()).replace("-", ""), + "00000000-0000-0000-0000-000000000000", + "00000000-0000-9000-8000-000000000001", + f"{uuid4()}, {uuid4()}", + ], +) +async def test_invalid_request_context_short_circuits_without_reflection( + invalid_id: str, +) -> None: + app = create_app() + called = False + + @app.get("/_test/not-called") + async def not_called() -> dict[str, bool]: + nonlocal called + called = True + return {"called": True} + + response = await _get(app, "/_test/not-called", headers={"X-Request-ID": invalid_id}) + + assert response.status_code == 400 + assert called is False + assert response.headers["x-request-id"] == response.headers["x-correlation-id"] + _assert_uuid(response.headers["x-request-id"]) + assert invalid_id not in response.text + assert response.json() == { + "detail": "Invalid request identifier", + "error": { + "code": "invalid_request", + "message": "Invalid request identifier", + "details": {}, + "correlation_id": response.headers["x-correlation-id"], + "retryable": False, + }, + } + + +@pytest.mark.parametrize( + "headers", + [ + [(b"x-request-id", str(uuid4()).encode())] * 2, + [ + (b"x-request-id", str(uuid4()).encode()), + (b"X-Request-ID", str(uuid4()).encode()), + ], + [(b"x-correlation-id", str(uuid4()).encode())] * 2, + [(b"x-request-id", b"\xff" * 36)], + ], +) +async def test_duplicate_and_non_ascii_request_context_is_rejected( + headers: list[tuple[bytes, bytes]], +) -> None: + response = await _get(create_app(), headers=headers) + + assert response.status_code == 400 + assert response.headers["x-request-id"] == response.headers["x-correlation-id"] + assert response.json()["error"]["code"] == "invalid_request" + + +async def test_concurrent_request_context_remains_isolated() -> None: + app = create_app() + identifiers = [(str(uuid4()), str(uuid4())) for _ in range(12)] + + async def call(request_id: str, correlation_id: str): + return await _get( + app, + headers={"X-Request-ID": request_id, "X-Correlation-ID": correlation_id}, + ) + + responses = await asyncio.gather(*(call(*pair) for pair in identifiers)) + + assert [ + (response.headers["x-request-id"], response.headers["x-correlation-id"]) + for response in responses + ] == identifiers + + +async def test_response_context_overwrites_ids_and_preserves_headers() -> None: + app = create_app() + request_id = str(uuid4()) + correlation_id = str(uuid4()) + + @app.get("/_test/headers") + async def headers() -> Response: + response = Response( + "ok", + headers={ + "X-Request-ID": str(uuid4()), + "X-Correlation-ID": str(uuid4()), + "Retry-After": "7", + }, + ) + response.raw_headers.extend( + [(b"set-cookie", b"first=1"), (b"set-cookie", b"second=2")] + ) + return response + + response = await _get( + app, + "/_test/headers", + headers={"X-Request-ID": request_id, "X-Correlation-ID": correlation_id}, + ) + + assert response.status_code == 200 + assert response.content == b"ok" + assert response.headers["content-length"] == "2" + assert response.headers["retry-after"] == "7" + assert response.headers.get_list("set-cookie") == ["first=1", "second=2"] + assert response.headers["x-request-id"] == request_id + assert response.headers["x-correlation-id"] == correlation_id + + +async def test_streaming_and_background_responses_keep_context() -> None: + app = create_app() + completed: list[str] = [] + + @app.get("/_test/stream") + async def stream(background_tasks: BackgroundTasks) -> StreamingResponse: + async def content(): + yield b"first" + yield b"second" + + background_tasks.add_task(completed.append, "done") + return StreamingResponse(content(), background=background_tasks) + + response = await _get(app, "/_test/stream") + + assert response.status_code == 200 + assert response.content == b"firstsecond" + _assert_uuid(response.headers["x-request-id"]) + assert response.headers["x-correlation-id"] == response.headers["x-request-id"] + assert completed == ["done"] + + +async def test_background_failure_is_bounded_after_response_start( + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + logger = logging.getLogger("app.core.api_controls") + # In-process Alembic tests disable existing loggers through fileConfig. + monkeypatch.setattr(logger, "disabled", False) + caplog.set_level(logging.ERROR, logger=logger.name) + app = create_app() + + @app.get("/_test/background-failure") + async def background_failure(background_tasks: BackgroundTasks) -> dict[str, bool]: + def fail() -> None: + raise RuntimeError("token=secret-background-value") + + background_tasks.add_task(fail) + return {"accepted": True} + + response = await _get(app, "/_test/background-failure") + + assert response.status_code == 200 + assert response.json() == {"accepted": True} + assert response.headers["x-request-id"] + assert "request_failed_after_response_start" in caplog.text + assert "secret-background-value" not in caplog.text + + +@pytest.mark.parametrize( + ("status_code", "code", "message", "retryable"), + [ + (400, "invalid_request", "Invalid request", False), + (401, "invalid_token", "Invalid bearer token", False), + (403, "permission_not_granted", "Permission not granted", False), + (409, "conflict", "Request conflict", False), + (422, "invalid_request", "Invalid request", False), + (429, "rate_limit_exceeded", "Rate limit exceeded", True), + (503, "service_unavailable", "Service unavailable", True), + ], +) +async def test_http_errors_use_stable_mapping_and_preserve_legacy_detail( + status_code: int, code: str, message: str, retryable: bool +) -> None: + app = create_app() + + async def fail() -> None: + headers = {"Retry-After": "9"} if status_code == 429 else None + raise HTTPException(status_code=status_code, detail="legacy detail", headers=headers) + + app.add_api_route(f"/_test/error/{status_code}", fail, methods=["GET"]) + response = await _get(app, f"/_test/error/{status_code}") + + assert response.status_code == status_code + assert response.json()["detail"] == "legacy detail" + assert response.json()["error"] == { + "code": code, + "message": message, + "details": {}, + "correlation_id": response.headers["x-correlation-id"], + "retryable": retryable, + } + ApiErrorResponse.model_validate(response.json()) + if status_code == 429: + assert response.headers["retry-after"] == "9" + + +@pytest.mark.parametrize("debug", [False, True]) +async def test_not_found_method_not_allowed_and_unhandled_errors_are_private( + debug: bool, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, +) -> None: + logger = logging.getLogger("app.core.api_controls") + monkeypatch.setattr(logger, "disabled", False) + caplog.set_level(logging.ERROR, logger=logger.name) + app = create_app(Settings(debug=debug)) + + @app.get("/_test/boom") + async def boom() -> None: + raise RuntimeError("token=secret database failure") + + not_found = await _get(app, "/_test/missing") + async with AsyncClient( + transport=ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://testserver", + ) as client: + method_not_allowed = await client.post("/health") + failed = await _get(app, "/_test/boom") + + for response, status_code, detail, code, message in [ + (not_found, 404, "Not Found", "resource_not_found", "Resource not found"), + ( + method_not_allowed, + 405, + "Method Not Allowed", + "method_not_allowed", + "Method not allowed", + ), + ( + failed, + 500, + "Internal server error", + "internal_error", + "Internal server error", + ), + ]: + assert response.status_code == status_code + assert response.json() == { + "detail": detail, + "error": { + "code": code, + "message": message, + "details": {}, + "correlation_id": response.headers["x-correlation-id"], + "retryable": False, + }, + } + ApiErrorResponse.model_validate(response.json()) + + failure_record = next( + record + for record in caplog.records + if record.message == "request_failed_before_response_start" + ) + assert failure_record.correlation_id == failed.headers["x-correlation-id"] + assert "secret" not in failed.text + assert "secret" not in caplog.text + + +class ManyInvalidValues(BaseModel): + """Request fixture that can produce more errors than the response cap.""" + + values: list[Annotated[int, Field(gt=0)]] + + +class SafeValidatorMessage(BaseModel): + """Request fixture for legacy sanitized validator compatibility.""" + + value: str + + @field_validator("value") + @classmethod + def reject_value(cls, _value: str) -> str: + raise ValueError("safe compatibility message") + + +async def test_validation_summary_is_bounded_and_legacy_messages_remain() -> None: + app = create_app() + + @app.post("/_test/many-values") + async def many_values(_payload: ManyInvalidValues) -> None: + return None + + @app.post("/_test/safe-message") + async def safe_message(_payload: SafeValidatorMessage) -> None: + return None + + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://testserver" + ) as client: + many = await client.post("/_test/many-values", json={"values": [-1] * 25}) + safe = await client.post("/_test/safe-message", json={"value": "rejected"}) + + assert many.status_code == 422 + assert len(many.json()["detail"]) == 20 + assert len(many.json()["error"]["details"]["errors"]) == 20 + assert many.json()["error"]["details"]["truncated"] is True + assert set(many.json()["error"]["details"]["errors"][0]) == {"type", "loc"} + assert safe.status_code == 422 + assert "safe compatibility message" in safe.text + assert "safe compatibility message" not in str(safe.json()["error"]["details"]) + assert safe.json()["detail"][0]["input"] == "redacted" + + +async def test_validation_summary_redacts_unsupported_location_parts() -> None: + app = create_app() + + @app.get("/_test/unsupported-location") + async def unsupported_location() -> None: + raise RequestValidationError( + [ + { + "type": "value_error", + "loc": ("query", None), + "input": "secret", + "ctx": "bounded context", + } + ] + ) + + response = await _get(app, "/_test/unsupported-location") + + assert response.status_code == 422 + assert response.json()["detail"][0]["loc"] == ["query", None] + assert response.json()["detail"][0]["ctx"] == "bounded context" + assert response.json()["error"]["details"]["errors"][0]["loc"] == [ + "query", + "redacted", + ] + + +def test_openapi_documents_request_error_and_response_context() -> None: + schema = create_app().openapi() + error_schema = schema["components"]["schemas"]["ApiError"] + validation_schema = schema["components"]["schemas"]["HTTPValidationError"] + + assert set(error_schema["required"]) == { + "code", + "message", + "details", + "correlation_id", + "retryable", + } + assert "error" in validation_schema["required"] + response_schema = schema["components"]["schemas"]["ApiErrorResponse"] + assert response_schema["required"] == ["error"] + assert set(response_schema["properties"]) == {"error", "detail"} + assert response_schema["additionalProperties"] is False + methods = {"get", "put", "post", "delete", "options", "head", "patch", "trace"} + route_inventory = sorted( + f"{method.upper()} {path}" + for path, path_item in schema["paths"].items() + for method in path_item + if method in methods + ) + protected_inventory = sorted( + f"{method.upper()} {path}" + for path, path_item in schema["paths"].items() + for method, operation in path_item.items() + if method in methods and operation.get("security") + ) + assert len(route_inventory) == 46 + assert sha256("\n".join(route_inventory).encode()).hexdigest() == ( + "991f50d0dd6009e96c1cd8d0a8b6f403d6f48d81bb247075c103a9d74341425b" + ) + assert len(protected_inventory) == 44 + assert sha256("\n".join(protected_inventory).encode()).hexdigest() == ( + "ae15e39df8b1710e16b9c20cfa588a8ee8f96a98505f34e1cbcc5fe4c96a17d4" + ) + assert set(schema["paths"]["/health"]["get"]["responses"]) == {"200", "400", "500"} + assert {"401", "403", "503"} <= set( + schema["paths"]["/api/v1/auth/me"]["get"]["responses"] + ) + assert {"404"} <= set( + schema["paths"]["/api/v1/tasks/{task_id}"]["get"]["responses"] + ) + assert {"404", "409"} <= set( + schema["paths"]["/api/v1/tasks/{task_id}/claim"]["post"]["responses"] + ) + assert {"404", "409"} <= set( + schema["paths"]["/api/v1/projects/{project_id}/guides/{guide_id}/activate"][ + "post" + ]["responses"] + ) + for path_item in schema["paths"].values(): + for method, operation in path_item.items(): + if method not in {"get", "put", "post", "delete", "options", "head", "patch"}: + continue + request_headers = { + parameter["name"] for parameter in operation["parameters"] + } + assert {"X-Request-ID", "X-Correlation-ID"} <= request_headers + for response in operation["responses"].values(): + assert {"X-Request-ID", "X-Correlation-ID"} <= set(response["headers"]) + assert response["headers"]["X-Request-ID"]["required"] is True + assert response["headers"]["X-Correlation-ID"]["required"] is True diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index 0ccd3c0e..fbd28867 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -28,6 +28,7 @@ from app.core.config import Settings, get_settings from app.core.permissions import PermissionDenied, require_any_role from app.db import session as db_session +from app.db.session import get_db_session from app.interfaces.auth import AuthVerificationError, AuthVerificationUnavailableError from app.main import create_app from app.modules.actors.service import ActorService @@ -716,9 +717,12 @@ async def test_service_and_agent_tokens_receive_no_legacy_authority( scope="workstream:service", ) ) - agent = await verifier.verify( - issue_asymmetric_token(private_key, subject_kind="agent", scope="agent:identity") + agent_token = issue_asymmetric_token( + private_key, + subject_kind="agent", + scope="agent:identity", ) + agent = await verifier.verify(agent_token) space = await verifier.verify( issue_asymmetric_token(private_key, subject_kind="space", scope="space:identity") ) @@ -729,6 +733,35 @@ async def test_service_and_agent_tokens_receive_no_legacy_authority( with pytest.raises(HTTPException) as exc_info: await get_registered_actor(agent, None) # type: ignore[arg-type] assert exc_info.value.status_code == 403 + assert exc_info.value.error_code == "unsupported_subject_kind" + + app = create_app(production_verifier_settings()) + app.state.auth_verifier = verifier + + async def no_database_session(): + yield None + + app.dependency_overrides[get_db_session] = no_database_session + async with AsyncClient( + transport=ASGITransport(app=app), + base_url="http://testserver", + ) as client: + response = await client.get( + "/api/v1/auth/me", + headers={"Authorization": f"Bearer {agent_token}"}, + ) + + assert response.status_code == 403 + assert response.json() == { + "detail": "Unsupported subject kind", + "error": { + "code": "unsupported_subject_kind", + "message": "Unsupported subject kind", + "details": {}, + "correlation_id": response.headers["x-correlation-id"], + "retryable": False, + }, + } with pytest.raises(AuthVerificationError, match="service scope"): await verifier.verify( @@ -1205,6 +1238,13 @@ async def test_missing_bearer_token_is_rejected() -> None: assert response.status_code == 401 assert response.json()["detail"] == "Missing bearer token" assert response.headers["www-authenticate"] == "Bearer" + assert response.json()["error"] == { + "code": "missing_token", + "message": "Missing bearer token", + "details": {}, + "correlation_id": response.headers["x-correlation-id"], + "retryable": False, + } async def test_invalid_bearer_token_is_rejected(monkeypatch: pytest.MonkeyPatch) -> None: @@ -1226,6 +1266,8 @@ async def test_invalid_bearer_token_is_rejected(monkeypatch: pytest.MonkeyPatch) assert response.status_code == 401 assert response.json()["detail"] == "Invalid bearer token" + assert response.json()["error"]["code"] == "invalid_token" + assert response.json()["error"]["retryable"] is False async def test_invalid_production_verifier_configuration_is_service_unavailable() -> None: @@ -1242,6 +1284,8 @@ async def test_invalid_production_verifier_configuration_is_service_unavailable( assert response.status_code == 503 assert response.json()["detail"] == "Identity verification unavailable" + assert response.json()["error"]["code"] == "identity_verification_unavailable" + assert response.json()["error"]["retryable"] is True async def test_valid_dev_token_resolves_actor_context( @@ -1314,6 +1358,9 @@ async def fail_register_actor(self, actor): assert response.status_code == 503 assert response.json()["detail"] == "Actor registry unavailable" + assert response.json()["error"]["code"] == "service_unavailable" + assert response.json()["error"]["message"] == "Service unavailable" + assert response.json()["error"]["retryable"] is True async def test_actor_id_uses_subject_and_issuer_not_email() -> None: diff --git a/backend/tests/test_tasks.py b/backend/tests/test_tasks.py index 7872ce2f..4701faef 100644 --- a/backend/tests/test_tasks.py +++ b/backend/tests/test_tasks.py @@ -21,6 +21,7 @@ from app.adapters.auth.dev import actor_id_from_external_identity from app.core.config import get_settings from app.core.hashing import canonical_json_hash +from app.core.permissions import PermissionDenied from app.db import models as db_models from app.db import session as db_session from app.db.base import Base @@ -52,6 +53,7 @@ WorkstreamTask, ) from app.modules.tasks.repository import TaskRepository +from app.modules.tasks.service import TaskService, TaskServiceError from app.schemas.auth import ActorContext @@ -808,6 +810,8 @@ def test_submission_create_openapi_documents_domain_error() -> None: domain_schema = next(option for option in response_422["oneOf"] if "properties" in option) assert domain_schema["properties"]["code"]["enum"] == ["pre_submission_checker_failed"] assert "details" in domain_schema["properties"] + assert set(domain_schema["required"]) == {"code", "details", "error"} + assert domain_schema["additionalProperties"] is False def test_task_context_openapi_documents_locked_context_domain_error() -> None: @@ -821,6 +825,8 @@ def test_task_context_openapi_documents_locked_context_domain_error() -> None: domain_schema = next(option for option in response_422["oneOf"] if "properties" in option) assert domain_schema["properties"]["code"]["enum"] == ["task_locked_context_invalid"] assert "details" in domain_schema["properties"] + assert set(domain_schema["required"]) == {"code", "details", "error"} + assert domain_schema["additionalProperties"] is False def test_task_locked_context_constraints_bind_task_submission_and_hashes() -> None: @@ -911,6 +917,67 @@ def test_task_locked_context_constraints_bind_task_submission_and_hashes() -> No } +async def test_task_router_service_errors_use_canonical_request_context( + task_client: AsyncClient, + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fail_with_service_error(*_args, **_kwargs): + raise TaskServiceError("bounded task failure") + + cases = [ + ("create_task", "POST", "/api/v1/projects/project-id/tasks", complete_task_payload()), + ("get_task", "GET", "/api/v1/tasks/task-id", None), + ("get_task_work_context", "GET", "/api/v1/tasks/task-id/work-context", None), + ( + "get_task_submission_requirements", + "GET", + "/api/v1/tasks/task-id/submission-requirements", + None, + ), + ("get_task_locked_context", "GET", "/api/v1/tasks/task-id/locked-context", None), + ("move_to_screening", "POST", "/api/v1/tasks/task-id/screen", None), + ("release_to_ready", "POST", "/api/v1/tasks/task-id/release", None), + ("claim_task", "POST", "/api/v1/tasks/task-id/claim", None), + ("start_task", "POST", "/api/v1/tasks/task-id/start", None), + ( + "create_submission", + "POST", + "/api/v1/tasks/task-id/submissions", + complete_submission_payload(), + ), + ("list_task_submissions", "GET", "/api/v1/tasks/task-id/submissions", None), + ("get_submission", "GET", "/api/v1/submissions/submission-id", None), + ("finalize_submission", "POST", "/api/v1/submissions/submission-id/finalize", None), + ("list_task_audit_events", "GET", "/api/v1/tasks/task-id/audit-events", None), + ] + + for service_method, method, path, payload in cases: + monkeypatch.setattr(TaskService, service_method, fail_with_service_error) + response = await task_client.request( + method, + path, + headers=auth_headers(), + json=payload, + ) + + assert response.status_code == 400 + assert response.json()["detail"] == "bounded task failure" + assert response.json()["error"]["code"] == "invalid_request" + assert response.json()["error"]["correlation_id"] == response.headers[ + "x-correlation-id" + ] + + async def fail_with_permission_error(*_args, **_kwargs): + raise PermissionDenied("bounded permission failure") + + monkeypatch.setattr(TaskService, "get_task", fail_with_permission_error) + denied = await task_client.get("/api/v1/tasks/task-id", headers=auth_headers()) + + assert denied.status_code == 403 + assert denied.json()["detail"] == "bounded permission failure" + assert denied.json()["error"]["code"] == "permission_not_granted" + + async def test_actor_profile_services_update_existing_actor_rows(task_database_env: str) -> None: async with db_session.get_session_factory()() as session: service = ActorService(session) @@ -1543,7 +1610,10 @@ async def test_task_context_apis_fail_closed_when_locked_context_is_missing( ) assert response.status_code == 422 + assert set(response.json()) == {"code", "details", "error"} assert response.json()["code"] == "task_locked_context_invalid" + assert response.json()["error"]["code"] == "task_locked_context_invalid" + assert response.json()["error"]["details"] == response.json()["details"] assert "locked_guide_version" in response.json()["details"]["missing_fields"] @@ -1830,7 +1900,10 @@ async def test_submission_runtime_uses_locked_project_policy_not_task_required_f assert non_contract_artifact_response.status_code == 422, non_contract_artifact_response.text detail = non_contract_artifact_response.json() + assert set(detail) == {"code", "details", "error"} assert detail["code"] == "pre_submission_checker_failed" + assert detail["error"]["code"] == "pre_submission_checker_failed" + assert detail["error"]["details"] == detail["details"] required_files = next( result for result in detail["details"]["results"] diff --git a/docs/operations_authorization_service.md b/docs/operations_authorization_service.md index b954650f..fb0636ec 100644 --- a/docs/operations_authorization_service.md +++ b/docs/operations_authorization_service.md @@ -98,6 +98,52 @@ fields remain `null`. Consumers must not treat token identity metadata as a profile source of truth; canonical profile metadata belongs to the later actor profile migration. +## Request And Error Context + +Clients may send one `X-Request-ID` and one `X-Correlation-ID`. Each supplied +value must be a canonical lowercase, hyphenated, non-nil RFC 9562 UUID using the +RFC variant and version 1 through 8. A missing request ID is generated as +UUIDv4; a missing correlation ID reuses the effective request ID. Duplicate, +comma-joined, malformed, non-ASCII, nil, or unsupported-version values stop +before route/dependency execution with HTTP 400 `invalid_request`. That response +uses one newly generated UUIDv4 for both safe headers and never reflects the +rejected bytes. + +Every success and error response returns the effective IDs in +`X-Request-ID` and `X-Correlation-ID`. Application-supplied response values for +those headers are overwritten. Status, content length, duplicate unrelated +headers, `WWW-Authenticate`, `Retry-After`, streaming chunks, and background +work remain unchanged. + +Errors add this canonical object while retaining the existing top-level +`detail` or domain `code/details` compatibility fields: + +```json +{ + "error": { + "code": "invalid_request", + "message": "Request validation failed", + "details": {}, + "correlation_id": "00000000-0000-4000-8000-000000000001", + "retryable": false + } +} +``` + +The authentication boundary uses `missing_token`, `invalid_token`, +`identity_verification_unavailable`, and `unsupported_subject_kind` for their +exact branches. Verification/registry unavailability is retryable; invalid +credentials, unsupported kinds, validation, permission, not-found, conflict, +and internal application errors are not. The canonical nested validation +summary is capped at 20 errors with type/location evidence only. Existing +sanitized validator messages remain solely in the bounded compatibility +`detail` list. + +Use the correlation ID to join operator evidence. Never add bearer tokens, +claims, subjects, emails, SQL, provider bodies, exception text, or secrets to +responses or logs. AUTH-04A does not activate rate controls, grant APIs, or new +product authority; those remain owned by later separately reviewed chunks. + ## JWKS Rotation And Outage Rotation procedure: