Skip to content

fix(test): budget Pi helper startup independently - #643

Merged
piekstra merged 2 commits into
mainfrom
piekstra/fix-pi-helper-startup-diagnostics
Oct 2, 2026
Merged

piekstra merged 2 commits into
mainfrom
piekstra/fix-pi-helper-startup-diagnostics

Conversation

@piekstra

@piekstra piekstra commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Summary

Give the Pi process-group test a bounded startup budget for both Node processes. Keep the helper's five-second runtime timeout and the cleanup assertion unchanged, and explicitly verify that the helper belongs to the runner's process group.

Capture runner diagnostics on failure, report helper tool errors, and wait for a complete positive PID rather than merely the existence of its file. No production adapter or Pi compatibility flags change. This does not overlap the settlement/accounting changes in #641.

Evidence

  • The successful main-CI test took 2.98 seconds against a three-second startup limit. The opt-out test exceeded that limit twice: main CI.
  • A deterministic 3.1-second runner-only delay reproduces the old assertion failure. The updated test passes three repeated delayed runs. This reproduces the insufficient-budget failure mode, not the exact runner scheduling conditions.
  • The unchanged Linux test passed ten isolated runs. Updated macOS race tests passed 25 repeats; lint reports zero issues.
  • The checked-in slow-start and detached-helper preloads are executable controls for the real generated extension, not mock Pi/provider results.
  • The detached-helper control fails the explicit process-group assertion in 0.30 seconds, confirming that the longer startup budget does not permit a detached helper.
  • The full normal Linux suite passes with both keyring tags. A separate full Linux race run failed two unchanged Claude subprocess timing assertions (timeout stop acknowledgement and stream duration); the focused Pi race repeats pass. Both actual GitHub CI test jobs passed on the initial PR head.

Re-run the normal test:

go test -race -tags keyring_nopassage,keyring_no1password ./internal/llmadapters -run '^TestPiRPCReviewerHelperStaysInParentProcessGroup$' -count=25

For the slow-start control, set NODE_OPTIONS to --require plus the absolute path of internal/llmadapters/testdata/pi_rpc_slow_start.cjs; the test should pass. With pi_rpc_detached_helper.cjs instead, it must fail the process-group assertion. These controls are test-only and must not be used for normal CLI execution.

The locally installed Pi 0.68.1 does not support the required reviewer isolation flags. This PR does not weaken those controls or claim to fix that separate runtime compatibility gap. Linux verification of the generated extension is not a real Pi/provider integration result.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: 8b77132f935d
Profile: codex-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0

Reviewer Coverage

  • go:implementation-tests — complete (broad); skipped: none; constraints: Review scoped to the assigned test and controls, with nearby extension and process helpers inspected. Validated on macOS with CGO_ENABLED=0: three normal runs and the slow-start control passed; the detached-helper control failed the process-group assertion as intended. Default CGO build was blocked by a compiler cache path error; Linux and race runs were not performed.
Inspected files (3)
  • internal/llmadapters/pi_rpc_extension_unix_test.go
  • internal/llmadapters/testdata/pi_rpc_detached_helper.cjs
  • internal/llmadapters/testdata/pi_rpc_slow_start.cjs

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 2m 04s | gpt-6.1-sol | cr 0.10.319
Field Value
Model gpt-6.1-sol
Reviewers go:implementation-tests
Engine codex_cli · gpt-6.1-sol
Reviewed by cr · monit-reviewer
Duration 2m 04s wall · 1m 51s compute
Cost unavailable
Tokens 493.6k in / 1.8k out

Per-workstream usage

  • orchestrator-selection — gpt-6.1-sol
    • In: 79.6k
    • Out: 316
    • Cache read: 59.0k
    • Cache create: unavailable
    • Cost: unavailable
    • Duration: 25s
  • go:implementation-tests — gpt-6.1-sol
    • In: 298.1k
    • Out: 1.1k
    • Cache read: 272.5k
    • Cache create: unavailable
    • Cost: unavailable
    • Duration: 1m 18s
  • orchestrator-rollup — gpt-6.1-sol
    • In: 115.8k
    • Out: 389
    • Cache read: 91.4k
    • Cache create: unavailable
    • Cost: unavailable
    • Duration: 7s

@piekstra
piekstra marked this pull request as ready for review October 2, 2026 11:35
@piekstra
piekstra marked this pull request as draft October 2, 2026 11:38

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: a4966103b244
Profile: codex-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0

Reviewer Coverage

  • go:implementation-tests — complete (constrained); skipped: none; constraints: Read-only sandbox prevented running tests that create build artifacts and temporary scripts. Prior-run validation was not treated as execution evidence for this head. Review scoped to the assigned files and nearby extension, polling, and process-existence helpers.
Inspected files (3)
  • internal/llmadapters/pi_rpc_extension_unix_test.go
  • internal/llmadapters/testdata/pi_rpc_detached_helper.cjs
  • internal/llmadapters/testdata/pi_rpc_slow_start.cjs

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 1m 10s | gpt-6.1-sol | cr 0.10.319
Field Value
Model gpt-6.1-sol
Reviewers go:implementation-tests
Engine codex_cli · gpt-6.1-sol
Reviewed by cr · monit-reviewer
Duration 1m 10s wall · 57s compute
Cost unavailable
Tokens 602.1k in / 2.0k out

Per-workstream usage

  • go:implementation-tests — gpt-6.1-sol
    • In: 447.1k
    • Out: 1.6k
    • Cache read: 402.9k
    • Cache create: unavailable
    • Cost: unavailable
    • Duration: 50s
  • orchestrator-rollup — gpt-6.1-sol
    • In: 155.0k
    • Out: 456
    • Cache read: 127.5k
    • Cache create: unavailable
    • Cost: unavailable
    • Duration: 6s

@piekstra
piekstra marked this pull request as ready for review October 2, 2026 11:41
@piekstra
piekstra merged commit 015bba0 into main Oct 2, 2026
10 checks passed
@piekstra
piekstra deleted the piekstra/fix-pi-helper-startup-diagnostics branch October 2, 2026 11:42
@piekstra

piekstra commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

Post-merge verification: v0.10.322 points to merge commit 015bba069b03c5117a54dd7236a65f3b73612730 and includes both #643 and #644.

  • Main CI passed, including both test jobs.
  • Release passed all five jobs: GoReleaser, Homebrew, Chocolatey, WinGet, and Linux dispatch. The real Windows archive resolver passed and both Windows jobs ran.
  • Downloaded the Darwin ARM64 archive into an isolated temporary directory. SHA-256 matches the published checksum; cr --version reports 0.10.322 and the exact merge commit. Build metadata reports CGO_ENABLED=1 and vcs.modified=false. The Homebrew cask pins 0.10.322 with the same archive checksum. No shared installation was changed.
  • Chocolatey accepted codereview-cli.0.10.322.nupkg; its API currently reports Submitted and IsApproved=false. Availability still awaits moderation.
  • WinGet submitted microsoft/winget-pkgs #445652; its validation and merge remain pending.
  • Downstream Linux repository publication is still running; the successful dispatch is not proof of completed repository publication.

Historical failed runs are unchanged. This verification uses the new merged commit and release. It does not claim a live Pi/provider integration result or resolve compatibility of older installed Pi versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants