Repository navigation
A local config's profile entry hid the account bound in the global config - #745
Merged
Merged
Conversation
…at hides a binding A trusted repo or local config that defined a profile with no account_id replaced the global config's entry whole, so an account bound there was never seen. Entries for the same Basecamp now merge field by field; an entry for another Basecamp still replaces the farther one. config.Load records which file each profile and field came from, and connect setup, auth agent connect and the headless logins use it to name the file that hides a binding, refusing to bind where it would not take effect.
github-actions
Bot
added
commands
Contributor
There was a problem hiding this comment.
🟢 Approved
The merge rules, provenance handling, command safeguards, and relevant edge cases are consistently implemented and tested.
Pull request overview
Updates profile layering so same-Basecamp entries merge per field while different-Basecamp entries replace prior configuration.
Changes:
- Adds profile and field provenance tracking.
- Prevents account-binding commands from writing ineffective global bindings.
- Adds integration coverage for layered profiles and improved remediation hints.
[!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 |
|---|---|
internal/config/config.go |
Implements layered profile merging and provenance. |
internal/config/config_test.go |
Tests merge and replacement semantics. |
internal/commands/profile.go |
Determines whether global bindings take effect. |
internal/commands/profile_layers_test.go |
Adds layered-command integration tests. |
internal/commands/profile_layers_helpers_test.go |
Provides layered-config test helpers. |
internal/commands/connect.go |
Improves binding and invalid-account hints. |
internal/commands/connect_unbound_test.go |
Tests provenance-aware connect hints. |
internal/commands/auth.go |
Guards headless profile binding. |
internal/commands/auth_login_test.go |
Tests headless login across config layers. |
internal/commands/auth_agent.go |
Guards agent connection binding. |
internal/auth/agent.go |
Clarifies generated login-command behavior. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…t replacing file An entry naming a different account on the same Basecamp now replaces the farther one whole, so a project or todolist from one account is never carried into another. The hint names the file that replaced the global entry even after a later entry replaced that one, and the test helper resolves the temp dir as the loader does.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 17, 2026 10:29
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Equivalent numeric account-ID spellings can incorrectly trigger whole-profile replacement and discard inherited fields.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
…the files Account IDs in the merge are compared as numbers, as the commands compare them. Each replaced entry now records the entry that replaced it, so the hint names that file and says whether it was another Basecamp or another account. A profile no global entry defines is told it can have one, which the closer entries refine, rather than being sent to write an operator's account into a shared repo config. Messages about a profile's account name the file it is bound in.
A global config the loader could not parse is skipped, profiles and all, so the layers it recorded are not evidence about which file defines a profile. The blocker now says nothing in that case, and the headless logins prove the global file can take a write before they do, as the connection already did: all three commands report the unusable file itself. The hint for a profile with no global entry names base_url as something to give the entry, since an entry there may exist without one.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 17, 2026 11:21
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The suggested global-config remedy can remain hidden when intervening profile layers switch Basecamps.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/profile.go:610
- This global-config remedy is not always effective when the profile has already crossed Basecamps in closer layers. For example, with repo entry A and local entry B, adding the suggested global entry for B loads as global/B → repo/A (replaces global) → local/B (replaces repo), so the account is still absent and every binding command refuses again. Offer the global alternative only when no intervening replacement can hide it; otherwise direct the operator to the effective entry (or explain which conflicting layers must change).
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Where entries for the profile already replace one another, an entry added to the global config is replaced among them, so the account in it would never hold. That case now names the entry that replaces everything farther and sends the operator to the effective one.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The binding blocker incorrectly rejects a valid global-entry remedy when only a system-layer profile was replaced.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
The global config is closer than the system one, so an entry added there replaces a system entry rather than being replaced by it. Only a replaced repo or local entry proves a new global entry would go the same way.
… guard An entry that names another project no longer inherits the farther file's todolist, which is a todolist of the project it replaced. And connect setup, the one path that reaches the guard for a global config that cannot be parsed, now has the test for it.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 17, 2026 12:58
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Numeric project equivalence and malformed global profile structures still produce incorrect behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/profile.go:652
- This only treats malformed JSON or a non-object root as unusable. A valid global file with
"profiles": []is skipped by the loader, but this returns false, soconnect setupcan recommend adding a global entry even thoughglobalProfilesMaprejects every writer with “profiles value is not an object.” Validate the profiles shape here too so setup falls back to the remedy that reports the unusable file.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
… unusable One rule for IDs: equal as decimal numbers, so a project spelled "09" is the project spelled "9" and its todolist is kept. A global config whose profiles value is not an object is skipped by the loader and refused by every writer, so it is reported as itself rather than recommended as the place to add an entry.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
profiles: null is incorrectly considered writable, producing a remediation that subsequent binding commands reject.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
globalProfilesMap refuses a "profiles" value of any shape but an object, null included, so the global config is reported as itself rather than recommended as the place to add an entry.
Contributor
There was a problem hiding this comment.
🟢 Approved
The merge rules, provenance handling, binding guards, and key edge cases are consistently implemented and comprehensively tested.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
An operator binds their agent profile's account in the global config, runs
basecamp connect setupagain, and is still told the profile isn't bound. A trusted repo or local config that names the same profile, even just to set its project, replaced the global entry whole, account and all.basecamp auth agent connecthad the same gap: it wrote the account into the global entry and reported success, and the next command still saw no account.Originally tracked in A local config can hide a profile's account binding from the global config. Found while fixing the hint in Point the account-binding hint at a command that exists, and make the check able to say so, which noted that closing it needs the config layer to know which file each profile came from.
The behavior chosen: layering per field, for the same Basecamp. A closer entry for the same
base_urlnow refines the farther one: what it sets wins, and what it leaves out is kept. That is how the top-level keys have always layered, where a missingaccount_idin a local file never hid the global one. It also keeps what a trusted repo config means: a repo that names a profile's project is saying which project, not that the profile has no account. Keeping whole replacement and only improving the error would leave that repo config impossible to combine with a binding without copying a personal account ID into it.An entry for a different Basecamp, or one that names a different account on the same Basecamp, still replaces the farther entry whole. A project, todolist or client ID from one account means nothing in another, so carrying them across would be a silent mis-binding. The same reading one level down: an entry that names another project does not inherit a todolist, which belongs to the project it replaced.
That exception is the one case where a binding can still be hidden, so it's handled explicitly. Config loading now records which files each profile and each field came from, including the entries it replaced.
connect setup,auth agent connectand the headless logins (--with-token,--with-client-credentials) all ask one question before binding: would writing the global entry take effect? When it wouldn't, they refuse before anything is approved or read from stdin. They name the file whose entry replaced the global one and the file to addaccount_idto. A profile that exists only in a repo or local config gets the same treatment. The hints from that PR now name files rather than "the config file that defines it", and so does its hint about anaccount_idthat isn't a number.The merge rule is written down once, in a comment on the merge itself. The command tests run over real global and trusted local config files through
config.Load. They failed on main: setup reported the profile unbound, connect bound an account that never took effect, and it refused a profile whose global entry was already bound.