From ca0460905dc92c1d5378479ea90e8dbe13a2a34f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 15:35:32 +0900 Subject: [PATCH 1/5] fix(review): require line-anchored current-head REQUEST_CHANGES findings GitHub rejects inline review comments on unchanged paths or past-EOF lines with HTTP 422. Fail-close those findings in the trusted normalizer so blockers attach on Files changed. --- CHANGELOG.md | 1 + .../review-line-anchored-findings.md | 50 +++++++++++++++++++ .../ci/opencode_review_normalize_output.py | 19 +++++++ scripts/ci/opencode_review_prompt_template.md | 2 +- scripts/ci/run_opencode_review_model_pool.sh | 2 +- tests/test_opencode_agent_contract.py | 4 ++ .../test_opencode_review_normalize_output.py | 44 ++++++++++++++-- 7 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 docs/doctoring/review-line-anchored-findings.md diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..370d4e1db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Rejected OpenCode `REQUEST_CHANGES` findings whose path is not an exact current-head changed file or whose line is past EOF, so GitHub can attach inline review comments instead of dropping unanchored blockers. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/docs/doctoring/review-line-anchored-findings.md b/docs/doctoring/review-line-anchored-findings.md new file mode 100644 index 000000000..75260a848 --- /dev/null +++ b/docs/doctoring/review-line-anchored-findings.md @@ -0,0 +1,50 @@ +# Line-anchored OpenCode REQUEST_CHANGES findings + +검토 기준일: **2026-08-13** + +## Incident + +OpenCode already posts GitHub pull-request reviews with a `comments: []` array so blockers can appear on the Files changed view. The control JSON required a positive `line`, but the trusted normalizer accepted any path string and any in-range integer. Two classes of output therefore survived the gate and then failed at the GitHub Reviews API: + +1. A finding on a file that is not in the current-head changed-file list. GitHub's review-comment endpoints only accept a `path` that is part of the pull request diff; otherwise they return HTTP 422 (GitHub, n.d.-a, n.d.-b). +2. A finding whose `line` is past the current-head file length. That number cannot be a RIGHT-side blob line, so GitHub again rejects the inline comment. The workflow then has to explain the anchor failure instead of showing the blocker next to the code. + +Unanchored blockers are also a weaker review artifact: modern code review is expected to name a concrete location the author can act on, not a file-level or repository-level remark (Bacchelli & Bird, 2013). + +## Decision + +`scripts/ci/opencode_review_normalize_output.py` now fail-closes each `REQUEST_CHANGES` finding through `finding_location_error()` before the review is published: + +- `path` must be a non-empty string. +- When the trusted changed-file artifact is present, `path` must be an exact current-head changed file. +- The path/line pair must then pass the existing bounded source-tree probe (`adversarial_probe_location_error`): the file exists in `OPENCODE_SOURCE_WORKDIR`, is a regular file under the 2 MiB bound, and `line` is `<=` the current-head line count. + +When the changed-file artifact is absent, membership cannot be proven; the source-tree existence and line-length checks still apply. Findings about files that *should* have been changed but were not belong in the review body, not in the inline `comments` array. + +The reviewer prompt states the same contract: path is an exact current-head changed file; line is a positive integer that exists in that file; never line 0, an unchanged path, or a line past EOF. + +This change does not alter APPROVE semantics, review-agent `edit: deny`, two-approval rules, or mention-dispatch. + +## Verification contract + +`tests/test_opencode_review_normalize_output.py` pins: + +1. A finding on `scripts/ci/example.py:7` still validates. +2. A finding on `README.md` is rejected as not a current-head changed file. +3. A finding at line 999 is rejected as past EOF. +4. An empty path is rejected. +5. With the changed-file artifact removed, a missing path still fails because it does not exist in the trusted source tree. + +`tests/test_opencode_agent_contract.py` pins the prompt phrases `exact current-head changed file` and `line past EOF`. + +## Rollback + +If a legitimate current-head blocker cannot be expressed as an exact changed-file path plus existing line, keep the finding in the review body and do not loosen the inline-comment gate. Do not accept `N/A`, line 0, or off-diff paths so that GitHub can attach a thread. + +## References (APA 7th) + +Bacchelli, A., & Bird, C. (2013). Expectations, outcomes, and challenges of modern code review. In *Proceedings of the 35th International Conference on Software Engineering* (pp. 712–721). IEEE. https://doi.org/10.1109/ICSE.2013.6606617 + +GitHub. (n.d.-a). *Create a review for a pull request*. GitHub Docs. Retrieved August 13, 2026, from https://docs.github.com/en/rest/pulls/reviews#create-a-review-for-a-pull-request + +GitHub. (n.d.-b). *Create a review comment for a pull request*. GitHub Docs. Retrieved August 13, 2026, from https://docs.github.com/en/rest/pulls/comments#create-a-review-comment-for-a-pull-request diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index 4045d457c..0da9c9b84 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -587,6 +587,22 @@ def required_adversarial_probe_count() -> int: return 1 +def finding_location_error(path: str, line: int) -> str: + """Return why a REQUEST_CHANGES finding is not line-anchored on current head. + + CodeRabbit-style blockers name a changed file and a real source line so + GitHub can attach an inline review comment. A positive integer on an + unchanged path or past EOF is not an anchor. + """ + + if not isinstance(path, str) or not path.strip(): + return "path must be a non-empty current-head file" + changed_files = current_changed_files() + if changed_files and path not in changed_files: + return "path is not a current-head changed file" + return adversarial_probe_location_error(path, line) + + def adversarial_probe_location_error(path: str, line: int) -> str: """Return why a probe path/line is not present in the bounded source tree.""" source_root_text = os.environ.get("OPENCODE_SOURCE_WORKDIR", "").strip() @@ -1350,6 +1366,9 @@ def reject(reason: str) -> None: return reject( f"finding {finding_index} field {field} must be a non-empty string" ) + location_error = finding_location_error(str(finding["path"]).strip(), line) + if location_error: + return reject(f"finding {finding_index} {location_error}") normalized_findings.append(finding) normalized = { diff --git a/scripts/ci/opencode_review_prompt_template.md b/scripts/ci/opencode_review_prompt_template.md index 32614dcfc..5fc6081b2 100644 --- a/scripts/ci/opencode_review_prompt_template.md +++ b/scripts/ci/opencode_review_prompt_template.md @@ -32,7 +32,7 @@ When a claim can be tested, use python3 scripts/ci/sandboxed_verify.py --repo-ro Draw the right diagram. The required DAG evidence is not a file inventory. Use CodeGraph and focused source reads to identify the PR's relevant functions, classes, routes, components, database objects, workflows, or domain transitions, then compare base branch behavior with PR head behavior when that affects review. Include the most useful compact Mermaid diagram: sequenceDiagram for runtime message flow, classDiagram for class/API shape, erDiagram for schema/data relationship changes, stateDiagram for state transitions, or flowchart/DAG for function/control flow. Node labels must be quoted, for example A["parse_request"], so spaces, punctuation, parentheses, and file counts render safely. If CodeGraph cannot represent the changed surface, say why and draw a source-backed focused flow instead. -Lead with severity-ordered findings. REQUEST_CHANGES findings must be actionable, source-backed, and line-specific: path, positive line, severity, title, problem, root_cause, fix_direction, regression_test_direction, and suggested_diff. The line value must be a positive integer from a current-head source, test, workflow, config, or evidence line; never use line 0. Include observable impact, trigger condition, exact failed log/check phrase when relevant, and a concrete verification command when the repository provides one. Do not request changes with only a check URL, workflow name, generic failure summary, raw tool-access failure, or missing-string marker. Suggested diffs must be GitHub suggestion-ready when possible, and every removed line must exist in the cited current local file. +Lead with severity-ordered findings. REQUEST_CHANGES findings must be actionable, source-backed, and line-specific: path, positive line, severity, title, problem, root_cause, fix_direction, regression_test_direction, and suggested_diff. The path must be an exact current-head changed file. The line value must be a positive integer that exists in that current-head file; never use line 0, an unchanged path, or a line past EOF. Include observable impact, trigger condition, exact failed log/check phrase when relevant, and a concrete verification command when the repository provides one. Do not request changes with only a check URL, workflow name, generic failure summary, raw tool-access failure, or missing-string marker. Suggested diffs must be GitHub suggestion-ready when possible, and every removed line must exist in the cited current local file. Before APPROVE, the JSON summary must name at least one exact changed file path and include these exact labels: Approval sufficiency:, Verification posture:, Linter/static:, TDD/regression:, Coverage:, Docstring coverage:, DAG:, PoC/execution:, DDD/domain:, CDD/context:, Similar issues:, Claim/concept check:, Standards search:, Compatibility/convention:, Breaking-change/backcompat:, Performance:, Developer experience:, User experience:, Visual/DOM:, Accessibility/i18n:, Supply-chain/license:, Packaging:, Security/privacy:. diff --git a/scripts/ci/run_opencode_review_model_pool.sh b/scripts/ci/run_opencode_review_model_pool.sh index 986982e9a..1b29b1236 100644 --- a/scripts/ci/run_opencode_review_model_pool.sh +++ b/scripts/ci/run_opencode_review_model_pool.sh @@ -185,7 +185,7 @@ write_prompt() { else printf 'If file reads do not execute for this non-inlined prompt, do not approve from memory or generic confidence. REQUEST_CHANGES only when the visible launcher text or executed file reads provide current-head evidence tied to a positive source/evidence line.\n' fi - printf 'Do not request changes solely because your tool call, MCP call, or full-file read was not executed. Treat that as a review source limitation unless current-head evidence explicitly reports a materialization failure; any such finding must be tied to that evidence, not a generic model-exhaustion message. REQUEST_CHANGES findings must cite a positive source/evidence line; never use line 0.\n' + printf 'Do not request changes solely because your tool call, MCP call, or full-file read was not executed. Treat that as a review source limitation unless current-head evidence explicitly reports a materialization failure; any such finding must be tied to that evidence, not a generic model-exhaustion message. REQUEST_CHANGES findings must cite an exact current-head changed file and a positive line that exists in that file; never use line 0, an unchanged path, or a line past EOF.\n' printf 'Always return a final control block instead of a progress summary. Return only the final review body.\n\n' printf 'Adversarial evidence must state a concrete observed pass, failure, rejection, return value, exit code, or trace outcome and copy exactly one source-line-sha256=<64 lowercase hex> receipt with its matching path and line from the trusted receipt section; generic source-inspection or coverage-verification claims are invalid.\n' printf 'Current-run identity values are head_sha=%s, run_id=%s, run_attempt=%s. Copy them into the one final control object required by the contract file.\n' "$HEAD_SHA" "$RUN_ID" "$RUN_ATTEMPT" diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index daeaa37a2..9566bf649 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1676,6 +1676,8 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "Never emit raw tool-call markup" in model_pool_runner assert "Do not request changes solely because your tool call" in model_pool_runner assert "never use line 0" in model_pool_runner + assert "exact current-head changed file" in model_pool_runner + assert "line past EOF" in model_pool_runner assert "retry budget exhausted" not in model_pool_runner assert ( 'OPENCODE_MODEL_CANDIDATES: "github-models/openai/gpt-5-nano"' not in workflow @@ -1736,6 +1738,8 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "Never print raw tool-call markup" in prompt_template assert "Do not request changes solely because your tool call" in prompt_template assert "never use line 0" in prompt_template + assert "exact current-head changed file" in prompt_template + assert "line past EOF" in prompt_template assert "Current-head authority order" in workflow assert "historical context only" in workflow assert "Do not infer active failed checks" in workflow diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index 590fb3e53..c5d790573 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -1509,7 +1509,7 @@ def test_check_structural_approval_rejects_invalid_or_unsafe_approvals( assert check_structural_approval(generic_deflection) == 4 -def test_valid_control_filters_shape_head_and_review_contract(): +def test_valid_control_filters_shape_head_and_review_contract(monkeypatch): kwargs = { "expected_head_sha": "head", "expected_run_id": "run", @@ -1598,10 +1598,48 @@ def test_valid_control_filters_shape_head_and_review_contract(): ) assert norm.valid_control(request, **kwargs)["result"] == "REQUEST_CHANGES" + off_list = finding(path="README.md", line=1) + reasons: list[str] = [] + assert ( + norm.valid_control( + dict(request, findings=[off_list]), + rejection_reasons=reasons, + **kwargs, + ) + is None + ) + assert any("not a current-head changed file" in reason for reason in reasons) + + past_eof = finding(line=999) + reasons.clear() + assert ( + norm.valid_control( + dict(request, findings=[past_eof]), + rejection_reasons=reasons, + **kwargs, + ) + is None + ) + assert any("exceeds the current-head file length" in reason for reason in reasons) + + assert norm.finding_location_error("scripts/ci/example.py", 7) == "" + assert ( + norm.finding_location_error("README.md", 1) + == "path is not a current-head changed file" + ) + assert ( + norm.finding_location_error("", 1) + == "path must be a non-empty current-head file" + ) + approve_without_findings_key = control() approve_without_findings_key.pop("findings") assert norm.valid_control(approve_without_findings_key, **kwargs)["findings"] == [] + monkeypatch.delenv("OPENCODE_CHANGED_FILES_FILE", raising=False) + norm.current_changed_files.cache_clear() + assert "does not exist" in norm.finding_location_error("README.md", 1) + def test_valid_control_canonicalizes_known_safe_finding_field_drift(): kwargs = { @@ -2554,8 +2592,8 @@ def test_escapes_html_comment_breakout(tmp_path): result="REQUEST_CHANGES", findings=[ { - "path": "test.py", - "line": 1, + "path": "scripts/ci/example.py", + "line": 7, "severity": "high", "title": "Test finding", "problem": "--> injected string with < and > and &", From 972c420c832bd33d600df79efbded53648a1ecd3 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 15:39:05 +0900 Subject: [PATCH 2/5] docs(review): cite CWE-1288 for line-anchored findings Record that a REQUEST_CHANGES path and line must be consistent with the trusted current-head artifact. Force the trusted-uv installer tests onto the linux x86_64 runner path and add the control-plane architecture diagram. --- ARCHITECTURE.md | 84 +++++++++++++++++++ CHANGELOG.md | 3 +- CLAUDE.md | 5 +- .../review-line-anchored-findings.md | 8 ++ ...st_materialize_base_python_requirements.py | 10 +++ 5 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 ARCHITECTURE.md diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 000000000..46938b5d0 --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,84 @@ +# Architecture — ContextualWisdomLab `.github` + +This repository is the organization control plane. It is not naruon and it +does not own product data. Sibling products remain standalone modules; this +repo publishes org profile assets, reusable required workflows, and the +review/merge schedulers those products consume. + +## System context + +```mermaid +flowchart LR + Buyer["Commercial buyer / reviewer"] + Agents["Agents on AGENTS.md"] + Project["GitHub Project #1"] + Hub["This repo: org .github"] + Products["Owned products
naruon · orchestrator · engines"] + Runner["Required workflows in each repo context"] + + Buyer --> Hub + Agents --> Project + Agents --> Hub + Project --> Hub + Hub --> Runner + Runner --> Products + Products -->|"standalone or as module"| Buyer +``` + +## Line-anchored REQUEST_CHANGES + +```mermaid +flowchart TD + Finding["REQUEST_CHANGES finding"] + Path{"Exact current-head changed file?"} + Line{"Line exists in current-head blob?"} + Inline["Publish GitHub inline comment"] + Body["Keep the remark in the review body"] + + Finding --> Path + Path -->|"no"| Body + Path -->|"yes"| Line + Line -->|"no"| Body + Line -->|"yes"| Inline +``` + +CWE-1288: path and line must be consistent with the trusted current-head +artifact. Reviewers stay `edit: deny`. + +## Control-plane data flow + +```mermaid +sequenceDiagram + participant PR as Pull request + participant RW as Required workflows + participant OC as OpenCode reviewer + participant SV as sandboxed_verify / web E2E + participant MS as Merge scheduler + + PR->>RW: pull_request_target on trusted base + RW->>OC: bounded evidence + NVIDIA NIM / OpenCode + OC->>SV: PoC command in isolated copy + SV-->>OC: redacted stdout/stderr + command metadata + OC-->>PR: APPROVE or request changes + MS->>PR: merge only on current-head approval + green checks +``` + +## Trust boundaries + +- Required review workflows execute **base-branch** scripts. +- Reviewer agents stay `edit: deny`. +- Logs redact credential shapes. They do not mask operational PII. +- LLM and scheduled agents bind `NVIDIA_NIM_API_KEY`. They never use + `COPILOT_GITHUB_TOKEN`. +- Rust remains the psychometric arithmetic owner. + +## Quality gates + +`scripts/ci/` ships with 100% statement/branch coverage and 100% +docstrings. + +## Related durable documents + +- [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) +- [`PR_GOVERNANCE_AUDIT.md`](PR_GOVERNANCE_AUDIT.md) +- [`docs/doctoring/review-line-anchored-findings.md`](docs/doctoring/review-line-anchored-findings.md) diff --git a/CHANGELOG.md b/CHANGELOG.md index 370d4e1db..52f4821a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ Semantic Versioning where the repository publishes a release. ### Fixed -- Rejected OpenCode `REQUEST_CHANGES` findings whose path is not an exact current-head changed file or whose line is past EOF, so GitHub can attach inline review comments instead of dropping unanchored blockers. +- Rejected OpenCode `REQUEST_CHANGES` findings whose path is not an exact current-head changed file or whose line is past EOF, so GitHub can attach inline review comments instead of dropping unanchored blockers. The decision record now cites CWE-1288 so path and line must stay consistent with the trusted current-head artifact. +- Recorded the org control-plane architecture, including the line-anchored finding gate, so agents reconstruct the inline-comment trust boundary from the repo instead of private memory. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/CLAUDE.md b/CLAUDE.md index 1c7bdb2f6..a176ba8e6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,7 +64,10 @@ Details: `README.md` and `PR_GOVERNANCE_AUDIT.md`. - `fuzz/` + `.clusterfuzzlite/` — Atheris fuzz targets for the review-output normalizer and the ClusterFuzzLite discovery marker. - `docs/` — master context, Project protocol, `org-required-workflow-rollout.md`, - `scorecard-governance.md`, SBOM inventory. + `scorecard-governance.md`, SBOM inventory. Doctoring records live under + `docs/doctoring/`. [`ARCHITECTURE.md`](ARCHITECTURE.md) is the control-plane + diagram for line-anchored REQUEST_CHANGES findings and merge trust + boundaries. - `.jules/` — recorded performance (`bolt.md`) and security (`sentinel.md`) learnings from past work on `scripts/ci/`; worth scanning before optimizing or hardening those scripts. diff --git a/docs/doctoring/review-line-anchored-findings.md b/docs/doctoring/review-line-anchored-findings.md index 75260a848..eda75c677 100644 --- a/docs/doctoring/review-line-anchored-findings.md +++ b/docs/doctoring/review-line-anchored-findings.md @@ -25,6 +25,11 @@ The reviewer prompt states the same contract: path is an exact current-head chan This change does not alter APPROVE semantics, review-agent `edit: deny`, two-approval rules, or mention-dispatch. +CWE-1288 forbids accepting internally inconsistent input (MITRE, 2026). +A finding whose path is not a current-head changed file, or whose line is +past EOF, is inconsistent with the trusted artifact and cannot become an +inline comment. + ## Verification contract `tests/test_opencode_review_normalize_output.py` pins: @@ -43,6 +48,9 @@ If a legitimate current-head blocker cannot be expressed as an exact changed-fil ## References (APA 7th) +MITRE. (2026). *CWE-1288: Improper validation of consistency within input*. +https://cwe.mitre.org/data/definitions/1288.html + Bacchelli, A., & Bird, C. (2013). Expectations, outcomes, and challenges of modern code review. In *Proceedings of the 35th International Conference on Software Engineering* (pp. 712–721). IEEE. https://doi.org/10.1109/ICSE.2013.6606617 GitHub. (n.d.-a). *Create a review for a pull request*. GitHub Docs. Retrieved August 13, 2026, from https://docs.github.com/en/rest/pulls/reviews#create-a-review-for-a-pull-request diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 8a383f0c2..10f682b3e 100644 --- a/tests/test_materialize_base_python_requirements.py +++ b/tests/test_materialize_base_python_requirements.py @@ -30,6 +30,13 @@ def _created_tool_directory(path: Path) -> str: return str(path) +def _force_linux_x86_64_installer(monkeypatch: pytest.MonkeyPatch) -> None: + """Exercise the installer path that GitHub-hosted linux x86_64 runners use.""" + monkeypatch.setattr(materializer.sys, "platform", "linux") + monkeypatch.setattr(materializer.platform, "machine", lambda: "x86_64") + materializer._install_trusted_uv.cache_clear() + + def test_materializes_only_regular_hash_locks_from_exact_base(tmp_path: Path) -> None: """A PR-modified lock cannot enter the networked coverage image build context.""" repo = tmp_path / "repo" @@ -644,6 +651,7 @@ def test_install_trusted_uv_verifies_version_and_caches_path( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """The installer writes one executable, verifies its version, and caches it.""" + _force_linux_x86_64_installer(monkeypatch) tool_dir = tmp_path / "uv" monkeypatch.setattr( materializer.tempfile, @@ -690,6 +698,7 @@ def test_install_trusted_uv_rejects_version_process_failures( failure: OSError | subprocess.TimeoutExpired, ) -> None: """A missing or hung downloaded executable is removed and rejected.""" + _force_linux_x86_64_installer(monkeypatch) tool_dir = tmp_path / "uv" monkeypatch.setattr( materializer.tempfile, @@ -721,6 +730,7 @@ def test_install_trusted_uv_rejects_wrong_version_or_exit_status( completed: subprocess.CompletedProcess[bytes], ) -> None: """Unexpected version output or a nonzero status cannot satisfy the pin.""" + _force_linux_x86_64_installer(monkeypatch) tool_dir = tmp_path / f"uv-{completed.returncode}-{len(completed.stdout)}" monkeypatch.setattr( materializer.tempfile, From 71cf52e030dbdd414e9e073022c275231f4f566d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 18:11:09 +0900 Subject: [PATCH 3/5] fix(review): reject line 0 and True as finding anchors finding_location_error deferred line-shape checks to the caller. Line 0 passes the EOF probe because 0 > line_count is false, so a current-head path could be treated as anchored. Reject bool and non-positive lines inside the helper. --- AGENTS.md | 2 ++ ARCHITECTURE.md | 3 ++- CHANGELOG.md | 1 + CLAUDE.md | 4 ++++ docs/doctoring/review-line-anchored-findings.md | 6 ++++++ scripts/ci/opencode_review_normalize_output.py | 5 ++++- tests/test_opencode_review_normalize_output.py | 8 ++++++++ 7 files changed, 27 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 688b33035..215e2e176 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,3 +2,5 @@ > **Agents: read the master context FIRST.** Before any work, read [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) (mission · naruon-as-platform + inter-component UML · cross-cutting disciplines · conventions · roadmap · current state), the live **GitHub Project #1** (work/roadmap source of truth), the full spec **ContextualWisdomLab/naruon#974**, and operate the Project per [`docs/agent-github-project-protocol.md`](docs/agent-github-project-protocol.md). The repo/Project — not any private agent memory — is the source of truth. + +`finding_location_error` rejects line `0` and `True` before the EOF probe. See [`ARCHITECTURE.md`](ARCHITECTURE.md) and [`docs/doctoring/review-line-anchored-findings.md`](docs/doctoring/review-line-anchored-findings.md). diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 46938b5d0..87da515b3 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -43,7 +43,8 @@ flowchart TD ``` CWE-1288: path and line must be consistent with the trusted current-head -artifact. Reviewers stay `edit: deny`. +artifact. Line `0` and `True` are not anchors (`0 > line_count` is +false). Reviewers stay `edit: deny`. ## Control-plane data flow diff --git a/CHANGELOG.md b/CHANGELOG.md index 52f4821a6..2a1123c6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Rejected OpenCode `REQUEST_CHANGES` line `0` and `True` inside `finding_location_error` so a current-head path cannot be treated as anchored when the EOF probe would accept `0 > line_count` as false. - Rejected OpenCode `REQUEST_CHANGES` findings whose path is not an exact current-head changed file or whose line is past EOF, so GitHub can attach inline review comments instead of dropping unanchored blockers. The decision record now cites CWE-1288 so path and line must stay consistent with the trusted current-head artifact. - Recorded the org control-plane architecture, including the line-anchored finding gate, so agents reconstruct the inline-comment trust boundary from the repo instead of private memory. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. diff --git a/CLAUDE.md b/CLAUDE.md index a176ba8e6..c24758488 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,3 +129,7 @@ repeatable compile command. cross-repo references as `owner/repo#num` or full URLs; durable knowledge in the repo/Project, not private memory; one roadmap phase at a time) are defined in `docs/CWL-MASTER-CONTEXT.md` §7 and apply here. + +`finding_location_error` rejects line `0` and `True` so the EOF probe +cannot treat `0 > line_count` as an anchor. See `ARCHITECTURE.md` and +`docs/doctoring/review-line-anchored-findings.md`. diff --git a/docs/doctoring/review-line-anchored-findings.md b/docs/doctoring/review-line-anchored-findings.md index eda75c677..f513244b9 100644 --- a/docs/doctoring/review-line-anchored-findings.md +++ b/docs/doctoring/review-line-anchored-findings.md @@ -16,6 +16,10 @@ Unanchored blockers are also a weaker review artifact: modern code review is exp `scripts/ci/opencode_review_normalize_output.py` now fail-closes each `REQUEST_CHANGES` finding through `finding_location_error()` before the review is published: - `path` must be a non-empty string. +- `line` must be a positive integer. Line `0` and JSON/`bool` `true` + (`isinstance(True, int)` is true in Python) are rejected inside + `finding_location_error` itself: `0 > line_count` is false, so the EOF + probe would otherwise treat line 0 as an anchor (CWE-1288; MITRE, 2026). - When the trusted changed-file artifact is present, `path` must be an exact current-head changed file. - The path/line pair must then pass the existing bounded source-tree probe (`adversarial_probe_location_error`): the file exists in `OPENCODE_SOURCE_WORKDIR`, is a regular file under the 2 MiB bound, and `line` is `<=` the current-head line count. @@ -39,6 +43,8 @@ inline comment. 3. A finding at line 999 is rejected as past EOF. 4. An empty path is rejected. 5. With the changed-file artifact removed, a missing path still fails because it does not exist in the trusted source tree. +6. Line `0` and `True` are rejected as not a positive integer even when + the path is an exact current-head changed file. `tests/test_opencode_agent_contract.py` pins the prompt phrases `exact current-head changed file` and `line past EOF`. diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index 0da9c9b84..a877927b4 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -592,9 +592,12 @@ def finding_location_error(path: str, line: int) -> str: CodeRabbit-style blockers name a changed file and a real source line so GitHub can attach an inline review comment. A positive integer on an - unchanged path or past EOF is not an anchor. + unchanged path or past EOF is not an anchor. Line 0 and ``bool`` + values are not anchors even when they would pass the EOF check. """ + if isinstance(line, bool) or not isinstance(line, int) or line <= 0: + return "line must be a positive integer" if not isinstance(path, str) or not path.strip(): return "path must be a non-empty current-head file" changed_files = current_changed_files() diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index c5d790573..d849f1fa4 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -1623,6 +1623,14 @@ def test_valid_control_filters_shape_head_and_review_contract(monkeypatch): assert any("exceeds the current-head file length" in reason for reason in reasons) assert norm.finding_location_error("scripts/ci/example.py", 7) == "" + assert ( + norm.finding_location_error("scripts/ci/example.py", 0) + == "line must be a positive integer" + ) + assert ( + norm.finding_location_error("scripts/ci/example.py", True) + == "line must be a positive integer" + ) assert ( norm.finding_location_error("README.md", 1) == "path is not a current-head changed file" From 67367344cba0be95c81832187436f85c31d65c9a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Fri, 14 Aug 2026 00:04:36 +0900 Subject: [PATCH 4/5] fix(coverage): accept only bounded relative requirement includes Materialize a base Python lock only when every package line is an exact SHA-256 pin or a two-token relative -r/--requirement include of a candidate lock path. A lone --require-hashes directive, ./dotted paths, and -r other-hashes.txt no longer enter the trusted build context. --- AGENTS.md | 2 +- CHANGELOG.md | 1 + .../review-line-anchored-findings.md | 2 +- .../materialize_base_python_requirements.py | 82 +++++++++++++++---- ...st_materialize_base_python_requirements.py | 19 ++++- 5 files changed, 88 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 215e2e176..896e5679a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,4 +3,4 @@ > **Agents: read the master context FIRST.** Before any work, read [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) (mission · naruon-as-platform + inter-component UML · cross-cutting disciplines · conventions · roadmap · current state), the live **GitHub Project #1** (work/roadmap source of truth), the full spec **ContextualWisdomLab/naruon#974**, and operate the Project per [`docs/agent-github-project-protocol.md`](docs/agent-github-project-protocol.md). The repo/Project — not any private agent memory — is the source of truth. -`finding_location_error` rejects line `0` and `True` before the EOF probe. See [`ARCHITECTURE.md`](ARCHITECTURE.md) and [`docs/doctoring/review-line-anchored-findings.md`](docs/doctoring/review-line-anchored-findings.md). +`finding_location_error` rejects line `0` and `True` before the EOF probe. Materialize accepts only exact SHA-256 pins or a bounded relative `-r` include (no `.`/`..`); a lone `--require-hashes` directive is not trust evidence. See [`ARCHITECTURE.md`](ARCHITECTURE.md) and [`docs/doctoring/review-line-anchored-findings.md`](docs/doctoring/review-line-anchored-findings.md). diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a1123c6b..4ded1258a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Materialized base Python locks only when every package line is an exact SHA-256 pin or a bounded relative `-r`/`--requirement` include. A lone `--require-hashes` directive, a dotted include such as `./lock.txt`, or `-r other-hashes.txt` no longer enters the trusted build context. - Rejected OpenCode `REQUEST_CHANGES` line `0` and `True` inside `finding_location_error` so a current-head path cannot be treated as anchored when the EOF probe would accept `0 > line_count` as false. - Rejected OpenCode `REQUEST_CHANGES` findings whose path is not an exact current-head changed file or whose line is past EOF, so GitHub can attach inline review comments instead of dropping unanchored blockers. The decision record now cites CWE-1288 so path and line must stay consistent with the trusted current-head artifact. - Recorded the org control-plane architecture, including the line-anchored finding gate, so agents reconstruct the inline-comment trust boundary from the repo instead of private memory. diff --git a/docs/doctoring/review-line-anchored-findings.md b/docs/doctoring/review-line-anchored-findings.md index f513244b9..6d50b7e68 100644 --- a/docs/doctoring/review-line-anchored-findings.md +++ b/docs/doctoring/review-line-anchored-findings.md @@ -13,7 +13,7 @@ Unanchored blockers are also a weaker review artifact: modern code review is exp ## Decision -`scripts/ci/opencode_review_normalize_output.py` now fail-closes each `REQUEST_CHANGES` finding through `finding_location_error()` before the review is published: +`scripts/ci/opencode_review_normalize_output.py` now fail-closes each `REQUEST_CHANGES` finding through `finding_location_error()` before the review is published. Materialize accepts only exact SHA-256 pins or a bounded relative `-r` include; a lone `--require-hashes` line is not lock evidence. - `path` must be a non-empty string. - `line` must be a positive integer. Line `0` and JSON/`bool` `true` diff --git a/scripts/ci/materialize_base_python_requirements.py b/scripts/ci/materialize_base_python_requirements.py index 98cdad459..9848c3ff6 100755 --- a/scripts/ci/materialize_base_python_requirements.py +++ b/scripts/ci/materialize_base_python_requirements.py @@ -87,6 +87,57 @@ def _is_candidate_lock_name(name: str) -> bool: ) +def _is_candidate_lock_path(path: pathlib.PurePosixPath) -> bool: + """Return whether one safe tracked path can name a pip requirements lock. + + In addition to conventional ``requirements*.txt`` names, repositories often + keep concrete environment closures as direct children such as + ``requirements/ci.txt`` or ``service/requirements/package.txt``. Only direct + ``.txt`` children of a directory named ``requirements`` gain this path-based + eligibility; content must still pass the independent complete hash-pin + validation before it reaches the trusted image build context. + """ + return _is_candidate_lock_name(path.name) or ( + path.suffix == ".txt" and path.parent.name == "requirements" + ) + + +def _is_bounded_requirement_include(line: str) -> bool: + """Return whether one requirements include names a bounded relative file. + + Includes are accepted only as a two-token ``-r``/``--requirement`` form + whose target is itself a candidate lock path written as a normalized + relative POSIX path. Absolute paths, ``.`` or ``..`` components, double + slashes, URLs, option-like targets, shell/Windows path separators, + fragments, queries, extra inline options or hashes, and includes of + non-lock files are rejected before a base-owned file can enter the + trusted build context. + The downstream installer still proves that the candidate is an independently + complete hash closure; this predicate grants syntax eligibility only. + """ + fields = line.split() + if len(fields) != 2 or fields[0] not in {"-r", "--requirement"}: + return False + target = fields[1] + if ( + target.startswith(("-", "~")) + or "\\" in target + or ":" in target + or "?" in target + or "#" in target + ): + return False + include_path = pathlib.PurePosixPath(target) + return ( + bool(include_path.parts) + and target == include_path.as_posix() + and not include_path.is_absolute() + and "." not in include_path.parts + and ".." not in include_path.parts + and _is_candidate_lock_path(include_path) + ) + + def _requirement_lines(content: bytes) -> list[str]: """Return logical requirement lines, joining backslash line-continuations. @@ -107,23 +158,26 @@ def _requirement_lines(content: bytes) -> list[str]: def _is_hash_pinned(content: bytes) -> bool: - """Return whether content carries hash pins and is safe to preflight. - - Discovery is content-based rather than name-based so hash-pinned locks in any - location (a service subdirectory, ``requirements-dev.txt``, - ``requirements-test.txt``) can be considered for offline coverage, while an - unpinned or PR-mutable requirements file is still excluded from the networked - build context. Hash syntax cannot prove that a file includes every transitive - dependency, so the trusted image installer separately preflights every - candidate as an independent ``--require-hashes`` closure. An empty file - carries no installable dependency and is not materialized. + """Return whether content carries only trusted pins or bounded includes. + + Discovery is content-based rather than name-based so exact hash-pinned locks + in service subdirectories and role-specific requirements files can be + considered for offline coverage. Candidate syntax is deliberately stricter + than a substring search: each package line must be an exact ``==`` pin with + one or more complete SHA-256 hashes, or a bounded relative requirements + include. A global ``--require-hashes`` directive is not trust evidence by + itself. The downstream installer separately preflights every candidate as an + independent ``pip --require-hashes`` closure, so syntax eligibility never + substitutes for dependency-closure proof. """ lines = _requirement_lines(content) - if not lines: + requirement_lines = [line for line in lines if line != "--require-hashes"] + if not requirement_lines: return False - return any(line == "--require-hashes" for line in lines) or all( - "--hash=" in line or line.startswith(("-r ", "--requirement ")) - for line in lines + return all( + _is_fully_hash_pinned_requirement(line) + or _is_bounded_requirement_include(line) + for line in requirement_lines ) diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 10f682b3e..317ab5f5c 100644 --- a/tests/test_materialize_base_python_requirements.py +++ b/tests/test_materialize_base_python_requirements.py @@ -157,9 +157,24 @@ def test_lock_name_candidates_are_pip_requirements_files() -> None: def test_hash_pin_detection_includes_pinned_and_excludes_unpinned_or_empty() -> None: """Only fully hash-pinned, non-empty lock content is materialized.""" assert not materializer._is_hash_pinned(b"# comment only\n\n") - assert materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") + assert not materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") assert materializer._is_hash_pinned(b"demo==1 --hash=sha256:" + b"a" * 64 + b"\n") - assert materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert materializer._is_hash_pinned(b"-r requirements-other.txt\n") + assert not materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert not materializer._is_hash_pinned(b"-r ./requirements-other.txt\n") + assert not materializer._is_hash_pinned(b"-r ../escape.txt\n") + assert materializer._is_bounded_requirement_include( + "--requirement requirements-other.txt" + ) + assert not materializer._is_bounded_requirement_include("-r .") + assert not materializer._is_bounded_requirement_include("-r -evil.txt") + assert not materializer._is_bounded_requirement_include("-r ~evil.txt") + assert not materializer._is_bounded_requirement_include("-r C:foo.txt") + assert not materializer._is_bounded_requirement_include("-r foo?bar.txt") + assert not materializer._is_bounded_requirement_include("-r foo#bar.txt") + assert not materializer._is_bounded_requirement_include(r"-r foo\\bar.txt") + assert not materializer._is_bounded_requirement_include("-r") + assert not materializer._is_bounded_requirement_include("-r /abs/requirements.txt") assert not materializer._is_hash_pinned(b"untrusted==1\n") # uv export / pip-compile multi-line continuation format (spec, then --hash= lines). assert materializer._is_hash_pinned( From a3b93b70b84a679f39faf32aa5e86adb4eaa97ac Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sat, 15 Aug 2026 22:30:37 +0900 Subject: [PATCH 5/5] chore(review): restore bounded line-anchor scope --- AGENTS.md | 2 - ARCHITECTURE.md | 85 ------------------- CLAUDE.md | 9 +- .../materialize_base_python_requirements.py | 82 +++--------------- ...st_materialize_base_python_requirements.py | 29 +------ 5 files changed, 17 insertions(+), 190 deletions(-) delete mode 100644 ARCHITECTURE.md diff --git a/AGENTS.md b/AGENTS.md index 896e5679a..688b33035 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,5 +2,3 @@ > **Agents: read the master context FIRST.** Before any work, read [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) (mission · naruon-as-platform + inter-component UML · cross-cutting disciplines · conventions · roadmap · current state), the live **GitHub Project #1** (work/roadmap source of truth), the full spec **ContextualWisdomLab/naruon#974**, and operate the Project per [`docs/agent-github-project-protocol.md`](docs/agent-github-project-protocol.md). The repo/Project — not any private agent memory — is the source of truth. - -`finding_location_error` rejects line `0` and `True` before the EOF probe. Materialize accepts only exact SHA-256 pins or a bounded relative `-r` include (no `.`/`..`); a lone `--require-hashes` directive is not trust evidence. See [`ARCHITECTURE.md`](ARCHITECTURE.md) and [`docs/doctoring/review-line-anchored-findings.md`](docs/doctoring/review-line-anchored-findings.md). diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md deleted file mode 100644 index 87da515b3..000000000 --- a/ARCHITECTURE.md +++ /dev/null @@ -1,85 +0,0 @@ -# Architecture — ContextualWisdomLab `.github` - -This repository is the organization control plane. It is not naruon and it -does not own product data. Sibling products remain standalone modules; this -repo publishes org profile assets, reusable required workflows, and the -review/merge schedulers those products consume. - -## System context - -```mermaid -flowchart LR - Buyer["Commercial buyer / reviewer"] - Agents["Agents on AGENTS.md"] - Project["GitHub Project #1"] - Hub["This repo: org .github"] - Products["Owned products
naruon · orchestrator · engines"] - Runner["Required workflows in each repo context"] - - Buyer --> Hub - Agents --> Project - Agents --> Hub - Project --> Hub - Hub --> Runner - Runner --> Products - Products -->|"standalone or as module"| Buyer -``` - -## Line-anchored REQUEST_CHANGES - -```mermaid -flowchart TD - Finding["REQUEST_CHANGES finding"] - Path{"Exact current-head changed file?"} - Line{"Line exists in current-head blob?"} - Inline["Publish GitHub inline comment"] - Body["Keep the remark in the review body"] - - Finding --> Path - Path -->|"no"| Body - Path -->|"yes"| Line - Line -->|"no"| Body - Line -->|"yes"| Inline -``` - -CWE-1288: path and line must be consistent with the trusted current-head -artifact. Line `0` and `True` are not anchors (`0 > line_count` is -false). Reviewers stay `edit: deny`. - -## Control-plane data flow - -```mermaid -sequenceDiagram - participant PR as Pull request - participant RW as Required workflows - participant OC as OpenCode reviewer - participant SV as sandboxed_verify / web E2E - participant MS as Merge scheduler - - PR->>RW: pull_request_target on trusted base - RW->>OC: bounded evidence + NVIDIA NIM / OpenCode - OC->>SV: PoC command in isolated copy - SV-->>OC: redacted stdout/stderr + command metadata - OC-->>PR: APPROVE or request changes - MS->>PR: merge only on current-head approval + green checks -``` - -## Trust boundaries - -- Required review workflows execute **base-branch** scripts. -- Reviewer agents stay `edit: deny`. -- Logs redact credential shapes. They do not mask operational PII. -- LLM and scheduled agents bind `NVIDIA_NIM_API_KEY`. They never use - `COPILOT_GITHUB_TOKEN`. -- Rust remains the psychometric arithmetic owner. - -## Quality gates - -`scripts/ci/` ships with 100% statement/branch coverage and 100% -docstrings. - -## Related durable documents - -- [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) -- [`PR_GOVERNANCE_AUDIT.md`](PR_GOVERNANCE_AUDIT.md) -- [`docs/doctoring/review-line-anchored-findings.md`](docs/doctoring/review-line-anchored-findings.md) diff --git a/CLAUDE.md b/CLAUDE.md index c24758488..1c7bdb2f6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,10 +64,7 @@ Details: `README.md` and `PR_GOVERNANCE_AUDIT.md`. - `fuzz/` + `.clusterfuzzlite/` — Atheris fuzz targets for the review-output normalizer and the ClusterFuzzLite discovery marker. - `docs/` — master context, Project protocol, `org-required-workflow-rollout.md`, - `scorecard-governance.md`, SBOM inventory. Doctoring records live under - `docs/doctoring/`. [`ARCHITECTURE.md`](ARCHITECTURE.md) is the control-plane - diagram for line-anchored REQUEST_CHANGES findings and merge trust - boundaries. + `scorecard-governance.md`, SBOM inventory. - `.jules/` — recorded performance (`bolt.md`) and security (`sentinel.md`) learnings from past work on `scripts/ci/`; worth scanning before optimizing or hardening those scripts. @@ -129,7 +126,3 @@ repeatable compile command. cross-repo references as `owner/repo#num` or full URLs; durable knowledge in the repo/Project, not private memory; one roadmap phase at a time) are defined in `docs/CWL-MASTER-CONTEXT.md` §7 and apply here. - -`finding_location_error` rejects line `0` and `True` so the EOF probe -cannot treat `0 > line_count` as an anchor. See `ARCHITECTURE.md` and -`docs/doctoring/review-line-anchored-findings.md`. diff --git a/scripts/ci/materialize_base_python_requirements.py b/scripts/ci/materialize_base_python_requirements.py index 9848c3ff6..98cdad459 100755 --- a/scripts/ci/materialize_base_python_requirements.py +++ b/scripts/ci/materialize_base_python_requirements.py @@ -87,57 +87,6 @@ def _is_candidate_lock_name(name: str) -> bool: ) -def _is_candidate_lock_path(path: pathlib.PurePosixPath) -> bool: - """Return whether one safe tracked path can name a pip requirements lock. - - In addition to conventional ``requirements*.txt`` names, repositories often - keep concrete environment closures as direct children such as - ``requirements/ci.txt`` or ``service/requirements/package.txt``. Only direct - ``.txt`` children of a directory named ``requirements`` gain this path-based - eligibility; content must still pass the independent complete hash-pin - validation before it reaches the trusted image build context. - """ - return _is_candidate_lock_name(path.name) or ( - path.suffix == ".txt" and path.parent.name == "requirements" - ) - - -def _is_bounded_requirement_include(line: str) -> bool: - """Return whether one requirements include names a bounded relative file. - - Includes are accepted only as a two-token ``-r``/``--requirement`` form - whose target is itself a candidate lock path written as a normalized - relative POSIX path. Absolute paths, ``.`` or ``..`` components, double - slashes, URLs, option-like targets, shell/Windows path separators, - fragments, queries, extra inline options or hashes, and includes of - non-lock files are rejected before a base-owned file can enter the - trusted build context. - The downstream installer still proves that the candidate is an independently - complete hash closure; this predicate grants syntax eligibility only. - """ - fields = line.split() - if len(fields) != 2 or fields[0] not in {"-r", "--requirement"}: - return False - target = fields[1] - if ( - target.startswith(("-", "~")) - or "\\" in target - or ":" in target - or "?" in target - or "#" in target - ): - return False - include_path = pathlib.PurePosixPath(target) - return ( - bool(include_path.parts) - and target == include_path.as_posix() - and not include_path.is_absolute() - and "." not in include_path.parts - and ".." not in include_path.parts - and _is_candidate_lock_path(include_path) - ) - - def _requirement_lines(content: bytes) -> list[str]: """Return logical requirement lines, joining backslash line-continuations. @@ -158,26 +107,23 @@ def _requirement_lines(content: bytes) -> list[str]: def _is_hash_pinned(content: bytes) -> bool: - """Return whether content carries only trusted pins or bounded includes. - - Discovery is content-based rather than name-based so exact hash-pinned locks - in service subdirectories and role-specific requirements files can be - considered for offline coverage. Candidate syntax is deliberately stricter - than a substring search: each package line must be an exact ``==`` pin with - one or more complete SHA-256 hashes, or a bounded relative requirements - include. A global ``--require-hashes`` directive is not trust evidence by - itself. The downstream installer separately preflights every candidate as an - independent ``pip --require-hashes`` closure, so syntax eligibility never - substitutes for dependency-closure proof. + """Return whether content carries hash pins and is safe to preflight. + + Discovery is content-based rather than name-based so hash-pinned locks in any + location (a service subdirectory, ``requirements-dev.txt``, + ``requirements-test.txt``) can be considered for offline coverage, while an + unpinned or PR-mutable requirements file is still excluded from the networked + build context. Hash syntax cannot prove that a file includes every transitive + dependency, so the trusted image installer separately preflights every + candidate as an independent ``--require-hashes`` closure. An empty file + carries no installable dependency and is not materialized. """ lines = _requirement_lines(content) - requirement_lines = [line for line in lines if line != "--require-hashes"] - if not requirement_lines: + if not lines: return False - return all( - _is_fully_hash_pinned_requirement(line) - or _is_bounded_requirement_include(line) - for line in requirement_lines + return any(line == "--require-hashes" for line in lines) or all( + "--hash=" in line or line.startswith(("-r ", "--requirement ")) + for line in lines ) diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 317ab5f5c..8a383f0c2 100644 --- a/tests/test_materialize_base_python_requirements.py +++ b/tests/test_materialize_base_python_requirements.py @@ -30,13 +30,6 @@ def _created_tool_directory(path: Path) -> str: return str(path) -def _force_linux_x86_64_installer(monkeypatch: pytest.MonkeyPatch) -> None: - """Exercise the installer path that GitHub-hosted linux x86_64 runners use.""" - monkeypatch.setattr(materializer.sys, "platform", "linux") - monkeypatch.setattr(materializer.platform, "machine", lambda: "x86_64") - materializer._install_trusted_uv.cache_clear() - - def test_materializes_only_regular_hash_locks_from_exact_base(tmp_path: Path) -> None: """A PR-modified lock cannot enter the networked coverage image build context.""" repo = tmp_path / "repo" @@ -157,24 +150,9 @@ def test_lock_name_candidates_are_pip_requirements_files() -> None: def test_hash_pin_detection_includes_pinned_and_excludes_unpinned_or_empty() -> None: """Only fully hash-pinned, non-empty lock content is materialized.""" assert not materializer._is_hash_pinned(b"# comment only\n\n") - assert not materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") + assert materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") assert materializer._is_hash_pinned(b"demo==1 --hash=sha256:" + b"a" * 64 + b"\n") - assert materializer._is_hash_pinned(b"-r requirements-other.txt\n") - assert not materializer._is_hash_pinned(b"-r other-hashes.txt\n") - assert not materializer._is_hash_pinned(b"-r ./requirements-other.txt\n") - assert not materializer._is_hash_pinned(b"-r ../escape.txt\n") - assert materializer._is_bounded_requirement_include( - "--requirement requirements-other.txt" - ) - assert not materializer._is_bounded_requirement_include("-r .") - assert not materializer._is_bounded_requirement_include("-r -evil.txt") - assert not materializer._is_bounded_requirement_include("-r ~evil.txt") - assert not materializer._is_bounded_requirement_include("-r C:foo.txt") - assert not materializer._is_bounded_requirement_include("-r foo?bar.txt") - assert not materializer._is_bounded_requirement_include("-r foo#bar.txt") - assert not materializer._is_bounded_requirement_include(r"-r foo\\bar.txt") - assert not materializer._is_bounded_requirement_include("-r") - assert not materializer._is_bounded_requirement_include("-r /abs/requirements.txt") + assert materializer._is_hash_pinned(b"-r other-hashes.txt\n") assert not materializer._is_hash_pinned(b"untrusted==1\n") # uv export / pip-compile multi-line continuation format (spec, then --hash= lines). assert materializer._is_hash_pinned( @@ -666,7 +644,6 @@ def test_install_trusted_uv_verifies_version_and_caches_path( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """The installer writes one executable, verifies its version, and caches it.""" - _force_linux_x86_64_installer(monkeypatch) tool_dir = tmp_path / "uv" monkeypatch.setattr( materializer.tempfile, @@ -713,7 +690,6 @@ def test_install_trusted_uv_rejects_version_process_failures( failure: OSError | subprocess.TimeoutExpired, ) -> None: """A missing or hung downloaded executable is removed and rejected.""" - _force_linux_x86_64_installer(monkeypatch) tool_dir = tmp_path / "uv" monkeypatch.setattr( materializer.tempfile, @@ -745,7 +721,6 @@ def test_install_trusted_uv_rejects_wrong_version_or_exit_status( completed: subprocess.CompletedProcess[bytes], ) -> None: """Unexpected version output or a nonzero status cannot satisfy the pin.""" - _force_linux_x86_64_installer(monkeypatch) tool_dir = tmp_path / f"uv-{completed.returncode}-{len(completed.stdout)}" monkeypatch.setattr( materializer.tempfile,