chore: Cherry-picked changes from upstream - #42
Conversation
|
🚀 PR Updated! The PR has been updated with the latest cherry-picked commits. @step-security/maintained-actions-dev Please review and approve the changes. 📦 Target Release Version: v1.0.183 |
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // RUNNER_TEMP is per-job, not per-step: remove the identity token, the | ||
| // profile, and the cached exchanged credential so they don't outlive | ||
| // this step. | ||
| rmSync(tokenDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
stop() can now throw, and it's the first unguarded statement in run.ts's cleanup finally (src/entrypoints/run.ts:371).
force: true only swallows ENOENT — EPERM/EACCES/EBUSY/ENOTEMPTY still throw, and maxRetries defaults to 0. ENOTEMPTY is plausible here specifically because ANTHROPIC_CONFIG_DIR is a subdirectory of tokenDir, so a claude subprocess that outlives the parent can repopulate it mid-walk.
If it throws, the rest of the finally never runs: the tracking comment stays on "Claude is working…", no step summary, and core.setOutput("github_token", …) (run.ts:412) never fires — so the always() "Revoke app token" step, which is gated on steps.run.outputs.github_token != '' (action.yml:483), is skipped and the GitHub App installation token stays valid for up to an hour.
| rmSync(tokenDir, { recursive: true, force: true }); | |
| try { | |
| rmSync(tokenDir, { | |
| recursive: true, | |
| force: true, | |
| maxRetries: 3, | |
| retryDelay: 100, | |
| }); | |
| } catch (error) { | |
| core.warning( | |
| `Failed to remove the workload identity token dir: ${error instanceof Error ? error.message : String(error)}`, | |
| ); | |
| } |
| tokenFile, | ||
| stop: () => clearInterval(refreshInterval), | ||
| stop: () => { | ||
| clearInterval(refreshInterval); |
There was a problem hiding this comment.
An in-flight refresh can recreate the directory right after rmSync deletes it, defeating the cleanup this PR adds.
clearInterval stops future ticks but can't cancel a writeIdentityToken() already awaiting fetchIdentityToken, which goes through retryWithBackoff (3 attempts, 5s → 10s) and can stay pending 15+ seconds. When it finally resolves it runs mkdirSync(tokenDir, …) + writeFileSync(tokenFile, …) (lines 149-150) and writes a fresh OIDC JWT to disk after cleanup. Since RUNNER_TEMP is per-job, it then survives for the rest of the job — exactly what the comment below says the rmSync prevents.
Suggest a stopped flag checked after the await:
let stopped = false;
const writeIdentityToken = async () => {
const identityToken = await fetchIdentityToken(audience);
if (stopped) return; // cleanup already ran; don't resurrect the token file
core.setSecret(identityToken);
mkdirSync(tokenDir, { recursive: true, mode: 0o700 });
writeFileSync(tokenFile, identityToken, { mode: 0o600 });
};…and set stopped = true before clearInterval here.
| */ | ||
| function writeFederationProfile(baseDir: string): string { | ||
| // Every input that changes which credential the exchange mints must be in | ||
| // here; service_account_id and scope are sent in the exchange request body. |
There was a problem hiding this comment.
The fingerprint omits ANTHROPIC_OIDC_AUDIENCE, which contradicts this comment's own invariant. It's a first-class input (action.yml:375) and is used to mint the JWT at line 139.
Two invocations in one job with identical rule/org/service-account/workspace/scope but different audiences land in the same config-<fp> dir. Per the doc comment above, the SDK's cache reuses on expires_at alone — so the second invocation silently reuses the first one's token instead of exchanging with the new audience assertion, bypassing any audience constraint the federation rule enforces.
| // here; service_account_id and scope are sent in the exchange request body. | |
| // Every input that changes which credential the exchange mints must be in | |
| // here; service_account_id and scope are sent in the exchange request body, | |
| // and the audience is bound into the identity token assertion. |
…plus process.env.ANTHROPIC_OIDC_AUDIENCE?.trim() ?? "" in the hashed array.
| } finally { | ||
| // Stop refreshing the workload identity token file so the process can exit | ||
| // Stop refreshing the workload identity token file (so the process can | ||
| // exit) and delete the token material so it doesn't outlive this step |
There was a problem hiding this comment.
This comment now documents an invariant the code doesn't hold: process.exit(1) at line 126 doesn't unwind the stack, so on every failure path of the standalone base-action this finally never runs and the token dir is left behind.
That was tolerable before (the leftover was a single-use OIDC JWT that fails with jti_reused), but this PR makes the same directory hold the SDK's cached exchanged Anthropic credential — a reusable bearer token valid until expires_at. RUNNER_TEMP is per-job, so any later step in the same job can read it.
core.setFailed already forces a non-zero exit, so dropping the explicit process.exit(1) is the smallest fix. (An always() cleanup step in action.yml — the convention this repo already uses for token revocation and SSH signing — would also cover SIGTERM/cancellation, which no finally catches in either entrypoint.)
| } | ||
|
|
||
| if (!isSuccess) { | ||
| if (resultMessage.subtype === "success" && resultMessage.is_error) { |
There was a problem hiding this comment.
This new diagnostic is unreachable when --json-schema is in play, and the user gets a misleading error instead.
Before this change subtype === "success" implied isSuccess, so the hasJsonSchema && !isSuccess && subtype === "success" combination couldn't happen. It can now — and the hasJsonSchema block at lines 218-237 throws first with --json-schema was provided but Claude did not return structured_output. Result subtype: success, which reads as "the model just didn't emit structured output" and hides the real cause.
Moving the if (!isSuccess) { … } block above the if (hasJsonSchema) block fixes it.
| // Reference-style images: ![alt][ref] -> ![][ref] (keep the label, drop the | ||
| // alt text, which is otherwise a hidden-instruction channel just like the | ||
| // inline form above). | ||
| content = content.replace(/!\[[^\]]*\](\[[^\]]*\])/g, "![]$1"); |
There was a problem hiding this comment.
This breaks collapsed reference images, which is the one case where the alt text is the lookup label.
![][] → ![][]: [^\]]* matches alt text, then the capture group matches the empty []. The label is now empty, so it no longer resolves to its [alt text]: url definition and GitHub renders the literal text instead of the image. The comment's stated intent ("keep the label, drop the alt text") is violated here. Guarding on a non-empty label fixes it:
| content = content.replace(/!\[[^\]]*\](\[[^\]]*\])/g, "![]$1"); | |
| content = content.replace(/!\[[^\]]*\](\[[^\]]+\])/g, "![]$1"); |
Two related gaps, if you want to close the channel more completely (happy to defer these to a follow-up):
- The shortcut form
![payload]+ a separate[payload]: urldefinition still renders as an image withalt="payload"and isn't matched at all. - Link reference definitions render as nothing, so
[x]: https://ex.com "instructions here"is 100% invisible to a reviewer yet reaches the prompt verbatim —stripMarkdownLinkTitlesonly handles inline](url "title"). Keeping[ref]labels intact (correctly) makes this channel more relevant than before.
| const modelFromClaudeArgs = extraArgs["model"] || undefined; | ||
| delete extraArgs["model"]; |
There was a problem hiding this comment.
This inverts CLI precedence: an explicit --model in claude_args now silently loses to an ambient env var.
--model is deleted from extraArgs (so it never reaches the CLI) and is only a fallback at line 310: model: options.model || modelFromClaudeArgs. But options.model is process.env.ANTHROPIC_MODEL (base-action/src/index.ts:106, src/entrypoints/run.ts:338) — an ambient env var, not an explicit input. A CLI flag beating an env var is the normal expectation; this does the opposite, with nothing logged.
Scenario: a workflow sets env: ANTHROPIC_MODEL: <opus> at the job level and one step passes claude_args: --model <haiku> for a cheap task. Previously the flag was passed through to the CLI; now it's dropped and opus runs, at ~10× the cost, with no indication in the logs.
Suggest modelFromClaudeArgs || options.model, or at minimum a core.warning() when both are set.
| ## Automated Documentation Updates | ||
|
|
||
| Automatically update documentation when specific files change (see [`examples/claude-pr-path-specific.yml`](../examples/claude-pr-path-specific.yml)): | ||
| Automatically update documentation when specific files change (see [`examples/pr-review-filtered-paths.yml`](../examples/pr-review-filtered-paths.yml)): |
There was a problem hiding this comment.
The file exists, but it doesn't match this section. examples/pr-review-filtered-paths.yml is name: Claude Review - Path Specific — a path-filtered code review workflow that posts inline comments and never touches docs, whereas this section is "Automated Documentation Updates" with a docs-update prompt.
The parallel change at line 50 (author filtering → pr-review-filtered-authors.yml) is a correct match; only this one is mismatched. Either retitle the section or drop the link.
|
Reviewed across code quality, security, performance, test coverage, and docs accuracy. The individual fixes here are well-motivated and the comments explaining why (the Things I verified as clean, since they were the obvious places to look:
Smaller items, take or leave:
🤖 Generated with Claude Code |
2 participants
No description provided.