From 03cf612a4b654a0e2e7eba4f3b4fb914bade3b15 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 11:46:54 -0700 Subject: [PATCH 1/8] Answer an agent's Pings from one connector only Every connector for an agent sees all of its Pings: a Ping is in no project, so serving projects can't divide them the way it divides the rest, and each connector that admitted a line answered it. With several connections per agent that is every Ping answered once per computer. Only a connector started with --pings now hears Pings, and anything else outside a project; one started with --no-pings hears only its projects. Without either, the agent's first connection (its credential is the agent client's own secret) hears them, a connection added after it (a bcac_ secret) doesn't, and a person's login, such as a bot user's, does as before. A run given --project still hears none, and --pings with --project is refused, since its feed brings no Ping. The first start saves the answer beside the profile's ledger (pings.json), and restarts keep it until a flag changes it, so a profile that answered Pings keeps answering after a reconnect turns its credential into an added connection. A shadow reads the profile's choice and saves none. The running connector says beside its lock whether it hears Pings; a connector that hears them warns when another for the same agent on this computer does too, and status and doctor say which this profile's does and why. Admission discards a confirmed Ping's line as pings_not_heard when the run doesn't hear them, and the handoff hands off a Ping's request only when it does. --- README.md | 21 +- docs/agent-connections-rollout.md | 38 +++ internal/commands/connect.go | 21 +- internal/commands/connect_doctor.go | 48 ++- internal/commands/connect_operator.go | 67 +++- internal/commands/connect_pings.go | 268 +++++++++++++++ .../commands/connect_pings_resolve_test.go | 105 ++++++ internal/commands/connect_pings_test.go | 319 ++++++++++++++++++ internal/commands/connect_run.go | 55 ++- internal/commands/connect_run_log_test.go | 2 +- internal/commands/connect_setup_guided.go | 52 ++- internal/commands/connect_setup_test.go | 9 +- internal/connector/admission/doc.go | 7 + internal/connector/admission/fakes_test.go | 5 +- internal/connector/admission/matrix.go | 4 + .../connector/admission/pings_heard_test.go | 134 ++++++++ internal/connector/admission/policy.go | 12 + internal/connector/admission/verdict.go | 6 + internal/connector/handoff.go | 6 +- internal/connector/handoff_test.go | 22 +- internal/connector/lock.go | 24 +- internal/connector/pings.go | 78 +++++ internal/connector/pings_test.go | 124 +++++++ internal/connector/setup/private_state.go | 83 +++++ skills/basecamp-connect/SKILL.md | 43 ++- 25 files changed, 1515 insertions(+), 38 deletions(-) create mode 100644 internal/commands/connect_pings.go create mode 100644 internal/commands/connect_pings_resolve_test.go create mode 100644 internal/commands/connect_pings_test.go create mode 100644 internal/connector/admission/pings_heard_test.go create mode 100644 internal/connector/pings.go create mode 100644 internal/connector/pings_test.go diff --git a/README.md b/README.md index f4a514579..6e42f9513 100644 --- a/README.md +++ b/README.md @@ -280,7 +280,26 @@ Basecamp's HTTP handshake. Do not reconnect just to upgrade the CLI. Each profile has its own projects, ledger, cursor, hold and local lock. When two connections serve the same project, a request both admit can be answered twice, so serve each project from one. Use a distinct `--device-name` for each -profile on the same computer. Where Basecamp lists several connections, you +profile on the same computer. + +Only one computer should answer an agent's Pings. A Ping is in no project, so +every connector for the agent sees all of them, and each one that heard them +would answer. A connector started with `--pings` hears Pings, and anything +else outside a project; one started with `--no-pings` hears only its projects. +Without either, the agent's first connection hears them, a connection added +after it doesn't, and a bot user's login does, as it always has. A run given +`--project` hears none. The first start saves the choice for the profile, and +restarts keep it until you pass the other flag: + +```bash +basecamp connect -P desktop --pings # this computer answers Pings +basecamp connect -P laptop --no-pings # this one leaves them to the desktop +``` + +A connector that hears Pings warns when another one on the same computer hears +the same agent's. It can't see other computers, so starting just one there is +yours to arrange. `basecamp connect status` and `basecamp connect doctor` say +whether a profile's connector answers them, and why. Where Basecamp lists several connections, you can rename them and disconnect one without using Disconnect all. Reconnecting a profile on an add-capable server leaves its old connection live until you disconnect it; older credential files can remain live too. Do not delete them diff --git a/docs/agent-connections-rollout.md b/docs/agent-connections-rollout.md index d81275f47..d29e10693 100644 --- a/docs/agent-connections-rollout.md +++ b/docs/agent-connections-rollout.md @@ -39,6 +39,44 @@ the approval page; do not reconnect merely to upgrade the CLI. A second run of the same profile is refused. Deliberate project overlap is allowed; requests admitted by both can be answered twice. There is no lease or cross-profile claim. +6. Decide which one computer answers the agent's Pings (see below), and start + every other connector for the agent with `--no-pings` once. + +## Only one computer answers Pings + +A Ping is in no project, so serving projects can't divide Pings between +connections: every connector for the agent sees all of them, and each one +that heard them would answer. Only one computer should answer Pings, and the +person running the agent arranges it; the CLI picks a default, keeps the +choice, and warns about a second it can see on the same computer. + +- `--pings` hears Pings and anything else outside a project. `--no-pings` + hears only the connector's projects. +- With neither, the agent's first connection (its credential is the agent + client's own secret) hears them, and a connection added after it (its + secret starts `bcac_`) doesn't. A profile that holds a person's login, a + bot user's, hears them as every connector did before this build. A run + given `--project` hears none, as before, and `--pings` with `--project` is + refused. +- The first start on this build saves the answer in the profile's state + directory (`pings.json`, beside the ledger), and every restart keeps it + until `--pings` or `--no-pings` changes it. A profile that heard Pings keeps + hearing them after a reconnect turns its credential into an added + connection. An unreadable `pings.json` refuses the start until a flag + chooses again. +- `basecamp connect status` and `basecamp connect doctor` say whether the + profile's connector answers Pings and why. A connector that hears them + warns when another for the same agent on this computer does too, an older + build's included (it hears them unless started with `--project`). + Connectors on other computers can't be seen. +- Upgrading changes nothing for one connection per agent: it is the first + connection, or a bot user's login, and it still hears Pings. Where an agent + already runs several connectors that hear Pings (a bot user on three + computers, say), choose one, and restart the others with `--no-pings`. + +Downgrading drops the choice: older builds ignore `pings.json` and every +connector they run hears Pings unless given `--project`. The file stays, and +the next start on this build reads it again. State is under `$XDG_STATE_HOME/basecamp/connect` and `connect-shadow` (default `~/.local/state/basecamp`). New profile directories canonicalize numeric account diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 607598d74..7a5e6de5b 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -50,9 +50,9 @@ the agent. Connect the agent to a profile first (basecamp auth agent connect -P ), then run setup on that profile: it records who may drive the agent, maps which Basecamp projects it serves, and checks the connector is -ready. Pings need no setup: an operator's ping is handed off too. Show prints what setup recorded. Then run the connector on it: +ready. Show prints what setup recorded. Then run the connector on it: - basecamp connect -P [--project ]... [--shadow] + basecamp connect -P [--project ]... [--pings | --no-pings] [--shadow] It runs in the foreground until interrupted. Stdout is one JSON object per line: events seen, admission's verdicts, and a "type":"request" line for each @@ -69,6 +69,18 @@ a new approval replaces the existing connection. Each profile serves its own projects and has its own hold. When two connections serve the same project, a request both admit can be answered twice. +Pings. Only one computer should answer an agent's Pings: every connector for +the agent sees all of them, since a Ping is in no project. A connector started +with --pings hears Pings, and anything else outside a project; one started +with --no-pings hears only projects. Without either, the agent's first +connection hears them, a connection added after it doesn't, and a person's +login (a bot user's) does, as before. A run given --project hears none. The +first start saves the choice for the profile, and every restart keeps it until +--pings or --no-pings changes it. A connector that hears Pings warns when +another one on this computer hears the same agent's; ones on other computers +can't be seen, so starting just one is yours to arrange. basecamp connect +status and doctor say which this profile's does. + basecamp connect status what it heard and holds basecamp connect doctor what it needs to run basecamp connect discard close a record without running it @@ -78,6 +90,7 @@ same project, a request both admit can be answered twice. basecamp connect import apply a cutover reconciliation file`, Example: ` basecamp connect setup -P agent --operator-profile me --serve 12345 basecamp connect -P agent + basecamp connect -P laptop --no-pings basecamp connect -P agent --project 12345 --shadow`, Args: cobra.NoArgs, Annotations: map[string]string{ @@ -401,7 +414,9 @@ The list is this profile's own. When two connections of one agent serve the same project, a request both admit there can be answered twice, so serve each project from one of them. -Pings. A Ping is in no project, so it is never served and needs no setup. +Pings. A Ping is in no project, so it is never served and needs no setup, +and only the connector started to hear Pings answers them (see basecamp +connect --help: only one computer should answer Pings). An operator (the operator, or a person passed with --allow) gives the agent work by Ping in every trust mode: every line of theirs in a direct Ping with the agent is a request, and in a group Ping a line that @mentions it. diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 29c70c4f2..4a8e64897 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -8,6 +8,7 @@ import ( "path/filepath" "runtime" "strconv" + "strings" "time" "github.com/spf13/cobra" @@ -25,9 +26,10 @@ func newConnectDoctorCmd() *cobra.Command { Use: "doctor", 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, and the ledger (its gaps, -open losses, hold, and lifecycle messages an older connector left waiting for -a person). +identity, the stream ticket mint, the account feed, whether it hears the +agent's Pings (and whether another connector on this computer does too), and +the ledger (its gaps, open losses, hold, and lifecycle messages an older +connector left waiting for a person). It writes nothing to the connector's ledger and posts nothing to Basecamp. Renewing the profile's own credential, which every command does when its token @@ -65,6 +67,7 @@ func runConnectDoctor(cmd *cobra.Command, _ []string) error { feedCheck(ctx, agent.client.ForAccount(agent.account)), ) } + checks = append(checks, pingsCheck(ctx, p)) checks = append(checks, ledgerChecks(ctx, p)...) result := summarizeChecks(asDoctorChecks(checks)) @@ -171,3 +174,42 @@ func connectUnsupportedOSCheck(goos string) setup.Check { Message: fmt.Sprintf("The connector does not run on %s: %s", goos, connectSupportedOSReason), Hint: "Run the connector on Linux or macOS; the rest of the CLI runs here."} } + +// pingsCheck says whether the profile's connector, started with neither +// --pings, --no-pings nor --project, hears the agent's Pings, and warns when +// another connector on this computer answers them too. Only one should; the +// ones on other computers can't be seen from here. +func pingsCheck(ctx context.Context, p connectProfile) setup.Check { + c := setup.Check{Name: "Pings"} + pings, ok, err := connectProfilePings(ctx, p.app, p.name, p.file) + switch { + case err != nil: + c.Status, c.Message = setup.StatusFail, "Whether it hears the agent's Pings was saved, and can't be read: "+errorMessage(err) + c.Hint = "Choose again: basecamp connect -P " + richtext.ShellQuote(p.name) + " --pings, or --no-pings to leave them to another connector." + return c + case !ok: + c.Status, c.Message = setup.StatusSkip, "Whether it hears the agent's Pings can't be told without the profile's credential" + return c + case !pings.Hears: + c.Status = setup.StatusPass + c.Message = "Doesn't hear the agent's Pings: " + pings.Describe() + ". Only one connector should answer them; start this one with --pings if it's that one" + return c + } + peers := connectPingsPeers(p.name, p.file) + if len(peers) == 0 { + c.Status, c.Message = setup.StatusPass, "Hears the agent's Pings: "+pings.Describe() + return c + } + others := make([]string, 0, len(peers)) + hint := "Only one connector should answer Pings. Start one of them with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" + for _, peer := range peers { + others = append(others, fmt.Sprintf("%s (pid %d)", peer.Words(), peer.PID)) + if peer.Older { + hint = "Only one connector should answer Pings, and an older build can't be told: upgrade it, or start this one with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" + } + } + c.Status = setup.StatusWarn + c.Message = fmt.Sprintf("Hears the agent's Pings (%s), and so does %s on this computer: each Ping would be answered twice", pings.Describe(), strings.Join(others, ", ")) + c.Hint = hint + return c +} diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index 0aa726d26..511efa11c 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -175,7 +175,9 @@ func newConnectStatusCmd() *cobra.Command { feed position (whether one is held, never the position), the last poll-served id, gaps and losses, queue depths, live tasks, lifecycle messages waiting for a person, held records, and the last 20 dispatches with -their outcomes. +their outcomes. It also says whether the profile's connector answers the +agent's Pings and why, and which other connectors on this computer answer +them too: only one should. It reads the ledger read-only and takes no lock, so it works while the connector runs. It shows no content, no feed position and no token; a held @@ -196,9 +198,25 @@ type connectStatusReport struct { Profile string `json:"profile"` Shadow bool `json:"shadow"` LockHolder *connectLockHolder `json:"lock_holder,omitempty"` + Pings connectStatusPings `json:"pings"` Status connector.Status `json:"status"` } +// connectStatusPings is whether the profile's connector hears the agent's +// Pings: the choice its starts keep, and the other connectors on this +// computer that hear them too. +type connectStatusPings struct { + // Saved is the choice the profile's last start kept; nil before any + // start has. + Saved *connector.PingsChoice `json:"saved,omitempty"` + // Error is why the saved choice can't be read, when it can't. + Error string `json:"error,omitempty"` + // Others are the connectors running on this computer for the same agent, + // under other profiles, that hear its Pings, or may. Never read for a + // shadow, which answers nothing. + Others []connectPingsPeer `json:"others,omitempty"` +} + // connectLockHolder is what a connector wrote beside its instance lock. It is // diagnostic, not an answer: the metadata is written best effort after the // lock is taken, it stays behind after a crash, and a pid may since belong to @@ -210,6 +228,9 @@ type connectLockHolder struct { // platform can give one. Present says a process has that pid, not that it // is that connector. PIDStatus string `json:"pid_status"` + // Pings is whether that connector hears the agent's Pings, as it wrote; + // absent when an older build wrote the file. + Pings *bool `json:"pings,omitempty"` } func runConnectStatus(cmd *cobra.Command, shadow bool) error { @@ -242,7 +263,15 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { } report := connectStatusReport{Profile: p.name, Shadow: shadow, Status: status} if holder, ok := connector.InstanceHolder(dir, p.file.AccountID, p.file.Agent.PersonID); ok { - report.LockHolder = &connectLockHolder{PID: holder.PID, StartedAt: holder.StartedAt, PIDStatus: processPresence(holder.PID)} + report.LockHolder = &connectLockHolder{PID: holder.PID, StartedAt: holder.StartedAt, PIDStatus: processPresence(holder.PID), Pings: holder.Pings} + } + if saved, ok, err := connector.LoadPingsChoice(dir); err != nil { + report.Pings.Error = errorMessage(err) + } else if ok { + report.Pings.Saved = &saved + } + if !shadow { + report.Pings.Others = connectPingsPeers(p.name, p.file) } if p.app.Output.EffectiveFormat() == output.FormatStyled { renderConnectStatus(cmd.OutOrStdout(), report) @@ -299,6 +328,7 @@ func renderConnectStatus(w io.Writer, r connectStatusReport) { } fmt.Fprintln(w) } + renderConnectStatusPings(w, r) if s.Hold != nil { fmt.Fprintf(w, " Hold set by %s at %s (%s, generation %d): nothing dispatches or posts until release\n", clean(s.Hold.HeldBy), stamp(s.Hold.HeldAt), clean(s.Hold.Cause), s.Hold.Generation) @@ -805,3 +835,36 @@ func readReconciliation(path string) ([]byte, error) { } return data, nil } + +// renderConnectStatusPings is status's Pings lines: whether the profile's +// connector hears them and why, what the running one does when that differs, +// and who else on this computer hears them. +func renderConnectStatusPings(w io.Writer, r connectStatusReport) { + p := r.Pings + switch { + case p.Error != "": + fmt.Fprintf(w, " Pings the saved choice can't be read (%s); the next start refuses until given --pings or --no-pings\n", richtext.SanitizeSingleLine(p.Error)) + case p.Saved == nil: + fmt.Fprintf(w, " Pings not chosen yet: the next start without --project chooses (an agent's first connection hears them, a connection added after it doesn't)\n") + default: + fmt.Fprintf(w, " Pings %s\n", connectPingsWords(connectPings{Hears: p.Saved.Hears, Because: p.Saved.Because})) + } + if h := r.LockHolder; h != nil && h.Pings != nil && h.PIDStatus != pidAbsent && (p.Saved == nil || *h.Pings != p.Saved.Hears) { + heard := "hears none" + if *h.Pings { + heard = "hears them" + } + fmt.Fprintf(w, " the connector running now %s\n", heard) + } + for _, other := range p.Others { + fmt.Fprintf(w, " Pings also heard by %s (pid %d) on this computer: only one connector should answer them\n", other.Words(), other.PID) + } +} + +// connectPingsWords is whether a connector hears Pings, and why, for a line. +func connectPingsWords(p connectPings) string { + if p.Hears { + return "heard: " + p.Describe() + } + return "not heard: " + p.Describe() + "; start with --pings to answer them from here" +} diff --git a/internal/commands/connect_pings.go b/internal/commands/connect_pings.go new file mode 100644 index 000000000..faeb45aa2 --- /dev/null +++ b/internal/commands/connect_pings.go @@ -0,0 +1,268 @@ +package commands + +import ( + "context" + "log/slog" + "os" + "path/filepath" + "slices" + "strings" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" +) + +// Which connector answers an agent's Pings. +// +// Every connector for an agent sees all of the agent's Pings: a Ping is in no +// project, so serving projects can't divide them up the way it divides the +// rest. Each connector that admitted a Ping's line would answer it, so only +// one should, and that one is the connector started with --pings. It hears +// Pings and anything else outside a project; every other connector hears its +// projects alone. +// +// Running exactly one is the person's to arrange: the CLI can't see the +// connectors on other computers, and nothing in Basecamp chooses between +// them. What it can do is pick the likely one by default, keep the choice +// across restarts, and say so when it sees a second on this computer. + +// The reasons a connector hears Pings or doesn't, as they are saved and shown +// in status --json. +const ( + // pingsBecauseChosen: --pings or --no-pings, on this start or an earlier + // one. + pingsBecauseChosen = "chosen" + // pingsBecauseFirstConnection: the profile holds the agent's first + // connection, whose secret is the agent client's own. + pingsBecauseFirstConnection = "first_connection" + // pingsBecauseAddedConnection: the profile holds a connection added after + // the first, whose secret is its own. + pingsBecauseAddedConnection = "added_connection" + // pingsBecauseNotAnAgentConnection: the profile holds a person's login, + // such as a bot user's, which hears Pings as every connector did before. + pingsBecauseNotAnAgentConnection = "not_an_agent_connection" + // pingsBecauseProject: --project narrowed the run to projects. Never + // saved: it is this run's, not the profile's. + pingsBecauseProject = "project" +) + +// agentConnectionSecretPrefix starts the secret of every connection Basecamp +// adds to an agent after its first (Agent::Installation::SECRET_PREFIX in +// bc3). The first connection authenticates with the agent client's own +// secret, which doesn't. +const agentConnectionSecretPrefix = "bcac_" + +// connectPings is whether a connector hears the agent's Pings, and why. +type connectPings struct { + Hears bool `json:"hears"` + Because string `json:"because"` +} + +// resolveConnectPings decides whether a start hears Pings, and whether it +// saves that for the profile. In order: +// +// - --pings or --no-pings, saved. +// - --project: a narrowed run's feed brings no Ping, so it hears none, as +// before. Nothing is saved; the profile's choice is the next un-narrowed +// run's. +// - the profile's saved choice. +// - the default for its credential, saved, so a restart keeps it even +// after the credential changes: a profile reconnected where Basecamp adds +// connections holds an added connection afterwards, and the person who +// had it answering Pings still does. +func resolveConnectPings(flag *bool, saved *connector.PingsChoice, narrowed bool, oauthType, secret string) (connectPings, bool) { + switch { + case flag != nil: + return connectPings{Hears: *flag, Because: pingsBecauseChosen}, true + case narrowed: + return connectPings{Hears: false, Because: pingsBecauseProject}, false + case saved != nil: + return connectPings{Hears: saved.Hears, Because: saved.Because}, false + } + return connectPingsDefault(oauthType, secret), true +} + +// connectPingsDefault is whether a profile that has chosen nothing hears +// Pings: an agent's first connection does, a connection added after it +// doesn't, and a person's login does, as before. +func connectPingsDefault(oauthType, secret string) connectPings { + switch { + case oauthType != "agent": + return connectPings{Hears: true, Because: pingsBecauseNotAnAgentConnection} + case strings.HasPrefix(secret, agentConnectionSecretPrefix): + return connectPings{Hears: false, Because: pingsBecauseAddedConnection} + } + return connectPings{Hears: true, Because: pingsBecauseFirstConnection} +} + +// Describe is why, in a person's words. +func (p connectPings) Describe() string { + switch p.Because { + case pingsBecauseChosen: + if p.Hears { + return "chosen with --pings" + } + return "chosen with --no-pings" + case pingsBecauseFirstConnection: + return "the agent's first connection" + case pingsBecauseAddedConnection: + return "a connection added after the agent's first" + case pingsBecauseNotAnAgentConnection: + return "a person's login, not an agent connection" + case pingsBecauseProject: + return "--project narrows this run to projects" + } + return "as saved for this profile" +} + +// connectPingsPeer is another connector on this computer, for the same agent +// in the same account, that hears its Pings. +type connectPingsPeer struct { + // Profile is its profile, or empty for a connector an older build runs on + // state from before profiles had their own. + Profile string `json:"profile"` + PID int `json:"pid"` + // Older says an older build started it, which doesn't say whether it + // hears Pings. It does unless it was given --project. + Older bool `json:"older,omitempty"` +} + +// Words is the peer for a log line or a check. +func (p connectPingsPeer) Words() string { + who := "an older connector" + if p.Profile != "" { + who = `profile "` + richtext.SanitizeSingleLine(p.Profile) + `"` + } + return who +} + +// connectPingsPeers lists the connectors running on this computer for the +// agent connect.json names, under profiles other than this one, that hear +// its Pings or may. It reads what each wrote beside its instance lock and +// takes no lock: a look must not make a starting connector find its own +// lock held. A shadow answers nothing, so only the connectors' own state is +// read. +func connectPingsPeers(profile string, file setup.File) []connectPingsPeer { + stateHome, err := connectStateHome() + if err != nil { + return nil + } + root := filepath.Join(stateHome, "basecamp", "connect") + entries, err := os.ReadDir(root) + if err != nil { + return nil + } + prefix := strings.TrimSuffix(connector.StateDirName(file.AccountID, file.Agent.PersonID, ""), "-") + legacy := connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID) + own := connector.StateDirName(file.AccountID, file.Agent.PersonID, profile) + var peers []connectPingsPeer + for _, entry := range entries { + name := entry.Name() + other, isProfile := strings.CutPrefix(name, prefix+"-") + if name == own || !entry.IsDir() || (name != legacy && (!isProfile || !isValidProfileName(other))) { + continue + } + holder, ok := connector.InstanceHolder(filepath.Join(root, name), file.AccountID, file.Agent.PersonID) + if !ok || !holderStillRuns(holder) || (holder.Pings != nil && !*holder.Pings) { + continue + } + peer := connectPingsPeer{PID: holder.PID, Older: holder.Pings == nil} + if name != legacy { + peer.Profile = other + } + peers = append(peers, peer) + } + slices.SortFunc(peers, func(a, b connectPingsPeer) int { return strings.Compare(a.Profile, b.Profile) }) + return peers +} + +// connectProfilePings is whether a start of the profile with neither flag +// nor --project would hear Pings: its saved choice, else the default for the +// credential it holds. It reads only this computer. It reports false when +// the credential can't be read, and a saved choice that can't be read as the +// error the start would refuse with. +func connectProfilePings(ctx context.Context, app *appctx.App, profile string, file setup.File) (connectPings, bool, error) { + dir, err := connectStatePath(profile, file, false) + if err != nil { + return connectPings{}, false, err + } + saved, ok, err := connector.LoadPingsChoice(dir) + switch { + case err != nil: + return connectPings{}, false, err + case ok: + return connectPings{Hears: saved.Hears, Because: saved.Because}, true, nil + } + creds, err := app.Auth.GetStore().LoadContext(ctx, app.Auth.CredentialKey()) + if err != nil { + return connectPings{}, false, nil //nolint:nilerr // the token checks report an unreadable credential + } + return connectPingsDefault(creds.OAuthType, creds.ClientSecret), true, nil +} + +// connectPingsFlag is the start's --pings or --no-pings, nil when neither was +// given. It refuses both at once, and --pings on a run given --project, +// whose feed brings no Ping to hear. +func connectPingsFlag(f *connectRunFlags, narrowed bool, profile string) (*bool, error) { + switch { + case f.pings && f.noPings: + return nil, output.ErrUsage("--pings and --no-pings are one choice: pass one of them") + case f.pings && narrowed: + return nil, output.ErrUsageHint("--pings can't be used with --project: a run given --project hears no Pings", + "To answer Pings here, serve this connector's projects with basecamp connect setup -P "+richtext.ShellQuote(profile)+" --serve , and start it without --project.") + case f.pings: + hears := true + return &hears, nil + case f.noPings: + hears := false + return &hears, nil + } + return nil, nil +} + +// connectRunPings decides whether this start hears Pings, from its flag, the +// profile's saved choice and its credential, and whether it saves the answer. +// The choice is the profile's, kept in its connector's state directory: a +// shadow reads it, so it admits what the connector it watches beside would, +// and never saves one. A saved choice that cannot be read refuses the start +// unless the start doesn't need it: guessing either way answers Pings twice +// or not at all. +func connectRunPings(profile string, file setup.File, shadow bool, flag *bool, narrowed bool, creds *auth.Credentials) (connectPings, bool, error) { + dir, err := connectStatePath(profile, file, false) + if err != nil { + return connectPings{}, false, output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) + } + var saved *connector.PingsChoice + choice, ok, err := connector.LoadPingsChoice(dir) + switch { + case err != nil && flag == nil && !narrowed: + return connectPings{}, false, output.ErrUsageHint("Whether this connector hears Pings was saved, and can't be read: "+setup.ErrorText(err), + "Choose again: basecamp connect -P "+richtext.ShellQuote(profile)+" --pings, or --no-pings to leave them to another connector.") + case ok: + saved = &choice + } + pings, save := resolveConnectPings(flag, saved, narrowed, creds.OAuthType, creds.ClientSecret) + return pings, save && !shadow, nil +} + +// warnConnectPingsPeers says, as a connector that hears Pings starts, that +// another connector on this computer hears the same agent's Pings, so each is +// answered twice. One that hears none says what it leaves to whom. +func warnConnectPingsPeers(logger *slog.Logger, pings connectPings, profile string, file setup.File) { + if !pings.Hears { + logger.Info("connector: hearing no Pings, and leaving them to the one connector that answers them; start with --pings to answer them here", + "pings_because", pings.Describe()) + return + } + for _, peer := range connectPingsPeers(profile, file) { + msg := "connector: another connector on this computer hears this agent's Pings too, so each is answered twice; only one connector should answer Pings: restart one of them with --no-pings" + if peer.Older { + msg = "connector: an older connector on this computer may hear this agent's Pings too, so each would be answered twice; only one connector should answer Pings: upgrade it, or restart this one with --no-pings" + } + logger.Warn(msg, "profile", richtext.SanitizeSingleLine(peer.Profile), "pid", peer.PID) + } +} diff --git a/internal/commands/connect_pings_resolve_test.go b/internal/commands/connect_pings_resolve_test.go new file mode 100644 index 000000000..4628cac69 --- /dev/null +++ b/internal/commands/connect_pings_resolve_test.go @@ -0,0 +1,105 @@ +package commands + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "github.com/basecamp/basecamp-cli/internal/connector" +) + +// Which connector hears an agent's Pings. Only one should, and the CLI +// cannot see the others on other computers, so the defaults pick the one a +// person most likely means: the agent's first connection, whose credential +// is the agent's own client secret, and not a connection added after it, +// whose secret Basecamp starts with bcac_. A profile that is no agent +// connection — a person's login, such as a bot user's — hears them as it +// always has. Either flag overrides any of it, and a profile's first start +// saves what it found, so a restart keeps it. +func TestWhichConnectorHearsPings(t *testing.T) { + yes, no := true, false + added := "bcac_" + "1111111111111111111111111111111111111111111" + saved := func(hears bool, because string) *connector.PingsChoice { + return &connector.PingsChoice{Hears: hears, Because: because, At: time.Date(2026, 10, 21, 12, 0, 0, 0, time.UTC)} + } + for name, tc := range map[string]struct { + flag *bool + saved *connector.PingsChoice + narrowed bool + oauthType string + secret string + want connectPings + save bool + }{ + "the agent's first connection": { + oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: true, Because: pingsBecauseFirstConnection}, save: true, + }, + "a connection added after it": { + oauthType: "agent", secret: added, + want: connectPings{Hears: false, Because: pingsBecauseAddedConnection}, save: true, + }, + "a person's login": { + oauthType: "bc5", secret: "", + want: connectPings{Hears: true, Because: pingsBecauseNotAnAgentConnection}, save: true, + }, + "--pings on an added connection": { + flag: &yes, oauthType: "agent", secret: added, + want: connectPings{Hears: true, Because: pingsBecauseChosen}, save: true, + }, + "--no-pings on a first connection": { + flag: &no, oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: false, Because: pingsBecauseChosen}, save: true, + }, + "--no-pings on a person's login": { + flag: &no, oauthType: "bc5", + want: connectPings{Hears: false, Because: pingsBecauseChosen}, save: true, + }, + "a restart keeps what the first start found": { + saved: saved(false, pingsBecauseAddedConnection), oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: false, Because: pingsBecauseAddedConnection}, + }, + "a restart keeps what a flag chose": { + saved: saved(true, pingsBecauseChosen), oauthType: "agent", secret: added, + want: connectPings{Hears: true, Because: pingsBecauseChosen}, + }, + "a flag replaces what was saved": { + flag: &no, saved: saved(true, pingsBecauseFirstConnection), oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: false, Because: pingsBecauseChosen}, save: true, + }, + "a run given --project, as before": { + narrowed: true, oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: false, Because: pingsBecauseProject}, + }, + "a run given --project leaves the saved choice for the next": { + narrowed: true, saved: saved(true, pingsBecauseChosen), oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: false, Because: pingsBecauseProject}, + }, + "--no-pings with --project is still a choice": { + flag: &no, narrowed: true, oauthType: "agent", secret: "not-a-real-secret", + want: connectPings{Hears: false, Because: pingsBecauseChosen}, save: true, + }, + } { + t.Run(name, func(t *testing.T) { + got, save := resolveConnectPings(tc.flag, tc.saved, tc.narrowed, tc.oauthType, tc.secret) + assert.Equal(t, tc.want, got) + assert.Equal(t, tc.save, save, "whether this start saves its choice") + }) + } +} + +// Every reason has words a person reads, in status, doctor and the log. +func TestEveryPingsReasonSaysSomething(t *testing.T) { + for _, because := range []string{pingsBecauseChosen, pingsBecauseFirstConnection, pingsBecauseAddedConnection, pingsBecauseNotAnAgentConnection, pingsBecauseProject} { + for _, hears := range []bool{true, false} { + words := connectPings{Hears: hears, Because: because}.Describe() + assert.NotEmpty(t, words, because) + assert.NotEqual(t, because, words, "the code is not the explanation") + assert.NotContains(t, words, "_", "nor is any other") + } + } + assert.Equal(t, "chosen with --pings", connectPings{Hears: true, Because: pingsBecauseChosen}.Describe()) + assert.Equal(t, "chosen with --no-pings", connectPings{Hears: false, Because: pingsBecauseChosen}.Describe()) + assert.NotEmpty(t, connectPings{Hears: true, Because: "something a newer build wrote"}.Describe()) +} diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go new file mode 100644 index 000000000..1550201e8 --- /dev/null +++ b/internal/commands/connect_pings_test.go @@ -0,0 +1,319 @@ +//go:build unix + +package commands + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// addedConnectionSecret is shaped as Basecamp shapes an added connection's +// secret, and is not one. +const addedConnectionSecret = "bcac_" + "1111111111111111111111111111111111111111111" + +// pingsWarning is the start's warning that another connector on this +// computer hears the agent's Pings too. +const pingsWarning = "only one connector should answer Pings" + +// asAddedConnection gives a profile the credential of a connection added +// after the agent's first: the same agent, with a secret of its own. +func asAddedConnection(t *testing.T, s *connectSetupServer, profile string) { + t.Helper() + app := newConnectSetupApp(t, s, profile) + key := auth.ProfileCredentialKey(profile) + creds, err := app.Auth.GetStore().Load(key) + require.NoError(t, err) + require.Equal(t, "agent", creds.OAuthType) + creds.ClientSecret = addedConnectionSecret + require.NoError(t, app.Auth.GetStore().Save(key, creds)) +} + +func savedPingsChoice(t *testing.T, f operatorFixture, profile string) (connector.PingsChoice, bool) { + t.Helper() + dir := filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, profile)) + choice, ok, err := connector.LoadPingsChoice(dir) + require.NoError(t, err) + return choice, ok +} + +func holderPings(t *testing.T, f operatorFixture, profile string) *bool { + t.Helper() + dir := filepath.Join(connectStateRoot(t), connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, profile)) + holder, ok := connector.InstanceHolder(dir, f.file.AccountID, f.file.Agent.PersonID) + require.True(t, ok, "%s's connector holds its lock", profile) + return holder.Pings +} + +func (r runningConnector) stop(t *testing.T) { + t.Helper() + r.cancel() + _ = r.awaitExit(t) +} + +// The agent's first connection hears its Pings, as every connector did +// before, and its first start saves that, beside the lock too. +func TestAnAgentsFirstConnectionHearsPings(t *testing.T) { + f := newOperatorFixture(t) + + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + + assert.Contains(t, r.out.String(), "pings=true") + choice, ok := savedPingsChoice(t, f, "agent") + require.True(t, ok, "the first start saves what it found") + assert.True(t, choice.Hears) + assert.Equal(t, pingsBecauseFirstConnection, choice.Because) + assert.False(t, choice.At.IsZero()) + hears := holderPings(t, f, "agent") + require.NotNil(t, hears) + assert.True(t, *hears) + assert.NotContains(t, r.out.String(), pingsWarning, "nobody else hears them") +} + +// A connection added after the first hears no Pings until started with +// --pings, and a restart keeps whatever was chosen last. +func TestAnAddedConnectionHearsPingsOnlyWhenStartedWithThem(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + asAddedConnection(t, f.s, "laptop") + + for _, step := range []struct { + args []string + hears bool + because string + }{ + {nil, false, pingsBecauseAddedConnection}, + {[]string{"--pings"}, true, pingsBecauseChosen}, + {nil, true, pingsBecauseChosen}, + {[]string{"--no-pings"}, false, pingsBecauseChosen}, + {nil, false, pingsBecauseChosen}, + } { + r := startProfileConnector(t, f.s, "laptop", step.args...) + r.awaitRunning(t) + assert.Contains(t, r.out.String(), fmt.Sprintf("pings=%t", step.hears), "started with %v", step.args) + choice, ok := savedPingsChoice(t, f, "laptop") + require.True(t, ok) + assert.Equal(t, step.hears, choice.Hears, "started with %v", step.args) + assert.Equal(t, step.because, choice.Because, "started with %v", step.args) + hears := holderPings(t, f, "laptop") + require.NotNil(t, hears) + assert.Equal(t, step.hears, *hears) + r.stop(t) + } +} + +// A run given --project hears no Ping, as before, and leaves the profile's +// saved choice for the next run that isn't narrowed. --pings can't make a +// narrowed run hear one: its feed brings none, so it is refused rather than +// taken. +func TestARunGivenProjectsHearsNoPings(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + r.stop(t) + + r = startProfileConnector(t, f.s, "agent", "--project", fmt.Sprint(setupProject)) + r.awaitRunning(t) + assert.Contains(t, r.out.String(), "pings=false") + hears := holderPings(t, f, "agent") + require.NotNil(t, hears) + assert.False(t, *hears) + choice, ok := savedPingsChoice(t, f, "agent") + require.True(t, ok) + assert.True(t, choice.Hears, "the saved choice is the next un-narrowed run's") + assert.Equal(t, pingsBecauseFirstConnection, choice.Because) + r.stop(t) + + _, err := f.run(t, output.FormatJSON, "--pings", "--project", fmt.Sprint(setupProject)) + require.Error(t, err) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + assert.Contains(t, err.Error(), "--project") +} + +func TestPingsAndNoPingsAreOneChoice(t *testing.T) { + f := newOperatorFixture(t) + _, err := f.run(t, output.FormatJSON, "--pings", "--no-pings") + require.Error(t, err) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + _, ok := savedPingsChoice(t, f, "agent") + assert.False(t, ok, "a refused start saves nothing") +} + +// Two connectors on this computer that both hear the agent's Pings answer +// each one twice. The second to start says so, naming the first; one that +// doesn't hear them, and a shadow, which answers nothing, say nothing. +func TestASecondConnectorThatHearsPingsHereWarns(t *testing.T) { + f := newOperatorFixture(t) + // A second profile on the same agent's first connection: Basecamp that + // cannot add connections replaces the first one's secret, and the CLI + // can't tell from here. + setUpSecondProfile(t, f.s, "laptop") + + first := startProfileConnector(t, f.s, "agent") + first.awaitRunning(t) + + second := startProfileConnector(t, f.s, "laptop") + second.awaitRunning(t) + warning := lineContaining(second.out.String(), pingsWarning) + require.NotEmpty(t, warning, "the second warned before it ran: %s", second.out.String()) + assert.Contains(t, warning, "level=WARN") + assert.Contains(t, warning, "profile=agent", "it names the other") + assert.NotContains(t, first.out.String(), pingsWarning, "the first saw nobody") + second.stop(t) + + quiet := startProfileConnector(t, f.s, "laptop", "--no-pings") + quiet.awaitRunning(t) + assert.NotContains(t, quiet.out.String(), pingsWarning) + quiet.stop(t) + + shadow := startProfileConnector(t, f.s, "laptop", "--shadow", "--pings") + shadow.awaitRunning(t) + assert.NotContains(t, shadow.out.String(), pingsWarning, "a shadow answers nothing") + shadow.stop(t) + + first.stop(t) + again := startProfileConnector(t, f.s, "laptop", "--pings") + again.awaitRunning(t) + assert.NotContains(t, again.out.String(), pingsWarning, "and a stopped connector is nobody") +} + +// status and doctor say whether the profile's connector hears Pings, why, +// and who else on this computer does. +func TestStatusAndDoctorSayWhoHearsPings(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + a := startProfileConnector(t, f.s, "agent") + a.awaitRunning(t) + b := startProfileConnector(t, f.s, "laptop") + b.awaitRunning(t) + + out, err := f.run(t, output.FormatJSON, "status") + require.NoError(t, err, out) + var envelope struct { + Data struct { + LockHolder *struct { + Pings *bool `json:"pings"` + } `json:"lock_holder"` + Pings struct { + Saved *struct { + Hears bool `json:"hears"` + Because string `json:"because"` + } `json:"saved"` + Others []struct { + Profile string `json:"profile"` + PID int `json:"pid"` + } `json:"others"` + } `json:"pings"` + } `json:"data"` + } + require.NoError(t, json.Unmarshal([]byte(out), &envelope), out) + require.NotNil(t, envelope.Data.LockHolder) + require.NotNil(t, envelope.Data.LockHolder.Pings) + assert.True(t, *envelope.Data.LockHolder.Pings, "the running connector hears them") + require.NotNil(t, envelope.Data.Pings.Saved) + assert.True(t, envelope.Data.Pings.Saved.Hears) + assert.Equal(t, pingsBecauseFirstConnection, envelope.Data.Pings.Saved.Because) + require.Len(t, envelope.Data.Pings.Others, 1) + assert.Equal(t, "laptop", envelope.Data.Pings.Others[0].Profile) + + styled, err := f.run(t, output.FormatStyled, "status") + require.NoError(t, err) + assert.Contains(t, styled, "Pings heard: the agent's first connection") + assert.Contains(t, styled, `profile "laptop"`) + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, "Pings") + assert.Contains(t, doctor, `profile "laptop"`) + assert.Contains(t, doctor, "--no-pings") + + b.stop(t) + doctor = f.doctorStyled(t) + assert.Contains(t, doctor, "Hears the agent's Pings: the agent's first connection") + assert.NotContains(t, doctor, `profile "laptop"`) +} + +// doctorStyled is doctor's styled report. The fake Basecamp serves no +// account feed, so doctor fails that check; the report is printed first. +func (f operatorFixture) doctorStyled(t *testing.T) string { + t.Helper() + app := newConnectSetupApp(t, f.s, "agent") + var buf bytes.Buffer + app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: &buf}) + cmd := NewConnectCmd() + cmd.SetArgs([]string{"doctor"}) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SilenceErrors, cmd.SilenceUsage = true, true + _ = cmd.Execute() + return buf.String() +} + +func lineContaining(out, needle string) string { + for line := range strings.SplitSeq(out, "\n") { + if strings.Contains(line, needle) { + return line + } + } + return "" +} + +// Guided setup tells a person whether this computer will answer the agent's +// Pings. A connection added after the first leaves them to the first, so +// "ping it" is no way to try it here. +func TestGuidedSetupOnAnAddedConnectionSaysWhoAnswersPings(t *testing.T) { + s := startConnectSetupServer(t) + ownedByTheOperator(s) + s.agentProjects = projectsNamed(setupProject) + s.connectionSecret = addedConnectionSecret + guided(t, &scriptedPrompter{confirms: []bool{true}}) + + out, err := runConnectSetupCmd(t, bareSetupApp(t, s, "agent")) + require.NoError(t, err, out) + + assert.Contains(t, out, "Once it's running, mention Marie Chef in one of those projects to try it.") + assert.NotContains(t, out, "or ping it") + assert.Contains(t, out, "only one computer should answer") + assert.Contains(t, out, "basecamp connect -P agent --pings") +} + +// A shadow admits what the connector it watches beside would: it reads the +// profile's choice, and never saves one of its own. +func TestAShadowHearsPingsAsItsConnectorWould(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent", "--no-pings") + r.awaitRunning(t) + r.stop(t) + + shadow := startProfileConnector(t, f.s, "agent", "--shadow") + shadow.awaitRunning(t) + assert.Contains(t, shadow.out.String(), "pings=false", "the profile's choice") + shadow.stop(t) + + shadow = startProfileConnector(t, f.s, "agent", "--shadow", "--pings") + shadow.awaitRunning(t) + assert.Contains(t, shadow.out.String(), "pings=true", "a flag is the shadow run's own") + shadow.stop(t) + + choice, ok := savedPingsChoice(t, f, "agent") + require.True(t, ok) + assert.False(t, choice.Hears, "a shadow's flag doesn't change the profile's choice") + shadowDir, err := connectStatePath("agent", f.file, true) + require.NoError(t, err) + _, saved, err := connector.LoadPingsChoice(shadowDir) + require.NoError(t, err) + assert.False(t, saved, "and a shadow saves none of its own") +} diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index f0df920bc..44480cb0a 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -37,13 +37,17 @@ type connectRunFlags struct { shadow bool since int64 hold bool + pings bool + noPings bool } func addConnectRunFlags(cmd *cobra.Command, f *connectRunFlags) { fl := cmd.Flags() // --project shadows the global flag of the same name and keeps its type, // so the flag reads the same everywhere; here it may be repeated. - fl.Var((*repeatedString)(&f.projects), "project", "Only hear events in this project id (repeatable; default every project and ping the agent can see, and a run given one hears no ping)") + fl.Var((*repeatedString)(&f.projects), "project", "Only hear events in this project id (repeatable; default every project the agent can see, and a run given one hears no Ping)") + fl.BoolVar(&f.pings, "pings", false, "Answer the agent's Pings, and anything else outside a project, from this connector; only one should (the default for an agent's first connection; kept for the profile)") + fl.BoolVar(&f.noPings, "no-pings", false, "Leave the agent's Pings to the connector that answers them, and hear only projects (the default for a connection added after the first; kept for the profile)") fl.BoolVar(&f.shadow, "shadow", false, "Admit and log in an isolated state directory; dispatch and post nothing") fl.Int64Var(&f.since, "since", 0, "Enter the feed just after this event id, whatever the ledger holds") fl.BoolVar(&f.hold, "hold", false, "Set this profile's durable hold: intake and admission run, nothing is dispatched or posted until release, and earlier records wait for review") @@ -191,6 +195,10 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { if err != nil { return err } + pingsFlag, err := connectPingsFlag(f, len(buckets) > 0, name) + if err != nil { + return err + } path, err := setup.Path(config.GlobalConfigDir(), name) if err != nil { @@ -278,17 +286,31 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { case err != nil: return output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) } - lock, err := acquireConnectLock(stateDir, account, agentID) + pings, savePings, err := connectRunPings(name, file, f.shadow, pingsFlag, len(buckets) > 0, creds) if err != nil { return err } - defer func() { _ = lock.Release() }() - policy, err := file.Policy(agentID) if err != nil { return output.ErrUsage(err.Error()) } policy.Buckets = buckets + policy.HearsPings = pings.Hears + // Admission, the handoff, the lock and the start line all read whether + // the run hears Pings from the policy, so the line says what admission + // was given. + heard := connectPings{Hears: policy.HearsPings, Because: pings.Because} + lock, err := acquireConnectorLock(stateDir, account, agentID, policy.HearsPings) + if err != nil { + return err + } + defer func() { _ = lock.Release() }() + if savePings { + // Under the lock: no other start of this profile writes it at once. + if err := connector.SavePingsChoice(stateDir, connector.PingsChoice{Hears: pings.Hears, Because: pings.Because, At: time.Now().UTC()}); err != nil { + return fmt.Errorf("the choice of whether this connector hears Pings could not be saved, so a restart would not keep it: %w", err) + } + } if _, err := os.Lstat(filepath.Join(stateDir, connector.LedgerFile)); errors.Is(err, os.ErrNotExist) { warnConnectSiblingState(logger, stateDir, file, name) @@ -384,7 +406,10 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { // Whatever ended the run, status says it is not running any more. _ = ledger.NoteConnection(context.WithoutCancel(ctx), stopState, stopDetail) }() - logger.Info("connector: running", connectRunningAttrs(name, account, agentID, f.shadow, len(file.Projects), buckets, stateDir)...) + if !f.shadow { + warnConnectPingsPeers(logger, heard, name, file) + } + logger.Info("connector: running", connectRunningAttrs(name, account, agentID, f.shadow, len(file.Projects), buckets, heard, stateDir)...) var ( wg sync.WaitGroup @@ -427,7 +452,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { started := time.Now() runPart("handoff", func(ctx context.Context) error { return connector.RunHandoff(ctx, connector.HandoffOptions{ - Ledger: ledger, Served: served.Current, Buckets: buckets, AgentID: agentID, + Ledger: ledger, Served: served.Current, Buckets: buckets, Pings: policy.HearsPings, AgentID: agentID, Lines: lines, Logger: logger, Started: started, AckContent: file.Ack().Content, Booster: connector.SDKBooster{Client: accountClient}, }) @@ -471,10 +496,19 @@ func connectorStoppedBy(err error, agent, profile string, refused func() bool) ( return connector.ConnectionDisconnected, e.Message, e } -// acquireConnectLock takes the connector's instance lock for the run, or -// says another connector for this profile already holds it. +// acquireConnectLock takes the connector's instance lock, or says another +// connector for this profile already holds it. func acquireConnectLock(stateDir, account string, agentID int64) (*connector.InstanceLock, error) { - lock, err := connector.AcquireInstanceLock(stateDir, account, agentID, time.Now()) + return lockUnavailable(connector.AcquireInstanceLock(stateDir, account, agentID, time.Now())) +} + +// acquireConnectorLock is acquireConnectLock for the run, which says beside +// the lock whether it hears the agent's Pings. +func acquireConnectorLock(stateDir, account string, agentID int64, pings bool) (*connector.InstanceLock, error) { + return lockUnavailable(connector.AcquireConnectorLock(stateDir, account, agentID, time.Now(), pings)) +} + +func lockUnavailable(lock *connector.InstanceLock, err error) (*connector.InstanceLock, error) { if errors.Is(err, connector.ErrAlreadyRunning) { return nil, &output.Error{Code: output.CodeLockUnavailable, Message: err.Error()} } @@ -749,12 +783,13 @@ func (r *repeatedString) Type() string { return "string" } // many projects the agent serves, and, only when --project narrowed the run, // which ones this run is limited to: a bare count of the narrowing read as // "serving nothing" when there was none. -func connectRunningAttrs(profile, account string, agentID int64, shadow bool, served int, only []int64, stateDir string) []any { +func connectRunningAttrs(profile, account string, agentID int64, shadow bool, served int, only []int64, pings connectPings, stateDir string) []any { attrs := []any{"profile", richtext.SanitizeSingleLine(profile), "account", account, "agent_person_id", agentID, "shadow", shadow, "served_projects", served} if len(only) > 0 { attrs = append(attrs, "only_projects", only) } + attrs = append(attrs, "pings", pings.Hears, "pings_because", pings.Describe()) return append(attrs, "state", richtext.SanitizeSingleLine(stateDir)) } diff --git a/internal/commands/connect_run_log_test.go b/internal/commands/connect_run_log_test.go index 7ba6fe1aa..59f797721 100644 --- a/internal/commands/connect_run_log_test.go +++ b/internal/commands/connect_run_log_test.go @@ -11,7 +11,7 @@ import ( func startLine(only []int64) string { var out bytes.Buffer slog.New(slog.NewTextHandler(&out, nil)).Info("connector: running", - connectRunningAttrs("agent", "181900405", 42, false, 3, only, "/state")...) + connectRunningAttrs("agent", "181900405", 42, false, 3, only, connectPings{Hears: true, Because: pingsBecauseFirstConnection}, "/state")...) return out.String() } diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index 35b383bcb..c39e8cab3 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -163,10 +163,18 @@ func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupF file = &loaded } - renderGuidedSummary(w, r, agent, *file, connectorRunning(name, *file), name) + renderGuidedSummary(w, r, agent, *file, connectorRunning(name, *file), guidedHearsPings(ctx, app, name, *file), name) return nil } +// guidedHearsPings is whether the profile's connector, started as the summary +// says to start it, would answer the agent's Pings. One that can't be told +// is said as before: every connector heard Pings until it could choose. +func guidedHearsPings(ctx context.Context, app *appctx.App, name string, file setup.File) bool { + pings, ok, err := connectProfilePings(ctx, app, name, file) + return err != nil || !ok || pings.Hears +} + // 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 { @@ -610,7 +618,7 @@ 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) { +func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, file setup.File, running, hearsPings bool, name string) { names := make(map[int64]string, len(agent.Projects)) for _, p := range agent.Projects { names[p.ID] = richtext.SanitizeSingleLine(p.Name) @@ -626,6 +634,12 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil slices.Sort(served) agentName := richtext.SanitizeSingleLine(agent.Me.Name) + quoted := richtext.ShellQuote(name) + // How to try it: a mention, and a ping where this computer answers them. + try := fmt.Sprintf("mention %s in one of those projects, or ping it, to try it.", agentName) + if !hearsPings { + try = fmt.Sprintf("mention %s in one of those projects to try it.", agentName) + } fmt.Fprintln(w) fmt.Fprintln(w, r.Success.Render(fmt.Sprintf("%s is set up on this computer", agentName))) if agent.HasOwner { @@ -636,13 +650,18 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil // try it. fmt.Fprintln(w, " Works in: no projects") fmt.Fprintln(w) - fmt.Fprintf(w, "It isn't working in any projects, so only your pings reach it. To add one: basecamp connect setup -P %s --serve \n", richtext.ShellQuote(name)) + if hearsPings { + fmt.Fprintf(w, "It isn't working in any projects, so only your pings reach it. To add one: basecamp connect setup -P %s --serve \n", quoted) + } else { + fmt.Fprintf(w, "It isn't working in any projects, and it leaves Pings to another connection, so nothing reaches it yet. To add one: basecamp connect setup -P %s --serve \n", quoted) + renderGuidedPingsElsewhere(w, quoted) + } 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, or ping it, to try it.\n", agentName) + fmt.Fprintf(w, "\n%s\n", capitalizeFirst(try)) } else { // The connector only listens: the person's own Claude Code session // does the work, through the basecamp-connect skill. @@ -650,7 +669,28 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil fmt.Fprintln(w) fmt.Fprintln(w, "To start it, run /basecamp-connect in your Claude Code session. It starts the") fmt.Fprintln(w, "connector and handles each request with your own setup. The connector on its own:") - fmt.Fprintln(w, " basecamp connect -P "+richtext.ShellQuote(name)) - fmt.Fprintf(w, "\nOnce it's running, mention %s in one of those projects, or ping it, to try it.\n", agentName) + fmt.Fprintln(w, " basecamp connect -P "+quoted) + fmt.Fprintf(w, "\nOnce it's running, %s\n", try) + } + if !hearsPings { + renderGuidedPingsElsewhere(w, quoted) + } +} + +// renderGuidedPingsElsewhere says a connection that hears no Pings leaves +// them to another, and how to make it the one that answers them. +func renderGuidedPingsElsewhere(w io.Writer, quoted string) { + fmt.Fprintln(w) + fmt.Fprintln(w, "It leaves Pings to the agent's first connection: only one computer should answer") + fmt.Fprintln(w, "Pings. To answer them from this one, start it with --pings, and start the other") + fmt.Fprintln(w, "with --no-pings:") + fmt.Fprintln(w, " basecamp connect -P "+quoted+" --pings") +} + +// capitalizeFirst upper-cases a sentence's first letter. +func capitalizeFirst(s string) string { + if s == "" { + return s } + return strings.ToUpper(s[:1]) + s[1:] } diff --git a/internal/commands/connect_setup_test.go b/internal/commands/connect_setup_test.go index 3a77a8ee6..61da23144 100644 --- a/internal/commands/connect_setup_test.go +++ b/internal/commands/connect_setup_test.go @@ -84,6 +84,9 @@ type connectSetupServer struct { rateLimitMints bool // mints counts the agent token mints. mints int + // connectionSecret is the secret a connection hands over; "" means + // fakeConnectSecret, shaped like the agent client's own. + connectionSecret string } func startConnectSetupServer(t *testing.T) *connectSetupServer { @@ -112,7 +115,11 @@ func startConnectSetupServer(t *testing.T) *connectSetupServer { if scope == "" { scope = "full" } - writeJSON(w, map[string]any{"client_id": "agent-client", "client_secret": fakeConnectSecret, "account_id": "999", "scope": scope}) + secret := s.connectionSecret + if secret == "" { + secret = fakeConnectSecret + } + writeJSON(w, map[string]any{"client_id": "agent-client", "client_secret": secret, "account_id": "999", "scope": scope}) }) mux.HandleFunc("/oauth/tokens", func(w http.ResponseWriter, _ *http.Request) { s.mu.Lock() diff --git a/internal/connector/admission/doc.go b/internal/connector/admission/doc.go index 4dbb586de..7c0511a73 100644 --- a/internal/connector/admission/doc.go +++ b/internal/connector/admission/doc.go @@ -84,6 +84,13 @@ // conversation is the Ping (PingConversationKey), answered in its one // Campfire. // +// Serving can't divide Pings between an agent's connectors, so only the one +// that hears them admits a line there (Policy.HearsPings, the run's +// --pings): every other one discards it as pings_not_heard once the Ping read +// says the bucket is a Ping, and reads nothing more. Outside a served project +// nothing but a Ping's line is ever admitted, so the flag decides everything +// there. +// // # Invariants // // Every rule below is held by a test that fails without it. diff --git a/internal/connector/admission/fakes_test.go b/internal/connector/admission/fakes_test.go index 270a4342e..cb1a4dd5b 100644 --- a/internal/connector/admission/fakes_test.go +++ b/internal/connector/admission/fakes_test.go @@ -29,8 +29,9 @@ const ( func basePolicy() Policy { return Policy{ - AgentID: agentID, - Trust: Trust{Mode: TrustOperator, OperatorID: operatorID}, + AgentID: agentID, + HearsPings: true, + Trust: Trust{Mode: TrustOperator, OperatorID: operatorID}, Projects: map[int64]Project{ servedProj: {Class: "internal"}, watchedProj: {Class: "internal", WatchCompletions: true}, diff --git a/internal/connector/admission/matrix.go b/internal/connector/admission/matrix.go index 6ccd22b09..5ddc4728c 100644 --- a/internal/connector/admission/matrix.go +++ b/internal/connector/admission/matrix.go @@ -123,6 +123,10 @@ const ( ReasonDelegated Reason = "delegated" // ReasonOutOfScope: the bucket is outside the --project scope. ReasonOutOfScope Reason = "out_of_scope" + // ReasonPingsNotHeard: a line in a Ping, read by a connector that does + // not hear Pings (Policy.HearsPings). The connector that hears them + // answers it, from its own ledger. + ReasonPingsNotHeard Reason = "pings_not_heard" // ReasonUntrustedPerformer: the performer is not in the trust set. ReasonUntrustedPerformer Reason = "untrusted_performer" // ReasonAssignmentNotOperator: an assignment performed by anyone other diff --git a/internal/connector/admission/pings_heard_test.go b/internal/connector/admission/pings_heard_test.go new file mode 100644 index 000000000..d1f33a44a --- /dev/null +++ b/internal/connector/admission/pings_heard_test.go @@ -0,0 +1,134 @@ +package admission + +import ( + "slices" + "testing" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Only one connector should answer an agent's Pings, so only one started to +// hear them does (Policy.HearsPings). Every other one hears its projects +// alone: a line in a Ping is discarded once the Ping read says the bucket is +// one, whoever wrote it, and nothing past that read is spent on it. +func TestAConnectorThatDoesNotHearPingsDiscardsEveryPingLine(t *testing.T) { + cases := map[string]func(t *testing.T, f *fakeReads) (Policy, Event){ + "the operator's line in their direct Ping": func(_ *testing.T, f *fakeReads) (Policy, Event) { + return basePolicy(), inPing(f, []int64{operatorID}, operatorID, "
what's on my plate?
") + }, + "the operator's mention in a group Ping": func(t *testing.T, f *fakeReads) (Policy, Event) { + return basePolicy(), inPing(f, []int64{operatorID, memberID}, operatorID, "
"+mentionOf(t, agentID)+" summarize this
") + }, + "a person let into that one Ping": func(t *testing.T, f *fakeReads) (Policy, Event) { + return guestPolicy(basePolicy()), inPing(f, []int64{operatorID, pingGuest}, pingGuest, "
"+mentionOf(t, agentID)+" what changed?
") + }, + } + for name, build := range cases { + t.Run(name, func(t *testing.T) { + f := newFakeReads() + p, ev := build(t, f) + require.True(t, p.HearsPings, "the fixtures' connector hears Pings") + heard := decide(t, newAdmitter(t, p, f), ev) + require.Equal(t, StateAdmitted, heard.State, "reason %q: a connector that hears Pings takes this request", heard.Reason) + + f = newFakeReads() + p, ev = build(t, f) + p.HearsPings = false + v := decide(t, newAdmitter(t, p, f), ev) + assert.Equal(t, StateDiscarded, v.State) + assert.Equal(t, ReasonPingsNotHeard, v.Reason) + assert.Nil(t, v.Snapshot, "a discard carries no content") + assert.Empty(t, v.ConversationKey) + assert.Equal(t, 1, f.pingCalls, "the Ping read is what says the line is a Ping's") + assert.Zero(t, f.summaryCalls, "and nothing past it is read") + }) + } +} + +// Not hearing Pings takes nothing away from a connector's projects: a line in +// a served project's Campfire, and a mention in a project it doesn't serve, +// are decided as they always were. +func TestAConnectorThatDoesNotHearPingsStillHearsItsProjects(t *testing.T) { + noPings := basePolicy() + noPings.HearsPings = false + + t.Run("a mention in a served project's Campfire", func(t *testing.T) { + f := newFakeReads() + s := summaryWith(recordingID, servedProj, "Chat::Lines::RichText", operatorID, "
"+mentionOf(t, agentID)+" ship it
") + s.CampfireID = campfireID + f.summaries[recordingID] = s + + v := decide(t, newAdmitter(t, noPings, f), Event{ID: eventID, EventType: "chat.line.created", BucketID: servedProj, RecordingID: recordingID, CreatorID: operatorID}) + assert.Equal(t, StateAdmitted, v.State, "reason %q", v.Reason) + assert.True(t, v.Served) + assert.Zero(t, f.pingCalls, "a served project's line is never asked about") + }) + + t.Run("a mention in an unserved project's Campfire", func(t *testing.T) { + f := newFakeReads() + s := summaryWith(recordingID, unservedProj, "Chat::Lines::RichText", operatorID, mentionOf(t, agentID)) + s.CampfireID = campfireID + f.summaries[recordingID] = s + + v := decide(t, newAdmitter(t, noPings, f), Event{ID: eventID, EventType: "chat.line.created", BucketID: unservedProj, RecordingID: recordingID, CreatorID: operatorID}) + assert.Equal(t, StateBlocked, v.State, "a project's, not a Ping's: not this flag's to drop") + assert.Equal(t, ReasonNoRoute, v.Reason) + }) +} + +// Outside a served project, the Ping flag decides everything: a connector +// that hears Pings admits nothing there but a Ping's line, and one that does +// not admits nothing there at all. Every type in the matrix is tried, in a +// Ping and in a project connect.json does not serve, as the operator's own +// words mentioning the agent — the strongest request there is. +func TestNothingOutsideAServedProjectIsAdmittedWithoutPings(t *testing.T) { + admittedPing := false + for _, eventType := range slices.Sorted(func(yield func(string) bool) { + for k := range V1Matrix() { + if !yield(k) { + return + } + } + }) { + for _, bucket := range []int64{pingID, unservedProj} { + for _, hears := range []bool{true, false} { + f := newFakeReads() + ev := outsideAProject(t, f, eventType, bucket) + p := basePolicy() + p.HearsPings = hears + v := decide(t, newAdmitter(t, p, f), ev) + if v.State != StateAdmitted { + continue + } + assert.True(t, hears, "%s in bucket %d was admitted by a connector that hears no Pings", eventType, bucket) + assert.True(t, IsPingConversation(v.ConversationKey, pingID), "%s in bucket %d was admitted outside a Ping", eventType, bucket) + admittedPing = true + } + } + } + assert.True(t, admittedPing, "the fixtures admit a Ping's line when Pings are heard, or this test sees nothing") +} + +// outsideAProject is the operator's event of eventType in bucket, which +// connect.json does not serve, on a recording the operator wrote that +// mentions the agent. A bucket that is pingID is a direct Ping with the +// operator. +func outsideAProject(t *testing.T, f *fakeReads, eventType string, bucket int64) Event { + t.Helper() + if bucket == pingID { + f.pings[pingID] = &basecamp.Ping{ID: pingID, People: []basecamp.Person{{ID: operatorID}}} + } + content := "
" + mentionOf(t, agentID) + " please look
" + s := summaryWith(recordingID, bucket, "Comment", operatorID, content) + s.Parent = &basecamp.Parent{ID: recordingID + 1} + s.CampfireID = campfireID + if eventType == "chat.line.created" { + s.Type = "Chat::Lines::RichText" + } + f.summaries[recordingID] = s + f.subscriptions[recordingID+1] = true + f.assignments[eventID] = []int64{agentID} + return Event{ID: eventID, EventType: eventType, BucketID: bucket, RecordingID: recordingID, CreatorID: operatorID} +} diff --git a/internal/connector/admission/policy.go b/internal/connector/admission/policy.go index 26edbe281..0b1d39862 100644 --- a/internal/connector/admission/policy.go +++ b/internal/connector/admission/policy.go @@ -316,6 +316,18 @@ type Policy struct { // Buckets is the --project scope. Empty means every project the agent can // see. Buckets []int64 `json:"-"` + // HearsPings says this connector answers the agent's Pings, and anything + // else outside a project (--pings). Only one connector should: every + // connector for the agent sees every Ping, and each one that admits a + // line answers it. One that does not hear them discards a line once the + // Ping read says its bucket is a Ping, whoever wrote it. Like Buckets it + // is the run's, never connect.json's, and its zero value hears none. + // + // "Anything else outside a project" is a rule for what is added later + // rather than a second check today: outside a served project, only a + // Ping's line is ever admitted (TestNothingOutsideAServedProjectIsAdmittedWithoutPings), + // so a trigger that admits elsewhere outside a project must ask this too. + HearsPings bool `json:"-"` Trust Trust `json:"trust"` // Projects maps a bucket id to the entry for the project it serves. diff --git a/internal/connector/admission/verdict.go b/internal/connector/admission/verdict.go index a293b07eb..680454059 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -378,6 +378,12 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error if reason != "" { return v.end(StateBlocked, reason), nil } + if ping != nil && !policy.HearsPings { + // Another connector answers the agent's Pings. Every connector + // sees every Ping, so one that admitted the line too would + // answer it twice. + return v.end(StateDiscarded, ReasonPingsNotHeard), nil + } if ping != nil && gate.Role != RoleOperator && !gate.InPing { // Project trust's participants are a project's members, and a // Ping is no project. An allowed agent never gets here: the gate diff --git a/internal/connector/handoff.go b/internal/connector/handoff.go index 999467450..af32832b9 100644 --- a/internal/connector/handoff.go +++ b/internal/connector/handoff.go @@ -135,6 +135,10 @@ type HandoffOptions struct { Served func() (map[int64]admission.Project, error) // Buckets narrows the served projects to a run's --project, when given. Buckets []int64 + // Pings hands off a Ping's requests: only the connector that hears the + // agent's Pings does (admission.Policy.HearsPings). A run narrowed by + // Buckets hands off none, whatever this says, as its feed brings none. + Pings bool // AgentID is the agent's Person id, whose mentions are stripped. AgentID int64 Lines *ndjson.Writer @@ -220,7 +224,7 @@ func handOffReady(ctx context.Context, opts HandoffOptions) error { buckets = slices.DeleteFunc(buckets, func(b int64) bool { return !slices.Contains(opts.Buckets, b) }) } // A Ping is in no project, so a run narrowed to projects hears none. - pings := len(opts.Buckets) == 0 + pings := opts.Pings && len(opts.Buckets) == 0 records, err := opts.Ledger.handoffRecords(ctx, buckets, pings, 50) if err != nil { return err diff --git a/internal/connector/handoff_test.go b/internal/connector/handoff_test.go index d12c57ff5..dafcd027a 100644 --- a/internal/connector/handoff_test.go +++ b/internal/connector/handoff_test.go @@ -118,7 +118,8 @@ func TestOnlyServedProjectsAreHandedOff(t *testing.T) { // A Ping is in no project connect.json serves, and its operator's line is // handed off all the same, saying it is a Ping's, even with no project -// served. A run narrowed to projects hears no Ping. +// served — by the one connector that hears Pings. Any other connector, and a +// run narrowed to projects, hands off none. func TestAPingsRequestIsHandedOffWithNoServedProject(t *testing.T) { const ping int64 = 2085958521 ping1 := func(t *testing.T, ledger *Ledger, id int64) { @@ -142,6 +143,7 @@ func TestAPingsRequestIsHandedOffWithNoServedProject(t *testing.T) { var out bytes.Buffer opts := handoffOptions(ledger, &out) opts.Served = func() (map[int64]admission.Project, error) { return map[int64]admission.Project{}, nil } + opts.Pings = true require.NoError(t, handOffReady(context.Background(), opts)) @@ -167,16 +169,32 @@ func TestAPingsRequestIsHandedOffWithNoServedProject(t *testing.T) { assert.False(t, lines[0].Ping) }) + t.Run("a connector that does not hear Pings", func(t *testing.T) { + ledger := newTestLedger(t) + ping1(t, ledger, 1) + admitOn(t, ledger, 2, "recording:1") + var out bytes.Buffer + opts := handoffOptions(ledger, &out) + + require.NoError(t, handOffReady(context.Background(), opts)) + + lines := handedOffLines(t, &out) + require.Len(t, lines, 1, "its served project's request is still handed off") + assert.Equal(t, int64(2), lines[0].EventID) + assert.Equal(t, StateAdmitted, getRecord(t, ledger, 1).State, "the Ping's is not") + }) + t.Run("a run narrowed to projects", func(t *testing.T) { ledger := newTestLedger(t) ping1(t, ledger, 1) var out bytes.Buffer opts := handoffOptions(ledger, &out) opts.Buckets = []int64{adapterBucketID} + opts.Pings = true require.NoError(t, handOffReady(context.Background(), opts)) - assert.Empty(t, out.String()) + assert.Empty(t, out.String(), "a narrowed run hears no Ping, whatever it was told") assert.Equal(t, StateAdmitted, getRecord(t, ledger, 1).State) }) } diff --git a/internal/connector/lock.go b/internal/connector/lock.go index 6def79436..e7a86f3b8 100644 --- a/internal/connector/lock.go +++ b/internal/connector/lock.go @@ -30,11 +30,29 @@ type instanceHolder struct { StartedAt string `json:"started_at"` AccountID string `json:"account_id"` AgentID int64 `json:"agent_person_id"` + // Pings is whether the holder hears the agent's Pings: true only for a + // connector that answers them. Every holder this build writes says, so + // nil is a holder an older build wrote, which does not. + Pings *bool `json:"pings,omitempty"` } // AcquireInstanceLock takes the lock in a profile's state directory, or refuses. // The account and agent name the file; the containing directory isolates profiles. +// The holder it describes hears no Pings: it is for a command that needs the +// connector stopped, not for the connector. func AcquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Time) (*InstanceLock, error) { + return acquireInstanceLock(dir, accountID, agentPersonID, now, false) +} + +// AcquireConnectorLock is AcquireInstanceLock for the running connector, +// which says beside the lock whether it hears the agent's Pings: another +// connector for the agent on this computer reads it to warn that two answer +// them (InstanceHolder). +func AcquireConnectorLock(dir, accountID string, agentPersonID int64, now time.Time, pings bool) (*InstanceLock, error) { + return acquireInstanceLock(dir, accountID, agentPersonID, now, pings) +} + +func acquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Time, pings bool) (*InstanceLock, error) { account, err := strconv.ParseUint(accountID, 10, 64) if err != nil || account == 0 || agentPersonID <= 0 { return nil, errors.New("connector: the instance lock needs a numeric account id and an agent person id") @@ -67,6 +85,7 @@ func AcquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Ti StartedAt: now.UTC().Format(time.RFC3339), AccountID: accountID, AgentID: agentPersonID, + Pings: &pings, }) if err == nil { // Best effort, and deliberately after the lock is held: a connector @@ -102,6 +121,9 @@ func describeHolder(path string) string { type InstanceHolderInfo struct { PID int StartedAt string + // Pings is whether it hears the agent's Pings; nil when an older build + // wrote it, which does not say. + Pings *bool } // InstanceHolder reads what a connector holding the lock in dir wrote about @@ -121,5 +143,5 @@ func InstanceHolder(dir, accountID string, agentPersonID int64) (InstanceHolderI if err := json.Unmarshal(raw, &holder); err != nil || holder.PID <= 0 { return InstanceHolderInfo{}, false } - return InstanceHolderInfo{PID: holder.PID, StartedAt: holder.StartedAt}, true + return InstanceHolderInfo{PID: holder.PID, StartedAt: holder.StartedAt, Pings: holder.Pings}, true } diff --git a/internal/connector/pings.go b/internal/connector/pings.go new file mode 100644 index 000000000..72a045fdf --- /dev/null +++ b/internal/connector/pings.go @@ -0,0 +1,78 @@ +package connector + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "time" + + "github.com/basecamp/basecamp-cli/internal/connector/setup" +) + +// PingsFile is where a profile's state keeps whether its connector hears the +// agent's Pings, beside the ledger. +// +// It is a file of its own rather than a ledger table or a connect.json key on +// purpose. A table is a schema migration, which an older build refuses to +// open; a connect.json key is one an older build's strict parse refuses, and +// connect.json is setup's to write, under the credential's lock. This file is +// the run's, written under the instance lock, and an older build never looks +// at it. +const PingsFile = "pings.json" + +// PingsChoice is whether a profile's connector hears the agent's Pings, and +// what decided it. A profile's first start decides it and saves it, and every +// start after keeps it until --pings or --no-pings decides again. +type PingsChoice struct { + Hears bool `json:"hears"` + // Because is what decided it: --pings or --no-pings, or the default the + // first start found for the profile's credential. The run names the + // values; this file only keeps them. + Because string `json:"because"` + At time.Time `json:"at"` +} + +// LoadPingsChoice reads the choice saved in a profile's state directory. It +// reports false when none has been saved. A file that is there and cannot be +// read as one is an error, never a default: guessing either way answers +// Pings twice or not at all. +func LoadPingsChoice(dir string) (PingsChoice, bool, error) { + path := filepath.Join(dir, PingsFile) + data, err := setup.ReadPrivateStateFile(path) + switch { + case errors.Is(err, os.ErrNotExist): + return PingsChoice{}, false, nil + case err != nil: + return PingsChoice{}, false, err + } + var raw struct { + Hears *bool `json:"hears"` + Because string `json:"because"` + At time.Time `json:"at"` + } + dec := json.NewDecoder(bytes.NewReader(data)) + if err := dec.Decode(&raw); err != nil { + return PingsChoice{}, false, fmt.Errorf("%s cannot be read: %w", path, err) + } + if _, err := dec.Token(); !errors.Is(err, io.EOF) { + return PingsChoice{}, false, fmt.Errorf("%s cannot be read: more than one object", path) + } + if raw.Hears == nil { + return PingsChoice{}, false, fmt.Errorf("%s cannot be read: it does not say whether the connector hears Pings", path) + } + return PingsChoice{Hears: *raw.Hears, Because: raw.Because, At: raw.At}, true, nil +} + +// SavePingsChoice saves the choice in a profile's state directory, replacing +// any earlier one. The run saves it while it holds the instance lock. +func SavePingsChoice(dir string, c PingsChoice) error { + data, err := json.Marshal(c) + if err != nil { + return err + } + return setup.WritePrivateStateFile(filepath.Join(dir, PingsFile), append(data, '\n')) +} diff --git a/internal/connector/pings_test.go b/internal/connector/pings_test.go new file mode 100644 index 000000000..6f3b56d13 --- /dev/null +++ b/internal/connector/pings_test.go @@ -0,0 +1,124 @@ +package connector + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/setup" +) + +// A running connector says beside its lock whether it hears the agent's +// Pings, so another connector for the agent on this computer can see two +// answering them. Anything else that takes the lock hears none, and a holder +// an older build wrote says nothing either way. +func TestTheLockSaysWhetherItsConnectorHearsPings(t *testing.T) { + const account, agent = "2914079", int64(52007412) + now := time.Now() + + for _, hears := range []bool{true, false} { + dir := t.TempDir() + lock, err := AcquireConnectorLock(dir, account, agent, now, hears) + require.NoError(t, err) + holder, ok := InstanceHolder(dir, account, agent) + require.True(t, ok) + require.NotNil(t, holder.Pings, "a connector always says") + assert.Equal(t, hears, *holder.Pings) + require.NoError(t, lock.Release()) + _, ok = InstanceHolder(dir, account, agent) + assert.False(t, ok, "a released lock says nothing") + } + + t.Run("a command that only takes the lock", func(t *testing.T) { + dir := t.TempDir() + lock, err := AcquireInstanceLock(dir, account, agent, now) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + holder, ok := InstanceHolder(dir, account, agent) + require.True(t, ok) + require.NotNil(t, holder.Pings) + assert.False(t, *holder.Pings, "it is no connector, and answers nothing") + }) + + t.Run("an older build's holder", func(t *testing.T) { + dir := t.TempDir() + lock, err := AcquireInstanceLock(dir, account, agent, now) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + require.NoError(t, os.WriteFile(lock.Path()+".json", []byte(`{"pid":4242,"started_at":"2026-10-21T10:00:00Z","account_id":"2914079","agent_person_id":52007412}`+"\n"), 0o600)) + holder, ok := InstanceHolder(dir, account, agent) + require.True(t, ok) + assert.Equal(t, 4242, holder.PID) + assert.Nil(t, holder.Pings, "an older connector does not say, and heard Pings unless narrowed to projects") + }) +} + +// A profile keeps whether its connector hears Pings beside its ledger, so a +// restart keeps the choice: written privately, read back as it was, and +// refused rather than guessed at when it isn't this user's own file. +func TestAProfileKeepsItsPingsChoiceBesideItsLedger(t *testing.T) { + dir := filepath.Join(t.TempDir(), "state") + require.NoError(t, setup.EnsurePrivateDir(dir)) + + _, ok, err := LoadPingsChoice(dir) + require.NoError(t, err) + assert.False(t, ok, "nothing chosen yet") + + at := time.Date(2026, 10, 21, 12, 0, 0, 0, time.UTC) + require.NoError(t, SavePingsChoice(dir, PingsChoice{Hears: true, Because: "first_connection", At: at})) + got, ok, err := LoadPingsChoice(dir) + require.NoError(t, err) + require.True(t, ok) + assert.Equal(t, PingsChoice{Hears: true, Because: "first_connection", At: at}, got) + info, err := os.Stat(filepath.Join(dir, PingsFile)) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + require.NoError(t, SavePingsChoice(dir, PingsChoice{Hears: false, Because: "chosen", At: at.Add(time.Hour)})) + got, ok, err = LoadPingsChoice(dir) + require.NoError(t, err) + require.True(t, ok) + assert.False(t, got.Hears, "a later choice replaces the first") + assert.Equal(t, "chosen", got.Because) + + t.Run("a file that says nothing", func(t *testing.T) { + for name, body := range map[string]string{ + "not JSON": "{hears", + "no hears": `{"because":"chosen"}`, + "hears not true": `{"hears":"yes","because":"chosen"}`, + "two objects": `{"hears":true}{"hears":false}`, + "an empty object": `{}`, + } { + sub := filepath.Join(t.TempDir(), "state") + require.NoError(t, setup.EnsurePrivateDir(sub)) + require.NoError(t, os.WriteFile(filepath.Join(sub, PingsFile), []byte(body), 0o600)) + _, _, err := LoadPingsChoice(sub) + assert.Error(t, err, name) + } + }) + + t.Run("a file this user does not keep alone", func(t *testing.T) { + sub := filepath.Join(t.TempDir(), "state") + require.NoError(t, setup.EnsurePrivateDir(sub)) + path := filepath.Join(sub, PingsFile) + require.NoError(t, os.WriteFile(path, []byte(`{"hears":true,"because":"chosen"}`), 0o644)) + require.NoError(t, os.Chmod(path, 0o644)) + _, _, err := LoadPingsChoice(sub) + assert.ErrorIs(t, err, setup.ErrNotPrivate, "a file others can read is refused") + + target := filepath.Join(t.TempDir(), "elsewhere.json") + require.NoError(t, os.WriteFile(target, []byte(`{"hears":true,"because":"chosen"}`), 0o600)) + require.NoError(t, os.Remove(path)) + require.NoError(t, os.Symlink(target, path)) + _, _, err = LoadPingsChoice(sub) + assert.Error(t, err, "a symlink is refused, not followed") + assert.Error(t, SavePingsChoice(sub, PingsChoice{Hears: false, Because: "chosen"}), "and not written through") + data, err := os.ReadFile(target) + require.NoError(t, err) + assert.Contains(t, string(data), `"hears":true`, "the symlink's target is untouched") + }) +} diff --git a/internal/connector/setup/private_state.go b/internal/connector/setup/private_state.go index e24d3c152..4310c8b29 100644 --- a/internal/connector/setup/private_state.go +++ b/internal/connector/setup/private_state.go @@ -3,6 +3,7 @@ package setup import ( "errors" "fmt" + "io" "os" "path/filepath" @@ -209,3 +210,85 @@ func checkPrivateReadableFile(f *os.File, path string) error { } return nil } + +// ReadPrivateStateFile reads a small file the connector keeps in its state, +// held to CheckPrivateFile's rules through the descriptor it reads: the +// directories on the way this user's alone, the file not a symlink, this +// user's own, and readable by nobody else. A missing file is an error +// satisfying errors.Is(err, os.ErrNotExist). +func ReadPrivateStateFile(path string) ([]byte, error) { + abs, err := filepath.Abs(path) + if err != nil { + return nil, err + } + if err := CheckPrivateDir(filepath.Dir(abs)); err != nil { + return nil, err + } + f, err := openNoFollow(abs) + if err != nil { + return nil, err + } + defer f.Close() + if err := checkPrivateReadableFile(f, abs); err != nil { + return nil, err + } + data, err := io.ReadAll(io.LimitReader(f, maxFileBytes+1)) + if err != nil { + return nil, fmt.Errorf("read %s: %w", abs, err) + } + if len(data) > maxFileBytes { + return nil, fmt.Errorf("%s is larger than %d bytes", abs, maxFileBytes) + } + return data, nil +} + +// WritePrivateStateFile replaces a small file the connector keeps in its +// state, atomically and owner-only (0600), in a directory EnsurePrivateDir +// vets. A file already there that is not this user's own regular file is +// refused rather than replaced, as connect.json's is. +func WritePrivateStateFile(path string, data []byte) error { + abs, err := filepath.Abs(path) + if err != nil { + return err + } + dir := filepath.Dir(abs) + if err := EnsurePrivateDir(dir); err != nil { + return err + } + switch f, err := openNoFollow(abs); { + case err == nil: + checkErr := checkPrivateFile(f, abs) + f.Close() + if checkErr != nil { + return checkErr + } + case !errors.Is(err, os.ErrNotExist): + return err + } + tmp, err := os.CreateTemp(dir, "."+filepath.Base(abs)+"-*") + if err != nil { + return fmt.Errorf("write %s: %w", abs, err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // a no-op once renamed + if err := tmp.Chmod(0o600); err != nil { + tmp.Close() + return fmt.Errorf("write %s: %w", abs, err) + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("write %s: %w", abs, err) + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return fmt.Errorf("write %s: %w", abs, err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("write %s: %w", abs, err) + } + if err := os.Rename(tmpName, abs); err != nil { + return fmt.Errorf("write %s: %w", abs, err) + } + syncDir(dir) + return nil +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 64d4af17f..378f52ba2 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -360,6 +360,28 @@ person, and run it again. Don't follow that error's hint. plainly, in the background, and read their output as it arrives, so you can show the link and code while they wait. +## Pings: only one computer answers them + +Every connector for an agent sees all of its Pings, since a Ping is in no +project, so only one computer should answer them. A connector started with +`--pings` hears Pings and anything else outside a project; one started with +`--no-pings` hears only the projects it serves. Without either, the agent's +first connection hears them, a connection added after it doesn't, and a bot +user's login does, as before. A run given `--project` hears none. The first +start saves the choice for the profile, and restarts keep it until `--pings` +or `--no-pings` changes it, so pass one only when the person asks: "answer my +Pings from this computer" is `--pings`, and the connector on the other +computer then needs `--no-pings`. Both are the owner's word, like any other +change to the connector. The start line says which: `pings=true` or +`pings=false`, and why. + +If the log warns `only one connector should answer Pings`, another connector +on this computer hears the same agent's Pings, and each is answered twice. +Tell the person, name the profile the warning names, and ask which should +answer them. Connectors on other computers can't be seen from here, so if the +person runs this agent elsewhere too, remind them once that only one computer +should answer Pings. + ## Invocation **The arguments are natural language, not a grammar.** Pull out the agent, the @@ -389,6 +411,10 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | +Whether this computer answers the agent's Pings is the connector's, not +setup's: "answer my Pings here" starts it with `--pings`, and "don't answer +Pings here" with `--no-pings` (*Pings: only one computer answers them*). + Then: 0. **Versions and leftovers**, once per session (*Before anything*). @@ -502,7 +528,8 @@ members admitted only by `project` are **participants**: they reach the agent by mention, by commenting on a thread it follows, and by completing something it has a stake in (context, not a request), never by assignment or by Ping. Operators can also ping the agent with no setup: every line in a direct Ping -with it is a request, and in a group Ping a mention is. +with it is a request, and in a group Ping a mention is. Only the connector +that answers Pings hears them (*Pings: only one computer answers them*). **Someone in one Ping only.** To let a person reach the agent in a single Ping (a group Ping you share with them, say) and nowhere else, pass @@ -634,16 +661,22 @@ Requests that arrive while it's stopped are not picked up later. ## Checking on it - `basecamp connect doctor -P ''`: the credential, the identity, the - event feed and the ledger. Start here when something seems wrong. It posts - nothing. + event feed, whether it answers Pings (and whether another connector on this + computer does too), and the ledger. Start here when something seems wrong. + It posts nothing. +- **"Why didn't it answer my Ping?"**: this connector may leave Pings to + another (`pings=false` in its start line, or doctor's Pings check). A + `"type":"event"` line with `reason` `pings_not_heard` is a Ping it left to + the connector that answers them. - **"Did that mention reach it?"**: the connector starts from the present, so anything asked more than a minute before it started is not picked up; check that first. Then search the connector's output file for the recording id. A `"type":"request"` line means it was handed to you. A `"type":"event"` line with a `reason` says why it was turned away (for example `untrusted_performer`). No line means the feed never delivered it. -- `basecamp connect status -P ''`: the feed's position, holds and - losses. Read-only, and safe while the connector runs. Handed-off requests +- `basecamp connect status -P ''`: the feed's position, holds, + losses, and whether it answers Pings. Read-only, and safe while the + connector runs. Handed-off requests don't appear here; the output file is the record of those. ## Stopping From 6b275a9ca01139bd037c002db4e3a26faf6f633f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 12:20:15 -0700 Subject: [PATCH 2/8] Decide Pings under the lock, and say what the running connector does The start read the saved choice before it took the instance lock, so a start of the same profile that saved another choice while this one waited for its token was overruled. It now takes the lock first, and the resolver takes the held lock as its argument; the lock's metadata says whether the connector hears Pings, and why, once that is decided. --pings=false and --no-pings=false were read as no flag at all. A flag given false is now the other choice. doctor and guided setup described the next un-narrowed start, so a connector running with --project read as hearing Pings beside the one that does. They now say what the running connector does, and doctor adds what a restart would do when that differs. status --shadow reads the profile's choice, which a shadow uses, rather than the shadow's directory, which never has one. A profile whose state is still in the legacy directory no longer counts an older build's connector on it as a second one. The skill says the flags came after CLI 0.16.0, without raising its version gate past a release that doesn't exist yet. --- internal/commands/connect_doctor.go | 44 +++++++--- internal/commands/connect_operator.go | 20 +++-- internal/commands/connect_pings.go | 91 +++++++++++++++------ internal/commands/connect_pings_test.go | 97 ++++++++++++++++++++++- internal/commands/connect_run.go | 33 ++++---- internal/commands/connect_setup_guided.go | 9 ++- internal/connector/lock.go | 62 ++++++++------- internal/connector/pings_test.go | 5 +- skills/basecamp-connect/SKILL.md | 4 +- 9 files changed, 271 insertions(+), 94 deletions(-) diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 4a8e64897..d6ceb5182 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -175,13 +175,14 @@ func connectUnsupportedOSCheck(goos string) setup.Check { Hint: "Run the connector on Linux or macOS; the rest of the CLI runs here."} } -// pingsCheck says whether the profile's connector, started with neither -// --pings, --no-pings nor --project, hears the agent's Pings, and warns when -// another connector on this computer answers them too. Only one should; the -// ones on other computers can't be seen from here. +// pingsCheck says whether the profile's connector hears the agent's Pings — +// the one running now, as its lock says, else a start with neither --pings, +// --no-pings nor --project — and warns when another connector on this +// computer answers them too. Only one should; the ones on other computers +// can't be seen from here. func pingsCheck(ctx context.Context, p connectProfile) setup.Check { c := setup.Check{Name: "Pings"} - pings, ok, err := connectProfilePings(ctx, p.app, p.name, p.file) + pings, running, ok, err := connectProfilePings(ctx, p.app, p.name, p.file) switch { case err != nil: c.Status, c.Message = setup.StatusFail, "Whether it hears the agent's Pings was saved, and can't be read: "+errorMessage(err) @@ -190,14 +191,25 @@ func pingsCheck(ctx context.Context, p connectProfile) setup.Check { case !ok: c.Status, c.Message = setup.StatusSkip, "Whether it hears the agent's Pings can't be told without the profile's credential" return c - case !pings.Hears: + } + who := "It" + if running { + who = "The connector running now" + } + if !pings.Hears { c.Status = setup.StatusPass - c.Message = "Doesn't hear the agent's Pings: " + pings.Describe() + ". Only one connector should answer them; start this one with --pings if it's that one" + c.Message = who + " doesn't hear the agent's Pings: " + pings.Describe() + ". Only one connector should answer them; start this one with --pings if it's that one" + if running { + c.Message += connectNextPingsNote(ctx, p) + } return c } peers := connectPingsPeers(p.name, p.file) if len(peers) == 0 { - c.Status, c.Message = setup.StatusPass, "Hears the agent's Pings: "+pings.Describe() + c.Status, c.Message = setup.StatusPass, who+" hears the agent's Pings: "+pings.Describe() + if running { + c.Message += connectNextPingsNote(ctx, p) + } return c } others := make([]string, 0, len(peers)) @@ -209,7 +221,21 @@ func pingsCheck(ctx context.Context, p connectProfile) setup.Check { } } c.Status = setup.StatusWarn - c.Message = fmt.Sprintf("Hears the agent's Pings (%s), and so does %s on this computer: each Ping would be answered twice", pings.Describe(), strings.Join(others, ", ")) + c.Message = fmt.Sprintf("%s hears the agent's Pings (%s), and so does %s on this computer: each Ping is answered twice", who, pings.Describe(), strings.Join(others, ", ")) c.Hint = hint return c } + +// connectNextPingsNote is what a restart with neither flag nor --project +// would do, when it differs from the connector running now. +func connectNextPingsNote(ctx context.Context, p connectProfile) string { + live, _ := connectRunningPings(p.name, p.file) + next, ok, err := connectNextPings(ctx, p.app, p.name, p.file) + if err != nil || !ok || next.Hears == live.Hears { + return "" + } + if next.Hears { + return "; restarted without --project, it would hear them (" + next.Describe() + ")" + } + return "; restarted, it would hear none (" + next.Describe() + ")" +} diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index 511efa11c..f06839555 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -228,9 +228,10 @@ type connectLockHolder struct { // platform can give one. Present says a process has that pid, not that it // is that connector. PIDStatus string `json:"pid_status"` - // Pings is whether that connector hears the agent's Pings, as it wrote; - // absent when an older build wrote the file. - Pings *bool `json:"pings,omitempty"` + // Pings is whether that connector hears the agent's Pings, as it wrote, + // and PingsBecause why; absent when an older build wrote the file. + Pings *bool `json:"pings,omitempty"` + PingsBecause string `json:"pings_because,omitempty"` } func runConnectStatus(cmd *cobra.Command, shadow bool) error { @@ -263,9 +264,13 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { } report := connectStatusReport{Profile: p.name, Shadow: shadow, Status: status} if holder, ok := connector.InstanceHolder(dir, p.file.AccountID, p.file.Agent.PersonID); ok { - report.LockHolder = &connectLockHolder{PID: holder.PID, StartedAt: holder.StartedAt, PIDStatus: processPresence(holder.PID), Pings: holder.Pings} + report.LockHolder = &connectLockHolder{PID: holder.PID, StartedAt: holder.StartedAt, PIDStatus: processPresence(holder.PID), Pings: holder.Pings, PingsBecause: holder.PingsBecause} } - if saved, ok, err := connector.LoadPingsChoice(dir); err != nil { + // The choice is the profile's, in its connector's state: a shadow reads + // it there and saves none of its own. + if normal, err := connectStatePath(p.name, p.file, false); err != nil { + report.Pings.Error = errorMessage(err) + } else if saved, ok, err := connector.LoadPingsChoice(normal); err != nil { report.Pings.Error = errorMessage(err) } else if ok { report.Pings.Saved = &saved @@ -850,11 +855,12 @@ func renderConnectStatusPings(w io.Writer, r connectStatusReport) { fmt.Fprintf(w, " Pings %s\n", connectPingsWords(connectPings{Hears: p.Saved.Hears, Because: p.Saved.Because})) } if h := r.LockHolder; h != nil && h.Pings != nil && h.PIDStatus != pidAbsent && (p.Saved == nil || *h.Pings != p.Saved.Hears) { + now := connectPings{Hears: *h.Pings, Because: h.PingsBecause} heard := "hears none" - if *h.Pings { + if now.Hears { heard = "hears them" } - fmt.Fprintf(w, " the connector running now %s\n", heard) + fmt.Fprintf(w, " the connector running now %s: %s\n", heard, now.Describe()) } for _, other := range p.Others { fmt.Fprintf(w, " Pings also heard by %s (pid %d) on this computer: only one connector should answer them\n", other.Words(), other.PID) diff --git a/internal/commands/connect_pings.go b/internal/commands/connect_pings.go index faeb45aa2..10925e79d 100644 --- a/internal/commands/connect_pings.go +++ b/internal/commands/connect_pings.go @@ -2,6 +2,7 @@ package commands import ( "context" + "errors" "log/slog" "os" "path/filepath" @@ -158,12 +159,17 @@ func connectPingsPeers(profile string, file setup.File) []connectPingsPeer { } prefix := strings.TrimSuffix(connector.StateDirName(file.AccountID, file.Agent.PersonID, ""), "-") legacy := connector.LegacyStateDirName(file.AccountID, file.Agent.PersonID) - own := connector.StateDirName(file.AccountID, file.Agent.PersonID, profile) + // This profile's own state is its profile directory, or, until a run of + // this build adopts it, the legacy one status and doctor read as its own. + own := map[string]bool{connector.StateDirName(file.AccountID, file.Agent.PersonID, profile): true} + if dir, err := connectStatePath(profile, file, false); err == nil { + own[filepath.Base(dir)] = true + } var peers []connectPingsPeer for _, entry := range entries { name := entry.Name() other, isProfile := strings.CutPrefix(name, prefix+"-") - if name == own || !entry.IsDir() || (name != legacy && (!isProfile || !isValidProfileName(other))) { + if own[name] || !entry.IsDir() || (name != legacy && (!isProfile || !isValidProfileName(other))) { continue } holder, ok := connector.InstanceHolder(filepath.Join(root, name), file.AccountID, file.Agent.PersonID) @@ -180,12 +186,39 @@ func connectPingsPeers(profile string, file setup.File) []connectPingsPeer { return peers } -// connectProfilePings is whether a start of the profile with neither flag -// nor --project would hear Pings: its saved choice, else the default for the -// credential it holds. It reads only this computer. It reports false when -// the credential can't be read, and a saved choice that can't be read as the -// error the start would refuse with. -func connectProfilePings(ctx context.Context, app *appctx.App, profile string, file setup.File) (connectPings, bool, error) { +// connectProfilePings is whether the profile's connector hears Pings: the +// one running now, as its lock says (running true), else what a start with +// neither flag nor --project would decide, from the saved choice or the +// default for the credential the profile holds. It reads only this computer. +// It reports false when it can't say, and a saved choice that can't be read +// as the error the next start would refuse with. +func connectProfilePings(ctx context.Context, app *appctx.App, profile string, file setup.File) (pings connectPings, running, ok bool, err error) { + if live, ok := connectRunningPings(profile, file); ok { + return live, true, true, nil + } + next, ok, err := connectNextPings(ctx, app, profile, file) + return next, false, ok, err +} + +// connectRunningPings is whether the profile's connector running now hears +// Pings, as it said beside its lock; false when none runs, or an older build +// runs it and doesn't say. +func connectRunningPings(profile string, file setup.File) (connectPings, bool) { + dir, err := connectStatePath(profile, file, false) + if err != nil { + return connectPings{}, false + } + holder, ok := connector.InstanceHolder(dir, file.AccountID, file.Agent.PersonID) + if !ok || holder.Pings == nil || !holderStillRuns(holder) { + return connectPings{}, false + } + return connectPings{Hears: *holder.Pings, Because: holder.PingsBecause}, true +} + +// connectNextPings is whether a start with neither flag nor --project would +// hear Pings: the profile's saved choice, else the default for its +// credential. +func connectNextPings(ctx context.Context, app *appctx.App, profile string, file setup.File) (connectPings, bool, error) { dir, err := connectStatePath(profile, file, false) if err != nil { return connectPings{}, false, err @@ -205,33 +238,41 @@ func connectProfilePings(ctx context.Context, app *appctx.App, profile string, f } // connectPingsFlag is the start's --pings or --no-pings, nil when neither was -// given. It refuses both at once, and --pings on a run given --project, -// whose feed brings no Ping to hear. +// given. A flag given false is the other choice: --pings=false is +// --no-pings. It refuses both at once, and hearing Pings on a run given +// --project, whose feed brings no Ping to hear. func connectPingsFlag(f *connectRunFlags, narrowed bool, profile string) (*bool, error) { + var hears bool switch { - case f.pings && f.noPings: + case f.pingsSet && f.noPingsSet: return nil, output.ErrUsage("--pings and --no-pings are one choice: pass one of them") - case f.pings && narrowed: + case f.pingsSet: + hears = f.pings + case f.noPingsSet: + hears = !f.noPings + default: + return nil, nil + } + if hears && narrowed { return nil, output.ErrUsageHint("--pings can't be used with --project: a run given --project hears no Pings", "To answer Pings here, serve this connector's projects with basecamp connect setup -P "+richtext.ShellQuote(profile)+" --serve , and start it without --project.") - case f.pings: - hears := true - return &hears, nil - case f.noPings: - hears := false - return &hears, nil } - return nil, nil + return &hears, nil } // connectRunPings decides whether this start hears Pings, from its flag, the // profile's saved choice and its credential, and whether it saves the answer. -// The choice is the profile's, kept in its connector's state directory: a -// shadow reads it, so it admits what the connector it watches beside would, -// and never saves one. A saved choice that cannot be read refuses the start -// unless the start doesn't need it: guessing either way answers Pings twice -// or not at all. -func connectRunPings(profile string, file setup.File, shadow bool, flag *bool, narrowed bool, creds *auth.Credentials) (connectPings, bool, error) { +// It takes the run's instance lock as proof it is held: a start of the same +// profile that saved another choice while this one waited must not be +// overruled by a choice read before. The choice is the profile's, kept in its +// connector's state directory: a shadow reads it, so it admits what the +// connector it watches beside would, and never saves one. A saved choice that +// cannot be read refuses the start unless the start doesn't need it: guessing +// either way answers Pings twice or not at all. +func connectRunPings(held *connector.InstanceLock, profile string, file setup.File, shadow bool, flag *bool, narrowed bool, creds *auth.Credentials) (connectPings, bool, error) { + if held == nil { + return connectPings{}, false, errors.New("whether the connector hears Pings is decided only under its instance lock") + } dir, err := connectStatePath(profile, file, false) if err != nil { return connectPings{}, false, output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index 1550201e8..ac5992570 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -7,9 +7,11 @@ import ( "context" "encoding/json" "fmt" + "os" "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -241,8 +243,97 @@ func TestStatusAndDoctorSayWhoHearsPings(t *testing.T) { b.stop(t) doctor = f.doctorStyled(t) - assert.Contains(t, doctor, "Hears the agent's Pings: the agent's first connection") + assert.Contains(t, doctor, "The connector running now hears the agent's Pings: the agent's first connection") assert.NotContains(t, doctor, `profile "laptop"`) + + a.stop(t) + doctor = f.doctorStyled(t) + assert.Contains(t, doctor, "It hears the agent's Pings: the agent's first connection", "with none running, what the next start does") +} + +// A connector running with --project hears no Pings whatever the profile's +// saved choice says, and doctor, status and guided setup say what it does +// now, not what the next un-narrowed start would: a narrowed run beside the +// one that answers Pings is no second answerer. +func TestDiagnosticsSayWhatTheRunningConnectorDoes(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + r.stop(t) + + narrowed := startProfileConnector(t, f.s, "agent", "--project", fmt.Sprint(setupProject)) + narrowed.awaitRunning(t) + other := startProfileConnector(t, f.s, "laptop") + other.awaitRunning(t) + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, "The connector running now doesn't hear the agent's Pings: --project narrows this run to projects") + assert.Contains(t, doctor, "restarted without --project, it would hear them (the agent's first connection)") + assert.NotContains(t, doctor, "answered twice", "the one answering Pings is the only one") + + styled, err := f.run(t, output.FormatStyled, "status") + require.NoError(t, err) + assert.Contains(t, styled, "Pings heard: the agent's first connection", "the saved choice") + assert.Contains(t, styled, "the connector running now hears none: --project narrows this run to projects") + + assert.False(t, guidedHearsPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file), + "guided setup doesn't suggest pinging a connector that can't hear it") +} + +// --pings=false is --no-pings, and --no-pings=false is --pings: a flag given +// false is a choice, never the flag's absence. +func TestAPingsFlagGivenFalseIsTheOtherChoice(t *testing.T) { + f := newOperatorFixture(t) + for _, step := range []struct { + arg string + hears bool + }{ + {"--pings=false", false}, + {"--no-pings=false", true}, + } { + r := startProfileConnector(t, f.s, "agent", step.arg) + r.awaitRunning(t) + assert.Contains(t, r.out.String(), fmt.Sprintf("pings=%t", step.hears), step.arg) + choice, ok := savedPingsChoice(t, f, "agent") + require.True(t, ok) + assert.Equal(t, step.hears, choice.Hears, step.arg) + assert.Equal(t, pingsBecauseChosen, choice.Because, step.arg) + r.stop(t) + } + + _, err := f.run(t, output.FormatJSON, "--no-pings=false", "--project", fmt.Sprint(setupProject)) + require.Error(t, err, "hearing Pings is refused on a narrowed run however it is spelled") + assert.Contains(t, err.Error(), "--project") + _, err = f.run(t, output.FormatJSON, "--pings=false", "--no-pings") + require.Error(t, err, "two flags are two choices, even when they agree") +} + +// A profile whose state is still in the legacy directory reads that state as +// its own, so an older build's connector running on it is this profile's, +// not a second one; to another profile of the agent it is a second one. +func TestAProfilesLegacyConnectorIsNotItsOwnPeer(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + stateHome, err := connectStateHome() + require.NoError(t, err) + legacy, err := ensurePrivateChain(stateHome, "basecamp", "connect", connector.LegacyStateDirName(f.file.AccountID, f.file.Agent.PersonID)) + require.NoError(t, err) + lock, err := connector.AcquireInstanceLock(legacy, f.file.AccountID, f.file.Agent.PersonID, time.Now().Add(-time.Minute)) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + older, err := json.Marshal(map[string]any{"pid": os.Getpid(), "started_at": time.Now().Add(time.Minute).UTC().Format(time.RFC3339), + "account_id": f.file.AccountID, "agent_person_id": f.file.Agent.PersonID}) + require.NoError(t, err) + require.NoError(t, os.WriteFile(lock.Path()+".json", older, 0o600), "as an older build writes it: nothing about Pings") + _, err = ensurePrivateChain(stateHome, "basecamp", "connect", connector.StateDirName(f.file.AccountID, f.file.Agent.PersonID, "laptop")) + require.NoError(t, err) + + assert.Empty(t, connectPingsPeers("agent", f.file), "the legacy state is agent's own until a run adopts it") + peers := connectPingsPeers("laptop", f.file) + require.Len(t, peers, 1) + assert.True(t, peers[0].Older) + assert.Empty(t, peers[0].Profile) } // doctorStyled is doctor's styled report. The fake Basecamp serves no @@ -316,4 +407,8 @@ func TestAShadowHearsPingsAsItsConnectorWould(t *testing.T) { _, saved, err := connector.LoadPingsChoice(shadowDir) require.NoError(t, err) assert.False(t, saved, "and a shadow saves none of its own") + + styled, err := f.run(t, output.FormatStyled, "status", "--shadow") + require.NoError(t, err) + assert.Contains(t, styled, "Pings not heard: chosen with --no-pings", "status --shadow shows the choice the shadow reads") } diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 44480cb0a..795b9cef2 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -39,6 +39,10 @@ type connectRunFlags struct { hold bool pings bool noPings bool + // pingsSet and noPingsSet say the flag was given, true or false: + // --pings=false is a choice, not the flag's absence. + pingsSet bool + noPingsSet bool } func addConnectRunFlags(cmd *cobra.Command, f *connectRunFlags) { @@ -169,6 +173,7 @@ func connectStateParts(profile string, file setup.File, shadow bool) []string { } func runConnect(cmd *cobra.Command, f *connectRunFlags) error { + f.pingsSet, f.noPingsSet = cmd.Flags().Changed("pings"), cmd.Flags().Changed("no-pings") if !connectSupportedOS(runtime.GOOS) { return connectUnsupportedOSError(runtime.GOOS) } @@ -286,7 +291,12 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { case err != nil: return output.ErrUsage("The connector's state directory cannot be used: " + err.Error()) } - pings, savePings, err := connectRunPings(name, file, f.shadow, pingsFlag, len(buckets) > 0, creds) + lock, err := acquireConnectLock(stateDir, account, agentID) + if err != nil { + return err + } + defer func() { _ = lock.Release() }() + pings, savePings, err := connectRunPings(lock, name, file, f.shadow, pingsFlag, len(buckets) > 0, creds) if err != nil { return err } @@ -300,11 +310,7 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { // the run hears Pings from the policy, so the line says what admission // was given. heard := connectPings{Hears: policy.HearsPings, Because: pings.Because} - lock, err := acquireConnectorLock(stateDir, account, agentID, policy.HearsPings) - if err != nil { - return err - } - defer func() { _ = lock.Release() }() + lock.NotePings(heard.Hears, heard.Because) if savePings { // Under the lock: no other start of this profile writes it at once. if err := connector.SavePingsChoice(stateDir, connector.PingsChoice{Hears: pings.Hears, Because: pings.Because, At: time.Now().UTC()}); err != nil { @@ -496,19 +502,10 @@ func connectorStoppedBy(err error, agent, profile string, refused func() bool) ( return connector.ConnectionDisconnected, e.Message, e } -// acquireConnectLock takes the connector's instance lock, or says another -// connector for this profile already holds it. +// acquireConnectLock takes the connector's instance lock for the run, or +// says another connector for this profile already holds it. func acquireConnectLock(stateDir, account string, agentID int64) (*connector.InstanceLock, error) { - return lockUnavailable(connector.AcquireInstanceLock(stateDir, account, agentID, time.Now())) -} - -// acquireConnectorLock is acquireConnectLock for the run, which says beside -// the lock whether it hears the agent's Pings. -func acquireConnectorLock(stateDir, account string, agentID int64, pings bool) (*connector.InstanceLock, error) { - return lockUnavailable(connector.AcquireConnectorLock(stateDir, account, agentID, time.Now(), pings)) -} - -func lockUnavailable(lock *connector.InstanceLock, err error) (*connector.InstanceLock, error) { + lock, err := connector.AcquireInstanceLock(stateDir, account, agentID, time.Now()) if errors.Is(err, connector.ErrAlreadyRunning) { return nil, &output.Error{Code: output.CodeLockUnavailable, Message: err.Error()} } diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index c39e8cab3..5e88fa92b 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -167,11 +167,12 @@ func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupF return nil } -// guidedHearsPings is whether the profile's connector, started as the summary -// says to start it, would answer the agent's Pings. One that can't be told -// is said as before: every connector heard Pings until it could choose. +// guidedHearsPings is whether the profile's connector answers the agent's +// Pings: the one running now, else one started as the summary says to start +// it. One that can't be told is said as before: every connector heard Pings +// until it could choose. func guidedHearsPings(ctx context.Context, app *appctx.App, name string, file setup.File) bool { - pings, ok, err := connectProfilePings(ctx, app, name, file) + pings, _, ok, err := connectProfilePings(ctx, app, name, file) return err != nil || !ok || pings.Hears } diff --git a/internal/connector/lock.go b/internal/connector/lock.go index e7a86f3b8..9668f11fc 100644 --- a/internal/connector/lock.go +++ b/internal/connector/lock.go @@ -21,6 +21,7 @@ var ErrAlreadyRunning = errors.New("connector: another connector already runs fo type InstanceLock struct { unlock func() error path string + holder instanceHolder } // instanceHolder is what a running connector writes beside its lock so the @@ -31,28 +32,18 @@ type instanceHolder struct { AccountID string `json:"account_id"` AgentID int64 `json:"agent_person_id"` // Pings is whether the holder hears the agent's Pings: true only for a - // connector that answers them. Every holder this build writes says, so - // nil is a holder an older build wrote, which does not. + // connector that answers them (NotePings). Every holder this build writes + // says, so nil is a holder an older build wrote, which does not. Pings *bool `json:"pings,omitempty"` + // PingsBecause is why, in the run's own words for its reason. + PingsBecause string `json:"pings_because,omitempty"` } // AcquireInstanceLock takes the lock in a profile's state directory, or refuses. // The account and agent name the file; the containing directory isolates profiles. -// The holder it describes hears no Pings: it is for a command that needs the -// connector stopped, not for the connector. +// The holder it describes hears no Pings until NotePings says otherwise: a +// command that only needs the connector stopped answers nothing. func AcquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Time) (*InstanceLock, error) { - return acquireInstanceLock(dir, accountID, agentPersonID, now, false) -} - -// AcquireConnectorLock is AcquireInstanceLock for the running connector, -// which says beside the lock whether it hears the agent's Pings: another -// connector for the agent on this computer reads it to warn that two answer -// them (InstanceHolder). -func AcquireConnectorLock(dir, accountID string, agentPersonID int64, now time.Time, pings bool) (*InstanceLock, error) { - return acquireInstanceLock(dir, accountID, agentPersonID, now, pings) -} - -func acquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Time, pings bool) (*InstanceLock, error) { account, err := strconv.ParseUint(accountID, 10, 64) if err != nil || account == 0 || agentPersonID <= 0 { return nil, errors.New("connector: the instance lock needs a numeric account id and an agent person id") @@ -80,20 +71,34 @@ func acquireInstanceLock(dir, accountID string, agentPersonID int64, now time.Ti return nil, fmt.Errorf("connector: take the instance lock: %w", err) } - holder, err := json.Marshal(instanceHolder{ + hears := false + lock := &InstanceLock{unlock: unlock, path: path, holder: instanceHolder{ PID: os.Getpid(), StartedAt: now.UTC().Format(time.RFC3339), AccountID: accountID, AgentID: agentPersonID, - Pings: &pings, - }) - if err == nil { - // Best effort, and deliberately after the lock is held: a connector - // that cannot describe itself still must not be a second connector. - _ = os.WriteFile(path+".json", append(holder, '\n'), 0o600) - } + Pings: &hears, + }} + // Best effort, and deliberately after the lock is held: a connector + // that cannot describe itself still must not be a second connector. + lock.describe() + return lock, nil +} + +// NotePings says beside the lock whether the connector holding it hears the +// agent's Pings, and why. Another connector for the agent on this computer +// reads it to warn that two answer them (InstanceHolder). The connector +// decides it while it holds the lock, so a start of the same profile can't +// change the choice between the decision and the run. +func (l *InstanceLock) NotePings(hears bool, because string) { + l.holder.Pings, l.holder.PingsBecause = &hears, because + l.describe() +} - return &InstanceLock{unlock: unlock, path: path}, nil +func (l *InstanceLock) describe() { + if holder, err := json.Marshal(l.holder); err == nil { + _ = os.WriteFile(l.path+".json", append(holder, '\n'), 0o600) + } } // Release drops the lock. @@ -122,8 +127,9 @@ type InstanceHolderInfo struct { PID int StartedAt string // Pings is whether it hears the agent's Pings; nil when an older build - // wrote it, which does not say. - Pings *bool + // wrote it, which does not say. PingsBecause is why. + Pings *bool + PingsBecause string } // InstanceHolder reads what a connector holding the lock in dir wrote about @@ -143,5 +149,5 @@ func InstanceHolder(dir, accountID string, agentPersonID int64) (InstanceHolderI if err := json.Unmarshal(raw, &holder); err != nil || holder.PID <= 0 { return InstanceHolderInfo{}, false } - return InstanceHolderInfo{PID: holder.PID, StartedAt: holder.StartedAt, Pings: holder.Pings}, true + return InstanceHolderInfo{PID: holder.PID, StartedAt: holder.StartedAt, Pings: holder.Pings, PingsBecause: holder.PingsBecause}, true } diff --git a/internal/connector/pings_test.go b/internal/connector/pings_test.go index 6f3b56d13..eeac7a060 100644 --- a/internal/connector/pings_test.go +++ b/internal/connector/pings_test.go @@ -22,12 +22,15 @@ func TestTheLockSaysWhetherItsConnectorHearsPings(t *testing.T) { for _, hears := range []bool{true, false} { dir := t.TempDir() - lock, err := AcquireConnectorLock(dir, account, agent, now, hears) + lock, err := AcquireInstanceLock(dir, account, agent, now) require.NoError(t, err) + lock.NotePings(hears, "chosen") holder, ok := InstanceHolder(dir, account, agent) require.True(t, ok) require.NotNil(t, holder.Pings, "a connector always says") assert.Equal(t, hears, *holder.Pings) + assert.Equal(t, "chosen", holder.PingsBecause) + assert.Equal(t, os.Getpid(), holder.PID, "and still says who holds the lock") require.NoError(t, lock.Release()) _, ok = InstanceHolder(dir, account, agent) assert.False(t, ok, "a released lock says nothing") diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 378f52ba2..b29b7b278 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -373,7 +373,9 @@ or `--no-pings` changes it, so pass one only when the person asks: "answer my Pings from this computer" is `--pings`, and the connector on the other computer then needs `--no-pings`. Both are the owner's word, like any other change to the connector. The start line says which: `pings=true` or -`pings=false`, and why. +`pings=false`, and why. The flags came after CLI 0.16.0: one that refuses +them as unknown, or a start line without `pings=`, is older, so run +`basecamp upgrade`, tell the person, and start it again. If the log warns `only one connector should answer Pings`, another connector on this computer hears the same agent's Pings, and each is answered twice. From 1de2eafff2aef6dfd2fb9344f312682a8bd209d5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 12:34:44 -0700 Subject: [PATCH 3/8] Report the running connector and the saved choice separately The diagnostics took the running connector's Pings from its lock and skipped the saved choice, so an unreadable pings.json passed doctor while the next start would refuse it. And a connector an older build runs, which doesn't say, was taken for none and reported by the choice this build saved, which the older build ignores. doctor now always reads the saved choice, and fails on one it can't read. It reports the running connector from its lock, and one an older build runs as one that may hear Pings, warning about a second beside it. Guided setup and status say the same. --- internal/commands/connect_doctor.go | 65 +++++++++++------------ internal/commands/connect_operator.go | 3 ++ internal/commands/connect_pings.go | 46 +++++++++------- internal/commands/connect_pings_test.go | 49 +++++++++++++++++ internal/commands/connect_setup_guided.go | 7 ++- 5 files changed, 116 insertions(+), 54 deletions(-) diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index d6ceb5182..028f26bf4 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -179,37 +179,50 @@ func connectUnsupportedOSCheck(goos string) setup.Check { // the one running now, as its lock says, else a start with neither --pings, // --no-pings nor --project — and warns when another connector on this // computer answers them too. Only one should; the ones on other computers -// can't be seen from here. +// can't be seen from here. The saved choice is read either way: a running +// connector doesn't make an unreadable one the next start's to discover. func pingsCheck(ctx context.Context, p connectProfile) setup.Check { c := setup.Check{Name: "Pings"} - pings, running, ok, err := connectProfilePings(ctx, p.app, p.name, p.file) - switch { - case err != nil: - c.Status, c.Message = setup.StatusFail, "Whether it hears the agent's Pings was saved, and can't be read: "+errorMessage(err) + next, nextOK, err := connectNextPings(ctx, p.app, p.name, p.file) + if err != nil { + c.Status, c.Message = setup.StatusFail, "Whether it hears the agent's Pings was saved, and can't be read, so the next start without a flag refuses: "+errorMessage(err) c.Hint = "Choose again: basecamp connect -P " + richtext.ShellQuote(p.name) + " --pings, or --no-pings to leave them to another connector." return c - case !ok: + } + running := connectRunningPings(p.name, p.file) + var who, why string + var hears bool + switch { + case running.Running: + who, why, hears = "The connector running now", running.Describe(), running.MayHear() + case nextOK: + who, why, hears = "It", next.Describe(), next.Hears + default: c.Status, c.Message = setup.StatusSkip, "Whether it hears the agent's Pings can't be told without the profile's credential" return c } - who := "It" - if running { - who = "The connector running now" + // What a restart without flags or --project would do, when it differs. + note := "" + if running.Running && nextOK && next.Hears != hears { + if next.Hears { + note = "; restarted without --project, it would hear them (" + next.Describe() + ")" + } else { + note = "; restarted, it would hear none (" + next.Describe() + ")" + } } - if !pings.Hears { + switch { + case !hears: c.Status = setup.StatusPass - c.Message = who + " doesn't hear the agent's Pings: " + pings.Describe() + ". Only one connector should answer them; start this one with --pings if it's that one" - if running { - c.Message += connectNextPingsNote(ctx, p) - } + c.Message = who + " doesn't hear the agent's Pings: " + why + note + ". Only one connector should answer them; start this one with --pings if it's that one" return c + case running.Running && running.Pings == nil: + c.Message = who + " may hear the agent's Pings: " + why + note + default: + c.Message = who + " hears the agent's Pings: " + why + note } peers := connectPingsPeers(p.name, p.file) if len(peers) == 0 { - c.Status, c.Message = setup.StatusPass, who+" hears the agent's Pings: "+pings.Describe() - if running { - c.Message += connectNextPingsNote(ctx, p) - } + c.Status = setup.StatusPass return c } others := make([]string, 0, len(peers)) @@ -221,21 +234,7 @@ func pingsCheck(ctx context.Context, p connectProfile) setup.Check { } } c.Status = setup.StatusWarn - c.Message = fmt.Sprintf("%s hears the agent's Pings (%s), and so does %s on this computer: each Ping is answered twice", who, pings.Describe(), strings.Join(others, ", ")) + c.Message += fmt.Sprintf(", and so does %s on this computer: each Ping is answered twice", strings.Join(others, ", ")) c.Hint = hint return c } - -// connectNextPingsNote is what a restart with neither flag nor --project -// would do, when it differs from the connector running now. -func connectNextPingsNote(ctx context.Context, p connectProfile) string { - live, _ := connectRunningPings(p.name, p.file) - next, ok, err := connectNextPings(ctx, p.app, p.name, p.file) - if err != nil || !ok || next.Hears == live.Hears { - return "" - } - if next.Hears { - return "; restarted without --project, it would hear them (" + next.Describe() + ")" - } - return "; restarted, it would hear none (" + next.Describe() + ")" -} diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index f06839555..ebe45008b 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -862,6 +862,9 @@ func renderConnectStatusPings(w io.Writer, r connectStatusReport) { } fmt.Fprintf(w, " the connector running now %s: %s\n", heard, now.Describe()) } + if h := r.LockHolder; h != nil && h.Pings == nil && h.PIDStatus == pidPresent { + fmt.Fprintf(w, " the connector running now may hear them: %s\n", connectRunning{Running: true}.Describe()) + } for _, other := range p.Others { fmt.Fprintf(w, " Pings also heard by %s (pid %d) on this computer: only one connector should answer them\n", other.Words(), other.PID) } diff --git a/internal/commands/connect_pings.go b/internal/commands/connect_pings.go index 10925e79d..4a89c1352 100644 --- a/internal/commands/connect_pings.go +++ b/internal/commands/connect_pings.go @@ -186,33 +186,41 @@ func connectPingsPeers(profile string, file setup.File) []connectPingsPeer { return peers } -// connectProfilePings is whether the profile's connector hears Pings: the -// one running now, as its lock says (running true), else what a start with -// neither flag nor --project would decide, from the saved choice or the -// default for the credential the profile holds. It reads only this computer. -// It reports false when it can't say, and a saved choice that can't be read -// as the error the next start would refuse with. -func connectProfilePings(ctx context.Context, app *appctx.App, profile string, file setup.File) (pings connectPings, running, ok bool, err error) { - if live, ok := connectRunningPings(profile, file); ok { - return live, true, true, nil +// connectRunning is the profile's connector running now, as it said beside +// its lock. Pings is nil when an older build runs it, which doesn't say, and +// hears Pings unless it was given --project. +type connectRunning struct { + Running bool + Pings *connectPings +} + +// MayHear reports whether the running connector hears Pings, or may. +func (r connectRunning) MayHear() bool { return r.Running && (r.Pings == nil || r.Pings.Hears) } + +// Describe is why, in a person's words. +func (r connectRunning) Describe() string { + if r.Pings == nil { + return "an older build runs it, which doesn't say, and hears them unless given --project" } - next, ok, err := connectNextPings(ctx, app, profile, file) - return next, false, ok, err + return r.Pings.Describe() } -// connectRunningPings is whether the profile's connector running now hears -// Pings, as it said beside its lock; false when none runs, or an older build -// runs it and doesn't say. -func connectRunningPings(profile string, file setup.File) (connectPings, bool) { +// connectRunningPings reads the profile's connector running now from its +// lock, without taking it. +func connectRunningPings(profile string, file setup.File) connectRunning { dir, err := connectStatePath(profile, file, false) if err != nil { - return connectPings{}, false + return connectRunning{} } holder, ok := connector.InstanceHolder(dir, file.AccountID, file.Agent.PersonID) - if !ok || holder.Pings == nil || !holderStillRuns(holder) { - return connectPings{}, false + if !ok || !holderStillRuns(holder) { + return connectRunning{} + } + r := connectRunning{Running: true} + if holder.Pings != nil { + r.Pings = &connectPings{Hears: *holder.Pings, Because: holder.PingsBecause} } - return connectPings{Hears: *holder.Pings, Because: holder.PingsBecause}, true + return r } // connectNextPings is whether a start with neither flag nor --project would diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index ac5992570..3b036aacd 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -412,3 +412,52 @@ func TestAShadowHearsPingsAsItsConnectorWould(t *testing.T) { require.NoError(t, err) assert.Contains(t, styled, "Pings not heard: chosen with --no-pings", "status --shadow shows the choice the shadow reads") } + +// A running connector doesn't hide a saved choice the next start can't read: +// doctor reads it either way, and fails the check. +func TestDoctorReadsTheSavedChoiceWhileAConnectorRuns(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent", "--project", fmt.Sprint(setupProject)) + r.awaitRunning(t) + dir, err := connectStatePath("agent", f.file, false) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, connector.PingsFile), []byte("{hears"), 0o600)) + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, "can't be read, so the next start without a flag refuses") + assert.Contains(t, doctor, "basecamp connect -P agent --pings") +} + +// A connector an older build runs on this profile's state doesn't say +// whether it hears Pings, and hears them unless given --project: it is +// reported as one that may, whatever choice this build saved, which the +// older build ignores. +func TestAnOlderBuildsRunningConnectorMayHearPings(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + r := startProfileConnector(t, f.s, "agent", "--no-pings") + r.awaitRunning(t) + r.stop(t) + + dir, err := connectStatePath("agent", f.file, false) + require.NoError(t, err) + lock, err := connector.AcquireInstanceLock(dir, f.file.AccountID, f.file.Agent.PersonID, time.Now()) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + older, err := json.Marshal(map[string]any{"pid": os.Getpid(), "started_at": time.Now().Add(time.Minute).UTC().Format(time.RFC3339), + "account_id": f.file.AccountID, "agent_person_id": f.file.Agent.PersonID}) + require.NoError(t, err) + require.NoError(t, os.WriteFile(lock.Path()+".json", older, 0o600), "as an older build writes it: nothing about Pings") + other := startProfileConnector(t, f.s, "laptop") + other.awaitRunning(t) + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, "The connector running now may hear the agent's Pings: an older build runs it") + assert.Contains(t, doctor, `and so does profile "laptop"`, "and a second one beside it is warned about") + assert.NotContains(t, doctor, "doesn't hear the agent's Pings", "the saved --no-pings is not what's running") + + styled, err := f.run(t, output.FormatStyled, "status") + require.NoError(t, err) + assert.Contains(t, styled, "the connector running now may hear them: an older build runs it") + assert.True(t, guidedHearsPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file)) +} diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index 5e88fa92b..756e2fe0c 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -172,8 +172,11 @@ func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupF // it. One that can't be told is said as before: every connector heard Pings // until it could choose. func guidedHearsPings(ctx context.Context, app *appctx.App, name string, file setup.File) bool { - pings, _, ok, err := connectProfilePings(ctx, app, name, file) - return err != nil || !ok || pings.Hears + if running := connectRunningPings(name, file); running.Running { + return running.MayHear() + } + next, ok, err := connectNextPings(ctx, app, name, file) + return err != nil || !ok || next.Hears } // checkGuidedScope refuses a credential that can't reply or acknowledge, as From 9224660bedc4634ab08ce3abbf6b7589c6991b2f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 12:59:41 -0700 Subject: [PATCH 4/8] Read who answers Pings through one view status, doctor and guided setup each interpreted the saved choice, the running connector's lock and the other connectors on their own, and each got a corner wrong. They now read one view of the profile: - The saved choice is always read, and one that can't be read names the file to remove before choosing again. A flag alone can't recover it when the file isn't this user's own, since the choice is never written over one. The start's refusal says the same. - The running connector is taken from its lock only when the process holding that pid started before the lock was taken, so stale metadata whose pid was reused describes nothing running. - Two connectors answering is the warning. The one answerer beside a connector that leaves Pings to it is named, not flagged. Guided setup no longer says an added connection leaves Pings to the agent's first connection, which an explicit choice may have moved. It doesn't advertise Pings a start would refuse, either. The docs say the first start without --project saves the choice, since a narrowed run saves nothing. --- README.md | 5 +- docs/agent-connections-rollout.md | 12 ++- internal/commands/connect.go | 7 +- internal/commands/connect_doctor.go | 78 ++++++++------- internal/commands/connect_operator.go | 81 +++++++++------ internal/commands/connect_pings.go | 115 +++++++++++++++++----- internal/commands/connect_pings_test.go | 84 +++++++++++++++- internal/commands/connect_run.go | 3 +- internal/commands/connect_setup_guided.go | 50 ++++++---- skills/basecamp-connect/SKILL.md | 7 +- 10 files changed, 317 insertions(+), 125 deletions(-) diff --git a/README.md b/README.md index 6e42f9513..d82fa9130 100644 --- a/README.md +++ b/README.md @@ -288,8 +288,9 @@ would answer. A connector started with `--pings` hears Pings, and anything else outside a project; one started with `--no-pings` hears only its projects. Without either, the agent's first connection hears them, a connection added after it doesn't, and a bot user's login does, as it always has. A run given -`--project` hears none. The first start saves the choice for the profile, and -restarts keep it until you pass the other flag: +`--project` hears none and saves nothing. The first start without `--project` +saves the choice for the profile, as either flag saves its own, and restarts +keep it until you pass the other flag: ```bash basecamp connect -P desktop --pings # this computer answers Pings diff --git a/docs/agent-connections-rollout.md b/docs/agent-connections-rollout.md index d29e10693..bb4798b4c 100644 --- a/docs/agent-connections-rollout.md +++ b/docs/agent-connections-rollout.md @@ -58,12 +58,14 @@ choice, and warns about a second it can see on the same computer. bot user's, hears them as every connector did before this build. A run given `--project` hears none, as before, and `--pings` with `--project` is refused. -- The first start on this build saves the answer in the profile's state - directory (`pings.json`, beside the ledger), and every restart keeps it - until `--pings` or `--no-pings` changes it. A profile that heard Pings keeps +- The first start on this build without `--project` saves the answer in the + profile's state directory (`pings.json`, beside the ledger), as `--pings` + and `--no-pings` save theirs, and every restart keeps it until one of them + changes it. A run given `--project` saves nothing, so a narrowed first run + leaves the choice to the first one that isn't. A profile that heard Pings keeps hearing them after a reconnect turns its credential into an added - connection. An unreadable `pings.json` refuses the start until a flag - chooses again. + connection. An unreadable `pings.json` refuses the start: remove it, then + choose again with a flag. - `basecamp connect status` and `basecamp connect doctor` say whether the profile's connector answers Pings and why. A connector that hears them warns when another for the same agent on this computer does too, an older diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 7a5e6de5b..a9b8e147c 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -74,9 +74,10 @@ the agent sees all of them, since a Ping is in no project. A connector started with --pings hears Pings, and anything else outside a project; one started with --no-pings hears only projects. Without either, the agent's first connection hears them, a connection added after it doesn't, and a person's -login (a bot user's) does, as before. A run given --project hears none. The -first start saves the choice for the profile, and every restart keeps it until ---pings or --no-pings changes it. A connector that hears Pings warns when +login (a bot user's) does, as before. A run given --project hears none, and +saves nothing. The first start without --project saves the choice for the +profile, as --pings and --no-pings save theirs, and every restart keeps it +until one of them changes it. A connector that hears Pings warns when another one on this computer hears the same agent's; ones on other computers can't be seen, so starting just one is yours to arrange. basecamp connect status and doctor say which this profile's does. diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 028f26bf4..7632d3e30 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -8,7 +8,6 @@ import ( "path/filepath" "runtime" "strconv" - "strings" "time" "github.com/spf13/cobra" @@ -177,64 +176,67 @@ func connectUnsupportedOSCheck(goos string) setup.Check { // pingsCheck says whether the profile's connector hears the agent's Pings — // the one running now, as its lock says, else a start with neither --pings, -// --no-pings nor --project — and warns when another connector on this -// computer answers them too. Only one should; the ones on other computers -// can't be seen from here. The saved choice is read either way: a running -// connector doesn't make an unreadable one the next start's to discover. +// --no-pings nor --project — and warns when two connectors on this computer +// answer them. Only one should; the ones on other computers can't be seen +// from here. The saved choice is read either way: a running connector +// doesn't make an unreadable one the next start's to discover. func pingsCheck(ctx context.Context, p connectProfile) setup.Check { c := setup.Check{Name: "Pings"} - next, nextOK, err := connectNextPings(ctx, p.app, p.name, p.file) - if err != nil { - c.Status, c.Message = setup.StatusFail, "Whether it hears the agent's Pings was saved, and can't be read, so the next start without a flag refuses: "+errorMessage(err) - c.Hint = "Choose again: basecamp connect -P " + richtext.ShellQuote(p.name) + " --pings, or --no-pings to leave them to another connector." + v := readConnectPingsView(ctx, p.app, p.name, p.file) + if v.SavedErr != nil { + c.Status, c.Message = setup.StatusFail, "Whether it hears the agent's Pings was saved, and can't be read, so the next start without a flag refuses: "+errorMessage(v.SavedErr) + c.Hint = v.Remedy(p.name) return c } - running := connectRunningPings(p.name, p.file) - var who, why string - var hears bool - switch { - case running.Running: - who, why, hears = "The connector running now", running.Describe(), running.MayHear() - case nextOK: - who, why, hears = "It", next.Describe(), next.Hears - default: + hears, known := v.Hears() + if !known { c.Status, c.Message = setup.StatusSkip, "Whether it hears the agent's Pings can't be told without the profile's credential" return c } + who, why := "It", v.Next.Describe() + if v.Running.Running { + who, why = "The connector running now", v.Running.Describe() + } + switch { + case !hears: + c.Message = who + " doesn't hear the agent's Pings: " + why + case v.Running.Running && v.Running.Pings == nil: + c.Message = who + " may hear the agent's Pings: " + why + default: + c.Message = who + " hears the agent's Pings: " + why + } // What a restart without flags or --project would do, when it differs. - note := "" - if running.Running && nextOK && next.Hears != hears { - if next.Hears { - note = "; restarted without --project, it would hear them (" + next.Describe() + ")" + if v.Running.Running && v.Next != nil && v.Next.Hears != hears { + if v.Next.Hears { + c.Message += "; restarted without --project, it would hear them (" + v.Next.Describe() + ")" } else { - note = "; restarted, it would hear none (" + next.Describe() + ")" + c.Message += "; restarted, it would hear none (" + v.Next.Describe() + ")" } } switch { + case v.Twice() && hears: + c.Status = setup.StatusWarn + c.Message += ", and so does " + peerList(v.Others) + " on this computer: each Ping is answered twice" + case v.Twice(): + c.Status = setup.StatusWarn + c.Message += ", and " + peerList(v.Others) + " on this computer all do: each Ping is answered more than once" + case len(v.Others) == 1: + c.Status = setup.StatusPass + c.Message += "; " + peerList(v.Others) + " answers them on this computer" + return c case !hears: c.Status = setup.StatusPass - c.Message = who + " doesn't hear the agent's Pings: " + why + note + ". Only one connector should answer them; start this one with --pings if it's that one" + c.Message += ". Only one connector should answer them; start this one with --pings if it's that one" return c - case running.Running && running.Pings == nil: - c.Message = who + " may hear the agent's Pings: " + why + note default: - c.Message = who + " hears the agent's Pings: " + why + note - } - peers := connectPingsPeers(p.name, p.file) - if len(peers) == 0 { c.Status = setup.StatusPass return c } - others := make([]string, 0, len(peers)) - hint := "Only one connector should answer Pings. Start one of them with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" - for _, peer := range peers { - others = append(others, fmt.Sprintf("%s (pid %d)", peer.Words(), peer.PID)) + c.Hint = "Only one connector should answer Pings. Start all but one of them with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" + for _, peer := range v.Others { if peer.Older { - hint = "Only one connector should answer Pings, and an older build can't be told: upgrade it, or start this one with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" + c.Hint = "Only one connector should answer Pings, and an older build can't be told: upgrade it, or start this one with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" } } - c.Status = setup.StatusWarn - c.Message += fmt.Sprintf(", and so does %s on this computer: each Ping is answered twice", strings.Join(others, ", ")) - c.Hint = hint return c } diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index ebe45008b..131604d88 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -203,18 +203,32 @@ type connectStatusReport struct { } // connectStatusPings is whether the profile's connector hears the agent's -// Pings: the choice its starts keep, and the other connectors on this -// computer that hear them too. +// Pings: the choice its starts keep, the connector running now, and the +// other connectors on this computer that hear them. It is the profile's, +// read the same with --shadow: a shadow uses the profile's choice and +// answers nothing. type connectStatusPings struct { - // Saved is the choice the profile's last start kept; nil before any - // start has. + // Saved is the choice the profile's starts keep; nil before one has. Saved *connector.PingsChoice `json:"saved,omitempty"` - // Error is why the saved choice can't be read, when it can't. + // Error is why the saved choice can't be read, when it can't, and Hint + // how to recover. Error string `json:"error,omitempty"` + Hint string `json:"hint,omitempty"` + // Running is the profile's connector running now, as its lock says; nil + // when none runs. + Running *connectStatusRunning `json:"running,omitempty"` // Others are the connectors running on this computer for the same agent, - // under other profiles, that hear its Pings, or may. Never read for a - // shadow, which answers nothing. + // under other profiles, that hear its Pings, or may. Others []connectPingsPeer `json:"others,omitempty"` + // Twice says two connectors on this computer answer them. + Twice bool `json:"twice,omitempty"` +} + +// connectStatusRunning is the profile's connector running now. Hears is nil +// when an older build runs it, which doesn't say. +type connectStatusRunning struct { + Hears *bool `json:"hears,omitempty"` + Because string `json:"because,omitempty"` } // connectLockHolder is what a connector wrote beside its instance lock. It is @@ -266,17 +280,18 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { if holder, ok := connector.InstanceHolder(dir, p.file.AccountID, p.file.Agent.PersonID); ok { report.LockHolder = &connectLockHolder{PID: holder.PID, StartedAt: holder.StartedAt, PIDStatus: processPresence(holder.PID), Pings: holder.Pings, PingsBecause: holder.PingsBecause} } - // The choice is the profile's, in its connector's state: a shadow reads - // it there and saves none of its own. - if normal, err := connectStatePath(p.name, p.file, false); err != nil { - report.Pings.Error = errorMessage(err) - } else if saved, ok, err := connector.LoadPingsChoice(normal); err != nil { - report.Pings.Error = errorMessage(err) - } else if ok { - report.Pings.Saved = &saved - } - if !shadow { - report.Pings.Others = connectPingsPeers(p.name, p.file) + // Status reads no credential, so a profile that has saved no choice has + // none to show. + pings := readConnectPingsView(cmd.Context(), nil, p.name, p.file) + report.Pings = connectStatusPings{Saved: pings.Saved, Others: pings.Others, Twice: pings.Twice()} + if pings.SavedErr != nil { + report.Pings.Error, report.Pings.Hint = errorMessage(pings.SavedErr), pings.Remedy(p.name) + } + if r := pings.Running; r.Running { + report.Pings.Running = &connectStatusRunning{} + if r.Pings != nil { + report.Pings.Running.Hears, report.Pings.Running.Because = &r.Pings.Hears, r.Pings.Because + } } if p.app.Output.EffectiveFormat() == output.FormatStyled { renderConnectStatus(cmd.OutOrStdout(), report) @@ -848,25 +863,31 @@ func renderConnectStatusPings(w io.Writer, r connectStatusReport) { p := r.Pings switch { case p.Error != "": - fmt.Fprintf(w, " Pings the saved choice can't be read (%s); the next start refuses until given --pings or --no-pings\n", richtext.SanitizeSingleLine(p.Error)) + fmt.Fprintf(w, " Pings the saved choice can't be read (%s), so the next start without a flag refuses. %s\n", richtext.SanitizeSingleLine(p.Error), p.Hint) case p.Saved == nil: - fmt.Fprintf(w, " Pings not chosen yet: the next start without --project chooses (an agent's first connection hears them, a connection added after it doesn't)\n") + fmt.Fprintf(w, " Pings not chosen yet: the first start without --project chooses (an agent's first connection hears them, a connection added after it doesn't)\n") default: fmt.Fprintf(w, " Pings %s\n", connectPingsWords(connectPings{Hears: p.Saved.Hears, Because: p.Saved.Because})) } - if h := r.LockHolder; h != nil && h.Pings != nil && h.PIDStatus != pidAbsent && (p.Saved == nil || *h.Pings != p.Saved.Hears) { - now := connectPings{Hears: *h.Pings, Because: h.PingsBecause} - heard := "hears none" - if now.Hears { - heard = "hears them" + if run := p.Running; run != nil { + switch { + case run.Hears == nil: + fmt.Fprintf(w, " the connector running now may hear them: %s\n", connectRunning{Running: true}.Describe()) + case p.Saved == nil || *run.Hears != p.Saved.Hears: + now := connectPings{Hears: *run.Hears, Because: run.Because} + heard := "hears none" + if now.Hears { + heard = "hears them" + } + fmt.Fprintf(w, " the connector running now %s: %s\n", heard, now.Describe()) } - fmt.Fprintf(w, " the connector running now %s: %s\n", heard, now.Describe()) - } - if h := r.LockHolder; h != nil && h.Pings == nil && h.PIDStatus == pidPresent { - fmt.Fprintf(w, " the connector running now may hear them: %s\n", connectRunning{Running: true}.Describe()) } for _, other := range p.Others { - fmt.Fprintf(w, " Pings also heard by %s (pid %d) on this computer: only one connector should answer them\n", other.Words(), other.PID) + if p.Twice { + fmt.Fprintf(w, " Pings heard by %s (pid %d) on this computer too: only one connector should answer them\n", other.Words(), other.PID) + } else { + fmt.Fprintf(w, " Pings answered on this computer by %s (pid %d)\n", other.Words(), other.PID) + } } } diff --git a/internal/commands/connect_pings.go b/internal/commands/connect_pings.go index 4a89c1352..9f5bd5dee 100644 --- a/internal/commands/connect_pings.go +++ b/internal/commands/connect_pings.go @@ -3,6 +3,7 @@ package commands import ( "context" "errors" + "fmt" "log/slog" "os" "path/filepath" @@ -223,28 +224,6 @@ func connectRunningPings(profile string, file setup.File) connectRunning { return r } -// connectNextPings is whether a start with neither flag nor --project would -// hear Pings: the profile's saved choice, else the default for its -// credential. -func connectNextPings(ctx context.Context, app *appctx.App, profile string, file setup.File) (connectPings, bool, error) { - dir, err := connectStatePath(profile, file, false) - if err != nil { - return connectPings{}, false, err - } - saved, ok, err := connector.LoadPingsChoice(dir) - switch { - case err != nil: - return connectPings{}, false, err - case ok: - return connectPings{Hears: saved.Hears, Because: saved.Because}, true, nil - } - creds, err := app.Auth.GetStore().LoadContext(ctx, app.Auth.CredentialKey()) - if err != nil { - return connectPings{}, false, nil //nolint:nilerr // the token checks report an unreadable credential - } - return connectPingsDefault(creds.OAuthType, creds.ClientSecret), true, nil -} - // connectPingsFlag is the start's --pings or --no-pings, nil when neither was // given. A flag given false is the other choice: --pings=false is // --no-pings. It refuses both at once, and hearing Pings on a run given @@ -290,7 +269,7 @@ func connectRunPings(held *connector.InstanceLock, profile string, file setup.Fi switch { case err != nil && flag == nil && !narrowed: return connectPings{}, false, output.ErrUsageHint("Whether this connector hears Pings was saved, and can't be read: "+setup.ErrorText(err), - "Choose again: basecamp connect -P "+richtext.ShellQuote(profile)+" --pings, or --no-pings to leave them to another connector.") + connectPingsRemedy(filepath.Join(dir, connector.PingsFile), profile)) case ok: saved = &choice } @@ -315,3 +294,93 @@ func warnConnectPingsPeers(logger *slog.Logger, pings connectPings, profile stri logger.Warn(msg, "profile", richtext.SanitizeSingleLine(peer.Profile), "pid", peer.PID) } } + +// connectPingsView is what this computer can say about who answers an +// agent's Pings, for one profile: the choice its next start makes, its +// connector running now, and the other connectors here that hear them. +// status, doctor and guided setup all read it, so they tell one story. +type connectPingsView struct { + // Path is the profile's saved choice, pings.json. + Path string + // Saved is that choice, nil when none is saved; SavedErr why it can't be + // read, which refuses the next start without a flag. + Saved *connector.PingsChoice + SavedErr error + // Next is what a start with neither flag nor --project decides: the + // saved choice, else the credential's default. Nil when it can't be told. + Next *connectPings + // Running is the profile's connector running now, from its lock. + Running connectRunning + // Others are the other connectors running here for the agent that hear + // its Pings, or may. + Others []connectPingsPeer +} + +// readConnectPingsView reads the view. With no app it reads no credential, +// and a profile that has saved no choice has no Next. +func readConnectPingsView(ctx context.Context, app *appctx.App, profile string, file setup.File) connectPingsView { + v := connectPingsView{Running: connectRunningPings(profile, file), Others: connectPingsPeers(profile, file)} + dir, err := connectStatePath(profile, file, false) + if err != nil { + v.SavedErr = err + return v + } + v.Path = filepath.Join(dir, connector.PingsFile) + saved, ok, err := connector.LoadPingsChoice(dir) + switch { + case err != nil: + v.SavedErr = err + case ok: + v.Saved = &saved + v.Next = &connectPings{Hears: saved.Hears, Because: saved.Because} + case app != nil: + if creds, err := app.Auth.GetStore().LoadContext(ctx, app.Auth.CredentialKey()); err == nil { + next := connectPingsDefault(creds.OAuthType, creds.ClientSecret) + v.Next = &next + } + } + return v +} + +// Hears is whether the profile's connector answers Pings, or may: the one +// running now, else the next start. It reports false when it can't be told. +func (v connectPingsView) Hears() (hears, known bool) { + switch { + case v.Running.Running: + return v.Running.MayHear(), true + case v.Next != nil: + return v.Next.Hears, true + } + return false, false +} + +// Twice reports whether two connectors on this computer answer, or may +// answer, the agent's Pings. +func (v connectPingsView) Twice() bool { + hears, _ := v.Hears() + return len(v.Others) > 1 || (hears && len(v.Others) > 0) +} + +// Remedy is how to recover from a saved choice that can't be read. The flag +// alone may not: a choice is never written over something that isn't this +// user's own file, so the file goes first. It holds nothing else. +func (v connectPingsView) Remedy(profile string) string { + return connectPingsRemedy(v.Path, profile) +} + +func connectPingsRemedy(path, profile string) string { + remove := "Remove the profile's pings.json" + if path != "" { + remove = "Remove " + richtext.SanitizeSingleLine(path) + } + return remove + ", then choose again: basecamp connect -P " + richtext.ShellQuote(profile) + " --pings, or --no-pings to leave them to another connector." +} + +// peerList names other connectors for a line. +func peerList(peers []connectPingsPeer) string { + words := make([]string, 0, len(peers)) + for _, peer := range peers { + words = append(words, fmt.Sprintf("%s (pid %d)", peer.Words(), peer.PID)) + } + return strings.Join(words, ", ") +} diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index 3b036aacd..7baff1854 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -277,7 +277,7 @@ func TestDiagnosticsSayWhatTheRunningConnectorDoes(t *testing.T) { assert.Contains(t, styled, "Pings heard: the agent's first connection", "the saved choice") assert.Contains(t, styled, "the connector running now hears none: --project narrows this run to projects") - assert.False(t, guidedHearsPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file), + assert.False(t, guidedPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file).hears, "guided setup doesn't suggest pinging a connector that can't hear it") } @@ -459,5 +459,85 @@ func TestAnOlderBuildsRunningConnectorMayHearPings(t *testing.T) { styled, err := f.run(t, output.FormatStyled, "status") require.NoError(t, err) assert.Contains(t, styled, "the connector running now may hear them: an older build runs it") - assert.True(t, guidedHearsPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file)) + assert.True(t, guidedPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file).hears) +} + +// The one connector that answers the agent's Pings, beside one that leaves +// them to it, is the arrangement wanted: status and doctor name the +// answerer, and warn about nothing. +func TestOneAnswererBesideAConnectorThatLeavesPingsIsNoConflict(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + a := startProfileConnector(t, f.s, "agent", "--no-pings") + a.awaitRunning(t) + b := startProfileConnector(t, f.s, "laptop") + b.awaitRunning(t) + + styled, err := f.run(t, output.FormatStyled, "status") + require.NoError(t, err) + assert.Contains(t, styled, `answered on this computer by profile "laptop"`) + assert.NotContains(t, styled, "only one connector should answer them") + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, `The connector running now doesn't hear the agent's Pings: chosen with --no-pings; profile "laptop"`) + assert.NotContains(t, doctor, "answered twice") +} + +// A saved choice that can't be read, or that isn't this user's own file, +// is recovered by removing it and choosing again: a flag alone can't write +// over a file it won't trust. Every place that meets it says so, with its +// path. +func TestAnUnusableSavedChoiceSaysHowToRecover(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + r.stop(t) + dir, err := connectStatePath("agent", f.file, false) + require.NoError(t, err) + path := filepath.Join(dir, connector.PingsFile) + require.NoError(t, os.Remove(path)) + require.NoError(t, os.Symlink(filepath.Join(t.TempDir(), "elsewhere.json"), path)) + + for _, args := range [][]string{nil, {"--no-pings"}} { + _, err := f.run(t, output.FormatJSON, args...) + require.Error(t, err, "started with %v", args) + e := usageError(t, err) + assert.Equal(t, output.CodeUsage, e.Code) + assert.Contains(t, e.Hint, "Remove "+path, "started with %v", args) + } + g := guidedPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", f.file) + assert.False(t, g.hears, "guided setup doesn't advertise Pings a start would refuse") + assert.Contains(t, g.remedy, "Remove "+path) + styled, err := f.run(t, output.FormatStyled, "status") + require.NoError(t, err) + assert.Contains(t, styled, "Remove "+path) + assert.Contains(t, f.doctorStyled(t), "Remove "+path) + + require.NoError(t, os.Remove(path)) + again := startProfileConnector(t, f.s, "agent", "--no-pings") + again.awaitRunning(t) + choice, ok := savedPingsChoice(t, f, "agent") + require.True(t, ok) + assert.False(t, choice.Hears, "and once it's gone, the flag chooses") +} + +// A lock file left behind by a connector that has stopped, whose pid now +// belongs to a process that started after it, describes nothing running. +func TestStaleLockMetadataIsNoRunningConnector(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent") + r.awaitRunning(t) + r.stop(t) + dir, err := connectStatePath("agent", f.file, false) + require.NoError(t, err) + stale, err := json.Marshal(map[string]any{"pid": os.Getpid(), "started_at": "2020-01-01T00:00:00Z", + "account_id": f.file.AccountID, "agent_person_id": f.file.Agent.PersonID, "pings": false, "pings_because": pingsBecauseProject}) + require.NoError(t, err) + holder := filepath.Join(dir, fmt.Sprintf("instance-%s-%d.lock.json", f.file.AccountID, f.file.Agent.PersonID)) + require.NoError(t, os.WriteFile(holder, stale, 0o600)) + + styled, err := f.run(t, output.FormatStyled, "status") + require.NoError(t, err) + assert.Contains(t, styled, "Pings heard: the agent's first connection") + assert.NotContains(t, styled, "the connector running now", "this pid's process started after the lock was taken") } diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 795b9cef2..30ce5f838 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -314,7 +314,8 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error { if savePings { // Under the lock: no other start of this profile writes it at once. if err := connector.SavePingsChoice(stateDir, connector.PingsChoice{Hears: pings.Hears, Because: pings.Because, At: time.Now().UTC()}); err != nil { - return fmt.Errorf("the choice of whether this connector hears Pings could not be saved, so a restart would not keep it: %w", err) + return output.ErrUsageHint("Whether this connector hears Pings could not be saved, so a restart would not keep it: "+setup.ErrorText(err), + connectPingsRemedy(filepath.Join(stateDir, connector.PingsFile), name)) } } diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index 756e2fe0c..716c5bff3 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -163,20 +163,27 @@ func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupF file = &loaded } - renderGuidedSummary(w, r, agent, *file, connectorRunning(name, *file), guidedHearsPings(ctx, app, name, *file), name) + renderGuidedSummary(w, r, agent, *file, connectorRunning(name, *file), guidedPings(ctx, app, name, *file), name) return nil } -// guidedHearsPings is whether the profile's connector answers the agent's -// Pings: the one running now, else one started as the summary says to start -// it. One that can't be told is said as before: every connector heard Pings -// until it could choose. -func guidedHearsPings(ctx context.Context, app *appctx.App, name string, file setup.File) bool { - if running := connectRunningPings(name, file); running.Running { - return running.MayHear() +// guidedPings is what the summary says about the agent's Pings: whether the +// profile's connector answers them — the one running now, else one started +// as the summary says to start it — and, when the saved choice can't be +// read, how to recover, since that start would refuse. One that can't be +// told is said as before: every connector heard Pings until it could choose. +type guidedPingsSummary struct { + hears bool + remedy string +} + +func guidedPings(ctx context.Context, app *appctx.App, name string, file setup.File) guidedPingsSummary { + v := readConnectPingsView(ctx, app, name, file) + if v.SavedErr != nil && !v.Running.Running { + return guidedPingsSummary{remedy: v.Remedy(name)} } - next, ok, err := connectNextPings(ctx, app, name, file) - return err != nil || !ok || next.Hears + hears, known := v.Hears() + return guidedPingsSummary{hears: hears || !known} } // checkGuidedScope refuses a credential that can't reply or acknowledge, as @@ -622,7 +629,8 @@ 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, hearsPings bool, name string) { +func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, file setup.File, running bool, pings guidedPingsSummary, name string) { + hearsPings := pings.hears names := make(map[int64]string, len(agent.Projects)) for _, p := range agent.Projects { names[p.ID] = richtext.SanitizeSingleLine(p.Name) @@ -658,7 +666,7 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil fmt.Fprintf(w, "It isn't working in any projects, so only your pings reach it. To add one: basecamp connect setup -P %s --serve \n", quoted) } else { fmt.Fprintf(w, "It isn't working in any projects, and it leaves Pings to another connection, so nothing reaches it yet. To add one: basecamp connect setup -P %s --serve \n", quoted) - renderGuidedPingsElsewhere(w, quoted) + renderGuidedPingsElsewhere(w, quoted, pings.remedy) } return } @@ -677,17 +685,23 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil fmt.Fprintf(w, "\nOnce it's running, %s\n", try) } if !hearsPings { - renderGuidedPingsElsewhere(w, quoted) + renderGuidedPingsElsewhere(w, quoted, pings.remedy) } } // renderGuidedPingsElsewhere says a connection that hears no Pings leaves -// them to another, and how to make it the one that answers them. -func renderGuidedPingsElsewhere(w io.Writer, quoted string) { +// them to another, and how to make it the one that answers them; or, when +// its saved choice can't be read, how to choose again. +func renderGuidedPingsElsewhere(w io.Writer, quoted, remedy string) { fmt.Fprintln(w) - fmt.Fprintln(w, "It leaves Pings to the agent's first connection: only one computer should answer") - fmt.Fprintln(w, "Pings. To answer them from this one, start it with --pings, and start the other") - fmt.Fprintln(w, "with --no-pings:") + if remedy != "" { + fmt.Fprintln(w, "Whether it answers Pings was saved, and can't be read, so it won't start until") + fmt.Fprintln(w, "you choose again. "+remedy) + return + } + fmt.Fprintln(w, "It leaves Pings to another connection: only one computer should answer Pings.") + fmt.Fprintln(w, "To answer them from this one, start it with --pings, and start the other with") + fmt.Fprintln(w, "--no-pings:") fmt.Fprintln(w, " basecamp connect -P "+quoted+" --pings") } diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index b29b7b278..a8f735d8e 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -367,9 +367,10 @@ project, so only one computer should answer them. A connector started with `--pings` hears Pings and anything else outside a project; one started with `--no-pings` hears only the projects it serves. Without either, the agent's first connection hears them, a connection added after it doesn't, and a bot -user's login does, as before. A run given `--project` hears none. The first -start saves the choice for the profile, and restarts keep it until `--pings` -or `--no-pings` changes it, so pass one only when the person asks: "answer my +user's login does, as before. A run given `--project` hears none and saves +nothing. The first start without `--project` saves the choice for the +profile, as either flag saves its own, and restarts keep it until one changes +it, so pass one only when the person asks: "answer my Pings from this computer" is `--pings`, and the connector on the other computer then needs `--no-pings`. Both are the owner's word, like any other change to the connector. The start line says which: `pings=true` or From a6afc4ab0800258af262758981ca77a9ba82e188 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 13:18:31 -0700 Subject: [PATCH 5/8] Don't crash doctor on a running connector with no next-start choice With no saved choice and a credential that can't be read, nothing says what the next start would do, while the running connector's lock still says what it does. doctor read the next start's choice first, which wasn't there, and crashed; it now describes the running connector first. --- internal/commands/connect_doctor.go | 8 +++++--- internal/commands/connect_pings_test.go | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 7632d3e30..bf5ae9ad6 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -193,9 +193,11 @@ func pingsCheck(ctx context.Context, p connectProfile) setup.Check { c.Status, c.Message = setup.StatusSkip, "Whether it hears the agent's Pings can't be told without the profile's credential" return c } - who, why := "It", v.Next.Describe() - if v.Running.Running { - who, why = "The connector running now", v.Running.Describe() + // A running connector is described by its lock; Next may be unknown + // then, with no saved choice and no credential to read a default from. + who, why := "The connector running now", v.Running.Describe() + if !v.Running.Running { + who, why = "It", v.Next.Describe() } switch { case !hears: diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index 7baff1854..415256ab5 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -541,3 +541,18 @@ func TestStaleLockMetadataIsNoRunningConnector(t *testing.T) { assert.Contains(t, styled, "Pings heard: the agent's first connection") assert.NotContains(t, styled, "the connector running now", "this pid's process started after the lock was taken") } + +// A running connector says what it does even when the profile has saved no +// choice and its credential can't be read, so nothing can say what the next +// start would do: doctor reports the run, and doesn't crash on the rest. +func TestDoctorReportsARunningConnectorWithoutACredential(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent", "--project", fmt.Sprint(setupProject)) + r.awaitRunning(t) + _, saved := savedPingsChoice(t, f, "agent") + require.False(t, saved, "a narrowed first run saves nothing") + require.NoError(t, newConnectSetupApp(t, f.s, "agent").Auth.GetStore().Delete(auth.ProfileCredentialKey("agent"))) + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, "The connector running now doesn't hear the agent's Pings: --project narrows this run to projects") +} From 4252976958132c631d6d8f6db64507db8629d515 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 15:58:51 -0700 Subject: [PATCH 6/8] Show guided setup's Pings remedy whenever the choice can't be read A running connector no longer hides an unreadable pings.json: its next start refuses either way, so the remove-then-choose remedy shows whether or not one runs, and whatever the running one hears. A ping is suggested only where this profile's connector hears them, which a start that would refuse doesn't. Guided setup speaks for this profile alone; other connectors and duplicate answers stay with the start's warning, status and doctor. --- internal/commands/connect_pings_test.go | 65 +++++++++++++++++++++++ internal/commands/connect_setup_guided.go | 64 ++++++++++++---------- 2 files changed, 101 insertions(+), 28 deletions(-) diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index 415256ab5..05cedb9d1 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -19,6 +19,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/setup" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -521,6 +522,70 @@ func TestAnUnusableSavedChoiceSaysHowToRecover(t *testing.T) { assert.False(t, choice.Hears, "and once it's gone, the flag chooses") } +// guidedSummary is guided setup's closing summary for the agent profile, as +// runGuidedConnectSetup renders it from what this computer says now: for +// connect.json as set up, or with its projects taken out. +func (f operatorFixture) guidedSummary(t *testing.T, serving bool) string { + t.Helper() + file := f.file + if !serving { + file.Projects = nil + } + agent := guidedAgent{Me: setup.Person{Name: "Marie Chef"}, Projects: []guidedProject{{ID: setupProject, Name: "Connector"}}} + pings := guidedPings(context.Background(), newConnectSetupApp(t, f.s, "agent"), "agent", file) + var buf bytes.Buffer + renderGuidedSummary(&buf, output.NewRenderer(&buf, false), agent, file, connectorRunning("agent", file), pings, "agent") + return buf.String() +} + +// Guided setup says how to recover a saved choice it can't read whether or +// not a connector runs, and whatever the running one hears: the next start +// refuses either way. It suggests a ping only where the profile's connector +// hears them, which a running one still does and a start that refuses +// doesn't. +func TestGuidedSetupAlwaysSaysHowToRecoverAnUnreadableChoice(t *testing.T) { + for _, tc := range []struct { + name string + args []string + runs bool + pings bool + }{ + {name: "none running"}, + {name: "running and hearing Pings", runs: true, pings: true}, + {name: "running with --no-pings", args: []string{"--no-pings"}, runs: true}, + {name: "running with --project", args: []string{"--project", fmt.Sprint(setupProject)}, runs: true}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newOperatorFixture(t) + r := startProfileConnector(t, f.s, "agent", tc.args...) + r.awaitRunning(t) + if !tc.runs { + r.stop(t) + } + dir, err := connectStatePath("agent", f.file, false) + require.NoError(t, err) + path := filepath.Join(dir, connector.PingsFile) + require.NoError(t, os.WriteFile(path, []byte("{hears"), 0o600)) + + for _, serving := range []bool{true, false} { + out := f.guidedSummary(t, serving) + assert.Contains(t, out, "Remove "+path, "serving=%t:\n%s", serving, out) + assert.Contains(t, out, "its next start will refuse", "serving=%t", serving) + assert.NotContains(t, out, "leaves Pings to another connection", "serving=%t: nothing says who answers them", serving) + ping := "or ping it" + if !serving { + ping = "only your pings reach it" + } + if tc.pings { + assert.Contains(t, out, ping, "serving=%t: the connector running now hears them", serving) + } else { + assert.NotContains(t, out, ping, "serving=%t", serving) + } + } + }) + } +} + // A lock file left behind by a connector that has stopped, whose pid now // belongs to a process that started after it, describes nothing running. func TestStaleLockMetadataIsNoRunningConnector(t *testing.T) { diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index 716c5bff3..5d4a35c36 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -167,11 +167,14 @@ func runGuidedConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupF return nil } -// guidedPings is what the summary says about the agent's Pings: whether the -// profile's connector answers them — the one running now, else one started -// as the summary says to start it — and, when the saved choice can't be -// read, how to recover, since that start would refuse. One that can't be -// told is said as before: every connector heard Pings until it could choose. +// guidedPings is what the summary says about the agent's Pings, for this +// profile alone: whether its connector answers them — the one running now, +// else one started as the summary says to start it — and, when its saved +// choice can't be read, how to recover, running or not, since the next start +// refuses. A start that refuses hears nothing; one that can't be told is said +// as before: every connector heard Pings until it could choose. Other +// connectors, and whether two answer, are the start's warning, status and +// doctor's to say. type guidedPingsSummary struct { hears bool remedy string @@ -179,11 +182,12 @@ type guidedPingsSummary struct { func guidedPings(ctx context.Context, app *appctx.App, name string, file setup.File) guidedPingsSummary { v := readConnectPingsView(ctx, app, name, file) - if v.SavedErr != nil && !v.Running.Running { - return guidedPingsSummary{remedy: v.Remedy(name)} - } hears, known := v.Hears() - return guidedPingsSummary{hears: hears || !known} + g := guidedPingsSummary{hears: hears || (!known && v.SavedErr == nil)} + if v.SavedErr != nil { + g.remedy = v.Remedy(name) + } + return g } // checkGuidedScope refuses a credential that can't reply or acknowledge, as @@ -662,12 +666,15 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil // try it. fmt.Fprintln(w, " Works in: no projects") fmt.Fprintln(w) - if hearsPings { + switch { + case hearsPings: fmt.Fprintf(w, "It isn't working in any projects, so only your pings reach it. To add one: basecamp connect setup -P %s --serve \n", quoted) - } else { + case pings.remedy != "": + fmt.Fprintf(w, "It isn't working in any projects, so nothing reaches it yet. To add one: basecamp connect setup -P %s --serve \n", quoted) + default: fmt.Fprintf(w, "It isn't working in any projects, and it leaves Pings to another connection, so nothing reaches it yet. To add one: basecamp connect setup -P %s --serve \n", quoted) - renderGuidedPingsElsewhere(w, quoted, pings.remedy) } + renderGuidedPingsNote(w, quoted, pings) return } fmt.Fprintf(w, " Works in: %s\n", strings.Join(served, ", ")) @@ -684,25 +691,26 @@ func renderGuidedSummary(w io.Writer, r *output.Renderer, agent guidedAgent, fil fmt.Fprintln(w, " basecamp connect -P "+quoted) fmt.Fprintf(w, "\nOnce it's running, %s\n", try) } - if !hearsPings { - renderGuidedPingsElsewhere(w, quoted, pings.remedy) - } + renderGuidedPingsNote(w, quoted, pings) } -// renderGuidedPingsElsewhere says a connection that hears no Pings leaves -// them to another, and how to make it the one that answers them; or, when -// its saved choice can't be read, how to choose again. -func renderGuidedPingsElsewhere(w io.Writer, quoted, remedy string) { - fmt.Fprintln(w) - if remedy != "" { - fmt.Fprintln(w, "Whether it answers Pings was saved, and can't be read, so it won't start until") - fmt.Fprintln(w, "you choose again. "+remedy) - return +// renderGuidedPingsNote says, when the profile's saved choice can't be read, +// how to choose again, whether or not a connector runs now: its next start +// refuses either way. Otherwise a connection that hears no Pings leaves them +// to another, and it says how to make it the one that answers them. +func renderGuidedPingsNote(w io.Writer, quoted string, pings guidedPingsSummary) { + switch { + case pings.remedy != "": + fmt.Fprintln(w) + fmt.Fprintln(w, "Whether it answers Pings was saved, and can't be read, so its next start will refuse.") + fmt.Fprintln(w, pings.remedy) + case !pings.hears: + fmt.Fprintln(w) + fmt.Fprintln(w, "It leaves Pings to another connection: only one computer should answer Pings.") + fmt.Fprintln(w, "To answer them from this one, start it with --pings, and start the other with") + fmt.Fprintln(w, "--no-pings:") + fmt.Fprintln(w, " basecamp connect -P "+quoted+" --pings") } - fmt.Fprintln(w, "It leaves Pings to another connection: only one computer should answer Pings.") - fmt.Fprintln(w, "To answer them from this one, start it with --pings, and start the other with") - fmt.Fprintln(w, "--no-pings:") - fmt.Fprintln(w, " basecamp connect -P "+quoted+" --pings") } // capitalizeFirst upper-cases a sentence's first letter. From 2a5add4fa5f1b8b3b3273e04348f5b85326d5dd4 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 15:58:57 -0700 Subject: [PATCH 7/8] Say "more than once" when two or more others hear Pings Beside one other connector that hears the agent's Pings, each is answered twice; beside two or more, more than once. Doctor now says so, and a starting connector logs one warning naming every other one rather than an "answered twice" line per peer. --- internal/commands/connect_doctor.go | 5 +++- internal/commands/connect_pings.go | 23 +++++++++++----- internal/commands/connect_pings_test.go | 36 +++++++++++++++++++++++++ skills/basecamp-connect/SKILL.md | 10 +++---- 4 files changed, 61 insertions(+), 13 deletions(-) diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index bf5ae9ad6..2b759a044 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -216,9 +216,12 @@ func pingsCheck(ctx context.Context, p connectProfile) setup.Check { } } switch { - case v.Twice() && hears: + case v.Twice() && hears && len(v.Others) == 1: c.Status = setup.StatusWarn c.Message += ", and so does " + peerList(v.Others) + " on this computer: each Ping is answered twice" + case v.Twice() && hears: + c.Status = setup.StatusWarn + c.Message += ", and so do " + peerList(v.Others) + " on this computer: each Ping is answered more than once" case v.Twice(): c.Status = setup.StatusWarn c.Message += ", and " + peerList(v.Others) + " on this computer all do: each Ping is answered more than once" diff --git a/internal/commands/connect_pings.go b/internal/commands/connect_pings.go index 9f5bd5dee..8d9a3986a 100644 --- a/internal/commands/connect_pings.go +++ b/internal/commands/connect_pings.go @@ -278,20 +278,29 @@ func connectRunPings(held *connector.InstanceLock, profile string, file setup.Fi } // warnConnectPingsPeers says, as a connector that hears Pings starts, that -// another connector on this computer hears the same agent's Pings, so each is -// answered twice. One that hears none says what it leaves to whom. +// other connectors on this computer hear the same agent's Pings, in one +// warning: beside one, each is answered twice; beside two or more, more than +// once. One that hears none says what it leaves to whom. func warnConnectPingsPeers(logger *slog.Logger, pings connectPings, profile string, file setup.File) { if !pings.Hears { logger.Info("connector: hearing no Pings, and leaving them to the one connector that answers them; start with --pings to answer them here", "pings_because", pings.Describe()) return } - for _, peer := range connectPingsPeers(profile, file) { - msg := "connector: another connector on this computer hears this agent's Pings too, so each is answered twice; only one connector should answer Pings: restart one of them with --no-pings" - if peer.Older { - msg = "connector: an older connector on this computer may hear this agent's Pings too, so each would be answered twice; only one connector should answer Pings: upgrade it, or restart this one with --no-pings" + peers := connectPingsPeers(profile, file) + switch { + case len(peers) == 1 && peers[0].Older: + logger.Warn("connector: an older connector on this computer may hear this agent's Pings too, so each would be answered twice; only one connector should answer Pings: upgrade it, or restart this one with --no-pings", + "profile", richtext.SanitizeSingleLine(peers[0].Profile), "pid", peers[0].PID) + case len(peers) == 1: + logger.Warn("connector: another connector on this computer hears this agent's Pings too, so each is answered twice; only one connector should answer Pings: restart one of them with --no-pings", + "profile", richtext.SanitizeSingleLine(peers[0].Profile), "pid", peers[0].PID) + case len(peers) > 1: + msg := "connector: other connectors on this computer hear this agent's Pings too, so each is answered more than once; only one connector should answer Pings: restart all but one of them with --no-pings" + if slices.ContainsFunc(peers, func(p connectPingsPeer) bool { return p.Older }) { + msg = "connector: other connectors on this computer hear this agent's Pings too, or may, so each is answered more than once; only one connector should answer Pings: upgrade any older one, and restart all but one of them with --no-pings" } - logger.Warn(msg, "profile", richtext.SanitizeSingleLine(peer.Profile), "pid", peer.PID) + logger.Warn(msg, "others", peerList(peers)) } } diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index 05cedb9d1..22efb68ee 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -586,6 +586,42 @@ func TestGuidedSetupAlwaysSaysHowToRecoverAnUnreadableChoice(t *testing.T) { } } +// With this connector and two others on this computer hearing the agent's +// Pings, each is answered more than once, not twice: doctor says so, and so +// does the start that finds the other two, in one warning naming both, not +// one per connector. Beside a single other, it is twice. +func TestThreeConnectorsHearingPingsAnswerEachMoreThanOnce(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + setUpSecondProfile(t, f.s, "desk") + laptop := startProfileConnector(t, f.s, "laptop") + laptop.awaitRunning(t) + desk := startProfileConnector(t, f.s, "desk") + desk.awaitRunning(t) + assert.Contains(t, lineContaining(desk.out.String(), pingsWarning), "answered twice", "beside one other") + + agent := startProfileConnector(t, f.s, "agent") + agent.awaitRunning(t) + out := agent.out.String() + assert.Equal(t, 1, strings.Count(out, pingsWarning), "one warning, not one per connector:\n%s", out) + warning := lineContaining(out, pingsWarning) + assert.Contains(t, warning, "level=WARN") + assert.Contains(t, warning, "more than once") + assert.NotContains(t, warning, "twice") + assert.Contains(t, warning, "desk") + assert.Contains(t, warning, "laptop") + + doctor := f.doctorStyled(t) + assert.Contains(t, doctor, "each Ping is answered more than once") + assert.NotContains(t, doctor, "answered twice") + assert.Contains(t, doctor, `profile "desk"`) + assert.Contains(t, doctor, `profile "laptop"`) + + desk.stop(t) + assert.Contains(t, f.doctorStyled(t), `and so does profile "laptop"`) + assert.Contains(t, f.doctorStyled(t), "each Ping is answered twice", "this one and one other") +} + // A lock file left behind by a connector that has stopped, whose pid now // belongs to a process that started after it, describes nothing running. func TestStaleLockMetadataIsNoRunningConnector(t *testing.T) { diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index a8f735d8e..11019322b 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -379,11 +379,11 @@ them as unknown, or a start line without `pings=`, is older, so run `basecamp upgrade`, tell the person, and start it again. If the log warns `only one connector should answer Pings`, another connector -on this computer hears the same agent's Pings, and each is answered twice. -Tell the person, name the profile the warning names, and ask which should -answer them. Connectors on other computers can't be seen from here, so if the -person runs this agent elsewhere too, remind them once that only one computer -should answer Pings. +on this computer hears the same agent's Pings, and each is answered twice, or +more than once beside two or more. Tell the person, name the profiles the +warning names, and ask which should answer them. Connectors on other +computers can't be seen from here, so if the person runs this agent elsewhere +too, remind them once that only one computer should answer Pings. ## Invocation From bf1635431c0a38286e6cb20c2e3024949f0e782e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 21 Oct 2026 16:16:03 -0700 Subject: [PATCH 8/8] Point doctor at the others when this profile hears no Pings Beside two or more connectors that hear the agent's Pings, doctor on a profile that hears none warned, then offered to restart that same profile with --no-pings, which changes nothing. It now says this one doesn't answer them and to restart all but one of the others, upgrading any older one first, since an older build can't be told. --- internal/commands/connect_doctor.go | 18 +++++++--- internal/commands/connect_pings_test.go | 46 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 2b759a044..e33b7a041 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strconv" "time" @@ -237,11 +238,18 @@ func pingsCheck(ctx context.Context, p connectProfile) setup.Check { c.Status = setup.StatusPass return c } - c.Hint = "Only one connector should answer Pings. Start all but one of them with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" - for _, peer := range v.Others { - if peer.Older { - c.Hint = "Only one connector should answer Pings, and an older build can't be told: upgrade it, or start this one with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" - } + // A profile that hears none can only point at the others: restarting it + // with --no-pings changes nothing. + older := slices.ContainsFunc(v.Others, func(o connectPingsPeer) bool { return o.Older }) + switch { + case !hears && older: + c.Hint = "Only one connector should answer Pings, and this one doesn't: upgrade any older one, then restart all but one of the others with --no-pings: basecamp connect -P --no-pings" + case !hears: + c.Hint = "Only one connector should answer Pings, and this one doesn't: restart all but one of the others with --no-pings: basecamp connect -P --no-pings" + case older: + c.Hint = "Only one connector should answer Pings, and an older build can't be told: upgrade it, or start this one with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" + default: + c.Hint = "Only one connector should answer Pings. Start all but one of them with --no-pings: basecamp connect -P " + richtext.ShellQuote(p.name) + " --no-pings" } return c } diff --git a/internal/commands/connect_pings_test.go b/internal/commands/connect_pings_test.go index 22efb68ee..fd8fc32d0 100644 --- a/internal/commands/connect_pings_test.go +++ b/internal/commands/connect_pings_test.go @@ -622,6 +622,52 @@ func TestThreeConnectorsHearingPingsAnswerEachMoreThanOnce(t *testing.T) { assert.Contains(t, f.doctorStyled(t), "each Ping is answered twice", "this one and one other") } +// Doctor on a profile that hears no Pings, beside two others here that do, +// points at those two: restarting this one with --no-pings changes nothing. +// An older one among them is upgraded first, since it can't be told. +func TestDoctorBesideTwoAnswerersPointsAtThem(t *testing.T) { + for _, older := range []bool{false, true} { + t.Run(fmt.Sprintf("older=%t", older), func(t *testing.T) { + f := newOperatorFixture(t) + setUpSecondProfile(t, f.s, "laptop") + agent := startProfileConnector(t, f.s, "agent", "--no-pings") + agent.awaitRunning(t) + laptop := startProfileConnector(t, f.s, "laptop") + laptop.awaitRunning(t) + if older { + stateHome, err := connectStateHome() + require.NoError(t, err) + legacy, err := ensurePrivateChain(stateHome, "basecamp", "connect", connector.LegacyStateDirName(f.file.AccountID, f.file.Agent.PersonID)) + require.NoError(t, err) + lock, err := connector.AcquireInstanceLock(legacy, f.file.AccountID, f.file.Agent.PersonID, time.Now().Add(-time.Minute)) + require.NoError(t, err) + t.Cleanup(func() { _ = lock.Release() }) + meta, err := json.Marshal(map[string]any{"pid": os.Getpid(), "started_at": time.Now().Add(time.Minute).UTC().Format(time.RFC3339), + "account_id": f.file.AccountID, "agent_person_id": f.file.Agent.PersonID}) + require.NoError(t, err) + require.NoError(t, os.WriteFile(lock.Path()+".json", meta, 0o600), "as an older build writes it: nothing about Pings") + } else { + setUpSecondProfile(t, f.s, "desk") + desk := startProfileConnector(t, f.s, "desk") + desk.awaitRunning(t) + } + + doctor := f.doctorStyled(t) + require.Contains(t, doctor, "each Ping is answered more than once", doctor) + hint := lineContaining(doctor, "Only one connector should answer Pings") + assert.Contains(t, hint, "this one doesn't", doctor) + assert.Contains(t, hint, "restart all but one of the others with --no-pings") + assert.NotContains(t, hint, "basecamp connect -P agent", "this profile already leaves them to another") + assert.NotContains(t, hint, "start this one with --no-pings") + if older { + assert.Contains(t, hint, "upgrade any older one") + } else { + assert.NotContains(t, hint, "older") + } + }) + } +} + // A lock file left behind by a connector that has stopped, whose pid now // belongs to a process that started after it, describes nothing running. func TestStaleLockMetadataIsNoRunningConnector(t *testing.T) {