Skip to content

fix(ci): paginate GitHub and TGit comments and rejection checks - #1783

Merged
SaulMoro merged 2 commits into
Tencent:mainfrom
pcbeingused333:fix/c1955-ci-pagination
Nov 8, 2026
Merged

SaulMoro merged 2 commits into
Tencent:mainfrom
pcbeingused333:fix/c1955-ci-pagination

Conversation

@pcbeingused333

Copy link
Copy Markdown
Contributor

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

  • Bug fix (non-breaking change that fixes an issue)

Test Plan

  • npm run build passed.
  • npm run typecheck passed.
  • Targeted oxlint --type-aware --deny-warnings --report-unused-disable-directives passed 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 the src/ci changes.
  • Added eight regression cases covering both providers' page-2 markers/rejections, page-2 GitHub reactions, later-page failures, the page limit, and short pages with next links.

Real CLI record: after building, teamai ci comment ran with a drafted learning, --individual false --preview false, and a local fetch stub returning 100 unrelated comments followed by marker comment 123. It printed MR comment updated and {}, 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).

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

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 SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@SaulMoro
SaulMoro merged commit e38701a into Tencent:main Nov 8, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants