Add basecamp connect setup: record trust, write connect.json, check readiness

[jorgemanrubia]

The connector has admission, which reads trust and routes from connect.json, and a way to connect an agent (basecamp auth agent connect). Nothing writes connect.json, and nothing checks that a connected profile can actually run a connector. Today that means hand-writing the trust anchor and finding out at run time that Basecamp refuses the agent's reads.

Tracked in 16 basecamp connect setup.

Why

connect.json is the only authority for who may drive the agent and which directory a project's work runs in. It has to come from something that verifies the identities in it and refuses a copy someone else could have edited. It is also where basecamp connect starts, so this adds the command group.

Scope: setup on a connected profile

basecamp connect setup -P <profile> works on a profile that already holds the agent's credential. It never obtains, changes or removes a credential. A profile with none is refused, and the refusal names the command that connects it:

basecamp auth agent connect -P agent                              # an Agent person
basecamp connect setup -P agent --operator-profile me --route 12345=~/Work/app

A first version also ran the connection itself and undid it on failure. Six review rounds kept finding the same kinds of problem there: locks released before the undo, a concurrent login's credential deleted by key, prose and a JSON envelope sharing stdout, and a stale account after the child step. A design pass over the invariants split it out. Setup running the credential step follows in its own PR, once the credential store can compare-and-delete what a run stored.

What changes

• Account. The account is the one the agent's profile is bound to. It is never taken from a config-wide default, and an explicit --account naming another account is refused.

