fix(cli): stop a deployment under aspect.<domain> from shadowing the Aspect account - #1368
Open
gregmagolan wants to merge 2 commits into
Open
fix(cli): stop a deployment under aspect.<domain> from shadowing the Aspect account#1368gregmagolan wants to merge 2 commits into
gregmagolan wants to merge 2 commits 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
|
gregmagolan
force-pushed
the
fix/reserved-deployment-name-collision
branch
from
August 2, 2026 06:36
48670e7 to
44f76db
Compare
gregmagolan
changed the title
`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 6, 2026 02:59
0528632 to
226a58a
Compare
Deployment-name derivation used the public-suffix list to strip a host's registrable domain, so a self-hosted endpoint lost its domain: `remote.foo.bar.com` became `foo`, and `remote.aspect.foo.com` became the bare `aspect` — the built-in account's reserved name. Derivation now drops the leading service label and then only a trailing `.aspect.build`, keeping everything else verbatim. Aspect-hosted endpoints still yield a bare deployment name (`bes.gcp.awd-gha-test-dev.aspect.build` → `gcp.awd-gha-test-dev`), while self-hosted ones keep their full domain (`remote.foo.bar.com` → `foo.bar.com`), which reads unambiguously and can never collide with a reserved name. This drops the `psl` dependency. An Aspect-hosted host can still derive a reserved name (`remote.aspect.aspect.build` → `aspect`), so `upsert_deployment` remains the guarantee rather than a backstop; the docstrings now say so. Also consolidates the config overlay: `load_deployments` and the `auth status` shadowed-entry report walked the two config files separately, reading and parsing both twice per `auth status` and risking drift. Both now come from one `load_deployments_and_shadowed` over a `ConfigSource` list, with the pure overlay split out so the overlay and shadowing rules are directly testable. Test coverage: derivation is now table-driven, covering Aspect-hosted, self-hosted, multi-label suffixes, IP literals, single labels, trailing dots, and dot-anchoring (`remote.acme.notaspect.build` must keep its domain — an unanchored suffix would corrupt it to `acme.not`). Adds overlay precedence coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregmagolan
force-pushed
the
fix/reserved-deployment-name-collision
branch
from
August 7, 2026 00:26
226a58a to
e1d42db
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.
A deployment served under
aspect.<domain>took the built-in Aspect account's name and silently replaced it.aspect auth configure remote.aspect.foo.comderived the nameaspect: derivation stripped the host's registrable domain (via the public-suffix list), leaving the bareaspect.load_deploymentsoverlaysconfig.jsononto the built-in seed by name, so that entry replaced the account.summarize_deploymentthen decided "is this the account?" with aname == "aspect"comparison, so the deployment was rendered underAspect account:— showing its own issuer — while the real account disappeared.auth remove aspectrefused to help, because the name looked built-in.Account identity is a flag, not a name
Deploymentcarries a#[serde(skip)]builtinfield set only bydefault_deployment(). A deployment merely namedaspectis an ordinary deployment, and a hand-editedconfig.jsoncannot claim account status. Everyd.name == DEFAULT_DEPLOYMENT_NAMEidentity check becamed.builtin.Reserved names
aspectanddefaultmay not be taken by a configured deployment — the first is the account's name, the second its credential profile (DEFAULT_PROFILE), and a deployment's credential is filed under its own name, so either would share the account's slot..aspect.buildmeans a self-hosted host keeps its full domain:remote.aspect.foo.com→aspect.foo.com,remote.foo.bar.com→foo.bar.com. Aspect-hosted hosts still reduce to a bare name (bes.gcp.awd-gha-test-dev.aspect.build→gcp.awd-gha-test-dev). This replaces the public-suffix approach and drops thepsldependency.upsert_deploymentrejects them. Everyconfigurepath funnels through it, so both an explicit--deployment=aspectand a name derived from an Aspect-hosted host (remote.aspect.aspect.build→aspect) are caught.config.jsonentry under a reserved name is ignored rather than allowed to replace the seed. Skipping rather than erroring keeps a config that already holds such an entry usable — erroring would fail every auth command, including theauth removeneeded to clean it up.auth use defaultis unaffected: onlyDEFAULT_DEPLOYMENT_NAMEis the "clear the configured default" sentinel, sodefaultfalls through to the unknown-deployment check instead of silently clearing every default.Reporting and recovery
auth statuswarns for each ignored entry, naming the file that declares it. The remedy depends on that file: the user's config is fixable withauth remove <name>; a checked-in repo config is told to edit the entry directly, sinceauth removeonly edits the user's file.auth removedeletes a reserved-name file entry while still refusing to remove the account itself.Sharing one issuer across several deployments is unaffected — nothing keys on the issuer.
Existing configs holding an
aspectentry need one manual step, since it predates the guard:aspect auth remove aspect, then re-runaspect auth configure <host>.auth statusprints exactly that.Changes are visible to end-users: yes
auth removeabove, whichauth statusprompts forSuggested release notes
aspect auth configureno longer derives a name that collides with the built-in Aspect account, which silently replaced it and made the deployment appear missing fromaspect auth status. Self-hosted hosts now keep their domain (remote.aspect.foo.com→aspect.foo.com,remote.foo.bar.com→foo.bar.com); Aspect-hosted hosts are unchanged (remote.acme.aspect.build→acme).aspect auth statuswarns when aconfig.jsondeployment is ignored for using a reserved name (aspect,default), naming the file that declares it and how to fix it.aspect auth remove <name>can delete a config entry that took a reserved name, while still refusing to remove the built-in account.Test plan
With a
~/.aspect/config.jsonholding{"name": "aspect", "hosts": ["remote.aspect.<domain>"]},aspect auth statusshows the genuine account (Issuer auth.aspect.build) plus a warning naming the ignored entry and its file.aspect auth remove aspectdeletes that entry; with no such entry it still errors withthe built-in "aspect" account cannot be removed.Rust unit tests (
cargo test -p axl-runtime, 370 passed):deployment_name_from_host_strips_service_label_and_aspect_domain— table-driven over Aspect-hosted, self-hosted, multi-label suffixes (.co.uk), IP literals, single labels, trailing dots, and dot-anchoring (remote.acme.notaspect.buildkeeps its domain; an unanchored suffix would corrupt it toacme.not).deployment_name_from_host_collides_only_under_aspects_own_domain— self-hosted hosts can never derive a reserved name; Aspect-hosted ones can, andupsert_deploymentrejects those.upsert_deployment_rejects_reserved_names,config_json_entry_cannot_shadow_the_account,remove_clears_a_shadowed_entry_but_protects_the_account.shadowed_entries_are_attributed_to_their_config_file— repo vs user attribution, dedup across both files, both reserved names.overlay_replaces_by_name_in_source_order— overlay precedence and seed-default reconciliation.seed_identity_is_the_builtin_flag_not_the_name,builtin_flag_is_not_deserialized_from_config,apply_set_default_switches_and_clears(coversauth use default).AXL:
aspect dev test-auth— 4 tests, including_test_shadowed_warningfor the warning wording in both the user-config and repo-config cases.cargo fmt --checkclean;cargo clippyfindings inauth.rsunchanged frommain(9 pre-existing).