Repository navigation
[jsweep] Clean workflow_job_durations.cjs - #69615
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Thanks for the automated cleanup — a focused refactor with new tests. Looks ready for review!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I did not find a changed-line issue here that justifies blocking the merge.
Notes
This patch is mostly a mechanical cleanup: helper extraction, equivalent object guards, an unused catch binding removal, and some additional tests. The new tests add a bit of duplication against cases already covered earlier in the file, but that is maintenance noise rather than a correctness regression.
Static review only: I did not execute the test suite.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 18.8 AIC · ⌖ 5.78 AIC · ⊞ 12.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /improve-codebase-architecture (per pr-triage: refactor_cleanup). No actionable issues found — approving.
📋 Key Themes & Highlights
Verification
isPlainObject(value)is verified equivalent to the original inlinevalue && typeof value === "object" && !Array.isArray(value)checks for both the cached-durations guard and the needs/names/eligible guard.JOB_ID_PATTERNis the same regex, now de-duplicated across its two use sites.catch (error) {}→catch {}is safe:errorwas unused in that branch and the comment above it (never log response/metadata) is preserved.- New JSDoc on
displayName/matchesNamematches the existing single-line multi-@paramconvention used elsewhere (e.g.agent_session.cjs,claude_runtime.cjs).
Positive Highlights
- ✅ Pure extraction, no logic changes — matches the PR description exactly.
- ✅ 5 new tests meaningfully cover the extracted branches: invalid cached durations (bad id, negative, array, non-JSON), valid-cache short-circuit, non-object metadata per env var, missing run context, and invalid id/non-terminal result skipping.
- ✅ Test coverage increased from 9 → 14 without weakening any existing assertions.
Note
- Could not execute the test suite directly in this sandbox (no network access to npm registry / no local node_modules for
actions/setup/js); relied on static diff verification. The PR body states formatting/lint/typecheck/tests all pass, which is consistent with the diff reviewed here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 29.7 AIC · ⌖ 16.7 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
The new invalid-ID regression test still passes when the validation guard is removed.
1 open issue
What changed in this PR
Cleans up gh-aw’s Actions job-duration collector without intended behavior changes.
Changes:
- Extracts shared validation helpers, adds JSDoc, and removes an unused catch binding.
- Adds tests for cached durations, metadata validation, and job filtering.
| File | Description |
|---|---|
| actions/setup/js/workflow_job_durations.test.cjs | Adds five validation and filtering tests. |
| actions/setup/js/workflow_job_durations.cjs | Consolidates validation and documents helper parameters. |
Review effort: Max
| const options = fixture({ "bad id": { result: "success" }, agent: { result: "skipped" } }, { agent: "Agent" }); | ||
| expect(await collectJobDurations(options)).toEqual({}); |
|
🎉 This pull request is included in a new release. Release: |
jsweep: workflow_job_durations.cjs
Context: github-script.
@ts-checkalready enabled (no@ts-nocheck).Changes
JOB_ID_PATTERNandisPlainObjecthelpers, replacing repeated inline checksdisplayNameandmatchesNameTests
Validation
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓workflow_job_durationstests all pass ✓. The fullnpm run test:jsrun shows unrelatedsend_otlp_span.test.cjsfailures (missing/tmp/gh-aw/aw_info.jsonin this environment), untouched by this change.