From b4ab35cc0a0065c7db48c4faf144c584f69aa0a7 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Wed, 19 Aug 2026 00:49:46 +0200 Subject: [PATCH] fix(review): stop attempting a cache save a comment run cannot make GitHub hands a run triggered by issue_comment a read-only cache token. The save is refused with "cache write denied: token has no writable scopes", which the cache action reports as "Unable to reserve cache with key ..., another job may be creating this cache" followed by "Cache save failed". Nothing is wrong, but every /codeboarding run ended with two warnings that read like a broken action, and the real reason was buried. Confirmed by running the same job both ways in this repository: identical permissions and action version, a pull_request run saves, a comment run is denied. Reads are unaffected either way, and a comment run restores sync's shared base entry normally. So the saves are skipped for that event, and the documentation stops claiming /codeboarding publishes a shared entry. Two consequences are written down instead: a repository reviewed only through the command never builds a chain, and /codeboarding refresh improves the comment it posts rather than what later runs start from. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- action.yml | 9 +++++++-- docs/COMMIT_STRATEGY.md | 15 +++++++++++---- tests/test_action_cache.py | 17 ++++++++++++++++- 4 files changed, 35 insertions(+), 8 deletions(-) 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")