• Identity. An Agent credential must read back as an Agent person with a positive id. A bot login (--expect-identity, v1's path) must be the pinned identity. On a rerun, the agent and account must match what connect.json records.

• Trust. Keyed on Person id, and never guessed.

  • A first setup names the operator, either by their own profile (--operator-profile, whose credential proves who they are) or by --operator <id>.
  • The operator and every --allow id go through one check before anything is written: a person in the account, not the agent, not an Agent, not a client. Person reads go through the operator's credential when there is one, since Basecamp refuses them to an Agent today.
  • An unverifiable id is refused, unless connect.json already records it in the same role.

• connect.json lives at <config>/connect/<profile>/connect.json. Trust and routes are written in admission's own types, and every write is re-parsed through admission.ParsePolicy + Validate.

  • It also records watch_completions per route, class, driver (spawn), concurrency (2), deadline (45m), worktrees, and the agent's person id, kind and canonical account id.
  • Rerunning setup changes only what is passed, and every setting can be reversed: --remove-route, --no-watch-completions, --class <id>=, --trust operator, --worktrees=false.

• Private file. connect.json is written 0600 in 0700 directories, atomically, under a per-profile setup lock. It is refused on read, and never replaced on write, in these cases:

  • the file, the config directory, connect/ or the profile directory is a symlink, not regular, owned by another user, or writable by group or others;
  • any ancestor up to /, as written or as its symlinks resolve, belongs to someone other than root or this user, or is writable by others without being sticky.

  Where ownership can't be verified (Windows), setup refuses. Unknown, duplicate or non-canonical keys are refused, and so is trailing data.

• Locking. The last look at the credential and the write are one step under that credential key's own cross-process lock, which every login, refresh, import and logout already takes, so no command can replace the profile's credential between them. setup.Save takes the held-lock value, so a write without the lock does not compile. Where the configuration directory's filesystem cannot lock at all — no writable lock directory, no flock — setup refuses with lock_unavailable and changes nothing, rather than running unsynchronized as an ordinary credential operation would. Another setup or another command holding the profile is reported as busy; both exit with the rate-limit status.

• Bound to the identity. connect.json records the agent's person id, kind and pinned identity, and File.VerifyAgent refuses a file that does not describe the identity a profile's credential authenticates as. Setup runs it inside the locked section; the connector runs it at start-up, so a credential replaced afterwards stops the connector instead of letting it act as the wrong agent.

• Checks, run as the agent: token, identity, trust, scope (only a granted full passes), stream-ticket mint, and each routed project's project and project-people reads.

  • A refused read under an Agent identity says, in words, that Basecamp refuses these reads to Agent identities today (47 Admission's reads are refused under an Agent identity in bc3). The check also names the bot-user path.
  • Readiness is derived from the checks, never stored. connect.json is written only when every check passes.

• Exit status.

  • usage: refused input.
  • auth: no credential, an unreadable one, or one that isn't the agent it should be. Every credential conflict names the command that fixes it.
  • busy / lock_unavailable: another command holds the profile, or the host cannot lock. Nothing was changed.
  • not_ready: a readiness check failed, and nothing was written.

Credential handling

Setup holds no secret, and connect.json holds no credential. The stream ticket is minted through the SDK's EventFeed().CreateStreamTicket and dropped at once. A failed read is reported by HTTP status only, so no server-written body reaches output, including a failed mint's. Setup's reads go through an SDK client with no request hooks, debug logger or cache, so -v cannot print a feed URL. Setup builds no feed connection and no poll source.

Merge order

No go.mod, vendored model or Nix hash change. The instance lock and ledger in Take the account event feed into a durable ledger are keyed on account and agent Person id. connect.json records both, with the account in the same canonical spelling.

[Copilot]

🟡 Changes recommended

Trust-anchor path validation, operator persistence, credential preflight, and readiness reporting contain unresolved correctness and security issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds basecamp connect setup to create and validate connector policy for an authenticated agent profile.

Changes:

• Adds credential, identity, operator, ticket, and route checks.
• Introduces validated, atomic connect.json persistence.
• Registers and documents the new command surface.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries

File	Description
skills/basecamp/SKILL.md	Documents connector setup.
internal/connector/setup/private.go	Implements private file I/O.
internal/connector/setup/private_unix.go	Adds Unix security checks.
internal/connector/setup/private_unix_test.go	Tests Unix file protections.
internal/connector/setup/private_other.go	Adds non-Unix file handling.
internal/connector/setup/file.go	Defines connect.json.
internal/connector/setup/file_test.go	Tests schema and validation.
internal/connector/setup/checks.go	Implements readiness checks.
internal/connector/setup/checks_test.go	Tests readiness checks.
internal/connector/setup/apply.go	Applies incremental configuration.
internal/connector/setup/apply_test.go	Tests configuration changes.
internal/commands/doctor.go	Shares check rendering.
internal/commands/connect.go	Implements the new command.
internal/commands/connect_setup_test.go	Tests setup workflows.
internal/commands/commands.go	Adds command catalog entry.
internal/commands/commands_test.go	Registers command in tests.
internal/cli/root.go	Registers the root command.
e2e/smoke/smoke_lifecycle.bats	Marks interactive setup out of scope.
.surface	Records the CLI surface.

Review details

Suppressed comments (2)

internal/commands/connect.go:403

• An explicit --deadline 0/0s is silently treated as absent, even though zero is below MinDeadline; this contradicts the pre-network validation applied to other invalid deadlines. Preserve whether the flag was supplied and reject zero.

	if f.deadline != 0 {

internal/commands/connect.go:460

• This login branch runs before checking the existing policy's credential kind and pinned identity. If an Agent-backed connect.json has been logged out, rerunning with --expect-identity performs and stores a bot login, then fails the later kind check while claiming nothing changed; a bot policy can similarly consume/store a login for a different identity before the person mismatch is detected. Compare expect against existing.Agent.Kind and existing.Agent.IdentityID before launching either ceremony.

	case kind == "" && expect != 0:

• Files reviewed: 19/19 changed files
• Comments generated: 6
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Trust verification, non-Unix privacy enforcement, strict parsing, and result reporting have unresolved correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 19/19 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Machine output, strict parsing, terminal sanitization, and documented error-code behavior have unresolved correctness issues.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

internal/commands/connect.go:359

• path includes the environment-controlled config directory, so it can contain newlines or terminal control sequences. This direct one-line styled write bypasses the sanitization used by the error renderer and lets such a path break or restyle terminal output; quote or sanitize it before rendering.
  internal/commands/connect.go:372
• The advertised contract says every failed token, identity, operator, scope, ticket, or route check exits with code not_ready, but this code is limited to post-write failures. Token failures return auth_required at lines 272-273, identity failures return auth errors at 280-286/630-654, and operator failures return usage at 322-323, so automation receives different codes for the documented checks. Preserve the pre-write behavior, but normalize these check failures to not_ready (or narrow the documented contract).

internal/connector/setup/file.go:248

• Decoder.More is only a container-element check; after the top-level decode it returns false when the next non-whitespace byte is ] or }. Consequently, a valid policy followed by an unmatched closing delimiter is accepted here even though admission.ParsePolicy/json.Unmarshal rejects the same file, breaking the promised parser round-trip. Check that only whitespace remains after InputOffset (or decode once more and require EOF).

	if dec.More() {
		return File{}, errors.New("parse connect.json: trailing data after the object")
	}

internal/commands/connect.go:352

• This result is constructed only after setup.Save succeeds, but Written is never set, so every machine-readable success reports "written": false for a file that was just written. Set it to true here (and assert it in the JSON-output test).

		Checks:        summarizeChecks(asDoctorChecks(checks)),

• Files reviewed: 19/19 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Credential scope can be misreported and ticket error handling can expose server-returned bearer data.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

internal/commands/connect.go:282

• This scope check reads profile configuration rather than the stored credential that just produced the token. Those values can diverge—for example, reconnecting an existing profile can store an approval downgraded to read without updating ProfileConfig.Scope—so setup can report a read-only connector as ready (or reject a full credential because of stale config). Load the credential's effective Scope from the credential store after AccessToken and base readiness on that value.
  internal/commands/connect.go:350
• path comes from the configurable config-directory path and is written directly to a one-line terminal sink. Newlines or terminal escapes in that path can inject or restyle output. Sanitize it with richtext.SanitizeSingleLine before rendering, as the new route-check output already does.
  internal/commands/connect.go:673
• This generated copy-paste command embeds a config-derived profile name without shell quoting. Profile keys loaded from configuration are not constrained here, so characters such as spaces or shell metacharacters can change the command when pasted. Use shellQuote(profile), following the established convention in internal/commands/templates.go:295-300 and internal/commands/files.go:1908-1918.

• Files reviewed: 20/20 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Credential scope can be misclassified, failed setup can retain a mismatched credential, and one styled path output is unsanitized.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/commands/connect.go:350

• path comes from XDG_CONFIG_HOME/the home directory and may contain newlines or terminal escape bytes. Writing it directly to this styled terminal sink allows output injection; quote or sanitize it before rendering.

internal/commands/connect.go:283

• The readiness check uses profile configuration rather than the credential that is actually being exercised. Those scopes can diverge: an agent approval/mint can narrow the stored credential to read, while an existing profile entry is not updated, so a read-only connector can be reported ready (and a full credential can be rejected because of stale config). Re-read Credentials.Scope after AccessToken and base this check on that value.

	if scope := profileScope(app, name); scope == "read" {
		checks = append(checks, setup.Check{Name: "Scope", Status: setup.StatusFail,
			Message: "The credential is read-only: the agent could not reply or acknowledge",
			Hint:    "Reconnect with full access."})
	}

• Files reviewed: 20/20 changed files
• Comments generated: 1
• Review effort level: Balanced

[jorgemanrubia]

🤖 On the two previously-missed comments in Copilot's reviews of 2c8f2ae and 79005d6, both fixed in 689a436:

• connect.go styled path output: every printed path now goes through richtext.SanitizeSingleLine, both in the styled line and in the not_ready error. TestConnectSetupSanitizesThePathItPrints uses a config directory whose name contains a newline, and is red on 79005d6.
• Scope from profile config: readiness now comes from setup.ScopeCheck on the stored credential's granted scope, never from the profile's configuration. Only full passes; a Launchpad grant, which carries no scope, counts as full. read, a missing scope and unknown values fail: TestScopeCheck is table-driven. TestConnectSetupReadsTheGrantedScopeNotTheProfiles uses a profile configured full with a read grant, and is red on 79005d6.
• Unquoted profile name (connect.go:673): suggested commands now shell-quote profile names. TestConnectSetupQuotesProfileNamesInSuggestedCommands is red on 79005d6.

[Copilot]

🟡 Changes recommended

Raw SDK usage violates repository policy, and transaction failure paths can leave or overwrite configuration state.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

internal/commands/connect.go:325

• The child can modify config.json before storing the credential: agent connect's BeforeStore commits the profile (internal/commands/auth_agent.go:249-280), then the credential write happens afterward (internal/auth/agent.go:648-654). If that write fails, this probe finds no credential, leaves tx.connected false, and the outer function skips rollback, so a failed setup leaves the profile registration/binding behind. Track the config mutation independently and restore it on this failure path too.

	if held == "" && connectCredentialStored(cmd.Context(), app) {
		tx.connected = true
	}

• Files reviewed: 20/20 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Lock failure classification and lock-proof enforcement do not yet uphold the documented concurrency and security guarantees.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 23/23 changed files
• Comments generated: 4
• Review effort level: Balanced

[jorgemanrubia]

🤖 On lock classification and the lock proof (Copilot's review of f6ac37f): both finished structurally in 9660119.

Classification. Every acquisition outcome is decided in lockFile and nowhere else:

• held: the release is returned;
• held by another: the wait expires into a retryable rate_limit refusal, and nothing is changed;
• cannot be created at all: an ordinary credential operation warns once and proceeds, as it always has, while a caller that asked for the lock itself (require, which only WithCredential sets) is refused with lock_unavailable. That branch returns an error, so there is no path that returns neither a lock nor an error.

Proof. auth.HeldCredential is now an interface with an unexported method, so no other package can implement or fabricate it, and WithCredential invalidates the value when the lock is released (Credentials and Key answer empty from then on). setup.Save takes one and refuses anything that is not live.

Writers. grep -rn 'setup.Save(\|writePrivate(' internal --include=*.go outside tests gives exactly two lines: the one call in connect.go, and writePrivate inside save, which only Save calls. No write path without the proof remains.

Tests: TestHeldCredentialIsInvalidAfterTheLockIsReleased, TestSaveRefusesWithoutAHeldCredential, TestWithCredentialRefusesWhenTheHostCannotLock, TestConnectSetupRefusesWhenTheHostCannotLock.

[Copilot]

🔵 Needs a closer look

Lock failure classification, retry metadata, and cancellation propagation need correction.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

internal/commands/connect.go:345

• These helpers turn every request error into a failed check. If the command context is canceled during the ticket mint or a project read, setup therefore returns not_ready and tells the user to fix readiness instead of propagating cancellation. Check ctx.Err() after each helper and return it before classifying the report.
  internal/commands/connect.go:428
• busy is transient, but this error leaves Retryable false. JSON consumers use that field—not the exit code or hint—to decide whether to retry (internal/output/output_test.go:473-476), so they will treat setup-lock contention as a permanent verdict. Set Retryable: true for this error.

internal/connector/setup/private.go:66

• If the filesystem rejects flock (for example, an unsupported network/FUSE mount), this returns a plain error. runConnectSetup treats every non-ErrSetupRunning error from Lock as usage, so this path never produces the promised lock_unavailable code. Wrap acquisition failures in a distinguishable sentinel/typed error and map that to CodeLockUnavailable.

	held, err := lock.TryLock()
	if err != nil {
		return nil, fmt.Errorf("take the setup lock: %w", err)

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Setup-lock acquisition failures are incorrectly classified as usage rather than lock_unavailable.

Review details

Suppressed comments (1)

internal/commands/connect.go:192

• A failure to create or flock the per-profile setup lock reaches this branch too, so an unsupported/read-only locking filesystem is currently reported as usage. That contradicts the command's lock_unavailable contract and prevents callers from distinguishing bad input from a host that cannot provide the required synchronization. Have setup.Lock wrap acquisition failures in a dedicated sentinel and map that sentinel to output.CodeLockUnavailable here, while retaining privacy/path failures as usage.

		return output.ErrUsage(err.Error())

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Lock classification, identity pinning, retry metadata, and credential-lock proof remain incomplete.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

internal/commands/connect.go:324

• On a rerun for a bot profile, an explicit --expect-identity replaces the identity pinned in the existing file before VerifyAgent runs. If the credential now reports that new identity while /my/profile still yields the same person ID, setup silently rebinds connect.json, despite the requirement that a rerun match the recorded agent identity. Compare expect with existing.Agent.IdentityID and require removing the old file for a deliberate rebind.
  internal/commands/connect.go:430
• This transient contention error leaves Retryable false. Machine-output consumers explicitly key on retryable rather than the code (internal/output/envelope.go:59-65), so they will treat busy as a permanent verdict even though the hint says to retry after the other process finishes.

internal/connector/setup/file.go:380

• The lock key returned by auth.Held is discarded, so a live hold for any credential (for example profile:other) authorizes writing this profile's policy. This defeats Save's documented type-level guarantee that the credential named by the file is the one locked. Require the held key to equal the credential key derived from f.Profile.

func Save(held auth.HeldCredential, path string, f File) error {
	if _, _, ok := auth.Held(held); !ok {
		return errors.New("connect.json is written only while the profile's credential is locked")
	}

internal/commands/connect.go:192

• When the setup lock file cannot be created or flock is unsupported, setup.Lock returns a non-busy error and this branch converts it to usage. That violates the command contract for an unavailable setup lock (lock_unavailable, exit 5); the existing test only exercises the later credential lock. Give setup-lock acquisition failures a distinct sentinel/type and map that to CodeLockUnavailable, while keeping privacy/path validation errors as usage.

	if err != nil {
		if isLockBusy(err) {
			return errBusy(name, err)
		}
		return output.ErrUsage(err.Error())

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[jorgemanrubia]

🤖 On the suppressed comments in the last three reviews, all fixed in db18519:

• Cancellation (connect.go:345): a cancelled run now returns the context's error rather than a readiness verdict. TestConnectSetupPropagatesCancellation cancels during the ticket mint; red without the check.
• busy not retryable (connect.go:428/430): Retryable is set, and credential-store contention is mapped to busy too.
• setup.Lock acquisition failures (private.go:66, connect.go:192): typed ErrLockUnavailable, mapped to lock_unavailable.
• Identity rebinding on a rerun (connect.go:324): --expect-identity naming another identity than connect.json records is refused as auth, and says to remove the file for a deliberate rebind. TestConnectSetupRefusesToRebindToAnotherIdentity is red without it.
• The held key was discarded (file.go:380): setup.Save now requires the held lock to be the one for the file's own profile, through auth.ProfileCredentialKey. TestSaveRefusesAHoldOnAnotherProfile holds profile:other and is refused.

[Copilot]

🟡 Changes recommended

Required credential locks need hardened paths, and final-lock cancellation is currently misclassified as usage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/commands/connect.go:411

• Cancellation while waiting for or reading under the final credential lock is reclassified as a usage error here. WithCredential returns context.Canceled/context.DeadlineExceeded directly, but classifyWriteError falls through to ErrUsage, unlike the earlier cancellation check which preserves the context error. Preserve context cancellation before applying write-error classifications so Ctrl-C during lock contention remains an interruption rather than invalid input.

• Files reviewed: 24/24 changed files
• Comments generated: 1
• Review effort level: Balanced

[jorgemanrubia]

🤖 On the suppressed comment (connect.go:411, cancellation under the final lock classified as usage): fixed in b526e73. classifyWriteError returns a context.Canceled or context.DeadlineExceeded as it is, before any other classification, so Ctrl-C while waiting for the credential lock stays an interruption and nothing is written. TestConnectSetupClassifiesCancellationUnderTheLock covers both, red on db18519.

[Copilot]

🟢 Approved

The implementation and extensive tests consistently enforce the stated identity, readiness, confidentiality, and locking guarantees.

Review details

• Files reviewed: 26/26 changed files
• Comments generated: 0 new
• Review effort level: Balanced