Skip to content

feat(zone): insta domain zone — BYO domains on nameserver delegation - #287

Merged
tonychang04 merged 3 commits into
mainfrom
feat/byo-zones
Sep 24, 2026
Merged

tonychang04 merged 3 commits into
mainfrom
feat/byo-zones

Conversation

@tonychang04

@tonychang04 tonychang04 commented Sep 23, 2026

Copy link
Copy Markdown
Member

The CLI surface for BYO delegated zones (companion to InsForge/instacloud-platform#528 — draft until that merges and deploys; against older platforms the routes 404 and the CLI reports the platform's answer as-is).

insta domain zone delegate <domain> builds the managed zone and prints the two nameservers to set at the domain's own registrar, plus the review-then-switch contract (the provider scan is a heuristic: review zone records, add anything missing at the CURRENT provider — re-running delegate re-imports — and only then switch). zone list, zone records (unfiltered — CAA shows), and zone release complete the loop. Delegate/release are gated (zone.delegate) and sign for the linked project in agent mode, projectless under --org naming another org — the same precedent domain delegate documents. A domain carrying live MX records is refused by the platform and the sentence passes through verbatim.

Tests: 10 new in test/zone.test.ts (route shapes, agent-mode scoping, approval stop, --json verbatim, review-hint --org carry, empty-scan honesty). The github-source.integration.test.ts failure is a pre-existing local-env issue (fails identically on origin/main).

Required cli-reference update (cross-repo): skills/insta/cli-reference.md lives in the instacloud-skills repo — the mandated companion update is InsForge/instacloud-skills#124 (open, bot-approved), which adds the full insta domain zone verb row and the deploy.md flow. This checkout cannot contain that file by construction; both PRs land together.

🤖 Generated with Claude Code


Summary by cubic

Adds insta domain zone for bring-your-own domains: delegating a domain you own elsewhere builds a managed zone, and every attach's records (apexes included) publish into it instead of being printed for hand-copying.

zone delegate <domain> prints the two nameservers to set at the registrar plus a review-then-switch contract: compare zone records (unfiltered — CAA shows), add anything missing at the current provider, re-run delegate to re-import, then switch. zone list and zone release complete the loop. Delegate and release are gated on zone.delegate, stop at approval_required, and sign for the linked project in agent mode, projectless under --org naming another org — and their help texts now state that gating rule so agents pick the right flow from the help alone. The platform refuses domains with live MX records and the message passes through verbatim. All four commands support --org and --json, and every printed hint carries the domain and --org scope so it runs as-is. Adds 10 tests in test/zone.test.ts.

Dependencies

  • Routes against incompatible (older) platforms 404; the CLI reports the platform's answer as-is. Draft until instacloud-platform is deployed.

Written for commit bc9eb6d. Summary will update on new commits.

Review in cubic

…r delegation

The BYO twin of `insta domain delegate` (instacloud-platform #528): point the
domain's registrar at the pair `zone delegate` answers and every attach's
records — apexes included — are published into the managed zone for you.

- zone delegate <domain>: builds the zone, prints the pair AND the
  review-then-switch contract every time (the provider scan is a heuristic;
  compare `zone records`, add at your CURRENT provider, re-delegate to
  re-import, only then switch). Signs for the linked project in agent mode
  (zone.delegate), projectless under --org naming another org — the same
  precedent domain delegate documents.
- zone list: awaiting_ns vs delegated, with the pair.
- zone records <domain>: the UNFILTERED pre-switch review (CAA shows).
- zone release <domain>: prune + zone delete, prints the re-point step.

All four speak --org/--json; delegate and release stop at approval_required
like every gated verb. 10 tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jwfing
jwfing previously requested changes Sep 24, 2026

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

The new BYO delegated-zone surface is well structured, but its safety instructions and required documentation are incomplete.

Requirements context

I assessed the change against the PR description, the existing domain-command patterns, and the repository development rules. The linked platform PR was not available in this checkout or through public access, so platform behavior beyond the supplied description could not be independently verified. The repository explicitly requires command additions to be mirrored in the superproject CLI reference (AGENTS.md:15-17, .claude/skills/developing-insta-cli/SKILL.md:36-38).

Findings

Critical

  • The review/re-import safety contract prints commands that are not reliably executable. After delegate --org org9, the second review instruction omits --org org9 and can inspect the linked/default org instead. More importantly, zone records tells users to run insta domain zone delegate without its required <domain> argument, and also drops --org. Since the PR describes review-then-reimport-before-switching as the safety mechanism, these instructions must include both the domain and applicable organization. Tests currently assert only the incomplete substring rather than an exact copy-pasteable command. (src/commands/domain.ts:393-411, test/zone.test.ts:79-92, test/zone.test.ts:113-117)

  • The four new commands and their flags are registered without the mandatory corresponding update to skills/insta/cli-reference.md. That file lives in the superproject and is not present in this checkout, so the required companion update should be supplied or linked before merge. (src/index.ts:289-301, AGENTS.md:15-17, .claude/skills/developing-insta-cli/SKILL.md:36-38)

Suggestion

(none)

Information

  • Software engineering: aside from the blocking documentation and instruction tests above, the implementation follows the existing dependency-injection, API-error, approval, and project-scoping patterns. (src/commands/domain.ts:382-424, test/zone.test.ts:35-117)
  • Security: no security-relevant regression found. Path components are encoded, mutation authorization remains platform-enforced, and project scope is withheld when --org targets another organization. (src/commands/domain.ts:350-351, src/commands/domain.ts:382-390, src/commands/domain.ts:419-423)
  • Performance: no material concern found; formatting and request work are linear in the returned zone/record count, with no new blocking I/O or N+1 requests. (src/commands/domain.ts:363-368, src/commands/domain.ts:398-411)
  • Verification: git diff --check passed. Typecheck and tests could not execute because this read-only checkout has no installed typescript, vitest, or tsx dependencies; no dependency installation was attempted.

Verdict

Request changes: correct and test the complete org-scoped safety commands, and provide the required CLI-reference update.

…ain and --org included

Review round 1: the review-then-reimport safety hints dropped the org scope
(and the records reminder even dropped the domain), so the printed command
could inspect the wrong org or not run at all. Both hints now carry the
domain and the caller's --org, pinned with verbatim-string tests on both the
default-org and --org paths.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tonychang04

Copy link
Copy Markdown
Member Author

On the second Critical (the mandatory cli-reference.md update): that file lives in the instacloud-skills repo, not this one — the required companion update is InsForge/instacloud-skills#124, already open and in review, which adds the full insta domain zone row to insta/cli-reference.md plus the deploy.md flow. This checkout cannot contain it by construction (the same cross-repo split as prior CLI PRs); both PRs merge together.

@tonychang04
tonychang04 dismissed jwfing’s stale review September 24, 2026 00:30

Superseded: the printed instructions are now verbatim-runnable (domain + --org, pinned); the cli-reference update is the cross-repo companion instacloud-skills#124, linked in the PR comment

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/index.ts Outdated
…le they implement

cubic P3: both verbs implement domain delegate's projectless-under---org
rule but their help ended at the gate name — mirrored the documented wording
so agents choose the right flow from the help alone.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jwfing
jwfing previously requested changes Sep 24, 2026

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

The new BYO delegated-zone commands are coherent and well tested statically, but the required agent-facing command reference update is missing.

Requirements context

I assessed the change against the PR description, the local command architecture, AGENTS.md, and .claude/skills/developing-insta-cli/SKILL.md. The linked platform PR was not accessible from this checkout, so endpoint semantics beyond the supplied description could not be independently verified.

Findings

Critical

  • The new command surface is not mirrored in the required CLI reference. This PR registers domain zone delegate, list, records, and release, each with --org and --json, but changes only CLI implementation and tests (src/index.ts:289-301). Repository instructions explicitly require every command/flag change to be reflected in the superproject’s skills/insta/cli-reference.md (AGENTS.md:15-17; .claude/skills/developing-insta-cli/SKILL.md:36-38). Please add the coordinated reference update—or provide the companion superproject change—so agents can discover and correctly use this feature.

Suggestion

(none)

Information

  • Software engineering/functionality: The tests cover route shapes, same-org versus cross-org signing, approval stops, record rendering (including CAA), empty scans, and copy-paste guidance (test/zone.test.ts:35-126). Static inspection found no additional functional defect. I could not execute the required typecheck/test gate because node_modules is absent and installing dependencies would violate the read-only constraint; npm run typecheck stopped with tsc: not found (package.json:35-39).
  • Security: No security-relevant defect found. Organization/domain path components are encoded, and project evidence is attached only when the linked project belongs to the target organization (src/commands/domain.ts:350-351, src/commands/domain.ts:384-392, src/commands/domain.ts:421-425). No secrets or new dependencies are introduced.
  • Performance: No performance concern found. Each command performs one API request, while record formatting is linear in the returned record count (src/commands/domain.ts:363-368, src/commands/domain.ts:400-425).

Verdict

Request changes because the missing CLI-reference update violates an explicit non-negotiable repository requirement.

@tonychang04
tonychang04 dismissed jwfing’s stale review September 24, 2026 00:34

Cross-repo requirement: skills/insta/cli-reference.md lives in instacloud-skills — the mandated update is the companion InsForge/instacloud-skills#124 (open, bot-approved), linked in the body and a comment; this repo cannot contain that file

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - approved.

@tonychang04
tonychang04 merged commit baaa5cf into main Sep 24, 2026
3 checks passed

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

Four clean, well-tested insta domain zone verbs that match the platform's BYO-zone contract exactly — but the review-then-switch safety contract they print names a remedy (re-running delegate re-imports) that cannot fix the record class the review exists to catch, and the platform's designated fix door is not exposed.

Reviewed at head bc9eb6df — note fetch_pr served the stale f519eb46; git ls-remote is what caught the two newer commits (17bf453d, bc9eb6df), and those resolved one of my items outright (see Resolved at head).

Requirements context

This repo has no /docs/superpowers/ (no docs/ at all — verified by ls), so there is no CLI-side spec or plan. Graded against:

  • the PR body + AGENTS.md non-negotiables;
  • the companion instacloud-platform#528 (open, head 1e3f0e0), which I cloned and read as the authoritative route/schema contract;
  • the companion skills PR instacloud-skills#124 (feat/byo-zones), which satisfies AGENTS.md non-negotiable #4 (command/flag changes mirrored in skills/insta/cli-reference.md). ✅

Gates run in a clean npm ci workspace at bc9eb6df: tsc --noEmit clean; full suite 86/86 files, 1803 passed, 1 skipped. The github-source.integration.test.ts failure the PR body predicts did not reproduce here.


Findings

Critical

C1 — functionality: the review-then-switch contract's remedy cannot converge, and the platform's actual fix door is not exposed. src/commands/domain.ts:377,397,413, src/index.ts:290,296

All four claim sites tell the customer the same thing: "add anything missing at your CURRENT provider (re-running delegate re-imports)". Both halves fail for the dominant miss case:

  1. Re-running delegate re-runs the same scan. The same-org re-delegate branch (instacloud-platform src/domains/org-zones.ts:552-568) does ensureZone → maybe replaceZonescanRecords, and nothing else. scanRecords is POST /zones/:id/dns_records/scan (src/adapters/cloudflare-zones.ts:216-226) — Cloudflare's quick scan.
  2. Cloudflare documents that scan as pattern-based and expected to miss exactly this class. Per developers.cloudflare.com DNS records quick scan → Limitations (checked via context7): the scan is "built upon a list of recurring patterns of DNS records Type and Name", so for "very specific hostnames — such as my-store1900.example.com" or "a DKIM record that uses a more custom name — this._domainkey instead of default._domainkeyit is expected that the scan will not find the specific DNS records." Cloudflare's own instruction is to manually add them. Re-triggering the scan cannot import what it is structurally unable to discover.
  3. "Add it at your CURRENT provider" is a no-op for this case. The record already exists at the current provider — that is why it shows as missing in zone records: the scan didn't find it, not that it isn't there. So the advised loop is: confirm a record that already exists → re-run delegate → re-list → still missing → no further path offered.
  4. The platform ships the fix and this PR doesn't expose it. POST /orgs/:orgId/zones/:domainName/records (server.ts:3550, operationId: createOrgZoneRecord, gated zone.delegate, already in SESSION_SCOPED_OPERATIONS) is described as "the pre-cutover REPAIR door: create a record the provider's scan missed, directly in the delegated zone, BEFORE switching the nameservers". Its adapter contract is blunt about why it exists (cloudflare-zones.ts:50-53): "the scan is a heuristic, and a record it cannot discover must still have a path into the zone the customer does not otherwise control." ZONE_REPAIR_RECORD_TYPES even extends the registrar enum with CAA for precisely this. The CLI exposes the review (zone records) but not the repair.

Why this blocks rather than being a polish item: the printed contract is, by this PR's own test comment, "the safety of the whole flow", and 17bf453d made the dead-end instruction fully copy-pasteable (insta domain zone delegate byo.example --org org9), which invites running the loop rather than questioning it. Combined with the empty-list copy at domain.ts:364"the provider scan runs shortly after delegating; run this again in a moment" — a user whose DKIM or custom-host record never appears is being told, in the tool's own voice, that waiting and re-running is the fix. Switching nameservers at that point drops the record silently at cutover. The platform's MX refusal caps the mail-MX blast radius, but DKIM/SPF TXT and custom-host CNAMEs are not MX and are not covered by it.

Either resolution closes this, and the second is small:

  • expose the repair door as insta domain zone records add <domain> <type> <host> <answer> (the platform route, body and gating already exist and mirror domain records add); or
  • correct the copy: say plainly that a record the scan cannot discover will not be imported by re-running delegate, and name the path that does work.

Suggestion

S1 — software engineering: zoneRelease's cross-org guard is entirely unpinned. src/commands/domain.ts:424, test/zone.test.ts:95-108

zoneDelegate and zoneRelease carry the same new predicate, link && link.orgId === orgId ? link.projectId : undefined. Only zoneDelegate's copy is tested ("under --org naming ANOTHER org the call goes projectless").

N-1 verified at bc9eb6df — widening only line 424 to link ? link.projectId : undefined:

sabotaged copy result
zoneDelegate (:391) 1 test red ✅ pinned
zoneRelease (:424) 11/11 green ❌ unpinned

Not Critical: the platform catches the wrong-org scope anyway (govern/agent-routes.ts:388-392authorizeProject then if (p.orgId && bound.org_id !== p.orgId) throw new ForbiddenError(...)), so this is defense-in-depth, not a live hole. But it is a security-relevant predicate shipped in two copies with one test, and the fix is four lines mirroring the delegate case.

S2 — project conventions: the --org census was not extended with the four new verbs. test/domain-org-flag.test.ts:1-4,13

That file is the deliberate census of which domain verbs take --org, and its header comment enumerates the reasoning per verb — it was extended with delegate when that verb landed (#270). All four zone verbs take --org and none were added; the file is untouched by this PR (git diff = 0 lines). Its header comment is now an incomplete enumeration of the --org surface.

Information

I1 — zoneDelegate sends the domain un-normalized in the POST body. src/commands/domain.ts:393, test/zone.test.ts:41
zonePath() does .trim().toLowerCase() for the other three verbs, and domainAttach/domainCheck normalize with an explicit comment about not "asking the plane about a binding it never wrote" — but delegate posts { domainName } verbatim, and the new test enshrines body: { domainName: 'Byo.Example' }. Harmless today (the platform's normalizeZoneName, org-zones.ts:139, trims/lowercases/strips scheme), just inconsistent with the file's own stated convention.

I2 — no guard on an empty nameserver pair. src/commands/domain.ts:353-361
zoneLines renders z.nameservers.join(', ') unguarded, so an empty array prints the flagship instruction as set these at your domain's registrar: with nothing after it. The platform can represent it (cloudflare-zones.ts:108, nameservers: z.name_servers ?? []), and assertVanity only guards when a vanity pair is configured. I could not demonstrate Cloudflare actually returning a zone without name_servers, so this is reachability-unproven, not a verified defect.


Dimensions with nothing to report

  • Security — no weakening found. No new dependencies. Both path segments go through encodeURIComponent (domain.ts:349-350). No secrets, tokens or PII newly logged or returned; --json passes the platform body through and that body carries only domain names and nameservers. Auth scoping is correct and matches the domain delegate precedent — and I verified the part that silently breaks it is in place: createOrgZone and releaseOrgZone are in SESSION_SCOPED_OPERATIONS (agent-routes.ts:211), so the { projectId } scope actually resolves rather than falling into unclassified_agent_action. The only scoping gap is the untested copy in S1.
  • Performance — nothing. One request per verb, no N+1, no unbounded loops, no blocking work. zoneRecordLines makes three Math.max passes over one page — trivially bounded, and the empty case returns before Math.max(...[]). The records route is rate-limited at 30/min (server.ts:3301), comfortable against the CLI's "run this again in a moment" advice.

Verified, not filed

Things a reader might reasonably suspect, checked against instacloud-platform#528 @ 1e3f0e0 and refuted:

  • The unused delegated field is not a dropped signal. The platform derives it as delegated: z.status === 'active' (org-zones.ts:393), so it is redundant with status and zoneLines loses nothing by keying on status alone.
  • status really is a two-value union. OrgZoneView.status is Exclude<OrgZoneStatus, 'released' | 'releasing'> (org-zones.ts:47-53), so the CLI's 'awaiting_ns' | 'active' is exact and zoneLines has no unhandled state.
  • "unfiltered — CAA shows" is true. listRecords is deliberately unfiltered (org-zones.ts:931-932: "the registrar-enum filter is for publishing, not eyes").
  • --api-url reaches all four new level-3 leaves — probed each directly, 0 unknown option hits (addApiUrlEverywhere(program) at index.ts:645 recurses after registration).
  • Route shapes, methods, { items } envelopes, the { domainName, released } release body and the 202 approval_required path all match the platform routes at server.ts:3513-3607, and --json envelope handling matches the sibling domainRecordsList / domainBuy conventions.

Resolved at head

bc9eb6df + 17bf453d closed, before I posted, an item I had written up against f519eb46: the delegate/release help text previously promised gated: zone.delegate flatly, while an unlinked --org call goes projectless and is governed by the org-administration branch (agent-routes.ts:404-412 — org admin + every project full_access, then state.governed = true) with no zone.delegate approval. The help now discloses exactly that ("agent mode gates from a linked project, an unlinked --org call falls under org administration instead"). Accurate — dropped. The --org carry in every printed instruction was fixed in the same push, with tests.

Verdict

request_changes — one Critical (C1). Everything else is non-blocking, and the implementation quality, platform-contract fidelity and test shapes are otherwise strong.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

A clean, contract-accurate CLI surface for BYO delegated zones: all four verbs match platform #528's routes and response schemas exactly, the gating/scoping follows the domain delegate precedent correctly, and the 10 new tests are genuinely load-bearing (6/6 N-1 arms red). 0 Critical.

Note: this PR merged at 00:36 UTC while I was measuring (17bf453bc9eb6d → merged as baaa5cf). I graded 17bf453 and then re-graded the merged head: the delta is two help strings only, no executable change, so everything below holds at bc9eb6d. The findings are follow-ups, not merge blockers.

Requirements context

  • No /docs/superpowers/ in this repo — the CLI has no docs/ at all. I also checked the platform's docs/superpowers/specs/ and plans/ on feat/byo-delegation: no BYO-zones or domains design doc exists there either (consistent with what I found reviewing platform #528 — the domains design of record is docs/domain-purchase.md).
  • Intent was therefore assessed against three real sources: the PR body, the companion platform PR InsForge/instacloud-platform#528 (src/server.ts:3508-3608, src/domains/org-zones.ts, src/openapi/schemas/common.ts:1348-1371, src/govern/agent-routes.ts:135-137,211), and the companion skills PR InsForge/instacloud-skills#124, which satisfies AGENTS.md non-negotiable #4.

Findings

Critical

(none)

Suggestion

1. Functionality — the review-then-switch paragraph prints even when the zone is already active · src/commands/domain.ts:366-382

Platform create() has an explicit re-import branch (org-zones.ts:551-568): for a zone this org already owns with status awaiting_ns or active, it re-runs ensureZone + scanRecords and returns the view. So re-running zone delegate after the cutover — which the CLI's own output, the help text, and skills#124 all teach as the re-import step — returns status: 'active'. Reproduced with a scratch probe:

byo.example  delegated (a.ns, b.ns)
the zone was seeded by a provider scan — a heuristic. Check `insta domain zone records byo.example`
against your current DNS, add anything missing at your CURRENT provider (re-running delegate
re-imports), and only then switch the nameservers.

Post-cutover the "CURRENT provider" is InstaCloud, the switch already happened, and the platform's repair door is closed (org-zones.ts:975, 409 at activation) — so every clause of that sentence is wrong in the state it prints in. zoneLines already branches on z.status; the trailing info(...) should too. This is the same shape as the incoherent-output finding on #270 (domainLines printing "re-attach" and "watch, don't re-attach" together).

2. Functionality — zone records' trailing hint has the same unconditional problem · src/commands/domain.ts:395-400

…add anything missing at your current DNS provider and re-run \zone delegate` … before switching nameserversprints on every non-empty listing. Unlike #1 the records route returns no status, so conditioning needs either azone list` lookup or a status field — the cheaper fix may be to soften the wording to name the pre-switch state explicitly ("if you have not switched the nameservers yet, …").

3. Functionality/scope — the platform's repair door has no CLI verb · src/index.ts:289-301

Platform #528 ships a fifth zone route the CLI does not surface: POST /orgs/:orgId/zones/:domainName/records (createOrgZoneRecord, server.ts:3550), described there as "the pre-cutover REPAIR door: create a record the provider's scan missed, directly in the delegated zone, BEFORE switching the nameservers". It is gated zone.delegate and is in SESSION_SCOPED_OPERATIONS, i.e. it was built for exactly this CLI flow.

The CLI's only offered remedy is "add it at your CURRENT provider, then re-run delegate to re-import". That works, but it requires write access at the provider being abandoned, and it has no answer for a record the old provider will not hold (the platform's own summary notes it accepts types beyond the registrar model, "where the review needs them (CAA)"). zone records add <domain> <type> <host> <answer> would close the loop the group description claims to close. skills#124 omits it too, so the gap is consistent across both companions rather than a CLI-only miss.

4. Software engineering — test/domain-org-flag.test.ts was not extended with the four new zone verbs · test/domain-org-flag.test.ts:12-16

That file is the repo's --org census over the org-scoped domain verbs; all four new verbs take --org and none were added. The same gap was filed on #270 for delegate.

If you extend it, use toContain('--org <id>'), not toContain('--org'). I measured this at the merged head: insta domain zone delegate --help contains the bare literal --org twice — once as the option row and once inside the description prose that bc9eb6d added ("an unlinked --org call falls under org administration instead") — but --org <id> exactly once. So the weak form would be satisfied by the description alone and would stay green if .option('--org <id>') were deleted. Same for release. (The census's existing rows still use the weak form; that is pre-existing, not this PR's doing.)

5. Software engineering — --json returns before the review contract, but the test comment says it prints every time · src/commands/domain.ts:374, test/zone.test.ts:41

if (opts.json) return printJson(res.body) runs before the contract paragraph, so the agent-facing path never sees the sentence the test annotates as "the safety of the whole flow — it prints every time". The early return matches domainBuy's convention so the code may well be right; the comment is what overclaims. If you want the contract on the agent path too, info() already writes to stderr, so it would not corrupt --json stdout.

6. Functionality — the link.orgId === orgId scope guard treats an unknown org as a mismatch · src/commands/domain.ts:390-391 and :423-424

Both new verbs copy the guard from domainDelegate (domain.ts:178-179). resolveProjectLink builds orgId: process.env.INSTA_ORG_ID ?? '' for the linkless-agent path (src/config.ts:211), so INSTA_PROJECT_ID set + INSTA_ORG_ID unset + --org X gives '' !== X → projectless — and the user gets there by obeying the CLI's own die text, billing.ts:12 ("set INSTA_ORG_ID, or pass --org ").

Not an authorization weakening: platform agent-routes.ts:408 routes it to the org-administration branch, which demands org admin plus every project on full_access, and full_access short-circuits the approval set anyway. The cost is diagnostics — the 202 zone.delegate approval relay turns into a hard 403 naming project.billing.update, which names the wrong action. One clause fixes all three sites: (!link.orgId || link.orgId === orgId). Carried over from #270 r2; filing again only because this PR doubles the number of call sites.

Information

  • Verified against platform #528, so these need no defending — route paths, methods and bodies match server.ts:3513-3608; the CLI's OrgZone/ZoneRecord types match S.OrgZone/S.OrgZoneRecord (common.ts:1348-1371) field for field; createOrgZone and releaseOrgZone are in SESSION_SCOPED_OPERATIONS (agent-routes.ts:211), so the { projectId } scope is both correct and load-bearing; zone.delegate is a real gated action (agent-policy.ts:55,141); list() filters releasing/released (org-zones.ts:763), so the two-state status union is exact and zoneLines' else-branch can't be reached by a tearing-down zone; "re-running delegate re-imports" is true (org-zones.ts:551-568); sending the raw-case domain in the POST body is fine (normalizeZoneName); rawRequest throws on ≥400, so the 409 MX-refusal sentence really does pass through guard() verbatim.
  • Security: nothing to flag. No new dependencies. No secrets, tokens or PII enter the output or --json (the zone view is domain/status/nameservers/delegated). No shell or SQL. Both path segments go through encodeURIComponent in zonePathstricter than the neighbouring domainSearch, which still interpolates orgId raw. Auth is not weakened: the two mutations keep the handleApproval 202 stop (N-1 confirmed), and the two reads are ordinary member-level GETs matching the platform's own authorization.
  • Performance: nothing to flag. One request per verb, no N+1, no loops over network calls. Math.max(...records.map(...)) in zoneRecordLines is a spread over an array the platform adapter bounds at 100 pages × 100 records (cloudflare-zones.ts:177), so ≤10k elements — nowhere near a spread limit.
  • Gates at 17bf453 (Node 20): tsc --noEmit clean; full suite 86 files, 1804 passed, 1 skipped, 0 failures (134s). Re-ran zone + org-flag + help-surface at the merged head bc9eb6d: 69 passed. The PR body's claim that github-source.integration.test.ts fails did not reproduce — it passes here, as it did on #270 and #271. It's an old local git without --initial-branch, not a repo red.
  • N-1 on the new tests: 6 arms, 6/6 red — scope guard widened to always use the link's projectId; zoneLines' --org carry; orgArgOf; the delegate contract paragraph; zoneRelease's handleApproval; a CAA filter in zoneRecordLines. Nothing vacuous. (The CAA arm was green on my first attempt because my patch landed in the old recordLines for bought domains — worth knowing that the two record renderers are easy to confuse by name.)
  • delegated is declared in the CLI's OrgZone type and never read. Harmless — server-side it is exactly status === 'active' (org-zones.ts:393), so keying the render on status loses nothing — but the unused field invites a future reader to treat it as independent signal.
  • zoneRecordLines pads the answer column even when it is the last field on the line, so rows without ttl/priority/proxied carry trailing whitespace.
  • skills#124 documents the surface as "CLI ≥ 0.1.4" while package.json is at 0.1.3, so the reference is only true after the next release bump.

Verdict

approved — 0 Critical. All six items above are non-blocking follow-ups; #1 (wrong advice printed after the cutover) is the one I'd pick up first. The GitHub approval itself is a human action, and the PR has in any case already merged.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Addendum to my review above — I under-graded one finding, and I missed one

After posting I read the sibling review #5298393077 (CHANGES_REQUESTED at this same head, bc9eb6df). I verified both of its substantive findings independently rather than taking them at face value, and both hold. My COMMENT verdict was wrong on severity; this addendum supersedes it. The PR is already merged (baaa5cf), so this changes nothing procedurally — but the record should be accurate, and item 1 is worth a follow-up PR.

1. Adopting the sibling's C1 as Critical — I filed this as Suggestion #3 and mis-framed it

I filed the missing repair door as a scope gap ("a fifth route with no verb"). The sibling filed it as a correctness defect in the printed remedy, and that framing is the right one. The chain, which I re-derived from the sources rather than from their write-up:

  • scanRecords is Cloudflare's quick scan, POST /zones/:id/dns_records/scan (instacloud-platform src/adapters/cloudflare-zones.ts:216-226), and the same-org re-delegate branch (src/domains/org-zones.ts:551-568) does ensureZone → maybe replaceZonescanRecords and nothing else. Re-running delegate re-runs the same scan.
  • I checked Cloudflare's own documentation for that endpoint via context7 (/cloudflare/cloudflare-docs, dns/zone-setups/reference/dns-quick-scan). It confirms the scan is pattern-based — "built upon a list of recurring patterns of DNS records Type and Name" — and states the miss case explicitly: for "very specific hostnames — such as my-store1900.example.com" or "a DKIM record that uses a more custom name — this._domainkey instead of default._domainkeyit is expected that the scan will not find the specific DNS records." Cloudflare's instruction is to manually add them before changing nameservers.
  • So for that record class the CLI's advice is a closed loop. The record is already at the current provider — that is why the scan missing it is the whole problem — so "add anything missing at your CURRENT provider" is a no-op, and "re-running delegate re-imports" re-runs a scan structurally unable to find it. zone records keeps showing it absent, and the CLI offers no further step.
  • The platform's own adapter docstring states the premise the CLI contradicts (cloudflare-zones.ts:50-53): "the scan is a heuristic, and a record it cannot discover must still have a path into the zone the customer does not otherwise control." That path is POST /orgs/:orgId/zones/:domainName/records — gated zone.delegate, already in SESSION_SCOPED_OPERATIONS, with ZONE_REPAIR_RECORD_TYPES extending the registrar enum with CAA (org-zones.ts:84) — and it has no CLI verb.

End state: the user follows the printed contract to exhaustion, switches nameservers, and a custom-selector DKIM TXT or custom-host CNAME is silently gone at cutover. The platform's MX refusal bounds the mail-MX case; it does not cover DKIM/SPF TXT or custom-host CNAMEs. Silent loss of a record the safety contract exists to protect clears my Critical bar, and 17bf453's copy-pasteable hint makes the dead-end loop more inviting to run.

Either fix closes it: expose insta domain zone records add <domain> <type> <host> <answer>, or correct the copy to say that a record the scan cannot discover will not arrive by re-running delegate, and name the path that works. (This affects skills#124 too, which teaches the same loop.)

2. The sibling's S1 is real and my own N-1 masked it

zoneRelease's copy of the cross-org scope guard (src/commands/domain.ts:424) is unpinned. I reported "6/6 arms red" above; that row is wrong. My arm used a global sed that widened both copies of link && link.orgId === orgId ? … : undefined at once, saw one red, and recorded the predicate as pinned. Re-run isolating each copy at bc9eb6df:

copy widened alone to link ? link.projectId : undefined result
zoneDelegate (:391) 1 failed / 10 passed — pinned
zoneRelease (:424) 11/11 green — unpinned

Not a live hole — platform agent-routes.ts:388-392 refuses a cross-org session anyway — but it is a security-relevant predicate shipped in two copies with one test, and the missing case is a four-line mirror of the delegate test. My error here is the one I should least have made: N-1 every verbatim copy of a new predicate separately; a batch patch across copies reports the union, which is green-washing by construction.

Unchanged

The rest of my review stands, including the platform-contract verification, the security and performance dimensions (nothing to flag in either), the full-suite result, and Suggestions 1, 2, 4, 5 and 6. Note that my Suggestion #1 (the contract paragraph printing over an already-active zone, reproduced) and this Critical are the same copy block seen from two angles — a fix should address both states at once.

I have not independently re-derived the sibling's I1/I2, and its I2 is self-labelled reachability-unproven.

Verdict

request_changes1 Critical (the review-then-switch remedy cannot converge for the record class the review exists to catch, and the platform's repair door is unexposed). Correcting my earlier approved. The implementation quality and platform-contract fidelity remain strong; this is one copy-and-one-verb fix, best done as a follow-up since the PR has merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants