fix: review inherits committed baseline depth for the PR diff#45
fix: review inherits committed baseline depth for the PR diff#45Svilen-Stefanov wants to merge 1 commit into
Conversation
Review mode hardcoded the analysis depth to 2 when depth_level was unset, ignoring the committed .codeboarding/analysis.json's own depth_level. When the committed baseline was deeper (e.g. depth 4), validate-base rejected it as "deeper than expected", so the action regenerated a shallower depth-2 base, diffed head(2) vs that, and wrote base_commit_found=false / base_commit_sha=null into the PR artifact. The webview then fell back to diffing the depth-2 head against the committed depth-4 base, reporting hundreds of phantom "deleted" components for sub-trees that only differ by analysis depth. Review now inherits the committed baseline's depth_level (mirroring sync's run_analyze) via a new stdlib-only `baseline-depth` subcommand invoked before the engine is installed, so head and base are analyzed at the same depth, the committed baseline is reused, and the artifact carries a real base_commit_sha the webview diffs against. The accepted depth ceiling is raised 3 -> 4 (the engine has no depth cap) so a committed depth-4 baseline is a first-class value. - engine_adapter.py: best-effort engine imports so metadata-only subcommands run without the engine installed; add baseline_depth() + `baseline-depth` command; widen _supported_depth and argparse choices to include 4. - action.yml: resolve_depth inherits the committed baseline depth in review mode (sync unchanged); depth guard + input doc widened to 1-4. - README: depth_level doc widened to 1-4. - tests: baseline-depth coverage incl. engine-absent subprocess run; depth-4 accepted across base/head/analyze/validate-base; depth-5 now rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Architecture review · 9 components changedgraph LR
n_Analysis_Orchestrator["Analysis Orchestrator"]
n_Structural_Diffing_Engine["Structural Diffing Engine"]
n_Mermaid_Visualization_Engine["Mermaid Visualization Engine"]
n_UX_Integration_Layer["UX #38; Integration Layer"]
n_Analysis_Orchestrator -- "triggers change detection and impact analysis" --> n_Structural_Diffing_Engine
n_Analysis_Orchestrator -- "orchestrates diagram generation from analysis s…" --> n_Mermaid_Visualization_Engine
n_Structural_Diffing_Engine -- "provides structural diff data for rendering" --> n_Mermaid_Visualization_Engine
n_Mermaid_Visualization_Engine -- "provides rendered diagrams for GitHub presentat…" --> n_UX_Integration_Layer
n_UX_Integration_Layer -- "validates diagram integrity via health checks" --> n_Mermaid_Visualization_Engine
n_Analysis_Orchestrator -- "provides execution metadata for final reporting" --> n_UX_Integration_Layer
n_Mermaid_Visualization_Engine -- "queries component metadata" --> n_Structural_Diffing_Engine
n_UX_Integration_Layer -- "filters changes for presentation" --> n_Structural_Diffing_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_Analysis_Orchestrator,n_Structural_Diffing_Engine modified;
linkStyle 1,5 stroke:#0b5d23,stroke-width:2px;
linkStyle 0,2,3,4 stroke:#7d4e00,stroke-width:2px;
linkStyle 6,7 stroke:#82071e,stroke-width:2px,stroke-dasharray:5 3;
Colors indicate component changes compared to target branch Download the PR analysis artifacts from this workflow artifact. Analysis Orchestrator : 1 changed sub-component, 1 file changed
Structural Diffing Engine : 1 file changed
Explore this PR’s architecture in your browser or VS Code. codeboarding-action · run 27824583696 |
Svilen-Stefanov
left a comment
Svilen-Stefanov
left a comment
There was a problem hiding this comment.
@ivanmilevtues pls take a look at this when you can and lmk what you think about the depth and the consequences of it.
| 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=range(1, 5)) |
There was a problem hiding this comment.
@ivanmilevtues what do you think about this?
On the one hand, we want to cap the max depth so that the analysis doesn't explore in terms of time and cost, on the other, as you can see from this PR, if the manually created analysis is of a larger depth that the action doesn't support, we will still fall back to depth 2, which seems bad.
Should we remove these choices overall, increase them or what do you think?
There was a problem hiding this comment.
I think we should match this if the license field is provided, if it isn't we limit you to much less. (If someone tries to fake it, they will just burn their freemium)
| exit 0 | ||
| fi | ||
| echo "Committed baseline has no usable depth_level; using default cold-start depth." | ||
| else |
There was a problem hiding this comment.
Looking at the size of these action, I feel like we should probably refactor it all to make sure it's generally simpler.
There was a problem hiding this comment.
For me it is almost good to go I will set matching/unlimited depth if license is provided.
If someone tries to abuse with random licenses they will just burn their freemium. For freemium I will keep it as is up-to 3 depth)
Putting as reject so that you ping me for final call on this and I know what it is in the end.
| def _supported_depth(metadata: dict) -> int | None: | ||
| depth = _metadata_depth(metadata) | ||
| return depth if depth in range(1, 4) else None | ||
| return depth if depth in range(1, 5) else None |
There was a problem hiding this comment.
I would rather cap this if you are unlicensed to 4 the way it was.
Also let's default to a number (2) instead of None :D
| 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=range(1, 5)) |
There was a problem hiding this comment.
I think we should match this if the license field is provided, if it isn't we limit you to much less. (If someone tries to fake it, they will just burn their freemium)
2 participants
Review mode hardcoded the analysis depth to 2 when depth_level was unset, ignoring the committed .codeboarding/analysis.json's own depth_level. When the committed baseline was deeper (e.g. depth 4), validate-base rejected it as "deeper than expected", so the action regenerated a shallower depth-2 base, diffed head(2) vs that, and wrote base_commit_found=false / base_commit_sha=null into the PR artifact. The webview then fell back to diffing the depth-2 head against the committed depth-4 base, reporting hundreds of phantom "deleted" components for sub-trees that only differ by analysis depth.
Example PR: CodeBoarding/CodeBoarding#383
Review now inherits the committed baseline's depth_level (mirroring sync's run_analyze) via a new stdlib-only
baseline-depthsubcommand invoked before the engine is installed, so head and base are analyzed at the same depth, the committed baseline is reused, and the artifact carries a real base_commit_sha the webview diffs against. The accepted depth ceiling is raised 3 -> 4 (the engine has no depth cap) so a committed depth-4 baseline is a first-class value.baseline-depthcommand; widen _supported_depth and argparse choices to include 4.