Skip to content

♻️ Migrate operational review documents from Show to built-in <If> - #406

Merged
taras merged 1 commit into
mainfrom
agent/issue-219-if-migration
Aug 9, 2026
Merged

♻️ Migrate operational review documents from Show to built-in <If>#406
taras merged 1 commit into
mainfrom
agent/issue-219-if-migration

Conversation

@taras

@taras taras commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Closes #219.

Why

The review library shipped its own Show.md conditional before the engine had
one. <If> is now a native structural directive, so the seven operational
review documents that still invoked Show duplicated engine behavior and let
the review system drift from the language its own specification documents.

The release-order blocker recorded in the original issue is gone:
.github/workflows/review.yml prepares and builds the checked-out revision and
runs ./dist/xmd, so no release or workflow change is involved.

What changes

Before:

<Show when={props.pr.stats.totalChanges > 20}>
...
</Show>

After:

<If condition={props.pr.stats.totalChanges > 20}>
...
</If>

Thirteen blocks across seven documents, renamed in place. Bodies, indentation,
line wrapping and surrounding blank lines are unchanged, and
.reviews/components/Show.md is deleted. Finding.md keeps its public when
prop; only its internal element becomes <If condition={props.when}>.

Every migrated condition already evaluates to a boolean and none passed
Show's optional fallback, so no <Else> branch is introduced and #258's
possible truthiness change stays independent of this PR.

How it works

<If> name → expansion engine (before filesystem resolution) → selected branch only

<If> is native, so it is handled by expansion and never resolved from
componentDirs. That is the property the new tests exercise directly: they run
the real operational sources with no If.md or Show.md anywhere in the stub
filesystem.

Review guide

Start with: scripts/tests/review-infrastructure.test.ts

Then review:

  1. The seven .reviews/ documents — a pure tag/prop rename.
  2. retires the Show component from the review library — the static guard.
  3. The five operational tests that execute the real documents.
  4. packages/core/tests/unused-in-diff.test.ts — fixture removal plus an exact
    output assertion.
  5. specs/code-review-agent-spec.md §5.10.

Look carefully at: the exact trimmed-equality assertions. They intentionally
lock the blank lines the migrated tags surround, including the five blank lines
OxlintSummary's two suppressed blocks leave behind.

What must stay true

  • Show is gone from the review library — enforced by deletion and checked by
    retires the Show component from the review library, which walks
    .reviews/**/*.md for an opening or closing Show element, rejects a
    Show.md path, and scans the focused test and the code-review spec. The guard
    asserts the walk found Finding.md first, so it cannot pass vacuously.
  • The structural name wins before filesystem component resolution — checked by
    every operational test, none of which supplies an If or Show component.
  • An unselected branch does no provider work — checked by
    skips the ExtraneousCodePolicy sample below the review threshold.
  • The scan stays scoped. packages/core/tests/expression-props.test.ts uses
    Show as an arbitrary component name and is deliberately untouched.

How to verify it

  • retires the Show component from the review library fails if any .reviews/
    document, the focused test, or the spec still carries Show.
  • renders Finding's selected icon and message and suppresses its false case
    proves exact branch output (🔴 Broken contract.) and an exactly empty false
    case; it fails on a whitespace change inside the migrated block.
  • renders OxlintSummary's clean section and its unavailable warning and
    renders RepoCleanupPolicy's clean section without running either branch
    prove the clean-section and warning paths byte-exactly.
  • suppresses ReleaseSpecWarning for ordinary files and warns on release changes proves both directions of its release-config condition.
  • expands UnusedInDiff and CommentReview to nothing without an If component
    proves CommentReview's typed data component executes (it fetches the PR's
    comments) while both captures and both trailing branches stay empty.
  • skips the ExtraneousCodePolicy sample below the review threshold installs a
    counting Sample provider: 20 changes make zero calls and render the clean
    section, 21 changes make exactly one call and surface the provider result. The
    selected control is what keeps the zero-call case from being vacuous.
  • renders the disclosure with symbol, location, count and reason in
    unused-in-diff.test.ts is now exact trimmed equality over the whole
    disclosure, so summary/table whitespace is locked.

Each new regression was mutation-checked: reverting Finding.md to <Show>
(with Show.md restored), reverting the spec example, widening the
ExtraneousCodePolicy threshold to >= 20, padding Finding's rendered line,
and removing a blank line in UnusedInDiff's table each fail the intended test
and nothing else.

Scope

Included

  • The thirteen-block rename in seven operational documents and the deletion of
    Show.md.
  • Focused regressions in the two suites that own this boundary.
  • §5.10 of the code-review-agent specification.

Intentionally unchanged

  • <If> implementation, syntax, validation, and truthiness semantics (Coerce the <If> condition with JS truthiness #258).
  • specs/executable-mdx-spec.md, which already specifies native <If>/<Else>,
    filesystem precedence, and selected-branch non-execution.
  • .github/workflows/review.yml and every other workflow.
  • packages/core/tests/expression-props.test.ts, where Show is an arbitrary
    component-resolution fixture rather than a review-library claim.
  • UnusedInDiff's selected-disclosure and suppression coverage stays in its own
    suite, because it crosses eval, <Each>, and filesystem fixture boundaries;
    the infrastructure matrix only uses its empty case.

Generated or mechanical changes

  • The seven .reviews/ documents contain no intended behavior change. The only
    cosmetic consequence is in OxlintSummary.md, whose wrapped condition keeps
    its authored continuation indentation and therefore no longer aligns under the
    longer condition={ prefix.

Verification

All required local checks are green on db78a89:

deno task lint                     0 errors, formatting clean
deno task check                    no errors
deno task check:jsr                Success Dry run complete
deno task test <3 suites>          11 passed (77 steps), 0 failed
pnpm exec tsx --tsconfig tsconfig.node.json --test <2 suites>   23 pass, 0 fail
bun test <2 suites>                23 pass, 0 fail
git diff --check                   clean

verify:clean and the full per-runtime suites were not run: this change touches
no dependency state, build purity, test discovery, shared setup, or runtime
adapter. CI owns the exhaustive corpus.

Risks and limitations

  • Local review integration evidence: not run. deno task review:local
    requires a reachable Ollama at localhost:11434 serving
    qwen3:30b-a3b plus GITHUB_TOKEN; neither is available in this
    environment. This PR's own review workflow builds the checked-out source and
    runs ./dist/xmd, so its marked review comment is the end-to-end evidence and
    is a merge blocker if it fails, errors, or posts nothing substantive.

Scope confirmation

  • Every changed file supports the purpose described above.
  • Unrelated cleanup and formatting changes are excluded.
  • Generated or mechanical changes are clearly identified.
  • The description matches the final diff and test results.

The review library shipped its own `Show.md` conditional before the engine
had one. `<If>` is now a native structural directive, so the seven operational
documents that still invoked `Show` duplicated engine behavior and let the
review system drift from the documented language.

Rename the thirteen conditional blocks in place — `when` becomes `condition`,
bodies and whitespace are unchanged — and delete `.reviews/components/Show.md`.
None of the conditions had a fallback and every one already evaluates to a
boolean, so no `<Else>` branch is introduced and issue #258's possible
truthiness change stays independent.

Deleting the component and scanning for its tag proves retirement but not
behavior, so the review-infrastructure suite now executes the real sources of
all seven documents with no `If.md` or `Show.md` in the stub filesystem: the
structural name has to win before filesystem component resolution. Compact
outputs and every suppressed case are asserted by exact trimmed equality, which
locks the summary, table, and section whitespace the tags surround.
`ExtraneousCodePolicy.md` carries the provider contract — a counting `Sample`
records zero calls below the review threshold and exactly one above it, so the
empty case is non-execution rather than a probe that never wired up.
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

PR #406: ♻️ Migrate operational review documents from Show to built-in

11 files, +339 / -64

Scope

🟡 403 lines changed. PRs under 400 receive more thorough review.

Structural

✅ No structural bloat detected.

Slop

✅ Slop indicators look low.

Static Analysis

✅ Oxlint found no issues.

Correctness

No extraneous code patterns detected.

@taras

taras commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Integration evidence

PR review workflow (required evidence). Run
31308999146,
revision db78a89. It ran deno task setup, deno task build, and
./dist/xmd run .reviews/ReviewPR.md, so the review was produced by this PR's
own engine and its own migrated documents. The posted <!-- xmd-review -->
comment is substantive and contains no ERROR marker.

Two of its sections are direct end-to-end proof of the migration:

  • ### Static Analysis is OxlintSummary.md with all three <If> blocks
    false, rendering only ReviewSection's clean text.
  • ### Correctness is ExtraneousCodePolicy.md with its <If> selected
    (403 changes > 20): the branch reached <Sample>, the provider ran, and the
    model returned No extraneous code patterns detected.

So the same document supplies both directions of the migrated condition in CI —
suppressed locally in the focused regression, selected here against a real
provider.

Local review (deno task review:local): not run. .reviews/ReviewPR.local.md
needs Ollama serving qwen3:30b-a3b at localhost:11434 plus GITHUB_TOKEN;
neither was available in the implementation environment. Recorded as a missing
prerequisite, not a test failure.

CI: all checks pass — green, lint, jsr, site, smoke, review,
test-deno, test-node, test-bun, deploy. composability skips by design
(it runs on main only).

@taras
taras marked this pull request as ready for review August 9, 2026 10:55
@taras
taras merged commit 35060af into main Aug 9, 2026
11 checks passed
@taras
taras deleted the agent/issue-219-if-migration branch August 9, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate operational review documents from Show to built-in <If>

1 participant