amadan
plan: draft 4 — cleared to implement from
Review 3's verdict was yes, subject to three under-specifications in the seam task 10 exports and one misdirected error message. The message first, because it is the kind of thing that ships. I had pointed scanNoEngine — which fires only when the Scanner is nil, a wiring state — at the fix for a disabled record. An operator whose record is already enabled would have been told to run enable, and the two 503s would have read the same, which is precisely what TestScanStatusReportsDisabledDistinctFromUnreachable exists to prevent. Wiring names CARLOS_CLAMD_ADDR; policy names the command. Drained could not feed the meter: §8 counts bytes scanned on demand and the struct had no size, and an item answered from an existing tag was scanned by the sweep — which the metering test says must not be billed to the tenant — with no way to tell the two apart. Size and Scanned added. DrainApp now owns Observe and Release outright. Draft 3 had it take the index and also return listedEmpty "so console can call Release", which would have needed console to read Observe before the listing — an ordering it cannot see, and exactly the ordering the generation counter protects. Also: a third fixture arms a scanner by hand rather than through either helper, so task 5's list was off by one; grant must stamp webhook_key_id or the first rotation restarts every app for nothing; the deployment-wide queue depth had no owner after task 6 was correctly ruled out; and the reorder left one stale cross-reference.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+44
−15
docs/superpowers/plans/2026-09-02-scan-on-demand.md+44 −15
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 61784beaf..a0dd8036a 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 3 (closes plan reviews 1 and 2). |
| +**Status:** draft 4 — cleared to implement from (closes plan reviews 1-3). |
| **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. |
| @@ -147,9 +147,17 @@ plus the comment sweep in `objstore.go:86`, `scanner.go:89`, `server.go:522-523` |
| - [ ] `TestRunSweepRefusesWhenDeploymentDisabled` — `sweep.go:163` still reads |
| `if sc == nil … (set CARLOS_STORE_SCANNER)`. |
| - [ ] `TestScanStatusReportsDisabledDistinctFromUnreachable` (`apiscan.go:71-81`). |
| -- [ ] Update the two tests pinning the old wording: `apiscan_test.go:84` and |
| - `:353` both assert `strings.Contains(msg, "CARLOS_STORE_SCANNER")`. |
| -- [ ] `apiscan.go:45`'s message names `carlos system antivirus enable`. |
| +- [ ] **Keep the two 503s distinct.** `scanNoEngine` (`apiscan.go:43-46`) fires |
| + only when `StoreScanner == nil` — a *wiring* state — so it names |
| + **`CARLOS_CLAMD_ADDR`**. The new refusal, for a record that says disabled, |
| + names **`carlos system antivirus enable`**. Draft 3 pointed the first at the |
| + second's fix, which would tell an operator whose record is already enabled to |
| + enable it, and would collapse the two states |
| + `TestScanStatusReportsDisabledDistinctFromUnreachable` exists to separate. |
| +- [ ] Update the two tests pinning the old wording — `apiscan_test.go:85` and |
| + `:353` — to assert `CARLOS_CLAMD_ADDR`. Both run under `scanArmed`, which |
| + after this task writes an enabled record and then nils the scanner: "enabled, |
| + not wired", not "disabled". |
| **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 |
| @@ -161,9 +169,13 @@ Absent record = **not enabled**, so: |
| 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 |
| + return 200 at `apiscan_test.go:105,168,283,330,343`, and |
| `TestScanGrantRefusedWithoutMemberRequest` (`:315`) expects 409 but would meet |
| the new 503 first. |
| +- **`TestScanEnforceWritesTheDenyToThePolicy` (`apiscan_test.go:204`) is the |
| + third fixture**, and uses neither helper: it arms via `storeArmed` plus a |
| + hand-set `cfg.StoreScanner` at `:207`, then expects `/grant` 200 at `:210`. |
| + Switch it to `scanArmed` or give it its own record. |
| **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 |
| @@ -313,7 +325,9 @@ func (i *QueueIndex) Depth() map[AppRef]int |
| type Drained struct { |
| Key, Version, ETag string |
| Verdict Verdict |
| - Recorded bool |
| + Recorded bool // the tag was written |
| + Scanned bool // Scan actually ran, vs answered from a tag |
| + Size int64 |
| Engine, Signature string |
| CallbackPath string |
| } |
| @@ -321,9 +335,18 @@ func DrainApp(ctx, s, sc, idx, acct, app string, cap int) (done []Drained, liste |
| ``` |
| **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. |
| +long-poll wake (13), the callbacks (16) and the per-tick usage fold (17). |
| +`Size` and `Scanned` are there for the meter specifically: §8 counts *bytes* |
| +scanned on demand, and an item answered from an existing tag was scanned by the |
| +**sweep**, which `TestScanUsageCountsOnDemandOnly` says must not be attributed |
| +to the tenant. Without `Scanned` the drain cannot tell the meter which is which. |
| + |
| +**`DrainApp` owns `Observe` and `Release`.** It takes `idx` and calls both; |
| +`listedEmpty` is returned for the caller's information, not as an instruction. |
| +The alternative — console calling them — requires reading `Observe` *before* the |
| +listing, an ordering console cannot see, and that ordering is exactly what |
| +§7.1's generation counter protects. 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 |
| @@ -380,8 +403,8 @@ Absence is `os.ErrNotExist` (`trash.go:64`), never an error. |
| ## 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 |
| -thing that collects a stranded item, so it is not optional" and task 11's age |
| -test would have tested a collector nobody called. |
| +thing that collects a stranded item, so it is not optional" and task 9's |
| +`TestResultObjectCollectedByAge` would have tested a collector nobody called. |
| **Files:** `internal/console/scandrain.go` (new) + test. |
| @@ -448,7 +471,10 @@ app `^[a-z][a-z0-9-]{0,31}$` (`release.go:242`). |
| hook), `apisystem.go`, root `system.go`, `authcli.go`. |
| - [ ] `TestGrantDeliversWebhookKey` — sealed via `appconfig.Apply` |
| - (`generation.go:119`), gated by `storeSealKey` (`apistore.go:425`). |
| + (`generation.go:119`), gated by `storeSealKey` (`apistore.go:425`). **Grant |
| + stamps `webhook_key_id` as well as rotation does**, or the first |
| + `rotate-webhook-key` after any grant re-seals and restarts every app under an |
| + unchanged master. |
| - [ ] `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 |
| @@ -488,8 +514,9 @@ hook), `apisystem.go`, root `system.go`, `authcli.go`. |
| ## Task 17: metering, surfaces and docs |
| **Files:** `internal/usage/scan.go` + **`scan_test.go` (new)**, |
| -`internal/console/scandrain.go` (the per-tick fold), `apiscan.go`, root |
| -`scan.go` and `store.go`, `docs/site/cli.md`, |
| +`internal/console/scandrain.go` (the per-tick fold), `apiscan.go`, |
| +**`apisystem.go`**, root `scan.go`, **root `system.go`**, root `store.go`, |
| +`docs/site/cli.md`, |
| `docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`. |
| - [ ] `TestScanUsageCountsOnDemandOnly` — sweep scans are not attributed to a |
| @@ -498,7 +525,9 @@ hook), `apisystem.go`, root `system.go`, `authcli.go`. |
| - [ ] `TestStoreScanStatusReportsDeploymentState` — a per-app record saying |
| `enabled: clamav` while the deployment switch is off is a new way for the |
| status output to lie. |
| -- [ ] `TestPrintScanStatusShowsQueueDepth`. |
| +- [ ] `TestPrintScanStatusShowsQueueDepth` (per app) and |
| + **`TestPrintSystemAntivirusStatusShowsQueueDepth`** (deployment-wide, §4.4). |
| + Task 6 lands before the drain exists and cannot surface either. |
| `usage/scan/<date>.json`, CAS-merged as `UpsertEdgeDay` (`edge.go:85-116`); |
| meter names frozen once merged (`usage.go:4-5`). Rollout plan step 4 becomes |