test(setup): cover the attribution gate in configure_claude_settings - #29
Merged
Conversation
The attribution block landed in 0e18d88 with no test behind it. The write itself is a one line jq merge, but the gate in front of it is not: it has to offer the change to a config that has never seen it, migrate one written before sessionUrl existed, and stay silent on a config where the user has chosen their own attribution. Nothing pinned any of that. The never-overwrite cases are the ones worth locking down. A run that clobbers "sessionUrl": true, or a custom commit trailer, silently reverses a decision the user made on purpose, and the only evidence would be the next commit. ./setup has no main guard, so sourcing it whole runs the installer. Everything above the argument parsing marker is constants and function definitions, so the helper sources just that prefix and calls configure_claude_settings against the isolated HOME that test_helper already sets up. That runs the real function rather than a copy of its jq, which is the point: a mutation dropping sessionUrl from the write fails three of these.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are test-only and the added cases directly and safely cover the stated attribution-gating behaviors without modifying runtime code.
Pull request overview
Adds a dedicated Bats test suite to exercise the “attribution” prompt/gating logic inside configure_claude_settings (defined in ./setup) without sourcing the full installer script.
Changes:
- Introduces
test/setup-settings.batswith fixtures to source only the definition prefix of./setupand invokeconfigure_claude_settingsagainst an isolated$HOME. - Adds coverage for “offer vs. don’t offer” cases, pre-
sessionUrlmigration behavior, and “never overwrite user choice” scenarios. - Verifies the resulting settings file remains valid JSON and preserves unrelated keys.
File summaries
| File | Description |
|---|---|
| test/setup-settings.bats | Adds 8 Bats tests validating the attribution gate/migration behavior in configure_claude_settings. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
test/setup-settings.bats, eight tests over the attribution handling inconfigure_claude_settings.Why
The attribution block landed in 0e18d88 with no test behind it. The write is a one line jq merge, but the gate in front of it is not: it has to offer the change to a config that has never seen it, migrate one written before
sessionUrlexisted, and stay silent on a config where the user has chosen their own attribution.The never-overwrite cases matter most. A run that clobbers
"sessionUrl": true, or a custom commit trailer, silently reverses a deliberate decision, and the only evidence would be the next commit.Coverage
attributionkey{"commit":"","pr":""}(pre-sessionUrl)sessionUrladded"sessionUrl": true"commit": "Made by Claude"Plus a check that the result is valid JSON and unrelated keys survive.
How it runs the real code
./setuphas no main guard, so sourcing it whole would run the installer. Everything above the argument parsing marker is constants and function definitions, so the helper sources just that prefix and callsconfigure_claude_settingsagainst the isolatedHOMEthattest_helperalready sets up. That exercises the shipped function rather than a copy of its jq.Verification
"sessionUrl": falsefrom the write insetupfails three of the new tests.setupitself is unchanged; this PR is tests only.