[code-simplifier] Simplify action_setup_otlp.cjs: dedupe conditional writeEnvLine calls - #52622
[code-simplifier] Simplify action_setup_otlp.cjs: dedupe conditional writeEnvLine calls#52622github-actions[bot] wants to merge 2 commits into
Conversation
Extract a writeIfValid helper to remove repeated if(isValid...) writeEnvLine(...) patterns for trace-id/span-id/parent-span-id output and env propagation. Behavior unchanged; all log messages and file writes are identical. Source: recent activity around #52565 (shared prompt extraction) and #52542 (shared reporting import) motivated looking for similar duplication-reduction opportunities in actions/setup/js. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review. No actionable review findings for PR #52622 after analyzing the changed lines; no GitHub write beyond completion signal needed.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52622 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #52622 · auto · 17.2 AIC · ⌖ 4.5 AIC · ⊞ 6.8K
Comment /ponytail to run again
|
|
||
| // Always expose trace ID as a step output for cross-job correlation, even | ||
| // when OTLP is not configured. This ensures needs.*.outputs.setup-trace-id | ||
| // is populated for downstream jobs regardless of observability configuration. |
There was a problem hiding this comment.
L179-189: yagni: 11-line helper (6 params + 8-line JSDoc) replacing 5 near-identical one-line if calls. Diff grew from 5 to 18 lines for the "dedup". Keep the plain if (isValidX(v)) writeEnvLine(...) lines, or shrink the helper to a 1-line arrow fn without the JSDoc.
There was a problem hiding this comment.
Pull request overview
Refactors OTLP trace/span propagation to reduce repeated validation-and-write logic without changing behavior.
Changes:
- Adds a local
writeIfValidhelper. - Uses it for five
GITHUB_OUTPUTandGITHUB_ENVwrites.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/action_setup_otlp.cjs |
Deduplicates validation-gated environment/output writes. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Clean refactor. The new writeIfValid helper correctly eliminates the repeated if (isValid(value)) writeEnvLine(...) pattern without changing any behavior. All call sites are updated consistently.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.4 AIC · ⌖ 10.7 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor documentation suggestion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Clean extraction of repeated
if (isValid...) writeEnvLine(...)pattern into a well-documented helper - ✅ No behaviour change: same guards, same file writes, same log labels
- ✅ JSDoc comment aids future readers;
@paramcoverage is good - ✅ Build and targeted tests pass; no new failures introduced
Minor Issue
- The
@param {string} valueannotation should be{string | undefined}sinceprocess.env.*values can be undefined — see inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 24.2 AIC · ⌖ 7.32 AIC · ⊞ 7K
Comment /matt to run again
| * @param {string} fileLabel | ||
| * @param {(value: string) => boolean} isValid | ||
| */ | ||
| const writeIfValid = (filePath, key, value, logLabel, fileLabel, isValid) => { |
There was a problem hiding this comment.
[/codebase-design] The JSDoc types value as string, but environment variables (process.env.*) are string | undefined. The helper already passes value straight to writeEnvLine after the guard, so the practical risk is low — but the type annotation is misleading.
💡 Suggested fix
Change the @param annotation:
* `@param` {string | undefined} valueThis makes it clear to future readers why the isValid guard is necessary.
@copilot please address this.
|
@copilot Quick nudge: this PR looks mergeable. Please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Update complete on
Maintainer handoff: re-trigger CI on current HEAD, resolve the remaining thread if satisfied by commit |
Summary
Simplified
actions/setup/js/action_setup_otlp.cjsto remove repeated conditionalwriteEnvLineboilerplate in therun()function's trace/span propagation section.Files simplified
actions/setup/js/action_setup_otlp.cjsChanges
writeIfValid(filePath, key, value, logLabel, fileLabel, isValid)helper insiderun()to replace 5 nearly-identicalif (isValid...) writeEnvLine(...)lines (3 for GITHUB_OUTPUT, 2 for GITHUB_ENV) with single-line calls.isValidTraceId/isValidSpanId).Validation
make build— passesvitest run js/action_setup_otlp.test.cjs js/action_otlp.test.cjs— same pass/fail counts before and after the change (14 pre-existing failures at baseline, unrelated to this edit and due to environment/module-mocking issues in this sandbox, not the code change). No new failures introduced.make fmt— no diffs produced by formatter.Process notes
This run used the deterministic precomputed inputs (
recent-context.json,source-files.json,history-summary.json) without re-fetching PR/commit history via GitHub tools, per the token-efficiency guardrails. Candidate files were triaged with a scope-filter pass; most of the 20 candidates were sample/demo/small files or already well-organized (e.g.add_reaction.cjs) and were skipped.purity_scan.go(830 lines) was inspected but judged too risky to simplify without deeper review of its fixed-point analysis logic, so it was left untouched this run.Recent related activity in the repo (PR #52565 shared-prompt extraction, PR #52542 shared reporting import) reflects an ongoing push toward de-duplicating repeated code across the workflow tooling, which motivated searching for similar opportunities in
actions/setup/js.