[robzolkos]

Summary

Stop the connector when Basecamp confirms that its credential is no longer accepted, even when the live connection remains open. Previously admission or intake could keep failing reads while the connector appeared to be running.

• Watch token-provider refusals and unauthorized requests, including handoff acknowledgements and loss-repair polling.
• Retain an observed refusal across concurrent shutdown, so another read finding the deleted credential cannot replace signed-out status with a generic stop.
• Stop immediately for a definitive agent renewal or BC5 refresh refusal. An invalid_grant body on a rate limit or server fault is not a definitive refusal; only HTTP 400/401 qualifies. Launchpad's ambiguous client-mismatch error is not treated as proven revocation.
• Confirm a 401 before stopping. Keep at most one cancellable confirmation in flight, without delaying definitive refusals. On shutdown, cancel and finish any still-pending confirmation before recording the final reason.
• Keep running after temporary refresh holds, network/server failures, and local preflight/configuration errors.
• Preserve profile-specific recovery advice for agents and bot users, including the recovery command in JSON status summaries.

Verification

• Full local bin/ci passes on b6a0424e3f7c445eb7cda4e94f039455bf1a3e85, rebased onto the merged newer-credential/client-refusal handling.
• Focused credential and auth regressions pass over 20 race-enabled runs on the final head. Shutdown, repair, boost, and confirmation checks also pass over 30 repeated race-enabled runs.
• Before/after tests reproduce the delayed definitive refusal, duplicate pending confirmation, repeated 401 during a refresh hold, local preflight failure misdiagnosis, and ambiguous Launchpad classification. They fail on the previous head and pass with the corrections.
• Existing tests exercise both credential kinds, confirmed and inconclusive 401s, rate limits/server faults, token-only logins, stop status, and recovery wording. An SDK boost test verifies that a renewal refusal stays visible to the watch even when the acknowledgement caller only logs its error.
• No new end-to-end harness or fake Basecamp. The old stacked testing PR is closed; this fix and its focused tests stand alone.

A proxy that refuses every authenticated request can also refuse the confirmation request. Stopping in that case is correct, but the message cannot distinguish the proxy from a revoked credential. No fresh live-account revocation test is claimed.

[Copilot]

Copilot review overview

🟡 Changes recommended

A synchronous confirmation can delay processing a definitive credential refusal.

Review effort: Balanced
Findings: 1

Open (1)

• Make confirmation cancellable to promptly handle watch refusal · New

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 run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to 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.

[cubic-dev-ai]

All reported issues were addressed across 3 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Copilot]

Copilot review overview

🟡 Changes recommended

Launchpad client mismatches are incorrectly classified as definitively expired logins, producing misleading shutdown guidance.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Preserve invalid_grant ambiguity for Launchpad credentials · New
• Make confirmation cancellable to promptly handle watch refusal
• Use “whose” instead of “who’s” · New

[cubic-dev-ai]

All reported issues were addressed across 10 files (changes from recent commits).

<sub>Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.

Re-trigger cubic</sub>

[Copilot]

🟡 Changes recommended

Refresh holds can falsely sign out a bot, and unwatched handoff renewals can leave a revoked connector running.

5 open issues

• Use watched token provider for SDKBooster handoff · New
• Avoid sign-out after rate-limited token refresh · New
• Preserve invalid_grant ambiguity for Launchpad credentials
• Make confirmation cancellable to promptly handle watch refusal
• Use “whose” instead of “who’s”

_{Review effort: Balanced}

[robzolkos]

Rebased and corrected the reviewed failure paths. The confirmation check is cancellable and does not delay definitive refusals; concurrent 401s share one check. Temporary refresh holds, local preflight failures, and ambiguous Launchpad refusals are not reported as proven revocations. The handoff booster now uses the watched client, and JSON status summaries include the pinned recovery command. Focused race regressions pass over 20 runs, including before/after failures for the new cases, and full local CI passes. Current head: d8927f1. No new harness.

[Copilot]

🔵 Needs a closer look

Concurrent renewal and shutdown behavior needs human validation, and terminal local credential failures remain unhandled by the watch.

1 open issue

• Use “whose” instead of “who’s”

1 previously missed issue

In code that hasn't changed since last review

Stop connector on terminal bot-token authentication failures

internal/​commands/​connect_credential.go:126

The watch misses terminal bot-token failures when another CLI process has already deleted a refused login, or an imported token with a recorded expiry reaches the refresh window without a refresh token. These token lookups return local auth errors without either refusal cause and send no HTTP request, so neither watch signal fires and the connector can keep silently blocking mentions while its socket stays up. Preserve distinguishable causes for these terminal states and stop the running connector with an auth error—not an unconfirmed Basecamp-refusal message—and add regression tests for both cases.

4 resolved since last review

• Avoid sign-out after rate-limited token refresh
• Use watched token provider for SDKBooster handoff
• Preserve invalid_grant ambiguity for Launchpad credentials
• Make confirmation cancellable to promptly handle watch refusal

_{Review effort: Balanced}

[robzolkos]

Rebased again onto the newly merged auth change in #959. Current head: 0bce547. Kept its newer-credential and refused-client handling intact, reran the credential and auth regressions (including the new main cases), and reran full local bin/ci successfully. Hosted checks are refreshed; earlier review-fix explanations still apply.

[Copilot]

🟡 Changes recommended

Loss-repair polls bypass credential monitoring and can discard a definitive refusal without triggering shutdown or signed-out recovery status.

1 open issue

• Route repair poll auth refusals through the credential watch · New

1 resolved since last review

• Use “whose” instead of “who’s”

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

A shutdown race can discard an observed definitive refusal, losing the signed-out status and its recovery guidance.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Preserve refused BC5 refresh status during concurrent cancellation

internal/​commands/​connect_run.go:464

A refused BC5 refresh deletes the credential, so a concurrent feed read can fail with a missing-credential error and cancel the run before the watch reports the definitive refusal. The watch can then select cancellation, or runPart can discard its refusal because runCtx is canceled, leaving status as stopped rather than signed_out and losing the pinned-login recovery hint. Retain the observed refusal independently of cancellation and prioritize it when choosing the final error, with a regression covering this shutdown ordering.

1 resolved since last review

• Route repair poll auth refusals through the credential watch

_{Review effort: Balanced}

[robzolkos]

Fixed the review-body shutdown-ordering finding in 386b968. The watch retains the first observed refusal independently of its wake-up channel. After all run parts finish, that retained refusal takes priority over a concurrent missing-credential stop, preserving signed-out status and the pinned recovery command. Explicit operator signals retain their existing exit handling. A regression models the refused refresh deleting the credential before another read cancels the run, and a control preserves ordinary stops when no refusal was observed. The shutdown, repair, boost, and confirmation checks pass over 30 race-enabled runs; full local bin/ci passes. Hosted checks are refreshed.

[Copilot]

🟡 Changes recommended

Server faults can be classified as definitive refusals, and a shutdown race can lose a confirmed refusal before status is recorded.

1 open issue

• Only classify invalid_grant as sign-out on 400/401 responses · New

1 previously missed issue

In code that hasn't changed since last review

Synchronize refusal publication with final shutdown snapshot

internal/​commands/​connect_credential.go:80

If confirmation deletes a refused BC5 credential, a concurrent feed read can find the credential missing and cancel the run before confirmation publishes its refusal. This return does not join the confirmation goroutine, so the final watch.finalError(firstErr) can miss the refusal and omit the signed-out state and recovery hint. Coordinate confirmation publication with the final shutdown snapshot while preserving prompt cancellation, and add a regression that pauses between credential deletion and refusal publication.

_{Review effort: Balanced}

[robzolkos]

The review-body confirmation-publication race is also fixed in b6a0424. On cancellation with no already-known refusal, the watch cancels and joins its in-flight confirmation before taking the final error snapshot. A known definitive refusal still returns immediately, so it is not delayed behind confirmation. The production confirmation honors its context and already has a timeout. A deterministic regression pauses after the modeled credential deletion but before publication, cancels another part, and verifies that the final signed-out cause is retained; a cooperative-cancellation control verifies prompt termination. These tests and the credential regressions pass over 20 race-enabled runs; full local CI passes. New head: b6a0424.

[Copilot]

🟢 Approved

Static review found no unresolved blocking issues, and focused regressions cover refusal classification, shutdown coordination, and recovery reporting.

No open issues 🎉

1 resolved since last review

• Only classify invalid_grant as sign-out on 400/401 responses

_{Review effort: Balanced}

[robzolkos]

Ready for review on b6a0424: local CI passes and all 32 hosted checks are accounted for (27 successful, 4 conditionally skipped, 1 neutral; none failed or pending). Copilot approved this exact head. There are 0 unresolved review threads and 0 unanswered findings. Cubic's fresh review is unavailable because its monthly quota is exhausted. Nothing merged. The cancellation/publication and server-fault findings in earlier review bodies are fixed and verified. An earlier head hit the unchanged real-time rate-limiter test flake; the final run passes all four hosted race shards and their aggregate without changing that unrelated test.

[jeremy]

🤖 Local review at b6a0424. CI is green on it (28 passed, 4 skipped, 1 neutral). One finding.

The live feed's client drops the credential watch's transport, so a 401 on a repair poll never reaches the watch. runConnect passes watchedOpts, which end in WithTransportWrapper(watch) (connect_run.go L355), to eventfeed.NewLive (L363). NewLive then appends its own redirect guard with WithTransportWrapper (live.go L134). The client has a single wrapper slot, and "a later option replaces an earlier one" (client.go L175-L181), so the guard replaces the watch on that client. db85e13 keeps only the token half. A refused renewal still reaches the watch through watch.Tokens, and that's the case TestARepairPollRetainsARefusedRenewalForTheCredentialWatch covers. A credential revoked in Basecamp while its cached access token is still unexpired is different: the repair poll gets a 401, and repairWalker logs "retrying on the repair cadence" (repair.go L482-L486) without the watch ever being asked to confirm. Feed mints and polls go through the same client, so they miss it too.

What I'd change: build live with the watch under the transport, e.g. basecamp.WithTransport(watch.WrapTransport(http.DefaultTransport)) in place of the wrapper option, so the SDK's guard composes over it. Then add a variant of that test with an unexpired token and a poll that answers 401, asserting the watch asks for confirmation. I ran that probe against this head. Built as it is, the 401 never reaches the watch. With WithTransport(watch.WrapTransport(...)) it does.

The local review ran go test ./internal/auth ./internal/commands ./internal/connector and the targeted credential-watch tests. auth and connector passed. The ./internal/commands failures are terminal-interactivity tests that fail the same way on main outside a TTY, so they're unrelated.