fix(helm): correct adminPwd secret key in NOTES.txt + add chart-publish workflow

[lyj7890]

Summary

Two fixes identified during TKE Serverless deploy testing (reported by @guosong).

---

Fix 1 — NOTES.txt adminPwd key mismatch (P0 usability bug)

Problem

NOTES.txt instructed users to retrieve the initial superAdmin password with:

kubectl get secret octo-secrets -o jsonpath='{.data.adminPwd}' | base64 -d

This returns an empty string. The actual key stored by secret.yaml is OCTO_ADMIN_PWD, not adminPwd.

Fix

Updated NOTES.txt to use the correct key name and show the full kubectl command with namespace:

kubectl get secret -n <namespace> <release>-secrets   -o jsonpath='{.data.OCTO_ADMIN_PWD}' | base64 -d

---

Fix 2 — Add chart-publish.yml workflow

Problem

The chart was merged in PR #68 but no publish pipeline existed. Installing via OCI:

helm install octo oci://ghcr.io/mininglamp-oss/octo --version 0.2.4

failed with 403 because the package was never pushed to ghcr.io.

Fix

Added .github/workflows/chart-publish.yml that:

• Triggers on GitHub Release publish (automatic) or workflow_dispatch (for backfills)
• Verifies Chart.yaml version matches the release tag before pushing
• Logs in to ghcr.io with GITHUB_TOKEN (no extra secrets needed, uses packages: write permission)
• Runs helm package + helm push oci://ghcr.io/mininglamp-oss
• Smoke-tests the push with helm show chart

After merge, a maintainer can trigger the workflow manually with version=0.2.4 to backfill the current chart.

---

Testing

• Fix 1: verified secret.yaml stores key as OCTO_ADMIN_PWD (line 24)
• Fix 2: workflow logic follows same pattern as existing release-publish.yml; actual push requires packages: write which Actions GITHUB_TOKEN provides for org repos

Refs: TKE Serverless deploy notes 2026-05-22

[Jerry-Xin]

The PR is relevant to octo-deployment, but the new chart publish workflow does not reliably integrate with the repository’s existing release flow.

🔴 Blocking

🔴 Critical — .github/workflows/chart-publish.yml:3-6: the workflow depends on release.published to auto-publish charts, but this repo’s release path is .github/workflows/release-publish.yml, which delegates release creation from a GitHub Actions workflow using the repository token permissions. GitHub does not create new workflow runs for most events triggered by GITHUB_TOKEN; only workflow_dispatch and repository_dispatch are exceptions. That means releases published by the existing release-publish.yml path are not expected to trigger this workflow automatically, so future chart releases can still fail to be pushed unless maintainers remember to run the manual dispatch. (docs.github.com)
Suggested fix: call chart publishing from the release workflow itself, trigger this workflow via workflow_dispatch/repository_dispatch, or publish with a GitHub App/PAT-backed release event if that is the intended architecture.

💬 Non-blocking

🟡 Warning — .github/workflows/chart-publish.yml:10-11: the workflow_dispatch input says the version “Must match helm/octo/Chart.yaml appVersion”, but the workflow actually checks Chart.yaml version at lines 45-54. This should say version, not appVersion.

🟡 Warning — .github/workflows/chart-publish.yml:36-49 and .github/workflows/chart-publish.yml:72-81: the manually supplied version is interpolated directly into shell scripts. Since dispatch inputs are maintainer-controlled this is not a public exploit path, but it is still safer to pass it through env: and quote shell references consistently.

✅ Highlights

🔵 Suggestion — helm/octo/templates/NOTES.txt:39-43: the secret key fix is correct. secret.yaml stores OCTO_ADMIN_PWD, and the rendered secret name matches {{ include "octo.fullname" . }}-secrets.

Validation note: I attempted helm lint and helm template, but helm is not installed in this review environment. git diff --check main...HEAD passed.

[yujiawei]

Code Review — PR #83 (octo-deployment)

Summary

Two small, well-scoped fixes:

1. NOTES.txt — corrects the documented secret key for the initial superAdmin password (adminPwd → OCTO_ADMIN_PWD).
2. .github/workflows/chart-publish.yml — new workflow to package and push the Helm chart to oci://ghcr.io/mininglamp-oss on release publish (or manual dispatch).

No production code changes; no security regressions identified. Approving with a few suggestions to consider in a follow-up.

