From 36370015448a725d6651560d81515a6ccb4b97a5 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 10:31:22 +0200 Subject: [PATCH 1/6] Point the account-binding hint at a command that exists, and make the check able to say so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A profile that holds a credential but no account stopped connector setup with "Bind it: basecamp profile set account_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 `, which takes the agent's own account, and `basecamp auth login -P --account ` 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/workflows/test.yml | 3 + .surface-skill-drift | 5 +- AGENTS.md | 19 ++- Makefile | 8 +- internal/commands/connect.go | 6 +- internal/commands/hint_commands_test.go | 155 ++++++++++++++++++++++++ internal/connector/ledger_recovery.go | 4 +- scripts/check-skill-drift.sh | 90 +++++++++++--- scripts/test-skill-drift.sh | 152 +++++++++++++++++++++++ 9 files changed, 417 insertions(+), 25 deletions(-) create mode 100644 internal/commands/hint_commands_test.go create mode 100755 scripts/test-skill-drift.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b9af336cb..24784d681 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -184,6 +184,9 @@ jobs: - name: Test the skills sync run: make test-sync-skills + - name: Test the skill drift check + run: make test-skill-drift + cli-surface: name: CLI Surface Check runs-on: ubuntu-latest diff --git a/.surface-skill-drift b/.surface-skill-drift index 73774ca0a..649560bc4 100644 --- a/.surface-skill-drift +++ b/.surface-skill-drift @@ -1,6 +1,9 @@ -# Known skill drift — skill references flags that don't exist on the surface command. +# Known skill drift — references the surface cannot confirm. # These are pre-existing mismatches, not regressions. Fix in SKILL.md when convenient. # Format: FLAG +# CMD — for a command reference the check refuses, which +# includes a hidden command (never in .surface) and a +# group that runs bare (.surface cannot say it does) FLAG basecamp comments list --in FLAG basecamp comments update --in FLAG basecamp people --in diff --git a/AGENTS.md b/AGENTS.md index 893ccb66c..ba204118d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,10 +80,21 @@ session, stop and run it first. *references* still exist in the `.surface` snapshot. It catches stale references, not missing coverage, so adding a command breaks none of them. -Removal is only partly caught. `resolve_cmd` walks up to the nearest existing ancestor, -so dropping a nested subcommand leaves the reference resolving against its parent and the -check still passes — `basecamp setup ` resolves as `basecamp setup`. Removing a -top-level command is caught; removing a subcommand is not. Don't lean on CI for this. Update the skill the change actually affects; +A reference has to resolve exactly, so removing a nested subcommand is caught too: +`basecamp setup ` no longer passes by falling back to `basecamp setup`. Words +past a resolved command are allowed only where they cannot name a subcommand — after a +command that takes positional arguments, and after a leaf that has none — so argument +values and prose still pass. What `.surface` cannot say is whether a group runs bare +(`basecamp skill` prints the skill file) or only dispatches (`basecamp profile`), and +hidden commands are not in `.surface` at all; acknowledge either in +`.surface-skill-drift` as `CMD `. `make test-skill-drift` runs the check against +fixtures that hold this behavior. + +Hints in Go code are covered by a test rather than by this script: +`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. A hint is an instruction an operator is about to run, so it is held to +the same exactness. Update the skill the change actually affects; basecamp-doctor deliberately covers only doctor, setup and auth remediation; basecamp-connect covers `basecamp auth agent connect` and `basecamp connect`, and its evals run against its own SKILL.md (`make -C skill-evals eval-connect`). diff --git a/Makefile b/Makefile index e70b3ffcf..1636023cf 100644 --- a/Makefile +++ b/Makefile @@ -383,7 +383,7 @@ check-smoke-coverage: build # Run all checks (local CI gate) .PHONY: check -check: fmt-check vet lint lint-actions test test-e2e test-sync-skills check-naming check-surface check-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage provenance-check tidy-check +check: fmt-check vet lint lint-actions test test-e2e test-sync-skills check-naming check-surface check-skill-drift test-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage provenance-check tidy-check # Lint GitHub Actions workflows (requires actionlint + zizmor) .PHONY: lint-actions @@ -459,6 +459,11 @@ check-skill-drift: @scripts/check-skill-drift.sh skills/basecamp-doctor/SKILL.md @scripts/check-skill-drift.sh skills/basecamp-connect/SKILL.md +# Verify the skill drift check itself: what it accepts, and what it refuses +.PHONY: test-skill-drift +test-skill-drift: + @scripts/test-skill-drift.sh + # Verify group commands show help bare (no RunE on parents with subcommands) .PHONY: check-bare-groups check-bare-groups: @@ -622,6 +627,7 @@ help: @echo " update-surface Regenerate committed .surface from the command tree" @echo " check-surface-diff Compare CLI surface snapshots (fails on removals)" @echo " check-skill-drift Verify skill references match CLI surface" + @echo " test-skill-drift Run the skill drift check against its own fixtures" @echo "" @echo "Dependencies:" @echo " update-nix-hash Recompute Nix vendorHash via Docker" diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 3be7c24e4..3f918155d 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -862,8 +862,12 @@ func connectAccount(app *appctx.App, name string) (string, error) { } bound, err := canonicalAccount(p.AccountID) if err != nil { + // The account is bound by the command that stores the credential — + // there is no command that binds one on its own — so the remedy is + // to store it again, which rewrites the entry with an account. return "", output.ErrUsageHint(fmt.Sprintf("Profile %q is not bound to an account", name), - "Bind it: basecamp profile set "+shellQuote(name)+" account_id ") + "Bind it where the credential is stored: basecamp auth agent connect -P "+shellQuote(name)+ + " takes the agent's own account, and on the bot-user path basecamp auth login -P "+shellQuote(name)+" --account binds the one you name.") } if accountGivenExplicitly(app) && !accountIDsEqual(app.Config.AccountID, bound) { return "", output.ErrUsageHint( diff --git a/internal/commands/hint_commands_test.go b/internal/commands/hint_commands_test.go new file mode 100644 index 000000000..2e23a8720 --- /dev/null +++ b/internal/commands/hint_commands_test.go @@ -0,0 +1,155 @@ +package commands_test + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A hint is a command an operator is about to run. `basecamp profile set +// account_id ` sat in connect.go's account-binding hint and was +// not a command at all — `profile` has no `set` — so anyone who followed it +// got "unknown command" at the moment they were already stuck. +// +// So every command named in a hint is resolved here against the real command +// tree. Resolving to the nearest existing ancestor is what let the invented +// subcommand through the skill drift check; this test resolves exactly, and +// allows leftover words only where they cannot be a subcommand: after a +// command that takes positional arguments (they are argument values), or +// after a leaf that has no subcommands (they are prose). +// +// Two scopes, for two reasons. Hint text everywhere in the package, because +// a hint is an instruction. Every string in connect.go, because its help and +// examples are the connector's setup documentation and are followed the same +// way. +func TestHintCommandsResolve(t *testing.T) { + root := buildRootWithAllCommands() + fset := token.NewFileSet() + + entries, err := os.ReadDir(".") + require.NoError(t, err) + + var checked int + for _, entry := range entries { + name := entry.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, filepath.Join(".", name), nil, 0) + require.NoError(t, err) + + for _, lit := range hintLiterals(file, name == "connect.go") { + text, err := strconv.Unquote(lit.Value) + if err != nil { + continue // a raw string with an unquotable escape; nothing to read + } + for _, phrase := range commandPhrases(text) { + checked++ + resolved, word, ok := resolveCommandPhrase(root, phrase) + assert.Truef(t, ok, "%s: hint names %q, and %q has no subcommand %q", + fset.Position(lit.Pos()), phrase, resolved, word) + } + } + } + + // A scan that stopped finding hints would pass silently forever. + assert.Greaterf(t, checked, 40, "only %d command references scanned — the scan has stopped seeing hints", checked) +} + +// hintLiterals returns the string literals in a file that an operator is +// meant to read: the hint argument of the hint-carrying error constructors, +// the Hint field of an output.Error, and — when everything is wanted — every +// string literal in the file. +func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { + var lits []*ast.BasicLit + collect := func(n ast.Node) { + ast.Inspect(n, func(n ast.Node) bool { + if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { + lits = append(lits, lit) + } + return true + }) + } + + if everything { + collect(file) + return lits + } + + // Which argument carries the hint, by constructor name. + hintArg := map[string]int{"ErrUsageHint": 1, "ErrNotFoundHint": 2} + + ast.Inspect(file, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.CallExpr: + sel, ok := node.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + if i, ok := hintArg[sel.Sel.Name]; ok && len(node.Args) > i { + collect(node.Args[i]) + } + case *ast.KeyValueExpr: + if key, ok := node.Key.(*ast.Ident); ok && key.Name == "Hint" { + collect(node.Value) + } + } + return true + }) + return lits +} + +// commandPhrase matches a command reference the way scripts/check-skill-drift.sh +// extracts one, so a hint and a skill are read by the same rule. +var commandPhrase = regexp.MustCompile(`basecamp( [a-z][-a-z0-9]+)+`) + +func commandPhrases(text string) []string { + return commandPhrase.FindAllString(text, -1) +} + +// resolveCommandPhrase walks a command phrase down the tree. It reports the +// deepest command the phrase reaches, the first word past it that command +// does not have, and whether the phrase resolves. +func resolveCommandPhrase(root *cobra.Command, phrase string) (resolved, word string, ok bool) { + words := strings.Fields(phrase) + cmd := root + i := 1 // words[0] is the binary name + for ; i < len(words); i++ { + sub := childNamed(cmd, words[i]) + if sub == nil { + break + } + cmd = sub + } + if i == len(words) { + return cmd.CommandPath(), "", true + } + // Leftover words cannot be an invented subcommand when the command runs + // as it stands — they are its arguments, or prose about running it — nor + // when it has no subcommands they could be mistaken for. What is left is + // a group that only dispatches, where the next word has to be one of the + // subcommands it dispatches to. + if cmd.Runnable() || !cmd.HasSubCommands() { + return cmd.CommandPath(), "", true + } + return cmd.CommandPath(), words[i], false +} + +func childNamed(cmd *cobra.Command, name string) *cobra.Command { + for _, sub := range cmd.Commands() { + if sub.Name() == name || containsAlias(sub, name) { + return sub + } + } + return nil +} diff --git a/internal/connector/ledger_recovery.go b/internal/connector/ledger_recovery.go index 8e82192fd..7c711de59 100644 --- a/internal/connector/ledger_recovery.go +++ b/internal/connector/ledger_recovery.go @@ -344,8 +344,8 @@ func (l *Ledger) MarkUnrecoveredThrough(ctx context.Context, lossID, id int64) ( } // UnrecoveredIDs returns every id the connector has given up on, across all -// losses. `basecamp connect status` and `doctor` show these: an unrecovered id -// is documented, not hidden, and the poll lane may still serve it later. +// losses. Nothing here discards them: an unrecovered id stays on the record, +// and the poll lane may still serve it later. func (l *Ledger) UnrecoveredIDs(ctx context.Context) ([]int64, error) { rows, err := l.db.QueryContext(ctx, `SELECT DISTINCT event_id FROM loss_ids WHERE state = ? ORDER BY event_id`, diff --git a/scripts/check-skill-drift.sh b/scripts/check-skill-drift.sh index d28864501..d6b165f01 100755 --- a/scripts/check-skill-drift.sh +++ b/scripts/check-skill-drift.sh @@ -29,26 +29,69 @@ is_baselined() { [ -f "$BASELINE" ] && grep -qxF "$1" "$BASELINE" } -# Resolve a candidate command path to the longest matching CMD in .surface. -# Prints the match (or nothing). Rejects fallback to bare "basecamp" — -# every extracted candidate has at least one subcommand word, so matching -# only the root means none of the subcommands exist. +# resolve_cmd prints the longest prefix of a candidate that is a CMD in +# .surface, and nothing when no prefix is one. resolve_cmd() { local candidate="$1" read -ra parts <<< "$candidate" for ((i=${#parts[@]}; i>=1; i--)); do local try="${parts[*]:0:$i}" if grep -qxF "CMD $try" "$SURFACE"; then - # Reject bare root — candidate always has subcommand tokens - if [ "$try" = "basecamp" ] && [ "${#parts[@]}" -gt 1 ]; then - return - fi echo "$try" return fi done } +# verify_cmd decides whether a candidate names a command that exists. +# +# Resolving to the nearest existing ancestor is not enough: "basecamp profile +# set" resolved to "basecamp profile" and passed, though "profile" has no +# "set" subcommand. So a candidate has to resolve EXACTLY, unless the words +# past the resolved command can only be argument values or prose — which is +# the case when the resolved command takes positional arguments (it has ARG +# entries), or is a leaf that takes none (no SUB entries, so no word after it +# could ever name a subcommand). +# +# What is left is the case the old fallback swallowed: a group that has +# subcommands, takes no arguments, and is followed by a word that is not one +# of its subcommands. That word is an invented subcommand, or a reference +# this check cannot verify. Either way it fails. +# +# Prints "|" and returns 0 when the +# candidate is verified, 1 when it is not. On a failure the resolved command +# is the deepest one that does exist — empty when not even the root does. +verify_cmd() { + local candidate="$1" matched + read -ra parts <<< "$candidate" + matched=$(resolve_cmd "$candidate") + if [ -z "$matched" ]; then + echo "|${parts[0]}" + return 1 + fi + + read -ra found <<< "$matched" + local depth=${#found[@]} + if [ "$depth" -eq "${#parts[@]}" ]; then + echo "$matched|" + return 0 + fi + + # Leftover words are argument values when the command takes arguments. + if grep -qE "^ARG ${matched} [0-9]{2} " "$SURFACE"; then + echo "$matched|" + return 0 + fi + # Leftover words are prose when the command has no subcommands to name. + if ! grep -qE "^SUB ${matched} [a-z]" "$SURFACE"; then + echo "$matched|" + return 0 + fi + + echo "${matched}|${parts[$depth]}" + return 1 +} + # Check whether a flag exists on a command or any of its subcommands. # Descendant matching is intentional: Cobra commands inherit flags and shortcut # commands delegate to subcommands (e.g. "basecamp cards --in" runs "cards list" @@ -68,14 +111,25 @@ skill_body=$(awk '/^---$/{n++; next} n>=2' "$SKILL") # --- Phase 1: Command references --- # Extract "basecamp ..." patterns, resolve to longest matching CMD. while IFS= read -r candidate; do - matched=$(resolve_cmd "$candidate") - - if [ -z "$matched" ]; then - key="CMD ${candidate}" + status=0 + result=$(verify_cmd "$candidate") || status=$? + + if [ "$status" -ne 0 ]; then + resolved="${result%%|*}" + word="${result#*|}" + if [ -z "$resolved" ] || [ "$resolved" = "basecamp" ]; then + # Nothing past "basecamp" names a command: the whole reference is the + # drift, and the whole reference is what a baseline entry names. + message="command not in surface: $candidate" + key="CMD ${candidate}" + else + message="cannot verify \"$candidate\": ${resolved} has no subcommand \"${word}\"" + key="CMD ${resolved} ${word}" + fi if is_baselined "$key"; then : # known drift else - echo "DRIFT: command not in surface: $candidate" + echo "DRIFT: $message" new_drift=$((new_drift + 1)) fi errors=$((errors + 1)) @@ -99,8 +153,11 @@ while IFS=: read -r lineno line; do continue fi - matched=$(resolve_cmd "$cmd_candidate") - [ -z "$matched" ] && continue + # A command phase 1 already reported is no base for a flag check. + status=0 + result=$(verify_cmd "$cmd_candidate") || status=$? + [ "$status" -ne 0 ] && continue + matched="${result%%|*}" for flag in $(echo "$line" | grep -oE -- '--[a-z][-a-z0-9]*' | sort -u); do # Skip cobra built-ins @@ -128,7 +185,8 @@ if [ $new_drift -gt 0 ]; then echo "" echo "Found ${new_drift} new skill drift issue(s) (${baselined} baselined)." echo "Update ${SKILL} to match the current CLI surface (.surface)," - echo "or add entries to ${BASELINE} if the drift is intentional." + echo "or add entries to ${BASELINE} if the drift is intentional —" + echo "a hidden command is not in .surface, and is baselined as \"CMD \"." exit 1 fi diff --git a/scripts/test-skill-drift.sh b/scripts/test-skill-drift.sh new file mode 100755 index 000000000..8be3c9a74 --- /dev/null +++ b/scripts/test-skill-drift.sh @@ -0,0 +1,152 @@ +#!/usr/bin/env bash +# test-skill-drift.sh — Run scripts/check-skill-drift.sh against fixture skills +# and a fixture surface, and prove it accepts what the CLI really has and +# refuses what it does not. +# +# The case that matters: the check used to resolve a referenced command to its +# nearest existing ancestor, so "basecamp profile set" resolved to "basecamp +# profile" and passed even though "profile" has no "set" subcommand — which is +# how an invented command reached an operator-facing hint. Resolution now has +# to be exact, except where the leftover words can only be argument values or +# prose. The fixtures below hold both halves: the invented subcommand fails, +# and the argument values and prose that used to need the fallback still pass. +# +# Usage: scripts/test-skill-drift.sh +# DRIFT_SCRIPT=path/to/check-skill-drift.sh scripts/test-skill-drift.sh + +set -uo pipefail + +here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +DRIFT_SCRIPT="${DRIFT_SCRIPT:-${here}/check-skill-drift.sh}" +[[ -x "$DRIFT_SCRIPT" ]] || { echo "ERROR: ${DRIFT_SCRIPT} is not an executable script" >&2; exit 1; } + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +failures=0 +ok() { echo "ok - $*"; } +not_ok() { echo "not ok - $*"; failures=$((failures + 1)); } + +# A surface small enough to read whole. It carries one group with subcommands +# and no arguments (profile), one group that takes an argument (recordings), +# one leaf (todos list), and the flag that leaf owns. +cat > "${work}/surface" <<'EOF' +ARG basecamp recordings 00 [type] +CMD basecamp +CMD basecamp profile +CMD basecamp profile list +CMD basecamp recordings +CMD basecamp recordings archive +CMD basecamp todos +CMD basecamp todos list +FLAG basecamp todos list --project type=string +SUB basecamp profile +SUB basecamp recordings +SUB basecamp todos +SUB basecamp profile list +SUB basecamp recordings archive +SUB basecamp todos list +EOF + +: > "${work}/baseline" + +# write_skill ... +write_skill() { + local name="$1"; shift + { + printf -- '---\nname: %s\n---\n\n' "$name" + printf '%s\n' "$@" + } > "${work}/${name}.md" +} + +# run_check [baseline file] — prints output, sets $status +run_check() { + local name="$1" baseline="${2:-${work}/baseline}" + out=$("$DRIFT_SCRIPT" "${work}/${name}.md" "${work}/surface" "$baseline" 2>&1) + status=$? +} + +# assert_passes +assert_passes() { + run_check "$1" + if [ "$status" -eq 0 ]; then ok "$2"; else not_ok "$2 — exited ${status}: ${out}"; fi +} + +# assert_fails +assert_fails() { + run_check "$1" + if [ "$status" -eq 0 ]; then + not_ok "$3 — the check passed: ${out}" + elif [[ "$out" != *"$2"* ]]; then + not_ok "$3 — expected ${2@Q} in: ${out}" + else + ok "$3" + fi +} + +# --- The invented subcommand the nearest-ancestor fallback used to swallow --- + +write_skill invented 'Bind it: `basecamp profile set agent account_id 123`' +assert_fails invented 'cannot verify' \ + 'an invented subcommand under an existing group fails' + +# The report says which command was resolved and which word it does not have, +# so the reader is not left to guess what "cannot verify" means. +run_check invented +if [[ "$out" == *'basecamp profile'* && "$out" == *'"set"'* ]]; then + ok 'the report names the resolved command and the word it does not have' +else + not_ok "the report names the resolved command and the word it does not have — got: ${out}" +fi + +# --- What the fallback existed for, and still has to pass --- + +write_skill exact 'Run `basecamp profile list` and `basecamp recordings archive`.' +assert_passes exact 'an exact command passes' + +write_skill argvalue 'Run `basecamp recordings cards` to list the cards.' +assert_passes argvalue 'an argument value after a command that takes arguments passes' + +write_skill prose 'Run `basecamp todos list` without a project and it fails.' +assert_passes prose 'prose after a leaf command passes' + +# --- What already failed, and still has to --- + +write_skill unknown 'Run `basecamp bogus thing` for this.' +assert_fails unknown 'command not in surface' \ + 'a command with no known prefix fails' + +write_skill flagdrift 'Run `basecamp todos list --nope`.' +assert_fails flagdrift 'flag --nope' \ + 'a flag the command does not have fails' + +write_skill goodflag 'Run `basecamp todos list --project 123`.' +assert_passes goodflag 'a flag the command does have passes' + +# --- The baseline still suppresses drift, now for commands too --- + +printf 'CMD basecamp profile set\n' > "${work}/baselined" +run_check invented "${work}/baselined" +if [ "$status" -eq 0 ]; then + ok 'a baselined command reference is not new drift' +else + not_ok "a baselined command reference is not new drift — exited ${status}: ${out}" +fi + +# --- One report per bad command, not one per flag on its line --- + +write_skill invented_flag 'Run `basecamp profile set agent --nope --also-nope`.' +run_check invented_flag +count=$(printf '%s\n' "$out" | grep -c '^DRIFT:') +if [ "$count" -eq 1 ]; then + ok 'a command that fails is not also reported once per flag' +else + not_ok "a command that fails is not also reported once per flag — ${count} DRIFT lines: ${out}" +fi + +echo +if [ "$failures" -gt 0 ]; then + echo "${failures} failure(s)" + exit 1 +fi +echo "All skill drift checks passed" From 4faf938e0d3ab46534400be47d850f0636fbbca9 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 10:43:32 +0200 Subject: [PATCH 2/6] Close the review comments: layered profiles, variable-built hints, removed subcommands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 27 +++---- internal/commands/connect.go | 36 ++++++++-- internal/commands/connect_unbound_test.go | 75 ++++++++++++++++++++ internal/commands/hint_commands_test.go | 86 +++++++++++++++++++++-- scripts/check-skill-drift.sh | 20 +++++- scripts/test-skill-drift.sh | 20 +++++- 6 files changed, 238 insertions(+), 26 deletions(-) create mode 100644 internal/commands/connect_unbound_test.go diff --git a/AGENTS.md b/AGENTS.md index ba204118d..17ffab270 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,21 +80,24 @@ session, stop and run it first. *references* still exist in the `.surface` snapshot. It catches stale references, not missing coverage, so adding a command breaks none of them. -A reference has to resolve exactly, so removing a nested subcommand is caught too: -`basecamp setup ` no longer passes by falling back to `basecamp setup`. Words -past a resolved command are allowed only where they cannot name a subcommand — after a -command that takes positional arguments, and after a leaf that has none — so argument -values and prose still pass. What `.surface` cannot say is whether a group runs bare -(`basecamp skill` prints the skill file) or only dispatches (`basecamp profile`), and -hidden commands are not in `.surface` at all; acknowledge either in -`.surface-skill-drift` as `CMD `. `make test-skill-drift` runs the check against -fixtures that hold this behavior. +A reference has to resolve exactly: `basecamp setup ` no longer passes by +falling back to `basecamp setup`. Words past a resolved command are allowed only where +they cannot name a subcommand — after a command that takes positional arguments, and +after a leaf that has none — so argument values and prose still pass. Those two escapes +cannot tell an argument value from a subcommand we dropped, so a word naming a command +in `.surface-breaking` fails before either escape can read it as prose; that file is the +record of removals, which is why removal is caught at all. What `.surface` cannot say is +whether a group runs bare (`basecamp skill` prints the skill file) or only dispatches +(`basecamp profile`), and hidden commands are not in `.surface` at all; acknowledge +either in `.surface-skill-drift` as `CMD `. `make test-skill-drift` runs the check +against fixtures that hold all of this. Hints in Go code are covered by a test rather than by this script: `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. A hint is an instruction an operator is about to run, so it is held to -the same exactness. Update the skill the change actually affects; +`internal/commands` — including hints built into a variable first — and every command +named in any string in `connect.go`, against the real command tree, by the same rule. +A hint is an instruction an operator is about to run, so it is held to the same +exactness. Update the skill the change actually affects; basecamp-doctor deliberately covers only doctor, setup and auth remediation; basecamp-connect covers `basecamp auth agent connect` and `basecamp connect`, and its evals run against its own SKILL.md (`make -C skill-evals eval-connect`). diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 3f918155d..cfbf3cea9 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -862,12 +862,12 @@ func connectAccount(app *appctx.App, name string) (string, error) { } bound, err := canonicalAccount(p.AccountID) if err != nil { - // The account is bound by the command that stores the credential — - // there is no command that binds one on its own — so the remedy is - // to store it again, which rewrites the entry with an account. - return "", output.ErrUsageHint(fmt.Sprintf("Profile %q is not bound to an account", name), - "Bind it where the credential is stored: basecamp auth agent connect -P "+shellQuote(name)+ - " takes the agent's own account, and on the bot-user path basecamp auth login -P "+shellQuote(name)+" --account binds the one you name.") + // An entry that names an account nothing can use is not an unbound + // profile: say what is wrong with the account it does name. + if p.AccountID != "" { + return "", err + } + return "", unboundProfileError(name) } if accountGivenExplicitly(app) && !accountIDsEqual(app.Config.AccountID, bound) { return "", output.ErrUsageHint( @@ -877,6 +877,30 @@ func connectAccount(app *appctx.App, name string) (string, error) { return bound, nil } +// unboundProfileError refuses a profile that names no account, with the +// remedy that will work for this one. +// +// Nothing binds an account on its own — `profile` has no command for it. The +// account is written by whichever command stores the credential, so the +// remedy is normally to store it again. But both of those refuse an +// accountless entry that is not the global config's, because the entry they +// would write would stay shadowed by the system, repo or local config that +// defines it (auth.go and auth_agent.go both say so). For that profile the +// only remedy is the file the entry comes from. +func unboundProfileError(name string) error { + message := fmt.Sprintf("Profile %q is not bound to an account", name) + profile := shellQuote(name) + // A global config that cannot be read is no reason to fail here: the + // commands named below report that themselves, in full. + if unbound, err := globalProfileIsUnbound(name); err == nil && !unbound { + return output.ErrUsageHint(message, + "This entry is not the global config's, so storing the credential again cannot bind it. Add account_id to the config file that defines it.") + } + return output.ErrUsageHint(message, + "Bind it where the credential is stored: basecamp auth agent connect -P "+profile+ + " takes the agent's own account, and on the bot-user path basecamp auth login -P "+profile+" --account binds the one you name.") +} + // connectCredentialKind is the kind of credential the active profile holds, // "" for none. A store that cannot be read is an error, not "none". func connectCredentialKind(ctx context.Context, app *appctx.App) (string, error) { diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go new file mode 100644 index 000000000..42d480a09 --- /dev/null +++ b/internal/commands/connect_unbound_test.go @@ -0,0 +1,75 @@ +//go:build unix + +package commands + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// writeGlobalConfig puts a config file where GlobalConfigDir will find it, +// and returns nothing: the test reads the hint, not the file. +func writeGlobalConfig(t *testing.T, contents string) { + t.Helper() + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + dir := filepath.Join(home, "basecamp") + require.NoError(t, os.MkdirAll(dir, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.json"), []byte(contents), 0o600)) +} + +// hintOf is the hint an error carries for the operator to act on. +func hintOf(t *testing.T, err error) string { + t.Helper() + var e *output.Error + require.ErrorAs(t, err, &e) + return e.Hint +} + +// The remedy for an accountless profile is the command that stores the +// credential, because that is what writes the account — there is no command +// that binds one on its own. +func TestUnboundProfileHintNamesTheCommandThatBinds(t *testing.T) { + writeGlobalConfig(t, `{"profiles":{"agent":{"base_url":"https://3.basecampapi.com"}}}`) + + hint := hintOf(t, unboundProfileError("agent")) + + assert.Contains(t, hint, "basecamp auth agent connect -P agent") + assert.Contains(t, hint, "basecamp auth login -P agent --account ") +} + +// Storing the credential again cannot bind a profile whose accountless entry +// comes from a system, repo or local config: both login paths refuse it, +// because the global entry they would write stays shadowed. Sending the +// operator there would be another command that fails on them. +func TestUnboundProfileHintNamesTheConfigFileWhenTheEntryIsNotGlobal(t *testing.T) { + writeGlobalConfig(t, `{"profiles":{"other":{"account_id":"999"}}}`) + + hint := hintOf(t, unboundProfileError("agent")) + + assert.Contains(t, hint, "account_id to the config file that defines it") + assert.NotContains(t, hint, "basecamp auth", "a command that would refuse this profile is no remedy") +} + +// A profile that names an account nothing can parse is not an unbound one, +// and calling it unbound sends the operator to bind an account it already +// has. The account it does name is what is wrong. +func TestConnectAccountReportsAnUnusableAccountAsItself(t *testing.T) { + app := &appctx.App{Config: &config.Config{ + Profiles: map[string]*config.ProfileConfig{"agent": {AccountID: "not-a-number"}}, + }} + + _, err := connectAccount(app, "agent") + + require.Error(t, err) + assert.Contains(t, err.Error(), `Invalid account ID "not-a-number"`) + assert.NotContains(t, err.Error(), "not bound") +} diff --git a/internal/commands/hint_commands_test.go b/internal/commands/hint_commands_test.go index 2e23a8720..504431aad 100644 --- a/internal/commands/hint_commands_test.go +++ b/internal/commands/hint_commands_test.go @@ -34,6 +34,7 @@ import ( // way. func TestHintCommandsResolve(t *testing.T) { root := buildRootWithAllCommands() + removed := removedCommands(t) fset := token.NewFileSet() entries, err := os.ReadDir(".") @@ -55,9 +56,13 @@ func TestHintCommandsResolve(t *testing.T) { } for _, phrase := range commandPhrases(text) { checked++ - resolved, word, ok := resolveCommandPhrase(root, phrase) - assert.Truef(t, ok, "%s: hint names %q, and %q has no subcommand %q", - fset.Position(lit.Pos()), phrase, resolved, word) + resolved, word, ok := resolveCommandPhrase(root, removed, phrase) + reason := "has no subcommand" + if removed[resolved+" "+word] { + reason = "no longer has" + } + assert.Truef(t, ok, "%s: hint names %q, and %q %s %q", + fset.Position(lit.Pos()), phrase, resolved, reason, word) } } } @@ -70,12 +75,25 @@ func TestHintCommandsResolve(t *testing.T) { // meant to read: the hint argument of the hint-carrying error constructors, // the Hint field of an output.Error, and — when everything is wanted — every // string literal in the file. +// +// A hint is not always written where it is passed: some are built into a +// variable first (boost.go builds one, then adds --event to it). So the +// names a hint expression mentions are collected too, and a second pass +// takes the literals assigned to them anywhere in the file. Over-reaching +// there costs nothing: a literal with no command in it is scanned and +// passes. func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { var lits []*ast.BasicLit + names := map[string]bool{} collect := func(n ast.Node) { ast.Inspect(n, func(n ast.Node) bool { - if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { - lits = append(lits, lit) + switch node := n.(type) { + case *ast.BasicLit: + if node.Kind == token.STRING { + lits = append(lits, node) + } + case *ast.Ident: + names[node.Name] = true } return true }) @@ -106,9 +124,62 @@ func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { } return true }) + + // Second pass: whatever was assigned to a name a hint is built from. + ast.Inspect(file, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.AssignStmt: + for _, lhs := range node.Lhs { + if id, ok := lhs.(*ast.Ident); ok && names[id.Name] { + for _, rhs := range node.Rhs { + collectStrings(rhs, &lits) + } + break + } + } + case *ast.ValueSpec: + for _, id := range node.Names { + if names[id.Name] { + for _, v := range node.Values { + collectStrings(v, &lits) + } + break + } + } + } + return true + }) return lits } +func collectStrings(n ast.Node, into *[]*ast.BasicLit) { + ast.Inspect(n, func(n ast.Node) bool { + if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { + *into = append(*into, lit) + } + return true + }) +} + +// removedCommands is every command path .surface-breaking records the CLI as +// having dropped. A word past a resolved command that names one of these is +// drift whatever else it could be — without it, "basecamp recordings archive" +// reads as the [type] argument the moment `archive` is removed, which is the +// hole the nearest-ancestor fallback used to leave everywhere. +func removedCommands(t *testing.T) map[string]bool { + t.Helper() + data, err := os.ReadFile("../../.surface-breaking") + require.NoError(t, err) + + removed := map[string]bool{} + for _, line := range strings.Split(string(data), "\n") { + if path, ok := strings.CutPrefix(strings.TrimSpace(line), "CMD "); ok { + removed[path] = true + } + } + return removed +} + // commandPhrase matches a command reference the way scripts/check-skill-drift.sh // extracts one, so a hint and a skill are read by the same rule. var commandPhrase = regexp.MustCompile(`basecamp( [a-z][-a-z0-9]+)+`) @@ -120,7 +191,7 @@ func commandPhrases(text string) []string { // resolveCommandPhrase walks a command phrase down the tree. It reports the // deepest command the phrase reaches, the first word past it that command // does not have, and whether the phrase resolves. -func resolveCommandPhrase(root *cobra.Command, phrase string) (resolved, word string, ok bool) { +func resolveCommandPhrase(root *cobra.Command, removed map[string]bool, phrase string) (resolved, word string, ok bool) { words := strings.Fields(phrase) cmd := root i := 1 // words[0] is the binary name @@ -134,6 +205,9 @@ func resolveCommandPhrase(root *cobra.Command, phrase string) (resolved, word st if i == len(words) { return cmd.CommandPath(), "", true } + if removed[cmd.CommandPath()+" "+words[i]] { + return cmd.CommandPath(), words[i], false + } // Leftover words cannot be an invented subcommand when the command runs // as it stands — they are its arguments, or prose about running it — nor // when it has no subcommands they could be mistaken for. What is left is diff --git a/scripts/check-skill-drift.sh b/scripts/check-skill-drift.sh index d6b165f01..a8e5b477e 100755 --- a/scripts/check-skill-drift.sh +++ b/scripts/check-skill-drift.sh @@ -6,6 +6,9 @@ set -euo pipefail SKILL="${1:-skills/basecamp/SKILL.md}" SURFACE="${2:-.surface}" BASELINE="${3:-.surface-skill-drift}" +# Removals are acknowledged in .surface-breaking before they can land, so it +# is the record of what the CLI used to have and no longer does. +BREAKING="${4:-.surface-breaking}" # Built-in flags not tracked in the surface BUILTINS="--help --version" @@ -58,6 +61,11 @@ resolve_cmd() { # of its subcommands. That word is an invented subcommand, or a reference # this check cannot verify. Either way it fails. # +# Those two escapes cannot tell an argument value from a subcommand we +# removed — "basecamp recordings archive" reads as the [type] argument once +# `archive` is gone — so a word that names a removed command fails first, +# whichever escape would otherwise have taken it. +# # Prints "|" and returns 0 when the # candidate is verified, 1 when it is not. On a failure the resolved command # is the deepest one that does exist — empty when not even the root does. @@ -77,6 +85,12 @@ verify_cmd() { return 0 fi + # A word that names a command we removed is drift whatever else it could + # be, so it is refused before the escapes below could read it as prose. + if [ -f "$BREAKING" ] && grep -qxF "CMD ${matched} ${parts[$depth]}" "$BREAKING"; then + echo "${matched}|${parts[$depth]}" + return 1 + fi # Leftover words are argument values when the command takes arguments. if grep -qE "^ARG ${matched} [0-9]{2} " "$SURFACE"; then echo "$matched|" @@ -123,7 +137,11 @@ while IFS= read -r candidate; do message="command not in surface: $candidate" key="CMD ${candidate}" else - message="cannot verify \"$candidate\": ${resolved} has no subcommand \"${word}\"" + if grep -qxF "CMD ${resolved} ${word}" "$BREAKING" 2>/dev/null; then + message="removed command: \"$candidate\" — ${resolved} no longer has \"${word}\"" + else + message="cannot verify \"$candidate\": ${resolved} has no subcommand \"${word}\"" + fi key="CMD ${resolved} ${word}" fi if is_baselined "$key"; then diff --git a/scripts/test-skill-drift.sh b/scripts/test-skill-drift.sh index 8be3c9a74..33eda3d6a 100755 --- a/scripts/test-skill-drift.sh +++ b/scripts/test-skill-drift.sh @@ -50,6 +50,14 @@ EOF : > "${work}/baseline" +# What the CLI used to have. Removals reach .surface-breaking before they can +# land, so the check reads it to tell an argument value from a command we +# dropped. +cat > "${work}/breaking" <<'EOF' +CMD basecamp recordings vaults +CMD basecamp todos list stale +EOF + # write_skill ... write_skill() { local name="$1"; shift @@ -62,7 +70,7 @@ write_skill() { # run_check [baseline file] — prints output, sets $status run_check() { local name="$1" baseline="${2:-${work}/baseline}" - out=$("$DRIFT_SCRIPT" "${work}/${name}.md" "${work}/surface" "$baseline" 2>&1) + out=$("$DRIFT_SCRIPT" "${work}/${name}.md" "${work}/surface" "$baseline" "${work}/breaking" 2>&1) status=$? } @@ -110,6 +118,16 @@ assert_passes argvalue 'an argument value after a command that takes arguments p write_skill prose 'Run `basecamp todos list` without a project and it fails.' assert_passes prose 'prose after a leaf command passes' +# --- A subcommand we removed, where an escape would otherwise read it as prose --- + +write_skill removed_under_args 'Run `basecamp recordings vaults` for the vaults.' +assert_fails removed_under_args 'removed command' \ + 'a removed subcommand fails even where the parent takes an argument' + +write_skill removed_under_leaf 'Run `basecamp todos list stale` for the stale ones.' +assert_fails removed_under_leaf 'removed command' \ + 'a removed subcommand fails even under a command with no subcommands left' + # --- What already failed, and still has to --- write_skill unknown 'Run `basecamp bogus thing` for this.' From 4551d2c4a2cb6906cff3736d402443629b2a697f Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 11:01:50 +0200 Subject: [PATCH 3/6] Name only what binds, and make both checks see what they were missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hint still named `basecamp auth login -P --account ` 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. --- .surface-skill-drift | 8 +- AGENTS.md | 38 +-- internal/commands/connect.go | 40 +++- internal/commands/connect_unbound_test.go | 15 +- internal/commands/hint_commands_test.go | 279 ++++++++++++++++------ scripts/check-skill-drift.sh | 94 ++++++-- scripts/test-skill-drift.sh | 37 +++ 7 files changed, 374 insertions(+), 137 deletions(-) diff --git a/.surface-skill-drift b/.surface-skill-drift index 649560bc4..3fb75345c 100644 --- a/.surface-skill-drift +++ b/.surface-skill-drift @@ -1,9 +1,11 @@ # Known skill drift — references the surface cannot confirm. # These are pre-existing mismatches, not regressions. Fix in SKILL.md when convenient. # Format: FLAG -# CMD — for a command reference the check refuses, which -# includes a hidden command (never in .surface) and a -# group that runs bare (.surface cannot say it does) +# CMD — a command reference the check refuses: the +# deepest command that exists and the word past it, exactly as the +# DRIFT line's "baseline entry" names it. This covers a hidden command +# (never in .surface) and a group that also runs bare (.surface cannot +# say it does). FLAG basecamp comments list --in FLAG basecamp comments update --in FLAG basecamp people --in diff --git a/AGENTS.md b/AGENTS.md index 17ffab270..fed67bc90 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,25 +83,33 @@ missing coverage, so adding a command breaks none of them. A reference has to resolve exactly: `basecamp setup ` no longer passes by falling back to `basecamp setup`. Words past a resolved command are allowed only where they cannot name a subcommand — after a command that takes positional arguments, and -after a leaf that has none — so argument values and prose still pass. Those two escapes -cannot tell an argument value from a subcommand we dropped, so a word naming a command -in `.surface-breaking` fails before either escape can read it as prose; that file is the -record of removals, which is why removal is caught at all. What `.surface` cannot say is -whether a group runs bare (`basecamp skill` prints the skill file) or only dispatches -(`basecamp profile`), and hidden commands are not in `.surface` at all; acknowledge -either in `.surface-skill-drift` as `CMD `. `make test-skill-drift` runs the check -against fixtures that hold all of this. - -Hints in Go code are covered by a test rather than by this script: -`TestHintCommandsResolve` resolves every command named in a hint across -`internal/commands` — including hints built into a variable first — and every command -named in any string in `connect.go`, against the real command tree, by the same rule. -A hint is an instruction an operator is about to run, so it is held to the same -exactness. Update the skill the change actually affects; +after a leaf that has none — so argument values and prose still pass; a word that runs +on into a filename or key (`upload report.pdf`, `config set project_id`) is read as the +argument it is. A word naming a command in `.surface-breaking`, the record of removals, +fails before either escape can take it, which is how removal is caught. Two limits +remain. Under a command that has both arguments and subcommands (`basecamp recordings`), +a subcommand that never existed passes as an argument value. And `.surface` cannot say +whether a group also runs bare (`basecamp skill` prints the skill file), so a word after +one fails even where the CLI would accept it; hidden commands are not in `.surface` at +all. Acknowledge a deliberate case with the baseline entry its DRIFT line names, in +`.surface-skill-drift`. `make test-skill-drift` runs the check against fixtures that hold +all of this. + +Update the skill the change actually affects; basecamp-doctor deliberately covers only doctor, setup and auth remediation; basecamp-connect covers `basecamp auth agent connect` and `basecamp connect`, and its evals run against its own SKILL.md (`make -C skill-evals eval-connect`). +**Hint commands**: a hint is an instruction an operator is about to run, so +`TestHintCommandsResolve` holds the commands hints name to the same exactness, against +the real command tree rather than `.surface`. It reads hint text in `internal/commands` +and `internal/connector/setup` — hints written inline, built into a variable, returned +by a helper, or assigned to a hint-named field — and every string in `connect.go`. Asking +the command lets it be exact where the script cannot: a word after a group passes only +if the group runs and its own argument validator accepts the word — which a subcommand +that never existed can still satisfy, under a group like `recordings` that takes one. It +checks commands, not flags. + ```bash bin/ci # The single command — run this ``` diff --git a/internal/commands/connect.go b/internal/commands/connect.go index cfbf3cea9..3d1f5d114 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "os" + "path/filepath" "runtime" "strconv" "strings" @@ -865,7 +866,9 @@ func connectAccount(app *appctx.App, name string) (string, error) { // An entry that names an account nothing can use is not an unbound // profile: say what is wrong with the account it does name. if p.AccountID != "" { - return "", err + return "", output.ErrUsageHint( + fmt.Sprintf("Profile %q names account %q, which is not an account ID", name, p.AccountID), + "Correct account_id in the config file that defines the profile.") } return "", unboundProfileError(name) } @@ -880,25 +883,36 @@ func connectAccount(app *appctx.App, name string) (string, error) { // unboundProfileError refuses a profile that names no account, with the // remedy that will work for this one. // -// Nothing binds an account on its own — `profile` has no command for it. The -// account is written by whichever command stores the credential, so the -// remedy is normally to store it again. But both of those refuse an -// accountless entry that is not the global config's, because the entry they -// would write would stay shadowed by the system, repo or local config that -// defines it (auth.go and auth_agent.go both say so). For that profile the -// only remedy is the file the entry comes from. +// No command binds an account on its own — `profile` has none for it. Of the +// commands that store a credential, only the headless ones write the account +// too, and of those only `auth agent connect` fits here: it takes the +// Agent's own account. A bot user logs in through the browser, and that login +// binds nothing, so a bot's account goes into its config entry by hand. +// +// Even `auth agent connect` refuses an accountless entry that is not the +// global config's, because the entry it would write stays shadowed by the +// system, repo or local config that defines it. For that profile the only +// remedy is the file the entry comes from. func unboundProfileError(name string) error { message := fmt.Sprintf("Profile %q is not bound to an account", name) - profile := shellQuote(name) + // The same question `auth agent connect` asks itself, so the hint is + // right whenever that command is: it takes an unbound global entry as + // its own to write. Where that reading is wrong — a trusted repo or + // local config defining the same accountless profile, which keeps + // shadowing the global entry after it is written — it is wrong for the + // command too, and telling them apart needs per-profile provenance the + // config layer does not keep. + // // A global config that cannot be read is no reason to fail here: the - // commands named below report that themselves, in full. + // command named below reports that itself, in full. if unbound, err := globalProfileIsUnbound(name); err == nil && !unbound { return output.ErrUsageHint(message, - "This entry is not the global config's, so storing the credential again cannot bind it. Add account_id to the config file that defines it.") + "The entry is not the global config's, so no command can bind it. Add account_id to the config file that defines it.") } + global := filepath.Join(config.GlobalConfigDir(), "config.json") return output.ErrUsageHint(message, - "Bind it where the credential is stored: basecamp auth agent connect -P "+profile+ - " takes the agent's own account, and on the bot-user path basecamp auth login -P "+profile+" --account binds the one you name.") + "For an Agent, connecting it binds its own account: basecamp auth agent connect -P "+shellQuote(name)+ + ". A bot user's login binds none, so for a bot add account_id to the profile's entry in "+richtext.SanitizeSingleLine(global)+".") } // connectCredentialKind is the kind of credential the active profile holds, diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index 42d480a09..0107a837d 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -34,16 +34,18 @@ func hintOf(t *testing.T, err error) string { return e.Hint } -// The remedy for an accountless profile is the command that stores the -// credential, because that is what writes the account — there is no command -// that binds one on its own. -func TestUnboundProfileHintNamesTheCommandThatBinds(t *testing.T) { +// No command binds an account on its own. Connecting an Agent writes its +// account; a bot user's browser login writes none — `auth login --account` +// binds only on the headless paths — so a bot's remedy is its config entry, +// and naming `auth login` would send the operator round the same loop. +func TestUnboundProfileHintNamesOnlyWhatBinds(t *testing.T) { writeGlobalConfig(t, `{"profiles":{"agent":{"base_url":"https://3.basecampapi.com"}}}`) hint := hintOf(t, unboundProfileError("agent")) assert.Contains(t, hint, "basecamp auth agent connect -P agent") - assert.Contains(t, hint, "basecamp auth login -P agent --account ") + assert.Contains(t, hint, "add account_id to the profile's entry in "+filepath.Join(config.GlobalConfigDir(), "config.json")) + assert.NotContains(t, hint, "auth login", "a browser login binds no account") } // Storing the credential again cannot bind a profile whose accountless entry @@ -70,6 +72,7 @@ func TestConnectAccountReportsAnUnusableAccountAsItself(t *testing.T) { _, err := connectAccount(app, "agent") require.Error(t, err) - assert.Contains(t, err.Error(), `Invalid account ID "not-a-number"`) + assert.Contains(t, err.Error(), `Profile "agent" names account "not-a-number"`) assert.NotContains(t, err.Error(), "not bound") + assert.Contains(t, hintOf(t, err), "Correct account_id") } diff --git a/internal/commands/hint_commands_test.go b/internal/commands/hint_commands_test.go index 504431aad..e666907f1 100644 --- a/internal/commands/hint_commands_test.go +++ b/internal/commands/hint_commands_test.go @@ -22,70 +22,132 @@ import ( // got "unknown command" at the moment they were already stuck. // // So every command named in a hint is resolved here against the real command -// tree. Resolving to the nearest existing ancestor is what let the invented -// subcommand through the skill drift check; this test resolves exactly, and -// allows leftover words only where they cannot be a subcommand: after a -// command that takes positional arguments (they are argument values), or -// after a leaf that has no subcommands (they are prose). +// tree, exactly. Leftover words past the deepest command a phrase reaches are +// allowed only where the CLI itself would not read them as a subcommand: past +// a leaf (nothing to mistake them for), or past a command that runs and whose +// own argument validator accepts the word. A word naming a command +// .surface-breaking records as removed fails whatever else it could be. // -// Two scopes, for two reasons. Hint text everywhere in the package, because -// a hint is an instruction. Every string in connect.go, because its help and -// examples are the connector's setup documentation and are followed the same -// way. +// This is stricter than scripts/check-skill-drift.sh can be, because it asks +// the command rather than .surface: `basecamp setup agentz` fails both, but +// only this test knows `basecamp skill anything` really runs. +// +// Scopes: hint text in this package and in internal/connector/setup, whose +// readiness checks write the hints `connect setup` shows; and every string in +// connect.go, whose help and examples are the connector's setup +// documentation and are followed the same way. Commands only — a hint's +// flags are not checked here. func TestHintCommandsResolve(t *testing.T) { root := buildRootWithAllCommands() removed := removedCommands(t) fset := token.NewFileSet() - entries, err := os.ReadDir(".") + type reference struct{ file, phrase string } + found := map[reference]bool{} + seen := map[token.Pos]bool{} + + for _, dir := range []string{".", "../connector/setup"} { + files, funcs := parsePackage(t, fset, dir) + for name, file := range files { + everything := dir == "." && name == "connect.go" + for _, lit := range hintLiterals(file, funcs, everything) { + if seen[lit.Pos()] { + continue // a helper reached from more than one hint + } + seen[lit.Pos()] = true + + text, err := strconv.Unquote(lit.Value) + require.NoError(t, err, "a string literal that compiled unquotes") + for _, phrase := range commandPhrases(text) { + pos := fset.Position(lit.Pos()) + found[reference{filepath.Base(pos.Filename), phrase}] = true + + resolved, word, ok := resolveCommandPhrase(root, removed, phrase) + reason := "has no subcommand" + if removed[resolved+" "+word] { + reason = "no longer has" + } + assert.Truef(t, ok, "%s: hint names %q, and %q %s %q", pos, phrase, resolved, reason, word) + } + } + } + } + + // A scan that stops seeing a kind of hint passes silently forever, so one + // known reference per kind has to be found. + for _, want := range []struct { + reference + kind string + }{ + {reference{"connect.go", "basecamp auth agent connect"}, "every string in connect.go"}, + {reference{"auth_agent.go", "basecamp auth login"}, "an ErrUsageHint argument"}, + {reference{"connect.go", "basecamp auth login"}, "a Hint field in a composite literal"}, + {reference{"boost.go", "basecamp boost list"}, "a hint built into a variable"}, + {reference{"wizard.go", "basecamp auth status"}, "a hint returned by a helper"}, + {reference{"doctor.go", "basecamp upgrade"}, "an assignment to a Hint field"}, + {reference{"feed.go", "basecamp events poll"}, "a hint-named field"}, + {reference{"checks.go", "basecamp connect setup"}, "a readiness check's hint"}, + } { + assert.Truef(t, found[want.reference], "no longer sees %s: %q in %s", want.kind, want.phrase, want.file) + } +} + +// parsePackage parses every non-test file in dir, and indexes the functions +// they declare by name so a hint built by a helper can be followed into it. +func parsePackage(t *testing.T, fset *token.FileSet, dir string) (map[string]*ast.File, map[string]*ast.FuncDecl) { + t.Helper() + entries, err := os.ReadDir(dir) require.NoError(t, err) - var checked int + files := map[string]*ast.File{} + funcs := map[string]*ast.FuncDecl{} for _, entry := range entries { name := entry.Name() if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { continue } - file, err := parser.ParseFile(fset, filepath.Join(".", name), nil, 0) + file, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, 0) require.NoError(t, err) + files[name] = file - for _, lit := range hintLiterals(file, name == "connect.go") { - text, err := strconv.Unquote(lit.Value) - if err != nil { - continue // a raw string with an unquotable escape; nothing to read - } - for _, phrase := range commandPhrases(text) { - checked++ - resolved, word, ok := resolveCommandPhrase(root, removed, phrase) - reason := "has no subcommand" - if removed[resolved+" "+word] { - reason = "no longer has" - } - assert.Truef(t, ok, "%s: hint names %q, and %q %s %q", - fset.Position(lit.Pos()), phrase, resolved, reason, word) + for _, decl := range file.Decls { + // Methods are keyed by name too: a name collision only widens + // what is scanned. + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Body != nil { + funcs[fn.Name.Name] = fn } } } + require.NotEmpty(t, files, dir) + return files, funcs +} - // A scan that stopped finding hints would pass silently forever. - assert.Greaterf(t, checked, 40, "only %d command references scanned — the scan has stopped seeing hints", checked) +// isHintName reports whether an identifier names hint text: Hint, +// forbiddenHint, agentReadsRefusedHint, hint. +func isHintName(name string) bool { + return strings.HasSuffix(strings.ToLower(name), "hint") } // hintLiterals returns the string literals in a file that an operator is // meant to read: the hint argument of the hint-carrying error constructors, -// the Hint field of an output.Error, and — when everything is wanted — every -// string literal in the file. +// anything written to a hint-named field, variable or constant, and — when +// everything is wanted — every string literal in the file. // -// A hint is not always written where it is passed: some are built into a -// variable first (boost.go builds one, then adds --event to it). So the -// names a hint expression mentions are collected too, and a second pass -// takes the literals assigned to them anywhere in the file. Over-reaching -// there costs nothing: a literal with no command in it is scanned and -// passes. -func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { +// A hint is not always written where it is passed. Some are built into a +// variable first (boost.go builds one, then appends --event), and some are +// returned by a helper (wizard.go passes wizardEscapeHint()). So the names a +// hint expression mentions are followed to their assignments, and the +// functions it calls are followed into their bodies. Over-reaching there +// costs nothing: a literal with no command in it is scanned and passes. +func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, everything bool) []*ast.BasicLit { var lits []*ast.BasicLit names := map[string]bool{} - collect := func(n ast.Node) { + visited := map[string]bool{} + + // collect takes every string in an expression, notes the names it + // mentions, and follows the functions it calls. + var collect func(ast.Node) + collect = func(n ast.Node) { ast.Inspect(n, func(n ast.Node) bool { switch node := n.(type) { case *ast.BasicLit: @@ -94,13 +156,30 @@ func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { } case *ast.Ident: names[node.Name] = true + case *ast.CallExpr: + callee := "" + switch fn := node.Fun.(type) { + case *ast.Ident: + callee = fn.Name + case *ast.SelectorExpr: + callee = fn.Sel.Name + } + if decl, ok := funcs[callee]; ok && !visited[callee] { + visited[callee] = true + collect(decl.Body) + } } return true }) } if everything { - collect(file) + ast.Inspect(file, func(n ast.Node) bool { + if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { + lits = append(lits, lit) + } + return true + }) return lits } @@ -110,29 +189,45 @@ func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { ast.Inspect(file, func(n ast.Node) bool { switch node := n.(type) { case *ast.CallExpr: - sel, ok := node.Fun.(*ast.SelectorExpr) - if !ok { - return true - } - if i, ok := hintArg[sel.Sel.Name]; ok && len(node.Args) > i { - collect(node.Args[i]) + if sel, ok := node.Fun.(*ast.SelectorExpr); ok { + if i, ok := hintArg[sel.Sel.Name]; ok && len(node.Args) > i { + collect(node.Args[i]) + } } case *ast.KeyValueExpr: - if key, ok := node.Key.(*ast.Ident); ok && key.Name == "Hint" { + if key, ok := node.Key.(*ast.Ident); ok && isHintName(key.Name) { collect(node.Value) } + case *ast.AssignStmt: + for _, lhs := range node.Lhs { + if isHintName(assignedName(lhs)) { + for _, rhs := range node.Rhs { + collect(rhs) + } + break + } + } + case *ast.ValueSpec: + for _, id := range node.Names { + if isHintName(id.Name) { + for _, v := range node.Values { + collect(v) + } + break + } + } } return true }) - // Second pass: whatever was assigned to a name a hint is built from. + // Whatever was assigned to a name a hint is built from. ast.Inspect(file, func(n ast.Node) bool { switch node := n.(type) { case *ast.AssignStmt: for _, lhs := range node.Lhs { - if id, ok := lhs.(*ast.Ident); ok && names[id.Name] { + if names[assignedName(lhs)] { for _, rhs := range node.Rhs { - collectStrings(rhs, &lits) + collect(rhs) } break } @@ -141,7 +236,7 @@ func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { for _, id := range node.Names { if names[id.Name] { for _, v := range node.Values { - collectStrings(v, &lits) + collect(v) } break } @@ -152,20 +247,21 @@ func hintLiterals(file *ast.File, everything bool) []*ast.BasicLit { return lits } -func collectStrings(n ast.Node, into *[]*ast.BasicLit) { - ast.Inspect(n, func(n ast.Node) bool { - if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { - *into = append(*into, lit) - } - return true - }) +// assignedName is the name an assignment writes: a variable's, or a field's. +func assignedName(lhs ast.Expr) string { + switch target := lhs.(type) { + case *ast.Ident: + return target.Name + case *ast.SelectorExpr: + return target.Sel.Name + } + return "" } // removedCommands is every command path .surface-breaking records the CLI as // having dropped. A word past a resolved command that names one of these is // drift whatever else it could be — without it, "basecamp recordings archive" -// reads as the [type] argument the moment `archive` is removed, which is the -// hole the nearest-ancestor fallback used to leave everywhere. +// reads as the [type] argument the moment `archive` is removed. func removedCommands(t *testing.T) map[string]bool { t.Helper() data, err := os.ReadFile("../../.surface-breaking") @@ -181,16 +277,35 @@ func removedCommands(t *testing.T) map[string]bool { } // commandPhrase matches a command reference the way scripts/check-skill-drift.sh -// extracts one, so a hint and a skill are read by the same rule. +// extracts one. var commandPhrase = regexp.MustCompile(`basecamp( [a-z][-a-z0-9]+)+`) +// commandPhrases returns the command references in text. A last word that +// runs on into a filename or a key — "upload report.pdf", "config set +// account_id" — is an argument, not a word of the command, and is dropped. func commandPhrases(text string) []string { - return commandPhrase.FindAllString(text, -1) + var phrases []string + for _, loc := range commandPhrase.FindAllStringIndex(text, -1) { + phrase, rest := text[loc[0]:loc[1]], text[loc[1]:] + if len(rest) > 1 && strings.ContainsRune("._/", rune(rest[0])) && isNameByte(rest[1]) { + i := strings.LastIndexByte(phrase, ' ') + if i <= len("basecamp") { + continue + } + phrase = phrase[:i] + } + phrases = append(phrases, phrase) + } + return phrases +} + +func isNameByte(b byte) bool { + return b >= 'a' && b <= 'z' || b >= 'A' && b <= 'Z' || b >= '0' && b <= '9' } // resolveCommandPhrase walks a command phrase down the tree. It reports the -// deepest command the phrase reaches, the first word past it that command -// does not have, and whether the phrase resolves. +// deepest command the phrase reaches, the first word past it that the CLI +// would not accept there, and whether the phrase resolves. func resolveCommandPhrase(root *cobra.Command, removed map[string]bool, phrase string) (resolved, word string, ok bool) { words := strings.Fields(phrase) cmd := root @@ -202,21 +317,37 @@ func resolveCommandPhrase(root *cobra.Command, removed map[string]bool, phrase s } cmd = sub } + path := cmd.CommandPath() if i == len(words) { - return cmd.CommandPath(), "", true + return path, "", true } - if removed[cmd.CommandPath()+" "+words[i]] { - return cmd.CommandPath(), words[i], false + if removed[path+" "+words[i]] { + return path, words[i], false } - // Leftover words cannot be an invented subcommand when the command runs - // as it stands — they are its arguments, or prose about running it — nor - // when it has no subcommands they could be mistaken for. What is left is - // a group that only dispatches, where the next word has to be one of the - // subcommands it dispatches to. - if cmd.Runnable() || !cmd.HasSubCommands() { - return cmd.CommandPath(), "", true + // Past a leaf there is no subcommand to mistake a word for: it is an + // argument, or prose about running the command. + if !cmd.HasSubCommands() { + return path, "", true + } + // Past a group, the word is fine only where the CLI would take it as an + // argument: the group runs, and its own validator accepts the word. + if cmd.Runnable() && acceptsArgument(cmd, words[i]) { + return path, "", true + } + return path, words[i], false +} + +// acceptsArgument asks cmd's own argument validator whether word can be its +// first argument, in an invocation of up to three. +func acceptsArgument(cmd *cobra.Command, word string) bool { + args := []string{word} + for len(args) <= 3 { + if cmd.ValidateArgs(args) == nil { + return true + } + args = append(args, "x") } - return cmd.CommandPath(), words[i], false + return false } func childNamed(cmd *cobra.Command, name string) *cobra.Command { diff --git a/scripts/check-skill-drift.sh b/scripts/check-skill-drift.sh index a8e5b477e..c0e69e46c 100755 --- a/scripts/check-skill-drift.sh +++ b/scripts/check-skill-drift.sh @@ -1,6 +1,12 @@ #!/usr/bin/env bash # Verify that commands and flags referenced in skill files exist in the CLI surface. # Catches stale skill references: renamed commands, removed flags, etc. +# +# Usage: scripts/check-skill-drift.sh [skill] [surface] [baseline] [breaking] +# skill the SKILL.md to read (default skills/basecamp/SKILL.md) +# surface the CLI surface snapshot (default .surface) +# baseline acknowledged drift (default .surface-skill-drift) +# breaking acknowledged surface removals (default .surface-breaking) set -euo pipefail SKILL="${1:-skills/basecamp/SKILL.md}" @@ -32,6 +38,25 @@ is_baselined() { [ -f "$BASELINE" ] && grep -qxF "$1" "$BASELINE" } +# Check whether a command path is one the CLI has acknowledged removing. +is_removed() { + [ -f "$BREAKING" ] && grep -qxF "CMD $1" "$BREAKING" +} + +# extract_candidates reads text and prints every "basecamp ..." command +# reference in it, one per line. +# +# A word that runs on into a filename or a key — "basecamp upload report.pdf", +# "basecamp config set account_id" — is not a word of the command: the match +# has to end where a word ends, and a ".", "_" or "/" followed by more of a +# name is not an end. So "basecamp upload report.pdf" yields "basecamp upload", +# and "basecamp auth status." at the end of a sentence still yields the whole +# command. +extract_candidates() { + grep -oE 'basecamp( [a-z][-a-z0-9]+)+([^-a-z0-9._/]|[._/]([^a-zA-Z0-9]|$)|$)' \ + | sed -E 's/[^a-z0-9-]+$//' || true +} + # resolve_cmd prints the longest prefix of a candidate that is a CMD in # .surface, and nothing when no prefix is one. resolve_cmd() { @@ -64,7 +89,9 @@ resolve_cmd() { # Those two escapes cannot tell an argument value from a subcommand we # removed — "basecamp recordings archive" reads as the [type] argument once # `archive` is gone — so a word that names a removed command fails first, -# whichever escape would otherwise have taken it. +# whichever escape would otherwise have taken it. They cannot tell one from a +# subcommand that never existed either: under a command that has both +# arguments and subcommands, an invented subcommand still passes as a value. # # Prints "|" and returns 0 when the # candidate is verified, 1 when it is not. On a failure the resolved command @@ -85,10 +112,11 @@ verify_cmd() { return 0 fi + local word="${parts[$depth]}" # A word that names a command we removed is drift whatever else it could # be, so it is refused before the escapes below could read it as prose. - if [ -f "$BREAKING" ] && grep -qxF "CMD ${matched} ${parts[$depth]}" "$BREAKING"; then - echo "${matched}|${parts[$depth]}" + if is_removed "${matched} ${word}"; then + echo "${matched}|${word}" return 1 fi # Leftover words are argument values when the command takes arguments. @@ -102,10 +130,24 @@ verify_cmd() { return 0 fi - echo "${matched}|${parts[$depth]}" + echo "${matched}|${word}" return 1 } +# drift_key prints the baseline entry that acknowledges a failed candidate, +# given verify_cmd's output for it. +drift_key() { + local candidate="$1" result="$2" + local resolved="${result%%|*}" word="${result#*|}" + if [ -z "$resolved" ] || [ "$resolved" = "basecamp" ]; then + # Nothing past "basecamp" names a command: the whole reference is the + # drift, and the whole reference is what a baseline entry names. + echo "CMD ${candidate}" + else + echo "CMD ${resolved} ${word}" + fi +} + # Check whether a flag exists on a command or any of its subcommands. # Descendant matching is intentional: Cobra commands inherit flags and shortcut # commands delegate to subcommands (e.g. "basecamp cards --in" runs "cards list" @@ -123,37 +165,33 @@ flag_exists() { skill_body=$(awk '/^---$/{n++; next} n>=2' "$SKILL") # --- Phase 1: Command references --- -# Extract "basecamp ..." patterns, resolve to longest matching CMD. +# Extract "basecamp ..." patterns and verify each resolves. while IFS= read -r candidate; do + [ -z "$candidate" ] && continue status=0 result=$(verify_cmd "$candidate") || status=$? if [ "$status" -ne 0 ]; then resolved="${result%%|*}" word="${result#*|}" + key=$(drift_key "$candidate" "$result") if [ -z "$resolved" ] || [ "$resolved" = "basecamp" ]; then - # Nothing past "basecamp" names a command: the whole reference is the - # drift, and the whole reference is what a baseline entry names. message="command not in surface: $candidate" - key="CMD ${candidate}" + elif is_removed "${resolved} ${word}"; then + message="removed command: \"$candidate\" — ${resolved} no longer has \"${word}\"" else - if grep -qxF "CMD ${resolved} ${word}" "$BREAKING" 2>/dev/null; then - message="removed command: \"$candidate\" — ${resolved} no longer has \"${word}\"" - else - message="cannot verify \"$candidate\": ${resolved} has no subcommand \"${word}\"" - fi - key="CMD ${resolved} ${word}" + message="cannot verify \"$candidate\": ${resolved} has no subcommand \"${word}\"" fi if is_baselined "$key"; then : # known drift else - echo "DRIFT: $message" + echo "DRIFT: $message (baseline entry: $key)" new_drift=$((new_drift + 1)) fi errors=$((errors + 1)) fi cmd_checked=$((cmd_checked + 1)) -done < <(echo "$skill_body" | grep -oE 'basecamp( [a-z][-a-z0-9]+)+' | sort -u) +done < <(echo "$skill_body" | extract_candidates | sort -u) # --- Phase 2: Flag references --- # For lines with "basecamp ... --flag", verify each flag exists on the @@ -164,18 +202,22 @@ trap 'rm -f "$tmpfile"' EXIT echo "$skill_body" | grep -nE 'basecamp [a-z].+--[a-z]' > "$tmpfile" || true while IFS=: read -r lineno line; do - # Extract command candidate using BASH_REMATCH to avoid grep|head SIGPIPE - if [[ "$line" =~ basecamp(\ [a-z][-a-z0-9]+)+ ]]; then - cmd_candidate="${BASH_REMATCH[0]}" - else - continue - fi + # The first command reference on the line, read whole so no pipe is cut + # short under pipefail. + candidates=$(printf '%s\n' "$line" | extract_candidates) + cmd_candidate="${candidates%%$'\n'*}" + [ -z "$cmd_candidate" ] && continue - # A command phase 1 already reported is no base for a flag check. status=0 result=$(verify_cmd "$cmd_candidate") || status=$? - [ "$status" -ne 0 ] && continue + if [ "$status" -ne 0 ]; then + # New drift was reported once, in phase 1, and is no base for a flag + # check. Acknowledged drift still has its flags checked, against the + # deepest command that does exist, as they always were. + is_baselined "$(drift_key "$cmd_candidate" "$result")" || continue + fi matched="${result%%|*}" + [ -z "$matched" ] && continue for flag in $(echo "$line" | grep -oE -- '--[a-z][-a-z0-9]*' | sort -u); do # Skip cobra built-ins @@ -203,8 +245,8 @@ if [ $new_drift -gt 0 ]; then echo "" echo "Found ${new_drift} new skill drift issue(s) (${baselined} baselined)." echo "Update ${SKILL} to match the current CLI surface (.surface)," - echo "or add entries to ${BASELINE} if the drift is intentional —" - echo "a hidden command is not in .surface, and is baselined as \"CMD \"." + echo "or add the entry each DRIFT line names to ${BASELINE} if the drift is intentional —" + echo "a hidden command is never in .surface, and has to be acknowledged that way." exit 1 fi diff --git a/scripts/test-skill-drift.sh b/scripts/test-skill-drift.sh index 33eda3d6a..39ace7fd7 100755 --- a/scripts/test-skill-drift.sh +++ b/scripts/test-skill-drift.sh @@ -128,6 +128,15 @@ write_skill removed_under_leaf 'Run `basecamp todos list stale` for the stale on assert_fails removed_under_leaf 'removed command' \ 'a removed subcommand fails even under a command with no subcommands left' +# A removed name is only a command when it is a whole word. The same word +# running on into a filename or a key is an argument value. +write_skill removed_name_as_value 'Run `basecamp recordings vaults.json` and `basecamp todos list stale_only`.' +assert_passes removed_name_as_value \ + 'a removed name running on into a filename or key is an argument value' + +write_skill sentence_end 'When in doubt, run basecamp profile list.' +assert_passes sentence_end 'a command at the end of a sentence still resolves whole' + # --- What already failed, and still has to --- write_skill unknown 'Run `basecamp bogus thing` for this.' @@ -143,6 +152,24 @@ assert_passes goodflag 'a flag the command does have passes' # --- The baseline still suppresses drift, now for commands too --- +# Whatever entry a DRIFT line names is the one that acknowledges it — including +# where the words past the command are prose, not a path. +write_skill prose_after_group 'Use basecamp profile to see your profiles.' +run_check prose_after_group +entry=$(printf '%s\n' "$out" | sed -nE 's/^DRIFT: .*\(baseline entry: (.*)\)$/\1/p') +if [ "$status" -ne 0 ] && [ "$entry" = "CMD basecamp profile to" ]; then + ok 'the report names the baseline entry that acknowledges it' +else + not_ok "the report names the baseline entry that acknowledges it — exited ${status}: ${out}" +fi +printf '%s\n' "$entry" > "${work}/named_entry" +run_check prose_after_group "${work}/named_entry" +if [ "$status" -eq 0 ]; then + ok 'adding the named entry to the baseline acknowledges the drift' +else + not_ok "adding the named entry to the baseline acknowledges the drift — exited ${status}: ${out}" +fi + printf 'CMD basecamp profile set\n' > "${work}/baselined" run_check invented "${work}/baselined" if [ "$status" -eq 0 ]; then @@ -151,6 +178,16 @@ else not_ok "a baselined command reference is not new drift — exited ${status}: ${out}" fi +# An acknowledged command is still a command whose flags can be wrong: they +# are checked against the deepest command that does exist, as they always were. +write_skill baselined_flag 'Run `basecamp profile set agent --totally-bogus`.' +run_check baselined_flag "${work}/baselined" +if [ "$status" -ne 0 ] && [[ "$out" == *'flag --totally-bogus not found on basecamp profile'* ]]; then + ok 'a baselined command still has its flags checked' +else + not_ok "a baselined command still has its flags checked — exited ${status}: ${out}" +fi + # --- One report per bad command, not one per flag on its line --- write_skill invented_flag 'Run `basecamp profile set agent --nope --also-nope`.' From 2f0cb581f439e2c2cf34f1cbbdcf9369d9538a62 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 11:18:44 +0200 Subject: [PATCH 4/6] Refuse invented top-level commands in hints, and say which login binds nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .surface-skill-drift | 10 +++++----- AGENTS.md | 2 +- internal/commands/connect.go | 14 ++++++++------ internal/commands/hint_commands_test.go | 20 ++++++++++++++++++-- 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/.surface-skill-drift b/.surface-skill-drift index 3fb75345c..85dfef0a0 100644 --- a/.surface-skill-drift +++ b/.surface-skill-drift @@ -1,11 +1,11 @@ # Known skill drift — references the surface cannot confirm. # These are pre-existing mismatches, not regressions. Fix in SKILL.md when convenient. # Format: FLAG -# CMD — a command reference the check refuses: the -# deepest command that exists and the word past it, exactly as the -# DRIFT line's "baseline entry" names it. This covers a hidden command -# (never in .surface) and a group that also runs bare (.surface cannot -# say it does). +# CMD — a command reference the check refuses, exactly as the DRIFT +# line's "baseline entry" names it: the deepest command that exists and +# the word past it, or the whole reference when nothing past "basecamp" +# is a command. This covers a hidden command (never in .surface) and a +# group that also runs bare (.surface cannot say it does). FLAG basecamp comments list --in FLAG basecamp comments update --in FLAG basecamp people --in diff --git a/AGENTS.md b/AGENTS.md index fed67bc90..90ca0b038 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,7 +106,7 @@ the real command tree rather than `.surface`. It reads hint text in `internal/co and `internal/connector/setup` — hints written inline, built into a variable, returned by a helper, or assigned to a hint-named field — and every string in `connect.go`. Asking the command lets it be exact where the script cannot: a word after a group passes only -if the group runs and its own argument validator accepts the word — which a subcommand +if the group is not the root, runs, and its own argument validator accepts the word — which a subcommand that never existed can still satisfy, under a group like `recordings` that takes one. It checks commands, not flags. diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 3d1f5d114..e293bb6f1 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -883,11 +883,13 @@ func connectAccount(app *appctx.App, name string) (string, error) { // unboundProfileError refuses a profile that names no account, with the // remedy that will work for this one. // -// No command binds an account on its own — `profile` has none for it. Of the -// commands that store a credential, only the headless ones write the account -// too, and of those only `auth agent connect` fits here: it takes the -// Agent's own account. A bot user logs in through the browser, and that login -// binds nothing, so a bot's account goes into its config entry by hand. +// No command binds an account on its own — `profile` has none for it. The +// account is written by a command that stores a credential: `auth agent +// connect`, which takes the Agent's own account, and the headless logins +// (`auth login --with-token` or `--with-client-credentials` with --account). +// A bot user logged in through the browser, which binds nothing, so for a +// bot that already has its credential the account goes into its config entry +// by hand rather than through another login. // // Even `auth agent connect` refuses an accountless entry that is not the // global config's, because the entry it would write stays shadowed by the @@ -912,7 +914,7 @@ func unboundProfileError(name string) error { global := filepath.Join(config.GlobalConfigDir(), "config.json") return output.ErrUsageHint(message, "For an Agent, connecting it binds its own account: basecamp auth agent connect -P "+shellQuote(name)+ - ". A bot user's login binds none, so for a bot add account_id to the profile's entry in "+richtext.SanitizeSingleLine(global)+".") + ". A bot user's browser login binds none, so for a bot add account_id to the profile's entry in "+richtext.SanitizeSingleLine(global)+".") } // connectCredentialKind is the kind of credential the active profile holds, diff --git a/internal/commands/hint_commands_test.go b/internal/commands/hint_commands_test.go index e666907f1..7859b9f1e 100644 --- a/internal/commands/hint_commands_test.go +++ b/internal/commands/hint_commands_test.go @@ -144,8 +144,13 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, everything boo names := map[string]bool{} visited := map[string]bool{} - // collect takes every string in an expression, notes the names it - // mentions, and follows the functions it calls. + // collect takes every string in an expression, notes the local names + // whose values it is built from, and follows the functions it calls. + // + // Only a name that is itself a value counts. `cmd` in cmd.CommandPath() + // and `fmt` in fmt.Sprintf are not hint text, and following them would + // pull in whole command definitions — help text included — as if they + // were hints. var collect func(ast.Node) collect = func(n ast.Node) { ast.Inspect(n, func(n ast.Node) bool { @@ -156,6 +161,8 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, everything boo } case *ast.Ident: names[node.Name] = true + case *ast.SelectorExpr: + return false // a field or a package member, not a local value case *ast.CallExpr: callee := "" switch fn := node.Fun.(type) { @@ -168,6 +175,10 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, everything boo visited[callee] = true collect(decl.Body) } + for _, arg := range node.Args { + collect(arg) + } + return false // the callee's name is not a value } return true }) @@ -321,6 +332,11 @@ func resolveCommandPhrase(root *cobra.Command, removed map[string]bool, phrase s if i == len(words) { return path, "", true } + // The root takes no word it does not know: cobra answers "unknown + // command" there whatever its validator says, since it has none. + if !cmd.HasParent() { + return path, words[i], false + } if removed[path+" "+words[i]] { return path, words[i], false } From 2d34a8e2c5e52baee68a7630711cec8ced737b38 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 11:29:41 +0200 Subject: [PATCH 5/6] Read hints handed to a wrapper, and stop reading help text as hints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 3 +- internal/commands/connect_unbound_test.go | 1 + internal/commands/hint_commands_test.go | 183 ++++++++++++++++++---- scripts/test-skill-drift.sh | 2 +- 4 files changed, 155 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 90ca0b038..b8b671503 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,7 +104,8 @@ evals run against its own SKILL.md (`make -C skill-evals eval-connect`). `TestHintCommandsResolve` holds the commands hints name to the same exactness, against the real command tree rather than `.surface`. It reads hint text in `internal/commands` and `internal/connector/setup` — hints written inline, built into a variable, returned -by a helper, or assigned to a hint-named field — and every string in `connect.go`. Asking +by a helper, passed through a wrapper, or assigned to a hint-named field — and every +string in `connect.go`. Asking the command lets it be exact where the script cannot: a word after a group passes only if the group is not the root, runs, and its own argument validator accepts the word — which a subcommand that never existed can still satisfy, under a group like `recordings` that takes one. It diff --git a/internal/commands/connect_unbound_test.go b/internal/commands/connect_unbound_test.go index 0107a837d..05ef31bcc 100644 --- a/internal/commands/connect_unbound_test.go +++ b/internal/commands/connect_unbound_test.go @@ -45,6 +45,7 @@ func TestUnboundProfileHintNamesOnlyWhatBinds(t *testing.T) { assert.Contains(t, hint, "basecamp auth agent connect -P agent") assert.Contains(t, hint, "add account_id to the profile's entry in "+filepath.Join(config.GlobalConfigDir(), "config.json")) + assert.Contains(t, hint, "browser login binds none", "a headless login can bind, so the claim is about the browser") assert.NotContains(t, hint, "auth login", "a browser login binds no account") } diff --git a/internal/commands/hint_commands_test.go b/internal/commands/hint_commands_test.go index 7859b9f1e..1b9bf2ad5 100644 --- a/internal/commands/hint_commands_test.go +++ b/internal/commands/hint_commands_test.go @@ -48,9 +48,10 @@ func TestHintCommandsResolve(t *testing.T) { for _, dir := range []string{".", "../connector/setup"} { files, funcs := parsePackage(t, fset, dir) + sinks := hintSinks(funcs) for name, file := range files { everything := dir == "." && name == "connect.go" - for _, lit := range hintLiterals(file, funcs, everything) { + for _, lit := range hintLiterals(file, funcs, sinks, everything) { if seen[lit.Pos()] { continue // a helper reached from more than one hint } @@ -84,6 +85,7 @@ func TestHintCommandsResolve(t *testing.T) { {reference{"connect.go", "basecamp auth login"}, "a Hint field in a composite literal"}, {reference{"boost.go", "basecamp boost list"}, "a hint built into a variable"}, {reference{"wizard.go", "basecamp auth status"}, "a hint returned by a helper"}, + {reference{"upgrade_selfupdate.go", "basecamp upgrade"}, "a hint passed through a wrapper"}, {reference{"doctor.go", "basecamp upgrade"}, "an assignment to a Hint field"}, {reference{"feed.go", "basecamp events poll"}, "a hint-named field"}, {reference{"checks.go", "basecamp connect setup"}, "a readiness check's hint"}, @@ -122,6 +124,103 @@ func parsePackage(t *testing.T, fset *token.FileSet, dir string) (map[string]*as return files, funcs } +// hintSinks maps every function that takes hint text to the positions of the +// arguments that carry it: the output constructors, and every package +// function with a parameter that reaches a hint inside it — written to a +// hint-named field or variable, or passed on to another sink. A wrapper like +// errUpgradeFailedHint(msg, hint) is then a sink like ErrUsageHint, and so is +// a wrapper of it: the search runs until no function gains a parameter. +func hintSinks(funcs map[string]*ast.FuncDecl) map[string]map[int]bool { + sinks := map[string]map[int]bool{ + "ErrUsageHint": {1: true}, + "ErrNotFoundHint": {2: true}, + } + + for changed := true; changed; { + changed = false + for name, fn := range funcs { + params := map[string]int{} + i := 0 + for _, field := range fn.Type.Params.List { + for _, id := range field.Names { + params[id.Name] = i + i++ + } + if len(field.Names) == 0 { + i++ + } + } + if len(params) == 0 { + continue + } + + // reach marks the parameters that are the hint text itself: the + // whole expression, or a side of a concatenation. A parameter the + // text is only formatted from — `cmd` in cmd.CommandPath() — is + // not hint text, and marking it would make every caller's `cmd` + // a hint. + var reach func(ast.Expr) + reach = func(expr ast.Expr) { + switch e := expr.(type) { + case *ast.ParenExpr: + reach(e.X) + case *ast.BinaryExpr: + if e.Op == token.ADD { + reach(e.X) + reach(e.Y) + } + case *ast.Ident: + if at, ok := params[e.Name]; ok && !sinks[name][at] { + if sinks[name] == nil { + sinks[name] = map[int]bool{} + } + sinks[name][at] = true + changed = true + } + } + } + + ast.Inspect(fn.Body, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.KeyValueExpr: + if key, ok := node.Key.(*ast.Ident); ok && isHintName(key.Name) { + reach(node.Value) + } + case *ast.AssignStmt: + for _, lhs := range node.Lhs { + if isHintName(assignedName(lhs)) { + for _, rhs := range node.Rhs { + reach(rhs) + } + break + } + } + case *ast.CallExpr: + for at := range sinks[calleeName(node)] { + if at < len(node.Args) { + reach(node.Args[at]) + } + } + } + return true + }) + } + } + return sinks +} + +// calleeName is the name a call is made through: a function's, or a method's +// or package member's selector. +func calleeName(call *ast.CallExpr) string { + switch fn := call.Fun.(type) { + case *ast.Ident: + return fn.Name + case *ast.SelectorExpr: + return fn.Sel.Name + } + return "" +} + // isHintName reports whether an identifier names hint text: Hint, // forbiddenHint, agentReadsRefusedHint, hint. func isHintName(name string) bool { @@ -129,56 +228,79 @@ func isHintName(name string) bool { } // hintLiterals returns the string literals in a file that an operator is -// meant to read: the hint argument of the hint-carrying error constructors, -// anything written to a hint-named field, variable or constant, and — when -// everything is wanted — every string literal in the file. +// meant to read: the hint arguments of every sink (see hintSinks), anything +// written to a hint-named field, variable or constant, and — when everything +// is wanted — every string literal in the file. // // A hint is not always written where it is passed. Some are built into a -// variable first (boost.go builds one, then appends --event), and some are -// returned by a helper (wizard.go passes wizardEscapeHint()). So the names a -// hint expression mentions are followed to their assignments, and the -// functions it calls are followed into their bodies. Over-reaching there +// variable first (boost.go builds one, then appends --event), some are +// returned by a helper (wizard.go passes wizardEscapeHint()), and some are +// handed to a wrapper (errUpgradeFailedHint). So the names a hint expression +// mentions are followed to their assignments, the functions it calls are +// followed into their bodies, and a wrapper's hint argument is a sink. Over-reaching there // costs nothing: a literal with no command in it is scanned and passes. -func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, everything bool) []*ast.BasicLit { +func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, sinks map[string]map[int]bool, everything bool) []*ast.BasicLit { var lits []*ast.BasicLit names := map[string]bool{} visited := map[string]bool{} // collect takes every string in an expression, notes the local names - // whose values it is built from, and follows the functions it calls. + // whose values the text is built from, and follows the functions it + // calls — including a function literal called in place, and the call a + // method chain starts from. // - // Only a name that is itself a value counts. `cmd` in cmd.CommandPath() - // and `fmt` in fmt.Sprintf are not hint text, and following them would - // pull in whole command definitions — help text included — as if they - // were hints. + // Only a name that is the text itself counts: the whole expression, or a + // side of a concatenation. A name passed as an argument is a value the + // text is formatted from, not the text — following one runs from + // `formatCardTableIDs(cardTables)` to `listProjectCardTables(cmd, ...)` + // and on to every command definition assigned to `cmd`, help text + // included — and neither is a selector's receiver (`cmd` in + // cmd.CommandPath(), `fmt` in fmt.Sprintf). var collect func(ast.Node) collect = func(n ast.Node) { + if id, ok := n.(*ast.Ident); ok { + names[id.Name] = true + return + } ast.Inspect(n, func(n ast.Node) bool { switch node := n.(type) { case *ast.BasicLit: if node.Kind == token.STRING { lits = append(lits, node) } - case *ast.Ident: - names[node.Name] = true + case *ast.BinaryExpr: + if node.Op == token.ADD { + for _, side := range []ast.Expr{node.X, node.Y} { + if id, ok := side.(*ast.Ident); ok { + names[id.Name] = true + } + } + } case *ast.SelectorExpr: - return false // a field or a package member, not a local value - case *ast.CallExpr: - callee := "" - switch fn := node.Fun.(type) { - case *ast.Ident: - callee = fn.Name - case *ast.SelectorExpr: - callee = fn.Sel.Name + if _, named := node.X.(*ast.Ident); !named { + collect(node.X) // newX("basecamp ...").String() } + return false + case *ast.CallExpr: + callee := calleeName(node) if decl, ok := funcs[callee]; ok && !visited[callee] { visited[callee] = true collect(decl.Body) } + switch fn := node.Fun.(type) { + case *ast.FuncLit: + collect(fn.Body) // func() string { ... }() + case *ast.SelectorExpr: + if _, named := fn.X.(*ast.Ident); !named { + collect(fn.X) + } + } for _, arg := range node.Args { - collect(arg) + if _, named := arg.(*ast.Ident); !named { + collect(arg) + } } - return false // the callee's name is not a value + return false } return true }) @@ -194,15 +316,12 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, everything boo return lits } - // Which argument carries the hint, by constructor name. - hintArg := map[string]int{"ErrUsageHint": 1, "ErrNotFoundHint": 2} - ast.Inspect(file, func(n ast.Node) bool { switch node := n.(type) { case *ast.CallExpr: - if sel, ok := node.Fun.(*ast.SelectorExpr); ok { - if i, ok := hintArg[sel.Sel.Name]; ok && len(node.Args) > i { - collect(node.Args[i]) + for at := range sinks[calleeName(node)] { + if at < len(node.Args) { + collect(node.Args[at]) } } case *ast.KeyValueExpr: diff --git a/scripts/test-skill-drift.sh b/scripts/test-skill-drift.sh index 39ace7fd7..fd4eb9fc0 100755 --- a/scripts/test-skill-drift.sh +++ b/scripts/test-skill-drift.sh @@ -86,7 +86,7 @@ assert_fails() { if [ "$status" -eq 0 ]; then not_ok "$3 — the check passed: ${out}" elif [[ "$out" != *"$2"* ]]; then - not_ok "$3 — expected ${2@Q} in: ${out}" + not_ok "$3 — expected '$2' in: ${out}" else ok "$3" fi From 3d2f69ceb17df62635a45e038f0637ab66b14033 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 17 Sep 2026 11:45:35 +0200 Subject: [PATCH 6/6] Drop the hint readers nothing exercises, and say what a name collision does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/commands/hint_commands_test.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/internal/commands/hint_commands_test.go b/internal/commands/hint_commands_test.go index 1b9bf2ad5..2814ca5f8 100644 --- a/internal/commands/hint_commands_test.go +++ b/internal/commands/hint_commands_test.go @@ -113,8 +113,10 @@ func parsePackage(t *testing.T, fset *token.FileSet, dir string) (map[string]*as files[name] = file for _, decl := range file.Decls { - // Methods are keyed by name too: a name collision only widens - // what is scanned. + // Keyed by bare name, methods included, so the last declaration + // of a name wins and the others are never followed or read as + // wrappers. No colliding name in these packages carries a hint + // (Error, label, NewTUICmd, the per-platform private_* files). if fn, ok := decl.(*ast.FuncDecl); ok && fn.Body != nil { funcs[fn.Name.Name] = fn } @@ -246,8 +248,7 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, sinks map[stri // collect takes every string in an expression, notes the local names // whose values the text is built from, and follows the functions it - // calls — including a function literal called in place, and the call a - // method chain starts from. + // calls. // // Only a name that is the text itself counts: the whole expression, or a // side of a concatenation. A name passed as an argument is a value the @@ -277,9 +278,6 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, sinks map[stri } } case *ast.SelectorExpr: - if _, named := node.X.(*ast.Ident); !named { - collect(node.X) // newX("basecamp ...").String() - } return false case *ast.CallExpr: callee := calleeName(node) @@ -287,14 +285,6 @@ func hintLiterals(file *ast.File, funcs map[string]*ast.FuncDecl, sinks map[stri visited[callee] = true collect(decl.Body) } - switch fn := node.Fun.(type) { - case *ast.FuncLit: - collect(fn.Body) // func() string { ... }() - case *ast.SelectorExpr: - if _, named := fn.X.(*ast.Ident); !named { - collect(fn.X) - } - } for _, arg := range node.Args { if _, named := arg.(*ast.Ident); !named { collect(arg)