Repository navigation
feat(implementer): pre-PR guard against unacknowledged chart MAJOR bumps - #14
Conversation
Long-tail timebomb from the 2026-05-01 ESO incident: external-secrets chart 0.10.x → 2.x landed via the implementer with no awareness that Helm doesn't refresh CRDs on upgrade. The 2.x controller required v1 CRDs the cluster didn't serve, ESO went down, and the read-path stayed wedged for 6 days. See feedback_eso_chart_2x_crd_lifecycle.md. Block the same shape from happening again: between commit-detection and `git push` in implement_one(), inspect the diff against origin/HEAD for `targetRevision: <semver>` lines on adjacent diff hunks, classify pairs where the new major > old major as MAJOR bumps, and require the proposal directory to contain a `crd-migration*` marker file before allowing the push. Marker is a flag, not a doc — empty file is fine. The contract is: the spec author saw the warning, named the file, and either wrote the migration plan inside it or linked to the relevant drill. No content inspection — keeps the guard from second-guessing the author's plan. Failure mode: revert .status.yaml to `accepted`, return ImplementResult with the bump summary. The shepherd's next tick re-evaluates; if the operator intentionally landed the proposal without a CRD plan, they add the marker file and re-trigger. Soft block, not hard delete. Tests: 17 new unit tests in tests/test_run_implementer_chart_guard.py covering _is_major_bump (parse/edge cases), _proposal_acks_crd_migration (case-insensitive, empty dir, missing dir), and _detect_chart_major_bumps (simple bump, minor not flagged, quoted version, no-op diff). Full suite (56 tests) green. Limitations: matches `targetRevision:` only — not chart `version:` fields nested deeper (e.g. Chart.yaml dependencies). Sufficient for the gitops Application shape that triggered the incident; a follow-up can extend to subchart pinning if a future incident exposes that path.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fe594f8d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # --------------------------------------------------------------------------- | ||
| # Pre-PR safety guard — chart MAJOR-version bump must acknowledge CRD migration | ||
| # --------------------------------------------------------------------------- | ||
| _TARGET_REVISION_RE = re.compile(r'targetRevision:\s*"?([0-9]+(?:\.[0-9]+){1,2})"?') |
There was a problem hiding this comment.
The guard regex only allows unquoted or double-quoted versions, so YAML using single quotes (for example targetRevision: '0.10.7' to targetRevision: '2.4.0') is silently ignored. In that case _detect_chart_major_bumps() returns no bump and implement_one() will open a PR without requiring the crd-migration* acknowledgement, which defeats the safety check for a common YAML style.
Useful? React with 👍 / 👎.
| proposal author must explicitly acknowledge the CRD migration | ||
| plan; otherwise the implementer fails fast before pushing the PR. | ||
| """ | ||
| proc = _run(["git", "diff", base, "--unified=0"], cwd=repo_dir, check=False) |
There was a problem hiding this comment.
Using git diff <base> here compares <base> to the current working tree, not just to committed HEAD, so leftover uncommitted edits can falsely trigger (or mask) the chart-major guard even though those edits are never pushed. If the implementer leaves a local scratch targetRevision change uncommitted, this path can incorrectly block PR creation and revert .status.yaml to accepted despite no pushed major bump.
Useful? React with 👍 / 👎.
Codex on PR #14: - P1 — `_TARGET_REVISION_RE` only matched unquoted or double-quoted versions. Single-quoted YAML (`targetRevision: '0.10.7'`) is the default style across half of platform-gitops manifests, so the guard had a hole: a chart MAJOR bump in single-quoted form would silently bypass the CRD-migration ack requirement. Widen the optional quote class to `['"]?`. - P2 — `git diff <base>` (no second arg) compares base to the WORKING tree, not committed HEAD. A leftover uncommitted scratch edit of `targetRevision` could falsely fire the guard against a PR that won't push that change; conversely an uncommitted revert could mask a real HEAD bump. The bumps we want to gate on are exactly what `git push` would deliver, i.e. `<base>..HEAD`. Pin to `git diff <base> HEAD --unified=0`. Tests: 2 new cases. test_detect_chart_single_quoted_version pins the regex fix; test_detect_ignores_uncommitted_working_tree_edits pins the HEAD-scope fix by leaving an uncommitted MAJOR edit in the worktree and asserting the guard sees only the committed minor bump. Full suite 58/58 (19 new + 39 shepherd) green.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 937df1307c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif line.startswith("-") and not line.startswith("---"): | ||
| m = _TARGET_REVISION_RE.search(line) | ||
| if m: | ||
| pending_old = m.group(1) |
There was a problem hiding this comment.
When a single diff hunk contains multiple targetRevision replacements, this logic keeps only the last - value in pending_old before processing + lines. In a hunk like -old1 -old2 +new1 +new2, new1 is compared against old2, which can both misreport the bump and miss a real MAJOR bump entirely (for example if old1->new1 is major but old2 has a higher major), allowing the guard to pass and PR push to proceed without CRD-migration acknowledgement.
Useful? React with 👍 / 👎.
Codex P1 on PR mctl-agents#14: with a scalar `pending_old`, two
`targetRevision` replacements inside the same diff hunk
(`-old1 -old2 +new1 +new2`) lost the first `-` value when the
second `-` overwrote it. The helper then paired `old2 ↔ new1` and
`new2` had no partner, missing a real MAJOR bump entirely.
Replace the scalar with a FIFO list. Each `-` appends, each `+`
pops from the front. A new hunk / file resets the queue so a
hanging `-` from one hunk can never pair with a `+` from another.
Test: test_detect_multiple_bumps_in_single_hunk constructs a
single-hunk diff with two Applications — the first a non-major
bump (0.5 → 0.6, ignored) and the second a real MAJOR bump
(1.4 → 2.0, must be reported). The previous scalar implementation
would have paired (1.4 → 0.6) — non-major — and reported nothing.
The FIFO version reports `[("apps/multi.yaml", "1.4.0", "2.0.0")]`.
Full suite: 59/59 (20 in this file + 39 shepherd) green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f00f4c07a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # --------------------------------------------------------------------------- | ||
| # Pre-PR safety guard — chart MAJOR-version bump must acknowledge CRD migration | ||
| # --------------------------------------------------------------------------- | ||
| _TARGET_REVISION_RE = re.compile(r"""targetRevision:\s*['"]?([0-9]+(?:\.[0-9]+){1,2})['"]?""") |
There was a problem hiding this comment.
The _TARGET_REVISION_RE pattern is unanchored and _detect_chart_major_bumps() uses search(), so it also matches non-field text such as # targetRevision: 2.4.0 comments or keys like some_targetRevision. If a diff contains paired -/+ comment edits with version-like numbers, the guard will report a MAJOR bump and block PR creation even though no Helm spec.source.targetRevision changed. This creates false-positive soft blocks in normal documentation/comment-only edits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f00f4c07a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # --------------------------------------------------------------------------- | ||
| # Pre-PR safety guard — chart MAJOR-version bump must acknowledge CRD migration | ||
| # --------------------------------------------------------------------------- | ||
| _TARGET_REVISION_RE = re.compile(r"""targetRevision:\s*['"]?([0-9]+(?:\.[0-9]+){1,2})['"]?""") |
There was a problem hiding this comment.
The regex _TARGET_REVISION_RE is too permissive because it uses an unanchored search, so it matches substrings like # targetRevision: 2.4.0, nottargetRevision: 2.4.0, or even targetRevision: 1.2.3.4 (captured as 1.2.3). In implement_one(), any such match can trigger the chart-major guard and block push/revert status even when no real spec.source.targetRevision field was bumped, creating false positives that stop valid proposals unless a crd-migration* file is added unnecessarily.
Useful? React with 👍 / 👎.
Codex P2 ×2 on PR mctl-agents#14: the `_TARGET_REVISION_RE` pattern was unanchored and used `re.search`, so it matched substrings: - YAML comments: `# targetRevision: 2.4.0` would falsely fire the guard on comment-only edits. - Substring keys: `customTargetRevision: 2.4.0` looks like a bump to the regex but isn't the Helm field we gate on. - 4-component pseudo-semvers: `1.2.3.4` got captured as `1.2.3`, silently truncating data. Rewrite as a small helper `_diff_line_targets_revision(line)` that: 1. Strips the diff `-`/`+` prefix and leading whitespace. 2. Rejects YAML comments (line starts with `#`). 3. Anchors the regex at `^targetRevision:` so substring keys never match. 4. Adds a `(?![.0-9])` negative lookahead so a 4-component value refuses to parse instead of getting truncated. Three new tests pin each false-positive shape: test_detect_ignores_yaml_comment_lines, test_detect_ignores_substring_key_match, test_detect_rejects_4component_pseudo_semver. Full suite 62/62 (23 in this file + 39 shepherd) green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a0cacca25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if line.startswith("--- ") or line.startswith("@@"): | ||
| # New hunk — drop any unmatched old versions from the | ||
| # previous hunk so we never falsely pair `-` from one hunk | ||
| # with `+` from another. | ||
| pending_olds = [] |
There was a problem hiding this comment.
Do not clear pending_olds on every @@ hunk boundary, because a real version bump is missed when targetRevision is moved and updated in the same commit. With --unified=0, Git can emit - targetRevision: 0.10.7 and + targetRevision: 2.4.0 in separate hunks (delete at old location, add at new location), so this reset drops the old value before the add is seen and the MAJOR bump bypasses the CRD-migration guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a0cacca25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ver and pending_olds and current_file is not None: | ||
| old_ver = pending_olds.pop(0) | ||
| if _is_major_bump(old_ver, ver): |
There was a problem hiding this comment.
The queue-based matching here pairs any later + targetRevision with the earliest pending - targetRevision anywhere in the same hunk, so unrelated delete/add edits can be misclassified as a MAJOR upgrade. For example, replacing one Application block (deleting targetRevision: 0.10.7) while separately adding a new Application (targetRevision: 2.4.0) in the same hunk produces a synthetic 0.10.7 → 2.4.0 bump and blocks PR creation even though no in-place upgrade occurred. This guard should only pair true replacement lines for the same field edit (or otherwise verify adjacency/structure) to avoid false positives.
Useful? React with 👍 / 👎.
Codex P1 on PR mctl-agents#14: when `targetRevision` is moved AND bumped in the same commit, `git diff --unified=0` emits two hunks — delete at the old line, add at the new line. The previous version cleared `pending_olds` on every `@@` boundary, so the delete's old version was dropped before the add was parsed and the MAJOR bump silently bypassed the guard. Drop the `@@` reset. The FIFO is now per-file (still reset on `+++ b/<file>`) rather than per-hunk. Trade-off: a stray un-paired `-` from one hunk could in theory pair with a later unrelated `+` in the same file, producing a false-positive bump report. For a safety guard, false-positive (block valid PR until operator adds a `crd-migration*` marker) is the right failure direction — false-negative (let a real bump through) is what caused the 2026-05-01 ESO incident. Test: test_detect_cross_hunk_moved_and_bumped builds a file where v1 has `targetRevision: 0.10.7` near the top and v2 has `targetRevision: 2.4.0` near the bottom (with padding swap). With unified=0 this splits into two hunks; the helper now reports the bump correctly. Full suite 63/63.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36bc1605e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ver and pending_olds and current_file is not None: | ||
| old_ver = pending_olds.pop(0) |
There was a problem hiding this comment.
_detect_chart_major_bumps() only evaluates a +targetRevision when pending_olds is already populated, so it misses real MAJOR bumps when git diff --unified=0 emits the add hunk before the delete hunk (e.g., moving targetRevision upward while changing 0.10.7 -> 2.4.0 produces + first, - second). In that ordering, the + line is ignored and the later - is never paired, allowing the CRD-migration guard to be bypassed for the exact class of risky upgrade this check is meant to block.
Useful? React with 👍 / 👎.
Summary
Why
2026-05-01 ESO incident long-tail: `eso-cve-patch` proposal had the implementer bump external-secrets chart 0.10.7 → 2.4.0. Helm doesn't refresh CRDs on upgrade, so the 2.x controller hard-failed on missing `v1` CRDs and stayed Degraded for 6 days. The implementer had no awareness that this class of bump requires a CRD pre-staging step. See `feedback_eso_chart_2x_crd_lifecycle.md` and `reference_eso_storedversion_recovery.md`.
The marker file is intentionally a flag, not a doc — empty file passes. The spec author owns the migration plan; the implementer just enforces that they wrote one down.
Test plan
Limitations
Matches `targetRevision:` only — not Chart.yaml `version:` nested deeper (subchart pinning). Sufficient for the Helm Application shape that caused the ESO incident; widen if a future incident exposes a different surface.