Provider accounts P4: destructive step — remove moved keys from `worker_env`; delete ConfigDir's old fallbacks by ryanrborn · Pull Request #1947 · ryanrborn/arbiter

ryanrborn · 2026-09-21T21:52:43Z

Summary

Phase P4 of docs/provider-account-design.md (§7.5). Originally this PR
deleted ConfigDir.oauth_token/1's two flag-off fallbacks (the arbiter
server's own process environment, and the install-wide-unambiguous
workspace token for a workspace-less spawn) on the theory that the
provider-account join was now the only path.

The coordinator held the PR as a draft before auto-merge: with
:provider_accounts_enabled false (the default, and the coordinator's own
production install's setting), that deletion silently dropped
CLAUDE_CODE_OAUTH_TOKEN from every workspace-less spawn — the
CredentialWatchdog probe, the quota probe, ad-hoc code-review checks —
because this install has never run mix arbiter.accounts.migrate and
authenticates those spawns through the now-deleted fallback chain.
v0.1.68's release notes promised flag-off behavior would match v0.1.67, and
this diff broke that promise for exactly the install running the
coordinator itself.

Operator ruling (ADR on PR #1947, option 1): keep the flag-off legacy
chain until the flag is deliberately turned on. This PR now:

Restores ConfigDir.oauth_token/1's pre-P4 precedence chain for every
flag-off spawn, workspace-less or not: workspace worker_env → the
arbiter server's own process environment (.arbiter.env / the service
unit) → the install-wide token, only when unambiguous across every
workspace that defines one. server_oauth_token/0, install_oauth_token/0,
workspace_oauth_tokens/0, any_workspace_oauth_token?/0 and
warn_token_scan_failed/1 are back.
Leaves the flag-on path (the provider-account join, P3/this PR's earlier
work) untouched — it is still the only source once
:provider_accounts_enabled is on, and still guards a workspace whose
own worker_env still carries a token by raising
MissingCredentialError rather than silently downgrading it.
Keeps the explicit {"CLAUDE_CODE_OAUTH_TOKEN", false} unset pair from
round 1's review (rather than reverting to a bare []) for the case
where the whole chain — flag on or off — comes up genuinely empty, so
Port.open's ambient environment inheritance still can't leak a stale
token into a spawn that was deliberately handed none. The lockstep
invariant (credential-seeding suppressed iff a token pair is injected)
holds across every flag/workspace shape;
config_dir_workspace_test.exs's "env/1 lockstep" describe block
asserts it directly.
Rewrites the README's Claude CLI authentication section and the
Claude.spawn_env/1 doc comment to describe the flag-off chain as still
supported, and the provider-account migration path (census → migrate →
flip the flag) as the way off it.

Net effect: this PR no longer changes flag-off behavior at all versus
v0.1.68/v0.1.67. It only prepares the ConfigDir module for the eventual
"flip" release, when :provider_accounts_enabled becomes permanently true
and the legacy chain is deleted for real. Per the ticket's acceptance 3,
that flip (and the flag's removal) is not done here — this PR is
scoped to keeping the flag-off path whole while the account-join path
exists alongside it.

Release classification (§7.5)

This PR's title calls it "the destructive step," but by the time this PR
merges, the actual destructive worker_env removal described in §7.5 as
"Release N+2" — deleting the allowlisted credential keys from
workspaces.encrypted_worker_env, gated on the encrypted
provider_account_migration_backups row, restorable via
mix arbiter.accounts.rollback — already shipped in P2 (bd-77j2if),
not here. See docs/provider-account-design.md's "What P2 actually
shipped" note (§7.5) for why P2 folded N/N+1/N+2 into one release. The
acceptance criteria this ticket names as the destructive step's proof are
satisfied by tests already on main:

Allowlisted keys stripped from worker_env, asserted against seeded
data: apps/arbiter/test/arbiter/accounts/migrate_test.exs:81.
Migration gated on the backup row existing before the strip:
migrate_test.exs:145.
mix arbiter.accounts.rollback proven to restore a workspace's
credential after the destructive step, using the backup row:
migrate_test.exs:249.

This PR is consequently not the point of no return. With
:provider_accounts_enabled false (the default), flag-off behavior is
byte-for-byte v0.1.68 — see "Operator ruling" above. What actually remains
destructive/irreversible — deleting ConfigDir's flag-off fallback chain
and removing the flag itself — is deferred to a new P13 "flip" phase,
tracked in docs/provider-account-design.md §10.

Test plan

MIX_ENV=test mix test (apps/arbiter): 6914 tests (1 doctest, 6
properties), 0 non-baseline failures. One test/arbiter/sessions/stream_test.exs
flake (an unrelated assert_receive timing test) reproduced clean in
isolation (mix test test/arbiter/sessions/stream_test.exs → 50/50).
mix format --check-formatted — clean.
mix credo --strict — 9272 mods/funs, no issues.
Targeted re-run of every touched test file (config_dir_test.exs,
config_dir_workspace_test.exs, claude_test.exs, preflight_test.exs,
read_flip_test.exs, migrate_test.exs): 118 tests, 0 failures.
Restored/added coverage: flag-off workspace-less spawn plus an
install-wide-unambiguous token now injects that token
(config_dir_workspace_test.exs); flag-off server-env token with no
workspace token still injects the server-env token
(config_dir_test.exs, claude_test.exs); flag-on remains
account-only (read_flip_test.exs, existing lockstep tests).

References

Task: bd-cblemv
Design doc: docs/provider-account-design.md §7.5
Prerequisite: P3 (bd-aiodva)
Operator ruling: Provider accounts P4: destructive step — remove moved keys from worker_env; delete ConfigDir's old fallbacks #1947 (comment)
Closes Provider accounts P4: destructive step — remove moved keys from worker_env; delete ConfigDir's old fallbacks #1850

🤖 Generated with Claude Code

…wide fallbacks (§7.5, destructive step) The worker_env removal + gating backup row + rollback (§7.5's Release N+2) already shipped as part of P2 (bd-77j2if), which folded the destructive step into itself rather than shipping it as a separate release. What's left for P4 is ConfigDir.oauth_token/1's now-dead steps 2 and 3: the arbiter server's own process environment, and the install-wide-unambiguous workspace token for a spawn with no workspace in hand (bd-bw3466). The provider-account join is now the only path, workspace-less or not. Delete server_oauth_token/1, install_oauth_token/0, workspace_oauth_tokens/0, any_workspace_oauth_token?/0 and their warn_token_scan_failed/1 helper (all unused outside this module post-deletion, confirmed by git grep). Update the surrounding tests and doc comments that pinned the old fallback behavior. Existing P2 tests (test/arbiter/accounts/migrate_test.exs) already prove acceptance 1/2/4: no allowlisted key survives worker_env post-migration, the backup row is written unconditionally before any strip (so removal without a backup can't happen), and rollback restores a workspace's credential.

…DME migration path Round-1 review requested changes: - (High) ConfigDir.env/1's oauth_token_pairs/1 now emits an explicit {"CLAUDE_CODE_OAUTH_TOKEN", false} unset pair rather than omitting the pair when no token is configured. Port.open's {:env, ...} extends rather than replaces the BEAM's own process environment, so a bare omission let a server-process CLAUDE_CODE_OAUTH_TOKEN (the README-recommended .arbiter.env setup) still reach every worker spawn even though the seeding gate had suppressed .credentials.json — reopening bd-6umoh9's dual-refresher race. `false` is understood by both Port.open directly and by Arbiter.Worker.ReleaseEnv.cmd/3 (System.cmd's env, which maps it to `nil`). Updated every test asserting the old `env/1`/`spawn_env/1` shapes across config_dir_test.exs, config_dir_workspace_test.exs, claude_test.exs, preflight_test.exs and read_flip_test.exs, and added a lockstep property test parameterised over both flag states. - (High) README's Claude CLI authentication section described the removed server-env fallback as the recommended setup with no migration path off it. Rewrote it around provider accounts as the supported path, and spelled out the required operator steps for a server-env-configured install before upgrading past this release (seed a workspace worker_env value so census can see it, run census + migrate, flip the flag, then remove the now-inert .arbiter.env var). - (Medium) Claude.spawn_env/1's doc comment described the deleted server-env fallback; rewrote it to describe the account-join / explicit- unset behaviour this PR ships. - (Medium) config_dir_workspace_test.exs's lockstep-invariant test was deleted without a replacement while the moduledoc still advertised the property; restored it as "env/1 lockstep: seeding suppressed iff a token pair is injected", parameterised over six flag/workspace shapes (flag on and off, with and without an account/workspace token, with a stale server-env value present). Full suite: 6908 tests, 0 non-baseline failures (two known sandbox- connection-kill flakes, both pass in isolation — bd-5scl0c); arbiter_web 1558/1558, arbiter_cli 799/799; mix format --check-formatted clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

ConfigDir.oauth_token_pairs/1 (17ed581) now always emits a pair, either the token or an explicit {token_var, false} unset, so ConfigDir.env/1 can no longer return []. The `if(env == [], ...)` branch that skipped passing :env to invoke_via_stdin's opts was therefore dead, which OTP 28's :exact_compare warning class caught — dialyxir 1.4.7 can't filter that warning type (see .dialyzer_ignore.exs group 5), so the fix is in source, matching how pr_patrol.ex handled the same class. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

ryanrborn · 2026-09-21T23:06:31Z

Held as draft by the coordinator before auto-merge.

With :provider_accounts_enabled false (the default, and this install's setting), oauth_token/1 becomes workspace_oauth_token(workspace) only. A spawn with no workspace (CredentialWatchdog probe, quota probe, code-review checks) then resolves to nil, where on main it falls through to the install-wide unambiguous token (step 3). This install's workers get CLAUDE_CODE_OAUTH_TOKEN from workspace worker_env and not from the server environment, and mix arbiter.accounts.migrate has never been run here. So merging would strip auth from every workspace-less spawn on the coordinator's own server, and the PR body says a flag-off unmigrated install "loses credentials rather than degrading." v0.1.68 just shipped telling operators the flag-off path behaves as v0.1.67.

Needs a decision before this merges: keep the step-3 floor for workspace-less spawns until the flag flip, or gate P4 on the install being migrated / the flag being on.

https://claude.ai/code/session_013bPK7XAK5cdTuNTDkuDCzY

…ing on #1947 The coordinator held PR #1947 as a draft: with :provider_accounts_enabled false (the default, and the coordinator's own install's setting), workspace-less spawns (CredentialWatchdog, quota probe, code-review checks) lost CLAUDE_CODE_OAUTH_TOKEN once P4 deleted the server-env and install-wide-unambiguous fallbacks, even though v0.1.68's release notes promised flag-off equals v0.1.67 behavior and this install has never run mix arbiter.accounts.migrate. Operator ruling (option 1): keep the fallback until the flag is deliberately turned on. Restores ConfigDir.oauth_token/1's pre-P4 legacy chain (workspace worker_env -> server process env -> install-wide unambiguous workspace token) for every flag-off spawn, workspace-less or not, by reinstating server_oauth_token/0, install_oauth_token/0, workspace_oauth_tokens/0, any_workspace_oauth_token?/0 and warn_token_scan_failed/1. The flag-on account-only path (P3/P4's own new work) is untouched. Keeps the explicit {"CLAUDE_CODE_OAUTH_TOKEN", false} unset from the prior review round for the case where the whole chain comes up empty, so the lockstep invariant (seeding suppressed iff a token is injected) still holds. Updates the doc comments and test suites (config_dir_test.exs, config_dir_workspace_test.exs, claude_test.exs, preflight_test.exs, read_flip_test.exs) to pin the restored behavior, and rewrites README's Claude CLI authentication section to describe the flag-off legacy chain as still supported alongside the provider-account migration path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…-accounts-p4-destructive-step-remove

- docs/provider-account-design.md: record the operator ruling on #1947 that the worker_env removal already shipped in P2, and that the ConfigDir fallback deletion (and :provider_accounts_enabled removal) is deferred to a new P13 "flip" phase — line 639 and the §10 P4 row no longer claim work this PR didn't do. - claude_test.exs: revert "never remaps the OAuth token onto ANTHROPIC_API_KEY" to actually exercise non-remapping (spawn_env([]) + refute) instead of duplicating the adjacent "composes alongside" test with an explicit api_key: input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Round-2 review finding: the "What P4 actually shipped" note claimed P4 shipped no code deletion in config_dir.ex, and §7.5 still described the server-env/install-wide fallbacks as a floor under the flag-on workspace-less path. Both are wrong — P4 removed that floor from the flag-on path (account_oauth_token/1 answers nil instead of falling through to legacy_oauth_token/1, config_dir.ex:270/:289) and added the explicit CLAUDE_CODE_OAUTH_TOKEN unset pair. The fallbacks only survive on the flag-off path (config_dir.ex:263-265), which is what P13 is scoped to delete. Corrected the two passages and narrowed the P13 row accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

- Update design doc Status line to note account model is opt-in (`:provider_accounts_enabled` defaults false) - Add "What P4 actually shipped" section documenting the deferred destructive step (flagged off per #1947 reversal commit 7eaaaaf) - Add account-model path subsection to README explaining `arb account attach` / `arb account rotate` as alternative to install-wide token setup (requires `:provider_accounts_enabled`) - Clarify precedence and use-case for account-scoped credentials Addresses reviewer findings from P12 acceptance criteria: - Verifies Status line reflects actual implementation state - Documents the one significant design deviation (P4 deferral) - Provides operators the account-model onboarding docs named in task

ryanrborn mentioned this pull request Sep 21, 2026

Provider accounts P4: destructive step — remove moved keys from worker_env; delete ConfigDir's old fallbacks #1850

Open

ryanrborn and others added 2 commits September 21, 2026 18:41

ryanrborn marked this pull request as draft September 21, 2026 23:06

ryanrborn and others added 4 commits September 22, 2026 11:45

Merge remote-tracking branch 'origin/main' into feature/1850-provider…

549726e

…-accounts-p4-destructive-step-remove

ryanrborn marked this pull request as ready for review September 22, 2026 21:05

ryanrborn mentioned this pull request Sep 23, 2026

Approved, green PRs are orphaned when their worker exits before auto-merge can succeed: nothing retries the merge #2002

Open

ryanrborn mentioned this pull request Sep 23, 2026

Provider accounts P12: docs + moduledocs — retire the "quota is per workspace" mental model #2003

Open

6 tasks

Navigation Menu

Provider accounts P4: destructive step — remove moved keys from `worker_env`; delete ConfigDir's old fallbacks - #1947

Provider accounts P4: destructive step — remove moved keys from `worker_env`; delete ConfigDir's old fallbacks#1947
ryanrborn wants to merge 7 commits into
mainfrom
feature/1850-provider-accounts-p4-destructive-step-remove

ryanrborn commented Sep 21, 2026 •

edited

Loading

Uh oh!

ryanrborn commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ryanrborn commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Release classification (§7.5)

Test plan

References

Uh oh!

ryanrborn commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ryanrborn commented Sep 21, 2026 •

edited

Loading