fix(extensions): preserve additionalFiles directory structure and add ctx.extensionFile() helper (swamp-club#146)

[stack72]

Summary

Closes swamp-club#146.

additionalFiles was flattened to basenames on push (src/libswamp/extensions/push.ts:1115-1118), dropping any declared subdirectory structure and silently clobbering entries that shared a basename. Pull's recursive copyDir preserves whatever's in the archive, so the asymmetry broke at push — contradicting design/extension.md:290 ("preserving their relative paths") and leaving authors to work around the bug by inlining assets as TS constants.

The fix restores directory preservation at push, rejects duplicate-basename entries at push-time with a clear error, and introduces ctx.extensionFile(relPath) so model and report code resolves bundled assets portably across source-loaded and pulled-mode extensions.

What changed

Bug fixes

• push.ts — preserves manifest.additionalFiles relative paths under files/ via parallel-array zip with Deno.mkdir for intermediate directories; asserts array-length invariant.
• resolve_extension_files.ts — rejects duplicate entries (NFC-normalized, case-folded for case-insensitive filesystem safety) and symlinks with typed UserErrors naming both offenders / explaining the rationale.

Runtime helper

• New src/domain/extensions/extension_file_resolver.ts — shared resolver with mode-aware errors (pulled suggests re-publish; source points at absolute path and manifest).
• MethodContext and MethodReportContext both gain extensionFile?: (relPath: string) => string.
• ModelDefinition.extensionFilesRoot populated at load time by UserModelLoader — pulled extensions via path-slice (zero filesystem work); source extensions via walk-up to manifest.yaml, deduped by a per-loader Map<manifestDir, filesRoot>.
• Wired through all three buildMethodContext sites and all four buildMethodReportContext sites.

Tests (15 new)

• resolve_extension_files_test.ts — preservation, duplicates, case-fold, NFC/NFD, symlinks, 0-byte, missing file.
• extension_file_resolver_test.ts — undefined root, unsafe paths, mode-aware errors, valid resolution.
• push_pull_roundtrip_test.ts — end-to-end archive build → untar → resolver verification, hermetic (no registry calls).

Docs

• design/extension.md — Additional Files section expanded with helper and symlink rationale.
• .claude/skills/swamp-extension-publish — directory-preservation note + helper section.
• .claude/skills/swamp-extension-model — ctx.extensionFile in the context field list + reading-assets subsection showing recommended usage vs the hardcoded-path antipattern.

Backwards compatibility

• Flat additionalFiles entries produce byte-identical archives — basename(\"README.md\") === \"README.md\". Common case is unaffected.
• Already-published archives on the registry pull unchanged (recursive copyDir is layout-agnostic).
• Re-publishing with subdirectory entries is the only author-visible change; helper's pulled-mode error explicitly suggests this.

Out of scope

• Docker/remote source-mode mounting (ADV-3 deferred — extensionFilesRoot in source mode can resolve outside repoDir; pulled paths remain container-accessible).
• Windows verification — native join() in push (tar normalizes archive paths on extract); CI is Linux-only.
• Direct-content extension source layouts legitimately have no manifest and therefore no additionalFiles. Verified via src/libswamp/sources/add.ts — resolver correctly returns undefined for these.

Follow-ups

• swamp-uat gap filed: systeminit/swamp-uat#157 (directory preservation, collision rejection, symlink rejection, missing file).
• swamp-club content/manual/reference/extension-manifest.md has a stale "Extracted to the models directory on pull" line — will open a draft PR in swamp-club referencing this PR's number.

Test Plan

• deno check — clean
• deno lint — clean (1066 files)
• deno run test — 4597 passed, 0 failed
• deno run compile — binary rebuilt
• End-to-end reproduction against /tmp/swamp-repro-issue-146: model calling ctx.extensionFile() resolves all 4 fixture files (flat + deeply nested)
• Live collision rejection with ./prompts/review.md + prompts/review.md (normalized dup) and prompts/review.md + prompts/REVIEW.md (case-fold dup)
• Architecture guard tests for both context types still pass
• Pulled-mode end-to-end against a real registry — not run locally; verified by unit tests + integration test that untars a real archive. CI exercises the pull pipeline.
• Docker driver — not exercised; audit deferred per plan ADV-3.

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

1. extensionFile typed as optional but documented and always-set as non-optional — src/domain/models/model.ts:357 and src/domain/reports/report_context.ts:59 both declare extensionFile?: (relPath: string) => string (optional). But buildMethodContext and buildMethodReportContext always populate it, and the SKILL.md examples call it without optional chaining:

   const promptPath = ctx.extensionFile("prompts/review.md");

   In TypeScript strict mode (enforced by this project), calling a ?: (relPath: string) => string field directly is a type error — "Cannot invoke an object which is possibly 'undefined'." Extension authors following the SKILL.md pattern will get a confusing TS error and have to reach for ctx.extensionFile!("...") or ctx.extensionFile?.("..."). Since the function is always present and throws a clear UserError when called on a non-extension model, the ?: buys nothing. Change both declarations to non-optional (extensionFile: (relPath: string) => string) so the documented pattern typechecks cleanly.

Suggestions

1. Inconsistent error message capitalization — New errors in src/domain/extensions/extension_file_resolver.ts start lowercase ("extension file not found", "ctx.extensionFile() is only available") while every existing error in src/cli/resolve_extension_files.ts starts with a capital ("Manifest file not found", "Model file not found", "Vault file not found", etc.). Capitalize the new messages to match.

2. Internal issue number in a user-facing error — The pulled-mode missing-file error in extension_file_resolver.ts:569 reads "(swamp issue-146)". External users filing bug reports or reading docs won't know where to locate that issue. Drop the number or replace with "re-publish the extension and re-pull" alone — the actionable instruction is already there.

3. Duplicate-entry advice slightly misleading for path-normalization collisions — src/cli/resolve_extension_files.ts:444 says "Remove one or rename the file so basenames differ". For pure path-normalization duplicates ("./prompts/review.md" vs "prompts/review.md"), the basenames are already identical and the real fix is simply to remove the duplicate entry. Suggest "Remove one entry from the manifest, or rename the file" to cover both cases.

Verdict

NEEDS CHANGES — the extensionFile?: ... optional type will cause TypeScript errors for extension authors following the SKILL.md examples; the field should be non-optional since it is always populated.

[github-actions]

CLI UX Review

Blocking

1. extensionFile typed as optional but documented and always-set as non-optional — src/domain/models/model.ts:357 and src/domain/reports/report_context.ts:59 both declare extensionFile?: (relPath: string) => string (optional). But buildMethodContext and buildMethodReportContext always populate it, and the SKILL.md examples call it without optional chaining:

   const promptPath = ctx.extensionFile("prompts/review.md");

   In TypeScript strict mode (enforced by this project), calling a ?: (relPath: string) => string field directly is a type error — "Cannot invoke an object which is possibly 'undefined'." Extension authors following the SKILL.md pattern will get a confusing TS error and have to reach for ctx.extensionFile!("...") or ctx.extensionFile?.("..."). Since the function is always present and throws a clear UserError when called on a non-extension model, the ?: buys nothing. Change both declarations to non-optional (extensionFile: (relPath: string) => string) so the documented pattern typechecks cleanly.

Suggestions

1. Inconsistent error message capitalization — New errors in src/domain/extensions/extension_file_resolver.ts start lowercase ("extension file not found", "ctx.extensionFile() is only available") while every existing error in src/cli/resolve_extension_files.ts starts with a capital ("Manifest file not found", "Model file not found", "Vault file not found", etc.). Capitalize the new messages to match.

2. Internal issue number in a user-facing error — The pulled-mode missing-file error in extension_file_resolver.ts reads "(swamp issue-146)". External users filing bug reports or reading docs won't know where to locate that issue. Drop the number or replace with "re-publish the extension and re-pull" alone — the actionable instruction is already there.

3. Duplicate-entry advice slightly misleading for path-normalization collisions — The new duplicate error in src/cli/resolve_extension_files.ts says "Remove one or rename the file so basenames differ". For pure path-normalization duplicates ("./prompts/review.md" vs "prompts/review.md"), the basenames are already identical and the real fix is simply to remove the duplicate entry. Suggest "Remove one entry from the manifest, or rename the file" to cover both cases.

Verdict

NEEDS CHANGES — the extensionFile?: ... optional type will cause TypeScript errors for extension authors following the SKILL.md examples; the field should be non-optional since it is always populated.

[github-actions]

Code Review

Well-structured fix for the additionalFiles directory flattening bug. The three-layer approach (push preservation, duplicate/symlink rejection at validation time, runtime resolver abstraction) is clean, and the 15 new tests provide solid coverage of the happy path, edge cases, and error modes.

Blocking Issues

None.

Suggestions

1. Hardcoded PULLED_MARKER in extension_file_resolver.ts — The resolver hardcodes "/.swamp/pulled-extensions/" while user_model_loader.ts correctly imports and uses SWAMP_DATA_DIR from src/infrastructure/persistence/paths.ts. They resolve to the same string today, but using the shared constant would keep them coupled. Minor DRY nit, not blocking since the value is stable.

2. Negative-result caching in resolveExtensionFilesRoot — The walk-up cache only stores positive hits (found manifest). For models not shipped as extensions (built-in types, direct-content layouts), the walk repeats to the filesystem root on every load. Model loading isn't a hot path, so this is fine in practice, but caching undefined for the initial dirname(sourcePath) after a full walk would eliminate the repeated I/O for those cases.

DDD Assessment

• extension_file_resolver.ts is correctly placed as a stateless domain service in src/domain/extensions/.
• extensionFilesRoot on ModelDefinition is an appropriate runtime property — it describes where the model's deployment context resolves assets, populated at load time without schema migration.
• The resolveExtensionFile function encapsulates mode-aware (source vs pulled) error semantics as domain logic, keeping MethodContext and MethodReportContext from knowing about deployment layout.
• Import boundaries are clean: domain layer has no libswamp imports; the libswamp roundtrip test imports from domain (established pattern); CLI/presentation code doesn't touch internal paths.

Other Observations

• Path traversal prevention via isSafeRelativePath is correct — rejects .. and absolute paths.
• Symlink detection uses lstat (not stat), so symlinks are caught before dereferencing. Good.
• The parallel-array invariant assertion in push.ts is a solid defensive check.
• All new .ts files include the AGPLv3 copyright header.
• The synchronous Deno.lstatSync in the resolver is an intentional design choice (keeps extensionFile() sync for caller ergonomics) and appropriate for a single-syscall stat.

[github-actions]

Adversarial Review

I traced every code path in the 16 changed files, probed for path traversal, race conditions, data corruption, and resource leaks. The fix is well-targeted and thoroughly tested.

Critical / High

None.

Medium

1. src/domain/extensions/extension_file_resolver.ts:60 — Windows pulled-mode detection inconsistency.
   resolveExtensionFile checks root.includes("/.swamp/pulled-extensions/") without normalizing backslashes. Meanwhile user_model_loader.ts:1514 does currentDir.replace(/\\/g, "/") before the same marker check. On a Windows host, the resolver would misclassify a pulled extension as source-loaded, producing the wrong error message (pointing at a disk path + manifest instead of suggesting re-publish). Functionality (file resolution) is unaffected — only the error message diverges. PR description explicitly defers Windows; noting for when that work lands.

2. src/domain/models/model.ts:357 / src/domain/reports/report_context.ts:59 — extensionFile typed as optional but always populated.
   buildMethodContext (line 129) and buildMethodReportContext (line 212) unconditionally assign the closure, so ctx.extensionFile is never undefined at runtime. Extension authors who write ctx.extensionFile?.("path") expecting undefined when no manifest exists will get a thrown UserError instead. The docs say "throws if the model is not shipped as an extension", so behavior is documented — but the ?: type annotation suggests the safer contract is "absent when unavailable". Not blocking since the thrown error is descriptive and the docs are clear.

Low

1. src/domain/extensions/extension_file_resolver.ts:56 — resolveExtensionFile("", root) resolves to the root directory itself.
   isSafeRelativePath("") returns true (no .., no leading /), and join(root, "") returns root. lstatSync would succeed on the directory, returning a directory path. A subsequent Deno.readTextFile(path) would fail with an OS error rather than a clear UserError. Unlikely in practice — extension authors would never pass an empty string — but a one-line guard (if (!relPath)) would turn this into a descriptive error.

2. src/domain/extensions/extension_manifest.ts:33 — isSafeRelativePath doesn't reject Windows drive-letter prefixes (C:\foo).
   On Linux this is harmless (becomes a literal path segment). On Windows, join(root, "C:\\foo") could resolve outside root. Explicitly out of scope per the PR, noting for completeness.

3. src/domain/extensions/extension_file_resolver.ts:58 — Runtime resolver doesn't reject symlinks.
   Push-time validation (resolve_extension_files.ts:456) rejects symlinks in additionalFiles, so archives won't contain them. However, if a symlink is manually created inside a pulled extension's files/ directory after extraction, resolveExtensionFile would follow it. Theoretical — pulled directories are swamp-managed.

Verdict

PASS. The core fix (directory-preserving push via parallel-array zip, duplicate/symlink rejection, ctx.extensionFile() resolver) is correct, well-tested (15 new tests including a round-trip integration test), and properly wired through all buildMethodContext (3 sites) and buildMethodReportContext (4 sites) call paths. The parallel-array length invariant in push.ts is a good defensive check. No security vulnerabilities, no data-loss paths, no resource leaks.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Internal invariant message (push.ts): The error "additionalFiles and additionalFilePaths length mismatch — this is a bug in extension file resolution." is thrown via validationFailed rather than UserError. If a user ever sees it (they shouldn't under normal conditions), the phrase "this is a bug" could leave them wondering whether to file an issue or fix their manifest. Consider appending "Please report this at https://github.com/systeminit/swamp/issues." or similar. Not blocking — the invariant is a programmer guard that should be unreachable in practice.

2. Source-mode missing-file message (extension_file_resolver.ts, line ~629): Uses Extension file not found: ${absPath} (absolute path inline, no quotes). The duplicate-entries error wraps both offenders in double-quotes; consistency would give users the same scanning pattern here. Minor.

Verdict

PASS — No blocking issues. All new user-facing error messages (Duplicate additionalFiles, symlink rejection, ctx.extensionFile() failures) are clear, actionable, and correctly use UserError (no stack traces exposed). No new CLI flags or commands, so there's nothing to document in help text. The behavioral change (directory preservation on push) is transparent to command users and fixes the stated asymmetry without altering any output format or exit codes.

[github-actions]

Code Review

Clean, well-structured bug fix with strong test coverage and good DDD layering. The three-layer approach (push preservation, duplicate/symlink rejection, runtime resolver) is well thought out, and the mode-aware error messages are a nice UX touch.

Blocking Issues

None.

Suggestions

1. Negative cache miss in resolveExtensionFilesRoot (src/domain/models/user_model_loader.ts:1499-1529): The extensionFilesRootCache only caches positive hits (manifest found). For built-in models or non-extension source dirs, the walk-up to filesystem root repeats on every model load. Not a correctness issue — model loading isn't a hot path — but if many non-extension models are loaded, consider caching a sentinel value (e.g., empty string) for the negative case and checking cache.has(key) instead of cache.get(key) !== undefined.

2. Synchronous fs in resolveExtensionFile (src/domain/extensions/extension_file_resolver.ts:62-63): Deno.lstatSync is used in the runtime resolver called during method execution. This is consistent with resolveExtensionFilesRoot (also sync) and the cost is one stat, so it's fine for now. If extension files ever resolve to network mounts, this could block the event loop — something to keep in mind for the Docker/remote source-mode follow-up (ADV-3).

3. withTempRoot in extension_file_resolver_test.ts: Two of the tests (unsafe relPath with '..' and absolute relPath) create a temp directory via withTempRoot but never actually use the filesystem — the assertions throw before any path resolution happens. The await Promise.resolve() at the end is there to satisfy the async signature. Minor, but these could be plain synchronous tests like the undefined root test above them.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. src/domain/extensions/extension_file_resolver.ts:63 — resolveExtensionFile does not verify the resolved path is a regular file. Deno.lstatSync(absPath) succeeds for both files and directories. If a model author calls ctx.extensionFile("prompts/") or ctx.extensionFile("prompts") (pointing at a directory), the function returns the path without complaint. The caller then gets an OS-level error ("Is a directory") from Deno.readTextFile instead of a typed UserError. This isn't a security issue — no escape occurs — but the error UX breaks the otherwise clean mode-aware messaging the PR introduces.

   • Breaking example: ctx.extensionFile("prompts") where prompts/ is a directory in additionalFiles.
   • Suggested fix: After the lstatSync, check info.isFile and throw a UserError if false: "Extension path is a directory, not a file: ...".

2. src/domain/models/user_model_loader.ts:1502-1528 — resolveExtensionFilesRoot walk-up can match an unrelated manifest.yaml. The method walks from the model's source directory to the filesystem root. If an ancestor directory (e.g., the user's home directory or project root) contains an unrelated manifest.yaml, it would be found and treated as the extension manifest. This sets an incorrect extensionFilesRoot, causing ctx.extensionFile() to resolve against the wrong directory. The blast radius is small — the file simply wouldn't exist and the user gets a missing-file error — but the error message would be misleading.

   • Breaking example: User has ~/manifest.yaml from an unrelated project; a source-loaded model at ~/projects/myext/extensions/models/m.ts has no extension manifest of its own. The walk-up reaches ~/manifest.yaml and incorrectly sets extensionFilesRoot to ~.
   • Suggested fix: Bound the walk-up at repoDir (already available on the UserModelLoader instance) rather than walking to filesystem root. This is a natural boundary since extensions live within a repo.

Low

1. src/domain/extensions/extension_file_resolver.ts:29 vs src/domain/models/user_model_loader.ts:1515 — duplicated pulled-extension marker. extension_file_resolver.ts hardcodes PULLED_MARKER = "/.swamp/pulled-extensions/" while user_model_loader.ts constructs the same string from the imported SWAMP_DATA_DIR constant. The code comment explains the DDD layer constraint, and the value is stable and tested. But if SWAMP_DATA_DIR ever changes, mode detection in resolveExtensionFile would silently diverge from the loader. Extremely low probability given the test coverage on SWAMP_DATA_DIR.

2. src/domain/models/user_model_loader.ts:1504-1506 — extensionFilesRootCache only caches positive hits. When the walk-up finds no manifest (returns undefined), intermediate directories are not cached. A project with many non-extension models would repeat the full walk-up for each one. This is purely a performance concern for an uncommon layout and has no correctness impact.

3. src/cli/resolve_extension_files.ts:432 — duplicate step number comment. Both step 15 (include files) and the new additionalFiles block are labeled // 15.. Cosmetic only.

Verdict

PASS — The core fix (preserving additionalFiles directory structure through push, rejecting duplicates/symlinks, adding the ctx.extensionFile() helper) is correct and well-tested. Path traversal is defended at two layers (Zod schema validation + isSafeRelativePath check in the resolver). The parallel-array invariant assertion in push.ts is good defensive practice. The 15 new tests cover the key edge cases (NFC/NFD collisions, case-folding, symlinks, missing files, round-trip preservation). The medium findings are UX polish opportunities, not blockers.