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
a83cfd2 to
f978c84
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Legacy Codex installs can remain after a home migration, and transient skill-write failures can leave installations permanently unretryable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
internal/commands/skill.go:351
- The predefined wizard has the same retry trap as the non-interactive installer: after the ownership gate creates a new directory, a failed first write leaves it empty, and the next run rejects that empty directory as unmanaged. Remove it on failure when it is still empty so a transient write error remains recoverable.
writeErr := writeWizardSkill(expandedPath, data, selectedPredefined)
if writeErr != nil {
return fmt.Errorf("writing skill file: %w", writeErr)
- Files reviewed: 15/15 changed files
- Comments generated: 2
- 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
f978c84 to
f63020b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Destructive filesystem cleanup and external plugin operations warrant final human validation despite extensive safety checks and tests.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- 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
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.
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 |
Summary
Adds
basecamp setup agents --remove, following the ownership and migration safety conventions established in HEY CLI.The command:
CLAUDE_CONFIG_DIRconsistently across setup, health checks, repair, and removalcodex plugin remove basecamp@37signals --jsonIt also tightens skill install/refresh behavior so unowned skill directories are not overwritten or claimed.
Validation
The broader CI suite has existing terminal-dependent assertions when run without a real TTY; all feature-specific tests pass.
Summary by cubic
Adds
basecamp setup agents --removeto safely remove Basecamp-managed coding-agent integrations without touching authentication, configuration, or project data. Setup now refuses to overwrite or claim unowned skill directories instead of falling back to them.Removal and safety
CLAUDE_CONFIG_DIRandCODEX_HOMEconsistently, rejecting relative Claude paths and expanding Codex home aliases.Written for commit 5271311. Summary will update on new commits.