Repository navigation
Conversation
johntmyers
added
gator:in-review
|
Label |
|
/ok to test 5e32a94 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This provider-boundary change is project-valid under linked issue #3442 and its pre-0.1.0 roadmap. The independent initial review found no blocking defects, and the required current-head branch, Helm, and E2E workflows are queued after creating the PR mirror.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Gator metadata
- Validation: Implements the defined provider-boundary work in #3442 under roadmap #3171 and the coordinated pre-0.1.0 effort #2565
- Docs: Fern provider and migration docs, provider examples, and architecture documentation are updated
- Checks: Current-head Branch Checks and Helm Lint workflows are queued
- E2E:
test:e2eapplied;/ok to testcreated the current-head mirror and Branch E2E Checks is queued - Head SHA:
5e32a9462576ccf25ba7a12ea98dc18e36576fe7 - Base SHA:
eef8bec0c96b384556d608f8d899a8a96f5d17a1 - Merge base SHA:
9f60f55c6b0811b1651099b50e4618d435ebc02e - Patch ID:
b24a544279787e3a990b12da55d77491e4d6bd1d - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
|
/ok to test 8e48662 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The follow-up review checked the new endpointless-provider E2E coverage added since the prior reviewed head and found no blocking defects. The current-head mirror is now updated, and the required branch, Helm, and E2E workflows are queued.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Gator metadata
- Validation: Implements the provider-boundary work defined by linked issue #3442 under the coordinated pre-0.1.0 roadmap
- Docs: Existing Fern provider and migration docs remain sufficient; this follow-up delta changes only E2E coverage
- Checks: Current-head Branch Checks and Helm Lint workflows are queued
- E2E:
test:e2eremains applied;/ok to testupdated the mirror and current-head Branch E2E Checks is queued - Head SHA:
8e48662edf04d770f1dfe1eaf5c4d1efb7a92b57 - Base SHA:
eef8bec0c96b384556d608f8d899a8a96f5d17a1 - Merge base SHA:
9f60f55c6b0811b1651099b50e4618d435ebc02e - Patch ID:
9ff76bb05f61eeef8087be0b1cbec4c271e650bc - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
5e32a9462576ccf25ba7a12ea98dc18e36576fe7 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
johntmyers
added
gator:blocked
8e48662 to
ab90b18
Compare
|
/ok to test ab90b18 |
|
I have updated the user documentation which was incomplete |
|
/ok to test baef723 |
|
@johntmyers @drew I wonder about the support of GCP metadata emulator. The implementation has been removed by PR #2942 ( |
Can you file a bug to add it back? This was likely done in error and missed if there were no E2E tests. |
|
I'd also like to note that we can't merge this PR (#3775) unless we make it backwards compatible. Breaking changes need to get slotted into the 0.2.0 release. |
I have created #3860 |
|
I rebased the branch to resolve conflicts |
|
/ok to test 1ac5b17 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @feloy, I checked your conflict-resolution rebase against the previously reviewed author series. The changed schema fingerprints and generated Go bindings account for the newer upstream schema; the bounded Critical-only review found no newly introduced Critical defect. The resolved Claude executable guidance remains intact. @drew's compatibility requirement remains unresolved: existing Google profiles still need new environment and discovery declarations to preserve their behavior.
Action required: @feloy, preserve compatibility for the current release or defer the breaking behavior to 0.2.0 as @drew directed. A maintainer also needs to approve the current-head Trivy workflow.
Blocking findings:
- No new Gator code-review blockers on this delta.
Carried findings:
GATOR-baef7232-01: resolved; the corrected executable guidance and resolved thread are preserved.- Maintainer requirement: backward compatibility remains unresolved.
I also checked your report of Windows and Docker E2E failures on the previous head. The refreshed mirror now runs Branch Checks, Helm Lint, E2E, and both Windows architectures on this rebased head; their results will determine whether those failures persist.
Gator metadata
- Validation: Implements the provider-boundary work defined by linked issue #3442.
- Docs: Fern provider and migration documentation retained; existing Google profiles must migrate environment and discovery declarations.
- Checks: DCO passed; current-head Branch Checks and Helm Lint running; Trivy requires maintainer workflow approval.
- E2E:
test:e2eretained; current-head/ok to testrefreshed the mirror and Branch E2E Checks is running. - Windows:
test:windowsretained; x64 and ARM64 lint/test jobs are running. - Head SHA:
1ac5b17f6370ad5391a3df9363495c9545325f6f - Base SHA:
71c3cd957abef062eb7f37010056717cd49f2ed3 - Merge base SHA:
71c3cd957abef062eb7f37010056717cd49f2ed3 - Patch ID:
5af9108eec82ff636ac9292043581fbb422ce7d3 - Gator payload:
10 - Review mode:
critical_only - Previous reviewed SHA:
89b62a3fb91fdcdfd968f9b5ed3c3e38ffb1b0fb - Review budget exhausted: yes
- Maintainer decision required: no — the existing compatibility direction is clear; workflow approval is a separate process action.
- Next state:
gator:blocked - Blocked reason:
workflow_approval_required
|
/ok to test 2757514 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @feloy, I compared the rebased author series with the last reviewed head. Five commits are patch-equivalent; the remaining differences update schema fingerprints and generated Go bindings for the newer upstream schema. The bounded Critical-only review found no newly introduced Critical defect, and the resolved Claude executable guidance remains intact. @drew's compatibility requirement remains unresolved: existing Google profiles still need new environment and discovery declarations to preserve their behavior.
Action required: @feloy, preserve compatibility for the current release or defer the breaking behavior to 0.2.0 as @drew directed. A maintainer also needs to approve the current-head Trivy workflow.
Blocking findings:
- No new Gator code-review blockers on this delta.
Carried findings:
GATOR-baef7232-01: resolved; the corrected executable guidance and resolved thread are preserved.- Maintainer requirement: backward compatibility remains unresolved.
The current-head test mirror is refreshed. Branch Checks, Helm Lint, E2E, and Windows MSVC are queued; their results will show whether the previous Windows lint and supervisor image publication failures recur.
Gator metadata
- Validation: Implements the provider-boundary work defined by linked issue #3442.
- Docs: Fern provider and migration documentation retained; existing Google profiles must migrate environment and discovery declarations.
- Checks: DCO passed; Branch Checks and Helm Lint queued; Trivy requires maintainer workflow approval.
- E2E:
test:e2eretained; current-head/ok to testrefreshed the mirror and Branch E2E Checks is queued. - Windows:
test:windowsretained; Windows MSVC is queued. - Head SHA:
27575147034ae4f1ea6047a39166057b828f3944 - Base SHA:
834b79a8c2351370e7c6c363423f5be18d440256 - Merge base SHA:
834b79a8c2351370e7c6c363423f5be18d440256 - Patch ID:
7ea17307b3130ce88b49fc3ac035ca9ff48146a4 - Gator payload:
10 - Review mode:
critical_only - Previous reviewed SHA:
1ac5b17f6370ad5391a3df9363495c9545325f6f - Review budget exhausted: yes
- Maintainer decision required: no — the existing compatibility direction is clear; workflow approval is a separate process action.
- Next state:
gator:blocked - Blocked reason:
workflow_approval_required
|
/ok to test 367047a |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @feloy, I checked your rebased author series against the last reviewed head. Five commits remain patch-equivalent; the remaining differences adapt to upstream credential-owner changes and updated schema fingerprints and Go bindings. The bounded Critical-only review found no newly introduced Critical defect, and the resolved Claude executable guidance remains intact. @drew's compatibility requirement remains unresolved: existing Google profiles still need new environment and discovery declarations to preserve their behavior.
Action required: @feloy, preserve compatibility for the current release or defer the breaking behavior to 0.2.0 as @drew directed. A maintainer also needs to approve the current-head Trivy workflow.
Blocking findings:
- No new Gator code-review blockers on this delta.
Carried findings:
GATOR-baef7232-01: resolved; its executable guidance and maintainer-resolved thread are preserved.- Maintainer requirement: backward compatibility remains unresolved.
The current-head test mirror is refreshed. Branch Checks, Helm Lint, E2E, and Windows MSVC are queued; their results will show whether the earlier failures recur.
Gator metadata
- Validation: Implements the provider-boundary work defined by linked issue #3442.
- Docs: Fern provider and migration documentation retained; existing Google profiles require migration.
- Checks: DCO passed; Branch Checks and Helm Lint queued; Trivy requires maintainer workflow approval.
- E2E:
test:e2eretained; current-head/ok to testrefreshed the mirror and Branch E2E Checks is queued. - Windows:
test:windowsretained; Windows MSVC is queued. - Head SHA:
367047a6d0f130edbb806c83acbffa753da73622 - Base SHA:
67adcf1a3cf054be54dfdbf6a44c65b4d426b1ec - Merge base SHA:
67adcf1a3cf054be54dfdbf6a44c65b4d426b1ec - Patch ID:
abec1acfcc518f6fa14565426a5bc98ecd926337 - Gator payload:
10 - Review mode:
critical_only - Previous reviewed SHA:
27575147034ae4f1ea6047a39166057b828f3944 - Review budget exhausted: yes
- Maintainer decision required: no — the existing compatibility direction is clear; workflow approval is a separate process action.
- Next state:
gator:blocked - Blocked reason:
workflow_approval_required
2e34c9e to
f7376ad
Compare
|
7949d5c to
abf3c8d
Compare
abf3c8d to
b9ff1d1
Compare
|
Heads-up: the Go SDK moves to ai-openshell/sdk-go on 2026-11-03 (#5031). This PR also changes hand-written SDK code under |
b9ff1d1 to
bbffd0e
Compare
|
Could I have review on this PR targeting next release? |
krishicks
left a comment
krishicks
left a comment
There was a problem hiding this comment.
Written by Claude Code, reviewed by @krishicks
Upgrade docs belong in a 0.2.0 guide
The Google migration text is appended to docs/upgrade/0-1-0.mdx, which is for a release that has already shipped. Move it to a new docs/upgrade/0-2-0.mdx with a docs/index.yml entry. Also describe what operators see if they don't migrate. A stored google-cloud or google-vertex-ai profile without an environment block still attaches without error. Its sandboxes get only the token placeholders: no project, region, GOOSE_PROVIDER/ANTHROPIC_VERTEX_PROJECT_ID, or GCE_METADATA_HOST. SDKs then fall back to 169.254.169.254, which is blocked. Nothing at attach time or in logs points to the cause.
required_platform_adapter doesn't enable or gate anything at runtime
ensure_platform_adapter_available(crates/openshell-providers/src/profiles.rs:851) checkscfg!(windows)on the gateway binary. Nothing in the supervisor reads the field.- The metadata relay is always on for Linux sandboxes (
crates/openshell-supervisor-network/src/proxy.rs:693). - The env vars that point SDKs at the relay come from
environment.fixed, so any profile can setGCE_METADATA_HOSTwithout declaring the adapter. - The check measures the gateway's platform, but the capability belongs to the sandbox runtime. A Linux gateway driving MXC sandboxes would accept the profile.
Two options:
- Have the adapter own its env, so declaring
gcp-metadatainjects the metadata host, IP and detection values.127.0.0.1:8174is currently duplicated betweenproviders/google-cloud.yamlandopenshell_core::google_cloud. Check availability against a capability the compute driver or supervisor advertises, not the gateway's platform. - Or drop the field and document the requirement in the example profile's header.
Caller-set GCP_PROJECT_ID makes the metadata emulator return 404 for project-id
When the sandbox spec or template sets GCP_PROJECT_ID, the caller-wins filter (crates/openshell-server/src/grpc/policy.rs:3976) drops that key from the snapshot, and with it from non_secret_environment_keys. The emulator only reads classified non-secret values (crates/openshell-supervisor-network/src/google_cloud_metadata.rs:219), so /computeMetadata/v1/project/project-id returns 404 "not configured as non-secret".
GOOGLE_CLOUD_PROJECT is still injected with the provider's value. The workload ends up with GCP_PROJECT_ID=<caller>, GOOGLE_CLOUD_PROJECT=<provider>, and a metadata server that knows neither. Most SDKs read GOOGLE_CLOUD_PROJECT first, so the impact is mostly limited to clients that ask the metadata server for the project directly (e.g. Go metadata.ProjectID()). Your call on whether to address it. Options: keep provider values in the supervisor's non-secret map and drop them only from the child env, have the emulator fall back to the caller value, or document it.
Denylist for profile-declared env keys
valid_environment_key (crates/openshell-providers/src/profiles.rs:3237) checks only syntax and the revision namespace. That doesn't change the trust tier: profile import already requires workspace admin, and files[].env_var can already name arbitrary vars. But environment.fixed widens the possible values to things like NODE_OPTIONS=--require …, LD_PRELOAD, PATH, or OPENSHELL_*. Consider one denylist shared by environment.* and files[].env_var, covering at least:
OPENSHELL_*,LD_*,DYLD_*,PATH,BASH_ENV,ENVNODE_OPTIONS,PYTHONPATH,PYTHONSTARTUP- the proxy vars, the TLS trust vars, and
OTEL_EXPORTER_*
This could be a follow-up issue instead.
Smaller items
- The trailer on
ef4a2c4still says the gcp-metadata adapter is "currently unavailable". - The docs now tell users to copy chosen profiles into a separate directory instead of running
import --from providers, because imports are atomic and the gcp-metadata example fails on Windows. State that tradeoff in the 0.2.0 guide. GOOGLE_SERVICE_ACCOUNT_KEYis now withheld for every profile ID, not justgoogle-vertex-ai. A custom fork that relied on injecting it loses it, and the only trace is a gatewaywarn!. Mention this in the migration notes.
Move non-secret environment defaults and discovery keys into bounded profile declarations. Validate credential collisions and required platform adapters, and keep profile revision encoding deterministic. Preserve sandbox template and spec environment values, including empty values, over non-secret profile defaults in both launch paths. Keep provider credential placeholders authoritative. Allow identical non-secret values from attached providers while rejecting conflicting values and credential key collisions. Never expose GOOGLE_SERVICE_ACCOUNT_KEY through credentials or non-secret defaults, including older stored profiles. Omit stale private-key records without withholding remaining Vertex tokens or SDK configuration. Remove ID-selected adapters and orphaned GCP metadata helpers; document migration and compatible example imports. Validation: provider profile parsing, environment, and discovery tests passed. Closes NVIDIA#3442 BREAKING CHANGE: Google Cloud and Vertex profiles must declare their environment and discovery effects. Configure Vertex private key material through credential refresh instead of GOOGLE_SERVICE_ACCOUNT_KEY. Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Allow standard non-Windows E2E lanes to import profiles requiring the restored supervisor metadata adapter. Keep the GCP token-response tests and remove unused compiled environment catalogs now declared by profiles. Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
c009c37 to
b8cfb96
Compare
Summary
Make imported provider profiles authoritative for non-secret environment defaults and discovery. Preserve sandbox environment precedence and Vertex credentials during migration from the legacy service-account key. Configure Google SDK access to the existing supervisor metadata service through declarative profile settings.
Related Issue
Closes #3442.
Changes
f7273e4. Thegoogle-cloudprofile declares the metadata host, IP, and detection environment values used by Google SDK discovery. The service is available in Linux sandboxes using Docker, Podman, Kubernetes, or VM; Windows/MXC does not provide it. Profile import and attachment do not enable the service or validate its availability.GOOGLE_SERVICE_ACCOUNT_KEYas an injectable credential or non-secret default. Older stored provider records can retain it without withholding the access token and SDK configuration.Testing
mise run pre-commitpassed before the rebase; the final commit's hooks passed after the rebasemise run cipassed before the rebaseAfter the rebase, core and provider tests, focused gateway tests, supervisor-network GCP metadata tests, and the restored token-response tests passed.
mise run test:e2e-provider-profilespassed, as did all three Google SDK metadata-discovery cases in the Podman sandbox E2E suite. The metadata relay, supervisor handler, proxy, and core metadata protocol code match main; the imported profile configures SDK discovery for that existing runtime path.Checklist