Skip to content

Add Pi provider support for agent runs#354

Draft
sepo-agent-app[bot] wants to merge 3 commits into
mainfrom
agent/implement-issue-350/codex-26416226505
Draft

Add Pi provider support for agent runs#354
sepo-agent-app[bot] wants to merge 3 commits into
mainfrom
agent/implement-issue-350/codex-26416226505

Conversation

@sepo-agent-app

Copy link
Copy Markdown

Summary

  • add pi to provider resolution, model policy, workflow setup, and runtime installation
  • restore PI_AUTH_JSON_B64 / PI_AUTH_JSON only for Pi runs with isolated auth and cleanup
  • keep Pi session setup conservative and bundle Pi session logs without auth secrets
  • update onboarding and docs for Pi configuration and OAuth refresh-token caveats

Tests

  • npm test
  • git diff --check

Closes #350

@sepo-agent-app sepo-agent-app Bot added the agent Handled by the agent label May 25, 2026
@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@lolipopshock

Copy link
Copy Markdown
Contributor

@sepo-agent /answer is the base64 version really that helpful?

@sepo-agent-app

Copy link
Copy Markdown
Author

If you mean PI_AUTH_JSON_B64: it is only modestly helpful.

It does not add security. It is just an ergonomics fallback for putting Pi auth.json into a single-line GitHub Secret and avoiding multiline JSON quoting/copy-paste issues across secret UIs, workflow inputs, and shells. The implementation already treats it correctly: restore only for agent == 'pi', fail if both PI_AUTH_JSON_B64 and PI_AUTH_JSON are set, write to $RUNNER_TEMP/pi-agent/auth.json, and clean up afterward.

I would keep it as an optional escape hatch, but avoid presenting it as preferred. The docs should frame the hierarchy as:

  1. provider API keys for normal GitHub-hosted CI;
  2. persistent self-hosted ~/.pi/agent/auth.json for Pi OAuth/subscription auth;
  3. PI_AUTH_JSON / PI_AUTH_JSON_B64 for testing or simple secret-based restore.

If you want to reduce surface area, dropping _B64 is also reasonable. The only real loss is convenience for users whose secret-management path handles one-line values better than multiline JSON.

@lolipopshock

Copy link
Copy Markdown
Contributor

@sepo-agent /answer can we make sure the model / reasoning effort is set in the right way for pi?

@sepo-agent-app

Copy link
Copy Markdown
Author

Yes. The right behavior for Pi should be:

One thing I would tighten: don’t advertise or rely on providers.pi.reasoning_effort as meaningful yet. Either ignore/clear it at resolver output for Pi, or document it explicitly as currently ignored. Model is supported; reasoning effort should stay Pi-specific only after we confirm Pi/acpx has a real equivalent setting.

@lolipopshock

Copy link
Copy Markdown
Contributor

@sepo-agent /answer given what we've discussed, what's the right way to fix this PR? detailed plan

@sepo-agent-app

Copy link
Copy Markdown
Author

I’d fix this PR with a tightening pass, not a rewrite. The core direction is still right: Pi should stay an acpx-backed provider, with conservative Pi-specific behavior only where Codex/Claude assumptions would leak.

Current PR state:

The right target behavior should be:

  • pi supports model.
  • pi does not support Sepo reasoning_effort yet.
  • Pi auth restoration remains optional and Secret-only.
  • PI_AUTH_JSON_B64 can stay, but should be documented as a convenience fallback, not a preferred or safer path.

