[TRTLLM-12152][infra] change based testing rules on tests

[crazydemo]

Summary by CodeRabbit

• New Features

  • Added three new change-based test selection rules: test definition matching, test list validation, and out-of-scope detection.
  • Introduced dry-run tool to test CBTS behavior across historical commits.
  • Enhanced test matching to support both test IDs and file path lookups.

• Documentation

  • Expanded rule system documentation with detailed scope definitions and fallback behaviors.
  • Added comprehensive rule architecture guide with trigger patterns and stage resolution logic.

Description

Test Coverage

PR Checklist

Please review the following before submitting your PR:

• PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

• PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

• Test cases are provided for new code paths (see test instructions)

• Any new dependencies have been scanned for license and vulnerabilities

• CODEOWNERS updated if ownership changes

• Documentation updated as needed

• Update tava architecture diagram if there is a significant design change in PR.

• The reviewers assigned automatically/manually are appropriate for the PR.

• Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR expands CBTS from a single-rule waive-matching system to a multi-rule architecture. It adds path-based YAML matching in YAMLIndex, introduces three new rules (TestsDefRule for test file changes, TestListRule for test-db YAML additions, OutOfScopeRule for exclusions), refactors WaivesRule to use shared helpers, updates scope-combination logic to merge testsonly-family scopes, and includes a dry-run tool for testing against historical commits.

Changes

CBTS Multi-Rule System

Layer / File(s)	Summary
Architecture & Documentation jenkins/scripts/cbts/README.md, jenkins/scripts/cbts/rules/README.md	READMEs expanded to document four-rule system, scope combinations, YAML lookup algorithms including path-based matching, trigger-mode filtering, and guidelines for adding rules.
YAMLIndex Path Matching jenkins/scripts/cbts/blocks.py	Added git-path-to-YAML-namespace translation (git_path_to_yaml_key), path-based block matching (find_match_for_path), and support data (_yaml_first_components, _path_lookup_anchor).
Shared Helper Utilities jenkins/scripts/cbts/rules/_helpers.py	New module providing diff parsing (iter_diff_changes, iter_diff_post_line_numbers), stage grouping (stages_by_yaml_stem), and block-filter resolution (lookup_ids_into_block_filters, lookup_paths_into_block_filters, resolve_affected_stages).
Rule Implementations jenkins/scripts/cbts/rules/out_of_scope_rule.py, jenkins/scripts/cbts/rules/test_list_rule.py, jenkins/scripts/cbts/rules/tests_def_rule.py, jenkins/scripts/cbts/rules/waives_rule.py	OutOfScopeRule identifies out-of-scope paths; TestListRule narrows stages from test-db YAML additions; TestsDefRule maps test file changes to affected blocks via AST and path matching; WaivesRule refactored to use shared helpers.
Rules Orchestration jenkins/scripts/cbts/main.py	RULE_CLASSES extended to register all four rules; build_rules() now accepts repo_root for TestsDefRule; _combine_scopes() merges waiveonly/testdefonly/testlistonly into testsonly; selector respects noop scope.
Dry-Run Testing Tool jenkins/scripts/cbts/tools/dryrun.py	New executable script replaying CBTS over historical git commits, generating per-commit output directories and an INDEX.md summary with scope distribution.

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: introducing new change-based testing rules focused on test file modifications to improve CI test selection.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Tools execution failed with the following error:

Failed to run tools: 14 UNAVAILABLE: read ECONNRESET

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@jenkins/scripts/cbts/rules/test_list_rule.py`:
- Around line 40-45: The _ENTRY_RE currently treats any indented list item as a
test entry; restrict matching so only list items that are children of a tests:
field are captured (or alternatively parse indentation context) by checking for
a preceding "tests:" key and matching indentation level before accepting "-
<pytest_id>" — update _ENTRY_RE and the related logic (also applied in the code
around lines 72-84) to require the same indentation as the tests: block or to
detect the nearest parent key == "tests" before classifying entries, so
wildcard/non-tests lists (e.g. "- \"*h100*\"") are not misclassified as test
IDs.

In `@jenkins/scripts/cbts/rules/tests_def_rule.py`:
- Around line 143-146: The try/except that reads the changed test asset with
(self._repo_root / git_path).read_text(encoding="utf-8") only catches OSError
but not decode failures; update the handler to also catch UnicodeDecodeError (or
UnicodeError) so binary/non-UTF8 fixtures fall back to returning [yaml_path]
instead of crashing, and make the same change for the second occurrence that
mirrors this logic later in the file (the other read_text block around the
second occurrence).
- Around line 167-181: The current per-model refinement is unsafe because
iter_diff_post_line_numbers can misattribute deleted top-level model sections;
fix by detecting deletions in the raw diff and falling back to the full file
when any deletion could affect top-level keys: in the block using diff, before
calling iter_diff_post_line_numbers/_yaml_top_keys_for_lines, check the diff
string for removal lines (e.g. any line starting with '-' in the unified diff
hunks) and if found return [yaml_path]; keep the existing flow (diff ->
iter_diff_post_line_numbers -> changed_models -> model_map -> anchors) only when
no deletion lines are present so changed_models are provable from the post-image
alone.
- Around line 218-247: In apply(), markdown docs under test YAML trees are being
processed into actionable scopes; before calling
yaml_index.git_path_to_yaml_key/_compute_anchors/_combine_scopes, detect
markdown (e.g., git_path.endswith(".md") or basename == "README.md") and treat
them as noop by adding the path to handled and no_match (or out_of_namespace
depending on existing semantics) and continue, so markdown files do not produce
testdefonly results; update the branch that currently handles yaml_path != None
to early-skip .md files before calling _compute_anchors/_combine_scopes.

In `@jenkins/scripts/cbts/tools/dryrun.py`:
- Around line 314-323: The CLI currently allows invalid numeric values for
--window and --limit; update the ap.add_argument calls for "--window" and
"--limit" to validate at parse time by using custom argparse types (or a
validator) that raise argparse.ArgumentTypeError for invalid inputs: implement a
positive_int(value) used for --window that ensures int(value) > 0, and use a
positive_int_or_none(value) (or accept None via default and validate if
provided) for --limit to ensure int(value) > 0 when not None; replace the
current type=int usages in the ap.add_argument calls and/or add explicit
argparse validation so invalid or negative values are rejected during parsing.
- Around line 252-255: The loop over pr_dir.iterdir() uses old.unlink() which
raises on directories and causes crashes when --keep-stale leaves
subdirectories; update the cleanup in dryrun.py to detect whether old is a
file/symlink or a directory (using old.is_dir()/old.is_file()/old.is_symlink())
and call the appropriate remover (unlink for files/symlinks, shutil.rmtree for
directories) or otherwise skip non-removable entries so per-label cleanup in the
for loop does not raise; adjust imports if needed and keep the rest of the logic
(the pr_dir.iterdir() loop and subsequent shared_out assignment) unchanged.
- Around line 100-113: The snapshot helpers (_snapshot_test_db and
_snapshot_groovy) currently call _git with check=False and unconditionally write
.stdout, which can create empty/missing snapshot files; change them to fail fast
by checking the _git result (use check=True or validate result.returncode and
result.stdout) and raise/abort if the git show/ls-tree calls failed or returned
empty content for any expected file; specifically validate the outputs from
_git(repo, "ls-tree", ..., sha, "--", TEST_DB_REL) and each _git(repo, "show",
f"{sha}:{f}") in _snapshot_test_db, and validate the _git(repo, "show",
f"{sha}:{GROOVY_REL}") call in _snapshot_groovy, and only write files when the
calls succeeded and stdout is non-empty, otherwise raise an error so snapshot
creation fails fast.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 622d9a53-626c-4e14-892b-a9e0af1b6311

📥 Commits

Reviewing files that changed from the base of the PR and between 2e4b05c and 2df702c.

📒 Files selected for processing (10)

• jenkins/scripts/cbts/README.md
• jenkins/scripts/cbts/blocks.py
• jenkins/scripts/cbts/main.py
• jenkins/scripts/cbts/rules/README.md
• jenkins/scripts/cbts/rules/_helpers.py
• jenkins/scripts/cbts/rules/out_of_scope_rule.py
• jenkins/scripts/cbts/rules/test_list_rule.py
• jenkins/scripts/cbts/rules/tests_def_rule.py
• jenkins/scripts/cbts/rules/waives_rule.py
• jenkins/scripts/cbts/tools/dryrun.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don’t classify every indented YAML list item as a test entry.

_ENTRY_RE also matches list items under non-tests: fields, e.g. a changed wildcard list item like - "*h100*". Those edits then get parsed as added/removed test IDs instead of structural YAML changes, so this rule can return noop or narrow when it should force fallback because stage selection semantics changed.

Also applies to: 72-84

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/rules/test_list_rule.py` around lines 40 - 45, The
_ENTRY_RE currently treats any indented list item as a test entry; restrict
matching so only list items that are children of a tests: field are captured (or
alternatively parse indentation context) by checking for a preceding "tests:"
key and matching indentation level before accepting "- <pytest_id>" — update
_ENTRY_RE and the related logic (also applied in the code around lines 72-84) to
require the same indentation as the tests: block or to detect the nearest parent
key == "tests" before classifying entries, so wildcard/non-tests lists (e.g. "-
\"*h100*\"") are not misclassified as test IDs.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle decode failures when changed test assets aren’t UTF-8 text.

Path.read_text() can raise UnicodeDecodeError for binary or non-UTF8 fixtures under tests/. Right now that bubbles out of CBTS instead of taking the intended file-level fallback, so a fixture-only PR can crash selection.

Suggested fix

-        except OSError:
+        except (OSError, UnicodeError):
             return [yaml_path]
@@
-        except OSError:
+        except (OSError, UnicodeError):
             return [yaml_path]

Also applies to: 172-175

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/rules/tests_def_rule.py` around lines 143 - 146, The
try/except that reads the changed test asset with (self._repo_root /
git_path).read_text(encoding="utf-8") only catches OSError but not decode
failures; update the handler to also catch UnicodeDecodeError (or UnicodeError)
so binary/non-UTF8 fixtures fall back to returning [yaml_path] instead of
crashing, and make the same change for the second occurrence that mirrors this
logic later in the file (the other read_text block around the second
occurrence).

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Per-model refinement is unsafe when the diff removes reference sections.

iter_diff_post_line_numbers() anchors - lines to the next post-image line, so deleting or moving an entire top-level model section in accuracy/references/*.yaml can be attributed to the following model. That narrows to the wrong test class. The safe behavior here is to fall back to [yaml_path] whenever the refinement can’t prove the changed top-level keys from the post-image alone.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/rules/tests_def_rule.py` around lines 167 - 181, The
current per-model refinement is unsafe because iter_diff_post_line_numbers can
misattribute deleted top-level model sections; fix by detecting deletions in the
raw diff and falling back to the full file when any deletion could affect
top-level keys: in the block using diff, before calling
iter_diff_post_line_numbers/_yaml_top_keys_for_lines, check the diff string for
removal lines (e.g. any line starting with '-' in the unified diff hunks) and if
found return [yaml_path]; keep the existing flow (diff ->
iter_diff_post_line_numbers -> changed_models -> model_map -> anchors) only when
no deletion lines are present so changed_models are provable from the post-image
alone.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Skip out-of-scope markdown before attempting testdef narrowing.

An in-namespace doc like tests/integration/defs/accuracy/README.md will currently be translated to accuracy/README.md, walked up to accuracy/, and produce an actionable testdefonly result. Because _combine_scopes() lets actionable scopes override noop, that defeats OutOfScopeRule and still schedules CI for markdown-only edits under YAML-covered trees.

Suggested fix

+from .out_of_scope_rule import _is_out_of_scope
+
     def apply(self, pr: PRInputs) -> Optional[RuleResult]:
         candidates = [f for f in pr.changed_files if f.startswith("tests/")]
         if not candidates:
             return None

         block_filters: dict[tuple[str, int], dict[str, set[str]]] = {}
@@
         for git_path in candidates:
