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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions .github/workflows/markdown-fenced-block-quality-ci.yml
Original file line number Diff line number Diff line change
@@ -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
Comment thread
seonghobae marked this conversation as resolved.
- 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"
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
### 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. 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`.
- 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

- `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.
Expand Down
Loading
Loading