Plan:

  1. Make model handling explicit and non-duplicative

    • For exec/no-session runs, pass Pi model through the existing global acpx --model <model> pi exec ... path.
    • For named sessions, rely on acpx pi set model <model> -s <session> before acpx pi prompt -s <session> ....
    • I would avoid passing both --model and set model for named sessions unless we have a confirmed reason. The pre-PR pattern already avoided global --model for named sessions, and session setup is the clearer place to persist model choice.
  2. Treat Pi reasoning effort as unsupported, not silently configurable

    • Update resolver behavior so when the resolved provider is pi, reasoning_effort output is always empty.
    • Keep the runtime guard in run.ts as defense in depth.
    • Update resolver tests so providers.pi.reasoning_effort and route-level reasoning_effort with provider: "pi" do not produce an output reasoning effort.
    • Optional: emit a warning when Pi has a configured reasoning effort, but I’d probably avoid noisy warnings and document the behavior instead.
  3. Keep Pi session setup conservative

    • Preserve the current Pi branch in buildSessionSetupCommands: model only, no thought_level, no set-mode full-access.
    • Keep or expand the test at acpx-adapter.test.ts to assert that Pi ignores thoughtLevel even when the workflow fallback passes one.
  4. Clean up docs

    • In AGENT_MODEL_POLICY examples, keep providers.pi as { "model": "..." } only.
    • Change wording that says model policy can set provider-specific reasoning effort for Pi. It should say reasoning effort applies to Codex/Claude today; Pi currently supports configured model only.
    • Keep the auth docs’ hierarchy clear: provider API keys first, persistent self-hosted Pi auth for OAuth/subscription auth, PI_AUTH_JSON / PI_AUTH_JSON_B64 for testing or simple secret restore.
  5. Do not broaden Pi auto-detection beyond actual Pi auth

    • auto should choose Pi only from Pi auth indicators, not from generic OPENAI_API_KEY / ANTHROPIC_API_KEY.
    • Explicit AGENT_DEFAULT_PROVIDER=pi can still rely on local Pi auth or provider keys available to Pi; that is the self-hosted/operator-choice path.
  6. Verification

    • Add/update focused tests:
      • resolver accepts providers.pi.model;
      • resolver clears/ignores Pi reasoning_effort;
      • Pi named-session setup only calls set model;
      • Pi exec uses model correctly;
      • Pi runtime does not expose MODEL_REASONING_EFFORT / CLAUDE_CODE_EFFORT_LEVEL.
    • Run npm test.
    • Run git diff --check.

I’d ask the implementer to make those changes as a focused follow-up on PR #354, with no new Pi adapter and no extra auth redesign.

@lolipopshock

Copy link
Copy Markdown
Contributor

I’d fix this PR with a tightening pass, not a rewrite. The core direction is still right: Pi should stay an acpx-backed provider, with conservative Pi-specific behavior only where Codex/Claude assumptions would leak.

Current PR state:

The right target behavior should be:

  • pi supports model.
  • pi does not support Sepo reasoning_effort yet.
  • Pi auth restoration remains optional and Secret-only.
  • PI_AUTH_JSON_B64 can stay, but should be documented as a convenience fallback, not a preferred or safer path.

Plan:

  1. Make model handling explicit and non-duplicative

    • For exec/no-session runs, pass Pi model through the existing global acpx --model <model> pi exec ... path.
    • For named sessions, rely on acpx pi set model <model> -s <session> before acpx pi prompt -s <session> ....
    • I would avoid passing both --model and set model for named sessions unless we have a confirmed reason. The pre-PR pattern already avoided global --model for named sessions, and session setup is the clearer place to persist model choice.
  2. Treat Pi reasoning effort as unsupported, not silently configurable

    • Update resolver behavior so when the resolved provider is pi, reasoning_effort output is always empty.
    • Keep the runtime guard in run.ts as defense in depth.
    • Update resolver tests so providers.pi.reasoning_effort and route-level reasoning_effort with provider: "pi" do not produce an output reasoning effort.
    • Optional: emit a warning when Pi has a configured reasoning effort, but I’d probably avoid noisy warnings and document the behavior instead.
  3. Keep Pi session setup conservative

    • Preserve the current Pi branch in buildSessionSetupCommands: model only, no thought_level, no set-mode full-access.
    • Keep or expand the test at acpx-adapter.test.ts to assert that Pi ignores thoughtLevel even when the workflow fallback passes one.
  4. Clean up docs

    • In AGENT_MODEL_POLICY examples, keep providers.pi as { "model": "..." } only.
    • Change wording that says model policy can set provider-specific reasoning effort for Pi. It should say reasoning effort applies to Codex/Claude today; Pi currently supports configured model only.
    • Keep the auth docs’ hierarchy clear: provider API keys first, persistent self-hosted Pi auth for OAuth/subscription auth, PI_AUTH_JSON / PI_AUTH_JSON_B64 for testing or simple secret restore.
  5. Do not broaden Pi auto-detection beyond actual Pi auth

    • auto should choose Pi only from Pi auth indicators, not from generic OPENAI_API_KEY / ANTHROPIC_API_KEY.
    • Explicit AGENT_DEFAULT_PROVIDER=pi can still rely on local Pi auth or provider keys available to Pi; that is the self-hosted/operator-choice path.
  6. Verification

    • Add/update focused tests:

      • resolver accepts providers.pi.model;
      • resolver clears/ignores Pi reasoning_effort;
      • Pi named-session setup only calls set model;
      • Pi exec uses model correctly;
      • Pi runtime does not expose MODEL_REASONING_EFFORT / CLAUDE_CODE_EFFORT_LEVEL.
    • Run npm test.

    • Run git diff --check.

I’d ask the implementer to make those changes as a focused follow-up on PR #354, with no new Pi adapter and no extra auth redesign.

@sepo-agent /orchestrate fix this first; then review, then merge from main branch; then review

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

Comment thread .agent/src/acpx-adapter.ts
@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

Copy link
Copy Markdown
Author

Sepo pushed fixes for this PR. Branch: agent/implement-issue-350/codex-26416226505. Requested by @sepo-agent.

  • Scrubbed MODEL_REASONING_EFFORT and CLAUDE_CODE_EFFORT_LEVEL from the actual acpx child-process env whenever the selected agent is pi.
  • Added a fake-acpx regression test that sets both vars on the parent process and verifies acpx pi exec receives neither.
  • Verification: node --test dist/__tests__/acpx-adapter.test.js dist/__tests__/runtime-env.test.js passed; env GITHUB_OUTPUT=/tmp/sepo-pr354-github-output npm test passed 710/710; git diff --check passed.
  • Note: plain npm test initially hit sandbox EPERM on the inherited runner GITHUB_OUTPUT path; rerun with GITHUB_OUTPUT redirected to /tmp passed.

codex | default model | xhigh | Macmini-runner-4

@sepo-agent-app

sepo-agent-app Bot commented May 31, 2026

Copy link
Copy Markdown
Author

Sepo is dispatching follow-up automation.

Source Next Target Round Status
fix-pr review PR #354 5 / 40 Dispatched

Reason: agent planner selected review: Fix-pr completed successfully on PR #354, and the original request explicitly asked to review after applying the tightening pass.

@sepo-agent-app

Copy link
Copy Markdown
Author

Rubrics Review

Total Score Verdict Rubrics Scored
95 PASS 11
Dimension Rubric Result Score Evidence
coding_workflow / generic Reuse existing code pass 10/10 Pi stays on the existing ACPX path; buildAcpxArgs builds generic acpx <agent> args and only adds conservative Pi-specific handling. Evidence: acpx-adapter.ts.
coding_style / generic Prefer simple implementations pass 9/10 The PR adds provider plumbing, setup, auth restore, and targeted guards without introducing a separate Pi adapter or auth redesign. Evidence: resolve-provider.js, setup-agent-runtime/action.yml.
coding_workflow / generic Make surgical changes pass 9/10 The diff is broad because provider selection is wired through workflows/docs/tests, but changes stay scoped to Pi provider support and the requested tightening.
coding_workflow / generic Prefer explicit structured inputs pass 10/10 Adds explicit install_pi, pi_version, Pi auth inputs, and Pi readiness flags rather than overloading existing provider fields. Evidence: setup-agent-runtime/action.yml, run-agent-task/action.yml.
coding_workflow / generic Name config by capability pass 9/10 Generic provider knobs remain AGENT_DEFAULT_PROVIDER / AGENT_MODEL_POLICY; Pi-specific names are limited to intentionally Pi-specific auth restoration. Evidence: configuration-list.md, configuration-list.md.
coding_workflow / generic Minimal defaults, native escape hatches pass 10/10 Pi supports configured model only; resolver clears Pi reasoning effort, named sessions use set model, and inline route_provider remains the native workflow override. Evidence: resolve-provider.js, supported-workflows.md.
coding_workflow / generic Keep docs in sync pass 10/10 Docs now cover Pi selection, model-only policy behavior, Secret-only auth restore, and OAuth refresh caveats. Evidence: configuration-list.md, supported-workflows.md.
communication / generic Write audience-first documentation pass 9/10 Entry docs stay task-oriented; deeper docs carry Pi auth trade-offs and limitations. Evidence: quick-start.md, configuration-list.md.
communication / generic Make setup guidance actionable pass 9/10 Setup docs tell users which provider secrets/config paths to use and when persistent self-hosted Pi auth is preferred. Evidence: install-existing-repository.md, self-hosted-github-action-runner.md.
coding_workflow / generic Separate diagnostics from continuity pass 10/10 Pi session logs are bundled, while auth secrets are excluded and covered by tests. Evidence: session-bundle.ts, session-bundle.test.ts.
coding_workflow / generic Surface stateful failures pass 10/10 Pi auth restore fails clearly for dual secrets or missing runtime env, uses isolated temp auth, cleans up restored auth, and tests cover ambiguity/permissions. Evidence: restore-pi-auth.sh, pi-auth.test.ts.

Notes

  • Current head d94ffffa7e264265c3dac0b6c950dff5c7d9e841 satisfies the requested tightening: Pi keeps model, resolver output clears Pi reasoning_effort, runtime clears Pi thought level, and runAcpx scrubs inherited MODEL_REASONING_EFFORT / CLAUDE_CODE_EFFORT_LEVEL from Pi child processes.
  • Focused tests cover Pi model args, named-session model setup, conservative session setup, resolver-cleared reasoning effort, runtime shared env, child-process env scrubbing, and auth restoration.
  • git diff --check origin/main...d94ffffa7e264265c3dac0b6c950dff5c7d9e841 passed.
  • No rubric-specific follow-up needed.

Findings

  • INFO: No blocking or warning-level rubric findings. The implementation satisfies the applicable selected rubrics.

Final Rubric Verdict

PASS


codex | gpt-5.5/medium | high | Macmini-runner-4

@sepo-agent-app

Copy link
Copy Markdown
Author

AI Review Synthesis

Dual-agent review by Claude and Codex.
Requested by @lolipopshock.

Summary of PR/Issue

PR #354 adds pi as an acpx-backed Sepo provider for issue #350. The agreed scope is provider plumbing through the existing acpx path, not a separate Pi adapter. It wires Pi into provider resolution, runtime setup, auth restoration, session setup, session bundles, workflows, onboarding, and docs. The later fix-pr pass tightened Pi to support configured model only while clearing Sepo reasoning-effort knobs from resolver output, shared env, runtime setup, and child acpx pi processes.

Review

The current PR head is safe to ship. Codex and rubrics review found no blocking or warning-level issues; Claude raised minor cleanup/readability concerns, but they do not change runtime behavior or require another automated branch change.

Issue Severity Description
No required branch-change issues INFO The prior Pi child-env reasoning-effort leak is fixed and covered by a fake-acpx regression test. Remaining reviewer disagreement is limited to optional cleanup/commentary nits, not required merge-blocking work.

Progress

  • Used all current review artifacts found under /reviews: Claude, Codex, and rubrics review outputs.
  • Re-fetched PR conversation, inline comments, and review threads before cleanup.
  • Resolved same-agent review thread PRRT_kwDOSOhEMM6F6yoA; its requested env scrub and fake-acpx test are present in current head d94ffffa7e264265c3dac0b6c950dff5c7d9e841.
  • No new inline comments were posted.
  • Re-ran git diff --check origin/main...origin/pr-354; it passed. Codex review reports npm test passed 710/710.

Recommended Next Step

NO_AUTOMATED_ACTION: no required branch-change work remains, and the remaining notes are optional human cleanup choices.

Final Verdict

SHIP

Action Items

  • No required branch-change action items.

codex | gpt-5.5/medium | xhigh | Macmini-runner-6

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

Labels

agent Handled by the agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Pi provider support for Sepo agent runs

2 participants