Repository navigation
Let an agent connect an agent and manage connector setup: the basecamp-connect skill - #737
Conversation
… setup A reference an agent works from to connect a Basecamp agent and manage basecamp connect setup for a person: which of auth agent connect and connect setup to run when, what connect.json holds and how each part is changed, how to inspect it, and how to explain every setup exit code and failed check, including Basecamp refusing reads to an Agent identity. Starting and supervising the connector is named as not built yet. The skill gets its own eval cases, run against its own SKILL.md, and the eval runner can now grade the agent's final reply as well as its commands.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
tests
There was a problem hiding this comment.
🟡 Changes recommended
The guided workflow can select an unverified Agent identity and emits commands without safely quoting dynamic values.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds the basecamp-connect skill and evaluation coverage for agent credential and connector setup workflows.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
Changes:
- Documents guided connector setup, trust, routing, readiness, and recovery.
- Adds response-aware skill evaluation and two connector scenarios.
- Integrates the skill into CI, drift checks, release validation, and contributor guidance.
File summaries
| File | Description |
|---|---|
skills/basecamp-connect/SKILL.md |
Adds the connector-management skill. |
skill-evals/run |
Grades final agent responses. |
skill-evals/Makefile |
Adds the connector eval target. |
skill-evals/cases/basecamp-connect/not-ready-agent-reads.yml |
Tests Agent-read refusal handling. |
skill-evals/cases/basecamp-connect/first-time-setup.yml |
Tests first-time setup behavior. |
Makefile |
Adds drift and eval integration. |
internal/release/manifests_test.go |
Validates the new skill path. |
AGENTS.md |
Documents drift and eval coverage. |
.github/workflows/test.yml |
Triggers evals for skill changes. |
Review details
Suppressed comments (1)
skills/basecamp-connect/SKILL.md:217
- The agent substitutes local profile names and directory paths into this shell command, but the example leaves those values unquoted. A route such as
/home/me/Work/Redesign 2is split into multiple arguments, and shell metacharacters can be interpreted. Require shell-quoting every substituted value, consistent withinternal/commands/files.go:1903-1918andinternal/commands/templates.go:295-300.
basecamp connect setup -P <profile> --operator-profile <operator-profile> \
--route <project-id>=<dir> --route <project-id>=<dir> --json
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… found The skill now confirms who a profile's credential is (basecamp me) before setting it up, and stops when it is not the agent the person named. It single-quotes every value it puts into a command and resolves project names to ids before they reach one. From review: the bot user signs in to a new profile with profile create; the auth code is auth_required and an unknown profile is api_error (exit 7, shared with not_ready); interactive logins run without machine output or BASECAMP_NONINTERACTIVE, in the background, and never follow a hint to --with-token or --with-client-credentials; the link and code stay in the local conversation; reconnecting rotates the agent's secret, so it needs consent and is never used on a bot-user profile; auth status is read precisely; moving XDG_CONFIG_HOME hides every profile; the last route cannot be removed alone; lookups name their profile. Evals: route-quoting and unconfirmed-identity cases, a realistic auth agent connect mock with a --json rejection, and quote-tolerant patterns.
There was a problem hiding this comment.
🟡 Changes recommended
Directly reading connect.json bypasses its security validation and could expose an arbitrary symlinked local file.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The skill had an agent read connect.json with a plain file read, which skips the no-follow, ownership and permission checks setup.Load applies: a planted symlink could put any local file into the conversation. basecamp connect show prints a profile's connect.json through setup.Load, changing nothing and making no request; a symlink, a file others could write, or one that does not parse is refused and nothing of it is shown. The skill now inspects policy only through it and says never to read the file directly. The eval cases mock it.
There was a problem hiding this comment.
🟡 Changes recommended
The default human output for connect show omits the nested agent, trust, and project policy fields.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The generic styled and Markdown renderers drop nested maps, which is where the agent, the trust and the routes live, so show printed a summary and the worker settings to anyone not using --json. Show now renders those formats itself, one setting to a line with the routes in project order. The not_found hint quotes its placeholders, and on Windows the refusal says the trust file is unsupported there rather than asking to fix a file.
There was a problem hiding this comment.
🟡 Changes recommended
Connector policy display misses profile-binding validation, and the skill’s examples contradict its required quoting rule.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
skills/basecamp-connect/SKILL.md:89
- The stated invariant and PR scope require every substituted command value to be single-quoted, but later runnable templates leave
<account-id>,<bot-identity-id>,<id>, and<person-id>bare (for example lines 234–236, 250, 279, 293–297, 356, and 370). This gives the agent conflicting instructions and the evals do not cover most of these paths. Quote those placeholders consistently, or narrow the rule and PR claim if only shell-sensitive values are intended.
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
…ind of value connect show accepted a valid policy copied from another profile's directory and labelled it as this profile's; setup refuses that, and now show does too. Its human output shows route paths exactly: quoted for a terminal, a code span in Markdown. The skill said to single-quote every value while its templates left ids bare. It now has one rule for each kind of value: numeric ids bare and checked to be digits, everything else single-quoted.
There was a problem hiding this comment.
🟡 Changes recommended
Markdown output can expose terminal controls, and response evals can incorrectly accept truncated model replies.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
skill-evals/run:124
- This treats any response without a tool call as the final reply, even when Anthropic reports
stop_reason: "max_tokens". A truncated reply can therefore satisfyaccept_responseand pass the new final-response grading. Checkstop_reasonand fail the sample unless the terminal response completed normally.
skills/basecamp-connect/SKILL.md:155 - The documented class grammar omits that the first character must be a lowercase letter or digit (
setup.ValidClassuses^[a-z0-9][a-z0-9_-]{0,39}$). As written, the skill can recommend values such as_internalthat setup rejects. Include the leading-character requirement.
This issue also appears on line 225 of the same file.
skills/basecamp-connect/SKILL.md:226
- The documented profile grammar omits that the first character must be a letter or digit;
isValidProfileNamerejects leading-or_(internal/commands/profile.go:625-629). State that constraint so the guided flow does not choose a profile name that all three commands reject.
**1. Profile and credential.** Agree on a profile name: letters, digits, `-`
and `_` only, for example the agent's name. Inspect it (steps 1 to 3 above).
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
…; exact name grammars connect show's Markdown output put a route path in a code span raw, and a clean absolute path can hold ESC or C1 controls: they are now written as escapes, as the styled output already quotes them. The eval runner treated a reply cut off at max_tokens as final, so a truncated reply could satisfy accept_response; it now fails the sample. The skill states that profile names and class labels start with a letter or digit, as the CLI requires, and quotes the profile placeholder in its overview too.
There was a problem hiding this comment.
🔵 Needs a closer look
Markdown output does not safely delimit the configurable policy file path.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/connect.go:137
- In Markdown mode the config path is inserted as raw Markdown.
XDG_CONFIG_HOMEcan contain brackets or backticks, so a valid path can render as a link or alter the document instead of being shown literally; route paths are already code-fenced for this reason. Render the file path as an escaped code span in Markdown mode as well.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
🔵 Needs a closer look
The first-time eval does not reject every machine-output mode refused by the real interactive connection command.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
skill-evals/cases/basecamp-connect/first-time-setup.yml:41
- The real interactive-login gate also rejects
--ids-only,--count, and--jq(internal/commands/auth.go:1254-1276), but this broad success mock plus the current rejection allows an agent using any of those machine-output modes to pass the first-setup eval. Include all three so this case actually enforces its “interactive connection uses no machine output” behavior.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
…connection The login gate also refuses --ids-only, --count and --jq.
|
🤖 The last Copilot review noted, in a suppressed comment, that the first-time eval didn't reject every machine-output mode. Fixed in 0fe4428: the rejection now also covers |
There was a problem hiding this comment.
🔵 Needs a closer look
The custom human renderer bypasses global styled-output and statistics behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/connect.go:115
- This direct write bypasses the output pipeline for both human formats. Consequently
basecamp connect show --styledemits no ANSI despite--styledmeaning “Force styled output (ANSI colors)” (internal/cli/root.go:249), and--statsis silently omitted even thoughApp.OKadds it and both styled/Markdown renderers display it (internal/appctx/context.go:263-272,internal/output/render.go:143-159,internal/output/render.go:1259-1263). Please route the custom policy presentation through an output-rendering hook/type, or otherwise preserve these global format features instead of returning afterfmt.Fprint.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Show wrote its styled and Markdown text straight to stdout, so --styled emitted no ANSI and --stats was dropped. It now hands the renderer a flat display view (one field per setting and per route, paths quoted or code spanned) through WithDisplayData, and JSON keeps the file's own shape.
|
🤖 The last Copilot review noted, in a suppressed comment, that show's human output bypassed the output pipeline (no ANSI under |
There was a problem hiding this comment.
🔵 Needs a closer look
Two connector evals can pass while targeting the wrong profile.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
skill-evals/cases/basecamp-connect/not-ready-agent-reads.yml:20
- The eval accepts a route update without asserting
-P/--profile helper. Because the real CLI otherwise resolves an active/default profile, a response can pass this not-ready scenario after attempting the change against the wrong connector profile. Require the named profile in the accepted setup invocation.
skill-evals/cases/basecamp-connect/route-quoting.yml:19 - This assertion does not require the setup command to target the
helperprofile, soconnect setup --route '222=…'can pass even though it may modify whichever profile is active instead of the agent named by the task. Add a separate acceptance pattern for-P/--profile helper, as the first-time case already does.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The styled renderer converts a value it takes for HTML, so a route path holding <b> lost it. Paths now escape < as \x3c in both formats, and a test pushes a path with HTML, a link and backticks through the real renderers. The route count reads before the routes, and the two connector evals require the setup run to name the profile.
|
🤖 The last Copilot review noted, in suppressed comments, that two connector evals accepted a setup run without asserting the profile. Fixed in faf9e7d: not-ready-agent-reads and route-quoting now both require |
There was a problem hiding this comment.
🔵 Needs a closer look
The first-time flow can retain undisclosed existing policy, and the catalog advertises unavailable connector execution.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/commands/commands.go:149
- The catalog still tells users this command runs a connector, but this PR and the command itself explicitly state that starting/running is not implemented; only
setupandshowexist. Update the user-facing description so help does not advertise unavailable behavior.
skills/basecamp-connect/SKILL.md:226 - This first-time flow explicitly skips the policy inspection in step 4. If the named profile already has
connect.json, setup preserves every route and setting not passed, so the flow can add the requested route while leaving undisclosed old trust/routes in place. Inspect the policy too and switch to the later-change flow when one exists.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The guided first setup skipped inspection, so adding a route to a profile that already had one would keep trust and routes nobody had mentioned. It now inspects first and hands over to the change flow when a policy exists. The command catalog said connect runs a local agent connector; it sets one up. The eval mocks answer per profile, so a run that checks the operator's profile gets the operator's answer.
|
🤖 The last Copilot review noted two suppressed comments, both fixed in 6a5b35b. The catalog description now says connect sets up a connector rather than runs one, matching the command's own help. And the guided first setup now inspects the profile fully first: if |
There was a problem hiding this comment.
🟡 Changes recommended
connect show -P cannot reach its intended not-found handling for unknown profiles because root profile resolution rejects them first.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
An unknown -P is refused for every command before it runs, which is how a typo is told from a profile that exists with no connector policy. Show's help and the skill now say so, instead of implying not_found covers both.
There was a problem hiding this comment.
🟢 Approved
The implementation, security checks, documentation, and automated coverage are consistent and no unresolved defects were identified.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Connecting an agent and setting up the connector is two commands,
basecamp auth agent connectandbasecamp connect setup, with a dozen flags, a trust file and five exit codes. Today a person has to learn all of that and type it. It should be managed from an agent: the person says which projects and which directories, and the agent does the rest.Tracked in 24 service install, the skill and the plugins. It follows Add basecamp connect setup: record trust, write connect.json, check readiness and Add the agent-connection handshake: basecamp auth agent connect.
Why
The setup half only needs those two commands, and both are merged, so it doesn't have to wait for the connector run. The skill is working knowledge an agent can manage from, with a guided first setup as one section of it.
What changes
A
basecamp-connectskill inskills/, which ships wherebasecamp-doctordoes. It covers:--operator-profile.--with-tokenor--with-client-credentialsis never followed.basecamp meconfirms the credential's identity before any setup.service installsubcommand, status/doctor/redispatch and the plugins, named as coming with card 24 behind step 21 so an agent does not look for them.basecamp connect show -P <profile>, a read-only command that prints connect.json through setup's own loader, so a symlink, a file others could write, or another profile's policy is refused, not shown. It renders every setting for a person, not only as JSON. The skill inspects policy only through it, never with a raw file read.Evals. Four cases, run against this skill's own SKILL.md (
make -C skill-evals eval-connect, also part ofmake skill-eval):$, for a project whose name contains$(...);The runner can now grade the agent's final reply (
accept_response/reject_response), since explaining a failure is the behavior under test.Drift.
make check-skill-driftcovers the new skill, and the release manifest test expects it.Verification
Neither this machine nor CI has an Anthropic API key, so CI's Skill Evals job skips. I ran the cases through the Claude Code CLI instead (Sonnet). A mock MCP server served the runner's
basecamptool and mocks, and the runner's owngradedid the grading.Against this skill: all four cases passed in 3 of 3 runs, on each head since they were added.
Against a deliberately wrong instruction, each case failed:
basecamp connect runbasecamp meand go straight to setupIn the passing not_ready run the model followed setup's own hint over the skill.
Mutations that proved nothing. Two earlier wrong instructions did not make their case fail, because the model refused or ignored them: pasting a token into
auth login --with-token, and dropping quoting altogether. They are not counted as proof.Every command and flag in the skill was also checked against the exact
.surfacecommand, not the parent-resolving drift check. A mutated flag or subcommand fails that check.bin/cipassed on the head that addsconnect show. Its refusal tests go red against a raw read of the file and pass with the loader.