feat(api): style-source assignment — manual template pick for non-DOCX masters#206
Merged
Conversation
…X masters Lets a user manually associate an existing style template with a spec — the narrowest useful slice of #125's three style sources. `.SEC`/`.txt`/PDF masters carry no adopted visual style, so this gives the generator (#32 / WT-6) a real house style to render with. One template per spec. - migration 027: nullable specs.style_template_id FK, ON DELETE RESTRICT (a spec must never silently lose formatting; a referenced template can't be deleted), with an index for the reference-count pre-check. - new src/db/queries/style-source.ts: get/set/clear + countSpecsUsingTemplate. - POST/DELETE /specs/:id/style-source; styleSource surfaced on GET /specs/:id (separate query merged in the handler — getSpecTree untouched) and MCP get_spec. - deleteTemplate now pre-checks the reference count and returns a discriminated result; deleteTemplateHandler maps in_use → 409 (sidesteps the pg 23503 ambiguity: same code means 404 on assign-missing, 409 on delete-referenced). - openapi.yaml + README updated. Closes #138. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughImplements style-source assignment (Issue ChangesStyle-source assignment (Issue
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as Express Router
participant setStyleSourceHandler
participant clearStyleSourceHandler
participant StyleSourceDB as style-source queries
participant TemplatesDB as deleteTemplate
rect rgba(100, 149, 237, 0.5)
note over Client,StyleSourceDB: POST /specs/:id/style-source
Client->>Router: POST /specs/:id/style-source { templateId }
Router->>setStyleSourceHandler: validateBody → handler
setStyleSourceHandler->>StyleSourceDB: getTemplate(templateId) — existence check
StyleSourceDB-->>setStyleSourceHandler: null → 404 template not found
setStyleSourceHandler->>StyleSourceDB: setSpecStyleSource(specId, templateId)
StyleSourceDB-->>setStyleSourceHandler: false → 404 spec not found
StyleSourceDB-->>setStyleSourceHandler: true → 200 { styleSource }
setStyleSourceHandler-->>Client: response
end
rect rgba(60, 179, 113, 0.5)
note over Client,StyleSourceDB: DELETE /specs/:id/style-source
Client->>Router: DELETE /specs/:id/style-source
Router->>clearStyleSourceHandler: handler
clearStyleSourceHandler->>StyleSourceDB: clearSpecStyleSource(specId)
StyleSourceDB-->>clearStyleSourceHandler: false → 404 spec not found
StyleSourceDB-->>clearStyleSourceHandler: true → 200 { styleSource: null }
clearStyleSourceHandler-->>Client: response
end
rect rgba(210, 105, 30, 0.5)
note over Client,TemplatesDB: DELETE /templates/:id — RESTRICT enforcement
Client->>Router: DELETE /templates/:id
Router->>TemplatesDB: deleteTemplate(id)
TemplatesDB->>StyleSourceDB: countSpecsUsingTemplate(id)
StyleSourceDB-->>TemplatesDB: count > 0 → { deleted: false, reason: in_use, inUseBy }
TemplatesDB-->>Router: in_use result → 409 Conflict
Router-->>Client: 409
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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-06-16-issue-138-design.md`:
- Line 40: Add the language identifier "text" to the fenced code block opening
on line 40 by changing the opening triple backticks to include the language
specification. On line 56, replace the markdown heading syntax "`#134`" with plain
text by rewriting it as "Issue `#134`" to prevent it from being parsed as a
malformed ATX heading. These changes will resolve the MD040 and MD018
markdownlint violations in the design document.
- Around line 66-68: The documented return types for setSpecStyleSource and
clearSpecStyleSource in this design spec are incorrectly specified as
Promise<void>, but the actual implementation in src/api/style-source.ts returns
boolean values (assigned/cleared flags) that are used to determine 404 responses
for missing specs. Update the return type documentation for both
setSpecStyleSource and clearSpecStyleSource from Promise<void> to
Promise<boolean> to accurately reflect the actual handler contract and prevent
confusion for future implementers.
In `@openapi.yaml`:
- Around line 1756-1757: The 409 response at lines 1756-1757 currently uses a
generic Conflict reference that describes a resource identity conflict, but the
actual endpoint implementation returns 409 when a template is in use by N specs.
Replace the generic $ref to `#/components/responses/Conflict` with an
operation-specific response description that accurately conveys the actual error
condition (template in use), ensuring the OpenAPI contract precisely matches the
behavior defined in the templates-crud.ts endpoint implementation.
In `@src/api/style-source.integration.test.ts`:
- Around line 79-84: The makeTemplate function does not validate the HTTP
response status before parsing JSON, so failed POST requests to /templates
endpoint result in cryptic JSON parsing errors instead of revealing the actual
HTTP failure. Add a guard check after the post call to verify the response is
successful (check the response status code is 2xx range), and if not, throw an
error that includes the actual HTTP status and response details before
attempting to parse the JSON. This will ensure that real HTTP failures are
properly surfaced rather than being masked by JSON parsing errors.
In `@src/api/style-source.ts`:
- Around line 24-40: There is a race condition between the template existence
check at line 24 and the setSpecStyleSource call at line 29. If the template is
deleted between these operations, the FK constraint violation error will be
caught by the generic error handler and returned as a 500 error. To fix this,
wrap the setSpecStyleSource call in its own try-catch block to specifically
handle the foreign key violation error (PostgreSQL error code 23503 or similar
FK constraint failures). When such an error occurs, respond with a 404 status
and 'template not found' error message instead of allowing it to propagate to
the outer catch block that returns 500. This maps the template deletion race
condition to the correct HTTP status code.
In `@src/db/queries/style-source.integration.test.ts`:
- Around line 65-66: The template fixture names created with Date.now() suffix
in the makeTemplate function calls can collide across parallel test workers at
millisecond granularity, causing flaky tests. Replace all instances where
makeTemplate is called with a Date.now() suffix (appearing in the template name
patterns like ss-get-${Date.now()}, ss-update-${Date.now()},
ss-delete-${Date.now()}, etc.) with UUID generation instead. Import a UUID
generation utility and use it to create unique suffixes for each template
fixture name to ensure consistent uniqueness across all parallel test
executions.
In `@src/db/queries/templates.ts`:
- Around line 243-250: The deleteTemplate function has a race condition between
the countSpecsUsingTemplate check at line 243 and the DELETE query at line 245.
Another request could assign a spec to the template between these two
operations, causing the DELETE to fail with a foreign key constraint error
instead of returning the expected in_use response. Wrap both the count check and
the delete operation in a database transaction, and use row-level locking
(SELECT FOR UPDATE) on the style_templates table to prevent concurrent
modifications to specs that reference this template during the check-and-delete
sequence. This makes the entire operation atomic and ensures that if a spec is
assigned after checking, the delete will properly fail with the FK RESTRICT
error instead of racing.
In `@src/mcp/server.integration.test.ts`:
- Around line 227-253: The try/finally block in this test starts after the
UPDATE statement that sets style_template_id, which means if that UPDATE
operation fails, the finally cleanup block will not execute. Move the `try`
keyword to begin before the UPDATE specs statement that sets style_template_id
to ensure the finally block runs and properly cleans up the database state
(resetting style_template_id to NULL and deleting the template) regardless of
whether any operation fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b0ba7d1-c368-418c-a671-57f73741e648
📒 Files selected for processing (20)
README.mddocs/superpowers/specs/2026-06-16-issue-138-design.mdopenapi.yamlsrc/api/router.tssrc/api/specs.test.tssrc/api/specs.tssrc/api/style-source.integration.test.tssrc/api/style-source.tssrc/api/templates-crud.tssrc/ast/index.tssrc/ast/schemas.tssrc/db/index.tssrc/db/migrations/027_spec_style_source.tssrc/db/queries/style-source.integration.test.tssrc/db/queries/style-source.tssrc/db/queries/templates.tssrc/mcp/handlers.test.tssrc/mcp/handlers.tssrc/mcp/server.integration.test.tssrc/mcp/tools.ts
…n + doc/test hardening CodeRabbit findings on PR #206 (8 threads): - style-source assign: map the check-then-update race (template deleted between the existence pre-check and the UPDATE → 23503) to 404, not a leaked 500. - deleteTemplate: catch the 23503 FK violation from the DELETE as the authoritative in_use signal (RESTRICT race). A SELECT … FOR UPDATE on the template row would not close this — the concurrent assign UPDATEs `specs`, not the locked row. Reuses getPgCode. - openapi: operation-specific 409 on DELETE /templates/:id ("in use by N specs") instead of the generic Conflict $ref. - tests: guard makeTemplate against non-2xx; randomUUID (not Date.now) fixture names; start the MCP test try/ before the style_template_id UPDATE. - design doc: markdownlint (fence language, escaped #134); void → boolean return types to match the handler contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Why
.SEC/.txt/PDF masters carry no adopted visual style — there is nothing to capture (#125). The narrowest useful slice of #125's three style sources is the manual pick: associate an existing style template (#30/#31) with a spec so the generator (#32 / WT-6) can later render it with a real house style. The other two sources stay with #125 (manual per-property building is #31's CRUD; library inherit waits for ADR-015 lineage, Phase 2d).This is Wave 3 (O-12) of the Onboarding & Editability program and the first concrete slice of #125. Design:
docs/superpowers/specs/2026-06-16-issue-138-design.md(approved by the owner).What
specs.style_template_idFK withON DELETE RESTRICT(owner decision: a spec must never silently lose its formatting because a template was deleted), plus an index for the reference-count check.NULL= "no style source yet"; never auto-nulled.POST /specs/:id/style-source { templateId }assigns (replaces on re-assign);DELETE /specs/:id/style-sourceclears (idempotent, falls back to generator defaults). Existence is pre-checked explicitly — unknown spec or unknown template → 404 (never via the FK error).styleSource: { templateId, templateName } | nullsurfaced onGET /specs/:id(a separategetSpecStyleSourcequery merged in the handler —getSpecTreeuntouched, disjoint from the parallel feat(db): persist editability classification + user override per paragraph #134) and on MCPget_spec.deleteTemplatenow pre-checkscountSpecsUsingTemplateand returns a discriminated result;deleteTemplateHandlermaps the in-use case to 409"template in use by N spec(s)". The pre-check sidesteps the pg23503ambiguity (same code means 404 on assign-missing-template, 409 on delete-referenced).src/db/queries/style-source.ts(get/set/clear + count), re-exported from the barrel. OpenAPI + README updated.Out of scope (per issue + design §10): project-level style + inheritance / resolution chain (#125, Phase 2d lineage), generator consumption of the association (#32 / WT-6), per-paragraph overrides (WT-4), conformance flagging (#145).
Testing
pnpm test— 940 passed)pnpm test:integration— 437 passed, 110 pre-existing skips), including: assign → read back, unknown spec/template → 404, re-assign replaces, DELETE clears, DOCX-imported spec re-pointed,DELETE /templates/:id while referenced → 409 + template still exists, and MCPget_specsurfacesstyleSource.pnpm lintclean (eslint + tsc + prettier)🤖 Co-authored by Claude Opus 4.8. Closes #138.
Summary by CodeRabbit
New Features
POST /specs/:id/style-source) and clear (DELETE /specs/:id/style-source) a spec’s style template.GET /specs/:id) now includestyleSource({ templateId, templateName }ornull).DELETE /templates/:idreturns 409 when still referenced by a spec.Documentation
Tests