+            if _is_out_of_scope(git_path):
+                continue
             yaml_path = self.yaml_index.git_path_to_yaml_key(git_path)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def apply(self, pr: PRInputs) -> Optional[RuleResult]:
	candidates = [f for f in pr.changed_files if f.startswith("tests/")]
	if not candidates:
	return None
	block_filters: dict[tuple[str, int], dict[str, set[str]]] = {}
	narrowed: set[str] = set()
	handled: set[str] = set()
	out_of_namespace: set[str] = set()
	no_match: set[str] = set()
	for git_path in candidates:
	yaml_path = self.yaml_index.git_path_to_yaml_key(git_path)
	if yaml_path is None:
	base = git_path.rsplit("/", 1)[-1]
	if base.startswith("test_") and base.endswith(".py"):
	# Standalone test_*.py not referenced by any L0 YAML —
	# pytest doesn't auto-import test files into other
	# tests, so this file's edits can't affect what L0
	# runs. Claim as noop contribution.
	handled.add(git_path)
	no_match.add(git_path)
	continue
	# conftest / __init__ / helper / data — could impact
	# selection via implicit pytest discovery; let Selector
	# report it as Unhandled → fallback.
	out_of_namespace.add(git_path)
	continue
	handled.add(git_path)
	anchors = self._compute_anchors(git_path, yaml_path, pr.diffs.get(git_path, ""))
	def apply(self, pr: PRInputs) -> Optional[RuleResult]:
	candidates = [f for f in pr.changed_files if f.startswith("tests/")]
	if not candidates:
	return None
	block_filters: dict[tuple[str, int], dict[str, set[str]]] = {}
	narrowed: set[str] = set()
	handled: set[str] = set()
	out_of_namespace: set[str] = set()
	no_match: set[str] = set()
	for git_path in candidates:
	if _is_out_of_scope(git_path):
	continue
	yaml_path = self.yaml_index.git_path_to_yaml_key(git_path)
	if yaml_path is None:
	base = git_path.rsplit("/", 1)[-1]
	if base.startswith("test_") and base.endswith(".py"):
	# Standalone test_*.py not referenced by any L0 YAML —
	# pytest doesn't auto-import test files into other
	# tests, so this file's edits can't affect what L0
	# runs. Claim as noop contribution.
	handled.add(git_path)
	no_match.add(git_path)
	continue
	# conftest / __init__ / helper / data — could impact
	# selection via implicit pytest discovery; let Selector
	# report it as Unhandled → fallback.
	out_of_namespace.add(git_path)
	continue
	handled.add(git_path)
	anchors = self._compute_anchors(git_path, yaml_path, pr.diffs.get(git_path, ""))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/rules/tests_def_rule.py` around lines 218 - 247, In
apply(), markdown docs under test YAML trees are being processed into actionable
scopes; before calling
yaml_index.git_path_to_yaml_key/_compute_anchors/_combine_scopes, detect
markdown (e.g., git_path.endswith(".md") or basename == "README.md") and treat
them as noop by adding the path to handled and no_match (or out_of_namespace
depending on existing semantics) and continue, so markdown files do not produce
testdefonly results; update the branch that currently handles yaml_path != None
to early-skip .md files before calling _compute_anchors/_combine_scopes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast when snapshot files are unavailable.

The current check=False + unconditional write_text(...stdout) path can silently create empty snapshots, which can produce misleading CBTS decisions instead of a clear replay failure.

Suggested fix

 def _snapshot_test_db(repo: Path, sha: str, dst: Path) -> Path:
     out = _git(repo, "ls-tree", "-r", "--name-only", sha, "--", TEST_DB_REL, check=False)
     files = [ln for ln in out.stdout.splitlines() if ln.endswith(".yml")]
     db = dst / "test-db"
     db.mkdir(parents=True, exist_ok=True)
     for f in files:
-        (db / Path(f).name).write_text(_git(repo, "show", f"{sha}:{f}", check=False).stdout)
+        shown = _git(repo, "show", f"{sha}:{f}", check=False)
+        if shown.returncode != 0:
+            raise subprocess.CalledProcessError(
+                shown.returncode, shown.args, output=shown.stdout, stderr=shown.stderr
+            )
+        (db / Path(f).name).write_text(shown.stdout)
     return db


 def _snapshot_groovy(repo: Path, sha: str, dst: Path) -> Path:
     p = dst / "L0_Test.groovy"
-    p.write_text(_git(repo, "show", f"{sha}:{GROOVY_REL}", check=False).stdout)
+    shown = _git(repo, "show", f"{sha}:{GROOVY_REL}", check=False)
+    if shown.returncode != 0:
+        raise subprocess.CalledProcessError(
+            shown.returncode, shown.args, output=shown.stdout, stderr=shown.stderr
+        )
+    p.write_text(shown.stdout)
     return p

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/tools/dryrun.py` around lines 100 - 113, The snapshot
helpers (_snapshot_test_db and _snapshot_groovy) currently call _git with
check=False and unconditionally write .stdout, which can create empty/missing
snapshot files; change them to fail fast by checking the _git result (use
check=True or validate result.returncode and result.stdout) and raise/abort if
the git show/ls-tree calls failed or returned empty content for any expected
file; specifically validate the outputs from _git(repo, "ls-tree", ..., sha,
"--", TEST_DB_REL) and each _git(repo, "show", f"{sha}:{f}") in
_snapshot_test_db, and validate the _git(repo, "show", f"{sha}:{GROOVY_REL}")
call in _snapshot_groovy, and only write files when the calls succeeded and
stdout is non-empty, otherwise raise an error so snapshot creation fails fast.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle stale directories during per-label cleanup.

old.unlink() will raise on directories. With --keep-stale, this can crash replay for a label if any subdirectory exists from prior runs.

Suggested fix

     for old in pr_dir.iterdir():
         if old.name != "summary.txt":
-            old.unlink()
+            if old.is_dir():
+                shutil.rmtree(old)
+            else:
+                old.unlink()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/tools/dryrun.py` around lines 252 - 255, The loop over
pr_dir.iterdir() uses old.unlink() which raises on directories and causes
crashes when --keep-stale leaves subdirectories; update the cleanup in dryrun.py
to detect whether old is a file/symlink or a directory (using
old.is_dir()/old.is_file()/old.is_symlink()) and call the appropriate remover
(unlink for files/symlinks, shutil.rmtree for directories) or otherwise skip
non-removable entries so per-label cleanup in the for loop does not raise;
adjust imports if needed and keep the rest of the logic (the pr_dir.iterdir()
loop and subsequent shared_out assignment) unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate numeric CLI bounds at parse time.

--window and --limit currently accept invalid values (e.g., --window 0, negative values), which can cause runtime failures or surprising selection behavior.

Suggested fix

+def _positive_int(value: str) -> int:
+    ivalue = int(value)
+    if ivalue <= 0:
+        raise argparse.ArgumentTypeError("must be > 0")
+    return ivalue
+
+
+def _non_negative_int(value: str) -> int:
+    ivalue = int(value)
+    if ivalue < 0:
+        raise argparse.ArgumentTypeError("must be >= 0")
+    return ivalue
+
+
 def main(argv: Optional[list[str]] = None) -> int:
@@
-    ap.add_argument(
-        "--window", type=int, default=500, help="commits back from --ref (default: 500)"
-    )
+    ap.add_argument(
+        "--window", type=_positive_int, default=500, help="commits back from --ref (default: 500)"
+    )
@@
-    ap.add_argument("--limit", type=int, default=None, help="cap commits after filter")
+    ap.add_argument("--limit", type=_non_negative_int, default=None, help="cap commits after filter")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jenkins/scripts/cbts/tools/dryrun.py` around lines 314 - 323, The CLI
currently allows invalid numeric values for --window and --limit; update the
ap.add_argument calls for "--window" and "--limit" to validate at parse time by
using custom argparse types (or a validator) that raise
argparse.ArgumentTypeError for invalid inputs: implement a positive_int(value)
used for --window that ensures int(value) > 0, and use a
positive_int_or_none(value) (or accept None via default and validate if
provided) for --limit to ensure int(value) > 0 when not None; replace the
current type=int usages in the ap.add_argument calls and/or add explicit
argparse validation so invalid or negative values are rejected during parsing.