Skip to content

fix(review): fail closed before every Noema skip path - #1198

Open
seonghobae wants to merge 52 commits into
mainfrom
fix/pip-audit-pip-2621
Open

fix(review): fail closed before every Noema skip path#1198
seonghobae wants to merge 52 commits into
mainfrom
fix/pip-audit-pip-2621

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fail closed unless the exact current head has a substantive primary OpenCode APPROVED review.
  • Apply that prerequisite before every Noema success/skip path, including Draft PRs, an existing Noema verdict, and a token actor that is also a primary-review actor.
  • Restack non-force onto protected main@4b115bdad2b3682f36639c4f978fd0e976a83ff0; current-main scheduler, workflow, security, and contextual-orchestrator behavior remains authoritative.

Current identity

  • exact head: b8f4adb5a54f561fc019d0856a43f56a19535141
  • tree: 1216d855f9cbda27a29dbd35a7ea2e481275a2a8
  • protected base: main@4b115bdad2b3682f36639c4f978fd0e976a83ff0
  • ancestry: regular two-parent non-force convergence; 0 behind / 49 ahead
  • effective protected-main delta: exactly scripts/ci/noema_review_gate.py and tests/test_noema_review_gate.py

TDD evidence

RED on protected-main source:

  • ordinary missing current-head approval: returned success
  • Draft missing current-head approval: returned success
  • existing Noema verdict without primary approval: returned success
  • primary-review token actor without primary approval: returned success
  • focused result: 4 failed
  • pagination RED: an exact-head approval followed by 100 later review events was lost; malformed paginated evidence was not typed fail-closed
  • fresh operational RED: fix(noema): validate stable OIDC exchange envelope #834 exact head 5a51a6124bfce2cfc73199b490ff684f230b0be3 used protected workflow 4b115bda, logged Current head does not have a primary OpenCode approval; Noema review skipped., and still concluded GitHub-success.

GREEN on the exact replacement tree:

  • focused Noema suite: 24 passed
  • full suite: 2,099 passed · 1 skipped · 21 subtests
  • statements: 10,466/10,466
  • branches: 4,172/4,172
  • complete Reviews API pagination preserves approvals older than 100 events and rejects malformed pages/reviews
  • docstrings: 100%
  • exact tree identity verified locally and through Git Data as 1216d855...

Acceptance gate

Do not merge from source evidence alone. Require terminal exact-head workflows, zero unresolved actionable threads, and a qualifying exact-head formal review under live protection. After protected-main integration, unchanged consumer canaries must prove that GitHub-success can no longer coexist with an absent exact-current-head primary verdict.

All predecessor workflow results are historical and non-passing for this head. Exact-head hosted security, coverage, Strix, OpenCode, and Noema evidence is regenerating and remains non-passing until terminal.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review Change Stack

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: a98974f5-7d7b-4c96-808f-d2d69b0217ac

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

CodeQL merge preview, 현재 HEAD 리뷰 게이트, PR 스케줄러, coverage 빌드, VCS 라이선스 검증을 갱신했습니다. 관련 계약 테스트와 운영 문서도 변경했습니다.

Changes

리뷰 제어 무결성

Layer / File(s) Summary
병합 미리보기 및 coverage 커밋 식별
.github/workflows/codeql-pr.yml, .github/workflows/opencode-review-dispatch.yml, tests/test_codeql_pr_workflow_contract.py, tests/test_opencode_agent_contract.py, scripts/ci/test_strix_quick_gate.sh
CodeQL과 coverage 경로가 base/head SHA를 확인하고, 정확한 head 및 검증된 merge SHA를 사용합니다. pnpm 선언도 base와 head에서 비교합니다.
현재 HEAD verdict 및 CodeGraph 게이트
scripts/ci/noema_review_gate.py, tests/test_noema_review_gate.py, docs/doctoring/required-review-check-is-not-a-verdict.md
현재 HEAD의 primary OpenCode verdict가 없으면 게이트가 실패합니다. CodeGraph 경로는 workspace 내부로 제한됩니다.
리뷰 디스패치 및 큐 정리
scripts/ci/pr_review_merge_scheduler.py, .github/workflows/pr-review-merge-scheduler.yml, tests/test_pr_review_merge_scheduler.py, tests/test_required_workflow_queue_contract.py, tests/test_opencode_workflow_shell_syntax.py, docs/doctoring/merge-scheduler-workflow-run-deduplication.md
스케줄러가 verdict 우선순위, draft review-only 흐름, 활성 실행 중복, 외부 fork head, stacked PR 예산 및 metadata-free workflow 실행을 처리합니다.
VCS 라이선스 검증 및 실행 경계
scripts/ci/validate_vcs_dependency_license.py, .github/workflows/opencode-review-dispatch.yml, tests/test_validate_vcs_dependency_license.py, docs/CWL-MASTER-CONTEXT.md, CLAUDE.md
VCS 의존성의 repository, commit, SPDX 식별자, 응답 크기, JSON, 리디렉션을 검증합니다. 검증 실패 시 materialization을 중단합니다.

Estimated code review effort: 5 (Critical) | ~120 minutes

Merge Risk: 🟠 High · up to 090b0

This PR changes privileged review scheduling and security validation, but newer actionable pull requests can be starved by the processing cap, and the current head still lacks required exact-head approval and terminal checks while workflow-integrity and license-policy concerns remain unresolved. These risks can delay required reviews or allow insufficiently validated automation changes, so the PR should not merge until they are fixed or explicitly accepted.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 114 functions across 16 files. (10 skippe… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 Noema skip 경로의 fail-closed 변경을 정확히 설명합니다. 이는 PR의 실제 변경 사항 중 하나이며, 전체 변경 사항을 모두 포함하지 않아도 되므로 제목 기준을 충족합니다.
Full details: Docstring Coverage

Explanation

Docstring coverage is 70.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 114 functions across 16 files. (10 skipped: 9 unsupported, 1 too large.)

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pip-audit-pip-2621

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 enabled auto-merge (squash) August 21, 2026 12:29

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head local verification (observed at head 081e9eb, base 731af58):

  • Diff is limited to pip 26.1.2 -> 26.2.1 and its two lock hashes in requirements-pip-audit-ci-hashes.txt.
  • Python 3.12 hash-locked install passed.
  • pip-audit --strict --desc=on -r requirements-pip-audit-ci-hashes.txt passed: no known vulnerabilities.
  • git diff --check passed.

Hosted Checks remain queued/pending and no exact-head approval is present; this PR remains on the normal protected path and was not merged or bypassed.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent @cwl-noema-review

Current-head review request for central security PR #1198.

Exact head: 081e9eb
Exact base: 731af58

Read the complete one-file hash-locked dependency diff. Verify that pip is upgraded to a fixed release, both hashes match the release artifact, no unrelated dependencies changed, and the lock remains compatible with the repository's install/audit contract. Confirm the PYSEC-2026-3721 pip-audit failure is resolved without weakening the security gate. Return an exact-head substantive verdict with path:line evidence; do not reuse predecessor evidence.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head security verification at 081e9eb: the one-file lock diff updates pip 26.1.2 to 26.2.1 with two replacement hashes. In a project-local CPython 3.12 uv environment, uv pip install --require-hashes -r requirements-pip-audit-ci-hashes.txt installed all 28 pinned packages, and the same environment ran pip-audit --strict --desc=on --no-deps --disable-pip -r requirements-pip-audit-ci-hashes.txt with No known vulnerabilities found. Live PR remains OPEN/MERGEABLE/BLOCKED; required hosted lanes are queued and no independent approval exists. No bypass or force push.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head revalidation after stack-only child merge: current head 7005e5c, base main 731af58. The tree includes the pip 26.2.1 lock repair plus the stack-only merge of #1168. Local verification on this exact tree: 1295 tests and 16 subtests passed; actionlint, Python compileall, bash -n for both changed shell scripts, git diff --check, 28-package --require-hashes installation in CPython 3.12, and pip-audit hard gate all passed; pip-audit reported No known vulnerabilities found. Live PR remains OPEN/MERGEABLE/BLOCKED; exact-head policy is in progress, remaining hosted required lanes are queued, and no independent approval exists. No bypass or force push.

devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

@opencode-agent @cwl-noema-review

Exact current HEAD e44bdef. This is the stack root: the pip 26.2.1 security lock repair remains together with the already-merged #1178 contextual-orchestrator hourly review-repair child. The child was preserved through normal inverse/reapply commits; no history rewrite or force push was used.

Exact-head verification: the stack-specific caller test passed (4 tests); Python compileall and git diff --check passed. Re-run the full current-head review and security evidence across the complete stack. Do not reuse predecessor-head status or review evidence.

seonghobae added a commit that referenced this pull request Aug 21, 2026
Revert the unrelated pip-audit lock/input policy commit from the Strix owner
branch. The canonical dependency-only security repair remains in PR #1198;
this branch continues to own only the Strix fail-closed contract.
@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head security verification

  • Exact head SHA: 19a747e80e7328ac89c28a072057f162841355c4
  • Exact base SHA: 731af58e954901c4f1cc853231c592abb1eaf617
  • GitHub PR Files API: one file only, requirements-pip-audit-ci-hashes.txt
  • Decision: WAIT_AND_REMEDIATE

The owner-scope repair restored the dependency-only tree without force-push. The diff remains limited to pip 26.1.2 -> 26.2.1 and the two replacement release hashes.

Exact-head local evidence:

  • uv run pytest -q: 1291 passed, 16 subtests passed.
  • uv run --group dev pytest --cov=scripts/ci --cov-branch --cov-fail-under=100: 8123 statements, 3144 branches, 100%.
  • CPython 3.12 project-local uv pip install --require-hashes -r requirements-pip-audit-ci-hashes.txt: 28 packages checked.
  • pip-audit --strict --desc=on --no-deps --disable-pip -r requirements-pip-audit-ci-hashes.txt: No known vulnerabilities found.
  • git diff --check, Python compileall, and changed shell bash -n: passed.

The repository-wide interrogate result is 99.9% because the unchanged base already has one undocumented __init__ at scripts/ci/organization_commercial_readiness_loop.py:241; this dependency-only PR does not absorb that unrelated documentation repair. Full actionlint likewise reports pre-existing workflow syntax errors in files outside this diff; no workflow was changed here.

Hosted required Checks for 19a747e remain queued and reviewDecision is empty. No approval, auto-merge, force merge, bypass, or force-push action was performed.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head lock regeneration evidence

  • Exact head: efea53a11c90c7ca519af56aed6918124f45826e
  • Exact base: 731af58e954901c4f1cc853231c592abb1eaf617
  • Re-ran the lock header command exactly as recorded in requirements-pip-audit-ci-hashes.txt: uv pip compile --generate-hashes --python-version 3.12 --python-platform x86_64-manylinux_2_28 requirements-pip-audit-ci.txt.
  • The regenerated pip==26.2.1 block matched the PR block byte-for-byte, including both SHA-256 hashes.
  • Hash-enforced resolver smoke passed: uv pip install --dry-run --system --require-hashes --only-binary=:all: -r requirements-pip-audit-ci-hashes.txt.

The lock was not hand-edited: the exact recorded uv generation contract reproduces the current pip block, and the hash-enforced install path accepts it. No unrelated lock refresh is being introduced because this PR intentionally changes only the vulnerable pip pin.

Disposition: resolved; no further source change is required for this finding.

@seonghobae seonghobae changed the title fix(security): update pip audit lock past PYSEC-2026-3721 fix(security): repair pip audit and schedule orchestrator review Aug 21, 2026
@seonghobae

seonghobae commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

@opencode-agent @cwl-noema-review

Review-only request for exact current HEAD e44bdef. Verify the pip 26.2.1 lock repair and the already-reviewed #1178 hourly caller as one six-file stack-root proposal to main. Confirm hash integrity, explicit credential scope, schedule contract, and exact-head Checks; do not reuse predecessor evidence.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head verification update

  • Repository: ContextualWisdomLab/.github
  • Pull request: fix(review): fail closed before every Noema skip path #1198
  • Exact head SHA: e44bdef3c4d8b6fc75639985ea256a59fe7f0fdd
  • PR base SHA recorded by GitHub: 731af58e954901c4f1cc853231c592abb1eaf617
  • Current protected main tip observed separately: dd58a88391e44a32fb399f7407f508d8e73cc1c7
  • State: OPEN, non-Draft, mergeable; merge state BEHIND; required review decision is empty.

Current six-file diff and local evidence

  • The current tree contains the intended pip 26.1.2 -> 26.2.1 hash-locked update plus the contextual-orchestrator hourly caller, focused contract test, doctoring, cadence trigger, and the required coordinator docstring. The earlier writer/tree race is not present in this exact tree.
  • uv pip install --require-hashes -r requirements-pip-audit-ci-hashes.txt in project-local CPython 3.12 installed 28 packages, including pip 26.2.1.
  • pip-audit --progress-spinner off: No known vulnerabilities found.
  • uv run pytest -q: 1295 passed, 16 subtests passed.
  • Full branch coverage command: 1295 passed, 16 subtests passed; 8123 statements, 3144 branches, 100%/100%.
  • uvx interrogate==1.7.0 --fail-under 100 on changed Python/test paths: 100%.
  • Targeted actionlint for the hourly workflows and git diff --check: passed.

Hosted state and decision

  • Hosted caller quality, exact-head policy, and Devin Review are passing; required Python, OSV, SAST, secret, SBOM, queue, and other lanes remain pending. CodeRabbit is rate-limited rather than a source verdict.
  • The visible scan-pr-queue failure is run 32492604827, whose job conclusion is cancelled; it is not a source/test failure and is not hosted success evidence.
  • No independent approval is present.
  • Because protected main advanced beyond the PR’s recorded base, the local results above are not merge-result evidence for the current base tip and must be rerun after restack.

Decision: WAIT_AND_REMEDIATE — restack/revalidate against current main, then re-run hosted Checks and obtain independent approval. No merge, bypass, force push, direct protected-branch push, fake status, or self-approval was used.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head verification update

  • PR: fix(review): fail closed before every Noema skip path #1198
  • Exact head: e44bdef3c4d8b6fc75639985ea256a59fe7f0fdd
  • Exact base: 731af58e954901c4f1cc853231c592abb1eaf617
  • The recorded uv pip compile --generate-hashes --python-version 3.12 --python-platform x86_64-manylinux_2_28 command was rerun in an isolated temporary output. The current pip==26.2.1 pin and both SHA-256 hashes match the generated resolution; unrelated fresh resolver drift was not copied into the lock.
  • Current six-file diff retains the contextual-orchestrator hourly caller, its contract test/doctoring, the NVIDIA-NIM workflow path registration, and the security lock update.
  • Local verification: 1,295 tests and 16 subtests passed; statement and branch coverage are both 100% (8,123 statements; 3,144 branches); Ruff, compileall, actionlint, and git diff --check passed.
  • Current review threads: all resolved; no exact-head source finding remains.
  • Hosted exact-head checks are queued after the latest head update; no failure is currently observed.
  • Decision: WAIT_AND_REMEDIATE pending hosted required Checks and qualifying approvals. Normal auto-merge remains enabled. No guarded force merge or bypass was used.

Copy link
Copy Markdown
Contributor Author

Fresh independent exact-head reproduction of the Noema fail-open boundary:

  • Target: #1438 exact head 50febfe7a9bd74c8c33d1eef6526a33f116f5c2f against protected main@1ff8268255b061461d9d49b4cab4febf9a8e7bfa.
  • Required OpenCode run 33331290131 / job 99310251736 correctly failed: Reviews API contained no exact-current-head APPROVED or CHANGES_REQUESTED OpenCode verdict.
  • Required Noema run 33331290121 / job 99310166887 and the follow-up run 33331342604 / job 99310299702 both printed Current head does not have a primary OpenCode approval; Noema review skipped. and nevertheless concluded GitHub-success.
  • This is not a provider-unavailable reproduction: both Noema jobs completed sidecar discovery/runtime preflight with at least one ready route before the gate reached the missing-primary-verdict branch.
  • Exact-head coverage, Security Scan, SAST, CodeQL, OSV, SBOM, Python Security, Secret Scan, and changed-path quality were terminal-success, so the first causal boundary remains Noema's success return for a skipped required review, not a source/security failure on fix(coverage): unblock org-wide OpenCode approval + docs(gaps) corrections #1438.

Acceptance remains fail-closed: missing primary current-head approval must make the required Noema result explicitly non-passing. Do not count either successful check as review evidence. Canonical owner head was re-fetched immediately before this comment as b051f5da34998fcd0ed42990d9f5e29b128a59ab; no duplicate source lane or consumer workaround was created.

Copy link
Copy Markdown
Contributor Author

Fresh exact-head operational evidence (2026-08-31 KST) reproduces the remaining Noema fail-open on two independent central heads:

Both heads lacked a qualifying exact-current-head primary OpenCode approval. Provider preflight/readiness does not make the absence of the prerequisite verdict passing evidence. Acceptance remains: this path must terminate non-zero / explicitly non-passing, while a qualifying current-head OpenCode approval may continue into the secondary review. These are central contract reproductions; no consumer workaround is appropriate.

seonghobae added a commit that referenced this pull request Aug 31, 2026
…1270)

Central PR merge scheduler now refuses direct-merge and auto-merge until GitHub reports reviewDecision APPROVED plus a non-author, non-OpenCode formal approval bound to the exact live head; disarms stale auto-merge otherwise. Repairs check-run chronology (check-suite creation time as the ordering signal via a shared check_run_recency_key/_newest_check_run_per_identity helper, so a canceled-before-start or queued rerun cannot lose to an older completed run merely because startedAt is null -- this fix now also covers strix_evidence_state's latest_check_run_attempts path, not just failed_status_checks) and re-validates approval immediately before every merge-authorizing mutation (closes a TOCTOU window where a same-head approval revoked mid-invocation could still authorize merge).

Eight+ rounds of reproduce-before/confirm-after TDD fixes across this PR's lifetime, each independently verified by Devin Review and CodeRabbit with all findings resolved; full suite passing with 100% coverage and docstrings on scripts/ci. Merge commit 98aa3a1 resolves a real conflict against main (from #1433) where two PRs independently fixed the same check-run dedup logic differently -- unified on the correct implementation rather than picking one side.

Bypass-merged (as a merge commit, not squash, to preserve the resolved-conflict history) per explicit user authorization: exact-head Security Scan, SAST, CodeQL, OSV, SBOM, Scorecard, Secret Scan, and Strix are terminal success; the required OpenCode/Noema formal-verdict dispatch has not completed for reasons independent of this PR's own correctness (a separate, already-diagnosed central review-gate defect tracked on #1198), and this scheduler hardening is foundational for every other PR's merge safety.

Copy link
Copy Markdown
Contributor Author

Consumer acceptance canary for the existing review-gate repair, from read-only ContextualWisdomLab/Orgmetra#47 (no foreign source/ref mutation): exact unchanged consumer head 92bdb21bf4b9e19190ccd23409d55ed7102ee663 against protected develop@9e3e4847510e1e612b48474ba42b177b8ed824df. Required Noema run 33301542590, attempt 2 / replacement job 99351490463, recovered the prior provider-discovery failure under trusted central source ContextualWisdomLab/.github@883edda5d6e765e0b1efff8ac8bd626c182c7669: repository-scoped reviewer credentials/target visibility passed, pinned contextual-orchestrator sidecar reached /healthz, provider-route preflight passed, and gateway chat/completions preflight passed. The actual review gate then emitted exactly Current head does not have a primary OpenCode approval; Noema review skipped. and submitted no authenticated Noema formal verdict, yet GitHub records the required Noema run/job as terminal SUCCESS. This is a clean downstream RED canary for this PR's fail-closed Reviews-API semantics, not an Orgmetra source defect. Acceptance after this owner repair integrates: rerun the same unchanged Orgmetra head through the supported path; with no exact-head primary OpenCode approval, Required Noema must be non-passing rather than green-with-skip. Once an authenticated primary OpenCode verdict does exist, Required Noema must materially execute and only then may terminal success qualify. Preserve exact repo/PR/head/base binding and do not treat predecessor/status/model-only evidence as a verdict.

Copy link
Copy Markdown
Contributor Author

Fresh exact-head Noema false-green reproduction (2026-08-31 UTC) on canonical canary #1437 head a1dc62dfc9a5a938deff9995913e013766e55e2c, protected main@1cf2f9120a2cd494ed5079135bb10822c27b5947:

  • Reviews API returns 0 review submissions on the exact head.
  • Required OpenCode run 33347600446 / job 99355178226 correctly failed closed at Fail closed without a current-head OpenCode verdict.
  • Required Noema run 33347600433 / job 99354619439 logged PR is draft; Noema review skipped. but concluded GitHub success.
  • Noema therefore emitted neither APPROVED nor CHANGES_REQUESTED yet satisfied its required check. This is status-only/skipped evidence and remains non-passing.

This is the same central contract owned by #1198, not a #1437 source defect or a consumer workaround. The acceptance boundary remains: Draft review dispatch may be review-only, but an intentional skip/absent exact-head primary verdict must conclude non-passing; Noema must never translate review skipped into required-check success. Keep #1198 unmerged until the exact-head source has deterministic RED/GREEN coverage and a fresh operational canary whose Reviews API and check conclusion agree.

Restack #1198 onto protected main while preserving current-main scheduler authority. Missing exact-current-head OpenCode approval now returns non-passing before Draft, prior-Noema, or primary-token skip paths.
@seonghobae seonghobae changed the title fix(security): repair pip audit and schedule orchestrator review fix(review): fail closed before every Noema skip path Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@opencode-agent review

Please review exact head 207d94ba7aee56ffc4dd19f8e43d27299d13106b. The PR is now a two-file current-main delta: missing exact-head primary approval fails closed before every Noema skip path. Local TDD evidence is recorded in the PR body; predecessor reviews and checks are historical.

Copy link
Copy Markdown
Contributor Author

Exact-head operational RED is now reproduced on 207d94ba.

  • Required Noema run 33348490919, job 99357109790 materialized protected-main source and logged: Current head does not have a primary OpenCode approval; Noema review skipped.
  • The job nevertheless concluded GitHub success.
  • Reviews API has no exact-current-head APPROVED verdict; 76 historical submissions are 74 COMMENTED and 2 CHANGES_REQUESTED, with zero unresolved threads.
  • Required OpenCode run 33348490912 correctly failed closed on the same absent verdict.

This is the precise false-green repaired by the two-file PR delta. Hosted Noema GREEN cannot exist until protected main integrates the source; predecessor/default-branch execution is authoritative RED, not passing evidence. Security Scan, SAST, CodeQL, OSV, SBOM, Python Security, Secret Scan, and Scorecard completed successfully on the exact head. Strix was cancelled and remains non-passing.

devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae added a commit to ContextualWisdomLab/contextual-orchestrator that referenced this pull request Aug 31, 2026
…ee (#933)

* fix(discovery): exclude vision-input-only models from orchestrator/free

ContextualWisdomLab/.github PR #1198's required Strix Security Scan check
failed (run 33325907333, job 99295892400): 3 independent scan attempts each
hit NVIDIA NIM's meta/llama-3.2-90b-vision-instruct via
orchestrator/free and got an identical HTTP 400 invalid_request_error
("Adjust the request parameters and retry"), exhausting the whole free pool
against this one agent.

Root cause: this model is zero-priced (Models.dev reports cost 0/0) and
passes every existing chat-capability check (text output modality, no
disqualifying model-id token), so free_discovered_models() admitted it into
the general-purpose free pool that Strix's tool-calling requests route
through. Models.dev's own tool_call field claims true for this exact model
(verified live against models.dev/api.json), so that field cannot gate this;
its declared input modality (text + image) is the only honest catalog
evidence that distinguishes it from an ordinary text-only free worker.

Two alternate fixes were considered and rejected against this repo's own
tested contracts:
- Loosening ModelClient's cross-provider failover to retry a plain HTTP 400
  on a different free candidate is explicitly foreclosed by
  test_non_transient_error_is_not_replayed and
  test_virtual_passthrough_keeps_non_size_tool_errors_sticky
  (tests/test_passthrough_provider_failover.py): "caller errors fail closed
  instead of duplicating a request across providers."
- The circuit breaker never records a sticky (non-failover-eligible)
  rejection at all, but recording it would not have prevented this incident
  either: each Strix attempt is a fresh sidecar process/gateway instance
  (in-memory circuit state), and the observed failure is the very first
  request each attempt makes.

Fix: free_discovered_models() now excludes a free model that declares a
non-text input modality from the general-purpose free pool. This is
pool-composition, not per-request retry: the model stays fully discovered
and price-evidenced (available to a pool that explicitly wants a
vision/multimodal capability) and is only withheld from the
capability-blind orchestrator/free default. Scoped to the free selector
specifically (not general chat-candidate eligibility) so a genuinely
tool-capable paid multimodal model elsewhere is unaffected.

Known limitation: NVIDIA NIM's own /v1/models listing carries no
tool/function-calling capability metadata at all, and Models.dev's tool_call
field is unreliable at the per-deployment granularity (proven wrong here).
If NIM's remaining free text-only catalog also turns out to lack genuine
tool-calling support for some models, broader free-tier discovery
(OpenRouter/Bytez/OpenAI free-tier offerings, today evidence-only or never
free) is a follow-up, not something this change can fabricate evidence for.

TDD: tests/test_model_discovery.py::test_free_discovered_models_excludes_a_free_vision_only_input_model
reproduces the bug against a DiscoveredModel fixture shaped like the broken
NIM agent (fails pre-fix, passes post-fix); a text-only free model and a
free model with no modality evidence at all remain eligible.

Verified: python -m pytest tests -q -> 2770 passed, 1 skipped.
interrogate (fail-under 100) -> 100.0%. python tests/test_self_check.py and
the naming-convention/chat-capability/provider-bootstrap check scripts named
in README.md all pass individually too.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX

* fix(discovery): centralize free-serving-pool modality gate in TaskOrchestrator

Devin's review on PR #933 found the vision-input exclusion added in
94e3b9a was incomplete on three points:

1. Runtime free pool remained unfiltered: `_auto_discover_runtime_agents`
   (`--auto-discover-model-agents`) and `provider_bootstrap`'s
   `_active_agent_from_discovered` (used by `bootstrap_provider_runtime`
   and, through it, `provider_catalog_bootstrap.bootstrap_provider_catalog_runtime`)
   both tag an agent `cost:free` from raw price evidence alone and never
   consulted the new exclusion, so NVIDIA NIM's free
   `meta/llama-3.2-90b-vision-instruct` could still reach a live,
   blindly-selectable `cost:free` agent through either path -- reproducing
   the original Strix tool-calling incident (ContextualWisdomLab/.github#1198).

2. `free_discovered_models()` conflated price-based inventory with
   serving-pool eligibility, so `--free-only`, `free_tier_count`, and the
   free-tier data-privacy totals silently undercounted a model that is
   genuinely free-priced but unfit for blind serving.

3. (Evaluated, not adopted) Devin also suggested narrowing the exclusion to
   spare a model that "also supports text as a standalone input". Rejected
   against this repo's own incident evidence: the incident model itself
   declares both `text` and `image` per Models.dev, so that narrowing would
   have silently re-admitted the exact model this fix is about. Kept the
   conservative "any declared non-text input modality disqualifies" reading,
   documented with a fixture for all three modality shapes (text-only,
   vision-only, text+image).

Fix:
- `free_discovered_models()` is pure price-based inventory again.
- New `model_discovery.general_free_serving_candidates()` carries the
  modality-based exclusion for composing a blind free pool, wired into the
  `discover-models` CLI report as `general_free_serving_count` alongside
  the restored `free_tier_count`.
- The actual enforcement is now `TaskOrchestrator._is_free_agent`, a single
  choke point every `orchestrator/free` selection path shares: an agent
  whose tags declare a non-text `input:<modality>` is never treated as
  free-pool eligible there, regardless of which code built it or how old
  that agent-pool row is (protects durable pool-store rows written before
  this exclusion existed, and any future pool-construction path). `cost:free`
  keeps meaning "honest zero price" everywhere else, preserving
  `provider_catalog_store.py`'s durable `is_free` round trip (see
  `test_serving_tags_preserve_only_explicit_free_and_modality_evidence` and
  `test_last_known_good_restores_free_and_modality_evidence`, both left
  unchanged).
- `review_gateway.py` audited: its agents never carry a `cost:free` tag
  (tags are fully replaced with `("review",)`) and no price is registered,
  so it was never reachable through this bug; no change needed there.

Also brought `interrogate --fail-under=100` back to green: it was already
failing at 97.9% on the unmodified base commit (94e3b9a) from six
pre-existing undocumented `fuzz/*.py` harness functions, unrelated to this
incident; added their docstrings.

TDD: new/relocated tests fail against the pre-fix source (verified by
temporarily restoring it) and pass after the fix --
`test_auto_discovery_activates_a_free_vision_model_but_free_pool_excludes_it`,
`test_active_agent_from_discovered_free_vision_model_is_not_free_pool_eligible`,
`test_free_discovered_models_still_counts_a_free_vision_only_input_model`,
`test_general_free_serving_candidates_modality_shapes`, and the relocated
`test_general_free_serving_candidates_excludes_a_free_vision_only_input_model`.

Verified: python -m pytest tests -q -> 2774 passed, 1 skipped (2770 passed,
1 skipped on the unmodified base commit; net +4 tests, zero regressions).
interrogate (fail-under 100) -> 100.0%. python tests/test_conventions.py
passes (new function name is valid two-or-more-word snake_case). git diff
--check clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX

* fix(discovery): scope the blind-free-pool modality gate to general chat only

Devin's second review round on PR #933 (against 5648567) found the modality
exclusion had overshot: TaskOrchestrator._is_free_agent had grown a
non-text-input check meant only for the capability-blind general chat pool,
but it also backed every capability-scoped free route
(_capability_agents -> /v1/audio/transcriptions, /v1/videos, image, speech,
rerank) and server.py's _require_pool_model. A free transcription agent
naturally carries an input:audio tag and a free image/video agent an
input:image tag -- exactly the modality its own capability-scoped free route
is asking for, not a surprise -- so the shared predicate made those
genuinely free agents unreachable through their own free route.

Fix: split the predicate.
- _is_free_agent reverts to plain, modality-blind price evidence (used by
  _capability_agents/_ranked_agents when chat_only=False, and by
  server.py's capability-scoped _require_pool_model branch).
- New _is_general_free_agent = _is_free_agent(agent) and not
  _agent_requires_non_text_input(agent) is the stricter, general-chat-only
  variant, now used at every blind general-chat FREE_MODEL call site
  (proxy_completion, _orchestrated_provider_completion, route_once, conduct,
  _ranked_agents when chat_only=True, list_openai_models's advertising
  check, and server.py's capability-agnostic _require_pool_model branch).

Also addressed the review's two informational notes:
- "Duplicate serving policies can drift": extracted the actual "what counts
  as non-text" classification into chat_capability.requires_non_text_input,
  a single shared predicate both model_discovery._requires_non_text_input
  (DiscoveredModel.input_modalities) and
  orchestrator._agent_requires_non_text_input (an agent's input:<modality>
  tags) now delegate to, so the two representations of the same catalog
  evidence cannot diverge independently. New cross-consistency test:
  test_discovery_and_orchestrator_modality_eligibility_cannot_drift.
- "Serving count uses a different population": documented in __main__.py
  that free_tier_count and general_free_serving_count are deliberately both
  computed over the complete `discovered` population regardless of
  --free-only, matching each other's established convention (not a new
  inconsistency).

TDD: new tests fail against 5648567 (verified by temporarily inserting them
against that commit) and pass after this fix --
test_free_virtual_model_selects_a_free_agent_whose_own_capability_needs_non_text_input
(orchestrator._capability_agents path),
test_require_pool_model_serves_capability_free_route_despite_non_text_input
(server._require_pool_model path), plus updated assertions on the two
existing regression tests from the first round confirming _is_free_agent
now returns True (capability-reachable) while _is_general_free_agent stays
False (blind-chat-excluded) for the same agent.

Verified: python -m pytest tests -q -> 2775 passed, 1 skipped (net +1 test
over the previous round's 2774; zero regressions). interrogate -> 100.0%.
python tests/test_conventions.py passes. git diff --check clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX

* fix(discovery): require routability in general_free_serving_candidates

Devin's review pass on PR #933 after efd44f6 found two issues:

1. general_free_serving_candidates() admitted evidence-only and
   non-chat-capable zero-cost text-input catalog rows, overcounting
   general_free_serving_count with models that could never actually
   become a serving agent. Now also requires is_routable_discovered_model
   -- the same predicate _auto_discover_runtime_agents and
   provider_bootstrap already require before promoting a discovered row
   to an ordinary chat agent. New regression test
   test_general_free_serving_candidates_excludes_unroutable_free_models
   (an evidence-only free text model and a free embedding-only model)
   fails red pre-fix.

2. A real, deterministic CI failure on efd44f6 itself (GitHub Actions
   "Full unit and contract suite" job 99313736725):
   test_discovery_and_orchestrator_modality_eligibility_cannot_drift built
   ModelAgent fixtures using hyphenated ids straight from provider model
   ids (e.g. "text-only-model", "meta/llama-3.2-90b-vision-instruct"),
   which fail this repo's require_object_name two-or-more-word snake_case
   convention. Fixed by deriving a compliant id (casefold + translate
   "/.-" to "_") distinct from the `model` field under test.

Also documents both this round's fix and the prior capability-route
modality-scoping round in CHANGELOG.md (neither had an entry yet).

Verified: python -m pytest tests -q -> 2779 passed, 1 skipped, 0 failed
(with fast-mlsirm installed via git+https, working around this sandbox's
plain-tarball-download 403 that is unrelated to the fix); interrogate ->
100%; tests/test_conventions.py -> ok; git diff --check -> clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX

---------

Co-authored-by: Claude <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor Author

@opencode-agent review

Please review exact current head 8bf311ce9c4336eed091e2e8771f0703fe79047c against protected main@2c69e31373607c3dc9b2d7b4b948a154c47aca65. This head adds complete paginated REST review-history retrieval for the Noema fail-closed gate, including deterministic regressions for an exact-head approval older than 100 later review events and malformed pagination payloads. Local verification: focused 24 passed; full 2,099 passed, 1 skipped, 21 subtests; statements 10,467/10,467; branches 4,172/4,172; docstrings 100%. Exact-head Security Scan, SAST, CodeQL, OSV, SBOM, Python Security, Secret Scan, and Scorecard are terminal success. Please record a substantive formal verdict on this exact head; predecessor-head or status-only evidence must not satisfy the gate.

Copy link
Copy Markdown
Contributor Author

Merge-conflict triage: genuine architectural divergence, not attempting a guessed resolution

Attempted the standard git fetch origin main && git merge --no-edit conflict repair in a scratch clone. mergeable_state is dirty, conflicting in scripts/ci/noema_review_gate.py and tests/test_noema_review_gate.py. Evidence that this is a real design divergence rather than a mechanical text conflict:

  • This branch's inspect_and_review(repo: str, number: int) -> int implements this PR's core feature — fail closed unless the current head has a primary OpenCode approval (current_primary_approval(pr)), checked first, before any skip path.
  • origin/main's current inspect_and_review(repo: str, number: int, expected_head: str) -> int has a different signature and a different fail-closed mechanism: it takes an explicit expected_head parameter, normalizes it, and calls require_expected_head(pr, expected_head) first to reject a closed/stale-head PR before any model work.
  • current_primary_approval (this branch's whole reason for existing) does not exist anywhere in scripts/ci/noema_review_gate.py on origin/main — confirmed via git show origin/main:scripts/ci/noema_review_gate.py | grep current_primary_approval (no match).
  • Downstream ordering also diverges: this branch's post-actor-check gate list (has_current_changes_requested, has_unresolved_threads, blocking_checks) is absent from origin/main's current version at the equivalent point.
  • The actor-is-primary-reviewer case changed from a soft skip (return 1 with a log line, this branch) to a hard raise RuntimeError (origin/main).

This means main has since evolved a different, already-deployed fail-closed mechanism (exact-head binding via an explicit expected_head argument threaded from the caller) for a related but distinct problem (stale/closed-PR staleness) than the one this PR adds (missing current-head OpenCode approval). Whether these two fail-closed properties should now compose (both checks), whether one subsumes the other, or whether this PR's whole approach needs re-architecting around the expected_head-passing caller contract main now has, is a design decision on a security-critical gate — not something to guess through via mechanical conflict resolution. Guessing wrong here risks silently dropping one of the two fail-closed properties.

Per this repo's own standard for this exact situation (see #1279, #1280, #1382): aborting the merge cleanly, not pushing a guessed resolution, and leaving this for the dedicated owner/a design-aware pass. No changes pushed to this branch.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4


Generated by Claude Code

Two conflicted files, four blocks. The branch's delta splits into two
independent parts with opposite verdicts.

PART A - paginated review evidence: KEPT (49 of the branch's 52 added
script lines).
  main still fetches reviews through GraphQL 'reviews(last: 100)'
  (noema_review_gate.py:324), so a review event older than the last 100
  is invisible to existing_noema_review() - main's only consumer of
  pr['reviews']['nodes'] - and Noema re-reviews a PR it already reviewed.
  fetch_complete_reviews() and its fetch_pr wiring merged cleanly outside
  every conflict block and are unchanged here.

PART B - fail-closed guard before Noema skip paths: DROPPED as superseded
(3 of 52 lines).
  a643d8a 'fix: make Noema review independent (#1477)' deleted all four
  helpers this guard is built on. Verified org-wide, not just in this
  file: current_primary_approval, has_current_changes_requested,
  has_unresolved_threads and blocking_checks each return 0 hits across
  all of origin/main (positive control: require_expected_head, 16 hits).
  main closed the same hole by removing the skip paths rather than
  guarding them, and made the primary-actor case raise RuntimeError where
  the branch only returned 1 - strictly stronger than this PR's fix.
  The branch's own test also called the pre-#1477 signature
  (inspect_and_review missing required 'expected_head'), so it was
  superseded twice over.

Block verdicts:
- noema_review_gate.py x2  -> main's inspect_and_review structure. The
  branch's guard has no surviving call target.
- test file, block 1       -> main's test_existing_noema_review_matches_
  actor_and_head, plus the branch's two Part A tests appended at EOF (the
  conflict boundary cut through the middle of main's function, so they
  cannot be inserted at the block site).
- test file, block 2       -> main's parametrized cases; the branch's used
  make_pr shapes for deleted behaviour.
- test_noema_never_skips_success_without_current_head_primary_approval
  merged cleanly outside the blocks and was removed: it is Part B.

One assertion was translated, not dropped:
test_fetch_pr_keeps_exact_head_approval_older_than_one_hundred_reviews
asserted through the deleted current_primary_approval(). It now asserts
main's live path directly - len(pr['reviews']['nodes']) == 101 and
nodes[0] is the pre-page-boundary approval - which is the test's stated
purpose and keeps its detection power.

Evidence:
- uvx ruff check --select F821 scripts/ci tests: All checks passed
- full suite, branch head b8f4adb (unmerged): 2099 passed, 0 failed
- full suite, this merge:                      2899 passed, 0 failed
  (failure-name diff empty both ways, denominators 2099 and 2899)
- negative control: deleting the fetch_pr REST wiring makes the kept
  pagination test fail by name -> it detects the regression it claims to
- coverage: TOTAL 100%, noema_review_gate.py 860/860 100%
- interrogate: PASSED (minimum 100.0%)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@seonghobae

Copy link
Copy Markdown
Contributor Author

main 병합 완료 — 이 PR의 델타가 둘로 갈라졌습니다 (6e32ebea)

충돌은 2개 파일 4블록이었고, 브랜치 델타는 서로 독립적인 두 부분으로 나뉘며 판정이 반대입니다.

Part A — 페이지네이션된 리뷰 증거: 보존 (스크립트 추가분 52줄 중 49줄)

main은 아직 GraphQL reviews(last: 100)으로 리뷰를 읽습니다(scripts/ci/noema_review_gate.py:324). main에서 pr["reviews"]["nodes"]유일한 소비처는 existing_noema_review()(:430)이므로, 리뷰 이벤트가 100건을 넘으면 이미 작성된 Noema 리뷰가 안 보이고 Noema가 같은 PR을 다시 리뷰합니다. fetch_complete_reviews()fetch_pr 배선은 충돌 블록 바깥에서 깨끗이 병합되었고 이번 병합에서 손대지 않았습니다.

Part B — Noema 스킵 경로 앞의 fail-closed 가드: 대체됨으로 폐기 (52줄 중 3줄)

a643d8ab fix: make Noema review independent (#1477) 가 이 가드가 딛고 선 헬퍼 네 개를 전부 삭제했습니다. 파일 하나가 아니라 origin/main 전체에서 확인했습니다:

심볼 main 전체 hits
current_primary_approval 0
has_current_changes_requested 0
has_unresolved_threads 0
blocking_checks 0
(포지티브 컨트롤) require_expected_head 16

main은 같은 구멍을 가드를 붙이는 대신 스킵 경로 자체를 없애서 막았고, 1차 리뷰어 액터 케이스는 return 0 스킵에서 raise RuntimeError로 바뀌었습니다 — 이 PR의 return 1보다 강합니다. 게다가 이 PR의 테스트는 #1477 이전 시그니처를 호출합니다(inspect_and_review() missing 1 required positional argument: 'expected_head'). 이중으로 대체된 상태입니다.

블록별 판정

블록 판정
noema_review_gate.py ×2 main의 inspect_and_review 구조 채택. 브랜치 가드는 호출 대상이 남아 있지 않습니다.
테스트 블록1 main의 test_existing_noema_review_matches_actor_and_head + 브랜치의 Part A 테스트 2개를 파일 말미에 추가. 충돌 경계가 main 함수 중간을 지나가서 원위치 삽입은 함수를 깨뜨립니다.
테스트 블록2 main의 파라미터 케이스 채택. 브랜치 케이스는 삭제된 동작의 make_pr 형태였습니다.
test_noema_never_skips_success_without_current_head_primary_approval 충돌 밖에서 병합되어 남아 있었고 제거했습니다 — Part B입니다.

단언 하나는 버리지 않고 번역했습니다

test_fetch_pr_keeps_exact_head_approval_older_than_one_hundred_reviews가 삭제된 current_primary_approval()을 통해 단언하고 있었습니다. main의 실제 경로로 바꿨습니다 — len(pr["reviews"]["nodes"]) == 101nodes[0]이 페이지 경계 이전의 승인인지. 테스트가 스스로 밝힌 목적("must not lose a valid approval behind GitHub's review page size") 그대로이고 탐지력도 유지합니다.

검증

  • uvx ruff check --select F821 scripts/ci tests — All checks passed
  • 전체 스위트, 병합 전 브랜치 헤드 b8f4adb5: 2099 passed / 0 failed
  • 전체 스위트, 이 병합: 2899 passed / 0 failed (실패 이름 차집합 양방향 공집합, 분모 2099·2899)
  • 네거티브 컨트롤: fetch_pr의 REST 배선을 지우면 보존한 페이지네이션 테스트가 이름을 찍고 실패 — 주장하는 회귀를 실제로 탐지합니다
  • coverage TOTAL 100%, noema_review_gate.py 860/860 100%
  • interrogate PASSED (minimum 100.0%)

fast-forward 푸시이며 force push는 없습니다.

확인 요청: 이 PR의 제목은 Part B(fail closed before every Noema skip path)인데 실질 내용은 이제 Part A입니다. 제목을 페이지네이션 수정에 맞게 바꾸시는 편이 리뷰어에게 정확합니다.

🤖 Generated with Claude Code

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

Labels

area: security Security boundary, hardening, or vulnerability prevention merge: urgent-review Central or critical pull request requiring immediate review attention priority: critical Immediate blocker, P0, urgent deadlock, or critical incident status: blocked Blocked by conflict, dependency, or required prerequisite type: bug Defect or incorrect behavior

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants