Repository navigation
Conversation
github-actions
Bot
added
family:setup
|
Following up on #1770, this holds command outputs in variables rather than redirecting to Added comprehensive unit test suites for both probes verifying no files are written under Ready for review, @potiuk! |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Holding the probe output in memory instead of a file under /tmp is the right fix for the read-only sandbox, and probe 5's change is correct; probe 3's capture needs two adjustments, and its new tests do not reach the line that changed.
Capturing the probe through a pipe makes it wait out the watchdog, and mixes stdout into the error (plugins/magpie-setup/skills/isolated-setup-doctor/scripts/probe-3-container-gateway.sh:104)
out=$(_probe_timeout 15 "$rt" info 2>&1); rc=$?
Two regressions from moving the output into $(…). When neither timeout nor gtimeout is on PATH (stock macOS), _probe_timeout starts (sleep 15 && kill …) &; that sleep inherits the capture pipe, and killing the subshell does not kill it, so $(…) waits the full 15 s even for a healthy runtime (30 s for both). And 2>&1 now captures stdout too: docker info prints its Client: block before failing on the server, so err becomes that first stdout line and the operation not permitted / 502 matches below never fire.
Flagged by both the primary and adversarial reviewers.
Smaller observations
- See inline comments on
plugins/magpie-setup/skills/isolated-setup-doctor/tests/test_probe_3_container_gateway.py:142,plugins/magpie-setup/skills/isolated-setup-doctor/tests/test_probe_5_signing_key.py:39.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| fi | ||
| if _probe_timeout 15 "$rt" info > /dev/null 2>"${TMPDIR:-/tmp}/$rt-probe.err"; then | ||
| # Held in a variable, not a file: /tmp is read-only inside the sandbox. | ||
| out=$(_probe_timeout 15 "$rt" info 2>&1); rc=$? |
There was a problem hiding this comment.
Two regressions here. (1) With no timeout/gtimeout on PATH (stock macOS), the watchdog's sleep in _probe_timeout inherits this capture pipe and outlives the killed subshell, so $(…) waits the full 15 s even when info returns at once: I reproduced it with a 3 s watchdog ($(f /bin/sleep 0.1 2>&1) takes 3.0 s). Redirecting the watchdog fixes it: (sleep "$secs" && kill "$cmd_pid") >/dev/null 2>&1 &. (2) 2>&1 also captures stdout, and docker info prints its Client: block before failing, so err becomes Client: … and the operation not permitted / 502 cases below never match. Capture stderr only: out=$(_probe_timeout 15 "$rt" info 2>&1 >/dev/null).
| out, | ||
| ) | ||
|
|
||
| def test_writes_nothing_under_tmp(self) -> None: |
There was a problem hiding this comment.
This exits at the gateway check (/var/run/podman.sock is not a running gateway here), before $rt info runs, so it passes on the unmodified script too, and no probe-3 test reaches the line this PR changes. To get there: run the probe with cwd set to a temp dir holding a stub tools/container-gateway/src/container_gateway/__main__.py that prints {"running": true, "serving": ["podman"]} (the probe reads status from that relative path), bind a real unix socket under self.tmp for CONTAINER_HOST, and make the fake podman fail with a stderr line. Then assert that line is reported and nothing lands in TMPDIR. A fake docker that prints Client: on stdout before failing would cover the stream mix-up above.
|
|
||
|
|
||
| def _find_bash() -> str | None: | ||
| if os.name == "nt": |
There was a problem hiding this comment.
_find_bash, _to_posix and the run_probe body are identical in test_probe_3_container_gateway.py; a shared helper module (or conftest.py) next to the tests would keep the Windows handling in one place.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Marking this as changes requested, so the PR's status matches where it stands: the points in my earlier review (#1776 (review)) are still open and need a change, or a reply on their threads. This review adds nothing new to them.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
Summary
${TMPDIR:-/tmp}/...inprobe-3-container-gateway.shandprobe-5-signing-key.sh, matching the sandbox contract documented inisolated-setup-doctor/scripts/README.md.socket.AF_UNIXand add UTF-8 encoding intest_probe_1_ssh_agent.pyto preventAttributeErrorcrashes on platforms without Unix domain socket support.test_probe_3_container_gateway.pyandtest_probe_5_signing_key.py.Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)plugins/magpie-setup/skills/isolated-setup-doctor/)Test plan
prek run/ruff checkandruff format --checkpass on touched files with zero warnings.python -m unittest discover -s plugins/magpie-setup/skills/isolated-setup-doctor/tests/(14 tests run: 14 passed / 3 skipped where AF_UNIX is unsupported).pytest plugins/magpie-setup/skills/isolated-setup-doctor/tests/(11 passed, 3 skipped, 0 failures)./tmpor$TMPDIR.RFC-AI-0004 compliance
<PROJECT>,<tracker>,<upstream>,<security-list>) used in all skill / tool prose (thecheck-placeholdersprek hook is the mechanical gate)Linked issues
Closes #1775
Refs #1770
Notes for reviewers (optional)
Following up on #1770 (which eliminated
/tmpwrites for Probe 1), Probes 3 and 5 still redirected stderr/stdout to files under${TMPDIR:-/tmp}, causing them to abort and report false failures inside read-only sandboxes. This PR brings them into full alignment with the README contract.