Repository navigation
fix(helm): reject uppercase demo image repositories - #3037
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThe demo Helm chart rejects uppercase image repository paths while preserving registry-host and tag casing. Example: Set images.prometheus to docker.io/Prometheus@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: current main still accepts unusable uppercase demo repositories, and the focused patch introduces no actionable correctness or security defect. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe optional observability demo chart consumes operator values and renders Prometheus, Grafana, and Loki resources; demo.validate checks image overrides before Helm emits manifests. flowchart TD
A[Operator image overrides] --> B[Demo chart validation]
B --> C[Repository casing error]
B --> D[Rendered Deployments]
D --> E[Kubernetes image handling]
Technical reviewBest possible solution: Keep the focused guard in demo.validate with native Helm regression coverage and unchanged valid image values. Do we have a high-confidence way to reproduce the issue? Current main's digest-only guard accepts docker.io/Prometheus with a valid digest, and the supplied parent/head transcript demonstrates the failure path; this review ran no target code. Is this the best way to solve the issue? The patch addresses the omitted demo consumer at its existing validation boundary while preserving the broader reference shape and valid casing controls. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 0c73a86a6cdd. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 22, 2026, 10:34 AM ET / 14:34 UTC (Revision 2). |
|
Free's clanker here. I checked the proof requested in the October 20 review. Eric added the terminal transcript after that review. The PR description now shows a parent-chart install at revision 1, an exact-head upgrade to revision 2 with I independently compared parent The independent comparison was render-only; the upgrade evidence is Eric’s. No code change or rebase was needed to produce it. |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
I checked the Helm validator against Docker’s reference parser. With the same digest, docker.io/Team/prometheus now fails at render time; Team/prometheus still works because Docker treats the first component as a registry. Mixed-case tags, registry ports, and bracketed IPv6 kept their spelling. The 18 cases passed on the proposed validator. This was a render test; I didn’t pull images or start workloads. I found no issue for the author to fix.
|
Free's clanker here. I checked the merge with main at The hosted chart test ran on the same tree as this head: 12 passed, none skipped, including the repository-spelling case. CI Required also passed. I found no code change to request. The published Claw verdict still names the previous commit; this result is for the new head and doesn’t replace maintainer review. |
|
Free's clanker here (an AI agent working on Free's behalf). Landed with the owner's OK after a review pass. The render-time guard follows Docker's registry-host rule, and CI Required plus First Agent Smoke were green on this head. Thanks! |
October 23 conflict refresh
Normally merged pinned upstream
e1788c775b168d1e0839dc6bc38242894e6d586eas6ff876986e95575124ca7858f42acccab2126e47, preserving both parents. The actual conflict was only the logging flow's metadata/changelog. Upstream prose, both histories, the repository-casing paragraph and Manual Notes are retained. The helper and owning test Git blobs are identical to their accepted83c8542aversions; the original runtime/upgrade transcripts below remain evidence for that unchanged behavior and are not represented as new executions.The merged candidate passed the actual owning Helm suite 11/11, zero skips; flow validation, official word count (2,409 total words), scoped lint, format and diff checks passed. Docs validation passed 290 pages/4,560 links; specs validation passed 96 specs/744 local links, with the same three historical external links unverified. No new deployment or image-pull proof was needed or performed. The normal full-repository pre-push formatting hook passed.
The bounded three-file independent review reported no P0–P2 findings (0.91), with a context limit: its prompt corrected an earlier test-count typo but incorrectly inferred that the helper/test full blobs differed. Exact Git comparison verified both blobs are identical; that fact was checked separately, not by a new corrected review. An additional canonical call was rejected as duplicate and did not execute; no retry or bypass followed. These limitations are retained rather than treating an unexecuted review as completed.
Current-head hosted CI and natural review reassessment are pending after the push/body update. Prior
83c8542agreen checks and reviews remain historical; this update does not claim maintainer approval or merge.Problem
The observability demo chart's image validation accepts uppercase repository paths. For example, a digest-pinned
docker.io/Prometheusreference renders a Deployment, but kubelet refuses it withInvalidImageNamebefore pulling the image. This leaves the operator with a failed demo workload instead of a render-time explanation.This extends the existing image validation from #1484 while retaining the intentionally loose reference-shape and IPv6 compatibility discussed in #2363.
Change
Reject uppercase repository components in each demo image before rendering resources. Keep uppercase registry hosts and tags, IPv6 registry hosts, and the existing SHA-256 spelling rule. Add actual Helm regression cases and document the render-time boundary in the owning flow. No image versions, defaults or production chart behavior change.
Verification
At parent
d331d24fdddc269c12900327733be0ae1ffc441e, realhelm templateaccepted an uppercase repository. Applied the chart's actual Prometheus Deployment, ConfigMap and ServiceAccount to a disposable K3s v1.35.5/containerd node: the Pod reachedInvalidImageName, and kubelet reported the repository must be lowercase. Removed the task-owned fixture afterward. No application image pull was required for this refusal.At exact head
83c8542a3d368495a72281d19ccac693d24aa690:node --test tests/integration/observability-demo-chart.test.mjs: 11 passed, zero failed or skipped.git diff --check: passed.node scripts/docs-site/word-count.mjs,node scripts/docs-site/build.mjs --check, andnode scripts/check-specs.mjs: passed; 258 pages and 4,153 links validated; owning flow 2,228 words. Three historical external artifact links remain unverified by the spec check.Local checks use already prepared exact-version dependencies; no dependency installation or lockfile change. The evidence establishes render-time rejection and the parent kubelet refusal, not a successful full observability deployment, image pull or production acceptance. Current exact-head Required, First Agent Smoke and Demo Observability Smoke are successful. Hosted review found no source defects and requested an inspectable behavior transcript; the actual terminal evidence below addresses that request. Reassessment is pending, so this remains Draft.
Risks
Previously accepted uppercase repository overrides now fail early because kubelet cannot use them. Registry and tag casing remains unchanged. The guard deliberately preserves the existing loose shape rule rather than introducing a complete image-reference parser.
Inspectable terminal behavior proof (exact head, 2026-10-20)
These are captured outputs, not anticipated results. Parent chart was extracted with
git archive d331d24fdddc269c12900327733be0ae1ffc441e deploy/helm/openclaw-observability-demo;$HEAD_CHARTis the chart at83c8542a3d368495a72281d19ccac693d24aa690. Helm reportsv4.3.0+gbec5b06. Only private filesystem paths, kubeconfig location and synthetic IP literals are replaced below;.invalidis a reserved synthetic registry, and all-adigests are deliberate test data.Exact parent/head rendering transcript
For each result row, the standalone runner actually invokes the following command against both extracted parent and exact-head charts, captures the exit code and stdout/stderr, and parses accepted Deployment images with
yq:The runner asserts exit 0 and exact image equality for every accepted output. For every rejected output it asserts exit 1, a field-specific lowercase-repository error and completely empty stdout (no resources emitted). Representative captured stderr from the head:
Complete captured case results (12 negative cases and 9 preserved controls):
{"name":"prometheus","image":"docker.io/Team/prometheus@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"prometheus","image":"Registry.Example.invalid/team/PROMETHEUS:release_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"prometheus","image":"[<synthetic-IPv6-redacted>]:5000/team/PROMETHEUS@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"prometheus","image":"PROMETHEUS@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"prometheus","image":"Registry.Example.invalid/team/prometheus:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"prometheus","image":"[<synthetic-IPv6-redacted>]:5000/team/prometheus:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"prometheus","image":"prometheus:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"grafana","image":"docker.io/Team/grafana@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"grafana","image":"Registry.Example.invalid/team/GRAFANA:release_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"grafana","image":"[<synthetic-IPv6-redacted>]:5000/team/GRAFANA@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"grafana","image":"GRAFANA@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"grafana","image":"Registry.Example.invalid/team/grafana:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"grafana","image":"[<synthetic-IPv6-redacted>]:5000/team/grafana:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"grafana","image":"grafana:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"loki","image":"docker.io/Team/loki@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"loki","image":"Registry.Example.invalid/team/LOKI:release_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"loki","image":"[<synthetic-IPv6-redacted>]:5000/team/LOKI@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"loki","image":"LOKI@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":1,"noDeployment":true} {"name":"loki","image":"Registry.Example.invalid/team/loki:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"loki","image":"[<synthetic-IPv6-redacted>]:5000/team/loki:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} {"name":"loki","image":"loki:RELEASE_1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","parent":0,"head":0,"preserved":true} PASS 12 parent-accepted/head-refused uppercase repositories, 9 host/tag/IPv6 controls preserve image spellingReal Helm upgrade retaining existing valid values
A separate owned disposable server-only K3s v1.35.5 API stored a real Helm release. It had no worker node, so no application image pulls, scheduling or startup can be inferred. The synthetic OCC namespace was created first.
$VALID_VALUESsupplied required namespace/release/CIDR/admin-Secret-name fields and these three custom image overrides shown in the output. They were installed with the parent chart, then upgraded using the exact-head chart with--reuse-valuesand no replacement values file. Neither operation used--wait.The harness ran
helm get values demo -n oce-packaging-upgrade -o jsonbefore and after and asserted deep equality. It also ranhelm get manifestafter upgrade and asserted all three exact custom image references remained. Actual transcript:This demonstrates the existing Helm release upgrade and value-retention path, not a working Prometheus/Grafana/Loki deployment. The prior node-backed parent
InvalidImageNameobservation is separate evidence. Initial fixture setup omitted the synthetic OCC namespace, so its first parent install failed with a missing-namespace error; that fixture was cleaned, the namespace prerequisite was supplied, and the shown rerun passed. No source change was needed.After the successful run the harness inspected the exact task labels, removed only its own container and network, deleted its private kubeconfig, and confirmed the matching container/network inventory was empty. Other existing containers were untouched.