Skip to content

fix(parallels): preserve final IP discovery query errors - #2480

Merged
steipete merged 1 commit into
mainfrom
codex/parallels-ip-query-evidence-n846
Sep 25, 2026
Merged

steipete merged 1 commit into
mainfrom
codex/parallels-ip-query-evidence-n846

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Problem and fix

When Parallels IP discovery times out after its final VM inventory query fails, the query's actual error is discarded. If an earlier query succeeded, the timeout can instead show stale DHCP and linked-clone boot advice—even though current inventory is unavailable.

Preserve the final query error in the timeout. Explicitly identify state/MAC values as the last successful observation, if any, and prioritize inventory access rather than stale guest-boot, DHCP, or clone-mode advice. A later successful query still clears the earlier failure and retains the existing normal discovery hints.

The six production lines change only diagnostics. Timeout exit code 5, polling cadence and budget, cancellation ordering, successful IP results, ambiguous DHCP handling, clone defaults, native commands, and VM lifecycle remain unchanged. Documentation and the Unreleased changelog are updated.

Proof

The new TestParallelsWaitForIPRetainsFinalQueryFailure extends the existing test harness and uses virtual time for command failure, malformed JSON, missing VM, successful query followed by failure, and failure followed by a successful query. Against the original code it failed for the missing query errors and stale DHCP/clone advice. It passes with this patch.

go test -mod=readonly -race -count=1 -timeout=20m ./internal/cli -run '^TestParallelsWaitForIPRetainsFinalQueryFailure$'
go test -mod=readonly -race -count=1 -timeout=20m ./internal/cli ./internal/providers/parallels -run 'Parallels|parallels'
go vet -mod=readonly ./internal/cli ./internal/providers/parallels

The broader Parallels race checks passed in 114.61s and vet in 14.14s. Independent Codex review of the complete introduced scope through P2 found no accepted/actionable findings. Regression runs used isolated credential-free environments with external networking denied.

A read-only direct native preflight with Parallels 27.0.2 (58673) confirmed the real prlctl list -i -f -j <random-missing-uuid> error. However, the candidate harness could not reach its query: the restricted test sandbox denied process inspection used by Parallels' launcher, and native initialization failed. That attempt is not candidate native proof. The restriction was not weakened, no alternate entrypoint was used, and no VM was started, cloned, or changed. The proof of this diagnostic-only fix is the failing-before/passing-after regression and unchanged command/lifecycle path, not a claimed live lifecycle test.

Issue boundary

Related: #2398. This does not resolve the underlying linked-clone boot failure or establish an Apple-silicon compatibility boundary, and must not close that issue as fixed. Current main already includes linked-clone troubleshooting; this patch repairs the separate loss of current query evidence discovered while rechecking that issue.

@clawsweeper

clawsweeper Bot commented Sep 22, 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 22, 2026
@clawsweeper

clawsweeper Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed September 25, 2026, 5:35 AM ET / 09:35 UTC (Revision 4).

ClawSweeper review

What this changes

The branch makes Parallels IP-wait errors report a failed final VM inventory query, identify older state and MAC observations, and suppress stale troubleshooting advice, with regression tests and documentation.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Current main and v0.66.0 still discard the final inventory-query error, so this PR remains useful. The patch has no identified correctness defect, but the maintainer’s explicit request for after-fix output through Crabbox’s Parallels path remains unmet.

Priority: P2
Reviewed head: 59a9adf9ab260da0109dfe181bb955d5717fc7a9

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A focused diagnostic repair with relevant regression coverage and no identified patch defect; the explicit real-path evidence hold remains a merge step.
Proof confidence 🌊 off-meta tidepool Not applicable: The host identifies this as maintainer-authored work, so the automatic contributor proof gate does not apply. Synthetic tests cover the changed WaitForIP formatter, but the supplied native preflight did not enter Crabbox’s IP wait; the maintainer’s separate real-path proof hold remains. No stored-data contract changes.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The host identifies this as maintainer-authored work, so the automatic contributor proof gate does not apply. Synthetic tests cover the changed WaitForIP formatter, but the supplied native preflight did not enter Crabbox’s IP wait; the maintainer’s separate real-path proof hold remains. No stored-data contract changes.
Evidence reviewed 9 items Introduced diagnostic repair: The pinned main-to-head patch stores the latest GetVM error, reports it at timeout, and directs failed-query diagnostics toward inventory access.
Current-main gap: The reviewed main revision tracks whether the latest VM query succeeded but drops its error, allowing older DHCP or clone guidance to survive a failed final query.
Production entrypoint: Parallels acquisition calls the IP wait after starting a VM and returns its error to the operator; fixed-lease preparation also calls this wait.
Findings None None.
Security None None.

