Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
64 changes: 64 additions & 0 deletions docs/doctoring/review-line-anchored-findings.md
Original file line number Diff line number Diff line change
@@ -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
22 changes: 22 additions & 0 deletions scripts/ci/opencode_review_normalize_output.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 = {
Expand Down
2 changes: 1 addition & 1 deletion scripts/ci/opencode_review_prompt_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:.
Expand Down
2 changes: 1 addition & 1 deletion scripts/ci/run_opencode_review_model_pool.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
4 changes: 4 additions & 0 deletions tests/test_opencode_agent_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
52 changes: 49 additions & 3 deletions tests/test_opencode_review_normalize_output.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 &",
Expand Down
Loading