Skip to content

fix(desktop): harden the renderer architecture ratchet base comparison - #4584

Open
chihumyum wants to merge 2 commits into
apache:mainfrom
chihumyum:fix/renderer-architecture-strict-base
Open

fix(desktop): harden the renderer architecture ratchet base comparison#4584
chihumyum wants to merge 2 commits into
apache:mainfrom
chihumyum:fix/renderer-architecture-strict-base

Conversation

@chihumyum

@chihumyum chihumyum commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Three follow-ups to #4249's base-tree ratchet, so the guard fails loudly and cannot be lowered from one side.

  • --strict-base: when the base commit's tree cannot be materialized or analyzed, the check fails instead of silently falling back to the base's committed ledger, a fallback that could reintroduce the stale-ledger failure CI red on main: "Check renderer architecture" fails from a stale renderer-architecture ledger #4250 demonstrated (a base ledger under-reporting its own tree wedged CI). CI now passes the flag; local runs without it are unchanged.
  • Cross-check under the base commit's checker: when check-renderer-architecture.mjs itself differs from the base commit, the base version is imported and its generateArchitectureConfig is run over both trees, and the current validateMonotonicDebt compares the two. Debt that the base rules would have flagged fails as a base-checker cross-check: violation, so one PR cannot weaken a rule and lower both sides of the ratchet at once. Nothing extra runs when the script is unchanged.
  • Git-fixture tests for the base-tree derivation, which fix(desktop): ratchet renderer debt against the base tree, not its ledger #4249 shipped without: equal-or-lower debt passes, a new unclassified legacy file fails, a base ledger that under-reports its own tree (CI red on main: "Check renderer architecture" fails from a stale renderer-architecture ledger #4250) no longer wedges, --strict-base fails when the base cannot be materialized, and the cross-check catches a weakened checker.

Refs #4582

