-
Notifications
You must be signed in to change notification settings - Fork 9
APPENG-4072 - Configs dependency loop and value mutation bugfix #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rh-aiq-main
Are you sure you want to change the base?
APPENG-4072 - Configs dependency loop and value mutation bugfix #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etsien The solution looks solid and addresses the critical issues you've mentioned.
I Would only make constant out of agent name ( cve_agent_executor) and put it as the default value for the agent_name across ToolConfig classes that has that agent_name field/attribute.
Another thing that i would do is to pass this agent name explicitly in the tools function configurations ( in all yaml files) and maybe use yaml anchoring to make it consistent in all places across tools configurations.
10q.
IlonaShishov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @etsien for the neat fixes, great job!
|
Good suggestions @zvigrinberg , just made those changes |
4e6b0c4 to
409925c
Compare
|
Merged in tool name changes from PR-146 by cherrypicking and force pushing those 4 commits |
IlonaShishov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Great :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @etsien,
The Agent doesn't startup , The YAML anchoring is fine and works good, beside its placement, in total there are 2 problems:
-
the yaml anchore itself should be placed under
generalgroup , because if it placed under thefunctions, the nat framework think it's another workflow' function, and then it crashes. -
The functions
_typein the YAML confiugrations must be aligned with thenameattribute of the config class of the function/tool, otherwise, the agent is crashing on startup.
For example, The following patch align the tools names with the local configuration to run the agent.
diff --git a/src/vuln_analysis/configs/config-http-openai.yml b/src/vuln_analysis/configs/config-http-openai.yml
index 6086366..a13cef3 100644
--- a/src/vuln_analysis/configs/config-http-openai.yml
+++ b/src/vuln_analysis/configs/config-http-openai.yml
@@ -14,6 +14,8 @@
# limitations under the License.
general:
+# Agent name anchor for consistency across all function configurations
+ _agent_executor_name: &agent_executor_name cve_agent_executor
use_uvloop: true
telemetry:
tracing:
diff --git a/src/vuln_analysis/tools/call_chain_analyzer.py b/src/vuln_analysis/tools/call_chain_analyzer.py
index a950923..62d05e1 100644
--- a/src/vuln_analysis/tools/call_chain_analyzer.py
+++ b/src/vuln_analysis/tools/call_chain_analyzer.py
@@ -39,19 +39,19 @@ from vuln_analysis.logging.loggers_factory import LoggingFactory
logger = LoggingFactory.get_agent_logger(__name__)
-class CallChainAnalyzerToolConfig(FunctionBaseConfig, name=ToolNames.CALL_CHAIN_ANALYZER):
+class CallChainAnalyzerToolConfig(FunctionBaseConfig, name="call_chain_analyzer"):
"""
Call Chain Analyzer tool used to check function reachability in source code.
"""
-class CallingFunctionNameExtractorToolConfig(FunctionBaseConfig, name=ToolNames.FUNCTION_CALLER_FINDER):
+class CallingFunctionNameExtractorToolConfig(FunctionBaseConfig, name="calling_function_name_extractor"):
"""
Function Caller Finder tool used to find functions calling specific library functions.
"""
-class PackageAndFunctionLocatorToolConfig(FunctionBaseConfig, name=ToolNames.FUNCTION_LOCATOR):
+class PackageAndFunctionLocatorToolConfig(FunctionBaseConfig, name="package_and_function_locator"):
"""
Function Locator tool used to validate package names and find function names using fuzzy matching.
"""I Think that the source of the confusion might be the tool names , that each tool needs a full name ( to be in context) and a short name written in snake_case to be in the configuration ( as _type) and in the name of the function/tool' configuration class.
Note: This PR is a branch from my previous tool bugfix PR
Jira:
Jira ticket APPENG-4072
Context:
Issue 1: Looping config dependencies
cve_generate_vdbs.py and cve_checklist.py both load the agent executor during initialization, but that occurs before the agent executor itself is loaded. This leads to a circular dependency issue, or a very messy config file change.
Solution - Issue 1: Move the config loading from init into runtime, incurs a minimal config load time penalty, removes this dependency issue.
Issue 2: Config mutation during initialization
Inside cve_generate_vdbs.py, once the configs are loaded, a check is run to see if the vdb and code search tools are active. This then modifies two config flags (
config.ignore_code_embedding = Trueandconfig.ignore_code_index = True). In anat serveenvironment, this config change could affect later jobs in a batch, which may cause unintended issues.Solution - Issue 2: Rewrite the config logic to compute the desired value, and use that computed value to decide vdb and tool activation. This avoids mutating the config, and the computed value remains contained within the run.