[#1208] WIP Added basic skeleton for the new LCA processor and proxy - #1217
[#1208] WIP Added basic skeleton for the new LCA processor and proxy#1217andre-senna wants to merge 11 commits into
Conversation
…ation_evolution POC
…ation_evolution POC
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe legacy link-creation agent, service, templates, and request processors are removed. New ChangesLink creation transition
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ProxyFactory
participant LinkCreationProxy
participant LinkCreationProcessor
participant LinkCreatorRegistry
participant LinkCreator
ProxyFactory->>LinkCreatorRegistry: initialize statics
ProxyFactory->>LinkCreationProxy: create default proxy
LinkCreationProcessor->>LinkCreationProxy: execute link-creation command
LinkCreationProxy->>LinkCreatorRegistry: resolve creator tag
LinkCreationRegistry->>LinkCreator: return creator
LinkCreationProxy->>LinkCreator: create links from query answers
LinkCreationProxy->>LinkCreationProcessor: return link statistics
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (11)
src/agents/link_creation_agent/link_creators/LinkCreator.h (1)
20-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the destructor
virtual.
LinkCreatoris a polymorphic base with a pure virtualcreate. The destructor is non-virtual. If any code deletes a derived creator through aLinkCreator*, the behavior is undefined and the derived state leaks.shared_ptr<LinkCreator>built bymake_shared<Derived>stays safe, but the class should not depend on that call site detail.♻️ Proposed fix
LinkCreator(); - ~LinkCreator(); + virtual ~LinkCreator();🤖 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/link_creation_agent/link_creators/LinkCreator.h` around lines 20 - 21, Update the LinkCreator destructor declaration to be virtual, preserving its role as a safe polymorphic base for deletion through LinkCreator pointers.src/agents/link_creation_agent/link_creators/LinkCreator.cc (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
this->for the member read.Line 17 reads
_visited_at_least_one_in_last_createdirectly while the next line usesthis->. Keep member access consistent.♻️ Proposed fix
- bool answer = _visited_at_least_one_in_last_create; + bool answer = this->_visited_at_least_one_in_last_create;As per coding guidelines: "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/link_creation_agent/link_creators/LinkCreator.cc` around lines 16 - 20, Update LinkCreator::get_and_reset_visited to access _visited_at_least_one_in_last_create through this-> for the initial member read, matching the existing qualified access when resetting it.Source: Coding guidelines
src/tests/cpp/link_creation_agent_test.cc (2)
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the debug output or route it through the logger.
The
coutlines print on every run and add nothing to the assertion on line 66. UseLOG_DEBUGif the token dump helps during failures.♻️ Proposed fix
- cout << "tokens1: " << Utils::join(tokens1) << endl; - cout << "tokens3: " << Utils::join(tokens3) << endl; - EXPECT_EQ(tokens1, tokens3); + EXPECT_EQ(tokens1, tokens3) << "tokens1: " << Utils::join(tokens1) + << " tokens3: " << Utils::join(tokens3);🤖 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/link_creation_agent_test.cc` around lines 64 - 65, Remove the unconditional cout token dumps around the assertions in link_creation_agent_test, or replace them with LOG_DEBUG so output is emitted only through the established logging path while preserving the assertion behavior.
22-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the new command surface, and free the bus resources.
The test exercises only local creation and token round-trip. The PR adds a proxy/processor split, so the behavior-relevant paths are untested:
stop_criteria_met()againstMAX_ROUNDSandMAX_VISITS_PER_ROUND,remote_link_creation()plusremote_link_creation_finished(),process_query_answer_response()merging intoget_remotely_created_links(), and the error path throughraise_error_on_peer. Add cases for an unknown creator tag reachingLinkCreatorRegistry::function()and for a secondinitialize_statics()call, since both raise.The two
Utils::sleep(1000)calls also make this a slowsize = "small"test with timing-dependent registration. Prefer a polled wait on processor readiness if a helper exists in the sibling tests.Do you want me to draft these test cases?
As per path instructions: "Behavior changes here should have matching *_test.cc updates; suggest concrete test cases (edge cases, error paths, concurrency) 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/link_creation_agent_test.cc` around lines 22 - 41, Expand link_creator_function to cover stop_criteria_met at MAX_ROUNDS and MAX_VISITS_PER_ROUND, remote_link_creation with remote_link_creation_finished, process_query_answer_response merging into get_remotely_created_links, and raise_error_on_peer failures. Add assertions for an unknown creator tag passed to LinkCreatorRegistry::function and a second initialize_statics call raising. Replace fixed Utils::sleep delays with the existing sibling-test readiness polling helper, and explicitly stop or release the ServiceBus instances before the test exits.Source: Path instructions
src/agents/link_creation_agent/link_creators/LinkCreatorRegistry.h (1)
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd brief Doxygen blocks for the public API, and drop
<mutex>or use it.
function()andinitialize_statics()are the public surface and have no/** ... */blocks. Also, the header includes<mutex>but the class declares no mutex, which hints at synchronization that was planned and not added. See the thread-safety note onLinkCreatorRegistry.cc.As per coding guidelines: "Use brief Doxygen
/** ... */blocks above public API methods in C++ header files".🤖 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/link_creation_agent/link_creators/LinkCreatorRegistry.h` around lines 20 - 30, Update LinkCreatorRegistry.h by adding brief Doxygen blocks above the public static methods function() and initialize_statics(), and remove the unused <mutex> include unless the registry implementation actually adds and uses a mutex for the documented synchronization requirement.Source: Coding guidelines
src/agents/link_creation_agent/link_creators/LinkCreatorRegistry.cc (1)
34-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one map lookup, and document the initialization contract.
function()callsfind()and thenFUNCTION[tag], which hashes twice and can insert on a typo path if the branch ever changes. Use the iterator fromfind(). Separately,FUNCTIONis a mutable static touched byinitialize_statics()with no lock, so all registration must complete before any worker thread callsfunction(). State that contract in the header comment, or guard both methods with a static mutex.♻️ Proposed fix
- if (FUNCTION.find(tag) != FUNCTION.end()) { - return FUNCTION[tag]; - } else { - RAISE_ERROR("Unkown link creation function: " + tag); - } + auto iterator = FUNCTION.find(tag); + if (iterator != FUNCTION.end()) { + return iterator->second; + } else { + RAISE_ERROR("Unknown link creation function: " + tag); + }Note the typo fix: "Unkown" → "Unknown".
🤖 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/link_creation_agent/link_creators/LinkCreatorRegistry.cc` around lines 34 - 50, Update LinkCreatorRegistry::function to perform a single FUNCTION.find(tag) lookup and return the found iterator’s value, avoiding FUNCTION[tag]; correct the error text from “Unkown” to “Unknown”. Document in the LinkCreatorRegistry header that initialize_statics() and all registration must complete before worker threads call function(), unless synchronization is added to both methods.src/agents/link_creation_agent/LinkCreationProcessor.cc (2)
54-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicate catch blocks and remove the dead code marker.
std::runtime_errorderives fromstd::exception, and both handlers run the same statement. Keep only thestd::exceptionhandler. Also remove the commented-outAttentionBrokerClient::health_check(true);at Line 20.♻️ Proposed change
- } catch (const std::runtime_error& exception) { - proxy->raise_error_on_peer(exception.what()); } catch (const std::exception& exception) { proxy->raise_error_on_peer(exception.what()); }🤖 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/link_creation_agent/LinkCreationProcessor.cc` around lines 54 - 70, In the exception handling around the command processing flow, remove the redundant std::runtime_error catch and retain the std::exception handler that calls proxy->raise_error_on_peer(exception.what()). Also delete the commented-out AttentionBrokerClient::health_check(true) marker near the processor setup.
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd processor tests for the paths that already have behavior.
link_creation()andissue_link_creation_query()are TBD, and that is fine for a skeleton.run_command()andthread_process_one_query()are not. Addsrc/tests/cpp/*_test.cccases for: a duplicatethread_idraising an error, a non-LinkCreationProxyargument,args.size() < 2propagating throughraise_error_on_peer, andquery_threadsreturning to empty after a command finishes.Do you want me to draft these test cases?
As per path instructions: "Behavior changes here should have matching *_test.cc updates; suggest concrete test cases (edge cases, error paths, concurrency) not trivial assertions."
Also applies to: 86-101
🤖 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/link_creation_agent/LinkCreationProcessor.cc` around lines 19 - 21, Add focused *_test.cc coverage for the existing run_command() and thread_process_one_query() behavior in LinkCreationProcessor: verify duplicate thread_id input raises an error, non-LinkCreationProxy arguments are rejected, args.size() < 2 propagates through raise_error_on_peer, and query_threads is empty after command completion. Leave the TBD link_creation() and issue_link_creation_query() paths unchanged.Source: Path instructions
src/agents/link_creation_agent/LinkCreationProcessor.h (1)
39-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the per-command
proxymember and addoverride.One
LinkCreationProcessorinstance serves every LINK_CREATION command. Ashared_ptr<LinkCreationProxy> proxymember invites shared per-command state across concurrent threads.LinkCreationProcessor.ccnever uses it. Remove it now, while the class is still a skeleton.Mark
factory_empty_proxy()andrun_command()withoverrideso a future signature change inBusCommandProcessorfails at compile time.♻️ Proposed change
- virtual shared_ptr<BusCommandProxy> factory_empty_proxy(); + virtual shared_ptr<BusCommandProxy> factory_empty_proxy() override; @@ - virtual void run_command(shared_ptr<BusCommandProxy> proxy); + virtual void run_command(shared_ptr<BusCommandProxy> proxy) override; @@ map<string, shared_ptr<StoppableThread>> query_threads; mutex query_threads_mutex; - shared_ptr<LinkCreationProxy> proxy; };🤖 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/link_creation_agent/LinkCreationProcessor.h` around lines 39 - 47, Remove the unused per-command proxy member from LinkCreationProcessor and ensure its methods do not depend on that shared state. Mark factory_empty_proxy() and run_command() with override in the class declaration so signature changes in BusCommandProcessor are caught at compile time.src/agents/link_creation_agent/LinkCreationProxy.h (1)
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the constructor parameter names with the implementation.
The header declares
link_creator_tagandlink_creator.LinkCreationProxy.ccuseslink_creator_tagandlink_creation_function, and the member islink_creation_function_tag. This divergence hides the argument bug flagged inLinkCreationProxy.cc(Line 39). Pick one name per concept.Also applies to: 147-158
🤖 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/link_creation_agent/LinkCreationProxy.h` around lines 40 - 44, Align the LinkCreationProxy constructor declaration with its implementation by using consistent parameter names for the creator tag and creator callback across LinkCreationProxy.h, LinkCreationProxy.cc, and the member link_creation_function_tag. Rename link_creator and any mismatched callback references to the established link_creation_function concept, ensuring the constructor forwards each argument to the corresponding member correctly.src/agents/link_creation_agent/LinkCreationProxy.cc (1)
126-131: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDo not hold
api_mutexacrossto_remote_peer().Both methods send to the peer while the mutex is held.
to_remote_peer()performs network work, so every other proxy call (to_string,stop_criteria_met,get_remotely_created_links) blocks for that duration.process_query_answeralso runs the whole link-creation loop under the lock.Reduce the scope: mutate the state under the lock, release it, then send. In
process_query_answer, alsobundle.reserve(args.size())and iterate withconst string& tokensto avoid one string copy per answer.As per path instructions: "Avoid heap allocations in hot paths and while holding api_mutex. Minimize lock scope."
Also applies to: 164-180
🤖 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/link_creation_agent/LinkCreationProxy.cc` around lines 126 - 131, Reduce api_mutex scope in remote_link_creation and process_query_answer: update shared state while locked, release the lock before calling to_remote_peer(), and keep the link-creation loop outside the mutex. In process_query_answer, reserve args.size() capacity for bundle and iterate answers as const string& tokens to avoid copies and hot-path allocations.Source: Path instructions
🤖 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/link_creation_agent/link_creators/BUILD`:
- Around line 11-13: Add //commons:commons_lib to the deps of the
link_creators_lib target in BUILD, alongside the existing query_answer
dependency, so LinkCreatorRegistry.cc can resolve Utils.h and RAISE_ERROR.
In `@src/agents/link_creation_agent/link_creators/LinkCreatorRegistry.cc`:
- Around line 6-9: Provide the missing UnitTestLinkCreator implementation and
corresponding declarations required by LinkCreatorRegistry.cc, including the
LinkCreator behavior used by FUNCTION["unit_test"], or remove the unit_test
registration and preserve the existing link-creation path. Ensure the registry’s
include and unit_test-based tests resolve to a valid implementation.
In `@src/agents/link_creation_agent/LinkCreationProcessor.cc`:
- Around line 92-95: Update LinkCreationProcessor::remove_query_thread to remove
entries by key using the container’s safe key-based erase operation, so missing
stoppable_thread_id values are handled without erasing end().
- Around line 33-47: Validate the dynamic_pointer_cast result in
LinkCreationProcessor::run_command before creating or attaching the
StoppableThread. If link_creation_proxy is null, raise the existing error and
return without starting a thread; otherwise preserve the current thread
registration and execution flow.
In `@src/agents/link_creation_agent/LinkCreationProxy.cc`:
- Around line 147-162: Update the LinkCreationProxy test methods in
link_creation_agent_test.cc to call the currently exposed
is_link_creation_function_remote() API instead of the obsolete
is_fitness_function_remote() name, and ensure link creation calls match the
current proxy API. Keep the integration test behavior unchanged while aligning
all references with LinkCreationProxy’s declared methods.
- Around line 78-82: Update LinkCreationProxy::untokenize after
BaseQueryProxy::untokenize(tokens) to verify that tokens still contains the
link-creation function tag before accessing tokens[0] or erasing the first
element; report the malformed or truncated payload through the existing
error-handling mechanism and return without performing either operation when no
token remains.
- Around line 84-98: Update LinkCreationProxy::link_creation to read
link_creation_function_tag and link_creation_function_object under api_mutex at
the start, using the protected values for subsequent checks and invocation to
avoid races with set_link_creation_function_tag(). Remove the post-RAISE_ERROR
return make_pair(0, 0) statements, since those branches already terminate
through RAISE_ERROR.
- Around line 29-40: Update LinkCreationProxy::LinkCreationProxy to pass the
link_creator_tag parameter to set_link_creation_function_tag instead of the
uninitialized member. Set the tag before assigning the creator, and preserve the
caller-supplied link_creation_function when it is non-null rather than replacing
it with the registry instance.
In `@src/agents/link_creation_agent/LinkCreationProxy.h`:
- Around line 88-106: Update the Doxygen comments for remote_link_creation,
get_remotely_created_links, and remote_link_creation_finished to describe remote
link creation rather than fitness evaluation, and correct the copied typos
“aswer” and “Remotelly.” Keep the documented return behavior accurate for each
method.
In `@src/main/bus_node.cc`:
- Line 17: Initialize LinkCreatorRegistry for ProcessorType::LINK_CREATION_AGENT
alongside the existing FitnessFunctionRegistry startup initialization, ensuring
it runs before the processor is created. Use
LinkCreatorRegistry::initialize_statics() and leave initialization for other
processor types unchanged.
In `@src/main/helpers/ProxyFactory.h`:
- Around line 161-163: The LINK_CREATION_AGENT branch in ProxyFactory must not
return a default LinkCreationProxy without required client context, tokens,
creator tag, and params. Construct the proxy using the client-side
initialization path and forward params so LINK_CREATION requests are executable;
if that path is unsupported, explicitly reject the request in this branch
instead of returning an invalid proxy.
In `@src/tests/cpp/BUILD`:
- Around line 614-637: Update the cc_test target link_creation_agent_test to
compile srcs = ["link_creation_agent_test.cc"] instead of
query_evolution_test.cc, and replace or augment its deps with the LCA-specific
libraries required by that source while preserving the existing test
configuration.
In `@src/tests/cpp/link_creation_agent_test.cc`:
- Around line 12-20: Update the namespace qualification in this test translation
unit to use link_creators, so LinkCreator in TestLinkCreator and
LinkCreatorRegistry at the existing call sites resolve to their declared
namespace. Replace the incorrect link_creation_agent namespace reference while
preserving the test implementation and registry usage.
- Around line 42-53: Update the remaining LinkCreationProxy test calls to use
the link-creation API: replace proxy2.compute_fitness(...) with
proxy2.link_creation(...), rename the remote-status assertions for proxy1,
proxy2, and proxy3 to is_link_creation_function_remote(), and replace
proxy3.compute_fitness(...) in EXPECT_THROW with proxy3.link_creation(...).
---
Nitpick comments:
In `@src/agents/link_creation_agent/link_creators/LinkCreator.cc`:
- Around line 16-20: Update LinkCreator::get_and_reset_visited to access
_visited_at_least_one_in_last_create through this-> for the initial member read,
matching the existing qualified access when resetting it.
In `@src/agents/link_creation_agent/link_creators/LinkCreator.h`:
- Around line 20-21: Update the LinkCreator destructor declaration to be
virtual, preserving its role as a safe polymorphic base for deletion through
LinkCreator pointers.
In `@src/agents/link_creation_agent/link_creators/LinkCreatorRegistry.cc`:
- Around line 34-50: Update LinkCreatorRegistry::function to perform a single
FUNCTION.find(tag) lookup and return the found iterator’s value, avoiding
FUNCTION[tag]; correct the error text from “Unkown” to “Unknown”. Document in
the LinkCreatorRegistry header that initialize_statics() and all registration
must complete before worker threads call function(), unless synchronization is
added to both methods.
In `@src/agents/link_creation_agent/link_creators/LinkCreatorRegistry.h`:
- Around line 20-30: Update LinkCreatorRegistry.h by adding brief Doxygen blocks
above the public static methods function() and initialize_statics(), and remove
the unused <mutex> include unless the registry implementation actually adds and
uses a mutex for the documented synchronization requirement.
In `@src/agents/link_creation_agent/LinkCreationProcessor.cc`:
- Around line 54-70: In the exception handling around the command processing
flow, remove the redundant std::runtime_error catch and retain the
std::exception handler that calls proxy->raise_error_on_peer(exception.what()).
Also delete the commented-out AttentionBrokerClient::health_check(true) marker
near the processor setup.
- Around line 19-21: Add focused *_test.cc coverage for the existing
run_command() and thread_process_one_query() behavior in LinkCreationProcessor:
verify duplicate thread_id input raises an error, non-LinkCreationProxy
arguments are rejected, args.size() < 2 propagates through raise_error_on_peer,
and query_threads is empty after command completion. Leave the TBD
link_creation() and issue_link_creation_query() paths unchanged.
In `@src/agents/link_creation_agent/LinkCreationProcessor.h`:
- Around line 39-47: Remove the unused per-command proxy member from
LinkCreationProcessor and ensure its methods do not depend on that shared state.
Mark factory_empty_proxy() and run_command() with override in the class
declaration so signature changes in BusCommandProcessor are caught at compile
time.
In `@src/agents/link_creation_agent/LinkCreationProxy.cc`:
- Around line 126-131: Reduce api_mutex scope in remote_link_creation and
process_query_answer: update shared state while locked, release the lock before
calling to_remote_peer(), and keep the link-creation loop outside the mutex. In
process_query_answer, reserve args.size() capacity for bundle and iterate
answers as const string& tokens to avoid copies and hot-path allocations.
In `@src/agents/link_creation_agent/LinkCreationProxy.h`:
- Around line 40-44: Align the LinkCreationProxy constructor declaration with
its implementation by using consistent parameter names for the creator tag and
creator callback across LinkCreationProxy.h, LinkCreationProxy.cc, and the
member link_creation_function_tag. Rename link_creator and any mismatched
callback references to the established link_creation_function concept, ensuring
the constructor forwards each argument to the corresponding member correctly.
In `@src/tests/cpp/link_creation_agent_test.cc`:
- Around line 64-65: Remove the unconditional cout token dumps around the
assertions in link_creation_agent_test, or replace them with LOG_DEBUG so output
is emitted only through the established logging path while preserving the
assertion behavior.
- Around line 22-41: Expand link_creator_function to cover stop_criteria_met at
MAX_ROUNDS and MAX_VISITS_PER_ROUND, remote_link_creation with
remote_link_creation_finished, process_query_answer_response merging into
get_remotely_created_links, and raise_error_on_peer failures. Add assertions for
an unknown creator tag passed to LinkCreatorRegistry::function and a second
initialize_statics call raising. Replace fixed Utils::sleep delays with the
existing sibling-test readiness polling helper, and explicitly stop or release
the ServiceBus instances before the test exits.
🪄 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: ea3fa05f-d5e9-4515-a697-07696ccd87f7
📒 Files selected for processing (38)
src/agents/link_creation_agent/BUILDsrc/agents/link_creation_agent/EquivalenceProcessor.ccsrc/agents/link_creation_agent/EquivalenceProcessor.hsrc/agents/link_creation_agent/ImplicationProcessor.ccsrc/agents/link_creation_agent/ImplicationProcessor.hsrc/agents/link_creation_agent/LCAQueue.hsrc/agents/link_creation_agent/LinkCreateTemplate.ccsrc/agents/link_creation_agent/LinkCreateTemplate.hsrc/agents/link_creation_agent/LinkCreationAgent.ccsrc/agents/link_creation_agent/LinkCreationAgent.hsrc/agents/link_creation_agent/LinkCreationAgentRequest.hsrc/agents/link_creation_agent/LinkCreationProcessor.ccsrc/agents/link_creation_agent/LinkCreationProcessor.hsrc/agents/link_creation_agent/LinkCreationProxy.ccsrc/agents/link_creation_agent/LinkCreationProxy.hsrc/agents/link_creation_agent/LinkCreationRequestProcessor.ccsrc/agents/link_creation_agent/LinkCreationRequestProcessor.hsrc/agents/link_creation_agent/LinkCreationRequestProxy.ccsrc/agents/link_creation_agent/LinkCreationRequestProxy.hsrc/agents/link_creation_agent/LinkCreationService.ccsrc/agents/link_creation_agent/LinkCreationService.hsrc/agents/link_creation_agent/LinkProcessor.hsrc/agents/link_creation_agent/MettaTemplateProcessor.ccsrc/agents/link_creation_agent/MettaTemplateProcessor.hsrc/agents/link_creation_agent/README.mdsrc/agents/link_creation_agent/TemplateProcessor.ccsrc/agents/link_creation_agent/TemplateProcessor.hsrc/agents/link_creation_agent/link_creators/BUILDsrc/agents/link_creation_agent/link_creators/LinkCreator.ccsrc/agents/link_creation_agent/link_creators/LinkCreator.hsrc/agents/link_creation_agent/link_creators/LinkCreatorRegistry.ccsrc/agents/link_creation_agent/link_creators/LinkCreatorRegistry.hsrc/commons/Utils.hsrc/main/bus_node.ccsrc/main/helpers/ProcessorFactory.hsrc/main/helpers/ProxyFactory.hsrc/tests/cpp/BUILDsrc/tests/cpp/link_creation_agent_test.cc
💤 Files with no reviewable changes (22)
- src/agents/link_creation_agent/TemplateProcessor.h
- src/agents/link_creation_agent/LinkCreationAgentRequest.h
- src/agents/link_creation_agent/EquivalenceProcessor.h
- src/agents/link_creation_agent/LinkCreationRequestProcessor.h
- src/agents/link_creation_agent/LinkCreateTemplate.h
- src/agents/link_creation_agent/LinkCreationRequestProxy.cc
- src/agents/link_creation_agent/README.md
- src/agents/link_creation_agent/MettaTemplateProcessor.h
- src/agents/link_creation_agent/TemplateProcessor.cc
- src/agents/link_creation_agent/LinkCreationAgent.h
- src/agents/link_creation_agent/LinkCreateTemplate.cc
- src/agents/link_creation_agent/ImplicationProcessor.h
- src/agents/link_creation_agent/LCAQueue.h
- src/agents/link_creation_agent/EquivalenceProcessor.cc
- src/agents/link_creation_agent/LinkProcessor.h
- src/agents/link_creation_agent/LinkCreationAgent.cc
- src/agents/link_creation_agent/LinkCreationRequestProcessor.cc
- src/agents/link_creation_agent/LinkCreationRequestProxy.h
- src/agents/link_creation_agent/ImplicationProcessor.cc
- src/agents/link_creation_agent/LinkCreationService.cc
- src/agents/link_creation_agent/MettaTemplateProcessor.cc
- src/agents/link_creation_agent/LinkCreationService.h
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/agents/link_creation_agent/link_creators/UnitTestLinkCreator.h (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
createmethod.The method returns the first handle's size in both result values. Add a brief Doxygen block that documents this behavior and the required
QueryAnswershape.As per coding guidelines, “Use brief Doxygen
/** ... */blocks above public API methods in C++ header files.” As per path instructions, “Public API in headers uses brief Doxygen comments only for public non-obvious APIs.”🤖 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/link_creation_agent/link_creators/UnitTestLinkCreator.h` around lines 15 - 19, Document the public create method with a brief Doxygen block immediately above its declaration, stating that it returns the first handle’s size in both result values and requires a QueryAnswer containing at least one handle.Sources: Coding guidelines, Path instructions
🤖 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/link_creation_agent/link_creators/LinkCreator.h`:
- Around line 59-65: Update visit() so _visited_at_least_one_in_last_create is
set only when _visited.insert(key).second indicates a newly inserted key;
duplicate visits must not mark a new creation before get_and_reset_visited()
reads the flag.
- Around line 64-66: The `LinkCreator` class has mutable fields `_visited` and
`_visited_at_least_one_in_last_create` that are modified by methods like
`create`, `visit`, `visited`, and `get_and_reset_visited` without any
synchronization mechanism. Since `LinkCreator` instances can be shared across
threads via `LinkCreationProxy`'s shared_ptr pattern, add a mutex member
variable to `LinkCreator` and protect all accesses to these mutable fields with
appropriate locking in each method that reads or modifies them to prevent race
conditions.
In `@src/agents/link_creation_agent/link_creators/UnitTestLinkCreator.h`:
- Line 3: Replace the self-include in UnitTestLinkCreator with the base
LinkCreator.h include so LinkCreator is declared before the derived class
declaration. If LinkCreator.h only forward-declares QueryAnswer, add the
complete QueryAnswer definition required by this header.
- Around line 15-18: Clarify the precondition in UnitTestLinkCreator::create
that query_answer must be non-null and contain at least one handle group before
calling get(0). Document this contract or add a guard that handles null and
empty inputs consistently with the LinkCreator interface.
---
Nitpick comments:
In `@src/agents/link_creation_agent/link_creators/UnitTestLinkCreator.h`:
- Around line 15-19: Document the public create method with a brief Doxygen
block immediately above its declaration, stating that it returns the first
handle’s size in both result values and requires a QueryAnswer containing at
least one handle.
🪄 Autofix
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: 669e55ad-d3d9-4b3c-b0e8-83fbcab039bc
📒 Files selected for processing (12)
src/agents/link_creation_agent/LinkCreationProcessor.ccsrc/agents/link_creation_agent/LinkCreationProxy.ccsrc/agents/link_creation_agent/LinkCreationProxy.hsrc/agents/link_creation_agent/link_creators/BUILDsrc/agents/link_creation_agent/link_creators/LinkCreator.ccsrc/agents/link_creation_agent/link_creators/LinkCreator.hsrc/agents/link_creation_agent/link_creators/UnitTestLinkCreator.hsrc/docker/Dockerfilesrc/main/bus_node.ccsrc/main/helpers/ProxyFactory.hsrc/tests/cpp/BUILDsrc/tests/cpp/link_creation_agent_test.cc
🚧 Files skipped from review as they are similar to previous changes (8)
- src/agents/link_creation_agent/link_creators/BUILD
- src/agents/link_creation_agent/link_creators/LinkCreator.cc
- src/main/helpers/ProxyFactory.h
- src/agents/link_creation_agent/LinkCreationProxy.cc
- src/agents/link_creation_agent/LinkCreationProcessor.cc
- src/agents/link_creation_agent/LinkCreationProxy.h
- src/tests/cpp/link_creation_agent_test.cc
- src/main/bus_node.cc
1 participant
WIP towards #1208
This is just the skeleton of the processor/proxy classes. I added the basic functionalities to select link creation function and basic proxy commands but didn't brought the new link creation algorithm yet.