Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
HOLD — exact-head planner review at 0f493b9
I reviewed the planner increment above the previously reviewed #12421 head 95e7af3bcc09ecf81d288636c86e1ff3a0f62c91, including the file entry, observation/intent readers, preservation transforms, review preimage and comparison, and the controls. This is a technical HOLD recorded as COMMENT through the PR author's connected account, not an independent GitHub vote.
The review-only scope is appropriate. Keeping the full observation and unknown provider fields, combining service changes and exact TXT changes in one proposed zone, and retaining mail_mode=unobserved / write_authority=withheld are good. The docs correctly distinguish supplied evidence references from verified evidence, a digest from authorization, and a comparison from provider CAS. I am NOT requiring live DNS mutation, OAuth, TLS, a production fenced writer, or dashboard cutover in this PR.
Three localized issues remain.
P2 — establish provider owner qualification before selecting records
dag/gunbc/namecheap/planning/plan.dag::dns_owner_matches equates all of these after case-folding:
tracker-dev
tracker-dev.gunb.ai
tracker-dev.gunb.ai.
Case folding is justified; inferring that the middle spelling is absolute is a separate, unestablished provider rule. Namecheap's host-record documentation describes the Host as relative to the domain and specifically warns that including the domain can duplicate it. The getHosts contract here retains only the raw Name; it does not carry evidence that a bare dotted Name is an absolute owner.
Under the documented relative-host interpretation, Name=tracker-dev.gunb.ai belongs to tracker-dev.gunb.ai.gunb.ai., not tracker-dev.gunb.ai.. Nevertheless, with a single such A record, the current planner selects and changes its Address and does not add the requested short owner. The same comparison lets a retirement for _acme-challenge.tracker-dev delete an equal TXT value from the different relative owner _acme-challenge.tracker-dev.gunb.ai.
I reproduced those selection/transform expressions offline with synthetic retained rows. This is not a claim about a live capture or a DNS write; it identifies an ambiguity that the current model resolves toward mutation rather than refusal. The synthetic FQDN-conflict witness repeats that interpretation instead of independently establishing it.
Repair: introduce one provider-owner projection from the raw Name plus the known zone and an established qualification rule; then use that qualified owner for equality, conflict, upsert and retirement. Preserve the original raw field. Support absolute provider spellings only where their interpretation is established; otherwise refuse ambiguity. Do not decide qualification merely because the string ends in the zone spelling. Reuse the existing DNS qualification/label concepts; the LDH host parser cannot simply be imposed on _acme-challenge unchanged.
Controls: ordinary relative/mixed-case owner; a dotted relative owner ending in the zone spelling that must remain unrelated; any explicitly supported absolute representation; retirement must preserve the same token under a different qualified owner.
Provider references consulted:
- https://www.namecheap.com/support/knowledgebase/article.aspx/434/2237/how-do-i-set-up-host-records-for-a-domain/
- https://www.namecheap.com/support/knowledgebase/article.aspx/9776/2237/how-to-create-a-subdomain-for-my-domain/
- https://www.namecheap.com/support/api/methods/domains-dns/get-hosts/
P2 — DNS-01-only planning misses an ancestor zone delegation
dns_challenge_conflict examines only records at the exact _acme-challenge.<service> owner. dns_service_conflict runs only for entries in intent.services.
A valid challenge-only intent with services=[], against a snapshot containing:
tracker-dev NS ns1.example.net.
therefore returns DnsReviewReady and adds:
_acme-challenge.tracker-dev TXT <token>
to the parent-zone proposal while retaining the delegation. The authoritative challenge lies below the zone cut, so this proposal cannot publish it through the parent zone. The existing delegated-owner test covers only a CNAME at the exact challenge owner and misses this case.
Repair: derive delegation coverage from the same qualified-owner projection: a non-apex NS cut at or above the requested challenge owner must refuse this no-delegation-following planner. Name the cut in the refusal. This must work for challenge-only renewal/cleanup inputs, not only combined service installation. Do not globally refuse unrelated delegations or mistake the current zone's own apex NS records for a child delegation.
Controls: parent NS + challenge-only publish/retire; exact challenge NS/CNAME; unrelated sibling NS remains admissible; apex NS is not a child cut. This asks for correct planning admission, not implementation of delegation-following.
DNS authority: https://www.rfc-editor.org/rfc/rfc9499.html (delegation / zone cut / authoritative data).
P2 — the distinct-output guard does not establish distinct files
dag/gunbc/namecheap/planning/run.dag::main rejects only literal equality between output_path and the two input-path strings, then calls ordinary Filesystem.Write.
For example:
observation_path = observation.json
output_path = ./observation.json
passes the guard but identifies the same file. A successful plan then replaces its input receipt with the review envelope. Relative/absolute aliases, symlinks and hard links create the same problem. I reproduced the lexical guard plus actual path aliasing/write locally in a TemporaryDirectory; I did not execute the .dag entry.
Repair the publication boundary using existing filesystem operations: create-only publication at a fresh output is a simple safe option; an overwrite-capable variant must establish that it cannot replace either input object. Do not describe lexical normalization alone as protection against every filesystem alias or race. Filesystem.WriteCreateNew already exists, so this need not introduce another write primitive.
Controls: relative/absolute spelling aliases and symlink/hard-link aliases leave both inputs unchanged; a distinct fresh output succeeds; failure does not publish a replacement plan.
What I would preserve
- The original observation and intent are retained and included in the actual hashed plan content; proposed records and unmet apply requirements are also bound. This is useful review integrity, not evidence that the source file is authentic or that apply is authorized.
- Address updates preserve the rest of the selected row; other TXT values are retained; duplicates and conflicting requests are not silently coalesced.
- The 30-minute review window is explicitly not a future mutation freshness policy.
- The shared zone-writer name is not falsely presented as a held fence.
- Before apply, keep lossless setHosts serialization, mail-mode observation, service/ACME ownership, writer coordination, fresh readback, exact-attempt approval and independent post-apply observation as real unmet prerequisites. Namecheap documents that omitted host records are deleted by setHosts, so this distinction matters.
Tests, CI and dependency
I inspected the 23 .dag controls and the actual-file-entry/digest harness, but did not rerun them: no gunbc executable is available in this environment. My executed controls are narrower Python mirrors of the supplied-value predicates/transforms plus a real local filesystem alias check, using synthetic data only. They do not establish the full file-entry or provider behavior.
At the final CI read, run 36348062433 was still in progress; compiler and clippy had succeeded, but the floor job already recorded failures in Nominal witnesses (one prepared subject, one fold) and D0-ADJUDICATE. The log download was not available (BlobNotFound), so I have not attributed those failures to these findings or to a particular source change.
#12421 remains the explicit landing dependency and was still open at the check. Keep its normal protected landing requirement; do not merge this planner ahead of it. After the above repairs, require green exact-head and merge-queue checks.
No repository code, IAM, secrets, DNS, certificates, Tailnet configuration, deployment, or merge was changed by this review.
1 participant
Depends on #12421 (reviewed observer head
95e7af3bcc09ecf81d288636c86e1ff3a0f62c91, currently queued). The new planner is commit5dd7cf4a3e1; this PR targets main so the normal witness workflow runs. Do not land it ahead of its dependency.The authenticated Namecheap receipt currently has no consumer that can review stable dev-service DNS and certificate challenges without losing unrelated records. This adds a runnable, file-based review planner for
tracker-dev.gunb.aiandapprovals-dev.gunb.ai, independent of backend placement. It preserves the original snapshot and unknown provider fields, combines service A records and exact DNS-01 TXT additions/retirements under one zone writer identity, and emits the proposed full record set plus its SHA-256 review binding.The entry joins the receipt to independently supplied run/attempt/revision, the expected account/domain/credential container and a numeric credential version. It refuses stale/future observations, public address candidates, duplicate or contradictory intent, multiple service A records, and incompatible/delegated owners. The initial intent supports IPv4; an existing managed AAAA refuses rather than being removed. Existing website/mail records and provider metadata survive. DNS-01 cleanup removes only the named value and preserves concurrent tokens.
The result remains
mail_mode=unobserved,write_authority=withheld. It lists outstanding apply evidence: actual service assignment/policy and challenge ownership, observed mail mode, lossless provider serialization, one fenced zone writer, fresh readback and exact-attempt approval through the existing app. Evidence references are review inputs, not proof fetched by this offline planner. Its comparison helper is not authorization or provider CAS. No live DNS, secret, Tailscale, certificate, approval or deployment action occurs.Validation:
.dagplanner controls passed, including preservation, no-op, case-folded conflicts, freshness boundaries, wrong run, exact credential generation and unknown-field drift.Usage, schema, provenance boundary and remaining apply obligations:
docs/plans/namecheap-dns-review.md. No generated workflow changes are needed for this offline entry.CI follow-up: the initial floor run classified shorthand JSON
membersbindings as unimported references to an unrelated test provider. The bindings now explicitly usemembers: object_members. Current main (5bfe79be45c) was merged without conflicts, and all 23 planner controls passed on combined headbc50c23e245. Repository-wide CI is rerunning on that head.