Skip to content

fix(bindings): write skills and commands under CLAUDE_CONFIG_DIR - #137

Merged
arcaven merged 1 commit into
mainfrom
build/sideshow/c07dl-claude-config-dir
Sep 30, 2026
Merged

arcaven merged 1 commit into
mainfrom
build/sideshow/c07dl-claude-config-dir

Conversation

@arcaven

@arcaven arcaven commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

With CLAUDE_CONFIG_DIR set, a sync still wrote skills and commands into $HOME/.claude, so a scratch or test install could rewrite the operator's live bindings. Now the write path resolves the config dir the same way the read path already did.

Change: claudeCommandsDir and claudeSkillsDir route through foreign.ConfigDir(): CLAUDE_CONFIG_DIR when it is set, ~/.claude otherwise. No new configuration surface. A package TestMain clears CLAUDE_CONFIG_DIR so that tests which isolate by setting HOME can't write into the config dir of whatever harness runs them.

Acceptance:

  • Red first: TestSync_HonorsClaudeConfigDir failed on main with all three assertions (skill missing from the config dir, command missing, $HOME/.claude created). It passes on this branch.
  • End to end, isolated HOME + CLAUDE_CONFIG_DIR + SIDESHOW_HOME, install then commands sync: the main binary wrote 2 files under $HOME/.claude and 0 under the config dir. This branch wrote 0 and 2.
  • Test guard control: running the bindings suite with CLAUDE_CONFIG_DIR pointing at a probe dir leaked 16 entries without TestMain and 0 with it. The full suite leaks 0.
  • just ci: fmt, golangci-lint (0 issues), vet, and test all pass.

Blast radius: this only changes behavior when CLAUDE_CONFIG_DIR is set. For such a user, bindings synced by an earlier version stay under $HOME/.claude; later syncs write, and reconcile, under the config dir. The old copies stay where they are and are not removed automatically. sideshow adopt --migrate-user-scope also resolves through foreign.ConfigDir(), so it reaches them only when run with CLAUDE_CONFIG_DIR unset. With the variable unset, the paths are byte-identical to before.

Opportunities (not touched here): the user-scope permissions writer (internal/permissions, ScopeUser) still resolves ~/.claude/settings.json directly. When SIDESHOW_HOME is set it is skipped, not redirected. That is the same shape as this bug and is left for its own change.

Refs: #136
Refs: aae-orc-c07dl

The write path resolved $HOME/.claude directly while the read path
(foreign.ConfigDir) honors CLAUDE_CONFIG_DIR, so a sync with the
variable set wrote into the operator's real config. Route
claudeCommandsDir and claudeSkillsDir through foreign.ConfigDir.

A package TestMain clears CLAUDE_CONFIG_DIR so HOME-isolated tests
cannot write into the config dir of the harness running them; without
it the suite leaked 16 entries into a probe dir.

Refs: #136
Refs: aae-orc-c07dl
@arcaven

arcaven commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

Handoff (token exhaustion, 2026-09-28):
State: draft, head 4322058 pushed and signed, just ci green, gated to the arcaven supervisor for review.
Next: after a non-author approval at this head, gh pr ready 137; never merge.
Open asks: review of this PR; jxaij (sideshow-packs r2) still waits on the human tag push. Full handoff: ~/.director/state/handoff/arcaven-sideshow-builder-g5-0-2026-09-28.md

@arcavenai arcavenai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at 4322058. The signature is valid.

  1. Red/green. I copied the new test files onto main (0f862ea). TestSync_HonorsClaudeConfigDir there: red, 1 fail, 0 pass, and all three assertions fire (skill missing from the config dir, command missing, $HOME/.claude created). On the branch: 1 pass.

  2. End to end. I built both binaries and ran install then commands sync against a two-artifact pack, with HOME, CLAUDE_CONFIG_DIR and SIDESHOW_HOME all isolated. Main wrote 2 files under $HOME/.claude and 0 under the config dir. The branch wrote 0 and 2. This matches the body.

  3. Test guard. I ran the full suite with CLAUDE_CONFIG_DIR pointed at an empty probe dir, which is the situation of a harness that sets it:

    • with the new TestMain: 0 entries leaked, 15 packages ok;
    • with TestMain removed as a control: 18 entries leaked.

    The guard is doing real work. It also covers the other packages that reach the bindings (enable, adopt, doctor, pack, coexistcheck): nothing leaks through them.

  4. Gate. just ci passes locally (golangci-lint 0 issues). The PR's checks are green.

  5. Scope and redaction. The change is exactly the two resolvers. Every write site in bindings (markdown_command.go, skill_dir.go, custom_skill_dir.go, and the reconcile in manifest.go) goes through them, so sync and reconcile move together. I scanned the diff, the commit message and the PR text for org, product and host tokens, with a positive control (control 1 hit, scan 0 hits). No em dashes and no attribution.

Merge recommended.

Notes for the follow-up, not blocking:

  • The permissions split is wider than the body's gap note says. permissions.SettingsPath (internal/permissions/permissions.go:33-34, ScopeUser) still resolves $HOME/.claude/settings.json. The gap note describes only the SIDESHOW_HOME case, where the write is skipped. The ordinary case is different: CLAUDE_CONFIG_DIR set and SIDESHOW_HOME unset. There, install still adds Read(<store>/packs/) to $HOME/.claude/settings.json, while the bindings now land in the config dir. I reproduced this with the branch binary. So a harness that reads the config dir gets the skills but not the Read grant for the pack store.

    This is not a regression. On main, that user got neither, in a file the harness does not read. But after this merge the two halves disagree where before they were consistently wrong. The fix is the same one-liner: ScopeUser returns filepath.Join(foreign.ConfigDir(), "settings.json"), with a test like this one. The same run confirms that --scope user with SIDESHOW_HOME set writes the $HOME file.

  • Ticket framing. aae-orc-c07dl's title and "Expected" line name SIDESHOW_HOME. With only SIDESHOW_HOME set, bindings still go to $HOME/.claude on this branch. I think that is right: SIDESHOW_HOME is the store, and the harness config belongs to CLAUDE_CONFIG_DIR, where the harness actually reads. The ticket allows "or CLAUDE_CONFIG_DIR", so this change meets its binding half. Isolation needs CLAUDE_CONFIG_DIR, and that is not documented anywhere in README.md or docs/. A line saying so would close the "docs present SIDESHOW_HOME as isolating" branch of the ticket.

@arcaven
arcaven marked this pull request as ready for review September 30, 2026 04:08
@arcaven
arcaven merged commit c33fdd2 into main Sep 30, 2026
10 checks passed
@arcaven
arcaven deleted the build/sideshow/c07dl-claude-config-dir branch September 30, 2026 04:51
@arcaven arcaven added the type.bug Broken behavior; something does not work as designed label Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type.bug Broken behavior; something does not work as designed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants