feat(minimald): Unit 3 DNS — B5 host-side egress proxy + .min.internal hostname registry - #546
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the systemd-resolved startup probe with a host-side HTTP egress proxy and in-memory hostname registry. Introduces Changes*.localhost Hostname Registry, Host-side Egress Proxy, and Session Lifecycle
Sequence Diagram(s)sequenceDiagram
participant daemon
participant proxy
participant registry
participant upstream
participant client
daemon->>proxy: bind_listener(DEFAULT_PROXY_ADDR)
alt bind succeeds
proxy-->>daemon: listener available
else bind fails
proxy-->>daemon: listener unavailable and warning logged
end
client->>proxy: HTTP request with Host or CONNECT authority
proxy->>registry: resolve(host)
alt host mapped
proxy->>upstream: connect and forward request
upstream-->>proxy: upstream bytes
proxy-->>client: relay response
else host not mapped
proxy-->>client: 502 Bad Gateway
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
crates/minimald/src/sessions.rs (1)
254-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment mismatch: hostname is captured before deletion, but deregistered after.
The comment on lines 254-255 states "Withdraw the PTask hostname before the record is removed", but the actual
deregistercall (lines 265-268) happens afterself.store.delete(&k)on line 264.What actually happens is the hostname is captured on line 257 before deletion, but the registry withdrawal occurs afterward. This is functionally correct (the captured name is still valid), but the comment should clarify: "Capture the hostname before deleting the record, then deregister it afterward."
📝 Proposed comment clarification
- // Withdraw the PTask hostname before the record is removed - // (R3.5). A no-op for a session that never registered one. + // Capture the PTask hostname before deleting the record, + // then deregister it afterward (R3.5). A no-op for a + // session that never registered one. #[cfg(target_os = "linux")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/sessions.rs` around lines 254 - 268, The comment describing "Withdraw the PTask hostname before the record is removed" does not accurately reflect the actual code flow. The hostname is indeed captured before deletion (on line 257 with registry_name), but the actual deregister call happens after the store deletion. Update the comment above the host_net_name variable assignment to clarify that the hostname is captured first before the record is deleted, and then deregistered afterward, making the sequence explicit to future readers.crates/minimald/src/net/dns.rs (2)
178-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded "systemd-resolved" warning may mislead on non-Linux platforms.
The warning at lines 188-193 specifies
resolver = "systemd-resolved"andremedy = "systemctl enable --now systemd-resolved", which are Linux-specific. Ifprobe_resolverwere called on macOS (where*.localhostsynthesis is built-in), a failure would emit misleading remediation advice.The current call site in
main.rsis Linux-only (UDS path, non-vsock), but the function itself is not platform-gated. Consider either:
- Adding
#[cfg(target_os = "linux")]toprobe_resolver, or- Making the warning message platform-conditional
🔧 Option 1: Gate the function
+#[cfg(target_os = "linux")] pub fn probe_resolver<F>(resolve: F) -> ProbeOutcome🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/net/dns.rs` around lines 178 - 198, The probe_resolver function contains hardcoded Linux-specific resolver names and remediation commands in its warning message but lacks platform-specific gating, which could mislead users on non-Linux systems like macOS. Add the #[cfg(target_os = "linux")] attribute above the probe_resolver function definition to restrict compilation of this function to Linux platforms only, ensuring platform-specific advice is not shown on incompatible systems.
96-109: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reducing hostname clones from two to one.
The hostname is cloned on line 99 for
by_session.insert()and again on line 100 forby_host.insert(). You could insert into one map first, then clone from the map key for the second insert, saving one allocation.♻️ Proposed optimization
pub fn register(&mut self, session_name: &str, target: IpAddr) -> Hostname { let hostname = Hostname::for_ptask(session_name, &self.host_id); - self.by_session - .insert(session_name.to_string(), hostname.clone()); - self.by_host.insert(hostname.clone(), target); + self.by_host.insert(hostname.clone(), target); + self.by_session + .insert(session_name.to_string(), hostname.clone()); tracing::info!(Or use entry API to avoid one clone entirely, though that's more complex.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/net/dns.rs` around lines 96 - 109, The register method in the DNS registration code is cloning the hostname value twice unnecessarily, once for the by_session.insert() call and once for the by_host.insert() call. Refactor this to reduce the number of clones from two to one by either inserting into one map first and then cloning from that map's stored value for the second insert, or use Rust's entry API to avoid the clones entirely while maintaining the same insertion logic. Ensure the returned hostname value at the end of the method remains the same.crates/sessions/src/core/decision.rs (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that
DenyOnceaborts resolution.The resolver currently turns
DenyOnceintoResolveError::Denied, so “Reject this item” can read like a per-item drop while the actual contract aborts the whole resolution.Suggested doc tweak
- /// Reject this item without recording a rule. + /// Reject this item without recording a rule, causing resolution to + /// fail with a denied-item error. DenyOnce,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sessions/src/core/decision.rs` around lines 44 - 47, The documentation comment for the DenyOnce variant in the decision enum currently states "Reject this item without recording a rule" which could be misinterpreted as only rejecting a single item rather than aborting the entire resolution process. Update the doc comment for DenyOnce to clarify that this decision aborts the whole resolution (converting to ResolveError::Denied) rather than just rejecting an individual item, making the contract of this variant explicit and unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sessions/docs/RESOLUTION.md`:
- Around line 163-168: The documentation in the RESOLUTION.md file references a
non-existent `ResolveError::PatchConfig` variant when describing how
configuration errors like `NoWalkRoot` are surfaced. The actual `ResolveError`
enum only contains the variants `Denied`, `Aborted`, `HookContract`,
`PatchWalk`, and `Expansion`. Update the documentation to remove the reference
to `PatchConfig` and instead clarify which actual variant from the
`ResolveError` enum (likely `PatchWalk` or another existing variant) is used for
handling configuration errors, ensuring the documentation matches the
implementation in crates/sessions/src/client/composer.rs.
- Around line 169-181: Update the documentation in the RESOLUTION.md file that
describes home resolution error handling. Replace the incorrect description of
ResolveError::HomeUnresolved with nested HomeResolutionFailure variants
(Unavailable, NotUtf8, NotAbsolute) with accurate documentation reflecting that
home resolution failures actually surface through
ResolveError::Expansion(ExpandError) with variants such as UndefinedVar (when
HOME is unresolved and no fallback is available) or NotAbsolute (when expansion
yields a non-absolute path). Ensure the documentation accurately describes how
and when these errors are raised during home directory expansion.
In `@crates/sessions/src/core/loadout.rs`:
- Around line 15-18: Update the documentation comment for the `dest` `~`
expansion behavior in the loadout.rs file to accurately reflect how the resolver
handles home directory lookup. The current comment incorrectly references
`dirs::home_dir` as the default, but the actual resolver uses a fallback through
the composer env lookup for `HOME`. Replace the mention of the `dirs::home_dir`
default with an accurate description of the HOME environment variable fallback
behavior used by the resolver in the composer context.
In `@crates/sessions/src/core/policy.rs`:
- Around line 354-365: The current implementation of `resolve_patches` expands
all raw policy lists via `PatchPolicy::expand_with` before applying the
user-origin bypass logic, which allows validation errors in `allow` and `deny`
patterns to fail user-only patches even though those fields should never apply
to user-origin patches. Refactor the expansion logic to either expand `ignore`
unconditionally first and only expand `allow` and `deny` patterns when non-user
patch sources are present, or explicitly document that validation of `allow` and
`deny` lists remains global even for user-origin patches. Ensure this behavior
is consistent across all call sites including those around lines 467-482 and
665-710 that also interact with patch policy expansion.
In `@crates/sessions/src/core/primitives.rs`:
- Around line 48-51: The documentation comment for Composer::resolve incorrectly
states that tilde expansion uses `dirs::home_dir` or `Composer::with_home(...)`
override, but the actual implementation derives the home directory from
`(self.env)("HOME").ok()` where `Composer::new` defaults to `std::env::var`.
Update the documentation comment to accurately reflect that the resolver expands
leading tilde in patch source patterns and PatchPolicy patterns by looking up
the HOME environment variable, with the environment accessor being customizable
via Composer initialization, rather than referencing dirs::home_dir.
---
Nitpick comments:
In `@crates/minimald/src/net/dns.rs`:
- Around line 178-198: The probe_resolver function contains hardcoded
Linux-specific resolver names and remediation commands in its warning message
but lacks platform-specific gating, which could mislead users on non-Linux
systems like macOS. Add the #[cfg(target_os = "linux")] attribute above the
probe_resolver function definition to restrict compilation of this function to
Linux platforms only, ensuring platform-specific advice is not shown on
incompatible systems.
- Around line 96-109: The register method in the DNS registration code is
cloning the hostname value twice unnecessarily, once for the by_session.insert()
call and once for the by_host.insert() call. Refactor this to reduce the number
of clones from two to one by either inserting into one map first and then
cloning from that map's stored value for the second insert, or use Rust's entry
API to avoid the clones entirely while maintaining the same insertion logic.
Ensure the returned hostname value at the end of the method remains the same.
In `@crates/minimald/src/sessions.rs`:
- Around line 254-268: The comment describing "Withdraw the PTask hostname
before the record is removed" does not accurately reflect the actual code flow.
The hostname is indeed captured before deletion (on line 257 with
registry_name), but the actual deregister call happens after the store deletion.
Update the comment above the host_net_name variable assignment to clarify that
the hostname is captured first before the record is deleted, and then
deregistered afterward, making the sequence explicit to future readers.
In `@crates/sessions/src/core/decision.rs`:
- Around line 44-47: The documentation comment for the DenyOnce variant in the
decision enum currently states "Reject this item without recording a rule" which
could be misinterpreted as only rejecting a single item rather than aborting the
entire resolution process. Update the doc comment for DenyOnce to clarify that
this decision aborts the whole resolution (converting to ResolveError::Denied)
rather than just rejecting an individual item, making the contract of this
variant explicit and unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbf4080a-97e6-4460-bea0-c213f30d7261
📒 Files selected for processing (27)
crates/minimald-rpc/src/lib.rscrates/minimald/src/main.rscrates/minimald/src/net/dns.rscrates/minimald/src/net/mod.rscrates/minimald/src/sessions.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/docs/RESOLUTION.mdcrates/sessions/src/client/composer.rscrates/sessions/src/client/enumerate.rscrates/sessions/src/client/hooks.rscrates/sessions/src/client/mod.rscrates/sessions/src/core/compose.rscrates/sessions/src/core/decision.rscrates/sessions/src/core/expansion.rscrates/sessions/src/core/lifecyclehook.rscrates/sessions/src/core/loadout.rscrates/sessions/src/core/mod.rscrates/sessions/src/core/policy.rscrates/sessions/src/core/primitives.rscrates/sessions/src/core/source.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/daemon/mod.rscrates/sessions/src/lib.rscrates/sessions/src/wire/errors.rscrates/sessions/src/wire/primitives.rscrates/sessions/src/wire/request.rsdocs/specs/03-spec-networking/03-spec-networking.md
💤 Files with no reviewable changes (6)
- crates/sessions/src/daemon/mod.rs
- crates/sessions/docs/COMPOSITION.md
- crates/sessions/src/lib.rs
- crates/sessions/src/daemon/composer.rs
- crates/sessions/src/core/mod.rs
- crates/sessions/src/core/compose.rs
| - **Source `~` is expanded at resolution; dest has no `~` to expand.** | ||
| Patch source `FileSet` patterns and `PatchPolicy` patterns have | ||
| their leading `~` expanded against the host home (via `Composer`'s | ||
| home lookup — `dirs::home_dir` by default) before the walker runs. | ||
| Patch *destination* paths (`PatchDest`) are always relative to the | ||
| sandbox user's home directory; `~` and absolute paths are rejected | ||
| at construction, so nothing needs to be expanded for dests. Patterns | ||
| retain their `~` form in returned policies, so save/load is | ||
| lossless. When any `~`-prefixed pattern is in scope, the home | ||
| lookup is invoked once up-front; failures surface as | ||
| `ResolveError::HomeUnresolved` (with an inner | ||
| `HomeResolutionFailure::{Unavailable, NotUtf8, NotAbsolute}` | ||
| distinguishing the cause) rather than silently matching nothing. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check home resolution error handling
# Find HomeUnresolved and HomeResolutionFailure definitions
ast-grep run --pattern 'HomeUnresolved' --lang rust crates/sessions/src/client/
ast-grep run --pattern 'HomeResolutionFailure' --lang rust crates/sessions/src/client/
echo "---"
echo "Check how home lookup failures are surfaced:"
rg -n 'home.*fallback|home.*lookup' -A 5 crates/sessions/src/client/composer.rsRepository: gominimal/minimal
Length of output: 1829
🏁 Script executed:
# Find ResolveError enum definition
rg -n "enum ResolveError" -A 30 crates/sessions/src/
# Also check if HomeUnresolved or HomeResolutionFailure exist anywhere
rg -n "HomeUnresolved|HomeResolutionFailure" crates/sessions/Repository: gominimal/minimal
Length of output: 3374
🏁 Script executed:
# Get the complete ResolveError enum definition
rg -n "enum ResolveError" -A 50 crates/sessions/src/client/composer.rs | head -80Repository: gominimal/minimal
Length of output: 2661
🏁 Script executed:
# Check the actual ExpandError type used for expansion failures
rg -n "enum ExpandError|pub enum ExpandError" -A 20 crates/sessions/src/core/expansion/Repository: gominimal/minimal
Length of output: 231
🏁 Script executed:
# Find the expansion module and ExpandError definition
fd -t f "expansion" crates/sessions/src/
rg -n "enum ExpandError|pub enum ExpandError" crates/sessions/src/ -A 20Repository: gominimal/minimal
Length of output: 2085
🏁 Script executed:
# Get complete ExpandError definition
rg -n "enum ExpandError" -A 50 crates/sessions/src/core/expansion.rs
# Also check if there's any home directory handling or home-specific errors
rg -n "home" -i crates/sessions/src/core/expansion.rsRepository: gominimal/minimal
Length of output: 7576
Update documentation to match actual error structure.
The documentation describes ResolveError::HomeUnresolved with nested HomeResolutionFailure variants (Unavailable, NotUtf8, NotAbsolute), but these do not exist. Home resolution failures actually surface through ResolveError::Expansion(ExpandError) with variants like UndefinedVar (when HOME is unresolved and no fallback is available) or NotAbsolute (when expansion yields a non-absolute path). Update the documentation to accurately reflect the actual error types returned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sessions/docs/RESOLUTION.md` around lines 169 - 181, Update the
documentation in the RESOLUTION.md file that describes home resolution error
handling. Replace the incorrect description of ResolveError::HomeUnresolved with
nested HomeResolutionFailure variants (Unavailable, NotUtf8, NotAbsolute) with
accurate documentation reflecting that home resolution failures actually surface
through ResolveError::Expansion(ExpandError) with variants such as UndefinedVar
(when HOME is unresolved and no fallback is available) or NotAbsolute (when
expansion yields a non-absolute path). Ensure the documentation accurately
describes how and when these errors are raised during home directory expansion.
|
Revise claim for head baf84d3. |
|
Auto-revise 1 of 3. |
This comment has been minimized.
This comment has been minimized.
| .insert(session_name.to_string(), hostname.clone()); | ||
| self.by_host.insert(hostname.clone(), target); | ||
| tracing::info!( | ||
| session_name, |
There was a problem hiding this comment.
MEDIUM — Spec compliance (R3.5): session_name field does not match the required session_id
R3.5 explicitly lists the expected structured-tracing fields:
session_id,hostname,ip,action(registered/deregistered)
Both register (here, line 102) and deregister (line 124) emit session_name instead of session_id. The PR notes acknowledge this deviation and explain the registry is keyed by name, but the field name still diverges from the spec. This matters for log-aggregation pipelines, alerting rules, or operator tooling that queries the canonical session_id field — they will miss these events.
Consider either:
- Aliasing the field at the tracing call site:
session_id = session_name(keeps the local variable name, satisfies the spec field name), or - Amending R3.5 in the spec to formally rename the field to
session_nameif the registry-by-name architecture is the intended long-term shape.
|
|
||
| /// The result of probing the system resolver for `*.localhost` synthesis (R3.4). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum ProbeOutcome { |
There was a problem hiding this comment.
MEDIUM — Rust coding standards: ProbeOutcome is public but lacks #[non_exhaustive]
Per the repository's Rust coding standards:
#[non_exhaustive]on public enums/structs that may grow.
ProbeOutcome is a pub enum with two variants. Without #[non_exhaustive], a downstream match that exhaustively covers Active and Inactive becomes a compile error the moment a third variant is added (e.g., a PartialSynthesis variant for resolvers that synthesize only some *.localhost names). Adding #[non_exhaustive] now keeps future extensions backwards-compatible.
| // Withdraw the PTask hostname before the record is removed | ||
| // (R3.5). A no-op for a session that never registered one. | ||
| #[cfg(target_os = "linux")] | ||
| let host_net_name = self.store.get(&k).ok().map(|o| registry_name(o.record())); |
There was a problem hiding this comment.
LOW — Correctness: hostname registry entry leaked when store.get fails during session teardown
let host_net_name = self.store.get(&k).ok().map(|o| registry_name(o.record()));If store.get returns Err (e.g. an I/O error reading the on-disk record), ok() converts it to None and the deregister call on line 267 is skipped. The hostname entry remains in self.hostnames for the lifetime of the process.
This is low-probability (the session was found moments earlier at line 248 via find_by_id), but the store read could still fail. Since the registry's deregister is a silent no-op for sessions that were never registered, the simpler and leak-proof alternative is to always call deregister using the name derived from the store key — or to propagate the ? here and let the caller handle the error before proceeding to store.delete.
This comment has been minimized.
This comment has been minimized.
| .insert(session_name.to_string(), hostname.clone()); | ||
| self.by_host.insert(hostname.clone(), target); | ||
| tracing::info!( | ||
| session_name, |
There was a problem hiding this comment.
MEDIUM · Spec compliance · R3.5
R3.5 (spec text unchanged in this PR) requires the tracing event field to be named session_id:
"...shall emit a structured
tracingevent with fields:session_id,hostname,ip,action..."
The implementation emits session_name instead. The same deviation is present in the deregister event (line ~125).
This is more than a renaming: SessionId is a UUID (stable, unique, unchangeable for the lifetime of a session record), while session_name is a human-readable string that is optional (falling back to the project dir basename), mutable via RenameSession, and potentially reused after destruction. A log-pipeline or monitoring consumer correlating events by the R3.5-specified session_id field will find nothing in these events.
The PR body notes "the registry is keyed by name per the architecture doc" — that is a valid reason for the registry’s internal key, but R3.5 governs the tracing event field, not the registry key. sessions::Record carries both id: SessionId and name: Option<String>, so emitting session_id alongside session_name is not blocked architecturally; the call sites in sessions.rs have access to the full record.
| if let Some(hnd) = self.running.remove(&k) { | ||
| hnd.destroy().await; | ||
| } | ||
| self.store.delete(&k)?; |
There was a problem hiding this comment.
MEDIUM · Correctness
If store.delete(&k)? returns Err here, it propagates early and self.hostnames.deregister(&name) (line 267) is never called. At this point the session actor is already destroyed (hnd.destroy().await on line 262 is irreversible), so the in-memory registry retains a stale entry for a session that no longer exists.
The proxy will continue routing Host: header lookups for this PTask to 127.0.0.1 until the daemon restarts (HostnameRegistry is rebuilt from scratch on restart). The failure is bounded in lifetime but real: requests routed to the dead session’s address will fail at the transport layer (connection refused) rather than receiving a clean not-found from the proxy.
The heading comment ("Withdraw the PTask hostname before the record is removed") captures the right intent but the actual deregistration call is placed after the fallible delete. Moving self.hostnames.deregister(&name) to before self.store.delete(&k)? — or at a minimum calling it unconditionally in the error path — would close this gap.
|
Commit pushed:
|
Respond to review comments on PR #546: - Mark `ProbeOutcome` `#[non_exhaustive]`, per the repository's Rust coding standards for public enums that may grow. - Stop leaking a hostname registry entry on session teardown: derive the deregister name from the record up front and let a read error propagate (the subsequent `delete` would fail on the same error) instead of silently dropping it via `.ok()`. `deregister` is called unconditionally since it is a no-op for a session that never registered a hostname. - Align spec R3.5 to the registry-by-name architecture: the structured tracing field is `session_name` (the registry key), not `session_id`, which is a distinct, separate identifier not available at this layer. Gate (native Linux, -p minimald, --locked): cargo fmt --check, build, clippy --all-targets -D warnings, and test (54 passed) all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the open, in-scope review comments and pushed Fixed:
Not actionable (skipped, with reason): the five CodeRabbit comments on Verification (native Linux,
|
This comment has been minimized.
This comment has been minimized.
| if let Some(hnd) = self.running.remove(&k) { | ||
| hnd.destroy().await; | ||
| } | ||
| self.store.delete(&k)?; |
There was a problem hiding this comment.
LOW · Correctness — Hostname not deregistered when store.delete fails.
The ? on store.delete(&k)? returns early if deletion fails, bypassing self.hostnames.deregister on line 270. At this point the session has already been torn down (lines 265–266), so the hostname entry persists in the in-memory registry indefinitely — a stale entry that would misdirect any future proxy routing until the daemon restarts.
The comment on line 254 claims that a store.get read error and a store.delete write error will always occur together ("the delete below would fail on the same error"), but that does not hold in general (e.g. the file is readable but the directory becomes read-only between the two calls). Consider calling self.hostnames.deregister before store.delete, so a delete failure leaves a stale on-disk record (repairable on restart) but never a stale routing entry:
#[cfg(target_os = "linux")]
self.hostnames.deregister(&host_net_name);
self.store.delete(&k)?;With this ordering the registry is always cleaned up; only the on-disk record outlives the failure.
| tracing::info!( | ||
| session_name, | ||
| hostname = %hostname, | ||
| ip = ?ip, |
There was a problem hiding this comment.
LOW · Correctness / Spec compliance (R3.5) — ip field in the deregistered event uses Debug format on Option<IpAddr>, producing ip=Some(V4(127.0.0.1)), while the registered event (line 104) uses Display on IpAddr, producing ip=127.0.0.1.
R3.5 requires both events to carry the ip field. With this formatting mismatch, any log-aggregation query filtering on ip=127.0.0.1 will match registered events but not deregistered ones.
by_host.remove returns Option<IpAddr> only because the map API requires it; in practice the entry will always be present (since by_session.remove found a matching hostname). Consider unwrapping with an expect to both make the invariant explicit and produce a consistently-formatted field:
let ip = self.by_host.remove(&hostname)
.expect("by_host inconsistent with by_session");
tracing::info!(
session_name,
hostname = %hostname,
ip = %ip,
action = "deregistered",
"deregistered PTask hostname"
);|
Commit pushed:
|
|
Addressed the open, in-scope review comments and pushed Fixed:
Already in place / no change:
Not actionable (skipped, with reason): the CodeRabbit comments on Verification (native Linux, Revert guard: the in-sandbox merged-change revert guard returned
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimald/src/sessions.rs (1)
188-208: 🩺 Stability & Availability | 🟠 MajorRenaming a running
HostNetsession leaks a stale registry entry.The registry is keyed by the mutable session name: registration mints the key using
registry_name(obj.record())at launch (line 207), and deregistration keys onregistry_name(self.store.get(&k)?.record())at teardown (line 273).RenameSession(lines 228–242) updates the stored record's name and pushes it to the running handle viaapply_record, but never re-keys thehostnamesregistry. A session started asA(registered asA.<host-id>.localhost→127.0.0.1) and later renamed toBwill callderegister("B")on destroy — a silent no-op since the registry still holds the old keyA— leaving the stale entry resident for the daemon's lifetime.Consider re-registering in the
RenameSessionarm forHostNetrecords (deregister the old name, register the new one), or keying the registry by the stableSessionId(already available inregister_host_net) so withdrawal is rename-independent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/sessions.rs` around lines 188 - 208, The hostnames registry for HostNet sessions uses the mutable session name as its key, but when a session is renamed via the RenameSession operation, the registry entry is not updated. This causes the old name to remain registered while deregistration on teardown fails silently because it looks for the new name. Fix this by adding re-registration logic in the RenameSession arm (around lines 228-242) to deregister the HostNet session under its old name and register it under the new name when the session record is renamed, or alternatively change the registry to be keyed by the stable SessionId instead of the mutable session name so that rename operations do not affect registry lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/minimald/src/sessions.rs`:
- Around line 188-208: The hostnames registry for HostNet sessions uses the
mutable session name as its key, but when a session is renamed via the
RenameSession operation, the registry entry is not updated. This causes the old
name to remain registered while deregistration on teardown fails silently
because it looks for the new name. Fix this by adding re-registration logic in
the RenameSession arm (around lines 228-242) to deregister the HostNet session
under its old name and register it under the new name when the session record is
renamed, or alternatively change the registry to be keyed by the stable
SessionId instead of the mutable session name so that rename operations do not
affect registry lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e91de4a-4b7f-4863-b524-eb21d1e81cee
📒 Files selected for processing (3)
crates/minimald/src/net/dns.rscrates/minimald/src/sessions.rsdocs/specs/03-spec-networking/03-spec-networking.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/specs/03-spec-networking/03-spec-networking.md
|
Revise claim for head 58bec9e. |
|
Auto-revise 2 of 3. |
This comment has been minimized.
This comment has been minimized.
| /// by hostname. The header's optional `:port` suffix is ignored and matching | ||
| /// is case-insensitive, matching how a real `Host:` header arrives. | ||
| #[must_use] | ||
| pub fn resolve(&self, host_header: &str) -> Option<IpAddr> { |
There was a problem hiding this comment.
LOW (Correctness) — split(':').next() strips the port by splitting on the first colon, which gives an incorrect host component for IPv6 Host: header values. For [::1]:8080 it yields [ rather than [::1]; for a bare [::1] it yields [ (the whole string is the correct result).
Currently harmless: the registry only holds *.localhost names and a proxy routing to a PTask will never receive an IPv6 literal as the Host: value. But the parsing is latently wrong and would silently return None instead of a potential match if this code path is ever reused or extended.
The standard approach for HTTP Host-header port stripping handles the bracket-enclosed IPv6 form:
let host = if host_header.starts_with('[') {
// IPv6 literal — strip the optional ":port" after the closing "]"
host_header
.find(']')
.map(|i| &host_header[..=i])
.unwrap_or(host_header)
} else {
host_header.split(':').next().unwrap_or(host_header)
};At minimum, a comment documenting that IPv6 Host: headers are intentionally unsupported here would prevent a future contributor from extending the code unaware of the edge case.
|
Re: the teardown deregistration ordering (review comment on
No source change to push for this thread.
|
|
Commit pushed:
|
|
Addressed the IPv6 Rather than only documenting the limitation, I took the robust-parse option: extracted a Added a focused unit test (
|
The `Host:` header port strip used split-on-first-colon, which yields `[` for a bracketed IPv6 literal such as `[::1]:8080` instead of `[::1]`. Extract a `host_component` helper that strips the optional `:port` after the closing bracket for the IPv6 form and at the first colon otherwise. The registry only holds `*.localhost` names so an IPv6 literal never routes, but the parse is now correct rather than latently wrong. Addresses a review comment on #546. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
Hold — do not merge. This PR's DNS mechanism (R3.4 systemd-resolved startup probe + Spec Re-scoping #500 to the spec B5 model:
|
|
/revise Re-align this PR to the re-scoped #500 task body (DNS mechanism = spec B5 host-side egress proxy, replacing the systemd-resolved narrowing). Concretely:
Factor |
|
/revise TLD decision: switch |
|
Commit pushed:
|
|
Pushed a follow-up commit addressing the actionable review feedback on the B5 egress proxy (
Gate (native Linux, in Not addressed here, by prior maintainer decision documented in the PR description: R3.1
|
|
Auto-resolve conflict 1 of 3. |
|
/revise resolve the merge conflict with main: merge main into this branch (or rebase onto it) and resolve the conflicts, then push to this PR branch. The conflict appeared after sibling PR #547 merged; most collisions are append-only unions in shared files (the crate manifest, the lib module / pub use list) — take the union of both sides. |
This comment has been minimized.
This comment has been minimized.
| // are resolved host-side and routed by `Host:` header through this proxy; | ||
| // the host resolver is never consulted. A bind failure warns with a | ||
| // remedy (this supersedes the former R3.4 systemd-resolved probe). | ||
| let _ = minimald::net::proxy::bind_listener(minimald::net::proxy::DEFAULT_PROXY_ADDR).await; |
There was a problem hiding this comment.
MEDIUM — Correctness / Spec compliance (R3.3)
bind_listener binds the proxy listener then immediately discards it — serve() is never called anywhere in the daemon startup path. PTask *.localhost hostnames are therefore not routable in production: any HTTP_PROXY-configured client will receive connection-refused rather than a routed response.
R3.3 requires that "a browser on the same host shall reach a webserver inside an OwnIp PTask by its hostname"; that end-to-end path depends on the proxy actually serving. The serve() function is implemented and covered by tests in proxy.rs, but is not wired in.
The task body for #500 explicitly scopes main.rs to a "reachability check" (bind and warn on failure), so this appears to be intentional scoping with the actual serve wiring deferred. If that's correct, the bind_listener comment (reachable rather than listening) is accurate, but R3.3 is not delivered by this PR and the deferred wiring should be tracked explicitly.
| let host = super::dns::host_component(authority); | ||
| let port = authority[host.len()..] | ||
| .strip_prefix(':') | ||
| .and_then(|rest| rest.parse().ok()); |
There was a problem hiding this comment.
LOW — Correctness
rest.parse::<u16>().ok() silently returns None for ports outside the u16 range (e.g., CONNECT myservice.dev.localhost:99999). Router::route then substitutes DEFAULT_UPSTREAM_PORT (80), misdirecting the CONNECT tunnel to port 80 instead of rejecting the malformed authority with a 400.
In a single-tenant local proxy this is an unlikely edge case, but a client with a misconfigured port gets a silent wrong-port connection rather than a clear error.
| use sessions::SessionId; | ||
|
|
||
| /// The DNS suffix every PTask hostname carries (see the module docs). | ||
| pub const LOCALHOST_SUFFIX: &str = "localhost"; |
There was a problem hiding this comment.
LOW — Spec compliance
The task body for #500 records the TLD as .min.internal ("TLD decided 2026-06-23 — Mike's preference"), but the implementation uses .localhost. The PR also amends the spec document (03-spec-networking.md) to reflect .localhost, making the implementation self-consistent.
The task body correctly notes "TLD is a free label since the proxy routes by Host: header", so .localhost is functionally equivalent. However, it diverges from the explicitly-stated format decision in the task body without a recorded rationale in the PR. Worth confirming the TLD change was intentionally agreed before merge.
…d HostNet registration
The execute branched before #528 (Split client and daemon) merged, so the diff reverted all of crates/sessions/** on top of #500's DNS work. Reset crates/sessions to match current main exactly, keeping only #500's intended minimald DNS registry changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
Respond to review comments on PR #546: - Mark `ProbeOutcome` `#[non_exhaustive]`, per the repository's Rust coding standards for public enums that may grow. - Stop leaking a hostname registry entry on session teardown: derive the deregister name from the record up front and let a read error propagate (the subsequent `delete` would fail on the same error) instead of silently dropping it via `.ok()`. `deregister` is called unconditionally since it is a no-op for a session that never registered a hostname. - Align spec R3.5 to the registry-by-name architecture: the structured tracing field is `session_name` (the registry key), not `session_id`, which is a distinct, separate identifier not available at this layer. Gate (native Linux, -p minimald, --locked): cargo fmt --check, build, clippy --all-targets -D warnings, and test (54 passed) all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address the open, in-scope review comments on the hostname registry: - R3.5 tracing fields (dns.rs:102, deregister): the events emitted `session_name` only. R3.5 governs the tracing event field and calls for `session_id` -- the stable, unique identifier log pipelines correlate on, where `session_name` is mutable (RenameSession) and reusable after a session exits. Plumb the `SessionId` through `register`/`register_host_net`, store it on the registration so `deregister` emits the same id, and carry both `session_id` and `session_name`. Realign R3.5 in the networking spec to list both. - `ip` field format mismatch (dns.rs deregister): the `deregistered` event used Debug on `Option<IpAddr>` (`ip=Some(V4(127.0.0.1))`) while `registered` used Display (`ip=127.0.0.1`), so a log filter on `ip=127.0.0.1` missed deregistrations. `by_host` is kept in sync with `by_session` by `register`, so unwrap with `expect` and format with Display to match. - Registry entry leaked on teardown (sessions.rs DestroySession): the `?` on `store.delete` returned early before `deregister`, leaving a stale routing entry pointing at a destroyed session. Deregister the hostname before the fallible on-disk delete, so a delete failure leaves a repairable on-disk record but never a stale routing entry. Gate (native Linux, -p minimald, --locked): cargo fmt --check, build, clippy --all-targets -D warnings, and test --lib (54 passed) all green.
The `Host:` header port strip used split-on-first-colon, which yields `[` for a bracketed IPv6 literal such as `[::1]:8080` instead of `[::1]`. Extract a `host_component` helper that strips the optional `:port` after the closing bracket for the IPv6 form and at the first colon otherwise. The registry only holds `*.localhost` names so an IPv6 literal never routes, but the parse is now correct rather than latently wrong. Addresses a review comment on #546. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-align PR #546 to the re-scoped #500 task body: the DNS mechanism is the spec B5 host-side egress proxy, not the systemd-resolved narrowing. - Keep the in-memory `HostnameRegistry` (register/deregister + R3.5/R3.6 structured tracing) but remove the resolver write/probe path; no host resolver is ever written or read. - Add `net/proxy.rs`: the B5 host-side egress proxy plus the shared `Router`/`HostRoute` routing core that #502 (B8 HTTPS/mTLS) extends. A forward/`CONNECT` proxy routes by `Host:` header / authority through the registry — `HostNet` to `127.0.0.1:<port>`, `OwnIp` to its gvproxy switch IP. The host resolver is never consulted; the TLD is opaque. - Replace the systemd-resolved startup probe (R3.4) with an egress-proxy listener reachability check that warns `component="dns-proxy", status="unavailable"` on bind failure. - Tests: registry/proxy routing contract, OwnIp routing to its switch IP, and the bind-failure warn; drop the systemd-resolved NXDOMAIN test. - Spec: close Open Question 1 with the B5 host-side-egress-proxy decision, superseding spike #485's systemd-resolved finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address CodeRabbit and sdd-review feedback on the B5 egress proxy: - Bound the request-head read with a 30s timeout, returning 408 so a client that connects but never sends \r\n\r\n cannot hold a connection task indefinitely (proxy.rs read_head deadline). - Add a forward-proxy test asserting an absolute-form request target (GET http://web.dev.localhost/path) is routed by Host: header and replayed verbatim to the upstream, per RFC 9112. - Document on Router::route that the upstream port is client-supplied and per-PTask loopback port restriction is a deferred multi-tenant follow-up (accepted single-tenant limitation). - Report the startup bind check as status="reachable" rather than "listening", since the probe drops the listener without serving.
Under the B5 host-side egress proxy the TLD is a free label (the proxy routes by Host header, no host resolver), so .min.internal is the semantically honest choice for internal PTask services and drops the misleading loopback connotation of .localhost. Renames the hostname suffix constant and updates the registry/proxy docs, routing tests, and the spec hostname format + Open Question 1 closure. Bare loopback localhost (mesh tunnel, listen address) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
8424380 to
d462f08
Compare
sdd-validate findings — PR #546Boundary resolved: Implementation Gate 1 — Proof artifacts re-executed and passingInfo — Deferred to consumer CI: Task #500 names three proof artifacts:
This agent runs inside the firewalled gh-aw container without a Rust toolchain or egress to crates.io; Warning — OwnIp routing proof artifact absent from this PR: Task #500 lists "Test: OwnIp routing" (R3.6) as a named proof artifact. The PR body explicitly deferred Gate 2 — Changed files within task scopeWarning —
Gate 3 — No real credentials in the diffClean. No tokens, keys, secrets, or credentials detected across all 7 changed files. LifecycleTracking issue #478 already carries Summary: 0 Blockers · 2 Warnings · 1 Info. No
|
| // as a startup reachability check. PTask `*.min.internal` hostnames (Unit 3) | ||
| // are resolved host-side and routed by `Host:` header through this proxy; | ||
| // the host resolver is never consulted. A bind failure warns with a | ||
| // remedy (this supersedes the former R3.4 systemd-resolved probe). |
There was a problem hiding this comment.
MEDIUM — Correctness / Spec R3.3
The egress proxy is never started. bind_listener binds the TCP socket to test reachability, but let _ = ... drops the returned Option<TcpListener> immediately at the semicolon (let _ is not a binding in Rust — it drops on the spot). serve() is never called anywhere, so the proxy never accepts a connection and no *.min.internal hostname is routable at runtime.
R3.3 requires a browser on the same host to reach a PTask via hostname through the host-side proxy. That cannot happen without a running serve() loop.
Wiring serve() would also need an architectural change: Manager owns hostnames: HostnameRegistry as a plain field with exclusive &mut self access, but Router::new() requires Arc<T: HostRoute>. The test suite bridges this with the Shared(RwLock<HostnameRegistry>) wrapper in proxy.rs, but nothing equivalent exists in the production path. Completing R3.3 therefore needs both the serve() call and a registry-sharing refactor.
If serve() wiring is intentionally deferred to #502 (the B8 mTLS extension that picks up this routing core), the PR description should name that as a known-deferred item alongside the #542 and Unit 4 deferrals, so a reviewer can confirm the gap is tracked.
| .read() | ||
| .expect("registry lock is never held across a panic") | ||
| .resolve(host) | ||
| } |
There was a problem hiding this comment.
LOW — Correctness
buf.windows(4) re-scans the buffer from byte 0 on every chunk read, making end-of-head detection O(n2) in the number of bytes buffered. With a 512-byte chunk and MAX_HEAD = 8 KiB, the worst case is at most 16 iterations × 8 192 comparisons ≈ 130 k comparisons — a negligible constant, and bounded further by HEAD_READ_TIMEOUT. In practice this is not a performance or DoS concern.
A trivial fix would scan only the tail on each iteration:
let scan_start = buf.len().saturating_sub(chunk[..n].len() + 3);
if buf[scan_start..].windows(4).any(|w| w == b"\r\n\r\n") {This keeps detection O(chunk_size) amortised. Fine to leave as-is given the bound, but worth a one-liner fix if this function is ever extracted for larger head limits.
|
Issue created: #549
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #549 Resolved the merge conflict with Collisions — the three files both sides touched, resolved as the union of both:
Main-only files ( Pre-PR CI gate (native Linux, The three re-scoped proof artifacts still pass:
|
Implements the native-Linux (DM2) slice of Unit 3 — DNS hostname management
for #478, on the re-scope agreed on the tracking issue (#500). The three
maintainer decisions are applied:
*.localhost+ host-side proxy is kept andproof artifact 1 asserts the registry/proxy contract;
session.rsregistration is scoped to
HostNetonly (OwnIp→ #542); the TCP/hostnameclient (R3.2/UC2c) stays out of scope (→ Unit 4).
What landed
crates/minimald/src/net/dns.rs(new) — an in-memoryHostnameRegistrykeyed by session name.
register/deregisteremit structuredtracingevents (R3.5);
register_host_netroutes aHostNetPTask's<session>.<host-id>.localhosthostname to127.0.0.1(R3.6).resolveis the registry/proxy routing contract: it maps a
Host:header (case- andport-insensitive) to the address a host-side proxy forwards to. Because every
*.localhostname is synthesized to loopback statically by the resolver, theper-PTask routing decision lives here, not in DNS.
probe_resolverchecks the systemresolver synthesizes
*.localhostand emitstracing::warn!(resolver = "systemd-resolved", status = "inactive", remedy = ...)when it does not. Wired into the DM2 startup path in
main.rs(off the asyncworker, since
getaddrinfois blocking).HostNetPTask's hostname on launch and withdraws it on teardown.
.min.local→.localhost, andOpen Question 1 closed with the spike spike: DNS hostname registration works rootlessly on target Linux distributions #485
*.localhost+ host-side-proxydecision.
Proof artifacts
1. Test — registry/proxy contract (
*.localhost+ host-proxy model, spike #485):register a
HostNetPTask"myservice"/host-id"dev", assert the registryholds
myservice.dev.localhostand routes aHost:header to127.0.0.1;after
deregister, assert it no longer routes. Fails on base (no registry).2. Test — R3.4 resolver probe (NXDOMAIN ⇒ warn): with an injected resolver
returning NXDOMAIN, the probe reports the resolver inactive and emits a
tracing::warn!carryingresolver="systemd-resolved"andstatus="inactive".Fails on base (no startup probe).
Supporting tests:
Gate (host: native Linux, in
crates/minimald):Notes for the reviewer
(
sessions.rs) rather than the per-session actor (session.rs) the task bodynamed: the manager is an actor with exclusive
&mut selfaccess and is thesession-lifecycle owner, so the registry needs no
Arc/lock. Same behaviour,fewer moving parts.
host-id. Stored once on the registry (default"local",DEFAULT_HOST_ID) rather than passed perregistercall, matching "a stableshort name for the
minimaldinstance"; making it configurable is a smallfollow-up.
session_name(the registry key) where R3.5lists
session_id; the registry is keyed by name per the architecture doc.OwnIpswitch-IP registration (feat(minimald,sandbox2): wire minimald::net switch into the live OwnIp session-launch path (R1.5) #542) and theTCP/hostname client (Unit 4) are not in this PR.
registeralready takes thetarget address, so feat(minimald,sandbox2): wire minimald::net switch into the live OwnIp session-launch path (R1.5) #542 is a caller change.
describes the
OwnIpgetaddrinfolifecycle; that is the deferred feat(minimald,sandbox2): wire minimald::net switch into the live OwnIp session-launch path (R1.5) #542 pathand was left untouched here.
Merging this PR closes #500. Once every task sub-issue of #478 is closed, the
pipeline advances the tracking issue to
sdd:donefor a final human review.Closes #500
Summary by CodeRabbit
<session-name>.<host-id>.localhostfor both local and HostNet access.Host:header (including CONNECT authority) using an in-memory hostname registry with automatic HostNet session register/deregister..min.localto.localhost, reflecting the proxy+registry approach as the resolved design.