feat(PBI-67): IDEA_REVIEW_PLAN — iterative multi-model plan review (#199)
* feat(ideas): upload-plan knop — short-circuit van Make-Plan AI-flow Voegt een 'Upload plan' knop toe in idea-row-actions (verschijnt in zowel list als idea-detail). Klik → file picker → kies .md → server-side parse + opslaan; idea-status springt naar PLAN_READY. Vandaaruit de bestaande 'Maak PBI' knop voor materialize. Server (uploadPlanMdAction): - Toegestaan vanuit DRAFT, GRILLED, PLAN_FAILED, PLAN_READY - DRAFT → skip-grill: status gaat direct naar PLAN_READY - PLAN_READY overschrijft het bestaande plan (consistent met updatePlanMdAction, geen confirmation) - Geblokkeerd in GRILLING/PLANNING (job loopt), PLANNED (al gematerialiseerd) - Parse-failure → 422 + details (NIET opslaan, zodat een onparseerbaar plan nooit in de DB belandt) - Empty / >100k chars → 422 - Schrijft IdeaLog NOTE met from_status + length - Rate-limit + demo-guard + ownership-check via loadOwnedIdea (zelfde patroon als updatePlanMdAction) UI (idea-row-actions.tsx): - Hidden <input type=file accept=".md,.markdown,text/markdown,text/plain"> - FileReader → text → action - Toast bij success + router.refresh() - Blocked-tooltip in andere statussen Tests: 10 nieuwe in __tests__/actions/ideas-crud.test.ts dekkend voor: happy paths (DRAFT/GRILLED/PLAN_READY-overwrite/PLAN_FAILED), blocks (PLANNED/GRILLING), validation (empty/oversized/parse-fail), 404. Full suite groen: 849/849. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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. * docs(plans): M8 bootstrap-wizard upload-variant v1.4 — backtick-paden Upload-variant van het volledige technische plan (docs/plans/M8-bootstrap-wizard.md), bedoeld voor de "Upload plan"-functie. Genereert 1 PBI + 4 Stories + 22 Tasks via materializeIdeaPlanAction. v1.4-aanpassingen tov eerdere generatie-iteratie: - Alle bestandspaden in implementation_plan in backticks (path-extractor matchen) - Expliciete "Bestanden:" blok per task vóór de stappen - Alle tasks op verify_required: ALIGNED_OR_PARTIAL (was deels ALIGNED — te strict voor ADR-stubs en multi-file edits) Fixt forward-only: T-963 cancelled_by_self door DIVERGENT verifier-verdict. Re-upload van dit bestand produceert tasks die door verify_task_against_plan als ALIGNED of PARTIAL geclassificeerd kunnen worden. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PBI-67: Add review-plan support to Idea model and job config - Add plan_review_log and reviewed_at fields to Idea model - Add REVIEWING_PLAN, PLAN_REVIEW_FAILED, PLAN_REVIEWED to IdeaStatus enum - Add IDEA_REVIEW_PLAN to ClaudeJobKind enum - Add IDEA_REVIEW_PLAN config to job-config.ts with model=opus, thinking_budget=6000 - Create migration record for schema changes (applied via db push) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * PBI-67 Phase 2: Add update-idea-plan-reviewed MCP tool - Create src/tools/update-idea-plan-reviewed.ts: saves review-log and transitions idea status to PLAN_REVIEWED - Add PLAN_REVIEW_RESULT to IdeaLogType enum (both repos) - Register tool in src/index.ts - Update Prisma schemas (both repos): add plan_review_log and reviewed_at fields to Idea model - Add REVIEWING_PLAN, PLAN_REVIEW_FAILED, PLAN_REVIEWED to IdeaStatus enum (MCP schema) - Add IDEA_REVIEW_PLAN to ClaudeJobKind enum (MCP schema) - Tool includes transaction safety and convergence metrics logging Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * feat(PBI-67): IDEA_REVIEW_PLAN Phases 3-6 — server actions, UI components, prompt & tests - Phase 3: startReviewPlanJobAction, cancelIdeaJobAction, status transitions (REVIEWING_PLAN / PLAN_REVIEWED / PLAN_REVIEW_FAILED), status colors, job-card/jobs-column filters, idea-list status tabs - Phase 4: review-plan-job.md prompt (multi-model orchestration with codex injection + active plan revision via update_idea_plan_md after each round), runbook, 13 unit tests - Phase 5: ReviewLogViewer component (rounds, convergence, approval, issues), idea-detail integration, proper ReviewLog TypeScript types exported from component - Phase 6.1: wait-for-job discriminator wired (IDEA_REVIEW_PLAN), plan-revision step made mandatory in prompt (was previously optional/missing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Janpeter Visser
GitHub
parent
b8e22539f6
commit
d84cdf664f
28 changed files with 4387 additions and 30 deletions
|
|
@ -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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue