Skip to content

fix(parallels): send guest prep scripts over stdin - #2431

Merged
steipete merged 2 commits into
mainfrom
codex/triage-20260920-pr2401-parallels-stdin
Sep 21, 2026
Merged

steipete merged 2 commits into
mainfrom
codex/triage-20260920-pr2401-parallels-stdin

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Parallels guest preparation and SSH-key installation now stream their POSIX scripts to /bin/sh -s over stdin. prlctl exec flattens argv, so the previous script argument could silently lose set -eu. Both local and SSH-to-host routes preserve stdin, host identity, PATH, and environment filtering; child commands cannot consume the remaining script input.

Fixes #2396.

This supersedes #2401 only because of its merge conflict after #2415 landed. The rebase preserves both the landed caller-context IP-discovery diagnostics and the original stdin fix. The Go changes applied unchanged; the changelog conflict was resolved by retaining both sides. The contributor's branch was not force-pushed. The coordinator will close the original PR as superseded when this PR merges.

Credit to @saariuslystoned for the implementation and regression tests. The implementation commit preserves Co-authored-by: Bobby Bones <saariuscrypto@gmail.com>, and the maintainer changelog thanks the contributor. No configuration or credential changes are required.

Validation on rebased head f51768e47b60f8315ca50e6cabfecc3e1837416c:

  • gofmt -l $(git ls-files '*.go'): clean.
  • GOTOOLCHAIN=go1.26.5 GOFLAGS='-mod=readonly -trimpath -p=2' GOMAXPROCS=4 go vet ./...: passed.
  • GOTOOLCHAIN=go1.26.5 GOFLAGS='-mod=readonly -trimpath -p=2' GOMAXPROCS=4 go test -race -count=1 -timeout=15m ./internal/cli/ -run 'Parallels': passed, 7.617 seconds; includes the caller-context diagnostics and stdin regression coverage.
  • Final Codex branch autoreview: clean, no actionable P0–P2 findings.

Prior full gate on pre-rebase head aea9efeb512674273fc3dbc13c7d28ab73fc689e, supplied by the coordinator: Crabbox AWS c7a.48xlarge, Go 1.26.5; gofmt OK, go vet OK, build OK, full go test -race -count=1 ./... with GO-TEST-RC=0, command-docs OK, docs-links OK. The origin/main baseline was green on the same setup. The coordinator will re-run the full gate on the rebased head before landing. This lane did not independently run a live Parallels guest proof.

@clawsweeper

clawsweeper Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 21, 2026
@clawsweeper

clawsweeper Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 21, 2026, 5:39 AM ET / 09:39 UTC (Revision 2).

ClawSweeper review

What this changes

The PR sends Parallels guest preparation and SSH-key installation scripts through stdin, isolates child-command input, and adds regression tests and documentation.

Merge readiness

Blocked before merge - 3 items remain

The repair remains necessary and has convincing native macOS evidence. No introduced correctness defect was found; the previously identified Linux compatibility coverage decision remains unresolved.

Priority: P2
Reviewed head: 74d001588d1a4f7507c9423b84ee4a96e05b3dca
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with strong native macOS proof and no actionable code finding; Linux compatibility remains the material coverage limit.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The original contributor's real local and remote macOS Crabbox runs exercise key installation, guest preparation, and stdin transport, showing successful leases and correct failure propagation under injection. Those production paths remain unchanged; Linux compatibility is a separate disclosed coverage decision.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The original contributor's real local and remote macOS Crabbox runs exercise key installation, guest preparation, and stdin transport, showing successful leases and correct failure propagation under injection. Those production paths remain unchanged; Linux compatibility is a separate disclosed coverage decision.
Evidence reviewed 9 items Repository policy: Read the complete root AGENTS.md and applied its provider boundaries, regression-testing guidance, and maintainer integration changelog policy. No applicable nested AGENTS.md or maintainer-notes directory was found.
Introduced transport repair: The pinned introduced diff changes both POSIX preparation entrypoints to /bin/sh -s, forwards stdin through both host routes, and protects potentially input-consuming child commands. Host selection, identity arguments, environment filtering, and Windows execution remain intact.
Current main still requires the repair: Both preparation entrypoints on fetched main still send generated scripts as argv following /bin/sh -lc. The merged IP-discovery diagnostics in #2415 address a separate problem.
Findings None None.
Security None None.

How this fits together

Crabbox prepares cloned Parallels VMs before running workloads over SSH. Generated preparation scripts travel through local Parallels tooling or SSH to a remote host, and their results determine whether provisioning proceeds or cleans up the clone.

flowchart TD
 A[Clone and lease settings] --> B[Generate preparation scripts]
 B --> C{Host location}
 C --> D[Local Parallels command]
 C --> E[SSH to remote host]
 D --> F[Guest shell reads stdin]
 E --> F
 F --> G{Preparation succeeds}
 G --> H[SSH readiness or clone cleanup]
Loading

Decision needed

Question Recommendation
Is the native macOS evidence plus Linux source and syntax coverage sufficient to accept the untested Linux preparation and upgrade behavior? Establish Linux compatibility: Require fresh and previously prepared Linux template evidence before landing.

Why: The implementation restores intended failure checks, but accepting the remaining platform compatibility uncertainty requires maintainer judgment.

