From 3a2d3c0e0f88662fd0dd29afddc1c11af507f283 Mon Sep 17 00:00:00 2001 From: AngryJay91 Date: Tue, 31 Mar 2026 11:21:35 +0900 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20standup=20feedback=20N+1=20=E2=80=94?= =?UTF-8?q?=20batch=20IN=20query=20&=20entries-with-feedback=20endpoint=20?= =?UTF-8?q?(issue=20#14)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - routes/v2-standup.ts: add GET /standup/entries-with-feedback Fetches entries + all feedback in exactly 2 queries (1 for entries, 1 IN query for feedback), eliminating the per-entry N+1 pattern. - mcp-tools/standup.ts: toolListStandupEntries with_feedback flag When with_feedback=true, batch-loads all feedback via single IN query instead of N individual SELECT calls per entry. - mcp.ts: expose with_feedback param in list_standup_entries tool schema --- scaffold/pm-api/src/mcp-tools/standup.ts | 31 +++++++++++++++- scaffold/pm-api/src/mcp.ts | 3 +- scaffold/pm-api/src/routes/v2-standup.ts | 45 ++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/scaffold/pm-api/src/mcp-tools/standup.ts b/scaffold/pm-api/src/mcp-tools/standup.ts index 534cdf03..a842cc98 100644 --- a/scaffold/pm-api/src/mcp-tools/standup.ts +++ b/scaffold/pm-api/src/mcp-tools/standup.ts @@ -42,6 +42,7 @@ export async function toolSaveStandup(user: string, args: Record): Promise { const sprint = await resolveSprint(args.sprint as string | undefined) const date = args.date as string | undefined + const withFeedback = Boolean(args.with_feedback) let sql: string const sqlArgs: (string | number)[] = [] @@ -59,16 +60,44 @@ export async function toolListStandupEntries(args: Record): Pro return err('Please specify a sprint or date.') } - const result = await query<{ user_name: string; entry_date: string; done_text: string | null; plan_text: string | null; blockers: string | null }>(sql, sqlArgs) + const result = await query<{ id: number; user_name: string; entry_date: string; done_text: string | null; plan_text: string | null; blockers: string | null }>(sql, sqlArgs) if (result.error) return err(result.error) if (result.rows.length === 0) return text('No standup records found.') + // Batch-fetch feedback in a single IN query when requested β€” eliminates N+1 + type FeedbackRow = { standup_entry_id: number; feedback_by: string; feedback_text: string; review_type: string; created_at: string } + const feedbackMap: Record = {} + if (withFeedback && result.rows.length > 0) { + const ids = result.rows.map(e => e.id) + const ph = ids.map(() => '?').join(',') + const fbResult = await query( + `SELECT standup_entry_id, feedback_by, feedback_text, review_type, created_at FROM pm_standup_feedback WHERE standup_entry_id IN (${ph}) ORDER BY created_at ASC`, + ids, + ) + if (!fbResult.error) { + for (const f of fbResult.rows) { + if (!feedbackMap[f.standup_entry_id]) feedbackMap[f.standup_entry_id] = [] + feedbackMap[f.standup_entry_id].push(f) + } + } + } + const lines = ['πŸ“ Standup Records', '─────────────'] for (const e of result.rows) { lines.push(`\nπŸ‘€ ${e.user_name} (${e.entry_date})`) if (e.done_text) lines.push(` βœ… ${e.done_text}`) if (e.plan_text) lines.push(` πŸ“Œ ${e.plan_text}`) if (e.blockers) lines.push(` 🚧 ${e.blockers}`) + if (withFeedback) { + const fb = feedbackMap[e.id] ?? [] + if (fb.length > 0) { + lines.push(` πŸ’¬ Feedback (${fb.length}):`) + for (const f of fb) { + const icon = f.review_type === 'approve' ? 'βœ…' : f.review_type === 'request_changes' ? 'πŸ”„' : 'πŸ’¬' + lines.push(` ${icon} ${f.feedback_by}: ${f.feedback_text}`) + } + } + } } return text(lines.join('\n')) } diff --git a/scaffold/pm-api/src/mcp.ts b/scaffold/pm-api/src/mcp.ts index a2b45265..78228f1c 100644 --- a/scaffold/pm-api/src/mcp.ts +++ b/scaffold/pm-api/src/mcp.ts @@ -388,12 +388,13 @@ const TOOLS = [ }, { name: 'list_standup_entries', - description: 'List standup entries (filter by sprint/date)', + description: 'List standup entries (filter by sprint/date). Use with_feedback=true to include all feedback in a single batch query (no N+1).', inputSchema: { type: 'object', properties: { sprint: { type: 'string', description: 'Sprint (default: active sprint)' }, date: { type: 'string', description: 'Date (YYYY-MM-DD)' }, + with_feedback: { type: 'boolean', description: 'Include feedback for each entry (batch-loaded, no N+1)' }, }, }, }, diff --git a/scaffold/pm-api/src/routes/v2-standup.ts b/scaffold/pm-api/src/routes/v2-standup.ts index 3f0171a6..435ee2e8 100644 --- a/scaffold/pm-api/src/routes/v2-standup.ts +++ b/scaffold/pm-api/src/routes/v2-standup.ts @@ -61,6 +61,51 @@ app.put('/entries', async (c) => { return c.json({ ok: true }) }) +// GET /entries-with-feedback β€” batch fetch entries + feedback in 2 queries (N+1 fix) +app.get('/entries-with-feedback', async (c) => { + const sprint = c.req.query('sprint') + const date = c.req.query('date') + + if (!sprint && !date) { + return c.json({ error: 'sprint or date query param required' }, 400) + } + + const conditions: string[] = [] + const params: (string | number)[] = [] + if (sprint) { conditions.push('e.sprint = ?'); params.push(sprint) } + if (date) { conditions.push('e.entry_date = ?'); params.push(date) } + const where = conditions.length ? `WHERE ${conditions.join(' AND ')}` : '' + + const { rows: entries } = await queryOrThrow( + `SELECT e.* FROM pm_standup_entries e ${where} ORDER BY e.entry_date DESC, e.user_name`, + params, + ) + + if (entries.length === 0) return c.json({ entries: [] }) + + // Batch-fetch all feedback for these entries in a single IN query β€” eliminates N+1 + const entryIds = (entries as Array<{ id: number }>).map(e => e.id) + const ph = entryIds.map(() => '?').join(',') + const { rows: allFeedback } = await queryOrThrow( + `SELECT * FROM pm_standup_feedback WHERE standup_entry_id IN (${ph}) ORDER BY created_at ASC`, + entryIds, + ) + + // Group feedback by standup_entry_id + const feedbackMap: Record = {} + for (const f of allFeedback as Array<{ standup_entry_id: number }>) { + if (!feedbackMap[f.standup_entry_id]) feedbackMap[f.standup_entry_id] = [] + feedbackMap[f.standup_entry_id].push(f) + } + + const result = (entries as Array<{ id: number }>).map(e => ({ + ...e, + feedback: feedbackMap[e.id] ?? [], + })) + + return c.json({ entries: result }) +}) + // ── Standup Feedback (1:N) ── // GET /feedback?standup_entry_id= or ?sprint=&user= From f2a7b39db946588269fcb1bf1268ce5d4d015281 Mon Sep 17 00:00:00 2001 From: AngryJay91 Date: Tue, 31 Mar 2026 11:26:52 +0900 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20Quinn=20review=20=E2=80=94?= =?UTF-8?q?=20boolean=20parse,=20error=20handling,=20LIMIT,=20explicit=20c?= =?UTF-8?q?olumns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scaffold/pm-api/src/mcp-tools/standup.ts | 8 +- scaffold/pm-api/src/routes/v2-standup.ts | 14 +-- verification-issue-14-v2.md | 107 +++++++++++++++++++++++ verification-issue-14.md | 42 +++++++++ verification-issue-16-r2.md | 46 ++++++++++ verification-issue-16.md | 50 +++++++++++ verification-issue-17.md | 86 ++++++++++++++++++ verification-issue-3-final.md | 37 ++++++++ verification-issue-3-r2.md | 38 ++++++++ verification-issue-3-r3.md | 53 +++++++++++ verification-issue-4-r2.md | 46 ++++++++++ verification-issue-6.md | 93 ++++++++++++++++++++ 12 files changed, 610 insertions(+), 10 deletions(-) create mode 100644 verification-issue-14-v2.md create mode 100644 verification-issue-14.md create mode 100644 verification-issue-16-r2.md create mode 100644 verification-issue-16.md create mode 100644 verification-issue-17.md create mode 100644 verification-issue-3-final.md create mode 100644 verification-issue-3-r2.md create mode 100644 verification-issue-3-r3.md create mode 100644 verification-issue-4-r2.md create mode 100644 verification-issue-6.md diff --git a/scaffold/pm-api/src/mcp-tools/standup.ts b/scaffold/pm-api/src/mcp-tools/standup.ts index a842cc98..33af936d 100644 --- a/scaffold/pm-api/src/mcp-tools/standup.ts +++ b/scaffold/pm-api/src/mcp-tools/standup.ts @@ -42,7 +42,7 @@ export async function toolSaveStandup(user: string, args: Record): Promise { const sprint = await resolveSprint(args.sprint as string | undefined) const date = args.date as string | undefined - const withFeedback = Boolean(args.with_feedback) + const withFeedback = args.with_feedback === true || args.with_feedback === 'true' let sql: string const sqlArgs: (string | number)[] = [] @@ -64,6 +64,8 @@ export async function toolListStandupEntries(args: Record): Pro if (result.error) return err(result.error) if (result.rows.length === 0) return text('No standup records found.') + const lines = ['πŸ“ Standup Records', '─────────────'] + // Batch-fetch feedback in a single IN query when requested β€” eliminates N+1 type FeedbackRow = { standup_entry_id: number; feedback_by: string; feedback_text: string; review_type: string; created_at: string } const feedbackMap: Record = {} @@ -79,10 +81,10 @@ export async function toolListStandupEntries(args: Record): Pro if (!feedbackMap[f.standup_entry_id]) feedbackMap[f.standup_entry_id] = [] feedbackMap[f.standup_entry_id].push(f) } + } else { + lines.push('⚠️ Warning: could not load feedback data.') } } - - const lines = ['πŸ“ Standup Records', '─────────────'] for (const e of result.rows) { lines.push(`\nπŸ‘€ ${e.user_name} (${e.entry_date})`) if (e.done_text) lines.push(` βœ… ${e.done_text}`) diff --git a/scaffold/pm-api/src/routes/v2-standup.ts b/scaffold/pm-api/src/routes/v2-standup.ts index 435ee2e8..f5e24499 100644 --- a/scaffold/pm-api/src/routes/v2-standup.ts +++ b/scaffold/pm-api/src/routes/v2-standup.ts @@ -12,7 +12,7 @@ app.get('/entries', async (c) => { if (date && sprint) { const { rows } = await queryOrThrow( - 'SELECT * FROM pm_standup_entries WHERE sprint = ? AND entry_date = ? ORDER BY user_name', + 'SELECT id, sprint, user_name, entry_date, done_text, plan_text, plan_story_ids, blockers, created_at, updated_at FROM pm_standup_entries WHERE sprint = ? AND entry_date = ? ORDER BY user_name', [sprint, date], ) return c.json({ entries: rows }) @@ -20,7 +20,7 @@ app.get('/entries', async (c) => { if (date) { const { rows } = await queryOrThrow( - 'SELECT * FROM pm_standup_entries WHERE entry_date = ? ORDER BY user_name', + 'SELECT id, sprint, user_name, entry_date, done_text, plan_text, plan_story_ids, blockers, created_at, updated_at FROM pm_standup_entries WHERE entry_date = ? ORDER BY user_name', [date], ) return c.json({ entries: rows }) @@ -28,7 +28,7 @@ app.get('/entries', async (c) => { if (sprint) { const { rows } = await queryOrThrow( - 'SELECT * FROM pm_standup_entries WHERE sprint = ? ORDER BY entry_date DESC, user_name LIMIT 50', + 'SELECT id, sprint, user_name, entry_date, done_text, plan_text, plan_story_ids, blockers, created_at, updated_at FROM pm_standup_entries WHERE sprint = ? ORDER BY entry_date DESC, user_name LIMIT 50', [sprint], ) return c.json({ entries: rows }) @@ -77,7 +77,7 @@ app.get('/entries-with-feedback', async (c) => { const where = conditions.length ? `WHERE ${conditions.join(' AND ')}` : '' const { rows: entries } = await queryOrThrow( - `SELECT e.* FROM pm_standup_entries e ${where} ORDER BY e.entry_date DESC, e.user_name`, + `SELECT e.id, e.sprint, e.user_name, e.entry_date, e.done_text, e.plan_text, e.plan_story_ids, e.blockers, e.created_at, e.updated_at FROM pm_standup_entries e ${where} ORDER BY e.entry_date DESC, e.user_name LIMIT 100`, params, ) @@ -87,7 +87,7 @@ app.get('/entries-with-feedback', async (c) => { const entryIds = (entries as Array<{ id: number }>).map(e => e.id) const ph = entryIds.map(() => '?').join(',') const { rows: allFeedback } = await queryOrThrow( - `SELECT * FROM pm_standup_feedback WHERE standup_entry_id IN (${ph}) ORDER BY created_at ASC`, + `SELECT id, standup_entry_id, sprint, target_user, feedback_by, feedback_text, review_type, created_at FROM pm_standup_feedback WHERE standup_entry_id IN (${ph}) ORDER BY created_at ASC`, entryIds, ) @@ -116,7 +116,7 @@ app.get('/feedback', async (c) => { if (entryId) { const { rows } = await queryOrThrow( - 'SELECT * FROM pm_standup_feedback WHERE standup_entry_id = ? ORDER BY created_at ASC', + 'SELECT id, standup_entry_id, sprint, target_user, feedback_by, feedback_text, review_type, created_at FROM pm_standup_feedback WHERE standup_entry_id = ? ORDER BY created_at ASC', [Number(entryId)], ) return c.json({ feedback: rows }) @@ -124,7 +124,7 @@ app.get('/feedback', async (c) => { if (sprint && user) { const { rows } = await queryOrThrow( - 'SELECT f.* FROM pm_standup_feedback f WHERE f.sprint = ? AND f.target_user = ? ORDER BY f.created_at DESC LIMIT 50', + 'SELECT f.id, f.standup_entry_id, f.sprint, f.target_user, f.feedback_by, f.feedback_text, f.review_type, f.created_at FROM pm_standup_feedback f WHERE f.sprint = ? AND f.target_user = ? ORDER BY f.created_at DESC LIMIT 50', [sprint, user], ) return c.json({ feedback: rows }) diff --git a/verification-issue-14-v2.md b/verification-issue-14-v2.md new file mode 100644 index 00000000..959acd6b --- /dev/null +++ b/verification-issue-14-v2.md @@ -0,0 +1,107 @@ +# Verification β€” Issue #14 (v2) + +PR: **#21** (`fix/issue-14-standup-n1-v2`) +Reviewer: **Quinn (πŸ§ͺ)** +Date: 2026-03-31 + +## Verdict +**REQUEST_CHANGES** + +## Checklist + +### 1) Diff scope: only pm-api files +Command: +```bash +git diff main...fix/issue-14-standup-n1-v2 --stat +``` +Result: +- `scaffold/pm-api/src/mcp-tools/standup.ts` +- `scaffold/pm-api/src/mcp.ts` +- `scaffold/pm-api/src/routes/v2-standup.ts` + +βœ… Scope is limited to expected `pm-api` files. + +--- + +### 2) N+1 elimination +- In MCP tool (`toolListStandupEntries`), feedback is fetched via one `IN (...)` query when `with_feedback=true`. +- In API route (`GET /entries-with-feedback`), entries are fetched first, then all feedback in one `IN (...)` query. + +βœ… N+1 pattern is eliminated in both new/extended paths. + +--- + +### 3) Existing response format maintained +- Existing `GET /entries` route response remains `{ entries: [...] }` and unchanged. +- Existing MCP `list_standup_entries` output format is unchanged when `with_feedback` is not provided/false. + +βœ… Backward-compatible for existing consumers in default path. + +--- + +### 4) New MCP tool added +- No **new MCP tool name** was added. +- Existing `list_standup_entries` was extended with a new optional param: `with_feedback`. + +⚠️ If requirement strictly means β€œnew tool”, this is **not met**. + +--- + +## Potential Issues (3+) + +### 1. Boolean parsing bug for `with_feedback` (High) +In `toolListStandupEntries`: +```ts +const withFeedback = Boolean(args.with_feedback) +``` +If caller sends string `'false'` (common in loosely typed clients), `Boolean('false') === true`, so feedback is unexpectedly included. + +**Impact:** surprising behavior, extra DB load, incorrect semantics. + +**Fix suggestion:** strict parse: +- `args.with_feedback === true || args.with_feedback === 'true'` +- or schema-level coercion/validation before tool handler. + +--- + +### 2. Silent failure on feedback query in MCP path (Medium) +In `toolListStandupEntries`, feedback query errors are ignored: +```ts +if (!fbResult.error) { ... } +``` +When feedback query fails, tool still returns standup entries without any warning. + +**Impact:** hidden data loss; operators cannot detect partial failures. + +**Fix suggestion:** +- Return error when `with_feedback=true` and feedback fetch fails, or +- include explicit warning in output (`⚠️ feedback unavailable: ...`). + +--- + +### 3. `entries-with-feedback` uses `SELECT *` for both tables (Medium) +Route query uses: +- `SELECT e.* FROM pm_standup_entries e ...` +- `SELECT * FROM pm_standup_feedback ...` + +**Impact:** response shape can change unintentionally on schema evolution, larger payloads than needed, tighter coupling to DB schema. + +**Fix suggestion:** enumerate required columns explicitly. + +--- + +### 4. Potentially unbounded result size in new endpoint (Medium) +`GET /entries-with-feedback` has no `LIMIT` (unlike sprint-only path in `/entries`, which limits to 50 for sprint query). + +**Impact:** large responses and heavy `IN (...)` list for large sprints/date ranges. + +**Fix suggestion:** add pagination/limit defaults and explicit max cap. + +--- + +## Summary +- Core N+1 fix intent is good and correctly implemented with batch fetch. +- However, there are correctness/operability concerns (boolean coercion, silent partial failures, unbounded payload, `SELECT *`). +- Also, requirement wording says β€œnew MCP tool added”, but implementation extends existing tool instead. + +**Final: REQUEST_CHANGES** \ No newline at end of file diff --git a/verification-issue-14.md b/verification-issue-14.md new file mode 100644 index 00000000..e7cf3c7b --- /dev/null +++ b/verification-issue-14.md @@ -0,0 +1,42 @@ +# Verification β€” Issue #14 / PR #20 + +## Scope checked +- PR: #20 (`fix/issue-14-standup-n1` β†’ `main`) +- Command run: `git diff --stat main...fix/issue-14-standup-n1` + +## 1) Diff stat +```bash + README.md | 197 +++++++++++++++++++++- + scaffold/spec-site/src/pages/MockupEditorPage.vue | 5 +- + scaffold/spec-site/src/pages/MockupListPage.vue | 5 +- + 3 files changed, 203 insertions(+), 4 deletions(-) +``` + +## 2) N+1 elimination verified? +- **Not verified / Fail.** +- There are **no pm-api backend query-layer changes** in this PR, so there is no evidence that per-member loop queries were replaced by JOIN/IN batch queries. + +## 3) Existing response format maintained? +- **Not applicable from implemented backend perspective.** +- Since target API endpoints were not modified in this PR, we cannot validate intended response-preservation for the claimed standup fix. + +## 4) New MCP tool added? +- **Fail.** +- No MCP server/tool registration changes appear in this PR diff. + +## 5) Only pm-api files modified (not spec-site)? +- **Fail.** +- Modified files are `README.md` and `scaffold/spec-site/...` only. +- This is the opposite of expected scope. + +## 6) Potential issues (>=3) +1. **PR content/scope mismatch (critical):** Title/body claim standup N+1 backend fix, but actual diff only changes docs/spec-site UI. +2. **Acceptance criteria not implemented:** Missing `GET /standup/entries-with-feedback` endpoint implementation in `pm-api`. +3. **No backend batch query evidence:** No SQL/query-builder code showing `IN (...)`/JOIN batch retrieval for feedback. +4. **No MCP tool addition:** Claimed `list_standup_entries_with_feedback` tool is absent from changed files. +5. **High merge risk:** If merged as-is, issue #14 remains unresolved while PR metadata indicates fixed. + +## 7) Verdict +## **REQUEST_CHANGES** + +PR #20 does not implement the described standup N+1 fix. Please update branch with actual `pm-api` endpoint + query changes (and MCP tool registration), then re-request review. diff --git a/verification-issue-16-r2.md b/verification-issue-16-r2.md new file mode 100644 index 00000000..f8edefd9 --- /dev/null +++ b/verification-issue-16-r2.md @@ -0,0 +1,46 @@ +# Verification Report β€” Issue #16 README (Round 2) + +## Scope Re-check + +Command run: +```bash +git diff main...origin/docs/issue-16-readme --stat +``` + +Result: +```text +README.md | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- +1 file changed, 195 insertions(+), 2 deletions(-) +``` + +βœ… Scope is correct: only `README.md` changed. + +--- + +## Content Checks + +### 1) CLI commands documented +Verified in README CLI Reference + Command Details: +- `init` +- `hydrate` +- `deploy` +- `migrate` +- `doctor` + +βœ… All required commands are documented. + +### 2) `--platform` option documented +Verified in README: +- `--platform=` listed in CLI options +- Dedicated `--platform` section with supported values (`claude-code`, `codex`, `gemini`) +- Usage examples included + +βœ… `--platform` is clearly documented. + +--- + +## Verdict + +**APPROVE** + +PR #19 (docs/issue-16-readme) satisfies the requested scope fix and documentation requirements. \ No newline at end of file diff --git a/verification-issue-16.md b/verification-issue-16.md new file mode 100644 index 00000000..2cc1f9c0 --- /dev/null +++ b/verification-issue-16.md @@ -0,0 +1,50 @@ +# Verification Report β€” PR #19 (`docs/issue-16-readme`) + +## Result +**REQUEST_CHANGES** + +## Checklist + +### 1) `git diff main...docs/issue-16-readme --stat` +```text +README.md | 197 ++++++++++++++++++++++++++++++- +scaffold/pm-api/src/mcp-tools/standup.ts | 57 ++++++++- +scaffold/pm-api/src/mcp.ts | 22 +++- +scaffold/pm-api/src/routes/v2-standup.ts | 57 ++++++++- +4 files changed, 323 insertions(+), 10 deletions(-) +``` + +### 2) Verify all 5 CLI commands documented +- `init` βœ… +- `hydrate` βœ… +- `deploy` βœ… +- `migrate` βœ… +- `doctor` βœ… + +All 5 are documented in README command reference and expanded command details. + +### 3) Verify `--platform` option documented +- `--platform=` is documented in Options βœ… +- Dedicated `--platform` section with supported IDs (`claude-code`, `codex`, `gemini`) βœ… +- Usage examples provided βœ… + +### 4) Only README.md modified (no code changes) +**FAILED** ❌ + +Files changed include non-doc code files: +- `scaffold/pm-api/src/mcp-tools/standup.ts` +- `scaffold/pm-api/src/mcp.ts` +- `scaffold/pm-api/src/routes/v2-standup.ts` + +So this is **not** a README-only/docs-only PR. + +### 5) 3+ potential issues +1. **Scope violation (blocking):** PR includes 3 non-README source code files, conflicting with the stated purpose (β€œREADME docs update for issue-16”). +2. **Reviewability risk:** Mixing docs expansion and runtime code changes in one PR makes validation and rollback harder (docs issue should be isolated or code should be moved to separate PR). +3. **Troubleshooting platform inconsistency:** Troubleshooting section says β€œRestart Claude Code” in MCP context, while README now supports multi-platform adapters (`codex`, `gemini`). Guidance should be platform-neutral or include alternatives. +4. **Potential over-assertion in migrations troubleshooting:** β€œduplicate column/already exists is expected and safe to ignore” can hide real migration faults if error cause is not actually idempotency-related; wording should be more guarded. + +### 6) APPROVE or REQUEST_CHANGES +**REQUEST_CHANGES** + +Primary blocker: requirement #4 failed (non-README files modified). diff --git a/verification-issue-17.md b/verification-issue-17.md new file mode 100644 index 00000000..901907bf --- /dev/null +++ b/verification-issue-17.md @@ -0,0 +1,86 @@ +# PR #18 Verification β€” fix/issue-17-mockup-icons + +## Verdict +**APPROVE** βœ… + +The core bug is fixed: both previously string-literal icon renderings were replaced with real Vue component rendering via `v-if/v-else`. + +--- + +## 1) Diff stat +```bash +git diff --stat main...fix/issue-17-mockup-icons +``` + +```text +scaffold/spec-site/src/pages/MockupEditorPage.vue | 5 ++++- +scaffold/spec-site/src/pages/MockupListPage.vue | 5 ++++- +2 files changed, 8 insertions(+), 2 deletions(-) +``` + +--- + +## 2) Icon rendering verification (component vs string) + +### A. `MockupEditorPage.vue` +- **Before:** + - Used mustache with string literals like: + - `''` + - `''` + - This renders text, not a Vue component. +- **After:** + - Replaced with: + - `` + - `` +- βœ… Correctly renders real Icon components. + +### B. `MockupListPage.vue` +- **Before:** + - `{{ m.viewport === 'mobile' ? 'πŸ“±' : '' }}` + - Desktop icon branch rendered as text. +- **After:** + - Replaced with: + - `πŸ“±` + - `` +- βœ… Correctly renders Icon component for desktop. + +--- + +## 3) File scope check +```bash +git diff --name-only main...fix/issue-17-mockup-icons +``` + +```text +scaffold/spec-site/src/pages/MockupEditorPage.vue +scaffold/spec-site/src/pages/MockupListPage.vue +``` + +βœ… Only spec-site files were modified. + +--- + +## 4) Potential issues (3+) + +1. **Viewport fallback behavior is broad** (Low) + - `v-else` in `MockupListPage.vue` treats any non-`mobile` value as desktop monitor icon. + - If backend starts sending unexpected values (`tablet`, `unknown`, `null`), UI may silently show incorrect desktop icon. + - Suggestion: explicit `v-else-if="m.viewport === 'desktop'"` + fallback icon/text. + +2. **Potential visual alignment mismatch** (Low) + - Mobile uses emoji in ``, desktop uses SVG Icon at fixed size `28`. + - Emoji font metrics differ by OS/browser; cards can appear slightly misaligned. + - Suggestion: normalize via wrapper style (`display:flex; align-items:center; justify-content:center;`) and fixed box size. + +3. **No regression test coverage in this PR** (Low) + - Template rendering bug was fixed, but no test/assertion added to prevent future regressions. + - Suggestion: add component test asserting that icon element is present and that raw `` text is absent. + +4. **Inline conditional complexity can grow** (Low) + - Current `v-if/v-else` is correct, but repeated template branching for icon selection may spread. + - Suggestion: move to computed helper (e.g., `isLocked`, `isMobile`, `viewportIconName`) if additional states are introduced. + +--- + +## Final decision +**APPROVE** β€” Fix is correct, scope is minimal, and the original rendering bug is resolved without unrelated changes. \ No newline at end of file diff --git a/verification-issue-3-final.md b/verification-issue-3-final.md new file mode 100644 index 00000000..bb8a5fa6 --- /dev/null +++ b/verification-issue-3-final.md @@ -0,0 +1,37 @@ +# Verification β€” Issue #3 Final (PR #10) + +## Verdict: βœ… APPROVE + +## Scope checked +Requested final checks only: +1. `scaffold/pm-api/src/routes/v2-admin.ts` regenerate UPDATE query +2. `008-hash-tokens.sql` target table sanity +3. Build check (if possible) + +## Findings + +### 1) Regenerate UPDATE query (PASS) +From: +`git diff main...fix/issue-3-token-hashing -- scaffold/pm-api/src/routes/v2-admin.ts | grep -A2 "regenerate\|UPDATE auth_tokens"` + +Observed: +- SQL is now: + - `UPDATE auth_tokens SET token = ?, token_hash = ?, created_at = CURRENT_TIMESTAMP WHERE token = ? OR token_hash = ?` +- Args are now: + - `[body.newToken, newTokenHash, token, await hashToken(token)]` + +This satisfies both required conditions: +- Uses `SET token = ?, token_hash = ?` (not `token = NULL`) +- Includes new token value as first parameter + +### 2) Migration target sanity (PASS) +`scaffold/pm-api/sql/008-hash-tokens.sql` clearly targets `auth_tokens`: +- `ALTER TABLE auth_tokens ADD COLUMN token_hash TEXT;` +- `CREATE UNIQUE INDEX ... ON auth_tokens(token_hash);` + +### 3) Build check (attempted; FAIL unrelated) +Ran `npm run build` in `scaffold/pm-api`. +TypeScript build currently fails, but errors are in other files (e.g. `src/index.ts`, `v2-meetings.ts`, `v2-rewards.ts`) and are not related to the regenerate token query change. + +## Conclusion +For the requested issue-specific final verification, implementation is correct. Approving. \ No newline at end of file diff --git a/verification-issue-3-r2.md b/verification-issue-3-r2.md new file mode 100644 index 00000000..bdbd12da --- /dev/null +++ b/verification-issue-3-r2.md @@ -0,0 +1,38 @@ +# Verification Report β€” PR #10 (fix/issue-3-token-hashing) β€” R2 + +## Scope checked +`git diff main...fix/issue-3-token-hashing` + +Files relevant to this re-check: +- `scaffold/pm-api/src/routes/v2-admin.ts` +- `scaffold/pm-api/src/routes/auth.ts` +- `scaffold/pm-api/sql/008-hash-tokens.sql` + +## Checklist + +1. **Diff reviewed** βœ… +2. **`v2-admin.ts`: new tokens store `token = NULL`, only `token_hash`** βœ… + - `INSERT INTO auth_tokens (token, token_hash, ...) VALUES (NULL, ?, ...)` + - Regenerate path also sets `token = NULL, token_hash = ?` +3. **Auth verify route uses dual lookup** βœ… + - `WHERE (token_hash = ? OR token = ?) AND is_active = 1` +4. **`008-hash-tokens.sql`: UNIQUE index + backfill comment** ⚠️ **Partially** + - UNIQUE index exists, but targets the wrong table: + - `CREATE UNIQUE INDEX ... ON members(token_hash);` ❌ + - Should be on `auth_tokens(token_hash)`. + - Backfill comment exists, but also references wrong table: + - `UPDATE members SET token = NULL ...` ❌ + - Should reference `auth_tokens`. + +## Additional blocking concern +- Current base schema (`schema-core.sql`) defines `auth_tokens.token` as `TEXT NOT NULL UNIQUE`. +- This PR now inserts `token = NULL` for new rows, so without a migration to relax/remove `NOT NULL`, inserts can fail. + +## Verdict +**REQUEST_CHANGES** + +## Required fixes before approval +1. In `008-hash-tokens.sql`, change UNIQUE index target to: + - `auth_tokens(token_hash)` +2. Fix backfill comment table name to `auth_tokens`. +3. Add/confirm schema migration step for making `auth_tokens.token` nullable (or otherwise compatible with storing `NULL`). diff --git a/verification-issue-3-r3.md b/verification-issue-3-r3.md new file mode 100644 index 00000000..c312cc90 --- /dev/null +++ b/verification-issue-3-r3.md @@ -0,0 +1,53 @@ +# Verification Report β€” PR #10 (fix/issue-3-token-hashing) + +## Verdict: REQUEST_CHANGES ❌ + +## Scope reviewed +- `git diff main...fix/issue-3-token-hashing` +- Focus files: + - `scaffold/pm-api/sql/008-hash-tokens.sql` + - `scaffold/pm-api/src/routes/v2-admin.ts` + - `scaffold/pm-api/src/auth.ts` + - `scaffold/pm-api/src/routes/auth.ts` + +## Required checks + +### 1) Diff reviewed +- Reviewed all relevant auth-token hashing changes in PR diff. + +### 2) `008-hash-tokens.sql` targets `auth_tokens` table (NOT members) +- βœ… PASS +- Migration clearly modifies `auth_tokens`. + +### 3) UNIQUE index on `auth_tokens(token_hash)` +- βœ… PASS +- `CREATE UNIQUE INDEX IF NOT EXISTS idx_auth_tokens_token_hash ON auth_tokens(token_hash);` + +### 4) `v2-admin.ts`: INSERT stores original token + token_hash (NOT NULL compat) +- βœ… PASS (for INSERT path) +- Both insert branches include `(token, token_hash, ...)` and pass original token plus hash. + +### 5) `auth.ts` + `routes/auth.ts`: dual lookup on `auth_tokens` +- βœ… PASS +- Both use `(token_hash = ? OR token = ?)` with hashed+raw token inputs. + +## Blocking issue found + +### πŸ”΄ `v2-admin.ts` regenerate path breaks NOT NULL/PK compatibility +- File: `scaffold/pm-api/src/routes/v2-admin.ts` +- Current code in `/members/:token/regenerate`: + - `UPDATE auth_tokens SET token = NULL, token_hash = ?, ...` +- Problem: + - In existing schema (`001-memo-v2.sql`), `auth_tokens.token` is `TEXT PRIMARY KEY` (implicitly NOT NULL). + - Setting `token = NULL` violates PK/NOT NULL constraint and will fail at runtime. +- This also contradicts the stated migration strategy of keeping legacy token column during transition. + +## Recommendation to pass +- In regenerate flow, keep `token` non-null (store new plaintext token placeholder or actual new token) while updating `token_hash`. +- Example direction: + - `SET token = ?, token_hash = ? ...` + - with lookup fallback `WHERE token = ? OR token_hash = ?` as currently intended. + +--- + +Given the blocking schema-compatibility bug above, this PR is **not safe to approve yet**. diff --git a/verification-issue-4-r2.md b/verification-issue-4-r2.md new file mode 100644 index 00000000..bac12bda --- /dev/null +++ b/verification-issue-4-r2.md @@ -0,0 +1,46 @@ +# PR #11 Re-Verification (Round 2) + +Branch: `fix/issue-4-auth-guard` +Base: `main` + +## Scope checked +- `git diff main...fix/issue-4-auth-guard` +- `scaffold/spec-site/src/pages/LoginPage.vue` +- `scaffold/spec-site/src/router.ts` + +## Checklist result + +1. **Diff verified** βœ… + Changed files are exactly: + - `scaffold/spec-site/src/pages/LoginPage.vue` (new) + - `scaffold/spec-site/src/router.ts` + +2. **LoginPage.vue redirect sanitization** βœ… + Implemented: + ```ts + const redirectTo = (raw.startsWith('/') && !raw.startsWith('//') && !/^[a-z][a-z0-9+\-.]*:/i.test(raw)) + ? raw + : '/' + ``` + This blocks protocol-relative (`//...`) and protocol-based (`http:`, `javascript:`, etc.) redirects. + +3. **router.ts requiresAuth cleanup + auth-by-default comment** βœ… + - Guard strategy now documented as **auth-by-default** with `meta.public` opt-out. + - Comment explicitly says not to use `requiresAuth`. + - Guard logic is centralized in `router.beforeEach`. + +4. **router.ts token format sanity check** βœ… + Implemented: + ```ts + const tokenValid = typeof token === 'string' && token.length >= 8 && token.length <= 512 + ``` + This is stronger than existence-only checks. + +5. **LoginPage.vue auto-login caching** βœ… + `sessionStorage` key (`auth-verified`) is used to avoid repeated verify API calls in same session. + +## Verdict + +**APPROVE** βœ… + +All previously raised issues are addressed in this PR revision and align with the requested fixes. \ No newline at end of file diff --git a/verification-issue-6.md b/verification-issue-6.md new file mode 100644 index 00000000..7cec33cd --- /dev/null +++ b/verification-issue-6.md @@ -0,0 +1,93 @@ +# PR #12 Verification β€” issue-6-get-by-id + +## Verdict +**APPROVE** βœ… + +μš”κ΅¬μ‚¬ν•­ κΈ°μ€€μœΌλ‘œ 핡심 λ²”μœ„λŠ” μΆ©μ‘±λ˜μ—ˆμŠ΅λ‹ˆλ‹€. + +--- + +## 1) Diff stat +```bash +git diff main...feat/issue-6-get-by-id --stat +``` + +κ²°κ³Ό: +- `scaffold/mcp-pm/src/index.ts` | 60 insertions +- `scaffold/pm-api/src/mcp.ts` | 48 insertions +- `scaffold/pm-api/src/routes/v2-nav.ts` | 11 insertions +- `scaffold/pm-api/src/routes/v2-pm.ts` | 24 insertions +- **총 4 files changed, 143 insertions** + +## 2) GET /:id 라우트 검증 (epics/stories/tasks/sprints + 404) +확인 파일: +- `scaffold/pm-api/src/routes/v2-pm.ts` + - `GET /epics/:id` + - `GET /stories/:id` + - `GET /tasks/:id` + - λͺ¨λ‘ `if (!result.rows.length) return c.json({ error: 'Not found' }, 404)` 처리 있음 +- `scaffold/pm-api/src/routes/v2-nav.ts` + - `GET /sprints/:id` + - λ™μΌν•˜κ²Œ not found μ‹œ 404 처리 있음 + +βœ… μš”κ΅¬μ‚¬ν•­ μΆ©μ‘± + +## 3) TOOLS λ°°μ—΄μ˜ 4개 MCP 도ꡬ 검증 +확인 파일: `scaffold/pm-api/src/mcp.ts` +- `name: 'get_sprint'` (좔가됨) +- `name: 'get_epic'` (좔가됨) +- `name: 'get_story'` (좔가됨) +- `name: 'get_task'` (κΈ°μ‘΄ 쑴재) + +βœ… μ΅œμ’… TOOLS λ°°μ—΄μ—λŠ” 4개(`get_epic/get_story/get_task/get_sprint`) λͺ¨λ‘ 쑴재 + +## 4) mcp-pm의 server.tool() 4개 검증 +확인 파일: `scaffold/mcp-pm/src/index.ts` +- `server.tool('get_task', ...)` (κΈ°μ‘΄) +- `server.tool('get_sprint', ...)` (μΆ”κ°€) +- `server.tool('get_epic', ...)` (μΆ”κ°€) +- `server.tool('get_story', ...)` (μΆ”κ°€) + +βœ… μš”κ΅¬μ‚¬ν•­ μΆ©μ‘± + +μ°Έκ³ : μΆ”κ°€λ‘œ `server.tool('get_task_raw', ...)`도 μƒˆλ‘œ μ‘΄μž¬ν•©λ‹ˆλ‹€. + +## 5) Build 검증 +μ‹€ν–‰: +```bash +cd scaffold/mcp-pm && npm run build +``` +κ²°κ³Ό: +- `tsc` 성곡 (μ—λŸ¬ μ—†μŒ) + +βœ… λΉŒλ“œ 톡과 + +--- + +## 6) Potential issues (μ΅œμ†Œ 3개) +μ•„λž˜λŠ” **잠재 이슈/μ •ν•©μ„± 리슀크**이며, 이번 PR의 λ¨Έμ§€ λΈ”λ‘œμ»€λŠ” μ•„λ‹™λ‹ˆλ‹€. + +1. **도ꡬ 넀이밍 μ •ν•©μ„± 혼재 (`get_task` vs `get_task_raw`)** + - `get_task`λŠ” κΈ°μ‘΄ 상세/μ»¨ν…μŠ€νŠΈ 포맷(`/api/tasks/:id`)을 μ‚¬μš©ν•˜κ³ , + - 이번 PRμ—μ„œ `get_task_raw`κ°€ `/api/v2/pm/tasks/:id`λ₯Ό 직접 λ…ΈμΆœν•©λ‹ˆλ‹€. + - 동일 λ„λ©”μΈμ—μ„œ κ²°κ³Ό ν˜•μ‹μ΄ μ΄μ›ν™”λ˜μ–΄ ν΄λΌμ΄μ–ΈνŠΈ ν˜Όλž€ κ°€λŠ₯성이 μžˆμŠ΅λ‹ˆλ‹€. + +2. **MCP κ΅¬ν˜„ 경둜 μ΄μ›ν™”λ‘œ μΈν•œ λ“œλ¦¬ν”„νŠΈ μœ„ν—˜ (`mcp.ts` DB 직접 쑰회 vs `mcp-pm` API 호좜)** + - `scaffold/pm-api/src/mcp.ts`λŠ” DB queryλ₯Ό 직접 μˆ˜ν–‰ν•˜κ³ , + - `scaffold/mcp-pm/src/index.ts`λŠ” HTTP APIλ₯Ό ν˜ΈμΆœν•©λ‹ˆλ‹€. + - μŠ€ν‚€λ§ˆ/ν•„λ“œ/μ—λŸ¬ 포맷 λ³€κ²½ μ‹œ 두 경둜의 λ™μž‘ 뢈일치 κ°€λŠ₯성이 ν½λ‹ˆλ‹€. + +3. **μ—λŸ¬ λ©”μ‹œμ§€ 포맷 비일관성 (`Not found` vs `Epic/Story/Sprint not found`)** + - REST λΌμš°νŠΈλŠ” `{ error: 'Not found' }` ν˜•νƒœ, + - MCP switchλŠ” `'Epic not found'`, `'Story not found'`, `'Sprint not found'` λ¬Έμžμ—΄. + - ν΄λΌμ΄μ–ΈνŠΈκ°€ μ—λŸ¬λ₯Ό κ·œκ²©ν™” μ²˜λ¦¬ν•  λ•Œ λΆ„κΈ° λ³΅μž‘λ„κ°€ μ¦κ°€ν•©λ‹ˆλ‹€. + +4. **GET /:id 응닡 μŠ€ν‚€λ§ˆ λͺ…μ‹œ λΆ€μ‘± (raw `SELECT *`)** + - epics/stories/tasksλŠ” `SELECT *` λ°˜ν™˜μ΄λΌ μŠ€ν‚€λ§ˆ 변경이 κ³§ API contract λ³€κ²½μœΌλ‘œ μ΄μ–΄μ§ˆ 수 μžˆμŠ΅λ‹ˆλ‹€. + - μž₯기적으둜 λͺ…μ‹œ ν•„λ“œ 선택(Projection) λ˜λŠ” response DTO 고정이 ν•„μš”ν•©λ‹ˆλ‹€. + +--- + +## μ΅œμ’… νŒλ‹¨ +- κΈ°λŠ₯ μš”κ΅¬μ‚¬ν•­(라우트 4μ’…, MCP 도ꡬ 4μ’… 쑴재, mcp-pm tool 등둝, λΉŒλ“œ 톡과)은 μΆ©μ‘±λ˜μ—ˆμŠ΅λ‹ˆλ‹€. +- 잠재 λ¦¬μŠ€ν¬λŠ” μžˆμœΌλ‚˜ μ¦‰μ‹œ 치λͺ… 결함은 ν™•μΈλ˜μ§€ μ•Šμ•„ **APPROVE**ν•©λ‹ˆλ‹€.