amadan
plan: tasks 14-17 are in, and what is still waiting on a person
Ticks what landed and, more usefully, records the three pieces that cannot land without approved copy — including the tenant-facing page the final review argues should ship WITH the merge rather than after it, because the enqueue door accepts callback_path the moment this merges and the console will POST signed bodies to apps with no documented way to verify them. Also records what is known-and-unfixed here rather than leaving it in a review transcript nobody will find: no `at` on a tag verdict and no reason on `skipped` (the Status seam carries neither), a size-ceiling `skipped` metered as bytes, and the long-poll checking consent once for the length of its wait.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+59
−27
docs/superpowers/plans/2026-09-02-scan-on-demand.md+59 −27
CI passed — run details
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 a883f2976..8e8f61eb5 100644 |
| --- a/docs/superpowers/plans/2026-09-02-scan-on-demand.md |
| +++ b/docs/superpowers/plans/2026-09-02-scan-on-demand.md |
| @@ -512,19 +512,19 @@ of it was written. The additions below are all its findings. |
| **Files:** `internal/console/scansign.go` (new — signing is its own file, not |
| more of the path validator) + test. |
| -- [ ] `TestSignatureRejectsTamperedBody`, `TestSignatureIsPerApp` (app A's does |
| +- [x] `TestSignatureRejectsTamperedBody`, `TestSignatureIsPerApp` (app A's does |
| not verify for app B), `TestSignatureRejectsStaleTimestamp`, |
| `TestTwoMasterRotationVerifies`. |
| -- [ ] **`VerifyCallback(key, header, body, now)` — the reference verifier**, and |
| +- [x] **`VerifyCallback(key, header, body, now)` — the reference verifier**, and |
| it is the deliverable §11.3 was missing. "A stale timestamp is refused" and |
| "both masters verify" are properties of the APP's verifier, which is tenant |
| code; without a reference implementation the tests test nothing anybody runs. |
| The docs reproduce it line for line. |
| -- [ ] `TestSignatureIsOverRawBodyBytes` — an app that re-serialises the JSON |
| +- [x] `TestSignatureIsOverRawBodyBytes` — an app that re-serialises the JSON |
| before verifying gets a different byte string and fails. Said in the docs. |
| -- [ ] `TestVerifierToleratesTwoSignaturesAndUnknownKeyIds` — the rotation |
| +- [x] `TestVerifierToleratesTwoSignaturesAndUnknownKeyIds` — the rotation |
| window depends on it. |
| -- [ ] `TestPreviousEqualToCurrentIsRefused` — a rotation that rotated to |
| +- [x] `TestPreviousEqualToCurrentIsRefused` — a rotation that rotated to |
| nothing, refused exactly as `NewTagSigner` refuses it. |
| Per-app key `HMAC(master, "carlos-scan-webhook-v1" + 0x00 + sqid + "/" + app + |
| @@ -547,12 +547,12 @@ label and the per-app `gen` are review 7's; so is the key id's distinct label. |
| `internal/console/apiscan.go` (`regrantForScanning`, `apiscan.go:492`, is the |
| hook), `apisystem.go`, root `system.go`, `authcli.go`. |
| -- [ ] `TestGrantDeliversWebhookKey` — sealed via `appconfig.Apply` |
| +- [x] `TestGrantDeliversWebhookKey` — sealed via `appconfig.Apply` |
| (`generation.go:119`), gated by `storeSealKey` (`apistore.go:425`). **Grant |
| stamps `webhook_key_id` as well as rotation does**, or the first |
| `rotate-webhook-key` after any grant re-seals and restarts every app under an |
| unchanged master. |
| -- [ ] `TestRotateWebhookKeySkipsUnchangedApps` — **not "re-seals identical |
| +- [x] `TestRotateWebhookKeySkipsUnchangedApps` — **not "re-seals identical |
| plaintext".** `appconfig.Seal` uses a fresh ephemeral key and nonce on every |
| call (`internal/appconfig/crypto.go:85-87`), so the ciphertext always differs |
| and Sync would restart the unit every time; the console holds only the public |
| @@ -560,25 +560,25 @@ hook), `apisystem.go`, root `system.go`, `authcli.go`. |
| id** in the record — `ScanBlock` gains `webhook_key_id`, additive and safe |
| under the PARSER LIST rule — and skips any app already carrying the current |
| one. The test asserts the skip. |
| -- [ ] `TestRotateWebhookKeyDoesNotDropPrevious` — dropping `_PREVIOUS` is a |
| +- [x] `TestRotateWebhookKeyDoesNotDropPrevious` — dropping `_PREVIOUS` is a |
| separate operator step, because "done" means sealed, not converged |
| (`server.go:566-569` makes this point for store rotation). |
| -- [ ] **`TestGrantFoldsBothSecretsIntoOneApply`** — a first grant writes the |
| +- [x] **`TestGrantFoldsBothSecretsIntoOneApply`** — a first grant writes the |
| store credential and the callback secret. Two `Apply` calls are two |
| generation bumps and therefore two unit restarts for one operator action. |
| -- [ ] **`TestRevokeUnsetsTheCallbackSecret`** — a key outliving the consent it |
| +- [x] **`TestRevokeUnsetsTheCallbackSecret`** — a key outliving the consent it |
| was issued under is the same mistake as a result object outliving it. |
| -- [ ] **`TestRotateIsBudgetedAndResumable`** — "paced" needs a mechanism, and |
| +- [x] **`TestRotateIsBudgetedAndResumable`** — "paced" needs a mechanism, and |
| it is `scan sweep`'s: one budget per call, an answer saying how many apps |
| remain, the CLI loops. No background job, no pacing state; idempotence by key |
| id is what makes a re-run safe. |
| -- [ ] **`TestNoMasterRefusesACallbackPath`** — with no `CARLOS_SCAN_WEBHOOK_KEY` |
| +- [x] **`TestNoMasterRefusesACallbackPath`** — with no `CARLOS_SCAN_WEBHOOK_KEY` |
| the enqueue door refuses `callback_path` with 503 `callbacks_not_configured`. |
| A 202 would promise a delivery that cannot happen. (The door already accepts |
| the field, so this is a fix, not only an addition.) |
| -- [ ] **`TestPerAppKeyGenerationRotatesOneApp`** — `webhook_gen` in `ScanBlock`; |
| +- [x] **`TestPerAppKeyGenerationRotatesOneApp`** — `webhook_gen` in `ScanBlock`; |
| a tenant that leaks its key must not need a deployment-wide rotation. |
| -- [ ] **`TestReclaimedAppNameGetsADifferentKey`** — and this one needs |
| +- [x] **`TestReclaimedAppNameGetsADifferentKey`** — and this one needs |
| `webhook_gen` to be RANDOM rather than a counter. `PurgeApp` deletes |
| `store.json` with the rest of the app prefix (`trash.go:229-241`), so a |
| counter from zero restarts at zero for whoever claims the name next and |
| @@ -606,7 +606,7 @@ re-sealed hold only new. |
| **Files:** `internal/console/scandeliver.go` (new — the worker pool), |
| `scandrain.go`, `server.go`. |
| -- [ ] **`TestDeliveryDoesNotBlockTheTick`** — review 7's blocker. Delivery in |
| +- [x] **`TestDeliveryDoesNotBlockTheTick`** — review 7's blocker. Delivery in |
| the drain's goroutine puts a tenant's hanging endpoint in the path of every |
| other tenant's verdict: 50 items times a bounded retry schedule, inside a |
| tick that sweeps apps sequentially. That is the starvation §7.2 and §7.3 |
| @@ -614,26 +614,26 @@ re-sealed hold only new. |
| fed from `done` after `scanNotify`, a bounded queue (overflow drops and logs |
| — best effort by construction), a per-app in-flight cap, retries scheduled by |
| the pool. |
| -- [ ] `TestCallbackGoesToPlatformHostOnly` — never a custom domain. |
| -- [ ] `TestCallbackRefusesRedirects` — Go follows them by default and a tenant |
| +- [x] `TestCallbackGoesToPlatformHostOnly` — never a custom domain. |
| +- [x] `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). |
| -- [ ] **`TestTransportDoesNotSkipTLSVerification`** — a tested ABSENCE, because |
| +- [x] **`TestTransportDoesNotSkipTLSVerification`** — a tested ABSENCE, because |
| TLS name verification is the second lock that makes DNS rebinding irrelevant |
| (§6.2). Needs `Config.CallbackTransport http.RoundTripper` (nil = production) |
| as the test seam; name it, or the test cannot intercept an https POST. |
| -- [ ] **`TestNeverPostsUnsigned`** — with no signer there is no request. |
| -- [ ] `TestCallbackRetriesAreBounded` — 2xx success; 3xx and 4xx not retried; |
| +- [x] **`TestNeverPostsUnsigned`** — with no signer there is no request. |
| +- [x] `TestCallbackRetriesAreBounded` — 2xx success; 3xx and 4xx not retried; |
| 5xx/429/timeout retried, three attempts at 1s/4s/16s; 10s client timeout; |
| response body drained through a 4KiB `LimitReader` and discarded. |
| -- [ ] `TestDroppedCallbackStillReadableFromStatusDoor` — the §13 promise that |
| +- [x] `TestDroppedCallbackStillReadableFromStatusDoor` — the §13 promise that |
| makes lazy retries acceptable. |
| -- [ ] `TestCallbackBodyCarriesRecorded` — or a callback saying `clean` and a |
| +- [x] `TestCallbackBodyCarriesRecorded` — or a callback saying `clean` and a |
| door saying `recorded:false` disagree. |
| -- [ ] **`TestCallbackBodyOmitsTheVersionToken`** — §11.7. The body carries |
| +- [x] **`TestCallbackBodyOmitsTheVersionToken`** — §11.7. The body carries |
| account, app, key, the bare ETag, verdict, recorded, and provenance; never |
| the opaque version. |
| -- [ ] **`TestNoPlatformHostIsNotAnError`** — an app with no ready route has |
| +- [x] **`TestNoPlatformHostIsNotAnError`** — an app with no ready route has |
| nowhere to be called back yet; it polls. Refuse at the door rather than |
| building `https:///hooks`. |
| @@ -649,12 +649,12 @@ re-sealed hold only new. |
| `docs/site/cli.md`, |
| `docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`. |
| -- [ ] `TestScanUsageCountsOnDemandOnly` — sweep scans are not attributed to a |
| +- [x] `TestScanUsageCountsOnDemandOnly` — sweep scans are not attributed to a |
| tenant who did not ask for them. |
| -- [ ] `TestScanUsageFoldsPerTick` — not per object. `Drained.Scanned` and |
| +- [x] `TestScanUsageFoldsPerTick` — not per object. `Drained.Scanned` and |
| `Size` already exist for this; the fold accumulates per account across |
| `drainScanApp` calls and writes once at the end of `scanDrainTick`. |
| -- [ ] `TestStoreScanStatusReportsDeploymentState` — a per-app record saying |
| +- [x] `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` (per app, `objstore.QueueDepth`) and |
| @@ -683,6 +683,38 @@ rollback becomes `disable`, no restart. |
| --- |
| +--- |
| + |
| +## Where this stands (2026-09-08) |
| + |
| +Tasks 1-17 are implemented on `worktree-store-scan-on-demand`. 14-17 were |
| +written by codex (gpt-5.6-sol) against this plan, then mutation-tested and in |
| +several places re-tested here; every commit says which parts came from where. |
| + |
| +**What is NOT done, and why.** The two printer tests above, and the docs, need |
| +copy a person has approved — a printer line is a sentence somebody reads, and |
| +this project reviews user-facing language before it ships. Three pieces are |
| +waiting: |
| + |
| +1. The CLI reference for both `rotate-webhook-key` verbs (drafted, held out of |
| + the commits, in `cli-copy-for-review.diff`). |
| +2. Two status lines: `queue: N item(s) waiting` and `queue: N apps with work`. |
| +3. **The tenant-facing half**, which the final review argues should land WITH |
| + the merge rather than after it: once merged, the enqueue door accepts |
| + `callback_path` from any Service credential and the console will POST signed |
| + bodies to apps that have no documented way to verify them. That page must |
| + carry `VerifyCallback`, and it must say that the delivered secret is HEX |
| + while the MAC is keyed on the raw bytes — an author who feeds the |
| + environment string straight into HMAC fails every verification, and the |
| + next move after that is to turn verification off. |
| + |
| +**Known and recorded, not fixed.** A tag verdict answers without `at`, and |
| +`skipped` without a reason, because the `Scanner.Status` seam carries neither; |
| +widening it is its own task and is the same seam §5.2's example assumes. |
| +Metering counts a size-ceiling `skipped` as bytes, which is wrong and unpriced |
| +— fix before pricing. The long-poll checks consent once for up to 30s of |
| +waiting, answering from tags rather than bytes. |
| + |
| ## Remaining §13 coverage, assigned |
| First-tick warning when wired with no record → task 4. `request`/`revoke`/ |