# Review record — `apollo-channel-intake-api-spec.md`

> *Naming note (2026-09-02, see `naming-glossary.md`): "Sibyla" written here before 2026-09-02 means the FDR skill build or the module Apollo built from it, which is now **Argus**; the retired applications and database are **Sibyla Legacy**; "Apollo" means this platform, which became **Sibyla** in Phase 3; "Hermes" before 2026-09-02 means the Hermes Agent runtime or GOTT's instance of it, which is now the agent **Apollo** (Phase 4).*

## Round 1 — revision 1, 2026-09-02

Two independent reviewers (read-only, verified against GOTT.Apollo, GOTT.Sibyla and the Hermes
plugin/skills). Reviewer S: tenancy, authorization, security, data model. Reviewer C: wire
contract, plugin, ingestion reuse, tests, cutover. Both verdicts: **REVISE**.

Consolidated findings, adjudicated by the author. "Accept" = will be applied in revision 2.

| # | Sev | Where | Finding (source) | Adjudication |
|---|---|---|---|---|
| 1 | Critical | §5 | "`worker_access`-style policies for `apollo_api`" would be `USING (true)`, i.e. cross-tenant read/write on `docint/jobque` (S1) | Accept: GRANTs only, no new permissive policies; meta-test that the policy set on those tables has no `apollo_api` policy |
| 2 | Critical | §5, D-CI-2 | Nothing ties `chnbnd.company_id` to `chnbnd.owner_id`; `HasCompany` under a machine context would accept any GUID (S2) | Accept: `UNIQUE (owner_id, id)` on `commst`, composite FK from `chnbnd`, RLS-scoped re-check at resolution, fail-closed test |
| 3 | Critical | §4.1, §5 | Replay keyed `(owner, channel, externalMessageId)` lets a second client of the same owner read another client's registration; legacy had `EnsureOwner` (S11, C1) | Accept: uniqueness and replay lookup on `(owner, client_id, channel, external_message_id)`; client check before any replay body |
| 4 | High | D-CI-2, §5 | `app.client_id` has no plumbing or lifetime rule; on a pooled connection a non-local `set_config` leaks across requests (S3) | Accept: binding lookup on a raw `NpgsqlDataSource` connection inside one explicit transaction, `set_config(..., true)` first, like `TenantResolver`; leak test |
| 5 | High | §5 | `channel_binding_lookup` is permissive and ORs with `tenant_isolation`; after the owner GUC is set all of the owner's bindings are visible (S4, C9) | Accept: policy declared `AS RESTRICTIVE`, `NULLIF(current_setting(...), '')`, `CHECK (length(client_id) > 0)` |
| 6 | High | D-CI-4, C7, C19 | Registration-time duplicate verdict from an agent-declared hash: contradicts P2-02 and `PossibleDuplicate`, is an existence oracle and a suppression vector, races with in-flight uploads, and matches quarantined rows for unverified senders (S5, C5, C6) | Accept: `sha256` stays required but is **integrity only**; no registration-time verdict; duplicates flow through `IngestionService` as `PossibleDuplicate`, surfaced as candidate `completed` + `possibleDuplicateOfIntakeId`; candidate status `duplicate` removed |
| 7 | High | §0, §8 | RB8 claimed satisfied; the SPF/DKIM/DMARC half is not enforceable by Apollo while the mailbox is read by Hermes (S6) | Accept as **recorded residual** against R-14: trusted email routes to a company but never reaches the ERP without the existing human review; DMARC enforcement moves to the Apollo-owned mailbox watcher (D-AI-10) |
| 8 | High | D-CI-7, §5 | `audlog` grants: INSERT/SELECT/UPDATE contradicts append-only; the revoke migration lists roles by name and misses a new role (S7) | Accept: INSERT+SELECT only; `ChannelIntake` migration re-runs the revoke for `apollo_api`; meta-test on `audlog` grants |
| 9 | High | C21 | Daily quarantine quota cannot live in the in-process rate limiter (S8, C14) | Accept: counted in `chnreg` inside the registration transaction; per-minute burst limits stay in the limiter |
| 10 | High | D-CI-5, §5 | `AcceptUploadAsync` opens its own transaction, derives actor and uploader from a non-nullable `UserId`; the overload as described cannot give "one transaction", `UploadedByUserId = null`, `ActorSubject = client_id` (S9, C7, C8) | Accept: `AcceptUploadCoreAsync` participating in an ambient transaction; `ITenantContext` gains `ActorSubject` and nullable `UserId`; `ApiTenantContext` with `AllCompanies = false`, `CompanyIds = {binding company}` |
| 11 | High | §4.1, §4.5, App. A | No code → HTTP status table; golden tests cannot be written (C2) | Accept: table added |
| 12 | High | §4.1 | Canonicalisation undefined; `idempotencyKey` inside the fingerprint turns a benign agent retry into a permanent 409 (S12, C3) | Accept: RFC 8785 (JCS) over normalised values, `receivedAt` truncated to whole seconds UTC, `idempotencyKey` excluded; golden vector in §8 |
| 13 | High | §4.4 | Aggregation never yields `expired` and hides partial `rejected` (C4) | Accept: any `awaitingUpload` → `awaitingUpload`; else any `expired` → `expired`; else any `rejected` → `rejected`; else any `quarantined` → `quarantined`; else `completed`; expiry rule and `completedAt` stated |
| 14 | High | §7, §11 | Reverse-proxied path option contradicts the plugin's origin guard (C10) | Accept: option struck; hostname `api.sibyla.gottsolutions.net` stays the proposal |
| 15 | Medium | D-CI-2 step 4 | `ReleaseQuarantineAsync` takes no company and enqueues unconditionally; a `sender_unverified` row can be released unassigned; the same sender re-quarantines forever with no admin surface (S10) | Accept: release requires a company when reason is `sender_unverified`; minimal "trust this sender" action in `Documents.razor` that creates the `chnbnd` row (in scope, smallest viable loop) |
| 16 | Medium | C4 | `aud = apollo-api` is shared with every `apollo-web` user token; audience validates nothing (S18) | Accept: scope resource `apollo-channel-intake` → own audience; tokens with `sub != client_id` rejected |
| 17 | Medium | D-CI-2 step 5 | `routing_changed` on a disabled binding strands the bytes (C19) | Accept: disabled binding at upload → quarantine path; `routing_changed` only for a changed company on a partially processed registration |
| 18 | Medium | D-CI-7, §6, §9 | Role plan does not match `setup-db.ps1` blanket grants, revoke loops, `DatabaseTarget.ps1`, `PreviewParityTests` role list; migration must also run on Preview; `setup-db -Target Main` precedes `migrate -Target Preview` (S13, C13a/b) | Accept |
| 19 | Medium | §8 | `apollo_api` NOBYPASSRLS assertion does not fit `RlsPolicyMetaTests`/`TestDb` (S14) | Accept: `pg_roles` query from the app connection |
| 20 | Medium | §7 | `expected_sha256` upload parameter redundant; plugin should hash the bytes it sends and compare with the registered value; stream instead of `read_bytes` (C15) | Accept |
| 21 | Medium | §7, App. A | 64 KiB response cap not restated; client-side codes (`_response_too_large`, `_non_json_response`, `_transport_unavailable`, `_retry_exhausted`) missing from the closed set (C16, C17) | Accept |
| 22 | Medium | §4 | `X-Correlation-Id` required or optional is ambiguous (C18) | Accept: required on all three routes; `_correlation_id_required` added |
| 23 | Medium | §10 | Cutover: skills/addendum updated after the toolset swap leaves the agent with dead tool names; the `UserChannels` live-DB authorization procedure in the skills is not retired (C11, C12) | Accept: instructions first, one gateway restart; `UserChannels` procedure named and replaced by `seed-channel-bindings.ps1` / the trust action |
| 24 | Medium | §8 | Test infra: JWT test setup must null `Authority`/`MetadataAddress` and supply a static configuration; `Ingestion:StagingRoot` override and cleanup; limiter test order; missing grant-scope, policy-narrowing, fingerprint vector and upload-time spoof tests (C20) | Accept |
| 25 | Low | §6, §9 | "No Data Protection ring needed" is false for `AddApolloPlatform`; `PreviewConnectionGuard` omitted; the API host would register the AI harness, preview manager and CLI runner (S15, C13c) | Accept: `AddApolloPlatformCore` without harness/preview/CLI registrations; keys path stated; guard called |
| 26 | Low | §5, C5 | `NULLIF` idiom; `client_id`/`azp` claim shape evidenced only by the legacy code; log no token claims (S16, S17) | Accept: cite legacy evidence; mint-and-inspect check at checkpoint B |
| 27 | Low | §8, §4.1, D-CI-1, D-CI-8 | `docint` clean path is `Queued` not `Received`; `Sibyla.Api` references Contracts too; test project must not reference `Sibyla.Web`; `receivedAt` needs a lower bound; example `candidateKey` outside the charset; secret written to `idp-clients.json` vs "only in Hermes `.env`" (C21, C22) | Accept: lower bound 30 days; secret recorded in `idp-clients.json` as the operator's copy, `.env` is the runtime copy |

