fix: bind Noema App slug to review identity - #1483
Conversation
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Warning Review limit reachedNext included review available in 39 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 (8)
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 |
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
Signed-off-by: Seongho Bae <me@seonghobae.me>
| if len(marker_heads) != 1 or len(body_heads) != 1: | ||
| continue | ||
| if marker_heads[0].lower() != head_sha.lower() or body_heads[0].lower() != head_sha.lower(): | ||
| continue |
There was a problem hiding this comment.
🟡 Rejected reviews block every redispatch
A malformed or duplicate head binding makes noema_review_state reject a review that the publication gate still reuses. Every redispatch then skips publication, so the handoff exhausts its polls.
Prompt for agents
Align the review producer's idempotency check with the handoff's new acceptance contract. In scripts/ci/noema_review_gate.py, existing_noema_review currently accepts any same-commit, same-actor terminal review containing the generic marker, including every malformed body rejected by noema_review_state. Make the producer reuse only reviews with exactly one marker head and one displayed head, both matching the current head, or otherwise publish a corrected review. Share the parsing logic if practical, and add an integration-style regression test showing a malformed existing review is replaced after repository_dispatch and then accepted by the handoff.
Was this helpful? React with 👍 or 👎 to provide feedback.
| action_actor = os.environ.get("NOEMA_REVIEW_ACTOR", "").strip() | ||
| installation_id = os.environ.get("NOEMA_REVIEW_INSTALLATION_ID", "").strip() | ||
| if action_actor or installation_id: | ||
| if ( | ||
| os.environ.get("NOEMA_REVIEW_TOKEN_SOURCE") != "noema-review-github-app" | ||
| or not GITHUB_APP_BOT_RE.fullmatch(action_actor) | ||
| or not installation_id.isdigit() | ||
| ): | ||
| raise RuntimeError("Noema GitHub App identity binding is invalid") | ||
| return action_actor |
| body = str(review.get("body") or "") | ||
| marker_heads = NOEMA_MARKER_HEAD_RE.findall(body) | ||
| body_heads = NOEMA_BODY_HEAD_RE.findall(body) | ||
| if len(marker_heads) != 1 or len(body_heads) != 1: | ||
| continue | ||
| if marker_heads[0].lower() != head_sha.lower() or body_heads[0].lower() != head_sha.lower(): | ||
| continue | ||
| state = str(review.get("state") or "").upper() | ||
| if state in TERMINAL_NOEMA_STATES: | ||
| return state |
There was a problem hiding this comment.
| marker_heads = NOEMA_MARKER_HEAD_RE.findall(body) | ||
| body_heads = NOEMA_BODY_HEAD_RE.findall(body) | ||
| if len(marker_heads) != 1 or len(body_heads) != 1: | ||
| continue |
There was a problem hiding this comment.
🟡 Valid reviews become unrecoverable
When model text repeats Head SHA, noema_review_state rejects the generated review for duplicate bindings. Later dispatches treat it as existing, so every handoff poll fails.
Prompt for agents
Align review deduplication with runtime receipt acceptance. noema_review_gate.py formats untrusted LLM summary and findings before its canonical Head SHA evidence, so those fields can add another matching Head SHA. noema_review_handoff.py then rejects the review because NOEMA_BODY_HEAD_RE finds multiple matches, while existing_noema_review accepts any current-head marker and prevents a replacement on redispatch. Preserve fail-closed exact-head validation, but make generated canonical evidence unambiguous or ensure rejected receipts can be replaced rather than permanently suppressing review generation. Add an end-to-end regression covering an LLM summary or finding that contains a Head SHA line followed by redispatch.
Was this helpful? React with 👍 or 👎 to provide feedback.
Signed-off-by: Seongho Bae <me@seonghobae.me>
| def test_noema_state_skips_newer_nonterminal_review(): | ||
| pending = noema_review("PENDING", HEAD) | ||
| assert handoff.noema_review_state([noema_review("APPROVED", HEAD), pending], HEAD) == "APPROVED" |
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
Summary
app-slugoutput to Noema as its verified bot actor/userand/installationdiscovery for PAT/OIDC-compatible credentialsRuntime evidence
The merged independent gate reached the LLM review step twice without OpenCode approval. Both runs failed closed only because installation tokens expose identity through the token-mint action output rather than
/useror/installationin this environment.Verification