amadan
Record the evidence door's first review: Astra rejects, Opus accepts with fixes; nothing fixed yet
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 files changed,
+218
−0
docs/superpowers/specs/CODEX-FINDINGS-bucket-hour-evidence-review1.md+71 −0docs/superpowers/specs/DISPOSITIONS-bucket-hour-evidence.md+20 −0docs/superpowers/specs/OPUS-REVIEW-bucket-hour-evidence-review1.md+127 −0
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-bucket-hour-evidence-review1.md b/docs/superpowers/specs/CODEX-FINDINGS-bucket-hour-evidence-review1.md| new file mode 100644 |
| index 000000000..9d4855df8 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-bucket-hour-evidence-review1.md |
| @@ -0,0 +1,71 @@ |
| +1. **P1 — Hand-written evidence can authorize billing and erase a withdrawal.** |
| + `internal/objstore/hourevidence.go:99–122,139–146,189–194`; `internal/usage/evidence.go:185–202`. |
| + |
| + A writer with access to `usage/bucket-evidence/<bucket>.json` can supply a well-formed entry naming the ledger’s eligible owner, invent `by`, and release the hour without using an operator door. Validation checks the current document’s shape, not its provenance or history. Removing withdrawal fields also passes. |
| + |
| + After a month closes, withdrawing evidence changes its digest as intended. But restoring the **exact frozen bytes** removes that withdrawal and passes `InputEvidence.Verify`: verification checks current content, not intervening changes. This requires raw storage access; it is not available through the three HTTP doors. |
| + |
| + **Smallest acceptable fix:** verify authenticated operator events against an independently protected, append-only history, including withdrawals. Signing the mutable document alone would not prevent replay of an earlier signed version. |
| + |
| +2. **P2 — The evidence requirement accepts a waiver in practice.** |
| + `internal/objstore/hourevidence.go:267–291`; `internal/console/billingoperator.go:379–404`. |
| + |
| + An operator can submit `"evidence":"bill it anyway"`. Nonempty text, capped at 2,000 bytes, satisfies the entire evidentiary requirement. The same operator creates the immediately standing entry; there is no provider artifact, coverage check, or independent approval. |
| + |
| + The code constrains **which hour and owner** can be asserted. Whether the provider’s history actually excludes an earlier owner remains entirely the person’s responsibility. A deleted-and-recreated bucket before the completeness probe is precisely the history this text is supposed to substantiate, but nothing checks that it does. |
| + |
| + **Smallest acceptable fix:** require a retained, immutable evidence artifact identifying the bucket and covered interval, plus a checked finding before the entry becomes standing. If that check is human, record a distinct reviewer’s approval. More required prose fields alone would retain the waiver. |
| + |
| +3. **P2 — A withdrawal retry can retract a replacement finding.** |
| + `internal/objstore/hourevidence.go:224–255,304–312`. |
| + |
| + Operator A starts withdrawing entry E1. Operator B withdraws E1 and records corrected evidence E2. A’s conditional write conflicts; its callback reruns against the latest document and withdraws **E2**, using A’s reason about E1. The API identifies only the bucket-hour, so it cannot distinguish the finding A inspected from its replacement. |
| + |
| + Ordinary concurrent records behave correctly: identical findings become no-ops; different findings conflict; retries preserve existing entries. |
| + |
| + **Smallest acceptable fix:** require an immutable entry identifier or expected entry digest on withdrawal and return a conflict if that entry has been replaced. |
| + |
| +4. **P2 — Successful changes can lack an audit event, while no-ops produce one.** |
| + `internal/console/billingoperator.go:400–404,435–453`; `internal/console/server.go:3316–3319`. |
| + |
| + Withdrawal’s preliminary evidence read or account lookup can fail, yet the subsequent mutation can succeed. With `acct == nil`, the handler skips auditing and returns success. Both mutation handlers also use `s.audit`, which logs database failures without preserving a retry obligation. |
| + |
| + Conversely, an identical recording by another operator returns success without changing the evidence, but writes a new `billing-bucket-evidence` audit event under that operator’s address. The evidence still names the original author. Audit detail contains only bucket and hour, so it does not identify the particular finding or distinguish this no-op. |
| + |
| + The evidence object retains actor metadata on normal writes, but that does not establish the claimed reliable console audit trail. |
| + |
| + **Smallest acceptable fix:** return the committed entry and a changed/no-op result from the mutation; derive audit attribution from that result and durably record an audit event or retry obligation with the change. |
| + |
| +5. **P3 — Raw-byte pinning rejects harmless record rewrites.** |
| + `internal/usage/evidence_month.go:33–40`; `internal/usage/evidence.go:79–94,201–202`. |
| + |
| + A legitimate reserialization that changes indentation or JSON field order while preserving every entry and withdrawal makes a correctly closed month fail verification. Bucket evidence has no semantic projection, so its raw digest changes. |
| + |
| + There is **no ordinary later-month append through this API** while the ledger’s completeness instant remains fixed: only its original partial hour is eligible. A later withdrawal of that hour should invalidate the month; that is not a false positive. |
| + |
| + **Smallest acceptable fix:** hash a validated canonical representation, retaining all relevant evidence and withdrawal fields. Any future month projection must preserve changes affecting the charged hour. This addresses harmless rewrites, not the rollback problem in finding 1. |
| + |
| +6. **P3 — The append-only test does not exercise the append-only guard.** |
| + `internal/objstore/hourevidence_test.go:138–188`; `internal/objstore/hourevidence.go:244–246`. |
| + |
| + Removing the call to `evidenceAppendOnly` leaves this test’s assertions satisfied: the public callbacks already preserve old entries. No new test injects a conditional-write conflict, so the replacement-withdrawal race is also untested. |
| + |
| + **Smallest acceptable fix:** test adversarial callbacks that remove or rewrite entries and reverse withdrawals, and inject deterministic conditional-write conflicts. The append-only tests must fail when the guard is removed. |
| + |
| +The remaining attribution checks hold in the actual fold: another hour, another owner/app, missing completeness, two different apps touching the hour, and a standing withdrawal cannot release it. Both documents are loaded and checked against the same bucket key. Equivalent timezone offsets do not change the accepted UTC hour. A changed ledger introducing a second holder blocks the old evidence. Normal deletion/recreation preserves the original completeness instant; it does not create another evidence-eligible hour. These checks do not establish the provenance of a hand-written record. |
| + |
| +Recording after month close is allowed, but it does not rewrite a closed statement or silently replace its pinned evidence. A withdrawal racing a fold is detected if present when closure verifies inputs (`internal/console/economicspass.go:1588–1594`); persisted changes are checked again by `BillingSnapshot` (`internal/usage/statement.go:231–236`). |
| + |
| +All three doors require a valid person CLI token and the global `users.operator` bit. Operators have fleet-wide access; there is no account-scoped operator restriction to bypass. Responses do not disclose the actual alternative owner. Successful complete folds log bucket names, hours, and operator addresses (`internal/console/storeegressfold.go:187,225`); error paths also log bucket names. No tests or network commands were run. |
| + |
| +**Claims the code contradicts** |
| + |
| +- `internal/objstore/hourevidence.go:31`: “cannot be used as [a waiver]” overstates a nonempty-text check. |
| +- `internal/objstore/hourevidence.go:42`: “entries are append-only” describes this writer’s behavior, not a property established when reading stored history. |
| +- `internal/objstore/hourevidence_test.go:190–191`: hand edits the door would not write are not universally rejected; well-shaped forged authorship, rewritten history, and removed withdrawals pass. |
| +- `internal/console/bucketevidence_test.go:167–168`: “each change leaves an audit entry” is not guaranteed by the handlers. |
| +- `internal/console/billingoperator.go:155–156`: “Everyone else gets the same 404” is false for missing, expired, revoked, or service credentials, which receive 401. |
| +- `internal/console/bucketevidence_test.go:180`: “nothing written” means no evidence object; valid-token requests update `cli_tokens.last_used` before operator authorization (`internal/console/authstore.go:545–547`). |
| +- `internal/console/storeegressfold.go:73–75`: evidenced hours are returned in `Evidenced`, but a held month’s diagnostic omits them; only complete folds emit that list. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-bucket-hour-evidence.md b/docs/superpowers/specs/DISPOSITIONS-bucket-hour-evidence.md| new file mode 100644 |
| index 000000000..a925775b9 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/DISPOSITIONS-bucket-hour-evidence.md |
| @@ -0,0 +1,20 @@ |
| +# Bucket-hour evidence door: review dispositions |
| + |
| +Branch `feat/carloku-bucket-hour-evidence-20261001`. Nothing on the launch branch calls this code. A store's partial first hour still holds by default. |
| + |
| +## Review one (reviewed `58c1870c`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-bucket-hour-evidence-review1.md`, `OPUS-REVIEW-bucket-hour-evidence-review1.md`. |
| + |
| +Both confirmed what the door is built to guarantee: through the fold, no charged bucket-hour is attributed on this record for another hour, another owner or app, a ledger with no completeness claim, an hour two apps share, or a withdrawn entry; equivalent time zone offsets do not change the hour; a ledger that later shows a second holder stops the entry answering. Nothing below is fixed yet. |
| + |
| +| Finding | Disposition | |
| +| --- | --- | |
| +| Both, P2: the evidence is free text. "bill it anyway" satisfies it, the same operator records it and it stands at once. The code constrains which hour and which owner; whether the provider's history really excludes an earlier holder is left wholly to the person. In practice that is a waiver. | Accepted. This is the question the door exists to answer, and it needs a ruling from Paul before more code: what counts as evidence (a retained artifact naming the bucket and the interval covered) and whether a second person must approve it before it stands. Until then the door is not fit to release anything. Open. | |
| +| Astra P1: someone with raw write access to the deployment bucket can hand-write a well-formed entry, or restore the exact bytes of a withdrawn-from record after a month closed | Accepted. The same access can rewrite the ownership ledgers themselves. It needs an append-only history the console cannot rewrite (object versioning or lock on the prefix, or signed events), which is infrastructure. Open. | |
| +| Opus P2-1: a withdrawal after a month closed on the entry makes every account's statement for that month fail verification, not only the one account's store transfer, and the door neither warns nor refuses | Accepted. Open. The withdraw door should refuse, naming the month, unless the request acknowledges it. | |
| +| Astra P2: a withdrawal retried after a CAS conflict can withdraw a replacement finding recorded meanwhile, because it names only the bucket-hour | Accepted. Open. Withdrawal should name the entry it read. | |
| +| Astra P2, Opus P3-1, P3-2: a withdrawal can succeed with no audit row; re-recording the same finding is audited as a change and answered `recorded: true` | Accepted. Open. | |
| +| Astra P3, Opus P2-3: the record is pinned by its raw digest, so a harmless reserialization reads as changed evidence, and a rollback to the pinned bytes does not | Accepted. Open. Pin a canonical form; the rollback needs the history above. | |
| +| Astra P3, Opus P3-6: the append-only test passes with the guard removed; no test injects a CAS conflict | Accepted. Open. | |
| +| Opus P3-3, P3-4, P3-5: a hand-written record passes whatever its `By` and `At`; logs carry operator addresses and bucket names; the list door can show a held hour the record door will refuse | Accepted. Open. | |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-bucket-hour-evidence-review1.md b/docs/superpowers/specs/OPUS-REVIEW-bucket-hour-evidence-review1.md| new file mode 100644 |
| index 000000000..7ea37bd7a |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-bucket-hour-evidence-review1.md |
| @@ -0,0 +1,127 @@ |
| +# Review: `feat/carloku-bucket-hour-evidence-20261001` @ 58c1870c (base d5705a63) |
| + |
| +**Verdict: accept with fixes.** I found no way to attribute a charged bucket-hour beyond what the claims allow. Both of these are rechecked at every fold against the ledger as it then stands: |
| + |
| +- **The hour.** It must be the one a complete ledger began vouching in, with that start strictly inside the hour. |
| +- **The owner.** Every interval touching the hour must name that one account and app. |
| + |
| +The CAS is create-only or If-Match, so no write is lost or duplicated. Charged lines must cover whole hours, which rules out truncation and time-zone errors. The doors reach operators only. |
| + |
| +The real problems are four: |
| + |
| +- Once a month is closed, a withdrawal blocks billing for every account that month, not just the one account. |
| +- Any non-empty sentence is accepted as evidence. |
| +- A pinned digest misreads corrections and cannot catch a rollback. |
| +- The audit trail has gaps. |
| + |
| +## Findings |
| + |
| +### P2-1. A withdrawal after close blocks the whole month's billing for every account |
| +**Where:** `internal/console/billingoperator.go:417-466`, `internal/usage/evidence_month.go:33-41`, `internal/usage/statement.go:231-235` |
| + |
| +- `usage.SaveEvidence` stores one evidence snapshot per month, shared by every account's statement. |
| +- `Statement.BillingSnapshot` calls `evidence.Verify` on that whole snapshot. |
| +- The evidence record has no month projection, so it is pinned by its raw digest. |
| + |
| +**Scenario:** September closes, and one bucket's held hour was released on evidence. On 10-06 an operator withdraws the entry because they found a mistake. From then on, `BillingSnapshot` returns `ErrEvidenceChanged` for every account's September statement and every meter, not just the one account's `store_egress_gb`. Closed statements are never rewritten, and the door neither warns nor refuses. |
| + |
| +**Smallest fix:** |
| +- The withdraw door looks for a closed statement month that pins this key. If one exists, refuse with 409 naming the month, unless the request carries an explicit `reopens_month` acknowledgement. |
| +- Document that a withdrawal after close is a fleet-wide hold for that month. |
| + |
| +### P2-2. The evidence text is free text, so in practice this is a waiver |
| +**Where:** `internal/objstore/hourevidence.go:268-271`, `:105-108` |
| + |
| +**What the code enforces:** |
| +- the caller is an operator; |
| +- the text is non-empty after trimming and at most 2000 bytes; |
| +- the owner named is the one the ledger names; |
| +- only the one held hour can be released. |
| + |
| +**What it leaves to the person:** |
| +- The text can be anything: `"ok"` or `"."` releases the hour. |
| +- There is no second reviewer. One operator can record, withdraw and re-record with nobody else involved. |
| +- Nothing ties the evidence to the window that needs covering, from the top of the hour to `CompleteSince`. |
| + |
| +A bucket deleted and recreated is not a new door: `CompleteSince` is write-once and the ledger is never deleted, so each bucket has at most one releasable hour. An entry recorded after the month closed changes nothing for that month, because the month cannot close while the line is held. It only matters through P2-1. |
| + |
| +**Smallest fix:** require structured fields alongside the free text: |
| +- `source` (for example `cloudtrail:<region>`); |
| +- `window_from` and `window_to`; |
| +- `event_ids`, or an explicit `none`. |
| + |
| +Validate that `window_from <= hour` and `window_to >= CompleteSince`, at the door and in `validate`. Whether a second operator must confirm is the supervisor's call; today, nothing makes this more than a one-person attestation. |
| + |
| +### P2-3. The raw-digest pin flags a legitimate correction and misses an illegitimate rollback |
| +**Where:** `internal/usage/evidence.go:169-205`, `internal/usage/evidence_month.go:33-41` |
| + |
| +**Legitimate change that reads as changed:** after close, an operator fixes a typo or adds an event id. They withdraw and re-record the same finding (same hour, same account and app). The attribution is unchanged, yet the record now has two entries, so the digest differs and P2-1 follows. |
| + |
| +**Illegitimate change that does not:** |
| +1. An operator withdraws after close. Verify fails, as intended. |
| +2. Someone with bucket write access puts the exact earlier bytes back. |
| +3. Verify passes again, and the withdrawal and its note are gone from the record. |
| + |
| +The digest cannot see a rollback. The only remaining trace is the console audit row, which itself may be missing (P3-1). |
| + |
| +**Smallest fix:** add an `evidence_hour_month` projection that pins only `{bucket, hour, standing entry's account/app}` for hours inside the month. A withdrawal still changes the projection; a re-record of the same finding does not. To catch rollbacks, record S3 version IDs (with versioning on that prefix), or accept the gap in writing. |
| + |
| +### P3-1. A withdrawal can persist with no audit row |
| +**Where:** `internal/console/billingoperator.go:436-452`, `internal/objstore/hourevidence.go:304-316` |
| + |
| +- **Retried PUT.** The SDK retries a PUT whose first attempt actually committed. The retry gets a 412, so `writeBucketEvidence` re-reads, finds no standing entry, and returns `ErrEvidenceNotFound`. The withdrawal is persisted, but the operator sees 404 and no audit row is written. |
| +- **No account.** If the entry's account no longer resolves (`AccountBySqid` fails, or the entry was hand-written), the withdrawal succeeds with no audit row. |
| + |
| +**Fix:** |
| +- In the retry loop, treat "the entry at this hour is already withdrawn by `by` with this note" as success. |
| +- When no account resolves, audit against the operator or ops account instead of skipping. |
| + |
| +### P3-2. Re-recording the same finding returns `recorded: true` and audits a change that did not happen |
| +**Where:** `internal/objstore/hourevidence.go:235-236`, `internal/console/billingoperator.go:401-404` |
| + |
| +**Scenario:** operator B posts the same text as A's standing entry. The record still says `By: A`, but the audit says B recorded it. Two audit rows now exist for one entry. |
| + |
| +**Fix:** return a distinct sentinel, or an `unchanged` bool. Answer `recorded: false, unchanged: true` and skip the audit. |
| + |
| +### P3-3. A hand-written record passes validation whatever its `By` and `At` |
| +**Where:** `internal/objstore/hourevidence.go:99-123` |
| + |
| +`validate` never checks that `At` is at or after `hour + 1h`, or after `CompleteSince`, or that `By` looks like an address. The fold then reports a forged `By` exactly as it reports a real operator. |
| + |
| +The trust is the same as for the ledger: anyone who can write `usage/` can forge either. Still, confirm in the infra repo that box roles cannot `PutObject` under `usage/bucket-evidence/`. Boxes do write `usage/edge/` and `usage/durability/`, so a `usage/*` grant would also cover this prefix. |
| + |
| +**Fix:** reject `At` earlier than `Hour + 1h`, and fix the infra if the grant is too wide. |
| + |
| +### P3-4. Logs and audit expose operator addresses and bucket names |
| +**Where:** `internal/console/storeegressfold.go:187`, `:225`, `internal/console/billingoperator.go:403` |
| + |
| +- Every complete fold logs `"<bucket> <hour> by <operator email>"` to the console log. |
| +- The audit row goes on the customer account's own event log, with the operator's address and bucket name. That is the customer's own bucket, so nothing crosses accounts. Still, decide on purpose whether customers should see operator addresses. |
| + |
| +No error message leaks another account's data: failures return generic 4xx/500 text. |
| + |
| +### P3-5. The list door can advertise a `held_hour` the record door will refuse |
| +**Where:** `internal/console/billingoperator.go:356-361` |
| + |
| +`held_hour` is computed from `CompleteSince` alone. If two apps share that hour, the list offers it and the record door then answers 409. |
| + |
| +**Fix:** compute it via an exported `firstHourOwner`, and report the owner it names. |
| + |
| +### P3-6. Gaps in the tests |
| +None of the tests passes vacuously. I checked each guard (`CompleteSince` nil, on-the-hour, owner match, standing) against its test. Missing coverage: |
| + |
| +- **Paths with no test:** |
| + - P3-1, the withdrawal retry after a committed PUT; |
| + - P3-2, the repeated record's duplicate audit; |
| + - a fold-level shared-hour case; |
| + - a door-level "another account" case. |
| +- **The pin is only tested directly.** `TestStatementClosedOnEvidencePinsIt` checks `ev.Verify` itself. It never checks `BillingSnapshot` for a different account, which would have exposed P2-1's blast radius. |
| + |
| +## Claims the code contradicts |
| +- **`hourevidence.go:31`, "It is not a waiver and it cannot be used as one."** Any non-empty sentence releases the hour (P2-2). |
| +- **`hourevidence.go:319-323`, "a month that does not rest on evidence never reads, or pins, the record."** A month with a held first-hour line reads and pins the record even when the record is missing, withdrawn or names the wrong owner. This is harmless, because such a month cannot close, but the sentence is false as written. |
| +- **`hourevidence.go:265-266`, "Recording the same finding again changes nothing", and the test comment "each change leaves an audit entry".** The door audits a non-change and reports `recorded: true` (P3-2). |
| +- **The branch claim "each change is audited."** A withdrawal can persist with no audit row (P3-1). |
| +- **`billingoperator.go:356`, "The hour evidence can be recorded for".** The door can advertise an hour it will refuse (P3-5). |
| + |
| +**Verdict: accept with fixes.** Before any month closes on evidence, fix P2-1 and P2-3 (a projection pin, plus a withdraw door that will not silently block a closed month's billing). P2-2 needs a ruling from the supervisor, whatever code follows. |