docs: record every re-review finding as closed with its evidence
All 38 findings from the 2026-08-14 adapter re-review are now FIXED. The twelve the first pass did not reach — RPC-RR-01, RT-RR-01 through RT-RR-04 and TR-RR-01 through TR-RR-07 — each landed with a named adversarial test that failed on the pre-fix source and passes on the landed one, verified by reverting the source file and re-running. The ledger also records the six contracts that changed shape and are therefore breaking for an external implementor: the Browser RPC server-stream lease, the optional recovery context on AuthSessionPort, the presigned registration protocol version, the resumable dispose result and cleanup deadline, and the already-landed required image resolve signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
5a76f95291
commit
8d6d84bfcc
@@ -229,10 +229,14 @@ Source: [`docs/reviews/adapters/RE-REVIEW-2026-08-14.md`](../reviews/adapters/RE
|
|||||||
38 findings (High 19 / Medium 17 / Low 2) raised against `3b481eb`.
|
38 findings (High 19 / Medium 17 / Low 2) raised against `3b481eb`.
|
||||||
|
|
||||||
**GOV-02.** That re-review found the previous section of this ledger closed a
|
**GOV-02.** That re-review found the previous section of this ledger closed a
|
||||||
number of rows as `FIXED_NOT_RELEASED` that were in fact partial. The table
|
number of rows as `FIXED_NOT_RELEASED` that were in fact partial. The tables
|
||||||
below is written the other way round: a row is `FIXED` only where a new
|
below are written the other way round: a row is `FIXED` only where a new
|
||||||
adversarial test failed first on the pre-fix source and passes on the landed
|
adversarial test failed first on the pre-fix source and passes on the landed
|
||||||
one, and everything else says plainly that it was not done.
|
one.
|
||||||
|
|
||||||
|
**All 38 are now `FIXED`.** The first pass closed 26; the second closed the
|
||||||
|
remaining twelve, which each needed a lifecycle or contract change rather than a
|
||||||
|
contained edit.
|
||||||
|
|
||||||
### Landed
|
### Landed
|
||||||
|
|
||||||
@@ -265,30 +269,43 @@ one, and everything else says plainly that it was not done.
|
|||||||
| GOV-01 | Low | `FIXED` | this commit | `scripts/check-adapter-inventory.ts` diffs `docs/reviews/adapters/INVENTORY.md` against `git ls-files src/adapters`. The missing `src/adapters/platform/abortable-operation.ts` row is restored and the total is 119/119. |
|
| GOV-01 | Low | `FIXED` | this commit | `scripts/check-adapter-inventory.ts` diffs `docs/reviews/adapters/INVENTORY.md` against `git ls-files src/adapters`. The missing `src/adapters/platform/abortable-operation.ts` row is restored and the total is 119/119. |
|
||||||
| GOV-02 | Medium | `FIXED` | this commit | This section replaces the over-closed rows with evidence-linked dispositions and an explicit not-done list. |
|
| GOV-02 | Medium | `FIXED` | this commit | This section replaces the over-closed rows with evidence-linked dispositions and an explicit not-done list. |
|
||||||
|
|
||||||
### Not done in this pass
|
### Landed in the second pass
|
||||||
|
|
||||||
These are recorded as **NOT_STARTED**, not as partial credit. Each needs a
|
The twelve findings the first pass did not reach are now closed on the same
|
||||||
lifecycle change larger than a contained edit, and none of them is in the
|
terms: a named adversarial test failed on the pre-fix source and passes on the
|
||||||
default composition today.
|
landed one.
|
||||||
|
|
||||||
| ID | Severity | Why it is still open |
|
| ID | Severity | Commit | Red-then-green evidence |
|
||||||
|
| --- | --- | --- | --- |
|
||||||
|
| RPC-RR-01 | High | `a7390e3` | `tests/unit/browser-rpc/browser-rpc-remediation.test.ts` — `openServerStream` returns a lease (`streamId`, `frames`, `cancel`, `waitClosed`) decoded from own data descriptors. A timed-out stream is cancelled exactly once, a second stream for the same operation is refused as `CONFLICT` / `RPC_STREAM_DRAINING` without reaching the transport, and admission resumes only after `waitClosed()` settles. |
|
||||||
|
| RT-RR-01 | High | `c0f53d1` | `tests/unit/realtime/stream-coordinator.test.ts` — a `close()` during a running apply reported success before the fix; tasks are now registered at invocation, so it reports `IDLE_TIMEOUT` and `DRAINING`. |
|
||||||
|
| RT-RR-02 | High | `c0f53d1` | Same suite — a queued event started running inside DRAINING, and a timed-out effect left its resume token in place. The queued event is now dropped as `CLOSED` at execution time and the token is discarded with `freshness: UNKNOWN`. |
|
||||||
|
| RT-RR-03 | Medium | `c0f53d1` | `tests/unit/realtime/live-poll-handoff-coordinator.test.ts` — a second `close()` replayed the cached timeout forever; only an in-flight close is shared now, fenced writers are retained until their tails settle, and a later close converges to success. |
|
||||||
|
| RT-RR-04 | High | `c0f53d1` | Same suite — `close()` reported quiescence while a checkpoint was still running; checkpoint work now joins the physical-task registry. |
|
||||||
|
| TR-RR-05 | High | `46e067e` | `tests/unit/abortable-operation.test.ts` — a rejection was reported as `TERMINAL/CLOSED` while `terminal()` said no owner, and a throwing scheduler released the caller listener leaving no owner at all, so later aborts were invisible. The primitive now distinguishes `REJECTED`, agrees with `terminal()`, snapshots the scheduler, closes atomically on install failure and compensates a late value exactly once. Both presigned subsystems migrated onto it, replacing two hand-written copies. |
|
||||||
|
| TR-RR-01 | High | `46e067e` | `tests/unit/presigned-transfer.test.ts` — `close()` released bookkeeping without aborting, and the consumer signal joined only after the fetch began. Both are fixed; the scheduler-failure test now pins fail-closed. |
|
||||||
|
| TR-RR-02 | High | `46e067e` | The upload scope is created before the digest and the digest races the caller and deadline; the vault claim and network call follow an owner re-check. |
|
||||||
|
| TR-RR-03 | High | `46e067e` | Same suite — the registration is a versioned exact union: unknown/missing protocol version, plaintext target, ambient credential and cookie headers, a non-2xx expected status and any extra own field are each refused at the issuer seam. |
|
||||||
|
| TR-RR-04 | Medium | `5a76f95` | Same suite — the presigned source was never closed on success, writer failure or abort; a holder now closes it exactly once at the outermost boundary on all three. |
|
||||||
|
| TR-RR-06 | High | `5a76f95` | `tests/unit/resumable-upload-runtime.test.ts` — a never-granting mutation lock made `dispose()` unbounded; it is now bounded by `cleanupDeadlineMs`, returns the drain result, and leaves the runtime `CLOSING` with the checkpoint store open when the drain is unproved. |
|
||||||
|
| TR-RR-07 | High | `5a76f95` | `tests/unit/image-cdn-runtime.test.ts` — after an abort a new verification was admitted while the abandoned verifier still ran; the slot is now held until the raw verifier settles. |
|
||||||
|
|
||||||
|
**All 38 re-review findings are closed.** No row remains `NOT_STARTED`, and the
|
||||||
|
structural gate for the shared `abortable-operation` primitive is now active —
|
||||||
|
it was withheld until the primitive actually had production importers, because a
|
||||||
|
gate that fails CI for a documented but unfixed defect reports the wrong thing.
|
||||||
|
|
||||||
|
Contracts that changed shape, and are therefore breaking for an external
|
||||||
|
implementor:
|
||||||
|
|
||||||
|
| Contract | Change | Reason |
|
||||||
| --- | --- | --- |
|
| --- | --- | --- |
|
||||||
| RPC-RR-01 | High | Requires changing `BrowserRpcTransport.openServerStream` from a bare `AsyncIterable` to a lease with `cancel(reason)`, `waitClosed()` and a physical identity, plus an active-lease registry and a `DRAINING` admission fence in the runtime. That is a transport contract migration, not an internal fix. |
|
| `BrowserRpcTransport.openServerStream` | returns `BrowserRpcServerStreamLease` instead of `AsyncIterable` | RPC-RR-01 needs cancellation and closure evidence |
|
||||||
| RT-RR-01 | High | `stream-coordinator` must register a physical task at the moment it calls an external effect, not only after a timeout, and `close` must drain every registered task. |
|
| `AuthSessionPort.recover` | accepts an optional `CredentialOperationContext` | LEG-01; optional for one release |
|
||||||
| RT-RR-02 | High | Queued events must re-check lifecycle and generation at execution time, and a non-cooperative timeout must discard the resume token and record `recoveryRequired`. |
|
| `PresignedCapabilityRegistration` | gains `protocol` | TR-RR-03 versioned exact union |
|
||||||
| RT-RR-03 | Medium | `live-poll-handoff-coordinator` caches the first close timeout forever; only an in-flight close promise may be shared, and the writer tail must be pruned in `finally`. |
|
| `ResumableUploadRuntime.dispose` | returns `BrowserDataResult<void>` | TR-RR-06 bounded drain result |
|
||||||
| RT-RR-04 | High | Handoff checkpoint work must live in the same physical-task registry as writer and recovery work. |
|
| `ResumableUploadRuntimePolicy` | gains `cleanupDeadlineMs` | TR-RR-06 teardown bound |
|
||||||
| TR-RR-01 | High | Presigned download `close` must compose the outer signal, the consumer stream signal and the deadline into one ownership signal before `fetch`, and cancel the reader lease explicitly. |
|
| `ImageCdnPresentationPort.resolve` | `signal` required | BT-IMG-01, landed earlier |
|
||||||
| TR-RR-02 | High | Upload digest computation must happen inside the operation abort scope and race the caller and deadline terminals. |
|
|
||||||
| TR-RR-03 | High | The capability vault registration contract must become a versioned exact union validated at registration time. |
|
|
||||||
| TR-RR-04 | Medium | Download delivery must preserve the closeable subtype through projection and call `close()` exactly once in the outermost `finally`. |
|
|
||||||
| TR-RR-05 | High | The shared `abortable-operation` primitive needs VALUE / REJECTED / TERMINAL results, a bound scheduler snapshot and atomic install-failure cleanup, and all four consumers must migrate onto it. It still has **zero production importers**, so the structural gate for that is deliberately not added yet — adding it now would fail CI for a defect that is documented rather than fixed. |
|
|
||||||
| TR-RR-06 | High | `dispose()` must bound its drain with a cleanup deadline from policy and track abort operations from the moment they are admitted. |
|
|
||||||
| TR-RR-07 | High | The image verification semaphore must hold its slot until the raw verifier settles or is confirmed cancelled, not until the wrapper returns. |
|
|
||||||
|
|
||||||
None of RPC-RR-01, RT-RR-01..04 or TR-RR-01..07 may be treated as closed, and no
|
|
||||||
capability they cover may be promoted, until each has its own red-then-green
|
|
||||||
evidence in this table.
|
|
||||||
|
|
||||||
### Gates after this pass
|
### Gates after this pass
|
||||||
|
|
||||||
@@ -299,14 +316,17 @@ Run on the landed tree. Only what actually passed is claimed as passing.
|
|||||||
| `corepack pnpm check:types` | 0 | all six projects |
|
| `corepack pnpm check:types` | 0 | all six projects |
|
||||||
| `corepack pnpm lint` | 0 | `--max-warnings=0` clean |
|
| `corepack pnpm lint` | 0 | `--max-warnings=0` clean |
|
||||||
| `corepack pnpm check:architecture` | 0 | 289 modules, 868 dependencies; 12 fixtures PASS |
|
| `corepack pnpm check:architecture` | 0 | 289 modules, 868 dependencies; 12 fixtures PASS |
|
||||||
| `corepack pnpm check:adapter-inventory` | 0 | 119 files, 7 shared asset extensions |
|
| `corepack pnpm check:adapter-inventory` | 0 | 119 files, 7 shared asset extensions, fixture linking, primitive importers |
|
||||||
| `corepack pnpm check:registries` | 0 | 11 registries PASS |
|
| `corepack pnpm check:registries` | 0 | 11 registries PASS |
|
||||||
| `corepack pnpm check:diagnostics` | 0 | 8 diagnostics / 5 telemetry producers |
|
| `corepack pnpm check:diagnostics` | 0 | 8 diagnostics / 5 telemetry producers |
|
||||||
| `corepack pnpm check:browser-file-storage-boundaries` | 0 | PASS, 34 rejections |
|
| `corepack pnpm check:browser-file-storage-boundaries` | 0 | PASS, 34 rejections |
|
||||||
| `corepack pnpm check:realtime-boundaries` | 0 | PASS |
|
| `corepack pnpm check:realtime-boundaries` | 0 | PASS |
|
||||||
| `corepack pnpm verify:documentation` | 0 | PASS_SCOPED |
|
| `corepack pnpm verify:documentation` | 0 | PASS_SCOPED |
|
||||||
| `git diff --check` | 0 | clean |
|
| `git diff --check` | 0 | clean |
|
||||||
| `tests/unit` (excluding the four below) + `tests/integration` | 0 | see the exclusion table |
|
| `tests/unit` + `tests/integration`, no exclusions | — | 1643 passed / 1747; the 104 failures are the four environmental files below |
|
||||||
|
| `corepack pnpm test:component` | 0 | 126 passed |
|
||||||
|
| `corepack pnpm test:recipes` | 0 | 17 passed |
|
||||||
|
| `corepack pnpm test:reference-feature` | 0 | 26 passed |
|
||||||
|
|
||||||
### Environmental failures, not claimed as green
|
### Environmental failures, not claimed as green
|
||||||
|
|
||||||
@@ -314,7 +334,7 @@ Run on the landed tree. Only what actually passed is claimed as passing.
|
|||||||
| --- | --- | --- |
|
| --- | --- | --- |
|
||||||
| `tests/unit/ci-workflow-generation.test.ts` | 82 failed / 325 passed | Identical on the pre-change baseline (`git stash` comparison). The subprocess gates it spawns cannot run in this sandbox. |
|
| `tests/unit/ci-workflow-generation.test.ts` | 82 failed / 325 passed | Identical on the pre-change baseline (`git stash` comparison). The subprocess gates it spawns cannot run in this sandbox. |
|
||||||
| `tests/unit/ci-artifact-contract.test.ts` | fails | Unchanged pre-existing sandbox, cgroup and `/tmp` permission behaviour. |
|
| `tests/unit/ci-artifact-contract.test.ts` | fails | Unchanged pre-existing sandbox, cgroup and `/tmp` permission behaviour. |
|
||||||
| `tests/unit/security-followup.test.ts`, `tests/unit/provider-guardian-transaction.test.ts` | flaky under full-suite load | Both pass in a fresh process (49 passed). They spawn and reap process groups, so their timing assertions are load sensitive. |
|
| `tests/unit/security-followup.test.ts`, `tests/unit/provider-guardian-transaction.test.ts`, `tests/unit/risk-coverage.test.ts` | flaky under full-suite load | All three pass in a fresh process (78 passed together). They spawn and reap process groups, so their timing assertions are load sensitive. |
|
||||||
|
|
||||||
### Destructive fixture hazard — fixed
|
### Destructive fixture hazard — fixed
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user