From f3c8ff13b71348d711044db47e68f428d4c791f4 Mon Sep 17 00:00:00 2001 From: seonghobae Date: Sun, 6 Sep 2026 13:40:14 +0000 Subject: [PATCH 1/4] test(docs): contract the fenced-block shapes a split conflict resolution leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLAUDE.md records that no test parses fenced code blocks and that `ARCHITECTURE.md` — five mermaid diagrams drawing the review, hourly repair, SBOM attestation and merge trust boundaries — is read by no test at all, so a merge or autofix conflict resolution that splits one block into two fragments ships green while the diagram renders to readers as a plain code listing. It also records why the obvious check does not work: a split leaves four fence lines where there were two, so the count stays even and every block still balances. `tests/test_markdown_fenced_block_integrity.py` keys on the shapes a split produces, across all 143 tracked Markdown files: - a block that closes and another that opens with nothing but blank lines between them (the seam) — position-independent, the primary detector; - a file that ends inside an unclosed block; - an untagged block whose body reads as mermaid: the orphaned half, which loses the tag because only the first fragment keeps the original opening line; - a ```mermaid block not opening on a mermaid diagram keyword: the orphan when the tag is what got duplicated. `ARCHITECTURE.md` is additionally pinned at five tagged mermaid blocks and no other fenced content. Four negative controls split a real diagram the way a resolution would and assert the matching detector fires. The seam and orphan controls run at every split point rather than one convenient offset — which is how the first draft of the orphan detector was caught recognising only a fragment beginning on the diagram's keyword line, and was widened to mermaid edges, `subgraph`, and quoted node declarations. The one split point the content-shape detectors cannot cover is the last body line, whose second fragment is empty; the seam control covers it and the docstring says so rather than implying otherwise. Zero findings on current `main`: this pins today's state, it does not report a backlog. The one untagged fenced block the first heuristic flagged (`docs/product-technical-gap-baseline.md:2698`) is a legitimate plain-text output block, which is why an untagged block is reported only when its body reads as mermaid. Verification: full suite 2965 passed, 1 skipped, 21 subtests passed; `interrogate` PASSED at 100.0%; `git diff --check` clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- CHANGELOG.md | 4 + tests/test_markdown_fenced_block_integrity.py | 324 ++++++++++++++++++ 2 files changed, 328 insertions(+) create mode 100644 tests/test_markdown_fenced_block_integrity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index bf192f6a9e..7962140b63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### Markdown fenced blocks get an executable contract, so a split diagram cannot ship green + +- No test parsed fenced code blocks, and `ARCHITECTURE.md` — the five mermaid diagrams that draw the review, hourly repair, SBOM attestation and merge trust boundaries — was read by no test at all, so a merge or autofix conflict resolution that split one block into two fragments passed every gate while rendering the diagram source to readers as a plain code listing. Counting fences cannot detect that: a split leaves four fence lines where there were two, so the count stays even and every block still balances. `tests/test_markdown_fenced_block_integrity.py` keys on the shapes a split actually produces across all 143 tracked Markdown files instead — a block that closes and reopens with nothing but blank lines between it (the seam), a file that ends inside an open block, an untagged block whose body reads as mermaid (the orphaned half, which loses the tag because only the first fragment keeps the original opening line), and a ```mermaid block that does not open on a mermaid diagram keyword (the orphan when the tag is what got duplicated). `ARCHITECTURE.md` is additionally pinned at five tagged mermaid blocks and no other fenced content. Four negative controls split a real diagram the way a resolution would and assert the detectors fire; the seam control and the orphan control run at every split point rather than one convenient offset, which is how the first draft of the orphan detector was caught recognising only a fragment that began on the diagram's keyword line. The one split point the content-shape detectors cannot cover is the last body line, whose second fragment is empty; the seam control covers it and the docstring says so rather than implying full coverage. Zero findings on current `main`, so this pins today's state rather than reporting a backlog. + ### Failed-check finding names the Strix sandbox instead of the gateway - `opencode-review-dispatch.yml`'s `emit_strix_provider_failure_finding` rendered one fixed finding for every `STRIX_PROVIDER_UNAVAILABLE` line, whose Root cause read "The contextual-orchestrator gateway or its discovered provider pool was unavailable for this run". `#1953` had just given the Strix sandbox bootstrap failure its own second verdict token (`STRIX_SANDBOX_UNAVAILABLE`) precisely because that attribution is wrong for it -- the sandbox container never reaches its Caido proxy, so the run dies before the gateway serves anything -- and this consumer re-applied the wrong attribution one step downstream, into the review findings and the failure census. The emitter now branches on the second token: a sandbox verdict gets a finding that names Strix's sandbox, says the verdict does not name the gateway, and tells the reader not to change gateway or provider configuration on its strength. A `STRIX_PROVIDER_UNAVAILABLE` line without the token keeps its existing text verbatim, so the gateway class has no regression surface. No test covered this finding text at all before (`gateway or its discovered provider pool` matched nothing under `tests/`); `tests/test_opencode_dispatch_strix_sandbox_finding.py` now runs the production emitter from the published run block and pins both directions plus the no-signal case. Refs #1953, #1935. diff --git a/tests/test_markdown_fenced_block_integrity.py b/tests/test_markdown_fenced_block_integrity.py new file mode 100644 index 0000000000..27ba805252 --- /dev/null +++ b/tests/test_markdown_fenced_block_integrity.py @@ -0,0 +1,324 @@ +"""Fenced code blocks in this repository's Markdown must survive conflict resolution. + +No existing test parses fenced blocks. `ARCHITECTURE.md` — five mermaid diagrams +that are the control-plane drawing for review, hourly repair, SBOM attestation and +merge trust boundaries — is read by no test at all. So a merge or autofix conflict +resolution that splits one fenced block into two fragments ships green, and the +diagram source renders to readers as a plain code block. + +Counting fences cannot catch that: a split leaves four fence lines where there were +two, so the count stays even and every block still "balances". These contracts key +on the shapes a split actually produces instead: + +* a closing fence immediately followed by another fence, with nothing but blank + lines between them (the seam where one block became two); +* a file that ends inside an unclosed block; +* an untagged block whose body reads as mermaid — the orphaned second half of a + split ```mermaid block, which loses the tag because only the first fragment + keeps the original opening line; +* a ```mermaid block whose body does not begin with a mermaid diagram keyword — + the orphaned second half when the *tag* is what got duplicated. + +The four negative controls at the end of this module prove the detectors fire: +each one splits a real `ARCHITECTURE.md` diagram the way a conflict resolution +would and asserts the matching helper reports it. +""" + +from __future__ import annotations + +import re +import subprocess +from pathlib import Path + +# Every mermaid diagram type used in this repository plus the rest of the +# documented set, so a new diagram kind is not reported as a split fragment. +MERMAID_KEYWORDS = ( + "graph", + "flowchart", + "sequenceDiagram", + "classDiagram", + "stateDiagram", + "stateDiagram-v2", + "erDiagram", + "journey", + "gantt", + "pie", + "quadrantChart", + "requirementDiagram", + "gitGraph", + "mindmap", + "timeline", + "sankey-beta", + "block-beta", + "architecture-beta", + "C4Context", + "C4Container", + "C4Component", + "C4Dynamic", + "C4Deployment", + "%%{", # an init directive may legitimately precede the diagram keyword +) + +ARCHITECTURE = Path("ARCHITECTURE.md") +ARCHITECTURE_MERMAID_DIAGRAMS = 5 + + +def _tracked_markdown_files() -> list[Path]: + """List the Markdown files git tracks, so untracked scratch files are ignored.""" + listed = subprocess.run( + ["git", "ls-files", "-z", "*.md"], + capture_output=True, + text=True, + check=True, + ).stdout + return [Path(name) for name in listed.split("\0") if name] + + +def _fence_lines(text: str) -> list[tuple[int, str]]: + """Return `(line number, info string)` for every fence line, in file order. + + A fence line is a line whose first non-space characters are three backticks. + The info string is whatever follows them (``"mermaid"``, ``"bash"``, or ``""`` + for an untagged fence). + """ + found: list[tuple[int, str]] = [] + for number, line in enumerate(text.split("\n"), 1): + stripped = line.strip() + if stripped.startswith("```"): + found.append((number, stripped[3:].strip())) + return found + + +def _blocks(text: str) -> list[tuple[int, str, list[str]]]: + """Return `(opening line number, info string, body lines)` for each fenced block. + + Fences alternate opening/closing, which is how Markdown itself reads them, so + the info string on a closing fence is reported by the caller rather than used + here. A trailing unclosed block is omitted; `test_every_file_closes_its_blocks` + is the contract that reports it. + """ + lines = text.split("\n") + fences = _fence_lines(text) + blocks: list[tuple[int, str, list[str]]] = [] + for opening, closing in zip(fences[0::2], fences[1::2]): + body = lines[opening[0] : closing[0] - 1] + blocks.append((opening[0], opening[1], body)) + return blocks + + +def _first_content_line(body: list[str]) -> str: + """Return the first non-blank line of a block body, or the empty string.""" + for line in body: + if line.strip(): + return line.strip() + return "" + + +# A mermaid node declaration with a quoted label — `Hub["This repo"]`, +# `Gate{"approved?"}`, `Run("strix")`. The quotes are what make this safe to +# match: the shell, JSON and log output this repository otherwise fences does +# not put a quoted string inside brackets immediately after a bare identifier. +MERMAID_NODE = re.compile(r'^\w[\w.-]*[\[({]"[^"]*"[\])}]$') + + +def _reads_as_mermaid(body: list[str]) -> bool: + """Report whether a block body looks like mermaid diagram source. + + Used to spot the orphaned half of a split ```mermaid block, which keeps the + diagram text but loses the language tag. A fragment can begin at any line of + the original diagram, so this matches the three shapes a mermaid line takes: + the opening diagram keyword, an edge, and a quoted node declaration. + """ + first = _first_content_line(body) + if first.startswith(MERMAID_KEYWORDS) or first.startswith("subgraph "): + return True + if any(edge in first for edge in ("-->", "-.->", "==>", "---|")): + return True + return bool(MERMAID_NODE.match(first)) + + +def _unclosed_files(paths: list[Path]) -> list[str]: + """Return a report line for every file that ends inside a fenced block.""" + return [ + f"{path}: file ends inside an unclosed fenced block" + for path in paths + if len(_fence_lines(path.read_text(encoding="utf-8"))) % 2 + ] + + +def _adjacent_fence_seams(text: str, path: Path) -> list[str]: + """Return a report line for each closing fence directly followed by a new fence. + + This is the seam a split leaves behind: the inserted closing fence and the + inserted opening fence end up next to each other, separated at most by blank + lines. Two genuinely separate blocks in this repository always have prose, + a heading or a list item between them. + """ + lines = text.split("\n") + fences = _fence_lines(text) + seams: list[str] = [] + for closing, following in zip(fences[1::2], fences[2::2]): + between = lines[closing[0] : following[0] - 1] + if all(not line.strip() for line in between): + seams.append( + f"{path}:{closing[0]}-{following[0]}: a block closes and another " + "opens with nothing between them, the shape a split block leaves" + ) + return seams + + +def test_every_file_closes_its_blocks() -> None: + """Every tracked Markdown file must end outside a fenced block.""" + assert _unclosed_files(_tracked_markdown_files()) == [] + + +def test_no_block_closes_and_reopens_with_nothing_between() -> None: + """No tracked Markdown file may carry the seam a split fenced block leaves.""" + seams: list[str] = [] + for path in _tracked_markdown_files(): + seams += _adjacent_fence_seams(path.read_text(encoding="utf-8"), path) + assert seams == [] + + +def test_no_untagged_block_reads_as_mermaid() -> None: + """An untagged block holding mermaid source is an orphaned diagram fragment.""" + orphans: list[str] = [] + for path in _tracked_markdown_files(): + for line_number, info, body in _blocks(path.read_text(encoding="utf-8")): + if not info and _reads_as_mermaid(body): + orphans.append( + f"{path}:{line_number}: untagged fenced block reads as mermaid " + f"source ({_first_content_line(body)!r})" + ) + assert orphans == [] + + +def test_every_mermaid_block_starts_with_a_diagram_keyword() -> None: + """A ```mermaid block must open with a mermaid diagram keyword, not a fragment.""" + bad: list[str] = [] + for path in _tracked_markdown_files(): + for line_number, info, body in _blocks(path.read_text(encoding="utf-8")): + if info != "mermaid": + continue + first = _first_content_line(body) + if not first.startswith(MERMAID_KEYWORDS): + bad.append( + f"{path}:{line_number}: mermaid block starts with {first!r}, " + "not a mermaid diagram keyword" + ) + assert bad == [] + + +def test_architecture_diagrams_are_all_present_and_tagged() -> None: + """`ARCHITECTURE.md`'s five control-plane diagrams stay five tagged mermaid blocks. + + This file is the one no other test reads, and a lost tag degrades a diagram to + a code listing without failing anything else. + """ + blocks = _blocks(ARCHITECTURE.read_text(encoding="utf-8")) + mermaid = [block for block in blocks if block[1] == "mermaid"] + assert len(mermaid) == ARCHITECTURE_MERMAID_DIAGRAMS + assert len(blocks) == ARCHITECTURE_MERMAID_DIAGRAMS + + +# --- negative controls ------------------------------------------------------- +# +# Each builds the exact damage a conflict resolution produces and asserts the +# matching helper reports it, so a detector that silently stopped matching fails +# here rather than passing the whole suite on an intact tree. + + +def _first_diagram_opening(text: str) -> int: + """Return the zero-based index of the first ```mermaid line.""" + lines = text.split("\n") + return next(index for index, line in enumerate(lines) if line.strip() == "```mermaid") + + +def _first_diagram_body_length(text: str) -> int: + """Return how many body lines the first ```mermaid block holds.""" + lines = text.split("\n") + opening = _first_diagram_opening(text) + closing = next( + index + for index, line in enumerate(lines) + if index > opening and line.strip() == "```" + ) + return closing - opening - 1 + + +def _split_first_diagram( + text: str, *, keep_tag: bool, after_body_line: int = 1 +) -> str: + """Split the first ```mermaid block in two, the way a bad resolution does. + + A closing fence and a new opening fence are inserted after `after_body_line` + body lines. `keep_tag` chooses which half keeps the ``mermaid`` info string: + ``False`` reproduces the common case where the second half becomes untagged, + ``True`` the case where the tag is duplicated onto a fragment. + """ + lines = text.split("\n") + seam = _first_diagram_opening(text) + 1 + after_body_line + reopened = "```mermaid" if keep_tag else "```" + return "\n".join(lines[:seam] + ["```", reopened] + lines[seam:]) + + +def test_split_block_is_reported_as_a_seam(tmp_path: Path) -> None: + """A split diagram must be reported by the closes-and-reopens contract. + + This is the position-independent detector: it holds at every split point, + including the last body line, where the orphaned fragment is empty and the + content-shape detectors have nothing to match. + """ + text = ARCHITECTURE.read_text(encoding="utf-8") + missed = [ + offset + for offset in range(1, _first_diagram_body_length(text) + 1) + if not _adjacent_fence_seams( + _split_first_diagram(text, keep_tag=False, after_body_line=offset), + tmp_path / "ARCHITECTURE.md", + ) + ] + assert missed == [] + + +def test_split_block_orphan_is_reported_as_untagged_mermaid() -> None: + """The untagged half of a split diagram must read as an orphaned fragment. + + Checked at every split point that leaves two non-empty fragments, not one + convenient offset: a real resolution splits wherever the conflict landed, and + the first draft of this detector only recognised a fragment that began on the + diagram's keyword line. The one excluded split point is the last body line, + whose second fragment is empty and so cannot read as anything; + `test_split_block_is_reported_as_a_seam` is what covers that case, and does. + """ + text = ARCHITECTURE.read_text(encoding="utf-8") + missed = [] + for offset in range(1, _first_diagram_body_length(text)): + damaged = _split_first_diagram(text, keep_tag=False, after_body_line=offset) + if not [ + body + for _, info, body in _blocks(damaged) + if not info and _reads_as_mermaid(body) + ]: + missed.append(offset) + assert missed == [] + + +def test_split_block_keeping_the_tag_is_reported_as_a_fragment() -> None: + """A re-tagged fragment must fail the diagram-keyword contract.""" + damaged = _split_first_diagram( + ARCHITECTURE.read_text(encoding="utf-8"), keep_tag=True + ) + fragments = [ + _first_content_line(body) + for _, info, body in _blocks(damaged) + if info == "mermaid" and not _first_content_line(body).startswith(MERMAID_KEYWORDS) + ] + assert fragments != [] + + +def test_unclosed_block_is_reported(tmp_path: Path) -> None: + """A file left inside an open block must be reported by the balance contract.""" + damaged = tmp_path / "unclosed.md" + damaged.write_text("# Title\n\n```mermaid\ngraph TD\n A --> B\n", encoding="utf-8") + assert _unclosed_files([damaged]) != [] From 1db0804ff7506be3b1d901964c743ece2ef6ba94 Mon Sep 17 00:00:00 2001 From: seonghobae Date: Sun, 6 Sep 2026 13:58:49 +0000 Subject: [PATCH 2/4] =?UTF-8?q?fix(test):=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20real=20fence=20parsing,=20mermaid=20comments,=20CI?= =?UTF-8?q?=20wiring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from Codex review of f3c8ff13, all verified against the sources before fixing. P1 — the contract never ran on the change it exists for. Every suite-running workflow here is path-filtered to Python, Rust, R or its own scripts, and `opencode-review-dispatch.yml` reaches `run_python_test_coverage` only under `has_changed_tracked_files '*.py'` (line 2121). A PR that only split a mermaid diagram in ARCHITECTURE.md would have shipped green; the introducing commit masked this by adding a .py file. Adds `.github/workflows/markdown-fenced-block-quality-ci.yml`, running the contract by name and then the complete suite on any `**.md` change. The glob is the documented `**.md`, not `**/*.md`, which is ambiguous about root-level files — and ARCHITECTURE.md is one. `test_a_markdown_only_change_runs_this_contract` pins both triggers and the run step; verified RED by removing the glob and, separately, the invocation. The workflow is not org-required and not in ruleset 18156473, so its path filters are the ordinary quality-CI pattern, not the required-workflow trap in docs/doctoring/required-workflow-path-filter-boundary.md. P2 — an even fence count is not a balance check. Per CommonMark a closing fence carries no info string, so ```bash … ```python … EOF is ONE unclosed block whose second tagged line is content; the parity check read it as two balanced fences and `_blocks` mis-paired them, leaving the remaining contracts green while the rest of the document renders inside the open block. `_parse_blocks` is now a state machine over opener length and info string. Controls: `test_a_tagged_fence_does_not_close_an_open_block` and `test_a_shorter_untagged_fence_does_not_close_a_longer_block`. P2 — mermaid comments. Mermaid ignores every `%%` line, both an ordinary `%% explanation` comment and a `%%{init: …}%%` directive, and renders the declaration that follows. Reading only the first non-blank line rejected an ordinary annotated diagram repo-wide. `_declaration_line` reads past `%%` lines; `%%{` leaves MERMAID_KEYWORDS since it is no longer a special case. Control: `test_a_commented_mermaid_block_declares_its_diagram`, covering both forms. Verification: full suite 2969 passed, 1 skipped, 21 subtests passed; `interrogate` PASSED at 100.0%; `python -m compileall -q tests`; `git diff --check` clean; workflow parses under `yaml.safe_load`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- .../markdown-fenced-block-quality-ci.yml | 94 ++++++ CHANGELOG.md | 6 +- tests/test_markdown_fenced_block_integrity.py | 277 ++++++++++++------ 3 files changed, 283 insertions(+), 94 deletions(-) create mode 100644 .github/workflows/markdown-fenced-block-quality-ci.yml diff --git a/.github/workflows/markdown-fenced-block-quality-ci.yml b/.github/workflows/markdown-fenced-block-quality-ci.yml new file mode 100644 index 0000000000..38d7b8e447 --- /dev/null +++ b/.github/workflows/markdown-fenced-block-quality-ci.yml @@ -0,0 +1,94 @@ +name: Markdown Fenced Block Quality CI + +# The fenced-block contract exists for docs-only damage -- a conflict resolution +# that splits a mermaid diagram in ARCHITECTURE.md -- but every other suite-running +# workflow in this repository is path-filtered to Python, Rust, R or its own +# scripts, and `opencode-review-dispatch.yml` runs the Python suite only when +# `has_changed_tracked_files '*.py'` is true. Without this workflow the contract +# would never be collected on the exact change it was written to catch. + +on: + pull_request: + branches: [main] + paths: + - "**.md" + - ".github/workflows/markdown-fenced-block-quality-ci.yml" + - "tests/test_markdown_fenced_block_integrity.py" + push: + branches: [main] + paths: + - "**.md" + - ".github/workflows/markdown-fenced-block-quality-ci.yml" + - "tests/test_markdown_fenced_block_integrity.py" + +concurrency: + group: markdown-fenced-block-quality-${{ github.repository }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +env: + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + +jobs: + quality: + runs-on: ubuntu-24.04 + timeout-minutes: 20 + steps: + - name: Harden runner + uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 + with: + egress-policy: audit + - name: Checkout exact head with comparison history + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.head.sha || github.sha }} + fetch-depth: 0 + persist-credentials: false + - name: Determine exact changed range + env: + PR_BASE_SHA: ${{ github.event.pull_request.base.sha || '' }} + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha || '' }} + PUSH_BEFORE_SHA: ${{ github.event.before || '' }} + PUSH_HEAD_SHA: ${{ github.sha }} + shell: bash --noprofile --norc -e -o pipefail {0} + run: | + if [ "$GITHUB_EVENT_NAME" = "pull_request" ]; then + base_sha="$PR_BASE_SHA" + head_sha="$PR_HEAD_SHA" + diff_range="${base_sha}...${head_sha}" + else + base_sha="$PUSH_BEFORE_SHA" + head_sha="$PUSH_HEAD_SHA" + if [[ "$base_sha" =~ ^0+$ ]]; then + base_sha="$(git rev-parse "${head_sha}^")" + fi + diff_range="${base_sha}..${head_sha}" + fi + git cat-file -e "${base_sha}^{commit}" + git cat-file -e "${head_sha}^{commit}" + { + echo "CHANGE_BASE_SHA=$base_sha" + echo "CHANGE_HEAD_SHA=$head_sha" + echo "CHANGE_DIFF_RANGE=$diff_range" + } >>"$GITHUB_ENV" + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + cache: pip + cache-dependency-path: requirements-opencode-review-ci-hashes.txt + - name: Install exact hash-locked tooling + run: >- + python -m pip install --disable-pip-version-check --require-hashes + -r requirements-opencode-review-ci-hashes.txt + - name: Run the fenced-block contract and the complete repository suite + shell: bash --noprofile --norc -e -o pipefail {0} + run: | + # Named first so a split block reports as its own failing step rather + # than one line inside a 2900-test run. + python -m pytest -q tests/test_markdown_fenced_block_integrity.py + python -m pytest -q + python -m compileall -q tests + git diff --check "$CHANGE_DIFF_RANGE" diff --git a/CHANGELOG.md b/CHANGELOG.md index 7962140b63..1b45f67e2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ ### Markdown fenced blocks get an executable contract, so a split diagram cannot ship green -- No test parsed fenced code blocks, and `ARCHITECTURE.md` — the five mermaid diagrams that draw the review, hourly repair, SBOM attestation and merge trust boundaries — was read by no test at all, so a merge or autofix conflict resolution that split one block into two fragments passed every gate while rendering the diagram source to readers as a plain code listing. Counting fences cannot detect that: a split leaves four fence lines where there were two, so the count stays even and every block still balances. `tests/test_markdown_fenced_block_integrity.py` keys on the shapes a split actually produces across all 143 tracked Markdown files instead — a block that closes and reopens with nothing but blank lines between it (the seam), a file that ends inside an open block, an untagged block whose body reads as mermaid (the orphaned half, which loses the tag because only the first fragment keeps the original opening line), and a ```mermaid block that does not open on a mermaid diagram keyword (the orphan when the tag is what got duplicated). `ARCHITECTURE.md` is additionally pinned at five tagged mermaid blocks and no other fenced content. Four negative controls split a real diagram the way a resolution would and assert the detectors fire; the seam control and the orphan control run at every split point rather than one convenient offset, which is how the first draft of the orphan detector was caught recognising only a fragment that began on the diagram's keyword line. The one split point the content-shape detectors cannot cover is the last body line, whose second fragment is empty; the seam control covers it and the docstring says so rather than implying full coverage. Zero findings on current `main`, so this pins today's state rather than reporting a backlog. +- No test parsed fenced code blocks, and `ARCHITECTURE.md` — the five mermaid diagrams that draw the review, hourly repair, SBOM attestation and merge trust boundaries — was read by no test at all, so a merge or autofix conflict resolution that split one block into two fragments passed every gate while rendering the diagram source to readers as a plain code listing. Counting fences cannot detect that: a split leaves four fence lines where there were two, so the count stays even and every block still balances. `tests/test_markdown_fenced_block_integrity.py` keys on the shapes a split actually produces across all 143 tracked Markdown files instead — a block that closes and reopens with nothing but blank lines between it (the seam), a file that ends inside an open block, an untagged block whose body reads as mermaid (the orphaned half, which loses the tag because only the first fragment keeps the original opening line), and a ```mermaid block that does not open on a mermaid diagram keyword (the orphan when the tag is what got duplicated). `ARCHITECTURE.md` is additionally pinned at five tagged mermaid blocks and no other fenced content. Four negative controls split a real diagram the way a resolution would and assert the detectors fire; the seam control and the orphan control run at every split point rather than one convenient offset, which is how the first draft of the orphan detector was caught recognising only a fragment that began on the diagram's keyword line. The one split point the content-shape detectors cannot cover is the last body line, whose second fragment is empty; the seam control covers it and the docstring says so rather than implying full coverage. Zero findings on current `main`, so this pins today's state rather than reporting a backlog. Two corrections from Codex review on the first head: the parser no longer counts fences at all -- per CommonMark a closing fence carries no info string, so ```` ```bash ```` ... ```` ```python ```` ... EOF is *one unclosed block* whose second tagged line is content, while a parity count reads it as two balanced fences and lets the rest of the document render inside the open block; `_parse_blocks` is a state machine over opener length and info string, with controls for the mixed-tag case and for a three-backtick line inside a four-backtick block. And a mermaid block is now read past its `%%` lines before its declaration is checked, because mermaid ignores both an ordinary `%% explanation` comment and a `%%{init: ...}%%` directive and renders what follows -- reading only the first non-blank line rejected an ordinary annotated diagram. + +### Markdown changes run the fenced-block contract + +- `.github/workflows/markdown-fenced-block-quality-ci.yml` (new) runs the fenced-block contract and then the complete suite on any `**.md` change. Without it the contract was dead on the change it exists for: every other suite-running workflow here is path-filtered to Python, Rust, R or its own scripts (`trusted-uv-materializer-quality-ci.yml`, `agent-review-runtime-quality-ci.yml`, `agent-mention-router-quality-ci.yml`), and `opencode-review-dispatch.yml` reaches `run_python_test_coverage` only under `has_changed_tracked_files '*.py'` (line 2121), so a PR that only split a mermaid diagram in `ARCHITECTURE.md` would have shipped green -- the introducing commit masked the gap by adding a `.py` file. Found by Codex review, verified against the workflow sources before fixing. The glob is the documented `**.md` rather than `**/*.md`, which is ambiguous about root-level files and `ARCHITECTURE.md` is one. `test_a_markdown_only_change_runs_this_contract` pins both event triggers and the run step, and fails when either the glob or the explicit invocation is removed. This workflow is not org-required and is not in ruleset `18156473`, so its path filters are the ordinary quality-CI pattern rather than the required-workflow trap in `docs/doctoring/required-workflow-path-filter-boundary.md`. ### Failed-check finding names the Strix sandbox instead of the gateway diff --git a/tests/test_markdown_fenced_block_integrity.py b/tests/test_markdown_fenced_block_integrity.py index 27ba805252..8325d875f1 100644 --- a/tests/test_markdown_fenced_block_integrity.py +++ b/tests/test_markdown_fenced_block_integrity.py @@ -1,27 +1,31 @@ """Fenced code blocks in this repository's Markdown must survive conflict resolution. -No existing test parses fenced blocks. `ARCHITECTURE.md` — five mermaid diagrams -that are the control-plane drawing for review, hourly repair, SBOM attestation and -merge trust boundaries — is read by no test at all. So a merge or autofix conflict +No other test parses fenced blocks. `ARCHITECTURE.md` — five mermaid diagrams that +are the control-plane drawing for review, hourly repair, SBOM attestation and merge +trust boundaries — is read by no test at all. So a merge or autofix conflict resolution that splits one fenced block into two fragments ships green, and the diagram source renders to readers as a plain code block. Counting fences cannot catch that: a split leaves four fence lines where there were -two, so the count stays even and every block still "balances". These contracts key -on the shapes a split actually produces instead: +two, so the count stays even and every block still "balances". Worse, a parity count +is wrong in its own right — per CommonMark a closing fence carries no info string, +so ```` ```bash ```` … ```` ```python ```` … EOF is *one unclosed block* whose second +tagged line is content, while a parity count reads it as two balanced fences and +lets the rest of the document render inside the open block. `_parse_blocks` is +therefore a state machine over opener length and info string, not a counter. -* a closing fence immediately followed by another fence, with nothing but blank - lines between them (the seam where one block became two); +The contracts key on the shapes a split actually produces: + +* a block that closes and another that opens with nothing but blank lines between + them (the seam) — position-independent, the primary detector; * a file that ends inside an unclosed block; -* an untagged block whose body reads as mermaid — the orphaned second half of a - split ```mermaid block, which loses the tag because only the first fragment - keeps the original opening line; -* a ```mermaid block whose body does not begin with a mermaid diagram keyword — - the orphaned second half when the *tag* is what got duplicated. - -The four negative controls at the end of this module prove the detectors fire: -each one splits a real `ARCHITECTURE.md` diagram the way a conflict resolution -would and asserts the matching helper reports it. +* an untagged block whose body reads as mermaid — the orphaned second half, which + loses the tag because only the first fragment keeps the original opening line; +* a ```mermaid block whose body does not begin with a mermaid diagram declaration — + the orphan when the *tag* is what got duplicated. + +The negative controls at the end prove each detector fires, at every split point +rather than one convenient offset. """ from __future__ import annotations @@ -30,6 +34,8 @@ import subprocess from pathlib import Path +import yaml + # Every mermaid diagram type used in this repository plus the rest of the # documented set, so a new diagram kind is not reported as a split fragment. MERMAID_KEYWORDS = ( @@ -56,11 +62,23 @@ "C4Component", "C4Dynamic", "C4Deployment", - "%%{", # an init directive may legitimately precede the diagram keyword ) +# A mermaid node declaration with a quoted label — `Hub["This repo"]`, +# `Gate{"approved?"}`, `Run("strix")`. The quotes are what make this safe to +# match: the shell, JSON and log output this repository otherwise fences does not +# put a quoted string inside brackets immediately after a bare identifier. +MERMAID_NODE = re.compile(r'^\w[\w.-]*[\[({]"[^"]*"[\])}]$') + +MERMAID_EDGES = ("-->", "-.->", "==>", "---|") + ARCHITECTURE = Path("ARCHITECTURE.md") ARCHITECTURE_MERMAID_DIAGRAMS = 5 +QUALITY_WORKFLOW = Path(".github/workflows/markdown-fenced-block-quality-ci.yml") +# GitHub documents `**.md` as "every Markdown file in the repository"; the +# `**/*.md` spelling is ambiguous about root-level files, and ARCHITECTURE.md -- +# the whole reason this contract exists -- is one. +MARKDOWN_GLOB = "**.md" def _tracked_markdown_files() -> list[Path]: @@ -74,102 +92,112 @@ def _tracked_markdown_files() -> list[Path]: return [Path(name) for name in listed.split("\0") if name] -def _fence_lines(text: str) -> list[tuple[int, str]]: - """Return `(line number, info string)` for every fence line, in file order. +def _fence(line: str) -> tuple[int, str] | None: + """Return `(backtick count, info string)` if this line is a fence, else `None`.""" + stripped = line.strip() + if not stripped.startswith("```"): + return None + ticks = len(stripped) - len(stripped.lstrip("`")) + return ticks, stripped[ticks:].strip() - A fence line is a line whose first non-space characters are three backticks. - The info string is whatever follows them (``"mermaid"``, ``"bash"``, or ``""`` - for an untagged fence). - """ - found: list[tuple[int, str]] = [] - for number, line in enumerate(text.split("\n"), 1): - stripped = line.strip() - if stripped.startswith("```"): - found.append((number, stripped[3:].strip())) - return found +def _parse_blocks(text: str) -> list[tuple[int, str, list[str], int | None]]: + """Return `(opening line, info string, body lines, closing line)` per block. -def _blocks(text: str) -> list[tuple[int, str, list[str]]]: - """Return `(opening line number, info string, body lines)` for each fenced block. - - Fences alternate opening/closing, which is how Markdown itself reads them, so - the info string on a closing fence is reported by the caller rather than used - here. A trailing unclosed block is omitted; `test_every_file_closes_its_blocks` - is the contract that reports it. + A state machine rather than a pairing of alternate fence lines, because + CommonMark closes a block only on a fence that carries **no** info string and + is at least as long as the opener. A tagged fence encountered while a block is + open is that block's content — the case a parity count silently mis-pairs. + `closing line` is `None` for a block the file never closes. """ lines = text.split("\n") - fences = _fence_lines(text) - blocks: list[tuple[int, str, list[str]]] = [] - for opening, closing in zip(fences[0::2], fences[1::2]): - body = lines[opening[0] : closing[0] - 1] - blocks.append((opening[0], opening[1], body)) + blocks: list[tuple[int, str, list[str], int | None]] = [] + opening: int | None = None + info = "" + ticks = 0 + for number, line in enumerate(lines, 1): + parsed = _fence(line) + if parsed is None: + continue + line_ticks, line_info = parsed + if opening is None: + opening, info, ticks = number, line_info, line_ticks + elif not line_info and line_ticks >= ticks: + blocks.append((opening, info, lines[opening : number - 1], number)) + opening = None + if opening is not None: + blocks.append((opening, info, lines[opening:], None)) return blocks -def _first_content_line(body: list[str]) -> str: - """Return the first non-blank line of a block body, or the empty string.""" +def _declaration_line(body: list[str]) -> str: + """Return a block body's first line that is neither blank nor a mermaid comment. + + Mermaid ignores any `%%`-prefixed line — both an ordinary `%% explanation` + comment and a `%%{init: …}%%` directive — and renders the declaration that + follows, so an annotated diagram must be read past them rather than rejected. + """ for line in body: - if line.strip(): - return line.strip() + stripped = line.strip() + if stripped and not stripped.startswith("%%"): + return stripped return "" -# A mermaid node declaration with a quoted label — `Hub["This repo"]`, -# `Gate{"approved?"}`, `Run("strix")`. The quotes are what make this safe to -# match: the shell, JSON and log output this repository otherwise fences does -# not put a quoted string inside brackets immediately after a bare identifier. -MERMAID_NODE = re.compile(r'^\w[\w.-]*[\[({]"[^"]*"[\])}]$') - - def _reads_as_mermaid(body: list[str]) -> bool: """Report whether a block body looks like mermaid diagram source. Used to spot the orphaned half of a split ```mermaid block, which keeps the diagram text but loses the language tag. A fragment can begin at any line of the original diagram, so this matches the three shapes a mermaid line takes: - the opening diagram keyword, an edge, and a quoted node declaration. + the opening declaration, an edge, and a quoted node declaration. """ - first = _first_content_line(body) + first = _declaration_line(body) if first.startswith(MERMAID_KEYWORDS) or first.startswith("subgraph "): return True - if any(edge in first for edge in ("-->", "-.->", "==>", "---|")): + if any(edge in first for edge in MERMAID_EDGES): return True return bool(MERMAID_NODE.match(first)) -def _unclosed_files(paths: list[Path]) -> list[str]: - """Return a report line for every file that ends inside a fenced block.""" +def _unclosed_blocks(path: Path, text: str) -> list[str]: + """Return a report line for every block in this file the text never closes.""" return [ - f"{path}: file ends inside an unclosed fenced block" - for path in paths - if len(_fence_lines(path.read_text(encoding="utf-8"))) % 2 + f"{path}:{opening}: fenced block opened here is never closed by an " + "untagged fence of the same length" + for opening, _, _, closing in _parse_blocks(text) + if closing is None ] def _adjacent_fence_seams(text: str, path: Path) -> list[str]: - """Return a report line for each closing fence directly followed by a new fence. + """Return a report line for each block that closes where the next one opens. This is the seam a split leaves behind: the inserted closing fence and the inserted opening fence end up next to each other, separated at most by blank - lines. Two genuinely separate blocks in this repository always have prose, - a heading or a list item between them. + lines. Two genuinely separate blocks in this repository always have prose, a + heading or a list item between them. """ lines = text.split("\n") - fences = _fence_lines(text) + blocks = _parse_blocks(text) seams: list[str] = [] - for closing, following in zip(fences[1::2], fences[2::2]): - between = lines[closing[0] : following[0] - 1] - if all(not line.strip() for line in between): + for (_, _, _, closing), (opening, _, _, _) in zip(blocks, blocks[1:]): + if closing is None: + continue + if all(not line.strip() for line in lines[closing : opening - 1]): seams.append( - f"{path}:{closing[0]}-{following[0]}: a block closes and another " - "opens with nothing between them, the shape a split block leaves" + f"{path}:{closing}-{opening}: a block closes and another opens " + "with nothing between them, the shape a split block leaves" ) return seams def test_every_file_closes_its_blocks() -> None: """Every tracked Markdown file must end outside a fenced block.""" - assert _unclosed_files(_tracked_markdown_files()) == [] + unclosed: list[str] = [] + for path in _tracked_markdown_files(): + unclosed += _unclosed_blocks(path, path.read_text(encoding="utf-8")) + assert unclosed == [] def test_no_block_closes_and_reopens_with_nothing_between() -> None: @@ -184,27 +212,27 @@ def test_no_untagged_block_reads_as_mermaid() -> None: """An untagged block holding mermaid source is an orphaned diagram fragment.""" orphans: list[str] = [] for path in _tracked_markdown_files(): - for line_number, info, body in _blocks(path.read_text(encoding="utf-8")): + for opening, info, body, _ in _parse_blocks(path.read_text(encoding="utf-8")): if not info and _reads_as_mermaid(body): orphans.append( - f"{path}:{line_number}: untagged fenced block reads as mermaid " - f"source ({_first_content_line(body)!r})" + f"{path}:{opening}: untagged fenced block reads as mermaid " + f"source ({_declaration_line(body)!r})" ) assert orphans == [] -def test_every_mermaid_block_starts_with_a_diagram_keyword() -> None: - """A ```mermaid block must open with a mermaid diagram keyword, not a fragment.""" +def test_every_mermaid_block_starts_with_a_diagram_declaration() -> None: + """A ```mermaid block must declare a diagram type, not open on a fragment.""" bad: list[str] = [] for path in _tracked_markdown_files(): - for line_number, info, body in _blocks(path.read_text(encoding="utf-8")): + for opening, info, body, _ in _parse_blocks(path.read_text(encoding="utf-8")): if info != "mermaid": continue - first = _first_content_line(body) + first = _declaration_line(body) if not first.startswith(MERMAID_KEYWORDS): bad.append( - f"{path}:{line_number}: mermaid block starts with {first!r}, " - "not a mermaid diagram keyword" + f"{path}:{opening}: mermaid block declares {first!r}, " + "not a mermaid diagram type" ) assert bad == [] @@ -215,7 +243,7 @@ def test_architecture_diagrams_are_all_present_and_tagged() -> None: This file is the one no other test reads, and a lost tag degrades a diagram to a code listing without failing anything else. """ - blocks = _blocks(ARCHITECTURE.read_text(encoding="utf-8")) + blocks = _parse_blocks(ARCHITECTURE.read_text(encoding="utf-8")) mermaid = [block for block in blocks if block[1] == "mermaid"] assert len(mermaid) == ARCHITECTURE_MERMAID_DIAGRAMS assert len(blocks) == ARCHITECTURE_MERMAID_DIAGRAMS @@ -246,9 +274,7 @@ def _first_diagram_body_length(text: str) -> int: return closing - opening - 1 -def _split_first_diagram( - text: str, *, keep_tag: bool, after_body_line: int = 1 -) -> str: +def _split_first_diagram(text: str, *, keep_tag: bool, after_body_line: int = 1) -> str: """Split the first ```mermaid block in two, the way a bad resolution does. A closing fence and a new opening fence are inserted after `after_body_line` @@ -287,8 +313,8 @@ def test_split_block_orphan_is_reported_as_untagged_mermaid() -> None: Checked at every split point that leaves two non-empty fragments, not one convenient offset: a real resolution splits wherever the conflict landed, and the first draft of this detector only recognised a fragment that began on the - diagram's keyword line. The one excluded split point is the last body line, - whose second fragment is empty and so cannot read as anything; + diagram's declaration line. The one excluded split point is the last body + line, whose second fragment is empty and so cannot read as anything; `test_split_block_is_reported_as_a_seam` is what covers that case, and does. """ text = ARCHITECTURE.read_text(encoding="utf-8") @@ -297,7 +323,7 @@ def test_split_block_orphan_is_reported_as_untagged_mermaid() -> None: damaged = _split_first_diagram(text, keep_tag=False, after_body_line=offset) if not [ body - for _, info, body in _blocks(damaged) + for _, info, body, _ in _parse_blocks(damaged) if not info and _reads_as_mermaid(body) ]: missed.append(offset) @@ -305,20 +331,85 @@ def test_split_block_orphan_is_reported_as_untagged_mermaid() -> None: def test_split_block_keeping_the_tag_is_reported_as_a_fragment() -> None: - """A re-tagged fragment must fail the diagram-keyword contract.""" + """A re-tagged fragment must fail the diagram-declaration contract.""" damaged = _split_first_diagram( ARCHITECTURE.read_text(encoding="utf-8"), keep_tag=True ) fragments = [ - _first_content_line(body) - for _, info, body in _blocks(damaged) - if info == "mermaid" and not _first_content_line(body).startswith(MERMAID_KEYWORDS) + _declaration_line(body) + for _, info, body, _ in _parse_blocks(damaged) + if info == "mermaid" and not _declaration_line(body).startswith(MERMAID_KEYWORDS) ] assert fragments != [] def test_unclosed_block_is_reported(tmp_path: Path) -> None: """A file left inside an open block must be reported by the balance contract.""" - damaged = tmp_path / "unclosed.md" - damaged.write_text("# Title\n\n```mermaid\ngraph TD\n A --> B\n", encoding="utf-8") - assert _unclosed_files([damaged]) != [] + text = "# Title\n\n```mermaid\ngraph TD\n A --> B\n" + assert _unclosed_blocks(tmp_path / "unclosed.md", text) != [] + + +def test_a_tagged_fence_does_not_close_an_open_block(tmp_path: Path) -> None: + """A second tagged fence is block content, so the first block stays unclosed. + + CommonMark closes a fenced block only on a fence with no info string, so + ```` ```bash ```` … ```` ```python ```` … EOF is one unclosed block whose + second tagged line is content. An even-fence-count check reads it as two + balanced blocks and reports nothing, which is why `_parse_blocks` tracks + opener state instead of pairing alternate fence lines. + """ + text = "```bash\necho one\n```python\nprint('two')\n" + blocks = _parse_blocks(text) + assert len(blocks) == 1 + assert blocks[0][1] == "bash" + assert blocks[0][3] is None + assert _unclosed_blocks(tmp_path / "mixed.md", text) != [] + + +def test_a_shorter_untagged_fence_does_not_close_a_longer_block() -> None: + """A three-backtick line inside a four-backtick block is content, not a close.""" + text = "````markdown\n```\nnested sample\n```\n````\n" + blocks = _parse_blocks(text) + assert len(blocks) == 1 + assert blocks[0][1] == "markdown" + assert blocks[0][3] == 5 + + +def test_a_commented_mermaid_block_declares_its_diagram() -> None: + """A `%%` comment or init directive before the declaration must not fail the check. + + Mermaid ignores `%%` lines and renders what follows, so reading only the first + non-blank line rejects an ordinary annotated diagram. Both comment forms are + checked because the init directive shares the prefix. + """ + commented = ["%% why this diagram exists", "", "flowchart LR", ' A["a"] --> B'] + directive = ["%%{init: {'theme': 'dark'}}%%", "sequenceDiagram", " A->>B: hi"] + assert _declaration_line(commented) == "flowchart LR" + assert _declaration_line(directive) == "sequenceDiagram" + assert _declaration_line(commented).startswith(MERMAID_KEYWORDS) + assert _declaration_line(directive).startswith(MERMAID_KEYWORDS) + assert _reads_as_mermaid(commented) + assert _reads_as_mermaid(directive) + + +def test_a_markdown_only_change_runs_this_contract() -> None: + """Markdown changes must trigger a workflow that executes this file. + + Without this the contract is dead on the change it was written for: every + other suite-running workflow here is path-filtered to Python, Rust, R or its + own scripts, and `opencode-review-dispatch.yml` runs the Python suite only + under `has_changed_tracked_files '*.py'`. The commit introducing this file + masked that gap by adding a `.py` file; a later docs-only corruption would + not have been caught. Asserted on both event triggers, and on the run step, + so a path filter that survives while the step stops invoking the suite fails. + """ + workflow = yaml.safe_load(QUALITY_WORKFLOW.read_text(encoding="utf-8")) + # `on:` parses as the YAML boolean True unless quoted; accept either key. + triggers = workflow.get("on", workflow.get(True)) + for event in ("pull_request", "push"): + assert MARKDOWN_GLOB in triggers[event]["paths"] + steps = workflow["jobs"]["quality"]["steps"] + commands = "\n".join(step.get("run", "") for step in steps) + assert f"pytest -q {Path(__file__).name}" in commands.replace( + "tests/", "" + ), commands From c22937d4f6aafa892b0c2c5cacf6bca1497c331d Mon Sep 17 00:00:00 2001 From: seonghobae Date: Sun, 6 Sep 2026 14:17:17 +0000 Subject: [PATCH 3/4] fix(test): drop the PyYAML import, require a keyword boundary, parse tilde fences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three further Codex findings on 1db0804f, all verified before fixing. P1 — the contract imported PyYAML, which is not in requirements-opencode-review-ci.txt or its generated lock, and nothing else in this repository imports it. On a clean actions/setup-python interpreter this file would have failed at collection and taken EVERY suite run in CI with it, not just the Markdown-triggered one; it passed locally only because the development sandbox happened to have PyYAML installed. Adding it to the hash-locked input would push a new dependency into every consumer repository's review sandbox for one assertion, so the dependency is removed instead: `_workflow_trigger_paths` reads the `on:` block with an indentation scan and the run step is matched as text — the idiom the other workflow-contract tests here already use. Verified by executing the module with `sys.modules['yaml'] = None`. P2 — `startswith` accepted keyword prefixes, so `graphical nonsense` and an orphaned `graphNode["orphan"] --> B` satisfied the declaration contract in every file except the separately pinned ARCHITECTURE.md. `MERMAID_DECLARATION` now requires a boundary after the keyword, longest alternative first so `stateDiagram-v2` is not truncated to `stateDiagram`. P2 — `_fence` recognised only backticks, so a CommonMark `~~~` document produced no parsed blocks at all and passed the closure, seam and orphan contracts silently. Both delimiters are recognised and a block closes only on a fence of its own delimiter. No tracked file uses `~~~` today, which is exactly why the hole was invisible. Controls: test_a_keyword_prefix_is_not_a_diagram_declaration and test_tilde_fences_are_parsed_and_must_match_their_delimiter. The first control's own first draft over-claimed — it listed `pie chart data` as an impostor, but `pie` is a real declaration followed by a boundary. The contract stops at "declares a diagram type"; validating a pie body is mermaid's business, and asserting otherwise would fail diagrams that render. The docstring says so. Verification: full suite 2971 passed, 1 skipped, 21 subtests passed; `interrogate` PASSED at 100.0%; `git diff --check` clean; the wiring contract re-verified RED with the glob removed and with the invocation removed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- CHANGELOG.md | 1 + tests/test_markdown_fenced_block_integrity.py | 149 ++++++++++++++---- 2 files changed, 123 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b45f67e2c..ecf85fe15d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Markdown changes run the fenced-block contract - `.github/workflows/markdown-fenced-block-quality-ci.yml` (new) runs the fenced-block contract and then the complete suite on any `**.md` change. Without it the contract was dead on the change it exists for: every other suite-running workflow here is path-filtered to Python, Rust, R or its own scripts (`trusted-uv-materializer-quality-ci.yml`, `agent-review-runtime-quality-ci.yml`, `agent-mention-router-quality-ci.yml`), and `opencode-review-dispatch.yml` reaches `run_python_test_coverage` only under `has_changed_tracked_files '*.py'` (line 2121), so a PR that only split a mermaid diagram in `ARCHITECTURE.md` would have shipped green -- the introducing commit masked the gap by adding a `.py` file. Found by Codex review, verified against the workflow sources before fixing. The glob is the documented `**.md` rather than `**/*.md`, which is ambiguous about root-level files and `ARCHITECTURE.md` is one. `test_a_markdown_only_change_runs_this_contract` pins both event triggers and the run step, and fails when either the glob or the explicit invocation is removed. This workflow is not org-required and is not in ruleset `18156473`, so its path filters are the ordinary quality-CI pattern rather than the required-workflow trap in `docs/doctoring/required-workflow-path-filter-boundary.md`. +- Three further Codex findings on the second head. The contract imported PyYAML, which is **not** in `requirements-opencode-review-ci.txt` or its lock, and nothing else in this repository imports it -- so on a clean `actions/setup-python` interpreter this file would have failed at collection and taken *every* suite run in CI with it, not only the Markdown-triggered one. It passed locally only because the development sandbox happened to have PyYAML. Rather than push a new dependency into every consumer repository's review sandbox for one assertion, `_workflow_trigger_paths` reads the `on:` block with an indentation scan and the run step is matched as text, which is also the idiom the other workflow-contract tests here already use. Separately, `startswith` accepted keyword prefixes, so `graphical nonsense` and an orphaned `graphNode["orphan"] --> B` both satisfied the declaration contract everywhere except the pinned `ARCHITECTURE.md`; matching is now a regex requiring a real boundary after the keyword, longest alternative first so `stateDiagram-v2` is not truncated. And `_fence` recognised only backticks, so a CommonMark `~~~` document produced *no parsed blocks at all* and passed the closure, seam and orphan contracts silently; both delimiters are recognised and a block closes only on its own delimiter. No tracked file uses `~~~` today, which is exactly why the hole was invisible. Controls: `test_a_keyword_prefix_is_not_a_diagram_declaration` and `test_tilde_fences_are_parsed_and_must_match_their_delimiter`. The boundary contract deliberately stops at "declares a diagram type" -- `pie chart data` stays acceptable because `pie` is the declaration; whether the rest of the pie body is valid is mermaid's business, and asserting otherwise would fail diagrams that render. ### Failed-check finding names the Strix sandbox instead of the gateway diff --git a/tests/test_markdown_fenced_block_integrity.py b/tests/test_markdown_fenced_block_integrity.py index 8325d875f1..32210e4cd8 100644 --- a/tests/test_markdown_fenced_block_integrity.py +++ b/tests/test_markdown_fenced_block_integrity.py @@ -34,8 +34,6 @@ import subprocess from pathlib import Path -import yaml - # Every mermaid diagram type used in this repository plus the rest of the # documented set, so a new diagram kind is not reported as a split fragment. MERMAID_KEYWORDS = ( @@ -64,6 +62,15 @@ "C4Deployment", ) +# A declaration must be the keyword itself, not merely start with it: +# `"graphical nonsense".startswith("graph")` is True, and such a block does not +# render. Longest alternative first so `stateDiagram-v2` is not truncated to +# `stateDiagram`, and the lookahead rejects any identifier character after it. +MERMAID_DECLARATION = re.compile( + "^(?:%s)(?![A-Za-z0-9_-])" + % "|".join(re.escape(word) for word in sorted(MERMAID_KEYWORDS, key=len, reverse=True)) +) + # A mermaid node declaration with a quoted label — `Hub["This repo"]`, # `Gate{"approved?"}`, `Run("strix")`. The quotes are what make this safe to # match: the shell, JSON and log output this repository otherwise fences does not @@ -92,13 +99,19 @@ def _tracked_markdown_files() -> list[Path]: return [Path(name) for name in listed.split("\0") if name] -def _fence(line: str) -> tuple[int, str] | None: - """Return `(backtick count, info string)` if this line is a fence, else `None`.""" +def _fence(line: str) -> tuple[str, int, str] | None: + """Return `(delimiter, run length, info string)` if this line is a fence. + + CommonMark supports both backtick and tilde fences, and a block opened with + one delimiter is closed only by the other of the same kind — so the delimiter + travels with the run length rather than being assumed to be a backtick. + """ stripped = line.strip() - if not stripped.startswith("```"): - return None - ticks = len(stripped) - len(stripped.lstrip("`")) - return ticks, stripped[ticks:].strip() + for delimiter in ("`", "~"): + if stripped.startswith(delimiter * 3): + run = len(stripped) - len(stripped.lstrip(delimiter)) + return delimiter, run, stripped[run:].strip() + return None def _parse_blocks(text: str) -> list[tuple[int, str, list[str], int | None]]: @@ -114,15 +127,16 @@ def _parse_blocks(text: str) -> list[tuple[int, str, list[str], int | None]]: blocks: list[tuple[int, str, list[str], int | None]] = [] opening: int | None = None info = "" - ticks = 0 + delimiter = "" + run = 0 for number, line in enumerate(lines, 1): parsed = _fence(line) if parsed is None: continue - line_ticks, line_info = parsed + line_delimiter, line_run, line_info = parsed if opening is None: - opening, info, ticks = number, line_info, line_ticks - elif not line_info and line_ticks >= ticks: + opening, info, delimiter, run = number, line_info, line_delimiter, line_run + elif line_delimiter == delimiter and not line_info and line_run >= run: blocks.append((opening, info, lines[opening : number - 1], number)) opening = None if opening is not None: @@ -153,7 +167,7 @@ def _reads_as_mermaid(body: list[str]) -> bool: the opening declaration, an edge, and a quoted node declaration. """ first = _declaration_line(body) - if first.startswith(MERMAID_KEYWORDS) or first.startswith("subgraph "): + if MERMAID_DECLARATION.match(first) or first.startswith("subgraph "): return True if any(edge in first for edge in MERMAID_EDGES): return True @@ -164,7 +178,7 @@ def _unclosed_blocks(path: Path, text: str) -> list[str]: """Return a report line for every block in this file the text never closes.""" return [ f"{path}:{opening}: fenced block opened here is never closed by an " - "untagged fence of the same length" + "untagged fence of the same delimiter and at least the same length" for opening, _, _, closing in _parse_blocks(text) if closing is None ] @@ -229,7 +243,7 @@ def test_every_mermaid_block_starts_with_a_diagram_declaration() -> None: if info != "mermaid": continue first = _declaration_line(body) - if not first.startswith(MERMAID_KEYWORDS): + if not MERMAID_DECLARATION.match(first): bad.append( f"{path}:{opening}: mermaid block declares {first!r}, " "not a mermaid diagram type" @@ -338,7 +352,7 @@ def test_split_block_keeping_the_tag_is_reported_as_a_fragment() -> None: fragments = [ _declaration_line(body) for _, info, body, _ in _parse_blocks(damaged) - if info == "mermaid" and not _declaration_line(body).startswith(MERMAID_KEYWORDS) + if info == "mermaid" and not MERMAID_DECLARATION.match(_declaration_line(body)) ] assert fragments != [] @@ -386,12 +400,55 @@ def test_a_commented_mermaid_block_declares_its_diagram() -> None: directive = ["%%{init: {'theme': 'dark'}}%%", "sequenceDiagram", " A->>B: hi"] assert _declaration_line(commented) == "flowchart LR" assert _declaration_line(directive) == "sequenceDiagram" - assert _declaration_line(commented).startswith(MERMAID_KEYWORDS) - assert _declaration_line(directive).startswith(MERMAID_KEYWORDS) + assert MERMAID_DECLARATION.match(_declaration_line(commented)) + assert MERMAID_DECLARATION.match(_declaration_line(directive)) assert _reads_as_mermaid(commented) assert _reads_as_mermaid(directive) +def _workflow_trigger_paths(text: str, event: str) -> list[str]: + """Return the `paths:` entries under one `on:` event, without a YAML parser. + + PyYAML is not in `requirements-opencode-review-ci-hashes.txt` and nothing else + in this repository imports it, so depending on it here would fail collection on + a clean CI interpreter — and adding it to the hash-locked input would push a new + dependency into every consumer repository's review sandbox for one assertion. + The scan is indentation-based over the fixed shape this workflow file has. + """ + lines = text.split("\n") + try: + event_at = next( + index + for index, line in enumerate(lines) + if line.startswith(f" {event}:") + ) + except StopIteration: + return [] + paths_at = next( + ( + index + for index in range(event_at + 1, len(lines)) + if lines[index].strip() == "paths:" + # stop at the next event key at the same indentation as `event` + and not any( + lines[between].startswith(" ") and not lines[between].startswith(" ") + and lines[between].strip().endswith(":") + for between in range(event_at + 1, index) + ) + ), + None, + ) + if paths_at is None: + return [] + entries: list[str] = [] + for line in lines[paths_at + 1 :]: + stripped = line.strip() + if not stripped.startswith("- "): + break + entries.append(stripped[2:].strip().strip('"').strip("'")) + return entries + + def test_a_markdown_only_change_runs_this_contract() -> None: """Markdown changes must trigger a workflow that executes this file. @@ -403,13 +460,51 @@ def test_a_markdown_only_change_runs_this_contract() -> None: not have been caught. Asserted on both event triggers, and on the run step, so a path filter that survives while the step stops invoking the suite fails. """ - workflow = yaml.safe_load(QUALITY_WORKFLOW.read_text(encoding="utf-8")) - # `on:` parses as the YAML boolean True unless quoted; accept either key. - triggers = workflow.get("on", workflow.get(True)) + workflow = QUALITY_WORKFLOW.read_text(encoding="utf-8") for event in ("pull_request", "push"): - assert MARKDOWN_GLOB in triggers[event]["paths"] - steps = workflow["jobs"]["quality"]["steps"] - commands = "\n".join(step.get("run", "") for step in steps) - assert f"pytest -q {Path(__file__).name}" in commands.replace( - "tests/", "" - ), commands + assert MARKDOWN_GLOB in _workflow_trigger_paths(workflow, event), event + assert f"pytest -q {Path('tests') / Path(__file__).name}" in workflow + + +def test_a_keyword_prefix_is_not_a_diagram_declaration() -> None: + """`startswith` accepts prefixes, so the declaration needs a real boundary. + + `"graphical nonsense".startswith("graph")` is True, and an orphaned node line + such as `graphNode["orphan"] --> B` starts with `graph` too. Neither renders, + yet outside the separately pinned `ARCHITECTURE.md` both satisfied the + repository-wide declaration contract. + + The boundary is the whole claim: this contract asserts a block *declares a + diagram type*, not that the diagram body is well formed. `pie chart data` + stays acceptable here because `pie` is the declaration and a boundary follows + it — whether the remaining pie syntax is valid is mermaid's business, not a + fence-integrity gate's, and pretending otherwise would make this test fail on + diagrams that render. + """ + for impostor in ("graphical nonsense", 'graphNode["orphan"] --> B', "flowcharts"): + assert not MERMAID_DECLARATION.match(impostor), impostor + for real in ("graph TD", "flowchart LR", "pie", "erDiagram", "stateDiagram-v2"): + assert MERMAID_DECLARATION.match(real), real + + +def test_tilde_fences_are_parsed_and_must_match_their_delimiter(tmp_path: Path) -> None: + """CommonMark tilde fences count, and a backtick fence does not close one. + + A parser that saw only backticks returned no blocks at all for a tilde + document, so an unclosed `~~~mermaid` block — or a split involving tilde + fences — passed the closure, seam and orphan contracts silently. + """ + closed = "~~~mermaid\ngraph TD\n A --> B\n~~~\n" + blocks = _parse_blocks(closed) + assert len(blocks) == 1 + assert blocks[0][1] == "mermaid" + assert blocks[0][3] == 4 + + unclosed = "~~~mermaid\ngraph TD\n A --> B\n" + assert _unclosed_blocks(tmp_path / "tilde.md", unclosed) != [] + + # A backtick fence is content inside a tilde block, not its closer. + mismatched = "~~~mermaid\ngraph TD\n```\n" + mismatched_blocks = _parse_blocks(mismatched) + assert len(mismatched_blocks) == 1 + assert mismatched_blocks[0][3] is None From 533b86b831419a8e5b07e56f84e645ff2dea2e4d Mon Sep 17 00:00:00 2001 From: seonghobae Date: Sun, 6 Sep 2026 14:25:43 +0000 Subject: [PATCH 4/4] fix(test): apply CommonMark's three-space fence indentation limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit finding on c22937d4, verified against CommonMark and the corpus. `_fence` read each line with `line.strip()`, so a four-space-indented fence-like line — the content of an indented code block, not a fence — opened a block. When such a sample runs to the end of the file, `_unclosed_blocks` reports valid Markdown as unclosed, and the Markdown-triggered CI job then blocks a correct documentation change. For a gate whose entire value is that it reports zero findings today, a false rejection is the one outcome it must never produce. CommonMark allows a fence at most three spaces of indentation, and counts a tab as four, so a leading tab exceeds the limit on its own. `_fence` now measures the indent, declines past three, and strips only the measured indent. `test_an_indented_code_block_is_not_a_fence` covers both the four-space and the tab form (each asserted to parse to no blocks and to report nothing unclosed) and pins the other half of the rule: three spaces is still a fence, closing at its own three-space fence. Verified RED against the previous `line.strip()` implementation. No tracked file carries an indented fence today — `git grep -n '^ \+```' -- '*.md'` is empty — which is exactly why this was invisible, the same reason the `~~~` hole was. Verification: full suite 2972 passed, 1 skipped, 21 subtests passed; `interrogate` PASSED at 100.0%; `git diff --check` clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- CHANGELOG.md | 2 +- tests/test_markdown_fenced_block_integrity.py | 38 ++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ecf85fe15d..300225046e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Markdown changes run the fenced-block contract - `.github/workflows/markdown-fenced-block-quality-ci.yml` (new) runs the fenced-block contract and then the complete suite on any `**.md` change. Without it the contract was dead on the change it exists for: every other suite-running workflow here is path-filtered to Python, Rust, R or its own scripts (`trusted-uv-materializer-quality-ci.yml`, `agent-review-runtime-quality-ci.yml`, `agent-mention-router-quality-ci.yml`), and `opencode-review-dispatch.yml` reaches `run_python_test_coverage` only under `has_changed_tracked_files '*.py'` (line 2121), so a PR that only split a mermaid diagram in `ARCHITECTURE.md` would have shipped green -- the introducing commit masked the gap by adding a `.py` file. Found by Codex review, verified against the workflow sources before fixing. The glob is the documented `**.md` rather than `**/*.md`, which is ambiguous about root-level files and `ARCHITECTURE.md` is one. `test_a_markdown_only_change_runs_this_contract` pins both event triggers and the run step, and fails when either the glob or the explicit invocation is removed. This workflow is not org-required and is not in ruleset `18156473`, so its path filters are the ordinary quality-CI pattern rather than the required-workflow trap in `docs/doctoring/required-workflow-path-filter-boundary.md`. -- Three further Codex findings on the second head. The contract imported PyYAML, which is **not** in `requirements-opencode-review-ci.txt` or its lock, and nothing else in this repository imports it -- so on a clean `actions/setup-python` interpreter this file would have failed at collection and taken *every* suite run in CI with it, not only the Markdown-triggered one. It passed locally only because the development sandbox happened to have PyYAML. Rather than push a new dependency into every consumer repository's review sandbox for one assertion, `_workflow_trigger_paths` reads the `on:` block with an indentation scan and the run step is matched as text, which is also the idiom the other workflow-contract tests here already use. Separately, `startswith` accepted keyword prefixes, so `graphical nonsense` and an orphaned `graphNode["orphan"] --> B` both satisfied the declaration contract everywhere except the pinned `ARCHITECTURE.md`; matching is now a regex requiring a real boundary after the keyword, longest alternative first so `stateDiagram-v2` is not truncated. And `_fence` recognised only backticks, so a CommonMark `~~~` document produced *no parsed blocks at all* and passed the closure, seam and orphan contracts silently; both delimiters are recognised and a block closes only on its own delimiter. No tracked file uses `~~~` today, which is exactly why the hole was invisible. Controls: `test_a_keyword_prefix_is_not_a_diagram_declaration` and `test_tilde_fences_are_parsed_and_must_match_their_delimiter`. The boundary contract deliberately stops at "declares a diagram type" -- `pie chart data` stays acceptable because `pie` is the declaration; whether the rest of the pie body is valid is mermaid's business, and asserting otherwise would fail diagrams that render. +- Three further Codex findings on the second head. The contract imported PyYAML, which is **not** in `requirements-opencode-review-ci.txt` or its lock, and nothing else in this repository imports it -- so on a clean `actions/setup-python` interpreter this file would have failed at collection and taken *every* suite run in CI with it, not only the Markdown-triggered one. It passed locally only because the development sandbox happened to have PyYAML. Rather than push a new dependency into every consumer repository's review sandbox for one assertion, `_workflow_trigger_paths` reads the `on:` block with an indentation scan and the run step is matched as text, which is also the idiom the other workflow-contract tests here already use. Separately, `startswith` accepted keyword prefixes, so `graphical nonsense` and an orphaned `graphNode["orphan"] --> B` both satisfied the declaration contract everywhere except the pinned `ARCHITECTURE.md`; matching is now a regex requiring a real boundary after the keyword, longest alternative first so `stateDiagram-v2` is not truncated. And `_fence` recognised only backticks, so a CommonMark `~~~` document produced *no parsed blocks at all* and passed the closure, seam and orphan contracts silently; both delimiters are recognised and a block closes only on its own delimiter. No tracked file uses `~~~` today, which is exactly why the hole was invisible. Controls: `test_a_keyword_prefix_is_not_a_diagram_declaration` and `test_tilde_fences_are_parsed_and_must_match_their_delimiter`. A fourth, from CodeRabbit: `_fence` read each line stripped, so four-space-indented fence-like text inside an indented code block opened a block, and when such a sample ran to end of file the gate reported valid Markdown as unclosed -- a docs gate rejecting a correct document is the one failure it must never produce. CommonMark allows a fence at most three spaces of indentation and counts a tab as four, so `_fence` now measures the indent and declines past the limit; `test_an_indented_code_block_is_not_a_fence` covers the space and tab forms and asserts three spaces is still a fence. As with `~~~`, no tracked file carries an indented fence today, which is why neither hole was visible. The boundary contract deliberately stops at "declares a diagram type" -- `pie chart data` stays acceptable because `pie` is the declaration; whether the rest of the pie body is valid is mermaid's business, and asserting otherwise would fail diagrams that render. ### Failed-check finding names the Strix sandbox instead of the gateway diff --git a/tests/test_markdown_fenced_block_integrity.py b/tests/test_markdown_fenced_block_integrity.py index 32210e4cd8..ab99388473 100644 --- a/tests/test_markdown_fenced_block_integrity.py +++ b/tests/test_markdown_fenced_block_integrity.py @@ -103,10 +103,19 @@ def _fence(line: str) -> tuple[str, int, str] | None: """Return `(delimiter, run length, info string)` if this line is a fence. CommonMark supports both backtick and tilde fences, and a block opened with - one delimiter is closed only by the other of the same kind — so the delimiter + one delimiter is closed only by one of the same kind — so the delimiter travels with the run length rather than being assumed to be a backtick. + + A fence also carries at most three spaces of indentation: at four the line is + the content of an indented code block, and a tab counts as four on its own. + Stripping indentation before looking would make this gate reject a valid + document whose indented sample contains fence-like text, which is the one + outcome a docs gate must not produce. """ - stripped = line.strip() + indent = len(line) - len(line.lstrip(" \t")) + if indent > 3 or "\t" in line[:indent]: + return None + stripped = line[indent:].rstrip() for delimiter in ("`", "~"): if stripped.startswith(delimiter * 3): run = len(stripped) - len(stripped.lstrip(delimiter)) @@ -508,3 +517,28 @@ def test_tilde_fences_are_parsed_and_must_match_their_delimiter(tmp_path: Path) mismatched_blocks = _parse_blocks(mismatched) assert len(mismatched_blocks) == 1 assert mismatched_blocks[0][3] is None + + +def test_an_indented_code_block_is_not_a_fence(tmp_path: Path) -> None: + """Four-space-indented fence-like text is content, not a fence. + + CommonMark allows a fence at most three spaces of indentation; at four the + line belongs to an indented code block. Reading the line stripped would have + opened a block on a document's indented sample and, when that sample ran to + the end of the file, reported valid Markdown as unclosed — a docs gate + rejecting a correct document is the one failure it must never produce. A tab + counts as four spaces, so a leading tab is over the limit by itself. + """ + for indented in ( + "Some prose:\n\n ```mermaid\n graph TD\n A --> B\n", + "Some prose:\n\n\t```mermaid\n\tgraph TD\n", + ): + assert _parse_blocks(indented) == [] + assert _unclosed_blocks(tmp_path / "indented.md", indented) == [] + + # Three spaces is still a fence -- the other half of the same rule. + allowed = " ```mermaid\n graph TD\n ```\n" + blocks = _parse_blocks(allowed) + assert len(blocks) == 1 + assert blocks[0][1] == "mermaid" + assert blocks[0][3] == 3