-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor application structure to use Agno #429
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: main
Are you sure you want to change the base?
Conversation
…and file operations
This PR refactors class attribute assignments by removing unsupported inline type declarations. The changes ensure compatibility with environments that do not support variable type annotations on assignment. - Unsupported `Type` declaration: The code originally declared types inline (e.g., `cls._tools: list[BaseTool] = ...`), which is not supported in all Python versions. The fix removes the `: type` portion, leaving standard assignments for `_tools`, `_llm`, and `_model`, while preserving functionality. > This Autofix was generated by AI. Please review the change before merging.
… and branch deletion
…roved job summaries
Refactor Docker workflows and improve CI configurations
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/chattr/app/settings.py (1)
40-65: Critical:MCPSettings.pathasFilePathmakes missingmcp.jsona hard error
MCPSettings.pathis annotated asFilePathwith a default ofPath.cwd() / "mcp.json", and you have validators that only warn when the file is missing:path: FilePath = Field(default_factory=lambda: Path.cwd() / "mcp.json") @model_validator(mode="after") def is_exists(self) -> Self: if not self.path.exists(): logger.warning("`mcp.json` not found.")In Pydantic v2,
FilePathis defined asAnnotated[Path, PathType('file')], which requires that the path point to an existing file at validation time. (docs.pydantic.dev) Ifmcp.jsonis absent in the working directory, model construction will fail before youris_exists/is_valid_schemevalidators run, contradicting the “just warn if missing” behavior and preventingSettings()from being created on a fresh install.This also makes
is_existseffectively unreachable in the missing‑file case.I recommend changing the type to
Path(or another non‑“must exist” path type) and leaving existence checks to your validators:-from pydantic import ( - ..., - FilePath, - ... -) +from pydantic import ( + ..., + ... +) @@ -class MCPSettings(BaseModel): - """Settings for MCP configuration.""" - - path: FilePath = Field(default_factory=lambda: Path.cwd() / "mcp.json") +class MCPSettings(BaseModel): + """Settings for MCP configuration.""" + + path: Path = Field(default_factory=lambda: Path.cwd() / "mcp.json")With this change:
Settings()can be instantiated even whenmcp.jsonis missing.is_existswill log the warning as intended.is_valid_schemewill still enforce structure when the file is present.Because a very similar concern was already raised in an earlier review, this is both a confirmation and a reiteration.
To reproduce and verify the behavior locally:
#!/bin/bash # Run in a clean directory with no mcp.json present. python - << 'PY' from pathlib import Path from pydantic import BaseModel, FilePath, Field class MCPSettings(BaseModel): path: FilePath = Field(default_factory=lambda: Path.cwd() / "mcp.json") print("Attempting to construct MCPSettings without mcp.json present...") try: MCPSettings() except Exception as e: print("Got exception:", repr(e)) PY
🧹 Nitpick comments (4)
src/chattr/app/settings.py (4)
19-24: Consider avoidingload_dotenv()at import timeCalling
load_dotenv()at module import mutates process environment for any importer and duplicates the.envhandling you already configure viaSettings.model_config.env_file. Consider movingload_dotenv()into the application entrypoint (or removing it and relying solely onenv_file) so that importingchattr.app.settingsis side‑effect free.
36-38: VerifyHttpUrldefault forhttp://localhost:6333
VectorDatabaseSettings.urlusesHttpUrl("http://localhost:6333")as a default. In Pydantic,HttpUrlenforces http/https schemes and, per docs, may require a TLD on the host; some versions treatlocalhostas invalid inHttpUrlwhileAnyHttpUrlis more permissive for local hosts. (docs.pydantic.dev)If you ever hit validation errors around this default, consider switching the type to
AnyHttpUrland using a plain string default:-from pydantic import ( - ..., - HttpUrl, - ... -) +from pydantic import ( + ..., + AnyHttpUrl, + ... +) -class VectorDatabaseSettings(BaseModel): - ... - url: HttpUrl = HttpUrl("http://localhost:6333") +class VectorDatabaseSettings(BaseModel): + ... + url: AnyHttpUrl = "http://localhost:6333"
60-65: Optionally wrapMCPSchemevalidation errors for a clearer message
is_valid_schemecurrently does:if self.path and self.path.exists(): _ = MCPScheme.model_validate_json(self.path.read_text())Any JSON parse or schema error will bubble up as a raw
ValidationErrorfromMCPScheme. That’s functionally fine, but you might consider catching it to add context like “invalid MCP config at {path}” while re‑raising the original error for easier debugging:from pydantic import ValidationError @model_validator(mode="after") def is_valid_scheme(self) -> Self: if self.path and self.path.exists(): try: MCPScheme.model_validate_json(self.path.read_text()) except ValidationError as exc: raise ValueError(f"Invalid MCP config in {self.path}") from exc return selfNot required, but improves UX when the config is malformed.
68-128: ClarifyDirectorySettings.basesemantics and computed field typesOverall,
DirectorySettingsis nicely structured andcreate_missing_dirsis clear. A couple of points worth considering:
base: DirectoryPathvs creating a new base directory
baseis typed asDirectoryPath:base: DirectoryPath = Field(default_factory=Path.cwd, frozen=True)In Pydantic,
DirectoryPathrequires that the path already exist and be a directory. (docs.pydantic.dev) That means if a user setsDIRECTORY__BASEto a path that does not exist yet, validation will fail beforecreate_missing_dirscan run, even though the docstring says “Ensure that all specified directories exist, creating them if necessary.”If you intend
DirectorySettingsto be able to create a new base directory, switchingbaseto a plainPath(orNewPathif you want “must not exist yet”) and leaving creation logic increate_missing_dirswould better match the documented behavior:from pydantic import (
- DirectoryPath,
- DirectoryPath,
NewPath, # optionally, if you want "new path" semantics
...
)class DirectorySettings(BaseModel):
"""Settings for application directories."""
base: DirectoryPath = Field(default_factory=Path.cwd, frozen=True)
base: Path = Field(default_factory=Path.cwd, frozen=True)
Computed fields annotated as
DirectoryPathThe
log,assets,audio,video, andpromptsproperties are@computed_fields annotated asDirectoryPathbut are created bycreate_missing_dirslater. In practice, Pydantic doesn’t enforceDirectoryPath’s “must exist” constraint on computed fields, so this works, but the annotation can be slightly misleading to readers.If you want the type hints to reflect actual semantics without implying pre‑existence, consider annotating these as
Pathinstead while keeping the creation logic as‑is.Neither of these is blocking, but aligning the types with the creation behavior will make the model less surprising.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.envrc(1 hunks)src/chattr/app/settings.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .envrc
🧰 Additional context used
🧬 Code graph analysis (1)
src/chattr/app/settings.py (1)
src/chattr/app/scheme.py (1)
MCPScheme(35-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Image / Build and push Docker image to ghcr.io
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (6)
src/chattr/app/settings.py (6)
26-38: Memory and vector DB settings definitions look solid
MemorySettingsandVectorDatabaseSettingsare straightforward and readable. The defaults (collection_name,embedding_dims,name) look reasonable for a local setup.
113-128: Directory creation + logging behavior looks goodThe
create_missing_dirsloop overbase,assets,log,audio,video, andpromptsis clear and guarded withtry/except OSError, logging both successful creation and failures before re‑raising. That fail‑fast behavior makes sense for an app‑level settings object.
131-168: Model credential validation logic is consistent
ModelSettingscleanly enforces that ifurlis set, bothapi_keyandnamemust be provided, with clear error messages that match yourenv_nested_delimiterconvention (MODEL__API_KEY,MODEL__NAME). Temperature bounds are also correctly constrained viage/le.No functional issues spotted here.
170-175: CharacterSettings is a minimal, flexible extension pointIntroducing
CharacterSettingswith an optionalnameis a reasonable way to keep persona/character configuration separate from model and memory settings, and leaves room for future fields without impacting core config.
179-194: Settings aggregation and env config look coherentThe
Settingsclass wires togetherdirectory,model,memory,vector_database,mcp,character, anddebugwith sensibledefault_factoryusage and amodel_configthat matches the documented env key patterns (e.g.,MODEL__NAME,DIRECTORY__BASE).This structure should make it straightforward to reason about and override each sub‑settings group via environment variables.
197-200: Rich-based self-inspection in__main__is handyUsing
rich.printonSettings().model_dump_json(indent=4)behindif __name__ == "__main__":is a convenient, low‑friction way to inspect the effective configuration, and keeps the Rich dependency out of import‑time side effects.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|



This pull request introduces several updates and improvements to CI/CD workflows, repository configuration, and project automation. The changes focus on enhancing Docker image builds, workflow flexibility, automation for versioning and releases, and repository hygiene.
CI/CD Workflow Enhancements:
.github/workflows/.docker.yaml. This includes using the correct credentials per registry, passing install source and Python version as build arguments, and summarizing Dockerfile check results. [1] [2] [3] [4] [5] [6] [7] [8] [9].github/workflows/build.yamlby updating permissions, ensuring the project version matches the branch/tag, and automating commits and tags when versions diverge. [1] [2] [3].github/workflows/release.yamlby removing the HuggingFace job and ensuring Docker images are built from the released version. [1] [2]Automation and Repository Management:
.github/workflows/ci_tools.yamlto respond to specific issue comments and run the opencode action. [1] [2]renovate[bot]actor. [1] [2]Repository and Project Hygiene:
.gitattributesfile to enforce LF line endings..envrcfile, likely to standardize environment management..ideaproject files to exclude thelogsdirectory and added a run configuration for "Vocalizr MCP". [1] [2]These changes collectively improve CI/CD reliability, automation, and maintainability across the project.
Summary by Sourcery
Refactor the Chattr application to leverage the Agno multi-agent framework, integrate advanced tooling and vector knowledge stores, and overhaul CI/CD, Docker, and project configuration for greater automation and maintainability.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: