Add reviews for Bootstrap-wizard plans v3.2 to v3.4

- Review v3.2: Addressed executor model, fire-and-forget issues, and PAT handling.
- Review v3.3: Improved transaction handling, stale recovery, and ID generation.
- Review v3.4: Finalized GitHub permissions, catalog versioning, and E2E verification queries.
- Updated recommendations for each version to enhance implementation readiness.
This commit is contained in:
Janpeter Visser 2026-05-14 01:20:53 +02:00
parent 9f8d41518a
commit e0ea1fe12b
7 changed files with 1780 additions and 1 deletions

View file

@ -2,7 +2,7 @@
# Documentation Index
Auto-generated on 2026-05-11 from front-matter and headings.
Auto-generated on 2026-05-13 from front-matter and headings.
## Architecture Decision Records
@ -43,6 +43,8 @@ Auto-generated on 2026-05-11 from front-matter and headings.
| [Plan: model + mode-selectie per ClaudeJob-kind](./plans/job-model-selection.md) | — | — |
| [Verbeterplan load/render Product Backlog, Sprint en Solo](./plans/load-render-improvement-plan-2026-05-10.md) | draft | 2026-05-10 |
| [M12 — Idea entity + Grill/Plan Claude jobs](./plans/M12-ideas.md) | planned | — |
| [Plan v3.5 — Bootstrap-wizard voor nieuwe Product-repo (Scrum4Me feature)](./plans/M8-bootstrap-wizard.md) | reviewed | — |
| [PBI-80 — Demo-gebruiker mag eigen UI-voorkeuren wijzigen](./plans/PBI-80-demo-prefs.md) | — | — |
| [Queue-loop verplaatsen van Claude naar runner](./plans/queue-loop-extraction.md) | — | — |
| [Sprint MCP-tools — create_sprint & update_sprint](./plans/sprint-mcp-tools.md) | draft | 2026-05-11 |
| [Advies - SprintRun, PR en worktree lifecycle als state machines](./plans/sprint-pr-worktree-state-machines.md) | draft | 2026-05-06 |
@ -112,6 +114,11 @@ Auto-generated on 2026-05-11 from front-matter and headings.
| [Scrum4Me — API Test Plan](./qa/api-test-plan.md) | `qa/api-test-plan.md` | active | 2026-05-03 |
| [Realtime smoke-checklist — PBI / Story / Task](./realtime-smoke.md) | `realtime-smoke.md` | active | 2026-05-03 |
| [Caveman plan — Beelink naar Ubuntu Scrum4Me server](./recommendations/beelink-ubuntu-scrum4me-server-caveman-plan.md) | `recommendations/beelink-ubuntu-scrum4me-server-caveman-plan.md` | draft | 2026-05-09 |
| [Review - Bootstrap-wizard plan](./recommendations/bootstrap-wizard-plan-review-2026-05-13.md) | `recommendations/bootstrap-wizard-plan-review-2026-05-13.md` | draft | 2026-05-13 |
| [Review - Bootstrap-wizard plan v2 met webresearch](./recommendations/bootstrap-wizard-plan-v2-web-research-review-2026-05-13.md) | `recommendations/bootstrap-wizard-plan-v2-web-research-review-2026-05-13.md` | draft | 2026-05-13 |
| [Review - Bootstrap-wizard plan v3.2](./recommendations/bootstrap-wizard-plan-v3-2-review-2026-05-14.md) | `recommendations/bootstrap-wizard-plan-v3-2-review-2026-05-14.md` | draft | 2026-05-14 |
| [Review - Bootstrap-wizard plan v3.3](./recommendations/bootstrap-wizard-plan-v3-3-review-2026-05-14.md) | `recommendations/bootstrap-wizard-plan-v3-3-review-2026-05-14.md` | draft | 2026-05-14 |
| [Review — M8 bootstrap-wizard plan v3.4](./recommendations/bootstrap-wizard-plan-v3-4-review-2026-05-14.md) | `recommendations/bootstrap-wizard-plan-v3-4-review-2026-05-14.md` | — | — |
| [Aanbeveling — Claude VM jobflow en gitstrategie](./recommendations/claude-vm-job-flow-git-strategy.md) | `recommendations/claude-vm-job-flow-git-strategy.md` | draft | 2026-05-09 |
| [Load/render implementatie review](./recommendations/load-render-implementation-review-2026-05-10.md) | `recommendations/load-render-implementation-review-2026-05-10.md` | review | 2026-05-10 |
| [Agent-flow: open issues & decision log](./runbooks/agent-flow-pitfalls.md) | `runbooks/agent-flow-pitfalls.md` | active | 2026-05-03 |

File diff suppressed because it is too large Load diff

View file

@ -0,0 +1,89 @@
---
title: "Review - Bootstrap-wizard plan"
status: draft
date: 2026-05-13
source_plan: "/Users/janpetervisser/.claude/plans/als-ik-een-nieuwe-virtual-turtle.md"
---
# Review - Bootstrap-wizard plan
## Korte conclusie
Het plan is functioneel sterk, maar niet uitvoerbaar zoals het nu geschreven is. De hoofdblokkade is dat `ClaudeJob` wordt gebruikt als deterministische queue, terwijl de huidige runner-architectuur `ClaudeJob` nog behandelt als een Claude CLI job met een verplicht model/config-pad. Trek dat eerst recht, anders eindigt de feature in typefouten, jobs die nooit terminal worden, of een worker die toch Claude probeert te starten.
## Bevindingen
### P1 - `BOOTSTRAP_REPO` met `model: null` breekt het huidige job-config contract
Het plan zet voor `BOOTSTRAP_REPO` expliciet `model: null` omdat er geen LLM draait (plan regels 101-106). In de huidige code is `JobConfig.model` niet nullable en beperkt tot `ClaudeModel`; `snapshotFromConfig` schrijft die waarde daarna naar `ClaudeJob.requested_model` als string (`lib/job-config.ts` regels 27-33 en 205-210). `getJobConfigSnapshot` is bovendien het bestaande enqueue-pad voor nieuwe jobs (`lib/job-config-snapshot.ts` regels 1-7 en 34-39).
Fix: maak deterministische jobs een expliciet ander runtime-pad. Bijvoorbeeld een discriminated union `runtime: 'claude' | 'deterministic'`, of laat `BOOTSTRAP_REPO` de Claude config snapshot volledig overslaan. Alleen een `KIND_DEFAULTS` entry met `model: null` is onvoldoende.
### P1 - De worker-eigenaar staat verkeerd of is te vaag
Het plan plaatst de dispatch in `scrum4me-mcp/src/lib/job-runner.ts` en noemt worker-bestanden in `scrum4me-mcp` (plan regels 172 en 235-238). De actuele runner-architectuur zegt iets anders: `scrum4me-docker/bin/run-one-job.ts` claimt jobs, resolve't config, bouwt CLI flags en spawnt `claude`; MCP levert tools/schema (`docs/runbooks/worker-idempotency.md` regels 170-176 en `docs/runbooks/mcp-integration.md` regel 12).
Als alleen Scrum4Me en `scrum4me-mcp` wijzigen, gaat de docker-runner de nieuwe kind nog steeds claimen en behandelen als Claude-job. Neem een expliciete wijziging op voor `scrum4me-docker`, of definieer een aparte bootstrap-executor. Let ook op: de huidige worker doet een Anthropic quota pre-flight voordat hij claimt (`docs/runbooks/mcp-integration.md` regels 80-93). Daardoor kan een no-LLM bootstrap-job onterecht wachten op quota.
### P1 - De worker-flow sluit de `ClaudeJob` niet terminal af
In de pseudo-flow wordt bij succes alleen `BootstrapRun` en `Product` bijgewerkt, gevolgd door een generieke `NOTIFY` (plan regels 185-186). Bij fouten noemt het plan eveneens vooral `BootstrapRun` (plan regel 187). Het bestaande queue-protocol verwacht dat de job zelf naar `DONE`, `FAILED` of `CANCELLED` gaat en dat een `claude_job_status` event wordt verstuurd (`docs/runbooks/mcp-integration.md` regels 44-49).
Fix: maak `BootstrapRun.status` en `ClaudeJob.status` een transactionele status-sync. Bij succes: `BootstrapRun.SUCCEEDED`, `ClaudeJob.DONE`, `finished_at`, `summary`, `repo_url`/`template_version`. Bij failure/cancel: beide terminal, inclusief `error`, en een `claude_job_status` notify. Anders blijven jobs `CLAIMED` of `RUNNING` en grijpt stale recovery later fout in.
### P1 - De enum-uitbreiding veroorzaakt build-fouten buiten de genoemde files
Het plan noemt `ClaudeJobKind.BOOTSTRAP_REPO`, maar niet alle plekken die exhaustief over `ClaudeJobKind` heen lopen. `JobCard` en `JobsColumn` gebruiken bijvoorbeeld `Record<ClaudeJobKind, string>` (`components/jobs/job-card.tsx` regels 28-34 en `components/jobs/jobs-column.tsx` regels 16-22). Na Prisma generate mist daar een key en faalt typecheck.
Fix: voeg jobs board labels/filters, initial SSE payloads, job detail rendering, cost/insight aggregaties en tests toe aan de scope. Dit is geen nice-to-have; het is build-path.
### P1 - `BootstrapRun` koppeling mist relationele details
Het plan zet `BootstrapRun.claude_job_id` als nullable FK en laat de worker de run ophalen via `run_id` (plan regels 83-90 en 175), maar `ClaudeJob` heeft nu alleen task/idea/sprint koppelingen (`prisma/schema.prisma` regels 385-424). Zonder helder model blijft onduidelijk hoe de geclaimde job precies bij de run komt.
Fix: maak `BootstrapRun.claude_job_id` `@unique`, voeg relation names en een reverse relation op `ClaudeJob` toe, en indexeer `product_id/status`. Leg ook vast dat `startBootstrapAction` atomair voorkomt dat er meerdere actieve `PENDING`/`RUNNING` runs voor hetzelfde product ontstaan. Dit staat nu als open punt (plan regel 323), maar hoort in MVP.
### P1 - PAT-encryptie botst met de huidige worker-secret boundary
Het plan staat encryptie met `SESSION_SECRET` of een optionele `BOOTSTRAP_ENCRYPTION_KEY` toe (plan regels 98-110), en laat de worker de PAT decrypten (plan regel 176). De docker-worker docs zeggen juist dat de worker geen `DATABASE_URL`, `SESSION_SECRET` of `CRON_SECRET` hoort te hebben (`docs/manual/05-docker.md` regels 52-64).
Fix: kies een expliciete credential-boundary. Waarschijnlijk moet `BOOTSTRAP_ENCRYPTION_KEY` verplicht worden voor app plus deterministische executor, of moet GitHub-side werk in de app/MCP-service gebeuren waar decryptie toegestaan is. Specificeer ook minimale PAT scopes, owner/namespace-keuze en voorkom dat de bestaande worker-level `GITHUB_TOKEN` per ongeluk repos onder de verkeerde account aanmaakt.
### P1 - `BootstrapAction.params` is te vrij voor filesystem-acties
Het plan gebruikt `params Json` voor acties en noemt alleen een bash allowlist als securitymaatregel (plan regels 57-75 en 210-214). Maar `COPY_FILE`, `WRITE_FILE`, `APPEND_TO_FILE` en `REPLACE_STRING` kunnen ook schade doen: path traversal via `../`, schrijven naar `.git/config`, absolute paden, te grote bestanden/logs, of onbedoelde workflow-mutaties.
Fix: valideer elke action-kind met een Zod-schema bij seed/admin-save en opnieuw bij uitvoering. Normaliseer paden en assert dat source/dest binnen de template root of output root blijven. Deny `.git/**`, absolute paden en parent traversal. Cap `output_log`, `content` en aantal acties per run.
### P1 - MVP spreekt de verplichte zes ADR-stubs tegen
Het plan noemt zes verplichte ADR-stubs voor deploy/auth/DB/styling/state/testing (plan regel 25), maar de MVP seed bevat alleen deploy/auth/database (plan regels 253-260). De verificatie checkt ook alleen ADR-0001 tot ADR-0003 (plan regels 287-294).
Fix: genereer de zes core ADR-stubs onvoorwaardelijk in MVP, of neem alle zes categorieen op in Sprint 1. Anders is de MVP niet consistent met de eigen acceptatie.
### P2 - Fysieke UI-paden kloppen niet met de App Router route groups
Het plan noemt fysieke files onder `app/products`, `app/settings` en `app/admin` (plan regels 159-164 en 240-244). In deze codebase zitten desktop routes onder `app/(app)/...` (`docs/architecture/project-structure.md` regels 18-42), bijvoorbeeld `app/(app)/products/[id]/page.tsx`.
Fix: corrigeer de filelijst naar `app/(app)/products/[id]/...`, `app/(app)/settings/...` en `app/(app)/admin/...`. De URL blijft hetzelfde; de fysieke implementatieplek niet.
### P2 - Verificatie noemt een niet-bestaand worker-script
De verificatie zegt "manual: `npm run worker`" (plan regel 291), maar `package.json` heeft geen `worker` script (`package.json` regels 5-26). Dat maakt de E2E-stap niet reproduceerbaar.
Fix: verwijs naar het echte `scrum4me-docker` runnercommando of voeg bewust een dev-script toe als onderdeel van de feature.
### P2 - Repo-slug en GitHub owner zijn nog onvoldoende gespecificeerd
De flow gebruikt `<productSlug>` en `<owner>` (plan regels 180-184), maar `Product` heeft nu `name`, optionele `code` en `repo_url`; geen slugveld (`prisma/schema.prisma` regels 196-227). `code` is bovendien niet hetzelfde als GitHub repo-validatie.
Fix: voeg `repo_slug` toe aan de wizard of maak een gesnapshotte derivatie met GitHub-regels, collision-check, owner-keuze en duidelijke foutmelding wanneer de repo al bestaat.
## Aanbevolen aanpassing van de volgorde
1. Ontwerp eerst het deterministic-job contract: status-sync, runner-eigenaar, quota-bypass, config-bypass en `BootstrapRun` relation.
2. Voeg daarna schema + seed toe met path/action validatie en zes minimale ADR-stubs.
3. Bouw PAT settings en GitHub token test met expliciete scopes en owner-keuze.
4. Bouw pas daarna de wizard UI en E2E runner.
Met die volgorde blijft de UI dun en voorkom je dat het meeste risico pas in de worker-integratie zichtbaar wordt.

View file

@ -0,0 +1,210 @@
---
title: "Review - Bootstrap-wizard plan v2 met webresearch"
status: draft
date: 2026-05-13
source_plan: "/Users/janpetervisser/.claude/plans/als-ik-een-nieuwe-virtual-turtle.md"
previous_review: "docs/recommendations/bootstrap-wizard-plan-review-2026-05-13.md"
---
# Review - Bootstrap-wizard plan v2 met webresearch
## Conclusie
De eerdere aanbevelingen zijn grotendeels verwerkt, maar nog niet "goed" genoeg om dit plan direct naar implementatie te brengen. V2 lost de meeste oude schema-, enum-, status- en path-safety punten op papier op. De grootste resterende fout is dat het plan twee executor-modellen tegelijk beschrijft: eerst `scrum4me-docker` als deterministic runner, later de Next.js app als executor met een fire-and-forget background promise. Kies er een.
Mijn advies: maak de app niet de lange-running executor. Gebruik voor MVP een aparte `bootstrap-service` of breid de bestaande docker-runner expliciet uit met een veilig secret-contract. Vercel/Next fire-and-forget is te broos voor clone, file mutation, GitHub repo-create en push.
## Zijn de eerdere aanbevelingen verwerkt?
| Reviewpunt | Status | Oordeel |
|---|---:|---|
| Deterministic runtime ipv `model: null` | Ja | Goed concept, maar nog te veel gekoppeld aan `JobConfig` als de app uiteindelijk executor wordt. |
| Worker-eigenaar expliciet maken | Deels | V2 spreekt zichzelf tegen: docker-runner dispatch versus app-orchestrator. |
| Transactionele `BootstrapRun` + `ClaudeJob` status-sync | Ja | Goed. Hou notify na commit, niet in de DB-transaction zelf. |
| `ClaudeJobKind` exhaustive consumers | Ja | Goed opgenomen. |
| `BootstrapRun.claude_job_id @unique` + reverse relation | Ja | Goed. |
| Concurrency guard | Ja | Goed, vooral met DB-level partial unique index. |
| PAT secret-boundary | Deels | Docker krijgt geen DB/secrets meer, maar PAT wordt nu in memory doorgegeven aan een background promise. Dat is niet duurzaam. |
| Action-param validatie/path-safety | Ja | Goed, maar `condition: String?` blijft een risico. |
| Zes ADR-stubs in MVP | Ja | Goed. |
| App Router paden | Ja | Goed. |
| Niet-bestaand `npm run worker` | Ja | Gecorrigeerd. |
| `Product.repo_slug` | Ja | Goed begin, maar uniekheid moet eigenlijk per GitHub owner + slug, niet per Scrum4Me user. |
## Nieuwe bevindingen
### P1 - V2 heeft nog twee executor-architecturen tegelijk
Regels 49-66 beschrijven dispatch in `scrum4me-docker/bin/run-one-job.ts`, inclusief deterministic dispatch en ephemeral PAT op job-claim. Regels 200-212 kiezen daarna voor "App is executor" en laten docker `BOOTSTRAP_REPO` juist niet claimen. Regels 285-322 werken vervolgens app-side fire-and-forget uit.
Dat is geen detail; dit bepaalt wie claimt, wie secrets heeft, wie retries doet en wie eigenaar is van leases/timeouts. Maak de keuze expliciet:
- Optie A: `bootstrap-service` claimt alleen `BOOTSTRAP_REPO`, heeft `DATABASE_URL` + `BOOTSTRAP_ENCRYPTION_KEY`, decrypt zelf de PAT per run, en gebruikt dezelfde status-sync.
- Optie B: bestaande docker-runner claimt ook deterministic jobs, maar dan moet de secret-boundary worden aangepast en gedocumenteerd.
- Optie C: Next.js app voert inline uit, maar dan geen queue/claim-semantiek en geen 60 minuten timeout claimen.
Voor Scrum4Me past Optie A het best: klein apart Node-proces, geen Claude quota, wel durable retries.
### P1 - Fire-and-forget in de app is niet betrouwbaar genoeg
Het plan kiest `runBootstrapInBackground(runId, pat)` na de server-action response. Vercel documenteert dat niet-geawait async werk in Functions kan blijven hangen in een bevroren execution context; helpers als `waitUntil()` zijn bovendien nog steeds gebonden aan de maximale function timeout. Vercel Functions hebben harde duration-limieten; het plan noemt zelf een 60-minuten watchdog, wat niet past bij normale serverless limits.
Fix: vervang `fire-and-forget` door een echte worker:
- `startBootstrapAction` maakt alleen `BootstrapRun` + `ClaudeJob`.
- `bootstrap-service` claimt atomair `BOOTSTRAP_REPO` runs.
- Service decrypt de PAT op basis van `run.user_id`, voert de recipe uit, en sync't terminal status.
- UI blijft exact hetzelfde via SSE.
### P1 - PAT doorgeven aan een background promise is de verkeerde secret-shape
Regel 197 zegt dat `startBootstrapAction` decrypt voor enqueue, en regel 298 geeft `pat` door aan de background runner. Als het proces wegvalt, is de job niet hervatbaar zonder opnieuw vanuit user context te starten. Als logs of closures uitlekken, zit de PAT in app-memory buiten een duidelijk lifecycle-contract.
Fix: geef alleen `runId` door. De executor haalt `User.github_pat_encrypted` zelf op, decrypt binnen de execution boundary, zeroized daarna best-effort, en logt nooit token-materiaal. Voeg `github_pat_verified_at`, `github_pat_scopes` en `github_pat_expires_at` toe of overweeg later GitHub App/OAuth.
### P1 - Gebruik geen lange-running local git push in een serverless function
De v2-flow gebruikt `mkdtemp`, template clone, lokale git commit, repo create en push. Dat is prima voor een worker/service, maar kwetsbaar in serverless: tijdslimieten, file descriptor limieten, cleanup bij timeout, en onduidelijke rollback wanneer push half lukt.
Fix: zet dit in `bootstrap-service` of Vercel Sandbox/Workflow. Als je toch app-side wilt blijven, maak de eerste versie veel kleiner: GitHub template endpoint aanroepen, geen lokale mutaties, geen push, geen `RUN_BASH_TEMPLATE`.
### P2 - Voeg een dry-run/preview toe voor de wizard en admin-catalog
Backstage Scaffolder heeft dry-run support en een Template Editor waarmee templates in een echte omgeving getest kunnen worden zonder externe mutaties. Scrum4Me mist dit nog.
Aanbevolen toevoeging:
- `previewBootstrapAction(productId, selections)` bouwt `recipe_snapshot`, valideert acties, draait alle non-mutating file handlers in tmpdir, en retourneert file tree + action log + warnings.
- UI toont "Review" voor "Create repo".
- Admin-UI mag een recipe pas activeren nadat dry-run groen is.
- Tests draaien per action ook in dry-run mode.
Dit verlaagt het risico van DB-gedreven recipes sterk.
### P2 - Maak repository owner/slug een echte picker, geen impliciete username
Backstage gebruikt een repository picker met allowed hosts, owners en repos. Het plan heeft `repo_slug`, maar owner blijft impliciet `user.github_username` en staat zelfs nog als open punt.
Fix voor MVP:
- `Product.repo_owner` of `BootstrapRun.repo_owner_snapshot`.
- `repo_slug` uniqueness op `(repo_owner, repo_slug)`, niet op `(user_id, repo_slug)`.
- `saveGitHubPatAction` haalt beschikbare orgs op en bewaart geen owner zonder permissiecheck.
- Wizard laat owner + slug zien en doet preflight `GET /repos/{owner}/{repo}` of equivalente Octokit call.
### P2 - Gebruik GitHub template API bewust, of leg uit waarom niet
GitHub heeft een officieel endpoint om een repository uit een template te maken. Dat is eenvoudiger en veiliger dan zelf init/remote/push doen, maar het endpoint werkt met de template repo en repo-name/owner, niet met een willekeurige tag/ref zoals `template_version`.
Aanbevolen beslissing:
- Als `template_version` hard nodig is: blijf bij "download/clone tagged template, mutate, push", maar documenteer dat GitHub's template endpoint bewust niet gebruikt wordt.
- Als default-branch voldoende is: gebruik GitHub's template endpoint voor MVP en beperk v1 tot variabelen die later via follow-up commits kunnen.
Voor dit plan zou ik tag-pinning behouden, maar de trade-off expliciet maken.
### P2 - Voeg action-permissions toe, niet alleen admin CRUD
Backstage kan parameters, steps en actions autoriseren. Scrum4Me v2 heeft alleen "admin-UI fase 2" en path-safety. Dat beschermt niet tegen een legitieme recipe die te veel doet.
Voeg toe aan `BootstrapAction` of `BootstrapOption`:
- `risk_level: LOW | MEDIUM | HIGH`
- `requires_role: ADMIN | PRODUCT_OWNER`
- `enabled: boolean`
- `supports_dry_run: boolean`
- `side_effects: FILESYSTEM | GITHUB_REPO | GITHUB_SETTINGS | NETWORK`
`RUN_BASH_TEMPLATE` en GitHub-mutaties mogen standaard alleen admin-authored en dry-run getest zijn.
### P2 - Vervang `condition: String?` door een getypte mini-DSL of haal hem uit MVP
Een vrije condition string in DB is op termijn een tweede interpreter. Gebruik liever:
```ts
condition: {
allOf?: Array<{ category: string; option: string }>
anyOf?: Array<{ category: string; option: string }>
not?: Array<{ category: string; option: string }>
}
```
Valideer met Zod en snapshot de resolved action list. Voor MVP: geen conditions, alleen expliciete selected options.
### P2 - Maak template/catalog versioning scherper
Het plan heeft `template_version` en `recipe_snapshot`, maar mist nog:
- `template_source_sha` of release asset checksum.
- `catalog_version` of `recipe_hash`.
- `action_schema_version`.
- `generated_from` metadata in de nieuwe repo, bijvoorbeeld `.scrum4me/bootstrap.json`.
Dat maakt update-detection en latere "rerun/update repo" veel simpeler.
## Webresearch: vergelijkbare ideeen
### GitHub template repositories
GitHub ondersteunt "create repository using a template" via REST. Belangrijk: token scopes verschillen voor public/private repos; het endpoint accepteert `owner`, `name`, `include_all_branches` en `private`. Dit bevestigt dat owner/slug en token-scope preflight first-class moeten zijn.
Bron: <https://docs.github.com/en/rest/repos/repos#create-a-repository-using-a-template>
### Backstage Software Templates / Scaffolder
Backstage is het dichtstbijzijnde patroon: skeleton code laden, variabelen templaten, en publishen naar GitHub/GitLab. Het heeft ook built-in actions voor fetch/publish, een template editor, dry-run, secrets, repository picker en permission controls.
Relevante lessen:
- Scrum4Me's `BootstrapActionKind` lijkt sterk op Backstage scaffolder actions.
- Dry-run en template editor horen vroeg in het plan, niet pas na MVP.
- Secrets moeten apart van gewone parameters blijven.
- Repository owner/host/repo hoort een picker met policy te zijn.
- Action-level permissions zijn belangrijk als recipes in DB/admin UI leven.
Bronnen:
- <https://backstage.io/docs/features/software-templates/>
- <https://backstage.io/docs/features/software-templates/builtin-actions/>
- <https://backstage.io/docs/features/software-templates/writing-templates/>
- <https://backstage.io/docs/next/features/software-templates/dry-run-testing/>
- <https://backstage.io/docs/next/features/software-templates/authorizing-scaffolder-template-details/>
### Cookiecutter, Plop, Hygen
Cookiecutter bevestigt het template-repo model met prompts/context/replay. Plop en Hygen bevestigen het action/generator model, maar zijn vooral lokaal/dev-tooling, niet server-side repo provisioning.
Lessen voor Scrum4Me:
- Houd de action-set klein en composable.
- Zorg voor replay: bewaar parameters, template versie en recipe hash.
- Maak custom actions code-owned, niet vrij definieerbaar vanuit DB.
Bronnen:
- <https://cookiecutter.readthedocs.io/en/stable/>
- <https://plopjs.com/documentation/>
- <https://hygen.ecmascript.pizza/docs/create/>
### Vercel Functions
Omdat het plan de app als executor overweegt, zijn Vercel limits relevant. Vercel Functions hebben maximale duur en background helpers zijn nog steeds aan die max duration gebonden. Dat maakt app-side fire-and-forget ongeschikt als robuuste bootstrap-queue.
Bronnen:
- <https://vercel.com/docs/functions/limitations>
- <https://vercel.com/kb/guide/troubleshooting-inconsistent-logs-in-vercel-functions>
## Aangepaste aanbeveling voor het plan
Vervang de executor-sectie door deze keuze:
1. `BOOTSTRAP_REPO` blijft een `ClaudeJobKind` alleen voor uniforme UI/SSE/status.
2. `scrum4me-docker` claimt `BOOTSTRAP_REPO` niet.
3. Nieuwe `bootstrap-service` claimt alleen `BOOTSTRAP_REPO` of `BootstrapRun(PENDING)`.
4. Service heeft `DATABASE_URL`, `DIRECT_URL`, `BOOTSTRAP_ENCRYPTION_KEY`, geen Anthropic key nodig.
5. Service decrypt PAT per run, voert recipe uit, en gebruikt dezelfde transactionele status-sync.
6. Voeg `previewBootstrapAction` dry-run toe voor wizard en admin.
7. Voeg owner picker, action permissions, catalog versioning en `.scrum4me/bootstrap.json` toe.
Met die aanpassing wordt het plan duidelijker, veiliger en veel dichter bij bewezen scaffolder-patronen.

View file

@ -0,0 +1,109 @@
---
title: "Review - Bootstrap-wizard plan v3.2"
status: draft
date: 2026-05-14
source_plan: "/Users/janpetervisser/.claude/plans/als-ik-een-nieuwe-virtual-turtle.md"
---
# Review - Bootstrap-wizard plan v3.2
## Conclusie
V3.2 is een stevige verbetering. De grote architectuurfout uit v2 is opgelost: er is nu één executor-model met een aparte `bootstrap-service`, geen app-side fire-and-forget. Ook snake_case tables, het bestaande SSE payload-contract, `lease_until`, owner/slug en tag-pinning zijn goed verwerkt.
Nog niet direct implementeren zonder de punten hieronder te verwerken. De belangrijkste resterende blokkades zitten in claim-identiteit, deploybaarheid van het gedeelde package, en recovery wanneer GitHub-repo-aanmaak/push half slaagt.
## Bevindingen
### P1 - Claim-query gebruikt een niet-bestaand `claimed_by` veld
Het claim-protocol zet `claimed_by = ${WORKER_ID}` op `claude_jobs`. Het huidige `ClaudeJob`-model heeft `claimed_by_token_id`, `claimed_at` en `lease_until`, maar geen `claimed_by`. Dit faalt in SQL/migratie tenzij je een nieuw veld toevoegt.
Fix: kies expliciet:
- Re-use `claimed_by_token_id` met een dedicated service `ApiToken`, of
- voeg `claimed_by_worker_id String?` / `claimed_by_service String?` toe, of
- laat claim-identiteit weg en vertrouw op `lease_until`.
Mijn voorkeur: voeg `claimed_by_worker_id String?` toe voor `bootstrap-service`, zodat je logs en recovery kunt correleren zonder `ApiToken`-semantiek te misbruiken.
### P1 - `file:../bootstrap-service/...` dependency maakt de app niet deploybaar
V3.2 kiest voor een shared package onder `~/Development/bootstrap-service/packages/bootstrap-actions/` en een lokale `file:` link vanuit de Scrum4Me-app. Dat werkt lokaal, maar niet in een normale Vercel/GitHub build van de Scrum4Me repo: de sibling-directory zit niet in de repository checkout.
Fix voor MVP:
- Zet `packages/bootstrap-actions/` in de Scrum4Me repo, want dit package bevat geen secrets.
- Laat `bootstrap-service` dit package consumeren via git/package release, of tijdelijk via copied source met een sync-script.
- Of publiceer meteen naar GitHub Packages en pin een versie.
Niet doen: de app afhankelijk maken van een sibling path buiten de repo.
### P1 - Crash-recovery na externe GitHub-mutaties is nog onvoldoende
De happy path en catch-path verwijderen een aangemaakte repo bij errors, maar er is geen duurzaam checkpoint als de service crasht nadat de repo is aangemaakt en voordat `SUCCEEDED` is opgeslagen. Stale recovery markeert dan alleen DB-statussen `FAILED`; de GitHub repo kan blijven bestaan als orphan.
Fix: voeg expliciete externe side-effect checkpoints toe op `BootstrapRun`:
- `github_repo_created_at`
- `github_repo_id`
- `github_repo_full_name`
- `push_completed_at`
Stale recovery kan dan beslissen: compensating delete proberen, of `FAILED_NEEDS_CLEANUP`/manual intervention markeren. Zonder dit is rollback niet betrouwbaar.
### P1 - Stale recovery moet strikt op `BOOTSTRAP_REPO` filteren
De stale-recovery beschrijving update `claude_jobs` waar status `CLAIMED/RUNNING` en `lease_until < NOW`. Dat mag niet generiek op alle job kinds draaien, want de bestaande Claude/sprint runner gebruikt dezelfde tabel.
Fix: filter altijd `kind = 'BOOTSTRAP_REPO'`, en update alleen de bijbehorende `bootstrap_runs`. Laat bestaande cleanup voor andere job kinds ongemoeid.
### P1 - Transaction-array kan geen generated `jobId` doorgeven aan `BootstrapRun`
De atomische enqueue pseudo-code gebruikt `prisma.$transaction([claudeJob.create(...), bootstrapRun.create({ claude_job_id }))])`. Als `jobId` door Prisma wordt gegenereerd, is die waarde in array-form niet beschikbaar voor de tweede create.
Fix: gebruik een transaction callback en pregenereer IDs, of maak eerst de job in de transaction en gebruik de returned ID voor de run. Bijvoorbeeld `const jobId = createId()` vooraf en beide records met expliciete IDs schrijven.
### P2 - Cancel kan alsnog door succes worden overschreven
`cancelBootstrapAction` zet `ClaudeJob.status='CANCELLED'`; de service "detecteert per-action". Dat is goed, maar `syncSuccess` moet ook conditioneel zijn. Anders kan een cancel tussen de laatste checkpoint en success-sync alsnog eindigen als `DONE/SUCCEEDED`.
Fix: voor terminal transitions eerst current job/run status lezen of conditional `updateMany` gebruiken. Als `CANCELLED`, geen success meer schrijven.
### P2 - `last_bootstrap_run_id` mist relationele details
Het plan noemt `Product.last_bootstrap_run_id String?`, maar niet de Prisma relation naar `BootstrapRun` met `onDelete: SetNull`. Voeg die expliciet toe, inclusief relation name om ambiguiteit met `Product.bootstrap_runs` te voorkomen.
### P2 - Action permissions staan op option-niveau, maar risico kan action-niveau zijn
`risk_level` en `requires_role` staan nu op `BootstrapOption`, terwijl `RUN_BASH_TEMPLATE` een action-kind is. Als een optie meerdere acties bevat, moet de optie-risk altijd afgeleid worden uit de zwaarste action, of je hebt action-level permissions nodig.
Fix: ofwel permissions verplaatsen naar `BootstrapAction`, of `BootstrapOption.risk_level`/`requires_role` server-side afleiden en niet handmatig laten driften.
### P2 - Houd ID-strategie consistent met de codebase
Nieuwe modellen gebruiken `@default(uuid())`, terwijl bestaande Scrum4Me-tabellen vrijwel overal `@default(cuid())` gebruiken. Technisch kan UUID, maar het wijkt af zonder duidelijke reden.
Fix: gebruik `cuid()` tenzij er een externe reden is voor UUID.
### P2 - Fine-grained GitHub PATs passen niet netjes in alleen `repo` scope
De verificatie verwacht `repo` in `x-oauth-scopes`. Dat is prima voor classic PATs, maar fine-grained PATs werken met repository permissions en tonen niet altijd hetzelfde scope-model.
Fix: maak MVP expliciet "classic PAT met `repo` scope" of ondersteun fine-grained tokens met aparte permission checks. Zet dit ook in de settings UI-copy.
### P2 - `.env.example` en deployment docs ontbreken in de filelijst
`BOOTSTRAP_ENCRYPTION_KEY` wordt verplicht in de app en service. Voeg `.env.example`, deployment runbook en bootstrap-service README setup toe aan de scope, anders breken lokale onboarding en CI/deploy snel.
## Aanbevolen aanpassing
Verwerk vóór implementatie minimaal:
1. Vervang `claimed_by` door een bestaand of nieuw veld.
2. Verplaats het shared package naar de Scrum4Me repo of publiceer het.
3. Voeg GitHub side-effect checkpoints toe.
4. Filter stale recovery hard op `kind='BOOTSTRAP_REPO'`.
5. Maak enqueue transaction-ID handling concreet.
Daarna is het plan implementatieklaar genoeg om naar `docs/plans/M8-bootstrap-wizard.md` te verplaatsen.

View file

@ -0,0 +1,73 @@
---
title: "Review - Bootstrap-wizard plan v3.3"
status: draft
date: 2026-05-14
source_plan: "/Users/janpetervisser/.claude/plans/als-ik-een-nieuwe-virtual-turtle.md"
---
# Review - Bootstrap-wizard plan v3.3
## Conclusie
V3.3 verwerkt de v3.2-review goed. De claim-identiteit, shared package locatie, GitHub side-effect checkpoints, stale-recovery filter, action-level permissions, classic PAT-keuze en env/docs zijn nu expliciet. Dit plan is dicht bij implementatieklaar.
Nog verwerken vóór uitvoering: de status-sync voorbeeldcode is nog niet echt transactioneel, stale-recovery zet runs te breed op `FAILED_NEEDS_CLEANUP`, en er staat nog een niet-bestaande ID-generator in het enqueue-voorbeeld.
## Bevindingen
### P1 - Status-sync is nog niet transactioneel genoeg
De sectie heet "transactional + post-commit NOTIFY", maar `syncSuccess` doet eerst `bootstrapRun.updateMany(...)` buiten een transaction en daarna pas een transaction met `claudeJob.updateMany(...)` en `product.update(...)`. Als de tweede transaction faalt, staat de run al op `SUCCEEDED`. Als de job-update `count=0` oplevert, wordt het product alsnog bijgewerkt en wordt alsnog `DONE` genotify'd.
Fix: doe run-update, job-update en product-update in één `prisma.$transaction(async tx => ...)`, check beide `updateMany.count` waarden, en notify pas na een volledig geslaagde commit. Zet ook `lease_until` en `claimed_by_worker_id` terminal op `null`.
### P1 - Stale recovery zet alle verlopen runs op `FAILED_NEEDS_CLEANUP`
De SQL zet alle bijbehorende `bootstrap_runs` op `FAILED_NEEDS_CLEANUP`, terwijl de tekst zegt dat dit alleen moet wanneer `github_repo_full_name IS NOT NULL`. Voor runs zonder externe side effects hoort status `FAILED` te zijn.
Fix: split recovery in twee updates:
- `FAILED_NEEDS_CLEANUP` alleen waar `github_repo_full_name IS NOT NULL` of `github_repo_created_at IS NOT NULL`.
- `FAILED` waar beide leeg zijn.
Hou de `kind='BOOTSTRAP_REPO'` filter; die is goed.
### P1 - Enqueue gebruikt `@paralleldrive/cuid2`, maar die dependency bestaat niet
Het plan importeert `createId` uit `@paralleldrive/cuid2`, maar deze repo heeft die dependency niet. De bestaande schema's gebruiken Prisma `cuid()` defaults; applicatiecode genereert die IDs nu niet zelf.
Fix: gebruik de transaction callback-vorm en laat Prisma de IDs genereren, of voeg expliciet een dependency toe en leg vast dat alle nieuwe ID-validatie `z.string().cuid()` blijft accepteren. Mijn voorkeur: transaction callback, geen nieuwe ID-library.
### P2 - Nieuwe non-null arrayvelden op `User` hebben defaults nodig
`github_pat_scopes String[]` is niet nullable en heeft geen default. Op een bestaande database met users maakt dat de migration lastig of onmogelijk zonder backfill.
Fix: maak dit `github_pat_scopes String[] @default([])` of gebruik `Json?` als je fine-grained tokenmetadata later flexibeler wilt opslaan.
### P2 - NOTIFY-status casing moet expliciet API-lowercase zijn
De voorbeelden sturen `status: 'DONE'` en `status: 'QUEUED'`. Bestaande helpers mappen jobstatussen naar lowercase API-strings (`done`, `queued`, etc.). Sommige bestaande paden sturen al lowercase via `jobStatusToApi`.
Fix: spreek af dat NOTIFY payloads API-lowercase gebruiken, en DB-writes UPPER_SNAKE houden. Dus `status: 'done'` in payload, `status: 'DONE'` in DB.
### P2 - Stale recovery hoort niet pas fase 2 te zijn
De service gebruikt leases in MVP, maar de verificatie noemt stale recovery "in fase-2". Zonder recovery kan een crash een job langdurig in `CLAIMED`/`RUNNING` laten hangen.
Fix: neem minimale stale recovery op in Sprint 1d: markeer verlopen `BOOTSTRAP_REPO` jobs en runs correct als `FAILED` of `FAILED_NEEDS_CLEANUP`.
### P2 - Org-owner preflight moet endpoint-gedreven zijn
Voor classic PAT MVP is `repo` scope helder, maar repo creation in een org hangt ook af van de daadwerkelijke org-permissions. Scope-check alleen is niet genoeg.
Fix: laat `RepoOwnerPicker` alleen owners tonen waarvoor de concrete Octokit preflight slaagt, en behandel de response als authority. Documenteer dat org-eigenaarschap/permissies via GitHub worden gevalideerd, niet afgeleid uit alleen scopes.
## Aanbevolen minimale patch op het plan
1. Herschrijf `syncSuccess/syncFailed/syncRunning` als één transaction callback met count-checks.
2. Split stale recovery in `FAILED` vs `FAILED_NEEDS_CLEANUP`.
3. Vervang pre-generated `createId()` door een transaction callback of voeg de dependency expliciet toe.
4. Voeg `@default([])` toe aan `github_pat_scopes`.
5. Maak NOTIFY statuswaarden lowercase.
Daarna is v3.3 goed genoeg om naar `docs/plans/M8-bootstrap-wizard.md` te promoveren.

