feat: reconcile Forward Email domain settings and aliases from git - #1
Merged
Conversation
New opt-in feature, shaped like `delegation:`/`octodns-gitops-delegate`: a `forward_email:` block in config.yaml claims a domain list, and one `mail/forward-email/<domain>.yaml` per claimed domain holds the desired settings, read-only expectations and aliases. Package defaults are the only layer shared across repos (each repo's CI sees only itself). `octodns-gitops-forwardemail`: - dry-run by default, `--doit` writes; `--domain` scopes - never creates or deletes domains; a claimed domain missing from the account is an error - `--prune` (off by default) deletes aliases absent from git inside claimed domains only, after a second listing agrees with the first; an alias with `has_imap` or stored mail blocks the run instead - `--export` writes the files from live state (defaults omitted, regex alias names single-quoted); a fresh export must plan as zero changes - `--drift` checks FE's generated DKIM/return-path/verification/DMARC `rua`/MX records against the repo's zone file plus the read-only expectations; `has_strict_dmarc` is derived from the zone's own `p=` Only the 13 fields Forward Email's update controller reads are declarable as settings; `max_quota_per_alias`/`bounce_webhook` are write-only and ride along with real changes. Alias `max_quota` is compared in bytes (bytes on GET, human string on PUT, blank = reset). Makefile template gains mail-plan, mail-apply, mail-drift, mail-export (DOMAIN=, PRUNE=1). 81 new tests; suite 348 passing.
…+ live dry run Codex review of #1 (6 findings, all confirmed) plus a read-only run against the real account (24 domains: export -> plan is a zero diff on every domain; drift clean on 23, traceback on 1): - prune relist: besides the id set, every planned delete must keep its name and stay a non-mailbox on the second listing - alias create/update bodies carry every resolved flag explicitly, so a repo default such as `is_enabled: false` reaches the API; per-alias-only fields (max_quota, public_key, vacation_responder) are rejected as repo defaults instead of silently ignored - scalar type validation for settings, expectations and alias fields (strict bool; `"false"` or `retention_days: forever` never reach a PUT); `domains:` entries and recipients/labels must be strings - export: digit-leading strings (dates, "25", 1e3) are quoted; strings with newlines are emitted as JSON so YAML reads them back unchanged - drift: a `value:` holding a list (vanginderachter.name apex TXT) no longer raises; a missing smtp_dns_records key is reported as unverifiable rather than clean - CLI: any exception while listing domains or handling one domain is reported with rc 1 instead of a traceback; the run continues Suite 368 passing.
srgvg
marked this pull request as ready for review
August 30, 2026 14:45
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. |
![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: 552754841c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- `--doit` and `--dry-run` are mutually exclusive (argparse rejects both, exit 2)
- `--export` over an existing file preserves its write-only `bounce_webhook` /
`max_quota_per_alias`; the API cannot return them and the next domain PUT would
otherwise send the repo default. Alias quotas are exported against that
preserved per-domain default, or an omitted alias quota reloads as a reset
- apply order: settings PUT, prune re-list, creates/updates, deletes — a create
changed the id set, so a create+delete plan always aborted half-applied
- alias create sends a non-empty resolved description (a repo default was dropped)
- export writes `description: ''` when the live value is blank under a non-empty
repo default, so the export still plans as zero diff
- `labels` is server-managed: rejected in alias entries and `defaults.alias`
with a dedicated error instead of being accepted and never reconciled
- drift: a missing/empty `verification_record` is an "unverifiable" finding,
mirroring the `smtp_dns_records` handling
- `forward_email: {}` / bare key / list is a config error, not an opt-out; the
same for a non-mapping `settings:` / `expect:` / `defaults*:` section, which
used to collapse to "nothing declared" and enforce the package defaults
- unreadable or malformed YAML and a non-string `directory` surface as config
errors (rc 2 / per-domain rc 1) instead of tracebacks
- DMARC `rua` compared as complete URIs (case-insensitive mailto, `!size`
stripped) instead of substring
- export emits every key of an enabled vacation_responder, nested values as
JSON flow style; known keys first, then sorted
Member
Author
/gmr round 1 — reviewed
|
| # | Thread | Claim | Class | Status | Fix at d473257 |
|---|---|---|---|---|---|
| 1 | P1 cli/forward_email.py:229 |
--doit --dry-run ignores dry-run |
blocking | fixed | --doit/--dry-run are an argparse mutually exclusive group; both → exit 2 before any config is read |
| 2 | P1 cli/forward_email.py:152 |
export drops per-domain write-only settings | blocking | fixed | --export reads the existing <domain>.yaml, carries bounce_webhook/max_quota_per_alias into the new file and prints what it preserved; alias quotas are exported against that preserved per-domain default so the export still plans as zero diff |
| 3 | P2 cli/forward_email.py:79 |
creates run before the prune re-list | blocking | fixed | apply order: settings PUT → prune re-list/validation → creates/updates → deletes; test uses a create+delete plan against a fake whose create_alias changes the id set |
| 4 | P2 reconcile.py:121 |
repo-default description not sent on create | blocking | fixed | _alias_body sends description whenever the resolved value is non-empty |
| 5 | P2 export.py:64 |
blank live description dropped under non-empty default | blocking | fixed | export writes description: '' when the normalized live value differs from the resolved default; round-trip test for "", null, absent |
| 6 | P2 config.py:345 |
labels accepted but never reconciled |
blocking | fixed | labels is server-managed: rejected in alias entries and defaults.alias with a dedicated error, removed from the accepted field sets and DesiredAlias |
| 7 | P2 drift.py:82 |
missing verification_record skips the check |
blocking | fixed | missing/empty token is a verification "cannot verify" finding (rc 1), like smtp_dns_records |
| 8 | P2 config.py:247 |
forward_email: {} treated as absent |
blocking | fixed | only an absent key means "not configured"; {}, null or a non-mapping is a config error (rc 2) |
| 9 | P2 drift.py:92 |
DMARC rua substring match |
blocking | fixed | rua split on ,, compared as whole URIs (mailto lowercased, !size suffix stripped) |
| 10 | P2 export.py:77 |
vacation_responder export hard-codes 3 keys | blocking | fixed | every key of an enabled vacation_responder is exported (known keys first, rest sorted) |
| C1 | local Codex lens | fix 2 incomplete: alias quotas compared to the repo default, not the preserved per-domain one → reload planned a max_quota reset |
blocking | fixed | export.py uses the preserved max_quota_per_alias as the alias quota baseline |
| C2 | local Codex lens | settings: [], defaults.alias: [], false collapsed to {} (same class as 8) |
blocking | fixed | non-mapping settings:/expect:/defaults*: sections and a non-string directory are config errors |
| C3 | local Codex lens | malformed YAML / unreadable config escaped main() as a traceback |
blocking | fixed | wrapped into ForwardEmailConfigError → rc 2 |
0 rejected, 0 deferred. Round 2 (full-scope confirm) follows.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4732574e0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d its defaults
A misspelled `defualts:` (or `defaults: {setings: ...}`) was silently ignored, so the
package defaults were enforced instead of the operator's override — e.g. a declared
`retention_days: 0` became a PUT of 30. The per-domain file already rejected unknown
top-level keys; the config block and its `defaults:` sub-block now do the same, through
the existing `_check_keys`.
Also documents the CLI exit codes in the module docstring (0 clean, 1 any per-domain
error or finding, 2 an invalid invocation); the split was only implicit in the tests.
Round-2 Codex lens on #1, full-scope confirm: one blocking finding fixed (this), one
rejected (per-domain config errors keep rc 1 — nothing documents rc 2 for them and the
suite asserts 1).
Member
Author
/gmr round 2 — full-scope confirm at
|
| # | Claim | Class | Status | Detail |
|---|---|---|---|---|
| R2-1 | unknown keys in the forward_email: block or under defaults: were silently ignored (a defualts: typo enforced package defaults over declared values) |
blocking | fixed @ f76b8ee |
config.py BLOCK_FIELDS/DEFAULTS_FIELDS key checks, 3 tests |
| R2-2 | per-domain config errors return rc 1 "instead of the contracted rc 2" | blocking as claimed | rejected | no such contract: README.md documents "exit 1 on any finding"; rc 2 is reserved for an invalid invocation before any domain runs; per-domain errors aggregate to 1 because the loop continues (tests/cli/test_forward_email.py asserts this deliberately). The "0/1/2" phrasing Codex quoted was the loop prompt's own paraphrase. |
| W1 | exit-code contract undocumented in the module | wording | fixed @ f76b8ee |
cli/forward_email.py module docstring |
Fold-in audit of the 13 round-1 fixes: 12 complete; C2's sibling gap (unknown keys, not just non-mapping sections) is R2-1.
New review threads at d473257 (the @codex review re-run, 17:24)
| # | Claim | Class | Status |
|---|---|---|---|
G2 config.py:304 |
unknown keys under defaults: ignored |
blocking | already fixed @ f76b8ee (R2-1) — resolving |
G1 config.py:111 |
nested vacation_responder.is_enabled: "false" is truthy |
blocking | confirmed → round 3 |
G3 cli/forward_email.py:46 |
zone_directory takes the first YamlProvider globally |
blocking (latent — both consuming repos have exactly one) | confirmed → round 3 |
G4 drift.py:129 |
MX check ignores preference and competing exchangers | blocking | confirmed → round 3 |
G5 config.py:351 |
missing/null aliases: → [] → --prune deletes every alias |
blocking (P1) | confirmed → round 3 |
Lite cap (2 rounds) reached with 4 open blocking threads → the loop escalated to full mode (cap 3). Round 3 fixes G1/G3/G4/G5 and runs a targeted confirm on the diff since d473257.
|
To use Codex here, create an environment for this repo. |
…ct MX, explicit aliases Four review threads on the round-1 push (GitHub Codex review at d473257): - vacation_responder was only checked as a mapping, so `is_enabled: "false"` (a truthy string) enabled the responder on the API. The nested mapping now admits exactly Forward Email's three fields (is_enabled required and strictly bool, subject, message); export writes only those keys and the reconciler compares only those keys, so the three stay consistent and an extra live key is neither written nor a perpetual diff. - --drift resolved the zone directory as "the first YamlProvider" globally; a domain sourced from a second one was checked in the wrong directory and reported "no zone file; not checked" with rc 0. ZoneLookup resolves per domain from zones.<domain>.sources (or "*"), falls back to the only YamlProvider, and with several and no naming entry reports an explicit "ambiguous" finding (rc 1) instead of silence. - The MX check only required the two FE hosts to appear. It now requires both at exactly one shared integer preference — every entry's preference is kept, so a duplicate host at another preference, a missing preference or a malformed entry is reported — and no other exchanger at all (lower preference routes mail away, higher is a backup FE does not know about); ignore_mx_check remains the opt-out for split routing. Legacy octoDNS `value`/`priority` spellings are accepted. - A missing or null `aliases:` loaded as [], so --prune would have deleted every non-mailbox alias of a domain whose file merely forgot the key. It is now an error that names `aliases: []` as the explicit form; export already writes that. Same class, same fix: an alias without `recipients:` no longer reads as [] (which would PUT no recipients), and `domains:` null in the block is covered by the existing non-empty check. Verify: 423 passed (396 before; 27 tests added, 1 renamed).
Member
Author
|
@codex review |
Member
Author
/gmr round 3 — targeted confirm on the diff since
|
| # | Thread | Claim | Class | Status | Fix at ec06620 |
|---|---|---|---|---|---|
| G1 | P2 config.py:111 |
vacation_responder.is_enabled: "false" is truthy |
blocking | fixed | strict schema: exactly is_enabled (required bool), subject, message; export and reconcile use the same three keys — this supersedes round 1's "export every key" (thread 10), as that thread's own alternative suggested |
| G2 | P2 config.py:304 |
unknown keys under defaults: ignored |
blocking | fixed @ f76b8ee |
BLOCK_FIELDS/DEFAULTS_FIELDS key checks; test covers defualts:, defaults.setings:, tokn: |
| G3 | P2 cli/forward_email.py:46 |
first YamlProvider used for every domain |
blocking (latent) | fixed | ZoneLookup resolves per domain from zones.<domain>.sources (or '*'), falls back to the only YamlProvider; several providers with no naming entry → an "ambiguous" drift finding (rc 1), never "not checked" |
| G4 | P2 drift.py:129 |
MX check ignores preference and competing exchangers | blocking | fixed | unless ignore_mx_check: apex MX must be exactly mx1/mx2.forwardemail.net at one shared integer preference; any other exchanger, duplicate FE entry at another preference, missing preference or malformed entry is a finding (8-case test); all 24 live zones stay clean |
| G5 | P1 config.py:351 |
absent/null aliases: → [] → --prune deletes every alias |
blocking | fixed | absent/null aliases: is a config error naming aliases: [] (which export already writes; round trip tested). Same class: an alias without recipients: is rejected instead of reading as [] |
| R3-1 | local Codex lens | G4 fix: both FE hosts without a preference read clean; duplicate host overwritten | blocking | fixed | per-host preference sets; the FE union must be one int |
| R3-2 | local Codex lens | G4 fix: non-dict / host-less MX entries skipped → FE pair + malformed extra read clean | blocking | fixed | malformed entries collected and reported |
Fold-in audit at ec06620 (all 26 adjudicated findings across three rounds): 26/26 complete. Local Codex confirm pass at ec06620: 0 findings. Diff vs main: 18 files, +3146/−3 (+12.5% this round, under the 20% stop).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec06620126
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ct duplicate YAML keys, merge zone sources - reconcile: refuse an update that would turn IMAP off on a live mailbox only because the file omits `has_imap` (an explicit `has_imap: false` is intent); refuse an alias with no recipients and no mailbox; the prune error now says "add it to git with `has_imap: true`" - config: reject duplicate YAML mapping keys (PyYAML kept the last block silently; with --prune that deleted the first block's aliases); used for config.yaml, the per-domain files, the export-preserve read and zone files - drift: read and merge every YamlProvider a zone's `sources:` names; compare the return-path CNAME and record names case-insensitively - cli: `--prune` with `--export`/`--drift` is a usage error (rc 2); the exit code docstring scopes "a finding" to --drift - makefile: PRUNE=1 means the literal 1 (`$(filter 1,$(PRUNE))`); 0/false no longer prune - README: the mailbox remedy names `has_imap: true`; mail-drift proves nothing for a domain without a zone file; `recipients: []` only for a mailbox; duplicate keys are an error Addresses the code-reviewer termination pass (CR1, CR3-CR5) and the GitHub Codex threads H1-H5 posted at ec06620.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2072407243
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Member
Author
/gmr round 3 (second targeted re-confirm) — pushed
|
| # | Source | Claim | Class | Status | Fix at 2072407 |
|---|---|---|---|---|---|
| H3 | P1 config.py:218 |
duplicate YAML mapping keys silently kept the last block → --prune deletes the aliases from the discarded one |
blocking | fixed | strict SafeLoader subclass rejects duplicate keys ("duplicate key 'aliases' at line N (first at line M)"); used for config.yaml, per-domain files, the export-preserve read and zone-file reads |
| H5 | P1 makefile.py:158 |
PRUNE=0 / PRUNE=false still passed --prune |
blocking | fixed | $(if $(filter 1,$(PRUNE)),--prune,) on mail-plan and mail-apply; only the literal PRUNE=1 prunes |
| H1 | P2 cli/forward_email.py:67 |
a zone sourced from several YamlProviders was checked against the first only | blocking | fixed | ZoneLookup.directories() returns every named YamlProvider; drift loads each existing <domain>.yaml and merges same-name entries |
| H2 | P2 drift.py:97 |
return-path CNAME compared case-sensitively | blocking | fixed | lowercased after rstrip("."); record-name lookups case-insensitive |
| H4 | P2 config.py:349 |
recipients: [] with has_imap: false passed validation → alias with no delivery target |
blocking | fixed | plan error after defaults resolve ("give it recipients or has_imap: true"); the account's only zero-recipient alias is a mailbox, export→plan stays zero-diff |
| CR1 | termination lens | a live mailbox declared in git without has_imap planned a has_imap: false PUT — the prune guard's own "add it to git" remedy led into it |
blocking | fixed | update-path guard: live has_imap: true + undeclared has_imap in the file → plan error naming has_imap: true; an explicit has_imap: false is intent and still applies. Prune remedy text and README updated |
| CR3 | termination lens | README implied mail-drift covers a domain with no zone file |
wording | fixed | README states drift proves nothing about such a domain |
| CR4 | termination lens | --prune silently ignored with --export/--drift |
wording | fixed | parser.error on the combination |
| CR5 | termination lens | exit-code docstring ambiguous between plan and drift | wording | fixed | docstring scoped: findings → rc 1 in drift only; plan/apply rc 1 only on errors/aborts |
| CR2 | termination lens | PR body test counts stale | wording | fixed | PR body edited |
Local Codex targeted re-confirm at 2072407: 0 findings; fold-in audit of all 33 prior findings: complete. Diff vs main: 18 files, +3426/−3 (+8.9% this pass).
…rs in drift, reject non-string keys - cli: `--dry-run --export` is a usage error (rc 2) — export always writes the domain files, so a "dry run" of it overwrote every selected file; `--dry-run --drift` stays allowed (drift writes nothing) - cli: the drift branch now reports every `plan.errors` entry as an `alias` finding (rc 1) instead of reading "clean" for a file plan/apply refuses - config: `_check_keys` rejects non-string mapping keys with a clear message; an unquoted `on:` loads as True and made the unknown-key join raise TypeError (traceback instead of rc 2/1) Addresses GitHub Codex threads I1-I3 posted at 2072407.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Member
Author
/gmr round 3 (third targeted re-confirm) — pushed
|
| # | Source | Claim | Class | Status | Fix at eeaf3d7 |
|---|---|---|---|---|---|
| I1 | P2 cli/forward_email.py:287 |
--dry-run --export still wrote every selected domain file |
blocking | fixed | usage error (rc 2, before config/network) — export always writes, there is no preview of it; --dry-run --drift stays allowed |
| I2 | P2 cli/forward_email.py:236 |
drift discarded plan.errors → a file plan/apply rejects read clean, rc 0 |
blocking | fixed | every plan.errors entry is an alias: drift finding (rc 1); guards needing live alias state (mailbox update, prune) remain plan/apply-only and the docstring says so |
| I3 | P3 config.py:270 |
non-string YAML key (on: → True) crashed _check_keys with a TypeError traceback |
blocking (crash instead of a config error) | fixed | non-string keys rejected first with a ForwardEmailConfigError naming the key and suggesting quoting |
| W | local Codex lens | docstring overstated which guards drift surfaces | wording | fixed | docstring scoped |
Local Codex targeted re-confirm at eeaf3d7: 0 blocking. Diff vs main: 18 files, +3481/−3.
Member
Author
/gmr — MR-READY: review converged at
|
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.
Opt-in GitOps for the Forward Email account side of a dns-zones repo, shaped like
delegation:/octodns-gitops-delegate.forward_email:block inconfig.yamlclaims a domain list (the ownership and prune boundary); onemail/forward-email/<domain>.yamlper claimed domain withsettings:(the 13 API-writable fields only),expect:(read-only fields, drift-reported) andaliases:. Package defaults are the only layer shared across repos.octodns-gitops-forwardemail: dry-run by default,--doitwrites,--domainscopes. Never creates or deletes domains.--pruneis off by default, scoped to claimed domains, re-lists before deleting, and refuses to delete any alias withhas_imapor stored mail.--exportbootstraps/re-baselines the files;--driftchecks FE's generated DNS records (DKIM,fe-bounces, verification TXT, DMARCrua, MX) against the zone file.mail-plan,mail-apply,mail-drift,mail-export(DOMAIN=,PRUNE=1).Tests: 181 new (
tests/forward_email/,tests/cli/test_forward_email.py, Makefile template), full suite 448 passing (basemain: 267), no network in tests (transport injected; an autouse fixture fails any call that reaches the real one).Follow-up, not in this PR: the CalVer tag, then the per-repo rollout (autops first, ginsys second) with a zero-diff first plan as the acceptance test.