Skip to content

fix(pi): keep session command failures structural - #14235

Closed
kvnloo wants to merge 10 commits into
pingdotgg:t3code/pi-provider-v1from
kvnloo:fix/pi-command-review-safety-20260929
Closed

kvnloo wants to merge 10 commits into
pingdotgg:t3code/pi-provider-v1from
kvnloo:fix/pi-command-review-safety-20260929

Conversation

@kvnloo

@kvnloo kvnloo commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Stacked on #13777. This resolves the remaining session-command error-surface review findings without changing Pi session behavior.

What changes

  • ProviderSessionCommandError now carries the bounded command category (copy|export|share) plus the underlying cause instead of copying cause.message into the RPC-visible detail.
  • RPC messages are structural: e.g. Failed to share provider session.
  • failed gh gist create stderr stays out of PiRpcError.message; it is retained only as the underlying cause.
  • tests assert provider/CLI text does not appear in the public error message.

Scope

No Pi turn/session lifecycle changes. No provider discovery or UI changes.

Validation

Focused regression coverage is included in:

  • apps/server/src/provider/PiSessionCommands.test.ts
  • apps/server/src/server.test.ts

This branch is an exact stack on #13777 head 5f344199fbd5ace71194302362dcb5b96a829d8b; upstream CI is the executable validation surface.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 29, 2026
Comment thread packages/contracts/src/provider.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This small bug-fix changes the shared WebSocket error contract and forwards raw provider or GitHub CLI causes to clients. Those causes may contain sensitive paths, account details, or credentials, warranting human security review.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

kvnloo commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the blocking review finding at 953b989c: the RPC error schema no longer contains cause, and the WebSocket mapping drops provider/CLI causes entirely. Raw gh stderr remains only inside the server-local PiRpcError cause; the serialized client error is structural (threadId + command) with a fixed message.

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

The PR lists two regression suites but gives no executed results, and current-head CI is awaiting approval. The verification requirement requires observed outcomes for the changed behavior. Please run the focused error-serialization tests or link a completed CI run that includes them, then request reconsideration.

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

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants