fix(docker): honor docker_network in backend probe (#76906) - #76911
fix(docker): honor docker_network in backend probe (#76906)#76911webtecnica wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. The premise is confirmed on current main: agent/prompt_builder.py:1074-1089 omits docker_network, while tools/terminal_tool.py:1602-1625 defaults a missing value to True and passes it to Docker. The added propagation matches the existing terminal path at tools/terminal_tool.py:2418.
Problems
- Please add a behavior-level regression test.
tests/agent/test_prompt_builder.py:729-767verifies that the probe reaches the factory, but does not capturecontainer_configor coverdocker_network=False; this leaves the security-relevant config propagation unguarded.
Suggested changes
- Capture the factory kwargs in the existing probe test (or a focused sibling test), configure
docker_network=False, and assert the resultingcontainer_config["docker_network"] is False.
This is an automated hermes-sweeper review.
| "docker_forward_env": config.get("docker_forward_env", []), | ||
| "docker_env": config.get("docker_env", {}), | ||
| "docker_run_as_host_user": config.get("docker_run_as_host_user", False), | ||
| "docker_network": config.get("docker_network", True), |
There was a problem hiding this comment.
Please add a behavior-level regression test that configures docker_network=False, captures _create_environment(..., container_config=...), and asserts this value reaches the factory. The existing probe test only verifies factory selection and output parsing, not this security-relevant propagation.
SummaryFour PRs address or underpin #76906: #46358 established the Docker network-control contract, while #62023, #76911, and #76926 each add the missing backend-probe propagation. Of the direct fixes, #76926 also provides the requested behavior-level regression test for docker_network=False. Related pull requests
Duplicates#62023, #76911, and #76926 implement the same backend-probe propagation fix; #62023 adds a structural source guard, #76911 has no test, and #76926 adds the requested behavior-level false-value propagation test. Suggested consolidationKeep #76926 open with a salvage path: preserve its focused propagation fix and behavior-level regression test. Close #76911 as a duplicate of #76926, and close #62023 as a duplicate of #76926 despite its keep_open verdict because #76926 verifies the security-relevant value through the actual probe factory boundary; leave #46358 closed as the merged-through-#59149 reference for the underlying toggle. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I76906(["issue #76906 (open)"])
subgraph Dup62023 ["PRs duplicating each other"]
P62023["PR #62023 (open)"]
P76911["PR #76911 (open)"]
P76926["PR #76926 (open)"]
end
P76911 -.->|partial| I76906
class I76906 open
class P62023 open
class P76911 open
class P76926 open
class P76926 best
class P76911 target
click I76906 "https://github.com/NousResearch/hermes-agent/issues/76906"
click P62023 "https://github.com/NousResearch/hermes-agent/pull/62023"
click P76911 "https://github.com/NousResearch/hermes-agent/pull/76911"
click P76926 "https://github.com/NousResearch/hermes-agent/pull/76926"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 4 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 14 kB of PR diffs, 13 kB of issue/PR text, 5 kB of discussion (5 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Closes #76906