amadan
spec: draft 7 — cleared to plan from
Review 6 returned no blockers. These are its two medium items and the smaller corrections, folded in so the plan does not have to invent them. The result object needed its write semantics stated. release.Store.Put has exactly two modes, and the second write for a key is a REPLACE — v1 unrecordable, tenant uploads v2, v2 also unrecordable — so a create-only write there conflicts and silently drops the v2 verdict, leaving the door on pending. Get, then replace with the etag or create when absent. And the drain now age-sweeps scan-results for each app it visits, not only at boot: a console that never restarts would otherwise accumulate them. Age collection needs the optional ModLister, so it type-asserts with a logged fallback the way storagePass already does for SizeLister. One correction worth its own line, because it makes an earlier guard load-bearing rather than prudent. I said the full-prefix passes all skip unknown key shapes. materialize does not, deliberately: it counts any key under an app prefix and restores a trashed row to live when it finds one. So a result object stranded under a purged prefix does not merely block the name claim — it resurrects the app. That is the safe direction for materialize and the reason §5.2.1's store.json re-read is not optional. Also: §11.9 and §7.4 disagreed once the drain started re-issuing failed and denied — a test written from §11.9's old wording would have failed §7.4. Status now says what it returns when HeadObject itself is denied. The callback body carries `recorded`, or a callback saying clean and a door saying recorded:false disagree in exactly the way §5.2.1 exists to prevent. And §13 said boot where §4.2.1 says first tick.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+56
−13
docs/superpowers/specs/2026-09-02-scan-on-demand-design.md+56 −13
diff --git a/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md b/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md| index 6910a9069..32f903f16 100644 |
| --- a/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md |
| +++ b/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md |
| @@ -1,6 +1,6 @@ |
| # Scan on demand: design |
| -**Status:** draft 6 (closes reviews 1-5). |
| +**Status:** draft 7 — final, cleared to plan from (closes reviews 1-6). |
| **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| @@ -423,7 +423,14 @@ exactly what keeping the version token inside the scanner forbids. Draft 2 had |
| the same hole under a different name. |
| `Status` needs `HeadObject`, which `scanS3API` (`clamav.go:25-29`) and |
| -`stubScanS3` (`clamav_test.go:24`) both gain. `memscanner.go` and |
| +`stubScanS3` (`clamav_test.go:24`) both gain. |
| + |
| +**When `HeadObject` itself is denied** there is no version or size to hand |
| +`Scan`, so `Status` returns `(VerdictDenied, ScanObject{}, nil)` and the drain |
| +does not call `Scan` on a zero `ScanObject`. The SSE-KMS case the landed |
| +comment names behaves differently and correctly: `HeadObject` succeeds, |
| +`GetObject` is denied, so the drain gets a populated object, scans, and gets |
| +`Denied` from the fetch. `memscanner.go` and |
| `scanner_contract_test.go` follow: `MemScanner` has no object model, so it |
| gains a seeding call — `PutObject(bucket, key, etag string, modified |
| time.Time, size int64)` — and the contract test asserts an absent object gives |
| @@ -477,7 +484,8 @@ writes a **key-derived result object** and deletes the queue item as normal: |
| ``` |
| apps/<acct>/<app>/scan-results/<sha256(key)>.json |
| {"schema":1, "key":"uploads/x.pdf", "verdict":"clean", |
| - "version":"<etag>@<RFC3339>", "at":"..."} |
| + "version":"<etag>@<RFC3339>", "engine":"clamav", "signature":"...", |
| + "at":"..."} |
| ``` |
| **It carries the version, and that is not optional.** Draft 5 keyed it by |
| @@ -495,6 +503,18 @@ is what §5.2 requires. |
| - under the app prefix, so `PurgeApp` and the claim gate cover it (§7.1's |
| argument for keeping the queue there applies unchanged) |
| +**Written read-then-CAS.** `release.Store.Put` has exactly two modes |
| +(`release/store.go:37-45`): create-only with `ifMatch ""`, or replace with an |
| +etag. The second write to the same key is a *replace* — v1's result object is |
| +present, the tenant uploads v2, v2 is scanned and again unrecordable — so a |
| +create-only write there returns `ErrCASConflict` and silently drops the v2 |
| +verdict, leaving the door on `pending` until a sweep tags it. Get, then Put |
| +with the etag if present or create-only if absent, retrying once on conflict: |
| +the `writeSidecar` posture at `sweep.go:370-395`. |
| + |
| +It carries the engine and signature alongside the verdict, because the |
| +`recorded:false` answer is still an audit answer and needs its provenance. |
| + |
| **Written with the same guard as a sidecar.** The drain re-reads `store.json` |
| immediately before writing, the way `writeSidecar` does (`sweep.go:358-367`), |
| because the identical race applies: read the record, scan for seconds, a |
| @@ -512,6 +532,14 @@ makes the app name permanently unclaimable (`server.go:2921`). |
| - swept by the boot rebuild (§7.1), which lists `scan-results/` as well as |
| `scan-queue/` and collects anything older than the TTL by `ListMod` age — |
| the name carries no timestamp, so age has to come from the store |
| +- and swept the same way by the drain for each app it visits, so a console that |
| + never restarts does not accumulate them. Note a *sweep* that later tags the |
| + object does not delete the result object; only the on-demand drain does. |
| + |
| +Age-based collection needs `release.ModLister`, which is optional and |
| +unimplemented by the test fakes. Type-assert it and fall back to skipping age |
| +collection with a single logged line, exactly as `storagePass` does for |
| +`SizeLister` (`economicspass.go:141-145`). |
| **TTL for both kinds is 24h**, one sweep cycle: after that the ordinary sweep |
| has had its chance to write a real verdict, and a stale on-demand answer is |
| @@ -621,6 +649,8 @@ unit** on a bundle change (`sync.go:33,42,331`). So: |
| restart anything: Sync restarts only when the rendered bundle changes |
| (`sync.go:340+`). |
| - constant-time compare; skew window 5 minutes |
| +- the body carries `recorded`, so a callback saying `clean` and a door saying |
| + `recorded:false` cannot disagree — the disagreement §5.2.1 exists to prevent |
| - the body carries account, app, key, the **bare ETag** *and* the opaque |
| version. Draft 2 carried only the version, contradicting §5.2's rule that |
| apps must not interpret it — the app has the ETag from its own PUT response |
| @@ -761,9 +791,11 @@ when the app is in the trash; and when the record no longer permits scanning — |
| `Scanning.Enabled == nil`. None of those can change back in a way that makes |
| the item useful. |
| -**Keep the item** when the deployment record is `disabled`, on engine drift, on |
| -an unreachable engine, and on any read error. All of those are conditions an |
| -operator can reverse. |
| +**Keep the item — up to the TTL** when the deployment record is `disabled`, on |
| +engine drift, on an unreachable engine, and on any read error. All of those are |
| +conditions an operator can reverse. A deployment disabled for longer than a day |
| +drops its queue, which is right: both doors answered `disabled` at the time, |
| +and the sweep re-covers those objects once it is re-enabled. |
| Deletion is driven by **absence**, never by error — and absence at the release |
| store layer is `os.ErrNotExist` (`trash.go:64`), not a nil value, so queue |
| @@ -881,7 +913,9 @@ Each is a test. |
| `revoke`, `sweep` or `enforce`. |
| 7. Version comparison is inside the scanner; no door or app sees the token. |
| 8. Enqueue and drain both enforce the consent chain (§5.1.1). |
| -9. Rate limits precede enqueue; a re-request of a judged key costs a tag read. |
| +9. Rate limits precede enqueue; a re-request of a key judged **clean, infected |
| + or skipped** costs a tag read and no fetch. (`failed` and `denied` are |
| + deliberately rescanned — §7.4.) |
| 10. A verdict that could not be persisted is reported as `recorded:false`, |
| never as `pending` and never as a silent success (§5.2.1); and a `failed` |
| or `denied` verdict is reported as itself, never as `pending` (§5.2). |
| @@ -912,6 +946,8 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| - `request`, `revoke`, `enforce off` still work while disabled |
| - boot accepts `CARLOS_STORE_SCANNER=clamav` as a deprecated wiring alias, |
| retaining the socket default; any other value stays fatal (§4.2.1) |
| +- the **first pass tick** — not boot — seeds the record and warns when a |
| + Scanner is wired with no record |
| - a `failed` and a `denied` verdict each answer as themselves, not `pending` |
| - unknown `system/antivirus.json` schema refuses |
| - metering counts on-demand scans and not sweep scans |
| @@ -929,10 +965,11 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| collected by the boot rebuild once past TTL |
| - the drain rescans an object whose tag says `failed` or `denied`, rather than |
| answering from the tag (§7.4) |
| -- boot warns when a Scanner is wired with no record; `enable` 503s when no |
| - Scanner is wired |
| -- boot seeds the record once from the retired variable, and does **not** |
| - override an existing record (§4.2.1) |
| +- `enable` 503s when no Scanner is wired |
| +- the seed happens once from the retired variable and does **not** override an |
| + existing record (§4.2.1) |
| +- a second result object for the same key **replaces** the first (§5.2.1), and |
| + does not fail create-only |
| - the drain keeps items and the index entry on a record **read error**, and |
| deletes only on absence |
| @@ -953,8 +990,14 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| `BucketBytes`, and the three full `List("apps/")` passes |
| (`provision/reconcile.go:586,1050`, `domains/reconcile.go:87`, |
| `materialize.go:292`) grow with a flooded queue until the TTL collects it. |
| - All of them skip unknown key shapes correctly, so this is cost, not |
| - breakage — named so it is measured. |
| + `provision` and `domains` skip unknown key shapes correctly, so for them |
| + this is cost, not breakage. **`materialize` does not, by design**: it counts |
| + *any* key under an app prefix (`materialize.go:333-345`, "convergence fails |
| + toward KEEPING the row") and restores a trashed row to live when it finds one |
| + (`:377-382`). So a result object stranded under a purged prefix does not only |
| + block the claim gate — it **resurrects the app row** on the next boot. That |
| + is the safe direction for materialize, and one more reason §5.2.1's purge |
| + guard is not optional. |
| - **Index size is bounded** by the number of apps that have enqueued since boot |
| and not yet been seen empty — at most the number of provisioned stores, a few |
| dozen bytes each. Trashed and purged apps clear on their next empty listing, |