fix: apple host.docker.internal DNS hint (#118) - #164
Conversation
BoldBlackBot
left a comment
BoldBlackBot
left a comment
There was a problem hiding this comment.
Code Review
Verdict: Comment (1 warning, 3 suggestions)
Reviewed the rebased diff. CI is all green (build, lint, e2e). The two core changes are sound — the contextDir fix is correct for Hermes's cwd-only context loading, and the Apple DNS check is well-structured with proper fail-open behavior.
One behavioral concern worth discussing re: AGENTS.md shadowing at /workspace. Details in the inline comment.
✅ Looks Good
appleHostDockerInternalDnsConfigured()fail-opens (returnstrueon error) — never blocks a runhasScript()upgrade correctly probes the actual flag combo (-qfec) instead of just PATH presence — catches the BSD vs util-linuxscriptgap- Test for absent
containerfilters PATH entries that contain the real binary — fixes false passes on Macs with apple/container installed - DNS warning test is properly conditional (skips silently when preconditions aren't met)
Reviewed by otacon (Hermes Agent)
| return "/home/harness/.hermes"; | ||
| // Hermes reads AGENTS.md / CLAUDE.md from the project cwd only, not | ||
| // ~/.hermes/. Mount global context files into /workspace so they apply. | ||
| return "/workspace"; |
There was a problem hiding this comment.
Changing contextDir() to /workspace means the global ~/.agents/AGENTS.md is bind-mounted at /workspace/AGENTS.md (line 837: ${cf.host}:${contextDir}/${cf.name}).
Since $PWD is already mounted at /workspace, this mount shadows any project-level AGENTS.md in the repo root. If a user has both ~/.agents/AGENTS.md (global) and ./AGENTS.md (project), only the global one is visible inside the container — the project-level instructions are lost.
This differs from the pi and opencode adapters, whose contextDir (/home/harness/.pi/agent, /home/harness/.config/opencode) is separate from /workspace, so both global and project context files coexist.
Worth documenting in the comment or README, or handling with a merge/concat strategy. If shadowing is intentional (global overrides project), a one-line note would clarify the design choice.
| ); | ||
| process.exit(1); | ||
| } | ||
| if (!appleHostDockerInternalDnsConfigured()) { |
There was a problem hiding this comment.
💡 Suggestion: The DNS warning prints on every run when the mapping is missing. Consider suppressing after the first occurrence, or adding a note like (this message prints once per run), to reduce noise in iterative workflows. Not blocking — the message is clear and actionable.
| } | ||
| if (!appleHostDockerInternalDnsConfigured()) { | ||
| console.error( | ||
| `harness: ${HOST_DOCKER_INTERNAL} is not configured for Apple's container runtime.\nLocal services on the Mac (e.g. LM Studio on :1234) will not be reachable from the container.\nOne-time fix (requires administrator):\n sudo container system dns create ${HOST_DOCKER_INTERNAL} --localhost ${APPLE_HOST_LOCALHOST_IP}\nSee https://github.com/apple/container/blob/main/docs/how-to.md#access-a-host-service-from-a-container`, |
There was a problem hiding this comment.
💡 Suggestion (nit): This console.error call is a single ~300-char line. Breaking it into a multi-line template literal (like the ensureReady error above at line 241) would improve readability. Not blocking.
| }); | ||
|
|
||
| test("HARNESS_CONTAINER_RUNTIME=apple warns when host.docker.internal DNS is missing", () => { | ||
| const which = spawnSync("sh", ["-c", "command -v container"], { |
There was a problem hiding this comment.
💡 Suggestion: Consider adding a brief comment like // Skip on Linux CI / when DNS already configured above the skip logic so the reason is obvious to future readers scanning the test file.
9e7e3d5 to
5b6eb73
Compare
BoldBlackBot
changed the title
5b24e4e to
a26947a
Compare
a26947a to
54b0907
Compare
On HARNESS_CONTAINER_RUNTIME=apple, harness checks 'container system dns list' and prints a one-time setup command when host.docker.internal is missing (LM Studio / local gateway reachability). Documents the Apple-recommended 'container system dns create --localhost 203.0.113.113' flow in README. E2e: skip PTY tests when macOS 'script' lacks util-linux -qfec; fix 'container absent from PATH' test when real container is on PATH; conditional test for the DNS warning. Co-Authored-By: Julio Capote <6135+capotej@users.noreply.github.com> Co-Authored-By: Hugo Gonzalez <601542+hggz@users.noreply.github.com>
54b0907 to
ae287bb
Compare
2 participants
Summary
Rebased replacement for #126 (originally by @hggz). Apple DNS hint changes only; the Hermes contextDir change from #126 was reverted and will be addressed separately to avoid shadowing project-level AGENTS.md.
HARNESS_CONTAINER_RUNTIME=apple, harness checkscontainer system dns listand prints a one-time setup command whenhost.docker.internalis missing (LM Studio / local gateway reachability). Documents the Apple-recommendedcontainer system dns create … --localhost 203.0.113.113flow in README.scriptlacks util-linux-qfec; fix "container absent from PATH" test when realcontaineris on PATH; conditional test for the DNS warning.Test plan
pnpm buildpnpm lint(biome, markdownlint)sudo container system dns create host.docker.internal --localhost 203.0.113.113Closes #118.
Supersedes #126.
cc @capotej @hggz