feat(update-openshell): bumps to openshell v0.1.2 by markturansky · Pull Request #374 · openshift-online/hypershell

markturansky · 2026-09-28T17:57:28Z

Summary

The update-openshell skill filed needs-decision issues when it hit contract drift requiring a human call, but had no mechanism to detect and act on the human's reply in those issues
Adds Step 0 to the Workflow: before resolving the target version, scan all open needs-decision issues for non-bot comments; if a human provided a version directive, use it as the target and proceed with the normal triage/bump flow
On success, the issue is closed with a link to the resulting PR
Skipped entirely when $ARGUMENTS is non-empty (explicit target given on the CLI)

Test plan

Run /update-openshell with issue chore(deps): OpenShell v0.1.0 upstream contract drift needs a maintainer decision before bumping past v0.0.116 #366 open and having markturansky's reply — skill should pick up v0.1.2-rhaiv.0, run triage, and close the issue
Run /update-openshell v0.1.2-rhaiv.0 explicitly — Step 0 should be skipped, direct triage proceeds
Open issue with needs-decision but no human reply — Step 0 reports "waiting for human direction" and continues to latest

🤖 Generated with Claude Code

coderabbitai · 2026-09-28T17:57:38Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 76c97304-5783-4a85-a225-eda1fc4c8645

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

hypershell-delivery · 2026-09-28T18:01:03Z

Amber review

Status: Complete

View the submitted review.

- Remove contradictory $ARGUMENTS paragraph: CLI arg wins = if $ARGUMENTS is set, Step 0 is skipped entirely; the ambiguous "pending issue takes precedence" branch was unreachable and inverted the stated contract - Add format validation in Step 0c: extracted version must match ^v[0-9] before use; malformed input now skips the issue with a diagnostic message - No em dash present in this branch (check_forbidden_terms passes) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions · 2026-09-28T18:43:13Z

HyperShell environment destroyed

This ephemeral OpenShift environment has been destroyed. Comment /pr-extend to redeploy it.

- Remove contradictory $ARGUMENTS paragraph: CLI arg wins = if $ARGUMENTS is set, Step 0 is skipped entirely; the ambiguous "pending issue takes precedence" branch was unreachable and inverted the stated contract - Add format validation in Step 0c: extracted version must match ^v[0-9] before use; malformed input now skips the issue with a diagnostic message - No em dash present in this branch (check_forbidden_terms passes) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…decision issues When the update-openshell skill files a needs-decision issue and a human replies with direction (e.g. the target midstream version), the skill had no mechanism to detect and act on that reply. A subsequent run would ignore the open issue and start fresh. Step 0 scans for open needs-decision issues before resolving the target version. For each issue with a non-bot comment, it extracts the human's version directive and uses it as the target for the normal triage/bump flow. On success the issue is closed with a link to the resulting PR. If $ARGUMENTS is non-empty (explicit target given), Step 0 is skipped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

