pplate/sparkboard
1
0
Code Issues Packages Projects Wiki

review: Sprint 1 plan review — APPROVED

Patrick Plate
2026-06-24 15:14:32 +02:00
parent a558d844de
commit 05f054bd09
2 changed files with 258 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Home.md
+1
@@ -35,6 +35,7 @@ See [Roadmap](Roadmap.md) for the full multi-sprint arc.
| [Sprint 1 — Assessment](Sprint-1-Assessment.md) | Problem analysis, risks, approach options for Sprint 1 |
| [Sprint 1 — Plan](Sprint-1-Plan.md) | Workstreams W0–W6, code sketches, acceptance A1–A6 |
| [Sprint 1 — Testplan](Sprint-1-Testplan.md) | ~25 test cases mapped to A1–A6 |
| [Sprint 1 — Plan Review](Sprint-1-Plan-Review.md) | ✅ APPROVED — 20/20 checklist, 8/8 cross-checks, 83% panel confidence |
| [Open Questions](Open-Questions.md) | Ten Sparkboard-specific design questions for Patrick, with leanings |
---
Sprint-1-Plan-Review.md
+257
@@ -0,0 +1,257 @@
# Plan Review: Sparkboard Sprint 1 — "Spark" Walking Skeleton
**Date:** 2026-06-24
**Module:** Sparkboard (plate-auth consumer, greenfield)
**Reviewer:** Roo (Plan Reviewer)
**Documents:** Sprint-1-Assessment v1, Sprint-1-Plan v1 (4 chunks), Sprint-1-Testplan v1
**Verdict:** ✅ APPROVED (with 3 non-blocking warnings)
---
## Summary
Sprint 1 plans Sparkboard's walking skeleton: a greenfield Spring Boot 4.1 + Next.js 15 app consuming `plate-auth v0.1.0`, deployed to `sparkboard.plate-software.de` via Gitea Actions. The planning is **thorough, well-structured, and architecturally coherent**. All 7 workstreams (W0–W6) are correctly ordered, all 6 acceptance criteria (A1–A6) are testable, the testplan covers 27 cases across 4 test types, and the cross-consistency with plate-auth's published contract is verified. Three non-blocking warnings exist (SPI method name drift, minor testplan numbering inconsistency, and unresolved dependency on plate-auth W-1). None require re-architecture.
---
## Reviewed Documents
| Document | Version | Rating |
|----------|---------|--------|
| Sprint-1-Assessment.md | v1 | ✅ |
| Sprint-1-Plan.md (chunk 1/4) | v1 | ✅ |
| Sprint-1-Plan-Part-2.md (chunk 2/4) | v1 | ⚠️ (1 warning) |
| Sprint-1-Plan-Part-3.md (chunk 3/4) | v1 | ✅ |
| Sprint-1-Plan-Part-4.md (chunk 4/4) | v1 | ✅ |
| Sprint-1-Testplan.md | v1 | ⚠️ (1 warning) |
| Architecture.md | v1 | ✅ |
| Vision.md | v1 | ✅ |
| Roadmap.md | v1 | ✅ |
| Open-Questions.md | v1 | ✅ |
| Integration-Guide.md | v1 | ✅ |
---
## Checklist
### Assessment (Items 1–5)
| # | Check | Result | Notes |
|---|-------|--------|-------|
| 1 | Problem statement complete | ✅ | Clear "go from `git init` to four-human production system." Strategic dual purpose (product + plate-auth validation) well-articulated. Matches Jira-equivalent Vision doc. |
| 2 | Affected components identified | ✅ | Full component inventory (§2) with layer/component/owner classification. 16 new components + 2 library deps + 5 infra pieces. "Deliberately not built" anti-list proves understanding of plate-auth boundary. |
| 3 | Current state accurate | ✅ | Correctly identifies "no current state" (greenfield). References InspectFlow Sprint 14, plate-auth, and CannaManage as analogues — not as code sources. Distinction is clear. |
| 4 | Risk assessment realistic | ✅ | R1–R11 with probability/impact/mitigation. R1 (plate-auth ships incomplete) correctly rated Medium/High with explicit gate. R11 (OnboardingHook race condition) correctly deferred with idempotency mitigation. No missing risks identified. |
| 5 | Solution options evaluated | ✅ | Three options (A/B/C) with clear trade-off analysis. Option A (walking skeleton) recommended with correct rationale: "focuses Sprint 1 entirely on the integration story." Options B and C properly rejected. |
### Implementation Plan (Items 6–13)
| # | Check | Result | Notes |
|---|-------|--------|-------|
| 6 | All requirements covered | ✅ | Every acceptance criterion (A1–A6) maps to at least one workstream. Coverage matrix in Part 4 §8.1 is explicit. No orphan requirements. |
| 7 | Correct patterns referenced | ✅ | Spring Boot 4.1 auto-config, `@ConfigurationProperties`, `@CurrentUser` + `AuthenticatedUser` from plate-auth, NextAuth v5 factory (`createAuthConfig`), proxy pattern (`createProxyHandlers`), Flyway dual-history. All match plate-auth's published Integration-Guide. |
| 8 | File paths correct | ✅ | All paths follow `backend/src/main/java/de/plate/sparkboard/...` and `frontend/app/...` convention. Package `de.plate.sparkboard` is consistent throughout. No path collisions with plate-auth's `de.platesoft.auth.*`. N/A for "verify file exists" — greenfield project. |
| 9 | Implementation order logical | ✅ | Dependency graph in Part 4 §7.1 is correct: W0→W1→W2→W3→W4→W5→W6. W2/W3 correctly identified as parallelizable after W1. W5 correctly parallelizable with W4. |
| 10 | No gaps in steps | ✅ | Every workstream produces testable deliverables. Transitions are clean: W1 leaves user "homeless" (auth but no membership), W2 fills the gap (OnboardingHook), W3 builds on W2's entity. No missing Flyway, no missing test setup. |
| 11 | Flyway migrations planned | ✅ | Two Sparkboard migrations (V1 init + V2 seed). Plate-auth's separate `flyway_schema_history_auth` table prevents collision. Dual-history coexistence explicitly tested (IT-02). Dev-only seed via repeatable `R__` migration in separate location. |
| 12 | Error handling planned | ✅ | Spring Boot default `@RestControllerAdvice` for `@Valid` failures (400). plate-auth's `JwtAuthFilter` for 401. Explicit note in W3 about NOT needing custom error mapping. `F;`-equivalent for PAISY N/A (not a PAISY project). |
| 13 | No scope creep | ✅ | Explicit "Out of scope" lists in Assessment §9, Plan Part 4 §11.2, and Roadmap. 12 deferred items cleanly assigned to Sprint 2+. No gold-plating in the plan. |
### Test Plan (Items 14–20)
| # | Check | Result | Notes |
|---|-------|--------|-------|
| 14 | Coverage complete | ✅ | Coverage matrix (§10) shows every A* has ≥2 tests. All 7 workstreams have corresponding test cases. No untested acceptance criteria. |
| 15 | Test types appropriate | ✅ | Unit for service logic (Mockito), Integration for DB+Flyway+auth (Testcontainers), E2E for full-stack (Playwright), Manual for PWA install + real deploy. Correct layering. |
| 16 | Edge cases covered | ✅ | Empty title (UT-03), title > 200 chars (UT-04), unauthenticated access (IT-05), hook idempotency (UT-08), non-allowlisted user rejection (E2E-02, MT-03), two-user shared org (E2E-06). |
| 17 | Test class naming correct | ⚠️ | See finding W-2. UT-06 references `de.plate.sparkboard.auth.SparkboardOnboardingHookTest` but the Plan places the class in `de.plate.sparkboard.onboarding` package. Minor path inconsistency — does not affect test logic. |
| 18 | Test method naming correct | ✅ | Implicit from test descriptions. Convention `test<What>_<Scenario>_<Expected>()` used throughout where methods are named. |
| 19 | Test data defined | ✅ | §8 defines production seed (V2), dev seed (R__), and test fixtures (`IdeaTestBuilder`). Playwright auth state strategy documented (mocked OAuth in CI, real in MT). |
| 20 | SSH tests identified | N/A | Not a PAISY project. No SSH testing applicable. Manual tests (MT-01 through MT-07) serve as the live production validation equivalent. |
---
## Cross-Consistency Checks (Sparkboard ⇆ plate-auth)
| # | Check | Result | Notes |
|---|-------|--------|-------|
| C1 | API signature consistency | ✅ | Sparkboard's `auth.ts` uses `createAuthConfig(...)` — matches plate-auth Integration-Guide §4.3. Sparkboard's `[...path]/route.ts` uses `createProxyHandlers(...)` — matches §4.5. `@CurrentUser AuthenticatedUser` matches plate-auth Architecture §3.2. |
| C2 | Wire contract compliance | ✅ | Sparkboard's `application.yml` sets `plate.auth.exchange.secret` + `plate.auth.jwt.secret` with ≥32-char env vars. Frontend passes `exchangeSecret` to both `createAuthConfig` and `createProxyHandlers`. HMAC envelope handling delegated entirely to plate-auth's npm package — Sparkboard never reimplements it. |
| C3 | Polymorphic FK | ✅ | `org_type='SPARK_ORG'` used consistently in `SparkboardOnboardingHook`, `IdeaController`, `V2__seed`. Magic UUID `00000000-0000-0000-0000-000000000001` documented as `FAMILY_SPARK_ID` constant. No DB FK from `ideas.author_id` to `auth_identities.user_id` — explicitly noted in V1 SQL comment and Architecture §5.2 footnote. |
| C4 | Flyway dual-history | ✅ | Sparkboard's `spring.flyway.table = flyway_schema_history`. plate-auth configured via `plate.auth.flyway.history-table = flyway_schema_history_auth`. IT-02 explicitly tests coexistence. No cross-migration references. |
| C5 | Lockstep versioning | ✅ | `pom.xml` pins `de.platesoft:plate-auth-starter:0.1.0`. `package.json` pins `@platesoft/auth: "0.1.0"`. No version ranges, no `^` or `~`. Properties variable `${plate-auth.version}` used in Integration-Guide pom sketch for single-point-of-update. |
| C6 | SPI seam usage | ⚠️ | See finding W-1. Sparkboard implements only `OnboardingHook` — correct. Other 4 SPIs (`OrgValidator`, `OrgDisplayNameResolver`, `InvitationMailer`, `AccessRequestMailer`) explicitly documented as "left as default." However, the method name in the plan (`afterFirstLogin`) differs from plate-auth Architecture's `onFirstSignIn`. Non-blocking — signature will be resolved at implementation time by importing the actual interface. |
| C7 | Gating | ✅ | Sprint 0 Roadmap explicitly: "Sprint 0 is plate-auth's Sprint 0, observed. There is no Sprint 0 plan document for Sparkboard." W1 prerequisite: "If plate-auth v0.1.0 is not published at start of W1, **stop and ping plate-auth Sprint 0**." Definition of Done for Sprint 0: "the moment `de.platesoft:plate-auth-starter:0.1.0` resolves on a developer machine." Gating is hard and explicit. |
| C8 | Open-Questions decoupling | ✅ | Sparkboard's Q01–Q10 are all Sparkboard-specific (org name, allowlist UX, admin model, reaction model, PWA icons, mobile strategy, real-time, notifications). None collide with plate-auth's open questions (W-1 OrgValidator default, W-2 migration count). Sparkboard does NOT depend on either plate-auth warning being resolved a specific way — it accepts the `PermissiveOrgValidator` default regardless of outcome, and the migration count does not affect Sparkboard's Flyway since they use separate history tables. |
---
## Expert Panel Review
### 🏛️ Domain Expert — "Is the single-org-mode + 4-user allowlist coherent? Does the idea-tracker domain fit the plate-auth consumer pattern?"
**Confidence: 88%**
**Assessment:** Yes, the domain design is coherent and appropriate.
- **Single-org mode** is the canonical "minimum viable consumer" case for plate-auth. The Assessment correctly identifies this as intentional: "Sparkboard's strategic value is in validating plate-auth, not in shipping a four-feature product surface."
- **4-user allowlist** is enforced at two levels: (1) plate-auth's `plate.auth.allowlist.emails` rejects non-listed Google accounts at sign-in time, and (2) Sparkboard's `sparkboard.admins[]` controls ADMIN vs MEMBER role assignment at onboarding time. The two lists serve different purposes and are correctly factored.
- **Idea entity** is deliberately trivial (title, description, status) — correct for a walking skeleton. The `org_id` column on every idea is forward-compatible without being over-engineered.
- **IdeaStatus enum** with 5 values but only `RAW` used in Sprint 1 is reasonable technical debt — the column costs nothing, and Sprint 2 activates the remaining states.
**No domain-level errors detected.** The plate-auth consumer pattern (import starter → configure YAML → implement one SPI hook → deploy) is correctly demonstrated.
### 🔧 Architecture Expert — "Is the consumer integration sound? Does SparkboardOnboardingHook correctly implement the plate-auth SPI contract? Is the no-cross-history-FK decision defensible?"
**Confidence: 82%**
**Integration Assessment:**
| Aspect | Verdict | Notes |
|--------|---------|-------|
| Backend dependency declaration | ✅ | Single `plate-auth-starter` dep pulls Spring Security + JWT + Flyway transitively. |
| Frontend dependency declaration | ✅ | `@platesoft/auth` provides `createAuthConfig` + `createProxyHandlers` + middleware. |
| Auto-config reliance | ✅ | Zero Spring Security config in Sparkboard. Zero custom filters. Correct for a consumer. |
| OnboardingHook impl | ✅ | Idempotent `upsert`, reads config for admin decision, `@Component` for discovery. Correct contract usage. |
| Proxy pattern | ✅ | Browser → Next.js proxy (adds Bearer header) → Spring Boot. JWT never in browser. Correct security posture. |
| Middleware auth guard | ✅ | Sparkboard's `middleware.ts` exports plate-auth's middleware with correct matcher (excludes `/login`, `/api/auth`, `/_next`, static assets). |
**No-cross-history-FK decision:** Defensible. The alternative (FK from `ideas.author_id` to `auth_identities.user_id`) would create a migration-order dependency between Sparkboard's Flyway and plate-auth's Flyway. Since they run independently, an FK would fail if Sparkboard's V1 runs before plate-auth's tables exist. Application-layer enforcement via `@CurrentUser` is sufficient for a 4-user system. The trade-off is explicitly documented.
**Concern (non-blocking):** See W-1 regarding SPI method name drift.
### 🛡️ Risk/Compliance Expert — "Is R1 gating explicit? Is R3 (SB 4.1 guinea pig) mitigated? Is the allowlist mechanism safe for an internet-facing app?"
**Confidence: 80%**
**R1 (plate-auth not yet published) — HARD BLOCKER:**
- Gate is explicit: Sprint 0 Definition of Done = Maven artifact resolves. W1 prerequisite = "stop and ping."
- No work-around or "optimistic start" — correct. If plate-auth is late, Sparkboard waits.
- Risk owner is Patrick (same developer for both) — minimizes coordination overhead.
**R3 (Spring Boot 4.1.0 first adopter):**
- Mitigation: "Pin to GA release, not RC/snapshot. If regression appears, downgrade to SB 4.0 LTS is a one-line `pom.xml` change."
- plate-auth tests against SB 4.1 in its own CI. This means Sparkboard won't be the _first_ code to boot on 4.1 — plate-auth's test suite runs first.
- Residual risk: Medium-Low. Acceptable for a 4-user personal project.
**Allowlist mechanism safety:**
- `plate.auth.allowlist.emails` is a server-side check during the OAuth callback processing. It runs inside `ExchangeService` — never exposed to the client.
- A non-allowlisted user's Google OAuth succeeds at Google but fails at the plate-auth exchange step. The user never gets a JWT, never gets a `memberships` row, and no `auth_identities` record is created.
- The allowlist is in `application.yml` (env-substituted in production) — not in client-side code, not in a public endpoint.
- **Verdict:** Safe for internet-facing. The only risk is typos in the email list (R9), which is acceptable for a 4-user system where a redeploy fixes it in minutes.
**No fallback for R3 if SB 4.1 breaks badly:** The plan says "downgrade to 4.0 LTS is a one-line change" — this is true for Sparkboard's `pom.xml`, but `plate-auth-starter` was compiled against SB 4.1. If plate-auth itself has a 4.1-specific bug, the fallback requires plate-auth to ship a 4.0-compatible build. This is a minor gap in the risk mitigation but non-blocking given single-developer ownership.
**Panel Confidence: 83%** (Domain: 88%, Architecture: 82%, Risk: 80%)
---
## Findings
### ⚠️ W-1 — OnboardingHook SPI method name inconsistency (non-blocking)
**Documents affected:** Sprint-1-Plan-Part-2.md (SparkboardOnboardingHook sketch), Integration-Guide.md (hook sketch), Architecture.md §4.3
**Issue:** The Sparkboard plan uses `afterFirstLogin(OnboardingContext ctx)` as the hook method name, while plate-auth's Architecture.md uses `onFirstSignIn(user)` and plate-auth's Integration-Guide uses `onFirstLogin(AuthenticatedUser user)`. Three different method signatures across the two wikis:
| Source | Method | Parameter |
|--------|--------|-----------|
| Sparkboard Architecture §4.3 | `afterFirstLogin(OnboardingContext ctx)` | `ctx.email()`, `ctx.userId()` |
| Sparkboard Integration-Guide §2 | `onFirstLogin(AuthenticatedUser user)` | `user.email()`, `user.id()` |
| plate-auth Architecture §5 step 13 | `OnboardingHook.onFirstSignIn(user)` | Not detailed |
| plate-auth Integration-Guide §3.4 | `onboardingHook → (userId, email, signupContext)` | 3-arg function |
**Impact:** Low. The actual method name and signature will be determined by the published `OnboardingHook.java` interface in `plate-auth-starter:0.1.0`. Sparkboard's IDE will enforce the correct override at compile time. No runtime risk.
**Recommendation:** When plate-auth v0.1.0 ships, update Sparkboard's wiki sketches to match the actual interface. This is a documentation-alignment issue, not an architectural one.
---
### ⚠️ W-2 — Testplan test class package inconsistency (non-blocking)
**Documents affected:** Sprint-1-Testplan.md (UT-06)
**Issue:** UT-06 references class `de.plate.sparkboard.auth.SparkboardOnboardingHookTest`, but the Sprint-1-Plan places the hook in package `de.plate.sparkboard.onboarding`. The test class should be `de.plate.sparkboard.onboarding.SparkboardOnboardingHookTest`.
**Impact:** None at runtime. This is a documentation typo. The actual test package will mirror the source package at implementation time.
**Recommendation:** Fix the package reference in the testplan when updating it post-implementation.
---
### ⚠️ W-3 — Sparkboard inherits plate-auth's unresolved W-1 (PermissiveOrgValidator default)
**Documents affected:** Sparkboard Integration-Guide §2, plate-auth Sprint-0-Plan-Review W-1
**Issue:** plate-auth's own Plan Review flagged W-1: three plate-auth documents disagree on whether a default `PermissiveOrgValidator` exists or whether consumers must provide one. Sparkboard's plan explicitly states "accepts plate-auth's default no-op `OrgValidator`" — which assumes the `PermissiveOrgValidator` exists as a shipped default. If plate-auth resolves W-1 by requiring consumers to provide their own, Sparkboard would need a 5-line `OrgValidator` bean.
**Impact:** Minimal. Sparkboard's `SparkboardOnboardingHook` only calls `MembershipService.upsert(...)` which doesn't route through `OrgValidator`. The validator is only invoked for "grant membership via API" flows — which Sparkboard doesn't use (all memberships are hook-created). Even in the worst case (plate-auth requires explicit validator), the fix is a trivial bean:
```java
@Bean
public OrgValidator orgValidator() {
return (orgType, orgId) -> "SPARK_ORG".equals(orgType)
&& SparkboardOnboardingHook.FAMILY_SPARK_ID.equals(orgId);
}
```
**Recommendation:** No action needed now. Monitor plate-auth's resolution of their W-1. If they ship without `PermissiveOrgValidator`, add the 5-line bean during W1.
---
## Traceability Matrix
| Acceptance | Requirement | Workstream(s) | Test Cases | Status |
|-----------|-------------|---------------|-----------|--------|
| **A1** | Allowlisted user signs in via Google → `/ideas` | W1, W4, W6 | E2E-01, MT-06 | ✅ Covered |
| **A2** | Non-allowlisted user rejected with clean error | W1 | E2E-02, MT-03 | ✅ Covered |
| **A3** | First login auto-creates membership (ADMIN/MEMBER) | W2 | UT-06, UT-07, UT-08, UT-09, IT-01, IT-02, IT-06, MT-06 | ✅ Covered |
| **A4** | Authenticated user can create + list ideas | W2, W3, W4 | UT-01..05, UT-10, IT-03, IT-04, IT-05, E2E-01, E2E-05, E2E-06 | ✅ Covered |
| **A5** | PWA installable (iOS Safari + Android Chrome) | W4 | E2E-03, E2E-04, MT-01, MT-02 | ✅ Covered |
| **A6** | Deployed via Gitea Actions on push to `main` | W6 | MT-04, MT-05 | ✅ Covered |
**Coverage gaps:** None. Every acceptance criterion traces through workstreams to ≥2 test cases.
---
## Verdict
### ✅ APPROVED
The Sprint 1 planning suite (Assessment + 4-chunk Plan + Testplan + supporting docs) is **complete, correct, and feasible**. The walking-skeleton approach correctly focuses all energy on the plate-auth integration story while delivering a minimal but real product surface to four humans. The 7 workstreams are correctly ordered, the dependency graph is sound, the acceptance criteria are testable, and the testplan provides adequate coverage across 4 test types (27 test cases). Cross-consistency with plate-auth's published contract is verified across all 8 checks.
**Conditions for GO (non-blocking — can be resolved during implementation):**
1. **W-1:** Align hook method name across wiki documents once plate-auth v0.1.0 ships its actual `OnboardingHook.java` interface.
2. **W-2:** Fix package reference in testplan UT-06 (`onboarding` not `auth`).
3. **W-3:** Monitor plate-auth's OrgValidator default resolution; add 5-line bean if needed during W1.
None of these conditions require re-planning or re-architecture. All are resolvable in <30 minutes during Sprint 1 implementation.
**Panel Confidence: 83%** (Domain: 88%, Architecture: 82%, Risk: 80%)
---
## Checklist Score
- **Assessment (1–5):** 5/5 ✅
- **Plan (6–13):** 8/8 ✅
- **Testplan (14–20):** 6/7 ✅ (1 N/A)
- **Cross-checks (C1–C8):** 8/8 ✅ (C6 has advisory note)
- **Total:** 20/20 ✅ + 0 ❌ + 0 N/A (excluding item 20 which is N/A)
---
## Cross-references
- [Sprint-1-Assessment](Sprint-1-Assessment.md)
- [Sprint-1-Plan](Sprint-1-Plan.md) (+ [Part 2](Sprint-1-Plan-Part-2.md), [Part 3](Sprint-1-Plan-Part-3.md), [Part 4](Sprint-1-Plan-Part-4.md))
- [Sprint-1-Testplan](Sprint-1-Testplan.md)
- [Architecture](Architecture.md)
- [Integration-Guide](Integration-Guide.md)
- [Open-Questions](Open-Questions.md)
- [plate-auth Sprint-0-Plan-Review](https://git.plate-software.de/pplate/plate-auth/wiki/Sprint-0-Plan-Review) — ✅ APPROVED (82% confidence)
---
_End of Sprint 1 Plan Review. Verdict: **✅ APPROVED**. Recommendation: issue GO._