From 53a4707e2ae073fa76d955694aa8faa20e50dc67 Mon Sep 17 00:00:00 2001 From: chaofengw Date: Thu, 3 Sep 2026 04:59:13 +0000 Subject: [PATCH 1/2] ci(families): reject source isolation symlinks Prevent model-owned source projections from preserving symlinks that can escape the selected ownership root. Document the existing single-family build and runtime proof, and cover the rejection path with a regression test. Signed-off-by: chaofengw --- tests/tools/test_model_plugin_isolation.py | 34 ++++++++++++++++++++++ tools/model_plugin_isolation.py | 9 ++++++ website/docs/extend/add-model-family.md | 6 ++++ 3 files changed, 49 insertions(+) diff --git a/tests/tools/test_model_plugin_isolation.py b/tests/tools/test_model_plugin_isolation.py index 76335a4100..1ad66984a0 100644 --- a/tests/tools/test_model_plugin_isolation.py +++ b/tests/tools/test_model_plugin_isolation.py @@ -313,6 +313,40 @@ def test_stage_source_masks_sibling_model_roots(tmp_path: Path) -> None: assert all(value > 0 for value in manifest["excluded_model_files"].values()) +def test_stage_source_rejects_model_owned_symlink_escape(tmp_path: Path) -> None: + repo_root = _make_repo(tmp_path) + _add_projection_fixture_files(repo_root) + borrowed = ( + repo_root + / "python/tensorrt_model_connect/families/decoder_family/borrowed.py" + ) + borrowed.symlink_to( + repo_root / "python/tensorrt_model_connect/families/sibling/plugin.py" + ) + subprocess.run(["git", "-C", str(repo_root), "add", str(borrowed)], check=True) + + result = subprocess.run( + [ + sys.executable, + str(TOOL), + "stage-source", + "--repo-root", + str(repo_root), + "--model", + "decoder-small", + "--output-dir", + str(tmp_path / "isolated"), + ], + cwd=REPO_ROOT, + text=True, + capture_output=True, + ) + + assert result.returncode != 0 + assert "source isolation rejects symlinks" in result.stderr + assert "decoder_family/borrowed.py" in result.stderr + + def test_stage_source_requires_clean_to_replace_output(tmp_path: Path) -> None: repo_root = _make_repo(tmp_path) _add_projection_fixture_files(repo_root) diff --git a/tools/model_plugin_isolation.py b/tools/model_plugin_isolation.py index 8748265baf..964eae71a3 100644 --- a/tools/model_plugin_isolation.py +++ b/tools/model_plugin_isolation.py @@ -383,6 +383,10 @@ def _include_source_path(path: Path, owners: dict[str, set[str]]) -> bool: return True +def _is_model_owned_path(path: Path) -> bool: + return any(_owner_under(path, root) is not None for root in _MODEL_OWNED_ROOTS) + + def _copy_source_files( repo_root: Path, output_dir: Path, @@ -405,6 +409,11 @@ def _copy_source_files( destination = output_dir / relative destination.parent.mkdir(parents=True, exist_ok=True) if source.is_symlink(): + if _is_model_owned_path(relative): + raise SystemExit( + "Model-owned source isolation rejects symlinks: " + f"{relative.as_posix()}" + ) destination.symlink_to(os.readlink(source)) elif source.is_file(): shutil.copy2(source, destination) diff --git a/website/docs/extend/add-model-family.md b/website/docs/extend/add-model-family.md index 4d6e789b93..b009b30613 100644 --- a/website/docs/extend/add-model-family.md +++ b/website/docs/extend/add-model-family.md @@ -35,6 +35,12 @@ tests/e2e/models/qwen/ Copy only the files the new family actually needs. Do not import or include a sibling family's model-owned implementation in production code. +Selective CI rebuilds affected models from a source projection containing only +the owning Python family, runtime model DSO, C++ model tests, and E2E family. +It also runs with only that model DSO available at runtime. Cross-family +imports, file reads, native dependencies, and symlink escapes therefore fail +the real model proof even when they evade static source checks. + `scripts/new_family.py` is only a preliminary Python bootstrap in this revision. It creates `plugin.py` and `__init__.py`, but it does not create the required `MODEL.toml`, family-local builder modules, runtime DSO, or E2E From 5af405db3edd6c836b3781a26608e641c04a91f7 Mon Sep 17 00:00:00 2001 From: chaofengw Date: Thu, 3 Sep 2026 04:59:31 +0000 Subject: [PATCH 2/2] ci(review): add semantic architecture checks Give CodeRabbit explicit family-ownership, shared-neutrality, benchmark-integrity, and blast-radius checks. Keep the checks advisory while their signal is calibrated, and preserve intentional family duplication through path-specific instructions and tests. Signed-off-by: chaofengw --- .coderabbit.yaml | 172 ++++++++++++++++++++++++++ REVIEW.md | 114 +++++++++++++++++ tests/tools/test_coderabbit_config.py | 73 +++++++++++ 3 files changed, 359 insertions(+) create mode 100644 REVIEW.md create mode 100644 tests/tools/test_coderabbit_config.py diff --git a/.coderabbit.yaml b/.coderabbit.yaml index ff788d6b40..d3aae3293a 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -13,12 +13,83 @@ reviews: request_changes_workflow: false high_level_summary: true high_level_summary_in_walkthrough: true + high_level_summary_instructions: | + Summarize intent and behavior, then add an "Architecture impact" section. + Identify family-owned files, changed shared surfaces, new dependency + directions, affected consumers, and unresolved blast-radius questions. + Use PASS, BLOCK, or HUMAN REVIEW REQUIRED as defined in REVIEW.md. Do not + repeat inline findings or claim that an absence of findings proves safety. poem: false in_progress_fortune: false auto_review: enabled: true drafts: false + auto_incremental_review: true + auto_pause_after_reviewed_commits: 0 + + # Keep semantic checks advisory until their signal has been calibrated on + # real pull requests. Architecture owners remain the decision authority. + pre_merge_checks: + override_requested_reviewers_only: true + custom_checks: + - name: "Family ownership boundary" + mode: "warning" + instructions: | + Evaluate only dependencies introduced, expanded, or newly relied on + by this pull request. Fail when one model family imports, includes, + links, loads, reads, inherits from, or reuses implementation, fixtures, + reference code, comparators, or validation artifacts owned by another + family. Also fail when adding or changing one family requires editing + another family or a central family registry, switch, source list, or + strategy map. Do not fail because model-specific code is duplicated or + similar; that duplication is intentional. Shared model-agnostic + contracts and mechanics are allowed. Cite exact changed lines and both + sides of the dependency. Return inconclusive when a dynamic dependency + cannot be resolved from repository evidence. + + - name: "Shared semantic neutrality" + mode: "warning" + instructions: | + Evaluate changed shared code outside the model-owned Python, runtime, + C++ test, E2E, and family-tool directories. Fail when shared code adds + or expands model-specific configuration, topology, tensor or output + semantics, engine composition, runtime orchestration, preprocessing, + postprocessing, reference behavior, validation evidence, datasets, + task metrics, thresholds, probes, or runtime strategies. A literal + family/model name, family conditional, or model tensor name in shared + code is evidence requiring investigation, not automatic proof. Pass a + shared change only when its behavior is model-agnostic and family-owned + data fully supplies specialization through an existing narrow + contract. Do not fail solely for pre-existing unchanged debt. Cite the + changed semantic decision and its owning family or affected consumers. + + - name: "Benchmark validation integrity" + mode: "warning" + instructions: | + Apply when benchmark, performance, reference, metric, gate, workload, + report, or validation behavior changes. Fail when compared + implementations time different semantic regions; synchronization, + device-to-host transfer, reduction, output validation, or serialization + is included on only one side; execution units such as shards or batches + are conflated with task units such as queries or samples; aggregation + changes the evidence level; or a shared behavior change lacks evidence + for affected consumers. Do not require identical implementations, only + equivalent measurement and validation meaning. Cite both compared + paths or the before/after accounting contract. + + - name: "Shared change blast radius" + mode: "warning" + instructions: | + Apply when a family-oriented pull request changes shared code, + contracts, tooling, examples, benchmarks, catalogs, or validation + infrastructure. Fail when the pull request description and repository + evidence do not identify the concrete model-agnostic need, affected + consumers, behavior or compatibility impact, validation evidence, and + why the implementation cannot remain family-owned. Do not fail a + family-local change that does not alter a shared surface. Return + inconclusive when consumer or compatibility evidence is unavailable; + do not infer that missing evidence is safe. # Do not let review finishing touches create follow-up code changes. Authors # remain responsible for applying and validating any suggested change. @@ -38,6 +109,100 @@ reviews: - "!third_party/**" path_instructions: + # Keep both the current split ownership roots and the consolidated family + # root used by the horizontal-scaling layout. This makes the review policy + # continuous across the repository migration. + - path: "families/**" + instructions: | + Treat each direct child as an independently owned model-family vertical + slice, including build code, runtime code, tests, fixtures, references, + comparators, thresholds, and probes. Similar or duplicated + model-specific code is intentional isolation; do not suggest extracting, + deduplicating, or importing sibling-family implementation solely because + code looks alike. Flag cross-family imports, includes, links, loads, + inheritance, file reads, fixtures, validation reuse, or required edits. + A new shared contract requires a concrete model-agnostic need; reduced + line count is not evidence. + + - path: "python/tensorrt_model_connect/families/**" + instructions: | + Treat each direct child as an independently owned model-family slice. + Similar or duplicated model-specific code is intentional isolation; do + not suggest extracting, deduplicating, or importing sibling-family + implementation solely because code looks alike. Flag new cross-family + imports, file reads, inheritance, fixtures, reference behavior, or + validation reuse. A new shared contract requires a concrete + model-agnostic need; reduced line count is not evidence. + + - path: "src/runtime/models/**" + instructions: | + Treat each direct child as an independently owned runtime model DSO. + Similar or duplicated implementation is intentional isolation; do not + suggest sharing model-specific helpers solely because code looks alike. + Flag cross-family includes, links, loads, source references, symlinks, + or runtime dependencies between model-owner directories. + + - path: "tests/e2e/models/**" + instructions: | + Treat each direct child as family-owned validation. Similar fixtures, + runners, references, comparators, thresholds, and probes may remain + duplicated as intentional isolation that preserves independent evidence + and rollback. Flag reuse across sibling families or movement of + model-specific validation semantics into the shared E2E harness. + + - path: "core/**" + instructions: | + Treat core as model-agnostic contracts and mechanics. Flag literal + model/family names, family conditionals, model topology or tensor + semantics, model-specific orchestration or validation, and dependencies + from core into families or applications. A family-specific need should + remain in its owning family unless multiple independent consumers prove + a narrow shared contract. + + - path: "apps/**" + instructions: | + Applications and benchmarks must consume public core and family + contracts without becoming a source of model semantics. Flag shared app + code that branches on a family, interprets family-specific tensors or + outputs, owns family validation policy, or becomes a dependency of core + or family implementation. + + - path: "apps/benchmark/**" + instructions: | + Check semantic equivalence of timed regions, synchronization, warmup, + preprocessing, transfer, execution, postprocessing, validation, and + serialization. Keep shards, batches, requests, queries, samples, tokens, + and generated artifacts as distinct accounting units. Flag family + behavior embedded in shared benchmark code. + + - path: "tools/**" + instructions: | + Treat tools as shared infrastructure. Flag model/family branches, + model-specific tensor or output semantics, datasets, task metrics, + thresholds, probes, reference behavior, aggregation policy, or runtime + strategies. Require a concrete model-agnostic need for shared changes. + + - path: "benchmarks/**" + instructions: | + Check semantic equivalence of timed regions, synchronization, warmup, + preprocessing, transfer, execution, postprocessing, validation, and + serialization. Keep shards, batches, requests, queries, samples, tokens, + and generated artifacts as distinct accounting units. Flag family + behavior embedded in shared benchmark code. + + - path: "examples/**" + instructions: | + Examples must consume public build, load, task, and runtime APIs. Flag + family-specific orchestration or output interpretation added to a shared + executable or worker, and any reverse dependency into example code. + + - path: "tests/validation/**" + instructions: | + Flag model-specific datasets, metrics, gates, thresholds, tensor + semantics, reference behavior, or runtime strategies stored in central + catalogs or implemented by shared validation code. Do not weaken + validation evidence or acceptance criteria. + - path: ".github/**" instructions: | Treat fork pull requests as untrusted. Preserve the trust boundary @@ -74,3 +239,10 @@ chat: auto_reply: true art: false allow_non_org_members: true + +knowledge_base: + code_guidelines: + enabled: true + filePatterns: + - files: "REVIEW.md" + applyTo: "**/*" diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000000..aaecc2a571 --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,114 @@ +# Pull Request Review Contract + +This file defines the repository-wide semantic review contract for automated +and human reviewers. `AGENTS.md` and the architecture and extension guides +under `website/docs/` remain authoritative. This contract turns those +decisions into a repeatable pull-request review. + +## Review Objective + +Review changed behavior, not only changed lines. Trace relevant callers, +consumers, configuration, tests, and data flow far enough to determine whether +the pull request preserves family ownership, shared-layer neutrality, and the +meaning of its validation evidence. + +Automated review is a first-pass architecture review. It must reduce human +search effort by reporting evidence-backed concerns and explicit uncertainty; +it must not claim that an absence of findings proves correctness. + +## Required Review Axes + +### Standards + +Check the pull request against the repository architecture and ownership rules: + +1. A model family remains an independently implementable, testable, changeable, + and revertible vertical slice. +2. One family does not import, include, link, load, read, inherit from, or reuse + implementation or validation artifacts owned by another family. +3. Model-specific configuration, topology, tensor semantics, engine + composition, runtime orchestration, preprocessing, postprocessing, + reference behavior, and validation remain in the owning family. +4. Shared code contains only model-agnostic contracts and mechanics. A literal + family or model name, family conditional, model tensor name, task-specific + metric, dataset, threshold, probe, or runtime strategy in shared code is a + reason to investigate ownership. +5. A family-oriented change does not require a central registry, switch, + source list, strategy map, or edit to another family. +6. Applications, examples, and benchmarks depend on public build, load, task, + and runtime APIs. Core, backend, and family implementations do not depend on + application code. +7. Code similarity is never evidence for a cross-family abstraction. + Duplication of model-specific code is intentional because it contains + defects, merge conflicts, validation, and rollback within one family. +8. A new shared contract is justified by a concrete model-agnostic need and + does not encode the behavior of its first consumer as a generic extension + point. + +Do not report pre-existing, unchanged architecture debt as a defect introduced +by the pull request. Report it only when changed code expands, depends on, or +makes that debt harder to remove. + +### Spec + +Check that the implementation and its evidence have the meaning claimed by the +pull request, linked issue, documentation, and tests: + +1. Public APIs, CLI flags, bundle fields, metrics, gates, and configuration + retain their documented semantics unless the change is explicit. +2. Shared behavior changes identify all consumers and provide proportionate + compatibility or regression evidence. A passing test for one family is not + evidence for all consumers of changed shared code. +3. Compared benchmark implementations measure equivalent regions. Treat + preprocessing, transfer, synchronization, execution, reduction, + postprocessing, validation, and serialization as distinct boundaries. +4. Workload accounting keeps execution units and task units distinct. Shards, + batches, requests, queries, samples, tokens, and generated artifacts are not + interchangeable denominators. +5. Aggregation and gate behavior preserve the intended level of evidence: + per-sample, worst-sample, percentile, task aggregate, and reference-relative + metrics are different contracts. +6. Thresholds, expected values, comparison oracles, and acceptance criteria are + not weakened merely to obtain a passing result. +7. Tests exercise the changed behavior and would fail for the regression they + claim to prevent. + +## Evidence And Severity + +Every finding must include: + +- severity and whether it blocks the pull request; +- the violated invariant or specification; +- exact changed-file and line evidence; +- the dependency direction or before/after behavior; +- the affected family or shared consumers; +- the smallest correction that restores the contract. + +Use these severities: + +- **Blocking**: incorrect results, invalid performance claims, public contract + breakage, direct cross-family dependency, or model-specific semantics added + to a shared implementation. +- **High**: a likely ownership or behavioral defect with concrete evidence and + material blast radius. +- **Medium**: non-blocking architecture debt introduced or expanded by this + change, or incomplete evidence for a material shared change. +- **Low**: do not emit as an automated comment unless it prevents a specific + future defect. Avoid style, naming, optional cleanup, and speculative + refactoring comments. + +Consolidate repeated manifestations of one root cause into one finding. Do not +repeat the same concern on every affected line. + +## Required Outcome + +The review must end with one of these outcomes: + +- `PASS`: no standards or spec violation was found in the reviewed evidence. +- `BLOCK`: at least one evidence-backed blocking violation was found. +- `HUMAN REVIEW REQUIRED`: available evidence cannot resolve a material + ownership, compatibility, measurement, or blast-radius question. + +`PASS` means no violation was found; it is not proof that the pull request is +correct. Use `HUMAN REVIEW REQUIRED` instead of guessing. Human reviewers own +the final decision and any architecture exception. diff --git a/tests/tools/test_coderabbit_config.py b/tests/tools/test_coderabbit_config.py new file mode 100644 index 0000000000..59d1063d68 --- /dev/null +++ b/tests/tools/test_coderabbit_config.py @@ -0,0 +1,73 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from pathlib import Path + +import yaml + + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +def _path_instructions() -> dict[str, str]: + config = yaml.safe_load( + (REPO_ROOT / ".coderabbit.yaml").read_text(encoding="utf-8") + ) + return { + entry["path"]: entry["instructions"] + for entry in config["reviews"]["path_instructions"] + } + + +def test_coderabbit_preserves_intentional_family_duplication() -> None: + instructions = _path_instructions() + family_paths = ( + "families/**", + "python/tensorrt_model_connect/families/**", + "src/runtime/models/**", + "tests/e2e/models/**", + ) + + for path in family_paths: + instruction = instructions[path].lower() + assert "intentional isolation" in instruction + assert "similar" in instruction or "duplicated" in instruction + assert "cross-family" in instruction or "sibling" in instruction + + +def test_coderabbit_covers_shared_paths_before_and_after_layout_migration() -> None: + instructions = _path_instructions() + + assert "model-agnostic" in instructions["core/**"] + assert "public core" in instructions["apps/**"] + assert "timed regions" in instructions["benchmarks/**"] + assert "timed regions" in instructions["apps/benchmark/**"] + + +def test_coderabbit_enables_semantic_architecture_checks() -> None: + config = yaml.safe_load( + (REPO_ROOT / ".coderabbit.yaml").read_text(encoding="utf-8") + ) + reviews = config["reviews"] + checks = { + check["name"]: check for check in reviews["pre_merge_checks"]["custom_checks"] + } + + assert set(checks) == { + "Family ownership boundary", + "Shared semantic neutrality", + "Benchmark validation integrity", + "Shared change blast radius", + } + assert all(check["mode"] == "warning" for check in checks.values()) + assert reviews["request_changes_workflow"] is False + assert reviews["auto_review"]["auto_incremental_review"] is True + assert reviews["auto_review"]["auto_pause_after_reviewed_commits"] == 0 + assert "REVIEW.md" in reviews["high_level_summary_instructions"] + + guidelines = config["knowledge_base"]["code_guidelines"] + assert guidelines["enabled"] is True + assert guidelines["filePatterns"] == [ + {"files": "REVIEW.md", "applyTo": "**/*"} + ] + assert (REPO_ROOT / "REVIEW.md").is_file()