Repository navigation
fix(ci): paginate GitHub and TGit comments and rejection checks - #1783
Conversation
|
No findings. The specified diff contains no issues I’m confident warrant reporting. The PR description provides sufficient testing: build, typecheck, targeted lint, 59 passing tests, and a representative real-CLI run confirming that a page-2 marker updates comment 123 rather than creating a duplicate. Simulated network calls and deferred live TGit verification are explicitly disclosed. I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving. I built 654f53b1 and the merge-base (05fe72a2) and ran teamai ci comment and ci write --individual true against a local fake API that serves the comment list in pages:
- GitHub: with the marker comment on page 2, head updates it (PATCH) and posts no duplicate. Base posts a second comment.
- GitHub: a thumbs-down on page 2 of the reactions blocks the write on head. Base writes and pushes the learning.
- TGit: with the marker on page 2, head updates it via
x-next-page. Base posts a duplicate. TGit's API base is a hard-coded constant, so I ran this against a dist copy that points to the fake. A live TGit run is still unobserved, as you said. - The lookup stops on the last page and returns early on a match. Exactly 20 full pages with no next page returns without an error.
[P3 nit] Over 2000 items, ci comment and ci write now exit 1 where main exits 0. I reproduced it with every page full. I don't count this as breaking: main gets the same case wrong silently (a duplicate comment, or a missed rejection), and the GitLab path on main already throws at the same page cap. A short sentence in the body would still help anyone who hits it.
[P2 non-blocking] The over-limit error gives no next step. src/ci/gitlab-paginate.ts:61-64 says what happened and why, but not what to do. Something like …, so ci cannot read all of them. Delete or hide old comments on the PR, then rerun. would cover it.
[P3 nit] The literal 20 appears five times (mr-comment.ts:178, :292; read-rejections.ts:87, :111, :165), while MAX_NOTE_PAGES is already defined in both files. Also, the header of gitlab-paginate.ts:4 still calls it the GitLab helper, but it now serves GitHub and TGit too.
CI review: it reports no findings. I agree there is no code defect. On testing, its view that the body is sufficient holds for the GitHub comment path. The rejection path and TGit are covered by the runs above.
Merge risk: low
- Alignment: good. It matches the body. The over-limit exit is mentioned in one clause.
- Impact: medium. It changes the GitHub and TGit comment lookup and the rejection read that gates
ci write. Tokens and redirects are handled as before. A long list now costs more API calls. - Verification: good. Built-CLI runs before and after for both providers, covering the comment and rejection paths and the page cap, and CI is green. Not run: live TGit, and the GitLab path through the real CLI.
- Rollback cost: low. Code only, no stored format.
- Error detection: good. A list that used to be read partially without any sign now fails visibly.
GitHub and TGit CI comment lookups and rejection checks only read the first page (up to 100 items). An existing TeamAI marker beyond that page causes a duplicate comment; a later rejected suggestion or GitHub thumbsdown can be missed when writing knowledge.
Summary
Page through GitHub comments/reactions and TGit notes, stopping marker lookups and rejection reaction checks when their match is found. The existing paginateGitLab helper validates each page; TGit uses x-next-page and GitHub opts into Link rel=next. A full page without a next-page header ends the scan. It constructs numbered requests rather than following response URLs. API failures and the 20-page limit fail explicitly instead of silently approving a truncated result.
Type of Change
Test Plan
npm run buildpassed.npm run typecheckpassed.oxlint --type-aware --deny-warnings --report-unused-disable-directivespassed on the four changed TypeScript files.npx vitest run src/__tests__/ci-pagination.test.ts src/__tests__/read-rejections.test.ts src/__tests__/mr-comment.test.ts: 59/59 pass; the 8 new cases fail on main without thesrc/cichanges.Real CLI record: after building,
teamai ci commentran with a drafted learning,--individual false --preview false, and a local fetch stub returning 100 unrelated comments followed by marker comment 123. It printedMR comment updatedand{}, exited 0, and the stub confirmed PATCH of comment 123. Network calls were simulated; no live GitHub or TGit write was performed. Live TGit verification is deferred.Related Issues
No issue filed; found alongside the existing GitLab pagination paths.
Notes for Reviewers
This change was prepared with AI assistance (Codex).