fix(standup): eliminate N+1 query in feedback retrieval (#14)

scaffold/pm-api/src/mcp-tools/standup.ts

@@ -42,6 +42,7 @@ export async function toolSaveStandup(user: string, args: Record<string, unknown
 export async function toolListStandupEntries(args: Record<string, unknown>): Promise<ToolResult> {
   const sprint = await resolveSprint(args.sprint as string | undefined)
   const date = args.date as string | undefined
+  const withFeedback = args.with_feedback === true || args.with_feedback === 'true'
 
   let sql: string
   const sqlArgs: (string | number)[] = []
@@ -59,16 +60,46 @@ export async function toolListStandupEntries(args: Record<string, unknown>): 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.')
 
   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<number, FeedbackRow[]> = {}
+  if (withFeedback && result.rows.length > 0) {
+    const ids = result.rows.map(e => e.id)
+    const ph = ids.map(() => '?').join(',')
+    const fbResult = await query<FeedbackRow>(
+      `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)
+      }
+    } else {
+      lines.push('⚠️ Warning: could not load feedback data.')
+    }
+  }
   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'))
 }

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)' },
       },
     },
   },

scaffold/pm-api/src/routes/v2-standup.ts

@@ -12,23 +12,23 @@ 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 })
   }
 
   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 })
   }
 
   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 })
@@ -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.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,
+  )
+
+  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 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,
+  )
+
+  // Group feedback by standup_entry_id
+  const feedbackMap: Record<number, unknown[]> = {}
+  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=
@@ -71,15 +116,15 @@ 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 })
   }
 
   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 })

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**

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.

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=<id>` 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.