.github/scripts/create-tag.test.js (1)

33-48: 💤 Low value

mockExecGit substring matching is fragile across the new test cases.

Patterns are matched by key.includes(pattern) and iterated in Object.entries insertion order, so any pattern that is a substring of another can shadow it. For example, in the "Wrong-commit existing tag" test (Lines 236‑240), "refs/tags/v0.1.0" is a substring of "rev-parse refs/tags/v0.1.0^{commit}", and the test passes only because the more specific "v0.1.0^{commit}" is declared first. Future additions are easy to break.

Consider switching to anchored/exact matching (e.g., key === pattern or a regex) and have callers pass full keys. Optional cleanup, not a blocker — the current tests are correct as written.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/create-tag.test.js around lines 33 - 48, The mockExecGit
helper uses substring matching (key.includes(pattern)) which allows shorter
patterns to shadow longer ones; change mockExecGit (fn) to use exact key
matching (key === pattern) or anchored regex matching when comparing keys
against responses so lookups are unambiguous, and update tests to pass full
command strings as response keys if necessary; ensure fn.calls remains populated
and behavior for Error responses is preserved.

.github/workflows/conformance.yml (2)

181-189: 💤 Low value

workflow_call is the only trigger — the if guard is dead code.

Since the on: block was reduced to workflow_call only (Lines 8‑9), github.event_name == 'workflow_call' is always true here and the conditional is always satisfied. This is harmless but misleading; consider removing the if to avoid implying there is another path.

♻️ Proposed fix

       - name: Validate required image references
-        if: ${{ github.event_name == 'workflow_call' }}
         run: |

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/conformance.yml around lines 181 - 189, The step named
"Validate required image references" contains a redundant if guard using
github.event_name == 'workflow_call' which is always true because the workflow
trigger is only workflow_call; remove the entire if: ${{ github.event_name ==
'workflow_call' }} line so the step always runs, keeping the run block that
checks CLI_IMAGE and TOOLKIT_IMAGE and prints their values (referencing
CLI_IMAGE and TOOLKIT_IMAGE in the existing run script).

---

70-78: ⚡ Quick win

Asymmetric semantics between CLI_IMAGE_NAME and CONTROLLER_IMAGE_NAME.

CLI_IMAGE_NAME is composed as ${REGISTRY}/$OWNER/cli (full ref including registry), while CONTROLLER_IMAGE_NAME is just $OWNER/kubernetes/controller (no registry). This is reflected at Line 145 where the controller usage manually re-prepends ${{ env.REGISTRY }}/. The asymmetry is easy to misuse — a future caller is likely to assume both variables follow the same convention.

Suggest making them consistent (either both include REGISTRY or both omit it).

♻️ Proposed fix

       - name: Compute image refs (lowercase owner)
         env:
           OWNER: ${{ github.repository_owner }}
         run: |
           OWNER="${OWNER,,}"
           {
             echo "CLI_IMAGE_NAME=${REGISTRY}/$OWNER/cli"
-            echo "CONTROLLER_IMAGE_NAME=$OWNER/kubernetes/controller"
+            echo "CONTROLLER_IMAGE_NAME=${REGISTRY}/$OWNER/kubernetes/controller"
           } >> "$GITHUB_ENV"

And update Line 145:

