Preserve unrelated skills in plugin-companion mode

[ihabadham]

Why

Plugin-companion delivery should coexist with unrelated personal skills, but the installer treated every unmanaged flat skill as a conflict. That made companion mode unusable for people who kept their own skills in the same agent home.

At the same time, relaxing that check must not permit the same skill to be delivered by both the active native plugin and the flat skill directory. Native plugin discovery also needs to fail closed when its provenance or active root is ambiguous rather than guessing which installation is authoritative.

Closes #191.

What changed

• Preserve unrelated personal flat skills during fresh and repeated plugin-companion installs.
• Block flat skills whose names are advertised by the verified active native plugin, including when source and plugin revisions have drifted in either direction.
• Resolve native plugin roots and Codex plugin provenance conservatively, failing closed on ambiguous or inconsistent state.
• Extend installer, delivery-state, and status regression coverage.
• Document the delivery invariant and add an Unreleased changelog entry.

Validation

• ruby bin/agent-workflows-delivery-state-test.rb — 35 runs, 184 assertions, 0 failures/errors.
• bash bin/install-agent-workflows-test.bash — all cases passed, including the source-removed/native-still-active collision regression.
• bin/validate — passed after the final behavior change, including 93-file RuboCop with no offenses.
• The Linux environment does not provide shasum; validation used an isolated /tmp shasum symlink to /usr/bin/sha1sum without modifying the repository.
• Final documentation/changelog-only commits passed git diff --check and targeted structure/content checks.

Review evidence

• Isolated Bloodhound review found two actionable issues: a source-removal collision gap and stale design prose. Both were fixed and regression-covered; the final review wave reported no meaningful issues.
• Hosted review found one additional scalar-JSON manifest crash. It was fixed with public status regressions for string, number, and boolean roots, followed by focused tests, full validation, and another clean Bloodhound pass.
• Claude Code /code-review ultra origin/main...HEAD produced no output and was stopped after approximately ten minutes.
• Claude Code /simplify origin/main produced no output and was stopped after approximately five minutes; no simplify edits were made.

Churn notes

• Local review waves: 4 follow-up waves after the initial implementation review.
• Accepted findings: 3.
• Declined optional suggestions: 2 (helper extraction and expanded diagnostic taxonomy); deferred or waived blockers: 0.
• Scope remained limited to companion delivery-state detection, installer/status coverage, design documentation, and changelog evidence.

[coderabbitai]

Walkthrough

plugin-companion delivery now preserves unrelated flat skills, blocks collisions with skills advertised by verified native plugins, and fails closed for ambiguous native roots or provenance. Codex and Claude state detection derive skill names from validated manifests, with expanded regression coverage.

Changes

Plugin-companion delivery

Layer / File(s)	Summary
Native plugin manifest and provenance validation bin/agent-workflows-delivery-state, bin/agent-workflows-delivery-state-test.rb, bin/agent-workflows-status-test.rb, bin/install-agent-workflows-test.bash	Codex and Claude native state validate plugin manifests, sources, versions, cache roots, and manifest-derived skill names.
Collision-aware flat inventory bin/agent-workflows-delivery-state, docs/plans/...plugin-companion-unrelated-skills-design.md	Flat inventory logic distinguishes legacy migration ownership from companion delivery and uses verified native skill names for collision checks.
Companion preservation and fail-closed regressions bin/agent-workflows-delivery-state-test.rb, bin/install-agent-workflows-test.bash, CHANGELOG.md, docs/plans/...plugin-companion-unrelated-skills-design.md	Tests cover unrelated-skill preservation, revision drift, malformed native roots, repeat-mode replay, and unchanged state on failure.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• shakacode/agent-workflows#136: Introduced the delivery-mode and delivery-state framework extended by this change.

Suggested reviewers: justin808

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address #191 by preserving unrelated skills, allowing fresh/repeat installs, and retaining fail-closed collision checks.
Out of Scope Changes check	✅ Passed	The added docs, tests, and changelog updates support the plugin-companion behavior change and do not appear unrelated.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: plugin-companion now preserves unrelated skills while handling companion collisions.

✨ 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 ihabadham/fix/plugin-companion-unrelated-skills

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Review Summary

What this PR does: Relaxes plugin-companion delivery so unrelated personal flat skills in <target>/skills survive fresh and repeat installs, while still blocking any flat skill whose name collides with one advertised by the verified active native plugin (covering both directions of source/native revision drift). It also hardens native-plugin root/provenance resolution (Codex version+source, Claude multi-receipt) to fail closed on ambiguity instead of guessing.

