refactor(modules): give module-serve failure responses one owner

[kojiwakayama]

Gives module-serve failure responses one owner, so the difference between a cacheable miss and an uncacheable rejection is stated once instead of re-spelled at ~29 call sites. Zero behaviour change.

The defect

createModuleResponse(...) appeared 29 times inline in module-server.ts. The literal pair "Module not found", HTTP_NOT_FOUND occurred with:

• Cache-Control: no-cache at one site — an ordinary miss, safe to revalidate
• Cache-Control: no-store at four others — protected-path and production-admission rejections, deliberately uncacheable

Same message, same status, different cacheability, and nothing in the code naming the difference. A future edit that "tidied up the duplication" could silently make an authorization decision cacheable.

The change

New src/modules/server/module-response.ts — the single owner of turning a module-serve failure into an HTTP Response, so the cacheability of a miss versus a rejection is decided in one place.

Six named helpers, no options bag, no exported generic builder. The two that carry the meaning:

• moduleNotFound(method) — ordinary miss, no-cache
• moduleRejected(method) — admission / protected-path rejection, no-store

moduleRejected's docstring states the rule outright: "Do not change this to no-cache to 'match' the not-found case; that is a security regression, not a cleanup."

Converted: 20 direct call sites (6 no-cache, 14 no-store) plus 7 that went through a local unknownDependencySnapshotModuleResponse wrapper, now deleted. module-server.ts is net −67 lines (−103/+36).

Deliberately left inline: the success responses (HTTP_OK + application/javascript), the two Transform Error JS-body blocks, the cached-release passthrough, and the dynamic catch-all whose status and body vary by error kind. Folding those in would have produced exactly the general-purpose builder this change exists to avoid.

Evidence

• deno task test:unit: 3801 passed / 27907 steps → 3807 / 27918, exactly +6 tests / +11 steps. module-server.test.ts (the 88-step fence, which asserts exact Cache-Control values) unchanged.
• deno task verify:quick exit 0; deno check src/modules/index.ts clean.
• Cache directives audited mechanically, not by eye. Every createModuleResponse call in the baseline file was parsed with a paren-balancing script capturing status + Cache-Control + Content-Type + Allow + body, then aligned against the new helper sites. 20 sites, perfect 1:1, zero directive drift — no no-store→no-cache flip and no no-cache→no-store flip. The pairing was confirmed context-anchored (each conversion sits against its original adjacent logger.warn), not merely ordinal.
• Independent arithmetic check: 0 no-store remain inline in module-server.ts (all 15 converted); 5 no-cache remain, being the deliberately-excluded success responses. 6 converted + 5 remaining = the original 11.
• The deleted wrapper had exactly 7 call sites; the new file has exactly 7 unknownDependencySnapshot(method) calls, all 409 + no-store. Zero stale references repo-wide.

Tests fence the property, not just the code

module-response.test.ts pins exact status + Content-Type + Cache-Control per helper. The load-bearing case asserts that moduleNotFound and moduleRejected share a status but differ in Cache-Control — so collapsing the two helpers, or copy-pasting one into the other, fails the suite.

One question this refactor answered

