Skip to content

refactor(manifest): delegate generation to plugin-manifest-tool#44

Merged
gentlementlegen merged 3 commits into
mainfrom
fix/320-copy-package-lockfiles-to-artifact
Mar 10, 2026
Merged

refactor(manifest): delegate generation to plugin-manifest-tool#44
gentlementlegen merged 3 commits into
mainfrom
fix/320-copy-package-lockfiles-to-artifact

Conversation

@gentlementlegen
Copy link
Copy Markdown
Member

@gentlementlegen gentlementlegen commented Mar 10, 2026

Summary

  • replace duplicated manifest-generation implementation in action-deploy-plugin with a thin wrapper
  • .github/scripts/update-manifest.js now delegates to:
    • bunx @ubiquity-os/plugin-manifest-tool@latest
  • keep env contract unchanged (MANIFEST_PATH, SKIP_BOT_EVENTS, EXCLUDE_SUPPORTED_EVENTS, GITHUB_*)
  • remove behavior-level duplicated tests and keep wrapper-focused tests only

Validation

  • node --test .github/scripts/__tests__/*.test.js
  • published @ubiquity-os/plugin-manifest-tool@latest now resolves to 1.2.2

Related to ubiquity-os/ubiquity-os-kernel#320

@gentlementlegen gentlementlegen changed the title fix(manifest): accept dotless webhook listeners fix(manifest): remove webhook-name listener validation Mar 10, 2026
@gentlementlegen gentlementlegen changed the title fix(manifest): remove webhook-name listener validation refactor(manifest): delegate generation to plugin-manifest-tool Mar 10, 2026
@gentlementlegen gentlementlegen marked this pull request as ready for review March 10, 2026 07:49
@gentlementlegen gentlementlegen merged commit 4154630 into main Mar 10, 2026
2 checks passed
@gentlementlegen gentlementlegen deleted the fix/320-copy-package-lockfiles-to-artifact branch March 10, 2026 07:49
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

The script has been refactored to delegate manifest building to an external tool rather than performing the work in-process. The bulk of helper functions and manifest derivation logic (1901 lines) have been removed, replaced with three new exports: MANIFEST_TOOL_PACKAGE, getManifestToolArgs, and runManifestTool. The script now spawns the external manifest tool via bunx and returns its exit code. Tests have been narrowed to verify wrapper behavior including argument construction, process invocation, environment propagation, and exit code handling.

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is well-structured with Summary and Validation sections, but does not reference any issue using the required 'Resolves #' keyword. Add 'Resolves #320' or appropriate issue reference at the top following the template requirement.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: refactoring to delegate manifest generation to plugin-manifest-tool.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/320-copy-package-lockfiles-to-artifact

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
.github/scripts/update-manifest.js (1)

1-4: Consider pinning the manifest tool version.

@latest may introduce breaking changes without warning, affecting reproducibility. PR notes it currently resolves to 1.2.2—pinning that version would be safer.

📌 Suggested pinned version
-const MANIFEST_TOOL_PACKAGE = "@ubiquity-os/plugin-manifest-tool@latest";
+const MANIFEST_TOOL_PACKAGE = "@ubiquity-os/plugin-manifest-tool@1.2.2";

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 38ad1249-a42d-4188-a97a-0ce0d915ebb0

📥 Commits

Reviewing files that changed from the base of the PR and between f8fe978 and fc1f37d.

📒 Files selected for processing (2)
  • .github/scripts/__tests__/update-manifest.test.js
  • .github/scripts/update-manifest.js

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