[MEDIUM] GITHUB_APP_TOKEN masking is not visible in this workflow — relying entirely on setup-credentials to mask it

The workflow sets GH_TOKEN: ${{ env.GITHUB_APP_TOKEN }} in multiple steps (lines 129, 198) after ./setup-credentials runs, but never calls echo "::add-mask::$GITHUB_APP_TOKEN" itself. Per AGENTS.md: "All secret values are masked with ::add-mask:: before any other step can log them."

If setup-credentials does mask the token internally (which it should given team conventions), this is a non-issue. However, this is a newly added workflow file and there is no explicit masking call here, which means:

1. If setup-credentials is updated in the future and the masking is moved or removed, this workflow silently loses protection.
2. Any debug/echo of $GITHUB_APP_TOKEN before setup-credentials runs (e.g., in error paths) would leak it.

Suggested fix: add an explicit mask call after the setup-credentials step completes:

- name: Mask app token
  run: echo "::add-mask::$GITHUB_APP_TOKEN"

This is belt-and-suspenders and makes the security invariant self-documenting in the workflow file.

[LOW] JSDoc comment for USES_RE lists 4 capture groups but the regex has 5

The comment above USES_RE documents groups 1–4 as prefix, subpath, ref, comment, omitting the closeQuote group that sits between ref and comment:

*   1. prefix     — everything before the slug
*   2. subpath    — optional sub-action / workflow path
*   3. ref        — the ref after `@` (SHA, tag, or branch)
*   4. comment    — optional trailing ` # vX.Y.Z` comment   ← should be group 5

The actual regex captures 5 groups and the destructuring is correct ([, prefix, subpath, ref, closeQuote, comment]), so there is no runtime bug. But a future maintainer reading the JSDoc comment would map closeQuote to "comment" and comment to nothing, which could cause an incorrect edit. Adding 4. closeQuote — optional closing quote (e.g. \"`)` would make the comment accurate.

[LOW] USES_RE trailing \s*$ silently strips trailing whitespace from rewritten uses: lines

The regex ends with \\s*$, which consumes any trailing spaces or tabs that follow the optional comment on a matched uses: line. Because the reconstructed line (${prefix}${NEW_SLUG}${newSubpath}@${newRef}${closeQuote}${newComment}) does not re-append those characters, any trailing whitespace present in the original line is silently dropped.

In practice YAML linters (and most editors) strip trailing whitespace anyway, so this is unlikely to matter. But the function's contract is "the rest of the file is preserved byte-for-byte" — this is a small hole in that guarantee. Changing the trailing \\s*$ to (\\s*)$ and capturing group 6, then appending it in the rebuild, would make the behaviour fully round-trip preserving.

question (non-blocking): just want to flag this in case it's surprising — github.workflow_sha is the commit SHA of the workflow file in the repo where the workflow is running, not the SHA of the thing being built. so when this workflow eventually lives in docker/docker-agent-action, github.workflow_sha will be a SHA in that repo, and this checkout will work correctly.

if you run this from cagent-action today (before the copy), this step would fail because that SHA won't exist in docker/docker-agent-action. that's expected and consistent with the "merging is inert" note in the PR description — just worth knowing why, in case you hit it in testing.

praise (🥇): the single EXIT trap + cleanup_workdir pattern is nicely done. registering a trap per iteration is a subtle footgun (each new trap replaces the previous one), so anchoring it once with a shared $CURRENT_WORK_DIR variable is the right call. the comment explains exactly why too.

nitpick (super non-blocking): the lastIndex reset after .test() is correct (stateful /g regexes in JS are a classic footgun), but if you ever want to simplify this, you can avoid the whole pattern by just not using the /g flag on PLAIN_RE and relying solely on .replace() with /g when you need global replacement. something like:

if (line.includes(OLD_SLUG)) {
  otherCount++;
  return hasCR ? `${line.replace(PLAIN_RE, NEW_SLUG)}\r` : line.replace(PLAIN_RE, NEW_SLUG);
}

the includes() check is faster than a regex .test() for a simple substring guard. but the current approach is also correct, so no action needed.

question (non-blocking): just want to flag this in case it's surprising — github.workflow_sha is the commit SHA of the workflow file in the repo where the workflow is running, not the SHA of the thing being built. so when this workflow eventually lives in docker/docker-agent-action, github.workflow_sha will be a SHA in that repo, and this checkout will work correctly.

if you run this from cagent-action today (before the copy), this step would fail because that SHA won't exist in docker/docker-agent-action. that's expected and consistent with the "merging is inert" note in the PR description — just worth knowing why, in case you hit it in testing.

praise (🥇): the single EXIT trap + cleanup_workdir pattern is nicely done. registering a trap per iteration is a subtle footgun (each new trap replaces the previous one), so anchoring it once with a shared $CURRENT_WORK_DIR variable is the right call. the comment explains exactly why too.

nitpick (super non-blocking): the lastIndex reset after .test() is correct (stateful /g regexes in JS are a classic footgun), but if you ever want to simplify this, you can avoid the whole pattern by just not using the /g flag on PLAIN_RE and relying solely on .replace() with /g when you need global replacement. something like:

if (line.includes(OLD_SLUG)) {
  otherCount++;
  return hasCR ? `${line.replace(PLAIN_RE, NEW_SLUG)}\r` : line.replace(PLAIN_RE, NEW_SLUG);
}

the includes() check is faster than a regex .test() for a simple substring guard. but the current approach is also correct, so no action needed.

question (non-blocking): just want to flag this in case it's surprising — github.workflow_sha is the commit SHA of the workflow file in the repo where the workflow is running, not the SHA of the thing being built. so when this workflow eventually lives in docker/docker-agent-action, github.workflow_sha will be a SHA in that repo, and this checkout will work correctly.

if you run this from cagent-action today (before the copy), this step would fail because that SHA won't exist in docker/docker-agent-action. that's expected and consistent with the "merging is inert" note in the PR description — just worth knowing why, in case you hit it in testing.

praise (🥇): the single EXIT trap + cleanup_workdir pattern is nicely done. registering a trap per iteration is a subtle footgun (each new trap replaces the previous one), so anchoring it once with a shared $CURRENT_WORK_DIR variable is the right call. the comment explains exactly why too.

nitpick (super non-blocking): the lastIndex reset after .test() is correct (stateful /g regexes in JS are a classic footgun), but if you ever want to simplify this, you can avoid the whole pattern by just not using the /g flag on PLAIN_RE and relying solely on .replace() with /g when you need global replacement. something like:

if (line.includes(OLD_SLUG)) {
  otherCount++;
  return hasCR ? `${line.replace(PLAIN_RE, NEW_SLUG)}\r` : line.replace(PLAIN_RE, NEW_SLUG);
}

the includes() check is faster than a regex .test() for a simple substring guard. but the current approach is also correct, so no action needed.

question (non-blocking): just want to flag this in case it's surprising — github.workflow_sha is the commit SHA of the workflow file in the repo where the workflow is running, not the SHA of the thing being built. so when this workflow eventually lives in docker/docker-agent-action, github.workflow_sha will be a SHA in that repo, and this checkout will work correctly.

if you run this from cagent-action today (before the copy), this step would fail because that SHA won't exist in docker/docker-agent-action. that's expected and consistent with the "merging is inert" note in the PR description — just worth knowing why, in case you hit it in testing.

praise (🥇): the single EXIT trap + cleanup_workdir pattern is nicely done. registering a trap per iteration is a subtle footgun (each new trap replaces the previous one), so anchoring it once with a shared $CURRENT_WORK_DIR variable is the right call. the comment explains exactly why too.

nitpick (super non-blocking): the lastIndex reset after .test() is correct (stateful /g regexes in JS are a classic footgun), but if you ever want to simplify this, you can avoid the whole pattern by just not using the /g flag on PLAIN_RE and relying solely on .replace() with /g when you need global replacement. something like:

if (line.includes(OLD_SLUG)) {
  otherCount++;
  return hasCR ? `${line.replace(PLAIN_RE, NEW_SLUG)}\r` : line.replace(PLAIN_RE, NEW_SLUG);
}

the includes() check is faster than a regex .test() for a simple substring guard. but the current approach is also correct, so no action needed.

issue: github.workflow_sha is the commit SHA of the workflow file in the repo where the workflow is running — which right now is cagent-action. but line 108 points actions/checkout at docker/docker-agent-action, so it's asking that repo to check out a SHA that only exists in cagent-action. that'll fail.

the intent here is to check out the current source so the CLI tools can be built. since the source lives in cagent-action for now, the fix is to just let the checkout default to the current repo:

- name: Checkout source for build
  uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
  with:
    ref: ${{ github.sha }}
    persist-credentials: false

drop the repository: line entirely so it defaults to whatever repo the workflow is running in. once the content is copied to docker/docker-agent-action, this will work correctly there too — github.sha will then be a SHA in that repo.