Skip to content

Fix Noema 429 continuation dispatch permissions - #2387

Draft
seonghobae wants to merge 1 commit into
mainfrom
fix/noema-retry-dispatch-permission
Draft

seonghobae wants to merge 1 commit into
mainfrom
fix/noema-retry-dispatch-permission

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Gap

Noema's bounded continuation after a provider HTTP 429 called repository_dispatch with its read-only review token. The contextual-orchestrator #1220 run 36146690612/job/108188118693 reached the retry step and received HTTP 403, so the planned continuation never started. GitHub requires Contents write permission for this endpoint.

Change

Move only the re-dispatch step into a dependent job using the repository's github.token with contents: write. The model review job keeps its existing read permission. The new job runs only after an admitted head and an eligible failed Noema review, rechecks the exact live PR head, bounds the delay, and restricts dispatch to the current repository.

Verification

  • actionlint .github/workflows/noema-review.yml
  • python -m pytest -q tests/test_noema_orchestrator_workflow_contract.py tests/test_noema_reviewer_token_lifetime.py tests/test_required_workflow_queue_contract.py (85 passed)
  • Same tests with GITHUB_ACTIONS=true (85 passed)

Hosted execution of the new retry job is still needed to confirm the fix.

Summary by CodeRabbit

  • 개선 사항
    • 리뷰 실패 후 재시도 요청을 별도 단계에서 처리합니다. 재시도 조건을 충족하고 지연 시간이 유효한 경우에만 요청을 다시 전송합니다.
    • 재시도 전 PR이 열린 상태인지, 대상 커밋이 변경되지 않았는지 확인해 최신 변경사항이 반영되지 않은 재시도가 진행되지 않도록 했습니다.

@seonghobae
seonghobae enabled auto-merge (squash) September 26, 2026 11:08
@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f11eab85-04c8-4331-a9ae-cd6f9cff3176

📥 Commits

Reviewing files that changed from the base of the PR and between e6334e2 and 33b9028.

📒 Files selected for processing (3)
  • .github/workflows/noema-review.yml
  • tests/test_noema_orchestrator_workflow_contract.py
  • tests/test_noema_reviewer_token_lifetime.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

리뷰 실패 시 재디스패치하던 단계를 별도 workflow job으로 옮겼습니다. 재시도 관련 출력값을 전달하고, 새 job에서 입력값과 PR 상태 및 head SHA를 확인한 뒤 조건에 맞으면 다음 attempt를 dispatch합니다.

Changes

리뷰 실패 재디스패치 흐름

Layer / File(s) Summary
재시도 정보 전달과 기존 단계 제거
.github/workflows/noema-review.yml
준비 단계가 재시도 eligibility, 지연, 다음 attempt, provider 시도 횟수, HTTP 상태를 job 출력으로 전달합니다. 리뷰 job 내부의 재디스패치 단계를 제거했습니다.
별도 재디스패치 job과 검증
.github/workflows/noema-review.yml, tests/test_noema_orchestrator_workflow_contract.py, tests/test_noema_reviewer_token_lifetime.py
새 job은 admission 성공, 리뷰 실패, 재시도 eligibility 조건에서 실행됩니다. 입력값을 검증하고, 지연 후 PR이 열려 있으며 head SHA가 예상값과 일치할 때 다음 attempt를 dispatch합니다. 테스트는 dispatch 조건과 job 텍스트 경계를 확인합니다.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ReviewJob as noema-review
  participant RedispatchJob as noema-transport-redispatch
  participant PullRequest as PR 상태 및 head SHA
  participant Dispatch as Repository dispatch
  ReviewJob->>RedispatchJob: 재시도 출력과 실패 상태 전달
  RedispatchJob->>PullRequest: 지연 후 PR 상태와 head SHA 확인
  PullRequest-->>RedispatchJob: 열린 상태와 현재 head SHA 반환
  RedispatchJob->>Dispatch: 조건 충족 시 다음 attempt 전달
Loading

Merge Risk: ⚪ Minimal · up to 33b90

No actionable merge-blocking risk is established. The new dispatch job still needs hosted execution to confirm the permission fix.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 33b90

The new dispatch job receives repository write authority, but it is separate from the model-review job and checks retry eligibility, the current repository, and the live PR head before dispatching. No introduced security bypass was established. Hosted execution of the new failure-and-retry path remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The new write credential belongs to the redispatch job and targets the current repository. The job rejects a different target repository before making its dispatch API call.

Security Findings and Attack Paths

  • inferred — No introduced bypass was established for PR or dispatch-payload values reaching the new write-authorized operation: admission checks the live head, and redispatch checks eligibility, repository identity, input formats, and the live PR state again. Hosted behavior remains unverified.

Trust Boundaries and Controls

  • observed — For PR-triggered admission, the workflow requires a same-repository head; dispatch-triggered admission validates metadata and the live head. The privileged job adds a current-repository restriction and performs no dispatch for a closed PR or changed head.

Resilience and Maintainability Implications

  • observed — Retry eligibility is tied to a capacity error and an available retry delay; the redispatch job rejects missing or malformed attempt and delay outputs rather than using them unchecked.

Hardening Proposals

  • proposed — Verify one hosted capacity-failure run end to end, including failed-job output propagation, successful dispatch, and retirement when the PR head changes. This is runtime validation, not an observed vulnerability.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed PR 제목은 Noema 429 연속 디스패치 권한 수정이라는 주요 변경 사항을 정확하고 간결하게 설명합니다.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as draft September 26, 2026 11:26
auto-merge was automatically disabled September 26, 2026 11:26

Pull request was converted to draft

Copy link
Copy Markdown
Contributor Author

Single-writer lifecycle correction — 2026-09-26

Current #2387 head 33b9028318ddd0cf6c34d1816b09b94e95346c10 and canonical owner #2373@8f9720240633f8a1cd39d193489f56057027e46a diverge from the same protected base.

The three #2387 paths are all owned by #2373. Direct exact-blob/contract comparison shows #2373 also preserves the intended least-privilege dispatch split while adding required protections absent here:

  • admission binds and rechecks the exact live base SHA as well as head/state;
  • the model job exports the typed transport_capacity_unavailable decision;
  • retry attempts are fail-closed to exactly 1|2, including malformed JSON/counter inputs;
  • target repository syntax and same-repository authority are both constrained;
  • ADR-0031, gate implementation, and valid/stale/cross-repository/malformed-counter fixtures accompany the workflow change.

Accordingly #2387 is a duplicate writer and is now Draft/Proposed. It is not closed: retirement requires protected integration or independently verified complete successor carryover. Current #2373 hosted checks are still nonterminal and its current-head review is not an approval, so neither PR has merge authority. No Force Push, evidence transfer, self-approval, bypass, or manual rerun was performed.

Copy link
Copy Markdown
Contributor Author

Canonical consolidation rationale before branch mutation:

#2373 exact head 8f9720240633f8a1cd39d193489f56057027e46a and this PR exact head 33b9028318ddd0cf6c34d1816b09b94e95346c10 diverge from the same protected main. This PR contributes the three-path least-privilege split (read-only model job, repository-scoped dispatch job, live-head check). #2373 carries those valid requirements and additionally binds the live base SHA/repository, requires the typed capacity output, constrains continuation attempts to 1/2, fails malformed counters closed, adds the executable valid/stale/cross-repository fixture, and records ADR-0031.

To preserve single-writer ancestry without weakening the stronger implementation, I will ordinary-forward #2373 with a two-parent commit whose first parent is #2373 current head, second parent is this exact head, and tree is byte-identical to #2373 current tree fb27729369c8491f38f53aef7c44b7e98f30e852. This PR remains Draft/open until the successor reaches protected integration and equivalence is independently accepted; this is not a Close or merge claim.

seonghobae added a commit that referenced this pull request Sep 26, 2026
Keep the stronger #2373 tree byte-identical while preserving #2387 as an explicit second parent. No force push or rebase.

Copy link
Copy Markdown
Contributor Author

Successor integration completed without closing this PR. #2373@bf88c9c1 now has this exact head 33b9028318ddd0cf6c34d1816b09b94e95346c10 as its second parent while retaining the stronger #2373 tree fb27729369c8491f38f53aef7c44b7e98f30e852 byte-identically. Compare from this head to the successor is 5 ahead / 0 behind; compare from prior #2373 head has zero changed files. Fresh successor Checks are queued and protected integration is not yet complete, so #2387 remains Draft/open.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant