diff --git a/AGENTS.md b/AGENTS.md index 16f0981c0..df06b7f4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,3 +5,4 @@ Materialize accepts only exact SHA-256 pins or a bounded relative `-r` include (no `.`/`..`); a lone `--require-hashes` directive is not trust evidence. See [`docs/doctoring/hourly-nvidia-nim-autofix.md`](docs/doctoring/hourly-nvidia-nim-autofix.md). Conflict-scope roots fail closed when the immediate parent directory is a symbolic link. +OpenCode APPROVE must name every current-head changed file as a whole path token; Unicode prefix/suffix siblings are not a disposition. See [`docs/doctoring/review-contract-per-file-disposition.md`](docs/doctoring/review-contract-per-file-disposition.md). diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 6fe6621b6..b12215fe6 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -25,6 +25,12 @@ flowchart LR Products -->|"standalone or as module"| Buyer ``` +## Per-file OpenCode APPROVE + +`valid_control` rejects APPROVE unless every current-head changed path +appears as a whole token. A longer sibling (`example.py.bak` or +`docs/한.md추가`) is not a disposition of the shorter path. + ## Hourly NVIDIA NIM repair gate ```mermaid diff --git a/CHANGELOG.md b/CHANGELOG.md index 1de9130a5..f7c43b40d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Require OpenCode APPROVE to name every current-head changed file as a whole path token so a sibling such as `example.py.bak` or `docs/한.md추가` cannot stand in for the shorter path. - 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. - Refused a conflict-scope repository root whose immediate parent is a symbolic link, so a swapped parent cannot redirect the canonical worktree after the last-component check (CWE-367). - 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 7127d3c1c..0d874c210 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -114,6 +114,8 @@ repeatable compile command. without running the test suite will break CI. - **100% coverage and 100% docstrings on `scripts/ci/`** are hard gates, not aspirations. New helper code needs matching tests and docstrings. +- **OpenCode APPROVE** must name every current-head changed file as a whole path token. + Prefix or suffix siblings such as `example.py.bak` or `docs/한.md추가` do not count. - **`pull_request_target` trust boundary.** The required review workflows run the *base branch's* trusted scripts. A PR that edits the trusted review workflows can fail its own checks until the base branch catches up; a same-head manual `workflow_dispatch` Strix run may supply review evidence diff --git a/docs/doctoring/review-contract-per-file-disposition.md b/docs/doctoring/review-contract-per-file-disposition.md new file mode 100644 index 000000000..084250d2c --- /dev/null +++ b/docs/doctoring/review-contract-per-file-disposition.md @@ -0,0 +1,76 @@ +# OpenCode approval must name every current-head changed file + +검토 기준일: **2026-08-13** + +## Incident + +The OpenCode normalizer accepted an APPROVE when the reason/summary named +*one* current-head path. CodeRabbit-style reviews walk every changed file. +Buyers comparing the two saw OpenCode approve after a single path citation +while leaving the rest of the diff unnamed. That is the “less detailed than +CodeRabbit” gap. + +## Decision + +`unnamed_changed_files(reason, summary)` returns every path from +`current_changed_files()` that is not named as a whole path token. +Materialize accepts only exact SHA-256 pins or a bounded relative `-r` +include; a lone `--require-hashes` line is not lock evidence. +A longer sibling such as ``example.py.bak`` or ``docs/한.md추가`` contains +the shorter path as a prefix substring; that is not a disposition of the +shorter file (CWE-1288; MITRE, 2026). Path-token continuation uses +Unicode letters and digits, not an ASCII-only class, so a Hangul suffix +or prefix sibling cannot stand in for the shorter path (Unicode +Consortium, 2024). ``path:line`` still counts. `valid_control` +rejects APPROVE when that tuple is non-empty, both before and after +bounded-evidence repair. Developer experience / User experience +section labels were already required; this change only closes the file-walk +hole. + +An empty changed-file set remains a no-op (`()`), so identity-only PRs are +unchanged. + +IEEE 1028 requires every software product in the review package to +receive a recorded disposition (IEEE, 2008). Naming one path therefore +cannot authorize APPROVE when the trusted artifact lists more files. + +## Trust boundary + +Changed-file identity still comes from the trusted workflow artifact, not +from model prose. The model cannot invent a smaller file set to pass the +walk. + +## Verification contract + +`test_approval_must_name_every_current_head_changed_file` uses a two-file +current-head list (`scripts/ci/example.py` and `.github/workflows/strix.yml`). +Naming only the first file is rejected. Naming both is accepted. A post-repair +reason that drops the second path is still rejected. Naming only +``scripts/ci/example.py.bak`` still leaves ``scripts/ci/example.py`` unnamed. +Naming only ``docs/한.md추가`` or ``추가docs/한.md`` still leaves +``docs/한.md`` unnamed. + +## Rollback + +Remove the `unnamed_changed_files` checks from the two APPROVE blocks and +the helper. Existing “at least one file” detection remains. + +## References (APA 7th) + +MITRE. (2026). *CWE-1288: Improper validation of consistency within input*. +https://cwe.mitre.org/data/definitions/1288.html + +IEEE. (2008). *IEEE standard for software reviews and audits* (IEEE Std +1028-2008). https://doi.org/10.1109/IEEESTD.2008.4601584 + +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.). *About pull request reviews*. GitHub Docs. Retrieved +August 13, 2026, from +https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews + +Unicode Consortium. (2024). *The Unicode Standard* (Version 16.0.0). +https://www.unicode.org/versions/Unicode16.0.0/ diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index 4045d457c..900e7d8fc 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -854,13 +854,83 @@ def contradicts_material_changed_file_scope(reason: str, summary: str) -> bool: return any(phrase in combined for phrase in MATERIAL_CHANGE_FALSE_PHRASES) +_PATH_TOKEN_SEPARATORS = frozenset("_-+/") + + +def _path_token_char(char: str) -> bool: + """Return True when ``char`` can appear inside a repository path token.""" + return char in _PATH_TOKEN_SEPARATORS or char.isalnum() + + +def _path_token_continues(text: str, index: int) -> bool: + """Return True when ``text[index]`` extends a repository path token.""" + if index < 0 or index >= len(text): + return False + char = text[index] + if _path_token_char(char): + return True + if char == "." and index + 1 < len(text): + nxt = text[index + 1] + if nxt == "." or _path_token_char(nxt): + return True + return False + + +def changed_file_named_in_text(text: str, path: str) -> bool: + """Return True when ``path`` appears as a whole path token in ``text``. + + A longer sibling such as ``example.py.bak`` or ``docs/한.md추가`` + contains the shorter path as a prefix substring. That is not a + disposition of the shorter file. A sentence period after ``.yml`` is + not a continuation. + """ + + if not path or not text: + return False + start = 0 + while True: + index = text.find(path, start) + if index < 0: + return False + after = index + len(path) + if not _path_token_continues(text, after) and not _path_token_continues( + text, index - 1 + ): + return True + start = index + 1 + + def mentions_actual_changed_file(reason: str, summary: str) -> bool: """Return whether an approval names an exact current-head changed file.""" changed_files = current_changed_files() if not changed_files: return False combined = f"{reason}\n{summary}" - return any(changed_file in combined for changed_file in changed_files) + return any( + changed_file_named_in_text(combined, changed_file) + for changed_file in changed_files + ) + + +def unnamed_changed_files(reason: str, summary: str) -> tuple[str, ...]: + """Return current-head changed files the approval never names. + + Naming one file is not a file-by-file walk. Live OpenCode approvals that + cited a single path while leaving the rest unnamed were thinner than the + CodeRabbit per-file contract the buyer asked for. + """ + + changed_files = current_changed_files() + if not changed_files: + return () + combined = f"{reason}\n{summary}" + return tuple( + sorted( + path + for path in changed_files + if not changed_file_named_in_text(combined, path) + ) + ) def mentions_verification_posture(reason: str, summary: str) -> bool: @@ -1289,6 +1359,12 @@ def reject(reason: str) -> None: return reject("approval admits missing structural review") if not mentions_actual_changed_file(reason, summary): return reject("approval does not cite changed-file evidence") + unnamed = unnamed_changed_files(reason, summary) + if unnamed: + return reject( + "approval does not name every current-head changed file: " + + ", ".join(unnamed) + ) if not mentions_verification_posture(reason, summary): return reject("approval does not include the required verification posture") if not mentions_full_coverage(reason, summary): @@ -1311,6 +1387,12 @@ def reject(reason: str) -> None: return reject("review prose does not follow the preferred PR language") if not mentions_actual_changed_file(reason, summary): return reject("approval does not cite changed-file evidence") + unnamed = unnamed_changed_files(reason, summary) + if unnamed: + return reject( + "approval does not name every current-head changed file: " + + ", ".join(unnamed) + ) if not mentions_verification_posture(reason, summary): return reject("approval does not include the required verification posture") if not mentions_full_coverage(reason, summary): diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index 590fb3e53..acc1bcd95 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -856,6 +856,190 @@ def test_changed_file_and_verification_posture_detection(): ) +def test_approval_must_name_every_current_head_changed_file(tmp_path, monkeypatch): + """A CodeRabbit-thin approval that names one of two files cannot pass.""" + + changed_files = tmp_path / "opencode-changed-files.txt" + changed_files.write_text( + "scripts/ci/example.py\n.github/workflows/strix.yml\n", + encoding="utf-8", + ) + monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + seal_artifacts(tmp_path, changed_files) + norm.current_changed_files.cache_clear() + + assert norm.unnamed_changed_files( + "Reviewed scripts/ci/example.py.", + FULL_SUMMARY, + ) == (".github/workflows/strix.yml",) + assert ( + norm.unnamed_changed_files( + "Reviewed scripts/ci/example.py and .github/workflows/strix.yml.", + FULL_SUMMARY, + ) + == () + ) + assert ( + norm.unnamed_changed_files( + "Reviewed scripts/ci/example.py.bak only.", + "Coverage: 100%.", + ) + == (".github/workflows/strix.yml", "scripts/ci/example.py") + ) + assert norm.changed_file_named_in_text( + "Reviewed scripts/ci/example.py:7.", "scripts/ci/example.py" + ) + assert not norm.changed_file_named_in_text( + "Reviewed scripts/ci/example.py.bak.", "scripts/ci/example.py" + ) + assert not norm.changed_file_named_in_text( + "Reviewed docs/한.md추가.", "docs/한.md" + ) + assert not norm.changed_file_named_in_text( + "Reviewed 추가docs/한.md.", "docs/한.md" + ) + assert not norm.changed_file_named_in_text( + "Reviewed docs/한.md.추가.", "docs/한.md" + ) + assert norm.changed_file_named_in_text("Reviewed docs/한.md.", "docs/한.md") + assert not norm.changed_file_named_in_text("", "scripts/ci/example.py") + assert not norm.changed_file_named_in_text("scripts/ci/example.py", "") + assert norm._path_token_char("한") + assert norm._path_token_char("_") + assert not norm._path_token_char(" ") + assert norm._path_token_continues("scripts/ci/example.py.bak", 22) + assert norm._path_token_continues("docs/한.md추가", 9) + assert norm._path_token_continues("docs/한.md.추가", 9) + assert norm._path_token_continues("file.py..bak", 8) + assert not norm._path_token_continues("scripts/ci/example.py.", 22) + assert not norm._path_token_continues("docs/한.md. Next", 9) + assert not norm._path_token_continues("x", -1) + assert not norm._path_token_continues("x", 1) + + reasons: list[str] = [] + assert ( + norm.valid_control( + control(reason="Reviewed scripts/ci/example.py only."), + expected_head_sha="head", + expected_run_id="run", + expected_run_attempt="attempt", + rejection_reasons=reasons, + ) + is None + ) + assert any( + "does not name every current-head changed file" in reason + and ".github/workflows/strix.yml" in reason + for reason in reasons + ) + + reasons.clear() + accepted = norm.valid_control( + control( + reason=( + "Reviewed scripts/ci/example.py and .github/workflows/strix.yml " + "on the current head." + ) + ), + expected_head_sha="head", + expected_run_id="run", + expected_run_attempt="attempt", + rejection_reasons=reasons, + ) + assert accepted is not None + assert accepted["result"] == "APPROVE" + + monkeypatch.delenv("OPENCODE_CHANGED_FILES_FILE", raising=False) + norm.current_changed_files.cache_clear() + assert norm.unnamed_changed_files("any reason", "any summary") == () + + monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + seal_artifacts(tmp_path, changed_files) + norm.current_changed_files.cache_clear() + monkeypatch.setattr(norm, "repair_approval_summary", lambda reason, summary: summary) + monkeypatch.setattr( + norm, + "repair_approval_reason", + lambda reason, summary: reason.replace(".github/workflows/strix.yml", ""), + ) + reasons.clear() + assert ( + norm.valid_control( + control( + reason=( + "Reviewed scripts/ci/example.py and .github/workflows/strix.yml " + "on the current head." + ) + ), + expected_head_sha="head", + expected_run_id="run", + expected_run_attempt="attempt", + rejection_reasons=reasons, + ) + is None + ) + assert any( + "does not name every current-head changed file" in reason + for reason in reasons + ) + + +def test_valid_control_rejects_kind_contradiction_after_naming_every_file( + tmp_path, monkeypatch +): + """Naming every path still fails when the approval denies source-file kinds.""" + + changed_files = tmp_path / "opencode-changed-files.txt" + changed_files.write_text("scripts/ci/example.py\n", encoding="utf-8") + monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + seal_artifacts(tmp_path, changed_files) + norm.current_changed_files.cache_clear() + + reasons: list[str] = [] + assert ( + norm.valid_control( + control( + reason="Reviewed scripts/ci/example.py with no source changes.", + summary=FULL_SUMMARY, + ), + expected_head_sha="head", + expected_run_id="run", + expected_run_attempt="attempt", + rejection_reasons=reasons, + ) + is None + ) + assert any("contradicts changed file kinds" in reason for reason in reasons) + + +def test_valid_control_rejects_material_trivialization_after_naming_every_file( + tmp_path, monkeypatch +): + """Naming every path still fails when the approval calls the change a string typo.""" + + changed_files = tmp_path / "opencode-changed-files.txt" + changed_files.write_text("scripts/ci/example.py\n", encoding="utf-8") + monkeypatch.setenv("OPENCODE_CHANGED_FILES_FILE", str(changed_files)) + seal_artifacts(tmp_path, changed_files) + norm.current_changed_files.cache_clear() + + reasons: list[str] = [] + assert ( + norm.valid_control( + control( + reason="Reviewed scripts/ci/example.py; no tests are needed.", + summary=FULL_SUMMARY, + ), + expected_head_sha="head", + expected_run_id="run", + expected_run_attempt="attempt", + rejection_reasons=reasons, + ) + is None + ) + assert any("trivializes material changed files" in reason for reason in reasons) + + def test_actual_changed_file_detection_prefers_current_head_file_list( tmp_path, monkeypatch ): @@ -1819,8 +2003,8 @@ def test_valid_control_accepts_model_confirms_with_bounded_current_head_receipt( candidate = control( reason=( - "src/main/java/example/LogSanitizer.java hardens log input and adds " - "a regression test." + "src/main/java/example/LogSanitizer.java hardens log input and " + "src/test/java/example/LogSanitizerTest.java adds a regression test." ), summary=FULL_SUMMARY.replace( "scripts/ci/example.py", @@ -1959,7 +2143,9 @@ def test_valid_control_rejects_fragile_approval_reason_before_evidence_repair( repaired = norm.valid_control( control( - reason="Dependency version bump with no source changes", + reason=( + "Dependency version bump in .github/workflows/r.yml with no source changes" + ), summary=( "Approval sufficiency: Dependency version bump with no source changes. " "Verification posture: No verification needed for workflow-only updates. " @@ -2157,7 +2343,10 @@ def test_valid_control_rejects_material_trivialization(tmp_path, monkeypatch): repaired = norm.valid_control( control( - reason="Current-head evidence was reviewed.", + reason=( + "Current-head .github/workflows/strix.yml and " + "scripts/ci/test_strix_quick_gate.sh were reviewed." + ), summary=( "The change is a simple typo fix in a string with no functional impact. " "No tests are needed for a string change."