Add variable_prefix input; upgrade to node24 + bump deps#2
Conversation
Adds an optional `variable_prefix` action input that prepends a string to every exported variable name (log, env, and output exporters alike), so callers can namespace exports and avoid collisions with other steps. The prefix is concatenated literally — the caller controls any separator (e.g. `MYAPP_`). Empty value (the default) is a no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- action.yml: runtime upgraded `node20` -> `node24` (latest supported
GitHub Actions JS runtime).
- package.json:
- engines.node: `>=20` -> `>=24`.
- @actions/core: ^1.10.1 -> ^2.0.3 (latest CJS major; 3.x is
ESM-only and would require migrating the build setup to ESM).
- ajv: ^8.11 -> ^8.17.
- @types/node: ^22 -> ^24, typescript ^5.5 -> ^5.9, ncc ^0.38.1 -> ^0.38.4.
- Jest stack to v30: jest, @jest/globals, @types/jest, jest-circus.
- ts-jest ^29.2 -> ^29.4, prettier-eslint ^16.3 -> ^16.4.
- Added @typescript-eslint/eslint-plugin as an explicit devDep
(was previously only resolved transitively; the bumps above
dropped that transitive install and broke `npm run lint`).
- ESLint stack kept at 8.57.1: 9/10 drop `.eslintrc` and require a
flat-config migration that's out of scope here.
- Workflows: actions/checkout v3 -> v5.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated via `npm run package` to pick up the variable_prefix support and the upgraded @actions/core / runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/integration-test.yml:
- Line 12: The workflow currently uses an unpinned checkout action "uses:
actions/checkout@v5" and is missing the persist-credentials setting; update each
step that uses actions/checkout (the occurrences of "uses: actions/checkout@v5")
to pin to a specific commit SHA (replace `@v5` with the exact commit SHA for
actions/checkout) and add persist-credentials: false under with: to prevent
token persistence; apply this change to all occurrences (the steps that
currently reference actions/checkout) so every checkout step is both SHA-pinned
and includes persist-credentials: false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1e4c59c-999e-4207-beab-7f38b2c9538a
⛔ Files ignored due to path filters (6)
dist/index.jsis excluded by!**/dist/**,!dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map,!dist/**,!**/*.mapdist/licenses.txtis excluded by!**/dist/**,!dist/**dist/sourcemap-register.jsis excluded by!**/dist/**,!dist/**package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (8)
.github/workflows/integration-test.yml.github/workflows/release.yml.github/workflows/unit-test.ymlREADME.md__tests__/exporter.test.tsaction.ymlsrc/exporter.tssrc/main.ts
## What Set `persist-credentials: false` on every `actions/checkout` step in `integration-test.yml` and `unit-test.yml`. These workflows do not run any subsequent `git` commands that need the token, so dropping the in-repo credential is defense-in-depth with no behavior cost. Skipped `release.yml` because it does `git fetch --prune --unshallow --tags` immediately after checkout and relies on the persisted token. ## Why CodeRabbit flagged the unpinned `actions/checkout@v5` and missing `persist-credentials: false`. Addressing the credential-persistence part here; declining SHA pinning as out of scope for this PR (the org-wide posture across sagansystems repos uses tagged refs). This commit supports [sc-191799]
## What Replace `kanga333/config-value-exporter@main` with an inline `jq` step that reads `.version` from `package.json` and writes it to `$GITHUB_OUTPUT` as `result` — preserving the existing `steps.version.outputs.result` references downstream. ## Why Removes the last third-party `kanga333/*` action dependency in this fork. `jq` is preinstalled on `ubuntu-latest` runners, so no setup is required, and the intent (pull a value out of a JSON file) is short enough to read inline. This commit supports [sc-191799]
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/unit-test.yml (1)
1-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit minimal permissions for CI.
Line 1 introduces workflow metadata changes, but there is still no
permissionsblock. Please pin token scope explicitly for least privilege.Suggested patch
name: unit test on: # rebuild any PRs and main branch changes pull_request: push: branches: - master +permissions: + contents: read + jobs:🤖 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/unit-test.yml around lines 1 - 10, Add an explicit top-level permissions block to the workflow to pin token scope to least privilege: update the "unit test" workflow YAML and add a permissions section (above jobs/build_and_test) that grants only the minimum scopes required for CI (for typical unit tests use e.g. "contents: read" and any other specific minimal scopes your actions need such as "id-token: write" only if required), replacing broad defaults; ensure the permissions block is present at the workflow root so the "build_and_test" job runs with the restricted token..github/workflows/release.yml (1)
1-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare explicit release-job token permissions.
No
permissionsblock is defined, and this workflow publishes releases. Set explicit scopes to avoid over-broad defaults.Suggested patch
on: push: branches: - master paths: - "package.json" name: Create Release if version is bumped +permissions: + contents: write + jobs:🤖 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 1 - 12, The workflow lacks an explicit permissions block for the release job; add a permissions stanza (preferably at the top-level or under the job named "build") granting the minimal scopes required to create a release, e.g. "contents: write" (and optionally "packages: write" if you publish packages), so the job "build" / name "Create Release" uses the OIDC/GITHUB_TOKEN with least privilege when publishing releases..github/workflows/integration-test.yml (1)
1-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege workflow permissions.
Line 1 defines the workflow but no
permissionsblock is set, so token scope depends on defaults. Please set explicit minimal permissions for this CI workflow.Suggested patch
name: integration test on: # rebuild any PRs and main branch changes pull_request: push: branches: - master +permissions: + contents: read + jobs:🤖 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/integration-test.yml around lines 1 - 8, The workflow declared by "name: integration test" lacks an explicit top-level permissions block, so add a top-level permissions: mapping that grants only the least-privilege scopes your jobs need (instead of relying on defaults). Update the workflow to include a permissions block (the symbol to add is permissions) and enumerate only required scopes such as contents: read and any other specific scopes your integration jobs require (e.g., packages: read, id-token: write) — remove any unneeded scopes; ensure the block sits at the top level of the workflow YAML so all jobs inherit the explicit least-privilege token.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In @.github/workflows/integration-test.yml:
- Around line 1-8: The workflow declared by "name: integration test" lacks an
explicit top-level permissions block, so add a top-level permissions: mapping
that grants only the least-privilege scopes your jobs need (instead of relying
on defaults). Update the workflow to include a permissions block (the symbol to
add is permissions) and enumerate only required scopes such as contents: read
and any other specific scopes your integration jobs require (e.g., packages:
read, id-token: write) — remove any unneeded scopes; ensure the block sits at
the top level of the workflow YAML so all jobs inherit the explicit
least-privilege token.
In @.github/workflows/release.yml:
- Around line 1-12: The workflow lacks an explicit permissions block for the
release job; add a permissions stanza (preferably at the top-level or under the
job named "build") granting the minimal scopes required to create a release,
e.g. "contents: write" (and optionally "packages: write" if you publish
packages), so the job "build" / name "Create Release" uses the OIDC/GITHUB_TOKEN
with least privilege when publishing releases.
In @.github/workflows/unit-test.yml:
- Around line 1-10: Add an explicit top-level permissions block to the workflow
to pin token scope to least privilege: update the "unit test" workflow YAML and
add a permissions section (above jobs/build_and_test) that grants only the
minimum scopes required for CI (for typical unit tests use e.g. "contents: read"
and any other specific minimal scopes your actions need such as "id-token:
write" only if required), replacing broad defaults; ensure the permissions block
is present at the workflow root so the "build_and_test" job runs with the
restricted token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: febe59d3-eb83-4c8d-8f92-f51b37de7de9
📒 Files selected for processing (3)
.github/workflows/integration-test.yml.github/workflows/release.yml.github/workflows/unit-test.yml
jeezcake
left a comment
jeezcake
left a comment
There was a problem hiding this comment.
Review Summary
The variable_prefix feature is cleanly implemented: withPrefix wraps each exporter at construction time so callers don't branch on the prefix, and the empty-prefix fast path keeps the no-op default free. Wiring through main.ts -> getExporters is straightforward, and the action input has a clear description.
A few suggestions inline — none blocking.
Verified externally:
actions/checkout@v6exists (latest is v6.0.2). 👍- The huge
dist/index.jsandpackage-lock.jsondeltas are expected from the@actions/corev1→v2 major bump plus the Node 24 / TypeScript / Jest 30 refresh. Worth a quick spot-check that nothing in@actions/corev2 changed semantics forgetInput/setOutput/exportVariable/info/setFailed(the usages here), but I don't see any that would affect this code.
Things I like:
- The integration test repo + step-by-step verification plan in the PR description is exactly what's needed for an Action whose only real runtime test is "GitHub actually runs it."
withPrefixreturningfnunchanged for empty prefix is a nice touch — keeps the default path identical to today's behavior at the function-identity level.getInput('variable_prefix')defaults to''so the default flows through cleanly without a conditional inmain.ts.
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: kanga333/variable-mapper@master |
There was a problem hiding this comment.
Suggestion (consistency): This new example uses kanga333/variable-mapper@master, but the integration-test workflow now uses sagansystems/variable-mapper@master for its readme-example jobs. The rest of the README still references kanga333, so matching the existing convention is defensible — but since you're adding a brand-new section in this PR, it would be a good moment to use sagansystems/variable-mapper@master here (and optionally do a one-pass update of the older examples in the same commit). Folks copy/pasting from README expect the example to point at the fork they're consuming.
| "env2": "value2" | ||
| } | ||
| } | ||
| export_to: env |
There was a problem hiding this comment.
Suggestion (demonstrate the docs claim): The action input description and PR body emphasize that variable_prefix is applied across log, env, and output targets, but this example only exercises env. Consider export_to: log,env,output plus a - run: echo "${{ steps.export.outputs.MYAPP_env1 }}" step (and add an id: to the action step) so the example visibly shows the prefix landing on every destination — matches the testing recipe you already wrote in the PR description.
| const [logExporter] = exporter.getExporters('log', '') | ||
| logExporter('key', 'value') | ||
| assertWriteCalls([`export key: value${os.EOL}`]) | ||
| }) |
There was a problem hiding this comment.
Suggestion (coverage): Both new tests exercise only the log exporter. The withPrefix wrapper is structurally identical for the other two targets, but a regression in the switch-case wiring (e.g., forgetting to wrap core.exportVariable or core.setOutput) wouldn't be caught here. A small jest.spyOn(core, 'exportVariable') / jest.spyOn(core, 'setOutput') test confirming each is called with MYAPP_key/value when getExporters('env,output', 'MYAPP_') is invoked would close that gap and is cheap to add.
| export function exportLog(name: string, val: string): void { | ||
| core.info(`export ${name}: ${val}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit (optional): Since withPrefix is module-private and pure, you could inline this as an arrow expression at the call sites — but the named-helper form is more readable, and the early-return-fn short-circuit for empty prefix is worth a comment-free function on its own. Leaving as-is is fine; flagging only to note I considered it.
| runs: | ||
| using: 'node20' | ||
| using: 'node24' | ||
| main: 'dist/index.js' |
There was a problem hiding this comment.
Question: Worth verifying in the testing step that GitHub Actions has Node 24 GA as a supported using: runtime — the docs list node20 as the current default, with node24 recently added. The PR's testing plan covers this ("in the 'Set up job' logs, confirm GitHub reports Node.js 24.x"), so this is just a flag that if Node 24 ends up unavailable on the runner, the action will fail to start with no useful error. If that becomes a problem, node20 would still work.
3 participants
What
Adds a new
variable_prefixaction input that prepends a literal string to every exported variable name across all destinations (log,env,output). Useful for namespacing exports to avoid collisions when this action is used multiple times in a workflow.Also upgrades the action runtime to
node24(latest supported GitHub Actions JavaScript runtime), bumps direct dependencies, and rebuildsdist/.The prefix is concatenated literally (
${prefix}${name}) so callers control any separator (e.g. passMYAPP_to getMYAPP_env1). Empty value is the default and is a no-op.Why
The branch carried a half-finished
variable-prefixinput with inverted logic that was never wired intogetExporters. Finishing the feature here, paired with a runtime + dependency refresh so the action keeps working as GitHub deprecates older Node runtimes.This commit supports [sc-191799]
Testing
In a separate test repo (or this repo on a throwaway branch), add a workflow that uses this action from the PR branch and verify the prefix is applied to all destinations.
In a test repo, create
.github/workflows/verify-prefix.yml:Trigger the workflow from the GitHub Actions UI ("Run workflow"). Confirm:
variable-mapperstep logs, the export lines readexport MYAPP_env1: value1andexport MYAPP_env2: value2(prefixed in the log destination too).Verify the no-op default: change
variable_prefix: 'MYAPP_'tovariable_prefix: ''(or remove the line), update the assertions to use$env1/outputs.env1, re-run the workflow, and confirm it passes — no prefix is applied when the input is empty.Verify the runtime upgrade: in the same step's "Set up job" logs, confirm GitHub reports the action runs on Node.js 24.x.
Release Notes
variable-mappernow accepts avariable_prefixinput that prepends a literal string to every exported variable name acrosslog,env, andoutputtargets. The default is empty (no behavior change). The action now runs on the Node 24 runtime.