| 1 | # Scan on demand: implementation plan |
| 2 | |
| 3 | **Status:** draft 4 — cleared to implement from (closes plan reviews 1-3). |
| 4 | **Spec:** `docs/superpowers/specs/2026-09-02-scan-on-demand-design.md` (draft 7, |
| 5 | cleared after six review rounds). |
| 6 | **Branch:** `worktree-store-scan-on-demand`, stacked on the landed scanner work. |
| 7 | |
| 8 | Seventeen tasks, each its own commit, each TDD. |
| 9 | |
| 10 | --- |
| 11 | |
| 12 | ## Global constraints |
| 13 | |
| 14 | - `go build ./...`, `go vet ./...`, `gofmt -l .` clean before every commit. |
| 15 | - **No new third-party dependencies.** Signing is `crypto/hmac` + `crypto/sha256`. |
| 16 | **There is no ULID library in this repo and none is being added** — queue ids |
| 17 | are hand-rolled: a big-endian millisecond timestamp plus random bytes, |
| 18 | base32-encoded, with a test that they sort by time. |
| 19 | - Prefix Go commands with `GOCACHE=/home/paulca/.cache/tmp/claude-1001/gocache`. |
| 20 | - Five root-package tests (`TestAccountsList*`, `TestResolveMode*`) fail on this |
| 21 | machine for an environmental reason confirmed at `8103270`. Not a regression. |
| 22 | - **`s3:PutObjectTagging` must never enter the app's policy.** |
| 23 | - `grant` and `provisioned` in `store.json` stay frozen. |
| 24 | |
| 25 | ### The mistake this design keeps making |
| 26 | |
| 27 | Three review rounds caught the same class of bug: **a new durable fact about an |
| 28 | object, introduced without carrying the object version across the seam.** The |
| 29 | ETag binding, then `Status` versus `Verdict`, then the result object. Tasks 7, |
| 30 | 9 and 13 carry the tests that exist because of it. |
| 31 | |
| 32 | ### Helpers that already exist — use them, do not invent |
| 33 | |
| 34 | `storeArmed` (`apistore_test.go:505`), `scanArmed` (`apiscan_test.go:24`), |
| 35 | `putRecord`/`storedRecord` (`apistore_test.go:60,48`), `cliToken` |
| 36 | (`authcli_test.go:474`), `newConsoleFixture` (`server_test.go:44`), |
| 37 | `sweepFixture`/`memLister`/`memLister.before` (`sweep_test.go:88,19,206`), |
| 38 | `stubScanS3`/`fakeEngine`/`taggingOutput` (`clamav_test.go:24,55,264`), |
| 39 | `rotateFixture`/`testSealKeypair`/`testDeployment` (`walk_test.go:573,20,52`), |
| 40 | `closedDoors` (`svcroutes_test.go:83`), **`serviceToken` |
| 41 | (`svcfixture_test.go:14`)** for the role tests, **`brokenStore` |
| 42 | (`apiledger_test.go:658`)** for read-error tests, `refusingLister`/`withLister` |
| 43 | (`scanpass_test.go:36,64`). |
| 44 | |
| 45 | --- |
| 46 | |
| 47 | ## Task 1: `svcrole.Service`, and per-app scope |
| 48 | |
| 49 | **Files:** `internal/svcrole/svcrole.go`, `internal/console/svcprincipal.go`, |
| 50 | `apiservices.go`, `svcpage.go`, root `services.go`. |
| 51 | **Tests:** `svcrole_test.go`, `apiservices_test.go`. |
| 52 | |
| 53 | - [ ] `TestServiceRoleIsMintable` — `Parse` round-trips, `All()` includes it, so |
| 54 | the mint door and the form pick it up unchanged. |
| 55 | - [ ] `TestServiceCredentialCannotBeAccountScoped` — in `NewServicePrincipal` |
| 56 | (`svcprincipal.go:170-172`), **not** a handler: two mint doors exist |
| 57 | (`apiservices.go:211`, `svcpage.go:299`) and a handler check leaves one open. |
| 58 | New sentinel `ErrAccountScopedService`, with its own 400 arm in both — |
| 59 | `apiservices.go:211-215` matches only `ErrServiceNameTaken` today. |
| 60 | - [ ] Confirm `TestInstanceRoleMatchesTodaysAppTokenReach` |
| 61 | (`svcroutes_test.go:262-291`) still passes rather than writing a second one. |
| 62 | |
| 63 | **Gotcha:** `services.go` hardcodes the role list **three times** (`:33`, `:157`, |
| 64 | `:164`). |
| 65 | |
| 66 | **Commit:** `svcrole: a Service role for apps calling platform services` |
| 67 | |
| 68 | --- |
| 69 | |
| 70 | ## Task 2: the deployment settings record |
| 71 | |
| 72 | **Files:** `internal/release/system.go` + test. |
| 73 | |
| 74 | - [ ] `TestSystemKeyShape`, `TestAntivirusSchemaRefusesUnknown` (schema 2 is an |
| 75 | error, not a guess — this record decides whether tenant bytes get read), |
| 76 | `TestPutAntivirusCreateOnlyThenReplace`. |
| 77 | |
| 78 | **Commit:** `release: system/<name>.json, a typed deployment settings record` |
| 79 | |
| 80 | --- |
| 81 | |
| 82 | ## Task 3: clamd over TCP |
| 83 | |
| 84 | **Moved ahead of the wiring task.** If wiring lands first, |
| 85 | `CARLOS_CLAMD_ADDR=host:port` is accepted and then dialled as a unix path — |
| 86 | `clamd.go:130` is the only `"unix"` literal and is unchanged until this commit. |
| 87 | |
| 88 | **Files:** `internal/clamd/clamd.go` + test. |
| 89 | |
| 90 | - [ ] `TestNewDialsTCPWhenAddrIsHostPort`, `TestNewDialsUnixWhenAddrBeginsWithSlash`. |
| 91 | - [ ] The existing INSTREAM tests pass unchanged over TCP — framing, deadlines |
| 92 | and parsing are transport-independent (`clamd.go:76-79,173-187`). |
| 93 | |
| 94 | `New(addr, timeouts)` keeps its arity, so no caller moves. |
| 95 | |
| 96 | **Commit:** `clamd: dial tcp or unix, decided by the address` |
| 97 | |
| 98 | --- |
| 99 | |
| 100 | ## Task 4: wiring, migration and the seed |
| 101 | |
| 102 | **Files:** `cmd/console/main.go`, `internal/console/server.go`, `scanpass.go`. |
| 103 | **Tests:** `scanpass_test.go`, `cmd/console/main_test.go`. |
| 104 | |
| 105 | - [ ] `TestScannerWiringAcceptsDeprecatedVar` / `...FatalsOnUnknownEngine` / |
| 106 | `...FatalsOnConflictingAddrAndSocket`. **Refactor a pure |
| 107 | `scannerWiring(getenv func(string) string) (addr string, seed bool, err error)`** |
| 108 | so the fatal path is testable; `main.go` calls `log.Fatal` on its error. |
| 109 | `main_test.go` exists today with only `TestMain`/`TestMigrateAppVerb`. |
| 110 | - [ ] `TestScanPassSeedsRecordOnFirstTick` — create-only, `by: "migration from |
| 111 | CARLOS_STORE_SCANNER"`. |
| 112 | - [ ] `TestScanPassSeedDoesNotOverrideExistingRecord`. |
| 113 | - [ ] `TestScanPassSeedIsSilentOnReadError` — use `brokenStore`. Seeding on a |
| 114 | failed read would enable scanning on a deployment that had disabled it. |
| 115 | - [ ] `TestScanPassDoesNothingWhenDisabled` — `refusingLister`. |
| 116 | |
| 117 | **Gotchas.** The seed goes at the top of the first tick, not `storeScannerFor`, |
| 118 | which runs at `main.go:262` before `openStore()` at `:269`; `StartScanPass` runs |
| 119 | `scanPass` before waiting on the ticker (`scanpass.go:43-49`), so it happens at |
| 120 | boot in practice. `ErrCASConflict` is success. |
| 121 | |
| 122 | **This task breaks five existing tests and must fix them in the same commit:** |
| 123 | `TestScanPassSweepsOnlyEnabledStores`, `...HonoursReSweepInterval`, |
| 124 | `...ContinuesPastOneAppsFailure`, `...AlwaysContinuesAnInFlightSweep`, |
| 125 | `...ResweepsAfterReattestationToPlaintext` (`scanpass_test.go:93,126,165,196,241`) |
| 126 | all drive `scanPass` and expect a sweep. `enableStore` (`scanpass_test.go:44`) |
| 127 | writes an enabled `system/antivirus.json`. Give the record to |
| 128 | `TestScanPassStillSkipsEncryptedStore` (`:275`) as well — it currently passes |
| 129 | for the wrong reason, since `refusingLister` is never consulted whether or not |
| 130 | encryption is the cause. |
| 131 | |
| 132 | **Commit:** `console: antivirus policy moves to a record, wiring stays in env` |
| 133 | |
| 134 | --- |
| 135 | |
| 136 | ## Task 5: the doors and `RunSweep` read the record |
| 137 | |
| 138 | **The plan's first draft omitted this entirely, so `disable` did not disable.** |
| 139 | The tick went quiet while `carlos store scan sweep` still read tenant bytes. |
| 140 | |
| 141 | **Files:** `internal/console/apiscan.go`, `scanpass.go`, `internal/objstore/sweep.go`, |
| 142 | plus the comment sweep in `objstore.go:86`, `scanner.go:89`, `server.go:522-523`, |
| 143 | `main.go:772`. |
| 144 | |
| 145 | - [ ] `TestScanGrantRefusedWhenDeploymentDisabled` (`apiscan.go:252`). |
| 146 | - [ ] `TestSweepNowRefusedWhenDeploymentDisabled` (`scanpass.go:198`). |
| 147 | - [ ] `TestRunSweepRefusesWhenDeploymentDisabled` — `sweep.go:163` still reads |
| 148 | `if sc == nil … (set CARLOS_STORE_SCANNER)`. |
| 149 | - [ ] `TestScanStatusReportsDisabledDistinctFromUnreachable` (`apiscan.go:71-81`). |
| 150 | - [ ] **Keep the two 503s distinct.** `scanNoEngine` (`apiscan.go:43-46`) fires |
| 151 | only when `StoreScanner == nil` — a *wiring* state — so it names |
| 152 | **`CARLOS_CLAMD_ADDR`**. The new refusal, for a record that says disabled, |
| 153 | names **`carlos system antivirus enable`**. Draft 3 pointed the first at the |
| 154 | second's fix, which would tell an operator whose record is already enabled to |
| 155 | enable it, and would collapse the two states |
| 156 | `TestScanStatusReportsDisabledDistinctFromUnreachable` exists to separate. |
| 157 | - [ ] Update the two tests pinning the old wording — `apiscan_test.go:85` and |
| 158 | `:353` — to assert `CARLOS_CLAMD_ADDR`. Both run under `scanArmed`, which |
| 159 | after this task writes an enabled record and then nils the scanner: "enabled, |
| 160 | not wired", not "disabled". |
| 161 | |
| 162 | **This task breaks roughly twenty-five existing tests, and every one of them |
| 163 | is a fixture that arms a scanner with no deployment record.** Draft 2 caught |
| 164 | this for task 4 and then repeated the omission in the task it added to fix it. |
| 165 | Absent record = **not enabled**, so: |
| 166 | |
| 167 | - `enableScanning` (`teardown_scan_test.go:16`) writes an enabled |
| 168 | `system/antivirus.json`. It backs 17 `sweepFixture` calls and 20 `RunSweep` |
| 169 | calls in `sweep_test.go`; without the record every one errors at the new |
| 170 | refusal. |
| 171 | - `scanArmed` (`apiscan_test.go:24`) writes one too. `/grant` is expected to |
| 172 | return 200 at `apiscan_test.go:105,168,283,330,343`, and |
| 173 | `TestScanGrantRefusedWithoutMemberRequest` (`:315`) expects 409 but would meet |
| 174 | the new 503 first. |
| 175 | - **`TestScanEnforceWritesTheDenyToThePolicy` (`apiscan_test.go:204`) is the |
| 176 | third fixture**, and uses neither helper: it arms via `storeArmed` plus a |
| 177 | hand-set `cfg.StoreScanner` at `:207`, then expects `/grant` 200 at `:210`. |
| 178 | Switch it to `scanArmed` or give it its own record. |
| 179 | |
| 180 | **Where the check goes in `RunSweep`:** replace the `sc == nil` arm at |
| 181 | `sweep.go:162-165`, *after* the `Enabled == nil` early return at `:128` — put it |
| 182 | before and `TestSweepIgnoresStoreWithoutScanning` changes meaning. |
| 183 | |
| 184 | Also reword `walk.go:466`, which still tells an operator to set |
| 185 | `CARLOS_STORE_SCANNER` and is pinned by `teardown_scan_test.go:72`. |
| 186 | |
| 187 | **Commit:** `console: disable actually disables` |
| 188 | |
| 189 | --- |
| 190 | |
| 191 | ## Task 6: `system antivirus` doors and CLI |
| 192 | |
| 193 | **Files:** `internal/console/apisystem.go` (new), `authcli.go`, |
| 194 | `svcroutes_test.go` (closedDoors), root `system.go` (new), `apimode.go`, |
| 195 | root `store.go` (dispatch only). |
| 196 | |
| 197 | - [ ] `TestSystemAntivirusNeedsOperator` — `storeOperator(addr)` |
| 198 | (`apistore.go:383`). Operators are addresses (`server.go:558-571`); a service |
| 199 | actor is `service:`/`app:` (`apicli.go:89-100`) and can never match, which is |
| 200 | why the role gate is not the authority here. |
| 201 | - [ ] `TestSystemAntivirusEnableRefusesWithoutScanner` — 503. |
| 202 | - [ ] `TestSystemAntivirusStatusReportsWiredWithNoRecord`. |
| 203 | - [ ] `TestPrintSystemAntivirusStatus` — against a literal reply. |
| 204 | |
| 205 | Registered `svcrole.Set{}` and added to `closedDoors`; no bespoke |
| 206 | "refuses service principals" test is needed — |
| 207 | `TestEveryClosedDoorRefusesAServiceCredential` (`svcroutes_test.go:640`) already |
| 208 | dials every closed door with a live Admin credential. |
| 209 | |
| 210 | **Gotcha:** no `{account}` segment, so `apiPrincipalAccount` |
| 211 | (`apicli.go:176-187`) cannot resolve it. `GET /api/cli/apps` (`authcli.go:471`) |
| 212 | is the person-only precedent. |
| 213 | |
| 214 | **Commit:** `console: carlos system antivirus` |
| 215 | |
| 216 | --- |
| 217 | |
| 218 | ## Task 7: `Scanner.Status`, and `Scan` reports whether it recorded |
| 219 | |
| 220 | **Files:** `internal/objstore/scanner.go`, `clamav.go`, `memscanner.go`, `sweep.go`. |
| 221 | **Tests:** `scanner_contract_test.go`, `clamav_test.go`, `sweep_test.go`. |
| 222 | |
| 223 | - [ ] `TestStatusReturnsObjectSoCallerCanScan` — the `ScanObject` is populated |
| 224 | on the no-tag path. `Scan` needs version and size and the drain has no |
| 225 | listing page, so a verdict-only `Status` forces a `HeadObject` outside the |
| 226 | scanner — what keeping the token inside it forbids. |
| 227 | - [ ] `TestStatusDoesNotApplyTheReIssueRule` — a stored `failed`/`denied` comes |
| 228 | back as itself. `Verdict` keeps mapping them to `Unknown` |
| 229 | (`clamav.go:151-163`) so the sweep retries; that is retry policy, and leaving |
| 230 | it in `Status` makes a failed scan read `pending` forever. |
| 231 | - [ ] `TestStatusOnDeniedHead` — `(VerdictDenied, ScanObject{}, nil)`. |
| 232 | - [ ] `TestStatusOnMissingObject` — `ErrObjectGone`. |
| 233 | - [ ] `TestScanReportsUnrecorded` — the ten-tag cap (`clamav.go:306-308`). |
| 234 | - [ ] `TestSweepCountsUnchangedByRecordedFlag`. |
| 235 | |
| 236 | **Gotchas.** `tag()` returns `nil` silently at `clamav.go:291,307`; it must |
| 237 | report whether it wrote or `recorded` is a lie. `Scan`'s denied path |
| 238 | (`clamav.go:203`) discards `tag`'s result — `recorded` is `false` there. The new |
| 239 | return touches **12 call sites in `clamav_test.go` and one in the contract test** |
| 240 | (`scanner_contract_test.go:33`) — and, unnamed in draft 2, **`goneScanner.Scan` |
| 241 | (`sweep_test.go:481`)**, which overrides the old signature and must change in |
| 242 | the same commit. It embeds `MemScanner` by value, so `Status` arrives for free. |
| 243 | `MemScanner.Verdict` (`memscanner.go:89-100`) does not apply the re-issue rule |
| 244 | today, so it gains it; `MemScanner` also gains `PutObject` and |
| 245 | `Unrecordable(key)`. `scanS3API` and `stubScanS3` gain `HeadObject`. |
| 246 | |
| 247 | Large, and necessarily atomic — the signature change cannot be split. |
| 248 | |
| 249 | **Commit:** `objstore: Status answers about an object, Verdict decides retries` |
| 250 | |
| 251 | --- |
| 252 | |
| 253 | ## Task 8: `release.PageLister` |
| 254 | |
| 255 | **Files:** `internal/release/store.go` + test. |
| 256 | |
| 257 | - [ ] `TestDirStorePageListerOrdersAndBounds` — `ListMod` is already key-sorted |
| 258 | (`release/store.go:290`). |
| 259 | - [ ] `TestS3StorePageLister` — **against an `httptest` handler**, asserting |
| 260 | `start-after`, `max-keys` and the prefix on the `ListObjectsV2` query and |
| 261 | returning two pages. Draft 2 said "RGW-gated, like every other S3Store test"; |
| 262 | that was wrong. `store_rgw_test.go:94,151,184` are **not** env-gated — they |
| 263 | stand up an in-process fake (`newRGWFake`, `:27`, then `NewS3Store`, |
| 264 | `:99-101`), which is exactly the seam draft 2 claimed did not exist. Gating |
| 265 | this test would leave the drain's `StartAfter`/`MaxKeys` path unexercised in |
| 266 | CI, which is the one path that matters here. |
| 267 | |
| 268 | An **optional interface** type-asserted at the call site as `SizeLister` |
| 269 | (`release/store.go:181-185`) is at `economicspass.go:141` — not a method on |
| 270 | `release.Store`, which has eleven implementations. |
| 271 | |
| 272 | **Commit:** `release: PageLister, an optional bounded listing` |
| 273 | |
| 274 | --- |
| 275 | |
| 276 | ## Task 9: the result object |
| 277 | |
| 278 | **Moved ahead of the queue core.** Draft 2 had the queue core first, so |
| 279 | `DrainApp` would have landed with nowhere to put an unrecorded verdict — the |
| 280 | exact §5.2.1 bug the result object exists to prevent. Task 9 is independent of |
| 281 | everything else; task 10 needs it. |
| 282 | |
| 283 | **Files:** `internal/objstore/scanresult.go` (new) + test. |
| 284 | |
| 285 | **Produces:** `WriteScanResult`, `ReadScanResult(ctx, s, acct, app, key, |
| 286 | currentVersion)` — absent on version mismatch, so no caller compares versions |
| 287 | itself. |
| 288 | |
| 289 | - [ ] `TestResultObjectIsVersionBound` — **review 5's blocker, the third |
| 290 | appearance of this bug class.** A result written for v1 must not answer for |
| 291 | v2. Unit-level: `ReadScanResult` with a different `currentVersion` is absent. |
| 292 | - [ ] `TestResultObjectReplacesOnSecondWrite` — read-then-CAS. `Put` has only |
| 293 | create-only and replace (`release/store.go:37-45`); a create-only second write |
| 294 | returns `ErrCASConflict` and silently drops the v2 verdict. |
| 295 | - [ ] `TestResultObjectNotWrittenUnderPurgedPrefix` — the `store.json` re-read |
| 296 | guard (`sweep.go:358-367`), using `memLister.before`. A stranded queue item is |
| 297 | drained next tick; a stranded **result** object is found by nothing, blocks the |
| 298 | claim gate (`server.go:2921`), and — because `materialize.go:333-345,377-382` |
| 299 | counts any key and restores a trashed row — **resurrects the app** on the next |
| 300 | boot. |
| 301 | - [ ] `TestResultObjectCollectedByAge` — with a `ModLister` |
| 302 | (`release/store.go:161`) fallback that skips and logs once, as `storagePass` |
| 303 | does (`economicspass.go:141-145`). |
| 304 | |
| 305 | **Commit:** `objstore: the scan result object, version-bound and guarded` |
| 306 | |
| 307 | --- |
| 308 | |
| 309 | ## Task 10: the queue core |
| 310 | |
| 311 | **Files:** `internal/objstore/queue.go` (new) + test. |
| 312 | |
| 313 | **Produces:** `QueueKey`, `QueueItem`, `NewQueueID`, **`Enqueue(ctx, s, idx, |
| 314 | acct, app, item)`** — which draft 2 dropped from Produces while keeping the test |
| 315 | that needs it — plus: |
| 316 | |
| 317 | ```go |
| 318 | type QueueIndex struct{ … } // exported: console holds the goroutine |
| 319 | func (i *QueueIndex) Bump(acct, app string) |
| 320 | func (i *QueueIndex) Observe(acct, app string) uint64 |
| 321 | func (i *QueueIndex) Release(acct, app string, observed uint64, listedEmpty bool) bool |
| 322 | func (i *QueueIndex) Apps() []AppRef // round-robin needs enumeration |
| 323 | func (i *QueueIndex) Depth() map[AppRef]int |
| 324 | |
| 325 | type Drained struct { |
| 326 | Key, Version, ETag string |
| 327 | Verdict Verdict |
| 328 | Recorded bool // the tag was written |
| 329 | Scanned bool // Scan actually ran, vs answered from a tag |
| 330 | Size int64 |
| 331 | Engine, Signature string |
| 332 | CallbackPath string |
| 333 | } |
| 334 | func DrainApp(ctx, s, sc, idx, acct, app string, cap int) (done []Drained, listedEmpty bool, err error) |
| 335 | ``` |
| 336 | |
| 337 | **The return type is not incidental.** Console needs per-item outcomes for the |
| 338 | long-poll wake (13), the callbacks (16) and the per-tick usage fold (17). |
| 339 | `Size` and `Scanned` are there for the meter specifically: §8 counts *bytes* |
| 340 | scanned on demand, and an item answered from an existing tag was scanned by the |
| 341 | **sweep**, which `TestScanUsageCountsOnDemandOnly` says must not be attributed |
| 342 | to the tenant. Without `Scanned` the drain cannot tell the meter which is which. |
| 343 | |
| 344 | **`DrainApp` owns `Observe` and `Release`.** It takes `idx` and calls both; |
| 345 | `listedEmpty` is returned for the caller's information, not as an instruction. |
| 346 | The alternative — console calling them — requires reading `Observe` *before* the |
| 347 | listing, an ordering console cannot see, and that ordering is exactly what |
| 348 | §7.1's generation counter protects. Without `Apps()` and `Depth()` console |
| 349 | reaches back for the mutex the split exists to hide. |
| 350 | |
| 351 | - [ ] `TestQueueIDSortsByTime` — hand-rolled, no ULID dependency. |
| 352 | - [ ] `TestEnqueueWritesObjectBeforeBump` — a 202 between the two invites a |
| 353 | client to stop retrying an item nothing points at. |
| 354 | - [ ] `TestIndexEntrySurvivesEnqueueDuringDrain` — the generation counter. |
| 355 | - [ ] `TestIndexEntrySurvivesPartialDrain` — **the test that would have caught |
| 356 | review 3's blocker.** More items than the cap: the entry stays. |
| 357 | - [ ] `TestIndexEntryReleasedOnlyOnEmptyListing` — "not truncated" was vacuous; |
| 358 | `S3Store.ListMod` never truncates (`release/store.go:555-569`). |
| 359 | - [ ] `TestDrainAppWritesResultWhenUnrecorded` and |
| 360 | `TestDrainAppDeletesResultWhenLaterRecorded` — §5.2.1's two drain duties, |
| 361 | which no task owned in draft 2. |
| 362 | - [ ] `TestDrainAppDeletesItemsForTrashedApp` — **deletes, not skips.** Draft 2 |
| 363 | named it "Skips" after the sweep's behaviour (`sweep.go:135-141`), but §5.1.1 |
| 364 | and §7.5 have the drain *delete*; a test named "skips" gets implemented as |
| 365 | skips and the items sit for thirty days. |
| 366 | - [ ] `TestDrainAppDeletesOnNotPermitted` — `not_granted`, `not_requested`, |
| 367 | `encrypted`, `not_enabled` — versus `TestDrainAppKeepsOnReversible` — |
| 368 | `disabled`, engine drift, unreachable engine, read error, each up to the TTL. |
| 369 | §7.5's two lists, separately, because "re-checks the consent chain" does not |
| 370 | say which side an item falls on. |
| 371 | - [ ] `TestDrainAppDedupesByKey`. |
| 372 | |
| 373 | Absence is `os.ErrNotExist` (`trash.go:64`), never an error. |
| 374 | |
| 375 | **Commit:** `objstore: the scan queue, its index and its drain` |
| 376 | |
| 377 | --- |
| 378 | |
| 379 | ## Task 11: the enqueue door |
| 380 | |
| 381 | **Files:** `internal/console/apiscan.go`, `authcli.go`, |
| 382 | `internal/console/scanhook.go` (new — the path validator only). |
| 383 | |
| 384 | - [x] One test per refusal: `not_granted`, `not_requested`, `encrypted`, |
| 385 | `not_enabled`, `disabled`, 429. |
| 386 | - [x] `TestEnqueueRefusesWithoutAttestation` — **the consent hole review 3 |
| 387 | found.** The sweep refuses without the member's attestation |
| 388 | (`sweep.go:151-160`) and grant refuses without a request |
| 389 | (`apiscan.go:268-285`); a door checking only the deployment switch went |
| 390 | straight through it. |
| 391 | - [x] `TestEnqueueRequiresServiceRole` — `serviceToken`; `Admin` reaches it by |
| 392 | construction (`svcrole.go:109`), which the test states rather than fights. |
| 393 | - [x] **`TestCallbackPathValidation`** — the §6.2 hostile table lands **here**, |
| 394 | not in the webhook task: absolute URLs, `//evil/x`, `/\evil/x`, |
| 395 | `@`-authority, traversal, raw and encoded CRLF, fragments, >1KB. Draft 1 |
| 396 | accepted the field in this task and validated it four commits later, so |
| 397 | commits in between stored hostile paths. |
| 398 | |
| 399 | **Commit:** `console: the scan enqueue door, behind the consent chain` |
| 400 | |
| 401 | **Landed** as `console: the scan enqueue door, behind the consent chain`. |
| 402 | Two things came out differently from this plan, both worth carrying forward: |
| 403 | |
| 404 | - The refusal mapping was moved OUT of the handler into `scanRefusalAnswer`. |
| 405 | Two of its arms are unreachable through the door — a trashed app is a 404 at |
| 406 | `apiAccountApp` before the chain is consulted, and an undefined refusal name |
| 407 | cannot be sent — and both survived mutation testing through HTTP for that |
| 408 | reason alone. Task 12's drain has the same pair of unreachable conclusions |
| 409 | (§7.5's keep-or-delete) and should be written as a table from the start |
| 410 | rather than as a switch inside the goroutine. |
| 411 | - `objstore.queuePermitted` is now exported as `QueuePermitted`, because the |
| 412 | door is in another package and §5.1.1 forbids a second derivation. |
| 413 | |
| 414 | `TestCallbackPathValidation` was already landed with the validator; what this |
| 415 | task added on top is the door-level proof that the door CALLS it, which is a |
| 416 | different claim and the one that keeps hostile paths out of the bucket. |
| 417 | |
| 418 | |
| 419 | --- |
| 420 | |
| 421 | ## Task 12: the drain goroutine and the boot rebuild |
| 422 | |
| 423 | **The boot rebuild had no task in draft 1**, though the spec calls it "the only |
| 424 | thing that collects a stranded item, so it is not optional" and task 9's |
| 425 | `TestResultObjectCollectedByAge` would have tested a collector nobody called. |
| 426 | |
| 427 | **Files:** `internal/console/scandrain.go` (new) + test. |
| 428 | |
| 429 | - [x] `TestBootRebuildFindsStrandedItem` — an item written with no index entry |
| 430 | is drained after a rebuild. |
| 431 | - [x] `TestBootRebuildCollectsExpiredItemsAndResults` — both prefixes, 24h TTL. |
| 432 | - [x] `TestDrainRunsWhileSweepIsBlocked` — **observable**, not nominal: |
| 433 | `TestDrainIsItsOwnGoroutine` as named would pass against anything. Park |
| 434 | `scanPass` in a blocking lister and assert an item still drains. `scanPass` |
| 435 | sweeps apps sequentially for a full budget each (`scanpass.go:86-103`), so a |
| 436 | queue phase behind it starves for up to N × 60s. |
| 437 | - [x] `TestDrainRoundRobinFairness` — one app's flood does not delay another's |
| 438 | item beyond one tick. |
| 439 | - [x] `TestDrainAgeSweepsResultObjects` — per visited app, so a console that |
| 440 | never restarts does not accumulate them. |
| 441 | - [x] `TestQueueDepthReported` — §4.4 and §10 both want it; **it is delivered |
| 442 | here** and surfaced in task 17. Task 6 lands before this and cannot surface |
| 443 | it. |
| 444 | |
| 445 | Implements: 30s ticker, 50 items per app per tick, in-process notify for |
| 446 | waiters, deleting the result object when a later scan records a real verdict. |
| 447 | |
| 448 | **Commit:** `console: the scan drain, its own goroutine and its own clock` |
| 449 | |
| 450 | --- |
| 451 | |
| 452 | **Landed** as `console: the scan drain, its own goroutine and its own clock`. |
| 453 | |
| 454 | Two lines in the tick are documented as NOT being the guarantee they look |
| 455 | like, because mutation testing showed neither changes anything observable: the |
| 456 | deployment-switch check and the engine check are both about avoiding work, |
| 457 | since `DrainApp` re-reads consent and refuses on its own. The first is asserted |
| 458 | the only way it can be — by counting listings — and the second is marked |
| 459 | unassertable in the comment rather than left looking tested. |
| 460 | |
| 461 | `TestDrainRoundRobinFairness` also pinned down something the spec left |
| 462 | implicit: an app's index entry is released on the first tick that lists |
| 463 | NOTHING, not on the tick that empties it, because a consumed page reports "not |
| 464 | empty". Task 17's queue-depth surface should say "apps with work" rather than |
| 465 | "apps with items", or it will read as off-by-one for one tick. |
| 466 | |
| 467 | --- |
| 468 | |
| 469 | ## Task 13: the status door and long-poll |
| 470 | |
| 471 | **Files:** `internal/console/apiscan.go` + test. |
| 472 | |
| 473 | - [x] `TestStatusDoorReadsTagThenResultObject`. |
| 474 | - [x] `TestStatusDoorRefusesStaleResultObject` — the door-level half of task |
| 475 | 9's unit test. |
| 476 | - [x] `TestStatusDoorReportsFailedAndDenied` — as themselves, never `pending`. |
| 477 | - [x] `TestStatusDoorGoneForMissingObject`. |
| 478 | - [x] `TestLongPollWakesOnDrain`, `TestLongPollCapsWaiters` — global **and** |
| 479 | per-app. |
| 480 | |
| 481 | **Commit:** `console: the scan status door, long-polling and honest` |
| 482 | |
| 483 | --- |
| 484 | |
| 485 | **Landed** as `console: the scan status door, long-polling and honest`. |
| 486 | Eleven mutations, all caught. Two gaps are recorded rather than papered over: |
| 487 | |
| 488 | - [ ] **`at`, `engine` and `signature` are missing from the TAG answer.** §5.2's |
| 489 | example shows `{"state":"clean","at":"...","engine":"clamav","signature":"..."}`, |
| 490 | but the `Scanner.Status` seam §5.2 itself specifies returns the verdict and |
| 491 | the object and nothing else. The door reports provenance where it genuinely |
| 492 | has it — a result object records all three — and says NOTHING in the tag |
| 493 | branch, because the only value available there is this deployment's CURRENT |
| 494 | engine, which may not be the one that judged the object. That would be a |
| 495 | guess dressed as a record, in the one feature whose whole purpose is not |
| 496 | reporting coverage it does not have. Closing this means widening `Status` to |
| 497 | return the tag's provenance, which is a change to a reviewed interface and |
| 498 | belongs in its own task rather than smuggled into this one. |
| 499 | - The register-before-read ordering in the long-poll is not asserted: catching |
| 500 | the wrong order needs a drain to land between two adjacent statements and |
| 501 | there is no seam to hold it there. Noted in the code. Losing that race costs |
| 502 | one expired wait, never a wrong answer — the door re-reads on its timer and |
| 503 | never answers from the notification itself. |
| 504 | |
| 505 | --- |
| 506 | |
| 507 | ## Task 14: callback signing (pure) |
| 508 | |
| 509 | **Revised after review 7**, which found five blockers in tasks 14-17 before any |
| 510 | of it was written. The additions below are all its findings. |
| 511 | |
| 512 | **Files:** `internal/console/scansign.go` (new — signing is its own file, not |
| 513 | more of the path validator) + test. |
| 514 | |
| 515 | - [x] `TestSignatureRejectsTamperedBody`, `TestSignatureIsPerApp` (app A's does |
| 516 | not verify for app B), `TestSignatureRejectsStaleTimestamp`, |
| 517 | `TestTwoMasterRotationVerifies`. |
| 518 | - [x] **`VerifyCallback(key, header, body, now)` — the reference verifier**, and |
| 519 | it is the deliverable §11.3 was missing. "A stale timestamp is refused" and |
| 520 | "both masters verify" are properties of the APP's verifier, which is tenant |
| 521 | code; without a reference implementation the tests test nothing anybody runs. |
| 522 | The docs show verifier examples implementing the same MAC and timestamp checks; they accept unknown signature fields. |
| 523 | - [x] `TestSignatureIsOverRawBodyBytes` — an app that re-serialises the JSON |
| 524 | before verifying gets a different byte string and fails. Said in the docs. |
| 525 | - [x] `TestVerifierToleratesTwoSignaturesAndUnknownKeyIds` — the rotation |
| 526 | window depends on it. |
| 527 | - [x] `TestPreviousEqualToCurrentIsRefused` — a rotation that rotated to |
| 528 | nothing, refused exactly as `NewTagSigner` refuses it. |
| 529 | |
| 530 | Per-app key `HMAC(master, "carlos-scan-webhook-v1" + 0x00 + sqid + "/" + app + |
| 531 | "/" + gen)` derived at sign time — nothing extra stored, and without it one |
| 532 | deployment secret lets app A forge callbacks to app B. Delimiter is |
| 533 | injection-safe: sqid `^[a-z]{3,16}$` (`qualified.go:56`), app |
| 534 | `^[a-z][a-z0-9-]{0,31}$` (`release.go:242`), verified in the repo. The domain |
| 535 | label and the per-app `gen` are review 7's; so is the key id's distinct label. |
| 536 | |
| 537 | `TestTargetURLBuiltStructurally` is dropped — it landed with the validator as |
| 538 | `TestCallbackTargetIsBuiltStructurally`. |
| 539 | |
| 540 | **Commit:** `console: per-app callback signing, and the verifier apps will run` |
| 541 | |
| 542 | --- |
| 543 | |
| 544 | ## Task 15: key delivery and rotation |
| 545 | |
| 546 | **Files:** `cmd/console/main.go` and `server.go` (both masters into `Config`), |
| 547 | `internal/console/apiscan.go` (`regrantForScanning`, `apiscan.go:492`, is the |
| 548 | hook), `apisystem.go`, root `system.go`, `authcli.go`. |
| 549 | |
| 550 | - [x] `TestGrantDeliversWebhookKey` — sealed via `appconfig.Apply` |
| 551 | (`generation.go:119`), gated by `storeSealKey` (`apistore.go:425`). **Grant |
| 552 | stamps `webhook_key_id` as well as rotation does**, or the first |
| 553 | `rotate-webhook-key` after any grant re-seals and restarts every app under an |
| 554 | unchanged master. |
| 555 | - [x] `TestRotateWebhookKeySkipsUnchangedApps` — **not "re-seals identical |
| 556 | plaintext".** `appconfig.Seal` uses a fresh ephemeral key and nonce on every |
| 557 | call (`internal/appconfig/crypto.go:85-87`), so the ciphertext always differs |
| 558 | and Sync would restart the unit every time; the console holds only the public |
| 559 | key and cannot compare by opening the old value. Rotation records a **key |
| 560 | id** in the record — `ScanBlock` gains `webhook_key_id`, additive and safe |
| 561 | under the PARSER LIST rule — and skips any app already carrying the current |
| 562 | one. The test asserts the skip. |
| 563 | - [x] `TestRotateWebhookKeyDoesNotDropPrevious` — dropping `_PREVIOUS` is a |
| 564 | separate operator step, because "done" means sealed, not converged |
| 565 | (`server.go:566-569` makes this point for store rotation). |
| 566 | - [x] **`TestGrantFoldsBothSecretsIntoOneApply`** — a first grant writes the |
| 567 | store credential and the callback secret. Two `Apply` calls are two |
| 568 | generation bumps and therefore two unit restarts for one operator action. |
| 569 | - [x] **`TestRevokeUnsetsTheCallbackSecret`** — a key outliving the consent it |
| 570 | was issued under is the same mistake as a result object outliving it. |
| 571 | - [x] **`TestRotateIsBudgetedAndResumable`** — "paced" needs a mechanism, and |
| 572 | it is `scan sweep`'s: one budget per call, an answer saying how many apps |
| 573 | remain, the CLI loops. No background job, no pacing state; idempotence by key |
| 574 | id is what makes a re-run safe. |
| 575 | - [x] **`TestNoMasterRefusesACallbackPath`** — with no `CARLOS_SCAN_WEBHOOK_KEY` |
| 576 | the enqueue door refuses `callback_path` with 503 `callbacks_not_configured`. |
| 577 | A 202 would promise a delivery that cannot happen. (The door already accepts |
| 578 | the field, so this is a fix, not only an addition.) |
| 579 | - [x] **`TestPerAppKeyGenerationRotatesOneApp`** — `webhook_gen` in `ScanBlock`; |
| 580 | a tenant that leaks its key must not need a deployment-wide rotation. |
| 581 | - [x] **`TestReclaimedAppNameGetsADifferentKey`** — and this one needs |
| 582 | `webhook_gen` to be RANDOM rather than a counter. `PurgeApp` deletes |
| 583 | `store.json` with the rest of the app prefix (`trash.go:229-241`), so a |
| 584 | counter from zero restarts at zero for whoever claims the name next and |
| 585 | derives the identical key. Mint a random positive int below 2^53 at grant |
| 586 | and again on each per-app rotation. Zero stays legal: records predating this |
| 587 | have no field, their apps hold keys derived with zero, and |
| 588 | `rotate-webhook-key` is the backfill. |
| 589 | |
| 590 | **The app-side variable is NOT `CARLOS_SCAN_WEBHOOK_KEY`.** Same name for the |
| 591 | master and the derived child is a copy-paste footgun that ends with a master in |
| 592 | a tenant environment. Follow `EnvNames(prefix)` (`walk.go:598`): |
| 593 | `<PREFIX>_SCAN_CALLBACK_SECRET`. |
| 594 | |
| 595 | **Multi-console ordering, which the spec now states:** every console must be |
| 596 | restarted with `KEY=new, PREVIOUS=old` BEFORE `rotate-webhook-key` runs, or a |
| 597 | console still on the old master signs only under old while apps another console |
| 598 | re-sealed hold only new. |
| 599 | |
| 600 | **Commit:** `console: webhook key delivery and rotation` |
| 601 | |
| 602 | --- |
| 603 | |
| 604 | ## Task 16: delivery |
| 605 | |
| 606 | **Files:** `internal/console/scandeliver.go` (new — the worker pool), |
| 607 | `scandrain.go`, `server.go`. |
| 608 | |
| 609 | - [x] **`TestDeliveryDoesNotBlockTheTick`** — review 7's blocker. Delivery in |
| 610 | the drain's goroutine puts a tenant's hanging endpoint in the path of every |
| 611 | other tenant's verdict: 50 items times a bounded retry schedule, inside a |
| 612 | tick that sweeps apps sequentially. That is the starvation §7.2 and §7.3 |
| 613 | exist to prevent and a trivial tenant-triggered DoS. A bounded worker pool |
| 614 | fed from `done` after `scanNotify`, a bounded queue (overflow drops and logs |
| 615 | — best effort by construction), a per-app in-flight cap, retries scheduled by |
| 616 | the pool. |
| 617 | - [x] `TestCallbackGoesToPlatformHostOnly` — never a custom domain. |
| 618 | - [x] `TestCallbackRefusesRedirects` — Go follows them by default and a tenant |
| 619 | 302 turns a signed POST into a GET at an internal address |
| 620 | (`sesevents.go:143-158` is the precedent). |
| 621 | - [x] **`TestTransportDoesNotSkipTLSVerification`** — a tested ABSENCE, because |
| 622 | TLS name verification is the second lock that makes DNS rebinding irrelevant |
| 623 | (§6.2). Needs `Config.CallbackTransport http.RoundTripper` (nil = production) |
| 624 | as the test seam; name it, or the test cannot intercept an https POST. |
| 625 | - [x] **`TestNeverPostsUnsigned`** — with no signer there is no request. |
| 626 | - [x] `TestCallbackRetriesAreBounded` — 2xx success; 3xx and 4xx not retried; |
| 627 | 5xx/429/timeout retried, three attempts at 1s/4s/16s; 10s client timeout; |
| 628 | response body drained through a 4KiB `LimitReader` and discarded. |
| 629 | - [x] `TestDroppedCallbackStillReadableFromStatusDoor` — the §13 promise that |
| 630 | makes lazy retries acceptable. |
| 631 | - [x] `TestCallbackBodyCarriesRecorded` — or a callback saying `clean` and a |
| 632 | door saying `recorded:false` disagree. |
| 633 | - [x] **`TestCallbackBodyOmitsTheVersionToken`** — §11.7. The body carries |
| 634 | account, app, key, the bare ETag, verdict, recorded, and provenance; never |
| 635 | the opaque version. |
| 636 | - [x] **`TestNoPlatformHostIsNotAnError`** — an app with no ready route has |
| 637 | nowhere to be called back yet; it polls. Refuse at the door rather than |
| 638 | building `https:///hooks`. |
| 639 | |
| 640 | **Commit:** `console: callback delivery, bounded and backstopped by the door` |
| 641 | |
| 642 | --- |
| 643 | |
| 644 | ## Task 17: metering, surfaces and docs |
| 645 | |
| 646 | **Files:** `internal/usage/scan.go` + **`scan_test.go` (new)**, |
| 647 | `internal/console/scandrain.go` (the per-tick fold), `apiscan.go`, |
| 648 | **`apisystem.go`**, root `scan.go`, **root `system.go`**, root `store.go`, |
| 649 | `docs/site/cli.md`, |
| 650 | `docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`. |
| 651 | |
| 652 | - [x] `TestScanUsageCountsOnDemandOnly` — sweep scans are not attributed to a |
| 653 | tenant who did not ask for them. |
| 654 | - [x] `TestScanUsageFoldsPerTick` — not per object. `Drained.Scanned` and |
| 655 | `Size` already exist for this; the fold accumulates per account across |
| 656 | `drainScanApp` calls and writes once at the end of `scanDrainTick`. |
| 657 | - [x] `TestStoreScanStatusReportsDeploymentState` — a per-app record saying |
| 658 | `enabled: clamav` while the deployment switch is off is a new way for the |
| 659 | status output to lie. |
| 660 | - [ ] `TestPrintScanStatusShowsQueueDepth` (per app, `objstore.QueueDepth`) and |
| 661 | **`TestPrintSystemAntivirusStatusShowsQueueDepth`** (deployment-wide, |
| 662 | `Server.ScanQueueDepth`, §4.4). Both landed in task 12; this surfaces them. |
| 663 | Note the deployment number counts APPS WITH WORK, and an app's entry survives |
| 664 | one extra tick after its last item drains — say "apps with work", not "apps |
| 665 | with items", or it reads as off by one. |
| 666 | |
| 667 | **Docs must carry, and this is review 7's list:** |
| 668 | - *If no callback arrives within your own timeout, poll the status door.* |
| 669 | Delivery is best effort and may be lost or duplicated; the callback is an accelerator, not the |
| 670 | delivery mechanism. |
| 671 | - The idempotency tuple: `(account, app, key, etag, verdict)`. |
| 672 | - The signature is over the RAW BODY BYTES. |
| 673 | - `store scan grant` and `rotate-webhook-key` RESTART apps as a side effect. |
| 674 | - The corrected platform-host sentence (§6.2): an app with no ready route has |
| 675 | no platform host yet — not "an app with only its own domain". |
| 676 | |
| 677 | `usage/scan/<date>.json`, CAS-merged as `UpsertEdgeDay` (`edge.go:85-116`); |
| 678 | meter names frozen once merged (`usage.go:4-5`), so they are named in this task |
| 679 | and not later. Rollout plan step 4 becomes `CARLOS_CLAMD_ADDR` + `enable`; |
| 680 | rollback becomes `disable`, no restart. |
| 681 | |
| 682 | **Commit:** `console+cli: scan metering, surfaces and docs` |
| 683 | |
| 684 | --- |
| 685 | |
| 686 | --- |
| 687 | |
| 688 | ## Where this stands (2026-09-08) |
| 689 | |
| 690 | Tasks 1-17 are implemented on `worktree-store-scan-on-demand`. 14-17 were |
| 691 | written by codex (gpt-5.6-sol) against this plan, then mutation-tested and in |
| 692 | several places re-tested here; every commit says which parts came from where. |
| 693 | |
| 694 | **What is NOT done, and why.** The two printer tests above, and the docs, need |
| 695 | copy a person has approved — a printer line is a sentence somebody reads, and |
| 696 | this project reviews user-facing language before it ships. Three pieces are |
| 697 | waiting: |
| 698 | |
| 699 | 1. The CLI reference for both `rotate-webhook-key` verbs (drafted, held out of |
| 700 | the commits, in `cli-copy-for-review.diff`). |
| 701 | 2. Two status lines: `queue: N item(s) waiting` and `queue: N apps with work`. |
| 702 | 3. **The tenant-facing half**, which the final review argues should land WITH |
| 703 | the merge rather than after it: once merged, the enqueue door accepts |
| 704 | `callback_path` from any Service credential and the console will POST signed |
| 705 | bodies to apps that have no documented way to verify them. That page must |
| 706 | carry `VerifyCallback`, and it must say that the delivered secret is HEX |
| 707 | while the MAC is keyed on the raw bytes — an author who feeds the |
| 708 | environment string straight into HMAC fails every verification, and the |
| 709 | next move after that is to turn verification off. |
| 710 | |
| 711 | **Known and recorded, not fixed.** A tag verdict answers without `at`, and |
| 712 | `skipped` without a reason, because the `Scanner.Status` seam carries neither; |
| 713 | widening it is its own task and is the same seam §5.2's example assumes. |
| 714 | Metering counts a size-ceiling `skipped` as bytes, which is wrong and unpriced |
| 715 | — fix before pricing. The long-poll checks consent once for up to 30s of |
| 716 | waiting, answering from tags rather than bytes. |
| 717 | |
| 718 | ## Remaining §13 coverage, assigned |
| 719 | |
| 720 | First-tick warning when wired with no record → task 4. `request`/`revoke`/ |
| 721 | `enforce off` still work while **disabled** → extend |
| 722 | `TestScanRevokeAndEnforceOffWorkWithoutScanner` in task 5. Enqueue racing |
| 723 | `PurgeApp` leaves no orphan beyond one tick → task 12. |
| 724 | |
| 725 | ## Sequencing |
| 726 | |
| 727 | 1, 2, 3 are independent. 4 needs 2 and 3. 5 needs 4. 7 and 8 are independent |
| 728 | seams. **9 (the result object) is independent and comes before 10 (the queue |
| 729 | core), which needs 7, 8 and 9** — draft 2 had these the other way round, which |
| 730 | would have landed a `DrainApp` that drops unrecorded verdicts. 11 needs 1, 2, |
| 731 | 10. 12 needs 10. 13 needs 7, 9, 12. 14 is pure. 15 needs 14. 16 needs 12, 14, |
| 732 | 15. 17 last, so the docs describe what exists. |
| 733 | |
| 734 | **Docs hold until task 17.** The `carlos store scan` section drafted for the |
| 735 | skills repo describes hourly-sweep behaviour and is in copy review; it is |
| 736 | revised here rather than shipped twice. |
| 737 | |
amadan