diff --git a/README.md b/README.md index e8310a6..74e1c1d 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ Each review seeds the head analysis from this pull request's own previous run, s Caching is best-effort: a cache miss, an unavailable cache service, or a GitHub Enterprise Server without one falls back to analyzing the merge base directly, exactly as before. -Actions cache entries are scoped to the ref that wrote them. Automatic `pull_request` runs therefore reuse each other's analysis and the shared base entry, while a `/codeboarding` command — which runs on the default branch ref — reuses the base entry but not a chain built by automatic runs, so it costs one base-seeded incremental. The action also accepts `pull_request_target`, which runs on the base branch ref and lets both share one chain; that trigger has its own trade-offs (a PR that adds this workflow will not run it until merged, and the fork gate becomes load-bearing), so `pull_request` remains the recommended default. +Actions cache entries are scoped to the ref that wrote them, and GitHub gives comment-triggered runs a read-only cache token. So automatic `pull_request` runs reuse each other's analysis and build the chain, `sync` publishes the base entry everyone shares, and a `/codeboarding` command reads both but writes neither — it costs one base-seeded incremental, and `/codeboarding refresh` improves the comment it posts rather than what later runs start from. The action also accepts `pull_request_target`, which runs on the base branch ref and lets both share one chain; that trigger has its own trade-offs (a PR that adds this workflow will not run it until merged, and the fork gate becomes load-bearing), so `pull_request` remains the recommended default. ## Authentication and providers diff --git a/action.yml b/action.yml index a433ee2..2827c48 100644 --- a/action.yml +++ b/action.yml @@ -324,8 +324,13 @@ runs: # Saved before the review is rendered: the analysis is the expensive part, so # a later rendering or posting failure must not throw it away. + # + # Skipped for issue_comment, where GitHub hands the run a read-only cache + # token: the save is refused with "token has no writable scopes" and the + # cache action reports it as a failure, so attempting it only prints a + # warning that suggests something is broken. Reads still work there. - name: Save pull request analysis - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.cache_keys.outputs.chain_key != '' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && github.event_name != 'issue_comment' && steps.cache_keys.outputs.chain_key != '' continue-on-error: true uses: actions/cache/save@v4 with: @@ -335,7 +340,7 @@ runs: # Only trusted runs publish a base entry: it is restorable repository-wide, # so a fork's run must never be able to place state there. - name: Save base analysis - if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_analyze.outputs.save_base == 'true' && steps.guard.outputs.is_fork != 'true' && steps.cache_keys.outputs.base_key != '' + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && github.event_name != 'issue_comment' && steps.review_analyze.outputs.save_base == 'true' && steps.guard.outputs.is_fork != 'true' && steps.cache_keys.outputs.base_key != '' continue-on-error: true uses: actions/cache/save@v4 with: diff --git a/docs/COMMIT_STRATEGY.md b/docs/COMMIT_STRATEGY.md index 48acb8a..53b59f7 100644 --- a/docs/COMMIT_STRATEGY.md +++ b/docs/COMMIT_STRATEGY.md @@ -68,13 +68,20 @@ cache entry is only visible to the ref that wrote it and to the default branch: commit it analyzed and the baseline commit it creates, because a pull request branched just before that baseline landed has the earlier commit as its merge base. -- **`/codeboarding`**, which runs on the default branch, also writes a shared - entry. - **an automatic `pull_request` run** writes into `refs/pull//merge`, so its entry serves only later runs of that same pull request. Another pull request with the identical merge base still computes its own. - -So review runs warm themselves, and sync is what warms everybody. +- **`/codeboarding` writes nothing.** GitHub gives a comment-triggered run a + read-only cache token, so a save is refused with "token has no writable + scopes" no matter what the workflow grants. Reads work normally: a slash + command restores sync's shared base entry like anything else. + +So sync warms everybody, pull request runs warm themselves, and slash commands +only consume. Two consequences follow. A repository reviewed exclusively through +`/codeboarding` never builds a chain, so every command re-derives the head from +the base. And `/codeboarding refresh` cannot persist what it recomputes: it +fixes the comment it posts, while the next run still restores the analysis it +was asked to replace. **Head** diff --git a/tests/test_action_cache.py b/tests/test_action_cache.py index e1d2f71..5f7cecc 100644 --- a/tests/test_action_cache.py +++ b/tests/test_action_cache.py @@ -416,7 +416,7 @@ def _cache_steps(self) -> list[dict[str, str]]: steps.append(current) elif current is not None: stripped = line.strip() - for field in ("uses", "path", "key", "restore-keys"): + for field in ("uses", "path", "key", "restore-keys", "if"): if stripped.startswith(f"{field}:"): current[field] = stripped.split(":", 1)[1].strip() return [step for step in steps if step.get("uses", "").startswith("actions/cache/")] @@ -434,6 +434,21 @@ def test_the_chain_is_restored_by_prefix_so_a_refresh_survives(self) -> None: # base's own analysis, a prefix hit is only a warm seed. self.assertNotEqual(base["key"], base["restore-keys"]) + def test_reviews_do_not_attempt_a_save_a_comment_run_cannot_make(self) -> None: + # GitHub hands an issue_comment run a read-only cache token, so the save + # is refused with "token has no writable scopes" and surfaces as a + # warning that reads like a broken action. Reads are unaffected. + for step in self._cache_steps(): + if not step["uses"].startswith("actions/cache/save"): + continue + if "review" not in step.get("if", ""): + continue + self.assertIn( + "github.event_name != 'issue_comment'", + step.get("if", ""), + f"{step['name']} would attempt a save that a comment-triggered run cannot make", + ) + def test_every_saved_path_is_a_restored_path(self) -> None: steps = self._cache_steps() self.assertTrue(steps, "no cache steps found in action.yml")