Repository navigation
Conversation
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.
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Startup can use or persist a stale Ping choice, and several diagnostic paths can misreport Ping handling.
5 open issues
What changed in this PR
Adds per-profile Ping handling so one connector can answer an agent’s Pings while others serve projects only.
Changes:
- Adds persisted
--pings/--no-pingschoices and admission/handoff controls. - Exposes Ping handling through startup logs, status, doctor, and local duplicate warnings.
- Updates setup guidance, documentation, and regression tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| skills/basecamp-connect/SKILL.md | Documents Ping ownership and troubleshooting. |
| README.md | Explains defaults and explicit choices. |
| internal/connector/setup/private_state.go | Adds private state-file reads and writes. |
| internal/connector/pings.go | Persists per-profile Ping choices. |
| internal/connector/pings_test.go | Tests persistence and lock metadata. |
| internal/connector/lock.go | Records whether holders hear Pings. |
| internal/connector/handoff.go | Gates Ping request handoff. |
| internal/connector/handoff_test.go | Tests Ping handoff restrictions. |
| internal/connector/admission/verdict.go | Discards Pings when disabled. |
| internal/connector/admission/policy.go | Adds runtime Ping policy. |
| internal/connector/admission/pings_heard_test.go | Tests Ping admission and project isolation. |
| internal/connector/admission/matrix.go | Adds the Ping discard reason. |
| internal/connector/admission/fakes_test.go | Enables Pings in existing fixtures. |
| internal/connector/admission/doc.go | Documents Ping admission rules. |
| internal/commands/connect.go | Updates connector help. |
| internal/commands/connect_setup_test.go | Supports added-connection credentials in fixtures. |
| internal/commands/connect_setup_guided.go | Tailors setup guidance to Ping handling. |
| internal/commands/connect_run.go | Wires flags, persistence, and runtime policy. |
| internal/commands/connect_run_log_test.go | Updates startup-log fixtures. |
| internal/commands/connect_pings.go | Resolves choices and detects local peers. |
| internal/commands/connect_pings_test.go | Tests startup, diagnostics, and shadow behavior. |
| internal/commands/connect_pings_resolve_test.go | Tests choice precedence and explanations. |
| internal/commands/connect_operator.go | Adds Ping details to status. |
| internal/commands/connect_doctor.go | Adds Ping diagnostics. |
| docs/agent-connections-rollout.md | Documents rollout and downgrade behavior. |
Review effort: Balanced
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03cf612a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
|
@codex review |
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b275a9ca0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
| if v.SavedErr != nil && !v.Running.Running { | ||
| return guidedPingsSummary{remedy: v.Remedy(name)} |
There was a problem hiding this comment.
Flagged for human review — this is the fourth round in a row with a finding about what guided setup says about Pings. Earlier ones covered the live connector, an unreadable choice and which connection answers; this one is a running connector beside an unreadable choice. Under this lane's rule, a third finding in one class goes to the operator rather than another patch. Recommendation: narrow guided setup's Pings text to two things and point to basecamp connect doctor for the rest. First, leave out "or ping it" when this connection won't hear Pings. Second, show the remove-then-choose remedy whenever the saved choice can't be read, whether or not a connector is running. That answers this thread, the matching Codex thread and the peers thread in one change.
| return guidedPingsSummary{remedy: v.Remedy(name)} | ||
| } | ||
| hears, known := v.Hears() | ||
| return guidedPingsSummary{hears: hears || !known} |
There was a problem hiding this comment.
Flagged for human review — this is the same class as the thread above: how much of doctor's Pings report guided setup should repeat. My recommendation is not to bring peers or conflicts into guided setup. A start that hears Pings already warns when a second connector on this computer hears them too, and status and doctor name every connector. The PR description claimed more for guided setup than that, and I've narrowed it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9224660bed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return guidedPingsSummary{remedy: v.Remedy(name)} | ||
| } |
There was a problem hiding this comment.
When guided setup is run while a connector is active and pings.json has become malformed or unsafe, this condition suppresses the recovery message and reports only the running connector's current behavior, potentially even suggesting that the user Ping it. Although the stopped-profile case is fixed, the new && !v.Running.Running branch still hides that the next ordinary start will refuse to run until the file is removed and the choice is made again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review — same finding as the Copilot thread on this line, and the same class: what guided setup says about Pings, now on its fourth round. Under this lane's rule it goes to the operator. Recommendation: show the remove-then-choose remedy whenever the saved choice can't be read, running or not, and keep guided setup's Pings text to that and whether to suggest a ping.
| 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() |
There was a problem hiding this comment.
When a connector is currently running but the profile has no saved choice and its credential cannot be loaded, Hears is known from the running lock while v.Next remains nil, so this value-receiver call panics before the following branch can use v.Running.Describe(). This can occur after starting with --project and then losing access to the credential store; connect doctor should report the credential failure and running Ping state rather than crash.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — in a6afc4a doctor describes a running connector from its lock before it looks at the next start's choice. That choice is now read only when nothing is running, where it's always known. TestDoctorReportsARunningConnectorWithoutACredential reproduced the crash: a narrowed first run with no saved choice and a deleted credential.
There was a problem hiding this comment.
When the inspected connector and two or more entries in v.Others all hear Pings, this branch reports that each Ping is answered twice even though it is answered at least three times. This is precisely the multi-connector fault the diagnostic is intended to quantify; use wording such as “more than once” here, or derive the count from len(v.Others)+1.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review — true: with this connector and two or more others hearing Pings, "twice" undercounts. The fix is one word. But this is the third round with a finding about how duplicate answerers are reported: first a profile's own legacy state, then the label on the sole answerer, now the count. Under this lane's rule that's the operator's call, together with the guided-setup threads. Recommendation: say "more than once" whenever two or more others hear Pings, in one push with the guided-setup narrowing if that's approved.
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.
Only one computer should answer an agent's Pings. A Ping is in no project, so serving projects can't divide Pings between connections the way it divides everything else: every connector for the agent sees all of them, and each one that admitted a line answered it. Once Basecamp adds connections (bc3 #13840), that is every Ping answered once per computer. This is the act-once rule for Pings, with no server change.
What a connector hears
--pings--no-pingsbcac_)--project--pings --projectis refusedEvery connector still serves the projects it's started with, as before.
--projectsaves the answer aspings.jsonbeside the profile's ledger (either flag saves its own), and restarts keep it until a flag changes it. The start decides it while holding its instance lock, so another start of the same profile can't override it in between. A flag given false means the other flag (--pings=falseis--no-pings). An unreadablepings.jsonrefuses the start, and every surface that meets one names it to remove before choosing again.pings=true|falseand why.connect status(styled and--json) andconnect doctor's Pings check read one view: the saved choice (always read, so a running connector can't hide an unreadable one), the connector running now from its lock (one an older build runs counts as "may hear"), and the other connectors on this computer for the same agent that hear Pings. They warn only when two would answer. The one answerer beside a connector that leaves Pings to it is named, not flagged. Guided setup uses the same view only to decide whether to suggest pinging the agent, and to say how to recover a saved choice that can't be read. How much more it should say is open for a decision; see the summary comment.pings_not_heardwhen the run doesn't hear Pings, after the one Ping read and before any other. The handoff hands off a Ping's request only when the run hears Pings, and never on a run narrowed by--project. A test holds that outside a served project, nothing but a Ping's line is ever admitted, so this flag decides everything there.connectandconnect setuphelp, README,docs/agent-connections-rollout.mdand the basecamp-connect skill say "only one computer should answer Pings". Guided setup on an added connection no longer suggests pinging the agent to try it, and says how to make this computer the one that answers. The skill's new section sits after the per-request steps, which were 6 characters inside the compaction budget.Calls I made that you may want to overrule
--pingswith--projectis refused instead of supported. A narrowed run's feed is filtered to its projects server-side, so it never receives a Ping. Supporting the pair would mean dropping that filter and separating the blocked-retry sweep's scope from the feed filter: a wider change to a guarded invariant, for a caseconnect setup --servealready covers. If you want it, it's a follow-up.bcac_one and quietly stop it answering Pings. The cost: a profile that started out as an added connection keeps not hearing Pings even if it later becomes the only one. That's fixed with--pings.pings.json, so a downgrade drops the choice and every connector hears Pings again, as before. The runbook says so.Tests
Tests came first and failed before the code.
pings_not_heard, spending one Ping read; projects are untouched; and the invariant that nothing outside a served project is admitted without Pings, checked for every matrix type.Pings, and never on a narrowed run.pings.json: round-trip, an older holder reads as unknown, and malformed, foreign-readable or symlinked files are refused.--project, the refusals, and flags given false. They cover the second-connector warning, and its absence for--no-pings, for a shadow and for a stopped connector. They also cover the shadow mirroring the profile, a legacy holder not being counted against its own profile, and status, doctor and guided setup reporting a narrowed run.bin/cipasses on Linux.The one thing these tests don't do is run a Ping through a live feed into
runConnect: the fake Basecamp here serves no account feed. The start line reads the decision from the admission policy itself, so the run tests catch a dropped wiring line, and the admission and handoff tests cover the behaviour.