Uh oh!
There was an error while loading. Please reload this page.
ADFA-5264: Stop tracking the test project's sync cache - #1740
Merged
Conversation
testing/resources/test-project/.cg/gradle-sync/{project.pb,sync.pb,sync.lock}
were tracked, and a test run rewrites them with the local machine's absolute
paths. So they turn up modified in everyone's working tree, and a `git add -A`
buries them in an unrelated commit -- including a 13 MB binary. That is how they
last changed: the most recent commit touching them is an AI-plugin extraction
refactor that had no reason to.
They are a cache, not a fixture. ProjectSyncHelper writes them, and the only
test that mentions the cache is "WHEN sync files are unreadable THEN sync
anyway" -- it exercises their absence. Nothing reads a committed copy.
A lock file was tracked too.
Verified: with them untracked and ignored, running :lsp:java:testV8DebugUnitTest
-- the suite that used to rewrite them -- leaves a clean working tree.
Note for whoever picks up ADFA-5068: :subprojects:tooling-api-impl is a
java-library, so its tests never ran in CI (the aggregate depends only on
testV8DebugUnitTest), and ToolingApiServerImplTest does not currently compile on
stage -- "No value passed for parameter 'buildId'". Once that gap is closed, that
failure becomes visible.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 Walkthrough
WalkthroughThe change ignores the test project's ChangesTest project cache cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk:⚪ Minimal · up to This change stops generated sync-cache files from being tracked or rewritten into developers' working trees. It is localized and merge-ready after normal checks, with no actionable merge-blocking risk remaining. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jatezzz
approved these changes
Aug 25, 2026
Uh oh!
There was an error while loading. Please reload this page.
davidschachterADFA added a commit
that referenced
this pull request
Aug 27, 2026
Both said the tracked-fixture hazard is live. ADFA-5264 (#1740) merged to stage in the meantime and ignores testing/resources/test-project/.cg/ entirely, so nothing under it is tracked any more. The learnings entry now records what happened and the shape worth remembering -- a tracked file a test run rewrites cannot be kept clean by discipline, only by untracking it -- in past tense. CLAUDE.md's git-add-A rule stands on its own; only its justification needed replacing. The untracked half of the risk is still live, and tests/ is the current example: :gradle-plugin:test leaves files there and tests/test-home is not ignored on stage today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU
davidschachterADFA added a commit
that referenced
this pull request
Aug 27, 2026
Brings in ADFA-5263 (#1739), which deleted copyToTestDir independently, and ADFA-5264 (#1740), which ignores the test project's .cg/ cache. With both in, this branch's diff is the three files it actually changes. Conflict was in .gitignore: stage removed both tests/ entries as part of #1739. Kept this branch's tests/test-home line -- the gradle-plugin tests still refill that directory -- and left the model.jar entry deleted, since the task that wrote it is gone. Moved the entry out from under the "Generated files for tooling API" header, which heads the composite-build output dirs and not this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU
davidschachterADFA added a commit
that referenced
this pull request
Aug 27, 2026
Brings in ADFA-5264 (#1740), whose merge is what made two claims in this retro stale -- corrected in 8cfc586. No conflicts; with stage in, the diff is the three documents this PR is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU
davidschachterADFA added a commit
that referenced
this pull request
Aug 28, 2026
…dictionary work (#1754) * docs: retro for the documentation transports and Brotli dictionary work Four independent reviews of PRs already reported as verified each found a real defect in them. The retrospective records what the two recurring shapes were -- fixing the instance in front of me rather than the class, and claims outrunning their checks -- and turns them into rules rather than resolutions. CLAUDE.md gains a "Verify before you claim" section (sweep the sibling sites, prove the regression test fails without the fix, match the handler to the failure, check every claim), a rule against `git add -A` in a repo whose test runs rewrite tracked fixtures, a rule that long commands are backgrounded and narrated rather than run silently, and a rule that a recommendation carries its own why. The last two come from the user's feedback: a silent multi-minute Spotless run was indistinguishable from a hang, and recommendations given as bare conclusions cost a round-trip to unpack. ADFA-5265 covers the Spotless cost itself. learnings.md gains four entries: the pre-push hook's double Spotless cost, the APPROVED badge not meaning the current code was approved, connectedAndroidTest's bouncycastle failure with the adb/am instrument workaround, and the tracked gradle-sync fixtures that a test run rewrites. Not done, deliberately: the reviewer-side revert check in REVIEW.md and dismissing stale approvals on stage. Both change artifacts other people rely on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: fix the rule the review found half-right The backgrounding rule I wrote from the retro collides with the thing it was about. spotlessApply writes to the worktree, and git push runs it through the hook, so backgrounding either races any edit or staging that happens while it is in flight. The rule now separates the two: narrate every long command, but background only the ones that do not write to the worktree -- builds, test runs, spotlessCheck. spotlessApply and git push stay in the foreground with the wait narrated. That is the same partial-application shape the retro is about: a real problem, a fix that covered most of it. Two corrections of fact. `git add -u` does not stage untracked files -- it is tracked paths and deletions -- so grouping it with `git add -A` as sweeping in "whatever else" was wrong; the section now states each one's actual scope and keeps the warning that -u still picks up a tracked file something rewrote behind your back, which is exactly what happened here. And the stale-approval check should compare the approving review's commit.oid with headRefOid rather than timestamps, since latestReviews can return an empty commit.oid. Blank lines around the retro tables for MD058. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct the Spotless timing claim by measuring it I wrote that Spotless costs ~4.5 minutes and that :spotlessShell accounts for nearly all of it by walking scripts/**. Both are wrong, and the rule I had just written -- every claim needs its check -- is what says to measure before repeating them. Measured on this repo, warm daemon: spotlessCheck 6.5s total, of which :spotlessShell is 750ms, :spotlessKotlin 4.4s, :spotlessXml 2.7s. Cold: spotlessCheck 22s, against 20s for `gradlew help` -- so essentially all of a cold run is daemon start plus configuring this many modules, and Spotless adds ~2s. scripts/ and .githooks together are 38 files; there is no walk to prune. The 4.5 minutes I measured earlier was real but misattributed: at that point the machine was carrying two Gradle daemons, a Kotlin daemon and an orphaned test JVM holding 830 MB, with a daemon having already been OOM-killed that session. What survives is the pre-push double cost -- the hook runs spotlessApply, and if it changes anything the push fails and you pay the invocation twice -- and the narration rule, which is about any long command rather than about Spotless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct two retro claims that went stale before merge Both said the tracked-fixture hazard is live. ADFA-5264 (#1740) merged to stage in the meantime and ignores testing/resources/test-project/.cg/ entirely, so nothing under it is tracked any more. The learnings entry now records what happened and the shape worth remembering -- a tracked file a test run rewrites cannot be kept clean by discipline, only by untracking it -- in past tense. CLAUDE.md's git-add-A rule stands on its own; only its justification needed replacing. The untracked half of the risk is still live, and tests/ is the current example: :gradle-plugin:test leaves files there and tests/test-home is not ignored on stage today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU * ADFA-5325: Fix three bot findings the retro's own text got wrong All three are in text this PR adds, and all three were marked resolved without a reply or a change. The backgrounding rule contradicted this PR's own git-add-A rule twelve lines later: it called a test run safe to background, while that rule says :gradle-plugin:test writes under tests/. Now says a build or spotlessCheck is safe, a test run is not automatically safe, and to check what a task writes rather than assuming tests are read-only. The metrics table did not add up: 11.4 + 18.7 + 83 = 113.1h against a 163h span. Idle was simply wrong. It is the remainder after hands-on (~133h), and the rows are not disjoint -- agent time overlaps both, because the agent works while the human is away. Said so under the table rather than leaving a reader to reconcile it. "end to end" -> "end-to-end". Not taken: the finding that `./gradlew spotlessApply` at CLAUDE.md:78 should carry the flox wrapper. It is right about the inconsistency, but that line predates this PR and this is a retrospective -- worth its own change rather than widening this diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU * ADFA-5325: Fix the two learnings entries a bot found and nobody answered Both threads were marked resolved with no reply and no change. The androidTest workaround could not work as written. An androidTest APK is built android:testOnly="true", so `adb install -r` fails with INSTALL_FAILED_TEST_ONLY; it needs -t. The entry also named only the test APK, not the app under test, so following it on a clean device fails twice. Both APKs and the -t flag are now spelled out. The Spotless entry stated its causes as fact. The timings are measured; "a cold daemon under memory pressure, or orphaned JVMs" is inference from one machine on one occasion. Marked as such -- which is the entry's own lesson, since ADFA-5265 was filed on an unmeasured premise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
testing/resources/test-project/.cg/gradle-sync/{project.pb,sync.pb,sync.lock}were tracked, and a test run rewrites them with the local machine's absolute paths. So they show up modified in everyone's working tree, and agit add -Aburies them in an unrelated commit — a 12.7 MB binary among them (12,678,925 bytes).That is exactly how they last changed: the most recent commit touching them is an AI-plugin extraction refactor (#1441) that had no reason to. It happened to me this week too, which is what prompted the ticket.
They are a cache, not a fixture
ProjectSyncHelperwrites them, and the only test that mentions the sync cache isGIVEN force sync not requested WHEN sync files are unreadable THEN sync anyway— it exercises their absence. Nothing reads a committed copy. A lock file was tracked as well.Verified
With them untracked and ignored,
:lsp:java:testV8DebugUnitTest— the suite that used to rewrite them — now leaves a clean working tree. Before this change that same run producedM …/project.pbandM …/sync.pbevery time.One thing found on the way, for ADFA-5068
:subprojects:tooling-api-implis ajava-library, so its tests have never run in CI — the aggregate task depends only ontestV8DebugUnitTest, which is precisely the gap hal identified on #1643. AndToolingApiServerImplTestdoes not currently compile onstage:No value passed for parameter 'buildId'. Once #1643 closes the CI gap, that failure becomes visible, so it is worth knowing about in advance rather than as a surprise red build.What this does not do
The blob stays in history — it entered in
f01e558d6(#1441) and remains reachable after thisdeletion, so no clone or CI checkout gets smaller. This stops the file being rewritten by a test
run into everyone's working tree; shrinking the repo would need a history rewrite, which is not
in scope here.
Two follow-ups worth deciding on, both from review:
git restore testing/resources/test-project/.cgbefore pulling, or
git pullaborts with "Your local changes would be overwritten by merge" —which is most of us, since a test run is what modifies it.
.cgisSharedEnvironment.PROJECT_CACHE_DIR_NAME—always generated, never source — so a repo-wide
.cg/would be safe and would also covergradle-plugin/src/test/resources/sample-project/.cg/andtesting/resources/sample-project/.cg/,neither of which is ignored today. The next test that syncs one of those repeats this incident.