fix(cli): stop a configured deployment from shadowing the Aspect account - #1368
Open
gregmagolan wants to merge 1 commit into
Open
fix(cli): stop a configured deployment from shadowing the Aspect account#1368gregmagolan wants to merge 1 commit into
gregmagolan wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48670e761f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
✨ Aspect Workflows Tasks📅 Sun Aug 2 06:43:43 UTC 2026 ❌ 1 failed task
|
`aspect auth configure remote.aspect.foo.com` derived the deployment name `aspect` — the public-suffix list makes `foo.com` the registrable domain, so stripping it from `aspect.foo.com` leaves `aspect`, which is `DEFAULT_DEPLOYMENT_NAME`. Because `load_deployments` overlaid config.json onto the built-in seed by name, the new entry silently *replaced* the Aspect account: `auth status` rendered the deployment under "Aspect account:" (its `builtin` flag was a `name == "aspect"` comparison), the account itself vanished, and `auth remove aspect` refused to help because the name looked built-in. Seed identity is now a `#[serde(skip)]` `builtin` field set only by `default_deployment()`, so a deployment merely *named* `aspect` is an ordinary deployment and a hand-edited config.json cannot claim account status. Three layers then keep the name from being taken at all: derivation keeps the registrable domain when it would produce a reserved name (`aspect.foo.com`), `upsert_deployment` rejects a reserved name whatever its origin (the choke point every `configure` path shares, covering explicit `--deployment`), and `load_deployments` skips — rather than honors — a reserved-name config entry. Skipping rather than erroring on load is deliberate: an already-shadowed config.json would otherwise fail every auth command, including the `auth remove` needed to clean it up. `auth status` now warns with that recovery step, and `auth remove` deletes a reserved-name *file entry* while still refusing to remove the account itself. `default` is reserved alongside `aspect`: a deployment's credential is filed under its own name, and `DEFAULT_PROFILE` is `"default"`, so such a deployment would share the account's credential slot. It is reserved from being *configured* only — `apply_set_default` still treats `DEFAULT_DEPLOYMENT_NAME` alone as the "clear the default" sentinel, so `auth use default` fails the unknown-deployment check instead of silently clearing every configured default. The `auth status` warning names the config file that declares the ignored entry and adapts its advice: `auth remove` only edits the user's config, so a checked-in repo config is told to edit the file directly. Sharing an issuer across deployments is unaffected — nothing keys on the issuer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregmagolan
force-pushed
the
fix/reserved-deployment-name-collision
branch
from
August 2, 2026 06:36
48670e7 to
44f76db
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
aspect auth configure remote.aspect.foo.comderived the deployment nameaspect. The public-suffix list makesfoo.comthe registrable domain, so stripping it fromaspect.foo.comleavesaspect— which isDEFAULT_DEPLOYMENT_NAME, the built-in Aspect account. Sinceload_deploymentsoverlaidconfig.jsononto the seed by name, the new entry silently replaced the account.The visible symptom was the deployment not appearing in
aspect auth status. It was actually there — rendered in the wrong section.summarize_deploymentdecidedbuiltinwith aname == "aspect"comparison, so the configured entry was tagged as the account and printed underAspect account:, showing that deployment's issuer rather than the account'sauth.aspect.build. The real account disappeared, andauth remove aspectrefused to help because the name looked built-in.Seed identity is now a
#[serde(skip)]builtinfield set only bydefault_deployment()— so a deployment merely namedaspectis an ordinary deployment, and a hand-editedconfig.jsoncannot claim account status. Three layers then stop the name being taken at all: derivation keeps the registrable domain when it would otherwise produce a reserved name (aspect.foo.com);upsert_deploymentrejects a reserved name whatever its origin (the choke point everyconfigurepath shares, so explicit--deployment=aspectis covered too); andload_deploymentsskips a reserved-name entry instead of honoring it.Skipping rather than erroring on load is deliberate: an already-shadowed
config.jsonwould otherwise fail every auth command — including theauth removeneeded to clean it up.auth statuswarns with that recovery step, andauth removenow deletes a reserved-name file entry while still refusing to remove the account itself.defaultis reserved alongsideaspect: a deployment's credential is filed under its own name andDEFAULT_PROFILEis"default", so such a deployment would share the account's credential slot. It is reserved from being configured only — see theauth usefix below.Sharing one issuer across several deployments is unaffected: nothing keys on the issuer.
Anyone already in this state needs one manual step, since the bad entry predates the guard:
aspect auth remove aspect, then re-runaspect auth configure <host>.auth statusnow prints exactly that instruction.Review feedback addressed
Both Codex findings were reproduced before fixing, and each now has a test that fails without the fix.
auth use defaultcleared every default (P2). Widening theapply_set_defaultsentinel fromDEFAULT_DEPLOYMENT_NAMEto all ofRESERVED_NAMESmeantSome("default")mapped toNone— soaspect auth use default, a name nobody can configure, silently cleared all configured defaults, reported success, and sent the next--remoteto the built-in account. The sentinel is back to the account name alone, sodefaultfalls through to the unknown-deployment check. Reproduced via a probe test that printed[("acme", false), ("emca", false)]before the fix.Repo-config warnings pointed at a file
auth removecannot edit (P2).shadowedcollected skipped entries from both$ASPECT_WORKSPACE/.aspect/config.jsonand~/.aspect/config.jsonbut returned bare names, so the advice offeredauth removeeven for a checked-in repo entry — which would error, or delete an unrelated user entry of the same name, while the warning persisted. Skipped entries are nowShadowedDeploymentrows carrying the declaringpathand whether it is the user's config; the repo case is told to edit that file directly. A name in both files is reported once, against the file that actually shadows.Changes are visible to end-users: yes
config.jsonalready holding anaspectentry needs the one-timeauth removeabove, whichauth statusnow prompts forSuggested release notes
aspect auth configurederiving the reserved nameaspectfor a deployment whose host sits underaspect.<domain>(e.g.remote.aspect.foo.com), which silently replaced the built-in Aspect account and made the deployment appear missing fromaspect auth status. Such hosts now derive a name that includes the domain (aspect.foo.com).aspect auth statusnow warns when aconfig.jsondeployment is ignored for using a reserved name, naming the file that declares it and how to fix it.aspect auth remove <name>can now delete a config entry that took a reserved name, while still refusing to remove the built-in account.Test plan
Against a
~/.aspect/config.jsoncontaining the reported entry ("name": "aspect"with a host underaspect.<domain>),aspect auth statusnow shows the genuine account (Issuer auth.aspect.build) plus a warning naming the ignored entry and its file, instead of the configured deployment wearing the account's clothes.aspect auth remove aspectdeletes that entry; with no such entry present it still errors withthe built-in "aspect" account cannot be removed.New Rust unit tests, each verified to fail if the reserved-name set is emptied:
deployment_name_from_host_avoids_reserved_names—remote.aspect.foo.com→aspect.foo.com,remote.aspect.aspect.build→aspect.aspect.build,remote.aspect.build→aspect.buildleft alone, and a bareaspecthost (no domain to fall back to) documented as caught downstream.upsert_deployment_rejects_reserved_names— the write choke point, for both reserved names.reported_config_json_no_longer_shadows_the_account— the reportedconfig.jsonshape, parsed from JSON through the real loader helper.apply_set_default_switches_and_clears— extended for theauth use defaultregression.shadowed_entries_are_attributed_to_their_config_file— repo vs user attribution, dedup across both, and both reserved names.seed_identity_is_the_builtin_flag_not_the_name,builtin_flag_is_not_deserialized_from_config,remove_clears_a_shadowed_entry_but_protects_the_account.New AXL test
_test_shadowed_warninginauth_test.axlcovers the warning wording for both sources; it fails if the repo case is givenauth removeadvice.cargo test -p axl-runtime— 370 passed.aspect dev test-auth(AXL) — OK, 4 tests.cargo fmt --checkclean;cargo clippyfindings inauth.rsunchanged frommain(9 pre-existing).