Add safe coding-agent integration removal - #676
Conversation
github-actions
Bot
added
commands
There was a problem hiding this comment.
🟡 Changes recommended
Removal can hang, mishandle managed fallback directories and cross-volume Claude paths, and drops structured partial results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds safe removal of Basecamp-managed coding-agent integrations while preserving user-owned files and configuration.
Changes:
- Adds
setup agents --removefor Claude, Codex, and managed skills. - Adds ownership safeguards and custom agent-home handling.
- Adds focused tests, documentation, and CLI surface updates.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
.surface |
Records the new flag. |
README.md |
Documents integration removal. |
skills/basecamp/SKILL.md |
Updates agent guidance. |
internal/harness/claude.go |
Honors custom Claude configuration paths. |
internal/harness/codex.go |
Adds custom-home detection and plugin queries. |
internal/harness/codex_test.go |
Tests custom Codex homes. |
internal/commands/wizard_agents.go |
Registers the removal flag. |
internal/commands/skill.go |
Adds skill ownership protections. |
internal/commands/skill_test.go |
Tests ownership-preserving behavior. |
internal/commands/setup_agents_remove.go |
Implements integration removal. |
internal/commands/setup_agents_remove_test.go |
Covers removal and migration cases. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
f3d34d0 to
195c817
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Removal can mask uninstall failures, leave legacy links broken, and overwrite additional files during copy fallback.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
195c817 to
cdee9b0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Legacy markerless installations are missed, and relative CODEX_HOME can direct deletion into project paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
cdee9b0 to
891159d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Predefined skill installation can follow a symlink, and managed refresh failures can be permanently skipped.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
891159d to
e2711d6
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
🟡 Changes recommended
Symlinked configuration paths can create broken links, and configuration and idempotence edge cases remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
e2711d6 to
fbd140a
Compare
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
f63020b to
cb0a8b7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Skill refresh still follows symlinked parent directories, conflicting with the removal safety boundary.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
cb0a8b7 to
c5d8d12
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Destructive filesystem cleanup and external plugin operations warrant final human validation despite comprehensive safeguards and tests.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
🟡 Changes recommended
Claude skill refresh and symlink traversal currently mishandle normal managed links and symlinked ancestors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
internal/commands/skill.go:562
- This
MkdirAllcan follow an existingskillssymlink and then create or replace the Basecamp link outside Claude's configured tree. Other new write/removal paths explicitly reject symlinked predefined ancestors, so this setup path should perform the same traversal check before writing.
if err := os.MkdirAll(symlinkDir, 0o755); err != nil { //nolint:gosec // G301: Skill files are not secrets
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
🔵 Needs a closer look
Destructive filesystem cleanup and external plugin mutations across multiple migration formats warrant final human validation.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
🟡 Changes recommended
Managed Claude symlinks currently cause every automatic skill refresh to be treated as failed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
internal/commands/skill.go:730
- A normal Basecamp-managed Claude skill symlink is always counted as a refresh failure here.
hasSymlinkComponentdetects thebasecamplink, whileownedOrLegacySkillDir(dir)follows that parent link when checking its marker, so it returns true. Consequently every CLI invocation for a Claude user leaves the version sentinel stale and repeatedly rewrites the other managed skills. Exempt a verified link to the managed canonical skill from failure accounting and letrepairClaudeSkillLinkhandle its health.
if ownedOrLegacySkillDir(dir) || invalidSkillMarker(dir) {
outcome.failed++
}
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues can cause false success reporting and prevent legacy Claude skill migration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
internal/commands/skill.go:627
- A pre-marker Claude installation created by the old wizard can contain only the shipped
SKILL.md. Other paths in this PR recognize that exact payload as legacy-managed, but this check requires a marker, sobasecamp skill installandbasecamp setup agentsreject the legacy directory instead of migrating it. Accept the same proven legacy payload here;copySkillFileswill then add the current ownership marker while preserving extra files for marked installs.
if !info.IsDir() || !ownedSkillDir(path) {
return &unmanagedSkillDirError{dir: path}
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
|
@cubic-dev-ai, you tired? |
@almirsarajcic Nope — this is a big change, but the added coverage is doing the heavy lifting. The symlink, ownership, partial-failure, config-path, and idempotency cases are all addressed here. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
Both releases shipped after the allowlist was assembled, so a wizard install from either at a non-canonical destination was treated as user-authored: never refreshed, never removed, and refused on reinstall.
jeremy
left a comment
jeremy
left a comment
There was a problem hiding this comment.
Confirm pass on cb4a355 (your 5271311 plus one small commit I pushed to the branch, see below). I read the full diff, ran the local gate target by target, exercised the new command and the touched install paths against a built binary from this branch and one from main, and spot-checked a sample of the resolved bot threads against the current code. Thanks for the care here: the ownership marker, the structured removed/failures metadata on the error envelope, the idempotent second run, the Claude registry scope handling, and computing the Claude link target from resolved directories (which fixes a real pre-existing broken-link bug for anyone whose ~/.claude is a symlink) are all good and match the HEY CLI shape.
Verdict: one change is required before this can merge, plus one I already pushed.
1. The parent-symlink traversal guard has to go
hasSymlinkComponent / safeSkillTraversal (and everything that exists only to feed them: claudeLinksHandled, predefinedSkillRoot, refreshLocationRoot, skillPathWithin, the claudeRoot/codexRoot derivations) refuse to touch any predefined path that has a symlink anywhere between the trusted root and the leaf. That is exactly how dotfiles-managed homes are laid out, and the guard regresses the install path every user runs, not just --remove. Reproduced against binaries built from main and from this branch, HOME pointed at a scratch directory:
| Scenario | main |
this branch |
|---|---|---|
~/.claude -> ~/dotfiles/claude, basecamp skill install |
ok, link created | ok:false, exit 7: "~/.claude/skills exists but was not written by basecamp-cli" (it does not exist) |
~/.agents -> ~/dotfiles/agents, basecamp setup agents (the piped-installer path) |
ok | "Baseline skill installation failed" |
~/.claude -> ~/dotfiles/claude, BASECAMP_SETUP_AGENT=claude basecamp setup agents |
ok | errors: ["claude: skill link: ... not written by basecamp-cli"] |
link installed by main, ~/.claude symlinked, then --remove |
n/a | ok:false "Claude Code skill: unsafe symlink traversal skipped", nothing removed, baseline retained because claudeLinksHandled is false. The user cannot reach ok:true without deleting their own symlink. |
clean home, --remove, cwd is a repo whose .claude/skills is a symlink (common for shared skill dirs) |
n/a | ok:false "project Claude Code skill: unsafe symlink traversal skipped". Same home from the directory next door: ok:true. |
The last row also means a fully successful removal reports failure: with a real install in ~ and that cwd, removed: ["Claude Code skill","agent skill"] and exit 7.
What the guard defends against is the wrong question here. The only party who can plant a symlink under a user's $HOME or inside their project is that user, and every destructive step already proves ownership at the leaf before it deletes anything: marker file, .installed-version, exact embedded/allowlisted payload in a flat directory, or a link that resolves to the managed baseline. A parent symlink changes where those checks run, not what they prove. The leaf-level Lstat refusals (a symlink at the skill dir or at a marker path is never followed) are the part that carries the safety, and they stay.
Change: delete hasSymlinkComponent, safeSkillTraversal, predefinedSkillRoot, refreshLocationRoot, skillPathWithin, claudeLinksHandled, and the root derivations that feed them; keep the leaf checks; drop the tests that assert the skip (...BehindSymlinkedParent, ...ReportsSkippedSymlinkedBaseline, TestSafeSkillTraversalRejectsSymlinkedDefaultParents) and replace them with one that installs through a symlinked ~/.claude and removes cleanly. That is also what HEY CLI's claimSkillDir does: it inspects the leaf only. Expect this to remove a few hundred lines and the "Claude Code (configured)" duplicate refresh entry along with it.
2. The legacy payload allowlist was already two releases stale (pushed)
legacyManagedSkillHashes stopped at v0.9.1, but v0.10.0 (e1394ab...) and v0.11.0 (dad3d2e..., current main) have shipped since. I verified by hashing skills/basecamp/SKILL.md at every tag. Effect before the fix: a v0.10.0 wizard install at ~/.codex/skills/basecamp/ (no marker, no .installed-version, which the old wizard only stamped in the canonical dir) was treated as user-authored: --remove left it, refresh skipped it forever, and re-running the wizard to that destination refused. A v0.9.1 payload in the same spot was removed. I pushed cb4a355 adding both hashes to the map and to TestLegacyManagedSkillHashAllowlistDoesNotShrink; the v0.10.0 case now removes.
For whoever merges: the list has to cover every payload that shipped without the marker, so if another release goes out before this lands, its hash goes in too. After that the list is closed.
Notes, not blocking
removeCodexPlugin: an existing~/.codexwith nocodexonPATHis a hard failure ("codex binary not found"), so anyone who uninstalled Codex but kept its home can never getok:true. The Claude side only demands the binary when the registry actually lists our plugin. Worth mirroring if there is any file underCODEX_HOMEthat records plugin state; if not, leave it and say so in the help text.--removeleaves the37signalsmarketplace registration in Claude in place. Defensible (it may serve other plugins), but the README andLongtext should say so, since "removes coding-agent plugins" reads as complete.unmanagedSkillDirErrorsurfaces throughskill install --jsonascode: "api_error". Not a regression (maindoes the same for its plain errors), but a local filesystem refusal deserves its own code now that it is a named type.removeOwnedOrLegacyCodexSkillis a pure alias ofremoveOwnedOrLegacySkill; fold it.
What I ran
- Local gate on
cb4a355, eachmake checktarget separately:fmt-check,vet,lint(0 issues),lint-actions,check-naming,check-surface,check-skill-drift,check-bare-groups,check-lint-lockstep,check-smoke-coverage,provenance-check,tidy-checkall pass.testandtest-e2efail only on the known non-TTY set (TestIsInteractive*,TestInteractive*,TestDeleteConfirmable...,TestBareBasecamp...,TestExplicitSetup...,TestPredicates..., e2e 340/341tcgetattr); the same set fails onmainin this environment and none touch this PR's files. basecamp setup agents --help --agentlists--removewith truthful usage;.surface,SKILL.md, README updated;--jsonerror envelope carriesmeta.removed/meta.failures; second--removeis a clean no-op; Claude plugin removal issues oneplugin uninstall <key> --scope <scope>per recorded scope with the project scope run from its recorded directory.- Sampled resolved bot threads (WaitDelay, relative
CODEX_HOME, OpenCode locations,TrimSpaceon config env, staleprojectPath, generic helper naming, wizard "Claude Code (Global)" afterskill install, error metadata): each is addressed in the current code, not just resolved.
Not doing: I am not asking for --remove to be split into per-agent setup claude --remove / setup codex --remove; the flag on setup agents matches HEY CLI and is one clear way to do it.
Note for maintainers: the Test, CodeQL and Security workflow runs on this head are sitting in action_required (first-time contributor approval); only the sensitive-change gate and labeler have actually run on GitHub. The gate results above are local.
| // hasSymlinkComponent refuses traversal through user-controlled aliases below | ||
| // a trusted installation root. Cleanup may inspect the root itself, but it must | ||
| // never follow a symlink in a predefined path into an unrelated directory. | ||
| func hasSymlinkComponent(root, target string) (bool, error) { |
There was a problem hiding this comment.
This is the guard that breaks dotfiles setups: any symlink between the root and the leaf, including a symlinked ~/.claude, ~/.agents, ~/.codex, or a project .claude/skills, turns a valid install or removal into a failure. Every deletion below already proves ownership at the leaf (marker, .installed-version, exact payload, or link to the baseline), so the parent walk adds no protection. Remove hasSymlinkComponent and safeSkillTraversal and the root derivations that feed them; keep the leaf Lstat refusals.
| removed = append(removed, "legacy Claude Code skill") | ||
| } | ||
| } | ||
| projectClaudeSkill := filepath.Join(".claude", "skills", "basecamp") |
There was a problem hiding this comment.
Reproduced: from a cwd whose .claude/skills is a symlink (a common layout for repos that share one skills directory), a clean home returns ok:false "project Claude Code skill: unsafe symlink traversal skipped" with nothing to remove, and a real removal in ~ reports failure after succeeding. The same command from the directory next door returns ok:true. With the parent guard gone, this block only needs removeOwnedOrLegacySkill(projectClaudeSkill).
| // A managed Claude link can only be recognized while its managed baseline | ||
| // remains intact. Retain the baseline after any link-slot inspection or | ||
| // removal failure so a retry can still prove and remove the link. | ||
| if baseline != "" && claudeLinksHandled { |
There was a problem hiding this comment.
With ~/.claude -> ~/dotfiles/claude and a link installed by v0.11.0, the skip above sets claudeLinksHandled false, so the baseline is retained and the run removes nothing while reporting failure; the user cannot reach ok:true without deleting their own symlink. This flag exists only to serve the traversal guard and goes with it.
|
|
||
| const agentRemoveTimeout = 20 * time.Second | ||
|
|
||
| var legacyManagedSkillHashes = map[string]struct{}{ |
There was a problem hiding this comment.
Pushed cb4a355 adding the v0.10.0 (e1394ab...) and v0.11.0 (dad3d2e...) payloads; both had shipped after this list was assembled, so a wizard install from either at a non-canonical destination was treated as user-authored. Verified by hashing skills/basecamp/SKILL.md at every tag. If another release ships before this merges, its hash belongs here too; after that the list is closed because every new install carries the marker.
| func removeCodexPlugin(parent context.Context) (bool, string) { | ||
| codexPath := harness.FindCodexBinary() | ||
| if codexPath == "" { | ||
| if codexHomeExists() { |
There was a problem hiding this comment.
Not blocking: an existing ~/.codex with no codex on PATH is a permanent failure, so someone who uninstalled Codex and kept its home never gets ok:true. The Claude path only requires the binary when the registry lists our plugin. If Codex keeps any plugin state under CODEX_HOME, gate on that instead; if it does not, leave this and say so in the help text.
| return false, err | ||
| } | ||
| if skillPathWithin(home, dir) { | ||
| symlinked, inspectErr := hasSymlinkComponent(home, filepath.Dir(dir)) |
There was a problem hiding this comment.
Reproduced: with ~/.agents -> ~/dotfiles/agents, basecamp setup agents (the piped-installer path) now reports "Baseline skill installation failed" and basecamp skill install exits 7; both succeed on main. The leaf check in claimSkillDirLeafForWrite is the part that protects unowned content; this parent walk should go.
|
|
||
| type unmanagedSkillDirError struct{ dir string } | ||
|
|
||
| func (e *unmanagedSkillDirError) Error() string { |
There was a problem hiding this comment.
When the refusal comes from the parent-symlink walk this message names a path that does not exist (~/.claude/skills exists but was not written by basecamp-cli). Goes away with the guard; if the type stays, it also deserves its own output.Error code rather than surfacing as api_error from skill install --json.
| if os.Getenv("CLAUDE_CONFIG_DIR") == "" { | ||
| claudeRoot = home | ||
| } | ||
| symlinked, inspectErr := hasSymlinkComponent(claudeRoot, symlinkDir) |
There was a problem hiding this comment.
Same guard, third site: with ~/.claude symlinked, BASECAMP_SETUP_AGENT=claude basecamp setup agents now records skill link: ... not written by basecamp-cli where main links successfully. Removing it also makes the resolved-directory relative target below (which is a genuine fix for symlinked Claude homes) reachable for the people it helps.
| outcome.failed++ | ||
| continue | ||
| } | ||
| symlinked, inspectErr := hasSymlinkComponent(root, dir) |
There was a problem hiding this comment.
Same guard in the version-change refresh. An owned copy-fallback directory under a symlinked ~/.claude counts as failed, which keeps the sentinel stale and re-runs the refresh on every command. With the leaf checks only, this branch and refreshLocationRoot are unnecessary.
| outcome.failed++ | ||
| } else { | ||
| configured := filepath.Join(claudeConfig, "skills", "basecamp", skillFilename) | ||
| locations = append(locations, skillLocation{Name: "Claude Code (configured)", Path: configured}) |
There was a problem hiding this comment.
When CLAUDE_CONFIG_DIR is unset this duplicates the Claude Code (Global) entry and the file is written twice per refresh. Harmless, but replace the ~/.claude entry when the configured path differs rather than appending.
Remove parent-symlink traversal guards while retaining leaf ownership checks and safe retry behavior. Cover install/removal through symlinked parents, preserve user files, and clarify removal requirements. Integrate current main without rewriting contributor or maintainer commits.
There was a problem hiding this comment.
🟡 Changes recommended
Markerless legacy baselines with user files can leave their managed Claude link behind.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Two ownership checks follow a symlinked skill leaf before proving that the leaf itself is managed, risking modification or removal of user-owned links.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
Adds
basecamp setup agents --removeto remove Basecamp-managed coding-agent integrations while preserving user-authored files, Basecamp authentication, configuration, and project data.The command removes managed shared and agent-specific skills, recognizes proven legacy installations, uninstalls recorded Claude plugin scopes from their owning project directories, and queries Codex before removing its plugin. Partial failures include structured
removedandfailuresmetadata; repeated removal is safe.The maintainer follow-up is addressed in
96f819c: install, refresh, and removal accept symlinked parent directories used by dotfiles-managed homes. The shared ownership check inspects the skill directory itself before trusting markers, and the wizard validates the original canonical directory before resolving paths. A symlink at the skill-directory leaf remains user-owned even when its target contains a marker; symlinked parent directories remain supported. Legacy-baseline detection permits unrelated user files, allowing cleanup to remove the managed Claude link and skill payload while retaining those files. Regression tests cover install/removal through symlinked.claudeand.agentsparents, project skill parents, and preservation of user files. Actual Claude link-cleanup failures still retain the shared baseline so a retry can prove link ownership.Preserves Jeremy’s
cb4a355legacy-payload allowlist update, including v0.10.0 and v0.11.0, and integrates currentmain. Removes the redundant Codex cleanup alias, reports an unowned install destination asusageinstead ofapi_error, and documents that removal retains the shared37signalsmarketplace registration and uses thecodexbinary for Codex plugin operations.Validation: targeted ownership/removal/error tests, all five maintainer-reported scenarios in isolated homes, idempotence and user-file preservation, vet, lint, formatting, surface, skill drift, naming, provenance, tidy, lockstep, and smoke-coverage checks. The end-to-end suite passed (464 listed cases, with platform/tool skips). Full unit tests encounter terminal-detection failures also reproduced on clean upstream
main; the feature-specific tests pass. Hosted Test/Security/CodeQL runs require maintainer approval before they can run.