Repository navigation
feat: report how the review base was built - #139
Svilen-Stefanov wants to merge 2 commits into
Conversation
A review's base comes from a saved artifact, a baseline committed at the merge base, or a full analysis in this run, and nothing said which. The slow path is the last one, and a reader had no way to tell why a run took ten minutes instead of two. analyze.sh now records base_source (saved, committed, computed), base_reason (no_baseline, incompatible), base_from_sha, catchup_commits, base_seconds and head_seconds. They go into the review metadata.json as strings, onto the comment's machine-readable marker, and into one "Base:" line in the comment. While a base is computed, the sticky progress comment is rewritten into two steps with the elapsed time and the reason, never an estimate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-ups on the base provenance: - code_paths_changed diffs against the first parent, so a --no-ff merge on the first-parent chain counts as the change it brought in; diff-tree printed nothing for merges and catch-up read 0. .gitattributes is not code either. - base_from_sha for a committed baseline comes only from a commit sync made (its bot committer, nothing but .codeboarding/): pushed directly, or on the second-parent side of a merged sync pull request. Anything else (a squash, a hand edit) leaves it empty rather than guessing. - An empty sha or count is unknown, not 0: the comment then says "saved diagram of <branch>, caught up" without a sha, and an ancestor never reads as "saved". - The progress ticker stops through a stop file and is waited for, and post-progress.sh checks the file right before editing, so a stale "running" edit cannot land after step 2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CodeBoarding reviewStatus: 0 changed components (no analysed file changed) See the full change in CodeBoarding. Base: saved diagram of main @a976e0a · changes 35 s graph LR
n_action_scripts["action_scripts"]
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;
|
|
[blockerish] Can we simplify the base-analysis reporting to:
The result is always an up-to-date analysis of the comparison-base commit, which we compare with the PR-head analysis—not a diff against the historical starting commit. Keep the starting SHA/count as optional detail in the incremental message rather than separate reporting fields. Don’t say “no analysis in the last 100 commits”: the current 100-commit lookup identifies the baseline origin, not searches for an analysis. |
ivanmilevtues
left a comment
There was a problem hiding this comment.
I wrote coments after looking at the change + trying to udnerstand it, i think it is a bit overcomplicating and the wording is misleading.
I wrote a general comment with a suggestion on what I believe the wording should look like (maybe).
Would love to bounce ideas if needed on this but i think the current wording of params is confusing the word base is used many times ;d, and some commits and so on.
TO me in a PR context there is the base commit and the head commit and that is it.
|
|
||
| # How far below the merge base this run looks for the commit a saved analysis | ||
| # describes. Past it, a catch-up count is reported as unknown. | ||
| CATCHUP_BOUND=100 |
There was a problem hiding this comment.
even 100 sounds quite a lot to me, but if it is not too slow, it's okay i suppose.
I was thinking more like 20 but 100 commits if you are bit squashiung can happen quickly I suppose.
| fi | ||
| ;; | ||
| esac | ||
| [ -z "$BASE_LINE" ] || printf '\n<sub>%s</sub>\n' "$BASE_LINE" >> "$BODY" |
There was a problem hiding this comment.
[please address] Please move the Base: line below the diagram, immediately above the artifacts/run footer, while keeping it wrapped in <sub>. This is supporting metadata, similar to the artifacts and run links, so the final result and diagram should come first.
Expected final layout:
### CodeBoarding review
**Status:** 3 changed components
See the full change in [CodeBoarding](…).
[Existing diagram appears here]
<sub>Base: built from scratch (no saved diagram), 8 m 54 s · changes 3 m 12 s</sub>
<sub>[download artifacts](…) · run [123456](…)</sub>
What changed and why
A review's base graph comes from one of three places: the saved
codeboarding-base-<cfg>-<merge_base>artifact, the.codeboarding/baseline committed at the merge base (caught up incrementally), or a full analysis in this run. Nothing reported which one ran, so a review that took ten minutes because it builtmainfrom scratch looked the same as one that took two.This records it and reports it, per the base-provenance contract shared with the webview:
analyze.shrecordsbase_source(saved/committed/computed),base_reason(no_baseline/incompatible, only withcomputed),base_from_sha,catchup_commits,base_secondsandhead_seconds, and emits them as step outputs.incompatiblemeans a candidate existed (a saved artifact or committed baseline with another depth cap, or a committed baseline the engine refused withrequiresFullAnalysis).base_secondsincludes the artifact lookup step's own time, whichfetch-state.shnow reports.base_from_shais the commit the baseline describes: the parent of the sync commit that wrote it, pushed directly or on the second-parent side of a merged sync PR. Only a commit sync made counts (its bot committer, nothing but.codeboarding/); a squash or hand edit leaves it empty rather than guessing. History is deepened to 101 commits.catchup_commitscounts first-parent commits from there to the merge base whose diff against their first parent changes code, so a--no-ffmerge counts and a sync or.gitattributes-only commit does not. Empty means unknown.metadata.jsongains the six keys, all strings, empty when not applicable.base=… base_reason=… base_seconds=… head_seconds=…after the existing keys;base_reason=is omitted when empty.Base:line with measured times.analyze.shrewrites the sticky progress comment (found by the sticky action's own marker, which the edit keeps) into two steps, and refreshes the elapsed minutes every 60 s from a background ticker. The ticker stops through a stop file and is waited for, and each edit re-checks the file just before it is sent, so a stale "running" edit cannot land after step 2. It then marks step 1 done with the measured time. Elapsed only, no estimate. A fork's read-only token makes the edit fail silently, as the progress step already does.docs/COMMIT_STRATEGY.md: metadata table gains the six keys plus the missingkindandanalysed_files_changedrows.base_source.What it looks like
This PR changes nothing visible in the web platform UI by itself. The comment text on GitHub changes:
Progress comment while the base is built:
Final comment, one of:
Marker:
How it was tested
--no-ffmerges counted as catch-up, a merged sync PR's analysed commit, an unknown-origin baseline left empty,.gitattributesnot counted, the unknown and ancestor comment lines, the stopped ticker; each base path's metadata (saved,computed/no_baseline, threeincompatiblecases,committedat the merge base with catch-up 0,committedwith 2 commits caught up), the progress comment rewrite (and no rewrite for a saved base),post-progress.shcopy and lookup caching, theBase:line for every source, and the marker keys.python -m unittest discover -s tests: 204 tests, all pass excepttest_sync_without_baseline_uses_configured_depth_directly, which fails identically onmainlocally because macOS ships bash 3.2 (${FORCE_FULL,,}); CI runs bash 5.🤖 Generated with Claude Code