fix(bundler): fix bugs - assemble full OCI artifact in path-based child apps; quote chart name (#1034)

[yuanchen8911]

Summary

Two argocd-helm OCI publishing bugs against the post-#1032 contract:

1. P1 — Path-based child Applications were resolving to a non-existent artifact because their spec.source.repoURL template never appended .Chart.Name. Under fix(bundler): derive argocd-helm chart name from OCI artifact path #1032's contract (--set repoURL=<parent namespace>), the parent App appends .Chart.Name via its separate source.chart field, but path-based children have no chart field — Argo CD's generic OCI source resolves repoURL directly. Bundles with manifest-only / mixed-local-chart children silently failed to sync.
2. P2 — writeChartYAML emitted name: and version: unquoted. Valid OCI artifact paths whose last segment is a YAML reserved scalar (null, true, false, yes, no, numeric strings) produced name: null etc., which Helm's YAML parser interprets as the non-string scalar, failing the chart with "chart.metadata.name is required".

Motivation / Context

Found via Codex review of origin/main at 51eb4b5f. P1 is a regression introduced today by #1032 (which landed the parent-App + bundle-chart-name fix for #1019 but left the path-based child template on the old --set repoURL=<full bundle URL> contract). P2 is a latent corner case that the same review surfaced.

Fixes: #1034
Related: #1032 (the parent-App contract change this PR completes), #1019 (original chart-name bug), #965 (DRA stale-NVML mitigation, unrelated but in the same OCI chain)

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Component(s) Affected

• Bundlers (pkg/bundler, pkg/component/*) — pkg/bundler/deployer/argocdhelm/argocdhelm.go

Implementation Notes

P1 — injectValuesIntoSingleSource (line 703). Append /{{ .Chart.Name }} to the rendered repoURL value so the assembled URL matches the artifact the parent's repoURL/chart:tag triple resolves to. Updated the required error message to reflect that the path-based template itself is doing the chart-name appending (the parent App also appends it, but via the separate source.chart field; the wording was misleading on the child).

P2 — writeChartYAML (line 944). Emit name and version via fmt.Fprintf with %q. OCI artifact path segments are constrained to printable ASCII by the docker reference grammar, which is exactly the safe charset for Go's %q (so the rendered YAML is always a clean double-quoted string).

Testing

unset GITLAB_TOKEN ; make qualify
# All 22 chainsaw tests pass.
# Coverage: 76.9% > 75% floor.
# Vulnerability scan: clean.

Tests added:

• TestInjectValuesIntoSingleSource_AppendsChartName — asserts the rendered child source.repoURL value is …/{{ .Chart.Name }} (with the original required directive intact).
• TestWriteChartYAML_QuotesYAMLReservedScalarsAsName — table-driven case across null, true, false, 123, yes, and a normal name. For each, writes Chart.yaml and asserts yaml.Unmarshal round-trips name back as the expected string.

Tests updated:

• TestHelmTemplate_RendersWithSetRepoURL — exercises the new contract (--set repoURL=oci://reg/org, parent namespace only) end-to-end via real helm template. Asserts the parent App's spec.source.repoURL equals the parent namespace and the path-based child's spec.source.repoURL equals parent-namespace + /aicr-bundle. This test was the canary: it was passing under the old contract (full bundle URL) because the parent App's source.chart was being ignored at install time; my fix surfaces the contract drift.
• TestGenerate_CustomChartName and the existing version: 2.5.0 assertion updated to expect quoted scalars.
• Golden fixtures (testdata/helm_and_manifest_only/ and testdata/mixed_component/) regenerated: child repoURL lines and Chart.yaml name/version lines now reflect the fix.

Risk Assessment

• Low — narrow scope, two-line code change plus assertion updates. No new external dependencies, no surface-area expansion. Bundles produced by this PR are correct for the new contract; bundles produced by the pre-fix(bundler): derive argocd-helm chart name from OCI artifact path #1032 codepath are unaffected (the child template change is additive — /{{ .Chart.Name }} only takes effect under the new --set repoURL=<parent namespace> contract).

Rollout notes: Anyone who picked up #1032's contract change since today (2026-05-26) needs this fix before publishing argocd-helm bundles with raw-manifest children to OCI. Existing bundles already pushed to OCI work as-is; the fix takes effect on the next aicr bundle --output oci://… run.

Checklist

• Tests pass locally (make test with -race) — via make qualify
• Linter passes (make lint) — via make qualify
• I did not skip/disable tests to make CI green
• I added/updated tests for new functionality
• I updated docs if user-facing behavior changed — no user-facing surface change; the error-message text inside the required directive is the only "doc" touched
• Changes follow existing patterns in the codebase
• Commits are cryptographically signed (git commit -S)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 53b4eb4d-c587-4d28-8b15-632f1ef02662

📥 Commits

Reviewing files that changed from the base of the PR and between 54709ad and 882d91a.

📒 Files selected for processing (6)

• pkg/bundler/deployer/argocdhelm/argocdhelm.go
• pkg/bundler/deployer/argocdhelm/argocdhelm_test.go
• pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yaml
• pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml
• pkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yaml
• pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml

---

📝 Walkthrough

Walkthrough

This PR fixes two correctness bugs in the argocd-helm bundler's OCI publishing path. First, it updates the templating logic for path-based child Applications to append .Chart.Name to the user-provided repoURL, ensuring they resolve to the correct OCI artifact path when the bundle is published. Second, it modifies the Chart.yaml generation to quote the name and version fields, preventing YAML reserved scalars from being misparsed by Helm. The changes include new unit tests validating both fixes and updates to existing tests and testdata to reflect the corrected behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• NVIDIA/aicr#1032: Introduced the path-based child Application .Chart.Name contract; this PR completes the implementation for the child Application template alongside Chart.yaml generation.

Suggested labels

size/L

Suggested reviewers

• mchmarny

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the two main bugs fixed: appending chart name to path-based child apps' OCI artifact URLs and quoting chart names in YAML output to prevent YAML reserved-scalar parsing errors.
Description check	✅ Passed	The description comprehensively explains both P1 and P2 bugs, their root causes, reproduction context, implementation details, testing approach, and risk assessment—all directly related to the changeset.
Linked Issues check	✅ Passed	The code changes fully address all acceptance criteria from #1034: injectValuesIntoSingleSource appends /{{ .Chart.Name }} for path-based children, writeChartYAML quotes name and version via %q, and tests verify both behaviors including YAML reserved-scalar handling.
Out of Scope Changes check	✅ Passed	All changes are narrowly scoped to the two bugs in #1034: the argocdhelm.go logic fix, test coverage, and golden fixture updates. No unrelated changes or scope creep detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[mchmarny]

Both fixes look correct and the test coverage is thorough.

P1 (path-based child repoURL): Append of /{{ .Chart.Name }} matches the artifact the parent App's repoURL/chart:tag triple resolves to (g.chartName() flows into both the parent's source.chart and the chart's Chart.yaml, so they can't drift). The updated required message accurately describes child-template behavior. The TestHelmTemplate_RendersWithSetRepoURL change is the right canary — it now exercises the new contract end-to-end via real helm template.

P2 (Chart.yaml quoting): %q is safe here because OCI artifact path segments are constrained to printable ASCII by the docker reference grammar, which is exactly Go's %q safe charset. Defense-in-depth quoting for version is also sound — Helm's semver validator would reject most reserved scalars downstream, but failing at Chart.yaml parse time with a clear YAML round-trip is friendlier than a Helm push error.

Two nits inline (trailing-slash robustness on the rendered template, and adding a version case to the reserved-scalar table) — neither blocks merge. All 22 chainsaw tests and the rest of CI are green.

[mchmarny]

nit: if a user passes --set repoURL=oci://reg/org/ (trailing slash, easy to copy-paste from a registry UI), this renders oci://reg/org//aicr-bundle — Argo CD will fail to sync with no obvious hint that the slash is the culprit. Cheap fix:

Value: `{{ required "..." .Values.repoURL | trimSuffix "/" }}/{{ .Chart.Name }}`

Same robustness for the parent App's source.chart/source.repoURL pair would be worth a follow-up if it doesn't already do this. Not a blocker.

[yuanchen8911]

Addressed in d8adac9 — the rendered value is now {{ required "..." .Values.repoURL | trimSuffix "/" }}/{{ .Chart.Name }}. So --set repoURL=oci://reg/org/ (with trailing slash) renders cleanly as oci://reg/org/aicr-bundle instead of the double-slash artifact path. Goldens regenerated.

Good catch on the parent App's source.chart / source.repoURL pair too — I'll file that as a follow-up issue since the parent template would need its own trimSuffix treatment and the same caveat applies (silent Argo-CD sync failure with no operator hint).

[mchmarny]

nit: the table covers the "name is a YAML reserved scalar" case but the same risk applies to version — a user-supplied version like "1.0" (no patch) renders as the YAML float 1 once Helm reparses, which is what makes version: "%q" defense-in-depth and not just cosmetic. Worth adding a case here (e.g., chartName: "aicr-bundle", version: "1.0") so the version branch of writeChartYAML is exercised by the same test that documents the rationale.

[yuanchen8911]

Addressed in d8adac9 — restructured the table to take an explicit (chartName, chartVersion) pair and added rows for float-looking ("1.0") and numeric-looking ("123") versions alongside the existing reserved-scalar name rows. The assertion now round-trips both fields through yaml.Unmarshal and checks each against its exact input string, so the version branch of writeChartYAML's %q wrap is exercised by the same test that documents the rationale.