Enable CI Fix bot for valkey-io/valkey-search - #46
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR extends CI fix polling and dispatch to support both ChangesCI Fix Multi-repo Support
Sequence Diagram(s)sequenceDiagram
participant Poller as comment_poll.main
participant Config as _target_repos
participant GitHub as poll_once
participant Dispatcher as dispatch
Poller->>Config: read CI_FIX_POLL_TARGET_REPO
Config-->>Poller: ordered deduplicated repositories
loop for each repository
Poller->>GitHub: poll_once(repository, lookback_minutes)
GitHub-->>Poller: matching comments or repository error
GitHub->>Dispatcher: dispatch matching CI fix
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/ci_fix/comment_poll.py (1)
257-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent no-op if override resolves to empty list.
If
CI_FIX_POLL_TARGET_REPOis set but parses to zero entries (e.g. empty string, only commas/whitespace),_target_repos()returns(), andmain()'s loop silently polls nothing — the only signal is "dispatched 0 fix(es)" in the log, indistinguishable from a normal quiet tick. Consider falling back to the default list when the parsed result is empty.♻️ Proposed fix
def _target_repos() -> tuple[str, ...]: raw = os.environ.get("CI_FIX_POLL_TARGET_REPO", _DEFAULT_TARGET_REPOS) repos = [entry.strip() for entry in raw.split(",") if entry.strip()] # Preserve order, drop duplicates. - return tuple(dict.fromkeys(repos)) + if not repos: + repos = [entry.strip() for entry in _DEFAULT_TARGET_REPOS.split(",")] + return tuple(dict.fromkeys(repos))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci_fix/comment_poll.py` around lines 257 - 267, The _target_repos() helper can return an empty tuple when CI_FIX_POLL_TARGET_REPO is set to only commas/whitespace, causing main() to do a silent no-op; update _target_repos() so it falls back to _DEFAULT_TARGET_REPOS whenever the parsed repos list is empty. Keep the existing dedupe/order behavior in _target_repos(), and make sure the polling loop in main() always receives at least one target repo unless explicitly configured otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/ci_fix/comment_poll.py`:
- Around line 257-267: The _target_repos() helper can return an empty tuple when
CI_FIX_POLL_TARGET_REPO is set to only commas/whitespace, causing main() to do a
silent no-op; update _target_repos() so it falls back to _DEFAULT_TARGET_REPOS
whenever the parsed repos list is empty. Keep the existing dedupe/order behavior
in _target_repos(), and make sure the polling loop in main() always receives at
least one target repo unless explicitly configured otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7983417-cfcb-40b1-a4f1-5eb4403959ba
📒 Files selected for processing (5)
.github/workflows/ci-fix-comment-poll.yml.github/workflows/ci-fix.ymlREADME.mdscripts/ci_fix/comment_poll.pytests/test_ci_fix_comment_poll.py
sarthakaggarwal97
left a comment
sarthakaggarwal97
left a comment
There was a problem hiding this comment.
sharing some comments. Looks simple otherwise.
| claim=claim_via_status, | ||
| ) | ||
| except Exception as exc: # noqa: BLE001 - one repo must not abort the tick | ||
| logger.warning("Skipping repo %s after error: %s", target_repo, exc) |
There was a problem hiding this comment.
what if all the repos failed due to some bad config / app installation etc. If it's just one maybe it's okay but if all the repos failed, we should fail the workflow instead of silently logging it.
also, let's see we can catch a non-generic exception as well.
There was a problem hiding this comment.
Now that this workflow supports multiple target repos, the concurrency key should include the repo as well as the PR number.
ci-fix-${{ inputs.pr }} makes valkey-io/valkey#123 and valkey-io/valkey-search#123 share the same queue even though they are unrelated branches. With cancel-in-progress: false, one long run can unnecessarily block the other.
Something like ci-fix-${{ inputs.repo }}-${{ inputs.pr }} would preserve same-PR serialization without cross-repo collisions.
The ci-fix engine is already repo-agnostic; only the workflow wiring was scoped to valkey-io/valkey. Extend that wiring to valkey-search: - ci-fix.yml: accept valkey-io/valkey-search in the repo gate and add it to the minted App token scope. - ci-fix-comment-poll.yml: add valkey-search to the poll token scope. - comment_poll: poll a comma-separated list of target repos (default now valkey and valkey-search), isolating per-repo read failures so one repo cannot abort the tick. No engine changes: diagnosis derives the reproduce command from the repo's own CI, and the verifier classifies the failed job's environment as before. Signed-off-by: Aksha Thakkar <thaakb@amazon.com>
4cfb5b8 to
a4f724f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 178-184: Update the README workflow documentation to state that
comment-triggered forwarding still requires a target-repository wrapper, while
workflow_dispatch remains available directly. Revise the claims about end-to-end
comments and App-token coverage, and align the Configuration section’s
repository scopes with both valkey-io/valkey and valkey-io/valkey-search.
In `@tests/test_ci_fix_comment_poll.py`:
- Around line 241-247: Reorder the target_repos fixture in the polling test so
owner/bad appears before owner/good, while preserving the assertions that one
dispatch occurs and both repositories are polled. This ensures the test observes
continuation after the first repository failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2060dee0-141f-49b6-ab31-114adafaaa70
📒 Files selected for processing (5)
.github/workflows/ci-fix-comment-poll.yml.github/workflows/ci-fix.ymlREADME.mdscripts/ci_fix/comment_poll.pytests/test_ci_fix_comment_poll.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci-fix-comment-poll.yml
- .github/workflows/ci-fix.yml
…tion test - README: the ci-fix Configuration section listed minted token scopes on valkey-io/valkey only; ci-fix.yml now scopes the token to both valkey and valkey-search, so document both consistently. - test_poll_all_repos_isolates_single_repo_failure: put the failing repo first so the test proves polling continues to the healthy repo after an exception, rather than passing even if the loop aborted on failure. Co-authored-by: Aksha Thakkar <thaakb@amazon.com> Signed-off-by: AkshaThakkar1812 <akshathakkar@gmail.com>
2 participants
What
Extends the on-demand CI Fix bot (
@valkeyrie-bot fix <ci-link>) to work onvalkey-io/valkey-searchbackport PRs, in addition tovalkey-io/valkey.Why
The
scripts/ci_fix/engine is already repo-agnostic — it threads the PR's repo through the gate, diagnosis, and verifier, and derives the reproduce command from the target repo's own CI rather than any hardcoded command list. The only thing scoping the bot tovalkey-io/valkeywas the workflow wiring:ci-fix.ymlhard-refused any repo!= valkey-io/valkey, and minted an App token scoped tovalkeyonly.ci-fix-comment-poll.ymlminted a poll token scoped tovalkey(+ agent repo), and the poller only ever scanned a single target repo.Changes
.github/workflows/ci-fix.yml— acceptvalkey-io/valkey-searchin the repo gate (now acaseallowlist) and addvalkey-searchto the minted App token scope..github/workflows/ci-fix-comment-poll.yml— addvalkey-searchto the poll token scope.scripts/ci_fix/comment_poll.py—main()now polls a comma-separated list of target repos (CI_FIX_POLL_TARGET_REPO, defaultvalkey-io/valkey,valkey-io/valkey-search), wrapping eachpoll_onceso one repo's read failure cannot abort the tick for the others. The reaction-claim idempotency is unchanged.README.md— document search support.test_target_repos_default_and_overridecovers the default, comma-split/trim, order-preservation, and de-dup.No engine changes. For a container-based search job, the verifier classifies and runs the reproduce command exactly as it does for core.
Notes / follow-ups
runner.py), while search's own integration workflow allows 140 min. A cold devcontainer build + from-source valkey-server/valkey-json build could approach that cap. Not changed here — better to observe a real run first, then raise the timeout (or make it per-repo) only if needed. Search unit tests should be well within budget.issue_commentevents; until then,workflow_dispatchwith--field repo=valkey-io/valkey-searchworks.Testing
ruff checkandmypyclean on changed files.pytest tests/test_ci_fix_*.py— 176 passed.🤖 Generated with Claude Code