Conversation
A running connector learned that its agent was disconnected in Basecamp, or connected on another computer, only from the event feed. The feed asks Basecamp for something only when it connects, and while its live connection stays up it asks for nothing. Admission and intake ask all the time, but to them a refusal was one failed read among others: admission blocked the mention it was deciding as read_failed, intake kept the projects it last read, and the connector ran on. Every mention addressed to the agent was blocked, and nothing said why, for as long as the socket held. Reproduced end to end against the fake Basecamp: disconnect the agent, or connect it on another computer and let its token come due, while the connector streams; the operator's next mention is blocked read_failed and the connector does not exit. A credential watch now sees the reads admission and intake make. A token renewal the token endpoint refuses is Basecamp's answer already, and stops the run at once. A read answered 401 is only a reason to ask, since something other than Basecamp can answer 401: the watch asks Basecamp itself, as the feed's own stop does, and stops the run only when it confirms. Either way the connector exits 3 with the words the feed's stop uses, and status records it as disconnected.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A synchronous confirmation can delay processing a definitive credential refusal.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds credential monitoring so a running connector stops when Basecamp rejects its agent credentials.
Changes:
- Watches API 401 responses and refused token renewals.
- Confirms ambiguous 401 responses before stopping.
- Adds focused credential-watch 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 |
|---|---|
internal/commands/connect_run.go |
Wires credential monitoring into connector reads and shutdown handling. |
internal/commands/connect_credential.go |
Implements token and transport credential monitoring. |
internal/commands/connect_credential_test.go |
Tests refusal, confirmation, and ignored-failure behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+69
to
+70
| if confirm(ctx) { | ||
| return errAgentCredentialNotTaken |
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/connect_credential_test.go">
<violation number="1" location="internal/commands/connect_credential_test.go:73">
P2: This only tests retries after a confirmation finishes, not coalescing while one is pending. Add a case that blocks `confirm`, sends multiple 401s, and asserts only one confirmation starts.</violation>
</file>
<file name="internal/commands/connect_credential.go">
<violation number="1" location="internal/commands/connect_credential.go:68">
P3: Running `confirm(ctx)` inline in the select blocks the loop for up to the confirm's 15s timeout, so a `ctx.Done()` or a freshly refused renewal arriving during the confirmation is not observed until it returns. The impact is bounded (a shutdown signal or a refuse arriving mid-confirm waits behind the network call), but this part's goroutine is what the connector's `wg.Wait()` and the second-signal "stop now" path wait on in connect_run.go. Consider restarting the select immediately after `confirm` returns, or making cancellation answerable while a confirmation is pending, so a refusal is not left buffered longer than necessary.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| for range 2 { | ||
| roundTrip(t, transport) | ||
| select { | ||
| case <-asks: |
Contributor
There was a problem hiding this comment.
P2: This only tests retries after a confirmation finishes, not coalescing while one is pending. Add a case that blocks confirm, sends multiple 401s, and asserts only one confirmation starts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_credential_test.go, line 73:
<comment>This only tests retries after a confirmation finishes, not coalescing while one is pending. Add a case that blocks `confirm`, sends multiple 401s, and asserts only one confirmation starts.</comment>
<file context>
@@ -0,0 +1,134 @@
+ for range 2 {
+ roundTrip(t, transport)
+ select {
+ case <-asks:
+ case <-time.After(10 * time.Second):
+ t.Fatal("the watch did not ask about a 401")
</file context>
| return nil | ||
| case err := <-w.refused: | ||
| return err | ||
| case <-w.suspect: |
Contributor
There was a problem hiding this comment.
P3: Running confirm(ctx) inline in the select blocks the loop for up to the confirm's 15s timeout, so a ctx.Done() or a freshly refused renewal arriving during the confirmation is not observed until it returns. The impact is bounded (a shutdown signal or a refuse arriving mid-confirm waits behind the network call), but this part's goroutine is what the connector's wg.Wait() and the second-signal "stop now" path wait on in connect_run.go. Consider restarting the select immediately after confirm returns, or making cancellation answerable while a confirmation is pending, so a refusal is not left buffered longer than necessary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_credential.go, line 68:
<comment>Running `confirm(ctx)` inline in the select blocks the loop for up to the confirm's 15s timeout, so a `ctx.Done()` or a freshly refused renewal arriving during the confirmation is not observed until it returns. The impact is bounded (a shutdown signal or a refuse arriving mid-confirm waits behind the network call), but this part's goroutine is what the connector's `wg.Wait()` and the second-signal "stop now" path wait on in connect_run.go. Consider restarting the select immediately after `confirm` returns, or making cancellation answerable while a confirmation is pending, so a refusal is not left buffered longer than necessary.</comment>
<file context>
@@ -0,0 +1,114 @@
+ return nil
+ case err := <-w.refused:
+ return err
+ case <-w.suspect:
+ if confirm(ctx) {
+ return errAgentCredentialNotTaken
</file context>
The credential watch only ever stopped an Agent: its confirm answered no for a bot user, so a bot whose login was revoked kept running and blocked every mention as read_failed, the failure the watch exists to end. A bot user's refresh the token endpoint refuses (invalid_grant) now carries auth.ErrLoginRefused, and the watch treats it as Basecamp's answer, as it does an Agent's refused mint. A 401 for a bot user is confirmed with a fresh token: the login is refreshed, a refused refresh is the answer, and so is a 401 for the fresh token. A rate-limited or failing refresh answers nothing. The exit says "<bot> is no longer signed in: Basecamp refused its login", exit 3, with the login that signs it in again under its pinned identity. Status records it as signed_out and leads with the same remedy. A start that meets a refused refresh says the same.
Comment on lines
+689
to
+690
| refused := m.errAuth(msg) | ||
| refused.Cause = ErrLoginRefused |
| return state, e.Message, e | ||
| } | ||
|
|
||
| // errCredentialRefused says Basecamp refused who's credential in the words |
Contributor
There was a problem hiding this comment.
1 existing issue remains and 3 new issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/connect_operator.go">
<violation number="1" location="internal/commands/connect_operator.go:252">
P2: `connect status --json` reports only `signed out`; the pinned identity and `botSignInHint` are available only to the styled renderer. Include the sign-in command in JSON-accessible output so non-TTY operators can recover the bot login.</violation>
</file>
<file name="internal/commands/connect_run.go">
<violation number="1" location="internal/commands/connect_run.go:530">
P2: `RefreshRefusal` can fail locally even when a refresh token exists, but this path then treats the old access token's 401 as proof the bot login was revoked. Return false for preflight failures when a refresh token exists; probe the current token only when no refresh token is stored.</violation>
</file>
<file name="internal/auth/auth.go">
<violation number="1" location="internal/auth/auth.go:690">
P2: Do not mark every Launchpad `invalid_grant` as a definitive login refusal; a client-ID/secret mismatch is recoverable and must not stop the connector with a misleading signed-out remedy.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| case connector.ConnectionDisconnected: | ||
| parts = append(parts, "disconnected") | ||
| case connector.ConnectionSignedOut: | ||
| parts = append(parts, "signed out") |
Contributor
There was a problem hiding this comment.
P2: connect status --json reports only signed out; the pinned identity and botSignInHint are available only to the styled renderer. Include the sign-in command in JSON-accessible output so non-TTY operators can recover the bot login.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_operator.go, line 252:
<comment>`connect status --json` reports only `signed out`; the pinned identity and `botSignInHint` are available only to the styled renderer. Include the sign-in command in JSON-accessible output so non-TTY operators can recover the bot login.</comment>
<file context>
@@ -241,8 +244,13 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error {
+ case connector.ConnectionDisconnected:
+ parts = append(parts, "disconnected")
+ case connector.ConnectionSignedOut:
+ parts = append(parts, "signed out")
+ }
}
</file context>
Suggested change
| parts = append(parts, "signed out") | |
| parts = append(parts, "signed out; "+botSignInHint(r.name(), r.identity)) |
Comment on lines
+530
to
+534
| if app.Auth.RefreshRefusal(creds) == nil { | ||
| if err := app.Auth.Refresh(ctx); err != nil { | ||
| return errors.Is(err, auth.ErrLoginRefused) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
P2: RefreshRefusal can fail locally even when a refresh token exists, but this path then treats the old access token's 401 as proof the bot login was revoked. Return false for preflight failures when a refresh token exists; probe the current token only when no refresh token is stored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_run.go, line 530:
<comment>`RefreshRefusal` can fail locally even when a refresh token exists, but this path then treats the old access token's 401 as proof the bot login was revoked. Return false for preflight failures when a refresh token exists; probe the current token only when no refresh token is stored.</comment>
<file context>
@@ -468,19 +492,60 @@ func agentDisconnectedAtStart(kind string, err error) bool {
+ if err != nil {
+ return false
+ }
+ if app.Auth.RefreshRefusal(creds) == nil {
+ if err := app.Auth.Refresh(ctx); err != nil {
+ return errors.Is(err, auth.ErrLoginRefused)
</file context>
Suggested change
| if app.Auth.RefreshRefusal(creds) == nil { | |
| if err := app.Auth.Refresh(ctx); err != nil { | |
| return errors.Is(err, auth.ErrLoginRefused) | |
| } | |
| } | |
| if creds.RefreshToken != "" { | |
| if app.Auth.RefreshRefusal(creds) != nil { | |
| return false | |
| } | |
| if err := app.Auth.Refresh(ctx); err != nil { | |
| return errors.Is(err, auth.ErrLoginRefused) | |
| } | |
| } |
| } | ||
| return m.errAuth(msg) | ||
| refused := m.errAuth(msg) | ||
| refused.Cause = ErrLoginRefused |
Contributor
There was a problem hiding this comment.
P2: Do not mark every Launchpad invalid_grant as a definitive login refusal; a client-ID/secret mismatch is recoverable and must not stop the connector with a misleading signed-out remedy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/auth/auth.go, line 690:
<comment>Do not mark every Launchpad `invalid_grant` as a definitive login refusal; a client-ID/secret mismatch is recoverable and must not stop the connector with a misleading signed-out remedy.</comment>
<file context>
@@ -679,7 +686,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C
}
- return m.errAuth(msg)
+ refused := m.errAuth(msg)
+ refused.Cause = ErrLoginRefused
+ return refused
}
</file context>
Suggested change
| refused.Cause = ErrLoginRefused | |
| if creds.OAuthType != "launchpad" { | |
| refused.Cause = ErrLoginRefused | |
| } |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure
A running
basecamp connectdoes not stop when Basecamp stops taking its agent's credential. This happens when an Agent is disconnected in Basecamp, or connected on another computer (which rotates its secret), and when a bot user's login is revoked or expires.read_failedand carries on.Found by the end-to-end scenarios in #826 (disconnect, then mention the agent).
The fix
A credential watch (
internal/commands/connect_credential.go), wired intorunConnect, sees the reads that admission and intake make:invalid_client,auth.ErrAgentCredentialRefused). For a bot user it is a refused refresh (invalid_grant), which now carries a new cause,auth.ErrLoginRefused.confirmCredentialRefused) and stops the run only when Basecamp confirms. Signals that arrive while a check is pending coalesce.agentCredentialRefused).basecamp connect setup -P <profile>. Status recordsdisconnected.basecamp auth login -P <profile> --expect-identity <id>. Status records a new state,signed_out, and leads with the same hint.Tests
internal/commands/connect_credential_test.go:internal/commands/connect_run_disconnect_test.go:connectorStoppedBywording, hint and status state for an Agent and for a bot user, for every way the refusal is met; a refused bot refresh at start; status leads with "Signed out" for a bot user.internal/auth/auth_test.go: aninvalid_grantrefresh carriesauth.ErrLoginRefused;invalid_request, 429 and 503 do not.End to end, in Test how basecamp connect fails and recovers, end to end #826 (
e2e/connect/failures_test.go), for an Agent:TestAnAgentDisconnectedWhileRunningStopsTheConnector: disconnect, then a read gets 401. This proves the watch's 401 wiring.TestAnAgentDisconnectedWhileRunningStopsTheConnectorOnReconnect: disconnect, then the feed's reconnect is refused.TestAnAgentConnectedElsewhereStopsTheConnectorAtItsNextRenewal: connected on another computer; the next renewal getsinvalid_client.Each exits 3 with the message. No end-to-end scenario covers the bot-user path yet.
This PR is independent of the end-to-end stack. #826 carries the first commit of this PR (not the bot-user commit), which drops out on rebase once this merges.
Summary by cubic
Stops a running
basecamp connectwhen Basecamp no longer takes its agent's credential - the agent was disconnected in Basecamp, or connected on another computer, which rotates its secret. Previously only the event feed checked for this, but it asks for nothing while its live WebSocket stays up, so the connector ran on blocking every mention silently.A credential watch now sees the reads admission and intake make:
invalid_client) is already Basecamp's answer and stops the run immediately.The start's own reads aren't watched; a refusal there already ends the start.
Written for commit bfd1e36. Summary will update on new commits.