Repository navigation
Conversation
github-actions
Bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
An agent profile's shared token is cached and served to every process on the host, so Basecamp can't tell one sandboxed launch from another. With --session-id (and optionally --session-label), auth token --stored mints a fresh self-token for that session, sending launch_id and launch_label on the client_credentials request, and prints it without caching it or touching the shared one. The token is bound to the session or not handed out: a server that refuses the parameters, or answers without echoing them back because it predates session attribution, fails the command and the unbound token is discarded. Holds on the client secret are honored and written as the shared mint's are; a refusal of the session's own parameters holds nothing.
A launcher that passes --session-id "$ID" with ID unset got the profile's shared, unattributed token, because session mode was decided by the flag's value. It is now decided by whether the flag was given, so an empty id is a usage error like any other malformed one.
bc3 refuses exactly control, format, private-use, surrogate and line or paragraph separator characters in a launch label, and no longer refuses unassigned code points: a host's Unicode tables can be newer than the server's, and a refused label costs the launch its whole token. The local check now refuses the same set, named category by category, since Go's unicode.C table also covers code points it doesn't know.
The mock echoes the launch id back whatever the request sent, so a second mint that dropped it would have passed. Each recorded mint is now checked.
#854 renamed MintHold and its helpers to RenewalHold after this branch was cut, so the session mint and its tests named things that no longer exist.
2b91776 to
c56bff3
Compare
|
@codex review |
|
@cubic-dev-ai review |
@jeremy I have started the AI code review. It will take a few minutes to complete. |
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. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…s sessions A server that echoed this session's id but a different label, or none, was still told it predates agent session attribution, which its own echo contradicts. Only a missing id says that now; an id for another session and a label that came back wrong or not at all each say so.
|
@codex review |
|
@cubic-dev-ai review |
@jeremy I have started the AI code review. It will take a few minutes to complete. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
Rebased onto main (943dbb5). Git found no textual conflicts, but the branch no longer built: #854 renamed Review threads: 3 resolved (2 fixed, 1 declined with the reasoning in its thread).
Declined:
CI is green on 40a798b. Codex reported on 40a798b with no major issues. cubic's review of that head raised only the declined finding. Copilot can't review because the requester's quota is exhausted. |
|
Parked with bc3 #13710, behind Rob's agent-connections plan, which defers per-launch attribution (card https://app.basecamp.com/2914079/buckets/48699913/card_tables/cards/10311028084). If revived, release this only after bc3 #13710 is deployed. This PR refuses to mint unless Basecamp records the session, and agent-sandbox-nono's check of the CLI's flags can't tell a server without #13710, so an early release would cut off its data-tier boxes again. |
Basecamp stores the launch id and label on the token, and its request lines carry only the id beside the agent (bc3 #13710 filters the label from logged parameters). The comment said the label went into the logs.
A session mint holds no credential lock across its round trip, so another launch's verdict on the same secret can be written while it is in flight. The last write won: an invalid_client refusal, followed by the 429 the abuse tracker answers once those refusals trip it, left a one-minute hold in place of a permanent one, and the dead secret went out again after it. A shorter 429 could likewise cut short a longer one. rememberRenewalHold now keeps a stored hold that still holds and outlasts the new one, and returns it, so the session mint answers with the verdict that stood and tells its caller the wait the next mint will meet.
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive token attribution and concurrent hold handling warrant human confirmation against the server-side contract, which was unavailable to reviewers.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
|
Review threads: 1 resolved (1 fixed, 0 declined).
Not reported on the head:
The branch merges cleanly into main at e2d5b2a (v0.17.0), so I didn't rebase it. I checked the contract against bc3 #13710 as merged (5d78ae75). bc3 accepts |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Why
An agent profile's shared token is cached per profile and served to every process on the host. Run a dozen sandboxed boxes and the connector on one computer, and Basecamp sees one token: every request reads as the agent, with nothing saying which launch made it.
What
basecamp auth token --stored --session-id <id> [--session-label <label>]mints a fresh self-token for that one session. The id and label ride theclient_credentialsrequest aslaunch_idandlaunch_label. bc3 stores them on the token and logs the id beside the agent. bc3 can't use "session" for this, because there it means web sign-in. The CLI keeps "session", the word its callers use.--stored, is a usage error. A session token never comes fromBASECAMP_TOKEN.--jsonaddssession_idandsession_labelbesidetoken.Decisions worth a look
Not cached. Each call mints. The shared cached token is neither served nor written. The caller is the per-launch cache: the sandbox's nono
cmd://capture holds each token for 240 s inside the launch's own proxy, which dies with the launch. Caching here would mean one keyring entry per launch that nothing ever cleans up. It would also make every launch contend on the profile's one credential lock for a token only that launch may use. A capture every 240 s is about 15 mints an hour per box, well inside the token endpoint's per-client budget.Fail closed: a token bound to the session, or none.
oauthErrorCodes), it doesn't repeaterror_description, because the request carried the client secret.launch_id(or echoes different values). The command exits non-zero with "this Basecamp predates agent session attribution", and the minted token is discarded: not printed, not cached.Holds. A session mint presents the same client secret the shared mint does. So it honors a stored hold before sending anything, and it remembers a refusal of the secret (
invalid_client,invalid_grant, a bare 401/403, a 429), so a dozen launches don't keep presenting a dead secret. It takes the credential key's lock only to write a hold; nothing is written on success.A refusal of the request itself, such as
invalid_requestfor a malformed launch parameter, holds nothing, so one bad launch can't stop the others. Per-(credential, session) holds, for a future "session stopped" refusal, belong with that step and aren't here.Tests
internal/auth/agent_launch_test.go, red then green. It covers:invalid_requestrefusal, which names the session and holds nothing;invalid_clientrefusal, which is held, so the next session mint sends nothing;internal/commands/auth_token_session_test.go: the command's JSON shape with and without a session, and the flag-combination usage errors..surfaceupdated for the two flags.--session-id ""(a launcher's unset variable) is a usage error, not a fall-back to the shared token. Session mode is decided by whether the flag was given, not by its value. Red then green inTestAuthTokenSessionFlagUsage.bin/ci: green.Review round
Copilot couldn't review (quota). My own adversarial pass found one real issue, fixed in the second commit:
--session-id "$ID"withIDunset fell back to the shared token. It also turned up two points I'm leaving as they are:Overlap with open PRs
git merge-treeis clean against Name a personal agent's boss in me #850, Tell an agent's owner to disconnect this connection, not rotate its secret #851, Read the agent connection ceremony as data with --json #852, Hold a rate-limited refresh, explain a blocked device login, and read invalid_grant by its code #854 and Say why a revoked login was revoked, and warn containers off copying one #855.MintHold→RenewalHold,rememberMintHold→rememberRenewalHold). Merged together, the tree doesn't compile until this PR's one call site and its tests take the new names. That's a mechanical fix for whichever lands second, so this is based onmainrather than stacked.Related
Step 1 of the multi-session proposal; draft for discussion with the agents team.
launch_id/launch_label, logs the id)