Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe ChangesIPv4-only
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was identified; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change narrows network reachability rather than expanding it. No introduced security attack path was identified, but preserved legacy configurations and interrupted upgrades leave limited uncertainty about uniform IPv4-only enforcement. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
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: 5b98363775
ℹ️ 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".
| comma_separated_hostnames(&route.primary_hostname)? | ||
| )); | ||
| output.push_str(" bind 127.0.0.1 ::1\n"); | ||
| output.push_str(" bind 127.0.0.1\n"); |
There was a problem hiding this comment.
When upgrading while a linked Project's config is unreadable or invalid, build_runtime_plan sets render_config: false, and gateway_project_config_fragments plus worker_project_config_fragments reuse the active fragments verbatim. These new IPv4-only renderers are therefore bypassed, leaving legacy bind 127.0.0.1 ::1 Gateway and worker fragments active after reconciliation and violating the new IPv4-only runtime contract. The startup test covers only a valid Project config, so the preserved-fragment path also needs to apply this bind transition.
Useful? React with 👍 / 👎.
| output.push_str("}\n"); | ||
| output.push_str(&format!( | ||
| "\nhttp://{GATEWAY_HEALTH_HOSTNAME} {{\n bind 127.0.0.1 ::1\n" | ||
| "\nhttp://{GATEWAY_HEALTH_HOSTNAME} {{\n bind 127.0.0.1\n" |
There was a problem hiding this comment.
With the Gateway now bound only to IPv4, pv ports:install still rejects every low port returned by loopback_tcp_listener_ports() and uses the same family-agnostic set when assigning Gateway backend ports. On macOS that set deliberately includes pure ::1 listeners, so an unrelated IPv6-only service on port 80 or 443 blocks setup even though both the generated inet PF redirects and Caddy use 127.0.0.1; an IPv6-only listener on a preferred backend port also needlessly forces fallback allocation. The conflict and allocation checks need to consider IPv4 occupancy for this IPv4-only contract.
Useful? React with 👍 / 👎.
| )); | ||
| } | ||
| _ => {} | ||
| if is_test_name(query.name()) && query.query_type() == RecordType::A { |
There was a problem hiding this comment.
For every common .test AAAA lookup, this condition now emits an authoritative NOERROR/NODATA response with empty answer and authority sections. RFC 2308 section 3 requires an authoritative server to include the zone SOA when the requested record type has no data, because that record supplies the negative-cache TTL; without it, resolvers should not cache the response and can query PV again on every lookup rather than honoring the intended five-second TTL. The new tests only verify that the answer section is empty, so they do not catch the missing SOA.
Useful? React with 👍 / 👎.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The IPv4-only bind policy does not cover preserved last-valid project fragments, so an upgrade can leave Gateway and worker IPv6 listeners active.
Reviewed changes Reviewed the full DNS, Gateway, worker, documentation, and test changes in this PR.
- DNS responses:
.testA queries retain127.0.0.1answers; AAAA queries now return authoritative NOERROR without answers. - Loopback listeners: Generated Gateway health, fallback, project, and worker sites bind explicitly to
127.0.0.1only. - Startup coverage: A new daemon startup test checks replacement of old dual-loopback fragments for a valid project, with updated renderer snapshots and DNS assertions.
- Design contract: IPv4-only serving becomes the v1 requirement; IPv6 serving is deferred until dual-loopback PF routing is implemented.
Local verification passed all eight renderer tests and the DNS response unit test. The daemon integration test could not run on this Linux host: daemon IPC is unsupported on linux; startup preservation behavior was reviewed statically.
gpt-6.1-sol ｜ 𝕏
| comma_separated_hostnames(&route.primary_hostname)? | ||
| )); | ||
| output.push_str(" bind 127.0.0.1 ::1\n"); | ||
| output.push_str(" bind 127.0.0.1\n"); |
There was a problem hiding this comment.
[P2] Apply the bind policy to preserved project fragments too
If an already-served project's pv.yml is invalid when this version starts, build_runtime_plan sets render_config: false and both fragment builders copy the old Caddyfiles verbatim, bypassing these changed renderers. The Gateway and worker can therefore retain bind 127.0.0.1 ::1 and continue listening on IPv6, contrary to the new runtime bind contract; the added startup test misses this because it seeds only a valid project.
Technical details
# Preserved fragments bypass IPv4-only rendering
## Affected sites
- `crates/daemon/src/gateway.rs:2656-2701` - invalid config selects `append_persisted_runtime_project`.
- `crates/daemon/src/gateway.rs:3303-3369` - reconstructs the previous runtime with `render_config: false`.
- `crates/daemon/src/gateway.rs:5075-5113` and `5116-5161` - reuse preserved Gateway and worker fragment contents without applying the new bind policy.
- `crates/daemon/src/jobs.rs:2691-2711` - system/startup reconciliation still attempts Gateway reconciliation after project errors.
- `crates/daemon/tests/daemon_foundation.rs:1655-1702` - the new replacement test covers valid config only.
## Required outcome
- Apply the IPv4-only runtime bind contract to preserved last-valid Gateway and worker fragments while retaining their last-valid routing and serving configuration.
- Add a macOS startup regression case with previously served/persisted project state, old dual-loopback fragments, and an invalid `pv.yml` before daemon startup; verify both layers no longer contain an IPv6 bind.
- Changing only the valid-project renderer or its snapshots is insufficient, because the invalid-project path never invokes those renderers.
This does not imply LAN exposure or a proven IPv4 routing failure. The concrete issue is that the old IPv6 listeners remain active, and the worker can retain its unchanged fragment/fingerprint on the no-op path.Code references: invalid-config planning, preserved fragment construction, and existing invalid-config preservation coverage. Caddy documents that each host in bind creates a listener for that interface.
1 participant
PV v1 now serves
.testover IPv4 loopback. AAAA queries return authoritative NOERROR with no answers, and Caddy keeps an explicit127.0.0.1bind so neither Gateway nor workers listen on LAN interfaces.This establishes the runtime bind contract needed by the macOS 27 port-probe fix. Dual-loopback serving needs IPv6 pf redirects and remains a separate feature. Actual daemon startup from old dual-loopback Gateway and worker fragments regenerates both without stale or duplicate sites; no migration shim was needed.
Validation: formatting, workspace Clippy with warnings denied, and cargo shear pass. All 9 focused tests pass. The full macOS 27 CI-profile suite ran 1,601 tests: 1,600 passed, with only the expected kernel-table acceptance failure tracked in #402. The final PR in this stack removes that API. #403 must merge after the stack.
Summary by CodeRabbit
.testhostnames now resolve to IPv4 loopback (127.0.0.1) for A queries. AAAA queries return no records.