View file

@ -0,0 +1,121 @@
# Review — M8 bootstrap-wizard plan v3.4
Datum: 2026-05-14
Bronplan: `docs/plans/M8-bootstrap-wizard.md`
Scope: plan-review, geen implementatie uitgevoerd. Ik heb ook kort vergeleken met bestaande repo-contracten zoals `prisma/schema.prisma`, `lib/job-status.ts`, `tsconfig.json` en `package.json`.
## Conclusie
De aanbevelingen uit de vorige review zijn grotendeels goed verwerkt. Ik zie geen P1-blocker meer in de laatste versie. De belangrijkste restpunten zitten in GitHub owner-permissies, catalog-hash determinisme en acceptatie-tests.
## Findings
### [P2] Org-owner preflight belooft meer zekerheid dan de beschreven checks kunnen leveren
Referentie: `docs/plans/M8-bootstrap-wizard.md:50`, `docs/plans/M8-bootstrap-wizard.md:540-567`
Het plan zegt dat `RepoOwnerPicker` alleen owners toont waarvoor een concrete repo-create-preflight slaagt. De uitgewerkte check doet echter `GET /orgs/{org}` plus membership-check. Dat bewijst lidmaatschap/zichtbaarheid, niet dat de PAT daadwerkelijk een private repo in die org mag maken.
GitHub documenteert voor org-repo creation dat de authenticated user org-lid moet zijn en dat classic PATs `repo` nodig hebben voor private repositories. Daarnaast kunnen org-instellingen repo creation beperken; de org API exposeert velden zoals `members_can_create_repositories` en `members_allowed_repository_creation_type`. De huidige plan-check gebruikt die velden niet en kan daardoor false positives of false negatives geven.
Aanbevolen wijziging:
- Noem dit expliciet een best-effort owner discovery, niet een harde create-permission proof.
- Valideer collision met `GET /repos/{owner}/{repo}`.
- Laat de echte create-call in de service de finale autorisatie zijn en vertaal `403/422` naar een duidelijke wizard-fout.
- Als je org-policy vooraf wilt meenemen: lees org creation settings waar beschikbaar, maar behandel ontbrekende rechten/SSO/admin-scope als onbekend in plaats van owner automatisch te verbergen.
Bronnen: GitHub REST docs voor [repositories](https://docs.github.com/en/rest/repos/repos) en [organizations](https://docs.github.com/en/rest/orgs/orgs).
### [P2] `syncRunning` mist expliciete timestamp-contracten
Referentie: `docs/plans/M8-bootstrap-wizard.md:230`, `docs/plans/M8-bootstrap-wizard.md:418-420`, `docs/plans/M8-bootstrap-wizard.md:965-968`
Het plan specificeert voor `syncRunning` alleen de status-overgang `PENDING -> RUNNING` en `CLAIMED -> RUNNING`. De modellen hebben `started_at`, en de verificatie sorteert later op `started_at`. Als `syncRunning` die velden niet atomair vult, worden metrics, UI-sortering en acceptatiequeries onbetrouwbaar.
Aanbevolen wijziging:
- Zet in dezelfde transaction `bootstrap_runs.started_at = now` en `claude_jobs.started_at = now`.
- Gebruik dezelfde `now`-waarde voor run en job.
- Voeg een unit/integration-test toe voor `CLAIMED/PENDING -> RUNNING` inclusief `started_at`.
### [P2] `catalog_version` is nog niet deterministisch genoeg gespecificeerd
Referentie: `docs/plans/M8-bootstrap-wizard.md:603-634`
`recipe_hash` is goed uitgewerkt, maar `catalog_version` blijft te vaag: `SELECT md5(string_agg(...)) FROM bootstrap_options ...` is zonder expliciete ordering niet deterministisch en lijkt alleen options te hashen. Catalog changes in categories, actions, params, roles, risk levels, `enabled`, `archived` of `supports_dry_run` kunnen dan gemist worden.
Aanbevolen wijziging:
- Gebruik dezelfde canonical JSON-aanpak als `recipe_hash`.
- Hash categories, options en actions samen.
- Sorteer expliciet op category `display_order/slug`, option `display_order/slug`, action `execution_order/id`.
- Include minstens: selection type, required/default flags, enabled/archived, action kind, action params, dry-run support, side effects, risk level en required role.
- Gebruik `sha256`, niet ad-hoc `md5(string_agg(...))`.
### [P2] De E2E-verificatiequery leest `lease_until` uit de verkeerde tabel
Referentie: `docs/plans/M8-bootstrap-wizard.md:965-968`
De query selecteert `lease_until > NOW()` uit `bootstrap_runs`, maar `lease_until` staat op `claude_jobs`. Deze acceptatiestap faalt zodra iemand het letterlijk uitvoert en kan lease-regressies maskeren.
Aanbevolen wijziging:
```sql
SELECT br.status,
br.repo_url,
br.recipe_hash,
cj.lease_until > NOW() AS lease_active
FROM bootstrap_runs br
JOIN claude_jobs cj ON cj.id = br.claude_job_id
ORDER BY br.started_at DESC NULLS LAST, br.created_at DESC
LIMIT 1;
```
### [P3] Startup stale-recovery uitleg is inconsistent met de worker-id definitie
Referentie: `docs/plans/M8-bootstrap-wizard.md:93`, `docs/plans/M8-bootstrap-wizard.md:149-151`
De worker-id bevat hostname, pid en start timestamp. Een herstartende service heeft dus niet dezelfde `claimed_by_worker_id`. De SQL in het plan is gelukkig globaal en kind-gefilterd, maar de uitleg zegt dat dezelfde service-instance zichzelf herkent via de oude hostname.
Aanbevolen wijziging:
- Beschrijf startup recovery als globale recovery voor verlopen `BOOTSTRAP_REPO` leases.
- Niet filteren op `claimed_by_worker_id` bij stale recovery.
- Bewaar `claimed_by_worker_id` alleen voor renewal/observability.
### [P3] Vendor-copy drift-mitigatie staat alleen als risico, niet als concrete sprint-taak
Referentie: `docs/plans/M8-bootstrap-wizard.md:749-751`, `docs/plans/M8-bootstrap-wizard.md:1023-1028`
Het plan erkent terecht dat vendor-copy drift tussen Scrum4Me en `bootstrap-service` gevaarlijk is. De mitigatie, een schema-hash CI-check, staat alleen bij accepted risks en niet bij fasering of verificatie.
Aanbevolen wijziging:
- Maak de hash-check onderdeel van Sprint 1a of Sprint 1d.
- Laat `bootstrap-service` bij startup loggen welke `ActionSchema` versie/hash geladen is.
- Voeg een verificatiestap toe die faalt als `packages/bootstrap-actions` in de service niet overeenkomt met de Scrum4Me-bron.
### [P3] `ADD_DEPENDENCY.version` regex is te smal voor normale npm specs
Referentie: `docs/plans/M8-bootstrap-wizard.md:770-778`
De regex accepteert alleen cijfers en operators. Geldige npm-versies zoals `latest`, prerelease labels (`^1.2.3-beta.1`), `workspace:*`, `npm:` aliases of git/tarball specs worden afgewezen. Voor MVP kan dit acceptabel zijn als seed-data alleen simpele semver gebruikt, maar het moet expliciet zijn.
Aanbevolen wijziging:
- Documenteer MVP als "alleen exact/range semver".
- Of gebruik een echte parser zoals `npm-package-arg`/`semver` en allowlist de toegestane spec-types.
## Wat goed verwerkt is
- Transactionele status-sync staat nu in één `prisma.$transaction` met post-commit NOTIFY.
- `FAILED_NEEDS_CLEANUP` wordt alleen gebruikt bij bekende GitHub side-effects.
- `claimed_by_worker_id` is terecht apart gehouden van `claimed_by_token_id`.
- De `@paralleldrive/cuid2` afhankelijkheid is verdwenen; Prisma `cuid()` blijft consistent met het bestaande schema.
- Lowercase SSE-status via `jobStatusToApi` matcht het bestaande contract.
- Stale recovery staat nu in Sprint 1d en is dus onderdeel van MVP.
## Go/no-go
Go na verwerking van de P2-punten. De P3-punten kunnen mee in dezelfde planupdate, maar hoeven geen implementatie te blokkeren zolang ze expliciet als MVP-beperking of verificatietaak worden vastgelegd.