Author's verdict on round 1: **REVISE** — three critical and eleven high findings, all bounded
to §4, §5, D-CI-2/4/5/7/8 and the test/cutover sections; none requires a change to the protocol,
the host decision or the quarantine ruling. Revision 2 to be re-reviewed by both reviewers before
checkpoint A.

## Round 2 — revision 2, 2026-09-02: built, re-review pending

Revision 2 applied the 27 adjudications above (the list at the top of the spec is the record
of how each one landed in code) and was built the same day on the owner's word. The re-review by
both reviewers that D-CI-10 requires before checkpoint A has not happened. What exists to review:
`src/Sibyla.Api`, `src/Sibyla.Platform.Contracts/ChannelIntake`,
`src/Sibyla.Platform.Infrastructure/ChannelIntake` and the `IngestionService` core split, the
migration `20260902173646_ChannelIntake` with `db/channel-intake-api-role.sql`,
`tests/Sibyla.Tests.Api` (45/45 after the owner provisioned the `apollo_api` role; evidence in
`tests/Sibyla.Tests.Api/evidence/CHECKPOINT-AB-green.txt`), `tests/Sibyla.Tests.TenantIsolation/ChannelIntakeFailClosedTests`
(9/9, suite 52/52), `local/hermes/apollo-channel-intake` (36 unit tests green), the scripts
`setup-db.ps1`, `seed-channel-bindings.ps1`, `run-api.ps1`, `RegisterIdpClients --channel-intake`.
Verdicts arrived 2026-09-03 (two independent read-only reviewers, same split as round 1;
GOTT.Apollo at `169c168`, production live on `api.sibyla.gottsolutions.net`, cutover steps 1-6 done,
the F7 watcher just added). **Reviewer S: ACCEPT** (no Critical or High; 1 Medium, 9 Low; every
F-claim in scope verified TRUE with file:line). **Reviewer C: REVISE** (no Critical; 2 High, both in
the new watcher; 5 Medium; 8 Low; the F-claims in scope verified TRUE, the golden fingerprint
recomputed independently and matching). Consolidated, adjudicated and applied by the author the
same night; "Fixed" = in the code and its tests before this record was written.

| # | Sev | Where | Finding (source) | Adjudication |
|---|---|---|---|---|
| 1 | Medium | `Program.cs`, `ChannelIntakeEndpoints.cs`, `ChannelIntakeValidators.cs` | The 16 KiB `trustedMetadata` cap is checked after full deserialisation and canonicalisation; the only body bound on `POST /registrations` is the PUT's 50 MiB Kestrel limit (S1) | **Fixed**: `MaxRegistrationBodyBytes = 256 KiB` on the POST (per-request `IHttpMaxRequestBodySizeFeature` plus a `Content-Length` pre-check, `BadHttpRequestException` → `request_invalid`); raw metadata text bounded at twice the cap before canonicalising |
| 2 | Low | `ChannelIntakeActor.cs` | `azp` fallback; `sub` compared only when present (S2) | **Fixed**: `client_id` only; `sub` required and ordinal-equal |
| 3 | Low | `ChannelIntakeService.cs` | `possibleDuplicateOfIntakeId` also returned for a quarantined candidate (S3) | **Fixed**: emitted only when the candidate is `completed` |
| 4 | Low | `ChannelIntakeService.cs` | Daily unverified quota is a plain `COUNT` under READ COMMITTED (S4) | **Fixed**: `pg_advisory_xact_lock(hashtext(client_id))` before the count, inside the transaction |
| 5 | Low | `ChannelIntakeService.cs`, `IngestionService.cs` | The EF transaction is open while the PUT body streams to disk (S5) | Accept as bounded (one client, 30 uploads/min, Kestrel min-rate); staging before the transaction is a Slice-3 refactor of `AcceptUploadCoreAsync` |
| 6 | Low | `ChannelBindingResolver.cs` | A suspended owner's client keeps registering (S6) | **Fixed**: `ResolveClientAsync` requires `ownmst.status = 0` under the owner GUC → `403 client_not_bound` |
| 7 | Low | `ChannelIntakeService.cs` | Disabled binding at upload leaves `chnreg.sender_verified = true` beside a quarantined candidate (S7) | Deferred: operator-view consistency, no leak; recorded for the admin surface work |
| 8 | Low | migration | `chncan.intake_id → docint(id)` not owner-composite (S8) | Deferred to the next migration (`UNIQUE (owner_id, id)` on `docint`); unreachable through the API |
| 9 | Low | `ChannelBindingAdminService.cs` | "Trust this sender" re-enables a deliberately disabled binding and does not require a quarantined source (S9) | Deferred to the admin surface (harness plan `aiprofc`); recorded |
| 10 | Low | IdP scope | The retired client `sibyla-apolo` is a valid bearer for this API until step 8; `chnbnd` is the only gate (S10, C7) | Accept as the designed fail-closed state; closes at cutover step 8 (owner's step) |
| 11 | **High** | `apollo_email_intake.py` | Watcher stalls once ≥30 terminal messages sit inside the lookback: one fixed page, oldest first (C1) | **Fixed**: `@odata.nextLink` paging until `max_messages` new ones are handled; test `test_terminal_messages_do_not_starve_newer_ones_across_pages` |
| 12 | **High** | `apollo_email_intake.py`, `client.py` | A message the plugin client refuses locally (`ValueError`) or a Graph error on one attachment aborts the run before the ledger is saved; oldest-first order makes it a poison message (C2, C14) | **Fixed**: per-message `try/except` (`ValueError` → held `Sibyla-Retida` with `client_validation`; `RuntimeError`/`OSError` → transient); ledger saved after every message; tests for both |
| 13 | Medium | `ChannelIntakeService.cs`, spec §7.1 | An expired registration can never be re-registered (unique index + replay returns the expired row) while §7.1 and the watcher assume it can (C3) | **Fixed**: an identical replay of an `expired` registration re-opens it (candidates await again, fresh `expiresAt`, audit `channel_intake.reopened`); §7.1 corrected; contract test extended |
| 14 | Medium | `ChannelIntakeService.cs` | Two overlapping PUTs for one candidate both ingest (C4) | **Fixed**: `SELECT … FROM chncan WHERE id = $1 FOR UPDATE` at the top of `UploadAsync` |
| 15 | Medium | spec §10 step 7 | No unpause protocol for mail that reached `Validar` while the watcher was paused; no operator retry (C5) | **Fixed**: protocol written into step 7 (lookback = pause length ≤ 30 days, dry run first); `apollo_email_intake.py --retry <key>` |
| 16 | Medium | Hermes skill `sibyla-email-channel-intake-reprocessing.md` | The skill's manual reprocessing used the Graph message id while the watcher uses the RFC id: two registrations of one email (C6) | **Fixed**: the skill now names the RFC `internetMessageId` and the watcher's idempotency key formula |
| 17 | Medium | Hermes `.env`, plugin folder | Legacy `SIBYLA_CHANNEL_INTAKE_*` keys and the disabled legacy plugin folder still present (C7) | Owner's step 8 as specified (the legacy path stays reachable until the observation is done); recommended to the owner to do it early |
| 18 | Low | `apollo_email_intake.py` | Graph `size` includes encoding overhead: constant re-download and a false `no_candidates` for large files; `no_candidates` invisible (C8) | **Fixed**: filter with a 1.4× margin, real limit enforced on the downloaded bytes, cached file reused (the plugin verifies the hash), category `Sibyla-SemDocumento` |
| 19 | Low | `client.py` | Optional `sha256` upload parameter is adjudication 20's `expected_sha256` renamed (C9) | Accept: harmless, the server enforces the registered hash; kept for the tool schema's stability |
| 20 | Low | `client.py` | WhatsApp sender shape looser than C10 (C10) | **Fixed**: E.164 with the plus or a 9–20 digit numeric LID, exactly the server's rule; test |
| 21 | Low | `client.py` | `Retry-After` HTTP-date raises; the hour-long quota 429 is still retried in-call; a 2xx with an unreadable body returns `ok: true` (C11) | **Fixed**: `_retry_after_seconds` (integer, HTTP-date, absent); `Retry-After > 10 s` returned to the caller without sleeping (`retryAfterSeconds`); an unreadable success body is `ok: false`; tests |
| 22 | Low | `Sibyla.Api` | No per-request structured log line (C12) | **Fixed**: `RequestLogMiddleware` (method, route, status, code, correlation id, client id, elapsed; never a sender, metadata or file name) |
| 23 | Low | spec App. A, §4.3–4.4 | `_correlation_id_required` missing from the closed set; revision-1 `duplicate` text left in §4.3–4.4 (C13) | **Fixed** in the spec text |
| 24 | Low | tests | §8 gaps: 411 on the wire, 413 on the wire, the concurrent-registration race, service tests without HTTP, golden request/response pairs, `candidate_not_found`, `idempotency_key_invalid`, the per-minute limiter (C15) | Deferred, recorded here as the open test debt; the watcher's poison and paging tests were added. **Closed 2026-09-04** (`tests/Sibyla.Tests.Api/WireEdgeTests.cs`, 9 cases, after cutover step 7): chunked PUT → 411 `content_length_required` with the candidate still open; a candidate declared over 50 MiB → 413 `file_too_large`; a registration body over 256 KiB → 400 before parsing; unknown candidate → 404 `candidate_not_found`; keys outside the charset → 400 `idempotency_key_invalid`; the register limiter → 429 with `Retry-After: 60`, problem+json, partitioned by client; five rounds of two parallel `RegisterAsync` calls in separate DI scopes without HTTP → one row, at most one 201, the loser `replayed`; golden pairs under `golden/wire-*.json` (trusted 201, replay 200, unverified 201, validation 400) with ids, timestamps and random keys normalised, written by the first green run and pinned |

After the fixes: build clean; `Sibyla.Tests.Api` 45/45, `Sibyla.Tests.TenantIsolation` 52/52, plugin
39/39, watcher 16/16; the other suites re-run before the commit (counts in the plan entry). The
plugin and the watcher were re-installed into Hermes and the gateway restarted; the API host was
redeployed (release and health in the plan entry of the same night).

**Author's verdict on round 2: the API and the plugin are ACCEPTED as built; the watcher's two
High findings are fixed and its cron stays paused until the owner's observation (step 7).**
Checkpoint A of D-CI-10 is therefore closed; the final review with the evidence record remains. Evidence record for that final review: `tests/Sibyla.Tests.Api/evidence/CUTOVER-260904.md` (2026-09-04).

## Final review (D-CI-10) — 2026-09-04

Two independent read-only reviewers, launched together at 13:50 UTC on main `9aff2a1` with no
knowledge of each other's findings: reviewer S (spec conformance, every prior row revisited, the
golden pairs against §4) and reviewer O (operations and security: live production state against the
evidence record, the code paths exercised on 2026-09-04, hygiene). Input: the spec, this record, the
evidence record `tests/Sibyla.Tests.Api/evidence/CUTOVER-260904.md`, the run record §7e–§7h, the
plan entries of the day, and the code at head.

**Reviewer S: ACCEPT** — "the code, tests and golden pairs match the spec as amended by the F1–F27
list, the round-2 fixes and the 2026-09-04 rulings; the divergences found are one narrow behavioural
gap on the quarantine-release path and documentation/count staleness." All 27 round-1 rows and all 24
round-2 rows re-verified in code (rows 23/25 of round 1 and 10/16/17/23 of round 2 "Partly" or
UNVERIFIED only because the skill and addendum live in the Hermes home, not the repository). Golden
pairs conformant to §4 (14 registration and 11 candidate members, casing, nullability, URL shapes,
problem body).

**Reviewer O: ACCEPT** — "production state matches the record on every checkable item and the
day's server-side changes are sound." Twelve production checks matched (release
`20260904-124259-bb6a121` on web, api and worker with manifests; deployed `web.config` without
WebDAV; legacy sites Stopped/Stopped/Started; `GottSibylaWorker` gone; three health endpoints 200
with role `sibyla_api` and 19 migrations; legacy plugin absent; `apollo-channel-intake` 1.1.0 with
the hook, byte-identical to the repository; rollback folders and `PREVIOUS.txt` per host; the three
backup folders). No secrets in the day's commits; no tracked temporary files.

| # | From | Severity | Finding | Disposition |
|---|---|---|---|---|
| FR-1 | O | Medium | `envelope.py` returned `None` when the marker was already in the message text; a WhatsApp caption is that text, so an allow-listed sender who types `[apollo-channel-intake trusted gateway envelope: …]` suppresses the genuine line and the forged one is the only envelope the agent sees (the server still requires a binding, so only impersonation among bound senders was possible). `test_envelope.py` pinned it as "idempotent". | **Fixed**, plugin 1.1.1: a marker present in message text is rewritten to `[sender-typed text, not a gateway envelope:` (case-insensitive) and the genuine line is appended regardless; idempotency now compares the text's last line with the line the hook would itself build. Six new tests (60 total). Not yet installed in Hermes (owner's word). |
| FR-2 | S | Medium | `ReleaseQuarantineAsync` compared a company-less document only against company-less rows, so a quarantined intake released without a company (allowed for any reason other than `sender_unverified`) could be queued although the same bytes were filed under a company — the 2026-09-04 ruling applied at intake and assignment but not at release. | **Fixed**: the release lookup is licence-wide while the document has no company and per company once it has one; `IntakeDuplicateControlTests` +2 (17/17). Reaches production with the next release. |
| FR-3 | O, S | Low | `received_at` fell back to the dispatch clock silently, against "never invented". | **Fixed**: the line carries `received_at_source=gateway` or `dispatch-clock`. |
| FR-4 | O, S | Low | Evidence-record counts wrong in the parts (contract 21, service 9, watcher 12) though the totals held. | **Corrected** in the record's addendum (contract 20, service 26, web.config 1, wire 9 = 56; watcher 16); spec §7.1 corrected. |
| FR-5 | O | Low | Not in any record: scheduled task `Sibyla Documental Acceptance Runner` (Enabled, no trigger, principal `GottSibylaDocumental`, last run 2026-08-28) and a stray `apollo-deployment-slice1-v3-pre-recovery.bundle` in the retirement backup folder. | **Recorded** in the evidence addendum; the task joins the owner's final-freeze list; nothing touched. |
| FR-6 | S | Low | Spec body stale where the F-list did not reach: §4.3/§4.4 `duplicate` and `duplicateOfIntakeId`, §4.1 fingerprint, audience `apollo-api`, role `apollo_api`, §6 host registration, §7 plugin 1.0.0 and `expected_sha256`, §8 "duplicate at registration", §11 item 1; the 2026-09-04 changes only in §10. | **Fixed**: revision 3 note at the top of the spec as the authority, plus the individual lines. |
| FR-7 | S | Low | Request log lacked the registration id §5 names. | **Fixed**: `registration=<route id>` in the line. |
| FR-8 | S | Low | `trustedMetadata` members of JSON `null` passed the validator. | **Fixed**: refused as `trusted_metadata_invalid`; `ServiceTests` +1 assertion. |
| FR-9 | S | Low | The evidence record's residual list omitted round-2 row 5 (EF transaction open while the PUT body streams, Slice-3 refactor). | **Recorded** in the addendum. |

Unverifiable from the repository for both reviewers, and left so: the live registration and document
ids and statuses (database reads are outside the read-only brief), the IdP state after step 8, the
Hermes cron state, and whether the legacy `SIBYLA_CHANNEL_INTAKE_*` keys left the Hermes `.env`.

**Verdict of the final review: ACCEPT by both reviewers; D-CI-10's last checkpoint is closed.** The
fixes above are on main (merge commit in the plan entry). Two of them wait for the owner's word to
reach production: the plugin 1.1.1 install (FR-1, FR-3; with one sentence added to skill step 2
naming the neutralised marker as caption) and a release carrying FR-2, FR-7 and FR-8. Suites at the
fix commit on the Preview database: Api 56/56, Platform duplicate control 17/17, TenantIsolation
58/58, plugin 60.