Feat/compile task caching - #81
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Also exclude new ba.sake.deder.{Diagnostic,Range,Severity} from the bsp4j
wildcard import in DederBspServer (same pattern already used for CompileResult).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename diagnostic model types to Compile* (CompileDiagnostic/CompileRange/ CompileSeverity) so they don't clash with bsp4j; reverts the import-exclusion workaround. Fix TaskExecResult.Success arity in SourceGeneratorsSuite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oot) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On success, derive the complete per-file diagnostics from Zinc's analysis SourceInfos (reported problems for all sources). On failure (compile threw), fall back to the reporter's problems for this run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- compileTask now uses CachedTaskBuilder.buildSummarized (input-hash skip) - CompileResult carries per-file diagnostics from the Zinc result - BSP renderCompileResult publishes the complete picture (reset per file): replayed on cache hit (no compiler ran), and used to finalize cache misses so incremental-untouched files keep their diagnostics Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds 3 integration tests on the multi sample project: - cache hit replays compile notifications (taskStart '(cached)' + COMPILE_REPORT) - cache hit replays error diagnostics without recompiling - incremental recompile keeps untouched-file diagnostics and clears fixed ones Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds compile diagnostics to the build system by introducing diagnostic data types, extending the task API to track cache hits, generating diagnostics from the Zinc compiler, threading cache state through execution, enabling compile task caching, and rendering diagnostics correctly in BSP for both cached and live compilations. ChangesCompile Diagnostics Caching and Rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/test/src/ba/sake/deder/bsp/BspIntegrationSuite.scala`:
- Around line 296-297: The isCachedCompileStart predicate is too broad by
matching "cached"; change it to check for the explicit marker "(cached)" in the
TaskStartParams message (i.e. update the logic in isCachedCompileStart to use
Option(p.getMessage).exists(_.contains("(cached)")) so only the server's
explicit marker counts as a cache hit).
In `@server/src/ba/sake/deder/TasksExecutor.scala`:
- Around line 110-118: TasksExecutor currently passes cacheHit as "!changed" to
internals.recordTaskExecution which conflates "output unchanged" with "served
from cache"; change the call in TasksExecutor (the block that computes
taskRes/changed/fromCache and calls internals.recordTaskExecution) to pass
fromCache instead of !changed so that recordTaskExecution receives true only
when the result was actually served from cache (see CachedTask.executeUnsafe
which sets fromCache). No other changes needed besides replacing the cacheHit
argument from !changed to fromCache in the ExecutionOutcome path that records
telemetry.
In `@server/src/ba/sake/deder/zinc/DiagnosticConversion.scala`:
- Around line 13-17: The diagnostic range can become inverted because startChar
uses a 1 fallback while endChar can fall back to 0; change the column fallbacks
so both use 0. Update the startChar binding (and the nested pos.startColumn()
fallback used by endChar if present) to use orElse(0) instead of orElse(1) so
startChar and endChar consistently default to 0 and avoid endChar < startChar;
leave the line fallbacks (startLine/endLine) as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6e41104-be84-41f7-a73a-a344233e140d
📒 Files selected for processing (11)
deder-common/src/ba/sake/deder/CompileResult.scalaintegration/test/src/ba/sake/deder/bsp/BspIntegrationSuite.scalaplugin-api/src/ba/sake/deder/Task.scalaserver/src/ba/sake/deder/CoreTasks.scalaserver/src/ba/sake/deder/DederProjectState.scalaserver/src/ba/sake/deder/TasksExecutor.scalaserver/src/ba/sake/deder/bsp/DederBspServer.scalaserver/src/ba/sake/deder/zinc/DiagnosticConversion.scalaserver/src/ba/sake/deder/zinc/ZincCompiler.scalaserver/test/src/ba/sake/deder/SourceGeneratorsSuite.scalaserver/test/src/ba/sake/deder/zinc/DiagnosticConversionSuite.scala
Comment on lines
+296
to
+297
| private def isCachedCompileStart(p: TaskStartParams): Boolean = | ||
| p.getDataKind == TaskStartDataKind.COMPILE_TASK && Option(p.getMessage).exists(_.contains("cached")) |
There was a problem hiding this comment.
Tighten cache-hit detection to the explicit marker.
On Line 297, matching "cached" is broader than needed and can let unintended messages satisfy the predicate. Match the explicit "(cached)" marker that the server emits.
Suggested change
private def isCachedCompileStart(p: TaskStartParams): Boolean =
- p.getDataKind == TaskStartDataKind.COMPILE_TASK && Option(p.getMessage).exists(_.contains("cached"))
+ p.getDataKind == TaskStartDataKind.COMPILE_TASK && Option(p.getMessage).exists(_.contains("(cached)"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private def isCachedCompileStart(p: TaskStartParams): Boolean = | |
| p.getDataKind == TaskStartDataKind.COMPILE_TASK && Option(p.getMessage).exists(_.contains("cached")) | |
| private def isCachedCompileStart(p: TaskStartParams): Boolean = | |
| p.getDataKind == TaskStartDataKind.COMPILE_TASK && Option(p.getMessage).exists(_.contains("(cached)")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration/test/src/ba/sake/deder/bsp/BspIntegrationSuite.scala` around
lines 296 - 297, The isCachedCompileStart predicate is too broad by matching
"cached"; change it to check for the explicit marker "(cached)" in the
TaskStartParams message (i.e. update the logic in isCachedCompileStart to use
Option(p.getMessage).exists(_.contains("(cached)")) so only the server's
explicit marker counts as a cache hit).
Comment on lines
+110
to
+118
| val (taskRes, changed, fromCache) = ti.task.executeUnsafe( | ||
| projectConfig, ti.module, depResults, transitiveResults, | ||
| args, watch, serverNotificationsLogger, dependencyResolver | ||
| ) | ||
| val taskDuration = Duration.ofNanos(System.nanoTime() - taskStartNanos) | ||
| val isUnsuccessful = !ti.task.isResultSuccessfulUnsafe(taskRes.value) | ||
| val errMsg = if isUnsuccessful then Some("result was unsuccessful") else None | ||
| internals.recordTaskExecution(ti.task.name, taskDuration, !changed, errorMessage = errMsg) | ||
| ExecutionOutcome(TaskExecResult.Success(ti, taskRes.value, changed), Some(taskRes)) | ||
| ExecutionOutcome(TaskExecResult.Success(ti, taskRes.value, changed, fromCache), Some(taskRes)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how cacheHit is used in recordTaskExecution and downstream metrics
rg -n -A5 "recordTaskExecution" --type scalaRepository: sake92/deder
Length of output: 9142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect recordTaskExecution implementation
sed -n '90,170p' server/src/ba/sake/deder/DederProjectInternalsImpl.scala
# Also inspect the call sites around TasksExecutor for context
sed -n '80,150p' server/src/ba/sake/deder/TasksExecutor.scala
# Search for cacheHits/cached metrics usage to infer intended semantics
rg -n "cacheHits|cacheHit" server/src/ba/sake/deder -SRepository: sake92/deder
Length of output: 7761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fromCache" --type scala server/src server/test/src
rg -n "TaskExecResult\.Success|case class Success\(|sealed trait TaskExecResult|cacheHits" --type scala server/src server/test/srcRepository: sake92/deder
Length of output: 2619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect semantics of changed/fromCache in DederProjectState
sed -n '260,390p' server/src/ba/sake/deder/DederProjectState.scala
# Inspect TaskExecResult.Success definition and how changed/fromCache are used in TasksExecutor
sed -n '1,230p' server/src/ba/sake/deder/TasksExecutor.scala
# Inspect any metric/telemetry usage of cacheHits / cacheHit beyond internals recording
sed -n '150,240p' server/src/ba/sake/deder/DederProjectInternalsImpl.scalaRepository: sake92/deder
Length of output: 14920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how changed/fromCache are produced in DederProjectState
sed -n '260,380p' server/src/ba/sake/deder/DederProjectState.scala
# Inspect how fromCache/changed are used in planner/execution summaries (if any)
rg -n "changed.*fromCache|fromCache.*changed|cacheHit" server/src/ba/sake/deder -SRepository: sake92/deder
Length of output: 6902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# View the changed/fromCache production code in DederProjectState
sed -n '280,380p' server/src/ba/sake/deder/DederProjectState.scala
# Find where task.executeUnsafe computes (res, changed, fromCache)
rg -n "executeUnsafe\\(" --type scala server/src/ba/sake/dederRepository: sake92/deder
Length of output: 4029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Task trait / executeUnsafe signature and implementations that compute (res, changed, fromCache)
rg -n "def executeUnsafe\\b" --type scala server/src/ba/sake/deder
rg -n "executeUnsafe\\(" --type scala server/src/ba/sake/deder | head -n 50
# Inspect DederProjectState around the executeTask caching decision
sed -n '300,380p' server/src/ba/sake/deder/DederProjectState.scala
# Print the Task-related definitions around where executeUnsafe is declared (best-effort locate nearby files)
fd -e scala "Task.scala" server/src/ba/sake/deder || true
fd -e scala ".*Task.*" server/src/ba/sake/deder || trueRepository: sake92/deder
Length of output: 38
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all occurrences of executeUnsafe across source
rg -n "executeUnsafe" --type scala server/src/ba/sake/deder -S || true
# Locate DederProjectState changed/fromCache builder more precisely
rg -n "fromCache: Boolean|changed: Boolean" server/src/ba/sake/deder/DederProjectState.scala
# List files that mention "fromCache" to find where caching booleans are computed
rg -n "fromCache" --type scala server/src/ba/sake/deder -SRepository: sake92/deder
Length of output: 1307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect BSP usage of fromCache (semantic intent)
sed -n '340,430p' server/src/ba/sake/deder/bsp/DederBspServer.scala
# Inspect Compile task execute wrapper that returns fromCache
sed -n '1160,1225p' server/src/ba/sake/deder/bsp/DederBspServer.scala
# Inspect DederProjectState around where it computes/returns (res, changed, fromCache)
sed -n '280,360p' server/src/ba/sake/deder/DederProjectState.scala
# Find any "changed" computation near fromCache
rg -n "fromCache|changed" server/src/ba/sake/deder/DederProjectState.scalaRepository: sake92/deder
Length of output: 11601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect DederProjectState.executeTask implementation that returns (res, changed, fromCache)
sed -n '300,370p' server/src/ba/sake/deder/DederProjectState.scala
# Search repo-wide for the definition of executeUnsafe (not just call sites)
rg -n "def executeUnsafe" --type scala . -S
# Search for where "changed" and "fromCache" booleans are computed/assigned
rg -n "fromCache\\s*=|changed\\s*=|fromCache\\b.*Boolean|changed\\b.*Boolean" --type scala server/src/ba/sake/deder -SRepository: sake92/deder
Length of output: 3335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show executeUnsafe implementations in plugin-api Task.scala around reported line ranges
sed -n '220,340p' plugin-api/src/ba/sake/deder/Task.scala
sed -n '340,440p' plugin-api/src/ba/sake/deder/Task.scala
sed -n '480,580p' plugin-api/src/ba/sake/deder/Task.scala
# Also find what executeUnsafe returns and meaning of `changed` vs `fromCache`
rg -n "changed: Boolean|fromCache: Boolean|fromCache\\b|changed\\b" plugin-api/src/ba/sake/deder/Task.scalaRepository: sake92/deder
Length of output: 12730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect DederProjectInternalsImpl.scala around metric definitions and TaskStats to infer semantic intent of cacheHit/cacheHits
sed -n '1,260p' server/src/ba/sake/deder/DederProjectInternalsImpl.scala
# Inspect OTEL metric names/attributes for task cache hits
rg -n "taskCacheHitsCounter|cacheHits|cacheHit" server/src/ba/sake/deder/DederProjectInternalsImpl.scala
# Inspect TaskStats / taskStats usage in any CLI/BSP outputs
rg -n "taskStats\\(|cacheHits|cacheHit" server/src server/test/src -SRepository: sake92/deder
Length of output: 12996
Fix cacheHit telemetry to match cache-served semantics
TasksExecutor records cacheHit = !changed, but CachedTask.executeUnsafe sets changed=false both when the result is actually served from cache (fromCache=true) and when the task is recomputed yet produces the same outputHash (fromCache=false). As a result, deder.task.cache.hits will count “output unchanged” as “cache hit”. If the metric is meant to track “computation skipped”, pass fromCache into recordTaskExecution instead of !changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/ba/sake/deder/TasksExecutor.scala` around lines 110 - 118,
TasksExecutor currently passes cacheHit as "!changed" to
internals.recordTaskExecution which conflates "output unchanged" with "served
from cache"; change the call in TasksExecutor (the block that computes
taskRes/changed/fromCache and calls internals.recordTaskExecution) to pass
fromCache instead of !changed so that recordTaskExecution receives true only
when the result was actually served from cache (see CachedTask.executeUnsafe
which sets fromCache). No other changes needed besides replacing the cacheHit
argument from !changed to fromCache in the ExecutionOutcome path that records
telemetry.
Comment on lines
+13
to
+17
| val startLine = pos.startLine().orElse(1) - 1 | ||
| val startChar = pos.startColumn().orElse(1) | ||
| val endLine = pos.endLine().orElse(pos.startLine().orElse(1)) - 1 | ||
| val endChar = pos.endColumn().orElse(pos.startColumn().orElse(0)) | ||
| val severity = p.severity() match { |
There was a problem hiding this comment.
Normalize default column bounds to avoid inverted diagnostic ranges.
When column info is missing, the current defaults can produce endChar < startChar (e.g., 1 vs 0). Use a consistent fallback (prefer 0) for both bounds.
Suggested patch
- val startChar = pos.startColumn().orElse(1)
+ val startChar = pos.startColumn().orElse(0)
val endLine = pos.endLine().orElse(pos.startLine().orElse(1)) - 1
- val endChar = pos.endColumn().orElse(pos.startColumn().orElse(0))
+ val endChar = pos.endColumn().orElse(pos.startColumn().orElse(startChar))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val startLine = pos.startLine().orElse(1) - 1 | |
| val startChar = pos.startColumn().orElse(1) | |
| val endLine = pos.endLine().orElse(pos.startLine().orElse(1)) - 1 | |
| val endChar = pos.endColumn().orElse(pos.startColumn().orElse(0)) | |
| val severity = p.severity() match { | |
| val startLine = pos.startLine().orElse(1) - 1 | |
| val startChar = pos.startColumn().orElse(0) | |
| val endLine = pos.endLine().orElse(pos.startLine().orElse(1)) - 1 | |
| val endChar = pos.endColumn().orElse(pos.startColumn().orElse(startChar)) | |
| val severity = p.severity() match { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/ba/sake/deder/zinc/DiagnosticConversion.scala` around lines 13 -
17, The diagnostic range can become inverted because startChar uses a 1 fallback
while endChar can fall back to 0; change the column fallbacks so both use 0.
Update the startChar binding (and the nested pos.startColumn() fallback used by
endChar if present) to use orElse(0) instead of orElse(1) so startChar and
endChar consistently default to 0 and avoid endChar < startChar; leave the line
fallbacks (startLine/endLine) as-is.
This was referenced Jun 14, 2026
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Tests