From 3f6820a4c2f967ee1dce1e3e0c8e75246da2ad10 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 20 Aug 2026 00:28:31 +0200 Subject: [PATCH] refactor(review): drop the /codeboarding refresh and full arguments Neither earns its surface any more. refresh re-derived the head from the base, as a remedy for a chain that had drifted. Chains are now bounded by the warm-start bundle's one-day life: a pull request pushed on Monday and Wednesday gets two runs derived from the base and no chain at all, so only same-day pushes chain, and briefly. full was worse than unnecessary, it was misleading. It rebuilt the head from scratch and then diffed it against the published base it had just fetched, so the one artifact a user might actually want to rebuild, the base shared by every pull request forking from that commit, was the one it could not touch. Fixing that or removing it were the honest options, and it is not clear anyone needs it: the engine already escalates to a full analysis by itself when incremental is not viable. A comment that still carries an argument runs an ordinary review rather than failing, so nothing breaks for anyone with the old form in muscle memory. Both can come back if a real need appears. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 6 +----- action.yml | 3 +-- docs/COMMIT_STRATEGY.md | 2 +- scripts/action/analyze.sh | 9 ++------- scripts/action/guard.sh | 11 +---------- tests/test_action_state.py | 21 --------------------- 6 files changed, 6 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 42b6447..89c36be 100644 --- a/README.md +++ b/README.md @@ -59,11 +59,7 @@ Automatic runs update one sticky **CodeBoarding review** comment. A trusted repo Keep the `concurrency` block if you keep `synchronize`: it is what makes a push continue from the push before it, and what stops a slower run for an older commit from overwriting the review comment for a newer one. Set `cancel-in-progress: true` instead to abandon a superseded run rather than queue it, which costs less when branches are pushed to rapidly, at the price of no analysis for the commits in between. -| Command | What it does | -|---|---| -| `/codeboarding` | Analyzes the current head, reusing this PR's previous analysis when one is available. | -| `/codeboarding refresh` | Ignores that previous analysis and re-derives the head from the merge base. | -| `/codeboarding full` | Forces a from-scratch full analysis of the head. | +`/codeboarding` analyzes the current head, reusing this pull request's previous analysis when there is one. It takes no arguments. The action checks out and analyzes the exact PR head SHA, and compares it with the PR's **merge base** — the commit the branch forked from, which is what GitHub's own "Files changed" tab uses. Commits pushed to the base branch after the fork point are therefore not reported as this PR's changes; the comment notes how far behind the branch is instead. It does not commit generated files to either branch. diff --git a/action.yml b/action.yml index 218599d..44eebc1 100644 --- a/action.yml +++ b/action.yml @@ -228,7 +228,7 @@ runs: run: "$GITHUB_ACTION_PATH/scripts/action/fetch-state.sh" - name: Fetch this pull request's last analysis - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.guard.outputs.seed_mode == 'chain' && steps.state.outputs.warmstart_name != '' && github.server_url == 'https://github.com' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.state.outputs.warmstart_name != '' && github.server_url == 'https://github.com' continue-on-error: true shell: bash env: @@ -333,7 +333,6 @@ runs: REVIEW_HEAD_SHA: ${{ steps.guard.outputs.head_sha }} REVIEW_BASE_REPO: ${{ steps.guard.outputs.base_repo }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} - SEED_MODE: ${{ steps.guard.outputs.seed_mode }} BASE_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/base WARMSTART_DIR: ${{ runner.temp }}/cb-state/${{ github.action }}/warmstart RENEW_BASE: ${{ steps.fetch_base.outputs.renew }} diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index a7913f0..55745ae 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -101,7 +101,7 @@ forking from that commit gets the first row. | Source | Covers | |---|---| | this pull request's warm-start bundle | only the commits pushed since that run | -| nothing to fetch: first run, moved merge base, changed config, `refresh`, or a fork | the whole pull request | +| nothing to fetch: first run, moved merge base, changed config, or a fork | the whole pull request | A restored bundle is used only when it grew from the very base graph this run diffs against, recorded as a digest in `origin.json`. Two runs of the engine over diff --git a/scripts/action/analyze.sh b/scripts/action/analyze.sh index 74ad967..bd5a75b 100755 --- a/scripts/action/analyze.sh +++ b/scripts/action/analyze.sh @@ -142,7 +142,6 @@ fetch_commit() { # lineage are read from the bundle itself, so they are checked here. warmstart_usable() { local base_analysis="$1" bundle_cap base_cap - [ "${SEED_MODE:-chain}" = chain ] || return 1 [ -f "${WARMSTART_DIR:-}/analysis.json" ] || return 1 bundle_cap="$(depth_cap_from "$WARMSTART_DIR/analysis.json")" base_cap="$(depth_cap_from "$base_analysis")" @@ -208,13 +207,9 @@ analyze_review() { fi rm -f "$head_state/origin.json" - if [ "${SEED_MODE:-chain}" = full ]; then + incremental "$CHECKOUT_DIR" "$head_state" + if [ "$REQUIRES_FULL" = true ]; then full "$CHECKOUT_DIR" "$head_state" "$depth" - else - incremental "$CHECKOUT_DIR" "$head_state" - if [ "$REQUIRES_FULL" = true ]; then - full "$CHECKOUT_DIR" "$head_state" "$depth" - fi fi write_origin "$head_state" "$seed_source" "$chain_depth" "$(analysis_digest "$base_analysis")" diff --git a/scripts/action/guard.sh b/scripts/action/guard.sh index 5b48ddc..84d9261 100755 --- a/scripts/action/guard.sh +++ b/scripts/action/guard.sh @@ -40,7 +40,6 @@ if [ "$MODE" = sync ]; then exit 0 fi -seed_mode=chain case "$EVENT" in pull_request|pull_request_target) pr_number="$EVENT_PR_NUMBER" @@ -51,20 +50,13 @@ case "$EVENT" in base_ref="${PULL_BASE_REF:-}" ;; issue_comment) - read -r first_word second_word <<< "$(printf '%s' "$COMMENT_BODY" | tr -d '\r' | awk 'NR == 1 {print $1, $2}')" + first_word="$(printf '%s' "$COMMENT_BODY" | tr -d '\r' | awk 'NR == 1 {print $1}')" [ "$first_word" = /codeboarding ] || skip "Comment is not a /codeboarding command." case "$AUTHOR_ASSOCIATION" in OWNER|MEMBER|COLLABORATOR) ;; *) skip "Only trusted collaborators may run /codeboarding." ;; esac [ -n "$ISSUE_PR_URL" ] || skip "The command was not posted on a pull request." - # refresh ignores the pull request's own cached analysis and re-seeds from - # the base; full additionally forces a from-scratch head analysis. - case "$second_word" in - "") ;; - refresh|full) seed_mode="$second_word" ;; - *) echo "::warning::Unknown /codeboarding argument '$second_word'; running the default incremental review." ;; - esac pr_json="$(gh api "$ISSUE_PR_URL")" pr_number="$(jq -r '.number // empty' <<< "$pr_json")" base_sha="$(jq -r '.base.sha // empty' <<< "$pr_json")" @@ -136,6 +128,5 @@ is_fork=false echo "checkout_repo=$head_repo" echo "checkout_ref=$head_sha" echo "comment_id=$comment_id" - echo "seed_mode=$seed_mode" echo "is_fork=$is_fork" } >> "$GITHUB_OUTPUT" diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 622a445..96ca757 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -181,7 +181,6 @@ def _analyze(self, **extra: str) -> dict[str, str]: "PR_NUMBER": "42", "ENGINE_VERSION": "0.13.8", "CFG_HASH": "cfg", - "SEED_MODE": "chain", "BASE_DIR": str(self.base_dir), "WARMSTART_DIR": str(self.warmstart_dir), "STAGE_DIR": str(self.stage_dir), @@ -227,15 +226,6 @@ def test_a_published_base_without_a_stored_head_seeds_from_the_base(self) -> Non self.assertEqual(values["chain_depth"], "1") self.assertEqual(len(self._engine_calls()), 1) - def test_refresh_ignores_the_stored_analysis(self) -> None: - _state(self.base_dir) - self._bind(chain_depth=3) - - values = self._analyze(SEED_MODE="refresh") - - self.assertEqual(values["seed_source"], "base") - self.assertEqual(values["chain_depth"], "1") - def test_depth_change_discards_the_stored_analysis(self) -> None: _state(self.base_dir, depth=2) _state(self.warmstart_dir, depth=1, chain_depth=3) @@ -273,17 +263,6 @@ def test_a_stored_analysis_with_no_recorded_base_is_discarded(self) -> None: self.assertEqual(values["seed_source"], "base") - def test_a_forced_full_rebuilds_at_the_configured_cap(self) -> None: - # The baseline stopped short of its cap. Rebuilding at the realized - # depth would ratchet the configured depth down for good. - _state(self.base_dir, depth=1, cap=2) - - self._analyze(SEED_MODE="full") - - calls = self._engine_calls() - self.assertEqual(calls[-1]["mode"], "full") - self.assertEqual(calls[-1]["depth"], "2") - def test_each_bundle_says_what_it_is(self) -> None: # A base bundle is otherwise an analysis.json and nothing else, which # unpacks exactly like a head artifact and would be rendered as one.