Repository navigation
Point the account-binding hint at a command that exists, and make the check able to say so - #744
Merged
Merged
Conversation
… check able to say so A profile that holds a credential but no account stopped connector setup with "Bind it: basecamp profile set <name> account_id <id>". There is no `profile set`. The account is bound by the command that stores the credential, so the hint now names those: `basecamp auth agent connect -P <name>`, which takes the agent's own account, and `basecamp auth login -P <name> --account <id>` on the bot-user path. The skill drift check could not catch it. It resolved a reference to the nearest existing ancestor, so `basecamp profile set` resolved as `basecamp profile` and passed. A reference now has to resolve exactly. Words past a resolved command are still allowed where they cannot name a subcommand — after a command that takes positional arguments, and after a leaf that has none — which is what the fallback was really covering: argument values and prose. What is left is a group that only dispatches followed by a word it does not dispatch to, and that fails. Hints in Go code are not skill files, so they get their own test: TestHintCommandsResolve resolves every command named in a hint across internal/commands, and every command named in any string in connect.go, against the real command tree. It is red on the old hint.
github-actions
Bot
added
commands
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Ambiguous command references and variable-built hints can still evade validation, and the replacement hint fails for layered profiles.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Corrects the account-binding hint and adds validation to prevent nonexistent CLI commands from appearing in skills and hints.
Changes:
- Replaces the invalid
profile setremedy. - Adds exact command-resolution checks and fixtures.
- Integrates drift tests into CI and updates documentation.
[!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 |
|---|---|
scripts/test-skill-drift.sh |
Adds drift-check fixtures. |
scripts/check-skill-drift.sh |
Tightens command resolution. |
Makefile |
Registers the new test target. |
internal/connector/ledger_recovery.go |
Corrects an inaccurate comment. |
internal/commands/hint_commands_test.go |
Validates commands referenced by hints. |
internal/commands/connect.go |
Replaces the invalid account-binding hint. |
AGENTS.md |
Documents drift and hint validation. |
.surface-skill-drift |
Documents command baselines. |
.github/workflows/test.yml |
Runs drift fixtures in CI. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…moved subcommands Three holes Copilot found in the first pass. The replacement hint named commands that refuse the very profile it fires for: both login paths turn away an accountless entry that is not the global config's, because the entry they would write stays shadowed by the layer that defines it. That profile now gets the only remedy it has — the config file the entry comes from. And an entry naming an account nothing can parse is no longer called unbound: the account it does name is what is wrong, and that is what it says. The hint test read only literals written where the hint is passed, so a hint built into a variable first (boost.go builds one, then appends --event) was invisible to it. It now follows the names a hint expression mentions. Exact resolution still could not tell an argument value from a subcommand we dropped: `basecamp recordings archive` reads as the [type] argument the moment `archive` is removed. Both checks now refuse a word that names a command in .surface-breaking before either escape can read it as prose — which is what makes the claim that removal is caught true.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Layered-profile handling, helper-returned hint coverage, and Bash compatibility remain unresolved.
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.
scripts/test-skill-drift.sh:89
${parameter@Q}requires Bash 4.4, while this repository's existing script gate supports Bash 4+ (scripts/check-smoke-coverage.sh:14-18). On Bash 4.0–4.3,make test-skill-driftexits withbad substitutionprecisely when an assertion mismatch should be reported. Use a portable quoted message (for example,expected '$2') or explicitly raise and enforce the minimum Bash version.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The hint still named `basecamp auth login -P <name> --account <id>` for the bot-user path. A browser login binds no account — only the headless --with-token and --with-client-credentials paths do — so the operator would log in, succeed, and hit the same refusal. The hint now names what binds: connecting an Agent takes its own account, and a bot's account goes into its entry in the global config. An unusable account_id now says which profile names it and where to correct it. The hint test missed hints assigned to a field (doctor.go's check.Hint), hint-named fields (feed.go), hints returned by a helper (wizard.go), and the readiness checks in internal/connector/setup whose hints connect setup shows. It reads all of those now, asserts one known reference per kind so a collector that goes blind fails, and asks a group's own argument validator whether a word past it is an argument — so `basecamp setup agentz` fails, as the CLI would. The drift check read "basecamp upload report.pdf" as the removed `upload report`, and "config set project_id" as `config set project`: a word running on into a filename or key is an argument, and extraction now stops before it. A baselined command's flags are checked again, against the deepest command that exists, and every DRIFT line names the baseline entry that acknowledges it.
Contributor
There was a problem hiding this comment.
🟢 Approved
The updated hints are actionable, and the new focused checks cover the command-resolution regression without unresolved issues.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
…s nothing The root runs and has no argument validator, which cobra's ValidateArgs reads as "accepts anything" — so `basecamp profilez set` in a hint passed, though the CLI answers "unknown command". The root now takes no word it does not know. "A bot user's login binds none" was too broad: a headless --with-token login with --account does bind. It is the browser login that binds nothing, and that is what the hint and its comment now say. The scan followed every name a hint expression mentioned, so `cmd` in cmd.CommandPath() pulled whole command definitions, help text included, in as hints. Only names that are values are followed now.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 17, 2026 09:18
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The hint scanner misses wrapper-forwarded hints, and the fixture harness contains a Bash 4.4-only expansion.
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/hint_commands_test.go:205
- This sink detection still misses hints passed through local wrapper functions. For example,
upgrade_selfupdate.go:324passes"Re-run basecamp upgrade"toerrUpgradeFailedHint; that helper assigns itshintparameter tooutput.Error.Hint, but this scan neither marks the wrapper argument as hint text nor traces callers backward, so changing that command to a nonexistent path leaves this test green. Please propagate arguments through local calls whose parameters reach aHintfield (or register those wrappers as sinks) and covererrUpgradeFailedHintwith a scanner fixture.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
A hint passed to a local wrapper — errUpgradeFailedHint(msg, "Re-run
basecamp upgrade ...") — was invisible: the scan knew only the output
constructors as places hint text goes. Every package function whose
parameter reaches a hint inside it is now one too, and so is a wrapper of
one, found by running until no function gains a parameter.
A parameter or name counts only when it is the text itself: the whole
expression, or a side of a concatenation. Following names a hint is merely
formatted from ran from formatCardTableIDs(cardTables) to every `cmd :=
&cobra.Command{...}` in the file, so help text was scanned as hints — and
with the root now refusing unknown words, "the way basecamp does it" in a
Short would have failed the test. A function literal called in place, and
the call a method chain starts from, are read.
The fixture harness used ${2@Q}, which bash 3.2 does not have; an
expected-substring mismatch there aborted the run instead of reporting it.
Contributor
There was a problem hiding this comment.
🟢 Approved
The updated implementation addresses prior findings with focused regression coverage and no remaining correctness issues.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
…n does Reading a function literal called in place and the receiver of a method chain changed nothing: no hint in either package takes either shape, the scanned set is identical without them, and no tripwire could notice them regress. They are gone. The package function index is keyed by bare name, so a collision narrows the scan — the shadowed declaration is never followed or read as a wrapper — rather than widening it as the comment said. No colliding name carries a hint today; the comment now says both.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 17, 2026 09:45
Contributor
There was a problem hiding this comment.
🟢 Approved
The revised hints are actionable, command validation is substantially strengthened, and regression coverage addresses the identified failure modes.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at 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 whose connector profile has no account was stopped by
basecamp connect setupwithBind it: basecamp profile set <name> account_id <id>. That is not a command —profilehas noset— so the one instruction given at the moment of being stuck fails with "unknown command".Originally tracked in connect.go points at a command that doesn't exist, and drift check can't catch it. The hint arrived with Add basecamp connect setup: record trust, write connect.json, check readiness and was spotted while building Let an agent connect an agent and manage connector setup: the basecamp-connect skill.
There is no command that binds an account on its own. The account is written by a command that stores a credential:
basecamp auth agent connect -P <name>takes the Agent's own account, and the headless logins (auth login --with-token/--with-client-credentialswith--account) take the one named. A browser login binds nothing. So the hint now names what actually works for the profile in front of it: connecting the Agent, or — for a bot user that already logged in through the browser — addingaccount_idto its entry in the global config. A profile whose accountless entry comes from a repo or local config gets only the config-file remedy, because every binding command refuses it. Anaccount_idthat is set but unusable is now reported as that, not as "unbound".The second half is why it survived review. The skill drift check resolved a referenced command to its nearest existing ancestor, so
basecamp profile setpassed asbasecamp profile. Resolution is now exact. Words past a command still pass where they cannot name a subcommand — argument values, and prose after a leaf — and a word naming a command recorded in.surface-breakingfails first, which is what lets removal be caught at all. A word that runs on into a filename or key is read as the argument it is. The check passes on all three skills, and its behavior is held by fixture tests that fail against the old script.Hints written in Go are not skill files, so they get a test of their own against the real command tree rather than
.surface: the commands named in hints acrossinternal/commandsand the connector's readiness checks — written inline, built into a variable, returned by a helper, passed through a wrapper, or assigned to a hint field — plus every string inconnect.go. It asks a group's own argument validator whether a word can follow it, so it is exact where the script can only approximate, and it fails on the old hint. A pass over the tree found one other reference to a command that doesn't exist, in a comment, which is corrected.What the checks still cannot see is documented in AGENTS.md: a subcommand that never existed, under a command that also takes an argument, reads as the argument.