Verification

  • node --test apps/desktop/scripts/check-renderer-architecture.test.mjs — 78 tests (71 existing + 7 new), 0 failures
  • npm run check:renderer-architecture -- --base upstream/main — passes; because this PR changes the script, the output shows the cross-check running (… differs from upstream/main; cross-checked debt under the base checker.)
  • npm run check:renderer-architecture (no base) — passes
  • Workflow policy tests (scripts/*workflow-policy*.test.mjs) — 69 tests, 0 failures
  • npm run check:asf-headers, Biome lint on both scripts, git diff --check — clean
  • renderer-architecture.json is untouched. Run under Node 24 (identical under Node 22).

Review focus

  • Strict-mode policy: under --strict-base, a base checker that exists but cannot be imported, or whose generator throws on either tree, fails closed; a base checker with no generateArchitectureConfig export, or whose output does not fit the current ledger schema, is a notice-and-skip in both modes. Consequence: a PR that makes the old checker unable to analyze the new tree must land the checker change first. Easy to flip if the other default is preferred.
  • The cross-check protects the measurement. A PR that weakens validateMonotonicDebt itself is not covered; that stays a review concern.
  • The scratch copy of the base checker is written next to the live script so @babel/parser resolves, and removed in finally; .gitignore covers a crash mid-run.
  • Surfaced but left alone: the script's import.meta.url === pathToFileURL(argv[1]) guard is realpath-sensitive, so invoking it through a symlinked path is a silent no-op. The fixtures use realpath.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — implementation, tests, and local validation. The commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above: CI fails instead of warning when the base tree cannot be derived, and a PR that weakens the checker while adding debt is rejected.
  • No

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 2, 2026
@chihumyum
chihumyum force-pushed the fix/renderer-architecture-strict-base branch from a8721f4 to 54bc30f Compare September 2, 2026 19:24
@chihumyum

Copy link
Copy Markdown
Contributor Author

The first CI run failed only in Storybook smoke, on the provider-settings.stories play; every other step passed. This PR changes only the architecture checker script, its tests, the CI step that invokes it, .gitignore, and the renderer README, so it cannot reach that story. Rebased onto current main (9225f80b0) to re-run CI; exact head 54bc30f0fde28541c4d7db2ebb123d39901a7e0f. The checker fixture suite (78/78) and check:renderer-architecture --base upstream/main (with the cross-check exercised, since the script differs from main) pass locally on this head.

Automated update from Claude Code on behalf of the PR author.

@chihumyum
chihumyum force-pushed the fix/renderer-architecture-strict-base branch from 54bc30f to 87d1e1d Compare September 3, 2026 15:10
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 68cda0b54 to clear the conflict. Exact head: 87d1e1dd36ec00cb8cb845ebf9b4020be1d4b0db. The checker fixture suite (78/78), check:renderer-architecture --base upstream/main with the cross-check exercised, and the workflow policy tests (69/69) pass on it; #4643's Vite plugin change on main merged cleanly with this branch.

Posted by Claude Code on behalf of the PR author.

@chihumyum
chihumyum force-pushed the fix/renderer-architecture-strict-base branch from 87d1e1d to 1c90237 Compare September 4, 2026 16:46
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 2c948e4e3 (after #4315). One conflict, in the renderer README: #4581 retired the same-count-replacement sentence, so the merged text keeps main's paragraph and appends this PR's --strict-base / cross-check paragraph after it. The combined checker fixture suite (#4315's controller-owner cases plus this PR's git fixtures) is 105/105; check:renderer-architecture --base upstream/main passes with the cross-check exercised; workflow policy tests 69/69. Exact head: 1c9023760fd3acca3f4f342286b426c9dc05ab81.

@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main e226ac532. The checker, its test file, and ci.yml had all moved on main since this branch's base (#4823 stopped pricing sanctioned imports, #4817 added the locale-hygiene ratchet, #4877 moved coverage out of the E2E tier); the rebase merged cleanly with no hand edits. Exact head: 9c3e285a9.

After the rebase, on Node 24:

  • node --test scripts/check-renderer-architecture.test.mjs: 108 tests, 108 pass (the three --strict-base / cross-check git-fixture cases included).
  • node scripts/check-renderer-architecture.mjs --base upstream/main --strict-base passes and prints the cross-check notice, since this branch changes the checker itself.
  • The CI step still runs --base "$BASE_SHA" --strict-base after the merge with chore(ci): ratchet locale hygiene #4817's workflow changes.

Posted by Claude Code on behalf of the PR author.

@chihumyum

Copy link
Copy Markdown
Contributor Author

@Astro-Han with #4491 and #4498 in, this is the last open item under #4582 from my side and it has never been put in front of you: three follow-ups to #4249's base-tree ratchet (--strict-base in CI so a base that cannot be materialized fails instead of falling back to the committed ledger, a cross-check under the base commit's checker when the script itself changes, and the git-fixture tests #4249 shipped without). Head 9c3e285a9 sits on main e226ac532 with no overlap with anything merged since; CI green, 108/108 fixture tests. Whenever you have time.

Posted by Claude Code on behalf of the PR author.

@chihumyum
chihumyum force-pushed the fix/renderer-architecture-strict-base branch from 9c3e285 to bba9df6 Compare September 7, 2026 15:56
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 86404941d. One conflict, in ci.yml: #4913 moved the architecture step and replaced the inline BASE_SHA expression (and this branch's ^0+$ guard) with the shared comparison step, so the branch now keeps main's step verbatim and only appends --strict-base to the --base "$BASE_SHA" invocation — that flag is the whole ci.yml diff against main. #4917's hoisted-binding cache in the checker merged cleanly. Exact head: bba9df665; 108/108 fixture tests, and --base upstream/main --strict-base passes with the cross-check notice.

Posted by Claude Code on behalf of the PR author.

Add a --strict-base flag that turns the silent fallback to the committed base
ledger (the pre-apache#4249 behaviour that wedged CI in apache#4250) into a hard failure,
and pass it from the CI step that supplies --base.
When the checker script differs from the base commit, also measure both trees
with the base commit's checker and ratchet those results, so a change cannot
weaken a rule and lower both sides of the comparison at once.
Cover the base-tree derivation with git-fixture integration tests.

Generated-by: Claude Code
@chihumyum
chihumyum force-pushed the fix/renderer-architecture-strict-base branch from bba9df6 to 81067b0 Compare September 7, 2026 16:07
@chihumyum

Copy link
Copy Markdown
Contributor Author

The CI failure at bba9df665 was scripts/ci-workflow-policy.test.mjs (shared comparison drives every diff gate…), which now pins the exact argv of every diff gate in ci.yml and still expected the architecture step without --strict-base. That expectation moves with the flag in the same commit, so the policy test now pins --base <sha> --strict-base as CI's invocation. Exact head: 81067b050; the four CI-planner suites pass 127/127 locally, lint clean, everything else unchanged from the previous head.

Posted by Claude Code on behalf of the PR author.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the strict base handling and real-git fixtures. The base materialization and CI wiring work, but the cross-check still trusts one part of the modified checker. A focused exact-head fixture reproduces false success; details inline.

中文

strict base 加载与 CI 接线成立,但交叉检查仍信任当前改动中的 comparator,定向 git fixture 已复现假绿。

AI-assisted review; the coordinating Codex agent independently reran the comparator-only counterexample.

Comment thread apps/desktop/scripts/check-renderer-architecture.mjs

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Following the scope correction in the previous thread, this audit found no P0–P2 within the stated measurement/strict-base contract. The seven new Git fixtures plus an independent comparator probe passed. The Git fixtures exercise the real CLI/history boundary and are not redundant solely because they exceed the implementation in lines. The remaining inline item is documentation-only; no larger checker framework is requested.

中文

更正范围后,已声明契约内无 P0–P2。7 个新增 Git 用例加独立反例通过,真实 CLI/历史边界测试不能因行数多而删成 helper 自证。剩余行内项只改文档,不要求更大的检查器框架。

AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.

Comment thread apps/desktop/src/renderer/README.md Outdated
into a hard error instead of a silent fallback to the committed ledger. When the
checker script itself differs from the base commit, the base commit's checker is
also imported and run over both trees, and any debt the base rules would have
flagged fails as a `base-checker cross-check:` violation, so one change cannot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Limit this guarantee to the base measurement rules

The base generator measures both trees, but the current comparator still decides the result. The PR body explicitly documents that scope; this paragraph's “any debt the base rules would have flagged” guarantee is broader. A normal comparator-only relaxation can pass, as the independent Git probe confirms. Please say that the cross-check protects measurement and that comparator changes still require review. This is a wording fix, not a request to expand the implementation.

中文

旧 generator 负责测量,当前 comparator 仍判决,正文已说明;这里对所有旧规则的保证过强。独立 Git 探针证明只放松 comparator 可以通过。请明确只保护测量规则,comparator 仍需 review;只改文案,不扩大实现。

AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 13ac30d42, README only: the paragraph now says the base checker is imported to measure both trees, that the cross-check catches debt the base measurement rules (generation and classification) would have flagged, and that the comparison still runs under the current checker, so a change to validateMonotonicDebt stays a review concern. The fallback sentence now describes a silent fallback as able to reintroduce the stale-ledger failure #4250 demonstrated, and the PR body's --strict-base bullet says the same instead of calling it the cause. No implementation change, per the withdrawn P2; the comparator-only counterexample is noted on #4582 under the ratchet follow-ups so it is not lost.

Posted by Claude Code on behalf of the PR author.

The base checker measures both trees; the comparison still runs under the
current checker, so a change to `validateMonotonicDebt` is a review
concern, not something the cross-check catches. Say so in the README, and
describe the fallback as able to reintroduce the stale-ledger failure
apache#4250 demonstrated rather than as its cause.
@chihumyum
chihumyum requested a review from Astro-Han September 10, 2026 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants