diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 60f14e171..ca132b5f4 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -430,11 +430,18 @@ func (m *Manager) agentMintRefusal(resp *http.Response, body []byte, mint *agent // fetch its secret again for one is advice that cannot help. bareUnauthorized := code == "" && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) if clientRefusalCodes[code] || bareUnauthorized { - return m.agentRemedy(output.ErrAuth("Minting an agent token was refused ("+detail+")"), mint.clientID, mint.scope) + refused := output.ErrAuth("Minting an agent token was refused (" + detail + ")") + refused.Cause = ErrAgentCredentialRefused + return m.agentRemedy(refused, mint.clientID, mint.scope) } return statusFailure("minting an agent token: "+detail, resp) } +// ErrAgentCredentialRefused is the cause of a mint the token endpoint +// refused for the credentials themselves: the agent was disconnected in +// Basecamp, or connected on another computer, which replaced its secret. +var ErrAgentCredentialRefused = errors.New("the agent's credential was refused") + // clientRefusalCodes are the RFC 6749 §5.2 codes that say THE CREDENTIALS // PRESENTED are wrong, which is the only thing piping the secret in again // can repair. diff --git a/internal/commands/auth_agent.go b/internal/commands/auth_agent.go index ef07dfcd7..a70239f35 100644 --- a/internal/commands/auth_agent.go +++ b/internal/commands/auth_agent.go @@ -88,49 +88,9 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P return output.ErrUsage("Invalid scope. Use 'read' or 'full'") } - target, err := resolveAgentConnectProfile(app) - if err != nil { - return err - } - - w := cmd.OutOrStdout() - r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) - - var isDefault bool - ctx, stop := loginContext(cmd) - result, err := app.Auth.ConnectAgent(ctx, auth.AgentConnectOptions{ - DeviceName: connectDeviceName(deviceName), - SoftwareName: softwareName, - Scope: scope, - NoBrowser: noBrowser, - Local: local, - Logger: func(msg string) { fmt.Fprintln(w, msg) }, - Progress: w, - BeforeStore: func(conn *auth.AgentConnection) error { - registered, commitErr := target.commit(app, conn) - isDefault = registered - return commitErr - }, + return connectAgentProfile(cmd, app, agentConnectFlags{ + deviceName: deviceName, softwareName: softwareName, scope: scope, noBrowser: noBrowser, local: local, }) - err = loginOutcome(ctx, err, w, r) - stop() - if err != nil { - return err - } - - fmt.Fprintln(w) - fmt.Fprintln(w, r.Success.Render(fmt.Sprintf("Connected profile %q to a Basecamp agent", target.name))) - fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: minted on demand, no refresh token", - target.name, result.AccountID, result.Scope))) - if target.existing == nil { - line := fmt.Sprintf("Created profile %q for account %s", target.name, result.AccountID) - if isDefault { - line += " (default)" - } - fmt.Fprintln(w, r.Muted.Render(line)) - } - fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Check it any time: basecamp auth status -P %s", target.name))) - return nil }, } @@ -143,6 +103,68 @@ client secret this holds. Drop the local copy with ` + "`basecamp auth logout -P return cmd } +// agentConnectFlags are the choices an agent connection takes. +type agentConnectFlags struct { + deviceName string + softwareName string + scope string + noBrowser bool + local bool + + // quiet leaves out what was stored, for the guided setup, which says + // in one line of its own which agent this computer is now connected as. + quiet bool +} + +// connectAgentProfile runs the agent-connection handshake for the active +// profile and says what it stored. Both `auth agent connect` and the guided +// `connect setup` run it, so a person sees one connection either way. +func connectAgentProfile(cmd *cobra.Command, app *appctx.App, f agentConnectFlags) error { + target, err := resolveAgentConnectProfile(app) + if err != nil { + return err + } + + w := cmd.OutOrStdout() + r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + + var isDefault bool + ctx, stop := loginContext(cmd) + result, err := app.Auth.ConnectAgent(ctx, auth.AgentConnectOptions{ + DeviceName: connectDeviceName(f.deviceName), + SoftwareName: f.softwareName, + Scope: f.scope, + NoBrowser: f.noBrowser, + Local: f.local, + Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Progress: w, + BeforeStore: func(conn *auth.AgentConnection) error { + registered, commitErr := target.commit(app, conn) + isDefault = registered + return commitErr + }, + }) + err = loginOutcome(ctx, err, w, r) + stop() + if err != nil || f.quiet { + return err + } + + fmt.Fprintln(w) + fmt.Fprintln(w, r.Success.Render(fmt.Sprintf("Connected profile %q to a Basecamp agent", target.name))) + fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Profile: %s · Account: %s · Access: %s · Token: minted on demand, no refresh token", + target.name, result.AccountID, result.Scope))) + if target.existing == nil { + line := fmt.Sprintf("Created profile %q for account %s", target.name, result.AccountID) + if isDefault { + line += " (default)" + } + fmt.Fprintln(w, r.Muted.Render(line)) + } + fmt.Fprintln(w, r.Muted.Render(fmt.Sprintf("Check it any time: basecamp auth status -P %s", target.name))) + return nil +} + // connectDeviceName is what the approval page calls this computer: what // the operator named, or this host's own name. A hostname that cannot be // read leaves it empty, and the flow refuses with the flag to pass — the diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 5f46a60b0..e359fade2 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -271,6 +271,17 @@ type connectSetupFlags struct { worker string parallel int deadline time.Duration + + // guided is the guided setup running this one: a setup that passes says + // so in a line, since the guided summary names the agent, its owner and + // its projects. A failure still lists every check. It only ever makes a + // connect.json, so it refuses one that appeared while it was asking. + guided bool + // shownAgent and shownAccount are the agent and account guided setup + // showed the person; setup refuses to save for any other (0 and "" when + // not guided). + shownAgent int64 + shownAccount string } func newConnectSetupCmd() *cobra.Command { @@ -291,11 +302,12 @@ On the bot-user path pass --expect-identity to setup as well, so it can prove the login is the bot and not you; later runs remember it. Operator. The person whose instructions the agent follows, keyed on Person -id. Name them by their own profile (--operator-profile, which proves who -they are), or by id (--operator), which the agent must be able to read — -Basecamp refuses that read to an Agent identity today, so on the agent -connection path use --operator-profile. With neither, setup keeps the operator -connect.json already has; on a first setup one of the two is required. +id. A personal agent's operator is its owner, whom Basecamp names in the +agent's own profile, so no flag is needed for one. Otherwise name them by +their own profile (--operator-profile, which proves who they are), or by id +(--operator), which the agent must be able to read. With neither, setup keeps +the operator connect.json already has; on a first setup of an agent with no +owner, one of the two is required. Trust. operator (default): the operator alone. allowlist: the operator and the people passed with --allow. project: the operator and any non-client @@ -328,18 +340,33 @@ as by any command. Run setup again to change any of it; what you do not pass is kept. +Guided. In a terminal, with none of the flags that set policy, setup walks +you through instead: it connects this computer to your agent when it is not +(or no longer) connected, takes your agent's owner as the operator, asks +which of your agent's projects it works in (all of them by default), and +writes connect.json, offering to set it up again when the one there can't +be used or is for another agent. It ends by saying how to start the +connector: in the folder it should work in, left running. Run it again any +time: it takes the next step, or says everything is set. + Examples: + basecamp connect setup # guided, in a terminal basecamp auth agent connect -P agent + basecamp connect setup -P agent --serve 12345 # a personal agent: its owner operates it basecamp connect setup -P agent --operator-profile me --serve 12345 basecamp connect setup -P agent --operator-profile me --trust allowlist --allow 111 --allow 222 basecamp connect setup -P bot --operator-profile me --expect-identity 4242 --serve 12345 basecamp connect setup -P agent --class 12345=internal --deadline 90m`, - Args: cobra.NoArgs, + Annotations: map[string]string{AnnotationProfileMayCreate: "true"}, + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { return fmt.Errorf("app not initialized") } + if connectSetupGuided(cmd, app) { + return runGuidedConnectSetup(cmd, app, &f) + } return runConnectSetup(cmd, app, &f) }, } @@ -419,16 +446,23 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) if existing.Profile != name { return output.ErrUsage(fmt.Sprintf("%s names profile %q, not %q", path, existing.Profile, name)) } + // Guided setup makes connect.json only where there was none. One written + // while it was asking is another setup's, and its trust may not be what + // guided setup told the person: it is left as it is. + if f.guided && exists { + return output.ErrUsageHint("This agent was set up by another command while setup was asking about it, so nothing was changed", + runGuidedSetupAgain(name)) + } // Everything refusable without the network is refused first. next, err := setup.Apply(existing, changes) if err != nil { return output.ErrUsage(err.Error()) } - if operatorID == 0 && f.operatorProfile == "" && existing.Trust.OperatorID == 0 { - return output.ErrUsageHint("Setup needs to know who the operator is", - "Pass --operator-profile (or --operator ). The operator is the person the agent takes instructions from, and is never guessed.") - } + // With no operator named or recorded, a personal agent's operator is the + // person it works for, which Basecamp says in the agent's own profile. + // Anyone else is never guessed: that is refused below, after the read. + operatorFromOwner := operatorID == 0 && f.operatorProfile == "" && existing.Trust.OperatorID == 0 var operatorMgr *operatorProfile if f.operatorProfile != "" { if operatorMgr, err = operatorProfileManager(ctx, app, f.operatorProfile); err != nil { @@ -497,6 +531,13 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) if err != nil { return output.ErrAuth(fmt.Sprintf("Could not read who profile %q is in account %s: %s", name, accountID, setup.ErrorText(err))) } + // Guided setup asked its questions about one agent; a credential stored + // under the profile since, for another, is not what the person answered + // for (Codex on #794). + if f.shownAgent != 0 && (me.ID != f.shownAgent || accountID != f.shownAccount) { + return output.ErrUsageHint("This computer was connected to a different agent while setup was asking about it, so nothing was set up", + "Run basecamp connect setup again.") + } identityCheck, err := checkConnectIdentity(ctx, app, client, kind, creds.OAuthType, me, expect) if err != nil { return err @@ -520,14 +561,25 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) trust.Recorded = existing.Trust } people := setup.Reader(reader) - if operatorMgr != nil { + switch { + case operatorFromOwner: + owner, ok, err := readAgentOwner(ctx, client.ForAccount(accountID), kind) + if err != nil { + return output.ErrAuth(fmt.Sprintf("Could not read who agent %q works for: %s", me.Name, setup.ErrorText(err))) + } + if !ok { + return errOperatorRequired() + } + trust.Operator = owner + trust.OperatorIsOwner = true + case operatorMgr != nil: op, opReader, err := resolveOperatorProfile(ctx, operatorMgr, f.operatorProfile, accountID) if err != nil { return err } trust.Operator = op people = opReader - } else { + default: if operatorID == 0 { operatorID = existing.Trust.OperatorID } @@ -608,7 +660,11 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) if !report.Ready() { return errConnectorNotReady(report) } - if styled { + switch { + case styled && f.guided: + renderGuidedChecks(w, report.Checks()) + return nil + case styled: renderChecksStyled(w, title, summary) fmt.Fprintf(w, " connect.json written: %s\n\n", richtext.SanitizeSingleLine(path)) return nil diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index d5dded65f..9953e8408 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -35,12 +35,13 @@ func newConnectDoctorCmd() *cobra.Command { Short: "Check what the connector needs to run", Long: `Check the connector for a set-up profile: connect.json, the token, the agent's identity, the stream ticket mint, the account feed, the ledger (its gaps, open -losses, hold and messages waiting for a person), the -worker the driver runs — the worker's own CLI on PATH under the spawn driver, -the pinned ACP adapter in the connector's adapters directory under the acp -driver, and the adapter's own refusal of configuration on this machine that -the connector cannot switch off, checked in the directory this command runs -in — and a handshake with the agent's Basecamp MCP server, started with a worker's +losses, hold and messages waiting for a person), the worker the driver runs — +under the spawn driver, the worker's own CLI started as the connector starts +it and asked, without any work or model call, whether it runs, knows the +connector's flags and is logged in; under the acp driver, the pinned ACP +adapter in the connector's adapters directory, and the adapter's own refusal +of configuration on this machine that the connector cannot switch off, +checked in the directory this command runs in — and a handshake with the agent's Basecamp MCP server, started with a worker's environment (without the basecamp_connect domain, which only a dispatched task's token opens). @@ -79,7 +80,7 @@ func runConnectDoctor(cmd *cobra.Command, _ []string) error { ) } checks = append(checks, ledgerChecks(ctx, p)...) - checks = append(checks, workerBinaryChecks(p.file)...) + checks = append(checks, workerChecks(ctx, p.file)...) checks = append(checks, mcpHandshakeCheck(ctx, p.name)) result := summarizeChecks(asDoctorChecks(checks)) @@ -186,6 +187,16 @@ func workerBinaries(file setup.File) []string { return []string{file.WorkerName()} } +// workerChecks is the worker as the connector would start it: the spawn +// driver's preflight, where the driver has one, and otherwise where its +// binary is found. +func workerChecks(ctx context.Context, file setup.File) []setup.Check { + if p, ok := connectWorkerPreflight(ctx, file); ok { + return preflightChecks(p) + } + return workerBinaryChecks(file) +} + // workerBinaryChecks looks for the worker where the driver that runs it // looks. The spawn driver runs the worker's own CLI, which is on PATH. The // acp driver runs a pinned adapter out of the connector's own npm prefix diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index 08efc3bbe..95c3ec7c1 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -330,8 +330,20 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { return p.app.OK(report, output.WithSummary(connectStatusSummary(report))) } +// notTakingWork is why the connector stopped taking work, when it did: its +// worker could not start (connector.StartFailuresToHold). +func notTakingWork(s connector.Status) (string, bool) { + if s.Connection == nil || s.Connection.State != connector.ConnectionNotTakingWork { + return "", false + } + return s.Connection.Detail, true +} + func connectStatusSummary(r connectStatusReport) string { parts := []string{} + if _, ok := notTakingWork(r.Status); ok { + parts = append(parts, "not taking work") + } if r.Status.Hold != nil { parts = append(parts, "held") } @@ -351,6 +363,9 @@ func renderConnectStatus(w io.Writer, r connectStatusReport) { title += " (shadow)" } fmt.Fprintf(w, "%s\n\n", title) + if why, ok := notTakingWork(s); ok { + fmt.Fprintf(w, " Not taking work: %s. %s\n\n", clean(why), connector.NotTakingWorkFix) + } switch { case r.LockHolder == nil: diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 6556c3ebd..0c6b5e7dd 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -757,6 +757,20 @@ func connectDispatcherOptions(d connectDispatch) connector.DispatcherOptions { Lines: d.Lines, Logger: d.Logger, StillRunning: connector.DefaultStillRunning, + Preflight: workerPreflight(d.Driver), + } +} + +// workerPreflight is the driver's preflight, for a dispatcher that stopped +// taking work because its worker could not start; nil for a driver without +// one. +func workerPreflight(d driver.Driver) func(context.Context) driver.Preflight { + p, ok := d.(driver.Preflighter) + if !ok { + return nil + } + return func(ctx context.Context) driver.Preflight { + return p.Preflight(ctx, connector.DefaultPolicy()) } } diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go new file mode 100644 index 000000000..81ab397c8 --- /dev/null +++ b/internal/commands/connect_setup_guided.go @@ -0,0 +1,697 @@ +package commands + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "slices" + "strconv" + "strings" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/spf13/cobra" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/driver" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/hostutil" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/tui" +) + +// The guided setup. Run in a terminal with none of the flags that set +// policy, `connect setup` works out where this computer and its agent are +// and moves them forward: connect this computer to the agent, reconnect one +// Basecamp stopped accepting, set up connect.json (the agent's owner as the +// operator, its projects by name), rebuild a connect.json that cannot be +// read, and offer to keep the connector running. Running it again either +// takes the next step or says everything is set. With a policy flag, or +// with nobody at the terminal, setup is the scriptable command it always +// was. + +// connectAgentProfileName is the profile the guided setup uses when none was +// named and the active one is not an agent's. +const connectAgentProfileName = "agent" + +// connectSetupPolicyFlags are setup's flags that decide policy. Any of them +// makes a run the scriptable one. +var connectSetupPolicyFlags = []string{ + "expect-identity", "operator", "operator-profile", "trust", "allow", + "serve", "unserve", "class", "watch-completions", "no-watch-completions", + "driver", "worker", "concurrency", "deadline", +} + +// connectSetupInteractive reports whether a person is at the terminal to +// answer. A variable so tests can say yes. +var connectSetupInteractive = setupCanRun + +// connectSetupConnection is how the guided setup connects this computer: as +// `auth agent connect` does, naming the software on the approval page and in +// Adminland's "Connected to". A variable so tests can keep the browser shut. +var connectSetupConnection = agentConnectFlags{softwareName: "basecamp connect"} + +// errNoProjectsYet is a new agent that is in no projects: the next step of a +// first setup, which happens in Basecamp, not a failure. +var errNoProjectsYet = errors.New("the agent isn't in any projects yet") + +// connectSetupAsk asks the guided setup's questions. A variable so tests +// can answer them. +var connectSetupAsk connectSetupPrompter = tuiConnectSetupPrompter{} + +type connectSetupPrompter interface { + Confirm(question string, yes bool) (bool, error) + Input(question string) (string, error) +} + +type tuiConnectSetupPrompter struct{} + +func (tuiConnectSetupPrompter) Confirm(question string, yes bool) (bool, error) { + return tui.Confirm(question, yes) +} + +func (tuiConnectSetupPrompter) Input(question string) (string, error) { + return tui.Input(question, "") +} + +// connectSetupGuided reports whether this run of setup is the guided one. +func connectSetupGuided(cmd *cobra.Command, app *appctx.App) bool { + for _, name := range connectSetupPolicyFlags { + if cmd.Flags().Changed(name) { + return false + } + } + return connectSetupInteractive(app) +} + +func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) error { + ctx := cmd.Context() + w := cmd.OutOrStdout() + r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + + if os.Getenv("BASECAMP_TOKEN") != "" { + return errEnvTokenShadows("connect setup cannot check the agent while BASECAMP_TOKEN is set") + } + // Before anything is connected: connecting replaces the secret another + // computer may be running this agent on, for a connector this one + // can't run. + if !connectSupportedOS(connectServiceGOOS) { + return connectUnsupportedOSError(connectServiceGOOS) + } + name, err := guidedSetupProfile(ctx, app) + if err != nil { + return err + } + if !isValidProfileName(name) { + return output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) + } + + connected, err := ensureGuidedAgentConnection(cmd, app, name, r) + if err != nil { + return err + } + agent, err := readGuidedAgent(ctx, app, name) + if err != nil { + return err + } + if connected { + fmt.Fprintln(w) + fmt.Fprintln(w, r.Success.Render("✓ Connected this computer as "+richtext.SanitizeSingleLine(agent.Me.Name))) + } + + path, err := setup.Path(config.GlobalConfigDir(), name) + if err != nil { + return output.ErrUsage(err.Error()) + } + file, err := guidedConnectFile(ctx, w, r, path, agent) + if err != nil { + return err + } + worker := setup.New(name) + if file != nil { + // A setup being resumed isn't run through setup's checks again, but + // the credential it will run on may have been replaced since. + if err := checkGuidedScope(ctx, app, name); err != nil { + return err + } + worker = *file + } + if err := checkGuidedWorker(ctx, w, r, worker, name); err != nil { + return err + } + if file == nil { + err := setUpGuidedConnectFile(cmd, app, w, r, agent, f) + if errors.Is(err, errNoProjectsYet) { + renderNoProjectsYet(w, agent, name) + return nil + } + if err != nil { + return err + } + loaded, err := setup.Load(path) + if err != nil { + return output.ErrUsage("connect.json cannot be used: " + err.Error()) + } + file = &loaded + } + + renderGuidedSummary(w, r, agent, *file, connectorRunning(*file), name) + return nil +} + +// checkGuidedScope refuses a credential that can't reply or acknowledge, as +// setup's own scope check does on a first setup. +func checkGuidedScope(ctx context.Context, app *appctx.App, name string) error { + creds, err := app.Auth.GetStore().LoadContext(ctx, app.Auth.CredentialKey()) + if err != nil { + return output.ErrAuth("Could not read this computer's connection to your agent: " + err.Error()) + } + c := setup.ScopeCheck(creds.OAuthType, creds.Scope) + if c.Status != setup.StatusFail { + return nil + } + return output.ErrUsageHint(richtext.SanitizeSingleLine(c.Message), + "Reconnect with full access: basecamp auth agent connect -P "+richtext.ShellQuote(name)+". "+runGuidedSetupAgain(name)) +} + +// guidedSetupProfile is the profile the guided setup works on: the one named +// on the command line or in the environment, else the active profile when it +// holds an agent, else the agent profile, created if need be by connecting. +func guidedSetupProfile(ctx context.Context, app *appctx.App) (string, error) { + name := app.Config.ActiveProfile + if app.Flags.Profile != "" || os.Getenv("BASECAMP_PROFILE") != "" { + return name, nil + } + if name != "" { + kind, err := credentialKindOf(ctx, app.Auth) + if err != nil { + return "", err + } + if kind == setup.KindAgent { + return name, nil + } + } + if _, ok := app.Config.Profiles[connectAgentProfileName]; ok { + if err := app.Config.ApplyProfile(connectAgentProfileName); err != nil { + return "", err + } + // As root does after applying a profile: what this invocation named, + // in its environment and flags, outranks the profile's own values, + // so an --account the profile isn't bound to is refused rather than + // replaced. + if err := config.LoadFromEnv(app.Config); err != nil { + return "", err + } + config.ApplyOverrides(app.Config, config.FlagOverrides{ + Account: app.Flags.Account, + Project: app.Flags.Project, + Todolist: app.Flags.Todolist, + CacheDir: app.Flags.CacheDir, + }) + // Root checked the base URL of the profile it started with; this one + // is checked as root would, before anything reaches it. + if err := hostutil.RequireSecureURL(app.Config.BaseURL); err != nil { + where := "the config file that defines the profile" + if path := profileFieldFile(app.Config, connectAgentProfileName, "base_url"); path != "" { + where = "the profile's entry in " + richtext.SanitizeSingleLine(path) + } + return "", output.ErrUsageHint(fmt.Sprintf("Profile %q's base_url: %s", connectAgentProfileName, err), + "Correct base_url in "+where+".") + } + } else { + app.Config.ActiveProfile = connectAgentProfileName + } + return connectAgentProfileName, nil +} + +// ensureGuidedAgentConnection leaves the profile holding a credential for an +// agent that Basecamp accepts: connecting this computer when it holds none, +// and offering to reconnect when Basecamp no longer takes the one it holds. +// It reports whether it connected. +func ensureGuidedAgentConnection(cmd *cobra.Command, app *appctx.App, name string, r *output.Renderer) (bool, error) { + ctx := cmd.Context() + w := cmd.OutOrStdout() + + kind := "" + if _, ok := app.Config.Profiles[name]; ok { + var err error + if kind, err = connectCredentialKind(ctx, app); err != nil { + return false, err + } + } + switch kind { + case "": + fmt.Fprintln(w, "First, connect this computer to your agent in Basecamp.") + fmt.Fprintln(w) + return true, connectGuidedAgent(cmd, app) + case setup.KindBotUser: + return false, output.ErrUsageHint(fmt.Sprintf("Profile %q holds a person's login, not an agent's", name), + "Set up the agent under a profile of its own: basecamp connect setup -P "+connectAgentProfileName) + } + + refused, err := agentCredentialRefused(ctx, app, name) + if err != nil || !refused { + return false, err + } + fmt.Fprintln(w, r.Warning.Render("This computer's connection to your agent was disconnected in Basecamp, or it was connected on another computer.")) + reconnect, err := connectSetupAsk.Confirm("Connect this computer to it again?", true) + if err != nil { + return false, err + } + if !reconnect { + return false, output.ErrAuth("This computer is not connected to the agent") + } + fmt.Fprintln(w) + return true, connectGuidedAgent(cmd, app) +} + +// connectGuidedAgent connects this computer, leaving out what was stored: +// the guided setup says which agent it connected in a line of its own. +func connectGuidedAgent(cmd *cobra.Command, app *appctx.App) error { + flags := connectSetupConnection + flags.quiet = true + return connectAgentProfile(cmd, app, flags) +} + +// agentCredentialRefused reports whether Basecamp refuses the profile's +// agent credential: the token endpoint refusing its secret, or the account +// refusing a token it minted before. +func agentCredentialRefused(ctx context.Context, app *appctx.App, name string) (bool, error) { + token, err := app.Auth.AccessToken(ctx) + if errors.Is(err, auth.ErrAgentCredentialRefused) { + return true, nil + } + if err != nil { + return false, output.ErrAuth(fmt.Sprintf("Profile %q holds a credential that does not produce a token: %s", name, setup.ErrorText(err))) + } + accountID, err := connectAccount(app, name) + if err != nil { + return false, err + } + client := connectSDKClient(app, &basecamp.StaticTokenProvider{Token: token}).ForAccount(accountID) + _, err = client.People().Me(ctx) + var apiErr *basecamp.Error + if errors.As(err, &apiErr) && apiErr.HTTPStatus == 401 { + return true, nil + } + return false, nil +} + +// guidedAgent is what the guided setup knows about the agent it sets up. +type guidedAgent struct { + Profile string + AccountID string + Me setup.Person + Owner setup.Person + HasOwner bool + Projects []guidedProject + Listed bool // whether Basecamp listed the agent's projects + client *basecamp.AccountClient + projectsE error +} + +type guidedProject struct { + ID int64 + Name string +} + +// readGuidedAgent reads the agent the profile holds: who it is, who it works +// for, and which projects it is in. +func readGuidedAgent(ctx context.Context, app *appctx.App, name string) (guidedAgent, error) { + accountID, err := connectAccount(app, name) + if err != nil { + return guidedAgent{}, err + } + client := connectSDKClient(app, &managerTokens{mgr: app.Auth}).ForAccount(accountID) + me, err := setup.SDKReader{Client: client}.Me(ctx) + if err != nil { + return guidedAgent{}, output.ErrAuth(fmt.Sprintf("Could not read who profile %q is in account %s: %s", name, accountID, setup.ErrorText(err))) + } + agent := guidedAgent{Profile: name, AccountID: accountID, Me: me, client: client} + if agent.Owner, agent.HasOwner, err = readAgentOwner(ctx, client, setup.KindAgent); err != nil { + return guidedAgent{}, output.ErrAuth(fmt.Sprintf("Could not read who agent %q works for: %s", me.Name, setup.ErrorText(err))) + } + agent.Projects, agent.projectsE = listAgentProjects(ctx, client) + agent.Listed = agent.projectsE == nil + return agent, nil +} + +// readAgentOwner is who a personal agent works for, as Basecamp names them in +// the agent's own profile (`boss`). ok is false for an agent that works for no +// one in particular, for a bot user's login, and for a Basecamp that does not +// say. +// +// The SDK's Person has no owner yet, so the profile is read through the +// account client's plain GET, the one `basecamp api get` makes, and only the +// one field is taken from it. +func readAgentOwner(ctx context.Context, client *basecamp.AccountClient, kind string) (setup.Person, bool, error) { + if kind != setup.KindAgent { + return setup.Person{}, false, nil + } + resp, err := client.Get(ctx, "/my/profile.json") + if err != nil { + return setup.Person{}, false, err + } + var profile struct { + Boss *struct { + ID int64 `json:"id"` + Name string `json:"name"` + } `json:"boss"` + } + if err := resp.UnmarshalData(&profile); err != nil { + return setup.Person{}, false, err + } + if profile.Boss == nil || profile.Boss.ID <= 0 { + return setup.Person{}, false, nil + } + return setup.Person{ID: profile.Boss.ID, Name: profile.Boss.Name, PersonableType: "User"}, true, nil +} + +// errOperatorRequired refuses a setup that names no operator for an agent +// Basecamp gives no owner. +func errOperatorRequired() error { + return output.ErrUsageHint("Setup needs to know who the operator is", + "Pass --operator-profile (or --operator ). The operator is the person the agent takes instructions from; only a personal agent's owner is taken without asking.") +} + +// listAgentProjects is every active project the agent is in. +func listAgentProjects(ctx context.Context, client *basecamp.AccountClient) ([]guidedProject, error) { + result, err := client.Projects().List(ctx, nil) + if err != nil { + return nil, err + } + projects := make([]guidedProject, 0, len(result.Projects)) + for _, p := range result.Projects { + projects = append(projects, guidedProject{ID: p.ID, Name: p.Name}) + } + return projects, nil +} + +// guidedConnectFile is the connect.json this profile already has, nil when +// the guided setup has to make one: there is none, or the person chose to +// set up afresh over one that cannot be used or names another agent. +func guidedConnectFile(ctx context.Context, w io.Writer, r *output.Renderer, path string, agent guidedAgent) (*setup.File, error) { + // What the person is asked about, to move aside only if it is still that. + shown, _ := os.ReadFile(path) + existing, err := setup.Load(path) + var problem string + switch { + case errors.Is(err, os.ErrNotExist): + return nil, nil + case err != nil: + problem = "Your connector's settings (connect.json) can't be used: " + setup.ErrorText(err) + case !guidedFileFits(existing, agent): + problem = "Your connector's settings (connect.json) are for a different agent than the one this computer is connected to." + default: + return &existing, nil + } + + fmt.Fprintln(w, r.Warning.Render(richtext.SanitizeSingleLine(problem))) + afresh, err := connectSetupAsk.Confirm("Set them up again? The old file is kept beside the new one.", true) + if err != nil { + return nil, err + } + if !afresh { + return nil, output.ErrUsageHint("connect.json was left as it is", + "Nothing was changed. Run setup again to set it up afresh, or fix "+richtext.SanitizeSingleLine(path)+" by hand.") + } + unlock, err := setup.Lock(ctx, path) + if err != nil { + return nil, classifyLockError(agent.Profile, err) + } + defer unlock() + // Read again under the lock: another setup may have repaired the file + // while this one was asking, and a repaired file is resumed, not moved + // aside. Any other change is another command's setup, perhaps for + // another agent, and isn't this one to move. + if again, err := setup.Load(path); err == nil && guidedFileFits(again, agent) { + return &again, nil + } + now, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil || !bytes.Equal(now, shown) { + return nil, output.ErrUsageHint("Your connector's settings (connect.json) were changed by another command while setup was asking, so nothing was changed", + runGuidedSetupAgain(agent.Profile)) + } + aside := path + ".broken-" + time.Now().UTC().Format("20060102T150405Z") + if err := os.Rename(path, aside); err != nil { + return nil, fmt.Errorf("could not move %s aside: %w", richtext.SanitizeSingleLine(path), err) + } + fmt.Fprintln(w, r.Muted.Render("Moved the old file to "+richtext.SanitizeSingleLine(aside))) + fmt.Fprintln(w) + return nil, nil +} + +// guidedFileFits reports whether a connect.json is for the agent this profile +// holds. +func guidedFileFits(f setup.File, agent guidedAgent) bool { + return f.Profile == agent.Profile && accountIDsEqual(f.AccountID, agent.AccountID) && f.Agent.PersonID == agent.Me.ID +} + +// setUpGuidedConnectFile writes connect.json through setup itself: the +// owner as the operator (setup's own default), and the projects the person +// picks by name. +// +// The owner is "you": only a personal agent's owner can connect a computer +// to it, so the person who approved this computer, and who is running setup, +// is the owner. +func setUpGuidedConnectFile(cmd *cobra.Command, app *appctx.App, w io.Writer, r *output.Renderer, agent guidedAgent, f *connectSetupFlags) error { + if agent.HasOwner { + fmt.Fprintln(w, r.Success.Render("✓ "+richtext.SanitizeSingleLine(agent.Owner.Name)+" (you) is the only person who can give it work.")) + } else { + return errOperatorRequired() + } + + serve, err := chooseGuidedProjects(w, agent) + if err != nil { + return err + } + fmt.Fprintln(w) + + setupFlags := *f + setupFlags.guided = true + setupFlags.shownAgent, setupFlags.shownAccount = agent.Me.ID, agent.AccountID + for _, p := range serve { + setupFlags.serve = append(setupFlags.serve, strconv.FormatInt(p.ID, 10)) + } + return runConnectSetup(cmd, app, &setupFlags) +} + +// chooseGuidedProjects asks which of the agent's projects it serves, +// defaulting to all of them. +func chooseGuidedProjects(w io.Writer, agent guidedAgent) ([]guidedProject, error) { + name := richtext.SanitizeSingleLine(agent.Me.Name) + if !agent.Listed { + return nil, output.ErrUsageHint(fmt.Sprintf("Could not list %s's projects: %s", name, setup.ErrorText(agent.projectsE)), + "Serve a project by its id: basecamp connect setup -P "+richtext.ShellQuote(agent.Profile)+" --serve ") + } + if len(agent.Projects) == 0 { + return nil, errNoProjectsYet + } + + names := make([]string, len(agent.Projects)) + for i, p := range agent.Projects { + names[i] = richtext.SanitizeSingleLine(p.Name) + } + question := fmt.Sprintf("Work in all %d of %s's projects? %s", len(names), name, strings.Join(names, ", ")) + if len(names) == 1 { + question = fmt.Sprintf("Work in %s's project, %s?", name, names[0]) + } + all, err := connectSetupAsk.Confirm(question, true) + if err != nil { + return nil, err + } + if all { + return agent.Projects, nil + } + + fmt.Fprintln(w, "Which projects should it work in?") + for i, n := range names { + fmt.Fprintf(w, " %d. %s\n", i+1, n) + } + answer, err := connectSetupAsk.Input("Numbers, separated by commas") + if err != nil { + return nil, err + } + var chosen []guidedProject + for part := range strings.SplitSeq(answer, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + n, err := strconv.Atoi(part) + if err != nil || n < 1 || n > len(agent.Projects) { + return nil, output.ErrUsage(fmt.Sprintf("%q is not a number from the list", part)) + } + chosen = append(chosen, agent.Projects[n-1]) + } + if len(chosen) == 0 { + return nil, output.ErrUsage("No projects were chosen, so nothing was set up") + } + return chosen, nil +} + +// checkGuidedWorker starts the worker as the connector would, before +// anything is written or anyone is told the agent is set up: an agent whose +// AI cannot start fails every request it is given, and the person who +// mentioned it is the last to be able to fix that. +func checkGuidedWorker(ctx context.Context, w io.Writer, r *output.Renderer, file setup.File, name string) error { + p, ok := connectWorkerPreflight(ctx, file) + if !ok { + return checkGuidedWorkerBinary(w, r, file, name) + } + for _, c := range p.Checks { + if c.Status == driver.PreflightWarn { + fmt.Fprintln(w, r.Warning.Render(richtext.SanitizeSingleLine(c.Message))) + } + } + failed, bad := p.Failed() + if !bad { + named := p.Product + if p.Version != "" { + named += " " + p.Version + } + fmt.Fprintln(w, r.Success.Render("✓ "+richtext.SanitizeSingleLine(named)+" is ready")) + return nil + } + hint := strings.TrimSpace(failed.Hint + " " + runGuidedSetupAgain(name)) + return output.ErrUsageHint(richtext.SanitizeSingleLine(failed.Message), richtext.SanitizeSingleLine(hint)) +} + +// checkGuidedWorkerBinary checks a worker whose driver has no spawn preflight +// (the acp driver) as doctor does: where the driver would find it. +func checkGuidedWorkerBinary(w io.Writer, r *output.Renderer, file setup.File, name string) error { + for _, c := range workerBinaryChecks(file) { + switch c.Status { + case setup.StatusWarn: + fmt.Fprintln(w, r.Warning.Render(richtext.SanitizeSingleLine(c.Message))) + case setup.StatusFail: + hint := strings.TrimSpace(c.Hint + " " + runGuidedSetupAgain(name)) + return output.ErrUsageHint(richtext.SanitizeSingleLine(c.Message), richtext.SanitizeSingleLine(hint)) + } + } + return nil +} + +// connectorRunning reports whether a connector for this setup is running: +// the instance lock's holder, as `connect status` reads it, is alive. +func connectorRunning(file setup.File) bool { + dir, err := connectStatePath(file, false) + if err != nil { + return false + } + holder, ok := connector.InstanceHolder(dir, file.AccountID, file.Agent.PersonID) + if !ok { + return false + } + return holderStillRuns(holder) +} + +// holderStillRuns reports whether the process a lock's metadata names is the +// one that took the lock. The metadata outlives a crash, and its pid can be +// given to something else since (Codex on #794), so a live pid is not +// enough: the process holding it must have started before the lock was +// taken. One the kernel handed the pid to afterwards started later. The lock +// itself is never touched: taking it, even briefly, could make a starting +// connector find it held. +func holderStillRuns(holder connector.InstanceHolderInfo) bool { + lockedAt, err := time.Parse(time.RFC3339, holder.StartedAt) + if err != nil { + return false + } + p, err := driver.LookupProcess(holder.PID) + if err != nil { + return false + } + // The metadata keeps whole seconds; the process may have started within + // the second the lock was taken. + return !p.StartedAt.After(lockedAt.Add(time.Second)) +} + +// renderGuidedChecks is a passing setup in the guided one's words: any +// warnings, by name, then that everything checks out. +func renderGuidedChecks(w io.Writer, checks []setup.Check) { + r := output.NewRendererWithTheme(w, false, tui.ResolveTheme(tui.DetectDark())) + warned := false + for _, c := range checks { + if c.Status == setup.StatusWarn { + fmt.Fprintln(w, r.Warning.Render(richtext.SanitizeSingleLine(c.Name+": "+c.Message))) + warned = true + } + } + if warned { + fmt.Fprintln(w, r.Success.Render("✓ Everything else checks out")) + } else { + fmt.Fprintln(w, r.Success.Render("✓ Everything checks out")) + } +} + +// renderNoProjectsYet is the step after connecting an agent that is in no +// projects: adding it to some, in Basecamp. +func renderNoProjectsYet(w io.Writer, agent guidedAgent, profile string) { + name := richtext.SanitizeSingleLine(agent.Me.Name) + fmt.Fprintln(w) + fmt.Fprintf(w, "%s isn't in any projects yet, so there's nothing for it to work on.\n", name) + fmt.Fprintf(w, "Next: add it to the projects it should work in (in Basecamp, Adminland → Manage agents → %s → Edit), then run `basecamp connect setup -P %s` again.\n", name, richtext.ShellQuote(profile)) +} + +// runGuidedSetupAgain is how a person picks setup up where it stopped: on the +// profile it was working on, which a bare `connect setup` may not choose again. +func runGuidedSetupAgain(name string) string { + return "Then run this again: basecamp connect setup -P " + richtext.ShellQuote(name) +} + +func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, file setup.File, running bool, name string) { + names := make(map[int64]string, len(agent.Projects)) + for _, p := range agent.Projects { + names[p.ID] = richtext.SanitizeSingleLine(p.Name) + } + served := make([]string, 0, len(file.Projects)) + for id := range file.Projects { + if n, ok := names[id]; ok { + served = append(served, n) + } else { + served = append(served, "project "+strconv.FormatInt(id, 10)) + } + } + slices.Sort(served) + + agentName := richtext.SanitizeSingleLine(agent.Me.Name) + fmt.Fprintln(w) + fmt.Fprintln(w, r.Success.Render(fmt.Sprintf("%s is set up on this computer", agentName))) + if agent.HasOwner { + fmt.Fprintf(w, " Works for: %s\n", richtext.SanitizeSingleLine(agent.Owner.Name)) + } + if len(served) == 0 { + // Every project was taken out of connect.json: a mention anywhere + // gets a holding reply, so there is nowhere to try it. + fmt.Fprintln(w, " Works in: no projects") + fmt.Fprintln(w) + fmt.Fprintf(w, "It isn't working in any projects, so a mention gets \"I'm not set up to work in this project yet\". To add one: basecamp connect setup -P %s --serve \n", richtext.ShellQuote(name)) + return + } + fmt.Fprintf(w, " Works in: %s\n", strings.Join(served, ", ")) + if running { + fmt.Fprintln(w, " Running: yes") + fmt.Fprintf(w, "\nMention %s in one of those projects to try it.\n", agentName) + } else { + // No background service yet: it would work in the home directory and + // is untested on real machines. The agent works, and may change files + // without asking, in the folder it is started in. + fmt.Fprintln(w, " Running: no") + fmt.Fprintln(w) + fmt.Fprintln(w, "To start it, run this in the folder it should work in, and leave it running.") + fmt.Fprintln(w, "It can change files in that folder without asking.") + fmt.Fprintln(w, " basecamp connect -P "+richtext.ShellQuote(name)) + fmt.Fprintf(w, "\nOnce it's running, mention %s in one of those projects to try it.\n", agentName) + } +} diff --git a/internal/commands/connect_setup_guided_test.go b/internal/commands/connect_setup_guided_test.go new file mode 100644 index 000000000..078fbd090 --- /dev/null +++ b/internal/commands/connect_setup_guided_test.go @@ -0,0 +1,630 @@ +//go:build unix + +package commands + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "testing" + "time" + + "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/connector" + "github.com/basecamp/basecamp-cli/internal/connector/admission" + "github.com/basecamp/basecamp-cli/internal/connector/driver" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// scriptedPrompter answers the guided setup's questions in order, and fails +// the question it has no answer for. +type scriptedPrompter struct { + confirms []bool + inputs []string + asked []string + onConfirm func(question string) +} + +func (p *scriptedPrompter) Confirm(question string, _ bool) (bool, error) { + p.asked = append(p.asked, question) + if p.onConfirm != nil { + p.onConfirm(question) + } + if len(p.confirms) == 0 { + return false, errors.New("unexpected question: " + question) + } + answer := p.confirms[0] + p.confirms = p.confirms[1:] + return answer, nil +} + +func (p *scriptedPrompter) Input(question string) (string, error) { + p.asked = append(p.asked, question) + if len(p.inputs) == 0 { + return "", errors.New("unexpected question: " + question) + } + answer := p.inputs[0] + p.inputs = p.inputs[1:] + return answer, nil +} + +// guided runs setup as if a person were at the terminal, answering with p, +// on Linux, with the browser kept shut and the state home a temp dir. +func guided(t *testing.T, p *scriptedPrompter) { + t.Helper() + prevInteractive, prevAsk, prevConnection, prevGOOS := connectSetupInteractive, connectSetupAsk, connectSetupConnection, connectServiceGOOS + connectSetupInteractive = func(*appctx.App) bool { return true } + connectSetupAsk = p + connectSetupConnection = agentConnectFlags{softwareName: "basecamp connect", noBrowser: true} + connectServiceGOOS = "linux" + t.Setenv("XDG_STATE_HOME", t.TempDir()) + t.Cleanup(func() { + connectSetupInteractive, connectSetupAsk, connectSetupConnection, connectServiceGOOS = prevInteractive, prevAsk, prevConnection, prevGOOS + }) + workerAnswers(t, readyWorker) +} + +// readyWorker is a worker whose preflight passes. +var readyWorker = driver.Preflight{Product: "Claude Code", Version: "2.1.283", Checks: []driver.PreflightCheck{ + {Name: driver.PreflightStarts, Status: driver.PreflightPass, Message: "Claude Code 2.1.283 starts (/usr/local/bin/claude)"}, + {Name: driver.PreflightFlags, Status: driver.PreflightPass, Message: "knows all 13 options the connector passes"}, + {Name: driver.PreflightLogin, Status: driver.PreflightPass, Message: "logged in"}, +}} + +// brokenLauncher is the worker a manual run met: a claude on PATH that hands +// over to a Claude Code that is not there. +var brokenLauncher = driver.Preflight{Product: "Claude Code", Checks: []driver.PreflightCheck{ + {Name: driver.PreflightStarts, Status: driver.PreflightFail, + Message: "The claude on your PATH is a launcher that couldn't find Claude Code (claude: line 2: /home/me/.local/share/mise/installs/claude/latest/claude: No such file or directory)", + Hint: "Reinstall Claude Code, or fix the launcher at /home/me/.local/bin/claude."}, +}} + +// workerAnswers answers the worker preflight with p, for the test. +func workerAnswers(t *testing.T, p driver.Preflight) { + t.Helper() + prev := connectWorkerPreflight + // As the real one: the acp driver has no spawn preflight. + connectWorkerPreflight = func(_ context.Context, f setup.File) (driver.Preflight, bool) { + return p, f.Driver != setup.DriverACP + } + t.Cleanup(func() { connectWorkerPreflight = prev }) +} + +func ownedByTheOperator(s *connectSetupServer) { + s.boss = map[string]any{"id": setupOperatorPerson, "name": "Operator"} +} + +func projectsNamed(ids ...int64) []map[string]any { + names := map[int64]string{setupProject: "Connector", setupProject2: "Launch"} + projects := make([]map[string]any, 0, len(ids)) + for _, id := range ids { + projects = append(projects, map[string]any{"id": id, "name": names[id]}) + } + return projects +} + +// A personal agent's operator is its owner, as Basecamp names them in the +// agent's own profile: no flag, and no second login. +func TestConnectSetupTakesAPersonalAgentsOwnerAsTheOperator(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), serveArg()) + require.NoError(t, err, out) + + assert.Contains(t, out, "the agent's owner") + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, setupOperatorPerson, f.Trust.OperatorID) +} + +// An agent Basecamp gives no owner still needs its operator named. +func TestConnectSetupWithoutAnOwnerStillNeedsAnOperator(t *testing.T) { + s := startConnectSetupServer(t) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), serveArg()) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "Setup needs to know who the operator is", apiErr.Message) + assertNotWritten(t, "agent") +} + +// From nothing: the guided setup connects this computer, takes the owner as +// the operator, serves every one of the agent's projects, and says so in a +// few plain lines: the ids, the profile and the file path stay with `auth +// agent connect`, `connect setup` with flags, and `connect doctor`. +func TestGuidedConnectSetupFromNothing(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject, setupProject2) + p := &scriptedPrompter{confirms: []bool{true}} + guided(t, p) + + out, err := runConnectSetupCmd(t, bareSetupApp(t, s, "agent")) + require.NoError(t, err, out) + + assert.Contains(t, out, "First, connect this computer to your agent") + assert.Contains(t, out, "✓ Connected this computer as Marie Chef\n") + assert.Contains(t, out, "✓ Operator (you) is the only person who can give it work.") + assert.Equal(t, []string{ + "Work in all 2 of Marie Chef's projects? Connector, Launch", + }, p.asked) + assert.Contains(t, out, "✓ Claude Code 2.1.283 is ready") + assert.Contains(t, out, "✓ Everything checks out") + assert.Contains(t, out, "Marie Chef is set up on this computer") + assert.Contains(t, out, "Works in: Connector, Launch") + assert.Contains(t, out, "Running: no") + assert.Contains(t, out, "To start it, run this in the folder it should work in, and leave it running.") + assert.Contains(t, out, " basecamp connect -P agent\n") + assert.Contains(t, out, "Once it's running, mention Marie Chef in one of those projects to try it.") + for _, internal := range []string{"Connected profile", "Account: 999", "auth status", "connect.json", "Identity", "Person " + strconv.FormatInt(setupOperatorPerson, 10)} { + assert.NotContains(t, out, internal) + } + + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, setupOperatorPerson, f.Trust.OperatorID) + assert.Len(t, f.Projects, 2) +} + +func TestGuidedConnectSetupPicksProjectsByNumber(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject, setupProject2) + guided(t, &scriptedPrompter{confirms: []bool{false}, inputs: []string{"2"}}) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + + assert.Contains(t, out, "1. Connector") + assert.Contains(t, out, "2. Launch") + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Len(t, f.Projects, 1) + assert.Contains(t, f.Projects, setupProject2) +} + +// Switching to the agent's profile keeps what this invocation named: an +// --account the profile isn't bound to is refused, as it is with -P, not +// quietly replaced by the profile's own (Codex on #794). +func TestGuidedConnectSetupKeepsAnExplicitAccount(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + connectSetupApp(t, s, "agent") + guided(t, &scriptedPrompter{confirms: []bool{true}}) + + app := newConnectSetupApp(t, s, "") + app.Flags.Account = "777" + app.Config.AccountID = "777" + app.Config.Sources["account_id"] = string(config.SourceFlag) + + out, err := runConnectSetupCmd(t, app) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "this command named account 777") + assertNotWritten(t, "agent") +} + +// Resuming a finished setup still checks the credential it will run on: a +// read-only one can't reply or acknowledge, so the resumed setup refuses it +// as a first setup does, rather than saying all is well (Codex on #794). +func TestGuidedConnectSetupResumedRefusesAReadOnlyCredential(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + app := newConnectSetupApp(t, s, "agent") + creds, err := app.Auth.GetStore().Load(app.Auth.CredentialKey()) + require.NoError(t, err) + creds.Scope = "read" + require.NoError(t, app.Auth.GetStore().Save(app.Auth.CredentialKey(), creds)) + guided(t, &scriptedPrompter{}) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "not full access") + assert.NotContains(t, out, "is set up on this computer") +} + +// A worker with no preflight of its own (the acp driver) is still checked, +// as doctor checks it: its pinned adapter must be installed, or setup says so +// instead of calling the agent set up (Codex on #794). +func TestGuidedConnectSetupChecksAnACPWorker(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + path := connectSetupPath(t, "agent") + raw, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(raw, &doc)) + doc["driver"] = setup.DriverACP + raw, err = json.Marshal(doc) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, raw, 0o600)) + t.Setenv("XDG_DATA_HOME", t.TempDir()) // no adapters installed + guided(t, &scriptedPrompter{}) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "adapter") + assert.NotContains(t, out, "is set up on this computer") +} + +// On a platform the connector doesn't run on, guided setup stops before it +// connects anything: connecting would replace the secret a Linux computer may +// be running this agent on, for a connector that can't run here (Codex on +// #794). +func TestGuidedConnectSetupRefusesAnUnsupportedPlatformBeforeConnecting(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + guided(t, &scriptedPrompter{}) + connectServiceGOOS = "darwin" + + out, err := runConnectSetupCmd(t, bareSetupApp(t, s, "agent")) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "Linux only, not darwin") + assert.NotContains(t, out, "connect this computer") + assertNotWritten(t, "agent") +} + +// A file another setup repaired while this one was asking is resumed, not +// moved aside (Codex on #794). +func TestGuidedConnectSetupResumesAFileRepairedWhileItAsked(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + path := connectSetupPath(t, "agent") + good, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte("{"), 0o600)) + guided(t, &scriptedPrompter{confirms: []bool{true}, onConfirm: func(string) { + require.NoError(t, os.WriteFile(path, good, 0o600)) // the other setup finishes first + }}) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + assert.Contains(t, out, "is set up on this computer") + aside, err := filepath.Glob(path + ".broken-*") + require.NoError(t, err) + assert.Empty(t, aside, "the repaired file was not moved aside") +} + +// With every project taken out of connect.json there is nowhere to try the +// agent: the summary says so and how to add one (Codex on #794). +func TestGuidedConnectSetupWithNoProjectsServedSaysHowToAddOne(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--unserve", strconv.FormatInt(setupProject, 10)) + require.NoError(t, err, out) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + guided(t, &scriptedPrompter{}) + + out, err = runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + assert.Contains(t, out, "Works in: no projects") + assert.Contains(t, out, "basecamp connect setup -P agent --serve ") + assert.NotContains(t, out, "in one of those projects") +} + +// Switching to the agent profile checks its base URL as root checks the one +// it started with: an insecure one is a setup error, not a panic in the SDK +// (Codex on #794). +func TestGuidedConnectSetupRefusesAnInsecureAgentProfileURL(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + guided(t, &scriptedPrompter{}) + _, err := registerProfile("agent", &config.ProfileConfig{BaseURL: "http://example.com", AccountID: "999", Scope: "full"}) + require.NoError(t, err) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "")) + require.Error(t, err, out) + assert.Contains(t, err.Error(), "base_url") +} + +// Setup never offers the background service, which would run the agent in +// the home directory, and never touches systemd: it says how to start the +// agent in the folder it should work in. +func TestGuidedConnectSetupNeverOffersTheBackgroundService(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + p := &scriptedPrompter{confirms: []bool{true}} + guided(t, p) + prev := runSystemctl + runSystemctl = func(args ...string) ([]byte, error) { + t.Errorf("setup ran systemctl %v", args) + return nil, nil + } + t.Cleanup(func() { runSystemctl = prev }) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + + assert.Equal(t, []string{"Work in Marie Chef's project, Connector?"}, p.asked) + assert.Contains(t, out, " basecamp connect -P agent\n") + for _, service := range []string{"whenever you log in", "in the background", "service install", "systemctl"} { + assert.NotContains(t, out, service) + } +} + +// An agent in no projects yet is the next step of a first setup, in +// Basecamp, and not a failure: setup says what to do there and ends cleanly. +func TestGuidedConnectSetupStopsWhenTheAgentIsInNoProjects(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + guided(t, &scriptedPrompter{}) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + assert.Contains(t, out, "Marie Chef isn't in any projects yet") + assert.Contains(t, out, "Next: add it to the projects it should work in (in Basecamp, Adminland → Manage agents → Marie Chef → Edit), then run `basecamp connect setup -P agent` again.") + assert.NotContains(t, out, "is set up on this computer") + assertNotWritten(t, "agent") +} + +// An AI that cannot start is caught here, before connect.json is written and +// before anyone mentions the agent: setup stops, says why in plain words, and +// says how to fix it. +func TestGuidedConnectSetupStopsWhenTheWorkerCannotStart(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + p := &scriptedPrompter{} + guided(t, p) + workerAnswers(t, brokenLauncher) + + app := bareSetupApp(t, s, "work") + app.Flags.Profile = "work" + out, err := runConnectSetupCmd(t, app) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, brokenLauncher.Checks[0].Message, apiErr.Message) + // The profile it was working on, which a bare `connect setup` may not + // choose again (Codex on #794). + assert.Equal(t, "Reinstall Claude Code, or fix the launcher at /home/me/.local/bin/claude. Then run this again: basecamp connect setup -P work", apiErr.Hint) + assert.Empty(t, p.asked, "nothing is asked of a person whose AI cannot start") + assert.NotContains(t, out, "is set up on this computer") + assertNotWritten(t, "work") +} + +// A finished setup whose AI has since stopped starting is not called set up. +func TestGuidedConnectSetupOnAFinishedSetupChecksTheWorker(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + guided(t, &scriptedPrompter{}) + loggedOut := driver.Preflight{Product: "Claude Code", Version: "2.1.283", Checks: []driver.PreflightCheck{ + readyWorker.Checks[0], readyWorker.Checks[1], + {Name: driver.PreflightLogin, Status: driver.PreflightFail, Message: "Claude Code is logged out on this computer — run `claude` and log in"}, + }} + workerAnswers(t, loggedOut) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Equal(t, "Claude Code is logged out on this computer — run `claude` and log in", apiErr.Message) + assert.Equal(t, "Then run this again: basecamp connect setup -P agent", apiErr.Hint) + assert.NotContains(t, out, "is set up on this computer") +} + +// A computer whose connection Basecamp no longer takes is offered a fresh +// one, and carries on from there. +func TestGuidedConnectSetupOffersToReconnectARefusedCredential(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + app := connectSetupApp(t, s, "agent") + expireAgentToken(t, app) + s.refuseSecret = true + p := &scriptedPrompter{confirms: []bool{true, true, false}, onConfirm: func(q string) { + if q == "Connect this computer to it again?" { + s.refuseSecret = false // the person approves a new connection + } + }} + guided(t, p) + + out, err := runConnectSetupCmd(t, app) + require.NoError(t, err, out) + + assert.Contains(t, out, "was disconnected in Basecamp, or it was connected on another computer") + assert.Equal(t, "Connect this computer to it again?", p.asked[0]) + assert.Equal(t, 2, s.intakeCount(), "a second connection ran") + _, err = setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) +} + +func TestGuidedConnectSetupLeavesARefusedCredentialThePersonKeeps(t *testing.T) { + s := startConnectSetupServer(t) + app := connectSetupApp(t, s, "agent") + expireAgentToken(t, app) + s.refuseSecret = true + guided(t, &scriptedPrompter{confirms: []bool{false}}) + + out, err := runConnectSetupCmd(t, app) + require.Error(t, err, out) + assert.Equal(t, 1, s.intakeCount()) +} + +// A file another command replaced while this one was asking is someone +// else's setup now, whatever agent it names: it isn't moved aside (Codex on +// #794). +func TestGuidedConnectSetupLeavesAFileReplacedWhileItAsked(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + path := connectSetupPath(t, "agent") + good, err := os.ReadFile(path) + require.NoError(t, err) + var other map[string]any + require.NoError(t, json.Unmarshal(good, &other)) + other["agent"].(map[string]any)["person_id"] = 424242 // reconnected to another agent meanwhile + replaced, err := json.Marshal(other) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte("{"), 0o600)) + guided(t, &scriptedPrompter{confirms: []bool{true}, onConfirm: func(string) { + require.NoError(t, os.WriteFile(path, replaced, 0o600)) + }}) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Contains(t, apiErr.Message, "changed by another command while setup was asking") + assert.Equal(t, "Then run this again: basecamp connect setup -P agent", apiErr.Hint) + now, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, replaced, now, "the other command's file is left in place") + aside, err := filepath.Glob(path + ".broken-*") + require.NoError(t, err) + assert.Empty(t, aside) +} + +// A connect.json that cannot be read is set aside and set up again, when +// the person says so. +func TestGuidedConnectSetupRebuildsABrokenConnectJSON(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + path := connectSetupPath(t, "agent") + require.NoError(t, os.WriteFile(path, []byte(`{"version": 1, "watch_completion": true`), 0o600)) + p := &scriptedPrompter{confirms: []bool{true, true, false}} + guided(t, p) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + + assert.Contains(t, out, "can't be used") + assert.Equal(t, "Work in Marie Chef's project, Connector?", p.asked[1]) + aside, err := filepath.Glob(path + ".broken-*") + require.NoError(t, err) + assert.Len(t, aside, 1, "the broken file is kept beside the new one") + f, err := setup.Load(path) + require.NoError(t, err) + assert.Contains(t, f.Projects, setupProject) +} + +// On a finished setup the guided setup changes nothing and says where +// things are. +func TestGuidedConnectSetupOnAFinishedSetupSaysSo(t *testing.T) { + s := startConnectSetupServer(t) + firstSetup(t, s) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + path := connectSetupPath(t, "agent") + before, err := os.ReadFile(path) + require.NoError(t, err) + p := &scriptedPrompter{} + guided(t, p) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent")) + require.NoError(t, err, out) + + assert.Empty(t, p.asked) + assert.Contains(t, out, "Marie Chef is set up on this computer") + assert.Contains(t, out, "Works in: Connector") + after, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, string(before), string(after)) +} + +// A policy flag makes setup the scriptable command, even at a terminal. +func TestConnectSetupWithAPolicyFlagIsNeverGuided(t *testing.T) { + s := startConnectSetupServer(t) + p := &scriptedPrompter{} + guided(t, p) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent"), "--operator", fmt.Sprint(setupOperatorPerson), serveArg()) + require.NoError(t, err, out) + assert.Empty(t, p.asked) + assert.NotContains(t, out, "is set up on this computer") +} + +// expireAgentToken leaves the profile's agent credential holding an expired +// token, so the next command has to mint a new one from the secret. +func expireAgentToken(t *testing.T, app *appctx.App) { + t.Helper() + store := app.Auth.GetStore() + creds, err := store.Load(app.Auth.CredentialKey()) + require.NoError(t, err) + creds.ExpiresAt = 1 + require.NoError(t, store.Save(app.Auth.CredentialKey(), creds)) +} + +// Guided setup asks about the agent it read. If the profile is connected to a +// different agent while a question is open, setup saves nothing for the new +// one: the person answered for the first (Codex on #794). +func TestGuidedConnectSetupRefusesAnAgentSwappedMidQuestion(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + p := &scriptedPrompter{confirms: []bool{true}, onConfirm: func(string) { + s.mu.Lock() + s.agentID++ // another agent's credential now answers for the profile + s.mu.Unlock() + }} + guided(t, p) + + out, err := runConnectSetupCmd(t, connectSetupApp(t, s, "agent")) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Contains(t, apiErr.Message, "connected to a different agent while setup was asking") + assertNotWritten(t, "agent") +} + +// A setup made for the same agent while guided setup was asking isn't merged +// into: guided setup told the person only its owner could give it work, and +// the other setup's trust may say otherwise (Codex on #794). +func TestGuidedConnectSetupRefusesASetupWrittenMidQuestion(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + app := connectSetupApp(t, s, "agent") + p := &scriptedPrompter{confirms: []bool{true}, onConfirm: func(string) { + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--trust", "project", serveArg()) + require.NoError(t, err, out) + }} + guided(t, p) + + out, err := runConnectSetupCmd(t, app) + require.Error(t, err, out) + var apiErr *output.Error + require.ErrorAs(t, err, &apiErr) + assert.Contains(t, apiErr.Message, "set up by another command while setup was asking") + loaded, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.Equal(t, admission.TrustProject, loaded.Trust.Mode, "the other setup's file is left as it wrote it") +} + +// A lock's metadata names a pid; the connector holding it started before it +// took the lock. A process the kernel gave the pid to after a crash started +// later, and isn't the connector (Codex on #794). +func TestHolderStillRunsOnlyForTheProcessThatTookTheLock(t *testing.T) { + now := time.Now().UTC().Format(time.RFC3339) + assert.True(t, holderStillRuns(connector.InstanceHolderInfo{PID: os.Getpid(), StartedAt: now}), + "this process started before now") + longAgo := time.Now().Add(-24 * time.Hour).UTC().Format(time.RFC3339) + assert.False(t, holderStillRuns(connector.InstanceHolderInfo{PID: os.Getpid(), StartedAt: longAgo}), + "a lock taken before this process existed was taken by another") + assert.False(t, holderStillRuns(connector.InstanceHolderInfo{PID: os.Getpid(), StartedAt: "not a time"})) +} diff --git a/internal/commands/connect_setup_test.go b/internal/commands/connect_setup_test.go index ac92509e2..c6c2abf59 100644 --- a/internal/commands/connect_setup_test.go +++ b/internal/commands/connect_setup_test.go @@ -34,6 +34,7 @@ const ( setupBotPerson int64 = 51177542 setupBotIdentity int64 = 4242 setupProject int64 = 48699913 + setupProject2 int64 = 48699914 setupClientPerson int64 = 1003 // Tokens the mock server tells apart. None is shaped like a real one. @@ -68,6 +69,14 @@ type connectSetupServer struct { duringMint func() // mintFailure, when set, answers the stream ticket mint. mintFailure func(w http.ResponseWriter) + // boss is who the agent works for, in its own profile; nil for a plain + // agent, or a Basecamp that does not say. + boss map[string]any + // agentProjects is what the agent's project list answers. + agentProjects []map[string]any + // refuseSecret answers every mint with invalid_client, as Basecamp does + // for an agent disconnected, or connected on another computer. + refuseSecret bool } func startConnectSetupServer(t *testing.T) *connectSetupServer { @@ -99,6 +108,12 @@ func startConnectSetupServer(t *testing.T) *connectSetupServer { writeJSON(w, map[string]any{"client_id": "agent-client", "client_secret": fakeConnectSecret, "account_id": "999", "scope": scope}) }) mux.HandleFunc("/oauth/tokens", func(w http.ResponseWriter, _ *http.Request) { + if s.refuseSecret { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"invalid_client"}`)) + return + } scope := s.grantScope if scope == "" { scope = "full" @@ -122,7 +137,11 @@ func startConnectSetupServer(t *testing.T) *connectSetupServer { if s.agentAsUser { personable = "User" } - writeJSON(w, map[string]any{"id": s.agentID, "name": "Marie Chef", "personable_type": personable}) + profile := map[string]any{"id": s.agentID, "name": "Marie Chef", "personable_type": personable} + if s.boss != nil { + profile["boss"] = s.boss + } + writeJSON(w, profile) case setupOperatorToken: writeJSON(w, map[string]any{"id": setupOperatorPerson, "name": "Operator", "personable_type": "User"}) case setupBotToken: @@ -172,12 +191,29 @@ func startConnectSetupServer(t *testing.T) *connectSetupServer { } writeJSON(w, body) } + mux.HandleFunc("/999/projects.json", func(w http.ResponseWriter, r *http.Request) { + if bearer(r) != setupAgentToken { + w.WriteHeader(http.StatusForbidden) + return + } + projects := s.agentProjects + if projects == nil { + projects = []map[string]any{} + } + writeJSON(w, projects) + }) mux.HandleFunc(fmt.Sprintf("/999/projects/%d", setupProject), func(w http.ResponseWriter, r *http.Request) { projectRead(w, r, map[string]any{"id": setupProject, "name": "Connector"}) }) mux.HandleFunc(fmt.Sprintf("/999/projects/%d/people.json", setupProject), func(w http.ResponseWriter, r *http.Request) { projectRead(w, r, []any{}) }) + mux.HandleFunc(fmt.Sprintf("/999/projects/%d", setupProject2), func(w http.ResponseWriter, r *http.Request) { + projectRead(w, r, map[string]any{"id": setupProject2, "name": "Launch"}) + }) + mux.HandleFunc(fmt.Sprintf("/999/projects/%d/people.json", setupProject2), func(w http.ResponseWriter, r *http.Request) { + projectRead(w, r, []any{}) + }) mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { s.mu.Lock() s.paths = append(s.paths, r.Method+" "+r.URL.Path) diff --git a/internal/commands/connect_worker_preflight.go b/internal/commands/connect_worker_preflight.go new file mode 100644 index 000000000..33ec02750 --- /dev/null +++ b/internal/commands/connect_worker_preflight.go @@ -0,0 +1,63 @@ +package commands + +import ( + "context" + + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/driver" + "github.com/basecamp/basecamp-cli/internal/connector/driver/spawn" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/richtext" +) + +// connectWorkerPreflight starts the worker connect.json names the way the +// connector would — its driver's binary, found on this PATH, with a +// session's environment — and asks it, with no work and no model call, +// whether it starts, knows the connector's flags, and is logged in. It +// reports false for a worker whose driver has no preflight (the acp driver +// has its own, acpPreflightCheck). A variable so tests can answer for the +// worker. +var connectWorkerPreflight = func(ctx context.Context, file setup.File) (driver.Preflight, bool) { + if file.Driver == setup.DriverACP { + return driver.Preflight{}, false + } + d, err := spawn.New(file.WorkerName(), spawn.Options{}) + if err != nil { + return driver.Preflight{}, false + } + p, ok := d.(driver.Preflighter) + if !ok { + return driver.Preflight{}, false + } + return p.Preflight(ctx, connector.DefaultPolicy()), true +} + +// preflightNames are doctor's names for a preflight's checks. +var preflightNames = map[string]string{ + driver.PreflightStarts: "starts", + driver.PreflightFlags: "options", + driver.PreflightLogin: "login", +} + +// preflightChecks are a preflight as doctor's rows: "Claude Code starts", +// "Claude Code options", "Claude Code login". +func preflightChecks(p driver.Preflight) []setup.Check { + checks := make([]setup.Check, 0, len(p.Checks)) + for _, c := range p.Checks { + status := setup.StatusPass + switch c.Status { + case driver.PreflightFail: + status = setup.StatusFail + case driver.PreflightWarn: + status = setup.StatusWarn + case driver.PreflightPass: + } + checks = append(checks, setup.Check{ + Name: p.Product + " " + preflightNames[c.Name], + Status: status, + Message: richtext.SanitizeSingleLine(c.Message), + Hint: richtext.SanitizeSingleLine(c.Hint), + }) + } + return checks +} diff --git a/internal/commands/connect_worker_preflight_test.go b/internal/commands/connect_worker_preflight_test.go new file mode 100644 index 000000000..9c3a2ec23 --- /dev/null +++ b/internal/commands/connect_worker_preflight_test.go @@ -0,0 +1,78 @@ +//go:build unix + +package commands + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/driver" + "github.com/basecamp/basecamp-cli/internal/connector/setup" +) + +// Doctor starts the worker connect.json names as the connector would: the +// claude on this PATH, with a session's environment. A launcher whose Claude +// Code is gone is a failed row that says so, not a pass for finding a file +// called claude. +func TestDoctorStartsTheWorkerTheConnectorWouldStart(t *testing.T) { + bin := t.TempDir() + launcher := filepath.Join(bin, "claude") + require.NoError(t, os.WriteFile(launcher, []byte("#!/bin/sh\nexec \"$HOME/.local/share/mise/installs/claude/latest/claude\" \"$@\"\n"), 0o700)) + t.Setenv("PATH", bin+":/usr/bin:/bin") + t.Setenv("HOME", t.TempDir()) + + checks := workerChecks(context.Background(), setup.New("agent")) + require.Len(t, checks, 1, "nothing more is asked of a worker that did not start") + assert.Equal(t, "Claude Code starts", checks[0].Name) + assert.Equal(t, setup.StatusFail, checks[0].Status) + assert.Contains(t, checks[0].Message, "The claude on your PATH is a launcher that couldn't find Claude Code (") + // The shell's own words vary (bash: "No such file or directory", dash: + // "not found"); the missing path is what every shell names. + assert.Contains(t, checks[0].Message, "/.local/share/mise/installs/claude/latest/claude") + assert.Equal(t, "Reinstall Claude Code, or fix the launcher at "+launcher+".", checks[0].Hint) +} + +// Each preflight check is a row of its own, and a warning stays a warning. +func TestDoctorShowsEachWorkerCheck(t *testing.T) { + workerAnswers(t, driver.Preflight{Product: "Codex", Version: "0.157.1", Checks: []driver.PreflightCheck{ + {Name: driver.PreflightStarts, Status: driver.PreflightPass, Message: "Codex 0.157.1 starts (/usr/local/bin/codex)"}, + {Name: driver.PreflightFlags, Status: driver.PreflightFail, Message: "Codex 0.157.1 is too old for the connector — update it", Hint: "It doesn't know --ignore-rules. Update Codex."}, + {Name: driver.PreflightLogin, Status: driver.PreflightWarn, Message: "Codex may be logged out on this computer — run `codex login`"}, + }}) + file := setup.New("agent") + file.Worker = setup.WorkerCodex + + checks := workerChecks(context.Background(), file) + require.Len(t, checks, 3) + assert.Equal(t, []string{"Codex starts", "Codex options", "Codex login"}, []string{checks[0].Name, checks[1].Name, checks[2].Name}) + assert.Equal(t, []string{setup.StatusPass, setup.StatusFail, setup.StatusWarn}, []string{checks[0].Status, checks[1].Status, checks[2].Status}) + assert.Equal(t, "It doesn't know --ignore-rules. Update Codex.", checks[1].Hint) +} + +// A connector that stopped taking work says so first, with why and the fix. +func TestConnectStatusLeadsWithNotTakingWork(t *testing.T) { + why := "Claude Code couldn't start twice in a row — Claude Code is logged out on this computer — run `claude` and log in" + report := connectStatusReport{Profile: "agent", Status: connector.Status{Connection: &connector.ConnectionStatus{ + State: connector.ConnectionNotTakingWork, PID: 42, ChangedAt: time.Now(), Detail: why, + }}} + var out bytes.Buffer + renderConnectStatus(&out, report) + lines := strings.Split(out.String(), "\n") + require.Greater(t, len(lines), 2) + assert.Equal(t, " Not taking work: "+why+". Fix it, then run `basecamp connect setup` or restart the connector.", lines[2]) + assert.True(t, strings.HasPrefix(connectStatusSummary(report), "not taking work")) + + report.Status.Connection.State, report.Status.Connection.Detail = connector.ConnectionRunning, "" + out.Reset() + renderConnectStatus(&out, report) + assert.NotContains(t, out.String(), "Not taking work") +} diff --git a/internal/connector/dispatcher.go b/internal/connector/dispatcher.go index 5b6f23274..b8bd0a8b1 100644 --- a/internal/connector/dispatcher.go +++ b/internal/connector/dispatcher.go @@ -161,6 +161,14 @@ type DispatcherOptions struct { Launcher driver.Launcher // NoAutomaticRetry: never retry a failed spawn (sandbox mode). NoAutomaticRetry bool + // Preflight checks the worker without any work (driver.Preflighter). + // While new work is held after the worker could not start + // (StartFailuresToHold), it says why, and a check that passes takes work + // again. Nil: only a start that works, or a restart, does. + Preflight func(ctx context.Context) driver.Preflight + // HoldCheck is how long held work waits before the worker is first + // checked again; HoldCheckInterval when zero. + HoldCheck time.Duration // MCP names what the worker's Basecamp MCP server runs as. MCP WorkerMCP @@ -231,6 +239,18 @@ type Dispatcher struct { mu sync.Mutex live map[string]*taskRun wg sync.WaitGroup + // noteMu orders what the start-failure hold says about the connection + // (dispatcher_starts.go): each writer asks the hold again under it, so a + // reason that arrives after the hold was cleared is not recorded. + noteMu sync.Mutex + // holds numbers each hold on new work as it's made. It is never reset, + // so a worker check or a reason that outlives its hold (a start that + // worked cleared it, and a newer one was made) can tell it isn't theirs. + holds uint64 + // launches numbers each launch in the order it was made, so a start + // that worked can tell whether failures came from launches made after + // it (dispatcher_starts.go). + launches uint64 // stopping is closed when Run is shutting down, which is what bounds the // adopted-reply rule's reads: their own context is the settlement's, // which a shutdown deliberately does not cancel. @@ -257,6 +277,9 @@ type Dispatcher struct { // path is too long for one; empty until the first attempt needs it. socketBase string socketBaseMu sync.Mutex + // starts is whether the worker has been starting (dispatcher_starts.go). + // Under mu. + starts startRecord } // NewDispatcher builds a dispatcher. @@ -311,6 +334,9 @@ func NewDispatcher(opts DispatcherOptions) (*Dispatcher, error) { if opts.ProgressInterval <= 0 { opts.ProgressInterval = DefaultProgressInterval } + if opts.HoldCheck <= 0 { + opts.HoldCheck = HoldCheckInterval + } // Every log line passes through the redaction rule; a task's own lines // through its task's (taskRedaction). opts.Redaction = opts.Redaction.With(driver.Redaction{Dirs: []string{opts.PrivateDir, opts.MCP.StateDir}}) @@ -323,6 +349,7 @@ func NewDispatcher(opts DispatcherOptions) (*Dispatcher, error) { live: map[string]*taskRun{}, stopping: make(chan struct{}), + starts: startRecord{checkEvery: opts.HoldCheck}, terminateRecorded: driver.TerminateRecorded, confirmGroupGone: driver.ConfirmGroupGone, @@ -347,6 +374,7 @@ func (d *Dispatcher) Run(ctx context.Context) error { if err := d.Recover(ctx); err != nil { return err } + d.checkWorkerAtStart(ctx) ticker := time.NewTicker(d.opts.Tick) defer ticker.Stop() for { @@ -512,7 +540,7 @@ func (d *Dispatcher) dispatchReady(ctx context.Context) error { return nil default: } - if d.free() <= 0 { + if d.free() <= 0 || d.holdingNewWork(ctx) { return nil } // Invariant 2, in the query: only records in a project this pass read as @@ -529,8 +557,11 @@ func (d *Dispatcher) dispatchReady(ctx context.Context) error { for _, record := range records { // Asked again on every record, not counted down: a start that failed // can have held its attempt, and a held attempt takes a slot as a - // running one does (Copilot). - if d.free() <= 0 { + // running one does (Copilot). The start-failure hold is asked again + // too: a start that failed at once gives its slot back, and the one + // that made the second failure in a row holds new work from the next + // record on, not the next pass. + if d.free() <= 0 || d.startsHeld() { break } switch err := d.start(ctx, record); { @@ -694,6 +725,10 @@ func (d *Dispatcher) start(ctx context.Context, record Record) error { if err != nil { return err } + d.mu.Lock() + d.launches++ + seq := d.launches + d.mu.Unlock() d.line(DispatchLine{Type: "dispatch", TaskID: launch.TaskID, AttemptID: launch.AttemptID, EventIDs: launch.EventIDs, State: string(AttemptLaunching)}) // Settling must outlive a shutdown that interrupts the start. @@ -706,7 +741,12 @@ func (d *Dispatcher) start(ctx context.Context, record Record) error { if err != nil { // Nothing was asked of the driver: no process exists. log.Warn("connector: could not prepare a session", "task_id", launch.TaskID, "error", err) - d.release(settleCtx, launch, driver.Process{}, TokenHolder{}, AttemptEnd{AttemptID: launch.AttemptID, Stop: StopFailed, SpawnFailed: true, NoAutomaticRetry: d.opts.NoAutomaticRetry}, nil) + settlement, settled := d.release(settleCtx, launch, driver.Process{}, TokenHolder{}, AttemptEnd{AttemptID: launch.AttemptID, Stop: StopFailed, SpawnFailed: true, NoAutomaticRetry: d.opts.NoAutomaticRetry}, nil) + // A start that ran nothing, as a driver's refusal is: it counts + // toward holding new work. + if settled { + d.noteStart(settleCtx, settlement, err.Error(), seq) + } return nil //nolint:nilerr // settled as a start that ran nothing } session, err := d.opts.Driver.NewSession(ctx, cfg) @@ -725,8 +765,11 @@ func (d *Dispatcher) start(ctx context.Context, record Record) error { cleanup() // A start that launched a process says so (driver.StartError); the // release point confirms that group gone before anything is settled. - d.release(settleCtx, launch, driver.StartedProcess(err), taker, AttemptEnd{AttemptID: launch.AttemptID, Stop: StopFailed, SpawnFailed: spawnFailed, + settlement, settled := d.release(settleCtx, launch, driver.StartedProcess(err), taker, AttemptEnd{AttemptID: launch.AttemptID, Stop: StopFailed, SpawnFailed: spawnFailed, NoAutomaticRetry: d.opts.NoAutomaticRetry || unusable}, nil) + if settled { + d.noteStart(settleCtx, settlement, strings.TrimPrefix(err.Error(), driver.ErrNotStarted.Error()+": "), seq) + } return nil } p := session.Process() @@ -751,7 +794,7 @@ func (d *Dispatcher) start(ctx context.Context, record Record) error { } d.line(DispatchLine{Type: "dispatch", TaskID: launch.TaskID, AttemptID: launch.AttemptID, State: string(AttemptRunning)}) - run := &taskRun{d: d, launch: launch, record: record, session: session, cleanup: cleanup, log: log, refusals: refusals, tokens: tokens} + run := &taskRun{d: d, launch: launch, record: record, session: session, cleanup: cleanup, log: log, refusals: refusals, tokens: tokens, seq: seq} d.mu.Lock() d.live[launch.AttemptID] = run d.mu.Unlock() @@ -1082,8 +1125,9 @@ const settleAttempts = 5 // It settles nothing until the worker's process group is confirmed gone, and // nothing if the ledger refuses the settlement. Either way the attempt stays // live: its token and its conversation are still its own, a person settles -// it, and this process goes on counting it among the workers it has. -func (d *Dispatcher) release(ctx context.Context, launch Launch, worker driver.Process, holder TokenHolder, end AttemptEnd, run *taskRun) { +// it, and this process goes on counting it among the workers it has. It +// returns the settlement, and false when there was none. +func (d *Dispatcher) release(ctx context.Context, launch Launch, worker driver.Process, holder TokenHolder, end AttemptEnd, run *taskRun) (Settlement, bool) { log := d.taskLog(d.taskRedaction(launch, driver.SessionConfig{})) err := d.confirmGroupGone(worker, d.opts.CancelGrace) if err == nil { @@ -1100,7 +1144,7 @@ func (d *Dispatcher) release(ctx context.Context, launch Launch, worker driver.P log.Error("connector: the worker's process group is still alive; its attempt stays live, holding its conversation and a worker slot", "attempt_id", end.AttemptID, "task_id", launch.TaskID, "error", err) d.line(DispatchLine{Type: "dispatch", TaskID: launch.TaskID, AttemptID: end.AttemptID, State: string(AttemptRunning), StopReason: "held"}) - return + return Settlement{}, false } settlement, err := d.settle(ctx, end) if err != nil { @@ -1111,7 +1155,7 @@ func (d *Dispatcher) release(ctx context.Context, launch Launch, worker driver.P log.Error("connector: could not settle an attempt; it stays live, holding its conversation and a worker slot", "attempt_id", end.AttemptID, "task_id", launch.TaskID, "error", err) d.line(DispatchLine{Type: "dispatch", TaskID: launch.TaskID, AttemptID: end.AttemptID, State: string(AttemptRunning), StopReason: "held"}) - return + return Settlement{}, false } reportUnreported(log, end.Stop, settlement) // Adoption is a read of Basecamp, bounded but slow, and no dispatch @@ -1122,8 +1166,13 @@ func (d *Dispatcher) release(ctx context.Context, launch Launch, worker driver.P d.wg.Go(func() { d.adopt(ctx, settlement) }) d.line(DispatchLine{Type: "dispatch", TaskID: launch.TaskID, AttemptID: launch.AttemptID, State: string(AttemptEnded), StopReason: string(end.Stop)}) if run != nil { + // Whether the worker started is noted while its slot is still taken: + // freed first, a pass could launch into the gap before a second + // failure's hold was set (Codex on #794). + d.noteStart(ctx, settlement, run.said, run.seq) d.forget(launch.AttemptID) } + return settlement, true } // settle ends an attempt in the ledger, retrying a failure with backoff: an @@ -1229,6 +1278,10 @@ type taskRun struct { // refusals records the session's refusals as they happen. refusals *refusalRecorder + // seq is the launch's place in the order launches were made. + seq uint64 + // said is what the worker said last, for a hold's reason. + said string } // supervise prompts the worker, delivers follow-ups, and settles the attempt @@ -1273,19 +1326,21 @@ func (r *taskRun) supervise(ctx context.Context) { // through the recorder; what the ledger would not take is settled now. unrecorded := r.refusals.unrecorded() - if stop != StopFinished { - if tail, ok := r.session.(interface{ StderrTail() string }); ok { - // The driver's StderrTail is already its redactor's Stderr: the - // last line, sanitized, never the text verbatim. - if text := strings.TrimSpace(tail.StderrTail()); text != "" { - r.log.Warn("connector: the worker's last output", "attempt_id", r.launch.AttemptID, - "stop_reason", string(stop), "stderr", richtext.SanitizeSingleLine(text)) - } - } + var said string + if tail, ok := r.session.(interface{ StderrTail() string }); ok { + // The driver's StderrTail is already its redactor's Stderr: the + // last line, sanitized, never the text verbatim. + said = richtext.SanitizeSingleLine(strings.TrimSpace(tail.StderrTail())) + } + if stop != StopFinished && said != "" { + r.log.Warn("connector: the worker's last output", "attempt_id", r.launch.AttemptID, + "stop_reason", string(stop), "stderr", said) } // Through the one release point: it confirms the worker's group is gone - // before the attempt is settled. + // before the attempt is settled, and notes whether the worker started + // before its slot is given back. + r.said = said d.release(settleCtx, r.launch, r.session.Process(), taker, AttemptEnd{AttemptID: r.launch.AttemptID, Stop: stop, UnrecordedRefusals: unrecorded}, r) } diff --git a/internal/connector/dispatcher_starts.go b/internal/connector/dispatcher_starts.go new file mode 100644 index 000000000..ba58b978f --- /dev/null +++ b/internal/connector/dispatcher_starts.go @@ -0,0 +1,294 @@ +package connector + +import ( + "context" + "strings" + "time" + + "github.com/basecamp/basecamp-cli/internal/richtext" +) + +// A worker that cannot start fails every request the same way: a launcher +// whose target is gone, a logged-out agent, a version too old for the +// connector's flags. Each request it is handed is answered "I couldn't start +// on this", and the next one meets the same computer. So after +// StartFailuresToHold attempts in a row whose worker never picked its request +// up, the dispatcher starts nothing new: records wait, admitted, for a worker +// that can take them. It says so once, as an ERROR line, and in `connect +// status` through the connection row. +// +// The hold is this process's, not the ledger's. It decides only whether a +// pass starts anything, which the dispatcher already declines to do when +// connect.json cannot be read; it changes no record, takes no worker slot, +// and every guarantee the ledger holds for a start holds for the start that +// ends it. A restart ends it, and so does the worker: the dispatcher checks it +// with its preflight (DispatcherOptions.Preflight), without any work, and a +// check that passes takes work again. When the check cannot see what stopped +// the worker, the next request finds out; one more failure holds work again, +// and each time that happens the dispatcher waits twice as long before its +// next check, up to HoldCheckLimit. + +// StartFailuresToHold is how many attempts in a row whose worker never picked +// its request up hold new work. One could be anything; two in a row is the +// computer. +const StartFailuresToHold = 2 + +// HoldCheckInterval is how long held work waits before the worker is first +// checked again; HoldCheckLimit is the longest it waits. +const ( + HoldCheckInterval = time.Minute + HoldCheckLimit = time.Hour +) + +// ConnectionNotTakingWork is a running connector holding new work because its +// worker could not start. The connection row's detail says why. +const ConnectionNotTakingWork = "not_taking_work" + +// NotTakingWorkFix is what a person does about it. +const NotTakingWorkFix = "Fix it, then run `basecamp connect setup` or restart the connector." + +// startRecord is whether the worker has been starting. +type startRecord struct { + // failed are the launch seqs of attempts in a row whose worker never + // picked its request up. + failed []uint64 + // workedThrough is the latest launch whose worker picked its request up. + // A failure launched before it settled late: the computer has started a + // worker since, so it counts for nothing. + workedThrough uint64 + // held is whether new work is held. + held bool + // checkAt is when a held dispatcher next checks the worker, and checking + // is whether a check is running. + checkAt time.Time + checking bool + // checkEvery is how long a hold waits before its first check. + checkEvery time.Duration +} + +// noteStart reads a settled attempt for whether its worker started: one that +// picked a request up did, whatever happened after; one refused a start, or +// gone before it picked its request up, did not. An attempt interrupted by +// the connector says nothing either way. said is what the worker or its start +// said last, for the person. +// seq is the launch's place in the order launches were made. +func (d *Dispatcher) noteStart(ctx context.Context, s Settlement, said string, seq uint64) { + switch { + case pickedUp(s): + d.startWorked(ctx, seq) + case s.SpawnFailed || workerNeverStarted(s): + d.startFailed(ctx, said, seq) + } +} + +func pickedUp(s Settlement) bool { + for _, e := range s.Events { + if e.Pulled { + return true + } + } + return false +} + +func workerNeverStarted(s Settlement) bool { + for _, e := range s.Events { + if neverStarted(e, s) { + return true + } + } + return false +} + +func (d *Dispatcher) startWorked(ctx context.Context, seq uint64) { + d.mu.Lock() + d.starts.workedThrough = max(d.starts.workedThrough, seq) + // Failures launched after this one say the computer changed since it + // started: they stand, and so does any hold they made. Only failures + // launched before it are answered by it. + var newer []uint64 + for _, f := range d.starts.failed { + if f > seq { + newer = append(newer, f) + } + } + if len(newer) > 0 { + d.starts.failed = newer + d.mu.Unlock() + return + } + wasHeld := d.starts.held + d.starts = startRecord{workedThrough: d.starts.workedThrough, checkEvery: d.opts.HoldCheck} + d.mu.Unlock() + if wasHeld { + d.takeWorkAgain(ctx, "the worker started") + } +} + +func (d *Dispatcher) startFailed(ctx context.Context, said string, seq uint64) { + d.mu.Lock() + if seq <= d.starts.workedThrough { + d.mu.Unlock() + return + } + d.starts.failed = append(d.starts.failed, seq) + hold := len(d.starts.failed) >= StartFailuresToHold && !d.starts.held + if hold { + // Checking until the hold is recorded: a check that passed in the + // meantime would take work again before the hold was said. + d.starts.held, d.starts.checking = true, true + d.holds++ + } + gen := d.holds + d.mu.Unlock() + if !hold { + return + } + + // The preflight runs outside the lock, so a start that worked may clear + // the hold meanwhile; noteNotTakingWork records the reason only if it + // still stands. + d.noteNotTakingWork(ctx, d.whyNotStarting(ctx, said), gen) + d.mu.Lock() + if d.holds == gen { + d.starts.checking = false + d.starts.checkAt = time.Now().Add(d.starts.checkEvery) + } + d.mu.Unlock() +} + +// checkWorkerAtStart runs the worker's preflight before the first pass. A +// worker that cannot start fails every request it is handed, so one found +// not ready holds new work from the start, as two failed starts in a row +// would, and is checked again on the hold's schedule. No request is spent +// finding out. +func (d *Dispatcher) checkWorkerAtStart(ctx context.Context) { + if d.opts.Preflight == nil { + return + } + p := d.opts.Preflight(ctx) + c, failed := p.Failed() + if !failed || ctx.Err() != nil { + return + } + product := p.Product + if product == "" { + product = d.opts.Driver.Name() + } + why := product + " isn't ready — " + strings.TrimRight(richtext.SanitizeSingleLine(c.Message), ".") + d.mu.Lock() + // As if StartFailuresToHold starts had failed before any launch: the + // first launch that works answers them. + d.starts.failed = make([]uint64, StartFailuresToHold) + d.starts.held = true + d.starts.checkAt = time.Now().Add(d.starts.checkEvery) + d.holds++ + gen := d.holds + d.mu.Unlock() + d.noteNotTakingWork(ctx, why, gen) +} + +// noteNotTakingWork says why new work is held, if hold gen still stands. +func (d *Dispatcher) noteNotTakingWork(ctx context.Context, why string, gen uint64) { + d.noteMu.Lock() + defer d.noteMu.Unlock() + if !d.holdStands(gen) { + return + } + d.log.Error("connector: not taking work: " + why + ". " + NotTakingWorkFix) + if err := d.ledger.NoteConnection(ctx, ConnectionNotTakingWork, why); err != nil { + d.log.Warn("connector: could not record that no work is being taken, for status", "error", err) + } +} + +// whyNotStarting is the hold's reason in a person's words: what the worker's +// preflight finds wrong, when it finds something, else what the worker said. +func (d *Dispatcher) whyNotStarting(ctx context.Context, said string) string { + product := d.opts.Driver.Name() + reason := strings.TrimSpace(said) + if d.opts.Preflight != nil { + p := d.opts.Preflight(ctx) + if p.Product != "" { + product = p.Product + } + if c, failed := p.Failed(); failed { + reason = c.Message + } + } + if reason == "" { + reason = "it stopped before it picked up its request" + } + reason = strings.TrimRight(richtext.SanitizeSingleLine(reason), ".") + return product + " couldn't start twice in a row — " + reason +} + +// holdStands reports whether hold gen is the one holding new work now. +func (d *Dispatcher) holdStands(gen uint64) bool { + d.mu.Lock() + defer d.mu.Unlock() + return d.starts.held && d.holds == gen +} + +// startsHeld reports whether new work is held, and nothing more: a pass asks +// holdingNewWork once, which may start a check; each launch in the pass asks +// this. +func (d *Dispatcher) startsHeld() bool { + d.mu.Lock() + defer d.mu.Unlock() + return d.starts.held +} + +// holdingNewWork reports whether new work is held, and starts a check of the +// worker when one is due. +func (d *Dispatcher) holdingNewWork(ctx context.Context) bool { + d.mu.Lock() + defer d.mu.Unlock() + if !d.starts.held { + return false + } + if d.opts.Preflight != nil && !d.starts.checking && !time.Now().Before(d.starts.checkAt) { + d.starts.checking = true + gen := d.holds + d.wg.Go(func() { d.checkHeldWorker(ctx, gen) }) + } + return true +} + +// checkHeldWorker runs the worker's preflight for a hold. A pass takes work +// again one failure short of holding it again, and doubles the wait before +// the next hold's first check: the check could not see what stopped the +// worker, or the worker has been fixed, and the next start says which. +func (d *Dispatcher) checkHeldWorker(ctx context.Context, gen uint64) { + p := d.opts.Preflight(ctx) + _, failed := p.Failed() + d.mu.Lock() + // A check for a hold since cleared, or replaced by a newer one, decides + // nothing: the newer hold's own checks do. + if !d.starts.held || d.holds != gen { + d.mu.Unlock() + return + } + d.starts.checking = false + if failed || ctx.Err() != nil { + d.starts.checkAt = time.Now().Add(d.starts.checkEvery) + d.mu.Unlock() + return + } + d.starts.held = false + // One more failure holds work again. + d.starts.failed = d.starts.failed[len(d.starts.failed)-(StartFailuresToHold-1):] + d.starts.checkEvery = min(d.starts.checkEvery*2, HoldCheckLimit) + d.mu.Unlock() + d.takeWorkAgain(ctx, "the worker started when it was checked") +} + +func (d *Dispatcher) takeWorkAgain(ctx context.Context, why string) { + d.noteMu.Lock() + defer d.noteMu.Unlock() + if d.startsHeld() { + return // held again since: its own reason stands + } + d.log.Info("connector: taking work again: " + why) + if err := d.ledger.NoteConnection(ctx, ConnectionRunning, ""); err != nil { + d.log.Warn("connector: could not record that work is being taken again, for status", "error", err) + } +} diff --git a/internal/connector/dispatcher_starts_test.go b/internal/connector/dispatcher_starts_test.go new file mode 100644 index 000000000..edd77b5e9 --- /dev/null +++ b/internal/connector/dispatcher_starts_test.go @@ -0,0 +1,430 @@ +package connector + +import ( + "context" + "fmt" + "log/slog" + "os" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/driver" +) + +// pickUp is a worker's first get_dispatch of its task's originating event. +func pickUp(t *testing.T, l *Ledger, taskID int64) { + t.Helper() + _, err := l.db.ExecContext(context.Background(), + `UPDATE task_events SET pulled_at = ? WHERE task_id = ? AND pulled_at IS NULL`, l.timestamp(), taskID) + require.NoError(t, err) +} + +// failingBeforePickUp is a worker that ends its session before it asks for +// its request, n times, and then works. +func failingBeforePickUp(t *testing.T, h **dispatchHarness, n int32) func(s *fakeSession, _ int, _ string) (driver.PromptResult, error) { + var sessions atomic.Int32 + return func(s *fakeSession, _ int, _ string) (driver.PromptResult, error) { + if sessions.Add(1) <= n { + return driver.PromptResult{}, fmt.Errorf("%w: %w: the agent closed its output before it confirmed the session", driver.ErrSessionUnverified, driver.ErrSessionEnded) + } + pickUp(t, (*h).ledger, s.cfg.Scope.TaskID) + return driver.PromptResult{Stop: driver.TurnEndTurn}, nil + } +} + +func connectionOf(t *testing.T, l *Ledger) ConnectionStatus { + t.Helper() + s, err := l.Status(context.Background()) + require.NoError(t, err) + if s.Connection == nil { + return ConnectionStatus{} + } + return *s.Connection +} + +func startsOf(fake *fakeDriver, calls *atomic.Int32) { + fake.onStart = func(driver.SessionConfig) { calls.Add(1) } +} + +// The hold is asked before every launch, not once a batch: starts that fail +// at once give their slots back, so a batch that only checked capacity would +// go on launching after the second failure (Codex on #794). +func TestAHoldStopsTheRestOfTheBatch(t *testing.T) { + fake := newFakeDriver() + notFound := fmt.Errorf("%w: exec: \"claude\": executable file not found in $PATH", driver.ErrNotStarted) + fake.startErr = []error{notFound, notFound, notFound, notFound, notFound} + var calls atomic.Int32 + startsOf(fake, &calls) + h := newDispatchHarness(t, fake, func(o *DispatcherOptions) { o.Concurrency = 2 }) + for id := int64(1); id <= 5; id++ { + admitOn(t, h.ledger, id, fmt.Sprintf("recording:%d", id)) + } + stop := h.run(t) + + require.Eventually(t, func() bool { return connectionOf(t, h.ledger).State == ConnectionNotTakingWork }, 10*time.Second, 10*time.Millisecond) + time.Sleep(150 * time.Millisecond) + stop() + assert.Equal(t, int32(StartFailuresToHold), calls.Load(), "nothing is started after the second failure, in the same batch or the next") +} + +// The worker is checked before anything is handed to it: a connector that +// starts with Claude Code logged out holds new work from the first pass, says +// why, and takes work once a check passes. No request is spent finding out +// (Codex on #794). +func TestAWorkerThatIsNotReadyAtStartHoldsWorkUntilItIs(t *testing.T) { + fake := newFakeDriver() + var h *dispatchHarness + fake.turn = func(s *fakeSession, _ int, _ string) (driver.PromptResult, error) { + pickUp(t, h.ledger, s.cfg.Scope.TaskID) + return driver.PromptResult{Stop: driver.TurnEndTurn}, nil + } + var calls atomic.Int32 + startsOf(fake, &calls) + var checks atomic.Int32 + var logs safeBuffer + h = newDispatchHarness(t, fake, func(o *DispatcherOptions) { + o.Concurrency = 1 + o.Logger = slog.New(slog.NewTextHandler(&logs, nil)) + o.HoldCheck = 300 * time.Millisecond + o.Preflight = func(context.Context) driver.Preflight { + p := driver.Preflight{Product: "Claude Code"} + if checks.Add(1) == 1 { + p.Checks = []driver.PreflightCheck{{Name: driver.PreflightLogin, Status: driver.PreflightFail, + Message: "Claude Code is logged out on this computer — run `claude` and log in."}} + } + return p + } + }) + admitOn(t, h.ledger, 1, "recording:1") + stop := h.run(t) + defer stop() + + require.Eventually(t, func() bool { return connectionOf(t, h.ledger).State == ConnectionNotTakingWork }, 5*time.Second, 5*time.Millisecond) + assert.Equal(t, "Claude Code isn't ready — Claude Code is logged out on this computer — run `claude` and log in", connectionOf(t, h.ledger).Detail) + assert.Equal(t, int32(0), calls.Load(), "nothing is handed to a worker that isn't ready") + assert.Contains(t, logs.String(), "level=ERROR msg=\"connector: not taking work: Claude Code isn't ready") + + require.Eventually(t, func() bool { return calls.Load() == 1 }, 10*time.Second, 10*time.Millisecond, "a check that passes takes work") + require.Eventually(t, func() bool { return connectionOf(t, h.ledger).State == ConnectionRunning }, 5*time.Second, 10*time.Millisecond) +} + +// A hold's reason is recorded only while the hold stands. Its preflight runs +// outside the lock, so a start that worked can clear the hold and record the +// connector running first; the late reason must not then say it isn't taking +// work (Codex on #794). +func TestAHoldClearedWhileItsPreflightRunsLeavesStatusRunning(t *testing.T) { + release := make(chan struct{}) + entered := make(chan struct{}, 1) + h := newDispatchHarness(t, newFakeDriver(), func(o *DispatcherOptions) { + o.Preflight = func(context.Context) driver.Preflight { + entered <- struct{}{} + <-release + return driver.Preflight{Product: "Claude Code", Checks: []driver.PreflightCheck{{Name: driver.PreflightLogin, + Status: driver.PreflightFail, Message: "Claude Code is logged out on this computer"}}} + } + }) + ctx := context.Background() + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) + done := make(chan struct{}) + go func() { + defer close(done) + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // the second in a row: holds, then asks the preflight why + }() + <-entered + h.d.startWorked(ctx, h.d.nextSeqForTest()) // a healthy task settles meanwhile + require.Equal(t, ConnectionRunning, connectionOf(t, h.ledger).State) + close(release) + <-done + + assert.Equal(t, ConnectionRunning, connectionOf(t, h.ledger).State, "the hold was cleared before its reason arrived") + assert.False(t, h.d.startsHeld()) +} + +// A session that can't even be prepared — its directory can't be made — is a +// start that never ran, like a driver's refusal: two in a row hold new work +// rather than failing every queued request (Codex on #794). +func TestSessionsThatCannotBePreparedHoldNewWork(t *testing.T) { + fake := newFakeDriver() + var calls atomic.Int32 + startsOf(fake, &calls) + h := newDispatchHarness(t, fake, func(o *DispatcherOptions) { o.Concurrency = 1 }) + for id := int64(1); id <= 4; id++ { + admitOn(t, h.ledger, id, fmt.Sprintf("recording:%d", id)) + } + require.NoError(t, os.RemoveAll(h.d.opts.PrivateDir)) // every session's directory now fails + stop := h.run(t) + + require.Eventually(t, func() bool { return connectionOf(t, h.ledger).State == ConnectionNotTakingWork }, 10*time.Second, 10*time.Millisecond) + time.Sleep(150 * time.Millisecond) + stop() + assert.Zero(t, calls.Load(), "no session reached the driver") + for id := int64(1); id <= 4; id++ { + assert.Equal(t, StateAdmitted, stateOf(t, h.ledger, id), "record %d waits, its automatic retry unspent, for a start that can work", id) + } +} + +// A worker check belongs to the hold it was started for. One still running +// when that hold was cleared, and a newer hold made, must not take work again +// for the newer hold when it passes (Codex on #794). +func TestAnOldWorkerCheckCannotClearANewerHold(t *testing.T) { + var calls atomic.Int32 + entered := make(chan struct{}, 1) + release := make(chan struct{}) + h := newDispatchHarness(t, newFakeDriver(), func(o *DispatcherOptions) { + o.Preflight = func(context.Context) driver.Preflight { + if calls.Add(1) == 2 { // the first hold's check: slow, and it passes + entered <- struct{}{} + <-release + return driver.Preflight{Product: "Claude Code"} + } + return driver.Preflight{Product: "Claude Code", Checks: []driver.PreflightCheck{{Name: driver.PreflightLogin, + Status: driver.PreflightFail, Message: "Claude Code is logged out on this computer"}}} + } + }) + ctx := context.Background() + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // hold 1, and its reason (preflight call 1) + h.d.mu.Lock() + first := h.d.holds + h.d.mu.Unlock() + done := make(chan struct{}) + go func() { + defer close(done) + h.d.checkHeldWorker(ctx, first) // preflight call 2, held open + }() + <-entered + h.d.startWorked(ctx, h.d.nextSeqForTest()) // a healthy task clears hold 1 + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // hold 2, and its reason (preflight call 3) + require.True(t, h.d.startsHeld()) + close(release) // hold 1's check passes, late + <-done + + assert.True(t, h.d.startsHeld(), "the newer hold stands") + assert.Equal(t, ConnectionNotTakingWork, connectionOf(t, h.ledger).State) +} + +// nextSeqForTest is the next launch's number, as start takes it. +func (d *Dispatcher) nextSeqForTest() uint64 { + d.mu.Lock() + defer d.mu.Unlock() + d.launches++ + return d.launches +} + +// A task launched before the failures, that finishes after them, started on a +// computer that has changed since: it doesn't clear the hold they made +// (Codex on #794). +func TestAnOlderLaunchThatWorkedDoesNotClearANewerHold(t *testing.T) { + h := newDispatchHarness(t, newFakeDriver(), func(o *DispatcherOptions) { + o.Preflight = func(context.Context) driver.Preflight { + return driver.Preflight{Product: "Claude Code", Checks: []driver.PreflightCheck{{Name: driver.PreflightStarts, + Status: driver.PreflightFail, Message: "claude isn't on PATH"}}} + } + }) + ctx := context.Background() + older := h.d.nextSeqForTest() // task A launches, and picks its request up + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // B and C, launched later, fail: held + require.True(t, h.d.startsHeld()) + + h.d.startWorked(ctx, older) // A finishes + assert.True(t, h.d.startsHeld(), "A's start says nothing about B and C") + assert.Equal(t, ConnectionNotTakingWork, connectionOf(t, h.ledger).State) + + h.d.startWorked(ctx, h.d.nextSeqForTest()) // a launch after them that works + assert.False(t, h.d.startsHeld()) +} + +// A launch that worked, made between two that failed, answers only the +// failure before it: the one after it stands, and so does the hold (Codex on +// #794). +func TestALaunchThatWorkedDoesNotClearAHoldMadeByALaterFailure(t *testing.T) { + h := newDispatchHarness(t, newFakeDriver(), func(o *DispatcherOptions) { + o.Preflight = func(context.Context) driver.Preflight { + return driver.Preflight{Product: "Claude Code", Checks: []driver.PreflightCheck{{Name: driver.PreflightStarts, + Status: driver.PreflightFail, Message: "claude isn't on PATH"}}} + } + }) + ctx := context.Background() + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // A fails + between := h.d.nextSeqForTest() // B starts, and runs a while + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // the worker breaks: C fails, held + require.True(t, h.d.startsHeld()) + + h.d.startWorked(ctx, between) // B finishes + assert.True(t, h.d.startsHeld(), "B started before C failed") + assert.Equal(t, ConnectionNotTakingWork, connectionOf(t, h.ledger).State) +} + +// A failure that settles after a later launch worked says nothing about the +// computer now: the worker has started since. It neither holds work nor +// counts towards a hold (Codex on #794). +func TestAFailureThatSettlesAfterALaterLaunchWorkedIsNotCounted(t *testing.T) { + h := newDispatchHarness(t, newFakeDriver(), func(o *DispatcherOptions) { + o.Preflight = func(context.Context) driver.Preflight { + return driver.Preflight{Product: "Claude Code", Checks: []driver.PreflightCheck{{Name: driver.PreflightStarts, + Status: driver.PreflightFail, Message: "claude isn't on PATH"}}} + } + }) + ctx := context.Background() + a, b := h.d.nextSeqForTest(), h.d.nextSeqForTest() // A and B launch together + h.d.startWorked(ctx, h.d.nextSeqForTest()) // C, launched after them, works + h.d.startFailed(ctx, "", a) // then A and B settle as failures + h.d.startFailed(ctx, "", b) + assert.False(t, h.d.startsHeld(), "C started after A and B launched") + + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) // one new failure is one, not three + assert.False(t, h.d.startsHeld()) + h.d.startFailed(ctx, "", h.d.nextSeqForTest()) + assert.True(t, h.d.startsHeld()) +} + +// Two starts in a row that never ran hold new work: the next record waits, +// admitted, and the connector says so once, with the fix. A restart takes +// work again. +func TestTwoFailedStartsInARowHoldNewWork(t *testing.T) { + fake := newFakeDriver() + notFound := fmt.Errorf("%w: exec: \"claude\": executable file not found in $PATH", driver.ErrNotStarted) + fake.startErr = []error{notFound, notFound} + var calls atomic.Int32 + startsOf(fake, &calls) + var logs safeBuffer + h := newDispatchHarness(t, fake, func(o *DispatcherOptions) { + o.Concurrency = 1 + o.Logger = slog.New(slog.NewTextHandler(&logs, nil)) + }) + admitOn(t, h.ledger, 1, "recording:1") + admitOn(t, h.ledger, 2, "recording:2") + stop := h.run(t) + + require.Eventually(t, func() bool { return connectionOf(t, h.ledger).State == ConnectionNotTakingWork }, 10*time.Second, 10*time.Millisecond) + time.Sleep(150 * time.Millisecond) + stop() + assert.Equal(t, int32(2), calls.Load(), "nothing is started after the second failure") + admitted := 0 + for _, id := range []int64{1, 2} { + if stateOf(t, h.ledger, id) == StateAdmitted { + admitted++ + } + } + assert.Positive(t, admitted, "the work waits for a worker that can take it") + why := "fake couldn't start twice in a row — exec: \"claude\": executable file not found in $PATH" + assert.Equal(t, why, connectionOf(t, h.ledger).Detail) + assert.Equal(t, 1, strings.Count(logs.String(), "level=ERROR msg=\"connector: not taking work: "), logs.String()) + assert.Contains(t, logs.String(), "Fix it, then run `basecamp connect setup` or restart the connector.") + + restarted := newFakeDriver() + opts := h.d.opts + opts.Driver = restarted + d, err := NewDispatcher(opts) + require.NoError(t, err) + h.d = d + h.run(t) + <-restarted.made +} + +// The second failure's hold is set while its worker still takes its slot, so +// no pass can launch into the gap between the slot freeing and the hold +// (Codex on #794). The hold's own check runs inside that window, and sees it. +func TestAFailedWorkerKeepsItsSlotUntilItsFailureIsCounted(t *testing.T) { + fake := newFakeDriver() + var h *dispatchHarness + fake.turn = failingBeforePickUp(t, &h, 2) + var slotTaken atomic.Bool + var holdChecked atomic.Bool + h = newDispatchHarness(t, fake, func(o *DispatcherOptions) { + o.Concurrency = 1 + o.Preflight = func(context.Context) driver.Preflight { + if h != nil && h.d.startsHeld() && !holdChecked.Swap(true) { + h.d.mu.Lock() + slotTaken.Store(len(h.d.live) > 0) + h.d.mu.Unlock() + } + return driver.Preflight{Product: "Claude Code"} + } + }) + for id := int64(1); id <= 3; id++ { + admitOn(t, h.ledger, id, fmt.Sprintf("recording:%d", id)) + } + stop := h.run(t) + require.Eventually(t, holdChecked.Load, 10*time.Second, 10*time.Millisecond) + stop() + assert.True(t, slotTaken.Load(), "the failed worker's slot was given back before its failure held work") +} + +// A worker whose session ends before it asks for its request did not start +// either, though a process existed: two in a row hold new work, and the +// preflight's reason is the one given. A check that passes takes work again. +func TestAWorkerThatNeverPicksItsRequestUpHoldsNewWorkUntilItStarts(t *testing.T) { + fake := newFakeDriver() + var h *dispatchHarness + fake.turn = failingBeforePickUp(t, &h, 2) + var calls atomic.Int32 + startsOf(fake, &calls) + var checks atomic.Int32 + var logs safeBuffer + h = newDispatchHarness(t, fake, func(o *DispatcherOptions) { + o.Concurrency = 1 + o.Logger = slog.New(slog.NewTextHandler(&logs, nil)) + o.HoldCheck = 50 * time.Millisecond + o.Preflight = func(context.Context) driver.Preflight { + p := driver.Preflight{Product: "Claude Code"} + // The check at start passes: what ends these sessions is not + // something it can see. The hold's own check, and the first check + // after it, find Claude Code logged out; then someone logs in. + if n := checks.Add(1); n == 2 || n == 3 { + p.Checks = []driver.PreflightCheck{{Name: driver.PreflightLogin, Status: driver.PreflightFail, + Message: "Claude Code is logged out on this computer — run `claude` and log in"}} + } + return p + } + }) + for id := int64(1); id <= 3; id++ { + admitOn(t, h.ledger, id, fmt.Sprintf("recording:%d", id)) + } + h.run(t) + + require.Eventually(t, func() bool { return connectionOf(t, h.ledger).State == ConnectionNotTakingWork }, 10*time.Second, 10*time.Millisecond) + assert.Equal(t, "Claude Code couldn't start twice in a row — Claude Code is logged out on this computer — run `claude` and log in", + connectionOf(t, h.ledger).Detail) + assert.Equal(t, StateAdmitted, stateOf(t, h.ledger, 3), "the third request waits") + for _, id := range []int64{1, 2} { + assert.Equal(t, StateCompleted, stateOf(t, h.ledger, id)) + assert.Equal(t, string(OutcomeUnknown), outcomeOf(t, h.ledger, id), "a process existed: never run again automatically") + } + + rows := h.attemptsEnded(t, 3) + assert.Equal(t, "finished", rows[2].StopReason) + assert.Equal(t, StateCompleted, stateOf(t, h.ledger, 3)) + assert.Equal(t, ConnectionRunning, connectionOf(t, h.ledger).State) + assert.Contains(t, logs.String(), "connector: taking work again: the worker started when it was checked") + assert.GreaterOrEqual(t, checks.Load(), int32(3)) +} + +// A start that works in between clears the count: failures are held only +// when they come in a row. +func TestAStartThatWorksClearsTheCount(t *testing.T) { + fake := newFakeDriver() + var h *dispatchHarness + var sessions atomic.Int32 + fake.turn = func(s *fakeSession, _ int, _ string) (driver.PromptResult, error) { + if n := sessions.Add(1); n == 2 || n == 4 { + pickUp(t, h.ledger, s.cfg.Scope.TaskID) + return driver.PromptResult{Stop: driver.TurnEndTurn}, nil + } + return driver.PromptResult{}, fmt.Errorf("%w: %w: gone", driver.ErrSessionUnverified, driver.ErrSessionEnded) + } + h = newDispatchHarness(t, fake, func(o *DispatcherOptions) { o.Concurrency = 1 }) + for id := int64(1); id <= 4; id++ { + admitOn(t, h.ledger, id, fmt.Sprintf("recording:%d", id)) + } + h.run(t) + h.attemptsEnded(t, 4) + assert.NotEqual(t, ConnectionNotTakingWork, connectionOf(t, h.ledger).State) +} diff --git a/internal/connector/driver/claude/preflight.go b/internal/connector/driver/claude/preflight.go new file mode 100644 index 000000000..5eb6f900e --- /dev/null +++ b/internal/connector/driver/claude/preflight.go @@ -0,0 +1,76 @@ +package claude + +import ( + "context" + "encoding/json" + "slices" + + "github.com/basecamp/basecamp-cli/internal/connector/driver" +) + +// Product is Claude Code's name for a person. +const Product = "Claude Code" + +var _ driver.Preflighter = (*Driver)(nil) + +// Preflight implements driver.Preflighter: `claude --version`, the session's +// flags in `claude --help`, and `claude auth status`, which reads the stored +// login without calling a model. A token that is present but expired still +// reads as logged in; only a session finds that out. +func (d *Driver) Preflight(ctx context.Context, policy driver.PermissionPolicy) driver.Preflight { + return d.probe(policy).Check(ctx) +} + +// loginArgs asks Claude Code whether it's logged in the way a session runs +// it: with the host's settings off (--setting-sources ""), so a user setting +// that blanks an API key can't make a worker that would run look logged out. +// The option goes before the subcommand; `auth status` takes no such option. +var loginArgs = []string{"--setting-sources", "", "auth", "status", "--json"} + +func (d *Driver) probe(policy driver.PermissionPolicy) driver.WorkerProbe { + return driver.WorkerProbe{ + Product: Product, + Binary: d.opts.Binary, + Env: d.env(driver.SessionConfig{Env: driver.BuildEnv(driver.BaseEnv, d.opts.Lookup, nil)}), + Help: []string{"--help"}, + Flags: SessionFlags(policy, d.opts.Model), + Update: "Update Claude Code: claude update", + Login: loginArgs, + LoggedIn: loggedIn, + LoginFix: "run `claude` and log in", + } +} + +// SessionFlags is every flag a session passes, new or resumed: what the +// preflight asks Claude Code's help for. +func SessionFlags(policy driver.PermissionPolicy, model string) []string { + if policy == nil { + return nil + } + cfg := driver.SessionConfig{Policy: policy} + var flags []string + for _, resume := range []bool{false, true} { + args, err := Args(cfg, "00000000-0000-4000-8000-000000000000", resume, "mcp.json", model) + if err != nil { + continue + } + for _, f := range driver.FlagsOf(args) { + if !slices.Contains(flags, f) { + flags = append(flags, f) + } + } + } + return flags +} + +// loggedIn reads `claude auth status --json`, which exits non-zero when +// logged out and says so either way. +func loggedIn(r driver.ProbeResult) (bool, bool) { + var status struct { + LoggedIn *bool `json:"loggedIn"` + } + if err := json.Unmarshal([]byte(r.Stdout), &status); err != nil || status.LoggedIn == nil { + return false, false + } + return *status.LoggedIn, true +} diff --git a/internal/connector/driver/claude/preflight_test.go b/internal/connector/driver/claude/preflight_test.go new file mode 100644 index 000000000..57dc52a78 --- /dev/null +++ b/internal/connector/driver/claude/preflight_test.go @@ -0,0 +1,70 @@ +//go:build unix + +package claude + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/driver" +) + +// The preflight asks Claude Code's help for every flag a session passes, new +// or resumed, so a flag added to Args is asked about without anyone +// remembering to. +func TestSessionFlagsAreEveryFlagASessionPasses(t *testing.T) { + flags := SessionFlags(policy{}, "opus") + for _, resume := range []bool{false, true} { + args, err := Args(driver.SessionConfig{Policy: policy{}}, "00000000-0000-4000-8000-000000000000", resume, "mcp.json", "opus") + require.NoError(t, err) + for _, f := range driver.FlagsOf(args) { + assert.Contains(t, flags, f) + } + } + assert.Contains(t, flags, "--session-id") + assert.Contains(t, flags, "--resume") + assert.Contains(t, flags, "--model") + assert.NotContains(t, SessionFlags(policy{}, ""), "--model", "a model is asked about only when one is passed") +} + +// The preflight starts the claude a session starts, with a session's +// environment: CLAUDE_CONFIG_DIR, which decides which login Claude Code +// reads, reaches it, and the connector's other variables do not. Its login is +// asked with the host's settings off, as a session runs. +func TestPreflightRunsClaudeAsASessionWould(t *testing.T) { + dir := t.TempDir() + seen := filepath.Join(dir, "env") + exe := filepath.Join(dir, "claude") + script := `#!/bin/sh +env > ` + seen + ` +case "$1" in + --version) echo "2.1.283 (Claude Code)" ;; + --help) echo "-p --input-format --output-format --verbose --setting-sources --permission-mode --permission-prompts --tools --allowed-tools --strict-mcp-config --mcp-config --session-id --resume" ;; + --setting-sources) if [ "$2" = "" ] && [ "$3" = auth ] && [ "$CLAUDE_CONFIG_DIR" = /config ]; then echo '{"loggedIn":true}'; else echo '{"loggedIn":false}'; exit 1; fi ;; + auth) echo '{"loggedIn":false}'; exit 1 ;; # asked with the host's settings on +esac +` + require.NoError(t, os.WriteFile(exe, []byte(script), 0o700)) + host := map[string]string{"PATH": "/usr/bin:/bin", "HOME": dir, "CLAUDE_CONFIG_DIR": "/config", "BASECAMP_TOKEN": "not-a-real-token"} + d := New(Options{Binary: exe, Lookup: func(k string) (string, bool) { v, ok := host[k]; return v, ok }}) + + p := d.Preflight(context.Background(), policy{}) + _, failed := p.Failed() + assert.False(t, failed, "%+v", p.Checks) + assert.Equal(t, Product, p.Product) + assert.Equal(t, "2.1.283", p.Version) + env, err := os.ReadFile(seen) + require.NoError(t, err) + assert.Contains(t, string(env), "CLAUDE_CONFIG_DIR=/config") + assert.NotContains(t, string(env), "BASECAMP_TOKEN") + + delete(host, "CLAUDE_CONFIG_DIR") + c, failed := d.Preflight(context.Background(), policy{}).Failed() + require.True(t, failed) + assert.Equal(t, "Claude Code is logged out on this computer — run `claude` and log in", c.Message) +} diff --git a/internal/connector/driver/codex/preflight.go b/internal/connector/driver/codex/preflight.go new file mode 100644 index 000000000..57bc8dd02 --- /dev/null +++ b/internal/connector/driver/codex/preflight.go @@ -0,0 +1,48 @@ +package codex + +import ( + "context" + + "github.com/basecamp/basecamp-cli/internal/connector/driver" +) + +// Product is Codex's name for a person. +const Product = "Codex" + +var _ driver.Preflighter = (*Driver)(nil) + +// Preflight implements driver.Preflighter: `codex --version`, the session's +// flags in `codex exec --help`, and `codex login status`. The login is only a +// warning: Codex's answer is a guess about stored credentials, and an API key +// in the environment works without one. +func (d *Driver) Preflight(ctx context.Context, policy driver.PermissionPolicy) driver.Preflight { + return d.probe(policy).Check(ctx) +} + +func (d *Driver) probe(policy driver.PermissionPolicy) driver.WorkerProbe { + return driver.WorkerProbe{ + Product: Product, + Binary: d.opts.Binary, + Env: d.env(driver.SessionConfig{Env: driver.BuildEnv(driver.BaseEnv, d.opts.Lookup, nil)}), + Help: []string{"exec", "--help"}, + Flags: SessionFlags(policy, d.opts.Model), + Update: "Update Codex.", + Login: []string{"login", "status"}, + LoggedIn: func(r driver.ProbeResult) (bool, bool) { return r.Exit == 0, r.Exit >= 0 }, + LoginFix: "run `codex login`", + LoginWarns: true, + } +} + +// SessionFlags is every flag a new session passes after `exec`: what the +// preflight asks `codex exec --help` for. +func SessionFlags(policy driver.PermissionPolicy, model string) []string { + if policy == nil { + return nil + } + args, err := Args(driver.SessionConfig{Policy: policy}, "", model) + if err != nil { + return nil + } + return driver.FlagsOf(args) +} diff --git a/internal/connector/driver/codex/preflight_test.go b/internal/connector/driver/codex/preflight_test.go new file mode 100644 index 000000000..b0404c55c --- /dev/null +++ b/internal/connector/driver/codex/preflight_test.go @@ -0,0 +1,53 @@ +//go:build unix + +package codex + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/driver" +) + +func TestSessionFlagsAreEveryFlagASessionPasses(t *testing.T) { + args, err := Args(driver.SessionConfig{Policy: testPolicy{}}, "", "") + require.NoError(t, err) + assert.Equal(t, driver.FlagsOf(args), SessionFlags(testPolicy{}, "")) + assert.Contains(t, SessionFlags(testPolicy{}, ""), "--strict-config") +} + +// Codex's preflight starts it and asks `codex exec --help` for the session's +// flags. A login it cannot confirm is a warning, never a stop. +func TestPreflightChecksCodexWithoutBlockingOnItsLogin(t *testing.T) { + dir := t.TempDir() + exe := filepath.Join(dir, "codex") + help := strings.Join(SessionFlags(testPolicy{}, ""), " ") + script := `#!/bin/sh +case "$1 $2" in + "--version ") echo "codex-cli 0.157.1" ;; + "exec --help") echo "` + help + `" ;; + "login status") echo "Not logged in" >&2; exit 1 ;; +esac +` + require.NoError(t, os.WriteFile(exe, []byte(script), 0o700)) + d := New(Options{Binary: exe, Lookup: func(k string) (string, bool) { + if k == "PATH" { + return "/usr/bin:/bin", true + } + return "", false + }}) + p := d.Preflight(context.Background(), testPolicy{}) + _, failed := p.Failed() + assert.False(t, failed) + assert.Equal(t, "0.157.1", p.Version) + require.Len(t, p.Checks, 3) + assert.Equal(t, driver.PreflightPass, p.Checks[1].Status) + assert.Equal(t, driver.PreflightWarn, p.Checks[2].Status) + assert.Equal(t, "Codex may be logged out on this computer — run `codex login`", p.Checks[2].Message) +} diff --git a/internal/connector/driver/preflight.go b/internal/connector/driver/preflight.go new file mode 100644 index 000000000..285238ec4 --- /dev/null +++ b/internal/connector/driver/preflight.go @@ -0,0 +1,345 @@ +package driver + +import ( + "bytes" + "context" + "errors" + "fmt" + "io/fs" + "os" + "os/exec" + "regexp" + "slices" + "strings" + "time" +) + +// A preflight starts a worker's own program the way a session would — the +// same binary, found on the same PATH, with the same environment — and asks +// it three things without any work and without a model call: does it start, +// does it know every flag a session passes it, and is it logged in. What it +// finds is said in a person's words, with the fix, because the person reading +// it is the one who runs the agent, not the one who mentioned it. + +// Preflighter is a driver that can check its worker before any work is given +// to it. +type Preflighter interface { + Preflight(ctx context.Context, policy PermissionPolicy) Preflight +} + +// Preflight is what a worker's preflight found. +type Preflight struct { + // Product is the worker's name for a person: "Claude Code". + Product string + // Version is what the worker said its version is; empty when it did not + // start. + Version string + Checks []PreflightCheck +} + +// Failed is the first check that failed. +func (p Preflight) Failed() (PreflightCheck, bool) { + for _, c := range p.Checks { + if c.Status == PreflightFail { + return c, true + } + } + return PreflightCheck{}, false +} + +// PreflightCheck is one question a preflight asked. +type PreflightCheck struct { + Name string + Status PreflightStatus + Message string + // Hint is the fix, where one is known. + Hint string +} + +// PreflightStatus is how a check went. A warning is something the preflight +// could not settle; it never stops anything. +type PreflightStatus string + +const ( + PreflightPass PreflightStatus = "pass" + PreflightFail PreflightStatus = "fail" + PreflightWarn PreflightStatus = "warn" +) + +// The checks, by name. +const ( + PreflightStarts = "starts" + PreflightFlags = "flags" + PreflightLogin = "login" +) + +// PreflightTimeout bounds each of a preflight's runs. +const PreflightTimeout = 30 * time.Second + +// WorkerProbe is how one driver's preflight runs. +type WorkerProbe struct { + // Product is the worker's name for a person: "Claude Code". + Product string + // Binary is the program a session starts, found on PATH as a session + // finds it. + Binary string + // Env is the environment a session gets. + Env []string + // Help is the arguments that list the flags Flags must be among. + Help []string + // Flags are the flags a session passes. + Flags []string + // Update is how a person updates the worker. + Update string + // Login is the arguments that ask whether the worker is logged in, with + // no model call. + Login []string + // LoggedIn reads Login's answer: whether the worker said it is logged in, + // and whether it said anything that could be read. + LoggedIn func(ProbeResult) (loggedIn, known bool) + // LoginFix is what a person does when the worker is logged out. + LoginFix string + // LoginWarns is a login check that never fails: its answer is a guess. + LoginWarns bool + // Timeout bounds each run; PreflightTimeout when zero. + Timeout time.Duration + // Run runs one probe; RunProbe when nil. A test seam. + Run func(ctx context.Context, cmd Command, timeout time.Duration) ProbeResult +} + +// ProbeResult is one short run of a worker's program. +type ProbeResult struct { + Stdout, Stderr string + // Exit is the exit status; -1 when the program did not exit on its own. + Exit int + // NotFound is a program PATH does not have. + NotFound bool + // TimedOut is a program that did not answer in time. + TimedOut bool + // Err is why the program could not be run, when it could not. + Err error +} + +// probeOutputLimit bounds what a probe keeps of each stream. +const probeOutputLimit = 256 << 10 + +// RunProbe runs cmd with no input, bounded by timeout. The program is found as +// StartWorker finds it. +func RunProbe(ctx context.Context, cmd Command, timeout time.Duration) ProbeResult { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + ec := exec.CommandContext(ctx, cmd.Path, cmd.Args...) //nolint:gosec // G204: the driver's own binary and flags, never content + ec.Dir = cmd.Dir + ec.Env = cmd.Env + if ec.Env == nil { + ec.Env = []string{} + } + ec.WaitDelay = 2 * time.Second + group := probeInItsOwnGroup(ec) + defer group.cleanup() + var stdout, stderr limitedBuffer + stdout.max, stderr.max = probeOutputLimit, probeOutputLimit + ec.Stdout, ec.Stderr = &stdout, &stderr + err := ec.Start() + if err == nil { + group.started() + err = ec.Wait() + } + out := ProbeResult{Stdout: stdout.String(), Stderr: stderr.String(), Exit: -1} + if ec.ProcessState != nil { + out.Exit = ec.ProcessState.ExitCode() + } + var exitErr *exec.ExitError + switch { + case ctx.Err() != nil && errors.Is(ctx.Err(), context.DeadlineExceeded): + out.TimedOut = true + case errors.Is(err, exec.ErrNotFound), errors.Is(err, fs.ErrNotExist) && ec.ProcessState == nil && !isFile(ec.Path): + // Missing only when the program itself is: a program that is there + // but whose interpreter or loader is gone fails its exec with the + // same ENOENT, and is a program that can't start, not one PATH lacks. + out.NotFound = true + out.Err = err + case errors.As(err, &exitErr): + case err != nil: + out.Err = err + } + return out +} + +func isFile(path string) bool { + info, err := os.Stat(path) + return err == nil && !info.IsDir() +} + +type limitedBuffer struct { + bytes.Buffer + max int +} + +func (b *limitedBuffer) Write(p []byte) (int, error) { + if room := b.max - b.Len(); room > 0 { + b.Buffer.Write(p[:min(len(p), room)]) + } + return len(p), nil +} + +// Check runs the preflight: the start first, and the rest only when it +// started. +func (w WorkerProbe) Check(ctx context.Context) Preflight { + out := Preflight{Product: w.Product} + red := NewRedactor(Redaction{Env: w.Env}) + path, _ := exec.LookPath(w.Binary) + + started := w.run(ctx, "--version") + if c, ok := w.startFailure(started, path, red); ok { + out.Checks = append(out.Checks, c) + return out + } + out.Version = versionIn(started.Stdout) + named := w.Product + if out.Version != "" { + named += " " + out.Version + } + out.Checks = append(out.Checks, PreflightCheck{Name: PreflightStarts, Status: PreflightPass, Message: named + " starts (" + path + ")"}) + + out.Checks = append(out.Checks, w.flagsCheck(ctx, named, red)) + if len(w.Login) > 0 { + out.Checks = append(out.Checks, w.loginCheck(ctx, red)) + } + return out +} + +func (w WorkerProbe) run(ctx context.Context, args ...string) ProbeResult { + run := w.Run + if run == nil { + run = RunProbe + } + timeout := w.Timeout + if timeout <= 0 { + timeout = PreflightTimeout + } + return run(ctx, Command{Path: w.Binary, Args: args, Env: w.Env}, timeout) +} + +func (w WorkerProbe) command(args ...string) string { + return "`" + strings.Join(append([]string{w.Binary}, args...), " ") + "`" +} + +// launcherMissing is what a shell or a wrapper says when the program it +// hands over to is not there. +var launcherMissing = regexp.MustCompile(`(?i)no such file or directory|not found|cannot execute`) + +// startFailure is the start check when the worker did not start. +func (w WorkerProbe) startFailure(r ProbeResult, path string, red *Redactor) (PreflightCheck, bool) { + c := PreflightCheck{Name: PreflightStarts, Status: PreflightFail} + said := red.Stderr(r.Stderr) + version := w.command("--version") + switch { + case r.NotFound: + c.Message = fmt.Sprintf("%s isn't installed here: %s is not on the PATH the connector starts with", w.Product, w.Binary) + c.Hint = fmt.Sprintf("Install %s, or put %s on the PATH the connector starts with.", w.Product, w.Binary) + case r.TimedOut: + c.Message = fmt.Sprintf("%s didn't answer %s in time", w.Product, version) + c.Hint = fmt.Sprintf("Run %s yourself to see what it's waiting for.", version) + case r.Err != nil: + c.Message = fmt.Sprintf("%s couldn't be started: %s", w.Product, red.Sanitize(r.Err.Error())) + c.Hint = fmt.Sprintf("Run %s yourself to see why.", version) + case r.Exit == 0: + return PreflightCheck{}, false + case r.Exit == 126 || r.Exit == 127 || launcherMissing.MatchString(said): + c.Message = fmt.Sprintf("The %s on your PATH is a launcher that couldn't find %s (%s)", w.Binary, w.Product, orExit(said, r.Exit)) + c.Hint = fmt.Sprintf("Reinstall %s, or fix the launcher at %s.", w.Product, path) + default: + c.Message = fmt.Sprintf("%s couldn't start: %s exited with status %d (%s)", w.Product, version, r.Exit, orExit(said, r.Exit)) + c.Hint = fmt.Sprintf("Run %s yourself to see why.", version) + } + return c, true +} + +func orExit(said string, exit int) string { + if said == "" { + return fmt.Sprintf("it exited with status %d and said nothing", exit) + } + return said +} + +// flagsCheck asks the worker's help for every flag a session passes. A +// worker too old to know one would refuse the session it was started for. +func (w WorkerProbe) flagsCheck(ctx context.Context, named string, red *Redactor) PreflightCheck { + c := PreflightCheck{Name: PreflightFlags} + r := w.run(ctx, w.Help...) + if r.Exit != 0 || r.TimedOut || r.Err != nil { + c.Status = PreflightFail + c.Message = fmt.Sprintf("%s wouldn't list its options: %s failed (%s)", named, w.command(w.Help...), orExit(red.Stderr(r.Stderr), r.Exit)) + c.Hint = fmt.Sprintf("Run %s yourself to see why.", w.command(w.Help...)) + return c + } + missing := MissingFlags(r.Stdout+"\n"+r.Stderr, w.Flags) + if len(missing) > 0 { + c.Status = PreflightFail + c.Message = fmt.Sprintf("%s is too old for the connector — update it", named) + c.Hint = fmt.Sprintf("It doesn't know %s. %s", strings.Join(missing, ", "), w.Update) + return c + } + c.Status = PreflightPass + c.Message = fmt.Sprintf("knows all %d options the connector passes", len(w.Flags)) + return c +} + +func (w WorkerProbe) loginCheck(ctx context.Context, red *Redactor) PreflightCheck { + c := PreflightCheck{Name: PreflightLogin} + r := w.run(ctx, w.Login...) + loggedIn, known := false, false + if !r.TimedOut && r.Err == nil { + loggedIn, known = w.LoggedIn(r) + } + switch { + case known && loggedIn: + c.Status = PreflightPass + c.Message = "logged in" + case known: + c.Status = PreflightFail + c.Message = fmt.Sprintf("%s is logged out on this computer — %s", w.Product, w.LoginFix) + if w.LoginWarns { + c.Status = PreflightWarn + c.Message = fmt.Sprintf("%s may be logged out on this computer — %s", w.Product, w.LoginFix) + } + default: + c.Status = PreflightWarn + c.Message = fmt.Sprintf("couldn't tell whether %s is logged in: %s answered %s", w.Product, w.command(w.Login...), orExit(red.Stderr(r.Stderr), r.Exit)) + } + return c +} + +var versionPattern = regexp.MustCompile(`\d+\.\d+[0-9A-Za-z.+-]*`) + +// versionIn is the version a --version answer names. +func versionIn(stdout string) string { + first, _, _ := strings.Cut(strings.TrimSpace(stdout), "\n") + return versionPattern.FindString(first) +} + +// FlagsOf is every flag in a command line, once each, in order. A lone "-" +// (read the prompt from stdin) is an argument, not a flag. +func FlagsOf(args []string) []string { + var flags []string + for _, a := range args { + if len(a) > 1 && a != "--" && strings.HasPrefix(a, "-") && !slices.Contains(flags, a) { + flags = append(flags, a) + } + } + return flags +} + +// MissingFlags is the flags help does not list. A flag counts as listed only +// as a whole word: --tools is not found inside --allowed-tools. +func MissingFlags(help string, flags []string) []string { + var missing []string + for _, f := range flags { + listed := regexp.MustCompile(`(^|[\s,\[(|])` + regexp.QuoteMeta(f) + `($|[\s,=\])|<])`) + if !listed.MatchString(help) { + missing = append(missing, f) + } + } + return missing +} diff --git a/internal/connector/driver/preflight_test.go b/internal/connector/driver/preflight_test.go new file mode 100644 index 000000000..98f40d9b1 --- /dev/null +++ b/internal/connector/driver/preflight_test.go @@ -0,0 +1,216 @@ +//go:build unix + +package driver + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeWorker writes a program that answers a preflight as a worker would: +// its version, a help listing flags, and a login status. +func fakeWorker(t *testing.T, script string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "worker") + require.NoError(t, os.WriteFile(path, []byte("#!/bin/sh\n"+script), 0o700)) + return path +} + +const fakeWorkerAnswers = ` +case "$1" in + --version) echo "2.1.283 (Claude Code)" ;; + --help) echo "Usage: worker [options]"; echo " -p, --print"; echo " --allowed-tools "; echo " --tools " ;; + auth) if [ -n "$LOGGED_IN" ]; then echo '{"loggedIn":true,"email":"someone@example.com"}'; else echo '{"loggedIn":false}'; exit 1; fi ;; +esac +` + +func testProbe(binary string, env ...string) WorkerProbe { + return WorkerProbe{ + Product: "Claude Code", + Binary: binary, + Env: append([]string{"PATH=/usr/bin:/bin"}, env...), + Help: []string{"--help"}, + Flags: []string{"-p", "--tools", "--allowed-tools"}, + Update: "Update Claude Code: claude update", + Login: []string{"auth", "status", "--json"}, + LoggedIn: func(r ProbeResult) (bool, bool) { return testLoggedIn(r) }, + LoginFix: "run `claude` and log in", + } +} + +func testLoggedIn(r ProbeResult) (bool, bool) { + var s struct { + LoggedIn *bool `json:"loggedIn"` + } + if json.Unmarshal([]byte(r.Stdout), &s) != nil || s.LoggedIn == nil { + return false, false + } + return *s.LoggedIn, true +} + +func TestPreflightPassesAWorkerThatStartsKnowsItsFlagsAndIsLoggedIn(t *testing.T) { + p := testProbe(fakeWorker(t, fakeWorkerAnswers), "LOGGED_IN=1").Check(context.Background()) + _, failed := p.Failed() + assert.False(t, failed) + assert.Equal(t, "2.1.283", p.Version) + require.Len(t, p.Checks, 3) + for _, c := range p.Checks { + assert.Equal(t, PreflightPass, c.Status, c.Name) + assert.NotContains(t, c.Message, "someone@example.com", "nothing the login answer says about the person is repeated") + } +} + +func TestPreflightRunsTheWorkerWithTheSessionsEnvironmentOnly(t *testing.T) { + t.Setenv("LOGGED_IN", "1") + p := testProbe(fakeWorker(t, fakeWorkerAnswers)).Check(context.Background()) + c, failed := p.Failed() + require.True(t, failed, "the connector's own environment is not the worker's") + assert.Equal(t, PreflightLogin, c.Name) +} + +func TestPreflightSaysWhyAWorkerCannotStart(t *testing.T) { + cases := map[string]struct { + binary string + message string + hint string + }{ + "not installed": { + binary: "basecamp-connect-no-such-worker", + message: "Claude Code isn't installed here: basecamp-connect-no-such-worker is not on the PATH the connector starts with", + hint: "Install Claude Code, or put basecamp-connect-no-such-worker on the PATH the connector starts with.", + }, + "a launcher whose target is gone": { + binary: fakeWorker(t, `exec "$HOME/.local/share/mise/installs/claude/latest/claude" "$@"`), + message: "The %s on your PATH is a launcher that couldn't find Claude Code", + }, + "a worker that fails": { + binary: fakeWorker(t, `echo "config is broken" >&2; exit 3`), + message: "Claude Code couldn't start: `%s --version` exited with status 3 (config is broken)", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + p := testProbe(tc.binary, "HOME="+t.TempDir()).Check(context.Background()) + require.Len(t, p.Checks, 1, "nothing else is asked of a worker that did not start") + c, failed := p.Failed() + require.True(t, failed) + assert.Equal(t, PreflightStarts, c.Name) + assert.Contains(t, c.Message, sprintfIf(tc.message, tc.binary)) + if tc.hint != "" { + assert.Equal(t, tc.hint, c.Hint) + } + assert.Empty(t, p.Version) + }) + } +} + +func TestPreflightNamesTheVersionTooOldForTheConnector(t *testing.T) { + probe := testProbe(fakeWorker(t, fakeWorkerAnswers), "LOGGED_IN=1") + probe.Flags = append(probe.Flags, "--permission-prompts", "--strict-mcp-config") + c, failed := probe.Check(context.Background()).Failed() + require.True(t, failed) + assert.Equal(t, PreflightFlags, c.Name) + assert.Equal(t, "Claude Code 2.1.283 is too old for the connector — update it", c.Message) + assert.Equal(t, "It doesn't know --permission-prompts, --strict-mcp-config. Update Claude Code: claude update", c.Hint) +} + +func TestPreflightSaysAWorkerIsLoggedOut(t *testing.T) { + c, failed := testProbe(fakeWorker(t, fakeWorkerAnswers)).Check(context.Background()).Failed() + require.True(t, failed) + assert.Equal(t, PreflightLogin, c.Name) + assert.Equal(t, "Claude Code is logged out on this computer — run `claude` and log in", c.Message) + + probe := testProbe(fakeWorker(t, fakeWorkerAnswers)) + probe.LoginWarns = true + p := probe.Check(context.Background()) + _, failed = p.Failed() + assert.False(t, failed, "a login the worker can only guess at never stops anything") + assert.Equal(t, PreflightWarn, p.Checks[2].Status) +} + +func TestPreflightCannotTellALoginItCannotRead(t *testing.T) { + probe := testProbe(fakeWorker(t, `case "$1" in --version) echo 1.0.0 ;; --help) echo "-p --tools --allowed-tools" ;; *) echo "unknown command" >&2; exit 2 ;; esac`)) + p := probe.Check(context.Background()) + _, failed := p.Failed() + assert.False(t, failed) + assert.Equal(t, PreflightWarn, p.Checks[2].Status) + assert.Contains(t, p.Checks[2].Message, "couldn't tell whether Claude Code is logged in") +} + +func TestMissingFlagsMatchesWholeFlagsOnly(t *testing.T) { + help := " -p, --print\n --allowed-tools \n --mcp-config \n -c, --config " + assert.Equal(t, []string{"--tools", "--mcp"}, MissingFlags(help, []string{"-p", "--tools", "--allowed-tools", "--mcp", "--mcp-config", "-c"})) +} + +func TestFlagsOfSkipsArguments(t *testing.T) { + assert.Equal(t, []string{"--json", "-c", "--disable"}, + FlagsOf([]string{"exec", "--json", "-c", "a=1", "-c", "b=2", "--disable", "x", "-"})) +} + +func sprintfIf(format, arg string) string { + if strings.Contains(format, "%s") { + return fmt.Sprintf(format, arg) + } + return format +} + +// A probe that times out ends its whole process tree: a launcher that started +// the real worker as a child and waited must not leave that child running +// (Codex on #794). +func TestRunProbeEndsTheLaunchersChildrenWhenItTimesOut(t *testing.T) { + dir := t.TempDir() + pidFile := filepath.Join(dir, "child.pid") + launcher := filepath.Join(dir, "claude") + require.NoError(t, os.WriteFile(launcher, []byte("#!/bin/sh\nsleep 60 &\necho $! > "+pidFile+"\nwait\n"), 0o700)) + + r := RunProbe(context.Background(), Command{Path: launcher, Dir: dir}, 300*time.Millisecond) + require.True(t, r.TimedOut) + raw, err := os.ReadFile(pidFile) + require.NoError(t, err) + pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) + require.NoError(t, err) + assert.Eventually(t, func() bool { return syscall.Kill(pid, 0) != nil }, 5*time.Second, 20*time.Millisecond, "the launcher's child is gone") +} + +// A program that is on PATH but can't be run — its interpreter is gone — is +// not reported as missing from PATH (Codex on #794). +func TestRunProbeTellsAMissingInterpreterFromAMissingProgram(t *testing.T) { + dir := t.TempDir() + launcher := filepath.Join(dir, "claude") + require.NoError(t, os.WriteFile(launcher, []byte("#!/nonexistent/interpreter\n"), 0o700)) + + r := RunProbe(context.Background(), Command{Path: launcher, Dir: dir}, 5*time.Second) + assert.False(t, r.NotFound, "the program is there") + require.Error(t, r.Err) + + missing := RunProbe(context.Background(), Command{Path: filepath.Join(dir, "absent"), Dir: dir}, 5*time.Second) + assert.True(t, missing.NotFound) +} + +// A launcher that leaves a child behind and exits on its own still has that +// child ended when the probe returns (Codex on #794). +func TestRunProbeEndsChildrenALauncherLeftWhenItExited(t *testing.T) { + dir := t.TempDir() + pidFile := filepath.Join(dir, "child.pid") + launcher := filepath.Join(dir, "claude") + require.NoError(t, os.WriteFile(launcher, []byte("#!/bin/sh\nsleep 60 &\necho $! > "+pidFile+"\necho 2.1.0\n"), 0o700)) + + r := RunProbe(context.Background(), Command{Path: launcher, Dir: dir}, 10*time.Second) + require.False(t, r.TimedOut) + raw, err := os.ReadFile(pidFile) + require.NoError(t, err) + pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) + require.NoError(t, err) + assert.Eventually(t, func() bool { return syscall.Kill(pid, 0) != nil }, 5*time.Second, 20*time.Millisecond, "the child is gone") +} diff --git a/internal/connector/driver/probe_reuse_unix_test.go b/internal/connector/driver/probe_reuse_unix_test.go new file mode 100644 index 000000000..b3505fb1d --- /dev/null +++ b/internal/connector/driver/probe_reuse_unix_test.go @@ -0,0 +1,69 @@ +//go:build unix + +package driver + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A probe's cleanup runs after the probe has been reaped, when its pid may +// already lead someone else's group. That group is left alone (Codex on #794). +func TestProbeCleanupLeavesAReusedPidAlone(t *testing.T) { + stranger := exec.CommandContext(t.Context(), "sleep", "30") + stranger.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + require.NoError(t, stranger.Start()) + exited := make(chan struct{}) + go func() { _ = stranger.Wait(); close(exited) }() + t.Cleanup(func() { _ = stranger.Process.Kill(); <-exited }) + pid := stranger.Process.Pid + + // The probe that had this pid started earlier: its recorded start time is + // not the stranger's. + g := &probeGroup{recorded: true, pid: pid, startedAt: time.Unix(1, 0), exact: true} + g.cleanup() + + select { + case <-exited: + t.Fatal("the stranger was signaled") + case <-time.After(300 * time.Millisecond): + } +} + +// A launcher can exit before the probe records it, leaving a child in its +// group: its start time is gone by then, and cleanup still ends the child +// (Codex on #794). +func TestProbeCleanupEndsTheChildOfALauncherThatExitedBeforeItWasRecorded(t *testing.T) { + dir := t.TempDir() + pidFile := filepath.Join(dir, "child.pid") + launcher := exec.CommandContext(t.Context(), "/bin/sh", "-c", "sleep 60 >/dev/null 2>&1 & echo $! > "+pidFile) + g := probeInItsOwnGroup(launcher) + require.NoError(t, launcher.Start()) + var child int + require.Eventually(t, func() bool { + raw, err := os.ReadFile(pidFile) + if err != nil || !strings.HasSuffix(string(raw), "\n") { + return false + } + child, err = strconv.Atoi(strings.TrimSpace(string(raw))) + return err == nil + }, 5*time.Second, 10*time.Millisecond) + t.Cleanup(func() { _ = syscall.Kill(child, syscall.SIGKILL) }) + require.Eventually(t, func() bool { _, err := processStartTime(launcher.Process.Pid); return err != nil }, + 5*time.Second, 10*time.Millisecond, "the launcher has exited") + + g.started() + _ = launcher.Wait() + g.cleanup() + + assert.Eventually(t, func() bool { return syscall.Kill(child, 0) != nil }, 5*time.Second, 20*time.Millisecond, "the child is gone") +} diff --git a/internal/connector/driver/worker_other.go b/internal/connector/driver/worker_other.go index df6f39846..ccc31de88 100644 --- a/internal/connector/driver/worker_other.go +++ b/internal/connector/driver/worker_other.go @@ -6,6 +6,7 @@ import ( "context" "errors" "io" + "os/exec" "time" ) @@ -52,3 +53,11 @@ func LookupProcess(int) (Process, error) { return Process{}, errUnsupported } // TerminateRecorded does nothing off Unix. func TerminateRecorded(Process, time.Duration) (bool, error) { return false, errUnsupported } + +// probeGroup leaves the probe as it is off Unix, where the connector doesn't +// run workers. +type probeGroup struct{} + +func probeInItsOwnGroup(*exec.Cmd) *probeGroup { return &probeGroup{} } +func (*probeGroup) started() {} +func (*probeGroup) cleanup() {} diff --git a/internal/connector/driver/worker_unix.go b/internal/connector/driver/worker_unix.go index b53bde913..12bde4d01 100644 --- a/internal/connector/driver/worker_unix.go +++ b/internal/connector/driver/worker_unix.go @@ -2,7 +2,12 @@ package driver -import "syscall" +import ( + "os/exec" + "sync" + "syscall" + "time" +) // newProcessGroup makes the child the leader of a new process group, so the // whole tree it starts is signaled as one. @@ -25,3 +30,75 @@ func signalGroup(pgid int, sig syscall.Signal) error { } return syscall.Kill(-pgid, sig) } + +// probeGroup is a probe run as the leader of a group of its own. +type probeGroup struct { + ec *exec.Cmd + + mu sync.Mutex + recorded bool // started has run: the probe may be waited for, and reaped + pid int + startedAt time.Time + exact bool // startedAt is the kernel's, for the pid while it was the probe +} + +// probeInItsOwnGroup runs a probe as the leader of a group of its own, and +// ends the whole group when its context does. +func probeInItsOwnGroup(ec *exec.Cmd) *probeGroup { + g := &probeGroup{ec: ec} + ec.SysProcAttr = newProcessGroup() + ec.Cancel = g.kill + return g +} + +// started records who the probe is while it can't have been reaped: RunProbe +// calls it after Start and before Wait. A launcher that has already exited is +// a zombie with no start time to read; its pid is recorded all the same. +func (g *probeGroup) started() { + g.mu.Lock() + defer g.mu.Unlock() + g.pid = g.ec.Process.Pid + if at, err := processStartTime(g.pid); err == nil { + g.startedAt, g.exact = at, true + } + g.recorded = true +} + +// cleanup ends the probe's group once the probe is over, however it ended: a +// launcher that starts the real worker as a child must not leave that child +// behind. +func (g *probeGroup) cleanup() { _ = g.kill() } + +// kill ends the probe's group, and only ever the probe's (Codex on #794). It +// runs from the context watcher, which can fire after Wait has reaped the +// probe, and from cleanup, which always does, so the pid may since have been +// given to someone else: +// - not yet recorded, the probe hasn't been waited for, so its pid is still +// its own; +// - a leader still running is killed only while its start time says it is +// the probe; +// - once the leader is gone, only its group's leftovers are: while a group +// has members the kernel won't give its id to another process (Linux and +// the BSDs keep a pid in use as a process group id), so a pid nobody holds +// with members left in its group is the probe's group, and a pid someone +// holds belongs to whoever reused it. +func (g *probeGroup) kill() error { + g.mu.Lock() + defer g.mu.Unlock() + if !g.recorded { + if g.ec.Process == nil { + return nil // it never started + } + return signalGroup(g.ec.Process.Pid, syscall.SIGKILL) + } + if g.exact { + gone, err := ProcessGone(Process{PID: g.pid, PGID: g.pid, StartedAt: g.startedAt, StartedExact: true}) + if err == nil && !gone { + return signalGroup(g.pid, syscall.SIGKILL) + } + } + if pidUnheld(g.pid) && GroupMembersRemain(Process{PID: g.pid, PGID: g.pid}) { + return signalGroup(g.pid, syscall.SIGKILL) + } + return nil +} diff --git a/internal/connector/ledger_tasks.go b/internal/connector/ledger_tasks.go index 6d3ffd371..add2ef71d 100644 --- a/internal/connector/ledger_tasks.go +++ b/internal/connector/ledger_tasks.go @@ -815,6 +815,10 @@ type SettledEvent struct { // Decided is a record a person has already redispatched or discarded, so // the completion notice asks nothing of them. Decided bool + // Pulled is whether a worker asked for the event's instruction. An event + // the launch exposed and no worker pulled is one the worker never picked + // up: it stopped before it began. + Pulled bool } // EndAttempt ends a live attempt with its stop reason, supersedes the task's @@ -869,9 +873,10 @@ UPDATE attempts SET state = 'ended', ended_at = ?, stop_reason = ?, spawn_failed outcome string replyID sql.NullInt64 exposedBy sql.NullString + pulled bool } rows, err := tx.QueryContext(ctx, ` -SELECT event_id, delivery, outcome, reply_id, exposed_attempt_id FROM task_events +SELECT event_id, delivery, outcome, reply_id, exposed_attempt_id, pulled_at IS NOT NULL FROM task_events WHERE task_id = ? AND retired_at IS NULL ORDER BY event_id`, taskID) if err != nil { return Settlement{}, fmt.Errorf("connector: settle task %d: %w", taskID, err) @@ -880,7 +885,7 @@ WHERE task_id = ? AND retired_at IS NULL ORDER BY event_id`, taskID) for rows.Next() { var r row var delivery string - if err := rows.Scan(&r.eventID, &delivery, &r.outcome, &r.replyID, &r.exposedBy); err != nil { + if err := rows.Scan(&r.eventID, &delivery, &r.outcome, &r.replyID, &r.exposedBy, &r.pulled); err != nil { _ = rows.Close() return Settlement{}, fmt.Errorf("connector: settle task %d: %w", taskID, err) } @@ -895,7 +900,7 @@ WHERE task_id = ? AND retired_at IS NULL ORDER BY event_id`, taskID) // exposure only on a task already superseded. var withdrawals []int for _, r := range events { - se := SettledEvent{EventID: r.eventID} + se := SettledEvent{EventID: r.eventID, Pulled: r.pulled} switch { case r.delivery == DeliveryCompleted: // A reported outcome stands (invariant 5). diff --git a/internal/connector/lifecycle.go b/internal/connector/lifecycle.go index d554e0d5d..ab3098a38 100644 --- a/internal/connector/lifecycle.go +++ b/internal/connector/lifecycle.go @@ -30,11 +30,17 @@ const GuardAckBody = "👀 received" // a person can tell a notice from the agent's own words. const lifecycleSignature = "automatic notice from basecamp connect" -// redispatchCommand is the only thing a lifecycle notice ever asks a person to -// do. Every notice that carries it is retractable, because a person's decision -// on the record answers it; every notice that does not — the guard -// acknowledgement, the still-running notice, a completion notice reporting an -// outcome nobody has to act on — is a record of a moment, and a record stands. +// Notices speak to the person in the thread, not to whoever runs the +// connector: what happened, in plain words, and the one thing that person can +// do, which is to mention the agent again. A new mention is a new request, so +// that retry never runs an instruction a worker has already seen. Ids stay +// only in the signature line, where reconciliation matches a notice against +// what Basecamp holds. The operator's recovery command lives in `connect +// status` and the log, not in the thread. +// +// Notices posted before this carried redispatchCommand as an ask. Those are +// still read back and answered when a person acts on them (renderRetraction); +// no notice written now carries it, so none needs answering. const redispatchCommand = "basecamp connect redispatch " func redispatchAsk(eventID int64) string { @@ -104,10 +110,10 @@ func eventList(ids []int64) string { // the connector does not serve. func renderHoldingReply(kind MessageKind, eventID int64) string { lines := []string{ - "I can't start on this here yet: this project is not one my connector is set up to work in, so nothing was run.", - "Once the project is added to connect.json, a person can run it with: " + redispatchAsk(eventID), + "I'm not set up to work in this project yet, so I haven't started on this.", + "Once it's added to my projects, mention me again.", "", - "Event " + strconv.FormatInt(eventID, 10) + " · " + lifecycleSignature, + "Ref " + strconv.FormatInt(eventID, 10) + " · " + lifecycleSignature, } return renderLines(kind, lines) } @@ -217,15 +223,15 @@ FROM events e WHERE e.id = ?1` // The stamp is the connector's own — when it saw the attempt live — and not // the worker's last progress, which is a different fact the notice reports // separately: a task can be alive and quiet for an hour. -func renderStillRunning(kind MessageKind, taskID int64, attemptID string, occurrence int, at, launchedAt, progressAt time.Time) string { +func renderStillRunning(kind MessageKind, _ int64, attemptID string, occurrence int, at, launchedAt, progressAt time.Time) string { progress := "No progress has been reported yet." if !progressAt.IsZero() { progress = "Last progress at " + clock(progressAt) + "." } lines := []string{ - "Working on this as of " + clock(at) + ": task " + strconv.FormatInt(taskID, 10) + " started at " + clock(launchedAt) + ". " + progress, + "Working on this as of " + clock(at) + ", started at " + clock(launchedAt) + ". " + progress, "", - "Attempt " + attemptID + ", update " + strconv.Itoa(occurrence) + " · " + lifecycleSignature, + "Ref attempt " + attemptID + ", update " + strconv.Itoa(occurrence) + " · " + lifecycleSignature, } return renderLines(kind, lines) } @@ -237,7 +243,7 @@ func renderStillRunning(kind MessageKind, taskID int64, attemptID string, occurr // a task of their own or withdrawn for their one automatic retry. func CompletionNeeded(s Settlement) bool { for _, e := range s.Events { - if completionLine(e) != "" { + if completionLine(e, s) != "" { return true } } @@ -245,45 +251,77 @@ func CompletionNeeded(s Settlement) bool { } // completionLine is what the notice says about one event; empty when it says -// nothing. -func completionLine(e SettledEvent) string { - id := strconv.FormatInt(e.EventID, 10) - redispatch := " Needs a person: " + redispatchAsk(e.EventID) - if e.Decided { - // A person already redispatched or discarded it: the notice says what - // happened, and asks for nothing. - redispatch = "" - } +// nothing. It is the notice's own sentence, so a notice about several events +// reads as one short paragraph. +func completionLine(e SettledEvent, s Settlement) string { switch { - case e.Blocked: - return "Event " + id + ": the worker could not be started." + redispatch + case neverStarted(e, s): + return "I couldn't start on this: something's wrong on the computer I run on." case e.Withdrawn, e.Returned: return "" + case e.Outcome == OutcomeFailed && e.ReplyID != nil: + // The worker replied and said why: the person already knows, and + // mentioning the agent again would not change a refusal. The + // operator still sees the failure in connect status. + return "" case e.Outcome == OutcomeFailed: - return "Event " + id + ": failed." + redispatch + return "Something went wrong and I couldn't finish this." case e.Outcome == OutcomeUnknown: - return "Event " + id + ": unknown, the worker did not report on it." + redispatch + return unfinishedSentence(s.Stop) case e.Outcome == OutcomeSucceeded && e.ReplyID == nil: - return "Event " + id + ": succeeded, with no reply reported." + // Only that no reply was reported: a worker may reply and not say so. + return "I finished this, but didn't report a reply." } return "" } -// stopSentence says how an attempt stopped. -func stopSentence(stop StopReason) string { +// unfinishedSentence says why a request the worker never reported on may be +// unfinished, in the words a person reading the thread would use. May: an +// unknown outcome isn't an unfinished one. The worker may have done the work, +// even replied, and stopped before it said so, so the notice doesn't claim +// it wasn't done. +func unfinishedSentence(stop StopReason) string { switch stop { - case StopFinished: - return "the worker finished" - case StopFailed: - return "the worker failed" + case StopShutdown, StopLost: + return "I was interrupted and may not have finished this." case StopDeadline: - return "the worker was stopped at the task's deadline" - case StopShutdown: - return "the connector shut down and stopped the worker" - case StopLost: - return "the worker was lost" + return "I ran out of time and may not have finished this." + case StopFinished, StopFailed: + return "I stopped and may not have finished this." } - return "the worker stopped" + return "I stopped and may not have finished this." +} + +// retryable reports whether mentioning the agent again is worth suggesting: a +// request that was not finished and that no person has already decided. +func retryable(e SettledEvent, s Settlement) bool { + if e.Decided || e.Withdrawn || e.Returned || neverStarted(e, s) { + return false + } + return e.Outcome == OutcomeFailed || e.Outcome == OutcomeUnknown +} + +// neverStarted reports whether the worker failed before it picked the request +// up: a start refused a second automatic retry, or an exposure no worker ever +// pulled from a worker that failed. Mentioning the agent again would meet the +// same computer, so the notice asks its operator to look instead. A worker +// that finished a turn without asking for its request ran; one that ran out +// its deadline was running; and an exposure the connector itself interrupted, +// by a shutdown or a crash, says it was interrupted. So does a worker that +// picked up another of the settlement's requests: it started, and what it +// failed to get to is a follow-up it left unfinished. +func neverStarted(e SettledEvent, s Settlement) bool { + switch { + case e.Blocked: + // A blocked record is also withdrawn from its task; blocked is what + // the notice reports, so it decides. + return true + case e.Withdrawn, e.Returned, e.Pulled: + return false + case e.Outcome != OutcomeUnknown: + return false + } + return s.Stop == StopFailed && !pickedUp(s) } // renderCompletion is an attempt's completion notice, or "" when the @@ -292,13 +330,39 @@ func renderCompletion(kind MessageKind, s Settlement) string { if !CompletionNeeded(s) { return "" } - lines := []string{"Task " + strconv.FormatInt(s.TaskID, 10) + " ended: " + stopSentence(s.Stop) + "."} + var ( + sentences []string + ids []string + retry bool + unsure bool + operator bool + ) for _, e := range s.Events { - if line := completionLine(e); line != "" { - lines = append(lines, line) + line := completionLine(e, s) + if line == "" { + continue + } + if !slices.Contains(sentences, line) { + sentences = append(sentences, line) } + ids = append(ids, strconv.FormatInt(e.EventID, 10)) + retry = retry || retryable(e, s) + unsure = unsure || (retryable(e, s) && e.Outcome == OutcomeUnknown) + operator = operator || neverStarted(e, s) } - lines = append(lines, "", "Attempt "+s.AttemptID+" · "+lifecycleSignature) + lines := []string{strings.Join(sentences, " ")} + if operator { + lines = append(lines, "The person who runs me needs to check it.") + } + switch { + case unsure: + // Mentioning again after work that was in fact done would do it + // twice: the reader looks first. + lines = append(lines, "If I didn't, mention me again to try again.") + case retry: + lines = append(lines, "Mention me again to try again.") + } + lines = append(lines, "", "Ref "+strings.Join(ids, ", ")+" · attempt "+s.AttemptID+" · "+lifecycleSignature) return renderLines(kind, lines) } @@ -614,7 +678,7 @@ FROM attempts a JOIN tasks t ON t.id = a.task_id WHERE a.id = ? AND a.state = 'e // equal to the attempt's end is not evidence that it came after: the // notice asks again, which is the safe direction. rows, err := q.QueryContext(ctx, ` -SELECT te.event_id, te.delivery, te.outcome, te.reply_id, te.withdrawn_at IS NOT NULL, e.state, e.reason, +SELECT te.event_id, te.delivery, te.outcome, te.reply_id, te.pulled_at IS NOT NULL, te.withdrawn_at IS NOT NULL, e.state, e.reason, e.state NOT IN ('completed', 'blocked') OR e.redispatch_decision IS NOT NULL OR COALESCE(e.authorized_at > (SELECT ended_at FROM attempts WHERE id = ?2), 0) FROM task_events te JOIN events e ON e.id = te.event_id @@ -631,7 +695,7 @@ ORDER BY te.event_id`, s.TaskID, attemptID) reason string reply sql.NullInt64 ) - if err := rows.Scan(&e.EventID, &delivery, &outcome, &reply, &e.Withdrawn, &state, &reason, &e.Decided); err != nil { + if err := rows.Scan(&e.EventID, &delivery, &outcome, &reply, &e.Pulled, &e.Withdrawn, &state, &reason, &e.Decided); err != nil { return Settlement{}, fmt.Errorf("connector: settlement of %s: %w", attemptID, err) } switch { diff --git a/internal/connector/lifecycle_legacy_test.go b/internal/connector/lifecycle_legacy_test.go new file mode 100644 index 000000000..949e1ce57 --- /dev/null +++ b/internal/connector/lifecycle_legacy_test.go @@ -0,0 +1,114 @@ +package connector + +import ( + "context" + "strconv" + "testing" + + "github.com/stretchr/testify/require" +) + +// Notices as they were written before notices stopped asking a person to run +// `basecamp connect redispatch`. A ledger in use still holds notices like +// these, sent and standing on somebody's card, and a person acting on one is +// still answered (renderRetraction). These are the fixtures for that path: +// the words are the earlier version's, verbatim, and nothing here renders a +// notice the connector sends now. + +func legacyHoldingReply(kind MessageKind, eventID int64) string { + lines := []string{ + "I can't start on this here yet: this project is not one my connector is set up to work in, so nothing was run.", + "Once the project is added to connect.json, a person can run it with: " + redispatchAsk(eventID), + "", + "Event " + strconv.FormatInt(eventID, 10) + " · " + lifecycleSignature, + } + return renderLines(kind, lines) +} + +func legacyCompletionLine(e SettledEvent) string { + id := strconv.FormatInt(e.EventID, 10) + redispatch := " Needs a person: " + redispatchAsk(e.EventID) + if e.Decided { + redispatch = "" + } + switch { + case e.Blocked: + return "Event " + id + ": the worker could not be started." + redispatch + case e.Withdrawn, e.Returned: + return "" + case e.Outcome == OutcomeFailed: + return "Event " + id + ": failed." + redispatch + case e.Outcome == OutcomeUnknown: + return "Event " + id + ": unknown, the worker did not report on it." + redispatch + case e.Outcome == OutcomeSucceeded && e.ReplyID == nil: + return "Event " + id + ": succeeded, with no reply reported." + } + return "" +} + +func legacyStopSentence(stop StopReason) string { + switch stop { + case StopFinished: + return "the worker finished" + case StopFailed: + return "the worker failed" + case StopDeadline: + return "the worker was stopped at the task's deadline" + case StopShutdown: + return "the connector shut down and stopped the worker" + case StopLost: + return "the worker was lost" + } + return "the worker stopped" +} + +func legacyCompletion(kind MessageKind, s Settlement) string { + lines := []string{"Task " + strconv.FormatInt(s.TaskID, 10) + " ended: " + legacyStopSentence(s.Stop) + "."} + for _, e := range s.Events { + if line := legacyCompletionLine(e); line != "" { + lines = append(lines, line) + } + } + lines = append(lines, "", "Attempt "+s.AttemptID+" · "+lifecycleSignature) + return renderLines(kind, lines) +} + +// obPostedByAnEarlierVersion makes a sent notice read as an earlier version +// wrote it — asking a person to run redispatch — both in the ledger, which is +// what a retraction reads back, and in Basecamp, which is what a reader sees. +// basecamp may be nil when the test holds no fake. +func obPostedByAnEarlierVersion(t *testing.T, ledger *Ledger, basecamp *fakeBasecamp, in Intent) Intent { + t.Helper() + ctx := context.Background() + require.Contains(t, []IntentState{IntentSent, IntentSending}, in.State, + "a notice that went out, or may have: those are the ones a retraction reads back") + + var body string + switch in.Kind { + case IntentHoldingReply: + body = legacyHoldingReply(in.Destination.Kind, in.EventID) + case IntentCompletion: + s, err := settlementFromRecords(ctx, ledger.db, in.AttemptID) + require.NoError(t, err) + body = legacyCompletion(in.Destination.Kind, s) + default: + t.Fatalf("no earlier wording for a %s notice", in.Kind) + } + require.NotEmpty(t, redispatchAsksIn(body), "an earlier notice asks") + + _, err := ledger.db.ExecContext(ctx, `UPDATE outbox SET body = ? WHERE id = ?`, body, in.ID) + require.NoError(t, err) + + if basecamp != nil && in.ReceiptID != nil { + basecamp.mu.Lock() + messages := basecamp.messages[obKey(in.Destination)] + for i := range messages { + if messages[i].ID == *in.ReceiptID { + messages[i].Content = body + } + } + basecamp.mu.Unlock() + } + + return obIntent(t, ledger, in.Key) +} diff --git a/internal/connector/lifecycle_test.go b/internal/connector/lifecycle_test.go index dbde2a06f..ab006d736 100644 --- a/internal/connector/lifecycle_test.go +++ b/internal/connector/lifecycle_test.go @@ -2,7 +2,6 @@ package connector import ( "context" - "strconv" "strings" "testing" "time" @@ -37,12 +36,39 @@ func TestLifecycleTemplatesRenderFromRecordsAlone(t *testing.T) { assert.Equal(t, in.Body, renderCompletion(in.Destination.Kind, settlement), "the ledger and the settlement agree") assert.Equal(t, Destination{BucketID: adapterBucketID, Kind: MessageComment, RecordingID: obReplyRecording}, in.Destination) assert.Equal(t, - "
Task "+itoa(l.TaskID)+" ended: the worker was stopped at the task's deadline.
"+ - "Event 1: unknown, the worker did not report on it. Needs a person: basecamp connect redispatch 1
"+ - "
Attempt "+l.AttemptID+" · automatic notice from basecamp connect
", + "
I ran out of time and may not have finished this.
"+ + "If I didn't, mention me again to try again.
"+ + "
Ref 1 · attempt "+l.AttemptID+" · automatic notice from basecamp connect
", in.Body, "event 2 was never exposed: it waits for a task of its own and is not named") }) + t.Run("a worker that never picked the request up", func(t *testing.T) { + for _, pulled := range []bool{false, true} { + ctx := context.Background() + ledger, _ := obLedger(t) + obAdmit(t, ledger, 1, "recording:10304028989") + l := obLaunch(t, ledger, 1) + if pulled { + d, err := ledger.Dispatch(ctx, l.Token, adapterAgentID) + require.NoError(t, err) + obPull(t, d, 1) + } + settlement, err := ledger.EndAttempt(ctx, AttemptEnd{AttemptID: l.AttemptID, Stop: StopFailed}) + require.NoError(t, err) + + in := obIntent(t, ledger, completionKey(l.AttemptID)) + fromRows, err := settlementFromRecords(ctx, ledger.db, l.AttemptID) + require.NoError(t, err) + assert.Equal(t, in.Body, renderCompletion(in.Destination.Kind, fromRows)) + assert.Equal(t, in.Body, renderCompletion(in.Destination.Kind, settlement), "the ledger and the settlement agree") + if pulled { + assert.Contains(t, MessageText(in.Body), "I stopped and may not have finished this. If I didn't, mention me again to try again.") + } else { + assert.Contains(t, MessageText(in.Body), couldNotStart+" "+operatorChecks+" Ref 1 ·") + } + } + }) + t.Run("still running", func(t *testing.T) { ctx := context.Background() ledger, clock := obLedger(t) @@ -58,8 +84,8 @@ func TestLifecycleTemplatesRenderFromRecordsAlone(t *testing.T) { assert.Equal(t, IntentStillRunning, in.Kind) assert.Equal(t, renderStillRunning(MessageComment, l.TaskID, l.AttemptID, 1, in.CreatedAt, l.LaunchedAt, tick.ProgressAt), in.Body) assert.Equal(t, - "
Working on this as of 12:10 UTC: task "+itoa(l.TaskID)+" started at 12:00 UTC. Last progress at 12:03 UTC.

"+ - "Attempt "+l.AttemptID+", update 1 · automatic notice from basecamp connect
", in.Body) + "
Working on this as of 12:10 UTC, started at 12:00 UTC. Last progress at 12:03 UTC.

"+ + "Ref attempt "+l.AttemptID+", update 1 · automatic notice from basecamp connect
", in.Body) }) t.Run("holding reply in a Campfire", func(t *testing.T) { @@ -84,8 +110,6 @@ func TestLifecycleTemplatesRenderFromRecordsAlone(t *testing.T) { }) } -func itoa(v int64) string { return strconv.FormatInt(v, 10) } - // A still-running notice can be the connector's last word on a task — an // attempt whose events all succeeded with a reply calls for no completion // notice — so it says when it was true and claims nothing about now. The @@ -103,7 +127,7 @@ func TestStillRunningNoticeIsDatedAndNotPresentTense(t *testing.T) { require.NoError(t, err) quiet := obIntent(t, ledger, stillRunningKey(l.AttemptID, 1)) assert.Contains(t, MessageText(quiet.Body), - "Working on this as of 12:40 UTC: task "+itoa(l.TaskID)+" started at 12:00 UTC. No progress has been reported yet.", + "Working on this as of 12:40 UTC, started at 12:00 UTC. No progress has been reported yet.", "an attempt with nothing to report is still dated by the sighting") require.NoError(t, ledger.RecordProgress(ctx, l.AttemptID)) @@ -112,7 +136,7 @@ func TestStillRunningNoticeIsDatedAndNotPresentTense(t *testing.T) { require.NoError(t, err) reported := obIntent(t, ledger, stillRunningKey(l.AttemptID, 2)) assert.Contains(t, MessageText(reported.Body), - "Working on this as of 13:00 UTC: task "+itoa(l.TaskID)+" started at 12:00 UTC. Last progress at 12:40 UTC.", + "Working on this as of 13:00 UTC, started at 12:00 UTC. Last progress at 12:40 UTC.", "the sighting and the worker's last progress are different facts, both said") for _, in := range []Intent{quiet, reported} { @@ -149,13 +173,21 @@ func TestLifecycleMessagesCarryNoContent(t *testing.T) { } } +const ( + couldNotStart = "I couldn't start on this: something's wrong on the computer I run on." + operatorChecks = "The person who runs me needs to check it." +) + // Completion: one notice per attempt, when anything is not succeeded with a -// reply; the notice names what needs redispatch. +// reply. It tells the person in the thread what happened in plain words, and +// suggests mentioning the agent again only when that would help. func TestCompletionNoticeRule(t *testing.T) { + const sig = "automatic notice from basecamp connect" cases := []struct { name string + stop StopReason events []SettledEvent - want []string + want string }{ {name: "all succeeded with replies", events: []SettledEvent{ {EventID: 1, Outcome: OutcomeSucceeded, Reported: true, ReplyID: id64(5)}, @@ -163,47 +195,99 @@ func TestCompletionNoticeRule(t *testing.T) { }}, {name: "succeeded without a reply", events: []SettledEvent{ {EventID: 1, Outcome: OutcomeSucceeded, Reported: true}, - }, want: []string{"Event 1: succeeded, with no reply reported."}}, - {name: "failed and unknown need redispatch", events: []SettledEvent{ + }, want: "I finished this, but didn't report a reply.\n\nRef 1 · attempt att_x · " + sig}, + {name: "failed and unknown suggest mentioning again", events: []SettledEvent{ {EventID: 1, Outcome: OutcomeSucceeded, Reported: true, ReplyID: id64(5)}, - {EventID: 2, Outcome: OutcomeFailed, Reported: true, ReplyID: id64(6)}, + {EventID: 2, Outcome: OutcomeFailed, Reported: true}, {EventID: 3, Outcome: OutcomeUnknown}, - }, want: []string{ - "Event 2: failed. Needs a person: basecamp connect redispatch 2", - "Event 3: unknown, the worker did not report on it. Needs a person: basecamp connect redispatch 3", + }, want: "Something went wrong and I couldn't finish this. I stopped and may not have finished this.\n" + + "If I didn't, mention me again to try again.\n\nRef 2, 3 · attempt att_x · " + sig}, + {name: "failed with a reply has already said why", events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeFailed, Reported: true, ReplyID: id64(6)}, }}, + {name: "decided by a person asks for nothing", events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeUnknown, Decided: true, Pulled: true}, + }, want: "I stopped and may not have finished this.\n\nRef 1 · attempt att_x · " + sig}, {name: "only returned or withdrawn for a retry", events: []SettledEvent{ {EventID: 1, Withdrawn: true}, {EventID: 2, Returned: true}, }}, {name: "blocked after a second failed start", events: []SettledEvent{ {EventID: 1, Withdrawn: true, Blocked: true}, - }, want: []string{"Event 1: the worker could not be started. Needs a person: basecamp connect redispatch 1"}}, + }, want: couldNotStart + "\n" + operatorChecks + "\n\nRef 1 · attempt att_x · " + sig}, + {name: "a worker that failed before it picked the request up", stop: StopFailed, events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeUnknown}, + }, want: couldNotStart + "\n" + operatorChecks + "\n\nRef 1 · attempt att_x · " + sig}, + {name: "a worker that finished a turn without picking the request up", events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeUnknown}, + }, want: "I stopped and may not have finished this.\nIf I didn't, mention me again to try again.\n\nRef 1 · attempt att_x · " + sig}, + {name: "a follow-up lost by a worker that had picked up the first request", stop: StopFailed, events: []SettledEvent{ + {EventID: 1, Pulled: true, Outcome: OutcomeSucceeded, Reported: true, ReplyID: id64(5)}, + {EventID: 2, Outcome: OutcomeUnknown}, + }, want: "I stopped and may not have finished this.\nIf I didn't, mention me again to try again.\n\nRef 2 · attempt att_x · " + sig}, + {name: "never picked up and decided by a person", stop: StopFailed, events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeUnknown, Decided: true}, + }, want: couldNotStart + "\n" + operatorChecks + "\n\nRef 1 · attempt att_x · " + sig}, + {name: "never picked up before the connector was interrupted", stop: StopLost, events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeUnknown}, + }, want: "I was interrupted and may not have finished this.\nIf I didn't, mention me again to try again.\n\nRef 1 · attempt att_x · " + sig}, + {name: "never picked up before its deadline", stop: StopDeadline, events: []SettledEvent{ + {EventID: 1, Outcome: OutcomeUnknown}, + }, want: "I ran out of time and may not have finished this.\nIf I didn't, mention me again to try again.\n\nRef 1 · attempt att_x · " + sig}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - s := Settlement{TaskID: 9, AttemptID: "att_x", Stop: StopFinished, Events: tc.events} - body := renderCompletion(MessageChatLine, s) - assert.Equal(t, len(tc.want) > 0, CompletionNeeded(s)) - if len(tc.want) == 0 { - assert.Empty(t, body) - return + stop := tc.stop + if stop == "" { + stop = StopFinished } - assert.Equal(t, "Task 9 ended: the worker finished.\n"+strings.Join(tc.want, "\n")+"\n\nAttempt att_x · automatic notice from basecamp connect", body) + s := Settlement{TaskID: 9, AttemptID: "att_x", Stop: stop, Events: tc.events} + assert.Equal(t, tc.want != "", CompletionNeeded(s)) + assert.Equal(t, tc.want, renderCompletion(MessageChatLine, s)) }) } } -// Every stop reason reads as itself; a failure is never called a cancel. -func TestCompletionNamesEachStopReason(t *testing.T) { - seen := map[string]bool{} +// Why a request was left unfinished, in a reader's words: an interruption, the +// deadline, or an unexplained stop. Never "cancel", and never the machinery. +func TestUnfinishedSaysWhyInPlainWords(t *testing.T) { + assert.Equal(t, unfinishedSentence(StopShutdown), unfinishedSentence(StopLost), "to a reader, a shutdown and a crash are both an interruption") + sentences := map[string]bool{} for _, stop := range []StopReason{StopFinished, StopFailed, StopDeadline, StopShutdown, StopLost} { - sentence := stopSentence(stop) - assert.NotEqual(t, "the worker stopped", sentence, stop) - assert.NotContains(t, sentence, "cancel", stop) - assert.False(t, seen[sentence], stop) - seen[sentence] = true + sentence := unfinishedSentence(stop) + sentences[sentence] = true + for _, jargon := range []string{"cancel", "worker", "task", "connector", "attempt"} { + assert.NotContains(t, sentence, jargon, stop) + } + } + assert.Len(t, sentences, 3, "interrupted, out of time, stopped") +} + +// Notices are for the person in the thread: no commands, no config files, no +// machinery, and no ask a person has to come back and answer. The ids live in +// the signature line, where reconciliation matches a notice against Basecamp. +func TestNoticesSpeakToThePersonInTheThread(t *testing.T) { + at := time.Date(2026, 9, 28, 12, 0, 0, 0, time.UTC) + bodies := map[string]string{ + "holding reply": renderHoldingReply(MessageComment, 41), + "still running": renderStillRunning(MessageComment, 7, "att_y", 1, at.Add(10*time.Minute), at, time.Time{}), + "completion": renderCompletion(MessageComment, Settlement{TaskID: 7, AttemptID: "att_y", Stop: StopLost, + Events: []SettledEvent{{EventID: 41, Outcome: OutcomeUnknown}}}), + "never started": renderCompletion(MessageComment, Settlement{TaskID: 7, AttemptID: "att_y", Stop: StopFailed, + Events: []SettledEvent{{EventID: 41, Outcome: OutcomeUnknown}}}), + } + for name, body := range bodies { + text := MessageText(body) + for _, jargon := range []string{"basecamp connect redispatch", "connect.json", "Needs a person", "worker", "Task 7", "Event 41"} { + assert.NotContains(t, text, jargon, name) + } + assert.True(t, strings.HasSuffix(text, "· "+lifecycleSignature), name) + assert.Empty(t, redispatchAsksIn(body), "%s asks nothing a person has to come back and answer", name) } + assert.Contains(t, MessageText(bodies["holding reply"]), "Ref 41 ·") + assert.Contains(t, MessageText(bodies["completion"]), "Ref 41 · attempt att_y ·") + assert.NotContains(t, MessageText(bodies["never started"]), "Mention me again", + "mentioning the agent again would meet the same computer") } // An attempt whose events all succeeded with replies posts nothing. @@ -262,7 +346,8 @@ func TestOutboxCompletionReadsBlockedBack(t *testing.T) { completions, err := ledger.Intents(ctx, IntentFilter{Kinds: []IntentKind{IntentCompletion}}) require.NoError(t, err) require.Len(t, completions, 1, "the first withdrawal retries quietly; the second needs a person") - assert.Contains(t, completions[0].Body, "Event 1: the worker could not be started. Needs a person: basecamp connect redispatch 1") + assert.Contains(t, MessageText(completions[0].Body), couldNotStart+" "+operatorChecks) + assert.NotContains(t, completions[0].Body, "Mention me again", "a mention would meet the same computer") } // The holding reply answers only a request blocked for want of a route. diff --git a/internal/connector/operator_invariants_test.go b/internal/connector/operator_invariants_test.go index 00b0fa487..40822de86 100644 --- a/internal/connector/operator_invariants_test.go +++ b/internal/connector/operator_invariants_test.go @@ -794,8 +794,8 @@ func TestACompletionNoticeAsksNothingOfADecidedRecord(t *testing.T) { intents, err := l.Intents(ctx, IntentFilter{Kinds: []IntentKind{IntentCompletion}}) require.NoError(t, err) require.Len(t, intents, 1) - assert.Contains(t, intents[0].Body, "Event 1: failed") - assert.NotContains(t, intents[0].Body, "redispatch 1", "a person already redispatched it") + assert.Contains(t, MessageText(intents[0].Body), "Something went wrong and I couldn't finish this.") + assert.NotContains(t, intents[0].Body, "Mention me again", "a person already redispatched it") } // An import that closes a record does not leave it a lifecycle message that @@ -831,7 +831,7 @@ func TestACompletionNoticeIsRenderedAgainWhenItIsClaimed(t *testing.T) { notices, err := l.Intents(ctx, IntentFilter{Kinds: []IntentKind{IntentCompletion}}) require.NoError(t, err) require.Len(t, notices, 1) - require.Contains(t, notices[0].Body, "redispatch 1") + require.Contains(t, notices[0].Body, "If I didn't, mention me again to try again.") _, err = l.Discard(ctx, 1, opBy) require.NoError(t, err) @@ -839,8 +839,8 @@ func TestACompletionNoticeIsRenderedAgainWhenItIsClaimed(t *testing.T) { require.NoError(t, err) require.True(t, ok) assert.Equal(t, IntentSending, claimed.State) - assert.Contains(t, claimed.Body, "Event 1: unknown") - assert.NotContains(t, claimed.Body, "redispatch 1", "a person already decided it") + assert.Contains(t, claimed.Body, "may not have finished this.") + assert.NotContains(t, claimed.Body, "Mention me again", "a person already decided it") } // Invariant 2, at the database: a task takes no follow-up while the hold @@ -861,7 +861,9 @@ func TestInvariant2ATaskTakesNoFollowUpUnderTheHold(t *testing.T) { } // A record a person authorized is decided too, though it stays blocked until -// its prerequisite runs: the notice claimed meanwhile asks nothing more. +// its prerequisite runs: the notice claimed meanwhile asks nothing more. A +// record blocked on its start asks the person who runs the agent, not the +// thread, so neither notice suggests a mention. func TestACompletionNoticeAsksNothingOfAnAuthorizedBlockedRecord(t *testing.T) { l := newTestLedger(t) l.SetHooks(LifecycleHooks(l, LifecycleOptions{})) @@ -876,7 +878,8 @@ func TestACompletionNoticeAsksNothingOfAnAuthorizedBlockedRecord(t *testing.T) { notices, err := l.Intents(ctx, IntentFilter{Kinds: []IntentKind{IntentCompletion}}) require.NoError(t, err) require.Len(t, notices, 1) - require.Contains(t, notices[0].Body, "redispatch 1") + require.Contains(t, notices[0].Body, "The person who runs me needs to check it.") + require.NotContains(t, notices[0].Body, "Mention me again") got, err := l.Redispatch(ctx, 1, opBy, []int64{adapterBucketID}) require.NoError(t, err) @@ -884,12 +887,17 @@ func TestACompletionNoticeAsksNothingOfAnAuthorizedBlockedRecord(t *testing.T) { claimed, ok, err := l.claimIntent(ctx) require.NoError(t, err) require.True(t, ok) - assert.NotContains(t, claimed.Body, "redispatch 1") + assert.NotContains(t, claimed.Body, "Mention me again") } // An authorization answers for the outcome it was made on. When the attempt it -// led to ends unknown again, or cannot start, the notice asks again. +// led to ends unknown again the notice asks again, and when it cannot start the +// notice asks the person who runs the agent. func TestAnEarlierAuthorizationDoesNotSilenceALaterNotice(t *testing.T) { + asks := map[string]string{ + "unknown again": "If I didn't, mention me again to try again.", + "blocked on its start": "The person who runs me needs to check it.", + } for name, second := range map[string]func(t *testing.T, l *Ledger){ "unknown again": func(t *testing.T, l *Ledger) { launch := launchOf(t, l, 1) @@ -926,7 +934,7 @@ func TestAnEarlierAuthorizationDoesNotSilenceALaterNotice(t *testing.T) { } } require.NotZero(t, latest.ID) - assert.Contains(t, latest.Body, "redispatch 1", "a person is asked again") + assert.Contains(t, latest.Body, asks[name], "a person is asked again") }) } } diff --git a/internal/connector/recovery_dispatch_test.go b/internal/connector/recovery_dispatch_test.go index 9405efda4..fcaec4f1e 100644 --- a/internal/connector/recovery_dispatch_test.go +++ b/internal/connector/recovery_dispatch_test.go @@ -8,6 +8,7 @@ import ( "math" "os" "os/exec" + "regexp" "slices" "strconv" "strings" @@ -98,11 +99,20 @@ func recordedWorkers(t *testing.T, l *Ledger) []int { return out } +// noticeRef is a completion notice's signature line, which names the events +// it speaks for: "Ref 101, 102 · attempt att_… · automatic notice…". +var noticeRef = regexp.MustCompile(`Ref ([0-9]+(?:, [0-9]+)*) · attempt `) + // notices are the connector's completion notices naming the event. func (h *harness) notices(eventID int64) []storedMessage { var out []storedMessage + id := strconv.FormatInt(eventID, 10) for _, m := range h.connectorPosts() { - if strings.Contains(m.Content, "automatic notice") && strings.Contains(m.Content, "Event "+strconv.FormatInt(eventID, 10)+":") { + text := MessageText(m.Content) + if !strings.Contains(text, "automatic notice") { + continue + } + if ref := noticeRef.FindStringSubmatch(text); ref != nil && slices.Contains(strings.Split(ref[1], ", "), id) { out = append(out, m) } } @@ -320,8 +330,9 @@ func TestRecoveryPostsUnknownAndRedispatchRunsItAgain(t *testing.T) { notices := h.notices(101) require.Len(t, notices, 1) - assert.Contains(t, notices[0].Content, "Event 101: unknown") - assert.Contains(t, notices[0].Content, "basecamp connect redispatch 101") + assert.Contains(t, notices[0].Content, "may not have finished this.") + assert.Contains(t, notices[0].Content, "If I didn't, mention me again to try again.") + assert.NotContains(t, notices[0].Content, "basecamp connect redispatch", "the operator's command is not the thread's business") assert.Equal(t, int64(5001), notices[0].RecordingID, "on the recording that asked") l := h.ledger() @@ -394,7 +405,11 @@ func TestRecoveryRetriesASpawnErrorOnce(t *testing.T) { assert.False(t, attempts[0].SpawnFailed) assert.Equal(t, string(StopFailed), attempts[0].StopReason) assert.Equal(t, 1, h.agentStarts()) - assert.Len(t, h.notices(101), 1) + notices := h.notices(101) + require.Len(t, notices, 1) + assert.Equal(t, couldNotStart+" "+operatorChecks, + strings.SplitN(MessageText(notices[0].Content), " Ref ", 2)[0], + "the worker never picked the request up: the thread is told the computer needs looking at") }) }) } @@ -412,7 +427,8 @@ func assertSpawnBlocked(t *testing.T, h *harness, l *Ledger) { assert.Zero(t, h.agentStarts(), "no worker process ever existed") notices := h.notices(101) require.Len(t, notices, 1) - assert.Contains(t, notices[0].Content, "could not be started") + assert.Equal(t, couldNotStart+" "+operatorChecks, + strings.SplitN(MessageText(notices[0].Content), " Ref ", 2)[0]) } // Follow-ups and their siblings survive a task's end, a crash included: each diff --git a/internal/connector/retraction_test.go b/internal/connector/retraction_test.go index 05bc03db4..0cc9f1150 100644 --- a/internal/connector/retraction_test.go +++ b/internal/connector/retraction_test.go @@ -55,6 +55,7 @@ func TestPostedAskIsRetractedWhenItIsAnswered(t *testing.T) { require.NoError(t, ob.Flush(ctx)) holding := obIntent(t, ledger, holdingKey(1)) require.Equal(t, IntentSent, holding.State) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) clock.Advance(12 * time.Minute) _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) @@ -93,6 +94,7 @@ func TestPostedAskIsRetractedWhenItIsAnswered(t *testing.T) { require.NoError(t, ob.Flush(ctx)) completion := obIntent(t, ledger, completionKey(l.AttemptID)) require.Equal(t, IntentSent, completion.State) + completion = obPostedByAnEarlierVersion(t, ledger, basecamp, completion) require.Contains(t, completion.Body, "Needs a person: basecamp connect redispatch 1") clock.Advance(3 * time.Minute) @@ -117,6 +119,7 @@ func TestPostedAskIsRetractedWhenItIsAnswered(t *testing.T) { require.NoError(t, ob.Flush(ctx)) holding := obIntent(t, ledger, holdingKey(1)) require.Equal(t, IntentSent, holding.State) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) clock.Advance(90 * time.Minute) _, err = ledger.Discard(ctx, 1, "jorge") @@ -142,6 +145,7 @@ func TestPostedAskIsRetractedWhenItIsAnswered(t *testing.T) { require.NoError(t, ob.Flush(ctx)) holding := obIntent(t, ledger, holdingKey(1)) require.Equal(t, IntentSent, holding.State) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) require.NoError(t, err) @@ -176,6 +180,7 @@ func TestARetractionSpeaksOnlyForItsOwnEvent(t *testing.T) { require.NoError(t, ob.Flush(ctx)) completion := obIntent(t, ledger, completionKey(l.AttemptID)) require.Equal(t, IntentSent, completion.State) + completion = obPostedByAnEarlierVersion(t, ledger, basecamp, completion) require.Contains(t, completion.Body, "Needs a person: basecamp connect redispatch 1") require.Contains(t, completion.Body, "Needs a person: basecamp connect redispatch 2") @@ -204,14 +209,16 @@ func TestARetractionSpeaksOnlyForItsOwnEvent(t *testing.T) { } // Which events a posted message asks about, read off the words that went out. +// Only notices an earlier version posted ask anything. func TestRedispatchAsksInReadsEveryAsk(t *testing.T) { - two := renderCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFailed, Events: []SettledEvent{ + two := legacyCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFailed, Events: []SettledEvent{ {EventID: 41, Outcome: OutcomeFailed}, {EventID: 42, Outcome: OutcomeSucceeded, Reported: true}, {EventID: 43, Outcome: OutcomeUnknown}, }}) assert.Equal(t, []int64{41, 43}, redispatchAsksIn(two), "the succeeded event is reported, not asked about") - assert.Equal(t, []int64{7}, redispatchAsksIn(renderHoldingReply(MessageComment, 7))) + assert.Equal(t, []int64{7}, redispatchAsksIn(legacyHoldingReply(MessageComment, 7))) + assert.Empty(t, redispatchAsksIn(renderHoldingReply(MessageComment, 7)), "a holding reply written now asks nothing") assert.Empty(t, redispatchAsksIn(GuardAckBody)) assert.Empty(t, redispatchAsksIn(renderStillRunning(MessageComment, 3, "a1", 1, time.Now(), time.Now(), time.Time{}))) assert.Equal(t, "event 42", eventList([]int64{42})) @@ -235,6 +242,7 @@ func TestARetractionWaitsWhileTheAskIsStillOpen(t *testing.T) { require.NoError(t, ob.Flush(ctx)) holding := obIntent(t, ledger, holdingKey(1)) require.Equal(t, IntentSent, holding.State) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) clock.Advance(12 * time.Minute) _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) @@ -369,6 +377,7 @@ func TestImportedDoneRetractsAPostedAsk(t *testing.T) { require.NoError(t, ob.Flush(ctx)) holding := obIntent(t, ledger, holdingKey(1)) require.Equal(t, IntentSent, holding.State) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) clock.Advance(30 * time.Minute) _, err = ledger.Import(ctx, Reconciliation{Version: ReconciliationVersion, @@ -406,6 +415,7 @@ func TestOnlyAnAskIsRetracted(t *testing.T) { require.NoError(t, ob.Flush(ctx)) completion := obIntent(t, ledger, completionKey(l.AttemptID)) require.Equal(t, IntentSent, completion.State) + completion = obPostedByAnEarlierVersion(t, ledger, basecamp, completion) clock.Advance(time.Minute) _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) @@ -438,8 +448,8 @@ func TestAnAskThatWasNeverSentIsNotRetracted(t *testing.T) { require.NoError(t, obOutbox(t, ledger, basecamp).Flush(ctx)) sent := obIntent(t, ledger, completionKey(l.AttemptID)) require.Equal(t, IntentSent, sent.State) - assert.NotContains(t, sent.Body, "Needs a person", "the ask stood down at its claim instead") - assert.Contains(t, sent.Body, "Event 1: unknown, the worker did not report on it.", "what happened is still reported") + assert.NotContains(t, sent.Body, "Mention me again", "a person already decided it, so it suggests nothing") + assert.Contains(t, sent.Body, "I ran out of time and may not have finished this.", "what happened is still reported") assert.Empty(t, obRetractions(t, ledger), "nothing to retract: the ask was never posted") } @@ -462,7 +472,7 @@ func TestANoticeThatNeverAskedIsNotRetracted(t *testing.T) { require.NoError(t, ob.Flush(ctx)) quiet := obIntent(t, ledger, completionKey(first.AttemptID)) require.Equal(t, IntentSent, quiet.State) - require.NotContains(t, quiet.Body, "Needs a person") + require.NotContains(t, quiet.Body, "Mention me again") // It runs again, ends unknown again, and this time a person closes it. clock.Advance(time.Minute) @@ -617,6 +627,7 @@ func obSendingHoldingReply(t *testing.T) (*Ledger, *obClock, *Outbox, *fakeBasec basecamp.beforePost = nil holding := obIntent(t, ledger, holdingKey(1)) require.Equal(t, IntentSending, holding.State) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) return ledger, clock, ob, basecamp, holding } @@ -631,6 +642,7 @@ func TestAnAskIsRetractedOnce(t *testing.T) { ob := obOutbox(t, ledger, basecamp) require.NoError(t, ob.Flush(ctx)) holding := obIntent(t, ledger, holdingKey(1)) + holding = obPostedByAnEarlierVersion(t, ledger, basecamp, holding) clock.Advance(12 * time.Minute) _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) @@ -858,7 +870,7 @@ VALUES (7, 'holding_reply:event:1', 'holding_reply', 'sent', 1, 48699913 '2026-09-17T11:00:00.000000000Z', '2026-09-17T11:00:00.000000000Z', '2026-09-17T11:01:00.000000000Z', '2026-09-17T11:02:00.000000000Z', 555), (8, 'holding_reply:refused:event:1', 'holding_reply', 'sent', 1, 48699913, 'comment', 10304028989, ?2, '2026-09-17T11:30:00.000000000Z', '2026-09-17T11:30:00.000000000Z', '2026-09-17T11:31:00.000000000Z', '2026-09-17T11:32:00.000000000Z', 556)`, - renderHoldingReply(MessageComment, 1), refusedStartReplyAsItWentOut(1)) + legacyHoldingReply(MessageComment, 1), refusedStartReplyAsItWentOut(1)) require.NoError(t, err) }) basecamp := newFakeBasecamp(clock.Now) @@ -894,20 +906,26 @@ VALUES (7, 'holding_reply:event:1', 'holding_reply', 'sent', 1, 48699913 // The ask is read off the words that went out, and read exactly: a notice // about event 12 is not an ask for event 1. func TestAsksRedispatchReadsTheAskExactly(t *testing.T) { - assert.True(t, asksRedispatch(renderHoldingReply(MessageComment, 1), 1)) - assert.True(t, asksRedispatch(renderHoldingReply(MessageChatLine, 12), 12)) - assert.False(t, asksRedispatch(renderHoldingReply(MessageComment, 12), 1)) - assert.False(t, asksRedispatch(renderHoldingReply(MessageComment, 1), 12)) + // Notices an earlier version posted, as a ledger in use still holds them. + assert.True(t, asksRedispatch(legacyHoldingReply(MessageComment, 1), 1)) + assert.True(t, asksRedispatch(legacyHoldingReply(MessageChatLine, 12), 12)) + assert.False(t, asksRedispatch(legacyHoldingReply(MessageComment, 12), 1)) + assert.False(t, asksRedispatch(legacyHoldingReply(MessageComment, 1), 12)) assert.False(t, asksRedispatch(GuardAckBody, 1)) assert.False(t, asksRedispatch(renderStillRunning(MessageComment, 3, "a1", 1, time.Now(), time.Now(), time.Time{}), 1)) - asking := renderCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFailed, + asking := legacyCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFailed, Events: []SettledEvent{{EventID: 1, Outcome: OutcomeFailed}}}) assert.True(t, asksRedispatch(asking, 1)) - reporting := renderCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFinished, + reporting := legacyCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFinished, Events: []SettledEvent{{EventID: 1, Outcome: OutcomeSucceeded, Reported: true}}}) - require.NotEmpty(t, reporting) assert.False(t, asksRedispatch(reporting, 1), "succeeded with no reply reported asks for nothing") + + // Notices written now ask nothing, whatever they report. + assert.False(t, asksRedispatch(renderHoldingReply(MessageComment, 1), 1)) + assert.False(t, asksRedispatch(renderHoldingReply(MessageChatLine, 12), 12)) + assert.False(t, asksRedispatch(renderCompletion(MessageComment, Settlement{TaskID: 3, AttemptID: "a1", Stop: StopFailed, + Events: []SettledEvent{{EventID: 1, Outcome: OutcomeFailed}}}), 1)) } // An upgraded ledger's legacy refused-start reply, in both states it can be @@ -994,3 +1012,67 @@ VALUES (8, 'holding_reply:refused:event:1', 'holding_reply', ?1, 1, 48699913, 'c string(state), refusedStartReplyAsItWentOut(1)) require.NoError(t, err) } + +// Notices written now speak to the person in the thread and ask nothing of +// whoever runs the connector, so a person's decision on the record leaves them +// standing: nothing is posted to answer them, and the thread holds the notice +// and whatever the retried work says, not a note about a note. +func TestANoticeWrittenNowIsNeverRetracted(t *testing.T) { + t.Run("a holding reply, once the record is redispatched and routed", func(t *testing.T) { + ctx := context.Background() + ledger, clock := obLedger(t) + seenRecord(t, ledger, 1) + _, err := ledger.Admission().Commit(ctx, obNoRouteVerdict(1, 0, obCommentReply)) + require.NoError(t, err) + basecamp := newFakeBasecamp(clock.Now) + ob := obOutbox(t, ledger, basecamp) + require.NoError(t, ob.Flush(ctx)) + holding := obIntent(t, ledger, holdingKey(1)) + require.Equal(t, IntentSent, holding.State) + require.Contains(t, MessageText(holding.Body), "Once it's added to my projects, mention me again.") + + clock.Advance(12 * time.Minute) + _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) + require.NoError(t, err) + obRoutedNow(t, ledger, 1) + clock.Advance(RetractionWait) + require.NoError(t, ob.Flush(ctx)) + + assert.Empty(t, obRetractions(t, ledger)) + assert.Len(t, basecamp.at(holding.Destination), 1, "the notice alone; nothing answers it") + }) + + t.Run("a completion notice, once the record is redispatched or discarded", func(t *testing.T) { + for _, decide := range []string{"redispatch", "discard"} { + t.Run(decide, func(t *testing.T) { + ctx := context.Background() + ledger, clock := obLedger(t) + obAdmit(t, ledger, 1, "recording:10304028989") + l := obLaunch(t, ledger, 1) + clock.Advance(5 * time.Minute) + _, err := ledger.EndAttempt(ctx, AttemptEnd{AttemptID: l.AttemptID, Stop: StopShutdown}) + require.NoError(t, err) + basecamp := newFakeBasecamp(clock.Now) + ob := obOutbox(t, ledger, basecamp) + require.NoError(t, ob.Flush(ctx)) + completion := obIntent(t, ledger, completionKey(l.AttemptID)) + require.Equal(t, IntentSent, completion.State) + require.Equal(t, "I was interrupted and may not have finished this. If I didn't, mention me again to try again. "+ + "Ref 1 · attempt "+l.AttemptID+" · automatic notice from basecamp connect", MessageText(completion.Body)) + + clock.Advance(time.Minute) + if decide == "redispatch" { + _, err = ledger.Redispatch(ctx, 1, "jorge", []int64{adapterBucketID}) + } else { + _, err = ledger.Discard(ctx, 1, "jorge") + } + require.NoError(t, err) + clock.Advance(RetractionWait) + require.NoError(t, ob.Flush(ctx)) + + assert.Empty(t, obRetractions(t, ledger)) + assert.Len(t, basecamp.at(completion.Destination), 1, "the notice alone; nothing answers it") + }) + } + }) +} diff --git a/internal/connector/setup/checks.go b/internal/connector/setup/checks.go index 2c5a663cf..77567d6ab 100644 --- a/internal/connector/setup/checks.go +++ b/internal/connector/setup/checks.go @@ -211,6 +211,11 @@ type Trust struct { // that profile's own credential read back, which proves who they are. Operator Person OperatorProfile string + // OperatorIsOwner says Operator is the person the agent works for, as + // Basecamp named them in the agent's own profile. Basecamp's word proves + // who they are, as a profile's own credential does, so they are not read + // again: a new agent may share no project with them yet. + OperatorIsOwner bool // Allowlist is every allowlisted Person id the file will hold. Allowlist []int64 // Recorded is the trust connect.json already holds for this same agent @@ -244,7 +249,14 @@ func VerifyTrust(ctx context.Context, people Reader, t Trust, agentID int64) []C slices.Sort(ids) ids = slices.Compact(ids) checks := make([]Check, 0, 1+len(ids)) - checks = append(checks, verifyPerson(ctx, people, "Operator", t.Operator, agentID, t.OperatorProfile, t.Recorded.OperatorID == t.Operator.ID && t.Operator.ID > 0)) + proof := "" + switch { + case t.OperatorProfile != "": + proof = fmt.Sprintf("the identity of profile %q", t.OperatorProfile) + case t.OperatorIsOwner: + proof = "the agent's owner" + } + checks = append(checks, verifyPerson(ctx, people, "Operator", t.Operator, agentID, proof, t.Recorded.OperatorID == t.Operator.ID && t.Operator.ID > 0)) for _, id := range ids { name := fmt.Sprintf("Allowlist %d", id) checks = append(checks, verifyPerson(ctx, people, name, Person{ID: id}, agentID, "", slices.Contains(t.Recorded.AllowlistIDs, id))) @@ -252,7 +264,9 @@ func VerifyTrust(ctx context.Context, people Reader, t Trust, agentID int64) []C return checks } -func verifyPerson(ctx context.Context, r Reader, name string, p Person, agentID int64, fromProfile string, recorded bool) Check { +// verifyPerson checks one trusted person. proof, when set, says what already +// proves who they are, and they are not read again. +func verifyPerson(ctx context.Context, r Reader, name string, p Person, agentID int64, proof string, recorded bool) Check { c := Check{Name: name} switch { case p.ID <= 0: @@ -263,8 +277,8 @@ func verifyPerson(ctx context.Context, r Reader, name string, p Person, agentID return c } source := "" - if fromProfile != "" { - source = fmt.Sprintf(", the identity of profile %q", fromProfile) + if proof != "" { + source = ", " + proof } else { read, err := r.Person(ctx, p.ID) switch { diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index a274d8831..4519c28d0 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -192,8 +192,10 @@ Explain the modes this way when you ask: In every mode, assigning work to the agent counts only from the operator, and agents never authorize anything, the agent itself included. -**The operator** is the person the agent takes instructions from. Name them by -their own CLI profile with `--operator-profile ''`: setup reads who +**The operator** is the person the agent takes instructions from. A personal +agent's operator is its owner, whom Basecamp names in the agent's own profile: +for one, pass no operator flag and setup takes the owner. For any other agent, +name them by their own CLI profile with `--operator-profile ''`: setup reads who that profile is through its own login, which proves it. `--operator ` needs the agent to read that person, which Basecamp refuses to an Agent identity today, so prefer `--operator-profile` always. The operator's @@ -273,8 +275,10 @@ not looked at leaves trust and projects in place that nobody mentioned. Then confirm the identity (`basecamp me`) with the person before going on. -**2. Operator.** Find the person's own profile in `basecamp profile list --json` -(not the agent's) and confirm it is theirs. Use `--operator-profile`. +**2. Operator.** For a personal agent (one that works for a person), pass no +operator flag: setup takes its owner. Otherwise find the person's own profile in +`basecamp profile list --json` (not the agent's) and confirm it is theirs. Use +`--operator-profile`. **3. Trust mode.** Explain the three modes in a sentence each and ask. Default to `operator`. For `allowlist`, get each person's Person id (for example @@ -434,11 +438,17 @@ that record or that step. proof that it runs), the hold, the feed position (held or not, never the position), gaps, queues, live tasks and their workers, lifecycle messages waiting for a person, held records, the last dispatches. - Read-only and safe while the connector runs. It shows no content. + Read-only and safe while the connector runs. It shows no content. When the + worker failed to start twice in a row, the connector stops taking new work + and status opens with "Not taking work:" and the reason. Explain the + reason. New work waits and nothing is lost. The connector takes work again + once the worker starts cleanly, or when it restarts. - `basecamp connect doctor -P ''`: token, identity, ticket mint, feed - poll, the ledger, the worker binary, and a handshake with the agent's MCP - server. It writes nothing to the ledger and posts nothing to Basecamp, - though it may renew the profile's credential as any command does. + poll, the ledger, the worker (started as the connector would start it, and + asked whether it knows the connector's flags and is logged in, with no model + call), and a handshake with the agent's MCP server. It writes nothing to the + ledger and posts nothing to Basecamp, though it may renew the profile's + credential as any command does. - `basecamp connect redispatch -P '' `: authorize a record to run again or for the first time. Accepted for an unknown or failed outcome (one whose task is still running waits for that task to end), a blocked