Skip to content

fix(setup): hold doctor probe 3 and 5 outputs in memory and add tests - #1776

Merged
potiuk merged 3 commits into
apache:mainfrom
Kaap10:fix/doctor-probes-no-tmp-write
Oct 24, 2026
Merged

potiuk merged 3 commits into
apache:mainfrom
Kaap10:fix/doctor-probes-no-tmp-write

Conversation

@Kaap10

@Kaap10 Kaap10 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Hold command outputs in variables rather than redirecting to ${TMPDIR:-/tmp}/... in probe-3-container-gateway.sh and probe-5-signing-key.sh, matching the sandbox contract documented in isolated-setup-doctor/scripts/README.md.
  • Guard socket.AF_UNIX and add UTF-8 encoding in test_probe_1_ssh_agent.py to prevent AttributeError crashes on platforms without Unix domain socket support.
  • Add comprehensive unit test suites test_probe_3_container_gateway.py and test_probe_5_signing_key.py.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other: Doctor diagnostic probe scripts and tests (plugins/magpie-setup/skills/isolated-setup-doctor/)

Test plan

  • prek run / ruff check and ruff format --check pass on touched files with zero warnings.
  • Ran unit test discovery via 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).
  • Ran pytest plugins/magpie-setup/skills/isolated-setup-doctor/tests/ (11 passed, 3 skipped, 0 failures).
  • Verified unit tests assert that no error files are created under /tmp or $TMPDIR.

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — no new unrestricted host access; network reach declared in the adapter
  • Vendor neutrality — placeholders (<PROJECT>, <tracker>, <upstream>, <security-list>) used in all skill / tool prose (the check-placeholders prek hook is the mechanical gate)
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
  • Privacy LLM — private content does not reach a non-approved LLM; redactor invoked where needed

Linked issues

Closes #1775
Refs #1770

Notes for reviewers (optional)

Following up on #1770 (which eliminated /tmp writes 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.

@github-actions github-actions Bot added family:setup setup-* skills capability:reassess Re-run historical issues against current code capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) labels Oct 24, 2026
@Kaap10

Kaap10 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Following up on #1770, this holds command outputs in variables rather than redirecting to /tmp in probe-3 and probe-5, preventing false failures inside read-only sandboxes.

Added comprehensive unit test suites for both probes verifying no files are written under /tmp or $TMPDIR, along with an AF_UNIX guard for test_probe_1.

Ready for review, @potiuk!

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Kaap10
Kaap10 force-pushed the fix/doctor-probes-no-tmp-write branch from 886c363 to 37c4307 Compare October 24, 2026 15:19
…t reachability

- Redirect watchdog background sleep to /dev/null to prevent pipe inheritance
  and 15s hang under command substitution on stock macOS.
- Redirect stdout to /dev/null when capturing probe output so docker info
  stdout does not mask stderr error messages.
- Extract shared test runner and POSIX path helpers into helpers.py for
  probe-3 and probe-5 test suites.
- Stub container gateway and bind unix domain socket in test_probe_3 to
  exercise info probe command, asserting stream separation and zero temp files.
@Kaap10
Kaap10 force-pushed the fix/doctor-probes-no-tmp-write branch from 37c4307 to dff5fd5 Compare October 24, 2026 15:23
@Kaap10

Kaap10 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @potiuk for the review and catching both the macOS pipe inheritance and the Docker stdout stream mix-up.

Addressed all findings in dff5fd54:

  1. Watchdog pipe leak (probe-3:55):

    • Redirected the watchdog subshell to /dev/null: (sleep "$secs" && kill "$cmd_pid" 2> /dev/null) > /dev/null 2>&1 &.
    • The background watchdog no longer inherits the capture pipe and does not delay command substitution when timeout/gtimeout is missing on macOS.
  2. Stream separation (probe-3:104):

    • Updated the info capture to out=$(_probe_timeout 15 "$rt" info 2>&1 > /dev/null); rc=$?.
    • Stdout (such as Docker's Client: preamble) is discarded to /dev/null, capturing only stderr for matching.
  3. Shared test helpers:

    • Extracted find_bash, to_posix, and run_probe_script into a shared helpers.py module used by both test_probe_3_container_gateway.py and test_probe_5_signing_key.py.
  4. Probe 3 test reachability & stream assertions:

    • Refactored test_writes_nothing_under_tmp to run with a stub container gateway and real unix domain socket under self.tmp with cwd=self.tmp. This exercises line 104 ($rt info), asserts the permission denial message, and verifies zero .err files are written under /tmp.
    • Added test_docker_stdout_does_not_mask_stderr_error with a fake docker printing Client: to stdout and 502 Bad Gateway to stderr, asserting the 502 error is reported and Client: is dropped.

…al base

Review fixup. The shared runner copied the host environment, so the host's
CONTAINER_HOST / DOCKER_HOST, or a gateway running on the machine, could
reach probe 3 and make the tests host-dependent. It now starts from PATH,
HOME, locale and temp variables (plus the Windows essentials) and adds the
test's own values.

Generated-by: Claude (Opus 5.5)
@potiuk
potiuk enabled auto-merge (squash) October 24, 2026 23:49
@potiuk
potiuk merged commit 66c11c8 into apache:main Oct 24, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) capability:reassess Re-run historical issues against current code family:setup setup-* skills

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Doctor probe 3 and probe 5 redirect to /tmp, falsely failing in read-only sandbox

2 participants