Repository navigation
feat(minimal): log out deletes the sign-in and refuses every reference to it - #2512
norrietaylor merged 5 commits into
Conversation
The logout deletes the sign-in item from the host keychain. The proxy reads the item for each request, so every box's reference to it is refused at its next request. The logout then asks GitHub to delete the App's grant. A refused or unreachable revocation is reported on stderr and does not fail the logout. A sign-in after the logout is a new instance, so it revives no reference minted before it.
Add the bep_logout_refuses_every_reference_at_next_request case. Two grant boxes answer gh api user, min auth logout runs, and both boxes then fail gh api user while they keep running. min box audit shows a refusal marked github_sign_in_required for each box. The case signs the machine out, so it runs only with E2E_BEP_LOGOUT=1, and it runs after every other live proxy case.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: no findings; approval withheld, 1 thread unresolved at APPROVE withheld: 1 unresolved thread — minimal-arch[bot] scripts/session-e2e.sh:12026. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Clobber: none seen. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The logout matches §6.10 line 839 in shape. It deletes the item first, refuses every reference through the per-request read, and calls GitHub best effort. Two conditions remain. C2: the delete read-back discards a read error and reports success. Line 839 makes local deletion the enforcement, so an unverified delete must fail. C1: the BEP-044 e2e leg is not_run unless E2E_BEP_LOGOUT=1, and no run is recorded. A not_run is not a pass. The grant endpoint revokes the App for the account on every host, but line 839 calls logout host-wide. A person must decide that question in the escalation.
Ruled at f359768 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 1 of 10 on a failed check suite: the coder changed nothing. Noop. The two failed checks do not come from this pull request. cargo-deny fails on a new RustSec advisory ('Multiple timing variability issues in operations intended to be constant-time') against ecdsa/primeorder/rfc6979, which the tree reaches through russh 0.64.1. This branch does not change Cargo.lock or deny.toml. It changes only crates/minimal/src/cli.rs, crates/minimal/src/cmd/auth.rs, crates/minimal/src/cmd/mod.rs and scripts/session-e2e.sh. ci-success fails only because it aggregates the cargo-deny failure. The fix is a dependency bump or a deny.toml ignore on main, and both files are outside this task's layers. When main has that fix, a merge of main into this branch clears the checks. Not run here: ./scripts/session-e2e.sh bep_logout_refuses_every_reference_at_next_request. It needs a KVM host and a GitHub sign-in, and no code changed this round. |
|
/revise Fix arch condition C2 and the two minimal-review findings at f359768. The arch escalation on the revocation endpoint (thread 4366102522) is with the owner; do not change the endpoint in this round. C1 (the unrun e2e leg) is held on #2500.
Keep the cli.rs variant, the mod.rs dispatch lines and the e2e case as they are. Generated by Claude Code |
The host keychain's read-back after a delete now propagates a read error, so a logout whose deletion cannot be confirmed fails. The logout reads the token before any delete. On Linux it clears the metadata item before the token item. When a delete stops after the token item is gone, the logout still asks GitHub to revoke the token and says what is left and how to remove it. The not-confirmed line gives each fact its own sentence and names one action.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 3 unresolved threads — minimal-foundry[bot] crates/minimal/src/cmd/auth.rs, minimal-arch[bot] crates/minimal/src/cmd/auth.rs, minimal-arch[bot] crates/minimal/src/cmd/auth.rs:308. 1 finding, 1 inline.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare makes min auth logout fail closed. A read-back that fails is now an error. On Linux the metadata item goes first, and the redeemer needs that item, so every reference is refused (§6.10 line 839). BEP-044 conforms in code. The live e2e case still reports NOT RUN, and an open leg is not a pass. C1 keeps it open under #2500 until the macOS lane runs it.
Ruled at d543fc1 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Revise round 1 ran against |
|
Round 2 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
After Notes from the agent: This round changes only crates/minimal/src/cmd/auth.rs, as the directive asks. I did not run ./scripts/session-e2e.sh bep_logout_refuses_every_reference_at_next_request. It needs a live GitHub sign-in and a VM, and the directive holds condition C1 on #2500. The e2e case is unchanged. It still matches because the new stderr line starts with "GitHub did not confirm the sign-out." The task's diagnostics line pins the old NOT_CONFIRMED text. The directive replaces that text, so the diagnostics line and the spec must follow. The new text is: "GitHub did not confirm the sign-out. The sign-in is removed from this machine. To be sure GitHub revoked it, revoke the app at https://github.com/settings/apps/authorizations." I did not check that URL against a live GitHub account. This round adds three error texts: NOT_KNOWN (the keychain could not be read after a failed delete, then run |
|
/revise The owner ruled on the escalation in thread 4366102522 (BEP-044, In
Do not touch the keychain delete order or the Generated by Claude Code |
Per the owner's ruling, use GitHub's per-token endpoint,
DELETE /applications/{client_id}/token, in place of the grant endpoint.
Logout is host-wide: it revokes this host's token and its refresh token,
and the App's grant stays, so the account's other hosts stay signed in.
The tests now pin the token path.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The new head changes only the GitHub revocation call. It now revokes this host's token, not the App's grant. Section 6.10 line 839 asks the logout to revoke the sign-in, and it calls logout host-wide, so this change conforms. The rest of the ruling on d543fc1 stands. C1 stays open: the e2e leg is NOT RUN, and #2500 owns its live run.
Ruled at 39c7f6b against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 2 unresolved threads — minimal-arch[bot] crates/minimal/src/cmd/auth.rs, minimal-arch[bot] scripts/session-e2e.sh:12021. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
Revise round 1 ran against |
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 39c7f6b0293c withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
|
Round 3 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. After Notes from the agent: Revise round 3: revoke() now calls the per-token endpoint DELETE /applications/{CLIENT_ID}/token. The method, basic auth, access_token body, 15 s timeout and 204 check did not change. The doc comment and the two fake-GitHub test paths now say token. NOT_CONFIRMED, the keychain delete order and the partly-deleted handling did not change. The PR body is written by the runner: put in the Proof that revocation is per token (host-wide), per the owner's ruling on thread 4366102522. I did not run ./scripts/session-e2e.sh bep_logout_refuses_every_reference_at_next_request because this box has no /dev/kvm. The e2e script did not change this round. Failed checks: cargo-deny fails on a RUSTSEC timing advisory in the ecdsa/primeorder chain under russh 0.64.1. It also warns that RUSTSEC-2026-0009 in deny.toml is not encountered. Both are outside this task's layers and are not caused by this diff. ci-success is the aggregator that reports that failure. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The merge from main changes no file of this change. The ruling on 39c7f6b stands. The logout deletes the sign-in, reads the keychain back, and revokes at GitHub best effort, as section 6.10 line 839 requires. C1 stays open: the e2e leg records NOT RUN, and #2500 owns the live run. The change can merge with C1 open.
Ruled at 7dee522 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter approval at 7dee522: every merge gate passes at this head (arch PASS, zero open threads, every lane aggregator green, bot resolutions re-verified). Squash title feat(minimal): log out deletes the sign-in and refuses every reference to it and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
T21 — Log out: delete the sign-in and refuse every reference to it
slice: S1
kind: task
covers: BEP-044
size: M
est: 220 code, 220 tests
executor: agent
difficulty: needs care
model: standard
diagnostics:
min auth logoutprints "Signed out of GitHub. Boxes created before now lose GitHub access at their next request." and, when GitHub cannot be reached, "GitHub did not confirm the sign-out; the sign-in is removed from this machine." on stderr, pinned by logout_revokes_at_github_best_effortobservability: one info log line per logout naming the account and whether GitHub confirmed the revocation
layers:
- cli:
- crates/minimal/src/cli.rs:
min auth logout- crates/minimal/src/cmd/auth.rs: deletes the item, asks GitHub to revoke best effort, reports either outcome without failing
test: cargo nextest run -p minimal logout_revokes_at_github_best_effort
test: cargo nextest run -p minimal sign_in_after_logout_revives_no_earlier_reference
- e2e:
- scripts/session-e2e.sh: adds the case
test: ./scripts/session-e2e.sh bep_logout_refuses_every_reference_at_next_request
produces: none
consumes: none
proof: After
min auth logout, every running box's next GitHub request is refused, and signing in again revives none of them.depends on:
- blocked by #2276
Proof
A person can run
min auth logout. It deletes the GitHub sign-in from the host keychain and prints "Signed out of GitHub. Boxes created before now lose GitHub access at their next request." It then asks GitHub to revoke this host's token, so the account's other hosts stay signed in (the per-token endpoint, the owner's ruling on the escalation thread). When GitHub refuses or cannot be reached, it also prints "GitHub did not confirm the sign-out; the sign-in is removed from this machine." on stderr, and the logout still succeeds. The test sign_in_after_logout_revives_no_earlier_reference runs the proxy's real redeemer against a shared keychain. In that test, references for two boxes resolve before the logout and are refused after it. After a newmin auth loginthey are still refused, and only a reference minted after the new sign-in resolves. The e2e case shows the same on running boxes, but no lane has run it yet (see notes).Notes from the agent: Outside the layers: crates/minimal/src/cmd/mod.rs. I added one match arm (
AuthCommand::Logout) and one re-export there. Without them the newcli.rsvariant does not compile, because themin authdispatch lives in that file.The keychain delete is in auth.rs, not in
bep::signin.bep::signin::Keychainhas no delete, and the bep crate is outside the layers. auth.rs adds a localSignInItemtrait for HostKeychain and MemoryKeychain. It runssecurity delete-generic-passwordon macOS andsecret-tool clearon the token item, then the metadata item, elsewhere. After the delete it reads the item back. A later task can move this intobep::signinbesidewrite.Revocation: the logout sends
DELETE https://api.github.com/applications/<client_id>/tokenwith basic auth under the embedded client secret. The per-token endpoint revokes this host's token and its refresh token and leaves the App's grant for the account in place, so other hosts stay signed in (section 6.10 line 839 calls logout host-wide; the owner ruled this on the escalation thread, round 3). A build with no embedded secret cannot ask GitHub, so it prints the not-confirmed line. A token that already expired will likely make GitHub refuse the call, which also gives the not-confirmed line. The client neve…[truncated]Tests
cargo nextest run -p minimal logout_revokes_at_github_best_effortcargo nextest run -p minimal sign_in_after_logout_revives_no_earlier_reference./scripts/session-e2e.sh bep_logout_refuses_every_reference_at_next_requestVerify
min task run agent-verifyexited 101 under rustc 1.98.1, which the repository does not pin (1.97.0 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Out of scope
These files are outside the task's
layers:; see Layer exception below:crates/minimal/src/cmd/mod.rsResiduals
bep_logout_refuses_every_reference_at_next_request(arch C1): the case signs the runner out of GitHub, so it runs withE2E_BEP_LOGOUT=1on the macOS lane once that lane has a proxy and a sign-in, and records NOT RUN everywhere else.Layer exception
Two lines in crates/minimal/src/cmd/mod.rs, outside T21's layers, accepted under the owner's layer-exception rule of 2026-10-19 (required one-line changes that make an in-layer change compile): the
cmd_auth_logoutname added to the existingpub use auth::{…}re-export, and theAuthCommand::Logoutarm in themin authdispatch match. Without them theLogoutvariant that cli.rs adds does not compile, because the dispatch lives in mod.rs. No other file outside the cli and e2e layers changed.Closes #2278
Generated by Claude Code