feat(ci): move Ready for Review items to In Review on reviewer activity

[beatrizmcouto]

Summary

• On the Compliance Automation planning board, items in Ready for Review now move to In Review when a non-author human comments on the issue/PR or submits a pull request review.
• The change runs in the existing 5-minute sync_project_board job (no new workflow). Author comments, bots, and unsubmitted (PENDING) reviews do not count, so an author marking their own work ready does not bounce it into In Review.
• Status names are prefixes in project-sync-config.yml so they still match the emoji options (Ready for Review 👀, In Review 🏁).

Related Issues

N/A — board automation requested for the Compliance Automation planning project.

Review Hints

• Start with advance_ready_for_review and content_has_reviewer_activity in scripts/sync-project-board.py.
• tests/test_sync_project_board.py covers the positive path (comment / submitted review) and the negatives (author, bot, PENDING, wrong column, missing Status options, field-write failure).
• After merge, the next scheduled sync tick will apply this to current Ready for Review items that already have later reviewer comments.

Test plan

• pytest tests/test_sync_project_board.py (already green locally: 74 passed)
• Confirm Status option prefixes in project-sync-config.yml still match the board (Ready for Review, In Review)
• Optional dry-run: Actions → Sync Compliance Automation Project Board with dry_run enabled, then a real run
• Put a test issue in Ready for Review, have a second person comment, wait up to 5 minutes, confirm it moves to In Review
• Confirm an author-only comment does not move the item

Made with Cursor

[github-actions]

CRAP Load Analysis

No Go code changes detected in this PR. No CRAP impact.

[em-redhat]

PR #554 Review: feat(ci): move Ready for Review items to In Review on reviewer activity

Verdict: APPROVE with minor suggestions

---

Summary

This PR adds board automation to the sync-project-board.py script: items in Ready for Review status are moved to In Review when a non-author human comments on the issue/PR or submits a PR review. The implementation is well-structured, thoroughly tested, and consistent with the existing codebase patterns.

---

Strengths

Architecture -- The new feature follows the exact same patterns as the existing Organization backfill and Priority copy features: config-driven prefixes, stats tracking, error handling via SYNC_EXCEPTIONS, and delegation to set_single_select for dry-run safety. The ProjectFields dataclass refactor is a meaningful DRY improvement over the previous tuple return from get_project().

Function decomposition -- The review-advance logic is broken into small, single-responsibility functions (actor_login, is_bot_actor, is_reviewer_actor, comment_is_reviewer_activity, review_is_reviewer_activity, content_has_reviewer_activity) that are individually testable. The _paginate_project_item_nodes extraction for reuse is clean.

Test coverage -- 397 new test lines covering: happy paths (issue/PR), author exclusion, bot exclusion ([bot] suffix, BOT association, ghost), timestamp boundary enforcement, PENDING review exclusion, status option prefix matching (exact preference, ambiguity handling), missing options, write failures, page size config propagation, and sync() integration. Existing test classes properly isolate the new function via monkeypatch.

Edge cases handled well -- The status_option_for_prefix function handles emoji-decorated status names (e.g., "Ready for Review 👀") via prefix matching with exact-match preference and ambiguity rejection. parse_github_datetime handles the Z suffix and garbage input. is_bot_actor catches both authorAssociation: BOT and the [bot] suffix convention.

Docs & config -- CHANGELOG, PROJECT_BOARD_SYNC.md, and project-sync-config.yml all updated. The inline comments in the config explaining prefix matching are helpful.

---

Findings

MEDIUM -- fetch_content_review_activity f-string in GraphQL query

scripts/sync-project-board.py:932-965

The function uses an f-string to embed REVIEW_ACTIVITY_PAGE_SIZE into the GraphQL query. While this is safe (the constant is a module-level integer, not user input), every other GraphQL query in this file uses plain strings with $variable bindings. The inconsistency is worth noting. This could be made consistent by passing page_size as a GraphQL variable, but it is cosmetic -- GitHub's ProjectV2 API last: argument does not accept variables in all contexts, so the current approach may be intentional.

Disposition: No change required. Informational.

LOW -- No test for advance_ready_for_review in dry-run mode

The function delegates dry-run behavior to set_single_select (which is tested separately), so the stats increment in dry-run mode is implicitly tested. However, adding a single test confirming advance_ready_for_review with a dry-run client still calls set_single_select and increments review_status_set would strengthen confidence that the full chain works under dry-run.

Disposition: Optional enhancement. Not a blocker.

LOW -- N+1 GraphQL query pattern in advance_ready_for_review

scripts/sync-project-board.py:1005-1019

For each board item in "Ready for Review" status, a separate fetch_content_review_activity GraphQL call is made. On a board with many items in that status, this could become expensive. However:

• The GitHub ProjectV2 API does not support server-side status filtering
• list_board_status_items already filters client-side, so only matching items trigger fetches
• This runs on a 5-minute cron, not in a hot path
• GitHub's rate limit (5000 points/hour for GraphQL) is unlikely to be hit

Disposition: Acceptable for current scale. Worth monitoring if the board grows substantially.

INFO -- updatedAt semantics as "since" timestamp

scripts/sync-project-board.py:1009 uses the Status field's updatedAt as the since boundary. This is the correct field -- it tracks when the Status single-select value last changed. If GitHub ever changes updatedAt semantics (e.g., to include board-level metadata changes), this could cause false positives. The PR documentation correctly describes the behavior.

Disposition: No action needed.

---

Convention Pack Compliance

Rule	Status
CS-001 (Conventional Commits)	Pass -- feat(ci): prefix
CS-002 (Signed-off-by)	N/A (not checked at PR level)
CS-003 (Assisted-by trailer)	Pass -- Co-authored-by present
TC-001 (Test coverage)	Pass -- 397 new test lines
TC-002 (Edge cases)	Pass -- author, bot, timestamp, ambiguity
DR-001 (DRY)	Pass -- _paginate_project_item_nodes extraction
AP-001 (Error handling)	Pass -- SYNC_EXCEPTIONS catch pattern
SC-001 (No secrets)	Pass

CI Status

All 18 checks passed. Linters clean (YAML warnings are pre-existing on lines 25/33, not in new code). All 242 tests pass.

---

Decision

APPROVE. The implementation is solid, well-tested, and follows established codebase patterns. The findings above are informational or optional improvements -- none are blockers.

---

Reviewed by OpenCode (claude-opus-4-6)

[beatrizmcouto]

Thanks @em-redhat — I went through your findings and landed them in 7608bb3:

• GraphQL page size: last: is now $pageSize: Int! instead of an f-string interpolate, matching the other queries.
• Dry-run test: advance_ready_for_review with a dry-run client still calls set_single_select and increments review_status_set.
• N+1 fetches: Ready for Review items are collected first, then comments/reviews are loaded in nodes(ids:) batches of 50.
• updatedAt since-boundary: Documented in PROJECT_BOARD_SYNC.md and on _ready_for_review_candidates that this is the Status option change, not other board metadata.

Would you mind taking another look?

[beatrizmcouto]

@em-redhat @marcusburghardt Coul I get a review/re-review on this one please?

[github-actions]

Stale Review Alert

This PR has review requests pending for 5+ business days:

• @marcusburghardt (5 business days)

Reviewer: Complete the review, or if unavailable, unassign yourself so another team member can pick it up.
Author (@beatrizmcouto): If the reviewer is unavailable, re-request review from another team member or convert this PR to draft if it's not ready.

This reminder will not repeat for 3 days.

[em-redhat]

PR #554 Review (v2): feat(ci): move Ready for Review items to In Review on reviewer activity

Verdict: APPROVE

---

CI Status

Check	Status	Classification
CodeQL	PASS	N/A
Analyze (actions)	PASS	N/A
Analyze (javascript-typescript)	PASS	N/A
Analyze (python)	PASS	N/A

Local Tool Results

Tool	Status	Notes
make lint (yamllint + ruff)	PASS	YAML warnings on lines 25/33 are pre-existing
make test (pytest)	PASS	242 tests pass in 1.11s

Walkthrough

File	Change	Focus
CHANGELOG.md	Add entry for review-status advance feature	documentation
docs/PROJECT_BOARD_SYNC.md	Document behavior, updatedAt semantics, and config keys	documentation
project-sync-config.yml	Add ready_for_review_status and in_review_status config keys	ci-cd
scripts/sync-project-board.py	Add review-status advance logic with batched GraphQL fetches	ci-cd
tests/test_sync_project_board.py	Add 521 lines of tests including dry-run, batching, and failure paths	test-quality

Summary

This PR adds board automation that moves items from Ready for Review to In Review when a non-author human comments or submits a PR review. The second commit (7608bb3) addresses all findings from the previous review: GraphQL page size is now a proper $pageSize variable, activity fetches are batched via nodes(ids:), dry-run behavior is explicitly tested, and Status.updatedAt semantics are documented in both the code and docs.

Prior Review Findings — Resolution Status