First minor-version midstream bump (0.0.116-rhaiv.6 -> 0.1.2-rhaiv.0). Triggered by Step 0: issue #366 had a human reply from markturansky indicating v0.1.2-rhaiv.0 as the current midstream target tracking upstream 0.1.2. Impact: mechanical pin sweep only. go build/vet/test and make check all pass. The breaking upstream changes (proto well-known time types #3113, policy L7 append #3380) are present in the commit range but did not require code changes to the HyperShell control-plane or API server at this version. Deferred: Agent Sandbox API version (v1.0.x / v1beta1 claim) is unverified against the live image - flag for next ROKS deploy. Files updated: - deploy/base/control-plane/deployment.yaml (source of truth) - deploy/base/platform-resources/controller.yaml (source of truth) - deploy/ibm/kustomization.yaml (IBM mirrored image paths) - OPENSHELL_VERSION (tag ref) - components/pr-test/e2e-openshell-roks.sh (ROKS e2e defaults) - specs/platform/openshell-gateway.spec.md (examples) - specs/platform/openshell-gateway-credentials.spec.md (examples) - specs/platform/data-model.spec.md (example) - specs/platform/openshell-image-auto-update.spec.md (example) - skills/deploy/ibm-cluster/SKILL.md (mirror commands) - skills/tooling/update-openshell/SKILL.md (learnings log) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Remove contradictory $ARGUMENTS paragraph: CLI arg wins = if $ARGUMENTS is set, Step 0 is skipped entirely; the ambiguous "pending issue takes precedence" branch was unreachable and inverted the stated contract - Add format validation in Step 0c: extracted version must match ^v[0-9] before use; malformed input now skips the issue with a diagnostic message - No em dash present in this branch (check_forbidden_terms passes) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… SC2 The learnings-log entry for v0.1.2-rhaiv.0 included a bare `curl -s https://quay.io/...` command. SkillSpector SC2 flags any skill instruction that fetches from an external HTTPS URL as high-severity External Script Fetching. Replace with a skopeo inspect invocation (local binary) plus a prose note about the Quay tag API for when skopeo is absent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The control plane Dockerfile clones and bundles the upstream Helm chart at the tag specified in OPENSHELL_VERSION. When OPENSHELL_VERSION is bumped (e.g. v0.1.2-rhaiv.0) the bundled chart changes, but the component-detection script did not list OPENSHELL_VERSION as a watched path for the control_plane component, so CI reused the stale pre-built image containing the old chart. This caused E2E failures: the old chart generated a config with app_armor_profile in [openshell.drivers.kubernetes], which the v0.1.2 gateway binary rejects as an unknown field. Adding OPENSHELL_VERSION to the control_plane paths ensures that any version bump triggers a control plane image rebuild with the correctly-tagged chart bundled inside. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The pre-v0.1.2 Helm chart (v0.0.116-rhaiv.6) defaults server.appArmorProfile to "Unconfined" and renders app_armor_profile into the gateway TOML config. The v0.1.2 gateway binary removed that config field and rejects it with a startup error, causing CrashLoopBackOff. CI E2E tests use the Konflux image built from main at the PR merge base (v0.0.116 chart bundled), while the deployment manifests reference v0.1.2 gateway images. This chart/binary mismatch causes the failure. Setting server.appArmorProfile to "" in ValuesBuilder suppresses the field (Helm if-block is falsy on empty string). The v0.1.2+ chart does not reference this key at all, so the override is a no-op going forward. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…file The Dockerfile previously ran git clone at build time to download the OpenShell Helm chart from GitHub. This breaks Konflux PR builds because the build-container task runs without outbound network access (network isolation is applied after the prefetch-dependencies step). Replace the runtime git clone with a committed chart source tree under charts/openshell/ (vendored from v0.1.2-rhaiv.0). The Dockerfile now runs helm package on the local copy, which requires no network access. When bumping OPENSHELL_VERSION in future, also update charts/openshell/ by extracting the chart from the new upstream tag: git clone --depth 1 --branch "${OPENSHELL_TAG}" "${OPENSHELL_CHART_REPO}" /tmp/openshell rm -rf charts/openshell && cp -r /tmp/openshell/deploy/helm/openshell charts/openshell Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

….1.2 The v0.1.2 chart reorganized image configuration from top-level image.* to gateway.image.{registry,repository,tag} and applies global.image.registry (ghcr.io/nvidia) as a prefix when per-image registry is unset. The old code set image.repository/tag which the new chart ignores, causing certgen to pull the default chart image (not available in Kind) and time out with DeadlineExceeded. Fix: split fully-qualified image references into registry+repo+tag using the Docker convention (first path component is a registry if it contains a dot or colon), then set gateway.image.registry explicitly to bypass the global prefix. Apply the same fix to supervisor.image. Additionally replace em dashes in vendored charts/openshell/ files with hyphens to satisfy the repository's check-forbidden-terms policy (pre-commit hook rejects U+2014 in committed text files). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…issions The v0.1.2 OpenShell Helm chart creates a ClusterRole that includes node.k8s.io/runtimeclasses:get and scheduling.k8s.io/priorityclasses:get. Kubernetes rejects ClusterRole creation when the creating subject does not itself hold every permission being granted (RBAC escalation prevention). The hypershell-controller ServiceAccount was missing these two permissions, causing helm install to fail with: clusterroles "openshell-gateway-node-reader-*" is forbidden: user "system:serviceaccount:hypershell-system:hypershell-controller" is attempting to grant RBAC permissions not currently held Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The v0.1.2 OpenShell Helm chart creates a sandbox Role that includes pods:create, pods:delete, and pods:patch. The hypershell-controller ServiceAccount only had pods:get,list,watch, which triggers Kubernetes RBAC escalation prevention when the controller installs the chart: roles "openshell-gateway-sandbox" is forbidden: user "system:serviceaccount:hypershell-system:hypershell-controller" is attempting to grant RBAC permissions not currently held Audited all four RBAC resources the chart creates (node-reader ClusterRole, peer Role, sandbox Role, certgen Role hook) against the controller's ClusterRole; pods is the only remaining gap after the prior commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…atibility The console dashboard image was not updated during the v0.1.2 bump. v0.1.2 introduced breaking proto changes (well-known time types #3113, policy L7 append #3380) that the old sha-978bcb5 console BFF cannot parse, causing the sandbox list gRPC call to hang indefinitely and the UI to show a loading spinner that never resolves. Update to sha-71335e5 (built 2026-09-23) which includes v0.1.2 compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…AC lessons from v0.1.2 Four gaps in the skill discovered during the v0.1.2-rhaiv.0 bump: - Step 3a: vendor the Helm chart from the upstream tag on every bump. The Dockerfile packages charts/openshell/ at build time (Konflux hermetic builds have no outbound network access); a stale chart silently applies the old RBAC rules and value schema even after the image tag is bumped. Includes the em-dash strip required by the pre-commit hook. - Step 3b: check the console image when proto surfaces change. A stale console BFF (quay.io/gkrumbach07/openshell-dashboard) cannot parse breaking proto changes and leaves the sandbox list UI in an infinite loading state with no visible error. Documents the Quay tag lookup, the two files that must agree (OPENSHELL_VERSION + config.go), and the symptom fingerprint. - Version footprint: add OPENSHELL_VERSION, charts/openshell/, and config.go (defaultConsoleImage) to the table. - Contract surfaces: extend the gRPC/proto row to note that the console BFF is also a gRPC client and must be checked alongside the proto. Learnings log updated with: chart vendoring requirement, chart RBAC escalation pattern (runtimeclasses/priorityclasses/pods), console image incompatibility with proto-breaking bumps, and appArmorProfile Kind issue. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…updates Automates the manual chart vendoring step required on every OpenShell version bump. The target reads OPENSHELL_TAG and OPENSHELL_CHART_REPO from OPENSHELL_VERSION, clones the upstream tag at depth 1, replaces charts/openshell/ wholesale, and strips em-dashes that the pre-commit hook rejects. Update the update-openshell skill Step 3a to call `make vendor-openshell-chart` instead of documenting the raw git clone/cp/sed sequence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Address Amber review findings: - global-architecture.spec.md: update the v1beta1 API version note from gateway 0.0.109 to 0.1.2-rhaiv.0 (confirmed by Kind E2E sandbox ops) - deploy-cluster/SKILL.md: refresh example env var values to v0.1.2-rhaiv.0 - gcp-cluster/SKILL.md: update active example commands to v0.1.2-rhaiv.0 (reference run sections at the old tag are historical records, left intact) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… console image docs - openshell-gateway-helm-adoption.spec.md: update the SHALL to reflect the vendored chart model - OPENSHELL_CHART_REPO/OPENSHELL_TAG are now provenance/re-vendor pointers consumed by make vendor-openshell-chart, not build-time git clone inputs to the Dockerfile - helm/values.go: drop the server.appArmorProfile suppression; the vendored v0.1.2 chart carries no appArmorProfile key (grep returns zero hits), making the override a confirmed no-op against the committed chart - e2e-console-browser-testing.spec.md: refresh pinned console image refs from sha-978bcb5 / sha256:c69c1f34... to sha-71335e5 / sha256:1d363311... to match config.go after the v0.1.2 console image bump - openshell-gateway.spec.md: remove app_armor_profile = "Unconfined" from the rendered gateway config example; the v0.1.2 gateway binary no longer accepts this TOML field Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

hypershell-delivery

Verdict

This head (9a26bdb) closes every previously open finding: the chart-sourcing Requirement now describes the vendored model, the appArmorProfile suppression is gone, and the console/gateway spec examples match the code. One new latent correctness nit remains in the image-key migration (digest handling), and the PR still bundles several independent runtime/CI changes, so I am landing on COMMENT rather than a clean approve.

Findings

[Minor] splitImageRefFull drops the digest and substitutes a tag for digest-pinned gateway/supervisor images. components/control-plane/internal/helm/values.go L303-329 calls splitImageRef to derive the tag; for a reference like quay.io/opendatahub/odh-openshell-gateway@sha256:<hex>, splitImageRef (L285-295) strips the @... and then returns tag="latest" (no tag colon after the last slash), so buildCoreValues sets gateway.image.tag=latest with no digest. The v0.1.2 chart natively supports digests (charts/openshell/templates/_helpers.tpl L155-156 renders repository@digest when image.digest is set) and ValidateImageReference explicitly accepts @sha256: refs (components/control-plane/internal/gateway/validation.go L17), so an operator who pins a GatewayRelease image by digest would silently get :latest instead of the pinned image. This is pre-existing behavior in splitImageRef (unchanged from main) that the new function carries forward, and the shipped defaults are tag-based (deploy/base/.../deployment.yaml uses v0.1.2-rhaiv.0), so impact is limited today. Since this migration is the right place to fix it, consider detecting @sha256: and setting gateway.image.digest (and clearing tag). Confidence: Medium.

[Minor] Scope bundling. This PR pairs a skill/doc feature (update-openshell Step 0) with a functional OpenShell version bump, a large chart-vendoring commit, a control-plane image-key migration, RBAC widening, a console image bump, and a new Makefile target. Commits are separate and conventionally messaged, but the functional portions each carry independent runtime/CI risk and deserve e2e sign-off.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.

Previous concerns

[Critical] Em dash (U+2014) in skills/tooling/update-openshell/SKILL.md (discussion): Addressed. Step 0b reads "skip this issue - ..."; no U+2014 in the skill or under charts/openshell/ (grep -rlP returns nothing).
[Major] Contradictory $ARGUMENTS precedence in Step 0 (discussion): Addressed. Step 0 states a single rule ("Skip this step if $ARGUMENTS is non-empty"); the contradictory branch is gone.
[Minor] Version parsed from untrusted issue comment without validation (discussion): Addressed. Step 0c validates the extracted version against ^v[0-9] before use.
[Critical] Em dashes in the vendored chart fail make check (discussion): Addressed. No U+2014 under charts/openshell/; scripts/check_forbidden_terms.py runs clean.
[Major] update-openshell workflow omits the re-vendor step (discussion): Addressed. make vendor-openshell-chart exists (Makefile L242-243) and the skill/spec make re-vendoring canonical.
[Major] Unverified Agent Sandbox API compatibility (discussion): Addressed (spec line). specs/platform/global-architecture.spec.md L894 pins v1beta1 to gateway 0.1.2-rhaiv.0, consistent with the vendored chart preflight. Runtime interop still relies on the e2e gate, which a static review cannot verify.
[Major] Dockerfile no longer clones the chart, contradicting the normative SHALL (discussion): Addressed. specs/platform/openshell-gateway-helm-adoption.spec.md L139-141 now describes the vendored model and frames OPENSHELL_CHART_REPO/OPENSHELL_TAG as provenance/re-vendor coordinates, matching the Dockerfile COPY charts/openshell + helm package flow.
[Minor] No unit test for the appArmorProfile suppression / no-op against the vendored chart (discussion): Addressed. The suppression was removed entirely; grep -rni apparmor returns nothing in components/control-plane/internal/helm/values.go or under charts/openshell/.
[Minor] Console image bump leaves stale pinned docs (discussion): Addressed. specs/platform/e2e-console-browser-testing.spec.md L650/L766 now show sha-71335e5 / sha256:1d363311..., matching config.go L30.
[Minor] openshell-gateway.spec.md documents the now-suppressed app_armor_profile: Addressed. grep app_armor_profile specs/platform/openshell-gateway.spec.md returns nothing.
[Minor] Deploy-skill image examples still at old version: Addressed. deploy-cluster, gcp-cluster, and ibm-cluster skills show v0.1.2-rhaiv.0.
[Minor] Skill feature bundled with a functional bump: Still present.

The committer addressed the previous substantive concerns; only the pre-existing scope-bundling note carries over, plus one new Minor nit.

Findings Summary (ordered by severity, highest first)

[Minor] splitImageRefFull drops the digest and emits tag=latest for digest-pinned gateway/supervisor images - Correctness (values.go L303-329)
[Minor] Skill feature bundled with functional bump, chart vendoring, RBAC, console bump, and Makefile target - Change Scope

Convention Checklist

Convention	Result
No em dashes (U+2014) / forbidden-terms gate	Pass
No `panic()` in production code	Pass
Errors wrapped / no secret leakage	Pass
SecurityContext / no security regression	Pass
RBAC grants are minimal and match the chart rules	Pass
Hermetic build (no build-time network)	Pass
Re-vendor step is canonical in the workflow	Pass
Image references match across the stack (deploy manifests, gateway/global-arch/console specs)	Pass
Test change is a strengthened assertion, not a weakened guarantee	Pass
Spec matches code (chart sourcing)	Pass
Conventional commit messages	Pass
Image reference handles digest-pinned inputs	Fail

hypershell-delivery · 2026-09-29T18:57:55Z

+// prefix when the per-image registry is unset. By extracting the registry here
+// and setting it explicitly, the chart's global prefix is suppressed.
+func splitImageRefFull(image string) (registry, repo, tag string) {
+	_, tag = splitImageRef(image)


[Minor] splitImageRefFull derives the tag via splitImageRef, which strips the @sha256:... digest and then returns tag="latest" when no tag colon remains. For a digest-pinned GatewayRelease image (ValidateImageReference accepts @sha256: refs), this sets gateway.image.tag=latest and never sets gateway.image.digest, so the chart renders registry/repository:latest instead of the pinned digest (the chart supports image.digest in _helpers.tpl L155-156). This is pre-existing behavior in splitImageRef carried into the new function, and shipped defaults are tag-based, so impact is limited today. Since this migration owns this code now, consider detecting @sha256: here and returning it as a digest so buildCoreValues can set gateway.image.digest (and leave tag empty). Confidence: Medium.