feat(mq)!: thread context through Ack/Nak with synchronous DoubleAck - #88
Merged
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability and shutdown behavior of the message queue processing pipeline. By transitioning to synchronous acknowledgments and threading context through the message lifecycle, the application can now ensure that message processing is properly confirmed and that network operations are correctly aborted during graceful shutdowns, preventing potential hangs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
go
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom clickhouseOutput for the Bento-based ingest pipeline, replacing the generic http_client output. It also updates the message acknowledgement mechanism to use DoubleAck and improves OpenTelemetry tracing by retroactively drawing spans for queue wait times. Several issues were identified, including a critical SQL injection risk in the new ClickHouse output, missing context propagation in NATS subscriptions, and a logic error in the unit tests for message callbacks.
…d interface contract
…hten test assertions
github-actions
Bot
added
the
documentation
github-actions
Bot
added
dependencies
EricAndrechek
requested changes
May 11, 2026
…rejection and pulled latest main changes
github-actions
Bot
removed
the
github_actions
Member
Author
|
@claude[agent] can you review this |
Copilot stopped work on behalf of
taitelee due to an error
May 12, 2026 01:04
taitelee
force-pushed
the
context_aware_mq_2
branch
from
May 12, 2026 01:17
c656aa0 to
a4f5a6b
Compare
Member
Author
|
@claude[agent] can you please review this |
Copilot stopped work on behalf of
taitelee due to an error
May 12, 2026 01:31
3 tasks
EricAndrechek
added a commit
that referenced
this pull request
May 12, 2026
) ## Summary Second-order fix on top of #105. The author-trust + required-check gate in `claude-review.yml` now uses the REST `check-runs` endpoint instead of GraphQL's `statusCheckRollup` field on the PR view. ## Why `gh pr view --json statusCheckRollup` asks GraphQL to navigate `checkSuite.workflowRun` on every check context on the PR's head commit. The workflow's `GITHUB_TOKEN` is not allowed to read that sub-field for checks owned by other integrations (Gemini Code Assist, Copilot, third-party statuses). When a PR has enough cross-integration checks the whole call fails with: ``` GraphQL: Resource not accessible by integration (...statusCheckRollup.contexts.nodes.5) (...nodes.0..4.checkSuite.workflowRun) ``` We don't actually use any of that traversal — the gate just needs `name`, `status`, `conclusion` for two named checks (`CI`, `PR housekeeping`). The REST endpoint returns exactly that and nothing else, so it doesn't trip the cross-integration permission issue. This failure was always there — #105 fixing `authorAssociation` just let the script reach the next bug. Visible on PR #88 (long-lived, many integrations) but not PR #7 (fewer integrations), which is why we didn't catch it in initial testing. ## What changed - `gh pr view --json` drops `statusCheckRollup`; keeps `isDraft`, `author`, `headRefOid`. - New `gh api repos/$REPO/commits/$head_sha/check-runs?per_page=100` call for the rollup, only inside the `workflow_run` branch (other event paths bypass the snapshot like before). - Status / conclusion comparisons updated from uppercase (GraphQL enum: `COMPLETED`, `SUCCESS`, ...) to lowercase (REST enum: `completed`, `success`, ...). - Gate behavior is otherwise identical: same set of pass-through conclusions, same skip notices, same fall-through to the Claude action when CI is green. ## Verified `gh api repos/Wave-RF/WaveHouse/commits/a4f5a6b1cc83ffdca5429c0d72fb593f3f232eda/check-runs` (PR #88's current head) returns both `CI` and `PR housekeeping` with `status=completed, conclusion=success`. No GraphQL errors. ## Out of scope (mentioned in investigation, not changed here) - `observability` branch and `context_aware_mq_2` branch still carry pre-#89 workflow files (`board-state-sync.yml`, `claude-agent.yml`) and a pre-#77 runner label. Rebase those branches onto current `main` to drop them. - The \"Claude\" run on `context_aware_mq_2` (event `dynamic`, runs on `ubuntu-24.04`) is GitHub Copilot Coding Agent in Claude mode, not our workflow file. - PR #107 (observability) `PR housekeeping` failures are legitimate — the PR title exceeds 72 chars. ## Test plan - [ ] Next workflow_run for PR #88 (or any internal PR with cross-integration checks) — the gate completes without GraphQL errors. - [ ] Draft / Dependabot / untrusted-author PRs still skip with their respective notices. - [ ] A green CI on a trusted-author PR still falls through to the Claude review step. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
EricAndrechek
added a commit
that referenced
this pull request
May 12, 2026
) ## Summary Replaces the workflow_run-triggered Claude review with a `pull_request_target`-triggered one, switches the trust gate from "PR author association" to "HEAD commit author/committer permission", and makes the orchestrator wait for Claude review before assigning a reviewer. Closes the chain of regressions we've been working through (#105, #108) and gives the PR a visible `Claude review` check. ## What was broken The previous design — trigger Claude review on `workflow_run` after CI succeeds — kept producing new failures because workflow_run runs with a different token context and the check doesn't attach to the PR: | Symptom | Root cause | |---|---| | `Unknown JSON field: authorAssociation` | Not a valid `gh pr view --json` field. Patched in #105. | | `Resource not accessible by integration` (GraphQL `statusCheckRollup`) | GraphQL navigates `checkSuite.workflowRun` across every context; the workflow's GITHUB_TOKEN can't read that on PRs with other integrations' checks. Patched in #108. | | `author_association: NONE` for jfwoods | The integration `GITHUB_TOKEN` collapses private org membership to `NONE` on REST. No good workaround at that field. | | No `Claude review` indicator on the PR | workflow_run-triggered checks attach to the source workflow run, not the PR head commit. | | Reviewer assigned before Claude review finished | Orchestrator was triggered by CI completion in parallel with Claude review, not after it. | ## What this PR does ### `claude-review.yml`: rebuild around `pull_request_target` - Triggers on `pull_request_target` (open/sync/reopen/ready_for_review) plus the existing `issue_comment` and `workflow_dispatch` paths. - Job renamed `Review` → `Claude review` — that's the check name that appears on the PR and that the orchestrator looks for. Keep them in sync. - Workflow-level `if:` filters Dependabot at the cheap level. All other gating moves inside the job. - Trust gate is now: `HEAD commit's author OR committer must have ≥ read permission on the repo`. Implementation: - `gh api repos/$REPO/commits/$head_sha` → pull `author.login`, `committer.login`. - For each candidate (skipping `web-flow` and `dependabot[bot]`), `gh api repos/$REPO/collaborators/$candidate/permission`. - First candidate whose permission is `admin/maintain/write/triage/read` wins. - This means: if Jack opens a fork PR and I push a fixup, the next HEAD is mine → committer = me, admin → gate passes. If Jack pushes again with no admin involvement → both author and committer = Jack, no perm → gate skips. The check still posts `success` so the orchestrator can move on (drive-by PRs still get a human reviewer assigned, just without AI auto-review). - Required-check snapshot (the part that fetched check-runs and gated on CI success) is gone. Claude can review red CI; it can read the failing diff and say so. The merge gate is still the branch ruleset, not this workflow. - ~80 lines of gate scaffolding deleted overall. ### `project-orchestrator.yml`: wait for Claude review - `workflow_run.workflows: [CI]` → `[CI, "Claude PR review"]`. Orchestrator re-fires when Claude completes. - Bot-clean required-checks list gains `Claude review`. First firing (after CI) sees Claude pending → bot-clean false → skip. Second firing (after Claude) sees all green → assign reviewer + set board state. - Bot-clean switches from `gh pr view --json statusCheckRollup` to REST `check-runs`, pre-empting the same GraphQL `Resource not accessible by integration` failure that hit `claude-review.yml` in #108. PR #7 wasn't tripping it yet; PR #88 (with Gemini) would have. `Claude review` is *not* added to the branch ruleset's `required_status_checks` — it's advisory. The orchestrator just waits for it locally. ## Behavioural changes worth flagging - **Drafts are now reviewed.** Previously the gate skipped drafts; the new gate doesn't filter on draft state. The orchestrator's existing draft → ready auto-flip still works (gated on bot-clean, which now includes Claude). Easy to add back a draft-filter inside the gate step if it turns out to be noisy. - **Claude reviews red CI.** It can read the failing diff and comment on it. Saves the "wait for CI, then review" round-trip; costs a few API calls on PRs that get force-pushed before CI finishes. - **Drive-by fork PRs skip Claude but the check still appears.** Conclusion is `success` with a notice in the step log explaining the skip. The orchestrator treats this as "Claude weighed in" and proceeds to assign a human reviewer. ## Test plan - [ ] Internal PR pushed by a private-org member: `Claude review` check appears on the PR; orchestrator waits for it before assigning. - [ ] Fork PR by a drive-by contributor: `Claude review` posts `success (skipped)`; orchestrator still assigns a reviewer. - [ ] Admin commits a fixup onto a fork PR: next push re-runs Claude review against the new HEAD with trust via committer. - [ ] `@claude` / `/review` re-trigger via comment still works. - [ ] Dependabot PR: filtered at workflow-level `if:`, no Claude check on PR, dependabot-automerge path handles it as before. - [ ] PR #88-style cross-integration check list: orchestrator's bot-clean no longer hits GraphQL Resource-not-accessible. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EricAndrechek
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
Without this change, if the network connection to NATS was interrupted during a shutdown, the application would "hang" indefinitely. The Ack(ctx) call now acts as a Kill Switch: it will attempt to acknowledge the message, but if the shutdown timer expires first, it will instantly abort the network call and allow the application to exit cleanly.
Type of Change
Checklist
make lintpasses)make test)make build)Related Issues
Closes #35
Screenshots / Logs
If applicable, add screenshots or log output to help explain the change.