Finding	Severity	Status
f-string in GraphQL query (inconsistent with $variable pattern)	MEDIUM	Resolved — now uses $pageSize: Int! variable binding
No dry-run test for advance_ready_for_review	LOW	Resolved — test_dry_run_calls_set_single_select_and_counts added
N+1 GraphQL query per Ready for Review item	LOW	Resolved — batched nodes(ids:) query with REVIEW_ACTIVITY_BATCH_SIZE = 50
updatedAt semantics as "since" timestamp	INFO	Addressed — documented in docs and _ready_for_review_candidates docstring

New in v2

The second commit also introduces:

• _ready_for_review_candidates() — extracted candidate filtering with clear updatedAt documentation
• _batched() — generic list chunking utility with validation
• test_records_batch_fetch_failure — verifies all candidates get error-counted on batch fetch failure
• test_fetches_ready_items_in_one_batch — confirms single-call batching with variable binding
• test_fetch_query_passes_page_size_as_variable — directly validates query structure
• test_fetch_skips_graphql_when_ids_empty — early return for empty ID list

Convention Pack Compliance

Rule	Status
CS-001 (Conventional Commits)	Pass — feat(ci): and fix(ci): prefixes
CS-002 (Signed-off-by)	Pass — both commits signed
CS-003 (Assisted-by trailer)	Pass — Assisted-by: Cursor Grok 4.6 on both commits
TC-001 (Test coverage)	Pass — 521 new test lines
TC-002 (Edge cases)	Pass — dry-run, batch failure, empty IDs, ambiguity
DR-001 (DRY)	Pass — _paginate_project_item_nodes and _batched extractions
AP-001 (Error handling)	Pass — SYNC_EXCEPTIONS catch pattern, batch error propagation
SC-001 (No secrets)	Pass

Verdict

APPROVE. All findings from the prior review have been addressed cleanly. The implementation is solid, well-tested (521 new test lines), and the batching improvement is a meaningful upgrade over the original N+1 pattern. No new issues found.

---

This review was generated by /review-pr (AI-assisted).

[beatrizmcouto]

@em-redhat I had to resolve merge conflicts and I need your review again please!🙏

[em-redhat]

PR #554 Review (v3): feat(ci): move Ready for Review items to In Review on reviewer activity

Verdict: APPROVE

---

CI Status

Check	Status	Classification
Standardized CI / Run linters	PASS	N/A
CRAP Load Analysis	PASS	N/A
CodeQL (actions, python, js-ts)	PASS	N/A
Trivy	PASS	N/A
Dependencies Review	PASS	N/A
OSV-Scanner / Trivy Source Scan	PASS	N/A
OpenSSF Scorecards	PASS	N/A

Local Tool Results

Tool	Status	Notes
make lint (yamllint + ruff)	PASS	YAML warnings on project-sync-config.yml lines 25, 33 are pre-existing (comments-indentation)
make test (pytest)	PASS	242 tests passed in 1.08s

Walkthrough

File	Change	Focus
scripts/sync-project-board.py	Add review-status advance: ProjectFields dataclass, batched activity fetch, _ready_for_review_candidates, advance_ready_for_review orchestrator	ci-cd
tests/test_sync_project_board.py	Add 521 lines of tests covering review-status prefixes, option matching, reviewer detection, advance flow, dry-run, batch splitting, error handling	test-quality
project-sync-config.yml	Add ready_for_review_status and in_review_status config keys with prefix matching comments	ci-cd
docs/PROJECT_BOARD_SYNC.md	Document behavior (item 7) and config keys	documentation
CHANGELOG.md	Add review-status advance entry under Unreleased > Added	documentation

Summary

This PR adds board automation that moves planning board items from Ready for Review to In Review when a non-author human comments or submits a PR review. The implementation is well-structured with proper batching (nodes(ids:) with REVIEW_ACTIVITY_BATCH_SIZE), parameterized GraphQL variables, comprehensive error handling, and thorough test coverage. All four findings from previous reviews (v1, v2) have been addressed. The two new commits since v2 are a clean merge of main (resolving conflicts with #544) and a ruff F821 lint fix (adding Optional import to tests).

Alignment

• No issues found. All changes are within stated scope.

Security

• No issues found. GraphQL uses proper $variable binding. No user-controlled input reaches queries. Batch sizes are bounded.

Constitution Compliance

• No issues found. Conventional commits, signed-off-by trailers, docstrings on all public functions, thorough test coverage.

Verdict

APPROVE

All findings from v1 and v2 reviews have been addressed. The two new commits (merge main, import fix) introduce no new concerns. Code quality is high with comprehensive test coverage across happy paths, edge cases, error handling, and dry-run behavior.

This review was generated by /review-pr (AI-assisted).