fix: correct Pi metadata and provider support - #32
Conversation
f25febb to
60cb639
Compare
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Submitted via review-guard-mcp
Cleans up the Pi extension: fixes the copy-pasted Claude metadata, splits openai (API key) from openai-codex (OAuth), adds google/mistral providers with explicit auth.json session pointers, allowlists pi.dev, and sets two env vars for telemetry/version-check suppression. Config and docs only, no Go changes. make build, make test, make lint and make generate are all clean and both golden files are correctly regenerated.
Focus on the provider table in extensions/tools/pi/spec.yaml. Two entries there change runtime behavior in ways the diff doesn't show: the new oauthPorts: [53692] on anthropic means enclave auto-binds a second host port on most sessions and fails when that port is taken, and dropping credentials: [openai-api-key] from openai-codex removes the HasEnvCredential escape hatch in OAuthPortValidateRun. The five auth.json pointer names are the substance of the fix and can't be verified from this repo, so they need an upstream reference. Remaining comments are doc placement and naming nits.
| authSession: | ||
| mode: any | ||
| checks: | ||
| - { file: agent/auth.json, type: json_pointer_non_null, pointer: /openai } |
There was a problem hiding this comment.
These five pointers are the substance of the PR and a reviewer can't check them against anything in-tree. A wrong key fails silently: checkProviderSession just reports "no session", which shows up as the OAuth port being auto-mapped forever rather than an error. Can you link the upstream auth.json shape you verified against?
There was a problem hiding this comment.
The pointers are correct:
- Pi stores credentials under the provider ID at the top level of auth.json.
- Upstream provider IDs are exactly openai, openai-codex, anthropic, google, and mistral.
- The provider documentation (https://github.com/earendil-works/pi/blob/main/packages/coding-agent/docs/providers.md) confirms four API-key entries; the openai-codex provider definition (https://github.com/earendil-works/pi/blob/main/packages/ai/src/providers/openai-codex.ts) confirms the OAuth provider ID.
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up round. Since the last review the author made Anthropic OAuth opt-in (autoHintWhenNoSession: false plus requireMappingWhenNoCredentials: false, both needed so OAuthPortValidateRun doesn't hard-fail), dropped the codestral.mistral.ai credential release, named pi.dev in the README, and added an unrelated kit-init.sh permission fix to the Dockerfile and the microVM bundle script. make build, make test, make lint and make generate are all clean; both golden files match.
Two areas for a human to focus on. First, the kit-init.sh commit: the diagnosis is right (the entrypoint sources the file, so 0644 fits), but it isn't Pi-related and it leaves collectAppRootModeIssues out of sync. Second, the Mistral change now leaves codestral.mistral.ai DNS-reachable through the shared fragment while MISTRAL_API_KEY arrives in the container as a placeholder.
Four findings from the previous round are still open and unanswered: openai-codex losing credentials: [openai-api-key] (the one with real runtime impact), the unverifiable auth.json pointer names, the long description, and the README Settings-section placement. GitHub rejected my attempt to resolve the threads that were fixed, so the Anthropic OAuth, pi.dev naming and Codestral threads can be closed manually.
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
The entrypoint sources /usr/local/share/enclave/kit-init.sh as the unprivileged agent user, but neither image build path relaxed its mode. A restrictive host umask (0007) left the copy root-owned 0660, so the source failed with "Permission denied" and no feature entrypoint ran. Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
25aa026 to
e436697
Compare
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Third round. The only new commit since the last review narrows the Mistral allowlist to api.mistral.ai by replacing the shared mistral.conf include with an inline server= line, which makes DNS reach and credential release cover the same single host. That closes the Codestral gap properly. make build, make test, make lint, make check-license-headers and make generate are all clean, and both golden files match the spec.
I also traced the kit-init.sh fix end to end and the diagnosis holds: the microVM chown -R agent covers /opt/enclave but not /usr/local/share/enclave, which is why this one asset broke while its siblings didn't, and debian/rules plus the RPM spec both chmod -R go+rX over runtime-assets, so packaged installs really were unaffected. Nothing else copied in either build path is left unnormalized.
Where to focus: the provider table in extensions/tools/pi/spec.yaml. Four findings from earlier rounds are still open and unanswered, and I have not repeated them as comments. The one with real runtime impact is openai-codex losing credentials: [openai-api-key]: OPENAI_API_KEY no longer suppresses the port-1455 bind test, so a session hard-fails when that host port is occupied. The others are the unverifiable auth.json pointer and PI_* env var names, the 74-char description outlier, and the README Settings-section placement. Only one new comment this round, a maintainability nit on the inline Mistral entry.
These previous comments can be resolved as they are now handled:
- make Anthropic OAuth opt-in instead of binding a port every session
- name
pi.devin the README instead of describing it - Codestral left DNS-reachable without header injection
- Codestral reachable via the shared fragment while
MISTRAL_API_KEYarrives as a placeholder
I can't resolve them myself as I would need write permission on this repository.
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Fourth round. The only new commit since my last review adds the inline explanation on the api.mistral.ai allowlist entry that I asked for, so there are no new findings and no new inline comments this round. make build, make test, make lint, make check-license-headers and make generate are all clean, and both golden files match the spec.
I re-verified the provider rename fallout from scratch this round: provider names are pure labels in the runtime (auth-state map keys and error strings), nothing in Go or docs hardcodes gemini, and no host state path is keyed by provider name, so gemini -> google is safe. --no-api-key / --ephemeral suppression also still covers openai-api-key through the new openai provider, so dropping credentials from openai-codex doesn't regress that path.
Where a maintainer should focus: the four earlier findings that are still open and unanswered, which I have not repeated as comments. The one with real runtime impact is openai-codex losing credentials: [openai-api-key], since OPENAI_API_KEY no longer lets the port-1455 bind test be skipped (both codex and opencode keep the key on their OAuth-port provider). The rest are unverifiable-from-this-repo upstream names (the five auth.json pointers, the two PI_* variables, port 53692), the 74-char description outlier, and the README Settings-section placement. Separately, collectAppRootModeIssues still omits runtime-assets/kit-init.sh, and whether to split the kit-init.sh commit into its own PR is a maintainer call the author has already deferred.
These previous comments can be resolved as they are now handled:
I can't resolve them myself as I would need write permission on this repository.
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Fifth round. The one new commit addresses three earlier comments: the description is short and no longer enumerates Pi's tools, the telemetry/version-check note moved out of the Settings section into a dedicated ## Environment section with an upstream link, and runtime-assets/kit-init.sh is now listed in collectAppRootModeIssues with a matching test case. make build, make test, make lint, make check-license-headers and make generate are all clean, and both golden files match the spec.
The kit-init.sh entry uses the non-executable check, which matches how the entrypoint loads it (sourced, not exec'd), and sits in the existing alphabetical list. Using environment.variables in the spec rather than exporting from entrypoint.d/setup.sh is the documented mechanism for sandbox specs. I also confirmed the empty mode on the new authSession blocks defaults to any, so dropping mode: any is behavior-neutral with one check per provider.
Where a maintainer should focus: nothing new in the code itself, only one doc-discoverability comment about the opt-in Anthropic OAuth port. The remaining open items are maintainer calls, not defects: whether to split the kit-init.sh commit into its own PR (the author has deferred that decision), and the [ -f ] vs [ -r ] guard in entrypoint.sh, which the PR description already lists as an intentional follow-up.
These previous comments can be resolved as they are now handled:
- shorten the
descriptionso it stops enumerating Pi's tools - move the telemetry/version-check note out of the Settings section
- list
runtime-assets/kit-init.shincollectAppRootModeIssues - link the upstream
auth.jsonshape the provider pointers were verified against
I can't resolve them myself as I would need write permission on this repository.
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Sixth round. The only commit since my last review (04affaf) adds a "Pi Anthropic OAuth note" to docs/auth.md, telling users to pass -p 53692 when they intend to log in with Anthropic OAuth. That's exactly the doc-discoverability gap I flagged last round (since autoHintWhenNoSession: false means the port is never auto-hinted), it matches the existing Codex OAuth note's format, and the port number matches oauthPorts in extensions/tools/pi/spec.yaml. No new findings.
Everything else is unchanged since round five. The remaining open items are maintainer calls the author already deferred: whether to split the kit-init.sh commit into its own PR, and the [ -f ] vs [ -r ] guard in entrypoint.sh (explicitly called out as a follow-up in the PR description).
These previous comments can be resolved as they are now handled:
I can't resolve them myself as I would need write permission on this repository.
What it does
Fixes the Pi tool extension so a Pi session comes up with correct metadata, the right provider set, and a working model catalog:
pi.devto the gateway allowlist so Pi's model catalog update works under network isolation.It also fixes an unrelated image-build defect found while testing this branch:
enclave --tool pi shelldied right after startup withDockerfileandruntime-assets/microvm/alpine/build-bundle.shboth copyruntime-assets/kit-init.shinto the image but never normalized its mode, unlike every other asset they copy. The entrypoint sources it as the unprivileged agent user underset -e, so a source checkout with a restrictive umask (files at0660) produced a root-owned unreadable file and killed the container before any feature entrypoint ran. Installed builds were unaffected: they build from embedded assets, whichappassets.FileModewrites as0644.How to test
Pi extension:
make build && make testbin/enclave --tool pi— the session starts, and Pi's model catalog update succeeds with network isolation active.Image permissions:
chmod 0660 runtime-assets/kit-init.sh.bin/enclave --tool pi shell— before this change the container exits with thePermission deniederror above; after it, the shell comes up.docker run --rm --entrypoint /bin/sh <image> -c 'ls -l /usr/local/share/enclave/kit-init.sh'reports a world-readable mode.Follow-ups
Two hardening items around the
kit-init.shfailure, neither included here:entrypoint.shguards this source with[ -f ]where the sibling asset loads (net.sh,build-scripts/lib/common.sh) use[ -r ]. With-ran unreadable asset would fail open and degrade instead of killing the container.collectAppRootModeIssuesininternal/app/build_permissions.gowarns about restrictive modes on the other copied runtime assets but omitsruntime-assets/kit-init.sh, so nothing pointed at the offending file.The image-permission fix is independent of the Pi changes and can be split into its own PR if you would rather keep this one scoped to Pi.
Breaking changes
Review checklist