feat(mcp+generator): Phase 2a — MCP server, Markdown renderer, tree reconstruction#24
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements Phase 2a of the MCP server roadmap: a stateless, read-only MCP server integrated into Express using Streamable HTTP transport, backed by new database query functions and a Markdown renderer for CSI trees. It also hardens the DOCX/SEC file upload path with pre-job validation. The implementation includes comprehensive documentation, specification, tests, and a local MCP client configuration. ChangesMCP Server Phase 2a Implementation
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Express as POST /mcp
participant Transport as StreamableHTTPServerTransport
participant McpServer as McpServer
participant Tools as MCP Tools
participant DB as Database
Client->>Express: POST JSON-RPC request (tool/resource)
Express->>Transport: instantiate per-request transport
Transport->>McpServer: connect new McpServer instance
McpServer->>Tools: invoke tool handler (search/get_spec/list_sections)
Tools->>DB: query (searchParagraphs / getSpecTree / listCsiSections)
DB-->>Tools: typed rows/result
Tools-->>McpServer: tool result (JSON content)
McpServer-->>Transport: JSON-RPC response
Transport-->>Client: streamed response
Express->>Transport: close transport on response finish
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
src/api/middleware/error.ts (1)
6-10: ⚡ Quick winLower log severity for expected client-side validation failures.
Right now these 4xx paths are logged as
unhandled errorbefore the branch, which can inflate error-level noise.Proposed adjustment
export const errorHandler: ErrorRequestHandler = (err, _req, res, _next) => { - logger.error({ err }, 'unhandled error'); if (err instanceof multer.MulterError) { + logger.warn({ err }, 'request rejected by upload validation'); res.status(400).json({ success: false, error: err.message }); return; } + logger.error({ err }, 'unhandled error'); const status = (err as { status?: number }).status ?? 500; res.status(status).json({ success: false, error: 'internal server error' }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/middleware/error.ts` around lines 6 - 10, The middleware currently logs every error as logger.error before checking for client-side Multer validation, inflating error-level noise; update the error handling in the middleware so you first detect client/4xx errors (e.g., using the multer.MulterError check and any other expected validation errors), and then: a) do not call logger.error for these expected 4xx cases (use logger.warn or logger.info instead or omit logging), b) return the 4xx response (res.status(400).json(...)), and only call logger.error({ err }, 'unhandled error') for unexpected/non-4xx errors; modify the code around the existing logger.error and the multer.MulterError branch to implement this control flow (referencing logger.error and multer.MulterError).src/parser/sec/safety.test.ts (1)
4-26: ⚡ Quick winAdd a direct invalid UTF-8 test case
Please add one test that feeds malformed UTF-8 bytes to lock in the decoder-failure branch.
Suggested test addition
describe('assertSecSafe', () => { + it('rejects invalid UTF-8 byte sequence', () => { + const buf = Buffer.from([0xc3, 0x28]); // malformed UTF-8 + expect(() => assertSecSafe(buf)).toThrow('invalid UTF-8'); + }); + it('accepts valid UTF-8 SEC content', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parser/sec/safety.test.ts` around lines 4 - 26, Add a new test in safety.test.ts that calls assertSecSafe with a Buffer containing malformed UTF-8 bytes to exercise the decoder-failure branch; for example construct a buffer with an invalid sequence (e.g. truncated multi‑byte like 0xC3 followed by 0x28 or lone 0xFF bytes) and assert that assertSecSafe(Buffer(...)) throws (matching the decoder/invalid UTF‑8 error).src/db/queries/specs.ts (1)
88-108: ⚖️ Poor tradeoffConsider immutable Map construction pattern.
The current implementation mutates
childrenByParentwithin the loop (line 91). As per coding guidelines, immutable patterns should be preferred over mutation.A functional approach would rebuild the Map on each iteration:
♻️ Immutable alternative
function buildNodeTree(rows: readonly ParaRow[]): readonly CsiNode[] { - const childrenByParent = new Map<string | null, ParaRow[]>(); - for (const row of rows) { - childrenByParent.set(row.parent_id, [...(childrenByParent.get(row.parent_id) ?? []), row]); - } + const childrenByParent = rows.reduce((acc, row) => { + const siblings = acc.get(row.parent_id) ?? []; + const updated = new Map(acc); + updated.set(row.parent_id, [...siblings, row]); + return updated; + }, new Map<string | null, ParaRow[]>());Note: This has a performance cost (O(n²) instead of O(n)). Since the Map is local and never exposed, the current mutation is pragmatically acceptable for tree-building. Consider this a stylistic preference per the immutability guideline rather than a functional defect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/queries/specs.ts` around lines 88 - 108, The code mutates childrenByParent inside buildNodeTree; to follow the immutability guideline, replace the in-loop mutation with an immutable Map build (e.g., use rows.reduce to produce a new Map< string | null, ParaRow[] > without mutating: for each row create a new Map based on the prior map, update the entry for row.parent_id to a new array [...(old ?? []), row], and return the new Map), then keep the existing buildNode function and final return unchanged; update the declaration of childrenByParent to be produced by that reduce instead of being mutated in the for-loop.src/mcp/server.integration.test.ts (1)
94-94: ⚡ Quick winAvoid global state for test data.
Storing
testSectionin theglobalobject is fragile. SincebeforeAllguarantees run before tests in the same suite, use a module-scoped variable instead.♻️ Refactor to module-scoped variable
let server: Server; let baseUrl: string; let mcpSpecId: string; +let mcpTestSection: string; // ... in beforeAll: - // Store for tests - (global as Record<string, unknown>)['mcpTestSection'] = testSection; + mcpTestSection = testSection; // ... in test: - const testSection = (global as Record<string, unknown>)['mcpTestSection'] as string; + const testSection = mcpTestSection;Also applies to: 173-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server.integration.test.ts` at line 94, The test stores testSection on the global object which is fragile; instead declare a module-scoped variable (e.g., let testSection: YourType | undefined) at the top of the test file and assign to it inside the beforeAll block; replace all reads/writes of (global as Record<string, unknown>)['mcpTestSection'] with the module variable, and do the same for the other occurrence around line 173 so tests access the module-scoped testSection rather than global state.src/parser/docx/safety.ts (2)
111-162: 💤 Low valueMinor: Local array mutation in promise scope.
Line 117 mutates
relEntrieswithpush. While this is local to the promise and never exposed (preserving practical immutability), a strict immutability approach would collect entries functionally.Given ESLint's
skipBlankLines: trueconfiguration, manual line counting isn't reliable for the 50-line limit—defer topnpm lintoutput.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parser/docx/safety.ts` around lines 111 - 162, The handler in scanZipEntries mutates the local relEntries array with relEntries.push(entry); change this to a functional update to avoid in-place mutation: declare relEntries with let (e.g., let relEntries: yauzl.Entry[] = [];) and replace push with an assignment that creates a new array (e.g., relEntries = [...relEntries, entry] or relEntries = relEntries.concat(entry)); keep all other logic (processEntry usage, fail, zip event handlers, and final resolve(relEntries)) the same.
193-196: 💤 Low valueRegEx from template literal is safe but flagged by static analysis.
The static analyzer warns about ReDoS because the regex is constructed from a variable. However,
DANGEROUS_EXTERNAL_REL_TYPESis a const array of safe literals (not user input), and the pattern[^>]*is bounded by XML structure, making this practically safe.If this warning becomes a blocker, consider pre-compiling the patterns.
💡 Optional: Pre-compile regex patterns
+const DANGEROUS_REL_PATTERNS = DANGEROUS_EXTERNAL_REL_TYPES.map( + (relType) => new RegExp( + `relationships/${relType}[^>]*TargetMode\\s*=\\s*["']External["']`, + 'i' + ) +); + function isDangerousExternalRel(xml: string): boolean { - return DANGEROUS_EXTERNAL_REL_TYPES.some((relType) => { - const pattern = new RegExp( - `relationships/${relType}[^>]*TargetMode\\s*=\\s*["']External["']`, - 'i' - ); - return pattern.test(xml); - }); + return DANGEROUS_REL_PATTERNS.some((pattern) => pattern.test(xml)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parser/docx/safety.ts` around lines 193 - 196, The dynamic RegExp construction using `relType` (the `pattern` variable) is triggering ReDoS warnings; replace runtime construction with pre-compiled safe regexes: at module init, map each entry of `DANGEROUS_EXTERNAL_REL_TYPES` to a compiled RegExp (e.g., build a Dictionary/Map of patterns) and then look up and use that precompiled regex instead of creating `new RegExp(...)` inside the code that checks `TargetMode="External"`; ensure you reference the same pattern structure (`relationships/${relType}[^>]*TargetMode\\s*=\\s*["']External["']`) when building the precompiled regexes so callers that use `pattern`/`relType` can simply fetch the compiled RegExp from the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/010-mcp-server.md`:
- Around line 97-101: The fenced code block that shows the MCP routes currently
has no language tag; update the opening fence from ``` to ```text so the block
starts with ```text and retains the same content (the three lines "POST /mcp
...", "GET /mcp ...", "DELETE /mcp ...") to satisfy markdown linting in the ADR
document.
In `@src/generator/markdown.ts`:
- Around line 3-21: The alphabetic label logic in getLabel (NodeType cases
'pr1', 'pr3', 'pr5') currently uses String.fromCharCode(65/97 + index) which
breaks after 26 items; replace that with a base-26 alphabetic converter used by
those three cases so indices map to A..Z, AA..AB... (or a..z, aa..ab... for
lowercase), then append the existing punctuation ('.' for 'pr1'/'pr3', ')' for
'pr5'); implement the converter as a helper (e.g., toAlpha(index, uppercase:
boolean)) and call it from getLabel for 'pr1', 'pr3', and 'pr5' to preserve
formatting while supporting >26 labels.
In `@src/mcp/server.ts`:
- Line 36: The error handler in src/mcp/server.ts currently returns
res.status(500).json({ error: 'internal server error' }) which violates the
ApiResponse<T> contract; update the response to match other endpoints by
returning res.status(500).json({ success: false, error: 'internal server error'
}) (ensure any code paths using the same handler or variable names like res or
the surrounding middleware keep the same semantics).
- Line 27: Remove the type assertion in the call to server.connect (i.e., stop
using "as Transport" on the StreamableHTTPServerTransport instance) and instead
add the recommended tsconfig workaround: set "skipLibCheck": true under
compilerOptions in tsconfig.json to resolve the optional-property mismatch
caused by exactOptionalPropertyTypes; update tsconfig.json accordingly, remove
the assertion in the server.connect call (referencing server.connect and
Transport/StreamableHTTPServerTransport), and run the TypeScript build to verify
the error is gone.
In `@src/parser/sec/safety.ts`:
- Around line 11-13: The catch block in src/parser/sec/safety.ts currently
swallows the original decode error; update the catch to capture the exception
(e.g., catch (err)) and rethrow a ParserError that preserves the original as its
cause (e.g., throw new ParserError('invalid UTF-8 in .sec file', { cause: err
})), so the original decode failure context is retained when using ParserError.
---
Nitpick comments:
In `@src/api/middleware/error.ts`:
- Around line 6-10: The middleware currently logs every error as logger.error
before checking for client-side Multer validation, inflating error-level noise;
update the error handling in the middleware so you first detect client/4xx
errors (e.g., using the multer.MulterError check and any other expected
validation errors), and then: a) do not call logger.error for these expected 4xx
cases (use logger.warn or logger.info instead or omit logging), b) return the
4xx response (res.status(400).json(...)), and only call logger.error({ err },
'unhandled error') for unexpected/non-4xx errors; modify the code around the
existing logger.error and the multer.MulterError branch to implement this
control flow (referencing logger.error and multer.MulterError).
In `@src/db/queries/specs.ts`:
- Around line 88-108: The code mutates childrenByParent inside buildNodeTree; to
follow the immutability guideline, replace the in-loop mutation with an
immutable Map build (e.g., use rows.reduce to produce a new Map< string | null,
ParaRow[] > without mutating: for each row create a new Map based on the prior
map, update the entry for row.parent_id to a new array [...(old ?? []), row],
and return the new Map), then keep the existing buildNode function and final
return unchanged; update the declaration of childrenByParent to be produced by
that reduce instead of being mutated in the for-loop.
In `@src/mcp/server.integration.test.ts`:
- Line 94: The test stores testSection on the global object which is fragile;
instead declare a module-scoped variable (e.g., let testSection: YourType |
undefined) at the top of the test file and assign to it inside the beforeAll
block; replace all reads/writes of (global as Record<string,
unknown>)['mcpTestSection'] with the module variable, and do the same for the
other occurrence around line 173 so tests access the module-scoped testSection
rather than global state.
In `@src/parser/docx/safety.ts`:
- Around line 111-162: The handler in scanZipEntries mutates the local
relEntries array with relEntries.push(entry); change this to a functional update
to avoid in-place mutation: declare relEntries with let (e.g., let relEntries:
yauzl.Entry[] = [];) and replace push with an assignment that creates a new
array (e.g., relEntries = [...relEntries, entry] or relEntries =
relEntries.concat(entry)); keep all other logic (processEntry usage, fail, zip
event handlers, and final resolve(relEntries)) the same.
- Around line 193-196: The dynamic RegExp construction using `relType` (the
`pattern` variable) is triggering ReDoS warnings; replace runtime construction
with pre-compiled safe regexes: at module init, map each entry of
`DANGEROUS_EXTERNAL_REL_TYPES` to a compiled RegExp (e.g., build a
Dictionary/Map of patterns) and then look up and use that precompiled regex
instead of creating `new RegExp(...)` inside the code that checks
`TargetMode="External"`; ensure you reference the same pattern structure
(`relationships/${relType}[^>]*TargetMode\\s*=\\s*["']External["']`) when
building the precompiled regexes so callers that use `pattern`/`relType` can
simply fetch the compiled RegExp from the map.
In `@src/parser/sec/safety.test.ts`:
- Around line 4-26: Add a new test in safety.test.ts that calls assertSecSafe
with a Buffer containing malformed UTF-8 bytes to exercise the decoder-failure
branch; for example construct a buffer with an invalid sequence (e.g. truncated
multi‑byte like 0xC3 followed by 0x28 or lone 0xFF bytes) and assert that
assertSecSafe(Buffer(...)) throws (matching the decoder/invalid UTF‑8 error).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dfb8f2fb-4d19-406d-8988-299bbca3e598
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.mcp.jsonARCHITECTURE.mdREADME.mddocs/adr/010-mcp-server.mddocs/superpowers/plans/2026-05-11-mcp-server-phase-2a.mddocs/superpowers/plans/2026-05-11-security-upload-hardening.mddocs/superpowers/specs/2026-05-11-mcp-server-design.mdeslint.config.jspackage.jsonsrc/api/middleware/error.test.tssrc/api/middleware/error.tssrc/api/parse.integration.test.tssrc/api/parse.test.tssrc/api/parse.tssrc/api/router.tssrc/db/index.tssrc/db/queries/search.integration.test.tssrc/db/queries/search.tssrc/db/queries/specs.integration.test.tssrc/db/queries/specs.tssrc/generator/markdown.test.tssrc/generator/markdown.tssrc/index.tssrc/mcp/resources.tssrc/mcp/server.integration.test.tssrc/mcp/server.tssrc/mcp/tools.tssrc/parser/docx/document.tssrc/parser/docx/index.tssrc/parser/docx/numbering.tssrc/parser/docx/safety.integration.test.tssrc/parser/docx/safety.test.tssrc/parser/docx/safety.tssrc/parser/docx/styles.tssrc/parser/docx/xml-audit.test.tssrc/parser/index.tssrc/parser/sec/index.tssrc/parser/sec/safety.test.tssrc/parser/sec/safety.ts
thewrz
added a commit
that referenced
this pull request
May 12, 2026
…r format, log severity, parse.ts fileFilter, UTF-8 test, ADR fence tag
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/010-mcp-server.md`:
- Around line 122-123: Update the sentence in docs/adr/010-mcp-server.md that
currently states "Tool and resource definitions in `tools.ts` / `resources.ts`
are unchanged by this upgrade" to explicitly indicate that the lists shown
earlier are the future/target scope and not the current Phase 2a implementation;
mention that Phase 2a ships a smaller read-only subset (rather than the full
set), and reference `tools.ts` / `resources.ts` as the target files for the
future expansion so readers understand current vs planned surface area.
In `@src/mcp/server.ts`:
- Around line 28-31: Subscribe the response cleanup handler before calling
transport.handleRequest and don't swallow promise rejections from close calls:
move the res.on/once('finish', ...) registration to before await
transport.handleRequest(req, res, req.body), and in that handler call
transport.close() and server.close() and handle their promise results (e.g.,
await or Promise.allSettled and log errors) instead of using void so failures
aren't dropped; reference transport.handleRequest,
res.on('finish')/res.once('finish'), transport.close, and server.close when
locating the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0bd998a-8b66-4f58-ad14-c90a25f66fa0
📒 Files selected for processing (7)
docs/adr/010-mcp-server.mdsrc/api/middleware/error.tssrc/api/parse.tssrc/generator/markdown.tssrc/mcp/server.tssrc/parser/sec/safety.test.tssrc/parser/sec/safety.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/parser/sec/safety.test.ts
- src/parser/sec/safety.ts
- src/api/middleware/error.ts
- src/generator/markdown.ts
- src/api/parse.ts
…cisions - ARCHITECTURE.md: Phase 2a rewritten — MCP Streamable HTTP integrated into Express (POST /mcp), not a separate process. Adds stateless/stateful upgrade path note, Phase 2b/2c renumbering, updated API table with /mcp routes, and corrected src/mcp/ file structure comments. - README.md: Phase 2 row split into 2a (In Progress) / 2b / 2c; "Not Yet Built" section distinguishes in-progress MCP scope from remaining planned work. - docs/adr/010-mcp-server.md: adds Decision Update section superseding the original stdio/SSE stance — rationale, route layout, stateful upgrade path, and auth hook point documented.
…function; use z.uuid() over deprecated z.string().uuid()
- POST /mcp initialize, tools/call (search_library, get_spec,
list_sections), resources/read (specr://specs/{id}, specr://sections),
GET /mcp 405 stateless check
- mcpCall helper adds Accept: application/json, text/event-stream and
handles SSE vs plain-JSON response from StreamableHTTPServerTransport
- fix(infra): remap docker-compose postgres to 5433 to avoid conflict
with wrzdj-db-1 which holds 127.0.0.1:5432
- refactor(generator): reformat switch cases in markdown.ts (no logic change)
5433 was a local workaround committed accidentally. The DATABASE_URL in .env (gitignored) handles local port overrides; docker-compose.yml should expose the standard 5432 for other developers and CI parity.
…r format, log severity, parse.ts fileFilter, UTF-8 test, ADR fence tag
thewrz
force-pushed
the
feat/parser-docx-1c-ii
branch
from
f1b69b1 to
92d0c3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-05-11-mcp-server-design.md`:
- Around line 27-31: The markdown in
docs/superpowers/specs/2026-05-11-mcp-server-design.md contains unlabeled fenced
code blocks; update each triple-backtick fence around the examples (the HTTP
endpoint list block and the src/ tree block further down) to include a language
token (e.g., ```text) so they satisfy MD040; ensure both occurrences referenced
in the comment (the block showing POST/GET/DELETE /mcp and the subsequent src/
tree block) are changed to use the same language tag.
- Around line 159-162: The inline code spans for table rows `pr2`, `pr3`, `pr4`,
and `pr5` contain leading internal spaces (e.g. "` 1. text`") which triggers
MD038; remove those internal leading spaces inside the backticks (or convert the
indented examples to regular paragraph/text outside code spans) so the code
spans contain only the literal content without leading spaces, ensuring the
table cells use `pr2`, `pr3`, `pr4`, `pr5` labels with clean inline code like
`"1. text"` or plain text instead of backticked values with internal
indentation.
In `@src/api/middleware/error.ts`:
- Around line 11-13: The middleware currently asserts (err as { status?: number
}).status and passes it directly to res.status; instead, inspect and normalize
err.status without a type assertion: read err?.status, if it's a string or
number convert using Number(), ensure it's an integer and in the valid HTTP
range (e.g., 100–599 or at least 400–599 for error responses), otherwise fall
back to 500, then call res.status(normalizedStatus).json(...). Update the code
around logger.error, the status extraction, and the res.status call to perform
these runtime checks (use Number.isInteger and range checks) and remove the
TypeScript type assertion.
In `@src/mcp/server.ts`:
- Around line 18-28: Add a Zod validation step for the incoming MCP envelope
before calling transport.handleRequest: define a minimal Zod schema for the
expected envelope shape and run it against req.body, returning 400 on failure;
only pass the parsed/validated value to
StreamableHTTPServerTransport.handleRequest. Locate this change around the POST
handler where StreamableHTTPServerTransport is instantiated and
createMcpServer() is connected (references: req.body, transport.handleRequest,
StreamableHTTPServerTransport, createMcpServer) and ensure invalid input
short-circuits with res.status(400).json(...) while valid input is the sanitized
object handed to handleRequest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd911076-1633-40a6-a194-940590d8181e
📒 Files selected for processing (22)
.mcp.jsonARCHITECTURE.mdREADME.mddocs/adr/010-mcp-server.mddocs/superpowers/plans/2026-05-11-mcp-server-phase-2a.mddocs/superpowers/specs/2026-05-11-mcp-server-design.mdsrc/api/middleware/error.tssrc/api/parse.tssrc/db/index.tssrc/db/queries/search.integration.test.tssrc/db/queries/search.tssrc/db/queries/specs.integration.test.tssrc/db/queries/specs.tssrc/generator/markdown.test.tssrc/generator/markdown.tssrc/index.tssrc/mcp/resources.tssrc/mcp/server.integration.test.tssrc/mcp/server.tssrc/mcp/tools.tssrc/parser/sec/safety.test.tssrc/parser/sec/safety.ts
💤 Files with no reviewable changes (1)
- src/api/parse.ts
✅ Files skipped from review due to trivial changes (3)
- .mcp.json
- README.md
- docs/superpowers/plans/2026-05-11-mcp-server-phase-2a.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src/index.ts
- src/db/queries/search.integration.test.ts
- src/parser/sec/safety.test.ts
- src/mcp/tools.ts
- src/db/queries/specs.ts
- src/db/queries/specs.integration.test.ts
- src/generator/markdown.ts
- src/generator/markdown.test.ts
- src/mcp/resources.ts
- ARCHITECTURE.md
- src/mcp/server.integration.test.ts
- src/db/queries/search.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp/server.ts`:
- Around line 19-21: The /mcp endpoint is unprotected; add authentication that
validates the Authorization: Bearer <token> header before accepting a transport
connection (same validation logic as the existing REST auth middleware) and
return 401 on failure. Implement this by applying the REST auth middleware (or
extracting its validation function) to the /mcp route/connection handler (the
transport connection acceptor) so that token parsing, verification and rejection
happen prior to any tool/write access. Ensure the middleware/validation function
is reused (not duplicated) and that failure paths send a 401 response and abort
the transport handshake.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ddeeaec-6a03-4892-b79b-b08c6fa15dcf
📒 Files selected for processing (2)
.github/workflows/ci.ymlsrc/mcp/server.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
4 tasks
thewrz
added a commit
that referenced
this pull request
May 13, 2026
…md update (#25) * docs: update README + CLAUDE.md for Phase 2a completion — MCP patterns, status, structure * docs(adr): ADR-010 status → Accepted (Phase 2a), add language tags to all fences
This was referenced May 16, 2026
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /mcp): 3 read-only tools (search_library,get_spec,list_sections) + 2 resources (specr://specs/{id}as Markdown,specr://sectionsas Markdown table). Configure in Claude Code via.mcp.json.src/generator/markdown.ts): purerenderMarkdown(CsiTree)+getLabel()helper. Used by MCP resources; shared with future DOCX generator. Vanish/note nodes render as> **[NOTE]**blockquotes for LLM context.getSpecTree(src/db/queries/specs.ts): reconstructs fullCsiNodehierarchy from flatparagraphstable + cross-reference resolution status (isResolved,isBroken). Feeds theget_spectool.searchParagraphs+listCsiSections(src/db/queries/search.ts): ILIKE text search with optional division filter; CSI section index withinDatabaseflag.ARCHITECTURE.md,README.md,ADR-010reflect stateless Streamable HTTP decision (supersedes ADR-010's original stdio/SSE stance).Architecture note
MCP server is integrated into the existing Express process (not a separate process). Transport:
StreamableHTTPServerTransportwithsessionIdGenerator: undefined(stateless). Upgrade to stateful sessions (Phase 5+) = one parameter change + session Map in route handler; tool/resource definitions unchanged. Auth hook comment insrc/mcp/server.tsmarks the insertion point for future auth.Test plan
pnpm test— 303 unit tests passDATABASE_URL=postgres://specr:specr@localhost:5432/specr pnpm test:integration— 120/121 integration tests pass (1 pre-existing CPI_BUSBAR failure, unrelated)pnpm lint— cleanpnpm build— compilespnpm dev→curl -s -X POST http://localhost:3000/mcp -H "Content-Type: application/json" -d '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"curl","version":"1.0"}}}' | jq '.result.serverInfo'→{"name":"specr","version":"0.1.0"}.mcp.jsonloads in Claude Code withpnpm devrunningOut of scope (deferred)
get_paragraph,parse_documenttools (Phase 2b follow-up)review_spec,suggest_paragraphs(Phase 6)get_spec_difftool / diff resource (Phase 3, needs merge engine)Summary by CodeRabbit
New Features
Improvements
Documentation
Tests