[#1009] New QueryAnswerElement to get hops in QueryAnswer paths - #1154
Merged
Conversation
…gets are being used
…lding and query evolution
Contributor
|
Important Review skippedNo new commits to review since the last review. Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR extends ChangesPATH_HOPS element type and DECODER integration
evaluation_evolution harness tuning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Finishing TouchesGenerate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tests/scripts/toy_tunning.sh (1)
8-8:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncomplete refactor: /tmp/run_toy.log managed but never populated.
Line 8 includes
/tmp/run_toy.login thelog_filesarray, which causes the script to clean it (lines 10-12) and append headers to it (lines 24-29). However, line 30 no longer pipes command output throughtee /tmp/run_toy.log, so the file will only contain headers and never capture the actual run output.Either remove
/tmp/run_toy.logfrom thelog_filesarray on line 8, or restore theteepipeline on line 30.Proposed fix: remove unused log file from array
-log_files=("/tmp/ab.log" "/tmp/qa.log" "/tmp/ev.log" "/tmp/run_toy.log") +log_files=("/tmp/ab.log" "/tmp/qa.log" "/tmp/ev.log")Also applies to: 30-30
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 `@src/tests/scripts/toy_tunning.sh` at line 8, The log_files array on line 8 includes /tmp/run_toy.log, which causes the script to initialize and manage this file (cleaning it and adding headers in lines 10-12 and 24-29), but the command execution on line 30 no longer pipes output to this file using tee, resulting in an empty log file with only headers. Either remove /tmp/run_toy.log from the log_files array definition on line 8, or restore the tee /tmp/run_toy.log pipe at the end of line 30 to actually capture the command output.src/agents/query_engine/QueryAnswer.h (1)
44-51:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUninitialized members in constructors.
The default constructor and
ElementTypeconstructor do not initializehop_peek_start,hop_peek_end,pop_first, andpop_last. These will have indeterminate values which get copied by the copy constructor and assignment operator regardless of element type, leading to potential undefined behavior.Proposed fix
QueryAnswerElement() - : type(NOTHING), path_index(0), element_index(0), name(""), reverse_path(false) {} + : type(NOTHING), path_index(0), element_index(0), name(""), + hop_peek_start(0), hop_peek_end(0), reverse_path(false), pop_first(false), pop_last(false) {} QueryAnswerElement(ElementType type) : type(type) { + this->hop_peek_start = 0; + this->hop_peek_end = 0; + this->reverse_path = false; + this->pop_first = false; + this->pop_last = false; if ((type <= VARIABLE) || (type == PATH_HOPS)) {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 `@src/agents/query_engine/QueryAnswer.h` around lines 44 - 51, The default QueryAnswerElement() constructor and the QueryAnswerElement(ElementType type) constructor are missing initializers for the member variables hop_peek_start, hop_peek_end, pop_first, and pop_last in their initialization lists. Add proper initialization for these members in both constructors (to appropriate default values like 0 or false depending on their types) to prevent undefined behavior from uninitialized data being copied by the copy constructor and assignment operator.
Nitpick comments (2)
src/agents/evolution/QueryEvolutionProcessor.h (1)
84-84: ⚡ Quick winRename
DECODERto snake_case and usethis->member access consistently.The new field name diverges from local member conventions and is then used directly across the implementation, which reduces consistency and readability.
Proposed fix
- HandleDecoder* DECODER; + HandleDecoder* decoder;- DECODER = static_pointer_cast<HandleDecoder>(AtomDBSingleton::get_instance()).get(); + this->decoder = static_pointer_cast<HandleDecoder>(AtomDBSingleton::get_instance()).get();- for (string handle : selected_answer->get_all(pair.first, DECODER)) { + for (string handle : selected_answer->get_all(pair.first, this->decoder)) {- path = target1->metta_representation(*DECODER) + path_link; + path = target1->metta_representation(*this->decoder) + path_link;As per coding guidelines, "Use snake_case for C++ method and field names" and "Access class members with
this->fieldconsistently in C++".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 `@src/agents/evolution/QueryEvolutionProcessor.h` at line 84, Rename the member variable DECODER to decoder_ to follow C++ snake_case conventions for field names in QueryEvolutionProcessor.h. Then update all references to this member variable throughout the implementation to use this->decoder_ instead of accessing it directly, ensuring consistent member access patterns across the class.Source: Coding guidelines
src/tests/cpp/query_answer_test.cc (1)
516-555: ⚡ Quick winAdd explicit assertions for the remaining PATH_HOPS decoder error branches.
This block covers nominal decoding and null decoder, but it does not assert the
get_atom(handle) == nullptrandnon-Link atomfailure paths inQueryAnswer::get_all. Adding those twoEXPECT_THROWcases will lock in the new contract and reduce regression risk.As per coding guidelines, "Prioritize tests for real behavior: error paths, boundary conditions, thread/proxy interactions, and regressions — not trivial assertions."
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 `@src/tests/cpp/query_answer_test.cc` around lines 516 - 555, The test block for QueryAnswer::get_all currently tests nominal decoding cases and one error path but is missing assertions for two error conditions in the PATH_HOPS decoder. Add two additional EXPECT_THROW assertions to test the get_atom(handle) == nullptr failure case and the non-Link atom failure case in QueryAnswer::get_all. These should be inserted appropriately in the test block to ensure both error paths are covered and will help prevent regressions by explicitly locking in the error contract.Source: Coding guidelines
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 `@src/agents/evolution/QueryEvolutionProcessor.cc`:
- Around line 483-486: The code in QueryEvolutionProcessor.cc calls
db->get_link() and immediately dereferences the result without null checks,
which can cause crashes if get_link returns nullptr. Additionally, indexing into
link->targets[1] and link->targets[2] without verifying the link has sufficient
targets is unsafe. Add null pointer checks after each db->get_link() call and
verify the link has adequate arity (check link->arity or targets.size()) before
accessing targets by index. Apply these guards at all locations mentioned: lines
484-485, 488-490, 513-516, and 518-520 in the file.
- Around line 565-567: The unconditional INFO logging in the population
iteration loops using LOG_INFO with answer_to_string calls is causing
performance issues on larger populations due to expensive DB lookups during
answer formatting. Modify or remove the per-individual logging statements in
both the loop iterating over the population variable (around the
answer_to_string call) and the corresponding logging at lines 576-578 to use
conditional DEBUG-level logging instead of unconditional INFO logging, or remove
them entirely if they are not critical for normal operation.
In `@src/agents/query_engine/QueryAnswer.cc`:
- Line 445: The variable `path` in the `get_path_vector(key.path_index)` call is
declared with `auto`, which creates an unnecessary copy of the returned vector
reference. Change the declaration from `auto path` to `auto& path` to use a
reference instead, avoiding the allocation overhead for paths with many handles.
- Around line 458-472: The code accesses link->targets with indices
key.hop_peek_start and key.hop_peek_end without validating these indices are
within bounds of the targets container. Add bounds checks before each access to
link->targets to ensure key.hop_peek_start and key.hop_peek_end do not exceed
the size of link->targets. Verify the container has sufficient elements before
attempting to access at these indices in both the first_handle block and the
subsequent key.reverse_path conditional blocks to prevent undefined behavior
from out-of-bounds access.
In `@src/agents/query_engine/QueryAnswer.h`:
- Around line 210-238: The code accesses s[2] at lines 217 and 224 without
verifying that the string has at least 3 characters. When the string has exactly
2 characters (like ">^"), accessing s[2] causes an out-of-bounds read. Add
length checks using s.size() > 2 before the inner conditionals that access s[2]
in both the pop_first and pop_last branches. This ensures that index 2 is only
accessed when it is guaranteed to exist within the string.
- Around line 135-145: The four-parameter `set()` method in QueryAnswer.h sets
the type to PATH_HOPS but fails to initialize the `hop_peek_start` and
`hop_peek_end` member fields, leaving them with undefined values that will cause
errors when `get_all()` uses them for indexing. Initialize these two fields in
the `set(unsigned int key_path, bool reverse, bool pop_first, bool pop_last)`
method to appropriate default values (such as 0 for `hop_peek_start` and
UINT_MAX for `hop_peek_end`) to match the initialization pattern used in the
PATH_HOPS constructors.
In `@src/tests/main/evaluation_evolution.cc`:
- Around line 775-776: The variable names count_visit_attemps and count_attemps
contain a spelling error where "attemps" should be "attempts". Rename both
variables to count_visit_attempts and count_attempts respectively to match the
spelling convention used in LINK_CREATION_MAX_ATTEMPTS and improve code
readability and greppability. Make sure to update all usages of these variables
throughout the code where they are referenced or incremented to maintain
consistency across the file.
---
Outside diff comments:
In `@src/agents/query_engine/QueryAnswer.h`:
- Around line 44-51: The default QueryAnswerElement() constructor and the
QueryAnswerElement(ElementType type) constructor are missing initializers for
the member variables hop_peek_start, hop_peek_end, pop_first, and pop_last in
their initialization lists. Add proper initialization for these members in both
constructors (to appropriate default values like 0 or false depending on their
types) to prevent undefined behavior from uninitialized data being copied by the
copy constructor and assignment operator.
In `@src/tests/scripts/toy_tunning.sh`:
- Line 8: The log_files array on line 8 includes /tmp/run_toy.log, which causes
the script to initialize and manage this file (cleaning it and adding headers in
lines 10-12 and 24-29), but the command execution on line 30 no longer pipes
output to this file using tee, resulting in an empty log file with only headers.
Either remove /tmp/run_toy.log from the log_files array definition on line 8, or
restore the tee /tmp/run_toy.log pipe at the end of line 30 to actually capture
the command output.
---
Nitpick comments:
In `@src/agents/evolution/QueryEvolutionProcessor.h`:
- Line 84: Rename the member variable DECODER to decoder_ to follow C++
snake_case conventions for field names in QueryEvolutionProcessor.h. Then update
all references to this member variable throughout the implementation to use
this->decoder_ instead of accessing it directly, ensuring consistent member
access patterns across the class.
In `@src/tests/cpp/query_answer_test.cc`:
- Around line 516-555: The test block for QueryAnswer::get_all currently tests
nominal decoding cases and one error path but is missing assertions for two
error conditions in the PATH_HOPS decoder. Add two additional EXPECT_THROW
assertions to test the get_atom(handle) == nullptr failure case and the non-Link
atom failure case in QueryAnswer::get_all. These should be inserted
appropriately in the test block to ensure both error paths are covered and will
help prevent regressions by explicitly locking in the error contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 65e510ce-a962-456f-976a-fe345b397cef
Files selected for processing (9)
src/agents/evolution/QueryEvolutionProcessor.ccsrc/agents/evolution/QueryEvolutionProcessor.hsrc/agents/query_engine/QueryAnswer.ccsrc/agents/query_engine/QueryAnswer.hsrc/tests/cpp/and_operator_test.ccsrc/tests/cpp/query_answer_test.ccsrc/tests/cpp/query_evolution_test.ccsrc/tests/main/evaluation_evolution.ccsrc/tests/scripts/toy_tunning.sh
💤 Files with no reviewable changes (1)
- src/tests/cpp/and_operator_test.cc
added 3 commits
June 19, 2026 10:41
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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 `@src/agents/evolution/QueryEvolutionProxy.cc`:
- Around line 224-238: The new_population_sampled method modifies shared state
variables (num_generations, best_reported_fitness, and
last_generation_with_answer_report) without acquiring the api_mutex lock, while
other parts of the code read these same variables under lock protection (as seen
at line 306), creating a race condition. Wrap the entire block that mutates
these three variables with a lock_guard pattern using this->api_mutex to ensure
thread-safe access to shared state and eliminate the undefined behavior window.
In `@src/agents/evolution/QueryEvolutionProxy.h`:
- Line 208: The member variable last_generation_with_answer_report is not
initialized in the init() method, leaving it uninitialized when the class is
constructed. This causes undefined behavior when last_improving_generation() is
called. Add an initialization statement in the init() method to set
last_generation_with_answer_report to 0 (or an appropriate sentinel value). This
ensures the member variable has a deterministic state from construction, even
before new_population_sampled() has a chance to conditionally assign it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40201405-6aec-494c-91ec-6508d6c670af
Files selected for processing (9)
src/agents/evolution/QueryEvolutionProcessor.ccsrc/agents/evolution/QueryEvolutionProcessor.hsrc/agents/evolution/QueryEvolutionProxy.ccsrc/agents/evolution/QueryEvolutionProxy.hsrc/agents/query_engine/QueryAnswer.ccsrc/agents/query_engine/QueryAnswer.hsrc/tests/cpp/query_answer_test.ccsrc/tests/main/evaluation_evolution.ccsrc/tests/scripts/toy_tunning.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- src/agents/evolution/QueryEvolutionProcessor.h
- src/tests/scripts/toy_tunning.sh
- src/tests/cpp/query_answer_test.cc
- src/agents/query_engine/QueryAnswer.cc
- src/agents/evolution/QueryEvolutionProcessor.cc
- src/tests/main/evaluation_evolution.cc
added 2 commits
June 19, 2026 11:14
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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 `@src/agents/evolution/QueryEvolutionProxy.cc`:
- Around line 225-227: The function
`QueryEvolutionProxy::new_population_sampled` has invalid C++ syntax where a
lock_guard statement with a semicolon appears in the parameter list instead of
the function body. Remove the `lock_guard<mutex> semaphore(this->api_mutex);`
line from the parameter list on line 225, then add it as the first statement
inside the function body of `new_population_sampled`, ensuring proper C++ syntax
by removing the semicolon from the parameter context and making it a valid
statement within the function scope.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3a7444e-2343-4ff7-9b39-ad6d6eda7887
Files selected for processing (1)
src/agents/evolution/QueryEvolutionProxy.cc
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
|
@coderabbitai approve |
Contributor
✅ Action performedComments resolved and changes approved. |
ccgsnet
approved these changes
Jun 19, 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
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.
This PR is a follow-up yet in the context of #1009
Here we introduced a new QueryAnswerElement to allow getting path hops instead of path links. For instance, if a QueryAnswer has a path like
[(a, b), (b, c), (c, d)], the previous QueryAnswerElement PATH allowed the retrieving of the links in the path, i.e.(a, b),(b, c)and(c, d). The new element PATH_HOPS allows retrieving of[a, b, c, d]with optional flags to reverse the path or pop out the first and/or the last hop.