Skip to content

refactor(config): remove SCM detection and per-SCM skill installation - #671

Merged
lavaman131 merged 1 commit into
mainfrom
refactor/remove-scm-config
Apr 18, 2026
Merged

refactor(config): remove SCM detection and per-SCM skill installation#671
lavaman131 merged 1 commit into
mainfrom
refactor/remove-scm-config

Conversation

@lavaman131

@lavaman131 lavaman131 commented Apr 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Simplifies the init/onboarding flow by removing all source-control-aware logic (GitHub/Sapling detection, per-SCM skill variants) and switches MCP auth headers from hardcoded placeholder strings to environment variable substitution.

Key Changes

  • Deleted src/commands/cli/init/scm.ts — removes installLocalScmSkills and syncProjectScmSkills entirely
  • Simplified ensureProjectSetup — drops SCM detection, skill presence checks, and conditional install paths; now only applies onboarding files and registers the trusted workspace
  • Removed SCM_CONFIG, SCM_SKILLS_BY_TYPE, ALL_SCM_SKILLS, detectScmType from src/services/config/definitions.ts
  • Global skill install no longer excludes SCM skillsbundle-configs.ts and skills.ts drop the exclusion lists; all skills are installed globally without VCS-specific filtering
  • Shell completions updated — removed --scm / -s flag and github/sapling completions from bash, fish, zsh, and PowerShell completion scripts
  • MCP auth headers.mcp.json and .opencode/opencode.json now reference ${GITHUB_PERSONAL_ACCESS_TOKEN} (env substitution) instead of the literal placeholder YOUR_GITHUB_PAT

Breaking Changes / Migration Notes

  • The --scm / -s flag for atomic init no longer exists; callers passing this flag will see an unrecognized option error
  • SCM-specific skills (gh-commit, gh-create-pr, sl-commit, sl-submit-diff) are now installed globally for all users instead of per-project based on detected SCM
  • MCP configs that used YOUR_GITHUB_PAT must now rely on the GITHUB_PERSONAL_ACCESS_TOKEN environment variable

Drops the source-control-aware init path (github/sapling detection,
gh-*/sl-* skill variants, SCM completions and settings fields) and
switches MCP auth headers to reference GITHUB_PERSONAL_ACCESS_TOKEN
via env substitution instead of a placeholder string.
@lavaman131
lavaman131 merged commit 1775a81 into main Apr 18, 2026
4 checks passed
@lavaman131
lavaman131 deleted the refactor/remove-scm-config branch April 18, 2026 17:32
@claude

claude Bot commented Apr 18, 2026

Copy link
Copy Markdown

Code Review

This is a clean, well-scoped refactor. Nice reduction — ~400 LOC removed, the init path is significantly simpler (no more SCM detection, no bunx skills add subprocess, no best-effort error-swallowing block), and the ensureProjectSetup function now reads at a glance. Centralizing SCM skills into the global install via bundle-configs.ts also removes an entire class of per-project failure modes. Below are a few follow-ups I'd recommend.

1. Incomplete cleanup — stale scm references

Several files outside the diff still carry the removed scm concept:

  • assets/settings.schema.json:15-19 — still declares scm as a valid property with an enum: [\"github\", \"sapling\"]. Users with schema-aware editors (VS Code) will get autocompletions for a field the code now silently drops.
  • .atomic/settings.json:2 — this repo's own project settings still contain \"scm\": \"github\". Worth removing so the committed example doesn't mislead.
  • README.md:1142,1154 — the documented settings example and the field table still list scm. Users reading the docs will write configs that get ignored.
  • src/services/config/atomic-config.ts:4 — the top-of-file comment still says "Project/source-control selections are stored in .atomic/settings.json".

These are cheap follow-ups but they leave the public contract (schema + docs) out of sync with the code.

2. Silent config-field drop on read

pickAtomicConfig / mergeConfigs in atomic-config.ts no longer recognize the scm field. For existing users who had set scm: \"sapling\" in their local .atomic/settings.json, the field will now be silently discarded on the next write. No warning, no migration. Given the bundle-configs change installs all SCM skills globally anyway, this is probably fine functionally — but a one-line console.warn when an unknown scm field is encountered, or a note in a release changelog, would save users confused by "why did my sapling config stop doing anything?".

3. Inconsistent env-substitution syntax across MCP configs

- .mcp.json:                 \"Bearer ${GITHUB_PERSONAL_ACCESS_TOKEN}\"
- .opencode/opencode.json:   \"Bearer ${env:GITHUB_PERSONAL_ACCESS_TOKEN}\"

Different syntaxes — ${VAR} vs. ${env:VAR}. This is almost certainly correct because the two clients (Claude Code MCP vs. opencode) use different substitution schemes, but it's the kind of difference that will bite the next person who tries to copy one config into the other. Worth a brief comment in each file noting which syntax belongs to which client, or a note in the README's MCP setup section.

Also worth verifying: when GITHUB_PERSONAL_ACCESS_TOKEN is unset, does each client handle the missing variable gracefully (warn / fail-closed), or does it send the literal \"Bearer ${GITHUB_PERSONAL_ACCESS_TOKEN}\" string to GitHub? A quick manual smoke-test with the env var unset would be reassuring.

4. Test coverage

The removed scm.ts had non-trivial logic (installLocalScmSkills spawning bunx, syncProjectScmSkills doing directory walks + copies). I don't see tests being deleted alongside it, which suggests that logic was already untested. The simplified ensureProjectSetup is now much easier to test — three serial awaits with no branching — so this might be a good moment to add a small integration test covering the happy path (fresh project → onboarding files applied + trusted workspace registered).

5. Minor — completions comment alignment

In src/completions/bash.ts the inline comment-column alignment got slightly nudged when -s|--scm was removed:

-            -a|--agent|-s|--scm|-n|--name) (( i++ )) ;;  # skip flag value
-            -*)                            ;;              # skip other flags
+            -a|--agent|-n|--name) (( i++ )) ;;  # skip flag value
+            -*)                   ;;              # skip other flags

The ;; # skip other flags comment is now over-indented by several spaces. Cosmetic only.


Summary

The core refactor is solid and the blast radius is well contained. The main follow-up is #1 (docs/schema sync) — without it, the removal isn't visible to users yet. #2 and #3 are smaller polish items. Nothing here blocks, but I'd recommend a short follow-up PR to close out the scm field in settings.schema.json, README.md, and the repo's own .atomic/settings.json.

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