fix(extensions): reject quality and fmt on pulled extensions (swamp-club#239)#1328
fix(extensions): reject quality and fmt on pulled extensions (swamp-club#239)#1328
Conversation
…lub#239) Pulled extension manifests use paths.base: typedDir (the default), so quality/fmt resolve model files against extensions/models/ in the repo root instead of the pulled extension directory. Rather than fixing path resolution for read-only copies, block both commands early with a clear error when the manifest is under .swamp/pulled-extensions/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
- Neither
UserErrorthrow includes a machine-readablecodeargument (second param to the constructor). Adding something like"pulled-extension"would let--jsonconsumers detect this case without fragile message-string matching — e.g.swamp extension quality --json | jq '.code'. Not a blocker since there's no established per-error-code pattern in this area of the CLI.
Verdict
PASS — Early-rejection errors are clear, actionable, and symmetric between the two commands. "Cannot run fmt/quality on a pulled extension" tells users exactly what went wrong; the follow-up sentence points them to the correct path (extensions/). Log mode shows a clean one-liner (no stack), JSON mode emits { "error": "..." } via the standard renderError path. Consistent with UserError conventions throughout the codebase.
There was a problem hiding this comment.
Code Review
Clean, well-scoped fix. The early guard catches the invalid operation before any expensive repo/manifest resolution work, and the error messages are clear and actionable.
Blocking Issues
None.
Suggestions
-
SEPARATORinstead of hardcoded separators (src/cli/resolve_extension_files.ts:110-111): ThestartsWith(pulledRoot + "/") || startsWith(pulledRoot + "\\")pattern works correctly, but sinceresolve()normalizes to the platform separator, a singlestartsWith(pulledRoot + SEPARATOR)(from@std/path) would be sufficient and more consistent with the CLAUDE.md guidance to use@std/pathfor path operations. The current approach is defensive and harmless, so not blocking. -
Test naming convention: The test names use a colon after the function name (e.g.,
"isPulledExtensionManifest: returns true for...") which matches the project convention in CLAUDE.md — good.
Overall: focused fix, good test coverage (5 tests covering absolute/relative paths, positive/negative cases, and the prefix-attack edge case), no import boundary violations, no security concerns. The + separator suffix correctly prevents prefix-matching attacks like .swamp/pulled-extensions-evil/.
Summary
extension qualityandextension fmtfail on pulled extensions because the manifest'spaths.basedefaults totypedDir, resolving model files againstextensions/models/in the repo root instead of the pulled extension's directoryisPulledExtensionManifest()helper that detects manifests under.swamp/pulled-extensions/and rejects both commands early with a clear error messageTest Plan
isPulledExtensionManifest(absolute/relative/local/non-.swamp paths)extension qualityon pulled manifest → "Model file not found"deno check,deno lint,deno fmtall cleanCloses swamp-club#239
🤖 Generated with Claude Code