amadan
plan: draft 2 — close plan review 1
Three blockers, fourteen tasks becomes seventeen. The worst: spec §4.2 items 3-6 landed in no task, so `disable` did not disable. An operator would run it, watch the tick go quiet, and `carlos store scan sweep` would still read tenant bytes — RunSweep's own refusal still reads "set CARLOS_STORE_SCANNER". That is now task 5, with its own commit, named for what it fixes. The boot rebuild had no task at all, though the spec calls it the only thing that collects a stranded item. Task 11's age test would have been testing a collector nobody called. And the queue index was unexported in objstore while console needed it, so tasks 8, 9 and 11 could not have compiled. The index and the drain core move into objstore behind an exported QueueIndex and DrainApp, mirroring RunSweep/scanPass: the walk in objstore, the goroutine and ticker in console. Task 9's tests then exercise real code rather than describing behaviour that lands two tasks later. Ordering fixes: clamd's TCP support now precedes the wiring that accepts a host:port address, or the address is parsed as a unix path for one commit. Task 4 breaks five existing scanpass tests and now says so, and fixes them in the same commit. Two smaller things worth the split. callback_path was accepted in the enqueue task and validated four commits later, so the commits between stored hostile paths; validation moves to the door that accepts it. And the webhook task was three tasks pretending to be one — signing, delivery-on-grant, and drain wiring each have their own tests and their own failure modes. Also: there is no ULID in this repo and the plan bans new dependencies, which draft 1 did not notice. Queue ids are hand-rolled with a test that they sort by time.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+368
−326
docs/superpowers/plans/2026-09-02-scan-on-demand.md+368 −326
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 60576e301..1b0f0de73 100644 |
| --- a/docs/superpowers/plans/2026-09-02-scan-on-demand.md |
| +++ b/docs/superpowers/plans/2026-09-02-scan-on-demand.md |
| @@ -1,11 +1,11 @@ |
| # Scan on demand: implementation plan |
| +**Status:** draft 2 (closes plan review 1). |
| **Spec:** `docs/superpowers/specs/2026-09-02-scan-on-demand-design.md` (draft 7, |
| -cleared to plan from after six review rounds). |
| +cleared after six review rounds). |
| **Branch:** `worktree-store-scan-on-demand`, stacked on the landed scanner work. |
| -Fourteen tasks, each its own commit, each TDD: write the failing tests, watch |
| -them fail for the right reason, implement, watch them pass. |
| +Seventeen tasks, each its own commit, each TDD. |
| --- |
| @@ -13,401 +13,443 @@ them fail for the right reason, implement, watch them pass. |
| - `go build ./...`, `go vet ./...`, `gofmt -l .` clean before every commit. |
| - **No new third-party dependencies.** Signing is `crypto/hmac` + `crypto/sha256`. |
| + **There is no ULID library in this repo and none is being added** — queue ids |
| + are hand-rolled: a big-endian millisecond timestamp plus random bytes, |
| + base32-encoded, with a test that they sort by time. |
| - Prefix Go commands with `GOCACHE=/home/paulca/.cache/tmp/claude-1001/gocache`. |
| - Five root-package tests (`TestAccountsList*`, `TestResolveMode*`) fail on this |
| - machine for an environmental reason confirmed at `8103270`: the checkout sits |
| - under `/home/paulca`, whose real `~/.carlos/config` `pick()` finds by walking |
| - ancestors. Not a regression; do not chase them. |
| -- **`s3:PutObjectTagging` must never enter the app's policy.** Unchanged. |
| -- `grant` and `provisioned` in `store.json` stay frozen — `release.PurgeApp` |
| - parses them with its own struct. |
| + machine for an environmental reason confirmed at `8103270`. Not a regression. |
| +- **`s3:PutObjectTagging` must never enter the app's policy.** |
| +- `grant` and `provisioned` in `store.json` stay frozen. |
| ### The mistake this design keeps making |
| -Three separate 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. Every task below that writes or reads a fact about an object must be |
| -checked for it explicitly, and the tests named in tasks 6, 10 and 11 exist |
| -because of it. |
| +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. |
| + |
| +### Helpers that already exist — use them, do not invent |
| + |
| +`storeArmed` (`apistore_test.go:505`), `scanArmed` (`apiscan_test.go:24`), |
| +`putRecord`/`storedRecord` (`apistore_test.go:60,48`), `cliToken` |
| +(`authcli_test.go:474`), `newConsoleFixture` (`server_test.go:44`), |
| +`sweepFixture`/`memLister`/`memLister.before` (`sweep_test.go:88,19,206`), |
| +`stubScanS3`/`fakeEngine`/`taggingOutput` (`clamav_test.go:24,55,264`), |
| +`rotateFixture`/`testSealKeypair`/`testDeployment` (`walk_test.go:573,20,52`), |
| +`closedDoors` (`svcroutes_test.go:83`), **`serviceToken` |
| +(`svcfixture_test.go:14`)** for the role tests, **`brokenStore` |
| +(`apiledger_test.go:658`)** for read-error tests, `refusingLister`/`withLister` |
| +(`scanpass_test.go:36,64`). |
| --- |
| ## Task 1: `svcrole.Service`, and per-app scope |
| **Files:** `internal/svcrole/svcrole.go`, `internal/console/svcprincipal.go`, |
| -`internal/console/apiservices.go`, `svcpage.go`, `services.go` (repo root). |
| -**Tests:** `internal/svcrole/svcrole_test.go`, |
| -`internal/console/apiservices_test.go`. |
| - |
| -**Produces:** `svcrole.Service`; `ErrAccountScopedService`. |
| - |
| -- [ ] **Failing tests** |
| - - `TestServiceRoleIsMintable` — `svcrole.Parse("service")` round-trips and |
| - `All()` includes it, so the mint door and form pick it up with no further |
| - change. |
| - - `TestServiceCredentialCannotBeAccountScoped` — `NewServicePrincipal` with |
| - `app == ""` and role `Service` returns `ErrAccountScopedService`; the same |
| - call with any other role still succeeds. **In `NewServicePrincipal`, not a |
| - handler:** there are two mint doors (`apiservices.go:211`, `svcpage.go:299`) |
| - and a handler-level check leaves one of them open. |
| - - `TestServiceRoleDoesNotWidenInstance` — `Instance` still reaches exactly |
| - its nine pinned doors. `TestInstanceRoleMatchesTodaysAppTokenReach` |
| - (`svcroutes_test.go:262-291`) already asserts this; confirm it still passes |
| - rather than writing a second one. |
| -- [ ] **Run:** `go test ./internal/svcrole/ ./internal/console/ -run 'Service|Instance' -count=1` — FAIL, undefined. |
| -- [ ] **Implement.** Add the constant and the catalogue entry; update |
| - `svcrole.go:41-44`'s "union of the three above" comment; add the sentinel and |
| - the check; add `service` to `services.go:164`'s hardcoded usage list. |
| - `apiservices.go:211-215` matches only `ErrServiceNameTaken` today, so the new |
| - sentinel needs its own 400 arm in both doors. |
| -- [ ] **Commit:** `svcrole: a Service role for apps calling platform services` |
| +`apiservices.go`, `svcpage.go`, root `services.go`. |
| +**Tests:** `svcrole_test.go`, `apiservices_test.go`. |
| + |
| +- [ ] `TestServiceRoleIsMintable` — `Parse` round-trips, `All()` includes it, so |
| + the mint door and the form pick it up unchanged. |
| +- [ ] `TestServiceCredentialCannotBeAccountScoped` — in `NewServicePrincipal` |
| + (`svcprincipal.go:170-172`), **not** a handler: two mint doors exist |
| + (`apiservices.go:211`, `svcpage.go:299`) and a handler check leaves one open. |
| + New sentinel `ErrAccountScopedService`, with its own 400 arm in both — |
| + `apiservices.go:211-215` matches only `ErrServiceNameTaken` today. |
| +- [ ] Confirm `TestInstanceRoleMatchesTodaysAppTokenReach` |
| + (`svcroutes_test.go:262-291`) still passes rather than writing a second one. |
| + |
| +**Gotcha:** `services.go` hardcodes the role list **three times** (`:33`, `:157`, |
| +`:164`). |
| + |
| +**Commit:** `svcrole: a Service role for apps calling platform services` |
| --- |
| ## Task 2: the deployment settings record |
| -**Files:** `internal/release/system.go` (new), `system_test.go`. |
| +**Files:** `internal/release/system.go` + test. |
| -**Produces:** `release.SystemKey(name)`, `AntivirusSettings`, |
| -`GetAntivirus`/`PutAntivirus` with CAS. |
| +- [ ] `TestSystemKeyShape`, `TestAntivirusSchemaRefusesUnknown` (schema 2 is an |
| + error, not a guess — this record decides whether tenant bytes get read), |
| + `TestPutAntivirusCreateOnlyThenReplace`. |
| -- [ ] **Failing tests** |
| - - `TestSystemKeyShape` — `system/antivirus.json`. |
| - - `TestAntivirusSchemaRefusesUnknown` — schema 2 is an error, not a guess. |
| - Same rule as `objstore.Record` and `ScanSidecar`, and for the same reason: |
| - this record decides whether tenant bytes get read. |
| - - `TestPutAntivirusCreateOnlyThenReplace` — `""` creates, an etag replaces, a |
| - stale etag is `ErrCASConflict`. |
| -- [ ] **Run:** FAIL, package does not exist. |
| -- [ ] **Implement.** A typed record, not a generic settings map — a map is |
| - `features.json` by another name, uninterpreted and unpinnable. |
| -- [ ] **Commit:** `release: system/<name>.json, a typed deployment settings record` |
| +**Commit:** `release: system/<name>.json, a typed deployment settings record` |
| --- |
| -## Task 3: wiring, migration and the seed |
| - |
| -**Files:** `cmd/console/main.go`, `internal/console/server.go`, |
| -`internal/console/scanpass.go`. |
| -**Tests:** `internal/console/scanpass_test.go`. |
| - |
| -**Produces:** `Config.ScanSeedFromDeprecatedEnv`; the Scanner built from |
| -wiring; the record read per tick. |
| - |
| -- [ ] **Failing tests** |
| - - `TestScanPassSeedsRecordOnFirstTick` — no record + the flag set → one |
| - create-only write, enabled, `by: "migration from CARLOS_STORE_SCANNER"`. |
| - - `TestScanPassSeedDoesNotOverrideExistingRecord` — a record saying |
| - `enabled:false` survives; an operator's `disable` is not undone by a |
| - restart. |
| - - `TestScanPassSeedIsSilentOnReadError` — a bucket read error neither seeds |
| - nor fatals. Seeding on a failed read would enable scanning on a deployment |
| - that had disabled it. |
| - - `TestScanPassDoesNothingWhenDisabled` — Scanner non-nil, record disabled → |
| - the lister is never consulted, and the goroutine keeps ticking. |
| - |
| - **Gotchas.** The seed goes at the top of the first tick, not in |
| - `storeScannerFor`, which runs at `main.go:262` before `openStore()` at |
| - `:269`. `StartScanPass` runs `scanPass` *before* waiting on the ticker |
| - (`scanpass.go:43-49`), so the seed happens at boot in practice. |
| - `ErrCASConflict` means another console seeded it — success, not failure. |
| -- [ ] **Run:** FAIL. |
| -- [ ] **Implement.** `storeScannerFor` builds whenever `CARLOS_CLAMD_ADDR`, |
| - `CARLOS_CLAMD_SOCKET`, **or `CARLOS_STORE_SCANNER=clamav`** is set, the last |
| - retaining the socket default at `main.go:775`. Any other value of the retired |
| - variable stays fatal. Setting `_ADDR` and `_SOCKET` to different values is |
| - fatal. `main.go` sets the Config flag; the pass never reads the environment. |
| -- [ ] **Commit:** `console: antivirus policy moves to a record, wiring stays in env` |
| +## Task 3: clamd over TCP |
| + |
| +**Moved ahead of the wiring task.** If wiring lands first, |
| +`CARLOS_CLAMD_ADDR=host:port` is accepted and then dialled as a unix path — |
| +`clamd.go:130` is the only `"unix"` literal and is unchanged until this commit. |
| + |
| +**Files:** `internal/clamd/clamd.go` + test. |
| + |
| +- [ ] `TestNewDialsTCPWhenAddrIsHostPort`, `TestNewDialsUnixWhenAddrBeginsWithSlash`. |
| +- [ ] The existing INSTREAM tests pass unchanged over TCP — framing, deadlines |
| + and parsing are transport-independent (`clamd.go:76-79,173-187`). |
| + |
| +`New(addr, timeouts)` keeps its arity, so no caller moves. |
| + |
| +**Commit:** `clamd: dial tcp or unix, decided by the address` |
| --- |
| -## Task 4: the `system antivirus` doors and CLI |
| +## Task 4: wiring, migration and the seed |
| + |
| +**Files:** `cmd/console/main.go`, `internal/console/server.go`, `scanpass.go`. |
| +**Tests:** `scanpass_test.go`, `cmd/console/main_test.go`. |
| + |
| +- [ ] `TestScannerWiringAcceptsDeprecatedVar` / `...FatalsOnUnknownEngine` / |
| + `...FatalsOnConflictingAddrAndSocket`. **Refactor a pure |
| + `scannerWiring(getenv func(string) string) (addr string, seed bool, err error)`** |
| + so the fatal path is testable; `main.go` calls `log.Fatal` on its error. |
| + `main_test.go` exists today with only `TestMain`/`TestMigrateAppVerb`. |
| +- [ ] `TestScanPassSeedsRecordOnFirstTick` — create-only, `by: "migration from |
| + CARLOS_STORE_SCANNER"`. |
| +- [ ] `TestScanPassSeedDoesNotOverrideExistingRecord`. |
| +- [ ] `TestScanPassSeedIsSilentOnReadError` — use `brokenStore`. Seeding on a |
| + failed read would enable scanning on a deployment that had disabled it. |
| +- [ ] `TestScanPassDoesNothingWhenDisabled` — `refusingLister`. |
| + |
| +**Gotchas.** The seed goes at the top of the first tick, not `storeScannerFor`, |
| +which runs at `main.go:262` before `openStore()` at `:269`; `StartScanPass` runs |
| +`scanPass` before waiting on the ticker (`scanpass.go:43-49`), so it happens at |
| +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`. |
| + |
| +**Commit:** `console: antivirus policy moves to a record, wiring stays in env` |
| -**Files:** `internal/console/apisystem.go` (new), `authcli.go`, |
| -`system.go` (repo root), `apimode.go`, `store.go`. |
| -**Tests:** `internal/console/apisystem_test.go`, `system_test.go`. |
| - |
| -- [ ] **Failing tests** |
| - - `TestSystemDoorsRefuseServicePrincipals` — registered `svcrole.Set{}`, so |
| - no service credential reaches them, and the door appears in `closedDoors` |
| - (`svcroutes_test.go:83`) or `TestEveryCLIDoorNamesItsRoles` fails. |
| - - `TestSystemAntivirusNeedsOperator` — `storeOperator(addr)`. Operators are |
| - addresses (`server.go:558-571`); a service actor is `service:`/`app:` |
| - (`apicli.go:89-100`) and can never match one, which is why the role gate |
| - alone is not the authority here. |
| - - `TestSystemAntivirusEnableRefusesWithoutScanner` — 503; a record must not |
| - claim enabled with nothing behind it. |
| - - `TestSystemAntivirusStatusReportsWiredWithNoRecord` — its own state. |
| - - `TestPrintSystemAntivirusStatus` — rendering, against a literal reply. |
| - |
| - **Gotcha.** A deployment-wide door has no `{account}` segment, so |
| - `apiPrincipalAccount` (`apicli.go:176-187`) cannot resolve it. `GET |
| - /api/cli/apps` (`authcli.go:471`) is the precedent for a person-only |
| - resolver. |
| -- [ ] **Implement.** `status|enable|disable` now; `rotate-webhook-key` in task 12. |
| -- [ ] **Commit:** `console: carlos system antivirus` |
| +--- |
| + |
| +## Task 5: the doors and `RunSweep` read the record |
| + |
| +**The plan's first draft omitted this entirely, so `disable` did not disable.** |
| +The tick went quiet while `carlos store scan sweep` still read tenant bytes. |
| + |
| +**Files:** `internal/console/apiscan.go`, `scanpass.go`, `internal/objstore/sweep.go`, |
| +plus the comment sweep in `objstore.go:86`, `scanner.go:89`, `server.go:522-523`, |
| +`main.go:772`. |
| + |
| +- [ ] `TestScanGrantRefusedWhenDeploymentDisabled` (`apiscan.go:252`). |
| +- [ ] `TestSweepNowRefusedWhenDeploymentDisabled` (`scanpass.go:198`). |
| +- [ ] `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`. |
| + |
| +**Commit:** `console: disable actually disables` |
| --- |
| -## Task 5: clamd over TCP |
| +## Task 6: `system antivirus` doors and CLI |
| + |
| +**Files:** `internal/console/apisystem.go` (new), `authcli.go`, |
| +`svcroutes_test.go` (closedDoors), root `system.go` (new), `apimode.go`, |
| +root `store.go` (dispatch only). |
| -**Files:** `internal/clamd/clamd.go`, `clamd_test.go`, `internal/objstore/clamav.go`. |
| +- [ ] `TestSystemAntivirusNeedsOperator` — `storeOperator(addr)` |
| + (`apistore.go:383`). Operators are addresses (`server.go:558-571`); a service |
| + actor is `service:`/`app:` (`apicli.go:89-100`) and can never match, which is |
| + why the role gate is not the authority here. |
| +- [ ] `TestSystemAntivirusEnableRefusesWithoutScanner` — 503. |
| +- [ ] `TestSystemAntivirusStatusReportsWiredWithNoRecord`. |
| +- [ ] `TestPrintSystemAntivirusStatus` — against a literal reply. |
| -- [ ] **Failing tests** |
| - - `TestNewDialsTCPWhenAddrIsHostPort` and |
| - `TestNewDialsUnixWhenAddrBeginsWithSlash` — one rule, stated once. |
| - - The existing INSTREAM tests must keep passing over TCP unchanged: the |
| - framing, deadlines and reply parsing are transport-independent |
| - (`clamd.go:76-79,173-187`), and `"unix"` appears exactly once |
| - (`clamd.go:130`). |
| -- [ ] **Implement.** `New(addr string, timeouts Timeouts)` picks the network. |
| -- [ ] **Commit:** `clamd: dial tcp or unix, decided by the address` |
| +Registered `svcrole.Set{}` and added to `closedDoors`; no bespoke |
| +"refuses service principals" test is needed — |
| +`TestEveryClosedDoorRefusesAServiceCredential` (`svcroutes_test.go:640`) already |
| +dials every closed door with a live Admin credential. |
| + |
| +**Gotcha:** no `{account}` segment, so `apiPrincipalAccount` |
| +(`apicli.go:176-187`) cannot resolve it. `GET /api/cli/apps` (`authcli.go:471`) |
| +is the person-only precedent. |
| + |
| +**Commit:** `console: carlos system antivirus` |
| --- |
| -## Task 6: `Scanner.Status`, and `Scan` reports whether it recorded |
| +## Task 7: `Scanner.Status`, and `Scan` reports whether it recorded |
| -**Files:** `internal/objstore/scanner.go`, `clamav.go`, `memscanner.go`, |
| -`sweep.go`. |
| +**Files:** `internal/objstore/scanner.go`, `clamav.go`, `memscanner.go`, `sweep.go`. |
| **Tests:** `scanner_contract_test.go`, `clamav_test.go`, `sweep_test.go`. |
| -**Produces:** `Status(ctx, bucket, key) (Verdict, ScanObject, error)`; |
| -`Scan(...) (Verdict, bool, error)`. |
| - |
| -- [ ] **Failing tests** |
| - - `TestStatusReturnsObjectSoCallerCanScan` — the `ScanObject` is populated on |
| - the no-tag path. **This is the whole point of the second return value:** |
| - `Scan` needs version and size, and the drain has no listing page, so a |
| - `Status` returning a verdict alone forces a `HeadObject` outside the |
| - scanner — exactly what keeping the token inside it forbids. |
| - - `TestStatusDoesNotApplyTheReIssueRule` — a stored `failed` or `denied` tag |
| - comes back **as itself**. `Verdict` keeps mapping them to `Unknown` |
| - (`clamav.go:151-163`) so the sweep retries; that is retry policy, not a |
| - fact about the object, and leaving it in `Status` makes a failed scan read |
| - as `pending` forever. |
| - - `TestStatusOnDeniedHead` — `(VerdictDenied, ScanObject{}, nil)`. |
| - - `TestStatusOnMissingObject` — `ErrObjectGone`. |
| - - `TestScanReportsUnrecorded` — the ten-tag cap (`clamav.go:306-308`) returns |
| - `recorded == false` with a real verdict. |
| - - `TestSweepCountsUnchangedByRecordedFlag` — Fresh/Reused count verdicts |
| - *reached*, so the sweep takes `_` and its numbers do not move. |
| - |
| - **Gotchas.** `tag()` currently returns `nil` silently at `clamav.go:291,309`; |
| - it must report whether it wrote, or `recorded` is a lie. `Scan`'s denied path |
| - (`:203`) discards `tag`'s result — `recorded` there is `false`. The third |
| - return value touches ~14 call sites in `clamav_test.go` plus the contract |
| - test. `MemScanner.Verdict` (`memscanner.go:89-100`) does **not** apply the |
| - re-issue rule today, so it gains it, or the split cannot be exercised against |
| - the fake; `MemScanner` also gains `PutObject` and `Unrecordable(key)`. |
| - `scanS3API` and `stubScanS3` gain `HeadObject`. |
| -- [ ] **Commit:** `objstore: Status answers about an object, Verdict decides retries` |
| +- [ ] `TestStatusReturnsObjectSoCallerCanScan` — the `ScanObject` is populated |
| + on the no-tag path. `Scan` needs version and size and the drain has no |
| + listing page, so a verdict-only `Status` forces a `HeadObject` outside the |
| + scanner — what keeping the token inside it forbids. |
| +- [ ] `TestStatusDoesNotApplyTheReIssueRule` — a stored `failed`/`denied` comes |
| + back as itself. `Verdict` keeps mapping them to `Unknown` |
| + (`clamav.go:151-163`) so the sweep retries; that is retry policy, and leaving |
| + it in `Status` makes a failed scan read `pending` forever. |
| +- [ ] `TestStatusOnDeniedHead` — `(VerdictDenied, ScanObject{}, nil)`. |
| +- [ ] `TestStatusOnMissingObject` — `ErrObjectGone`. |
| +- [ ] `TestScanReportsUnrecorded` — the ten-tag cap (`clamav.go:306-308`). |
| +- [ ] `TestSweepCountsUnchangedByRecordedFlag`. |
| + |
| +**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**. |
| +`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`. |
| + |
| +Large, and necessarily atomic — the signature change cannot be split. |
| + |
| +**Commit:** `objstore: Status answers about an object, Verdict decides retries` |
| --- |
| -## Task 7: `release.PageLister` |
| +## Task 8: `release.PageLister` |
| + |
| +**Files:** `internal/release/store.go` + test. |
| -**Files:** `internal/release/store.go`, `store_test.go`. |
| +- [ ] `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. |
| -- [ ] **Failing tests** |
| - - `TestDirStorePageListerOrdersAndBounds` — key-sorted, `startAfter` |
| - exclusive, `limit` respected. `DirStore.ListMod` is already key-sorted |
| - (`release/store.go:290`). |
| - - `TestS3StorePageLister` — `StartAfter` + `MaxKeys`, the shape `sweep.go:71-78` |
| - already uses. |
| -- [ ] **Implement.** An **optional interface**, type-asserted at the call site |
| - exactly as `SizeLister` (`release/store.go:181-185`) is at |
| - `economicspass.go:141`. **Not** a method on `release.Store`, which has eleven |
| - implementations — three real, `fleetapi.store`, and seven test fakes. |
| -- [ ] **Commit:** `release: PageLister, an optional bounded listing` |
| +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 |
| +`release.Store`, which has eleven implementations. |
| + |
| +**Commit:** `release: PageLister, an optional bounded listing` |
| --- |
| -## Task 8: the queue and its index |
| - |
| -**Files:** `internal/objstore/queue.go` (new), `queue_test.go`. |
| - |
| -**Produces:** `QueueKey(acct, app, id)`, `QueueItem`, `Enqueue`, `queueIndex`. |
| - |
| -- [ ] **Failing tests** |
| - - `TestEnqueueWritesObjectBeforeMarkingIndex` — order matters: a 202 sent |
| - between the two invites a client to stop retrying an item nothing points |
| - at. |
| - - `TestIndexEntrySurvivesEnqueueDuringDrain` — the generation counter. Drain |
| - reads `observed`, an enqueue increments, the drain must **not** remove. |
| - - `TestIndexEntrySurvivesPartialDrain` — **the test that would have caught |
| - review 3's blocker.** More items than the per-tick cap: the entry stays, |
| - and successive ticks drain the rest. |
| - - `TestIndexEntryRemovedOnlyOnEmptyListing` — "listing not truncated" was |
| - vacuous, because `release.Store.List` never truncates |
| - (`release/store.go:555-569`). Zero items is the test. |
| -- [ ] **Implement.** `apps/<acct>/<app>/scan-queue/<ulid>.json`, one object per |
| - item — distinct keys never collide, so an enqueue needs no coordination, and |
| - ULIDs sort by time. A top-level queue was rejected: `PurgeApp` lists only the |
| - app prefix (`trash.go:229-238`) and the claim gate never looks outside it. |
| -- [ ] **Commit:** `objstore: the scan queue, one object per item` |
| +## Task 9: the queue core |
| + |
| +**Files:** `internal/objstore/queue.go` (new) + test. |
| + |
| +**Produces:** `QueueKey`, `QueueItem`, `NewQueueID`, **exported** |
| +`QueueIndex{Bump, Observe, Release}`, and `DrainApp(ctx, s, sc, acct, app, cap)`. |
| + |
| +**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. |
| + |
| +- [ ] `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. |
| +- [ ] `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`. |
| + |
| +**Commit:** `objstore: the scan queue, its index and its drain` |
| --- |
| -## Task 9: the enqueue door and the consent chain |
| - |
| -**Files:** `internal/console/apiscan.go`, `authcli.go`. |
| -**Tests:** `internal/console/apiscan_test.go`. |
| - |
| -- [ ] **Failing tests** — one per refusal, because each is a separate promise: |
| - - `not_granted` (`Provisioned == nil`), `not_requested`, `encrypted`, |
| - `not_enabled`, `disabled`, and 429 past the limit. |
| - - `TestEnqueueRefusesWithoutAttestation` — **the consent hole review 3 |
| - found.** The landed sweep refuses to read tenant bytes without the member's |
| - attestation (`sweep.go:151-160`) and `scan grant` refuses without a request |
| - (`apiscan.go:268-285`), on the stated ground that an operator must not start |
| - that alone. An enqueue door that checked only the deployment switch put a |
| - door straight through it. |
| - - `TestEnqueueRequiresServiceRole` — and `Admin` reaches it by construction |
| - (`svcrole.go:109`), which the test states rather than fights. |
| -- [ ] **Implement.** Role `Service`, `svcWrite` limiter keyed on principal name |
| - (`svcprincipal.go:375-380`) — not a new bucket. |
| -- [ ] **Commit:** `console: the scan enqueue door, behind the consent chain` |
| +## Task 10: the enqueue door |
| + |
| +**Files:** `internal/console/apiscan.go`, `authcli.go`, |
| +`internal/console/scanhook.go` (new — the path validator only). |
| + |
| +- [ ] One test per refusal: `not_granted`, `not_requested`, `encrypted`, |
| + `not_enabled`, `disabled`, 429. |
| +- [ ] `TestEnqueueRefusesWithoutAttestation` — **the consent hole review 3 |
| + found.** The sweep refuses without the member's attestation |
| + (`sweep.go:151-160`) and grant refuses without a request |
| + (`apiscan.go:268-285`); a door checking only the deployment switch went |
| + straight through it. |
| +- [ ] `TestEnqueueRequiresServiceRole` — `serviceToken`; `Admin` reaches it by |
| + construction (`svcrole.go:109`), which the test states rather than fights. |
| +- [ ] **`TestCallbackPathValidation`** — the §6.2 hostile table lands **here**, |
| + not in the webhook task: absolute URLs, `//evil/x`, `/\evil/x`, |
| + `@`-authority, traversal, raw and encoded CRLF, fragments, >1KB. Draft 1 |
| + accepted the field in this task and validated it four commits later, so |
| + commits in between stored hostile paths. |
| + |
| +**Commit:** `console: the scan enqueue door, behind the consent chain` |
| --- |
| -## Task 10: the result object |
| - |
| -**Files:** `internal/objstore/scanresult.go` (new), `scanresult_test.go`. |
| - |
| -- [ ] **Failing tests** |
| - - `TestResultObjectIsVersionBound` — **the review 5 blocker, and the third |
| - time this class of bug appeared.** A result object written for v1 must not |
| - answer for v2: the door compares `result.version` against |
| - `ScanObject.Version()` and says `pending` on mismatch. |
| - - `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`). A stranded queue item is drained next tick; a |
| - stranded **result** object is found by nothing, blocks the app-name 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` fallback that skips |
| - collection and logs once, as `storagePass` does (`economicspass.go:141-145`). |
| -- [ ] **Commit:** `objstore: the scan result object, version-bound and guarded` |
| +## 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 |
| +thing that collects a stranded item, so it is not optional" and task 11's age |
| +test would have tested a collector nobody called. |
| + |
| +**Files:** `internal/console/scandrain.go` (new) + test. |
| + |
| +- [ ] `TestBootRebuildFindsStrandedItem` — an item written with no index entry |
| + is drained after a rebuild. |
| +- [ ] `TestBootRebuildCollectsExpiredItemsAndResults` — both prefixes, 24h TTL. |
| +- [ ] `TestDrainRunsWhileSweepIsBlocked` — **observable**, not nominal: |
| + `TestDrainIsItsOwnGoroutine` as named would pass against anything. Park |
| + `scanPass` in a blocking lister and assert an item still drains. `scanPass` |
| + sweeps apps sequentially for a full budget each (`scanpass.go:86-103`), so a |
| + queue phase behind it starves for up to N × 60s. |
| +- [ ] `TestDrainRoundRobinFairness` — one app's flood does not delay another's |
| + item beyond one tick. |
| +- [ ] `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. |
| + |
| +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. |
| + |
| +**Commit:** `console: the scan drain, its own goroutine and its own clock` |
| --- |
| -## Task 11: the drain |
| - |
| -**Files:** `internal/console/scandrain.go` (new), `scandrain_test.go`. |
| - |
| -- [ ] **Failing tests** |
| - - `TestDrainIsItsOwnGoroutine` — it cannot share the sweep's: `scanPass` |
| - sweeps apps sequentially for a full budget each (`scanpass.go:86-103`), so |
| - a queue phase behind it starves for up to N × 60s. |
| - - `TestDrainReChecksConsentChain` — the record can change between enqueue and |
| - drain, and the drain is where bytes are read. |
| - - `TestDrainSkipsTrashedApp` — items enqueued before `TrashApp` survive the |
| - 30-day cooloff with an intact record; without the tombstone check |
| - (`sweep.go:135-141`) the drain reads bytes the sweep deliberately will not. |
| - - `TestDrainReIssuesFailedAndDenied` — scans when `Status` is Unknown, |
| - Failed or Denied. Without it, on-demand is *less* able than the hourly |
| - sweep it exists to improve on. |
| - - `TestDrainKeepsItemsOnReadError` — deletion is driven by absence |
| - (`os.ErrNotExist`, `trash.go:64`), never by error, or a transient bucket |
| - failure discards a tenant's queue. |
| - - `TestDrainRoundRobinFairness` — one app's flood does not delay another's |
| - item beyond one tick. |
| - - `TestDrainDedupesByKeyPerTick`. |
| -- [ ] **Implement.** 30s ticker, 50 items per app per tick, `Status` before |
| - `Scan`, the §7.5 delete/keep lists, result objects on `recorded == false`, |
| - in-process notify for waiters. |
| -- [ ] **Commit:** `console: the scan drain, its own goroutine and its own clock` |
| +## Task 13: the status door and long-poll |
| + |
| +**Files:** `internal/console/apiscan.go` + test. |
| + |
| +- [ ] `TestStatusDoorReadsTagThenResultObject`. |
| +- [ ] `TestStatusDoorRefusesStaleResultObject` — the door-level half of task |
| + 11's unit test. |
| +- [ ] `TestStatusDoorReportsFailedAndDenied` — as themselves, never `pending`. |
| +- [ ] `TestStatusDoorGoneForMissingObject`. |
| +- [ ] `TestLongPollWakesOnDrain`, `TestLongPollCapsWaiters` — global **and** |
| + per-app. |
| + |
| +**Commit:** `console: the scan status door, long-polling and honest` |
| --- |
| -## Task 12: the status door and long-poll |
| +## Task 14: callback signing (pure) |
| + |
| +**Files:** `internal/console/scanhook.go` + test. |
| -**Files:** `internal/console/apiscan.go`. |
| -**Tests:** `apiscan_test.go`. |
| +- [ ] `TestSignatureRejectsTamperedBody`, `TestSignatureIsPerApp` (app A's does |
| + not verify for app B), `TestSignatureRejectsStaleTimestamp`, |
| + `TestTwoMasterRotationVerifies`, `TestTargetURLBuiltStructurally`. |
| -- [ ] **Failing tests** |
| - - `TestStatusDoorReadsTagThenResultObject` — tag first; on `Unknown`, the |
| - result object; absent, `pending`. |
| - - `TestStatusDoorRefusesStaleResultObject` — version mismatch → `pending`. |
| - - `TestStatusDoorReportsFailedAndDenied` — as themselves, never `pending`. |
| - - `TestStatusDoorGoneForMissingObject`. |
| - - `TestLongPollWakesOnDrain` and `TestLongPollCapsWaiters` — global *and* |
| - per-app, or it is a cheap way to occupy the console. |
| -- [ ] **Commit:** `console: the scan status door, long-polling and honest` |
| +Per-app key `HMAC(master, sqid + "/" + app)` derived at sign time — nothing |
| +extra stored, and without it one deployment secret lets app A forge callbacks |
| +to app B. Delimiter is injection-safe: sqid `^[a-z]{3,16}$` (`qualified.go:56`), |
| +app `^[a-z][a-z0-9-]{0,31}$` (`release.go:242`). |
| + |
| +**Commit:** `console: per-app callback signing` |
| --- |
| -## Task 13: the signed webhook |
| - |
| -**Files:** `internal/console/scanhook.go` (new), `scanhook_test.go`, |
| -`internal/objstore/walk.go` (key delivery on grant). |
| - |
| -- [ ] **Failing tests** — a table of hostile `callback_path` inputs: |
| - absolute URLs, `//evil/x`, `/\evil/x`, `@`-authority, traversal, raw and |
| - percent-encoded CRLF, fragments, >1KB. Each must be refused. |
| - - `TestCallbackGoesToPlatformHostOnly` — never a custom domain. The outbound |
| - fetcher picks the first ready non-static route (`instances.go:88-97`), which |
| - a tenant can point where they like. |
| - - `TestCallbackRefusesRedirects` — Go follows them by default, and a tenant |
| - 302 turns a signed POST into a GET at an internal address |
| - (`sesevents.go:143-158` is the existing precedent). |
| - - `TestSignatureRejectsTamperedBody`, `TestSignatureIsPerApp` (app A's does |
| - not verify for app B), `TestSignatureRejectsStaleTimestamp`, |
| - `TestTwoMasterRotationVerifies`. |
| - - `TestCallbackBodyCarriesRecorded` — or a callback saying `clean` and a door |
| - saying `recorded:false` disagree. |
| -- [ ] **Implement.** Per-app key `HMAC(master, sqid + "/" + app)` derived at |
| - sign time — nothing extra stored, and without it one deployment secret lets |
| - app A forge callbacks to app B. Delivered sealed on `store scan grant` |
| - (`apistore.go:425` gates the seal key). URL built structurally, never by |
| - concatenation. Add `rotate-webhook-key`; dropping `_PREVIOUS` is a separate |
| - operator step, because "done" means sealed, not converged |
| +## Task 15: key delivery and rotation |
| + |
| +**Files:** `cmd/console/main.go` and `server.go` (both masters into `Config`), |
| +`internal/console/apiscan.go` (`regrantForScanning`, `apiscan.go:492`, is the |
| +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. |
| +- [ ] `TestRotateWebhookKeyDoesNotDropPrevious` — dropping `_PREVIOUS` is a |
| + separate operator step, because "done" means sealed, not converged |
| (`server.go:566-569` makes this point for store rotation). |
| -- [ ] **Commit:** `console: signed scan callbacks, to the app's own host` |
| + |
| +**Commit:** `console: webhook key delivery and rotation` |
| + |
| +--- |
| + |
| +## Task 16: delivery |
| + |
| +**Files:** `internal/console/scandrain.go`, `scanhook.go`. |
| + |
| +- [ ] `TestCallbackGoesToPlatformHostOnly` — never a custom domain; the |
| + outbound fetcher picks the first ready non-static route |
| + (`instances.go:88-97`), which a tenant can point where they like. |
| +- [ ] `TestCallbackRefusesRedirects` — Go follows them by default and a tenant |
| + 302 turns a signed POST into a GET at an internal address |
| + (`sesevents.go:143-158` is the precedent). |
| +- [ ] `TestCallbackRetriesAreBounded` — §6.4. |
| +- [ ] `TestDroppedCallbackStillReadableFromStatusDoor` — the §13 promise that |
| + makes lazy retries acceptable. |
| +- [ ] `TestCallbackBodyCarriesRecorded` — or a callback saying `clean` and a |
| + door saying `recorded:false` disagree. |
| + |
| +**Commit:** `console: callback delivery, bounded and backstopped by the door` |
| --- |
| -## Task 14: metering, surfaces and docs |
| +## Task 17: metering, surfaces and docs |
| -**Files:** `internal/usage/scan.go` (new), `internal/console/apiscan.go`, |
| -`scan.go` and `store.go` (repo root), `docs/site/cli.md`, |
| +**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`, |
| `docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`. |
| -- [ ] **Failing tests** |
| - - `TestScanUsageCountsOnDemandOnly` — sweep scans are **not** attributed to a |
| - tenant who did not ask for them. |
| - - `TestScanUsageFoldsPerTick` — not per object; each fold is a GET plus a |
| - conditional PUT of a deployment-wide object. |
| - - `TestStoreScanStatusReportsDeploymentState` — a per-app record saying |
| - `enabled: clamav` while the deployment switch is off is a new way for the |
| - status output to lie. Note `apiscan_test.go:254` asserts `engine == |
| - "clamav"` on a fixture with no record; that fixture must write one. |
| -- [ ] **Implement.** `usage/scan/<date>.json`, CAS-merged the way |
| - `UpsertEdgeDay` does (`edge.go:85-116`); meter names are **frozen once |
| - merged** (`usage.go:4-5`). Update the rollout plan: step 4 becomes |
| - `CARLOS_CLAMD_ADDR` + `enable`, and rollback becomes `disable`, which needs |
| - no restart. |
| -- [ ] **Commit:** `console+cli: scan metering, surfaces and docs` |
| +- [ ] `TestScanUsageCountsOnDemandOnly` — sweep scans are not attributed to a |
| + tenant who did not ask for them. |
| +- [ ] `TestScanUsageFoldsPerTick` — not per object. |
| +- [ ] `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`. |
| + |
| +`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 |
| +`CARLOS_CLAMD_ADDR` + `enable`; rollback becomes `disable`, no restart. |
| + |
| +**Commit:** `console+cli: scan metering, surfaces and docs` |
| --- |
| -## Sequencing |
| +## Remaining §13 coverage, assigned |
| -1-2-3 unlock everything and are independent of each other. 5-6-7 are the seams; |
| -8-9-10-11 are the queue; 12-13 the surfaces; 14 last so the docs describe what |
| -exists. |
| +First-tick warning when wired with no record → task 4. `request`/`revoke`/ |
| +`enforce off` still work while **disabled** → extend |
| +`TestScanRevokeAndEnforceOffWorkWithoutScanner` in task 5. Enqueue racing |
| +`PurgeApp` leaves no orphan beyond one tick → task 12. |
| -**Docs hold until the feature is whole.** The `carlos store scan` section for |
| -the skills repo is drafted and in copy review, and describes hourly-sweep |
| -behaviour. It gets revised as part of task 14 rather than shipped twice. |
| +## Sequencing |
| -## Self-review |
| +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. |
| -Spec §3 → tasks 1, 9. §4 → tasks 2, 3, 4. §5 → tasks 6, 9, 12. §5.2.1 → tasks |
| -6, 10, 12. §6 → task 13. §7 → tasks 7, 8, 11. §8 → task 14. §9 → task 5. |
| -§11 → the security tests in 9, 10, 12, 13. §13's list is distributed across |
| -every task's step 1. |
| +**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 |
| +revised here rather than shipped twice. |