[Continuous Learning 5/14] Candidate merge and deduplication#77
Open
xinhuagu wants to merge 1 commit into
Open
[Continuous Learning 5/14] Candidate merge and deduplication#77xinhuagu wants to merge 1 commit into
xinhuagu wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete candidate storage and management system for learning signals, including two enums for candidate classification and lifecycle tracking, a LearningCandidate record with merge capabilities, a similarity scoring utility for candidate deduplication, and a persistent CandidateStore with HMAC-signed JSONL storage and upsert logic. Comprehensive tests validate similarity computation and merge behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CandidateStore
participant CandidateSimilarity
participant FileSystem
participant MemorySigner
Client->>CandidateStore: upsert(CandidateObservation)
CandidateStore->>CandidateSimilarity: score(newCandidate, recentCandidate)
CandidateSimilarity-->>CandidateStore: similarity score
alt Similarity >= Threshold
CandidateStore->>CandidateStore: mergeWith(existing, new)
Note over CandidateStore: Update evidence counts,<br/>merge sourceRefs,<br/>aggregate timestamp bounds
else Similarity < Threshold
CandidateStore->>CandidateStore: append new candidate
end
CandidateStore->>MemorySigner: sign(candidatePayload)
MemorySigner-->>CandidateStore: hmac signature
CandidateStore->>FileSystem: write/append candidates.jsonl
FileSystem-->>CandidateStore: write complete
CandidateStore-->>Client: return LearningCandidate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryImplements candidate lifecycle infrastructure for continuous learning: Key additions:
Issue found:
Confidence Score: 4/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class CandidateStore {
-CopyOnWriteArrayList~LearningCandidate~ candidates
-MemorySigner signer
-ReentrantLock fileLock
-Duration recentWindow
-double mergeThreshold
+upsert(CandidateObservation) LearningCandidate
+load() void
+all() List~LearningCandidate~
-findMergeIndex(LearningCandidate) int
-sign(LearningCandidate) LearningCandidate
-loadOrCreateKey() byte[]
}
class LearningCandidate {
+String id
+Category category
+CandidateKind kind
+CandidateState state
+String content
+String toolTag
+List~String~ tags
+double score
+int evidenceCount
+Instant firstSeenAt
+Instant lastSeenAt
+String hmac
+mergeWith(LearningCandidate) LearningCandidate
+signablePayload() String
}
class CandidateSimilarity {
+score(LearningCandidate, LearningCandidate) double
+jaccard(String, String) double
-tokenize(String) Set~String~
}
class CandidateObservation {
+Category category
+CandidateKind kind
+String content
+String toolTag
+List~String~ tags
+double score
+int successDelta
+int failureDelta
+String sourceRef
+Instant occurredAt
}
class CandidateKind {
<<enumeration>>
ERROR_RECOVERY
PREFERENCE
WORKFLOW
ANTI_PATTERN
SKILL_SEED
}
class CandidateState {
<<enumeration>>
SHADOW
PROMOTED
REJECTED
}
CandidateStore --> LearningCandidate : stores/manages
CandidateStore --> CandidateObservation : receives
CandidateStore ..> CandidateSimilarity : uses for merge decision
LearningCandidate --> CandidateKind : has
LearningCandidate --> CandidateState : has
CandidateObservation --> CandidateKind : has
Last reviewed commit: a2516c2 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| firstSeenAt.isBefore(incoming.firstSeenAt) ? firstSeenAt : incoming.firstSeenAt, | ||
| lastSeenAt.isAfter(incoming.lastSeenAt) ? lastSeenAt : incoming.lastSeenAt, | ||
| List.copyOf(mergedSources), | ||
| hmac |
There was a problem hiding this comment.
HMAC from old candidate reused after merge, but signablePayload() includes all fields that changed (evidenceCount, successCount, failureCount, lastSeenAt, sourceRefs). Merged candidate has invalid signature.
Suggested change
| hmac | |
| null |
Prompt To Fix With AI
This is a comment left during a code review.
Path: aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.java
Line: 85
Comment:
HMAC from old candidate reused after merge, but `signablePayload()` includes all fields that changed (evidenceCount, successCount, failureCount, lastSeenAt, sourceRefs). Merged candidate has invalid signature.
```suggestion
null
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java (1)
83-104: Ordering drifts after upserts.Candidates are sorted by
lastSeenAton load, but merges/appends don’t preserve that order. If consumers rely on recency order (or you want to short‑circuit recent-window scans), consider re-sorting after upsert or documenting that ordering is undefined. As per coding guidelines, aceclaw-memory/**: Verify HMAC signing is maintained, check for memory tier ordering issues, and ensure workspace isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java` around lines 83 - 104, The upsert method updates candidates but doesn’t preserve the sorted-by-lastSeenAt invariant; after merging or appending you should resort the in-memory list by lastSeenAt (newest first) and then ensure the stored entries remain HMAC-signed and workspace-isolated: after calling sign(...) and before rewriteFile()/append(...), insert a step to update the candidates collection ordering (e.g., Collections.sort or equivalent comparator on LearningCandidate.getLastSeenAt), then call rewriteFile() when you replaced the file or append() when adding (but ensure append preserves the on-disk order if you rely on it), and verify sign(...) is applied to the candidate returned by mergeWith(...) and to any newly created candidate from createUnsignedCandidate(...) so HMAC remains valid and workspace isolation is maintained.aceclaw-memory/src/test/java/dev/aceclaw/memory/CandidateStoreTest.java (1)
80-114: Perf assertion is extremely lenient.A 10,000 ms ceiling is unlikely to catch regressions; consider tightening the budget or making it configurable to reflect a meaningful baseline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-memory/src/test/java/dev/aceclaw/memory/CandidateStoreTest.java` around lines 80 - 114, The perf assertion in mergeBaselineAtTenThousandCandidates is too loose (asserting elapsedMs < 10_000); update the test to use a tighter, meaningful budget or make it configurable: replace the hardcoded 10_000 with a named constant (e.g., PERF_MS_LIMIT) or read a test-only env/jvm property (e.g., "perf.ms.limit") and assert elapsedMs < PERF_MS_LIMIT; adjust the constant value to a realistic baseline (for example 1000ms or whatever your CI baseline supports) and document/update any test fixtures if needed. Ensure changes are made in the mergeBaselineAtTenThousandCandidates test and references to CandidateStore.upsert/elapsedMs remain unchanged.
🤖 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-memory/src/main/java/dev/aceclaw/memory/CandidateSimilarity.java`:
- Around line 3-5: Add explicit null-guards for the score parameters in
CandidateSimilarity by calling Objects.requireNonNull on the parameters (e.g., a
and b) before any dereference (before using a.category(), a.toolTag(),
a.content(), a.tags() and likewise for b) so the downstream jaccard() calls
never receive nulls; also add import java.util.Objects if missing and keep the
requireNonNull calls at the top of the method that performs these comparisons
(referencing the CandidateSimilarity method that invokes jaccard()).
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java`:
- Around line 263-270: The record canonical constructor for CandidateObservation
currently only null-checks tags but does not defensively copy it; update the
constructor (the CandidateObservation record) to call
Objects.requireNonNull(tags, "tags") and then replace the incoming list with an
immutable defensive copy (e.g., tags = List.copyOf(tags) or new
ArrayList<>(tags) as appropriate) before assigning to the record component; also
consider validating that the list contains no null elements if your conventions
require it.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.java`:
- Around line 61-86: Add a null-guard at the start of
LearningCandidate.mergeWith(LearningCandidate incoming) (e.g.,
Objects.requireNonNull(incoming) or an early return) to avoid accessing incoming
when it is null, and ensure the returned merged LearningCandidate clears the
stale signature by passing null for the hmac field (since
score/evidence/success/failure/firstSeenAt/lastSeenAt/sourceRefs affect
signablePayload()). Reference mergeWith, signablePayload, and hmac when making
these changes.
- Around line 34-46: The record constructor for LearningCandidate currently
null-checks tags and sourceRefs before copying; change this to defensively
null-guard by assigning tags = tags != null ? List.copyOf(tags) : List.of() and
sourceRefs = sourceRefs != null ? List.copyOf(sourceRefs) : List.of(); in
addition, update mergeWith(...) to null-guard the incoming parameter before
dereferencing it and ensure the new LearningCandidate created in mergeWith does
not reuse the old hmac—recompute or clear the hmac appropriately (i.e., do not
pass the original hmac unchanged when other payload fields like scores, counts,
timestamps, or sourceRefs change).
---
Nitpick comments:
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java`:
- Around line 83-104: The upsert method updates candidates but doesn’t preserve
the sorted-by-lastSeenAt invariant; after merging or appending you should resort
the in-memory list by lastSeenAt (newest first) and then ensure the stored
entries remain HMAC-signed and workspace-isolated: after calling sign(...) and
before rewriteFile()/append(...), insert a step to update the candidates
collection ordering (e.g., Collections.sort or equivalent comparator on
LearningCandidate.getLastSeenAt), then call rewriteFile() when you replaced the
file or append() when adding (but ensure append preserves the on-disk order if
you rely on it), and verify sign(...) is applied to the candidate returned by
mergeWith(...) and to any newly created candidate from
createUnsignedCandidate(...) so HMAC remains valid and workspace isolation is
maintained.
In `@aceclaw-memory/src/test/java/dev/aceclaw/memory/CandidateStoreTest.java`:
- Around line 80-114: The perf assertion in mergeBaselineAtTenThousandCandidates
is too loose (asserting elapsedMs < 10_000); update the test to use a tighter,
meaningful budget or make it configurable: replace the hardcoded 10_000 with a
named constant (e.g., PERF_MS_LIMIT) or read a test-only env/jvm property (e.g.,
"perf.ms.limit") and assert elapsedMs < PERF_MS_LIMIT; adjust the constant value
to a realistic baseline (for example 1000ms or whatever your CI baseline
supports) and document/update any test fixtures if needed. Ensure changes are
made in the mergeBaselineAtTenThousandCandidates test and references to
CandidateStore.upsert/elapsedMs remain unchanged.
Comment on lines
+3
to
+5
| import java.util.HashSet; | ||
| import java.util.Locale; | ||
| import java.util.Set; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CandidateSimilarity.java" -type fRepository: xinhuagu/AceClaw
Length of output: 134
🏁 Script executed:
head -100 aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateSimilarity.javaRepository: xinhuagu/AceClaw
Length of output: 1732
Add null-guards for score parameters.
Parameters a and b are dereferenced immediately (via .category(), .toolTag(), .content(), and .tags()) and passed to downstream jaccard() calls without null checks. Add explicit Objects.requireNonNull() guards per coding guidelines.
💡 Proposed fix
import java.util.HashSet;
import java.util.Locale;
+import java.util.Objects;
import java.util.Set;
public static double score(LearningCandidate a, LearningCandidate b) {
+ Objects.requireNonNull(a, "a");
+ Objects.requireNonNull(b, "b");
if (a.category() != b.category()) return 0.0;
if (!a.toolTag().equalsIgnoreCase(b.toolTag())) return 0.0;
double content = jaccard(a.content(), b.content());
double tags = jaccard(String.join(" ", a.tags()), String.join(" ", b.tags()));
return (0.7 * content) + (0.3 * tags);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateSimilarity.java`
around lines 3 - 5, Add explicit null-guards for the score parameters in
CandidateSimilarity by calling Objects.requireNonNull on the parameters (e.g., a
and b) before any dereference (before using a.category(), a.toolTag(),
a.content(), a.tags() and likewise for b) so the downstream jaccard() calls
never receive nulls; also add import java.util.Objects if missing and keep the
requireNonNull calls at the top of the method that performs these comparisons
(referencing the CandidateSimilarity method that invokes jaccard()).
Comment on lines
+263
to
+270
| public CandidateObservation { | ||
| Objects.requireNonNull(category, "category"); | ||
| Objects.requireNonNull(kind, "kind"); | ||
| Objects.requireNonNull(content, "content"); | ||
| Objects.requireNonNull(tags, "tags"); | ||
| if (score < 0.0 || score > 1.0) { | ||
| throw new IllegalArgumentException("score must be in [0.0, 1.0], got: " + score); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd aceclaw-memory && find . -name "CandidateStore.java" -type fRepository: xinhuagu/AceClaw
Length of output: 114
🏁 Script executed:
cd aceclaw-memory && sed -n '250,280p' src/main/java/dev/aceclaw/memory/CandidateStore.javaRepository: xinhuagu/AceClaw
Length of output: 845
Null-guard and defensively copy tags list field in CandidateObservation record constructor.
The tags field is currently only validated for non-null but not defensively copied. Per coding guidelines, all List fields in record constructors must be null-guarded and defensively copied to prevent NPEs from downstream mutations.
Proposed fix
- Objects.requireNonNull(tags, "tags");
+ tags = tags != null ? List.copyOf(tags) : List.of();📝 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
| public CandidateObservation { | |
| Objects.requireNonNull(category, "category"); | |
| Objects.requireNonNull(kind, "kind"); | |
| Objects.requireNonNull(content, "content"); | |
| Objects.requireNonNull(tags, "tags"); | |
| if (score < 0.0 || score > 1.0) { | |
| throw new IllegalArgumentException("score must be in [0.0, 1.0], got: " + score); | |
| } | |
| public CandidateObservation { | |
| Objects.requireNonNull(category, "category"); | |
| Objects.requireNonNull(kind, "kind"); | |
| Objects.requireNonNull(content, "content"); | |
| tags = tags != null ? List.copyOf(tags) : List.of(); | |
| if (score < 0.0 || score > 1.0) { | |
| throw new IllegalArgumentException("score must be in [0.0, 1.0], got: " + score); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java` around
lines 263 - 270, The record canonical constructor for CandidateObservation
currently only null-checks tags but does not defensively copy it; update the
constructor (the CandidateObservation record) to call
Objects.requireNonNull(tags, "tags") and then replace the incoming list with an
immutable defensive copy (e.g., tags = List.copyOf(tags) or new
ArrayList<>(tags) as appropriate) before assigning to the record component; also
consider validating that the list contains no null elements if your conventions
require it.
Comment on lines
+34
to
+46
| public LearningCandidate { | ||
| Objects.requireNonNull(id, "id"); | ||
| Objects.requireNonNull(category, "category"); | ||
| Objects.requireNonNull(kind, "kind"); | ||
| Objects.requireNonNull(state, "state"); | ||
| Objects.requireNonNull(content, "content"); | ||
| Objects.requireNonNull(toolTag, "toolTag"); | ||
| Objects.requireNonNull(tags, "tags"); | ||
| Objects.requireNonNull(firstSeenAt, "firstSeenAt"); | ||
| Objects.requireNonNull(lastSeenAt, "lastSeenAt"); | ||
| Objects.requireNonNull(sourceRefs, "sourceRefs"); | ||
| tags = List.copyOf(tags); | ||
| sourceRefs = List.copyOf(sourceRefs); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "LearningCandidate.java" -type fRepository: xinhuagu/AceClaw
Length of output: 132
🏁 Script executed:
cat -n ./aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.java | head -100Repository: xinhuagu/AceClaw
Length of output: 3942
🏁 Script executed:
# Check if there are any tests or other usages that might rely on current behavior
rg "LearningCandidate" --type java -A 3 -B 3 | grep -E "(null|mergeWith)" | head -20Repository: xinhuagu/AceClaw
Length of output: 689
🏁 Script executed:
# Check if incoming parameter is used without null-checks in mergeWith
sed -n '61,87p' ./aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.java | grep -E "incoming\."Repository: xinhuagu/AceClaw
Length of output: 575
Null-guard list fields in constructor and fix stale HMAC in mergeWith.
The record constructor currently calls Objects.requireNonNull() on tags and sourceRefs before copying them. Per the coding guideline, these should be defensively null-guarded instead: tags = tags != null ? List.copyOf(tags) : List.of().
Additionally, the mergeWith() method creates a new LearningCandidate with a significantly different payload (merged scores, counts, timestamps, and sourceRefs), but reuses the original hmac. This causes HMAC verification to fail. The incoming parameter must also be null-guarded before dereferencing.
💡 Proposed fixes
Constructor (lines 41, 44-46):
- Objects.requireNonNull(tags, "tags");
Objects.requireNonNull(firstSeenAt, "firstSeenAt");
Objects.requireNonNull(lastSeenAt, "lastSeenAt");
- Objects.requireNonNull(sourceRefs, "sourceRefs");
- tags = List.copyOf(tags);
- sourceRefs = List.copyOf(sourceRefs);
+ tags = tags != null ? List.copyOf(tags) : List.of();
+ sourceRefs = sourceRefs != null ? List.copyOf(sourceRefs) : List.of();mergeWith (lines 61, 85):
public LearningCandidate mergeWith(LearningCandidate incoming) {
+ Objects.requireNonNull(incoming, "incoming");
int mergedEvidence = evidenceCount + incoming.evidenceCount;
...
- hmac
+ null📝 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
| public LearningCandidate { | |
| Objects.requireNonNull(id, "id"); | |
| Objects.requireNonNull(category, "category"); | |
| Objects.requireNonNull(kind, "kind"); | |
| Objects.requireNonNull(state, "state"); | |
| Objects.requireNonNull(content, "content"); | |
| Objects.requireNonNull(toolTag, "toolTag"); | |
| Objects.requireNonNull(tags, "tags"); | |
| Objects.requireNonNull(firstSeenAt, "firstSeenAt"); | |
| Objects.requireNonNull(lastSeenAt, "lastSeenAt"); | |
| Objects.requireNonNull(sourceRefs, "sourceRefs"); | |
| tags = List.copyOf(tags); | |
| sourceRefs = List.copyOf(sourceRefs); | |
| public LearningCandidate { | |
| Objects.requireNonNull(id, "id"); | |
| Objects.requireNonNull(category, "category"); | |
| Objects.requireNonNull(kind, "kind"); | |
| Objects.requireNonNull(state, "state"); | |
| Objects.requireNonNull(content, "content"); | |
| Objects.requireNonNull(toolTag, "toolTag"); | |
| Objects.requireNonNull(firstSeenAt, "firstSeenAt"); | |
| Objects.requireNonNull(lastSeenAt, "lastSeenAt"); | |
| tags = tags != null ? List.copyOf(tags) : List.of(); | |
| sourceRefs = sourceRefs != null ? List.copyOf(sourceRefs) : List.of(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.java`
around lines 34 - 46, The record constructor for LearningCandidate currently
null-checks tags and sourceRefs before copying; change this to defensively
null-guard by assigning tags = tags != null ? List.copyOf(tags) : List.of() and
sourceRefs = sourceRefs != null ? List.copyOf(sourceRefs) : List.of(); in
addition, update mergeWith(...) to null-guard the incoming parameter before
dereferencing it and ensure the new LearningCandidate created in mergeWith does
not reuse the old hmac—recompute or clear the hmac appropriately (i.e., do not
pass the original hmac unchanged when other payload fields like scores, counts,
timestamps, or sourceRefs change).
Comment on lines
+61
to
+86
| public LearningCandidate mergeWith(LearningCandidate incoming) { | ||
| int mergedEvidence = evidenceCount + incoming.evidenceCount; | ||
| double mergedScore = ((score * evidenceCount) + (incoming.score * incoming.evidenceCount)) | ||
| / Math.max(1, mergedEvidence); | ||
| var mergedSources = new LinkedHashSet<>(sourceRefs); | ||
| mergedSources.addAll(incoming.sourceRefs); | ||
| while (mergedSources.size() > MAX_SOURCE_REFS) { | ||
| mergedSources.remove(mergedSources.iterator().next()); | ||
| } | ||
| return new LearningCandidate( | ||
| id, | ||
| category, | ||
| kind, | ||
| state, | ||
| content, | ||
| toolTag, | ||
| tags, | ||
| mergedScore, | ||
| mergedEvidence, | ||
| successCount + incoming.successCount, | ||
| failureCount + incoming.failureCount, | ||
| firstSeenAt.isBefore(incoming.firstSeenAt) ? firstSeenAt : incoming.firstSeenAt, | ||
| lastSeenAt.isAfter(incoming.lastSeenAt) ? lastSeenAt : incoming.lastSeenAt, | ||
| List.copyOf(mergedSources), | ||
| hmac | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.java" | grep -i "aceclaw-memory" | head -20Repository: xinhuagu/AceClaw
Length of output: 1454
🏁 Script executed:
find . -path "*aceclaw-memory*" -name "LearningCandidate.java"Repository: xinhuagu/AceClaw
Length of output: 132
🏁 Script executed:
wc -l aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.javaRepository: xinhuagu/AceClaw
Length of output: 133
🏁 Script executed:
cat -n aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.javaRepository: xinhuagu/AceClaw
Length of output: 3942
🏁 Script executed:
rg "mergeWith" aceclaw-memory --type java -B 2 -A 2Repository: xinhuagu/AceClaw
Length of output: 1236
🏁 Script executed:
rg -A 5 -B 5 "hmac" aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.javaRepository: xinhuagu/AceClaw
Length of output: 693
🏁 Script executed:
rg "MemorySigner\|hmac" aceclaw-memory --type java | head -20Repository: xinhuagu/AceClaw
Length of output: 42
🏁 Script executed:
cat aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java | head -100Repository: xinhuagu/AceClaw
Length of output: 3678
🏁 Script executed:
rg "sign\(" aceclaw-memory/src/main/java/dev/aceclaw/memory/CandidateStore.java -B 2 -A 5Repository: xinhuagu/AceClaw
Length of output: 986
🏁 Script executed:
rg "mergeWith" aceclaw-memory --type java -A 10 -B 2Repository: xinhuagu/AceClaw
Length of output: 2925
🏁 Script executed:
rg "new LearningCandidate\(" aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.javaRepository: xinhuagu/AceClaw
Length of output: 97
Add null-guard for incoming and clear stale HMAC.
The mergeWith() method modifies payload fields (score, evidenceCount, successCount, failureCount, firstSeenAt, lastSeenAt, sourceRefs) that are all part of signablePayload(), making the old HMAC invalid. Setting it to null prevents callers from accidentally persisting a record with a stale signature. Additionally, add a null-guard for the incoming parameter which is accessed multiple times without validation.
Proposed fix
public LearningCandidate mergeWith(LearningCandidate incoming) {
+ Objects.requireNonNull(incoming, "incoming");
int mergedEvidence = evidenceCount + incoming.evidenceCount;
double mergedScore = ((score * evidenceCount) + (incoming.score * incoming.evidenceCount))
/ Math.max(1, mergedEvidence);
@@
lastSeenAt.isAfter(incoming.lastSeenAt) ? lastSeenAt : incoming.lastSeenAt,
List.copyOf(mergedSources),
- hmac
+ null
);
}📝 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
| public LearningCandidate mergeWith(LearningCandidate incoming) { | |
| int mergedEvidence = evidenceCount + incoming.evidenceCount; | |
| double mergedScore = ((score * evidenceCount) + (incoming.score * incoming.evidenceCount)) | |
| / Math.max(1, mergedEvidence); | |
| var mergedSources = new LinkedHashSet<>(sourceRefs); | |
| mergedSources.addAll(incoming.sourceRefs); | |
| while (mergedSources.size() > MAX_SOURCE_REFS) { | |
| mergedSources.remove(mergedSources.iterator().next()); | |
| } | |
| return new LearningCandidate( | |
| id, | |
| category, | |
| kind, | |
| state, | |
| content, | |
| toolTag, | |
| tags, | |
| mergedScore, | |
| mergedEvidence, | |
| successCount + incoming.successCount, | |
| failureCount + incoming.failureCount, | |
| firstSeenAt.isBefore(incoming.firstSeenAt) ? firstSeenAt : incoming.firstSeenAt, | |
| lastSeenAt.isAfter(incoming.lastSeenAt) ? lastSeenAt : incoming.lastSeenAt, | |
| List.copyOf(mergedSources), | |
| hmac | |
| ); | |
| public LearningCandidate mergeWith(LearningCandidate incoming) { | |
| Objects.requireNonNull(incoming, "incoming"); | |
| int mergedEvidence = evidenceCount + incoming.evidenceCount; | |
| double mergedScore = ((score * evidenceCount) + (incoming.score * incoming.evidenceCount)) | |
| / Math.max(1, mergedEvidence); | |
| var mergedSources = new LinkedHashSet<>(sourceRefs); | |
| mergedSources.addAll(incoming.sourceRefs); | |
| while (mergedSources.size() > MAX_SOURCE_REFS) { | |
| mergedSources.remove(mergedSources.iterator().next()); | |
| } | |
| return new LearningCandidate( | |
| id, | |
| category, | |
| kind, | |
| state, | |
| content, | |
| toolTag, | |
| tags, | |
| mergedScore, | |
| mergedEvidence, | |
| successCount + incoming.successCount, | |
| failureCount + incoming.failureCount, | |
| firstSeenAt.isBefore(incoming.firstSeenAt) ? firstSeenAt : incoming.firstSeenAt, | |
| lastSeenAt.isAfter(incoming.lastSeenAt) ? lastSeenAt : incoming.lastSeenAt, | |
| List.copyOf(mergedSources), | |
| null | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/LearningCandidate.java`
around lines 61 - 86, Add a null-guard at the start of
LearningCandidate.mergeWith(LearningCandidate incoming) (e.g.,
Objects.requireNonNull(incoming) or an early return) to avoid accessing incoming
when it is null, and ensure the returned merged LearningCandidate clears the
stale signature by passing null for the hmac field (since
score/evidence/success/failure/firstSeenAt/lastSeenAt/sourceRefs affect
signablePayload()). Reference mergeWith, signablePayload, and hmac when making
these changes.
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
Issue
Closes #56
Summary by CodeRabbit
New Features
Tests