Overall impression: This is a well-scoped, carefully test-driven change. The core logic in managed_flat_inventory/check correctly separates the "legacy flat migration" path (unchanged, still fails closed on any unrecognized child) from the new "fresh/companion" path (only blocks names actually advertised by the verified active native plugin, ignores everything else). The new regression coverage (mixed valid/invalid candidate roots, native-skill-added-after-recorded-revision, native-skill-removed-from-current-source) exercises the right edge cases, and the design doc clearly documents the invariant.

Code quality / correctness

• codex_native_state, claude_native_state, and plugin_manifest_skill_names consistently fail closed (unknown/blocking) on symlinked roots, malformed manifests, version/source mismatches, and multi-candidate ambiguity — good defensive posture for a script that gates destructive migration behavior.
• Left an inline question on the biggest risk I could find: codex_plugin_cli_state treats the CLI's PATH column as a source/provenance URL and compares it against plugin.json's repository field. If real codex plugin list output puts a filesystem path there instead of a URL, this would make Codex plugin-companion detection permanently "unknown" for real users — worth an explicit confirmation this was checked against the real CLI, since all current test coverage is self-authored fixtures.
• Two minor nits left inline: duplicated skill-name-globbing logic between plugin_manifest_skill_names and the new current_skill_names, and a generic "no valid installed cache" error message now covering several distinct failure causes (including the new source mismatch), which could make troubleshooting harder.

Security

No injection or traversal issues found — external process calls use array-form Open3/system args (no shell interpolation), and skill names are derived from single-level Dir.glob matches so they can't escape target/skills. Symlink handling throughout (skills_path, cache roots, plugin roots) is conservative and fails closed.

Test coverage

Strong — both the Ruby unit tests (agent-workflows-delivery-state-test.rb) and the bash integration tests (install-agent-workflows-test.bash) cover the fresh-install, repeat-install, revision-drift, and mixed-candidate-root scenarios described in the PR body, and assert no mutation occurs on the blocking paths.

Performance

No concerns — all additions are bounded filesystem/JSON operations proportional to the number of installed skills/candidates.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

bin/agent-workflows-delivery-state (1)

201-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Array(receipts) silently mangles a single-object receipt.

Array({"installPath" => "..."}) yields [["installPath", "..."]], so a receipt recorded as an object instead of a list is reported as "invalid candidate install receipt" rather than being either honored or diagnosed accurately. It still fails closed, but an explicit shape check keeps the reason truthful and avoids the Kernel#Array Hash-coercion gotcha.

♻️ Proposed refactor

-    candidates = Array(receipts)
+    unless receipts.nil? || receipts.is_a?(Array)
+      return { "state" => "unknown", "reason" => "invalid Claude scw plugin receipt list shape" }
+    end
+
+    candidates = Array(receipts)

🤖 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 `@bin/agent-workflows-delivery-state` around lines 201 - 211, Update the
receipt normalization before the candidate validation in the delivery-state
flow: explicitly distinguish an array of receipts from a single receipt object
instead of applying Array(receipts), which coerces hashes into key-value pairs.
Preserve valid list handling and either honor a valid single receipt or return a
specific unknown result for the unsupported shape, ensuring invalid-shape
diagnostics remain accurate.

🤖 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 `@bin/agent-workflows-delivery-state`:
- Around line 26-42: Validate that the parsed manifest is a Hash immediately
after JSON.parse and return nil for any other root type before calling fetch in
the manifest-processing helper. Preserve the existing validation and rescue
behavior for valid Hash manifests so scalar plugin.json roots produce the
structured unknown result instead of an uncaught NoMethodError.

---

Nitpick comments:
In `@bin/agent-workflows-delivery-state`:
- Around line 201-211: Update the receipt normalization before the candidate
validation in the delivery-state flow: explicitly distinguish an array of
receipts from a single receipt object instead of applying Array(receipts), which
coerces hashes into key-value pairs. Preserve valid list handling and either
honor a valid single receipt or return a specific unknown result for the
unsupported shape, ensuring invalid-shape diagnostics remain accurate.

🪄 Autofix (Beta)

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 21b1d055-d208-4039-ae20-80bcda54e9f2

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf1615 and 6219c1a.

📒 Files selected for processing (6)

• CHANGELOG.md
• bin/agent-workflows-delivery-state
• bin/agent-workflows-delivery-state-test.rb
• bin/agent-workflows-status-test.rb
• bin/install-agent-workflows-test.bash
• docs/plans/2026-07-17-plugin-companion-unrelated-skills-design.md

[claude]

Review

Overview

