fix: address PR review findings and scope reviewer to diff only#23
Conversation
⚡ AgentMeter
All 12 runs
|
|
Findings:
Overall the rest of the diff looks reasonable, but these two changes can cause incorrect production data. |
|
Findings:
Otherwise the implementation looks solid, but I’d want tests for the gate failure path and the workflow-name fallback before merging. |
… name over misattributed companion
|
Findings
No other blocking issues stood out in the diff. |
…ate on transient list failure
|
No major correctness issues found in the changed code paths. The retry-once gate in Residual risk:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Found 4 type safety/code style issues in the diff:
Type annotations missing:
src/comment.ts:317-errorparameter needsunknowntypesrc/workflow-run.ts:205-firstErrorparameter needsunknowntypesrc/workflow-run.ts:211-secondErrorparameter needsunknowntype
Missing JSDoc:
src/workflow-run.ts:183-attemptCheckarrow function needs JSDoc block
Logic & tests: The retry logic and test coverage look solid. The regex anchor fix and trigger type preference changes are correct improvements.
Generated by Agent: Code Review for issue #23 · ● 104.3K
| if (!existing) return null; | ||
| return { id: existing.id, body: existing.body ?? '' }; | ||
| } catch { | ||
| } catch (error) { |
There was a problem hiding this comment.
Missing type annotation on error parameter. In strict TypeScript, all parameters should have explicit types:
} catch (error: unknown) {| workflowRunId: number; | ||
| }): Promise<boolean> { | ||
| try { | ||
| const attemptCheck = async (): Promise<boolean> => { |
There was a problem hiding this comment.
Missing JSDoc block for the attemptCheck function. Per code style requirements, every function should have a JSDoc comment describing its purpose, even internal arrow functions:
/**
* Attempts to verify the conclusion job has completed for this workflow run.
* `@returns` true if the conclusion job completed successfully, false otherwise
*/
const attemptCheck = async (): Promise<boolean> => {|
|
||
| try { | ||
| return await attemptCheck(); | ||
| } catch (firstError) { |
There was a problem hiding this comment.
Missing type annotation on firstError parameter. In strict TypeScript, all parameters should have explicit types:
} catch (firstError: unknown) {| ); | ||
| try { | ||
| return await attemptCheck(); | ||
| } catch (secondError) { |
There was a problem hiding this comment.
Missing type annotation on secondError parameter. In strict TypeScript, all parameters should have explicit types:
} catch (secondError: unknown) {
1 participant
Summary
workflow-run.ts: retrycheckConclusionJobCompletedonce on API error, then proceed instead of dropping the ingestcomment.ts:findExistingCommentnow throws on paginate error soupsertCommentaborts rather than falling through to create a duplicaterun.ts: only overwriteresolvedWorkflowNamewhenrunData.workflowNameis non-empty, preserving the companion workflow name as fallbackworkflow-run.ts: anchor issue-branch regex to full name (^agent/issue-N$) to prevent substring matches in longer branch namesrun.ts: preferresolvedTriggerTypeoverresolvedTriggerEventinbuildTriggerRefso a manualtrigger_numberwithouttrigger_eventdoesn't mislabel PRs as#NREADME.md: maketurnsfield conditional in printf snippet so an unsetTURNSvar doesn't write invalid JSON into the artifactagent-review.md: remove bash tools (no full-file reads), instruct reviewer to only comment on lines present in the diffTest plan