fix(review): fix durationSeconds null bug, async misuse, param ordering, and Octokit type cast#24
Conversation
…ng, and Octokit type cast - run.ts: durationSeconds returns null (not 0) when timestamps are unavailable, fixing misleading "0s duration" in API payloads for runs with unknown timing - workflow-run.ts: remove async from parseAgentTokensZip — it uses only unzipSync and JSON.parse, both synchronous; the async wrapper added unnecessary overhead - workflow-run.ts: fix alphabetical param ordering in resolveTrigger, parseAgentTokensArtifact, and parseAgentStdioLog; add missing inline JSDoc comments on parseAgentTokensArtifact and parseAgentStdioLog params - comment.ts: replace @octokit/core Octokit import + runtime type casts with a local type alias from @actions/github getOctokit, eliminating unsafe assertions - comment.ts: fix alphabetical param ordering in findExistingComment and upsertComment; add missing inline JSDoc on findExistingComment params
⚡ AgentMeter
All 7 runs
Token breakdown
Model: claude-sonnet-4-5 · 7 turns · 78% cache hit rate |
|
Findings:
Overall the implementation is well structured, and the test coverage around token extraction, comment updates, and workflow-run resolution is a good sign. The main remaining risk is correctness under GitHub API edge cases, especially in |
…A in fast path checkConclusionJobCompleted now fails open (returns true) when both API attempts fail, rather than silently dropping the ingest. The server deduplicates by githubRunId so proceeding on an uncertain gate is safer than data loss. resolveTrigger now validates pull_requests entries against head_sha before accepting the first entry, matching the same SHA-validation logic already used in the branch-lookup fallback. On a SHA mismatch it falls through to the branch lookup rather than trusting a potentially stale entry. Tests: updated fail-closed → fail-open expectation; added two new tests covering SHA match success and SHA-mismatch fallthrough in the pull_requests fast path.
|
Two correctness issues stood out:
Everything else looked broadly solid, especially the PR/issue resolution and artifact parsing logic. |
…l for missing timestamps
workflow_run status: rawConclusion in workflow_run mode now reads from
github.context.payload.workflow_run.conclusion instead of inputs.status.
The action.yml default is 'success', which would silently misattribute
failed/timed-out runs when callers omit the status input. Falls back to
inputs.status for backward compatibility when the payload field is absent.
Timestamps: resolvedStartedAt / resolvedCompletedAt now default to null
(not '') when unavailable in workflow_run mode. IngestPayload.startedAt
and completedAt are updated to string | null. durationSeconds computation
handles null timestamps directly rather than relying on new Date('') → NaN.
This prevents invalid ISO-8601 empty strings from reaching the ingest API.
|
One regression to watch:
One additional risk:
Otherwise the PR looks directionally good, especially the SHA validation for PR attribution and the comment-upsert typing cleanup. |
…rom payload Conclusion gate: revert fail-open (previous commit) back to fail-closed. Without a guaranteed backend upsert-by-githubRunId contract, failing open risks duplicate or partial records on every gh-aw firing (~5x per run). Upgraded the terminal failure from core.warning to core.notice so it surfaces in the job summary and is not silently swallowed. Timestamps: change IngestPayload.startedAt/completedAt from string|null to optional string (?:). Payload construction now uses spread to omit the fields entirely when null, rather than sending explicit null/empty values. This is universally safe across API implementations that may not accept null.
|
Findings
Overall the structure is solid, but I’d fix those before merging. |
…ith explicit overrides comment.ts: add MAX_STORED_RUNS=20 cap on allRuns to prevent the comment from growing unboundedly and eventually exceeding GitHub's 65 536-byte limit. Collapsible summary label switches to 'Last 20 runs' when the cap is hit. workflow-run.ts: refactor checkConclusionJobCompleted to re-check once after a 3s delay when the conclusion job reads in_progress. A single stale jobs-API read should not permanently drop an ingest — the workflow_run event can arrive before the API reflects the terminal job state. Tests use vi.useFakeTimers() to avoid real delays; new test covers the lag→completed path. run.ts: isApproximate is now always false when the caller provides any explicit token override. Previously the flag could leak from regex-extracted baseTokens even when authoritative manual counts were supplied, making exact data appear uncertain in the dashboard and comment.
|
Findings:
|
…erride check comment.ts: parseExistingRuns regex now matches both 'All N runs' and 'Last N runs' summary labels, so the collapsible history is preserved across updates after the 20-run cap is hit. Without this, capped comments fell back to the 5-row table and dropped hidden history on the next update. run.ts: isApproximate is now only forced to false when all four token fields are explicitly provided (hasAllExplicit). A partial override — e.g. only input_tokens — leaves the remaining fields sourced from extracted or artifact data that may be approximate, so the flag from baseTokens is correctly preserved in that case.
|
Two production issues stood out:
Everything else looks reasonably solid, especially the token parsing and comment upsert path. |
…kflow A transiently stale jobs API could return an empty list for a real gh-aw run, causing the not_found path to proceed without gating and ingest on all ~5 workflow_run firings. The fix applies the same 3s re-check already used for in_progress: if not_found is confirmed on the second read we treat it as non-gh-aw and proceed; if the job appears (completed) we gate normally. Skipping finding 2 (trigger resolution for workflow_dispatch/push) as that is a feature request already covered by the explicit trigger_number input.
Overall the action wiring looks solid, but these three cases can misreport or mis-ingest production runs. |
…sponse shape run.ts: add early return when normalizeConclusion(inputs.status) === 'skip' before submitRun(). workflow-run mode already gates on skip (via resolveWorkflowRun), but inline mode had no equivalent guard — a caller passing status='skipped' (e.g. steps.agent.outcome) would ingest a run record unnecessarily. Also hoists normalizedStatus to a const so the two call sites no longer recompute. ingest.ts: add isValidIngestResponse type guard before returning the parsed JSON. A malformed 200 response (wrong shape, unexpected envelope) now returns null + warning instead of passing through undefined fields to outputs and the PR comment link.
|
Findings:
Otherwise the implementation looks solid, and the error handling around API failures is generally sensible. |
1 participant
Summary
run.ts:durationSecondsnow returnsnullinstead of0when timestamps are unavailable — fixes misleading "0s duration" being sent to the ingest API for runs with unknown timingworkflow-run.ts: removed spuriousasyncfromparseAgentTokensZip(uses onlyunzipSync+JSON.parse, both synchronous); fixed alphabetical param ordering inresolveTrigger,parseAgentTokensArtifact,parseAgentStdioLog; added missing inline JSDoc onparseAgentTokensArtifact/parseAgentStdioLogparamscomment.ts: replaced@octokit/core Octokitimport + runtime type casts with a localtype Octokit = ReturnType<typeof github.getOctokit>alias; fixed alphabetical param ordering and added missing inline JSDoc infindExistingComment/upsertCommentTest plan
npm run type-checkpasses (verified locally)npm run lintpasses with no warnings (verified locally)npm test) (verified locally)durationSeconds: nullreaches the API when run timestamps are absent (workflow_run mode with no timestamps)