Before merge

  • Resolve merge risk (P1) - Fresh and previously prepared Linux templates lack runtime compatibility evidence: effective set -eu can now abort provisioning on package or desktop setup errors that previously went unnoticed. The documented intent does not explicitly accept this coverage limit.
  • Complete next step (P2) - Resolve the Linux compatibility coverage decision before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +54/-36 (net +18); tests +161/-8 (net +153) Production growth supports stdin transport and input isolation, with regression coverage for both scripts and host routes.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #2396
Summary: This integration PR carries the candidate repair for the argv-flattening issue; the original implementation PR remains open pending integration.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Verify Linux template compatibility (recommended)
    Provide Linux preparation and readiness results for fresh and previously prepared templates, including required-step failure propagation.
  2. Accept the disclosed platform limit
    Record explicit maintainer acceptance that Linux runtime compatibility remains unverified.

Technical review

Best possible solution:

Retain shared stdin delivery and explicit best-effort exceptions, with Linux fresh-template and upgrade compatibility demonstrated or its bounded coverage limit explicitly accepted.

Do we have a high-confidence way to reproduce the issue?

Yes: main retains the faulty argv transport, and the supplied native control demonstrates swallowed shell failures. This review inspected the path without independently executing it.

Is this the best way to solve the issue?

Yes: stdin delivery follows the existing SSH bootstrap pattern and avoids guest-side argv reconstruction; child-input isolation addresses the resulting consumption hazard.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 05415421726c.

Labels

Label justifications:

  • P2: This repairs a bounded Parallels provisioning defect that silently bypasses intended shell failure checks.
  • merge-risk: 🚨 compatibility: Restored fail-fast behavior can reject previously tolerated Linux preparation failures, and runtime upgrade coverage remains unresolved.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The original contributor's real local and remote macOS Crabbox runs exercise key installation, guest preparation, and stdin transport, showing successful leases and correct failure propagation under injection. Those production paths remain unchanged; Linux compatibility is a separate disclosed coverage decision.
  • proof: sufficient: Contributor real behavior proof is sufficient. The original contributor's real local and remote macOS Crabbox runs exercise key installation, guest preparation, and stdin transport, showing successful leases and correct failure propagation under injection. Those production paths remain unchanged; Linux compatibility is a separate disclosed coverage decision.

Evidence

What I checked:

  • Repository policy: Read the complete root AGENTS.md and applied its provider boundaries, regression-testing guidance, and maintainer integration changelog policy. No applicable nested AGENTS.md or maintainer-notes directory was found. (AGENTS.md:1, 74d001588d1a)
  • Introduced transport repair: The pinned introduced diff changes both POSIX preparation entrypoints to /bin/sh -s, forwards stdin through both host routes, and protects potentially input-consuming child commands. Host selection, identity arguments, environment filtering, and Windows execution remain intact. (internal/cli/parallels.go:433, 74d001588d1a)
  • Current main still requires the repair: Both preparation entrypoints on fetched main still send generated scripts as argv following /bin/sh -lc. The merged IP-discovery diagnostics in fix(parallels): explain IP discovery timeouts by clone mode #2415 address a separate problem. (internal/cli/parallels.go:432, 05415421726c)
  • Latest release retains argv delivery: GitHub identifies v0.63.0 as the latest release. Its preparation entrypoints also use /bin/sh -lc with the script in argv; this repair is not present there. (internal/cli/parallels.go:336, 8a8b22d6fd4f)
  • Native production-path evidence: The supplied snapshot, sourceRevision 872bf15cf23f352c6f6e34fec107ff4d036a1c82b0503583b386ff06c359908c, includes fix(parallels): send guest prep scripts over stdin #2401 with local and remote Apple-silicon macOS guest traces. Real Crabbox runs reach GUEST_OK, while an injected work-root collision stops during preparation on the fixed branch and incorrectly passes preparation on the main control. Host-side argv sampling observes /bin/sh -s. These are contributor runs, not executions by this reviewer.
  • Proof and review continuity: Read historical source through GitHub contents APIs and compared it in memory with the checkout. Relative to the prior reviewed head f51768e and the native-evidence implementation 47459e3, preparation scripts and stdin transport are unchanged; differences in this source file concern upstream IP-discovery diagnostics. Local historical diffs and blame encountered unavailable objects, so no introducing-author claim is made. (internal/cli/parallels.go:543, 74d001588d1a)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • saariuslystoned: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Establish fresh and previously prepared Linux template compatibility, or record explicit acceptance of that coverage limit.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-21T08:19:56.491Z sha f51768e :: blocked before merge. :: none

steipete and others added 2 commits September 21, 2026 02:32
Integrate the contributor fix from #2401 for #2396. Preserve the original production code and regression tests unchanged.

Co-authored-by: Bobby Bones <saariuscrypto@gmail.com>
Document the behavior restored by #2401 and credit its contributor in the Unreleased changelog.
@steipete
steipete force-pushed the codex/triage-20260920-pr2401-parallels-stdin branch from f51768e to 74d0015 Compare September 21, 2026 09:35
@steipete
steipete merged commit d858157 into main Sep 21, 2026
24 of 27 checks passed
@steipete
steipete deleted the codex/triage-20260920-pr2401-parallels-stdin branch September 21, 2026 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parallels: prlctl exec flattens argv, silently dropping set -eu from guest prep scripts

1 participant