fix: verify Noema GitHub App identity - #1480
Merged
Merged
Conversation
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Signed-off-by: Seongho Bae <me@seonghobae.me>
seonghobae
changed the title
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
… cap no-op bug Rebases this branch onto main's provider_account/account_cap rename (#1468) and Noema-independence work (#1477/#1480), then fixes a real bug this branch's own batched-preflight merge introduced: _catalog_family_cap() defaulted to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES (24) whenever ORCHESTRATOR_CATALOG_FAMILY_CAP was unset, silently disabling per-account diversification. Live production evidence: probed_count 12, ready_count 2, rejected_count 10 (83% rejected via 429/404/timeout), with the admitted free-pool catalog 100% nvidia_nim/nvidia_nim_sub -- two credentials sharing one rate-limited upstream jointly occupying the entire 12-slot preflight batch. Reported at #1415 (comment) Fix (mirrors the pattern in open main PR #1487's diff, not yet merged): renamed the helper to _catalog_account_cap(default), which now requires the caller to supply contextual_orchestrator_review_policy.DEFAULT_ACCOUNT_CAP (4) as the default instead of hand-typing/defaulting to a total-routes-scale constant. An explicit ORCHESTRATOR_CATALOG_ACCOUNT_CAP override remains honored. The sidecar shell script's equivalent CATALOG_FAMILY_CAP default (also baked to the total 24-route budget) is renamed to CATALOG_ACCOUNT_CAP and restored to a real, smaller cap (8, main's existing operational value). Also corrects CHANGELOG.md and docs/adr/0003 prose that had described the no-op 24-route cap as an intentional fix, and updates two contract tests (test_contextual_orchestrator_review_runtime_preflight.py, test_contextual_orchestrator_review_sidecar_contract.py) that pinned the old family_cap naming and the buggy default. Verification: coverage run -m pytest tests -> 2122 passed, 1 skipped, 21 subtests; coverage report -> 100% on scripts/ci; interrogate -> 100% docstrings; bash -n on the touched shell script; git diff --check clean. Red-then-green: reverting _catalog_account_cap's default back to REVIEW_PREFLIGHT_MAX_TOTAL_ROUTES makes the new regression tests fail; restoring the fix makes them pass again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
This was referenced Aug 31, 2026
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Devin's follow-up review of PR #1500 found a real remaining gap: the whole-line MULTILINE anchor on NOEMA_BODY_HEAD_RE narrowed the collision surface from "anywhere in the body" to "any full line before the end marker" — but that still lets LLM-generated summary/findings text spoof a rejection if it happens to contain a complete, correctly-formatted standalone "- Head SHA: `<sha>`" line of its own (plausible in exactly the self-referential scenario that makes the underlying bug likely: Noema discussing or quoting this exact review-body format). A content pattern can never fully close this off, since nothing stops the LLM's own unsanitized text from reproducing any fixed string. So instead of tightening the pattern further, submit_review() now wraps the start of its fixed-format footer in a second, purpose-specific HTML marker (NOEMA_REVIEW_FOOTER_MARKER) that — like the existing closing "noema-review-gate head_sha=..." comment — only submit_review() itself ever emits. noema_review_state() (via the new _isolate_trusted_footer() helper) now isolates the footer by *position*: the span strictly between that footer-start marker and the closing marker, both machine-emitted and never reachable by the LLM's own generated text. Only that trusted span is searched for the "- Head SHA:" bullet, so no amount of LLM text anywhere else in the body — mid-sentence or a complete standalone bullet, a different SHA or the same one — can ever count. The MULTILINE whole-line anchor is kept as defense-in-depth inside that now-trusted span. The dual-binding property #1480/#1483 introduced is unchanged: the marker comment and the human-readable bullet must still independently agree with the live head, exactly as before. Extended the regression tests: the existing mid-sentence-prose case is updated for the new required footer marker, and a new parametrized test (test_noema_state_ignores_standalone_body_head_bullet_before_footer) proves a complete standalone bullet line before the real footer — the exact case Devin flagged, both a different SHA and the same SHA — no longer causes a false rejection. The negative-control tests (missing/stale/duplicate/ conflicting bindings) are extended to exercise those failure modes from inside the new trusted footer span and still correctly reject. Also updates the two other hand-rolled Noema review body fixtures (test_repository_branch_coverage_review_schedulers.py) to include the new footer marker so they keep testing the intended happy path rather than incidentally exercising the new fail-closed missing-marker case. Full suite (2122 tests) passes; 100% coverage and 100% docstrings on scripts/ci/ maintained. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
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
1 participant
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.
Summary
/installationapp slug when/useris unavailableRuntime evidence
The first post-merge independent run reached the LLM review step without OpenCode approval and failed closed because a GitHub App installation token cannot resolve
/user. This patch uses the installation identity returned by GitHub itself.Verification
Acceptance still requires an actual current-head Noema review body.