Repository navigation
feat(bep): renew an expiring GitHub sign-in once at the proxy - #2518
Conversation
A token that has expired, or expires within 5 minutes, renews from its refresh material before substitution. One renewal runs per item, and other requests wait for its outcome. The renewed material is written update-only before first use. A failed write or a deleted item refuses the request. The renewal is its own audit record. (BEP-069)
After sign-in, min auth reads only the sign-in item's metadata and sends nothing to GitHub. Renewal is the proxy's work. (BEP-069)
…xpiring-sign-in-once-at-the-proxy # Conflicts: # crates/bep/src/forward.rs # crates/bep/src/main.rs # crates/bep/src/redeemer.rs
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen. |
…xpiring-sign-in-once-at-the-proxy
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The renewal path follows Gatehouse §6.10 line 839 in its shape, but three parts deviate. First, the host keychain write is check-then-write. A logout in that window brings the item back, so a revoked reference can come back. C1 adds a shared lock. Second, the renewal is written as a second admit decision. Line 839 says the renewal is audited as its own event, and C2 adds Kind::Renewal. Third, the proxy reads the App secret from an environment variable that no launcher sets, so a browser-flow sign-in never renews. C3 embeds it at build time, as min does.
Ruled at aca6293 against gominimal/arch@109541a48c31 by claude-opus-5-5.
A failed renewal is not retried for 60 seconds. The token exchange has one 30-second deadline, the lookup included, and a waiter stops waiting after 45 seconds. The host keychain's update-only write now checks and writes under a lock file that the logout also holds.
A renewal at the proxy checks and writes the item under the same lock, so it cannot write the item back after the logout deleted it.
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The renewal path conforms to Gatehouse §6.10 line 839: renew in the window, persist update-only before first use, single-flight, and refuse on a failed write or a deleted item. One gap remains. Line 839 says a browser-flow sign-in refreshes under the embedded published-App secret. bep reads that secret only from a run-time variable that no host code sets, but min embeds it at build time. Thus a browser-flow sign-in cannot renew at the proxy, and it refuses after 8 hours. Make the change in C1. Then the change conforms.
Ruled at 2f00ee4 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A box that runs past the token's 8 hours still clones and pushes. The proxy renews the sign-in once, and no new sign-in is necessary. In this round, a refused renewal is not retried on every request, a stalled renewal does not hold the waiting requests, and a renewal cannot write the item back after Notes from the agent: Revise round 1. All four review threads are valid, and this round fixes each of them. The host keychain tools have no update-only write. For that reason, |
|
/revise Round 2 of #2518, on the arch condition at 2f00ee4 and the two review findings that stay open. No round is in flight.
One or two commits, Conventional Commits subjects, body lines at most 100 characters. Generated by Claude Code |
…xpiring-sign-in-once-at-the-proxy
The proxy took the GitHub App client secret only from the run-time environment, and no launcher sets it, so a browser-flow sign-in never renewed. The secret now comes from option_env!, as min's BUILT_IN_SECRET does. A non-empty MINIMAL_GITHUB_APP_CLIENT_SECRET at run time overrides it.
GitHub revokes the old refresh token when it renews. When the item write failed, the proxy dropped the new pair, and the item kept a dead refresh token. The proxy now holds the pair in memory, and the next renewal writes it in place of a call to GitHub. The request is still refused, and the renewed token is substituted only after the write.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 4 unresolved threads — minimal-arch[bot] crates/bep/src/main.rs, minimal-foundry[bot] crates/bep/src/signin.rs:660, minimal-foundry[bot] crates/bep/src/main.rs:968, minimal-arch[bot] crates/bep/src/main.rs. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The renewal path now keeps a renewed token and refresh pair in BoxRedeemer::unsaved after a failed keychain write, and it holds that pair across requests. Gatehouse §6.10 line 844 lets the proxy hold plaintext for the request only. Line 839 says that a persist failure refuses and that re-login recovers. The held pair also escapes the GitHub revoke in min auth logout. Remove the hold (C1), and change the test so that it proves the design's outcome (C2). The embedded browser-flow secret in 28c73e4c conforms.
Ruled at cecf11b against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 2 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
A box that runs past the token's 8 hours still clones and pushes with no new sign-in. The proxy renews the sign-in once, and now a browser-flow sign-in renews too, under the App secret embedded at build time. A renewal that the keychain did not write is kept and written on the next renewal, so the item does not keep a refresh token that GitHub revoked. Notes from the agent: Round 2. Commit 28c73e4: GitHubTokenEndpoint::new takes the secret from option_env!("MINIMAL_GITHUB_APP_CLIENT_SECRET") (BUILT_IN_SECRET), and a non-empty run-time CLIENT_SECRET_ENV overrides it. Doc comments now say where the secret comes from. New test a_browser_sign_in_renews_under_the_embedded_secret pins it. The refresh body is split into refresh_body/exchange so the test checks the body with no network call. Commit cecf11b: for finding 4368951615 I did not substitute the new token for the current request, because the BEP-069 sub-requirement says a renewal that is not written refuses the request and substitutes nothing (renewal_not_persisted_is_refused). Instead signin::renew_holding keeps the accepted pair as signin::Unsaved, BoxRedeemer holds it in a Mutex<Option>, and the next renewal writes it without calling GitHub. If that pair's token also needs renewal, the next renewal refreshes from the pair's refresh token, not the item's revoked one. The item lock, the back-off and the renewal audit record are unchanged. Item 3 of the directive (reply on threads and resolve them) is not possible from this session: the runtime posts the verdicts below, and resolution is the reviewer's decision. just clippy-strict was not run. |
|
/revise Round 3 of #2518, on the arch ruling at cecf11b. No round is in flight. Commit 28c73e4 (the embedded secret) is accepted; commit cecf11b (the in-memory hold of an unsaved renewal) is refused, and the review finding 4368951615 that prompted it is ruled against by the design.
One commit, Conventional Commits subject, body lines at most 100 characters. Generated by Claude Code |
Revert the in-memory hold of an unsaved renewal (cecf11b). The proxy holds the renewed plaintext for the request only, and a failed write refuses the request, with a new sign-in as the recovery. renew drops the renewed pair and returns RenewError::NotSaved, and BoxRedeemer::credential calls signin::renew again. The new test shows that after a refused write the request is refused github_sign_in_required. After the back-off, the next renewal asks GitHub again with the refresh token in the item, and the item keeps its old material.
|
Round 3 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
A box that runs past the 8-hour token life still clones and pushes. The proxy renews the sign-in once and writes it to the item, with no new sign-in. When the item write fails, the proxy discards the renewed token and refuses the request with github_sign_in_required. Notes from the agent: One commit, b88a8b0. It reverts cecf11b exactly: Unsaved, renew_holding and BoxRedeemer::unsaved are gone, credential calls signin::renew, and the NotSaved doc and warn text are back to the 'discarded' wording. It also adds an_unsaved_renewal_is_discarded_and_renewed_from_the_item, which uses SwitchedWrites with writes refused on both requests. The test asserts both requests refuse github_sign_in_required, the endpoint sees two calls and gets the item's refresh token both times, and the item keeps the old material. Directive item 3 asks to resolve thread 4368951615. My instructions forbid resolving threads, so I did not. My verdict on it is now invalid, with the design reason, and the reviewer can resolve it. Commit 28c73e4 and the rest of the branch are unchanged. cargo clippy -p bep --all-targets is clean. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head removes the in-memory hold of a renewal that the keychain did not save. A failed write now discards the renewed pair and refuses the request with github_sign_in_required. Gatehouse §6.10 line 839 says that a persist failure refuses, and that re-login recovers. I checked each clause of line 839 again on this head, and the change conforms. The PR description still says that a logout can recreate the item between the check and the write. The item lock removes that window, so correct the description.
Ruled at b88a8b0 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter approval at b88a8b0: every merge gate passes at this head (arch PASS, zero open threads, every lane aggregator green, bot resolutions re-verified). Squash title feat(bep): renew an expiring GitHub sign-in once at the proxy and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
…reap #2518 added the GitHub token renewal beside serve() and its tests at the end of the test module. Both sides keep their additions: supervised() before the renewal code, and the supervisor tests before the renewal tests. Claude-Session: https://claude.ai/code/session_01WhCC5TEBNzPeWgytRsU4rc
T24 — Renew an expiring sign-in once at the proxy
slice: S2
kind: task
covers: BEP-069
size: L
est: 350 code, 420 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics:
min box audit <box>shows each renewal as its own record, and a sign-in that cannot renew as a refusal markedgithub_sign_in_requiredobservability: one info log line per renewal naming the account and the new expiry, and one warn line when GitHub refuses it
layers:
- renew:
- crates/bep/src/signin.rs: renews from the refresh material when the token has expired or expires within 5 minutes, writing the item update-only before first use
- crates/bep/src/forward.rs: one renewal in flight per item, the others waiting for its outcome; refuses when the write fails or the item is gone
- crates/bep/src/redeemer.rs:
BoxRedeemer::credentialno longer refuses an item whose standing isLivebut expired; it hands the item to the renewal path (widened 2026-10-24)- crates/bep/src/main.rs: wires a GitHub token-endpoint client into the proxy, carrying the app's client id and the
MINIMAL_GITHUB_APP_CLIENT_SECRETthe host passes it (widened 2026-10-24)test: cargo nextest run -p bep an_expiring_sign_in_is_renewed_at_the_proxy
test: cargo nextest run -p bep concurrent_redemptions_share_one_renewal
test: cargo nextest run -p bep renewal_not_persisted_is_refused
test: cargo nextest run -p bep renewal_finding_item_deleted_refuses_and_creates_nothing
test: cargo nextest run -p bep a_sign_in_with_time_to_live_is_not_renewed
test: cargo nextest run -p bep unrenewable_sign_in_refuses_with_sign_in_required
- client:
- crates/minimal/src/cmd/auth.rs: reads only the item's metadata after sign-in
test: cargo nextest run -p minimal client_never_presents_or_refreshes_the_token
produces: none
consumes: none
proof: A box running past the token's 8 hours still clones and pushes, renewed once at the proxy, with no new sign-in.
depends on:
- blocked by #2276
Generated by Claude Code
Proof
A box request that redeems a GitHub sign-in whose token has expired, or expires within 5 minutes, now renews the sign-in once at the proxy. The proxy writes the renewed token to the same keychain item and substitutes it, so git and gh in a box that runs past the token's 8 hours keep working with no new
min auth login. The tests prove this against a stand-in token endpoint. No test in this session called the real github.com token endpoint.Notes from the agent: Audit record: audit.rs is not in the layers, so I did not add a new record kind for renewals. Each renewal is written as its own
decisionrecord, with outcome admit and markergithub_sign_in_renewed, before the decision record of the request. A later task can add a dedicated kind in audit.rs.Refusal markers: a failed keychain write and a deleted item both refuse the request with
github_sign_in_required. The spec names no other marker for them. The text the box receives in the refusal is unchanged (the generic 'did not accept the sealed credential' body). I did not add a 'run min auth login' body, because a sign-in after a logout gets a new instance, and that does not heal a box that holds the old reference.Behaviour the spec does not cover: if GitHub refuses an early renewal (the token is still valid but expires within 5 minutes), the proxy substitutes the held token and does not refuse the request.
Update-only write:
Keychain::updateis atomic in MemoryKeychain. For HostKeychain it uses the default, which checks the held instance and then writes. Neithersecuritynorsecret-toolhas an update-only write, so a logout between the check and the write can still create the item again. The window is a few milliseconds.main.rs:
GitHubTokenEndpointis a small synchronous HTTPS client built on rustls and std TcpStream. I wrote it this way so that Cargo.toml needs no new dependency. It POSTs to https://github.com/login/oauth/access_token with the App client idIv23liv6KetAFJp4Eb5K.…[truncated]Tests
cargo nextest run -p bep an_expiring_sign_in_is_renewed_at_the_proxycargo nextest run -p bep concurrent_redemptions_share_one_renewalcargo nextest run -p bep renewal_not_persisted_is_refusedcargo nextest run -p bep renewal_finding_item_deleted_refuses_and_creates_nothingcargo nextest run -p bep a_sign_in_with_time_to_live_is_not_renewedcargo nextest run -p bep unrenewable_sign_in_refuses_with_sign_in_requiredcargo nextest run -p minimal client_never_presents_or_refreshes_the_tokenVerify
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.Base
The base branch moved while the coder worked and was merged in before the tests ran. These files changed on BOTH sides since the branch point — read them first, because a rewrite from a stale reading can undo a merge without a conflict:
crates/bep/src/forward.rscrates/bep/src/main.rscrates/bep/src/redeemer.rscrates/minimal/src/cmd/auth.rsCloses #2279