How this fits together

Crabbox’s Parallels provider polls VM inventory and optionally checks host DHCP records while waiting for a guest address. It either passes that address to guest preparation or returns an operator-facing diagnostic.

flowchart TD
 A[VM acquisition or lookup] --> B[Poll Parallels inventory]
 B --> C{Guest address found?}
 C -->|Yes| D[Prepare guest connection]
 C -->|No| E[Check optional DHCP fallback]
 E --> F{Wait expired?}
 F -->|No| B
 F -->|Yes| G[Report query failure or discovery hints]
Loading

Before merge

  • Complete next step (P2) - Provide redacted after-fix terminal output or logs from Crabbox’s Parallels IP-discovery path showing the final inventory error and corrected hints, then update the PR body for re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +8/-0; tests +56/-4 Production growth is confined to timeout diagnostics, with five focused synthetic cases.

Technical review

Best possible solution:

Keep timeout guidance tied to the latest inventory result while preserving the existing IP discovery, cancellation, and VM cleanup behavior.

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

Yes, from source: a failed final inventory query reaches the timeout formatter without its error on current main. The synthetic regression cases exercise this path; this review did not establish a native reproduction.

Is this the best way to solve the issue?

Yes. The branch repairs the existing timeout owner without adding a competing discovery path or changing clone defaults.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against dc28186e4ee5.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: Misleading Parallels timeout diagnostics impede troubleshooting for one provider without changing VM discovery or lifecycle behavior.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The host identifies this as maintainer-authored work, so the automatic contributor proof gate does not apply. Synthetic tests cover the changed WaitForIP formatter, but the supplied native preflight did not enter Crabbox’s IP wait; the maintainer’s separate real-path proof hold remains. No stored-data contract changes.

Evidence

What I checked:

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)

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 (3 earlier review cycles)
  • reviewed 2026-09-22T06:21:34.513Z sha 634a725 :: needs real behavior proof before merge. :: [P3] Leave the changelog entry to maintainer integration
  • reviewed 2026-09-22T06:49:47.056Z sha 634a725 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-25T05:13:46.075Z sha 9f7c216 :: needs changes before merge. :: none

@steipete
steipete marked this pull request as draft September 22, 2026 06:43
@steipete

Copy link
Copy Markdown
Contributor Author

Maintainer disposition: the missing after-fix real-path proof remains a merge hold. The PR already distinguishes its passing synthetic regressions from the unsuccessful candidate native attempt, and no native proof is claimed. Returning this to draft until that evidence is available; the test restriction has not been weakened and no alternate native entrypoint was used.

The changelog P3 finding does not apply: this is maintainer-authored integration work under steipete, not an external contributor PR. Root AGENTS.md says maintainers and agents add user-visible fixes under Unreleased as work lands. The entry and full PR link remain.

No functional code finding was reported. The underlying linked-clone boot issue remains separate and open: #2398.

@steipete
steipete force-pushed the codex/parallels-ip-query-evidence-n846 branch from 634a725 to 9f7c216 Compare September 25, 2026 05:09
@steipete
steipete marked this pull request as ready for review September 25, 2026 05:09
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 25, 2026
@steipete
steipete force-pushed the codex/parallels-ip-query-evidence-n846 branch from 9f7c216 to 59a9adf Compare September 25, 2026 09:30
@steipete
steipete merged commit cd8c20b into main Sep 25, 2026
49 checks passed
@steipete
steipete deleted the codex/parallels-ip-query-evidence-n846 branch September 25, 2026 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor 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.

1 participant