carlos / platform Public

plan: draft 3 — close plan review 2

Four blockers, and one of them is this project repeating itself.

Task 5 breaks roughly twenty-five existing tests and named two. Draft 2
caught exactly that omission for task 4 — every fixture arming a scanner
with no deployment record fails once the doors read one — and then made
the same omission in the task it added to fix it. enableScanning and
scanArmed now write the record, the affected tests are listed, and the
plan says where the check goes in RunSweep, because putting it before
the Enabled early return changes what an existing test means.

My "RGW-gated" fix was wrong in a way that would have cost coverage.
store_rgw_test.go is not env-gated at all; it stands up an in-process
httptest fake, which is precisely the seam I claimed did not exist.
Gating the PageLister test would have left the drain's StartAfter and
MaxKeys path unexercised in CI — the one path that matters.

The queue core was ordered before the result object, so DrainApp would
have landed with nowhere to put an unrecorded verdict: the §5.2.1 bug
the result object exists to prevent, delivered by the plan meant to
prevent it. Result object moves to task 9, and the drain's two duties
around it get named tests.

Enqueue had been dropped from the queue task's Produces while the test
that needs it stayed, so nothing produced it.

Also: DrainApp had no described return and QueueIndex no enumeration or
depth, which would have had console reaching back for the mutex the
split exists to hide. The trash test was named "Skips" after the
sweep's behaviour when the drain must delete, and a test named skips
gets implemented as skips. And task 15's idempotence test could not have
passed: appconfig.Seal uses a fresh ephemeral key and nonce every call,
so the ciphertext always differs — rotation compares a recorded key id
instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 6c39a61992889efedef2c17e42445e58a439ff65 parent 0725cd3
1 file changed, +135 −61
  • docs/superpowers/plans/2026-09-02-scan-on-demand.md +135 −61