The findSourceFile miss site logs "Module not found" but uses no-store, unlike every other pure-miss site. Naming the distinction surfaced it immediately. Git provenance settles it: commit e6083e701 (#3290, "fix(security): bind hosted source and environment identity") added that no-store where previously there was no Cache-Control at all.

It is deliberate. That site sits after the protected-path and production-admission gates and computes its answer by probing the tenant's projectDir through secureFs — so a cacheable answer would make project-layout probing results storable. moduleRejected is substantively correct there. A comment at the site now records this, because the adjacent "Module not found" log is otherwise an invitation to "correct" it into a security regression.

Known follow-ups (not blocking)

• respond()'s status→metric mapping (not_found vs error) is failure-only by design; a comment now says a success shape must not be added without revisiting it.
• Header key order on the 405 changed (Allow, Content-Type, Cache-Control). Distinct keys into a Headers map — no observable difference.

Summary by CodeRabbit

• Bug Fixes
  • Standardized module error responses with consistent status codes, headers, cache behavior, and metrics.
  • Improved handling for missing modules, rejected requests, invalid requests, unsupported methods, unavailable services, and unknown dependencies.
  • Preserved correct HEAD response behavior and non-caching for unavailable or rejected requests.
• Tests
  • Added comprehensive coverage for response statuses, headers, bodies, custom messages, caching, and HEAD requests.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Warning

Review limit reached

@kwakayama, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d6122cd0-4058-4e91-8527-33395b2daf68

📥 Commits

Reviewing files that changed from the base of the PR and between 42ca477 and d20c566.

📒 Files selected for processing (1)

• src/modules/server/module-response.ts

📝 Walkthrough

Walkthrough

Module failure-response construction moved into shared helpers. module-server.ts now uses these helpers across request validation, path checks, dependency resolution, production admission, and transform handling. Tests verify statuses, headers, bodies, cache policies, and HEAD behavior.

Changes

Module response centralization

Layer / File(s)	Summary
Response helpers and validation .superpowers/sdd/module-response-review.diff, .superpowers/sdd/module-response-report.md, src/modules/server/module-response.ts, src/modules/server/module-response.test.ts	Six helpers standardize module failure responses, metrics, headers, cache policies, custom messages, and HEAD handling. Tests cover all helper categories.
Initial module-server integration src/modules/server/module-server.ts	Method, path, snippet, browser, and cross-project failure branches use the shared helpers.
Admission and exception-path integration src/modules/server/module-server.ts	Project admission, source lookup, production, dependency, transform, and exception branches use standardized responses. The obsolete local helper is removed.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: kwakayama

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: centralizing module-serving failure responses in one module.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/module-serve-response-mapping

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/modules/server/module-response.ts (1)

9-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the required documentation style.

Replace em dashes with ASCII punctuation. Replace uppercase MUST with must.

• src/modules/server/module-response.ts#L9-L10: Replace the em dash with a period or comma.
• src/modules/server/module-response.ts#L56-L60: Use lowercase must for the requirement.
• .superpowers/sdd/module-response-report.md#L1-L1: Replace the em dash in the heading.
• .superpowers/sdd/module-response-report.md#L13-L14: Replace the em dashes in the table descriptions.

As per coding guidelines, use must for requirements and do not use em dash or en dash characters.

🤖 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/modules/server/module-response.ts` around lines 9 - 10, Apply the
required documentation style across all listed sites: in
src/modules/server/module-response.ts lines 9-10, replace the em dash with ASCII
punctuation; in lines 56-60, change uppercase MUST to lowercase must; in
.superpowers/sdd/module-response-report.md lines 1 and 13-14, replace each em
dash with ASCII punctuation. Ensure no em dash or en dash characters remain in
these documentation passages.

Source: Coding guidelines

🤖 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 @.superpowers/sdd/module-response-report.md:
- Around line 55-58: Update the direct call-site total in the response report
from 21 to 20, while leaving the separately reported seven indirect wrapper call
sites and the cache-mode breakdown unchanged.

---

Nitpick comments:
In `@src/modules/server/module-response.ts`:
- Around line 9-10: Apply the required documentation style across all listed
sites: in src/modules/server/module-response.ts lines 9-10, replace the em dash
with ASCII punctuation; in lines 56-60, change uppercase MUST to lowercase must;
in .superpowers/sdd/module-response-report.md lines 1 and 13-14, replace each em
dash with ASCII punctuation. Ensure no em dash or en dash characters remain in
these documentation passages.

🪄 Autofix

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: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e2e01b9-c69a-4118-a011-9f04dc5b2272

📥 Commits

Reviewing files that changed from the base of the PR and between 36c073f and 42ca477.

📒 Files selected for processing (5)

• .superpowers/sdd/module-response-report.md
• .superpowers/sdd/module-response-review.diff
• src/modules/server/module-response.test.ts
• src/modules/server/module-response.ts
• src/modules/server/module-server.ts

[kwakayama]

Documentation-style nitpick addressed in d20c566.

src/modules/server/module-response.ts now has no em dash and no uppercase MUST. The three flagged passages read as direct ASCII sentences, and the moduleRejected requirement is This must stay no-store.

The .superpowers/sdd/module-response-report.md half of that nitpick no longer applies. That file was an internal SDD working artifact and was removed from the branch in bee0edf.