amadan
In a refused record each block that names no bucket is resolved; the eighth review's small items
The eighth reading, of the whole change, is the first neither reviewer rejected. Opus found one more shape of the omitted holder (P2): a refused record whose grant names a bucket and whose provisioned block names none counted only the named bucket. A refused record is now read as widely as it can be. A record a walk could act on is still read as the walks read it. The teardown test is pinned to mid-day and asserts the store is provisioned. Four comments and claims the reviewers found broader or narrower than the code are corrected.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+274
−16
docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review8.md+121 −0docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md+17 −1docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review8.md+93 −0implementation/carloku-paid-launch/OUTSTANDING.md+1 −1internal/console/economicspass.go+5 −3internal/console/economicspass_test.go+17 −3internal/objstore/walk.go+20 −8
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review8.md b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review8.md| new file mode 100644 |
| index 000000000..cc06dd88a |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review8.md |
| @@ -0,0 +1,121 @@ |
| +1. **P3 — One teardown regression still permits a hold caused by a failed reading.** |
| + `internal/console/economicspass_test.go:1549`, call at `:1566`, assertions at `:1578`, at `ade000f1`. |
| + |
| + `TestStoragePassDoesNotSizeAStoreUnderTeardown` still uses the real clock with date `2026-08-12`. It therefore runs in last-chance mode today. |
| + |
| + A regression that makes the pass’s own record read return a transient error would satisfy all its assertions: no sizing, `StoreUnswept:true`, and zero bytes. The test would pass without establishing that the teardown was recognized. At midday on the fixture date, that same failure would fail the pass. |
| + |
| + **Smallest acceptable fix:** call `storageMidDay(t, "2026-08-12")` in this test. Also asserting `StoreProvisioned` would distinguish the intended teardown result from the read-error result. |
| + |
| +No additional P1 or P2 found within the stated exclusions. This was source inspection of both requested diffs at `ade000f1`; no Go tests, mutation checks, or backend checks were run. |
| + |
| +**0. Holder inventory: the previous fault is closed across the requested record combinations.** |
| + |
| +The following tables compose exhaustively: choose the grant/provisioned row, apply the teardown rules, then apply the name/decoding rules. `Coherent` acceptance is determined by those fields; it is not an independent choice. |
| + |
| +Notation: `—` means an absent block; `empty` means a present block without a bucket; `G`, `P`, and `T` are explicit bucket names; `M` is the deployment’s minted name. |
| + |
| +| Grant | Provisioned | Bucket candidates before teardown | Grant/provisioned coherence | |
| +|---|---|---|---| |
| +| — | — | None | Accepts | |
| +| empty | — | `M` | Accepts | |
| +| `G` | — | `G` | Accepts | |
| +| — | empty | `M` | Accepts | |
| +| empty | empty | `M` | Accepts | |
| +| `G` | empty | `G` | Refuses: `ErrTwoBuckets` | |
| +| — | `P` | `P` | Accepts | |
| +| empty | `P` | `P` | Accepts | |
| +| `G` | `P`, with `G = P` | That bucket | Accepts | |
| +| `G` | `P`, with `G ≠ P` | Both `G` and `P` | Refuses: `ErrTwoBuckets` | |
| + |
| +This matches `bucketFor`’s preference for nonempty provisioned coordinates, then nonempty grant coordinates (`internal/objstore/walk.go:202`). For an unnamed estate, a future grant can instead receive an explicit adoption argument; that grant must open the selected bucket’s ledger before creating or delivering access (`:452`). The adoption argument is not evidence of an existing holding. |
| + |
| +Teardown and rotation use the provisioned coordinates when that block exists; teardown otherwise uses the grant coordinates (`:939`, `:1017`). An empty provisioned block beside a named grant is therefore correctly refused rather than treated as a coherent fallback. |
| + |
| +For teardown validation, `Coherent` computes its “own” bucket from nonempty provisioned coordinates, otherwise the grant’s bucket. **It does not substitute `M`.** |
| + |
| +| Teardown shape | Coherence and inventory result | |
| +|---|---| |
| +| Absent | Apply the grant/provisioned row above. | |
| +| Present, no deletion time and no bucket note | Same candidates remain counted. Starting teardown does not remove a storage holder. | |
| +| Present, no deletion time, note names the explicit own bucket | Accepts; the bucket remains counted. | |
| +| Present, no deletion time, note names another bucket | Refuses with `ErrUnfinishedTeardown`; every explicit name remains counted, including the note. An unnamed estate also contributes `M` or `Unresolved`. | |
| +| Deletion time and explicit own bucket named | If otherwise coherent and names are valid, counts no bucket: its own deletion is recorded. | |
| +| Deletion time and another bucket named | Refuses with `ErrUnfinishedTeardown`; counts every explicit name and any unnamed-estate candidate. | |
| +| Deletion time but no bucket named | Refuses with `ErrUnfinishedTeardown`; counts existing grant/provisioned names, or resolves the unnamed estate. | |
| + |
| +Two boundary cases matter: |
| + |
| +- If neither estate block names a bucket, a teardown note naming `M` still disagrees with `Coherent`’s empty own name. The record is refused and `M` remains counted; the deletion is not trusted. |
| +- If both estate blocks are absent, a named teardown contributes its named bucket through the refused-record branch. A teardown with only a deletion time names no bucket and has no grant/provisioned estate to resolve; it contributes nothing. There is no bucket-holding evidence omitted in that case. |
| + |
| +`ErrTwoBuckets` takes precedence when both coherence conditions fail. Either refusal reaches the same conservative inventory branch. |
| + |
| +| Account name | App name | Decoding | Inventory treatment | |
| +|---|---|---|---| |
| +| Valid | Valid | Fails | Literal key in `Unparsed`, regardless of other fields. | |
| +| Valid | Invalid | Fails | Literal key in `Unparsed`. | |
| +| Invalid | Valid | Fails | Literal key in `Unparsed`. | |
| +| Invalid | Invalid | Fails | Literal key in `Unparsed`. | |
| +| Valid | Valid | Succeeds | Apply coherence and teardown rules above. | |
| +| Valid | Invalid | Succeeds | Refused treatment regardless of coherence: count all explicit names; unnamed estate goes in `Unresolved`. | |
| +| Invalid | Valid | Succeeds | Same refused treatment. | |
| +| Invalid | Invalid | Succeeds | Same refused treatment. | |
| + |
| +For a decoded refused record, `ReadHolderInventory` counts the union of nonempty grant, provisioned, and teardown names (`internal/objstore/walk.go:1803`). If a grant or provisioned block exists but **neither names a bucket**, it additionally resolves the unnamed estate (`:1789`, `:1818`): |
| + |
| +- Valid names and a nonempty minted answer: count that answer. |
| +- Invalid account/app name, missing rule, or empty answer: append the literal key to `Unresolved`. |
| + |
| +A record can therefore contribute explicit bucket names **and** be unresolved. Teardown notes do not suppress this fallback. |
| + |
| +**There is no combination in this matrix where a bucket the record can hold is omitted while the record is in neither `Unparsed` nor `Unresolved`.** The sixth- and seventh-reading omission is closed as a class over these dimensions. |
| + |
| +**1. Attribution and hold scope** |
| + |
| +I found no additional path that records bytes against a potentially shared store within the stated review boundaries. |
| + |
| +`sizeSoleHolder` checks the inventory baseline, record holders, and open ledger intervals before sizing; rereads the store afterward; and compares the ledger generation again before assigning `StoreBytes` (`internal/console/economicspass.go:2486`, `:2501`, `:2521`, `:2529`, `:2536`). |
| + |
| +The ledger baseline precedes the record walk. New access through the current grant walk requires an ownership interval before provider operations. A visitor absent from the inventory consequently leaves either an open interval or a changed generation. |
| + |
| +Known-bucket uncertainty holds the measured store’s own account. Unparsed or unresolved records can concern any bucket, so the broader hold is justified. The statement completeness check derives the affected account from the day entry’s key (`:939`); it does not assign the uncertainty to a neighbouring account. |
| + |
| +**2. Existing callers outside the storage pass** |
| + |
| +- `Get` and `Inspect` now return decode failures with `ErrRecordMalformed` and changed diagnostic text (`internal/objstore/objstore.go:263`). The underlying decoder error remains reachable through `errors.Is`/`errors.As`. The wrapper now supports multiple wrapped errors; existing callers do not depend on single-error unwrapping or the former message. |
| +- Existing console doors, settings readers, scanning paths, queue checks, and store walks retain their error-handling decisions. |
| +- Grant’s take-back decision remains equivalent after extraction into `OtherHolders` (`internal/objstore/walk.go:303`, `:1626`). Coherent departed records are filtered; refused records remain; foreign open intervals block; same-account holders remain exempt at the caller. Deduplication does not change the decision. |
| +- `SameStore` itself is unchanged from the base. The new inventory and ledger helpers introduce no other existing caller changes. |
| + |
| +**3. Day objects and existing readers** |
| + |
| +The day remains version 1 with unchanged JSON fields (`internal/usage/storage.go:10`). `StoreUnswept` gains ownership uncertainty as another reason for an unmeasured store, while retaining its existing meaning to readers. |
| + |
| +The statement fold still adds recorded deployment and store bytes, and completeness excludes held storage days for the affected account. The economics view uses the same storage fold. Old and new days decode and fold together correctly. Reading them together does not repair old shared-bucket measurements. |
| + |
| +**4. Test strength and ordering** |
| + |
| +The three tests changed since `4ed6b71e` now pin midday. The during-sizing test also checks that sizing ran once. Its visitor closes its interval before the final read, so detecting that visitor requires the generation comparison rather than merely finding an open interval. |
| + |
| +The before-turn test selects whichever bucket has not yet been sized, avoiding a dependence on traversal order. Map-driven cases use fresh fixtures. The clock overrides are restored through cleanup, and these console tests do not run in parallel. |
| + |
| +The `nthGet` tests deliberately depend on per-key read order: inventory, pre-sizing read, post-sizing read. Removing the relevant reading prevents the injected failure and causes the expected failure/hold assertion to fail. The inventory-only ledger failure now isolates `LedgerUnread`. |
| + |
| +The remaining clock weakness is finding 1. I did not find another new test that demonstrably passes when the particular guard it claims to protect is removed. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **Minted-name coverage is described too broadly.** |
| + `internal/objstore/walk.go:1708` and `implementation/carloku-paid-launch/OUTSTANDING.md:68` describe a bucketless grant as holding the minted name. With `Grant.Bucket == ""` and `Provisioned.Bucket == X`, the inventory correctly counts only `X`. Qualify the claim with **“when neither the grant nor the provisioned block names a bucket.”** |
| + |
| +- **`Unresolved` is described too narrowly.** |
| + `internal/objstore/walk.go:1679` says it contains records carrying a grant and naming no bucket anywhere. A provisioned-only empty block also qualifies. A refused record can also name a teardown bucket while its unnamed estate remains unresolved. Describe it as an unresolved grant/provisioned estate, allowing explicit teardown names alongside it. |
| + |
| +- **The pass’s invalid-name comment conflicts with the new inventory.** |
| + `internal/console/economicspass.go:235` says an invalid prefix “cannot have a store record.” The inventory and regression fixtures explicitly recognize such records. State that this per-app reader skips invalid names, while the holder inventory reads their literal keys. |
| + |
| +- **The seventh disposition overstates the previous review’s conclusion.** |
| + `docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md:100` says both reviewers found no path recording bytes against another holder. Its next table records precisely such a path. Change this to **“no additional attribution path beyond the omitted-holder finding.”** |
| + |
| +**Verdict: accept with fixes.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md| index 282132b0c..d66b749ab 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| @@ -97,7 +97,7 @@ Six readings. The whole-diff reading found one more P1, fixed in the commit that |
| ## Review seven (the whole diff `c68ecddd..4ed6b71e`; Astra rejected, Opus accepted with fixes) |
| -Reports: `CODEX-FINDINGS-storage-sole-holder-review7.md`, `OPUS-REVIEW-storage-sole-holder-review7.md`. Both read the whole change again. Both found the sixth reading's P1 closed for a record a walk could act on, and both found the same gap beside it, which Astra rated P1 and Opus P3: the fix did not reach a record no walk may act on. Both again found no path that records bytes against a store with another holder or holds the wrong account, no behaviour change for callers outside the storage pass, and the day object unchanged. |
| +Reports: `CODEX-FINDINGS-storage-sole-holder-review7.md`, `OPUS-REVIEW-storage-sole-holder-review7.md`. Both read the whole change again. Both found the sixth reading's P1 closed for a record a walk could act on, and both found the same gap beside it, which Astra rated P1 and Opus P3: the fix did not reach a record no walk may act on. Both again found no further path that records bytes against a store with another holder, beyond the omitted holder in the table below, none that holds the wrong account, no behaviour change for callers outside the storage pass, and the day object unchanged. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| @@ -111,3 +111,19 @@ Reports: `CODEX-FINDINGS-storage-sole-holder-review7.md`, `OPUS-REVIEW-storage-s |
| Mutation checks in a scratch copy, each failing its test: a refused record's unnamed bucket not counted (the inventory test and both refused shapes of the storage pass test); a name minted for a name that is not an app's; the generation comparison before the sizing off; the comparison after the sizing off; a failed reading put in place of the comparison's hold (the during-sizing subtest now fails, where in last-hour mode it would have passed). |
| Seven readings. Each of the last two whole-diff readings found one more shape of the same fault, a holder the inventory left out. This one is fixed in the commit that adds this section and has not been read by anyone. This branch is not accepted, and it is not merged. |
| + |
| +## Review eight (the whole diff `c68ecddd..ade000f1`; both accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-storage-sole-holder-review8.md`, `OPUS-REVIEW-storage-sole-holder-review8.md`. The first reading neither reviewer rejected. Both were asked to enumerate every shape a store record can take and say which buckets the inventory counts it under. Astra found the fault closed as a class over those shapes. Opus found one shape more by the branch's own rule. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2: a refused record whose grant names a bucket and whose provisioned block names none counted only the named bucket. By the walks' own reading that is right; by this branch's rule (a refused record holds what it holds without naming) the unnamed block was left out. | Fixed. In a refused record each block that names no bucket is resolved on its own, to the minted name or to unresolved. A record a walk could act on is still read as the walks read it. | `objstore/walk.go` `ReadHolderInventory` | `TestHolderInventoryRules` (`refprov`), `TestStoragePassCountsAGrantThatNamesNoBucket` (the fourth shape) | |
| +| Astra P3: the teardown test ran in last-hour mode, where a failed reading would also have produced its hold | Fixed: pinned to mid-day, and it asserts the store is recorded as provisioned. | `economicspass_test.go` | `TestStoragePassDoesNotSizeAStoreUnderTeardown` | |
| +| Opus: `TestStoragePassDegradesOnStoreRecordReadError` exercises only the last-hour path | Said so in its comment. The mid-day behaviour is `TestStoragePassRetriesEachOfAStoresReadings`. | `economicspass_test.go` | none | |
| +| Both, claims: the `Unresolved` doc was narrower than the code; the minted-name claim was broader than the code; the pass's comment said an invalid prefix cannot have a store record; the seventh disposition said both reviewers found no attribution path | Corrected, each as the reviewer worded it. | `walk.go`, `economicspass.go`, this file | none | |
| + |
| +Opus ran tests in a scratch worktree of its own, against its instructions, and reported that it could not remove the worktree's metadata entry. Nothing of this branch was changed by it. |
| + |
| +Eight readings. This is the first that neither reviewer rejected. The fixes above are in the commit that adds this section and have not been read. The branch is not merged, and it still needs Paul's ruling on approval K. |
| + |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review8.md b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review8.md| new file mode 100644 |
| index 000000000..4b2b8d1b1 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review8.md |
| @@ -0,0 +1,93 @@ |
| +# Reading eight of `feat/carloku-storage-sole-holder` at `ade000f1`: accept with fixes |
| + |
| +The seventh reading's P1 is closed. One more record shape in the same class is still missing from the inventory: a refused record whose **provisioned** block names no bucket. It is P2, and the fix is one line. |
| + |
| +I read the whole diff `c68ecddd..ade000f1` and the delta from `4ed6b71e`. In a throwaway worktree I ran `go vet` and `go test` on `internal/objstore` and `internal/console` (the Storage, Holder, Statement and Unswept tests). All passed. I also ran one probe test, described in the P2 finding below. Nothing here was run against a real backend. |
| + |
| +## 0. Is the fault closed as a class? |
| + |
| +Not quite. |
| + |
| +**What a record can hold.** I checked this against `bucketFor` (`walk.go:202`), Teardown's coordinates (`walk.go:1023`), `storeHolders` (`walk.go:1360`) and `Coherent` (`walk.go:1569`). Coherent refuses a record for three reasons: |
| +- **R1:** the grant block names a bucket and the provisioned block names a different one, or none. |
| +- **R2:** the teardown note names a bucket that is not the record's own. |
| +- **R3:** the teardown notes a deletion and does not say of which bucket. |
| + |
| +| Record | What the walks say it holds | What the inventory does | |
| +|---|---|---| |
| +| Does not decode | unknown | `Unparsed`, so every store is held ✔ | |
| +| Account or app name invalid, block names a bucket | that bucket | counted as refused ✔ | |
| +| Account or app name invalid, grant or provisioned block names nothing | unknown | `Unresolved` ✔ | |
| +| Valid, coherent, no grant and no provisioned block (any teardown that stays coherent) | nothing (Teardown deletes such a record) | nothing ✔ | |
| +| Valid, coherent, provisioned names X (grant names X, nothing, or is absent), no teardown or teardown with no deletion | X | X ✔ | |
| +| Valid, coherent, grant names X, no provisioned block, no teardown or teardown with no deletion | X | X ✔ | |
| +| Valid, coherent, grant and/or provisioned present and both name nothing | the minted name (bucketFor) | minted ✔ | |
| +| Valid, coherent, teardown noted its own X deleted | nothing by the record | skipped ✔. The deleting store's ledger interval stays open until `DeleteUser`, and a regrant voids the note in the same write that moves the generation, so the ledger covers it. | |
| +| Refused R1, provisioned Y, grant X | X and Y | X and Y ✔ | |
| +| Refused R1, provisioned names nothing, grant X | X by bucketFor. By Coherent's own wording, also an unknown second bucket | **X only, and not in `Unresolved`** ✘ | |
| +| Refused R2, own bucket X or none, note names Y | X (or the minted name) and Y | X (or the minted name via `addUnnamed`) and Y ✔ | |
| +| Refused R3, any blocks | own bucket or the minted name | the same ✔ | |
| +| Refused R2 or R3, no grant and no provisioned block | the noted bucket, or nothing | the same ✔ | |
| + |
| +Measured against bucketFor alone, there is **no** combination left out. Measured against the branch's own rule there is one: |
| +- The `ReadHolderInventory` doc says a provisioned block that names no bucket "holds a bucket and does not say which". |
| +- The refused-path comment says such a record holds "the one it holds without naming". |
| +- Coherent says an empty provisioned name beside a grant that names X "is the same disagreement" as two buckets. |
| + |
| +## Findings |
| + |
| +**P2: a refused record's bucketless provisioned block is not resolved** (`internal/objstore/walk.go:1797–1800` and `1808–1823`) |
| +- **Scenario:** `bcd/old` is `{grant: {bucket: carlos-store-adopted}, provisioned: {bucket: "", key_id: K}}`. Coherent refuses it with `ErrTwoBuckets`. |
| +- **What happens:** `unnamed` is false because the grant names a bucket. The inventory adds only `carlos-store-adopted`. Another account's store provisioned on `MintedBucket(bcd, old)`, with no ledger interval for `bcd/old`, is sized as the sole holder. |
| +- **Probe:** a test at `ade000f1` found 0 holders on the minted name, 1 on the adopted name, and `Unresolved=[]`. |
| +- **Why P2 and not P1:** the only writer, `walk.go:587`, always sets `Bucket` (since `675e3eac`). So this record can only be a hand edit, the same provenance as the bucketless grant that was fixed. bucketFor itself resolves this record to X. |
| +- **Smallest fix:** in the refused branch, resolve each bucketless block on its own: `if (rec.Provisioned != nil && rec.Provisioned.Bucket == "") || (rec.Grant != nil && rec.Grant.Bucket == "") { addUnnamed(true) }`. Add this shape to `TestHolderInventoryRules` and to `TestStoragePassCountsAGrantThatNamesNoBucket`. |
| + |
| +**P3: the `Unresolved` field's doc is narrower than the code** (`walk.go:1681–1687`) |
| +- It says "records that carry a grant block and name no bucket anywhere". |
| +- The code also lists records that have only a bucketless provisioned block. |
| +- It also lists records under invalid names that do name a bucket elsewhere, such as a teardown note. Those are counted under that bucket *and* listed as unresolved. |
| +- **Fix:** reword to "a grant or provisioned block that names no bucket, which the minted-name rule could not resolve". |
| + |
| +## 1. Bytes recorded against a possible co-holder, or a hold on the wrong account |
| + |
| +None found beyond the P2 above. |
| +- **Bytes:** a grant opens its ledger interval before it delivers a credential, so the generation checks before and after the sizing see grants that land after the inventory. That includes a grant over a bucketless record with an adopted name: its interval opens before line 503 writes the name. |
| +- **Inventory order:** ledgers are read first, then records, and a grant writes its grant block before it opens the interval. |
| +- **Holds:** every hold is on the store's own entry. The only exception is `holdEveryStore` (on `Unparsed`, on `Unresolved`, or on an inventory that failed in the last hour), which 19i accepts. |
| + |
| +## 2. Callers outside the storage pass |
| + |
| +No behaviour change found. |
| +- **`Get` and `Inspect`:** decode errors now wrap both `ErrRecordMalformed` and the JSON error, so `errors.Is` and `errors.As` still work. Only the message text changed, and no caller matches on it or on `json.SyntaxError`. |
| +- **`OtherHolders` in Grant:** it matches the old take-back check. |
| + - The grant validates the bucket name before this check, so the new `ValidBucketName` guard never fires there. |
| + - A holder that has left is still filtered only when it is not refused. |
| + - The dedupe and the narrower exclusion of the asking app (now account and app, then the caller skips the same account) do not change whether a holder of another account exists. |
| +- **`storageClock`** is a package global, and no console test uses `t.Parallel`. |
| + |
| +## 3. The day object |
| + |
| +- **Shape:** unchanged: `V:1`, same fields. |
| +- **Meaning:** `StoreUnswept` now also means "not shown to be sole holder". `StorageExcluded` and `SourceDays` already fold that per account. |
| +- **Mixed builds:** days from the old and new builds decode together. Old days on a shared bucket read as complete, which 19i states. |
| + |
| +## 4. Tests |
| + |
| +- **Unpinned tests that call `storagePass` with a past date:** |
| + - `TestStoragePassMarksEmptyStoreProvisioned` |
| + - `TestStoragePassDegradesOnStoreRecordReadError` |
| + - `TestStoragePassDoesNotSizeAStoreUnderTeardown` |
| + |
| + With a past date `storageLastChance` is always true, so these are deterministic (not wall-clock dependent). They exercise only the last-hour path. |
| +- **`TestStoragePassDegradesOnStoreRecordReadError`:** its hold is the last-hour fallback. It would fail mid-day, so its name overstates what it covers. |
| +- Every test whose claim depends on a hold happening mid-day is now pinned. I found no other test that would pass without the code it names. |
| + |
| +## Claims the code contradicts |
| + |
| +- `walk.go:1709–1714` (doc), `walk.go:1808–1811` (comment) and `OUTSTANDING.md:68` (19i): "a record nothing may act on holds… the one it holds without naming". This is false for the R1 shape with a bucketless provisioned block (P2). 19i speaks only of grant blocks, so it is accurate as written, but it is silent on this shape. |
| +- `walk.go:1681–1687`: the `Unresolved` doc (P3). |
| + |
| +**Verdict: accept with fixes.** Add the P2 shape before calling the class closed. |
| + |
| +The scratch worktree's files are deleted. Its metadata entry, `.git/worktrees/ss`, could not be removed from inside the sandbox ("Device or resource busy"). Run `git worktree prune` outside the sandbox to clear it. My `prune` also tried to remove another entry, `.git/worktrees/platform`, and that failed too, so nothing of anyone else's was deleted. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index 2a925ce08..ccd1c9d44 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -65,7 +65,7 @@ Each is concrete. None is waived. |
| 19. **No review of the whole branch has passed.** Astra rejected rounds one to sixteen; Opus accepted each with fixes. Round seventeen read only the diff since round sixteen: Opus accepted, Astra accepted with fixes (one P2, fixed since and unconfirmed). That is not a pass of the branch: the last whole-branch reading was round sixteen's, and it was rejected. Every round has closed the shapes the round before reported and found neighbouring ones, in the store walks since round seven. Some concern records no walk of this build writes; others need only this build's ordinary walks (a failed rotation, a retained teardown, a scanning door racing a teardown), and two were faults in the previous round's own fixes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings were about records no walk of this build writes (a teardown note for another bucket, a record naming two buckets) being acted on by teardown, rotation and the scanning doors; a rotation started after a grant that failed at its last write; and a take-back that returned access to a bucket another account holds, through the member's own door. Round thirteen read those fixes: closed for their reported shapes, Astra rejected again (two P1, four P2) and Opus accepted with fixes (one P2, five P3). The new findings were paths that read such a record and acted without the check (the holder inventory, the storage pass, the declare door), a rotation's own failed last write, and a retained record blocking another account's take-back. The check now sits in the record's reader and writer. Round fourteen read those: closed for their reported shapes; Astra rejected (two P1, two P2), Opus accepted with fixes (two P2, two P3). Round fifteen read those: all closed; Astra rejected (one P1, one P2), Opus accepted with fixes (one P2, two P3), all about readers of a bucket that a retained teardown left to another account (a sweep in flight, the scan status, the storage pass). Round sixteen read those: all closed, and one of them had made a single store under teardown hold storage for every account. Astra rejected (one P1, two P2), Opus accepted with fixes (one P1, one P2, two P3). The storage hold is now per account; two findings are left open as 19f and 19i. Those fixes are on the launch branch and unconfirmed until the next round reads them. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| 19g. **Refusals whose doors say the wrong thing, and records only a person can repair.** A record that names two buckets (`ErrTwoBuckets`) or whose teardown note is about another bucket (`ErrUnfinishedTeardown`) is refused by the record's one reader and one writer, so nothing acts on it and nothing writes it; the settings page shows such a store as unreadable. No walk of this build writes either; whether an earlier build left such records in production has not been checked. There is no operator verb to repair one. The doors that meet these refusals answer with sentences written for other failures: the grant and provision doors say to run the grant again, the destroy door and the purge say to try again, the scanning doors say to try again or to re-run the grant. The same goes for a rotation refused because a grant must be run again first (`ErrGrantUnfinished`) and for a member's take-back refused because another account holds the bucket (`ErrBucketHeld`). Correct copy for each needs approval (A). |
| -19i. **A live bucket shared by two stores was sized in full for each of them.** Fixed on this branch (`feat/carloku-storage-sole-holder-20261001`), not on the launch branch: the daily storage pass sizes a store only when it is the sole holder of its bucket, by one inventory of the store records per pass and the bucket's ownership ledger, read before and after the sizing. Otherwise the store is held for its own account. Holds that last until a person acts: a store that left the bucket keeps its record, and holds the remaining store, until its retained teardown is finished, which nothing runs (19a); a record no walk can follow, until it is repaired by hand (19g); another store's grant that died after writing its grant block, until it is run again and the sharing it then creates is itself resolved, or, for a grant that was refused and whose undo write failed, until the block is removed by hand; a record under an account or app name no build writes, which no walk can touch, until it is removed by hand; an open ledger interval of a store whose record is gone, until the ledger is reconciled by hand; two apps of one account on one bucket, always; any store record that will not parse holds every store. Whose bytes a departed store's leftover objects are needs Paul's ruling (approval K). Not covered: one bucket adopted in two deployments is invisible to each and is sized in full in both. The walks' own holder scan still skips records under names no build writes. A store on a bucket name this build keeps no ledger for is held, for good: its ownership evidence cannot be checked, and nothing short of moving the store to a valid name changes that. A ledger that will not parse holds its bucket's stores until it is repaired by hand. A grant block written after the inventory's walk, by a grant whose ledger write then failed, is not seen; that store has no credential, so the bytes sized are the bucket's own store's. A reading the store fails to return (a record, a ledger, a bucket listing) fails the pass for a retry until the day's last hour (23:00 UTC), when the day is written with the affected stores held; a pass that keeps failing repeats every listing and sizing each hour until then, and a reading that never succeeds (a bucket deleted out of band, a ledger the console may not read) does that every day. Open: the pass runs hourly, so the last hour has about one tick, and a failure of the listings it starts with, of the day's own write, or a pass that runs past that tick, still leaves the day unwritten, which holds storage for every account; it needs a short retry timer after 23:00. A grant block that names no bucket counts as holding the name the deployment mints for it, in a record a walk could act on and in one it refuses; no build is known to have written one, so it is a hand edit. Under a name that is not an account's or an app's it cannot be resolved, and holds every store. Storage days written before every console runs this build are not billable for any store on a shared bucket: the previous build sized such a bucket in full for each, and those days read as complete. The same goes for days taken while any console still runs the previous build: it grants an adopted bucket without touching a ledger, so a pass on this build cannot see that grant. Reviewed seven times (`DISPOSITIONS-storage-sole-holder.md`): the fifth reading, of a diff, was accepted with fixes by both reviewers; the sixth and seventh, each of the whole change, were accepted with fixes by Opus and rejected by Astra on one P1 each time, both a holder the inventory left out. The seventh's is fixed since and unread. |
| +19i. **A live bucket shared by two stores was sized in full for each of them.** Fixed on this branch (`feat/carloku-storage-sole-holder-20261001`), not on the launch branch: the daily storage pass sizes a store only when it is the sole holder of its bucket, by one inventory of the store records per pass and the bucket's ownership ledger, read before and after the sizing. Otherwise the store is held for its own account. Holds that last until a person acts: a store that left the bucket keeps its record, and holds the remaining store, until its retained teardown is finished, which nothing runs (19a); a record no walk can follow, until it is repaired by hand (19g); another store's grant that died after writing its grant block, until it is run again and the sharing it then creates is itself resolved, or, for a grant that was refused and whose undo write failed, until the block is removed by hand; a record under an account or app name no build writes, which no walk can touch, until it is removed by hand; an open ledger interval of a store whose record is gone, until the ledger is reconciled by hand; two apps of one account on one bucket, always; any store record that will not parse holds every store. Whose bytes a departed store's leftover objects are needs Paul's ruling (approval K). Not covered: one bucket adopted in two deployments is invisible to each and is sized in full in both. The walks' own holder scan still skips records under names no build writes. A store on a bucket name this build keeps no ledger for is held, for good: its ownership evidence cannot be checked, and nothing short of moving the store to a valid name changes that. A ledger that will not parse holds its bucket's stores until it is repaired by hand. A grant block written after the inventory's walk, by a grant whose ledger write then failed, is not seen; that store has no credential, so the bytes sized are the bucket's own store's. A reading the store fails to return (a record, a ledger, a bucket listing) fails the pass for a retry until the day's last hour (23:00 UTC), when the day is written with the affected stores held; a pass that keeps failing repeats every listing and sizing each hour until then, and a reading that never succeeds (a bucket deleted out of band, a ledger the console may not read) does that every day. Open: the pass runs hourly, so the last hour has about one tick, and a failure of the listings it starts with, of the day's own write, or a pass that runs past that tick, still leaves the day unwritten, which holds storage for every account; it needs a short retry timer after 23:00. A record whose grant and provisioned blocks name no bucket between them counts as holding the name the deployment mints for it; in a record no walk may act on, each block that names none is counted that way as well; no build is known to have written one, so it is a hand edit. Under a name that is not an account's or an app's it cannot be resolved, and holds every store. Storage days written before every console runs this build are not billable for any store on a shared bucket: the previous build sized such a bucket in full for each, and those days read as complete. The same goes for days taken while any console still runs the previous build: it grants an adopted bucket without touching a ledger, so a pass on this build cannot see that grant. Reviewed eight times (`DISPOSITIONS-storage-sole-holder.md`): the sixth and seventh readings, each of the whole change, were accepted with fixes by Opus and rejected by Astra on one P1 each time, both a holder the inventory left out. The eighth, again of the whole change, was accepted with fixes by both: Astra found that fault closed as a class and one P3; Opus found one more shape of it (P2). Those fixes are in and unread. |
| 19h. **A rotation whose delivery fails is repaired by a grant, which swaps the app's key with no overlap.** A failed delivery may still land, so the record stays marked, the next rotation is refused, and the grant run again replaces whichever key the app holds. Hosts hold a dead key until they adopt the new config. Before this branch such a rotation was resumed by running it again, which kept the overlap when the delivery had not landed and broke the app when it had. A repair that waits out the signing window and then reads which key was delivered would keep the overlap where the window is enforced (observed on AWS only, 19d); it is not built. |
| 19f. **A store that leaves a bucket with an unsupported name does not hand on its scanner debt.** There is no ledger for such a name. The last holder would delete the bucket and leave a scanning plan behind. Today's scanner (ClamAV) has no plan to leave, so this is latent. Also latent for the same reason: the scan grant door checks that no teardown has begun and then enables scanning as two steps, so a retained teardown landing between them would leave scanning enabled on the other account's bucket with nothing recording that it is owed. And the scan queue's drain checks permission once per batch, so a drain admitted before a retained teardown could return the other account's object metadata; nothing calls the drain or enqueues today, and it must be fixed before the queue is wired. |
| 19e. **A purge can end with a store that was granted during it.** A declare and a grant admitted while the app was live can land between the purge's last check and the removal of its marker. The purge now reports that as an error and never deletes the record, but it does not prevent it: the doors check liveness when they admit a request, not when they write. And after that error the estate is named only by its record and a log line. The app's console row is still trashed, so the settings page and the store-destroy door answer 404 and a second purge answers "not trashed", until the console restarts and restores the row from the record. The flash the member sees says "Nothing was deleted", which is false in this case; correct copy needs approval (A). |
diff --git a/internal/console/economicspass.go b/internal/console/economicspass.go| index be17afbbe..ffdb42e62 100644 |
| --- a/internal/console/economicspass.go |
| +++ b/internal/console/economicspass.go |
| @@ -233,9 +233,11 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| st.BucketBytes += ks.Size |
| } |
| // A prefix that is not an account's app (a legacy unqualified |
| - // one, say) cannot have a store record: no build writes one |
| - // under an invalid name. That is "no store", not a store that |
| - // could not be measured. |
| + // one, say) is not read for a store record here: no build |
| + // writes one under an invalid name, and for this prefix that |
| + // is "no store", not a store that could not be measured. The |
| + // holder inventory does read such a key, literally, because a |
| + // record somebody put there by hand can still name a bucket. |
| var rec *objstore.Record |
| var gerr error |
| if release.ValidAccount(acct) && release.ValidApp(app) { |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 8c7b9cfa8..6d0832d48 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -331,6 +331,10 @@ func TestStoragePassMarksUnreachableStoreUnswept(t *testing.T) { |
| } |
| } |
| +// (This runs with a past date, so in last-hour mode: the hold it asserts is |
| +// the last hour's. Mid-day the same failure fails the pass for a retry, |
| +// which TestStoragePassRetriesEachOfAStoresReadings covers.) |
| +// |
| // TestStoragePassDegradesOnStoreRecordReadError: objstore.Get's "absent" |
| // return is (nil, "", nil) — a non-nil error means a REAL failure (bucket |
| // read, malformed store.json), not "no store". A pass that treated the two |
| @@ -1549,6 +1553,8 @@ func TestStoragePassDoesNotMeasureARefusedRecord(t *testing.T) { |
| func TestStoragePassDoesNotSizeAStoreUnderTeardown(t *testing.T) { |
| fx := newConsoleFixture(t) |
| ctx := context.Background() |
| + // Mid-day: a failed reading would fail the pass, not hold the store. |
| + storageMidDay(t, "2026-08-12") |
| at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| rec := &objstore.Record{ |
| Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-shared-x", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| @@ -1575,8 +1581,8 @@ func TestStoragePassDoesNotSizeAStoreUnderTeardown(t *testing.T) { |
| t.Fatal(err) |
| } |
| app := day.Apps[fixtureAcct+"/"+fixtureApp] |
| - if sized != 0 || !app.StoreUnswept || app.StoreBytes != 0 { |
| - t.Fatalf("sized %d time(s), day entry %+v; want no sizing and an unswept store", sized, app) |
| + if sized != 0 || !app.StoreUnswept || !app.StoreProvisioned || app.StoreBytes != 0 { |
| + t.Fatalf("sized %d time(s), day entry %+v; want no sizing and a provisioned, unswept store", sized, app) |
| } |
| } |
| @@ -2232,6 +2238,9 @@ func TestHolderInventoryRules(t *testing.T) { |
| "refnote": {Grant: unnamedGrant(), Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at, DeletedBucket: "carlos-store-elsewhere"}}, |
| "refblank": {Grant: unnamedGrant(), Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at}}, |
| } |
| + // And a grant that names a bucket beside a provisioned block that |
| + // names none: refused as two buckets, and the unnamed one is resolved. |
| + refused["refprov"] = &objstore.Record{Grant: grant("carlos-store-adopted"), Provisioned: &objstore.Provisioned{Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}} |
| for app, rec := range refused { |
| if objstore.Coherent(rec, "bcd", app) == nil { |
| t.Fatalf("fixture: %s is a record a walk could act on; this case needs one it refuses", app) |
| @@ -2244,7 +2253,7 @@ func TestHolderInventoryRules(t *testing.T) { |
| t.Fatalf("fixture: %q is a valid app name", badApp) |
| } |
| put("bcd", badApp, &objstore.Record{Grant: unnamedGrant()}) |
| - if bare, err := objstore.ReadHolderInventory(ctx, fx.rel, nil); err != nil || len(bare.Unresolved) != 4 { |
| + if bare, err := objstore.ReadHolderInventory(ctx, fx.rel, nil); err != nil || len(bare.Unresolved) != 5 { |
| t.Fatalf("with no minted-name rule: %+v %v; want every record with an unnamed bucket listed as unresolved", bare, err) |
| } |
| inv, err := objstore.ReadHolderInventory(ctx, fx.rel, func(acct, app string) string { return "carlos-store-minted-" + acct + "-" + app }) |
| @@ -2260,6 +2269,8 @@ func TestHolderInventoryRules(t *testing.T) { |
| "carlos-store-minted-bcd-refnote": 1, |
| "carlos-store-elsewhere": 1, |
| "carlos-store-minted-bcd-refblank": 1, |
| + "carlos-store-adopted": 1, |
| + "carlos-store-minted-bcd-refprov": 1, |
| } { |
| if got := len(inv.Others(bucket, "bab", "asking")); got != want { |
| t.Errorf("%s: %d holder(s), want %d", bucket, got, want) |
| @@ -2581,6 +2592,9 @@ func TestStoragePassCountsAGrantThatNamesNoBucket(t *testing.T) { |
| Grant: unnamed(), Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at, DeletedBucket: "carlos-somewhere-else"}}, |
| "with a teardown note that says deleted and not what": { |
| Grant: unnamed(), Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at}}, |
| + "with a grant that names another bucket and a provisioned block that names none": { |
| + Grant: &objstore.GrantBlock{Backend: "mem", Bucket: "carlos-somewhere-adopted", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}}, |
| } { |
| t.Run(name, func(t *testing.T) { |
| fx := newConsoleFixture(t) |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index 693394da6..6fab29caa 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -1676,10 +1676,13 @@ type HolderInventory struct { |
| ledgerUnread map[string]bool |
| // Unparsed lists the keys of store records that will not parse. |
| Unparsed []string |
| - // Unresolved lists the keys of records that carry a grant block and |
| - // name no bucket anywhere, which the caller's minted-name rule could |
| - // not resolve either. Such a grant may have created a bucket and |
| - // delivered a credential for it, so, as with an unparsed record, no |
| + // Unresolved lists the keys of records with a grant or provisioned |
| + // block that names no bucket, which the caller's minted-name rule |
| + // could not resolve either (no rule, no answer, or an account or app |
| + // name the rule is not for). Such a record may also name buckets |
| + // elsewhere, in a teardown note say, and is counted under those as |
| + // well. What its unnamed block stands for may be a bucket with a |
| + // credential delivered for it, so, as with an unparsed record, no |
| // store can be shown to be the only holder of anything while one |
| // exists. |
| Unresolved []string |
| @@ -1705,9 +1708,12 @@ func (inv *HolderInventory) LedgerUnread(bucket string) bool { |
| // ReadHolderInventory walks apps/<account>/<app>/store.json. |
| // |
| -// minted gives the bucket name the deployment mints for an app. A grant or |
| -// provisioned block that names no bucket holds a bucket and does not say |
| -// which, and minted resolves it. No build is known to have written one |
| +// minted gives the bucket name the deployment mints for an app. A record |
| +// whose grant and provisioned blocks name no bucket between them holds a |
| +// bucket and does not say which, and minted resolves it. (In a record a |
| +// walk could act on, a block that names none beside one that does is read |
| +// as the walks read it: the named bucket. In a refused record, each |
| +// unnamed block is resolved as well.) No build is known to have written one |
| // (every grant walk records the bucket before anything else), so such a |
| // record is a hand edit; it is counted all the same, whether or not the |
| // record is otherwise one a walk could act on. With no rule (nil), an |
| @@ -1815,7 +1821,13 @@ func ReadHolderInventory(ctx context.Context, s release.Store, minted func(acct, |
| if rec.Teardown != nil { |
| add(rec.Teardown.DeletedBucket, true) |
| } |
| - if unnamed { |
| + // Each block that names no bucket, taken on its own: beside |
| + // a block that does name one, an empty name is itself why |
| + // the record is refused, and what it stands for is not |
| + // known. A record a walk could act on is read as the walks |
| + // read it (one bucket); a refused one is read as widely as |
| + // it can be. |
| + if unnamed || (rec.Provisioned != nil && rec.Provisioned.Bucket == "") || (rec.Grant != nil && rec.Grant.Bucket == "") { |
| addUnnamed(true) |
| } |
| continue |