Preserve Asuswrt-Merlin dnsmasq postconf hooks

[maghuro]

Important

Local staging/review draft in the fork only. Per upstream contribution guidelines, an upstream issue must be filed before any PR is submitted to Control-D-Inc/ctrld. This draft will not be submitted upstream until that issue exists.

Summary

Make ctrld coexist safely with an existing Asuswrt-Merlin main dnsmasq.postconf hook without changing the existing dnsmasq snapshot/NVRAM lifecycle.

Changes

• manage only ctrld-owned logic inside explicit # BEGIN ctrld / # END ctrld markers
• migrate the legacy # GENERATED BY ctrld - EOF format used by ctrld <= 1.5.7
• preserve unrelated hook content and block ordering
• remove the generated exit 0 so later user/addon hook logic still executes
• preserve existing hook modes and symlink topology
• revalidate the shared hook before setup and cleanup replacements
• write via sibling temporary file, fsync the file and parent directory, then rename
• keep synthetic shebang bytes explicitly ctrld-owned and reversible
• add regression tests for legacy migration, idempotence, exact marker matching, round trips, permissions, symlinks and concurrent hook changes

Intentionally out of scope

This PR does not change the existing full /jffs/configs/dnsmasq*.conf snapshot behavior and does not add Guest Network Pro / SDN support. Those broader Merlin 3006 changes are staged separately in #2.

Context: Control-D-Inc#229.

[maghuro]

@codex review

Please focus especially on:

• migration from the legacy # GENERATED BY ctrld - EOF postconf format;
• preserving unrelated Merlin hook content and execution order;
• idempotence of repeated setup/cleanup;
• dnsmasq-sdn.postconf compatibility on Merlin 3006;
• whether any error path could remove or corrupt an existing user/addon hook.

This is intentionally the first, narrow layer of a broader Merlin integration cleanup.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a2cf7483e

ℹ️ 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".

[maghuro]

@codex review

Addressed all four findings from the previous review:

• legacy migration now removes only the bounded ctrld header..EOF range and preserves prefixes/suffixes;
• an existing managed block is replaced in place instead of being moved;
• ctrld_setup/NVRAM restoration now happens only after hook/snapshot cleanup succeeds, keeping cleanup retryable;
• shared postconf files are replaced atomically via sibling temp file + chmod + write + fsync + close + rename;
• added regression tests for legacy prefixes/suffixes and block-position preservation.

Please re-review the current head, especially migration byte preservation and atomic-write/error paths.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b0389a096

ℹ️ 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".

[maghuro]

@codex review

One more pass please. The two new findings are now addressed:

• ctrld records whether it created a hook file; cleanup only deletes a minimal stub when that ownership marker exists, otherwise a pre-existing stub is preserved;
• atomicWriteFile resolves existing symlink targets and atomically replaces the target, preserving the symlink itself;
• tests cover both cases.

Please review current head 7dddf37, especially marker lifecycle if another addon edits a ctrld-created hook while ctrld is running.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7dddf37115

ℹ️ 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".

[maghuro]

@codex review

Addressed the stale ownership-marker finding by removing deletion ownership entirely: cleanup now only removes ctrld's managed block and never deletes the shared hook path, even if the remaining file is just a shell stub. This deliberately favors preserving user/addon state over reclaiming a few bytes.

Please review current head df2be2d, with focus on whether any remaining path can delete or reorder unrelated hook content.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df2be2d67e

ℹ️ 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".

[maghuro]

@codex review

Addressed both findings from the previous pass:

• ownership markers are now recognized only when they occupy complete lines, for both the current BEGIN/END format and the legacy GENERATED/EOF format;
• cleanup consumes ctrld-owned separator line endings so setup -> cleanup restores the original hook bytes;
• added regressions for marker-like shell variables/strings and an exact byte-for-byte setup/cleanup round trip, including CRLF.

Also fixed the old legacy fixture so it actually uses the legacy ctrld header.

Please review current head d2e7540.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2e7540898

ℹ️ 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".

[maghuro]

@codex review

Addressed the latest three findings:

• legacy migration/cleanup now models the real <=1.5.7 writer layout, including the three newline separators after the EOF marker, and normalizes that layout when migrating to BEGIN/END blocks;
• pre-existing shared hooks must already have a newline-terminated shebang; ctrld refuses to synthesize wrapper bytes into third-party hook content, while newly created hooks still get their own shebang;
• atomic replacement preserves the resolved target's existing mode and still preserves symlink topology;
• added regressions for exact legacy shebang restoration, invalid existing hooks, and 0700/0770 permission preservation.

Please review current head fcae247, including compile/test consistency after the merlinPostConfBlock signature change.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcae2470c1

ℹ️ 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".

[maghuro]

@codex review

Addressed all findings from the previous pass:

• setup now establishes ctrld's existing NVRAM/setup transaction state before modifying shared hooks or persistent dnsmasq files, and defers Cleanup on any later failure, so failures in hook install, snapshot setup, dnsmasq restart, etc. are no longer invisible to the cleanup guard;
• hook preflight now records path type/symlink target/content and revalidates those immediately before each commit; appeared/disappeared paths, changed content, type changes and symlink-target changes abort rather than overwrite concurrent addon changes;
• the rollback failure test now injects a deterministic second-write error and no longer depends on chmod/root/Windows semantics;
• added regressions for content changes and appeared/disappeared hook paths between preflight and commit.

Please review current head c77595c, especially the whole-Setup rollback path and whether moving SetKV before dnsmasq regeneration creates any remaining ordering/recovery issue.

[chatgpt-codex-connector]

💡 Codex Review

ctrld/internal/router/merlin/merlin.go

Lines 407 to 409 in c77595c

	for _, path := range dnsmasq.AdditionalConfigFiles() {
	jffsConfPath := filepath.Join(dnsmasq.MerlinJffsConfDir, filepath.Base(path))
	cfgs = append(cfgs, &dnsmasqConfig{path, jffsConfPath})

Track SDN snapshots instead of rediscovering them

When an SDN runtime config exists during Setup but is removed or renamed before Cleanup—for example, because that guest network is deleted—AdditionalConfigFiles no longer returns it, so the matching ctrld-modified /jffs/configs/<basename> file is never removed. If that SDN is later recreated after ctrld has stopped, the stale persistent configuration can still direct it to ctrld. Persist or otherwise enumerate the destination paths actually installed rather than deriving cleanup solely from the current /tmp/etc files.

ℹ️ 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".

[maghuro]

@codex review

Current head c77595c now addresses the previous review by:

• making rollback failure injection deterministic through an injected writer;
• revalidating path existence/type, symlink target and content immediately before each commit;
• publishing ctrld_setup before shared-hook/snapshot mutations and deferring Cleanup so failures after successful hook writes are rollback-visible.

Please review the current head from scratch, especially transaction boundaries, rollback safety, pre-existing /jffs/configs/dnsmasq*.conf handling, symlink races, and test portability.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c77595c6b0

ℹ️ 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".

[maghuro]

@codex review

The PR has now been deliberately narrowed back to one concern: safe coexistence in the shared main Asuswrt-Merlin dnsmasq.postconf.

Current head 390142b:

• does not change Setup/NVRAM/snapshot lifecycle;
• does not add SDN support (that remains in stacked PR Prefer Merlin dnsmasq hooks and fix Guest Network Pro integration #2);
• manages only ctrld's exact BEGIN/END block in the main hook;
• migrates the <=1.5.7 legacy format;
• removes the enclosing-script exit 0;
• preserves unrelated hook bytes, symlink topology and existing modes;
• revalidates shared path/type/symlink/content before both setup and cleanup writes;
• uses atomic write + file fsync + parent-directory fsync on Merlin/Unix;
• has Windows-safe permission assertions.

Please review this current narrow head from scratch and ignore findings tied to the older lifecycle/SDN version of the branch.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 390142b363

ℹ️ 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".

[maghuro]

@codex review

Addressed the remaining P2 on current head 57f4a5d:

• the entire ctrld-managed postconf template now runs inside a subshell, isolating config_file, pid, sourced helper functions/variables and any other temporary state from the surrounding shared hook;
• the no-exit 0 regression now also asserts that the managed template is subshell-isolated.

Please review the current narrow head again.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57f4a5d00d

ℹ️ 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".

[maghuro]

@codex review

Fixed the subshell regression on current head 4de5592:

• MerlinPostConfTmpl now uses an actual newline immediately after (, not a literal raw-string \n;
• regression test now requires newline-delimited subshell boundaries and explicitly rejects a literal \nconfig_file sequence.

Please review the current narrow head again.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 4de5592fe9

ℹ️ 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".

[maghuro]

Final local validation note (no further Codex review requested due review-credit limit):

• The last Codex review of the implementation at 4de5592 reported no major issues.
• The remaining Ubuntu CI failure was traced to the test fixture itself: os.WriteFile obeyed the runner umask, so a requested 0770 fixture was actually 0750 before atomicWriteFile ran.
• Current head 9418d08 fixes the fixture with an explicit chmod. Ubuntu go test -race ./... and staticcheck now pass on this head.
• macOS continues to fail in setup-go-faster before tests, unrelated to the PR.

No implementation finding remains open from the Codex review history.

[maghuro]

Final local readiness note:

• current head: 9418d08163;
• last production-code Codex review passed with "Didn't find any major issues. Breezy!" at 4de5592fe9;
• the only later change is a test-fixture/umask correction in merlin_test.go, with no production-code change;
• current-head CI passes on Ubuntu and Windows;
• macOS fails in WillAbides/setup-go-faster@v1.8.0 before repository tests start.

No material Codex finding remains unaddressed.

[maghuro]

Superseded by the maintained v1.x port in #6, now merged into v1.0. This main-based draft is retained only as review history/source-of-truth for the port.