Repository navigation
feat(exchange): trust the App installation instead of a fixed owner list - #733
seonghobae wants to merge 8 commits into
Conversation
The exchange refused every caller and target outside one hard-coded owner and owner id, so reviewing another organization needed a code change and a deployment. The GitHub App installation is already the owner's consent: tokens only reach repositories where an administrator installed the App. Keep the exact central workflow ref, SHA pin, issuer, audience, replay and rate controls. Drop the owner allowlist; require a valid owner login and numeric id, a repository claim owned by that owner, and an installation account whose login is the target owner and whose id is the OIDC owner id for same-owner callers. Cache the binding with the installation id and re-check it on every hit. A caller still targets only its own repository; only the central repository may target others. Signed-off-by: Seongho Bae <me@seonghobae.me> Commit-Message-Assisted-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019hNrSzZrSNyJDUjXpaJMWs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (22)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOIDC 호출자와 대상 저장소의 소유자 허용 목록 검사를 변경했습니다. GitHub App 설치 응답의 계정 정보를 검증하고, 설치 계정 바인딩을 캐시에 저장해 캐시 조회 시에도 동의 조건을 확인합니다. Changes소유자 검증 및 대상 저장소 규칙
설치 동의 기반 소유자 신뢰
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant exchange
participant verifyGithubOidcJwt
participant createInstallationToken
participant resolveInstallationId
participant GitHubAPI
exchange->>verifyGithubOidcJwt: OIDC 소유자 정보 검증
verifyGithubOidcJwt->>createInstallationToken: 호출자 소유자 정보 전달
createInstallationToken->>resolveInstallationId: 대상 저장소와 호출자 전달
resolveInstallationId->>GitHubAPI: 설치 정보 조회
GitHubAPI-->>resolveInstallationId: 설치 계정 ID와 로그인 반환
resolveInstallationId-->>createInstallationToken: 검증된 설치 바인딩 반환
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 18 files. (4 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
PR 제목은 고정 owner 목록 대신 GitHub App 설치를 신뢰 경계로 사용하는 핵심 변경을 정확하고 간결하게 설명합니다. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 18 files. (4 skipped: 4 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 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.
Comment @coderabbitai help to get the list of available commands.
|
Exact-head admission correction — Ready is review admission only. Fresh audit against base
This PR is moved to Draft/Proposed until the causal owner repair is present on a successor exact head and re-audited. Queued/pending work is neither an additional blocker nor passing evidence. No Close, force push, destructive rebase, manual rerun, synthetic status/approval, merge, auto-merge, or bypass was performed. |
Bind the new OIDC-owner and target-owner grammar guards to executable exchange responses so the repository 100% gate does not leave either fail-closed branch unmeasured. No production behavior changes.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review for cab54e5d6c27768f67361b824feebfb210493397.
I reviewed the complete App-installation consent delta and the two newly added exchange-boundary tests. I found no additional source-level blocker in the claimed boundary: OIDC owner identity remains validated, repository/owner consistency is enforced, installation account login is bound to the target owner, same-owner calls also bind the numeric owner id, cache hits are revalidated, and only the central repository may target another repository.
This is a COMMENT, not an approval. The two new tests have not yet produced hosted GREEN evidence: reviewer-ci 36681968837, Security Scan 36681968857, ci 36681968888, and patch-validator-image 36681968884 are queued. The protected-base Node runtime vulnerability reported by predecessor patch-validator run 36504105548 also remains a canonical Noema prerequisite. Keep this PR Draft / Proposed; do not merge until exact-head Checks are terminal GREEN, that owner prerequisite is resolved through the normal release path, and a qualifying independent approval exists.
|
현재 실패를 exact-head 증거로 갱신합니다.
따라서 #733은 Draft/Proposed 상태를 유지합니다. #736의 ordinary merge와 immutable release 전에는 이 PR을 restack/merge하거나 실패 검사를 우회·무의미하게 재실행하지 않습니다. prerequisite release가 생기면 non-force restack 후 새 exact head 전체 검사를 다시 수행합니다. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head COMMENT review for 65d3036f18e0b11e281d44245e690a3c550c7461 (tree fc8648d712608e286b3ebce37a96b21e4dbc2696). Predecessor image evidence failed on the superseded Node 24.19.0/Undici 7.29.0 runtime. This ordinary two-parent merge integrates current #737 bf2cd03d… without conflict and the PR is retargeted to that prerequisite branch. Combined verification: 23 files / 260 tests, direct TypeScript typecheck, and diff check passed. Fresh hosted exact-head gates and qualifying independent approvals remain required; Draft / Proposed / merge HOLD.
Current exact-head hosted evidence — 2026-10-03
Draft / Proposed remains correct because prerequisite #737 is open and unmerged and no qualifying approval exists. This receipt does not claim ordinary merge, immutable release, downstream maturity, or predecessor retirement. No force update, rerun/cancel substitution, no-op commit, self-approval, bypass, merge, release, or Close was used. |
Re-fetch the live PR head/base identity before and after verification, rerun on edited PRs, and reject every identity drift without trusting stale event base fields.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head follow-up review for 42da900e1fdcd2e436b8865d069771c03f2d2b02 (9d4fbeecf385bbaf47949c93ba283e1566a5628e).
RCA/RED/GREEN: run 36939785675 proved stale event-base admission after retarget event 32296185938. The repair now fetches the live PR (head SHA, base ref, base SHA) at both gates, rejects each field’s drift independently, grants only pull-requests: read, and subscribes to pull_request.edited so a completed same-head retarget starts fresh verification. Focused/adjacent tests are GREEN; independent local review found no Critical issue, and its Important/Minor findings are incorporated.
This is a COMMENT, not approval or merge authority. Fresh hosted exact-head Checks, qualifying independent approval, prerequisite #737 ordinary merge, and protected-main revalidation remain open; keep Draft.
Exact-head hosted evidence — pre-step failure (2026-10-03)Exact head
For every job, GitHub reports zero steps ( |
Stop treating pull-request .base.sha as current branch-tip authority. Resolve the live base ref independently, strengthen the GitHub and git test doubles, and preserve the remaining post-completion base-push observer gap as Draft/HOLD.
|
Correction and exact-head evidence for The previous RED → GREEN
The repository-owned Vitest command and full suite are not claimed locally because Node 24.19.0 fails the exact Node 24.21.0 |
Replace nested template literals in the generated gh program with ordinary string concatenation. Record the invalid exact-head syntax claim and the bounded verification evidence.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head COMMENT review for 7e0a2f18ec6444ef8b4f7cbd85852fe8d8046973 / tree a4ead9eeb17533edfb8d1452252b7a31d3216810.
Correction and RCA: predecessor 539bfe3… was not parseable because two unescaped nested JavaScript template literals terminated the outer TypeScript fixture template. The earlier direct-syntax GREEN claim is retracted. This head replaces only those two generated expressions with ordinary string concatenation; exact remote comparison confirms no other test-code change.
Fresh verification: exact predecessor RED is ERR_INVALID_TYPESCRIPT_SYNTAX; repaired stripTypeScriptTypes, direct Node syntax, Bash syntax, focused external Vitest 9/9, and independent dependency-free execution of all eight identity scenarios pass. Independent review reports Critical/Important/Minor 0/0/0. The repository-owned/full Vitest suite is not claimed because available Node 24.19.0 is rejected by exact Node 24.21.0 devEngines.
Hosted exact-head runs are not GREEN: ci 37590609325 and 37590570270, reviewer-ci 37590570255, and patch-validator-image 37590570232 all failed with zero job steps; exact log downloads return 404 BlobNotFound. This is a COMMENT, not approval. Keep Draft / Proposed / merge HOLD pending hosted executable GREEN, #737, post-completion base-only observer/merge authority, and qualifying independent approval.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head COMMENT review for 92382107de07e40572e63251543fddb1e2907569 / tree 229fa400fa31c65f7ee080699ad49bdac44b3c46.
RCA/RED/GREEN: the live PR/Git-ref double-read could not invalidate a terminal same-head result after a later base-only push, and application CI had no merge_group/checks_requested path. The first focused RED also exposed that simply adding the trigger would skip lockfile change control. The repaired workflow checks out the exact merge-group head, requires the canonical event base SHA to be its ancestor, supplies that same base to the existing lockfile comparison, and runs ordinary release verification on GitHub's latest-base synthetic group. GitHub's documented merge-queue behavior supports this source contract; protected-main ruleset enablement remains separate authority.
Exact remote-tree verification is GREEN for TypeScript fixture stripping, executable workflow assertions, YAML parsing, Bash syntax, current prerequisite ancestry, and diff integrity. Full repository Vitest is not claimed locally because Node 24.19.0 is rejected by the repository's exact Node 24.21.0 devEngines contract.
Initial exact-head runs reviewer-ci 37902654458/job 113728573318, ci 37902654453/job 113728573355, and patch-validator-image 37902654447/job 113728574046 failed before source execution: every job has zero steps and each log download returns 404 BlobNotFound. Reviewer and image jobs were explicitly rerun; edited-event CI 37902716056 is queued. This is a COMMENT, not approval. Keep Draft / Proposed / merge HOLD pending #737, hosted GREEN, protected-main merge-queue/ruleset evidence, and qualifying independent approval.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Hosted retry follow-up for exact head 92382107de07e40572e63251543fddb1e2907569: edited-event CI job 113728775084, reviewer-ci retry job 113728936876, and patch-validator-image retry job 113728938078 also completed with zero steps. Each exact job-log request returns 404 BlobNotFound. This reproduces the existing pre-source GitHub Actions infrastructure failure; it is not source GREEN and exposes no source log to repair. Keep the lane Draft / HOLD and continue independent work without treating these failures as code evidence.
1 participant
2026-10-09 merge-queue base-authority repair (authoritative)
92382107de07e40572e63251543fddb1e2907569, tree229fa400fa31c65f7ee080699ad49bdac44b3c46; both ref updates were expected-head, non-force fast-forwards.synchronizeevent, and application CI did not subscribe to GitHub's separatemerge_groupevent. The first repair draft also left lockfile change control restricted topull_request, which would have made merge-queue validation weaker than the ordinary PR lane.merge_group/checks_requestedand then merge-group lockfile authority were absent. CI now checks out the exact merge-grouphead_sha, validates canonicalbase_shaancestry, feeds that same base SHA to the existing lockfile change-control gate, and runs ordinary release verification on the latest-base synthetic group.devEnginescontract.2026-10-07 generated-fixture parse repair (authoritative)
7e0a2f18ec6444ef8b4f7cbd85852fe8d8046973, treea4ead9eeb17533edfb8d1452252b7a31d3216810; branch advancement was lease-checked andforce=false.539bfe34524ae3a414f58c9a5fbda6e4ad2e5a9anested two unescaped JavaScript template literals inside the outer TypeScript fixture template.stripTypeScriptTypesfailed withERR_INVALID_TYPESCRIPT_SYNTAX; the preceding direct-syntax GREEN statement was invalid.ghpath and diagnostic now use ordinary string concatenation. The test file differs from the predecessor at only those two expressions; the non-vacuous API/argument oracle is unchanged.stripTypeScriptTypes, direct Node syntax, Bash syntax, the two focused files (9/9 via an already-available external Vitest runner), and an independent dependency-free execution of all eight identity scenarios are GREEN. The repository-owned Vitest invocation and full suite remain unclaimed because Node 24.19.0 is rejected by the exact Node 24.21.0devEnginescontract.2026-10-07 live base-ref tip repair (authoritative)
539bfe34524ae3a414f58c9a5fbda6e4ad2e5a9a; it advanced the feature branch without force and targets prerequisite basefix/patch-validator-node-24-21@bf2cd03dd817e0db8ec67aa9eb91458aaca36c14..base.shafrom the live pull-request response, but that field is still pull-request snapshot metadata and can lag the current base-ref tip. Its test double also ignoredgh/gitoperands, so the oracle could pass vacuously.(head SHA, base ref)from the PR endpoint, independently resolvesrefs/heads/{base_ref}through the Git-ref endpoint, checks exact ancestry, and repeats both reads after verification. Test doubles now reject unexpected API andmerge-baseoperands and cover base-SHA-only movement plus non-ancestry.bash -n, and direct TypeScript syntax checks are GREEN. Full Vitest is not claimed because this workspace has Node 24.19.0 while the exact tree requires Node 24.21.0.37585966072, patch-validator-image37585966262, and ci37585965869failed before source execution. Both original attempts and explicit failed-job reruns contain zero steps and their logs returnBlobNotFound; therefore hosted GREEN is absent.docs/product-technical-gap-baseline.md.2026-10-03 live PR identity TOCTOU repair (authoritative)
42da900e1fdcd2e436b8865d069771c03f2d2b02with tree9d4fbeecf385bbaf47949c93ba283e1566a5628e; it is a non-force fast-forward from65d3036f18e0b11e281d44245e690a3c550c7461.base.ref=mainafter GitHub event32296185938had retargeted this PR tofix/patch-validator-node-24-21. Both gates re-resolved the stale ref instead of the live PR identity.pull_request.editedrevalidation. The workflow fetches and compares the live(head SHA, base ref, base SHA)before and after verification.2026-10-02 prerequisite merge-forward and stack repair (authoritative)
65d3036f18e0b11e281d44245e690a3c550c7461with exact treefc8648d712608e286b3ebce37a96b21e4dbc2696.cab54e5d6c27768f67361b824feebfb210493397and current prerequisite fix(security): upgrade patch validator and bind gateway endpoints #737 exactbf2cd03dd817e0db8ec67aa9eb91458aaca36c14; the ref advanced withforce=false.36681968884, job109779171456, built Node 24.19.0/Undici 7.29.0 and failed closed on blocking embedded-runtime vulnerabilities. The true-owner fix(security): upgrade patch validator and bind gateway endpoints #737 repair now supplies Node 24.21.0/Undici 7.29.1, exact final-receipt binding, gateway admission, and bounded scanner acquisition.fix/patch-validator-node-24-21@bf2cd03d…, keeping the installation-consent feature delta bounded to its own 22 files. Fresh exact-head hosted review/Checks, qualifying independent approvals, fix(security): upgrade patch validator and bind gateway endpoints #737 ordinary merge, protected-main retarget/revalidation, immutable release, and downstream consumer repair remain open.Why
The exchange accepted only callers whose OIDC
repository_ownerequalledALLOWED_REPOSITORY_OWNERwith one hard-coded owner id, and refused anytarget_repositoryoutside that owner. Reviewing another organization (e.g.HYOSUNG-ITX-AI-Business-Department) required a code change and a redeploy. The owner asked to remove the fixed repository/owner restriction.The GitHub App installation is already the owner's consent: installation tokens reach only repositories where an administrator installed the App.
Change (ADR-0019)
job_workflow_ref/workflow_ref,ALLOWED_WORKFLOW_SHA, issuer, audience, replay guard, rate limit.repository_ownermust be a valid GitHub login andrepository_owner_ida numeric id; a well-formedrepositoryclaim must belong to that owner. The central workflow owner's id and known repository ids stay pinned.account.loginmust equal the target owner; for a same-owner calleraccount.idmust equal the OIDCrepository_owner_id(defeats org-name reuse). The binding is cached with the installation id and re-checked on every cache hit. A response withoutaccountfails closed (502 ERR_GITHUB_API).docs/adr/0019-installation-consent-owner-trust.md,docs/api-spec.md,docs/CONTEXT_MAP.md,docs/onboarding.md.Tests
test/installation-consent-owner-trust.test.ts(10): foreign-owner mint, account id mismatch, account login mismatch, missing account, cross-org target refusal, non-central workflow refusal, central→installed org, cache-hit re-check (discriminating: a fresh lookup would 502, the cache path returns 403).accountfield; two tests that encoded the old allowlist were rewritten to the new rule (owner/repository claim consistency; an uninstalled outside org mints nothing).tsc --noEmitclean. Full Vitest locally: 5158 passed, 11 failed — the same macOS-only Linux-isolation (codegraph-sandbox-*,codegraph-directory-binding) and Unix-socket (dependency-license-inventory-special-output) tests that fail on unmodifiedmainon this host. Exact-head CI on Linux is the authority.Not in this PR
ContextualWisdomLab/.githubnoema-review.ymlstill hard-codes^ContextualWisdomLab/target checks andowner: ContextualWisdomLabApp-token steps; separate PR.GITHUB_APP_INSTALLATION_IDset stays pinned to one installation.🤖 Generated with Claude Code
https://claude.ai/code/session_019hNrSzZrSNyJDUjXpaJMWs
Summary by CodeRabbit
Exact-head repair and remaining gates
29bffbb72b43b6d0d5405aa1c0d335063464cca4executed 5,169 passing tests but failed the 100% coverage gate (99.97% statements). The uncovered fail-closed branches weresrc/index.ts:658(invalid OIDC owner login) andsrc/index.ts:790(invalid central target owner).cab54e5d6c27768f67361b824feebfb210493397adds two exchange-boundary tests for those branches. It changes tests only; hosted GREEN is not claimed while current exact-head runs remain queued.11031436406, reports the protected-base Node 24.19.0 runtime embedding Undici 7.29.0 with GHSA-3wwx-pv8p-q78v (fixed in Undici 7.29.1). This is a canonical Noema runtime prerequisite rather than a change introduced by this App-installation delta.