---

1. Verification

Item	Status	Evidence
secret.yaml stores the key as OCTO_ADMIN_PWD (not adminPwd)	✅	helm/octo/templates/secret.yaml:24 — OCTO_ADMIN_PWD: {{ .Values.secrets.adminPwd | default "" | quote }}
NOTES.txt uses correct key name	✅	helm/octo/templates/NOTES.txt:43
{{ include "octo.fullname" . }}-secrets matches the actual Secret name	✅	helm/octo/templates/_helpers.tpl:122-124 defines octo.secretName as printf "%s-secrets" (include "octo.fullname" .), which is what the rendered kubectl command will resolve to
Workflow action SHAs pinned with version comments	✅	actions/checkout@11bd71… (v4.2.2), azure/setup-helm@b9e51… (v4.3.0)
Workflow permissions minimally scoped	✅	contents: read + packages: write only
set -euo pipefail in shell steps	✅	All run: blocks
GITHUB_TOKEN piped via --password-stdin, never echoed	✅	chart-publish.yml:56-61
Version cross-check before push	✅	chart-publish.yml:45-54

---

2. Findings

P0 / P1

None.

P2 — suggestions (non-blocking)

S1. NOTES.txt should reuse the octo.secretName helper

helm/octo/templates/NOTES.txt:42 writes {{ include "octo.fullname" . }}-secrets, duplicating the formula from _helpers.tpl:122-124. If the secret naming convention ever changes (e.g. adds a suffix variant), this line will silently drift. Prefer:

kubectl get secret -n {{ .Release.Namespace }} {{ include "octo.secretName" . }} \
  -o jsonpath='{.data.OCTO_ADMIN_PWD}' | base64 -d

S2. workflow_dispatch backfill only works while Chart.yaml.version equals the requested version

The "Verify Chart.yaml version matches" step (chart-publish.yml:45-54) reads Chart.yaml from whatever ref actions/checkout resolved — which for workflow_dispatch is the default branch (main). That means:

• Backfilling the current chart version works (today: 0.2.4 ✅).
• Once Chart.yaml is bumped to 0.2.5 on main, a maintainer can no longer dispatch a backfill of 0.2.4 from this workflow — the version check will fail.

Two options if you want true backfill:

• Add a ref input to workflow_dispatch and pass with: { ref: ${{ inputs.ref }} } to actions/checkout, or
• Resolve the version from a tag instead of an input when a tag is supplied.

If backfill-of-current-only is the intended scope, please document it in the input description so a future maintainer doesn't get burned.

S3. helm push to OCI overwrites existing tags by default

ghcr.io OCI tags are mutable. A double-trigger (release republish + a manual dispatch) would silently overwrite a previously-published chart of the same version. Consider either:

• Guarding with a helm show chart oci://… --version <v> probe before push and failing if it already exists (matching the spirit of an "immutable release"), or
• Adding a force boolean input gated behind an explicit opt-in.

S4. Missing concurrency: group

Add to prevent overlapping runs of the same version if two events fire near-simultaneously:

concurrency:
  group: chart-publish-${{ github.event.release.tag_name || inputs.version }}
  cancel-in-progress: false

S5. Brittle Chart.yaml parser

grep '^version:' helm/octo/Chart.yaml | awk '{print $2}' (chart-publish.yml:48) works today because the current version: 0.2.4 is unquoted. If anyone ever quotes it (version: "0.2.4") or adds a comment on the same line, the captured string will include the quotes/comment and the equality check will fail with a confusing message. yq (already a common helm-CI tool) would be more robust:

CHART_VERSION="$(yq '.version' helm/octo/Chart.yaml)"

S6. (Optional) Consider provenance / cosign signing

The reusable release-publish flow already exists; chart publishing is a good place to layer in cosign sign for the OCI chart artifact in a future PR. Not in scope here.

---

3. Security review (PR was flagged needs-human-review)

• No new secret material is introduced in code; the workflow consumes the built-in GITHUB_TOKEN only.

• packages: write is required for ghcr.io chart push and is the correct minimal scope.

• No pull_request_target, no untrusted-input run: interpolation that could enable script injection. inputs.version flows into a shell variable (VERSION="${{ inputs.version }}"); since this is workflow_dispatch only triggerable by repo maintainers, the attack surface is acceptable, but a future hardening pass could quote-then-printenv the inputs via env: mapping to fully neutralise injection vectors:

  - env:
      VERSION_INPUT: ${{ inputs.version }}
    run: |
      VERSION="${VERSION_INPUT#v}"
      …

• The NOTES.txt change is a docs-only fix and discloses no new sensitive info; the password it points to was already user-supplied via secrets.adminPwd.

Suggest a human verify (per the security_sensitive classification) that:

1. The ghcr package settings for mininglamp-oss/octo allow GITHUB_TOKEN writes from this repo (org Package settings → "Manage Actions access"). If not pre-configured, the first push will 403 even with packages: write granted at the workflow level.
2. There's no organisational requirement to sign chart artifacts before publish (cosign / sigstore policy).

---

4. Additional observations (out of scope)

• release-publish.yml runs as a workflow_dispatch only and calls a centralised reusable workflow. The new chart-publish.yml auto-fires on release: published, which is the right linkage point — together they form a clean two-step (manual release → automatic chart push). Worth a one-line comment in release-publish.yml cross-referencing chart-publish.yml so future maintainers see the chain.
• Chart.yaml appVersion: "latest" is a separate quality issue: it prevents downstream consumers from pinning to a concrete server image release. Not introduced by this PR, but flagging while we're here.

---

Verdict

Approve. The two fixes are correct, minimal, and address real reported issues. Suggestions above are quality-of-life and supply-chain hardening, none of which block merge.

[Jerry-Xin]

Relevant to this repository, but the PR leaves the YAML CI job broken.

🔴 Blocking

• 🔴 Critical — .yamllint is not used by CI. The repo’s CI still runs yamllint with an inline config at .github/workflows/ci.yml:49, so the new ignore rule in .yamllint:2 is bypassed. I reproduced the CI command locally and it still lints Helm templates, failing with syntax errors such as helm/octo/templates/statefulset-redis.yaml:1 and helm/octo/templates/secret.yaml:5. Update CI to use the repository config, e.g. yamllint -f github ., or add the helm/octo/templates/ ignore to the inline -d config.

💬 Non-blocking

• 🔵 Suggestion — .github/workflows/chart-publish.yml:11 says the manual version must match Chart.yaml appVersion, but the workflow checks Chart.yaml version at lines 45-54. The logic is correct for chart publishing; the input description should say version.

✅ Highlights

• The NOTES change correctly uses OCTO_ADMIN_PWD, matching helm/octo/templates/secret.yaml:24.
• The secret name in helm/octo/templates/NOTES.txt:42 matches the helper-defined <fullname>-secrets convention in helm/octo/templates/_helpers.tpl:122.

[lml2468]

Verdict

CHANGES_REQUESTED — The NOTES.txt fix is correct, but the chart-publish workflow has an architectural trigger issue (confirmed independently from the existing review), and CI is red due to a missing rebase.

Blocking Items

1. CI RED — yamllint still scans Helm templates (.github/workflows/ci.yml not updated)

The PR adds .yamllint with the correct ignore: | block, but the CI workflow (.github/workflows/ci.yml) on this branch still uses the old inline -d config that contains the broken \n escape. yamllint ignores .yamllint when -d is passed explicitly.

PR #82 (still open) fixes ci.yml to use yamllint -f github . (which reads .yamllint). This PR should be rebased on top of PR #82 once it merges — otherwise CI will remain red and the .yamllint file in this PR is dead code.

Additionally, both PRs add an identical .yamllint file, which will cause a merge conflict (or at minimum a confusing add/add resolution). After rebase, this PR's .yamllint addition should be dropped since PR #82 already provides it.

2. release.published auto-trigger will not fire (chart-publish.yml:3-6)

Confirming Allen's finding independently. release-publish.yml delegates to a reusable workflow with permissions: contents: write — meaning it uses GITHUB_TOKEN to create releases. Per GitHub docs:

Events triggered by the GITHUB_TOKEN will not create a new workflow run.

The release: published event in chart-publish.yml will therefore never fire from the automated release path. The workflow_dispatch fallback works but requires manual intervention every release — defeating the purpose of automation.

Suggested fix: either trigger chart-publish.yml via workflow_call from the release workflow itself, or use a PAT/GitHub App token for release creation so downstream events fire.

Non-blocking Notes