-          IMAGE_NAME: ${{ env.REGISTRY }}/${{ env.CONTROLLER_IMAGE_NAME }}
+          IMAGE_NAME: ${{ env.CONTROLLER_IMAGE_NAME }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/conformance.yml around lines 70 - 78, The two environment
variables have inconsistent semantics: CLI_IMAGE_NAME includes the registry
while CONTROLLER_IMAGE_NAME does not, causing callers to re-prepend env.REGISTRY
later; change CONTROLLER_IMAGE_NAME to include the registry as with
CLI_IMAGE_NAME by composing it as ${REGISTRY}/$OWNER/kubernetes/controller (so
both variables are full image refs), and then remove any manual re-prepending of
env.REGISTRY where CONTROLLER_IMAGE_NAME is used (update references that expect
a raw owner/path to use the new full ref).

.github/scripts/publish-final-release.test.js (1)

118-126: ⚡ Quick win

Truncation test bound is too loose.

Line 123 asserts result.length <= 125000, but MAX_RELEASE_BODY_LENGTH is 120000 — the assertion would pass even if truncation only partially worked (e.g., trimmed to 124000). Consider asserting the exact contract instead.

♻️ Proposed fix

-  assert.ok(result.length <= 125000, `Expected length <= 125000, got: ${result.length}`);
+  assert.strictEqual(result.length, 120000, `Expected exact MAX_RELEASE_BODY_LENGTH (120000), got: ${result.length}`);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/publish-final-release.test.js around lines 118 - 126, The
test currently checks result.length <= 125000 which is laxer than the actual
MAX_RELEASE_BODY_LENGTH; update the assertion in the truncation test to compare
against the real limit used by the implementation (use MAX_RELEASE_BODY_LENGTH
or the literal 120000) so it enforces the exact contract from
prepareReleaseNotes; locate the test block in
.github/scripts/publish-final-release.test.js referencing prepareReleaseNotes
and replace the length assertion to assert.ok(result.length <=
MAX_RELEASE_BODY_LENGTH, ...) (and update the failure message accordingly).

.github/scripts/publish-final-release.js (2)

5-13: ⚡ Quick win

Tighten the comment block and fix the typo.

Two small things:

• Line 7: preconfiugred → preconfigured.
• The "What is this and why is it here?" / "I'm willing to drop this …" framing is meta-commentary that belongs in the PR discussion, not the source. A concise rationale is sufficient here.

Also, the literal 125000-character in TRUNCATION_NOTICE is decoupled from MAX_RELEASE_BODY_LENGTH = 120000 — if you ever bump the cap, the notice will silently drift. Consider a GITHUB_RELEASE_BODY_LIMIT constant and interpolating it.

♻️ Proposed fix

-// What is this and why is it here? GitHub has a hard limit on the length of the changelog.
-// We could use git-cliff's [limit_commits](https://git-cliff.org/docs/configuration/git#limit_commits) setting, however,
-// that is a _hard_ limit. What does that mean? It will basically cut off at a preconfiugred number.
-// Meaning, the user will have no idea that there are supposed to be more commits if a release genuinely has
-// more than 200 commits (or whatever number is configured).
-// Now, this is fine, it's not really complexity, but I'm willing to drop this if we think it's just overkill.
-// I ran into this while testing the entire flow and had too many releases and went over the 125Kb limit.
-const MAX_RELEASE_BODY_LENGTH = 120000;
-const TRUNCATION_NOTICE = "\n\n---\n\n*Release notes truncated to fit GitHub's 125000-character body limit. See the source changelog or `git log` for the complete history.*";
+// GitHub's release body has a hard 125000-character limit. We truncate with
+// a notice instead of using git-cliff's `limit_commits`, which silently drops
+// commits beyond a preconfigured count without informing the reader.
+const GITHUB_RELEASE_BODY_LIMIT = 125000;
+const MAX_RELEASE_BODY_LENGTH = GITHUB_RELEASE_BODY_LIMIT - 5000; // safety buffer
+const TRUNCATION_NOTICE = `\n\n---\n\n*Release notes truncated to fit GitHub's ${GITHUB_RELEASE_BODY_LIMIT}-character body limit. See the source changelog or \`git log\` for the complete history.*`;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/publish-final-release.js around lines 5 - 13, The comment
block around MAX_RELEASE_BODY_LENGTH is too wordy and contains a typo; shorten
it to a concise rationale (e.g., explain GitHub's release body limit and why
truncation logic exists) and fix "preconfiugred" → "preconfigured". Replace the
hard-coded 125000 literal by introducing a GITHUB_RELEASE_BODY_LIMIT constant,
set MAX_RELEASE_BODY_LENGTH from it (or derive consistently), and update
TRUNCATION_NOTICE to interpolate that constant so the notice and limit never
drift; reference MAX_RELEASE_BODY_LENGTH, TRUNCATION_NOTICE and add
GITHUB_RELEASE_BODY_LIMIT in the same snippet.

---

66-69: 💤 Low value

Truncation can split a UTF-16 surrogate pair.

String.prototype.substring operates on UTF-16 code units, so cutting at an arbitrary safeLength may slice a surrogate pair (e.g. emoji used in headings, contributor names) and produce a broken character right before the truncation notice. Low likelihood for changelog content, but trivial to harden:

♻️ Proposed fix

-  if (notes.length > MAX_RELEASE_BODY_LENGTH) {
-    const safeLength = MAX_RELEASE_BODY_LENGTH - TRUNCATION_NOTICE.length;
-    notes = notes.substring(0, safeLength) + TRUNCATION_NOTICE;
-  }
+  if (notes.length > MAX_RELEASE_BODY_LENGTH) {
+    let safeLength = MAX_RELEASE_BODY_LENGTH - TRUNCATION_NOTICE.length;
+    // Avoid splitting a UTF-16 surrogate pair.
+    const code = notes.charCodeAt(safeLength - 1);
+    if (code >= 0xd800 && code <= 0xdbff) safeLength -= 1;
+    notes = notes.substring(0, safeLength) + TRUNCATION_NOTICE;
+  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/publish-final-release.js around lines 66 - 69, The
truncation using notes.substring(safeLength) can split a UTF-16 surrogate pair
and produce a broken character before TRUNCATION_NOTICE; update the truncation
logic around MAX_RELEASE_BODY_LENGTH so it trims by code points rather than code
units (or if using substring, decrement safeLength until the code unit at
safeLength-1 is not a high surrogate (0xD800–0xDBFF)) before appending
TRUNCATION_NOTICE; keep the same variables (notes, safeLength,
MAX_RELEASE_BODY_LENGTH, TRUNCATION_NOTICE) and ensure the resulting string
length stays ≤ MAX_RELEASE_BODY_LENGTH while avoiding splitting surrogate pairs.

.github/scripts/create-tag.js (1)

76-108: 💤 Low value

Helpers don't need to be async, but the entrypoints do — minor consistency note.

tagAtHead and tagAtCommit are synchronous and correctly typed. createRcTags / createNewReleaseTags are declared async but never await anything — they're effectively synchronous wrappers that return a Promise<void>. That's fine for actions/github-script callers, but if any future step performs real async work (e.g., waiting on a network call after pushing), be aware that the current loop will swallow rejections only at the call site.

No change required.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/create-tag.js around lines 76 - 108, The helpers tagAtHead
and tagAtCommit are synchronous and fine, but the public entrypoints
createRcTags and createNewReleaseTags are declared async yet perform no awaits
which can hide future rejections; either keep them synchronous (remove async)
or, preferably, make them correctly async by awaiting any internal async work
and propagating errors (e.g., if you add network calls, await those promises
inside createRcTags/createNewReleaseTags and rethrow or let the Promise reject
so callers can handle failures); locate createRcTags/createNewReleaseTags and
update their signatures/implementation to consistently await internal async
calls and add try/catch where you need to convert errors to core.setFailed.

cliff.toml (1)

38-44: 💤 Low value

Add --rm to the example docker run command.

The example will leave a stopped container behind on every invocation. For a one-shot CLI usage example, prefer --rm. Also, -t without -i is unusual for a --help run; consider dropping it or using -it.

✏️ Proposed fix

 ```bash
-docker run -t ghcr.io/open-component-model/cli:latest --help
+docker run --rm ghcr.io/open-component-model/cli:latest --help

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @cliff.toml around lines 38 - 44, Replace the example command "docker run -t
ghcr.io/open-component-model/cli:latest --help" by adding --rm to remove the
container after exit and remove the unnecessary -t; i.e. change that specific
"docker run -t ghcr.io/open-component-model/cli:latest --help" occurrence to
"docker run --rm ghcr.io/open-component-model/cli:latest --help" (or "docker run
--rm -it ghcr.io/open-component-model/cli:latest --help" if you prefer
interactive tty).

</details>

</blockquote></details>
<details>
<summary>.github/workflows/kubernetes-controller.yml (1)</summary><blockquote>

`91-94`: _💤 Low value_

**Image-ref computation is duplicated in `build` and `E2E` jobs.**

The same `Compute image refs (lowercase owner)` step appears at Lines 91‑94 and Lines 194‑197 with identical logic. Since each job runs on its own runner, you can't share the env, but you can avoid drift by exposing `IMAGE_NAME` as a `build` job output and referencing it from `E2E` via `needs.build.outputs.image_name`.

<details>
<summary>♻️ Proposed fix sketch</summary>

```diff
   build:
     ...
     outputs:
       artifact_name: ${{ env.ARTIFACT_NAME }}
       artifact_id: ${{ steps.upload-artifacts.outputs.artifact-id }}
       version: ${{ steps.version.outputs.version }}
+      image_name: ${{ steps.image-name.outputs.image_name }}
     steps:
       - name: Compute image refs (lowercase owner)
+        id: image-name
         env:
           OWNER: ${{ github.repository_owner }}
-        run: echo "IMAGE_NAME=${REGISTRY}/${OWNER,,}/kubernetes/controller" >> "$GITHUB_ENV"
+        run: |
+          IMAGE_NAME="${REGISTRY}/${OWNER,,}/kubernetes/controller"
+          echo "IMAGE_NAME=${IMAGE_NAME}" >> "$GITHUB_ENV"
+          echo "image_name=${IMAGE_NAME}" >> "$GITHUB_OUTPUT"
   ...
   E2E:
     ...
     steps:
-      - name: Compute image refs (lowercase owner)
-        env:
-          OWNER: ${{ github.repository_owner }}
-        run: echo "IMAGE_NAME=${REGISTRY}/${OWNER,,}/kubernetes/controller" >> "$GITHUB_ENV"
+      - name: Propagate IMAGE_NAME from build
+        run: echo "IMAGE_NAME=${{ needs.build.outputs.image_name }}" >> "$GITHUB_ENV"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/kubernetes-controller.yml around lines 91 - 94, Compute
the image ref once in the build job by turning the existing "Compute image refs
(lowercase owner)" step (which uses OWNER=${{ github.repository_owner }} and
REGISTRY) into a step that writes IMAGE_NAME to the job output (e.g., set-output
or outputs block named image_name) and remove the duplicate step from the E2E
job; then in the E2E job use needs.build.outputs.image_name to reference the
computed IMAGE_NAME instead of recomputing it, ensuring the build job declares
the output (job outputs) and the E2E job references that output.

.github/workflows/pipeline.yml (2)

175-178: 💤 Low value

TAG discovery from the OCI layout is fragile.

oras repo tags --oci-layout ${LAYOUT} | head -n 1 silently picks an arbitrary tag if the layout ever contains more than one. If the CLI build later starts emitting multiple tags (e.g. a version + latest), this will push the wrong reference. Consider passing the version explicitly from build_cli.outputs.version (the same way publish_controller does on line 285) so the published tag is deterministic.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/pipeline.yml around lines 175 - 178, Replace the fragile
TAG discovery that uses "oras repo tags --oci-layout ${LAYOUT} | head -n 1" with
a deterministic value from the build job output; set TAG to the
build_cli.outputs.version (the same source used by publish_controller) instead
of parsing the OCI layout so the published tag is explicit and stable (update
the step that currently writes "TAG=$TAG" to GITHUB_ENV to source
build_cli.outputs.version).

---

145-206: ⚡ Quick win

Inconsistent floating-tag handling between CLI and controller publishers.

publish_controller retags the image (and chart) as main when REF == 'main' (lines 287–291, 316–320), but publish_cli has no equivalent step. Anyone consuming ghcr.io/<owner>/cli:main to track the default branch will not get an updated digest from this workflow.

If the CLI image is intentionally not floating-tagged, please document that here; otherwise add a parallel "Tag image with main floating tag" step to keep the two publishers symmetric.

.github/scripts/release-versioning.js (1)

299-306: 💤 Low value

Sort comparator returns -1 for equal items — fine for .pop(), but technically not a total order.

isStableNewer(va, vb) returns false when va === vb, so the comparator returns -1 for equal versions, violating the Array.prototype.sort contract (should return 0). It happens to be safe here because you only use the largest element via .pop() and duplicates of canonical release tags shouldn't exist on GitHub. Worth a small tweak if you want a clean comparator:

♻️ Suggested cleanup

-    return versions.sort((a, b) => isStableNewer(`v${a}`, `v${b}`) ? 1 : -1).pop();
+    return versions.sort((a, b) => {
+        if (isStableNewer(`v${a}`, `v${b}`)) return 1;
+        if (isStableNewer(`v${b}`, `v${a}`)) return -1;
+        return 0;
+    }).pop();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/release-versioning.js around lines 299 - 306, The sort
comparator in extractHighestPreviousReleaseVersion uses isStableNewer and
returns -1 when versions are equal, violating Array.sort's contract; update the
comparator in extractHighestPreviousReleaseVersion so it returns 0 for equal
items (e.g., compare a and b for equality first, then call
isStableNewer(`v${a}`, `v${b}`) to return 1 or -1), ensuring a proper total
order before calling .pop().

.github/workflows/release.yml (1)

496-522: 💤 Low value

Chart-equivalence check is a good safeguard — minor caveat on the tag-normalization regex.

Diffing the RC chart against the repackaged final chart after stripping version/appVersion and normalizing the manager image tag prefix is a nice integrity check that catches accidental drift between RC and final. One caveat: '.manager.image.tag |= sub("^[^@]*", "")' only works if the rendered tag actually contains @<digest>. If a future change ships a chart that pins by tag without the digest pinning, this would clear the entire string in one and not the other depending on shape, masking a real difference. Worth a brief comment in the workflow noting that contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release.yml around lines 496 - 522, The yq normalization '
.manager.image.tag |= sub("^[^@]*", "")' will zero-out the tag when there is no
'@' digest present; change the substitution to only strip the digest suffix by
using sub("@.*$", "") so tags without digests remain unchanged, update the two
yq invocations that operate on "$dir/values.yaml" (the lines that currently call
'.manager.image.tag |= sub("^[^@]*", "")'), and add a brief inline comment in
the workflow explaining the normalization only removes digest portions and will
not mask tag-vs-digest changes when a tag has no digest.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.github/workflows/release.yml (1)

519-529: ⚡ Quick win

Fail fast if the chart stops pinning manager.image.tag by digest.

Right now the normalization can turn a tag-only value into an empty string on both sides, so diff -r would still pass after a real RC/final drift. Add an explicit digest-pinning assertion before rewriting the field.

Suggested hardening

           for dir in "$work/rc" "$work/final"; do
+            grep -q '@sha256:' "$dir/values.yaml" \
+              || { echo "::error::$dir/values.yaml is not digest-pinned"; exit 1; }
             yq -i 'del(.version, .appVersion)' "$dir/Chart.yaml"
             yq -i '.manager.image.tag |= sub("^[^@]*", "")' "$dir/values.yaml"
           done

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release.yml around lines 519 - 529, Before normalizing
manager.image.tag, validate that the chart pins the image by digest: in the loop
over "$work/rc" and "$work/final" (the block that runs yq -i 'del(.version,
.appVersion)' "$dir/Chart.yaml" and yq -i '.manager.image.tag |= sub("^[^@]*",
"")' "$dir/values.yaml"), read .manager.image.tag (with yq) and assert it
contains a digest marker (e.g. "@sha256:"); if the check fails print a clear
error referencing the chart (Chart.yaml/values.yaml and manager.image.tag) and
exit non‑zero so the workflow fails fast before performing the substitution.
Ensure the assertion runs for each "$dir" prior to the sub(...) rewrite.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes