fix: implement ArtifactStore for retrievable tool output overflow#2149
fix: implement ArtifactStore for retrievable tool output overflow#2149praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
#2148) - Add concrete FileSystemArtifactStore implementing ArtifactStoreProtocol - Wire into tool_execution.py to spill large outputs instead of discarding - Add artifact retrieval tools (head, tail, grep, chunk, load) - Add ToolOutputConfig for configurable limits (max_bytes, retention_days) - Register retrieval tools dynamically on first artifact creation - Add garbage collection for old artifacts This preserves full tool outputs that exceed limits in retrievable artifacts instead of silently truncating and discarding the overflow.
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a concrete ChangesTool Output Artifact Spill
Sequence Diagram(s)sequenceDiagram
participant Agent
participant ToolExecutionMixin
participant FileSystemArtifactStore
participant LLM
Agent->>ToolExecutionMixin: execute tool call
ToolExecutionMixin->>ToolExecutionMixin: convert result to string
alt result exceeds tool_output_limit
ToolExecutionMixin->>FileSystemArtifactStore: store(full_output, ArtifactMetadata)
FileSystemArtifactStore-->>ToolExecutionMixin: ArtifactRef
ToolExecutionMixin->>ToolExecutionMixin: _register_artifact_tools (first spill)
ToolExecutionMixin->>LLM: truncated preview + ArtifactRef.to_inline()
LLM->>ToolExecutionMixin: call artifact_head / artifact_grep / artifact_chunk
ToolExecutionMixin->>FileSystemArtifactStore: head / grep / chunk(artifact_ref)
FileSystemArtifactStore-->>ToolExecutionMixin: partial content
ToolExecutionMixin-->>LLM: partial content
else result within limit
ToolExecutionMixin->>LLM: full result
end
Agent->>FileSystemArtifactStore: cleanup_old_artifacts() (on __del__)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-agents/praisonaiagents/agent/tool_execution.py (1)
393-400:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDict results can lose the artifact reference after spill.
When overflow happens,
truncatedmay includeartifact_ref.to_inline(), but for dict results Line 396 replaces the output with_truncate_dict_fields(...), dropping the reference and making spilled content undiscoverable by the model.🧠 Suggested fix
- if len(truncated) < len(result_str): + if len(truncated) < len(result_str): logging.debug(f"Truncated {function_name} output from {len(result_str)} to {len(truncated)} chars") # For dicts, truncate large string fields (e.g., raw_content from search) if isinstance(result, dict): max_field_chars = getattr(self, 'tool_output_limit', 16000) if not self.context_manager else None result = self._truncate_dict_fields(result, function_name, max_field_chars) + if 'artifact_ref' in locals() and artifact_ref is not None: + result["_artifact_ref"] = artifact_ref.to_dict() else: result = truncated🤖 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/praisonai-agents/praisonaiagents/agent/tool_execution.py` around lines 393 - 400, The issue is that when dict results are truncated due to overflow, the artifact reference that is included in the truncated string variable is being discarded when _truncate_dict_fields is called on line 396. Instead of replacing result with the output of _truncate_dict_fields, which loses the artifact reference, you need to preserve the artifact reference from the truncated string and ensure it is included in the final dict result. Modify the dict handling branch to extract or append the artifact reference from the truncated string so that the model can still discover the spilled content through the artifact reference.
🧹 Nitpick comments (2)
test_artifact_store.py (2)
41-41: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winIsolate artifact storage path in tests with
TemporaryDirectory.Using the default home directory can pollute local artifacts and create non-deterministic test behavior across runs.
🧪 Suggested fix
- store = FileSystemArtifactStore() + with tempfile.TemporaryDirectory() as tmp_dir: + store = FileSystemArtifactStore(base_dir=tmp_dir) + metadata = ArtifactMetadata( + agent_id="test_agent", + run_id="test_run", + tool_name="test_tool", + turn_id=1 + ) + large_content = "TEST" * 5000 + ref = store.store(large_content, metadata) + ...🤖 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 `@test_artifact_store.py` at line 41, The FileSystemArtifactStore instantiation is using the default home directory path which can pollute local artifacts and cause non-deterministic test behavior. To fix this, wrap the test setup with Python's TemporaryDirectory context manager to create an isolated temporary directory, then pass this temporary directory path to the FileSystemArtifactStore constructor instead of using defaults, ensuring each test run is isolated and does not affect the actual file system.
20-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd an assertion for the actual spill-on-overflow execution path.
This test currently validates store APIs directly, but it does not verify the changed integration behavior in tool execution (spill + retrievable reference/tools), which is the core risk in this PR.
Also applies to: 56-88
🤖 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 `@test_artifact_store.py` around lines 20 - 34, The test_artifact_storage test currently only validates the configuration setup but does not verify the actual spill-on-overflow behavior during tool execution. Add assertions that execute the agent (by calling agent.run() or similar) with input that triggers the generate_large_output tool, then verify that the returned output exceeds the max_bytes threshold and is properly stored as an artifact with a retrievable reference. Assert that the artifact reference is accessible and can be retrieved, confirming the integration of the spill-on-overflow mechanism in the actual tool execution path rather than just validating the ToolOutputConfig APIs.
🤖 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/praisonai-agents/praisonaiagents/agent/agent.py`:
- Line 595: The tool_output parameter added to the constructor is not being
propagated in the clone_for_channel() method, causing cloned agents to lose this
configuration and fall back to default behavior. Locate the clone_for_channel()
method and ensure that when it creates cloned agent instances, it passes the
self.tool_output value to the new agent constructor to maintain the tool output
handling and artifact storage configuration across cloned agents.
- Around line 5565-5571: The exception handler for cleanup_old_artifacts in the
artifact store cleanup block is silently suppressing errors with a bare except
pass statement. Replace this silent exception handling with proper debug logging
that captures the actual exception and includes agent context such as the agent
name or session ID to aid in debugging artifact cleanup issues like retention or
permission problems. This will help diagnose failures without silencing them
completely.
- Around line 1510-1554: The tool_output_limit is being set from the tool_output
config parameter in the ToolOutputConfig handling block (where
self.tool_output_limit is assigned from config.max_bytes), but this value is
later overwritten at line 1793. To fix this, modify the code at line 1793 to
only set tool_output_limit if it hasn't already been customized through the
tool_output parameter. You can do this by checking if the current
self.tool_output_limit differs from the DEFAULT_TOOL_OUTPUT_LIMIT before
overwriting it, or by tracking whether a custom tool_output_limit was explicitly
provided and skipping the assignment in that case.
In `@src/praisonai-agents/praisonaiagents/agent/tool_execution.py`:
- Around line 39-44: The tools_to_add list in the tool_execution block is
incomplete and excludes artifact_load and artifact_list even though these tools
are available in artifact_tools.py. Add artifact_tools.artifact_load and
artifact_tools.artifact_list to the tools_to_add list alongside the existing
artifact_head, artifact_tail, artifact_grep, and artifact_chunk entries to
ensure agents have access to all available artifact retrieval tools, including
full-content load and listing functionality.
In `@src/praisonai-agents/praisonaiagents/config/feature_configs.py`:
- Line 45: Remove the import of FileSystemArtifactStore from module-level scope
(line 45 in feature_configs.py) and convert it to a lazy import instead.
Identify the function or method where FileSystemArtifactStore is actually
instantiated or used, and move the import statement inside that function/method.
This ensures that the concrete filesystem implementation is only loaded at
runtime when actually needed, keeping the core config module free of heavy
implementations and maintaining protocol-driven architecture as per coding
guidelines.
- Around line 1510-1571: The ToolOutputConfig dataclass does not validate its
input parameters, allowing invalid configurations like max_bytes <= 0, negative
retention_days, or unsupported direction values to be accepted and cause runtime
failures later. Add a __post_init__ method to the ToolOutputConfig class to
validate that max_bytes is positive, retention_days is non-negative, and
direction is one of the supported values ("head", "tail", or "both"), raising
appropriate ValueError exceptions with clear messages for any invalid values.
In `@src/praisonai-agents/praisonaiagents/context/artifact_store.py`:
- Around line 70-145: The store, load, delete, and cleanup_old_artifacts methods
perform stateful operations but do not emit EventBus lifecycle events, reducing
observability and monitoring integration. For each of these methods, emit
appropriate events via the EventBus at key lifecycle points: emit an event at
the start of the operation, emit a success event upon completion with relevant
artifact metadata, and emit an error event if an exception occurs. Ensure the
events include sufficient context such as artifact_id, operation type, and
status to support monitoring and debugging.
- Around line 95-99: The redaction logic in the artifact_store.py file only
processes string content, allowing structured payloads like dictionaries and
lists to bypass redaction before being serialized at line 91. Additionally, the
summary generation at line 125 uses unredacted content. To fix this, refactor
the redaction logic to handle all content types before serialization occurs.
Apply the secret pattern matching to the content regardless of whether it is a
string, dictionary, or list, and ensure the summary generation at line 125 also
applies the same redaction to any generated summary text to prevent secret
leakage through both the stored artifact and its summary representation.
- Around line 156-158: The artifact store directly uses ref.path without
validating it's within the store root directory, allowing arbitrary file
read/delete operations outside the artifact directory. For all artifact
retrieval and deletion entry points (at lines 156, 185-187, 208-210, 239-241,
285-287, and 308-309), resolve the provided path to an absolute path and verify
it's contained within the artifact store root directory using path resolution
methods that prevent directory traversal attacks. Only proceed with file
operations if the resolved path is confirmed to be within the store root;
otherwise raise an appropriate security exception. This applies to all locations
where ref.path is directly used for file I/O operations.
- Around line 347-360: The list_artifacts method's run_id filter is being
ignored when provided without an agent_id. The conditional logic at line 347
only branches on agent_id without considering the case where run_id is supplied
independently. Add an additional conditional branch after the elif agent_id
block to handle the case where only run_id is provided. When run_id is set
without agent_id, iterate through all agent directories in base_dir and for each
agent, include only the run directory that matches the specified run_id in the
search_paths list, ensuring the run_id filter is properly applied regardless of
whether agent_id is also provided.
In `@src/praisonai-agents/praisonaiagents/tools/artifact_tools.py`:
- Around line 15-23: The module-level `_artifact_store` variable and
`set_artifact_store()` function create process-global state that is shared
across all agents, causing one agent to overwrite another agent's retrieval
backend. Replace the global `_artifact_store` variable with an agent-scoped
storage mechanism such as a dictionary keyed by agent identifiers, and modify
`set_artifact_store()` to accept an agent identifier parameter so each agent
maintains its own isolated artifact store. Update any retrieval functions that
access `_artifact_store` to accept and use the agent identifier to retrieve the
correct store instance for that specific agent.
- Around line 167-168: The artifact_load function creates a synthetic
ArtifactRef with hardcoded mime_type="text/plain" and empty checksum, which
causes JSON artifacts to fail deserialization and skips checksum verification.
Retrieve the actual artifact metadata (mime type and checksum) from storage
before constructing the ArtifactRef, or refactor to query the artifact store for
the proper ArtifactRef with correct metadata instead of creating a synthetic
one. Ensure the ArtifactRef passed to _artifact_store.load contains the real
mime type and checksum from the stored artifact.
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/agent/tool_execution.py`:
- Around line 393-400: The issue is that when dict results are truncated due to
overflow, the artifact reference that is included in the truncated string
variable is being discarded when _truncate_dict_fields is called on line 396.
Instead of replacing result with the output of _truncate_dict_fields, which
loses the artifact reference, you need to preserve the artifact reference from
the truncated string and ensure it is included in the final dict result. Modify
the dict handling branch to extract or append the artifact reference from the
truncated string so that the model can still discover the spilled content
through the artifact reference.
---
Nitpick comments:
In `@test_artifact_store.py`:
- Line 41: The FileSystemArtifactStore instantiation is using the default home
directory path which can pollute local artifacts and cause non-deterministic
test behavior. To fix this, wrap the test setup with Python's TemporaryDirectory
context manager to create an isolated temporary directory, then pass this
temporary directory path to the FileSystemArtifactStore constructor instead of
using defaults, ensuring each test run is isolated and does not affect the
actual file system.
- Around line 20-34: The test_artifact_storage test currently only validates the
configuration setup but does not verify the actual spill-on-overflow behavior
during tool execution. Add assertions that execute the agent (by calling
agent.run() or similar) with input that triggers the generate_large_output tool,
then verify that the returned output exceeds the max_bytes threshold and is
properly stored as an artifact with a retrievable reference. Assert that the
artifact reference is accessible and can be retrieved, confirming the
integration of the spill-on-overflow mechanism in the actual tool execution path
rather than just validating the ToolOutputConfig APIs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f4c5c01-9b8f-414b-b54f-40051cf24701
📒 Files selected for processing (7)
src/praisonai-agents/praisonaiagents/agent/agent.pysrc/praisonai-agents/praisonaiagents/agent/tool_execution.pysrc/praisonai-agents/praisonaiagents/config/feature_configs.pysrc/praisonai-agents/praisonaiagents/context/__init__.pysrc/praisonai-agents/praisonaiagents/context/artifact_store.pysrc/praisonai-agents/praisonaiagents/tools/artifact_tools.pytest_artifact_store.py
| runtime: Optional[Union[str, Dict[str, Any], 'AgentRuntimeConfig']] = None, # Model-scoped runtime configuration | ||
| interrupt_controller: Optional['InterruptController'] = None, # G2: Cooperative cancellation | ||
| tool_search: Optional[Union[bool, str, Dict[str, Any], 'ToolSearchConfig']] = False, # Progressive tool disclosure | ||
| tool_output: Optional[Union[bool, Dict[str, Any], 'ToolOutputConfig']] = None, # Tool output handling and artifact storage |
There was a problem hiding this comment.
Propagate tool_output in clone paths to avoid silent feature drop.
Line [595] adds tool_output to constructor, but clone_for_channel() (same file, Lines [2161]-[2247]) does not pass it. Cloned agents fall back to default truncation behavior and lose artifact spill configuration.
🧰 Tools
🪛 Ruff (0.15.17)
[error] 595-595: Undefined name ToolOutputConfig
(F821)
🤖 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/praisonai-agents/praisonaiagents/agent/agent.py` at line 595, The
tool_output parameter added to the constructor is not being propagated in the
clone_for_channel() method, causing cloned agents to lose this configuration and
fall back to default behavior. Locate the clone_for_channel() method and ensure
that when it creates cloned agent instances, it passes the self.tool_output
value to the new agent constructor to maintain the tool output handling and
artifact storage configuration across cloned agents.
| # Process tool_output config (artifact storage for large outputs) | ||
| self._artifact_store = None | ||
| self.tool_output_limit = DEFAULT_TOOL_OUTPUT_LIMIT | ||
|
|
||
| if tool_output is None or tool_output is False: | ||
| # Disabled - use default truncation only | ||
| pass | ||
| elif tool_output is True: | ||
| # Enabled with defaults | ||
| from ..config.feature_configs import ToolOutputConfig | ||
| from ..context.artifact_store import FileSystemArtifactStore | ||
| config = ToolOutputConfig() | ||
| self.tool_output_limit = config.max_bytes | ||
| if config.enable_artifacts: | ||
| self._artifact_store = config.artifact_store or FileSystemArtifactStore( | ||
| retention_days=config.retention_days, | ||
| redact_secrets=config.redact_secrets | ||
| ) | ||
| elif isinstance(tool_output, dict): | ||
| # Dict -> config overrides | ||
| from ..config.feature_configs import ToolOutputConfig | ||
| from ..context.artifact_store import FileSystemArtifactStore | ||
| config = ToolOutputConfig(**tool_output) | ||
| self.tool_output_limit = config.max_bytes | ||
| if config.enable_artifacts: | ||
| self._artifact_store = config.artifact_store or FileSystemArtifactStore( | ||
| retention_days=config.retention_days, | ||
| redact_secrets=config.redact_secrets | ||
| ) | ||
| else: | ||
| from ..config.feature_configs import ToolOutputConfig | ||
| if isinstance(tool_output, ToolOutputConfig): | ||
| config = tool_output | ||
| self.tool_output_limit = config.max_bytes | ||
| if config.enable_artifacts: | ||
| from ..context.artifact_store import FileSystemArtifactStore | ||
| self._artifact_store = config.artifact_store or FileSystemArtifactStore( | ||
| retention_days=config.retention_days, | ||
| redact_secrets=config.redact_secrets | ||
| ) | ||
| else: | ||
| raise TypeError( | ||
| "tool_output must be False/None, True, a dict of ToolOutputConfig fields, or ToolOutputConfig" | ||
| ) | ||
|
|
There was a problem hiding this comment.
tool_output.max_bytes is overwritten later, so custom limits don’t persist.
Lines [1522]/[1533]/[1543] set self.tool_output_limit, but Line [1793] later overwrites it with tool_output_limit derived from output config. This makes custom tool_output.max_bytes ineffective.
💡 Proposed fix
- self._artifact_store = None
- self.tool_output_limit = DEFAULT_TOOL_OUTPUT_LIMIT
+ self._artifact_store = None
+ _resolved_tool_output_limit = tool_output_limit
@@
- self.tool_output_limit = config.max_bytes
+ _resolved_tool_output_limit = config.max_bytes
@@
- self.tool_output_limit = config.max_bytes
+ _resolved_tool_output_limit = config.max_bytes
@@
- self.tool_output_limit = config.max_bytes
+ _resolved_tool_output_limit = config.max_bytes
@@
- self.tool_output_limit = tool_output_limit # Configurable tool output limit
+ self.tool_output_limit = _resolved_tool_output_limit # Configurable tool output limit🤖 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/praisonai-agents/praisonaiagents/agent/agent.py` around lines 1510 -
1554, The tool_output_limit is being set from the tool_output config parameter
in the ToolOutputConfig handling block (where self.tool_output_limit is assigned
from config.max_bytes), but this value is later overwritten at line 1793. To fix
this, modify the code at line 1793 to only set tool_output_limit if it hasn't
already been customized through the tool_output parameter. You can do this by
checking if the current self.tool_output_limit differs from the
DEFAULT_TOOL_OUTPUT_LIMIT before overwriting it, or by tracking whether a custom
tool_output_limit was explicitly provided and skipping the assignment in that
case.
| # Clean up old artifacts if artifact store is configured | ||
| artifact_store = getattr(self, "_artifact_store", None) | ||
| if artifact_store and hasattr(artifact_store, 'cleanup_old_artifacts'): | ||
| try: | ||
| artifact_store.cleanup_old_artifacts() | ||
| except Exception: | ||
| pass # Best effort cleanup |
There was a problem hiding this comment.
Avoid silent except: pass during artifact cleanup.
Lines [5568]-[5571] suppress all cleanup errors with no trace. At least debug-log the exception with agent context so retention/permission issues can be diagnosed.
As per coding guidelines, "Error messages must include remediation hints and propagate context (agent name, tool name, session ID) for debugging".
🧰 Tools
🪛 Ruff (0.15.17)
[error] 5570-5571: try-except-pass detected, consider logging the exception
(S110)
[warning] 5570-5570: Do not catch blind exception: Exception
(BLE001)
🤖 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/praisonai-agents/praisonaiagents/agent/agent.py` around lines 5565 -
5571, The exception handler for cleanup_old_artifacts in the artifact store
cleanup block is silently suppressing errors with a bare except pass statement.
Replace this silent exception handling with proper debug logging that captures
the actual exception and includes agent context such as the agent name or
session ID to aid in debugging artifact cleanup issues like retention or
permission problems. This will help diagnose failures without silencing them
completely.
Sources: Coding guidelines, Linters/SAST tools
| tools_to_add = [ | ||
| artifact_tools.artifact_head, | ||
| artifact_tools.artifact_tail, | ||
| artifact_tools.artifact_grep, | ||
| artifact_tools.artifact_chunk, | ||
| ] |
There was a problem hiding this comment.
Register all artifact retrieval tools, not just a subset.
artifact_load and artifact_list exist in artifact_tools.py but are never appended here, so agents can’t access full-content load or listing after spill.
➕ Suggested fix
tools_to_add = [
artifact_tools.artifact_head,
artifact_tools.artifact_tail,
artifact_tools.artifact_grep,
artifact_tools.artifact_chunk,
+ artifact_tools.artifact_load,
+ artifact_tools.artifact_list,
]📝 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.
| tools_to_add = [ | |
| artifact_tools.artifact_head, | |
| artifact_tools.artifact_tail, | |
| artifact_tools.artifact_grep, | |
| artifact_tools.artifact_chunk, | |
| ] | |
| tools_to_add = [ | |
| artifact_tools.artifact_head, | |
| artifact_tools.artifact_tail, | |
| artifact_tools.artifact_grep, | |
| artifact_tools.artifact_chunk, | |
| artifact_tools.artifact_load, | |
| artifact_tools.artifact_list, | |
| ] |
🤖 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/praisonai-agents/praisonaiagents/agent/tool_execution.py` around lines 39
- 44, The tools_to_add list in the tool_execution block is incomplete and
excludes artifact_load and artifact_list even though these tools are available
in artifact_tools.py. Add artifact_tools.artifact_load and
artifact_tools.artifact_list to the tools_to_add list alongside the existing
artifact_head, artifact_tail, artifact_grep, and artifact_chunk entries to
ensure agents have access to all available artifact retrieval tools, including
full-content load and listing functionality.
| from typing import TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
| from ..compaction.strategy import CompactionStrategy | ||
| from ..context.artifact_store import FileSystemArtifactStore |
There was a problem hiding this comment.
Move the concrete artifact-store import out of module scope.
Line [45] pulls a concrete filesystem implementation into a core config module at import time. This tightens coupling and adds unnecessary import-path work even when artifact spilling is disabled. Keep this as a lazy/runtime import in the wiring path instead.
As per coding guidelines, "Core SDK (praisonaiagents) must use Protocol-Driven Core architecture with only protocols, hooks, adapters, base classes, and decorators - no heavy implementations" and "All optional dependencies must use lazy imports - never import heavy dependencies at module level; instead import inside functions to avoid import-time performance impact".
🤖 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/praisonai-agents/praisonaiagents/config/feature_configs.py` at line 45,
Remove the import of FileSystemArtifactStore from module-level scope (line 45 in
feature_configs.py) and convert it to a lazy import instead. Identify the
function or method where FileSystemArtifactStore is actually instantiated or
used, and move the import statement inside that function/method. This ensures
that the concrete filesystem implementation is only loaded at runtime when
actually needed, keeping the core config module free of heavy implementations
and maintaining protocol-driven architecture as per coding guidelines.
Source: Coding guidelines
| # Redact secrets if enabled | ||
| if self.redact_secrets and isinstance(content, str): | ||
| for pattern in self._secret_patterns: | ||
| content = re.sub(pattern, "[REDACTED]", content) | ||
| content_bytes = content.encode("utf-8") |
There was a problem hiding this comment.
Redaction currently misses non-string payloads and can leak secrets via summary.
Line 96 only redacts when content is str. Structured payloads (dict, list) are serialized at Line 91 and bypass redaction; Line 125 then generates summary from unredacted content.
🛡️ Suggested fix
- # Redact secrets if enabled
- if self.redact_secrets and isinstance(content, str):
- for pattern in self._secret_patterns:
- content = re.sub(pattern, "[REDACTED]", content)
- content_bytes = content.encode("utf-8")
+ # Redact secrets if enabled (apply to textual representation for all JSON/text payloads)
+ content_text_for_summary = content if isinstance(content, str) else content_bytes.decode("utf-8", errors="replace")
+ if self.redact_secrets and mime_type in {"text/plain", "application/json"}:
+ redacted_text = content_text_for_summary
+ for pattern in self._secret_patterns:
+ redacted_text = re.sub(pattern, "[REDACTED]", redacted_text)
+ content_bytes = redacted_text.encode("utf-8")
+ content_text_for_summary = redacted_text
@@
- summary = generate_summary(content, max_chars=200)
+ summary = generate_summary(content_text_for_summary, max_chars=200)Also applies to: 125-125
🤖 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/praisonai-agents/praisonaiagents/context/artifact_store.py` around lines
95 - 99, The redaction logic in the artifact_store.py file only processes string
content, allowing structured payloads like dictionaries and lists to bypass
redaction before being serialized at line 91. Additionally, the summary
generation at line 125 uses unredacted content. To fix this, refactor the
redaction logic to handle all content types before serialization occurs. Apply
the secret pattern matching to the content regardless of whether it is a string,
dictionary, or list, and ensure the summary generation at line 125 also applies
the same redaction to any generated summary text to prevent secret leakage
through both the stored artifact and its summary representation.
| path = Path(ref.path) | ||
| if not path.exists(): | ||
| raise FileNotFoundError(f"Artifact not found: {ref.path}") |
There was a problem hiding this comment.
Constrain artifact paths to the store root before file I/O.
Line 156 and the other read/delete entry points trust ref.path as-is. Since retrieval tools accept artifact_path input, this enables arbitrary local file read/delete outside the artifact directory.
🔒 Suggested fix
+ def _resolve_artifact_path(self, ref: ArtifactRef) -> Path:
+ path = Path(ref.path).expanduser().resolve()
+ base = self.base_dir.expanduser().resolve()
+ if base not in path.parents:
+ raise PermissionError(f"Artifact path is outside configured store: {path}")
+ if path.suffix != ".artifact":
+ raise ValueError(f"Invalid artifact file extension: {path}")
+ return path
+
def load(self, ref: ArtifactRef) -> Any:
- path = Path(ref.path)
+ path = self._resolve_artifact_path(ref)
if not path.exists():
raise FileNotFoundError(f"Artifact not found: {ref.path}")Also applies to: 185-187, 208-210, 239-241, 285-287, 308-309
🤖 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/praisonai-agents/praisonaiagents/context/artifact_store.py` around lines
156 - 158, The artifact store directly uses ref.path without validating it's
within the store root directory, allowing arbitrary file read/delete operations
outside the artifact directory. For all artifact retrieval and deletion entry
points (at lines 156, 185-187, 208-210, 239-241, 285-287, and 308-309), resolve
the provided path to an absolute path and verify it's contained within the
artifact store root directory using path resolution methods that prevent
directory traversal attacks. Only proceed with file operations if the resolved
path is confirmed to be within the store root; otherwise raise an appropriate
security exception. This applies to all locations where ref.path is directly
used for file I/O operations.
| elif agent_id: | ||
| agent_path = self.base_dir / agent_id | ||
| if agent_path.exists(): | ||
| search_paths = [p for p in agent_path.iterdir() if p.is_dir()] | ||
| else: | ||
| search_paths = [] | ||
| else: | ||
| # Search all | ||
| search_paths = [] | ||
| for agent_dir in self.base_dir.iterdir(): | ||
| if agent_dir.is_dir(): | ||
| for run_dir in agent_dir.iterdir(): | ||
| if run_dir.is_dir(): | ||
| search_paths.append(run_dir) |
There was a problem hiding this comment.
run_id filter is ignored unless agent_id is also set.
The list_artifacts() contract accepts independent optional filters, but Line 347 branches only on agent_id; supplying only run_id scans all runs without filtering.
✅ Suggested fix
- elif agent_id:
+ elif agent_id:
agent_path = self.base_dir / agent_id
if agent_path.exists():
search_paths = [p for p in agent_path.iterdir() if p.is_dir()]
else:
search_paths = []
else:
# Search all
search_paths = []
for agent_dir in self.base_dir.iterdir():
if agent_dir.is_dir():
for run_dir in agent_dir.iterdir():
if run_dir.is_dir():
- search_paths.append(run_dir)
+ if run_id is None or run_dir.name == run_id:
+ search_paths.append(run_dir)📝 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.
| elif agent_id: | |
| agent_path = self.base_dir / agent_id | |
| if agent_path.exists(): | |
| search_paths = [p for p in agent_path.iterdir() if p.is_dir()] | |
| else: | |
| search_paths = [] | |
| else: | |
| # Search all | |
| search_paths = [] | |
| for agent_dir in self.base_dir.iterdir(): | |
| if agent_dir.is_dir(): | |
| for run_dir in agent_dir.iterdir(): | |
| if run_dir.is_dir(): | |
| search_paths.append(run_dir) | |
| elif agent_id: | |
| agent_path = self.base_dir / agent_id | |
| if agent_path.exists(): | |
| search_paths = [p for p in agent_path.iterdir() if p.is_dir()] | |
| else: | |
| search_paths = [] | |
| else: | |
| # Search all | |
| search_paths = [] | |
| for agent_dir in self.base_dir.iterdir(): | |
| if agent_dir.is_dir(): | |
| for run_dir in agent_dir.iterdir(): | |
| if run_dir.is_dir(): | |
| if run_id is None or run_dir.name == run_id: | |
| search_paths.append(run_dir) |
🤖 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/praisonai-agents/praisonaiagents/context/artifact_store.py` around lines
347 - 360, The list_artifacts method's run_id filter is being ignored when
provided without an agent_id. The conditional logic at line 347 only branches on
agent_id without considering the case where run_id is supplied independently.
Add an additional conditional branch after the elif agent_id block to handle the
case where only run_id is provided. When run_id is set without agent_id, iterate
through all agent directories in base_dir and for each agent, include only the
run directory that matches the specified run_id in the search_paths list,
ensuring the run_id filter is properly applied regardless of whether agent_id is
also provided.
| # Module-level artifact store reference | ||
| _artifact_store = None | ||
|
|
||
|
|
||
| def set_artifact_store(store): | ||
| """Set the global artifact store for retrieval tools.""" | ||
| global _artifact_store | ||
| _artifact_store = store | ||
|
|
There was a problem hiding this comment.
Avoid module-global artifact store state across agents.
_artifact_store is process-global and overwritten by set_artifact_store(). In multi-agent runs, one agent can change another agent’s retrieval backend, causing cross-agent data exposure and nondeterministic behavior.
🤖 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/praisonai-agents/praisonaiagents/tools/artifact_tools.py` around lines 15
- 23, The module-level `_artifact_store` variable and `set_artifact_store()`
function create process-global state that is shared across all agents, causing
one agent to overwrite another agent's retrieval backend. Replace the global
`_artifact_store` variable with an agent-scoped storage mechanism such as a
dictionary keyed by agent identifiers, and modify `set_artifact_store()` to
accept an agent identifier parameter so each agent maintains its own isolated
artifact store. Update any retrieval functions that access `_artifact_store` to
accept and use the agent identifier to retrieve the correct store instance for
that specific agent.
| ref = ArtifactRef(path=artifact_path, summary="", size_bytes=0, mime_type="text/plain") | ||
| return _artifact_store.load(ref) |
There was a problem hiding this comment.
artifact_load bypasses MIME/checksum metadata, so load semantics are incorrect.
Line 167 constructs a synthetic ArtifactRef with mime_type="text/plain" and empty checksum. JSON artifacts won’t deserialize and checksum verification is skipped.
🧩 Suggested fix
`@tool`("artifact_load")
def artifact_load(
artifact_path: str
) -> Any:
@@
try:
- ref = ArtifactRef(path=artifact_path, summary="", size_bytes=0, mime_type="text/plain")
+ meta = {}
+ meta_path = artifact_path.replace(".artifact", ".meta.json")
+ try:
+ import json
+ from pathlib import Path
+ p = Path(meta_path)
+ if p.exists():
+ meta = json.loads(p.read_text())
+ except Exception:
+ meta = {}
+
+ ref = ArtifactRef(
+ path=artifact_path,
+ summary="",
+ size_bytes=meta.get("size_bytes", 0),
+ mime_type=meta.get("mime_type", "application/octet-stream"),
+ checksum=meta.get("checksum", ""),
+ artifact_id=meta.get("artifact_id", ""),
+ agent_id=meta.get("agent_id", ""),
+ run_id=meta.get("run_id", ""),
+ tool_name=meta.get("tool_name"),
+ turn_id=meta.get("turn_id", 0),
+ )
return _artifact_store.load(ref)🤖 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/praisonai-agents/praisonaiagents/tools/artifact_tools.py` around lines
167 - 168, The artifact_load function creates a synthetic ArtifactRef with
hardcoded mime_type="text/plain" and empty checksum, which causes JSON artifacts
to fail deserialization and skips checksum verification. Retrieve the actual
artifact metadata (mime type and checksum) from storage before constructing the
ArtifactRef, or refactor to query the artifact store for the proper ArtifactRef
with correct metadata instead of creating a synthetic one. Ensure the
ArtifactRef passed to _artifact_store.load contains the real mime type and
checksum from the stored artifact.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Architecture Review & Fixes ✅Completed Tasks
Summary of ChangesSecurity Fixes:
Architecture Fixes: Functional Fixes: Files Modified
Test Results✅ Artifact storage tests pass successfully Final VerdictThe implementation is now architecturally sound and secure. All critical issues identified by reviewers have been addressed:
Recommendation: ✅ Ready to merge - All critical issues resolved and tests passing. |
…tore - Add path traversal protection by validating artifact paths are within store directory - Replace global artifact store with thread-local storage for multi-agent support - Preserve artifact references through context manager truncation - Add missing artifact_load and artifact_list tools to registration - Fix tool_output_limit being overwritten by tracking custom config - Propagate tool_output config in clone_for_channel for cloned agents - Add proper error logging for artifact cleanup failures - Add validation for ToolOutputConfig parameters - Fix run_id filter in list_artifacts when agent_id not provided - Fix cleanup_old_artifacts to handle days=0 correctly - Fix secret redaction to cover JSON and list content - Fix artifact_load to use correct mime_type from metadata Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
| if _artifact_store is None: | ||
| return [{"error": "Artifact store not available"}] |
There was a problem hiding this comment.
NameError crash in artifact_grep and artifact_list
Both artifact_grep (line 119) and artifact_list (line 241) reference _artifact_store — a name that does not exist at module scope. The module-level variable is _artifact_stores (the threading.local object), and the correct accessor is _get_artifact_store(). Every call to either tool will immediately raise NameError: name '_artifact_store' is not defined, making these two tools completely non-functional. The other four tools (artifact_head, artifact_tail, artifact_chunk, artifact_load) correctly call _get_artifact_store() first.
| else: | ||
| truncated = result_str | ||
|
|
||
| if self.context_manager and hasattr(self, '_truncate_tool_output'): | ||
| # Use context-aware truncation if available, but preserve artifact reference | ||
| if artifact_ref: | ||
| # Extract the artifact reference from the truncated string | ||
| artifact_inline = artifact_ref.to_inline() | ||
| # Remove the artifact reference before context truncation | ||
| truncated_without_ref = truncated.replace(artifact_inline, "").rstrip() | ||
| # Apply context truncation | ||
| truncated_without_ref = self._truncate_tool_output(function_name, truncated_without_ref) | ||
| # Re-append the artifact reference | ||
| truncated = f"{truncated_without_ref}\n\n{artifact_inline}" | ||
| else: | ||
| truncated = result_str | ||
| truncated = self._truncate_tool_output(function_name, truncated) |
There was a problem hiding this comment.
UnboundLocalError when output fits within limit and context manager is active
artifact_ref is only assigned inside the if len(result_str) > limit: branch (line 348). When tool output is within the configured limit, execution takes the else: truncated = result_str path and artifact_ref is never set. The subsequent check at line 392 (if artifact_ref:) then raises UnboundLocalError: local variable 'artifact_ref' referenced before assignment for every tool call where both conditions hold: result fits in the limit AND self.context_manager is active. The fix is to initialise artifact_ref = None immediately before the if len(result_str) > limit: block.
| # Check if we need to spill to artifact store | ||
| if len(result_str) > limit: | ||
| # Try to use artifact store if available | ||
| artifact_ref = None |
There was a problem hiding this comment.
artifact_ref to None before the size-check branch so the variable is always defined when the context-manager block reads it, preventing UnboundLocalError on outputs that fit within the limit.
| # Check if we need to spill to artifact store | |
| if len(result_str) > limit: | |
| # Try to use artifact store if available | |
| artifact_ref = None | |
| # Check if we need to spill to artifact store | |
| artifact_ref = None | |
| if len(result_str) > limit: | |
| # Try to use artifact store if available |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Fixes #2148
Summary
Implementation Details
Core Components
FileSystemArtifactStore: Concrete implementation of ArtifactStoreProtocol
Tool Execution Integration: Modified tool_execution.py to:
Artifact Retrieval Tools: Added agent-callable tools:
Configuration: Added ToolOutputConfig for:
Backward Compatibility
Test Plan
This solves the data loss issue where large tool outputs were silently truncated and the overflow was unrecoverable.
Summary by CodeRabbit
Release Notes
New Features
Tests