| 1 | # Whole-branch code review brief: Plans 1 (catalog and account limits) |
| 2 | |
| 3 | You are a senior Go reviewer on the carlos platform. Review the |
| 4 | implementation of Plan 1 on branch `design-plans-and-caps`, range |
| 5 | `3d444a1b..HEAD` (`git diff 3d444a1b..HEAD`; ignore the docs under |
| 6 | docs/superpowers/ except as requirements). The review is read-only: do not |
| 7 | modify the working tree, index, HEAD or branches, and do not build into the |
| 8 | repo. You may run `go test` for packages you want to probe. |
| 9 | |
| 10 | Requirements, in order of authority: |
| 11 | 1. Spec: docs/superpowers/specs/2026-10-20-plans-caps-packs-design.md |
| 12 | (sections 4, 7, 8.7, 9.4, 10 are what Plan 1 implements; section 3 |
| 13 | lists product decisions that are not to be argued with). |
| 14 | 2. Plan: docs/superpowers/plans/2026-10-20-plans-1-catalog-and-limits.md. |
| 15 | 3. The executor's rulings (deviations from the plan, each with its cost): |
| 16 | `.superpowers/sdd/2026-10-20-plans-1-catalog-and-limits/progress.md`, |
| 17 | lines containing `Ruling:`. Weigh each; say if any is wrong. |
| 18 | |
| 19 | The spec is a vision document. For behaviour it is silent on, judge by what |
| 20 | a reasonable operator or customer would expect, and grade by effect. |
| 21 | |
| 22 | Check these deliberately (the plan's Review Focus, inputs its tests may not |
| 23 | exercise): |
| 24 | 1. A catalog edited without a version bump: the console must refuse to |
| 25 | start or treat it as new, never publish records whose catalog_id lies. |
| 26 | 2. A catalog change that orphans an assignment (plan or pack removed): skip |
| 27 | that account, keep its last record, keep publishing everyone else. |
| 28 | 3. A deleted (tombstoned) account: no row ensured, no record written. |
| 29 | 4. An operator change racing the publish pass: the newer revision wins in |
| 30 | the bucket and in published_revision; a stale publish never marks a |
| 31 | newer revision published. |
| 32 | 5. Partial sponsored limits: unset fields leave the plan's value; |
| 33 | min_schedule_seconds 0 must not mean "no minimum". |
| 34 | |
| 35 | Also look hard at: the host-claim boundary (hostops classifier entry for |
| 36 | accounts/<sqid>/limits.json, claim fixtures and probes), behaviour with |
| 37 | CARLOS_PLAN_CATALOG unset (must be exactly today's behaviour), the operator |
| 38 | door's authorization (fleet operators only; everyone else gets the stdlib |
| 39 | 404), CAS and retry correctness in publishAccountLimits, SQL in |
| 40 | accountplans.go, and cross-compilation (darwin/windows). |
| 41 | |
| 42 | Do not review the wording of human-facing strings: they passed Paul's copy |
| 43 | review and are applied verbatim (accounts_plan_copy.go, |
| 44 | internal/console/apiaccountplan_copy.go, docs/site/plans.md, the cli.md |
| 45 | entry, nav.json, the accounts usage block). |
| 46 | |
| 47 | Output format: |
| 48 | - Findings, most severe first, each labelled Critical / Important / Minor, |
| 49 | with file:line, the concrete failure scenario, and the fix you suggest. |
| 50 | - "Declined to judge": every behaviour you considered and set aside as |
| 51 | outside the plan or spec, one line each with the reason. |
| 52 | - A verdict: ready to merge, ready with fixes, or not ready. |
| 53 | |
amadan