This PR fixes #191: plugin-companion delivery previously treated any unmanaged flat skill directory as a blocking conflict, even skills unrelated to the Source Pack. The fix narrows the "collision" definition for companion installs to only skills whose names are actually advertised by the currently verified active native plugin, while preserving the existing strict (name-agnostic) fail-closed behavior for legacy flat→companion migrations. It also hardens native-plugin root/provenance resolution (Codex version+source matching against the cache, Claude mixed-candidate handling) to fail closed rather than guess, and fixes a crash when a native plugin.json manifest is a scalar JSON value instead of an object.

Core logic lives entirely in bin/agent-workflows-delivery-state; bin/install-agent-workflows and bin/agent-workflows-status needed no changes since they only consume the check/migrate JSON payload, which gained additive fields (version, source, skill_names). Test coverage is substantial: new Ruby unit tests for collision drift in both directions (skill added to native after the recorded revision, skill removed from current source while an older native stays active) and mixed valid/invalid candidate cache roots, plus matching bash installer-level regressions.

Code quality

• plugin_manifest_skill_names cleanly replaces the old boolean plugin_manifest_valid?, returning the actual skill-name list (or nil) so callers can use it both for validity and for collision detection — good reuse, and I confirmed no remaining references to the old method.
• The legacy_flat split in managed_flat_inventory (full snapshot-based ownership walk for legacy-flat migrations vs. name-only collision check against collision_names for fresh/companion) is a clean way to keep the two behaviors from bleeding into each other. Verified: for fresh installs metadata is nil so legacy_flat is falsy via short-circuit even though prior_delivery_mode defaults to "flat" — correct, if slightly subtle; a one-line comment on that default would help future readers.
• Fail-closed posture is consistent throughout: unmatched version, ambiguous cache candidates, scalar manifests, mismatched source/version, and multiple Claude receipt candidates all resolve to "unknown" rather than silently picking one.
• migrate re-derives check(...) including the new collision logic, so there's no divergence risk between the check and migrate code paths.

Minor observations (non-blocking)

• In codex_native_state, candidates = Dir.children(cache_root).filter_map { |entry| File.join(cache_root, entry) if entry == version } can only ever produce 0 or 1 matches (Dir.children never returns duplicate names), so the "#{candidates.length} matching cache candidates" wording implying a >1 case is effectively dead/unreachable. Harmless, just slightly misleading if someone reads the reason string literally.
• normalized_plugin_source strips trailing slashes and a .git suffix but doesn't normalize scheme/case (e.g. HTTPS://GitHub.com/... vs https://github.com/..., or SSH vs HTTPS remotes) — a real mismatch here fails closed (safe direction) but could be a source of false "unknown" states for users with non-canonical remotes. Worth a mental note if support requests come in about this.
• The Codex source provenance check assumes the CLI's PATH column reports a repository URL rather than a filesystem path (tests model it that way, and the design doc explicitly calls this out). Since I can't independently verify real Codex CLI output in this environment, this is the one assumption I'd double check against a real codex plugin list run in case the column's contents differ by Codex version — a false mismatch here is safe (fails closed to "unknown"), but could unnecessarily block legitimate companion installs.

Security

No concerns. The version-string regex (\A[0-9A-Za-z][0-9A-Za-z._+-]*\z) blocks path-traversal-style values before they're used in File.join, though it's largely belt-and-suspenders since candidates are still matched against real Dir.children entries. Manifest parsing is defensively guarded (is_a?(Hash) checks, rescued JSON::ParserError/TypeError/KeyError) so a malformed or scalar plugin.json can no longer raise NoMethodError, confirmed by the new test_companion_status_reports_scalar_native_manifests_as_unknown regression covering string/number/boolean roots. No unrelated skill path is ever deleted/modified for the newly-allowed cases — only used for name-matching.

Performance

No meaningful impact — the added work is a few Dir.glob/Dir.children calls per check/status invocation, same order of magnitude as before.

Test coverage

Strong. Both bash installer-level and Ruby unit-level regressions cover: unrelated skill preserved on fresh companion install, preserved on repeat install, collision blocked when native gains the skill name after the recorded revision, collision blocked when native still advertises a skill the current source removed, and mixed valid/invalid candidate native roots for both Codex and Claude. I was not able to execute the test suites myself in this sandboxed session (command execution required approval that wasn't available), so I relied on static review — the PR description reports agent-workflows-delivery-state-test.rb (184 assertions) and install-agent-workflows-test.bash passing, plus a clean bin/validate/RuboCop run.

Documentation

CHANGELOG.md entry and the new docs/plans/2026-07-17-plugin-companion-unrelated-skills-design.md design doc follow the existing repo convention (other dated design docs already live under docs/plans/) and clearly scope what changed vs. what's intentionally left alone (ownership proof for Source Pack skills, migration staging/rollback, metadata formats).

Overall: a well-scoped, carefully fail-closed fix with matching regression coverage. I'd merge as-is; the notes above are polish, not blockers.