feat/one-api

enableMonitoringShell 409 uses VALIDATION_ERROR code for a conflict condition

domains/cross-domain/src/contract-wizard.shells.ts:572

The handler maps WizardDecisionError to fail(409, 'VALIDATION_ERROR', msg). HTTP 409 is returned correctly, but the error code VALIDATION_ERROR semantically describes a validation failure, not a conflict. Pre-existing pattern in this handler, not introduced by this diff, but the new case is a semantic mismatch that could confuse callers inspecting the error code.

telemetry guard walk skips __tests__ — re-exports from test files undetected

apps/platform/src/api/utils/__tests__/public-v1-telemetry.test.ts:191

The walk skips __tests__ directories. A re-export of next/server placed inside __tests__/ would not be caught. Explicitly accepted by the commit: computed specifier evasions are 'deliberate rather than accidental'. The __tests__ exclusion is correct — test mock files legitimately reference next/server.

afterAll cleanup does not remove entity_relationships rows

domains/cross-domain/src/__tests__/contract-wizard-sites.integration.test.ts:96

Cleanup deletes site_utility_contracts, utility contracts, sites, memberships, orgs — but not entity_relationships. Since the fix removes entity_relationships writes, this is not a current correctness issue. The new test asserts rels.length===0 which catches any regression. Low cross-test leak risk given unique RPU generation.

catch block classifies ALL SDK errors as transient — ResourceNotFoundException included

packages/event-bus/src/handlers/webhook-dispatcher.lambda.ts:268

The catch block sets transient=true for any thrown SDK error, including ResourceNotFoundException (a secret that was deleted or renamed — a deterministic condition). This causes one Secrets Manager lookup per invocation indefinitely for a misconfigured secret name, rather than caching null after the first miss. Operationally safe (deliveries are skipped with logging, not sent unsigned), but the comment says 'transient by assumption' without narrowing the scope. Throttling and IAM propagation are genuinely transient; ResourceNotFoundException is not.

loadSealingKey exported from Lambda entrypoint — minor surface expansion

packages/event-bus/src/handlers/webhook-dispatcher.lambda.ts:234

loadSealingKey and __resetSealingKeyCacheForTests are now exported from the Lambda entrypoint. No production caller imports them; the Lambda bundle is private. The risk is that a future barrel re-export or package restructure could inadvertently expose the test seam. The established convention (__resetXForTests) is correct; a comment marking it test-only would reduce the surface.

Guard returns WizardDecisionError tag instead of a dedicated discriminated variant

domains/cross-domain/src/contract-wizard.shells.ts:572

The already-sited guard uses WizardErrors.decisionError({message, statusCode:409}), which produces _tag:'WizardDecisionError' — the catch-all for forwarded decision failures. The handler cannot distinguish 'contract already sited' from a genuine decideCreateSite error. ADR-016 requires unique, named _tag values per business rule. A dedicated WizardContractAlreadySitedError with _tag:'WizardContractAlreadySited' and fixed statusCode:409 should be added to the EnableMonitoringError union.

Guard belongs in a decision function by FCIS, not inlined in the shell

domains/cross-domain/src/contract-wizard.shells.ts:572

The `if (hasPath1Access)` precondition is a business rule — 'enableMonitoringShell is for requires_site contracts only'. FCIS places business rules in pure decision functions testable at ~5000/sec. The condition could live in a decideEnableMonitoring function receiving hasPath1Access. Not blocking since the surrounding shell already mixes access-check logic, but the untestability at unit speed is the cost.

Block comment over-explains WHAT the guard does

domains/cross-domain/src/contract-wizard.shells.ts:557

The 14-line block comment before the guard describes the hazard at length (duplicate site creation, unbounded). The non-obvious WHY — routing semantics from contract-access.ts (requires_site vs ok) — is in the last two lines. The code comment convention asks only for the non-obvious WHY. The hazard description is derivable from `hasPath1Access` and the surrounding context.

New no-site-info test placed in wrong describe block

domains/cross-domain/src/__tests__/contract-wizard-sites.integration.test.ts:161

The test 'creates a site named after the RPU when NO site information is given' was added inside 'createFromWizardShell — explicit existing site', which is the opposite of what this test covers. A reader scanning describe-block headings will miss this coverage, defeating the discoverability goal. Should be its own block, e.g. 'createFromWizardShell — no site information (RPU-named fallback)'.

enableMonitoringShell integration tests likely broken — invisible to CI

apps/platform/src/__tests__/integration/utility-contracts-api.test.ts:375

The beforeAll at line 375 creates a contract with enableMonitoring:false and no site info. After the earlier branch commit (345f42e2) made createFromWizardShell always create a site, that contract now has a site. The new 409 guard added in this commit fires when hasPath1Access=true — so every subsequent enableMonitoringHandler call in this describe block gets 409 instead of 200. The file is not in CI's explicit run list (only public-v1-idor, api-telemetry-persistence, rls-machine-claim), so the breakage is invisible. No test exists for the 409 path of enableMonitoringShell.

skippedUnsigned counter has no dispatch-level assertion

packages/event-bus/__tests__/unit/webhook-dispatcher.sealing-key.test.ts:1

The new skippedUnsigned field in DispatchSummary is incremented when open() returns null inside the handler's endpoint loop. The sealing-key test file only exercises loadSealingKey in isolation — it never instantiates the handler or asserts that skippedUnsigned propagates to the summary. The counter could silently revert to 0 without breaking any test — defeating the 'no longer looks like an idle bus' purpose of the field.

SecretBinary test asserts null via the wrong mechanism

packages/event-bus/__tests__/unit/webhook-dispatcher.sealing-key.test.ts:118

The test mocks {SecretBinary: new Uint8Array(32)} — so SecretString is undefined. The code computes raw = res.SecretString ?? '' = '', and Buffer.from(''.trim(), 'base64') produces a 0-byte buffer failing the 32-byte length check. The test returns null for the wrong reason: it would return null even if the code were changed to never check SecretBinary at all. A stronger test would pass a non-empty SecretString AND a SecretBinary and assert only the SecretString path is used.

CI db-changes filter misses handler-only changes to enableMonitoringShell's callers

.github/workflows/pr-checks.yml:176

The db-changes regex includes domains/cross-domain/ so contract-wizard changes trigger the cross-domain suite. But apps/platform/src/api/handlers/ is not in the filter — a change to utility-contracts.handler.ts routing enableMonitoringShell would not trigger the suite. Minor coverage gap for handler-layer-only refactors.

No enableMonitoringShell 409 integration test exists anywhere

domains/cross-domain/src/__tests__/contract-wizard-sites.integration.test.ts:1

The new guard (409 when contract already has a site) is not covered by any integration test. Per testing patterns, shell error flows should have integration tests. The guard is simple and a mutation would be caught eventually at runtime, but a targeted integration test would give CI-level confidence.

Stale JSDoc on cachedSealingKey contradicts new transient semantics

packages/event-bus/src/handlers/webhook-dispatcher.lambda.ts:210

The comment still says 'null is cached too, so a misconfigured environment costs one lookup'. After this fix, null is only cached for deterministic failures — transient errors return null without caching. The declaration-site comment should be updated to reflect the new two-case distinction.

transient flag can be replaced with an early return in the catch block

packages/event-bus/src/handlers/webhook-dispatcher.lambda.ts:249

A `return null` inside the catch block is exactly equivalent and removes the need for the boolean flag. Current: set transient=true in catch, then `if (transient) return null` after the block closes. Simpler: `catch (err) { console.error(...); return null; }`. No behavioural change.

REFUSING to purge grep anchor is robust — confirmed

scripts/db/rehearse-upgrade.sh:139

The migration RAISE format produces 'REFUSING to purge N webhook endpoint(s)...' — grep -q 'REFUSING to purge' matches correctly. Not fragile.