Skip to content

Added claudeAgent and memory manager#267

Open
kosstbarz wants to merge 12 commits intomainfrom
kosst/claude-agent2
Open

Added claudeAgent and memory manager#267
kosstbarz wants to merge 12 commits intomainfrom
kosst/claude-agent2

Conversation

@kosstbarz
Copy link
Contributor

No description provided.

@kosstbarz kosstbarz requested review from a team, mare5x and pickleerik March 18, 2026 15:48
@kosstbarz kosstbarz requested a review from catstrike March 19, 2026 13:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new Claude-based executor under databao.agent.executors.claude, including a filesystem-backed “memory” mechanism and dbt-project context retrieval prompts. It also adds required runtime dependencies and extends agent configuration to point at a dbt project.

Changes:

  • Add ClaudeAgent executor (Claude Agent SDK + MCP tools) with DuckDB SQL execution and result submission flow.
  • Add MemoryManager and prompt templates (system_prompt.jinja, retriever_prompt.jinja) for memory + dbt context retrieval.
  • Add anyio dependency and extend AgentConfig with dbt_path.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
uv.lock Adds anyio and updates locked dependency versions (incl. DuckDB).
pyproject.toml Declares anyio>=4.11.0 runtime dependency.
databao/agent/executors/claude/system_prompt.jinja New system prompt template including memory index section.
databao/agent/executors/claude/retriever_prompt.jinja New schema/context retriever prompt template.
databao/agent/executors/claude/memory_manager.py New file-based memory index + CRUD operations.
databao/agent/executors/claude/claude_agent.py New Claude executor implementation, MCP tools, and execution loop.
databao/agent/configs/agent.py Adds `dbt_path: Path

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +46 to +48
"""Register additional tools to be available during execution."""
# TODO: add to allowed tool list

Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register_tools is currently a no-op, but Agent.add_mcp() calls executor.register_tools(...) to make MCP tools available. As written, ClaudeAgent will silently ignore extra tools, which will break MCP integrations for this executor. Either implement tool registration (and plumb the tools into allowed_tools / MCP server wiring), or raise NotImplementedError with a clear message that extra tools aren't supported.

Suggested change
"""Register additional tools to be available during execution."""
# TODO: add to allowed tool list
"""Register additional tools to be available during execution.
Note:
ClaudeAgent currently does not support dynamic tool or MCP server
registration. Callers that need MCP integrations should use an
executor that implements tool registration.
"""
raise NotImplementedError(
"ClaudeAgent does not currently support registering additional tools "
"or MCP integrations via `register_tools`."
)

Copilot uses AI. Check for mistakes.

def drop_last_opa_group(self, cache: "Cache", n: int = 1) -> None:
"""Drop last n groups of operations from the message history."""
# TODO: implement
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop_last_opa_group is currently unimplemented, but Thread.drop() relies on this method to keep cache/message history consistent when dropping past operations. Please implement cache history removal (similar to GraphExecutor.drop_last_opa_group) or explicitly raise to prevent inconsistent thread state.

Suggested change
# TODO: implement
# NOTE: This executor does not currently support dropping past operation groups.
# Implement cache/message history removal here (mirroring GraphExecutor.drop_last_opa_group)
# before enabling Thread.drop() with ClaudeAgent. For now, fail fast to avoid
# inconsistent cache/thread state.
raise NotImplementedError(
"ClaudeAgent.drop_last_opa_group is not implemented. "
"Thread.drop() cannot be used safely with ClaudeAgent until this is implemented."
)

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +55
path = self.project_path / "memories" / folder / filename
path.write_text(content)
entries.append({"name": name, "path": f"memories/{folder}/{filename}"})
self._write_entries(entries)
return f"Memory '{name}' saved to memories/{folder}/{filename}."
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filename is used directly to build a filesystem path. A value like "../pyproject.toml" (or an absolute path) can escape the intended memories/<folder>/ directory and overwrite arbitrary files under project_path. Sanitize/validate filename (e.g., disallow path separators, resolve and enforce it stays within the memories dir) before writing.

Suggested change
path = self.project_path / "memories" / folder / filename
path.write_text(content)
entries.append({"name": name, "path": f"memories/{folder}/{filename}"})
self._write_entries(entries)
return f"Memory '{name}' saved to memories/{folder}/{filename}."
# Sanitize and validate filename to prevent directory traversal and escapes
filename_path = Path(filename)
# Disallow absolute paths and parent directory components
if filename_path.is_absolute() or ".." in filename_path.parts:
return "Error: invalid filename."
# Disallow path separators by requiring the filename to be a simple name
safe_filename = filename_path.name
if safe_filename != filename or not safe_filename:
return "Error: invalid filename."
base_dir = (self.project_path / "memories" / folder).resolve()
path = (base_dir / safe_filename).resolve()
# Ensure the final path is within the intended base_dir
if base_dir not in path.parents and base_dir != path.parent:
return "Error: invalid filename."
path.write_text(content)
entries.append({"name": name, "path": f"memories/{folder}/{safe_filename}"})
self._write_entries(entries)
return f"Memory '{name}' saved to memories/{folder}/{safe_filename}."

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +63
(self.project_path / entry["path"]).unlink(missing_ok=True)
self._write_entries([e for e in entries if e["name"] != name])
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete() trusts entry['path'] from the index and unlinks it. If the index file is edited (or corrupted), this could delete files outside the memories/ directory. Consider validating that the resolved path stays under project_path / 'memories' before unlinking.

Copilot uses AI. Check for mistakes.
entry = next((e for e in entries if e["name"] == name), None)
if not entry:
return f"Error: memory '{name}' not found. Use add_memory to create it."
(self.project_path / entry["path"]).write_text(content)
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update() trusts entry['path'] from the index and writes to it. If the index file is edited (or corrupted), this can overwrite files outside the intended memories/ directory. Validate the resolved path is within project_path / 'memories' before writing.

Copilot uses AI. Check for mistakes.
kosstbarz and others added 7 commits March 20, 2026 14:20
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@gasparian gasparian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here are some refactoring suggestions, which I've put to the separate PR, to your current branch:
#270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants