Repository navigation
Conversation
Enable CLI VibeLog from the target Unity project's debug define so consumer runs keep CLI-side evidence without a shell override. Add structured compile request, send, polling, status, and result-store diagnostics so timeouts and duplicate Unity stores can be diagnosed after the fact. Harden Unity VibeLog JSONL appends with serialized writes, retrying, and integrity diagnostics.
Limit compile status duplicate-suppression state and avoid full-file VibeLog integrity scans so debug diagnostics do not grow unbounded in long-lived Unity sessions.
Contributor
📝 WalkthroughWalkthroughThis PR implements comprehensive structured logging across the compile request lifecycle and status polling to enable post-mortem diagnostics of compile wait issues. Changes span VibeLog persistence integrity (JSONL append validation), CLI debug mode resolution from Unity project settings, compile request/result/status logging on the Unity side, and extensive compile wait polling diagnostics on the CLI side. ChangesEnhanced Compile Diagnostics and CLI VibeLog Debug Detection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
hatayama
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/internal/cli/compile_wait.go (1)
412-426: ⚡ Quick winConsider clarifying the bridge recovery log context.
Both
old_endpointandnew_endpointare set tooptions.connection.Endpoint.Address, which means they will always have identical values. If the endpoint address doesn't actually change during bridge recovery (only the connection is re-established), consider either:
- Renaming the fields to clarify this (e.g.,
endpointinstead ofold_endpoint/new_endpoint)- Removing one of the duplicate fields
- Or documenting that the endpoint remains the same but the connection was recovered
The current field names suggest they should differ, which could mislead during diagnostics.
🤖 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 `@cli/internal/cli/compile_wait.go` around lines 412 - 426, In logCompileBridgeRecoveryObserved, old_endpoint and new_endpoint are both set to options.connection.Endpoint.Address which is misleading; update the log to either use a single field (e.g., "endpoint") or rename/remove one of the duplicate fields and set it to options.connection.Endpoint.Address, or add a clarifying context entry that the endpoint remained unchanged while the connection was re-established; adjust the cliVibeLogEntry Context map inside logCompileBridgeRecoveryObserved accordingly (referencing old_endpoint/new_endpoint and options.connection.Endpoint.Address).Packages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.cs (1)
210-222: 💤 Low valueConsider more gradual cache eviction.
The cache clearing strategy at Line 218 drops all 256 entries when the limit is reached. This could cause previously-deduplicated status observations to be logged again if the same request is polled after the cache clears. While the current approach achieves the bounded-state objective, a more gradual eviction (e.g., removing the oldest 25% of entries) would reduce diagnostic noise during high-request-volume scenarios.
However, given that this is debug-only logging and most compile-wait scenarios poll a single request ID until completion (at which point Line 204 removes the entry), the simple clear-all strategy is acceptable.
🤖 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 `@Packages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.cs` around lines 210 - 222, StoreLoggedStatusSignature currently clears the entire LastLoggedStatusByRequestId when Count >= MaxLoggedStatusSignatureCacheEntries causing abrupt eviction and potential re-logging; change this to a gradual eviction strategy: when the cache is full remove the oldest ~25% of entries instead of clearing all. Implement this by tracking insertion order (e.g., maintain an ordered collection or queue alongside LastLoggedStatusByRequestId) and, inside StoreLoggedStatusSignature, pop and remove the oldest N = MaxLoggedStatusSignatureCacheEntries/4 keys from LastLoggedStatusByRequestId before adding the new signature; keep the existing Debug.Assert checks and continue to set LastLoggedStatusByRequestId[requestId] = signature afterwards.
🤖 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.
Nitpick comments:
In `@cli/internal/cli/compile_wait.go`:
- Around line 412-426: In logCompileBridgeRecoveryObserved, old_endpoint and
new_endpoint are both set to options.connection.Endpoint.Address which is
misleading; update the log to either use a single field (e.g., "endpoint") or
rename/remove one of the duplicate fields and set it to
options.connection.Endpoint.Address, or add a clarifying context entry that the
endpoint remained unchanged while the connection was re-established; adjust the
cliVibeLogEntry Context map inside logCompileBridgeRecoveryObserved accordingly
(referencing old_endpoint/new_endpoint and options.connection.Endpoint.Address).
In `@Packages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.cs`:
- Around line 210-222: StoreLoggedStatusSignature currently clears the entire
LastLoggedStatusByRequestId when Count >= MaxLoggedStatusSignatureCacheEntries
causing abrupt eviction and potential re-logging; change this to a gradual
eviction strategy: when the cache is full remove the oldest ~25% of entries
instead of clearing all. Implement this by tracking insertion order (e.g.,
maintain an ordered collection or queue alongside LastLoggedStatusByRequestId)
and, inside StoreLoggedStatusSignature, pop and remove the oldest N =
MaxLoggedStatusSignatureCacheEntries/4 keys from LastLoggedStatusByRequestId
before adding the new signature; keep the existing Debug.Assert checks and
continue to set LastLoggedStatusByRequestId[requestId] = signature afterwards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e241893d-5e18-492f-aaed-e38811ff5a88
⛔ Files ignored due to path filters (1)
Assets/Tests/Editor/VibeLoggerTests.cs.metais excluded by none and included by none
📒 Files selected for processing (15)
Assets/Tests/Editor/CompileSessionResultServiceTests.csAssets/Tests/Editor/CompileStatusBridgeCommandTests.csAssets/Tests/Editor/VibeLoggerTests.csPackages/src/Editor/FirstPartyTools/Compile/CompileController.csPackages/src/Editor/FirstPartyTools/Compile/CompileSessionResultService.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.csPackages/src/Editor/Infrastructure/Api/CompileStatusBridgeCommand.csPackages/src/Editor/Infrastructure/Server/DomainReloadDetectionFileService.csPackages/src/Editor/Infrastructure/Server/UnityCliLoopServerController.csPackages/src/Editor/ToolContracts/VibeLogger.cscli/internal/cli/cli_vibe.gocli/internal/cli/cli_vibe_test.gocli/internal/cli/compile_wait.gocli/internal/cli/compile_wait_test.gocli/internal/cli/run.go
Owner
Author
|
Closing this PR to redo the work on a fresh branch. The ProjectSettings.asset-based CLI debug detection will be removed, and CLI VibeLog enablement will stay controlled by the ULOOP_DEBUG environment variable. |
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
uloop compilewaits through a Unity script reload, both CLI-side and Unity-side details are written to external VibeLog files under.uloop/outputs/VibeLogs/.ULOOP_DEBUGin the shell.User Impact
uloop compileflow.uloop compileis waiting for the result, the request ID, polling state, reload recovery, and stored result state remain available in VibeLog files for diagnosis.Changes
Verification
scripts/check-go-cli.shcli/dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"cli/dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value "io\\.github\\.hatayama\\.UnityCliLoop\\.Tests\\.Editor\\.(CompileStatusBridgeCommandTests|CompileSessionResultServiceTests|VibeLoggerTests)\\."codex-review v3-betaCloses #1280