Skip to content

fix(extensions): preserve binaries field in push archive manifest (swamp-club#309)#1354

Merged
stack72 merged 1 commit intomainfrom
worktree-309
May 10, 2026
Merged

fix(extensions): preserve binaries field in push archive manifest (swamp-club#309)#1354
stack72 merged 1 commit intomainfrom
worktree-309

Conversation

@stack72
Copy link
Copy Markdown
Contributor

@stack72 stack72 commented May 10, 2026

Summary

  • swamp extension push was dropping the binaries: field from the re-emitted archive manifest.yaml, causing pull-side consumers to miss the security warning and chmod 0o755 step
  • Added the missing binaries conditional spread to the stringifyYaml call in push.ts, matching the existing pattern for skills, include, platforms, and labels
  • Added a round-trip regression test asserting binaries survives push → extract in both the manifest YAML and the file tree

Fixes https://swamp-club.com/lab/issues/309

Test plan

  • deno check passes
  • deno lint passes
  • deno fmt passes
  • New round-trip test verifies binaries field in archive manifest and binary file presence under extension/files/
  • Verified against reproduction at /tmp/swamp-repro-issue-309 with compiled binary

🤖 Generated with Claude Code

…amp-club#309)

The archive re-emit in extensionPushPrepare omitted binaries from the
stringifyYaml call, so pulled extensions lost the security warning and
chmod 0o755 step even when the source manifest declared binaries.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — this PR fixes silent data loss in the archive manifest (the binaries field was dropped during swamp extension push) but introduces no changes to user-visible output, flags, error messages, or JSON shape. The behavioral fix is correct: pull-side consumers now correctly receive the binaries field, enabling the expected security warning and chmod 0o755 step. The regression test confirms the round-trip. No UX concerns.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Minor inconsistency in manifest path resolution: The new test hardcodes join(dst, "extension", "manifest.yaml") while the adjacent test (line 295) uses the findManifestRoot(dst) helper. Not blocking since the path is deterministic in the archive format, but using the helper would be more resilient to future archive layout changes.

Overall: Clean, minimal bug fix with a thorough regression test. The conditional spread for binaries correctly matches the established pattern for skills, include, platforms, and labels. LGTM.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Critical / High

None.

Medium

None.

Low

  1. Test cleanup on Windows (push_pull_roundtrip_test.ts:389-390): The finally block calls Deno.remove(src, { recursive: true }) and Deno.remove(dst, { recursive: true }) without the Windows EBUSY .catch(() => {}) pattern mentioned in CLAUDE.md. However, this is consistent with all other tests in this file (lines 320-321, 210-211) — none of them use the catch pattern either. Not a regression from this PR.

  2. Hardcoded archive root (push_pull_roundtrip_test.ts:382-387): The test uses join(dst, "extension", ...) rather than findManifestRoot(dst) used in the second test. If the archive root path ever changes, this test would break. Again, consistent with the first test in the file (line 192), so not a regression.

Verdict

PASS — Clean, minimal 3-line fix that adds the missing binaries conditional spread to the archive manifest serialization at line 1057. The pattern exactly mirrors the existing handling of skills, include, platforms, and labels in the same stringifyYaml block. ExtensionManifest.binaries is typed as string[] (normalized from optional via ?? [] in the parser at extension_manifest.ts:200), so .length access is safe. The binary files themselves were already being copied correctly (lines 1201-1222) — only the YAML field was missing. Test properly validates the round-trip.

@stack72 stack72 merged commit 82c8ed8 into main May 10, 2026
11 checks passed
@stack72 stack72 deleted the worktree-309 branch May 10, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant