diff --git a/README.md b/README.md index ef3c100..af3c8a6 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ Review mode does not need `contents: write`: PR-specific generated files are sto | `github_token` | both | `${{ github.token }}` | Token for GitHub API calls; in review mode it posts or updates the PR comment. | | `push_token` | sync | `${{ github.token }}` | Token used for sync-mode pushes to `target_branch`. The workflow token can push when the workflow grants `permissions: contents: write`. Separate from `github_token` so commenting can use a GitHub App token while the push uses the workflow token. | | `codeboarding_version` | both | `0.12.3` | CodeBoarding PyPI package version used as the analysis engine. Pin for reproducibility. | -| `depth_level` | both | empty (`2` for cold starts) | Analysis depth, 1 to 3, used for first analysis and `force_full` rebuilds. Once `.codeboarding/analysis.json` exists, its `metadata.depth_level` is the source of truth for incremental analysis and fallback-full recovery. | +| `depth_level` | both | empty (`2` for cold starts) | Analysis depth for first analysis and `force_full` rebuilds. Max depends on tier: **3** on the free hosted tier, **10** with a CodeBoarding license or your own `llm_api_key`. Once `.codeboarding/analysis.json` exists, its `metadata.depth_level` is the source of truth: sync runs incremental at the baseline depth, and review analyzes the PR head at the committed baseline depth so the diff is apples-to-apples (clamped to the tier max). | | `render_depth` | review | `1` | Display depth for the PR diagram. Keep `1` for a clean top-level view. | | `diagram_direction` | review | `LR` | Mermaid direction: `LR`, `TD`, `TB`, `RL`, or `BT`. | | `changed_only` | review | `false` | Render only changed components and incident edges. | diff --git a/action.yml b/action.yml index 4aad166..2e96013 100644 --- a/action.yml +++ b/action.yml @@ -36,7 +36,7 @@ inputs: required: false default: '0.12.3' depth_level: - description: 'Analysis depth (1-3) for cold-start or force_full rebuilds. Once .codeboarding/analysis.json exists, its metadata.depth_level is the source of truth for incremental analysis and fallback-full recovery. Empty (default): 2 for cold starts.' + description: 'Analysis depth for cold-start or force_full rebuilds. Max depends on tier: 3 on the free hosted tier, 10 with a CodeBoarding license or your own llm_api_key. Once .codeboarding/analysis.json exists, its metadata.depth_level is the source of truth: sync runs incremental at the baseline depth, and review analyzes the PR head at the committed baseline depth so the diff is apples-to-apples (clamped to the tier max). Empty (default): 2 for cold starts.' required: false default: '' agent_model: @@ -209,9 +209,12 @@ runs: review|sync) ;; *) echo "::error::mode must be 'review' or 'sync' (got '$MODE')."; exit 1 ;; esac + # Structural range only (1-10). The per-tier ceiling (free=3, licensed=10) + # is enforced later in 'Resolve analysis depth', once the credential mode + # is known, with a message pointing at the license/BYO-key upgrade path. case "$DEPTH" in - ''|1|2|3) ;; - *) echo "::error::depth_level must be 1, 2, or 3 (empty = default cold-start depth 2)."; exit 1 ;; + ''|1|2|3|4|5|6|7|8|9|10) ;; + *) echo "::error::depth_level must be an integer from 1 to 10 (empty = default cold-start depth 2; the usable max depends on your tier)."; exit 1 ;; esac echo "mode=$MODE" >> "$GITHUB_OUTPUT" @@ -468,25 +471,6 @@ runs: git cat-file -e "$BASE_SHA" && echo "Target branch commit reachable." || \ (echo "::error::Target branch commit $BASE_SHA is not reachable." && exit 1) - - name: Resolve analysis depth - id: resolve_depth - if: steps.guard.outputs.skip != 'true' - shell: bash - env: - INPUT_DEPTH: ${{ inputs.depth_level }} - run: | - set -euo pipefail - # Explicit input controls cold-start/force_full rebuilds. Existing - # analysis.json metadata is the source of truth for incremental runs. - if [ -n "$INPUT_DEPTH" ]; then - echo "depth=$INPUT_DEPTH" >> "$GITHUB_OUTPUT" - echo "Using explicit depth_level=$INPUT_DEPTH." - exit 0 - fi - DEPTH=2 - echo "depth=$DEPTH" >> "$GITHUB_OUTPUT" - echo "Using default cold-start depth_level=$DEPTH." - - name: Set up Python 3.12 if: steps.guard.outputs.skip != 'true' uses: actions/setup-python@v5 @@ -704,6 +688,70 @@ runs: echo "cache_parsing_model=$(_safe "${PARSING_MODEL:-default}")" } >> "$GITHUB_OUTPUT" + # Runs AFTER the LLM-key step so the credential mode (byokey/license/oidc) is + # known: it sets the per-tier depth ceiling (free = 3, licensed/BYO = 10). The + # CodeBoarding package is installed by now, so baseline-depth runs against the + # set-up python (stdlib-only parse either way). + - name: Resolve analysis depth + id: resolve_depth + if: steps.guard.outputs.skip != 'true' + shell: bash + working-directory: target-repo + env: + INPUT_DEPTH: ${{ inputs.depth_level }} + MODE: ${{ steps.guard.outputs.mode }} + BASE_SHA: ${{ steps.guard.outputs.base_sha }} + EMPTY_BASE: ${{ steps.guard.outputs.empty_base }} + ACTION_PATH: ${{ github.action_path }} + CRED_MODE: ${{ steps.llm.outputs.mode }} + run: | + set -euo pipefail + # Licensed = BYO key or a CodeBoarding license; the free OIDC tier is capped lower. + if [ "$CRED_MODE" = "byokey" ] || [ "$CRED_MODE" = "license" ]; then + LICENSED_FLAG="--licensed"; MAX_DEPTH=10; TIER="licensed" + else + LICENSED_FLAG=""; MAX_DEPTH=3; TIER="free-tier" + fi + # Explicit input controls cold-start/force_full rebuilds. It is capped to the + # tier ceiling (an over-cap request is a clear error, not a silent clamp). + if [ -n "$INPUT_DEPTH" ]; then + if [ "$INPUT_DEPTH" -gt "$MAX_DEPTH" ]; then + echo "::error::depth_level=$INPUT_DEPTH exceeds the $TIER maximum of $MAX_DEPTH. Use a CodeBoarding license or your own llm_api_key for deeper analysis." + exit 1 + fi + echo "depth=$INPUT_DEPTH" >> "$GITHUB_OUTPUT" + echo "Using explicit depth_level=$INPUT_DEPTH (max $MAX_DEPTH for $TIER)." + exit 0 + fi + # Review against a committed baseline: analyze the PR head at the SAME + # depth the committed .codeboarding/analysis.json was generated with, so + # head and base diff apples-to-apples. Defaulting to a shallower depth + # would make validate-base reject the deeper baseline, force the action to + # regenerate a shallower base, and (because the artifact then carries no + # committed base SHA) leave the webview diffing the deeper committed base + # against the shallower head — reporting phantom "deleted" components. + # baseline-depth clamps the inherited value to the tier ceiling and only + # parses the committed JSON (stdlib). + if [ "$MODE" = "review" ] && [ "$EMPTY_BASE" != "true" ] && [ -n "$BASE_SHA" ]; then + BASE_ANALYSIS="$(mktemp)" + if git show "${BASE_SHA}:.codeboarding/analysis.json" > "$BASE_ANALYSIS" 2>/dev/null; then + INHERITED="$(python3 "$ACTION_PATH/scripts/engine_adapter.py" baseline-depth --analysis "$BASE_ANALYSIS" $LICENSED_FLAG | sed -n 's/^depth_level=//p')" + rm -f "$BASE_ANALYSIS" + if [ -n "$INHERITED" ]; then + echo "depth=$INHERITED" >> "$GITHUB_OUTPUT" + echo "Inheriting committed baseline depth_level=$INHERITED for the PR-head analysis (max $MAX_DEPTH for $TIER)." + exit 0 + fi + echo "Committed baseline has no usable depth_level; using default cold-start depth." + else + rm -f "$BASE_ANALYSIS" + echo "No committed baseline at ${BASE_SHA}; using default cold-start depth." + fi + fi + DEPTH=2 + echo "depth=$DEPTH" >> "$GITHUB_OUTPUT" + echo "Using default cold-start depth_level=$DEPTH." + - name: Resolve base analysis (committed baseline) if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' id: base @@ -933,6 +981,7 @@ runs: BASELINE_SHA: ${{ steps.base.outputs.baseline_sha }} HEAD_SHA: ${{ steps.guard.outputs.head_sha }} EMPTY_BASE: ${{ steps.base.outputs.empty_base }} + CRED_MODE: ${{ steps.llm.outputs.mode }} run: | # Export the key under the selected provider's env var (only this one), # so the engine auto-selects that provider. @@ -969,6 +1018,11 @@ runs: --target-ref "$HEAD_SHA" --source-sha "$HEAD_SHA" ) + # Raise the depth ceiling for licensed/BYO-key runs (controls the + # fallback-full depth when incremental can't run). + if [ "$CRED_MODE" = "byokey" ] || [ "$CRED_MODE" = "license" ]; then + head_args+=(--licensed) + fi if [ "$EMPTY_BASE" = "true" ]; then head_args+=(--force-full) fi @@ -1131,6 +1185,7 @@ runs: TARGET_SHA: ${{ steps.guard.outputs.target_sha }} DEPTH: ${{ steps.resolve_depth.outputs.depth }} FORCE_FULL: ${{ inputs.force_full }} + CRED_MODE: ${{ steps.llm.outputs.mode }} run: | set -euo pipefail # Export the key under the selected provider's env var (only this one), @@ -1160,6 +1215,9 @@ runs: --source-sha "$TARGET_SHA" --depth "$DEPTH" ) + if [ "$CRED_MODE" = "byokey" ] || [ "$CRED_MODE" = "license" ]; then + args+=(--licensed) + fi [ "$FORCE_FULL" = "true" ] && args+=(--force-full) LOG="${RUNNER_TEMP}/cb-sync-analyze.log" python "$ACTION_PATH/scripts/engine_adapter.py" analyze "${args[@]}" | tee "$LOG" diff --git a/scripts/engine_adapter.py b/scripts/engine_adapter.py index eb2f04d..190cfca 100644 --- a/scripts/engine_adapter.py +++ b/scripts/engine_adapter.py @@ -1,11 +1,12 @@ """CLI adapter between the action and the CodeBoarding analysis ENGINE. No analysis logic lives here. The engine is the published ``codeboarding`` PyPI -package installed by the action and imported lazily inside each function -(``codeboarding_workflows`` etc.); this module just turns the action's shell -steps into typed, tested calls into it. The lazy imports mean this file imports -fine without the package present — the tests stub those modules and assert we -call the engine with the right args. +package installed by the action (``codeboarding_workflows`` etc.); this module +just turns the action's shell steps into typed, tested calls into it. The engine +imports are best-effort at module load, so this file imports fine without the +package present — the metadata-only subcommands (``baseline-info``, +``baseline-depth``, ``validate-base``) run with the stdlib alone, and the tests +stub the engine modules to assert we call the engine with the right args. Subcommands (all paths/refs come in as argv, never interpolated into source): @@ -15,6 +16,7 @@ health --artifact-dir D --repo P --name N --issues-out FILE validate-base --analysis F --expected-sha SHA [--expected-depth K] baseline-info --analysis F + baseline-depth --analysis F analyze --repo P --out D --name N --run-id ID --source-sha SHA --depth K [--force-full] render --analysis F --out D --repo-name N --repo-ref R [--format .md] concat --docs-dir D --out F @@ -58,14 +60,26 @@ import os import re import shutil +import sys from pathlib import Path -from codeboarding_workflows.analysis import BaselineUnavailableError, run_full, run_incremental -from codeboarding_workflows.rendering import render_docs -from diagram_analysis.exceptions import IncrementalCacheMissingError -from static_analyzer import get_static_analysis -from static_analyzer.analysis_cache import StaticAnalysisCache -from static_analyzer.cluster_helpers import build_all_cluster_results +# The engine packages are imported best-effort so the metadata-only subcommands +# (``baseline-info``, ``baseline-depth``, ``validate-base``) run with the stdlib +# alone — they parse a committed analysis.json and never touch the engine. The +# action invokes them BEFORE the engine package is pip-installed (e.g. while +# resolving the review depth), so a hard import here would break that step. The +# analysis subcommands that DO need the engine fail loudly when these are None. +try: + from codeboarding_workflows.analysis import BaselineUnavailableError, run_full, run_incremental + from codeboarding_workflows.rendering import render_docs + from diagram_analysis.exceptions import IncrementalCacheMissingError + from static_analyzer import get_static_analysis + from static_analyzer.analysis_cache import StaticAnalysisCache + from static_analyzer.cluster_helpers import build_all_cluster_results +except Exception: # engine package not installed (metadata-only subcommands don't need it) + BaselineUnavailableError = IncrementalCacheMissingError = _MissingEngine = type("_MissingEngine", (Exception,), {}) + run_full = run_incremental = render_docs = None + get_static_analysis = StaticAnalysisCache = build_all_cluster_results = None try: from health.models import Severity @@ -80,6 +94,18 @@ _SHA_RE = re.compile(r"^[0-9a-f]{7,40}$") _DEFAULT_DEPTH = 2 +# Per-tier ceiling on analysis depth. The engine has no hard cap (depth_level is +# just the abstraction-expansion bound), so the limit is a product decision: +# the free hosted tier is capped to keep per-run cost bounded; licensed/BYO-key +# users (who pay for their own tokens or hold a license) get a much higher ceiling. +_FREE_MAX_DEPTH = 3 +_LICENSED_MAX_DEPTH = 10 + + +def _max_depth(licensed: bool) -> int: + return _LICENSED_MAX_DEPTH if licensed else _FREE_MAX_DEPTH + + # On the free hosted tier the proxy returns HTTP 402 with this message when the # repo owner's weekly token budget is spent. We detect it so the action can post # a helpful "add a key/license" comment instead of a generic failure. @@ -149,17 +175,43 @@ def _metadata_depth(metadata: dict) -> int | None: return None -def _supported_depth(metadata: dict) -> int | None: - depth = _metadata_depth(metadata) - return depth if depth in range(1, 4) else None - +def _resolve_depth(metadata: dict, licensed: bool, default_depth: int = _DEFAULT_DEPTH) -> int: + """Resolve a usable analysis depth from a committed baseline's metadata. -def _analysis_depth_or_default(output_dir: Path, default_depth: int = _DEFAULT_DEPTH) -> int: + Always returns a number in [1, tier-max] — never None. Each case is logged + with its exact condition: + * missing/unparseable/non-positive depth_level -> the default cold-start + depth (every "not a usable depth" spec violation is treated the same); + * a depth above the tier ceiling -> clamped down to the ceiling. + The tier ceiling is the free cap for unlicensed runs and a much higher cap + for licensed/BYO-key runs (see ``_max_depth``). A valid in-range depth passes + through unchanged. + """ + cap = _max_depth(licensed) + depth = _metadata_depth(metadata) + # Diagnostics go to stderr so stdout stays a clean machine-readable channel + # (the ``baseline-depth`` subcommand prints only ``depth_level=`` to stdout). + if depth is None: + print( + f"Baseline metadata.depth_level is missing/unparseable; using default depth {default_depth}.", + file=sys.stderr, + ) + return min(default_depth, cap) + if depth < 1: + print(f"Baseline depth_level {depth} is not positive; using default depth {default_depth}.", file=sys.stderr) + return min(default_depth, cap) + if depth > cap: + tier = "licensed" if licensed else "free-tier" + print(f"Baseline depth_level {depth} exceeds the {tier} max {cap}; clamping to {cap}.", file=sys.stderr) + return cap + return depth + + +def _analysis_depth_or_default(output_dir: Path, licensed: bool, default_depth: int = _DEFAULT_DEPTH) -> int: metadata = _load_metadata(output_dir / "analysis.json") if not isinstance(metadata, dict): - return default_depth - depth = _supported_depth(metadata) - return depth if depth is not None else default_depth + return min(default_depth, _max_depth(licensed)) + return _resolve_depth(metadata, licensed, default_depth) def _metadata_commit(metadata: dict) -> str: @@ -180,6 +232,24 @@ def baseline_info(analysis_path: Path) -> str: return commit if _SHA_RE.match(commit) else "" +def baseline_depth(analysis_path: Path, licensed: bool) -> int | None: + """Return the depth to analyze the PR head at, inherited from the committed + baseline and clamped to the tier ceiling — or None when there is NO committed + baseline at all (genuine cold start; the caller then uses its own default). + + Review mode uses this (via the ``baseline-depth`` subcommand) so the head is + analyzed at the SAME depth as the base it is diffed against (apples-to-apples). + A present-but-out-of-range or unparseable depth is clamped/defaulted (and + logged) by ``_resolve_depth`` rather than rejected, so review never silently + shallows a usable baseline. Parsing lives here so the action shell never reads + the JSON inline (mirrors ``baseline_info``). + """ + metadata = _load_metadata(analysis_path) + if not isinstance(metadata, dict) or not metadata: + return None # no baseline / no metadata → cold start, caller defaults + return _resolve_depth(metadata, licensed) + + def validate_base_analysis( analysis_path: Path, expected_sha: str, expected_depth: int | None = None ) -> tuple[bool, str]: @@ -198,6 +268,12 @@ def validate_base_analysis( expands persists depth_level 1 — rejecting that would force a full regeneration on every PR without ever converging. A missing or unparseable depth_level is accepted — legacy baselines predate the field. + + Review now derives ``expected_depth`` from the committed baseline's own + depth_level (via the ``baseline-depth`` subcommand), so the deeper-than-expected + rejection no longer fires for the normal case — head and base are analyzed at + the same depth. The rejection remains a safety net for an explicit + ``depth_level`` input that is shallower than the committed baseline. """ try: data = json.loads(analysis_path.read_text(encoding="utf-8")) @@ -283,6 +359,7 @@ def _incremental_or_full( target_ref: str, source_sha: str, log_name: str, + licensed: bool, ) -> str: """Run incremental from *base_ref*; on a cache/baseline miss, clear the output dir and run a full analysis. Returns "incremental" or "full". @@ -307,7 +384,7 @@ def _incremental_or_full( except (IncrementalCacheMissingError, BaselineUnavailableError) as exc: print(f"Incremental unavailable ({exc}); running full analysis.") out_path = Path(output_dir) - fallback_depth = _analysis_depth_or_default(out_path, depth) + fallback_depth = _analysis_depth_or_default(out_path, licensed, depth) _clear_dir(out_path) res = run_full( repo_name=repo_name, @@ -332,6 +409,7 @@ def run_head( target_ref: str, source_sha: str, force_full: bool = False, + licensed: bool = False, ) -> None: """Review PR head: incremental from the PR target branch tip, full on a cache miss. @@ -364,6 +442,7 @@ def run_head( target_ref=target_ref, source_sha=source_sha, log_name="cb-head.log", + licensed=licensed, ) print(f"head_analysis_mode={mode}") @@ -376,11 +455,19 @@ def run_analyze( source_sha: str, depth: int, force_full: bool = False, + licensed: bool = False, ) -> str: """Sync analysis: incremental from the committed baseline, full when the - baseline is missing/unparseable or ``force_full`` is set. Prints - ``analysis_mode=full|incremental`` on stdout — the action greps that line, - so it must be printed exactly once per run. + baseline is absent or untrusted (no commit_hash) or ``force_full`` is set. + Prints ``analysis_mode=full|incremental`` on stdout — the action greps that + line, so it must be printed exactly once per run. + + The baseline's depth_level is resolved (clamped/defaulted, never None) for the + incremental run; whether to do a full rebuild is decided by trustworthiness + (commit_hash), NOT by the depth. The engine's incremental path needs a usable + cache + base commit, not a particular depth, and falls back to full itself if + the cache is missing — so a present baseline with an odd depth_level still runs + incremental at a sane depth rather than forcing a costly full rebuild. """ out_path = Path(output_dir) @@ -401,15 +488,13 @@ def full(reason: str, analysis_depth: int) -> str: return "full" if force_full: - return full("Full analysis forced (force_full)", depth) + return full("Full analysis forced (force_full)", min(depth, _max_depth(licensed))) metadata = _load_metadata(out_path / "analysis.json") if metadata is None: - return full("No baseline analysis.json found", depth) + return full("No baseline analysis.json found", min(depth, _max_depth(licensed))) - baseline_depth = _supported_depth(metadata) - if baseline_depth is None: - return full("Baseline metadata.depth_level is missing", _DEFAULT_DEPTH) + baseline_depth = _resolve_depth(metadata, licensed) base_ref = _metadata_commit(metadata) if not base_ref: @@ -425,6 +510,7 @@ def full(reason: str, analysis_depth: int) -> str: target_ref=source_sha, source_sha=source_sha, log_name="cb-sync.log", + licensed=licensed, ) print(f"analysis_mode={mode}") return mode @@ -520,10 +606,16 @@ def main(argv=None) -> int: p = argparse.ArgumentParser(description=__doc__) sub = p.add_subparsers(dest="cmd", required=True) + # The structural ceiling is the licensed max; the per-run tier cap (free vs + # licensed) is enforced by the action guard (explicit input) and _resolve_depth + # (inherited baseline), so a too-deep value is rejected or clamped, not silently + # truncated by argparse. + depth_choices = range(1, _LICENSED_MAX_DEPTH + 1) + b = sub.add_parser("base") for a in ("--repo", "--out", "--name", "--run-id", "--source-sha"): b.add_argument(a, required=True) - b.add_argument("--depth", required=True, type=int, choices=range(1, 4)) + b.add_argument("--depth", required=True, type=int, choices=depth_choices) s = sub.add_parser("seed") for a in ("--repo", "--out", "--source-sha"): @@ -532,7 +624,8 @@ def main(argv=None) -> int: h = sub.add_parser("head") for a in ("--repo", "--out", "--name", "--run-id", "--base-ref", "--target-ref", "--source-sha"): h.add_argument(a, required=True) - h.add_argument("--depth", required=True, type=int, choices=range(1, 4)) + h.add_argument("--depth", required=True, type=int, choices=depth_choices) + h.add_argument("--licensed", action="store_true", help="Licensed/BYO-key run (raises the depth ceiling).") h.add_argument("--force-full", action="store_true", help="Run a full PR-head analysis instead of incremental.") hc = sub.add_parser("health") @@ -542,15 +635,20 @@ def main(argv=None) -> int: vb = sub.add_parser("validate-base") vb.add_argument("--analysis", required=True) vb.add_argument("--expected-sha", required=True) - vb.add_argument("--expected-depth", type=int, choices=range(1, 4)) + vb.add_argument("--expected-depth", type=int, choices=depth_choices) bi = sub.add_parser("baseline-info") bi.add_argument("--analysis", required=True) + bd = sub.add_parser("baseline-depth") + bd.add_argument("--analysis", required=True) + bd.add_argument("--licensed", action="store_true", help="Licensed/BYO-key run (raises the depth ceiling).") + an = sub.add_parser("analyze") for a in ("--repo", "--out", "--name", "--run-id", "--source-sha"): an.add_argument(a, required=True) - an.add_argument("--depth", required=True, type=int, choices=range(1, 4)) + an.add_argument("--depth", required=True, type=int, choices=depth_choices) + an.add_argument("--licensed", action="store_true", help="Licensed/BYO-key run (raises the depth ceiling).") an.add_argument("--force-full", action="store_true", help="Ignore any committed baseline and run a full analysis.") rn = sub.add_parser("render") @@ -582,6 +680,7 @@ def main(argv=None) -> int: args.source_sha, # action.yml adds --force-full for EMPTY_BASE PRs (no comparison baseline). args.force_full, + args.licensed, ) elif args.cmd == "health": Path(args.issues_out).write_text(str(run_health(args.artifact_dir, args.repo, args.name))) @@ -591,8 +690,13 @@ def main(argv=None) -> int: return 0 if ok else 1 elif args.cmd == "baseline-info": print(f"commit_hash={baseline_info(Path(args.analysis))}") + elif args.cmd == "baseline-depth": + depth = baseline_depth(Path(args.analysis), args.licensed) + print(f"depth_level={depth if depth is not None else ''}") elif args.cmd == "analyze": - run_analyze(args.repo, args.out, args.name, args.run_id, args.source_sha, args.depth, args.force_full) + run_analyze( + args.repo, args.out, args.name, args.run_id, args.source_sha, args.depth, args.force_full, args.licensed + ) elif args.cmd == "render": run_render(args.analysis, args.out, args.repo_name, args.repo_ref, args.format) elif args.cmd == "concat": diff --git a/tests/test_engine_adapter.py b/tests/test_engine_adapter.py index 78f4fcc..ad64fec 100644 --- a/tests/test_engine_adapter.py +++ b/tests/test_engine_adapter.py @@ -189,7 +189,9 @@ def test_main_sets_github_action_source(self): self.assertEqual(os.environ["CODEBOARDING_SOURCE"], "github_action") def test_main_rejects_invalid_depth(self): - for depth in ("0", "4", "x"): + # argparse enforces the structural range 1-10; the per-tier cap (free=3) + # is applied later by the action/resolver, not here. + for depth in ("0", "11", "x"): with self.subTest(depth=depth): with redirect_stderr(StringIO()): with self.assertRaises(SystemExit): @@ -211,6 +213,30 @@ def test_main_rejects_invalid_depth(self): ] ) + def test_main_accepts_depth_four(self): + # The action's accepted depth ceiling is 4 so a committed depth-4 baseline + # is a first-class value review can inherit (the engine has no depth cap). + rf = _Rec() + self._install(run_full=rf) + engine_adapter.main( + [ + "base", + "--repo", + "/repo", + "--out", + "/out", + "--name", + "myrepo", + "--run-id", + "rid-base", + "--depth", + "4", + "--source-sha", + "abc123", + ] + ) + self.assertEqual(rf.calls[0]["depth_level"], 4) + def test_head_uses_incremental(self): ri, rf = _Rec(), _Rec() self._install(run_full=rf, run_incremental=ri) @@ -497,6 +523,25 @@ def test_validate_base_without_expected_depth_ignores_depth(self): self.assertTrue(ok) self.assertIn("matches", message) + def test_validate_base_accepts_depth_four_baseline(self): + # The core fix: review inherits the committed baseline's depth, so a + # depth-4 baseline validated at --expected-depth 4 is accepted (reused, + # not regenerated). Validated at a shallower expected depth it is still + # rejected (an explicit shallower depth_level input). + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "analysis.json" + path.write_text( + json.dumps({"metadata": {"commit_hash": "abc123", "depth_level": 4}}), + encoding="utf-8", + ) + + ok_same, _ = engine_adapter.validate_base_analysis(path, "abc123", expected_depth=4) + ok_shallower, message = engine_adapter.validate_base_analysis(path, "abc123", expected_depth=2) + + self.assertTrue(ok_same) + self.assertFalse(ok_shallower) + self.assertIn("deeper", message) + def test_main_validate_base_expected_depth_exit_codes(self): # patch.dict: main() setdefaults CODEBOARDING_SOURCE; don't leak it. with patch.dict(os.environ), tempfile.TemporaryDirectory() as tmp: @@ -518,10 +563,18 @@ def test_main_validate_base_expected_depth_exit_codes(self): ), 1, ) + # depth 4 is now an accepted value (against a depth-2 baseline a + # shallower-or-equal expected depth passes the depth check). + self.assertEqual( + engine_adapter.main( + ["validate-base", "--analysis", str(path), "--expected-sha", "abc123", "--expected-depth", "4"] + ), + 0, + ) with redirect_stderr(StringIO()): - with self.assertRaises(SystemExit): # depth outside 1-3 rejected by argparse + with self.assertRaises(SystemExit): # depth outside 1-10 rejected by argparse engine_adapter.main( - ["validate-base", "--analysis", str(path), "--expected-sha", "abc123", "--expected-depth", "4"] + ["validate-base", "--analysis", str(path), "--expected-sha", "abc123", "--expected-depth", "11"] ) diff --git a/tests/test_sync_subcommands.py b/tests/test_sync_subcommands.py index 6b02f6e..bceec64 100644 --- a/tests/test_sync_subcommands.py +++ b/tests/test_sync_subcommands.py @@ -5,6 +5,7 @@ import json import os +import subprocess import sys import tempfile import types @@ -191,6 +192,39 @@ def test_deeper_baseline_still_runs_incremental(self): self.assertTrue((out / "stale.json").exists()) self.assertTrue((out / "health").exists()) + def test_deep_baseline_runs_incremental_regardless_of_tier(self): + # A committed depth-7 baseline still runs incremental (the depth value + # doesn't gate incremental — only commit_hash does); on the free tier the + # depth is clamped for any eventual run, but incremental is unaffected. + rf, ri = _Rec(), _Rec() + self._install(run_full=rf, run_incremental=ri) + out = Path(tempfile.mkdtemp()) + _write_analysis(out, commit="metadata-base", depth=7) + + mode = engine_adapter.run_analyze("/repo", str(out), "myrepo", "rid", "head123", 2) + + self.assertEqual(mode, "incremental") + self.assertEqual(len(rf.calls), 0) + self.assertEqual(len(ri.calls), 1) + + def test_over_cap_baseline_depth_clamped_on_full_fallback(self): + # When a full rebuild happens (here: baseline has no commit_hash), the + # resolved depth is clamped to the tier ceiling: free clamps 7 -> 3, + # licensed keeps 7. + for licensed, expected in ((False, 3), (True, 7)): + with self.subTest(licensed=licensed): + rf, ri = _Rec(), _Rec() + self._install(run_full=rf, run_incremental=ri) + out = Path(tempfile.mkdtemp()) + out.joinpath("analysis.json").write_text( + json.dumps({"metadata": {"depth_level": 7}}), encoding="utf-8" # no commit_hash -> full + ) + + mode = engine_adapter.run_analyze("/repo", str(out), "myrepo", "rid", "head123", 2, licensed=licensed) + + self.assertEqual(mode, "full") + self.assertEqual(rf.calls[0][1]["depth_level"], expected) + def test_shallower_baseline_runs_incremental(self): # The engine records the depth REACHED, not requested: a depth-2 push on # a repo that never expands keeps writing depth_level 1, so a strict != @@ -206,7 +240,12 @@ def test_shallower_baseline_runs_incremental(self): self.assertEqual(len(rf.calls), 0) self.assertEqual(len(ri.calls), 1) - def test_missing_depth_in_baseline_runs_full_at_default_depth(self): + def test_missing_depth_with_valid_commit_runs_incremental(self): + # A missing/unparseable depth_level is no longer a reason to force a full + # rebuild: incremental needs a usable cache + base commit (commit_hash), + # not a particular depth. With a valid commit_hash we run incremental; + # the depth defaults to 2 for the run (and the engine itself falls back to + # full if the cache is actually absent). rf, ri = _Rec(), _Rec() self._install(run_full=rf, run_incremental=ri) out = Path(tempfile.mkdtemp()) @@ -216,10 +255,10 @@ def test_missing_depth_in_baseline_runs_full_at_default_depth(self): mode = engine_adapter.run_analyze("/repo", str(out), "myrepo", "rid", "head123", 3) - self.assertEqual(mode, "full") - self.assertEqual(len(rf.calls), 1) - self.assertEqual(len(ri.calls), 0) - self.assertEqual(rf.calls[0][1]["depth_level"], 2) + self.assertEqual(mode, "incremental") + self.assertEqual(len(rf.calls), 0) + self.assertEqual(len(ri.calls), 1) + self.assertEqual(ri.calls[0][1]["base_ref"], "metadata-base") def test_missing_commit_in_baseline_runs_full_at_baseline_depth(self): rf, ri = _Rec(), _Rec() @@ -369,7 +408,9 @@ def test_main_parses_depth_as_int_and_sets_sync_source(self): self.assertEqual(os.environ["CODEBOARDING_SOURCE"], "sync") def test_main_rejects_invalid_depth(self): - for depth in ("0", "4", "x"): + # argparse enforces the structural range 1-10; the per-tier cap is applied + # later by the action/resolver, not here. + for depth in ("0", "11", "x"): with self.subTest(depth=depth): with redirect_stderr(StringIO()): with self.assertRaises(SystemExit): @@ -524,5 +565,101 @@ def test_main_prints_empty_for_bad_baseline(self): self.assertNotIn("nope", buf.getvalue()) +class TestBaselineDepth(_Base): + """baseline-depth lets review inherit the committed baseline's depth_level + (clamped to the tier ceiling) so the PR head is analyzed at the same depth as + the base it is diffed against. It returns a usable number for any present + baseline, and None only when there is no baseline at all (cold start).""" + + def _write(self, metadata): + out = Path(tempfile.mkdtemp()) + (out / "analysis.json").write_text(json.dumps({"metadata": metadata}), encoding="utf-8") + return out / "analysis.json" + + def test_in_range_passes_through(self): + for depth in (1, 2, 3): # within the free cap + with self.subTest(depth=depth): + self.assertEqual(engine_adapter.baseline_depth(self._write({"depth_level": depth}), False), depth) + + def test_clamps_over_cap_per_tier(self): + # depth 4-10 exceed the free cap (3) -> clamp to 3; licensed cap is 10. + self.assertEqual(engine_adapter.baseline_depth(self._write({"depth_level": 7}), False), 3) + self.assertEqual(engine_adapter.baseline_depth(self._write({"depth_level": 7}), True), 7) + self.assertEqual(engine_adapter.baseline_depth(self._write({"depth_level": 4}), False), 3) + self.assertEqual(engine_adapter.baseline_depth(self._write({"depth_level": 4}), True), 4) + self.assertEqual(engine_adapter.baseline_depth(self._write({"depth_level": 99}), True), 10) + + def test_invalid_depth_uses_default(self): + # Every spec violation that isn't an over-cap clamp falls back to the + # default depth (2): a non-positive depth, an unparseable value, or a + # missing depth_level are all handled the same way. + for metadata in ( + {"depth_level": 0}, + {"depth_level": -3}, + {"depth_level": "x"}, + {"commit_hash": "deadbeef1234"}, + ): + with self.subTest(metadata=metadata): + self.assertEqual(engine_adapter.baseline_depth(self._write(metadata), False), 2) + + def test_none_only_when_no_baseline(self): + # No file, or an empty/no-metadata object -> cold start (caller defaults). + self.assertIsNone(engine_adapter.baseline_depth(Path(tempfile.mkdtemp()) / "absent.json", False)) + self.assertIsNone(engine_adapter.baseline_depth(self._write({}), False)) + + def test_main_prints_depth_line(self): + path = self._write({"depth_level": 3}) + buf = StringIO() + with patch.dict(os.environ, {}, clear=True), redirect_stdout(buf): + rc = engine_adapter.main(["baseline-depth", "--analysis", str(path)]) + self.assertEqual(rc, 0) + self.assertIn("depth_level=3", buf.getvalue()) + + def test_main_licensed_raises_ceiling(self): + path = self._write({"depth_level": 7}) + free, lic = StringIO(), StringIO() + with patch.dict(os.environ, {}, clear=True), redirect_stdout(free): + engine_adapter.main(["baseline-depth", "--analysis", str(path)]) + with patch.dict(os.environ, {}, clear=True), redirect_stdout(lic): + engine_adapter.main(["baseline-depth", "--analysis", str(path), "--licensed"]) + self.assertIn("depth_level=3", free.getvalue()) # clamped + self.assertIn("depth_level=7", lic.getvalue()) # within licensed cap + + def test_main_prints_empty_for_no_baseline(self): + buf = StringIO() + with patch.dict(os.environ, {}, clear=True), redirect_stdout(buf): + engine_adapter.main(["baseline-depth", "--analysis", str(Path(tempfile.mkdtemp()) / "absent.json")]) + self.assertIn("depth_level=", buf.getvalue()) + self.assertNotIn("depth_level=None", buf.getvalue()) + + def test_diagnostics_go_to_stderr_not_stdout(self): + # Clamp/default messages must not pollute the machine-readable stdout line. + path = self._write({"depth_level": 7}) + adapter = Path(__file__).resolve().parent.parent / "scripts" / "engine_adapter.py" + result = subprocess.run( + [sys.executable, str(adapter), "baseline-depth", "--analysis", str(path)], + capture_output=True, + text=True, + cwd=tempfile.mkdtemp(), + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip(), "depth_level=3") # stdout is JUST the value + self.assertIn("clamping to 3", result.stderr) # the log is on stderr + + def test_runs_without_engine_installed(self): + # The action calls baseline-depth BEFORE the engine package is installed, + # so it must work as a subprocess with no engine modules on sys.path. + path = self._write({"depth_level": 3}) + adapter = Path(__file__).resolve().parent.parent / "scripts" / "engine_adapter.py" + result = subprocess.run( + [sys.executable, str(adapter), "baseline-depth", "--analysis", str(path)], + capture_output=True, + text=True, + cwd=tempfile.mkdtemp(), # not the repo: no stub engine modules importable + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("depth_level=3", result.stdout) + + if __name__ == "__main__": unittest.main()