Skip to content

feat(buzz-agent): add DatabricksAuthCoordinator single-flight OAuth - #5545

Open
wpfleger96 wants to merge 15 commits into
mainfrom
duncan/databricks-auth-coordinator
Open

feat(buzz-agent): add DatabricksAuthCoordinator single-flight OAuth#5545
wpfleger96 wants to merge 15 commits into
mainfrom
duncan/databricks-auth-coordinator

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What

Consolidates all Databricks OAuth acquisition behind one coordinator on PkceOAuthTokenSource. Every entry point — the four TokenSource methods (bearer, bearer_no_browser, refresh_now, interactive_login) plus the public acquire_with_intent — routes through a single acquire()/acquire_locked() core that owns browser and cooldown policy.

Before this, acquisition logic was scattered across those methods with no coordination: concurrent callers (Desktop discovery, the saved-agent model picker, managed-runtime inference) could each pop their own browser, and a just-denied attempt would immediately re-prompt on the next passive read.

How

  • Intent policy. AuthIntent::{Auto, UserInitiated, Headless} decides whether a caller may open a browser and whether it honors the cooldown. Headless never browses; Auto browses but honors an unexpired cooldown; UserInitiated browses and bypasses+clears the cooldown.
  • Two-layer single-flight per cache key. An in-process registry (INFLIGHT) coalesces same-key, same-intent callers onto one leader's attempt before the file lock. The slot key is (lock_path, AuthIntent), so a UserInitiated sign-in never inherits an Auto leader's result. A joined result is revalidated against the waiter's own contract: if the leader's published rejected-token digest differs from the joiner's own (both-None matches), the joiner reruns its own acquire_leader — a leader's rejection-relative failure is not valid for a joiner that carried a different rejected token. Across processes, callers serialize on a flock-based advisory lock (fs2::FileExt::try_lock_exclusive, per open-file-description) and share success through the on-disk cache. RAII Drop releases both lock and leader slot, so a crashed holder never wedges a successor.
  • Validate-before-persist boundary. finish() is the candidate-token persistence boundary for refresh and browser results (cache-hit paths bypass it). Before a refresh- or browser-issued token is written to cache or the cooldown is cleared, a bearer equal to the caller's rejected bytes yields a typed failure — never a loop, never a poison write.
  • Token neutralization. When acquire_locked enters with rejected = Some(bytes), it calls expire_rejected() under the state lock before any cache check. This force-expires the in-memory entry and the on-disk copy when their access token byte-equals the rejected value, so a later plain bearer() or a fresh process cannot serve the proven-dead token. Disk neutralization uses a bounded three-stage fallback: (1) atomic rewrite via persist() (temp-file + rename); (2) in-place truncating overwrite via OpenOptions::write().truncate(true) on the existing file (succeeds even when the parent directory is non-writable, since only the file's own mode matters); (3) remove_file as a last resort. Our cache files are created 0600, so stage (2) covers the proven hostile case (0600 file under 0500 parent). The refresh token stays intact for recovery.
  • Cross-process failure single-flight. An AttemptRecord sidecar (.attempt file alongside the cache key) records a monotonically-increasing generation, intent, result code, and the SHA-256 digest of the completing caller's rejected token (non-secret; None when no rejection). A caller snapshots the generation before queueing; on acquiring the lock, it adopts a recorded terminal failure only when (a) the generation advanced, (b) the intent matches, (c) the result is recognized, and (d) the rejected_digest matches its own (both-None matches). Digest mismatch triggers a normal attempt — a rejection-relative failure recorded for token X is not adopted by a waiter that carried a different rejected token. Adopters do NOT re-write the attempt generation: re-writing would relay the failure indefinitely through chained adopters. The adoption contract is temporal: a caller whose pre-queue snapshot predates the current generation was already queued during the attempt and adopts — including UserInitiated callers, mirroring the in-process INFLIGHT coalescing. A UserInitiated caller arriving after the failure snapshots the new generation and naturally runs its own attempt. write_attempt reads the current on-disk generation at write time so each completed attempt strictly advances the value.
  • Typed outcomes. AuthError with stable code()/from_code() replaces display-text matching. RefreshRejected and NoCredential are reconstructed for the attempt-adoption path in addition to the existing cooldown-worthy variants.
  • Durable cooldown sidecar. Every failed browser attempt is recorded next to the cache key. Auto reads it and returns the recorded outcome; UserInitiated bypasses and clears it; success clears it. 5-minute expiry.
  • Classification by category, not status class. Both refresh and exchange paths classify a provider response by its OAuth error body: only a 4xx invalid_grant is a genuine rejection. Transport errors, 429, 5xx, and malformed success bodies are infrastructural (NetworkUnavailable) and never poison the cooldown or pop a needless browser.
  • Windows disk persistence disabled. On non-Unix platforms, read_private_cache returns Unsupported without reading (and best-effort removes any legacy token file left by older builds). persist() is a #[cfg(unix)] no-op. Lock, cooldown, and attempt sidecars hold no secrets and are active on all platforms — cross-process failure adoption works on Windows. Cross-process success handoff requires the on-disk token cache, so on non-Unix each process performs its own acquisition (serialized by the lock, failure-adoption intact) until owner-only DACL persistence exists. Tests that seed or assert on the on-disk token cache are #[cfg(unix)]-gated; Windows CI exercises lock serialization, cooldown + attempt sidecars, and the legacy-file deletion path.
  • Injected browser opener, invoked while the localhost callback listener is live — a launch failure never returns a URL pointing at a torn-down listener.

Tests

  • crates/buzz-agent/tests/databricks_auth_coordinator.rs: the browser/cooldown/classification acceptance matrix driven through the public API with a scripted BrowserOpener and a stub OIDC provider. Covers single-flight coalescing, cooldown adoption, UserInitiated bypassing Auto denial, 401-recovery rejecting re-issued tokens, joiner with a different rejected token reruns rather than inheriting a rejection-relative failure (both in-process and cross-process), cross-process lock contention + crash release + single-grant + cache race (real second processes via lock-holder and auth-worker helper binaries), cross-process failure adoption (both Headless and UserInitiated queued workers adopt with matching digest; post-failure arrivals and mismatched-digest waiters run their own attempt), token neutralization (in-place fallback for non-writable parent — proven case), and refresh/exchange classification.
  • auth.rs in-crate tests: lock-primitive edge cases needing private access — contended waiter times out with LockTimeout, RAII drop releases successor, lock-free disk recheck on shared failure.

Scope / follow-ups

  • Runtime 401 handling is deferred. This PR owns acquisition single-flight and policy. The runtime-side behavior when the provider rejects a live token mid-inference is tracked separately.
  • Desktop wiring is Phase 2 (not in this PR's boundary). This change is confined to crates/buzz-agent/.
  • Windows DACL is a follow-up once the windows-sys binding is available; disk persistence re-enables then.

Stack

Built on #5534 (hayt/databricks-oauth-cache-hardening), now merged. Retargeted to main.

@wpfleger96
wpfleger96 requested a review from a team as a code owner August 11, 2026 05:09
Base automatically changed from hayt/databricks-oauth-cache-hardening to main August 11, 2026 13:58
@wpfleger96
wpfleger96 force-pushed the duncan/databricks-auth-coordinator branch from 90a4f0b to c581d2e Compare August 11, 2026 14:09
@wpfleger96 wpfleger96 mentioned this pull request Aug 11, 2026
Merged
wpfleger96 added a commit that referenced this pull request Aug 12, 2026
@wpfleger96 Duncan
fix(desktop): launch Databricks OAuth from passive model discovery (#…
1ff98fa 
…5607)

When a user's agent runtime is `buzz-agent` with no cached Databricks
OAuth token, the desktop app's passive model-discovery surfaces were
forbidden from launching interactive auth. Discovery failed silently, so
the model dropdown showed only built-in fallback models behind a vague
"Could not load live models for `databricks_v2`" note (reported
internally by Nick and Jose).

## What changed

Both discovery surfaces — the passive draft-form discovery and the
explicit saved-model picker — now launch the browser OAuth flow,
matching goose's behavior. The only behavioral difference between them
is cooldown handling:

- **Passive draft discovery** fires on every form-state change, so a
failed, cancelled, or timed-out sign-in records a per-host cooldown (5
min) that suppresses re-popping the browser on the next keystroke. While
the cooldown is active it returns the "sign-in required" guidance
instead of relaunching.
- **The explicit model picker** is a deliberate user action, so it
always launches and clears any stale cooldown first.

Safety rails:

- A 150s hard timeout (`AUTH_FLOW_TIMEOUT`) bounds the whole interactive
flow so an abandoned SSO tab fails discovery cleanly rather than wedging
the dropdown. Success clears the cooldown; failure and timeout both
record it.
- `AuthCooldown` recovers from a poisoned lock rather than wedging every
future sign-in on one panic.

The frontend maps the terminal Databricks sign-in states to typed,
actionable copy in `formatModelDiscoveryErrorStatus`: "sign-in required"
is a muted note pointing at the picker and `buzz-agent auth databricks`;
a failed or timed-out sign-in is a warning pointing at the explicit
retry. Other Databricks failures fall through to the existing generic
notice.

## Scope

Changes are confined to Databricks discovery and its frontend status
formatter — no `agent_models.rs` call sites are touched. The
interactive-auth helper takes an injected timeout so the
timeout/cooldown policy is unit-testable without a live browser.

## Deferred

Cooldown keys use the raw trimmed `DATABRICKS_HOST`, while the catalog
and OAuth cache normalize trailing slashes
(`crates/buzz-agent/src/catalog.rs:96`,
`crates/buzz-agent/src/llm.rs:2046`). So `https://workspace/` and
`https://workspace` share credentials but get separate cooldown entries
— an equivalent-spelling change to the host field mid-cooldown can
re-pop passive OAuth once within the 5-minute window. Self-limiting (one
extra browser launch, never auth corruption). Follow-up: a
`trim_end_matches('/')` on the cooldown key plus an equivalent-host
test, picked up with the coordinator migration if
[#5545](#5545) ever merges.

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
shelman09 added a commit to Namleh-Studios/buzz that referenced this pull request Aug 13, 2026
@shelman09 @wpfleger96
Hayt Duncan @Chessing234 @shellz-n-stuff
OPS-248 Port critical upstream security and relay fixes (#7)
4f95dfc 
* fix(buzz-agent): harden Databricks OAuth token cache and callback (block#5534)

Hardens the Databricks PKCE OAuth code in
`crates/buzz-agent/src/auth.rs`. Two fixes.

## Token cache is owner-only across its whole lifecycle, and race-safe

The PKCE cache holds both the access and refresh tokens, but `save()`
wrote it with a bare `fs::write` + `fs::rename`. Under a `022` umask the
file landed world-readable, and the fixed `*.json.tmp` temp name races
across concurrent savers sharing `$HOME` — one writer's `rename` can
fail on another's half-written temp.

**On write**, `write_private_cache()` creates a temp file with
owner-only permissions from the moment it exists — mode `0o600` on Unix
via `OpenOptions::mode` — writes and fsyncs it, then renames over the
destination. The rename swaps the inode wholesale, so a pre-existing
cache file with loose permissions is *replaced* by the new private inode
rather than inheriting its mode. `unique_suffix()` (getrandom, timestamp
fallback) gives each write a distinct temp name, and a drop guard
removes the temp on any failure path.

**On load**, owner-only is enforced as a cache lifecycle invariant, not
just a write-path property. A world-readable cache left by an older
buzz-agent was previously read straight into memory and returned on the
fresh cache-hit path without ever invoking `save()`, so a token file
with no advertised expiry could stay exposed indefinitely.
`read_cache()` now funnels every load — initial and cross-process
re-reads — through `read_private_cache()`, which on Unix opens with
`O_NOFOLLOW` (kernel-level symlink refusal, no stat/open TOCTOU),
requires a regular file, and `fchmod`s the pinned handle to `0o600` when
any group/other bit is set. A cache that cannot be secured is treated as
absent, so callers fail closed to a fresh flow rather than trusting an
exposed file.

## OAuth callback no longer reflects untrusted input

The localhost callback embedded the untrusted `error` query param
straight into the HTML response — an XSS sink on the redirect page — and
routed that same raw value into the error string that reaches the logs.

`callback_outcome()` is now a pure function returning `(result,
static_page)`: the browser always sees a fixed literal page that embeds
no request parameter, and failure detail travels only through the result
channel. `sanitize_callback_detail()` strips control characters (CR/LF
log-line injection) and caps length before that detail enters the error
string bound for the logs.

## Deferred: Windows owner-only ACLs

Windows owner-only protection is out of scope for this change. The
goose-parity route (`CreateFileW` with an owner-only SDDL
`D:P(A;;FA;;;OW)`) requires `unsafe` FFI, which this crate's
`#![forbid(unsafe_code)]` prohibits; reconciling that conflict is a
separate decision. Both platform seams — `create_private_temp_file`
(write) and `read_private_cache` (load) — have a `#[cfg(not(unix))]`
branch that relies on the default per-user ACLs and is the drop-in point
if Windows protection is added later. No new dependency and no `unsafe`
are introduced here.

---------

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Hayt <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz>
(cherry picked from commit 5e4d0fe)

* fix(desktop): launch Databricks OAuth from passive model discovery (block#5607)

When a user's agent runtime is `buzz-agent` with no cached Databricks
OAuth token, the desktop app's passive model-discovery surfaces were
forbidden from launching interactive auth. Discovery failed silently, so
the model dropdown showed only built-in fallback models behind a vague
"Could not load live models for `databricks_v2`" note (reported
internally by Nick and Jose).

## What changed

Both discovery surfaces — the passive draft-form discovery and the
explicit saved-model picker — now launch the browser OAuth flow,
matching goose's behavior. The only behavioral difference between them
is cooldown handling:

- **Passive draft discovery** fires on every form-state change, so a
failed, cancelled, or timed-out sign-in records a per-host cooldown (5
min) that suppresses re-popping the browser on the next keystroke. While
the cooldown is active it returns the "sign-in required" guidance
instead of relaunching.
- **The explicit model picker** is a deliberate user action, so it
always launches and clears any stale cooldown first.

Safety rails:

- A 150s hard timeout (`AUTH_FLOW_TIMEOUT`) bounds the whole interactive
flow so an abandoned SSO tab fails discovery cleanly rather than wedging
the dropdown. Success clears the cooldown; failure and timeout both
record it.
- `AuthCooldown` recovers from a poisoned lock rather than wedging every
future sign-in on one panic.

The frontend maps the terminal Databricks sign-in states to typed,
actionable copy in `formatModelDiscoveryErrorStatus`: "sign-in required"
is a muted note pointing at the picker and `buzz-agent auth databricks`;
a failed or timed-out sign-in is a warning pointing at the explicit
retry. Other Databricks failures fall through to the existing generic
notice.

## Scope

Changes are confined to Databricks discovery and its frontend status
formatter — no `agent_models.rs` call sites are touched. The
interactive-auth helper takes an injected timeout so the
timeout/cooldown policy is unit-testable without a live browser.

## Deferred

Cooldown keys use the raw trimmed `DATABRICKS_HOST`, while the catalog
and OAuth cache normalize trailing slashes
(`crates/buzz-agent/src/catalog.rs:96`,
`crates/buzz-agent/src/llm.rs:2046`). So `https://workspace/` and
`https://workspace` share credentials but get separate cooldown entries
— an equivalent-spelling change to the host field mid-cooldown can
re-pop passive OAuth once within the 5-minute window. Self-limiting (one
extra browser launch, never auth corruption). Follow-up: a
`trim_end_matches('/')` on the cooldown key plus an equivalent-host
test, picked up with the coordinator migration if
[block#5545](block#5545) ever merges.

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
(cherry picked from commit 1ff98fa)

* fix(relay): stop panicking the ingest worker on reactions to project events (block#5294)

A NIP-25 reaction whose target is a project root or project comment
(kind
1621 issue, 1618 PR, or a kind-1 comment on one) carries no h tag, so
channel_id is None on the reaction write path. The conformance-trace
emission asserted a channel was always present:

channel: channel_label(channel_id.expect("reaction path has channel")),

so the worker panicked at ingest.rs:2824. The row was inserted before
the
panic, so the client saw a failed request for a persisted event and
retried,
and the duplicate branch carried the same expect, head-of-line blocking
a
durable publish queue forever.

Mirror the message write's three-way split at the same seam:
(Some, true) -> WriteInsert, (Some, false) -> WriteDuplicate, (None, _)
-> WriteInsertGlobal. The conformance vocabulary already models
channel-less
writes; only the reaction path was missing it.

Closes block#4936

Signed-off-by: Taksh <takshkothari09@gmail.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
(cherry picked from commit 16b7ae7)

* Harden shared agent instruction review (block#4220)

## Summary

- render shared-agent instructions as literal text so Markdown cannot
conceal spoiler contents, link destinations, or image sources
- reject non-reviewable Unicode controls at every agent-definition
boundary while preserving legitimate rendered emoji sequences
- verify shared catalog event IDs and signatures before trusting
authorship, coordinates, pagination, or executable content
- preserve the exact system-prompt bytes between review and execution
instead of silently stripping or normalizing content

## Security rationale

Shared system prompts are executable configuration. Previously, catalog
prompts were projected through the chat Markdown renderer, which could
hide text, replace link destinations with benign labels, and turn image
syntax into remote loads. Zero-width and bidirectional controls could
also make reviewed text differ from what the agent executes.

This change establishes a review invariant: the prompt a user sees is
the prompt the agent executes. Definitions that cannot be reviewed
faithfully are rejected rather than rewritten. Catalog events must also
pass Nostr ID/signature verification before they can claim a publisher,
coordinate, or cursor.

## What changed

- catalog instructions render as exact literal text rather than rich
Markdown
- catalog relay events are verified on a fresh wire-shaped object before
paging, coordinate selection, attribution, or projection
- forged content, pubkeys, signatures, and invalid newer heads are
ignored and cannot shadow a valid signed definition
- TypeScript catalog parsing rejects unsafe remote definitions before
they reach the UI
- shared Rust validation covers persona create/update/import, inbound
relay sync, definition-less managed-agent sync, and catalog publication
paths
- definition-less managed agents now fail closed on local create, local
update, and publication before persistence or relay retention
- linked managed agents validate their local name while treating the
persona definition as authoritative; their inert record-level prompt is
not executed or published
- names reject layout controls; prompts retain ordinary newlines and
tabs
- legitimate emoji composition is supported, including contextual VS16,
ZWJ, skin-tone, family, flag, and keycap sequences
- detached selectors/joiners, bidirectional controls, tag characters,
zero-width concealment, and other default-ignorables remain rejected
- names are bounded to 128 characters and prompts to 64 KiB
- contributor guidance documents the byte-for-byte review requirement
for future sharing paths

Validation reports the offending code point and never silently removes
it.

## E2E recording

[buzz-shared-agent-security-e2e.webm](https://github.com/user-attachments/assets/44d6b75f-0877-490f-bda4-a716fae3f700)

The recording demonstrates:

- a safe definition remains visible
- a prompt containing zero-width `U+200B` is rejected
- a name containing bidi override `U+202E` is rejected
- the prompt is preserved exactly
- spoiler, link, and image syntax remains literal and does not render or
load

## Verification

Passed locally:

- `just test`: all 10 unit and Docker-backed integration stages
- desktop frontend unit suite: 4,295 tests
- persona catalog relay unit suite: 32 tests, including forged-event and
cursor-shadowing cases
- focused Rust definition-validation coverage: 3 local create/update
tests and 6 publication-filtered tests
- complete desktop Tauri library suite after rebase: 2,263 passed, 14
ignored, 0 failed
- desktop Tauri clippy with warnings denied and Rust formatting
- complete agent Playwright spec: 34 tests
- the exact formerly failing `inbox-edit` immediate-attachment smoke
test after rebase: 1 test
- focused shared-agent publish, literal-review, hidden-control,
signature, and cross-member import Playwright coverage
- desktop E2E production build and TypeScript typecheck
- changed-file formatting/lint and file-size ratchet
- pre-commit secret scan and DCO signoff

The branch was rebased onto current `main`, which includes the upstream
attachment-button label fix. Fresh post-rebase GitHub CI is green for
every required and selected check: Desktop Core, all four Desktop Smoke
E2E shards, both Desktop E2E Integration shards and their aggregate,
Desktop E2E Relay, Desktop Build (macOS), Windows Rust, Rust Lint, DCO,
security scanners, and Desktop Release Candidate. The previously failing
`Desktop Smoke E2E (3)` shard now passes.

The repository-wide desktop check also reports existing CSS
formatting/`!important` findings in `components.css` and `terminal.css`;
neither file is changed by this PR. GitHub's Desktop Core lint and
format stage passes on the rebased branch.

---------

Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
(cherry picked from commit a96af89)

* fix: reject non-reviewable Unicode formatting

* fix: close upstream security review gaps

* fix: authorize inbound agent sync events

---------

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Taksh <takshkothari09@gmail.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Hayt <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Co-authored-by: Taksh Kothari <takshkothari09@gmail.com>
Co-authored-by: Alex Rosenzweig <64241648+shellz-n-stuff@users.noreply.github.com>
bhargavms pushed a commit to EWA-Services/buzz that referenced this pull request Aug 18, 2026
@wpfleger96 Duncan @bhargavms
fix(desktop): launch Databricks OAuth from passive model discovery (b…
961c8ff 
…lock#5607)

When a user's agent runtime is `buzz-agent` with no cached Databricks
OAuth token, the desktop app's passive model-discovery surfaces were
forbidden from launching interactive auth. Discovery failed silently, so
the model dropdown showed only built-in fallback models behind a vague
"Could not load live models for `databricks_v2`" note (reported
internally by Nick and Jose).

## What changed

Both discovery surfaces — the passive draft-form discovery and the
explicit saved-model picker — now launch the browser OAuth flow,
matching goose's behavior. The only behavioral difference between them
is cooldown handling:

- **Passive draft discovery** fires on every form-state change, so a
failed, cancelled, or timed-out sign-in records a per-host cooldown (5
min) that suppresses re-popping the browser on the next keystroke. While
the cooldown is active it returns the "sign-in required" guidance
instead of relaunching.
- **The explicit model picker** is a deliberate user action, so it
always launches and clears any stale cooldown first.

Safety rails:

- A 150s hard timeout (`AUTH_FLOW_TIMEOUT`) bounds the whole interactive
flow so an abandoned SSO tab fails discovery cleanly rather than wedging
the dropdown. Success clears the cooldown; failure and timeout both
record it.
- `AuthCooldown` recovers from a poisoned lock rather than wedging every
future sign-in on one panic.

The frontend maps the terminal Databricks sign-in states to typed,
actionable copy in `formatModelDiscoveryErrorStatus`: "sign-in required"
is a muted note pointing at the picker and `buzz-agent auth databricks`;
a failed or timed-out sign-in is a warning pointing at the explicit
retry. Other Databricks failures fall through to the existing generic
notice.

## Scope

Changes are confined to Databricks discovery and its frontend status
formatter — no `agent_models.rs` call sites are touched. The
interactive-auth helper takes an injected timeout so the
timeout/cooldown policy is unit-testable without a live browser.

## Deferred

Cooldown keys use the raw trimmed `DATABRICKS_HOST`, while the catalog
and OAuth cache normalize trailing slashes
(`crates/buzz-agent/src/catalog.rs:96`,
`crates/buzz-agent/src/llm.rs:2046`). So `https://workspace/` and
`https://workspace` share credentials but get separate cooldown entries
— an equivalent-spelling change to the host field mid-cooldown can
re-pop passive OAuth once within the 5-minute window. Self-limiting (one
extra browser launch, never auth corruption). Follow-up: a
`trim_end_matches('/')` on the cooldown key plus an equivalent-host
test, picked up with the coordinator migration if
[block#5545](block#5545) ever merges.

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Signed-off-by: bhargavms <bhargav.m@ewa-services.com>
BradGroux pushed a commit to BradGroux/buzz that referenced this pull request Aug 23, 2026
@wpfleger96 Duncan
fix(desktop): launch Databricks OAuth from passive model discovery (b…
60bc36c 
…lock#5607)

When a user's agent runtime is `buzz-agent` with no cached Databricks
OAuth token, the desktop app's passive model-discovery surfaces were
forbidden from launching interactive auth. Discovery failed silently, so
the model dropdown showed only built-in fallback models behind a vague
"Could not load live models for `databricks_v2`" note (reported
internally by Nick and Jose).

## What changed

Both discovery surfaces — the passive draft-form discovery and the
explicit saved-model picker — now launch the browser OAuth flow,
matching goose's behavior. The only behavioral difference between them
is cooldown handling:

- **Passive draft discovery** fires on every form-state change, so a
failed, cancelled, or timed-out sign-in records a per-host cooldown (5
min) that suppresses re-popping the browser on the next keystroke. While
the cooldown is active it returns the "sign-in required" guidance
instead of relaunching.
- **The explicit model picker** is a deliberate user action, so it
always launches and clears any stale cooldown first.

Safety rails:

- A 150s hard timeout (`AUTH_FLOW_TIMEOUT`) bounds the whole interactive
flow so an abandoned SSO tab fails discovery cleanly rather than wedging
the dropdown. Success clears the cooldown; failure and timeout both
record it.
- `AuthCooldown` recovers from a poisoned lock rather than wedging every
future sign-in on one panic.

The frontend maps the terminal Databricks sign-in states to typed,
actionable copy in `formatModelDiscoveryErrorStatus`: "sign-in required"
is a muted note pointing at the picker and `buzz-agent auth databricks`;
a failed or timed-out sign-in is a warning pointing at the explicit
retry. Other Databricks failures fall through to the existing generic
notice.

## Scope

Changes are confined to Databricks discovery and its frontend status
formatter — no `agent_models.rs` call sites are touched. The
interactive-auth helper takes an injected timeout so the
timeout/cooldown policy is unit-testable without a live browser.

## Deferred

Cooldown keys use the raw trimmed `DATABRICKS_HOST`, while the catalog
and OAuth cache normalize trailing slashes
(`crates/buzz-agent/src/catalog.rs:96`,
`crates/buzz-agent/src/llm.rs:2046`). So `https://workspace/` and
`https://workspace` share credentials but get separate cooldown entries
— an equivalent-spelling change to the host field mid-cooldown can
re-pop passive OAuth once within the 5-minute window. Self-limiting (one
extra browser launch, never auth corruption). Follow-up: a
`trim_end_matches('/')` on the cooldown key plus an equivalent-host
test, picked up with the coordinator migration if
[block#5545](block#5545) ever merges.

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
wesbillman
wesbillman requested changes Aug 27, 2026
View reviewed changes

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes required at e58204b88601af14460f17940d2bc3d008f78b49:

  1. [P1] Preserve UserInitiated retry semantics across in-process coalescing. Auto and UserInitiated use the same (lock_path, may_open_browser = true) slot, and a joiner returns the leader’s result without applying its own intent (auth.rs lines 675–699). If an explicit user action joins an Auto leader that returns an active cooldown at lines 802–807, the user receives the prior Denied/TimedOut result instead of clearing the cooldown and opening sign-in as UserInitiated promises. Key the slot by the relevant intent policy, or make a user-initiated joiner retry when it inherited an automatic cooldown result. Add the mixed-intent race test; the current tests cover same-intent coalescing and sequential cooldown bypass only.

  2. [P1] Do not accept an expired replacement after a 401. In cached_hit, rejected = Some(t) considers any token whose bytes differ from t usable (lines 603–623), without checking is_expired. An expired in-memory or on-disk token B can therefore be returned as the presumed sibling replacement for rejected token A, skipping refresh for refresh_now and every public rejected-token acquisition. A replacement must differ from the rejected token and still be unexpired.

  3. [P1] Separate authorization-code rejection from exchange infrastructure failure. The code-exchange path maps every non-success status, including 429 and 5xx, plus malformed 2xx JSON/token payloads, to ExchangeFailed (lines 1544–1556). That variant is terminal LlmAuth and cooldown-worthy, so a transient provider outage after callback is reported as a rejected code and suppresses automatic auth for five minutes. Classify transport/429/5xx and malformed success responses as NetworkUnavailable; reserve ExchangeFailed for an OAuth response that establishes the authorization grant was rejected. Mirror the refresh classifier’s status/body coverage in exchange tests.

I reviewed the public entry-point × intent × cache/refresh/browser/cooldown matrix, same-process and cross-process coordination, cancellation/deadline behavior, OAuth classification, and platform paths using pinned GitHub source only. I did not execute PR code.

@wpfleger96
wpfleger96 force-pushed the duncan/databricks-auth-coordinator branch from 54cbda2 to 4513d23 Compare August 27, 2026 16:09
wesbillman
wesbillman requested changes Aug 27, 2026
View reviewed changes

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes required at 4513d2385fcd4e09109c99756f83c309ea3346b9:

  1. [P1] Include rejected-token identity in same-process single-flight. cached_hit correctly makes the caller's rejected bearer part of cache validity, but InflightKey contains only (lock_path, AuthIntent) and a joiner returns the leader's result without applying its own rejected value (auth.rs lines 656–707 and 1061–1132). This breaks the public 401/403 retry contract when concurrent requests reject different bearer generations. For example, while refresh_now(A) is refreshing, refresh_now(B) joins the same Headless slot; if the leader publishes B, the second request receives the exact token it just reported rejected and retries its provider call with known-bad credentials. The inverse can also inherit a terminal refresh/network result even though that caller's cached replacement was already valid. Include a non-secret digest of rejected in the slot key, or revalidate a joined result against the waiter's rejected value and rerun its own cache/acquisition policy. Add a deterministic concurrent different-rejected-values test proving a waiter never receives its rejected bytes.

The three blockers from the previous head are fixed: full AuthIntent now separates mixed cooldown policy, rejected replacements must differ and be unexpired, and exchange infrastructure failures remain NetworkUnavailable. I reviewed public entry points, cache/refresh/browser/cooldown transitions, same/cross-process coordination, cancellation/deadlines, OAuth classification, platform cache paths, and the test matrix using pinned GitHub source only. Exact-head CI is otherwise green; I did not execute PR code.

wesbillman
wesbillman requested changes Aug 27, 2026
View reviewed changes

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

[P1] Do not persist a bearer after proving it equals the caller’s rejected token.

The new equality guard runs in acquire_leader only after acquire_locked returns (crates/buzz-agent/src/auth.rs:786-810). Both successful live-token paths call finish first (:845, :906), and finish atomically saves the token to disk and memory before returning it (:921-930). If a provider reissues the exact bearer just rejected with 401/403, this call correctly returns RefreshRejected or NetworkUnavailable, but leaves that known-bad bearer cached as fresh. The next ordinary bearer() calls acquire(Headless, None) and cached_hit accepts it because no rejected identity is supplied (:605-623, :943-946). The runtime can therefore restore and send credentials this flow already proved unusable; a fresh process does the same from disk.

Validate the candidate before committing it, or explicitly invalidate/remove the persisted candidate on equality without destroying a still-valid concurrent replacement. Add lifecycle regressions for both sticky refresh and sticky browser exchange: after the rejected-aware acquisition fails, a following bearer() and a newly constructed token source must not return the rejected bytes.

The prior different-rejected-joiner race itself is fixed: joiners revalidate the published result and perform a bounded rerun. This remaining blocker is the persistence boundary after that rerun.

Read-only exact-head source review; no PR code was checked out or executed.

wesbillman
wesbillman requested changes Aug 28, 2026
View reviewed changes

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested at exact head 5cdc56bae2ece14fdacce7f8c0de064eaab5ffa6:

  1. P1: rejected-token reissuance still leaves the original known-bad cache entry live. The new finish guard correctly refuses to persist a newly issued token equal to rejected, but it does not invalidate the existing cached token. In the real lifecycle, bearer() returns cached unexpired A, the provider rejects A, and refresh_now(A) receives sticky A from refresh. finish returns RefreshRejected before saving, leaving the original unexpired A in memory and on disk. The next plain bearer(None), or a fresh process, accepts A again. The new poison tests seed expired-seed while passing a different rejected value (sticky-token / sticky-browser), so they cannot detect this path. Add regressions that seed the same unexpired A passed as rejected, then prove the failure cannot be followed by a cache hit for A; invalidate conditionally so a concurrently persisted distinct replacement is preserved.

  2. P1: the file lock serializes cross-process failures but does not single-flight them for UserInitiated or Headless. After process A fails and releases the lock, an already-waiting process B enters acquire_locked. A UserInitiated waiter clears A's just-written cooldown and opens a second browser; a Headless 401 waiter repeats the failed refresh. The process-local INFLIGHT slot cannot publish A's failure across processes. The current denial test only uses an Auto waiter, whose cooldown policy masks the gap; success shares through the cache. Preserve an attempt generation/outcome so callers already queued behind that generation can adopt failures while later explicit user retries still bypass cooldown, and add real two-process denial and failed-refresh regressions.

  3. P1 security: Windows persistent OAuth cache files are not owner-only. The non-Unix read path accepts any cache ACL unchanged, and the non-Unix temp-file path creates cache/cooldown files with inherited default ACLs. These files hold access and refresh tokens; the implementation comments explicitly defer the owner-only DACL. On a permissive parent or pre-existing broad ACL, other local principals can read persisted credentials. Create and validate/repair an owner-only Windows DACL, or disable persistent OAuth caching there until that guarantee exists. The Unix path's O_NOFOLLOW, fd-based mode repair, 0600 creation, and atomic rename do not cover Windows.

The previous exact-head blocker about persisting the newly returned rejected token before validation is partly fixed by moving equality validation ahead of save; item 1 is the remaining full-lifecycle gap. CI's normal build/test/security gates are green; the Codex security-review job was cancelled. Review was read-only; no PR code was executed.

@wpfleger96
wpfleger96 force-pushed the duncan/databricks-auth-coordinator branch from 5cdc56b to d0dded8 Compare August 28, 2026 23:04
wesbillman
wesbillman requested changes Aug 29, 2026
View reviewed changes

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested on exact head 8af6afb3c8f9164dfd6cc765d5416f70c9d174b6.

  1. P1: Do not share rejected-token-relative failures across different rejected bearers. The in-process slot key contains only (lock_path, intent) (crates/buzz-agent/src/auth.rs:780-840). If Headless caller A rejects X and refresh reissues X, finish returns RefreshRejected; concurrent caller B rejecting Y inherits that error, although the successful refresh proved the grant remains live and X does not equal B’s rejected value. The cross-process attempt record has the same problem because it stores intent/result but not rejected identity (:958-963, 1253-1262). Existing tests cover the inverse shared-success collision where the leader returns the joiner’s rejected bytes, not a leader-relative failure. Key rejection-relative outcomes by a non-secret rejected-token digest, or rerun joiners when the shared result may not satisfy their rejected identity; cover both in-process and cross-process transitions.

  2. P1: An adopter must not create a new attempt generation. When a queued process adopts a predecessor’s terminal failure, acquire_locked immediately calls write_attempt again (:958-963). That records work which never ran. A caller arriving after the real attempt can snapshot generation 1, queue behind the adopter that advances it to 2, and incorrectly adopt the old failure; overlapping arrivals can relay it indefinitely. Return the adopted error without advancing the sidecar, and add a three-process regression where C arrives after A’s failure but while B is adopting it.

  3. P1: The non-Unix memory-only policy breaks the cross-process success contract and its newly required Windows CI lane. persist is a no-op and read_private_cache refuses/deletes disk tokens on non-Unix (:512-527, 1578-1595), so a Windows waiter serialized by LockFileEx cannot consume the winner’s bearer and performs another refresh/browser flow. Exact-head Windows CI proves this: test_crossprocess_two_coordinators_race_to_one_grant_and_cache gets browser-token-2 versus browser-token-1, and the added auth-coordinator step fails 20 of 33 tests. Either provide secure Windows success handoff, or explicitly scope the product contract and Windows tests to invariants that remain true without persistence. Do not leave a required platform lane structurally red.

  4. P2: Rejected-token neutralization is not fail-closed for a readable stale file. expire_rejected rewrites best-effort and then removes best-effort (:544-570). If both fail while the original token file remains readable, such as an owner-writable cache file in a now read-only directory where temp creation and unlink fail, a fresh process still serves the locally-unexpired bearer already proven dead. The regression substitutes a directory at the cache path, which the read path rejects independently, so it does not cover this case. Add a fallback that durably expires/removes the readable regular file or make subsequent loads reject it, with the actual double-failure regression.

The OAuth status classification, Unix no-follow/0600/atomic cache path, bounded lock/browser lifetimes, public intent routing, and prior rejected-token persistence fixes otherwise look coherent. The unrelated Desktop smoke layout failure is not attributed to this PR. Review was read-only; PR code was not executed.

Duncan and others added 5 commits August 29, 2026 13:38
Duncan @wpfleger96
feat(buzz-agent): add DatabricksAuthCoordinator single-flight OAuth
68a02fb 
Databricks OAuth acquisition was scattered across the TokenSource methods
with no coordination: concurrent callers (Desktop discovery, the model
picker, managed-runtime inference) could each pop their own browser, and a
just-denied attempt would immediately re-prompt. Consolidate every
acquisition path behind one coordinator on PkceOAuthTokenSource.

- Intent policy (Auto/UserInitiated/Headless) decides browser and cooldown
  behavior; all four TokenSource methods plus interactive_login route
  through one acquire()/acquire_locked() core.
- Single-flight per cache key via a std File advisory lock. try_lock is
  per open-file-description, so distinct handles contend in-process AND
  across processes — one primitive covers both with no in-memory registry.
  RAII drop releases, so a crashed holder never wedges a successor.
- Typed AuthError outcomes with stable code()/from_code() for the Phase 2
  Tauri boundary, replacing display-text matching.
- Durable cooldown sidecar: every failed browser attempt is recorded; Auto
  honors an unexpired record instead of re-launching, UserInitiated bypasses
  and clears it. Success clears it.
- Browser opener injected and invoked while the localhost callback listener
  is live, so a launch failure never returns a URL pointing at a torn-down
  listener.

The crate now uses std File::try_lock/unlock (stable in 1.89), so its
rust-version is pinned above the 1.88 workspace floor.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(buzz-agent): rejected-aware auth, typed refresh, real process tests
5f3f553 
Round-2 fixes to the DatabricksAuthCoordinator:

- Rejected-aware acquisition: `acquire_with_intent(intent, rejected)` lets
  the saved-picker recovery path replace a locally-fresh bearer the server
  just 401'd, so Auto/UserInitiated escalate to a browser instead of
  re-returning the dead token. `refresh_now`'s hardcoded Headless could not.
- Typed `RefreshOutcome`: only a token-endpoint grant rejection becomes
  RefreshRejected/browser-fallback; transport/timeout/5xx/decode failures stay
  NetworkUnavailable so a transient fault never pops a browser.
- In-process shared-future joiner (INFLIGHT/InflightSlot/LeaderGuard): a
  pre-existing joiner receives the leader's SAME failure result rather than
  acquiring the lock afterward and launching a second browser.
- Deadline holes closed: the HTTP client build error propagates (no untimed
  fallback), and every interactive timeout exits through the common outcome
  writer so TimedOut is recorded in the cooldown sidecar under the held lock.
- Real cross-process tests: a `lock-holder` child bin takes the advisory lock
  so single-flight and crash-release are proven across processes, not
  simulated with same-process handles. buzz-agent added to the Justfile
  test-unit lane so CI executes these tests.
- MSRV: use fs2::FileExt instead of std File::try_lock/unlock (1.89+),
  restoring the crate to the repo's declared 1.88 floor.

The refresh-timeout test injects a short real-time HTTP timeout rather than
pausing the clock: under start_paused tokio auto-advances into the timer while
the real loopback discovery call is still in flight, tripping the timeout on
the wrong request.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(buzz-agent): classify refresh 4xx by OAuth error, add cross-proce…
50ac03b 
…ss auth tests

Refresh classification now keys on the OAuth error body, not the bare HTTP
status class. Per RFC 6749 §5.2 only `error == "invalid_grant"` means the
refresh token is dead — the one failure a browser sign-in can repair. Every
other 4xx (`invalid_request`, `invalid_client`, `unsupported_grant_type`,
`invalid_scope`, 408, 429), an unparseable error body, and all 5xx stay in the
infrastructural bucket as `NetworkUnavailable`, so a rate limit or a
misconfigured request can no longer pop a needless browser.

Prove the cross-process single-flight contract with a real second process. The
in-memory `INFLIGHT` registry coalesces same-key callers within one process
before the file lock, so two in-process handles cannot exercise the
cross-process protocol. A new `auth-worker` test binary runs the public
coordinator API against a shared temp cache and a scripted opener, driven by
barrier-marker files, covering (a) a `UserInitiated` denial in one process
shared with an already-waiting `Auto` in another and (b) two coordinator
processes racing to one grant and one cache artifact. Rename the two tests that
falsely claimed to be cross-process to reflect the in-process single-flight
they actually exercise. Run the coordinator integration suite on the Windows CI
job so `LockFileEx` contention and crash release execute rather than compile
only.

Gate the first provider response in the steer fold test so round 1 cannot
complete until the steer is sent and observed accepted, removing a
nextest-scheduling race in which round 2's boundary could drain an empty steer
queue before the steer was dispatched.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
test(buzz-agent): trim redundant databricks auth coordinator tests
850786b 
Remove three tests whose guarantees are covered by strictly stronger
existing tests, and collapse two near-duplicate cases, without changing
any production code or what the suite proves:

- Drop the same-process denial-sidecar read test (proven cross-process by
  test_crossprocess_userinitiated_denial_shared_with_waiting_auto).
- Drop the UserInitiated rejected-fresh-bearer case (behaves identically
  to the retained Auto case here; the load-bearing contrast is the
  retained Headless variant).
- Drop the RAII lock-drop test (subsumed by the real-process crash-release
  test that kills an actual holder).
- Merge the 429 refresh classifier case into the invalid_request test as a
  second table row, preserving the no-OAuth-body distinction.
- Deduplicate the gated fake-LLM helper by threading an optional gate
  through the shared connection loop instead of copying its HTTP handler.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(buzz-agent): correct three OAuth coordinator classification gaps
9ce9be4 
Carl's re-review found three P1 correctness gaps in the Databricks auth
coordinator, each a case where an intent's promised behavior or a
transient fault was mishandled.

1. Mixed-intent coalescing keyed the in-process single-flight slot by
   browser capability alone, so a `UserInitiated` joiner could inherit an
   `Auto` leader's cooldown-suppressed `Denied`/`TimedOut` instead of the
   cooldown bypass and fresh browser it promises. Key the slot by the full
   `AuthIntent` so intents with different outcome policy never share a slot.

2. After a 401, `cached_hit` accepted any token whose bytes merely differed
   from the rejected one — including an expired sibling — skipping the
   refresh the 401 demanded. A replacement must differ AND be unexpired.

3. The code-exchange path mapped every non-success status (429, 5xx) and
   malformed 2xx bodies to terminal `ExchangeFailed`, poisoning the
   5-minute cooldown on a transient provider outage after callback.
   Mirror the refresh classifier: only a 4xx `invalid_grant` is a rejected
   grant; transport/429/5xx/malformed-2xx are `NetworkUnavailable`.

Each fix ships the test Carl asked for; all fail against the pre-fix
source and pass after.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan and others added 9 commits August 29, 2026 13:38
Duncan @wpfleger96
test(buzz-agent): cover exchange transport-timeout classification
e31f846 
The code-exchange transport branch (send().await -> NetworkUnavailable)
had no regression test: ExchangeMode only modeled Succeed/Fail/malformed.
Add a Hang variant that outlasts a short injected HTTP timeout and assert
the exchange surfaces NetworkUnavailable, opens exactly one browser, and
leaves no cooldown so a second Auto caller launches its own browser.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(buzz-agent): revalidate a joined single-flight result against the…
329b973 
… waiter's rejected token

The in-process INFLIGHT slot is keyed by (lock path, intent) only, so a
401-recovery joiner shares a leader that ran with a different rejected
value. A joiner could therefore be handed the exact token it just
reported 401-rejected (leader's cache re-read/refresh landed on that
generation), and retry the provider with known-bad credentials; the
inverse could inherit a terminal failure though a sibling had already
written a valid replacement.

On wait(), a joiner now rejects a token equal to its own rejected bytes
and runs its own bounded, leader-eligible acquisition (the slot is
evicted before publish, so this is a fresh attempt, not a re-join or a
loop), and re-checks the cache before adopting a shared failure.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(agent): make joiner revalidation lossless under contention
cb80239 
The 401-recovery joiner path had two lossy cases. On a shared leader
failure the recheck used state.try_lock(): when several waiters wake
together a try_lock loser skipped the disk read and inherited the
terminal error even though a sibling had written a valid replacement.
Replace it with a lock-free read_cache so every waiter recovers the
replacement, and so the read cannot serialize behind a new leader
holding state across its ~60s browser flow.

The joiner's bounded rerun (and a plain leader) also returned the
refresh result unchecked: a provider that re-issues the identical
access token would hand back the exact bytes the caller reported
401-rejected. Guard the refresh-success choke point so a token equal
to rejected fails with a typed error (RefreshRejected headless,
NetworkUnavailable interactive) instead of escaping or looping.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(agent): validate 401-recovery result at the true choke point
b54a823 
The rejected-token guard covered only the refresh-success arm, so an
interactive acquisition whose dead refresh fell through to a browser
sign-in could complete auth and still return the exact bearer the
caller reported 401-rejected — the same hole for a plain leader and a
colliding joiner's bounded rerun.

Move the check to acquire_leader, validating the Ok(token) of
acquire_locked against rejected once. That is the single point every
successful acquisition returns through — cache, refresh, and browser —
so it covers all paths for both leaders and reruns; the refresh-arm
guard is now redundant and removed. A match fails typed (RefreshRejected
headless, NetworkUnavailable interactive) with no loop: finish() has
already cached the token, but the next recovery passes the same
rejected, so cached_hit excludes it and forces a fresh attempt rather
than serving it back.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(buzz-agent): use then_some for the disk-recheck token move
0a10c0a 
The CI clippy config (Windows Rust, Rust Lint) rejects then() with a
closure that only moves a value; then_some is the idiomatic form here —
the value is a plain field move with no side effects.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(agent): guard 401-recovery at the persistence boundary
afee6b9 
The r5 guard validated the recovered token in acquire_leader, after
acquire_locked's finish() had already persisted it and cleared the
cooldown. A provider re-issuing the exact rejected bearer thus cached the
proven-dead token as fresh: the recovering caller got its typed failure,
but the next plain bearer() (rejected = None) or a fresh process reading
the same cache served the dead token straight back.

Move the invariant into finish() itself, the single persistence boundary
every live token flows through. A refresh- or browser-issued token equal
to the caller's rejected bytes now fails typed (NetworkUnavailable
interactive / RefreshRejected headless) before it is written or the
cooldown is cleared, so the cache is never poisoned. The redundant
post-hoc guard in acquire_leader is deleted; cached_hit and
usable_from_disk already exclude rejected, so the two live-token sites are
the only paths that can produce the bytes.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(agent): address three Carl P1s — token neutralization, cross-proc…
28f5ba6 
…ess failure adoption, Windows disk disable

P1-1: A locally-unexpired cached access token reported 401-rejected was not
neutralized — the persistence-boundary guard in finish() refused to save a
re-issued rejected token, but the original live copy remained in memory and on
disk. A later plain bearer() (rejected = None) or a fresh process would serve
the proven-dead bytes via is_expired's clock-only check. expire_rejected(),
called under the state lock at the top of acquire_locked, force-sets expires_at
to 0 on both the in-memory cell and the on-disk cache when their access token
byte-equals the reported rejected value, while leaving the refresh token intact.
Conditional on byte-equality so a sibling's concurrently-written distinct
replacement is preserved.

P1-2: Two separate processes both queued on the cross-process file lock did not
share failures — when process A held the lock and got RefreshRejected or Denied,
process B acquired the lock after A and re-ran the full flow from scratch
(second dead-refresh call, second browser). Adds an AttemptRecord sidecar
(.attempt file alongside the cache) that records a monotonically-increasing
generation, intent, and result code on each completed slow-path attempt. A
caller snapshots the current generation before queueing on the file lock;
on acquiring it, if the generation advanced and the recorded intent matches
and the result is a recognized terminal failure, it adopts that failure rather
than re-running. UserInitiated callers never adopt — they always run their own
attempt (fresh browser, cooldown bypass), mirroring the in-process INFLIGHT
contract. Two cross-process regressions: headless dead-refresh adoption
(exactly one refresh grant across two racing processes) and UserInitiated
non-adoption (the waiter opens its own browser rather than inheriting the
predecessor's denial).

P1-3: On non-Unix the token cache write path (write_private_cache) creates
files with default ACLs rather than owner-only DACL. Disable on-disk token
persistence on non-Unix by making persist() a #[cfg(unix)] no-op. Memory-only
cache is correct and safe until a Windows DACL implementation exists; lock and
cooldown sidecars hold no secrets and are unchanged. Cost is re-auth per
process on Windows.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(agent): correct adoption contract, close P1-1/P1-2/P1-3 residuals
b612198 
Fix 1 (P1-2 adoption contract): The cross-process adoption contract is
temporal, not intent-based. Remove the `UserInitiated` exclusion at the
adoption site — a caller whose pre-queue snapshot is older than the current
generation was already queued while the predecessor ran and adopts its
same-intent failure, including UserInitiated, mirroring the in-process
INFLIGHT registry that coalesces same-intent UserInitiated callers within
one process. A post-failure arrival naturally has a current-generation
snapshot and does not adopt without any special case. Invert the
cross-process UserInitiated regression: both queued workers receive Denied
with one total browser launch. Add a companion post-failure-arrival test
showing a UserInitiated source arriving after the failure runs its own
attempt.

Fix 2a (P1-2 refresh-arm attempt record): The RefreshOutcome::Refreshed
arm returned self.finish() without recording the attempt when finish()
failed typed (rejected-equal reissuance). A queued headless process would
repeat the sticky refresh instead of adopting. Record recognized terminal
finish() failures before returning.

Fix 2b (P1-2 stale snapshot in writers): All write_attempt call sites
passed the caller's pre-queue snapshot_gen rather than the current
under-lock generation. An intervening different-intent attempt that
advanced the sidecar between snapshot and lock-acquire would cause the
next attempt to rewrite the same generation, making its own queued waiters
see no advance and re-run. write_attempt now reads the current on-disk
generation itself so every completed attempt strictly advances the value.
Drop the generation parameter from write_attempt and update all call sites.

Fix 3 (P1-1 fail-closed): expire_rejected swallowed disk rewrite failures,
leaving an unexpired-but-401'd token on disk for later plain bearer() or
fresh sources. On persist failure, best-effort remove the cache file. If
removal also fails the entry stays, but read_private_cache's O_NOFOLLOW +
type check refuses non-regular-file entries, and the in-memory layer is
always neutralized unconditionally. Two regressions: one proving the
removal path fires (persist fails via EISDIR, directory removed), one
proving in-memory neutralization when disk is unreachable.

Fix 4 (P1-3 read path): Update the non-Unix read_private_cache to retire
legacy token files left by older builds rather than serving them. Adds a
platform-gated regression: on Unix the seeded token is served (expected);
on Windows the legacy file is not consumed and no new file is created.

Narrative: update AttemptRecord doc (write coverage, temporal contract,
drop 'every attempt writes' overclaim), write_attempt doc (reads current
gen), acquire_locked doc (temporal adoption, UserInitiated included),
finish() doc ('candidate-token persistence boundary'), expire_rejected doc
(removal fallback). generation field: 'strictly increasing'.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan @wpfleger96
fix(agent): scope in-process and cross-process failure adoption by re…
679e604 
…jected-token digest

P1 (Fix 1): rejection-relative failures are now scoped to the specific
rejected token that triggered them. The in-process InflightSlot publishes
the leader's rejected-token SHA-256 digest alongside the result; a joiner
whose own digest differs reruns via acquire_leader rather than inheriting a
failure that is only valid for the leader's specific rejected bytes.

Cross-process: AttemptRecord gains a rejected_digest field (SHA-256 hex,
serde(default) for backward compat). Adoption requires digest equality
(both-None matches) in addition to generation-advance + intent-match +
terminal-code.

P1 (Fix 2): Adoptors no longer write a new attempt record. Re-writing
would advance the generation, causing a post-adoption arrival to see no
further advance and run its own attempt, then a fourth caller inheriting
the re-written record — relaying the corpse indefinitely. Removed the
write_attempt call at the adoption site and documented why.

P1 (Fix 3): All disk-seeding tests gated #[cfg(unix)]. Windows CI now
exercises lock serialization, cooldown+attempt sidecars (secret-free), and
the legacy-file deletion path. The non-Unix contract is documented in the
save() doc: cross-process success handoff requires the on-disk cache, so
each process performs its own acquisition on non-Unix; failure adoption
still works via the attempt sidecar.

P1/P2 (Fix 4 — convergent): expire_rejected disk neutralization is now
fail-closed with a three-stage fallback: (1) atomic rewrite via persist(),
(2) in-place truncating overwrite via OpenOptions::write().truncate(true)
on the existing file (succeeds even when the parent directory is
non-writable — only the file's own mode matters for writing an existing
0600 file), (3) remove_file as last resort. The primary hostile case —
a 0600 file under a 0500 parent — is covered by stage (2). The vacuous
regression (directory-at-cache-path, never reached the fallback) is replaced
with a real test: readable regular token file under a 0500 parent, atomic
rewrite fails, in-place write succeeds, fresh source re-validates over the
network. Updated expire_rejected doc and PR body to match.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 force-pushed the duncan/databricks-auth-coordinator branch from 8af6afb to 679e604 Compare August 29, 2026 18:37
Duncan @wpfleger96
fix(agent): close r9 test/CI gaps — fix regressions and ungate Window…
4fb07e3 
…s tests

Four targeted fixes, all test/CI closure — production logic unchanged.

Fix-1 P1-1 regression: chmod cache_file.parent() (token_dir/databricks),
not token_dir. Assert the protected dir equals cache_file.parent() so a
future path-resolution change breaks loudly. Pre-create the lock file
before the chmod so acquire_auth_lock can open it in a 0500 dir. Rename
test: ...neutralizes_in_place_when_parent_blocks_rewrite. Mutation check:
deleting the in-place stage makes the test fail (atomic persist now fails
too, because the direct parent is 0500).

Fix-2 three-process regression (adopt does not relay failure to C):
test_crossprocess_adopter_does_not_relay_failure_to_third_process.
B queues during A (LAUNCHED_MARKER + PROCEED_MARKER), A fails (gen=1),
B adopts without re-writing (gen stays 1). Phase-5 sidecar check asserts
gen==1 — FAILS when the deleted write_attempt is restored (gen becomes 2).
C arrives post-failure, runs its own browser flow, succeeds.
Cache-free, runs on Windows.

Fix-3 digest test made deterministic: swap SucceedSticky for
HangThenSucceedSticky(300ms). Spawn A first, wait for A's READY_MARKER,
then spawn B. A holds the lock for >=300ms so B always queues before A
finishes. Assertions are now deterministic: A=RefreshRejected, B=ok,
refresh_grants==2. Mutation check documented: r8 shape yields B adopts
A's failure -> refresh_grants stays 1 -> assertion fails.

Fix-4 Windows CI (Rust Lint + Windows Rust):
- Remove two unused  bindings (worker_a/b in digest test).
- Ungate test_crossprocess_userinitiated_waiter_adopts_predecessor_denial
  and test_crossprocess_post_failure_userinitiated_runs_own_attempt —
  neither seeds nor asserts token-cache state; both are Windows-valid and
  prove the attempt-sidecar adoption contract on Windows.
- New three-process test (Fix-2) is also cache-free — runs on Windows.
- #[cfg(unix)]-gated items (SucceedSticky, HangThenSucceedSticky,
  ExchangeMode::SucceedSticky, lock_file_path, seed_fresh_rejectable,
  WorkerOutcome.bearer) are no longer stranded: they're used exclusively
  in Unix-gated tests, and their own cfg gates suppress dead-code warnings.
- Replace stray 'fail-closed' wording in expire_rejected doc with
  'bounded three-stage'.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesbillman wesbillman wesbillman requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wpfleger96 @wesbillman