• .github/workflows/chart-publish.yml:10-11: input description says "Must match helm/octo/Chart.yaml appVersion" but the actual check (line 48) validates version:, not appVersion. Misleading.
• chart-publish.yml:36-49: ${{ inputs.version }} interpolated directly in shell. Since it's maintainer-controlled via workflow_dispatch, not a public exploit, but passing through env: would be cleaner.
• Chart.yaml appVersion: "latest" means the chart doesn't pin a concrete server image version — separate concern, not introduced here.
• Missing concurrency: group to prevent overlapping publishes of the same version.

Highlights

• The NOTES.txt fix is correct and well-documented. secret.yaml:24 stores OCTO_ADMIN_PWD, and the kubectl command in NOTES.txt correctly references it with the right secret name template.
• Actions are SHA-pinned with version comments — good supply-chain practice.
• set -euo pipefail in all shell steps — correct.
• helm show chart smoke-test after push is a nice verification step.

[yujiawei]

Code Review — PR #83 (octo-deployment)

Verification Summary

Item	Result	Evidence
NOTES.txt key now matches actual secret data key	✅	helm/octo/templates/secret.yaml:24 stores OCTO_ADMIN_PWD; helm/octo/templates/NOTES.txt:43 retrieves the same key. Cross-checked against helm/octo/templates/deployment-octo-server.yaml:189,193,249 which also reads OCTO_ADMIN_PWD from the secret.
Pre-fix bug is real	✅	On main, helm/octo/templates/NOTES.txt says If secrets.adminPwd was set … but never told users how to retrieve it; the legacy command (-o jsonpath='{.data.adminPwd}') returns empty because that key doesn't exist on the rendered Secret.
Inlined secret name {{ include "octo.fullname" . }}-secrets matches the actual secret resource name	✅	helm/octo/templates/_helpers.tpl:122-124 defines octo.secretName as printf "%s-secrets" (include "octo.fullname" .), so the inlined form produces the same string.
Workflow action versions pinned to commit SHA	✅	actions/checkout@11bd719… (v4.2.2), azure/setup-helm@b9e5190… (v4.3.0). Helm CLI pinned to 3.15.3.
permissions: block follows least-privilege	✅	contents: read + packages: write only — minimum needed for helm push to ghcr.io.
Chart version verification step matches Chart.yaml to release/input version	✅	.github/workflows/chart-publish.yml:45-54 greps version: from Chart.yaml and exit 1s on mismatch.
Smoke test (helm show chart oci://…) runs after push	✅	chart-publish.yml:76-81.
GHCR login uses GITHUB_TOKEN with stdin (no token leakage in logs)	✅	chart-publish.yml:56-61 pipes via --password-stdin.
release: published trigger pairs correctly with the existing release-publish.yml flow	✅	release-publish.yml calls the reusable Mininglamp-OSS/.github/.../reusable-release-publish.yml@main and supports draft=false → publishes a real release that fires the release.published event.
Default values still produce a valid render (secrets.adminPwd: "" path)	✅	helm/octo/templates/secret.yaml:24 uses default "" so the key always exists; NOTES.txt only mentions the key — it doesn't crash if the value is empty.
.yamllint correctly excludes Helm templates so chart {{ ... }} doesn't break CI	✅	.yamllint:2-3 ignores helm/octo/templates/.

P0 / P1 Issues

None. The two fixes are correct, scoped, and verifiable from the diff alone.

Suggestions / Nits (non-blocking)

Nit 1 — Prefer the existing octo.secretName helper in NOTES.txt

helm/octo/templates/NOTES.txt:42 reconstructs the secret name inline:

kubectl get secret -n {{ .Release.Namespace }} {{ include "octo.fullname" . }}-secrets \

The chart already has octo.secretName (_helpers.tpl:122-124), which secret.yaml:8 uses as the source of truth. Inlining the suffix risks drift if someone ever changes the secret naming convention via that helper. Suggested:

kubectl get secret -n {{ .Release.Namespace }} {{ include "octo.secretName" . }} \

Functionally identical today; just keeps the rendered name behind a single helper.

Nit 2 — Harden against template injection in shell run: blocks (security_sensitive context)

chart-publish.yml interpolates ${{ inputs.version }}, ${{ steps.version.outputs.version }}, and ${{ github.actor }} directly inside run: shell scripts (lines 37, 49, 72, 74, 81, 60). workflow_dispatch is restricted to actors with write permission so the blast radius is bounded, but GitHub's own hardening guidance is to bind these through env: first to neutralize shell metacharacters:

- name: Resolve chart version
  id: version
  env:
    EVENT_NAME: ${{ github.event_name }}
    INPUT_VERSION: ${{ inputs.version }}
  run: |
    set -euo pipefail
    if [[ "$EVENT_NAME" == "workflow_dispatch" ]]; then
      VERSION="$INPUT_VERSION"
    else
      VERSION="${GITHUB_REF_NAME#v}"
    fi
    echo "version=${VERSION}" >> "$GITHUB_OUTPUT"

Same pattern for the verify / push / smoke-test / login steps. Not blocking for this PR, but flagging because the routing tag is security_sensitive and this is the kind of thing that's easier to add now than retrofit. Reference: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#good-practices-for-mitigating-script-injection-attacks

Nit 3 — set -euo pipefail is missing in the GHCR login step

chart-publish.yml:56-61 is the one run: block without set -euo pipefail. A failing helm registry login inside a single-pipe command can still surface as success in some shells. Cheap to fix:

- name: Log in to ghcr.io
  env:
    GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
    ACTOR: ${{ github.actor }}
  run: |
    set -euo pipefail
    echo "$GH_TOKEN" | helm registry login ghcr.io --username "$ACTOR" --password-stdin

Nit 4 — PR description doesn't mention the .yamllint change

The diff includes a new .yamllint (cherry-pick from PR #82, per the second commit message), but the PR body only describes the two named fixes. Not a blocker — the file is benign and its purpose is obvious from CI context — but for the audit trail it's worth one line in the description so future archeology doesn't have to guess.

Nit 5 (info) — appVersion is "latest" while chart version is pinned

helm/octo/Chart.yaml has version: 0.2.4 (immutable) and appVersion: "latest" (floating). The verify step only checks version, which is the correct field for OCI publish. Just noting that appVersion: latest means the chart isn't pinned to a reproducible app build — outside the scope of this PR but worth pinning in a follow-up before public-facing GA.

Security-Sensitive Review Notes (for human verifier)

Items I'd want a human to eyeball even though I see no concrete defect:

1. First-time packages: write use on this repo. permissions: is correctly scoped to the workflow file (no global change), and the only network destination is ghcr.io. No third-party registry, no extra secrets. Safe.
2. GITHUB_TOKEN is the only credential. No long-lived PAT; the token is repo-scoped and ephemeral. Confirmed no secrets.* reference other than GITHUB_TOKEN.
3. First push to oci://ghcr.io/mininglamp-oss/octo will create the package. Maintainer should confirm the org has GHCR enabled and that Inherit access from source repository is the desired visibility default after the first publish — package visibility is not yet set in the repo settings as far as the diff shows. Recommend setting the new package to public post-first-push if the chart is meant to be installed without auth (the PR description's helm install … oci://ghcr.io/mininglamp-oss/octo example assumes public).
4. Shell-injection vector noted in Nit 2. Bounded by workflow_dispatch write-permission gating; not a P1 because GitHub's permission model already limits the attacker set, but a defense-in-depth fix is cheap.

Recommendation

Approve. Two fixes are correct, verifiable, and minimal. The only changes I'd push for are non-blocking polish (helper reuse + workflow hardening) that can ship in a follow-up.

[lml2468]

Re-review — 📖 齐静春 (incremental check at 403fa0d)

Status: 🔴 CI still red — both P0s from previous review remain unresolved

Previous P0 #1 — yamllint CI failure:

• New commit added .yamllint config (extends: relaxed) — correct content ✓
• But CI still uses inline -d config from ci.yml on main, so the .yamllint file is ignored at runtime
• Fix: depends on PR #82 being merged first (which updates ci.yml to read .yamllint). PR #82 is still OPEN.

Previous P0 #2 — release.published trigger won't fire:

• chart-publish.yml still uses on: release: types: [published] but release-publish.yml creates releases with GITHUB_TOKEN, which does not trigger downstream workflows
• The incremental diff shows no changes to chart-publish.yml — this is unresolved.
• Fix: use a PAT or workflow_dispatch trigger from release-publish.yml to chain the chart publish.

Gate

Per review policy: CI red → cannot approve or do deep review. Please resolve both P0s (likely: rebase on PR #82 once merged + fix the trigger chain), then request re-review.