feat(domain): delegate a bought domain's zone, and take it to another registrar - #263
Conversation
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/domain.test.ts">
<violation number="1" location="test/domain.test.ts:422">
P3: The new commands `domainNameserversSet`, `domainNameserversReset`, and `domainTransferLock` each print the full `Purchased` domain object under `--json`, but none of the added tests exercises those `--json` branches, while the PR calls out JSON shape as an addressed edge case. Add a `--json` test for each (e.g. asserting `JSON.parse(out())` equals the returned domain) so the JSON contract of the new commands is locked down.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -1,5 +1,5 @@ | |||
| import { describe, it, expect, vi, afterEach, afterAll } from 'vitest' | |||
There was a problem hiding this comment.
P3: The new commands domainNameserversSet, domainNameserversReset, and domainTransferLock each print the full Purchased domain object under --json, but none of the added tests exercises those --json branches, while the PR calls out JSON shape as an addressed edge case. Add a --json test for each (e.g. asserting JSON.parse(out()) equals the returned domain) so the JSON contract of the new commands is locked down.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/domain.test.ts, line 422:
<comment>The new commands `domainNameserversSet`, `domainNameserversReset`, and `domainTransferLock` each print the full `Purchased` domain object under `--json`, but none of the added tests exercises those `--json` branches, while the PR calls out JSON shape as an addressed edge case. Add a `--json` test for each (e.g. asserting `JSON.parse(out())` equals the returned domain) so the JSON contract of the new commands is locked down.</comment>
<file context>
@@ -319,3 +319,107 @@ describe('domain records', () => {
+ // The one new shape that is not a purchased domain.
+ it('answers --json with the code object, not the domain', async () => {
+ const { deps: d } = deps({ '/auth-code': { authCode: 'EPP-123' } })
+ await domainTransferCode('myapp.com', { json: true }, d)
+ expect(JSON.parse(out())).toEqual({ authCode: 'EPP-123' })
+ })
</file context>
There was a problem hiding this comment.
Reviewed by Wang Miao
This adds domain nameservers set|reset and domain transfer lock|code, wired straight onto the platform routes that already exist for them, with the delegated state folded into the shared domain renderer. The verbs, the request shapes and the transfer-code POST all match the control plane, and the tests cover them well — but the renderer's follow-up hints were not reconciled with the new state or the new callers, so the primary command's own output tells you to run something the platform refuses, and it does it against the wrong org under --org. Both are one-line fixes in lines this diff already touches; I'd want them before merge.
After a delegation the CLI's first hint is an attach the platform refuses
important · defect · correctness · src/commands/domain.ts:105
Delegating marks every hostname failed on the platform side (src/domains/service.ts:627, :722), so d.hostnames.every(h => h.state === 'failed') is always true on the answer nameservers set prints — and vacuously true for a delegated domain with no hostnames. The "nothing serving" line therefore fires on the success path of the new command and names insta domain attach <host>, which the attach path rejects with a 409 (src/domains/service.ts:384), immediately contradicted by the very next line this change adds. I ran it: the output is nothing serving — insta domain attach www.myapp.com followed by delegated to … — attach is refused until: insta domain nameservers reset myapp.com. Gating the existing branch on !d.delegated leaves only the correct instruction.
Evidence
read-the-code — src/commands/domain.ts:102-115, :151-158; platform src/domains/service.ts:204, :384, :620-630, :718-724; ran the renderer against the PR's own delegated fixture
The new hints re-emit follow-up commands for the wrong org under --org
important · defect · conventions · src/commands/domain.ts:112
domainLines' linked parameter exists precisely to drop next-action commands when --org names an org they do not act on — added in 2686697 and pinned by "drops the follow-up commands when --org names an org they do not act on". The new delegated line ignores linked entirely and prints insta domain nameservers reset <domain> with no --org, and domainNameserversSet/Reset call domainLines(r) with the default linked = true even when opts.org was given. So insta domain list --org org9 on a delegated domain, and insta domain nameservers set --org org9 …, both hand the reader a reset that resolves the linked project's org instead — and the attach hint above it, which takes no --org at all. Pass !opts.org from the new callers and gate the delegated line on linked (or carry the --org into the printed command).
Evidence
read-the-code — src/commands/domain.ts:102-115, :124-130, :151-165; test/domain.test.ts:236-244; git log -L102,116 showing 2686697 introducing linked; ran domainNameserversSet with { org: 'org9' } and saw both hints printed unqualified
There was a problem hiding this comment.
Reviewed by Yang Dong
This adds registrar nameserver delegation/reset and transfer-lock/auth-code commands over the new platform endpoints. The endpoint wiring is sound, but the generated guidance includes commands that cannot succeed or target the wrong organization, and the required agent CLI reference is missing, so I would request changes.
Delegation prints an attach command that the platform will reject
important · defect · correctness · src/commands/domain.ts:105
After delegating, the platform marks every serving hostname failed, so this condition emits insta domain attach ...; an empty hostname list also passes it vacuously. Because the returned domain is delegated, that attach is necessarily refused—as the next output line itself says—so suppress the retry when d.delegated is true and present reset as the repair.
Evidence
read-the-code — src/commands/domain.ts:102-114, test/domain.test.ts:354-366, platform/src/domains/service.ts:312-359
Follow-up commands discard the explicit --org target
important · defect · correctness · src/commands/domain.ts:112
All four new commands accept --org and correctly send their initial request there, but the printed nameservers reset, domain attach, and transfer code commands omit it. Running one after a cross-org operation therefore falls back to the linked project’s organization, producing a 404 or acting on a same-named domain in the wrong org; preserve opts.org in these commands or suppress the hints as existing list/status output does.
Evidence
read-the-code — src/commands/domain.ts:102-112, src/commands/domain.ts:151-179, src/commands/billing.ts:7-13, src/index.ts:218-232, test/domain.test.ts:237-245
The new command surface is absent from the required agent reference
important · judgement · conventions · src/index.ts:218
The repository explicitly requires every command or flag change to update skills/insta/cli-reference.md in the same change set, but that reference still stops at the existing domain record commands. Add entries for nameserver set/reset and transfer lock/code, including their --org and human/agent authorization behavior, so agents can discover and safely hand off these operations.
Evidence
read-the-code — .claude/skills/developing-insta-cli/SKILL.md:36-38, src/index.ts:218-232, skills/insta/cli-reference.md:75-80, platform/src/govern/agent-routes.ts:339-365
… registrar Platform #484 added both exits; the CLI had neither. - `domain nameservers show|set|reset` — whose nameservers the registrar holds, delegate the zone away, put it back. `set` takes down every hostname the domain serves, because the records an attach published live in the zone being left, so it prints each one with the platform's reason rather than reporting success. - `domain transfer unlock|lock|code` — the registrar lock and the EPP code, two steps because the code alone moves nothing. `unlock` names the registry's own 60-day lock when that still holds the domain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2294fed to
e3ebaad
Compare
|
Head is now The success path offered an attach the same answer refuses (both reviewers)Confirmed by running it. Delegating fails every hostname on the platform side, so The branch is now gated on The hints ignored
|
There was a problem hiding this comment.
Reviewed by Wang Miao
Both findings from round 1 are fixed in e3ebaad, and I checked rather than took the word for it: the "nothing serving" retry is now gated on !d.delegated, so the success path of nameservers set no longer offers an attach the platform refuses; and the delegated hint, the reset command and the transfer-code hint all follow linked/opts.org, so --org gets the fact without a command that would act on the other org. I ran the renderer in both modes and the new tests pin each case. The rest of the change is unchanged from last round and still lines up with the platform routes, the PurchasedDomain shape and the repo's on|off argument convention.
No findings. Approve.
There was a problem hiding this comment.
Reviewed by Yang Dong
This adds registrar nameserver delegation/reset and transfer-lock/auth-code commands with sound endpoint wiring and user guidance. However, the new public command surface is absent from the mandatory agent reference, so I would request changes.
The new commands are missing from the required agent reference
important · judgement · conventions · src/index.ts:218
The repository requires every command or flag change to update skills/insta/cli-reference.md in the same change set, but its domain section still stops at the existing record commands. Add entries for nameserver set/reset and transfer lock/code, including --org, admin requirements, and the fact that agent credentials are refused, so agents can discover and safely hand off these operations.
Evidence
read-the-code — AGENTS.md:15-17, .claude/skills/developing-insta-cli/SKILL.md:36-38, src/index.ts:218-232, skills/insta/cli-reference.md:75-80, platform/src/govern/agent-routes.ts:339-366, platform/src/server.ts:3363-3436
Platform #484 shipped both ways a bought domain leaves; the CLI had neither.
insta domain nameservers set <domain> <ns...>/reset <domain>— delegate the zone away, or put it back.insta domain transfer lock <domain> <on|off>/code <domain>— the registrar lock and the EPP code.Where the delegation shows up
Not in a command of its own.
domainLines— the one rendererlistandstatusalready print — now carries it, soinsta domain status myapp.comsays the domain is delegated and names the reset that undoes it. My first draft added anameservers showthat re-read the same endpoint and re-implementedstatus's find-by-name; it leftlistandstatusstill silent about a domain nothing can attach to, which is the surface a user is already looking at.setandresetprint that same renderer rather than a bespoke summary. It already renders a failed hostname aswww.myapp.com failed → web — <reason>, de-dupes the apex/www pair, and names which hostname to re-attach — all of which my own version did worse, and one of whose lines printed the platform's 130-character reason once per hostname whendelegatedReasonis a single shared string.Two things the first draft claimed that are not true
reset. The platform deliberately does not delete the published records (service.ts: "The published records STAY: a zone nothing answers from is inert"), so reset republishes nothing — it un-delegates, and anattachre-adopts what is already there. The line was also unconditional, so areseton a healthy domain told the user to go attach something.serverTransferProhibited) is a different mechanism a user would go asking the wrong party about.PurchasedDomain.transferLockExpiresAtsays "ICANN 60-day lock after registration". Corrected here and in the skills row;platform/docs/domain-purchase.mdstill says "registry's own" and is the remaining copy of the error.transfer lock <domain> <on|off>is one command with a mode, matchingcompute always-on <mode>rather than inventing a pair of sibling verbs.Coverage
40 tests in
test/domain.test.ts. Five mechanisms my own review found unguarded are pinned now: the lapsed-ICANN-lock comparison (the platform never clears the date, so an old domain always carries a past one and only this comparison suppresses the claim), the trailing-dot strip, the delegated line indomainLines, theon|offguard, andtransfer code --json— the one new shape that is not a purchased domain.Gate
npm run typecheck,npx vitest run test/domain.test.ts40/40,npm run build+ all three--helppages.npx vitest runover the whole suite fails 6 tests intest/setup-agent.test.tswith "org not found". That is an ancestor.insta/project.jsonin the parent checkout, it reproduces onorigin/main, and it is not this diff.Not here
The MCP server gets nothing. All four of these writes answer 403 to an agent credential — the auth code is a POST precisely so an agent cannot read it — so the tools would fail every call. Its real gap is that bought domains have no read tool at all, which is its own scope.
🤖 Generated with Claude Code
Summary by cubic
Adds the CLI half of the two ways a bought domain can leave InstaCloud, both already supported by the platform: delegating its DNS zone and transferring it to another registrar. Delegating takes down the hostnames that domain serves, and
status/listnow show when a domain is delegated.New Features
insta domain nameservers set <domain> <ns...>delegates the zone;reset <domain>un-delegates without republishing records, and an attach re-adopts what is still there.insta domain transfer lock <domain> <on|off>toggles the registrar transfer lock, andtransfer code <domain>prints the EPP authorization code.Written for commit e3ebaad. Summary will update on new commits.