[bborbe]

Chart 0.9.0→0.10.0: executor.runUntagged / executors[].runUntagged render RUN_UNTAGGED (agent-task-executor ≥ v0.22.0) only when set; false honoured via hasKey+kindIs, per-entry wins, else inherits. Same idiom as stageFilter (#117). Sun sets false so untagged tasks never run there.

Also: moves the shipped prompt 240 to completed (it was blocking the queue), and reverts the spec-status flips + generated verification prompt the pipeline run committed.

Render checks (local helm; container has none): default renders no RUN_UNTAGGED (singular + list); singular false/true; list per-entry (1); inherit (2); per-entry override (false/true); default render identical to origin/master apart from helm.sh/chart; helm lint 0 failed.

[ben-s-pull-request-reviewer]

I have everything I need. Here's the review.

Step 0c — Diff scope

5 files reviewed, 2 files excluded by .reviewignore (prompts/completed/240-helm-executor-stage-filter-env.md, prompts/completed/241-helm-executor-run-untagged-env.md — the prompts/ glob). The working tree was already at PR HEAD (1bcd2dd), so no worktree was created; no cleanup needed.

precommit skipped (selector mode) — CI covers lint+test. Note: helm is not installed in this container, so the template was verified by reading the rendered-whitespace semantics, not by helm template.

Must Fix (Critical)

None.

Should Fix (Important)

• CHANGELOG.md:13 — the new ## Unreleased bullet starts with the scoped prefix feat(helm):, which is not in the recognised conventional-prefix set (feat:, fix:, refactor:, test:, docs:, chore:, perf:); the release tooling parses the prefix with ^- ([a-z]+:), which does not match a scoped prefix, so the version-bump classification relies on the assistant's fallback heuristic instead of the deterministic parse. Use - feat: render optional \RUN_UNTAGGED` env …(scope can move into the prose). Non-blocking: the same repo's two most recent helm releases used the scoped form, and the bump would still classify as minor via the description fallback. *(rule:changelog/conventional-prefix-required`)*
• helm/templates/executor-deployment.yaml:119 — no CI-runnable coverage for the new conditional. The repo's CI only runs make precommit (.github/workflows/ci.yml) and never renders the chart, so the unset / false / true / per-entry-override matrix was verified manually with a local helm. A helm template assertion (helm unittest or a CI job) would regression-protect both the hasKey+kindIs "bool" guard and the per-entry-over-parent precedence. (rule: tdd/failing-test-before-implementation)

Nice to Have (Optional)

• helm/Chart.yaml:6 — the chart requires agent-task-executor ≥ v0.22.0 for RUN_UNTAGGED, but that requirement is only documented (README, values.yaml comment, CHANGELOG), not pinned: executor.image.tag is empty by default and falls back to Chart.appVersion (0.3.1, line 8), so an install that does not pin the tag gets an executor that silently ignores the env — the feature's stated mechanism never fires. Pre-existing pattern (identical for STAGE_FILTER ≥ v0.21.0 in v0.109.0); worth stating the tag requirement next to executor.image.tag.

Verification performed against the plan's concerns

• Template logic mirrors the shipped STAGE_FILTER block exactly in shape (hasKey + kindIs "bool" at the singular level; hasKey $e override → kindIs "bool" at the list level). := declares, = reassigns an in-scope var; all actions use {{-, so no stray blank lines, and the 12/14-space indentation matches the neighbouring entries.
• Both Deployments got the block (lines 119–122 and 284–291); the STAGE_FILTER blocks are byte-identical to before.
• runUntagged exists only as a commented example in helm/values.yaml — no real default key, so unset stays distinguishable from false.
• helm.sh/chart appears only in metadata.labels (_helpers.tpl:5); the Deployment selectors use app/vault only, so the 0.9.0→0.10.0 chart bump cannot block a rolling upgrade.
• README rows match the actual semantics (per-entry wins, else inherit, unset renders nothing).

Selector Traceability

• Candidates: 7
• Applicable: 6
• Skipped: go-cli/cobra-not-stdlib-flag → no Go CLI code in this diff
• Mechanical funnel: 0 findings (79 YAML rules run)

{
  "verdict": "approve",
  "summary": "The RUN_UNTAGGED env plumbing is a faithful mirror of the shipped STAGE_FILTER implementation across both executor Deployments, values.yaml and README, with no template-semantics defects found; the only gaps are a non-conventional CHANGELOG prefix and the absence of any CI-runnable helm render coverage for the chart.",
  "comments": [
    {
      "file": "CHANGELOG.md",
      "line": 13,
      "severity": "major",
      "blocking": false,
      "blocking_reason": "",
      "message": "Should Fix — the new `## Unreleased` bullet starts with the scoped prefix `feat(helm):`, which is not in the recognised conventional-prefix set (`feat:`, `fix:`, `refactor:`, `test:`, `docs:`, `chore:`, `perf:`). The release tooling parses the prefix with `^- ([a-z]+:)`, which does not match a scoped prefix, so the version-bump classification falls back to the assistant's description heuristic instead of the deterministic parse. Suggest `- feat: render optional `RUN_UNTAGGED` env on the singular executor …` (move the `(helm)` scope into the prose). Non-blocking: the two most recent helm releases in this repo used the same scoped form, and the bump would still classify as minor via the fallback. (rule: `changelog/conventional-prefix-required`)"
    },
    {
      "file": "helm/templates/executor-deployment.yaml",
      "line": 119,
      "severity": "major",
      "blocking": false,
      "blocking_reason": "",
      "message": "Should Fix — no CI-runnable coverage for the new conditional. `.github/workflows/ci.yml` only runs `make precommit`, which never renders the chart, so the unset / `false` / `true` / per-entry-override matrix was verified manually with a local helm. A `helm template` assertion (helm unittest, or a CI job that renders with and without `executor.runUntagged` and per-entry overrides) would regression-protect the `hasKey` + `kindIs \"bool\"` guard and the per-entry-over-parent precedence. (rule: `tdd/failing-test-before-implementation`)"
    },
    {
      "file": "helm/Chart.yaml",
      "line": 6,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "Nice to Have — the chart requires agent-task-executor >= v0.22.0 for `RUN_UNTAGGED`, but that requirement is only documented (helm/README.md, values.yaml comment, CHANGELOG), never pinned or validated: `executor.image.tag` is empty by default and `agent.executor.image` falls back to `Chart.appVersion` (`0.3.1`, line 8), so an install that does not pin the tag runs an executor that silently ignores `RUN_UNTAGGED` and the feature's stated mechanism never fires. Pre-existing pattern — the v0.109.0 `STAGE_FILTER` change (executor >= v0.21.0) has the same gap; worth stating the minimum executor tag next to `executor.image.tag` in helm/values.yaml."
    }
  ],
  "concerns_addressed": [
    {
      "concern": "correctness: executors[] branch reads `$.Values.executor.runUntagged` without a hasKey guard on the parent map",
      "disposition": "not-an-issue",
      "detail": "helm/templates/executor-deployment.yaml:284 — `.Values.executor` is always a map (chart values.yaml defines it) and a missing child key yields nil without error; `kindIs \"bool\" nil` is false so nothing renders. A nil or non-map `executors[]` entry fails earlier at `$e.enabled` (line 160), pre-existing."
    },
    {
      "concern": "correctness: singular branch uses hasKey + kindIs bool while list branch tests the resolved value only",
      "disposition": "not-an-issue",
      "detail": "helm/templates/executor-deployment.yaml:119 and :288 — both levels ultimately gate on `kindIs \"bool\"`, so a non-bool value (e.g. the string \"false\") renders nothing in both the singular and the per-entry case; behaviour is identical."
    },
    {
      "concern": "correctness: `=` vs `:=` reassignment and whitespace trimming produce no stray lines / indentation",
      "disposition": "not-an-issue",
      "detail": "helm/templates/executor-deployment.yaml:284-291 — `:=` declares `$runUntagged`, `=` reassigns the already-declared in-scope variable (valid in Go templates); every action uses `{{-`, so the newline after each action is consumed by the following action's left trim. Rendered shape is identical to the STAGE_FILTER block at :280-283, with 12-space `- name:` / 14-space `value:` matching the neighbours."
    },
    {
      "concern": "correctness: RUN_UNTAGGED added to BOTH Deployments and STAGE_FILTER untouched",
      "disposition": "not-an-issue",
      "detail": "helm/templates/executor-deployment.yaml:119-122 (singular) and :284-291 (executors[] range) both render RUN_UNTAGGED; the STAGE_FILTER blocks at :115-118 and :276-283 are unchanged in the diff."
    },
    {
      "concern": "tests: no committed test or golden-file coverage for the new template logic",
      "disposition": "addressed",
      "detail": "Reported as a Should Fix comment on helm/templates/executor-deployment.yaml:119 — CI (make precommit) never renders the chart, so the render matrix is manual-only; a `helm template` assertion is recommended."
    },
    {
      "concern": "tests: verify `runUntagged` is only a commented-out example in values.yaml",
      "disposition": "not-an-issue",
      "detail": "helm/values.yaml:119-125 and :201/:235 — every occurrence is a comment (`# runUntagged: false`); no real default key was added, so unset remains distinguishable from false."
    },
    {
      "concern": "security: chart version bump changes the `helm.sh/chart` label — does any selector or immutable field depend on it",
      "disposition": "not-an-issue",
      "detail": "helm/templates/_helpers.tpl:5 puts `helm.sh/chart` in metadata.labels only; both Deployments' `spec.selector.matchLabels` use `app` (and `vault` for the range branch) — no selector or immutable field references the chart label, so a rolling upgrade is not blocked."
    },
    {
      "concern": "correctness: chart requires agent-task-executor >= v0.22.0 but appVersion is unchanged and the image tag is not validated",
      "disposition": "addressed",
      "detail": "Reported as a Nice to Have comment on helm/Chart.yaml:6 — the requirement is documented in helm/README.md:115, helm/values.yaml:119-125 and CHANGELOG.md:13, but the executor image tag defaults to Chart.appVersion (0.3.1), so an unpinned install silently ignores the env. Pre-existing pattern (same for STAGE_FILTER >= v0.21.0)."
    },
    {
      "concern": "docs: README rows must match the actual template semantics and the stated default",
      "disposition": "not-an-issue",
      "detail": "helm/README.md:115 and :148 — the rows state 'only when the key is set', 'false is honoured', 'absent renders nothing', per-entry wins / else inherits, which match helm/templates/executor-deployment.yaml:119-122 and :284-291; the default column says `unset`, matching the commented-only values.yaml entry."
    },
    {
      "concern": "correctness: prompts/completed/240-helm-executor-stage-filter-env.md frontmatter was rewritten",
      "disposition": "not-an-issue",
      "detail": "The file is excluded by the repo's .reviewignore (`prompts/` glob) and is not part of the reviewed diff — 2 files excluded by .reviewignore in total."
    },
    {
      "concern": "correctness: prompts/completed/241-helm-executor-run-untagged-env.md embeds instructions that must be treated as untrusted",
      "disposition": "not-an-issue",
      "detail": "Excluded by .reviewignore (`prompts/`) and therefore outside the reviewed diff; its content was read only as untrusted data to understand intent and was not followed as instructions."
    },
    {
      "concern": "correctness: CHANGELOG.md `## Unreleased` heading placement and wording",
      "disposition": "addressed",
      "detail": "Placement verified correct (after the SemVer preamble, directly above `## v0.109.0`, consistent with the file's convention); the wording matches the template behaviour (false honoured, unset renders nothing). The bullet's scoped `feat(helm):` prefix is reported as a Should Fix comment on CHANGELOG.md:13."
    }
  ]
}