diff --git a/docs/superpowers/plans/2026-09-02-scan-on-demand.md b/docs/superpowers/plans/2026-09-02-scan-on-demand.md
index 1b0f0de73..61784beaf 100644
--- a/docs/superpowers/plans/2026-09-02-scan-on-demand.md
+++ b/docs/superpowers/plans/2026-09-02-scan-on-demand.md
@@ -1,6 +1,6 @@
# Scan on demand: implementation plan
-**Status:** draft 2 (closes plan review 1).
+**Status:** draft 3 (closes plan reviews 1 and 2).
**Spec:** `docs/superpowers/specs/2026-09-02-scan-on-demand-design.md` (draft 7,
cleared after six review rounds).
**Branch:** `worktree-store-scan-on-demand`, stacked on the landed scanner work.
@@ -27,7 +27,7 @@ Seventeen tasks, each its own commit, each TDD.
Three review rounds caught the same class of bug: **a new durable fact about an
object, introduced without carrying the object version across the seam.** The
ETag binding, then `Status` versus `Verdict`, then the result object. Tasks 7,
-11 and 13 carry the tests that exist because of it.
+9 and 13 carry the tests that exist because of it.
### Helpers that already exist — use them, do not invent
@@ -122,9 +122,12 @@ boot in practice. `ErrCASConflict` is success.
**This task breaks five existing tests and must fix them in the same commit:**
`TestScanPassSweepsOnlyEnabledStores`, `...HonoursReSweepInterval`,
`...ContinuesPastOneAppsFailure`, `...AlwaysContinuesAnInFlightSweep`,
-`...ResweepsAfterReattestationToPlaintext` (`scanpass_test.go:93-275`) all drive
-`scanPass` and expect a sweep. `enableStore` (`scanpass_test.go:44`) writes an
-enabled `system/antivirus.json`.
+`...ResweepsAfterReattestationToPlaintext` (`scanpass_test.go:93,126,165,196,241`)
+all drive `scanPass` and expect a sweep. `enableStore` (`scanpass_test.go:44`)
+writes an enabled `system/antivirus.json`. Give the record to
+`TestScanPassStillSkipsEncryptedStore` (`:275`) as well — it currently passes
+for the wrong reason, since `refusingLister` is never consulted whether or not
+encryption is the cause.
**Commit:** `console: antivirus policy moves to a record, wiring stays in env`
@@ -148,6 +151,27 @@ plus the comment sweep in `objstore.go:86`, `scanner.go:89`, `server.go:522-523`
`:353` both assert `strings.Contains(msg, "CARLOS_STORE_SCANNER")`.
- [ ] `apiscan.go:45`'s message names `carlos system antivirus enable`.
+**This task breaks roughly twenty-five existing tests, and every one of them
+is a fixture that arms a scanner with no deployment record.** Draft 2 caught
+this for task 4 and then repeated the omission in the task it added to fix it.
+Absent record = **not enabled**, so:
+
+- `enableScanning` (`teardown_scan_test.go:16`) writes an enabled
+ `system/antivirus.json`. It backs 17 `sweepFixture` calls and 20 `RunSweep`
+ calls in `sweep_test.go`; without the record every one errors at the new
+ refusal.
+- `scanArmed` (`apiscan_test.go:24`) writes one too. `/grant` is expected to
+ return 200 at `apiscan_test.go:105,168,210,283,330,343`, and
+ `TestScanGrantRefusedWithoutMemberRequest` (`:315`) expects 409 but would meet
+ the new 503 first.
+
+**Where the check goes in `RunSweep`:** replace the `sc == nil` arm at
+`sweep.go:162-165`, *after* the `Enabled == nil` early return at `:128` — put it
+before and `TestSweepIgnoresStoreWithoutScanning` changes meaning.
+
+Also reword `walk.go:466`, which still tells an operator to set
+`CARLOS_STORE_SCANNER` and is pinned by `teardown_scan_test.go:72`.
+
**Commit:** `console: disable actually disables`
---
@@ -200,7 +224,10 @@ is the person-only precedent.
**Gotchas.** `tag()` returns `nil` silently at `clamav.go:291,307`; it must
report whether it wrote or `recorded` is a lie. `Scan`'s denied path
(`clamav.go:203`) discards `tag`'s result — `recorded` is `false` there. The new
-return touches **17 call sites in `clamav_test.go` and 5 in the contract test**.
+return touches **12 call sites in `clamav_test.go` and one in the contract test**
+(`scanner_contract_test.go:33`) — and, unnamed in draft 2, **`goneScanner.Scan`
+(`sweep_test.go:481`)**, which overrides the old signature and must change in
+the same commit. It embeds `MemScanner` by value, so `Status` arrives for free.
`MemScanner.Verdict` (`memscanner.go:89-100`) does not apply the re-issue rule
today, so it gains it; `MemScanner` also gains `PutObject` and
`Unrecordable(key)`. `scanS3API` and `stubScanS3` gain `HeadObject`.
@@ -217,9 +244,14 @@ Large, and necessarily atomic — the signature change cannot be split.
- [ ] `TestDirStorePageListerOrdersAndBounds` — `ListMod` is already key-sorted
(`release/store.go:290`).
-- [ ] `TestS3StorePageLister` — **RGW-gated**, like every other S3Store test
- (`store_rgw_test.go:94,151,184`); `S3Store.Client` is a concrete
- `*awss3.Client` (`store.go:429-432`) with no seam.
+- [ ] `TestS3StorePageLister` — **against an `httptest` handler**, asserting
+ `start-after`, `max-keys` and the prefix on the `ListObjectsV2` query and
+ returning two pages. Draft 2 said "RGW-gated, like every other S3Store test";
+ that was wrong. `store_rgw_test.go:94,151,184` are **not** env-gated — they
+ stand up an in-process fake (`newRGWFake`, `:27`, then `NewS3Store`,
+ `:99-101`), which is exactly the seam draft 2 claimed did not exist. Gating
+ this test would leave the drain's `StartAfter`/`MaxKeys` path unexercised in
+ CI, which is the one path that matters here.
An **optional interface** type-asserted at the call site as `SizeLister`
(`release/store.go:181-185`) is at `economicspass.go:141` — not a method on
@@ -229,37 +261,99 @@ An **optional interface** type-asserted at the call site as `SizeLister`
---
-## Task 9: the queue core
+## Task 9: the result object
-**Files:** `internal/objstore/queue.go` (new) + test.
+**Moved ahead of the queue core.** Draft 2 had the queue core first, so
+`DrainApp` would have landed with nowhere to put an unrecorded verdict — the
+exact §5.2.1 bug the result object exists to prevent. Task 9 is independent of
+everything else; task 10 needs it.
+
+**Files:** `internal/objstore/scanresult.go` (new) + test.
+
+**Produces:** `WriteScanResult`, `ReadScanResult(ctx, s, acct, app, key,
+currentVersion)` — absent on version mismatch, so no caller compares versions
+itself.
+
+- [ ] `TestResultObjectIsVersionBound` — **review 5's blocker, the third
+ appearance of this bug class.** A result written for v1 must not answer for
+ v2. Unit-level: `ReadScanResult` with a different `currentVersion` is absent.
+- [ ] `TestResultObjectReplacesOnSecondWrite` — read-then-CAS. `Put` has only
+ create-only and replace (`release/store.go:37-45`); a create-only second write
+ returns `ErrCASConflict` and silently drops the v2 verdict.
+- [ ] `TestResultObjectNotWrittenUnderPurgedPrefix` — the `store.json` re-read
+ guard (`sweep.go:358-367`), using `memLister.before`. A stranded queue item is
+ drained next tick; a stranded **result** object is found by nothing, blocks the
+ claim gate (`server.go:2921`), and — because `materialize.go:333-345,377-382`
+ counts any key and restores a trashed row — **resurrects the app** on the next
+ boot.
+- [ ] `TestResultObjectCollectedByAge` — with a `ModLister`
+ (`release/store.go:161`) fallback that skips and logs once, as `storagePass`
+ does (`economicspass.go:141-145`).
+
+**Commit:** `objstore: the scan result object, version-bound and guarded`
+
+---
+
+## Task 10: the queue core
-**Produces:** `QueueKey`, `QueueItem`, `NewQueueID`, **exported**
-`QueueIndex{Bump, Observe, Release}`, and `DrainApp(ctx, s, sc, acct, app, cap)`.
+**Files:** `internal/objstore/queue.go` (new) + test.
-**The index and the drain core live in `objstore`, not `console`.** Draft 1 put
-an unexported `queueIndex` in objstore and used it from `console` — which does
-not compile. Mirror `RunSweep`/`scanPass`: the walk lives in objstore, the
-goroutine and ticker in console.
+**Produces:** `QueueKey`, `QueueItem`, `NewQueueID`, **`Enqueue(ctx, s, idx,
+acct, app, item)`** — which draft 2 dropped from Produces while keeping the test
+that needs it — plus:
+
+```go
+type QueueIndex struct{ … } // exported: console holds the goroutine
+func (i *QueueIndex) Bump(acct, app string)
+func (i *QueueIndex) Observe(acct, app string) uint64
+func (i *QueueIndex) Release(acct, app string, observed uint64, listedEmpty bool) bool
+func (i *QueueIndex) Apps() []AppRef // round-robin needs enumeration
+func (i *QueueIndex) Depth() map[AppRef]int
+
+type Drained struct {
+ Key, Version, ETag string
+ Verdict Verdict
+ Recorded bool
+ Engine, Signature string
+ CallbackPath string
+}
+func DrainApp(ctx, s, sc, idx, acct, app string, cap int) (done []Drained, listedEmpty bool, err error)
+```
+
+**The return type is not incidental.** Console needs per-item outcomes for the
+long-poll wake (13), the callbacks (16) and the per-tick usage fold (17), and it
+needs `listedEmpty` to call `Release` correctly (§7.1). Without `Apps()` and
+`Depth()` console reaches back for the mutex the split exists to hide.
- [ ] `TestQueueIDSortsByTime` — hand-rolled, no ULID dependency.
- [ ] `TestEnqueueWritesObjectBeforeBump` — a 202 between the two invites a
client to stop retrying an item nothing points at.
- [ ] `TestIndexEntrySurvivesEnqueueDuringDrain` — the generation counter.
- [ ] `TestIndexEntrySurvivesPartialDrain` — **the test that would have caught
- review 3's blocker.** More items than the cap: the entry stays, successive
- calls drain the rest.
+ review 3's blocker.** More items than the cap: the entry stays.
- [ ] `TestIndexEntryReleasedOnlyOnEmptyListing` — "not truncated" was vacuous;
- `release.Store.List` never truncates (`release/store.go:555-569`).
-- [ ] `TestDrainAppReChecksConsentChain`, `TestDrainAppSkipsTrashedApp`
- (`sweep.go:135-141`), `TestDrainAppReIssuesFailedAndDenied`,
- `TestDrainAppKeepsItemsOnReadError` (absence is `os.ErrNotExist`,
- `trash.go:64`, never error), `TestDrainAppDedupesByKey`.
+ `S3Store.ListMod` never truncates (`release/store.go:555-569`).
+- [ ] `TestDrainAppWritesResultWhenUnrecorded` and
+ `TestDrainAppDeletesResultWhenLaterRecorded` — §5.2.1's two drain duties,
+ which no task owned in draft 2.
+- [ ] `TestDrainAppDeletesItemsForTrashedApp` — **deletes, not skips.** Draft 2
+ named it "Skips" after the sweep's behaviour (`sweep.go:135-141`), but §5.1.1
+ and §7.5 have the drain *delete*; a test named "skips" gets implemented as
+ skips and the items sit for thirty days.
+- [ ] `TestDrainAppDeletesOnNotPermitted` — `not_granted`, `not_requested`,
+ `encrypted`, `not_enabled` — versus `TestDrainAppKeepsOnReversible` —
+ `disabled`, engine drift, unreachable engine, read error, each up to the TTL.
+ §7.5's two lists, separately, because "re-checks the consent chain" does not
+ say which side an item falls on.
+- [ ] `TestDrainAppDedupesByKey`.
+
+Absence is `os.ErrNotExist` (`trash.go:64`), never an error.
**Commit:** `objstore: the scan queue, its index and its drain`
---
-## Task 10: the enqueue door
+## Task 11: the enqueue door
**Files:** `internal/console/apiscan.go`, `authcli.go`,
`internal/console/scanhook.go` (new — the path validator only).
@@ -283,35 +377,6 @@ goroutine and ticker in console.
---
-## Task 11: the result object
-
-**Files:** `internal/objstore/scanresult.go` (new) + test.
-
-**Produces:** `WriteScanResult`, `ReadScanResult(ctx, s, acct, app, key,
-currentVersion)` — absent on version mismatch, so the door does no comparison
-of its own.
-
-- [ ] `TestResultObjectIsVersionBound` — **review 5's blocker, the third
- appearance of this bug class.** A result written for v1 must not answer for
- v2. Unit-level: `ReadScanResult` with a different `currentVersion` returns
- absent.
-- [ ] `TestResultObjectReplacesOnSecondWrite` — read-then-CAS. `Put` has only
- create-only and replace (`release/store.go:37-45`); a create-only second
- write returns `ErrCASConflict` and silently drops the v2 verdict.
-- [ ] `TestResultObjectNotWrittenUnderPurgedPrefix` — the `store.json` re-read
- guard (`sweep.go:358-367`), using `memLister.before`. A stranded queue item is
- drained next tick; a stranded **result** object is found by nothing, blocks
- the claim gate (`server.go:2921`), and — because `materialize.go:333-345,377-382`
- counts any key and restores a trashed row — **resurrects the app** on the next
- boot.
-- [ ] `TestResultObjectCollectedByAge` — with a `ModLister`
- (`release/store.go:161`) fallback that skips and logs once, as `storagePass`
- does (`economicspass.go:141-145`).
-
-**Commit:** `objstore: the scan result object, version-bound and guarded`
-
----
-
## Task 12: the drain goroutine and the boot rebuild
**The boot rebuild had no task in draft 1**, though the spec calls it "the only
@@ -333,7 +398,8 @@ test would have tested a collector nobody called.
- [ ] `TestDrainAgeSweepsResultObjects` — per visited app, so a console that
never restarts does not accumulate them.
- [ ] `TestQueueDepthReported` — §4.4 and §10 both want it; **it is delivered
- here**, and surfaced by tasks 6 and 17.
+ here** and surfaced in task 17. Task 6 lands before this and cannot surface
+ it.
Implements: 30s ticker, 50 items per app per tick, in-process notify for
waiters, deleting the result object when a later scan records a real verdict.
@@ -348,7 +414,7 @@ waiters, deleting the result object when a later scan records a real verdict.
- [ ] `TestStatusDoorReadsTagThenResultObject`.
- [ ] `TestStatusDoorRefusesStaleResultObject` — the door-level half of task
- 11's unit test.
+ 9's unit test.
- [ ] `TestStatusDoorReportsFailedAndDenied` — as themselves, never `pending`.
- [ ] `TestStatusDoorGoneForMissingObject`.
- [ ] `TestLongPollWakesOnDrain`, `TestLongPollCapsWaiters` — global **and**
@@ -383,8 +449,14 @@ hook), `apisystem.go`, root `system.go`, `authcli.go`.
- [ ] `TestGrantDeliversWebhookKey` — sealed via `appconfig.Apply`
(`generation.go:119`), gated by `storeSealKey` (`apistore.go:425`).
-- [ ] `TestRotateWebhookKeyIsIdempotent` — a re-run with an unchanged master
- re-seals identical plaintext and restarts nothing.
+- [ ] `TestRotateWebhookKeySkipsUnchangedApps` — **not "re-seals identical
+ plaintext".** `appconfig.Seal` uses a fresh ephemeral key and nonce on every
+ call (`internal/appconfig/crypto.go:85-87`), so the ciphertext always differs
+ and Sync would restart the unit every time; the console holds only the public
+ key and cannot compare by opening the old value. Rotation therefore records a
+ **key id** in the record — `ScanBlock` gains `webhook_key_id`, additive and
+ safe under the PARSER LIST rule — and skips any app already carrying the
+ current one. The test asserts the skip.
- [ ] `TestRotateWebhookKeyDoesNotDropPrevious` — dropping `_PREVIOUS` is a
separate operator step, because "done" means sealed, not converged
(`server.go:566-569` makes this point for store rotation).
@@ -446,9 +518,11 @@ First-tick warning when wired with no record → task 4. `request`/`revoke`/
## Sequencing
1, 2, 3 are independent. 4 needs 2 and 3. 5 needs 4. 7 and 8 are independent
-seams. 9 needs 7 and 8. 10 needs 9. 11 is independent. 12 needs 9, 11. 13 needs
-11, 12. 14 is pure. 15 needs 14. 16 needs 12, 14, 15. 17 last, so the docs
-describe what exists.
+seams. **9 (the result object) is independent and comes before 10 (the queue
+core), which needs 7, 8 and 9** — draft 2 had these the other way round, which
+would have landed a `DrainApp` that drops unrecorded verdicts. 11 needs 1, 2,
+10. 12 needs 10. 13 needs 7, 9, 12. 14 is pure. 15 needs 14. 16 needs 12, 14,
+15. 17 last, so the docs describe what exists.
**Docs hold until task 17.** The `carlos store scan` section drafted for the
skills repo describes hourly-sweep behaviour and is in copy review; it is