feat(learning): add human review surface for learned signals#250
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds human-review features: CLI /learning subcommands (signals, reviews, review) with RPC endpoints, persistent JSONL review store, LearningSignalReview model, recorder integrations for explanations/validations, and daemon wiring to surface and apply reviews. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as TerminalRepl
participant Daemon as AceClawDaemon
participant Store as ReviewStore
participant ExplRec as ExplanationRecorder
participant ValRec as ValidationRecorder
User->>CLI: /learning review SUPPRESS trend abc [note]
CLI->>Daemon: RPC learning.review.apply(action,type,id,note)
Daemon->>Store: append(projectRoot, LearningSignalReview)
Store-->>Daemon: persisted
Daemon->>ExplRec: recordHumanReview(...)
ExplRec-->>Daemon: recorded
Daemon->>ValRec: recordHumanReview(...)
ValRec-->>Daemon: recorded
Daemon-->>CLI: success response
CLI-->>User: review applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 2848-2863: The left-hand identifier (constructed as left =
targetType:targetId) is being truncated by fitWidth(..., 32) in
renderLearningSignals which makes the exact key uncopyable; replace the
width-limited rendering for that identifier in renderLearningSignals so the full
sanitized identifier (sanitizeTerminalText(left)) is printed verbatim
(optionally pad to 32 chars when shorter) instead of being trimmed by fitWidth,
leaving the summary still width-limited; update the printf call in
TerminalRepl.renderLearningSignals to use the full sanitized left value (and
padding logic if desired) so users can copy targetType:targetId exactly for
/learning review.
- Around line 2676-2688: The current parsing in TerminalRepl accepts only exact
"signals", "reviews", or "review <id>", but silently treats other non-empty
inputs as successful; update the logic around trimmedArg (in TerminalRepl) so
that: if trimmedArg equalsIgnoreCase "review" (no argument) return a clear
usage/error message, if trimmedArg starts with "review " call
handleLearningReviewApplyCommand(trimmedArg.substring(...)) as it does, and if
trimmedArg is non-blank and does not match "signals" or "reviews" or the
"review" patterns then reject it by returning a usage/error (use the same output
stream variable 'out' to print the message). Reference
handleLearningSignalsCommand, handleLearningReviewsCommand,
handleLearningReviewApplyCommand and trimmedArg when adding these explicit
checks before falling through to the default summary.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Line 1329: The code currently limits latest-by-target results by passing 200
into learningSignalReviewStore.latestByTarget(projectPath), causing older
targets to lose reviewAction/reviewSummary; update the call in AceClawDaemon
(the latestReviews assignment using learningSignalReviewStore.latestByTarget) to
load the full latest-by-target map (i.e., remove the hard cap argument) or, if
performance concerns exist, add/use a store API that accepts only the target
keys needed by the endpoint; apply the same change at the other occurrence noted
(around the second call at ~1469) so both endpoints no longer truncate to 200.
- Around line 1471-1488: The loop that builds signals from
learningExplanationStore.recent(...) is accidentally selecting synthetic review
rows (created by recordHumanReview()) because it dedupes by target key and keeps
the first sight; fix it by skipping explanations whose actionType equals
"human_review_applied" (or whatever constant represents the synthetic review
event) before creating the key/adding to seen so the original explanation
remains the reviewable signal; update the block handling
explanation.actionType() in AceClawDaemon (the loop using
learningExplanationStore.recent, the seen set, and applyReviewMetadata) to
filter out those synthetic review events.
- Around line 1371-1383: The current loop increments reviewCounts while
iterating only over learningSignalReviewStore.recent(projectPath, limit), so
reviewCounts reflects just the returned page (default limit=10); change the
logic to compute reviewCounts from a broader/bounded window separate from the
recentReviews page—e.g., call a dedicated method or an additional iterator like
learningSignalReviewStore.recent(projectPath, countWindow) (use a configurable
countWindow) to aggregate counts into reviewCounts, and keep the existing
learningSignalReviewStore.recent(projectPath, limit) loop solely to populate
recentReviews; update any callers or configuration for countWindow and ensure
you still lowercase action names (key) when incrementing reviewCounts.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.java`:
- Around line 28-40: The append method in LearningSignalReviewStore can throw
OverlappingFileLockException when concurrent append() calls run in the same JVM;
add a process-local ReentrantLock field (e.g., private final ReentrantLock
writeLock = new ReentrantLock()) to the LearningSignalReviewStore class and use
writeLock.lock()/writeLock.unlock() to serialize the entire append write path
(acquire the ReentrantLock before creating/opening the FileChannel and releasing
it in a finally block after the channel lock and write complete); also add the
necessary import for java.util.concurrent.locks.ReentrantLock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b66e144a-331a-49ec-afea-036645be9e54
📒 Files selected for processing (8)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.javaaceclaw-cli/src/test/java/dev/aceclaw/cli/TerminalReplTest.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningExplanationRecorder.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReview.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningValidationRecorder.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/LearningSignalReviewStoreTest.java
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review latest changes |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java (1)
1373-1388: Consider collapsing the two review-store reads into one materialized window.
learning.summarycurrently scans reviews once for counts and again forrecentReviews. Reusing a single fetched list would reduce disk I/O and keep summary latency steadier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java` around lines 1373 - 1388, The code currently calls learningSignalReviewStore.recent(projectPath, REVIEW_COUNT_WINDOW) and learningSignalReviewStore.recent(projectPath, limit) separately causing duplicate reads; consolidate by materializing a single List (e.g., var reviews = learningSignalReviewStore.recent(projectPath, Math.max(REVIEW_COUNT_WINDOW, limit))) and iterate that list twice (first to populate reviewCounts by using review.action().name() and REVIEW_COUNT_WINDOW semantics, then to build recentReviews via objectMapper.createObjectNode using review.timestamp(), targetType/Id, action, summary, note, reviewer); ensure you still respect the intended windows (trim or filter the materialized list if needed) and replace references to direct learningSignalReviewStore.recent calls with the new local variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 1525-1537: The current flow appends LearningSignalReview via
learningSignalReviewStore.append before calling
learningExplanationRecorder.recordHumanReview and
learningValidationRecorder.recordHumanReview, which can leave a durable append
if either follow-up fails; change the flow to either (A) perform
recordHumanReview calls first and only call learningSignalReviewStore.append
after both succeed, or (B) wrap the three operations in a transactional/atomic
boundary if supported, or if transactions aren't available perform the append
last and on any failure of explanation/validation do not append (or if append is
unavoidable, implement a compensating delete via learningSignalReviewStore to
roll back the append when recordHumanReview fails); update the code around
learningSignalReviewStore.append, learningExplanationRecorder.recordHumanReview,
and learningValidationRecorder.recordHumanReview accordingly to ensure no
durable partial-write remains on RPC error.
---
Nitpick comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 1373-1388: The code currently calls
learningSignalReviewStore.recent(projectPath, REVIEW_COUNT_WINDOW) and
learningSignalReviewStore.recent(projectPath, limit) separately causing
duplicate reads; consolidate by materializing a single List (e.g., var reviews =
learningSignalReviewStore.recent(projectPath, Math.max(REVIEW_COUNT_WINDOW,
limit))) and iterate that list twice (first to populate reviewCounts by using
review.action().name() and REVIEW_COUNT_WINDOW semantics, then to build
recentReviews via objectMapper.createObjectNode using review.timestamp(),
targetType/Id, action, summary, note, reviewer); ensure you still respect the
intended windows (trim or filter the materialized list if needed) and replace
references to direct learningSignalReviewStore.recent calls with the new local
variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7786748c-24f6-414a-b034-a761112c182c
📒 Files selected for processing (4)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.javaaceclaw-cli/src/test/java/dev/aceclaw/cli/TerminalReplTest.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.java
🚧 Files skipped from review as they are similar to previous changes (1)
- aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.java
Owner
Author
|
@coderabbitai review latest changes |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 1519-1524: The code currently calls
LearningSignalReview.Action.valueOf(actionText.toUpperCase(Locale.ROOT)) which
will throw a generic IllegalArgumentException for invalid actionText; catch that
exception (or pre-validate actionText against
LearningSignalReview.Action.values()) and rethrow a new IllegalArgumentException
with a user-friendly message that includes the invalid value and the list of
valid actions, keeping the existing checks for targetType/targetId/actionText
and preserving the subsequent construction of the summary variable; reference
the LearningSignalReview.Action enum and the local variables actionText,
targetType, and targetId when implementing the change.
- Line 1474: LearningSignalReviewStore currently writes under writeLock in
append() but reads in recent() and latestByTarget() use Files.readAllLines()
unsynchronized, causing races; fix by adding a
java.util.concurrent.locks.ReadWriteLock field to LearningSignalReviewStore,
keep append() holding the existing writeLock (and file lock) and acquire the
read lock around the body of recent() and latestByTarget() (and any other reader
helpers) so readers cannot see partially-written lines—use try/finally to
release the read lock and ensure all call sites (methods referenced: recent(),
latestByTarget(), append()) are updated to use the new lock consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28395c8c-5f3b-466a-8ad1-6b366245da21
📒 Files selected for processing (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java
|
|
||
| var result = objectMapper.createObjectNode(); | ||
| var signals = objectMapper.createArrayNode(); | ||
| var latestReviews = learningSignalReviewStore.latestByTarget(projectPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the LearningSignalReviewStore synchronization pattern
ast-grep --pattern $'class LearningSignalReviewStore {
$$$
public $_ recent($$$) {
$$$
}
$$$
}'
# Check if there's any locking in the recent method
rg -nA 20 'public.*recent\(Path projectRoot' aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.javaRepository: xinhuagu/AceClaw
Length of output: 1072
🏁 Script executed:
#!/bin/bash
# Check the append() method implementation
rg -nA 15 'public void append\(' aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.javaRepository: xinhuagu/AceClaw
Length of output: 894
🏁 Script executed:
#!/bin/bash
# Check the latestByTarget() method implementation
rg -nA 20 'public.*latestByTarget' aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.javaRepository: xinhuagu/AceClaw
Length of output: 708
🏁 Script executed:
#!/bin/bash
# Get the full class structure to understand synchronization
head -100 aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningSignalReviewStore.javaRepository: xinhuagu/AceClaw
Length of output: 3537
Data race in LearningSignalReviewStore reads requires synchronization.
The append() method holds writeLock during writes (with additional file-level locking), but recent() and latestByTarget() perform unsynchronized reads via Files.readAllLines(). This creates a race condition where concurrent RPC handlers calling latestByTarget() (lines 1474, 1444, 1377) can read partially-written lines while another thread appends. Although recent() catches and skips parse exceptions from malformed lines, this masks data corruption.
Add a ReadWriteLock to LearningSignalReviewStore: hold the read lock during recent() and wrap latestByTarget() calls appropriately. This ensures readers cannot race with writers.
Also applies to: 1444-1444, 1377-1377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java` at line
1474, LearningSignalReviewStore currently writes under writeLock in append() but
reads in recent() and latestByTarget() use Files.readAllLines() unsynchronized,
causing races; fix by adding a java.util.concurrent.locks.ReadWriteLock field to
LearningSignalReviewStore, keep append() holding the existing writeLock (and
file lock) and acquire the read lock around the body of recent() and
latestByTarget() (and any other reader helpers) so readers cannot see
partially-written lines—use try/finally to release the read lock and ensure all
call sites (methods referenced: recent(), latestByTarget(), append()) are
updated to use the new lock consistently.
Comment on lines
+1519
to
+1524
| if (targetType.isBlank() || targetId.isBlank() || actionText.isBlank()) { | ||
| throw new IllegalArgumentException("targetType, targetId, and action are required"); | ||
| } | ||
| var action = LearningSignalReview.Action.valueOf(actionText.toUpperCase(Locale.ROOT)); | ||
| String summary = "Human review marked " + targetType + " '" + targetId + "' as " | ||
| + action.name().toLowerCase(Locale.ROOT).replace('_', '-') + "."; |
There was a problem hiding this comment.
Provide a user-friendly error for invalid action values.
Action.valueOf() throws IllegalArgumentException with an unhelpful message if actionText is not a valid enum constant. Per coding guidelines, wrap this into a more descriptive error.
🛠️ Suggested fix
if (targetType.isBlank() || targetId.isBlank() || actionText.isBlank()) {
throw new IllegalArgumentException("targetType, targetId, and action are required");
}
- var action = LearningSignalReview.Action.valueOf(actionText.toUpperCase(Locale.ROOT));
+ LearningSignalReview.Action action;
+ try {
+ action = LearningSignalReview.Action.valueOf(actionText.toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid action: " + actionText
+ + ". Valid values: " + java.util.Arrays.toString(LearningSignalReview.Action.values()));
+ }
String summary = "Human review marked " + targetType + " '" + targetId + "' as "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java` around
lines 1519 - 1524, The code currently calls
LearningSignalReview.Action.valueOf(actionText.toUpperCase(Locale.ROOT)) which
will throw a generic IllegalArgumentException for invalid actionText; catch that
exception (or pre-validate actionText against
LearningSignalReview.Action.values()) and rethrow a new IllegalArgumentException
with a user-friendly message that includes the invalid value and the list of
valid actions, keeping the existing checks for targetType/targetId/actionText
and preserving the subsequent construction of the summary variable; reference
the LearningSignalReview.Action enum and the local variables actionText,
targetType, and targetId when implementing the change.
Owner
Author
|
@coderabbitai review latest changes |
|
✅ Actions performedReview triggered.
|
3 tasks
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
Testing
Summary by CodeRabbit
New Features
Improvements
Tests