diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..4ded1258a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ 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. - 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..6d50b7e68 --- /dev/null +++ b/docs/doctoring/review-line-anchored-findings.md @@ -0,0 +1,64 @@ +# 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. 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` + (`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. + +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. + +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: + +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. +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`. + +## 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) + +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 + +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..a877927b4 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -587,6 +587,25 @@ 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. 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() + 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 +1369,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..d849f1fa4 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,56 @@ 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("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" + ) + 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 +2600,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 &",