Skip to content

fix(ci): don't skip fork-PR label reconciliation on schedule/workflow_dispatch runs#234

Merged
edelauna merged 1 commit into
mainfrom
fix/truncation-display-count-2ac8m
Jul 4, 2026
Merged

fix(ci): don't skip fork-PR label reconciliation on schedule/workflow_dispatch runs#234
edelauna merged 1 commit into
mainfrom
fix/truncation-display-count-2ac8m

Conversation

@roomote

@roomote roomote Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

What problem this solves

label-pr-review-state.yml's reconcileLabels() skipped label mutations for any fork PR, regardless of what triggered the workflow run. That was overly broad: only pull_request/pull_request_review runs triggered from a fork actually get a read-only GITHUB_TOKEN (which 403s on label writes). The hourly schedule cron and manual workflow_dispatch runs always execute in the base repo's context with a full read/write token, even when the PR they're reconciling happens to come from a fork.

As written, this meant stale awaiting-author / awaiting-review / stale-awaiting-author labels on fork PRs would never get cleaned up by the scheduled reconciliation pass — only PR-triggered events could skip them, and those were exactly the runs that couldn't write labels anyway.

Why this change was made

Added isReadOnlyRun, which checks whether this run's own trigger (context.payload.pull_request) is itself a fork PR — true only for pull_request/pull_request_review events fired from a fork. reconcileLabels now skips a PR's label mutation only when both isReadOnlyRun && isForkPR(pr) are true, so schedule/workflow_dispatch runs reconcile fork PRs normally, while PR-triggered runs on a fork's own PR still avoid the guaranteed 403.

Verified against GitHub's documented GITHUB_TOKEN permission model (read/write for schedule/workflow_dispatch, read-only only for fork-triggered pull_request), and validated the updated skip logic against all six realistic trigger/PR-origin combinations.

User impact

Fork PRs now get their review-state labels correctly reconciled by the hourly schedule and manual dispatch runs, instead of silently never receiving label cleanup.

@roomote roomote Bot added the roomote:auto-resolve-conflicts Allow Roomote to auto-resolve merge conflicts for this PR label May 21, 2026
@roomote

roomote Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

No code issues outstanding; 1 check still pending. See task

  • Fallback truncation still walks the raw stored history, so earlier-condense conversations can report a truncation without actually shrinking the effective API history.
  • getEffectiveVisibleIndices() still counts messages that getEffectiveApiHistory() drops when they only contain orphan tool_result blocks after a fresh-start condense, so fallback truncation can still spend its slice on non-effective history.
  • CI is still pending for platform-unit-test (windows-latest).

@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@edelauna edelauna marked this pull request as ready for review May 21, 2026 13:22
@edelauna

Copy link
Copy Markdown
Contributor

@roomote can you review

@roomote

roomote Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

I started a PR review task for this request. See task

Comment thread src/core/context-management/index.ts Outdated
@roomote

roomote Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the latest follow-up review comment in 0755fa5.

Fallback truncation now derives its candidate indices from the same effective-history filtering used for provider requests, including fresh-start cleanup of orphan tool_result-only messages after condense. I also added a focused regression for that path.

Validation:

  • pnpm --dir src exec vitest run core/context-management/__tests__/truncation.spec.ts core/context-management/__tests__/context-management.spec.ts core/condense/__tests__/index.spec.ts
  • pnpm --dir apps/web-roo-code test

The code review item is fixed; GitHub checks are still running.

Comment thread src/core/context-management/index.ts Outdated
@github-actions github-actions Bot added the awaiting-review PR changes are ready and waiting for maintainer re-review label Jun 30, 2026
@edelauna edelauna force-pushed the fix/truncation-display-count-2ac8m branch from 0755fa5 to 2752d15 Compare July 4, 2026 02:15
@github-actions github-actions Bot removed the awaiting-review PR changes are ready and waiting for maintainer re-review label Jul 4, 2026
@edelauna edelauna changed the title [Fix] Context truncation shows inflated token counts after earlier condenses fix(ci): don't skip fork-PR label reconciliation on schedule/workflow_dispatch runs Jul 4, 2026
@edelauna edelauna added this pull request to the merge queue Jul 4, 2026
@github-actions github-actions Bot added the awaiting-review PR changes are ready and waiting for maintainer re-review label Jul 4, 2026
Merged via the queue into main with commit fa3af3f Jul 4, 2026
13 checks passed
@edelauna edelauna deleted the fix/truncation-display-count-2ac8m branch July 4, 2026 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-review PR changes are ready and waiting for maintainer re-review roomote:auto-resolve-conflicts Allow Roomote to auto-resolve merge conflicts for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant