feat(noema): review any owner that installed the Noema App - #2505
seonghobae wants to merge 3 commits into
Conversation
The central Noema review accepted only ContextualWisdomLab targets and minted App tokens with a fixed `owner: ContextualWisdomLab`, so another organization could not be reviewed without editing this workflow. The App installation on the target owner is the consent (noema ADR-0019). Accept any valid owner/name target, derive the App-token owner from the validated target, and keep the workflow's own identity (this repository, its trusted ref and source tarball) pinned. 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
|
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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough워크플로에 Changes대상 저장소 소유자 처리
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant TARGET_REPOSITORY
participant noema_credential
participant GitHub_App_token_action
TARGET_REPOSITORY->>noema_credential: owner/name 형식 검증
noema_credential->>GitHub_App_token_action: 검증된 owner 전달
Merge Risk: ⚪ Minimal · up to The documented external-repository review path appears ready to merge after normal checks; no actionable defect was established. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Reviews can now operate across organizations. Tokens issued through the App are limited to the selected repository, but the newly widened target policy also reaches an existing dispatch path whose authority to select another organization's target is not established here. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Add a workflow_call trigger so a repository outside ContextualWisdomLab can run this exact workflow from its own pull_request_target caller. The OIDC job_workflow_ref still names this file, which is the Noema exchange trust anchor, and the caller's own repository becomes the token target. 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
|
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. |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head stacked review for 1898bcd664c26ac690b20bf7fbd3700cf87aeaf8 (tree 05339c05620172ae2229d0de2d1f6a6358b505b1) against prerequisite #2531@d1aa3659fca527a6c7330151f3ab4df3d7578391.
Topology: ordinary two-parent merge preserves prior #2505 head 0aeca39457d89e2c3ecdddde07f869480514207d and the canonical PyJWT/PyO3 repair head; compare is ahead 3 / behind 0 with exactly five intended Noema paths (+62/-13). No force push, copied dependency repair, or security suppression.
Reviewed the complete five-path product delta. External owners are admitted only after the owner/name shape check; non-central origins remain restricted to their own repository; every App token receives the validated target owner/repository; the central workflow identity and orchestrator/free pin remain unchanged. Fresh stacked-tree verification: direct product + prerequisite contracts 115 passed; expanded Noema/required-workflow/security dependency impact suite 496 passed, 2 skipped under GITHUB_ACTIONS=true, -W error, Python 3.12.14. I found no source-backed Critical, Important, or Minor defect in this exact delta.
This COMMENT is not approval. #2505 remains Draft/Proposed until #2531 lands on protected main; after retargeting to fresh main it still needs exact-head hosted GREEN and qualifying independent approval.
Why
noema-review.ymlaccepted only^ContextualWisdomLab/...targets and minted App tokens with a fixedowner: ContextualWisdomLab, so another organization (e.g.HYOSUNG-ITX-AI-Business-Department) could not be reviewed. The owner asked to remove the fixed repository restriction. Companion to ContextualWisdomLab/noema#733 (ADR-0019: the App installation is the owner's consent).Change
owner/name(^[A-Za-z0-9][A-Za-z0-9-]{0,38}/[A-Za-z0-9_.-]+$).owner=${TARGET_REPOSITORY%%/*}; all fouractions/create-github-app-tokensteps use that output instead ofContextualWisdomLab.ContextualWisdomLab/.github, trustednoema-review.yml@ref, source tarball, re-dispatch target), PR/head/base validation, and the rule that a non-central origin may only target itself.Tests
tests/test_noema_review_installed_owner.py;test_required_workflow_queue_contract.py,test_noema_reviewer_token_lifetime.pyand theforeigncase oftest_noema_native_metadata_credentials.pyupdated to the new rule (foreign owner admitted withowner=OtherOwner; malformed metadata still rejected).05339c05620172ae2229d0de2d1f6a6358b505b1: direct product + prerequisite contracts 115 passed; expanded Noema/required-workflow/security dependency impact suite 496 passed, 2 skipped underGITHUB_ACTIONS=trueand-W error(Python 3.12.14).Stack and causal owner
This PR is intentionally stacked on #2531, the canonical owner of the shared PyJWT/PyO3 security baseline. Ordinary two-parent merge
1898bcd664c26ac690b20bf7fbd3700cf87aeaf8preserves both prior head0aeca39457d89e2c3ecdddde07f869480514207dand prerequisite headd1aa3659fca527a6c7330151f3ab4df3d7578391; no force push, suppression, or copied dependency repair was used. The previous Bandit/Semgrep/PyJWT/PyO3 failures were exact logs from the stale pre-prerequisite head and are not reused as current evidence.Keep this PR Draft/Proposed until #2531 reaches protected main. Then retarget to current
main, regenerate exact-head hosted Checks/review, and only use ordinary protected merge after terminal GREEN and qualifying approval.Still needed to use it from a new organization
The Noema GitHub App must be installable there and installed by an org admin; the consumer repository then needs a way to trigger this workflow (it is
pull_request_target/repository_dispatch, notworkflow_call).🤖 Generated with Claude Code
https://claude.ai/code/session_019hNrSzZrSNyJDUjXpaJMWs