Conversation
|
Warning Rate limit exceeded@KaQuMiQ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/draive/postgres/memory.py (1)
95-114: Fix inconsistent connection usage.Line 99 uses
PostgresConnection.execute()while line 118 (same function) usesconnection.execute(). Within the explicit connection context, all execute calls should use theconnectioninstance to ensure they run within the transaction.Apply this diff to fix the inconsistency:
if variables is not None: ctx.log_info(f"...remembering {len(variables)} variables...") - await PostgresConnection.execute( + await connection.execute( """ INSERT INTO memories_variables (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(1 hunks)src/draive/postgres/configuration.py(5 hunks)src/draive/postgres/instructions.py(5 hunks)src/draive/postgres/memory.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports from the draive package (e.g., from draive import ...), not relative imports
Follow Ruff import ordering: standard library, third-party, then local imports
Use Python 3.12+ typing features (e.g., | unions, PEP 695 generics)
Prefer abstract/base collection types (Sequence, Mapping, Set, Iterable) over concrete list, dict, set in type hints
Define and raise custom exception types for specific error cases
Use NumPy docstring convention for all functions, classes, and methods
Skip module-level docstrings unless explicitly requested
Docstrings should include Parameters, Returns, Raises, and Notes (if needed) sectionsFormat all Python code with Ruff formatter (run via
make format); do not use any other formatter
Files:
src/draive/postgres/instructions.pysrc/draive/postgres/configuration.pysrc/draive/postgres/memory.py
src/draive/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/draive/**/*.py: Import Haiway symbols directly asfrom haiway import State, ctx
Usectx.scope(...)to bind activeStateinstances; avoid global state
Route all logging throughctx.log_*and never use
Enforce strict typing on public APIs; avoid untyped definitions andAnyexcept at verified third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamicgetattrexcept at narrow boundaries
Use abstract immutable protocols (Mapping,Sequence,Iterable) instead of concretedict/list/setin public types
Usefinalwhere applicable; prefer composition over complex inheritance
Use precise unions (|), narrow withmatch/isinstance, and avoidcastunless provably safe and localized
Model immutable configuration/data ashaiway.State; avoid in‑place mutation and preferState.updated(...)/functional builders
Access active state viahaiway.ctxwithin async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance must use@statemethod
Add structured, concise logs around generation calls, tool dispatch, and provider requests/responses (without secrets); add metrics viactx.recordwhere applicable
All I/O is async; keep boundaries async, usectx.spawnfor detached tasks, and avoid custom threading
Translate provider/SDK errors into typed exceptions; never raise bareException; preserve context
Compose multimodal content withMultimodalContent.of(...); useResourceContent/Referencefor blobs; wrap custom data inArtifactContent(use hidden when needed)
Public symbols require NumPy-style docstrings with Parameters/Returns/Raises and rationale when non-obvious
Avoid docstrings for internal helpers; prefer self-explanatory names; skip module-level docstrings
Never log secrets or full request bodies containing keys/tokens
Read credentials from environment variables; resolve via helpers likegetenv_str
Files:
src/draive/postgres/instructions.pysrc/draive/postgres/configuration.pysrc/draive/postgres/memory.py
🧬 Code graph analysis (2)
src/draive/postgres/instructions.py (1)
src/draive/parameters/model.py (1)
from_json(705-720)
src/draive/postgres/memory.py (1)
src/draive/stages/stage.py (1)
execute(1978-2042)
🔇 Additional comments (11)
pyproject.toml (1)
8-8: LGTM! Appropriate patch version bump.The version increment from 0.87.0 to 0.87.1 correctly follows semantic versioning for bug fixes related to postgres migrations.
src/draive/postgres/configuration.py (4)
40-43: LGTM! Cache configuration is optimal.The
limit=1is correct here sincelisting()has no varying parameters, meaning there's only one possible cache key.
48-61: LGTM! Explicit type casting improves robustness.The
identifier::TEXTcast ensures consistent text handling and aligns with postgres best practices for explicit type conversion.
74-103: LGTM! Consistent text-based JSON handling.Casting
content::TEXTbeforejson.loads()is the correct pattern for safely handling JSONB data from postgres.
111-126: LGTM! Explicit parameter typing in INSERT.Using
$1::TEXTand$2::JSONBwithjson.dumps()ensures type safety and correct JSONB storage.src/draive/postgres/instructions.py (2)
57-86: LGTM! Defensive parsing with fallback values.The explicit TEXT casts combined with fallback values (
"{}"for meta,"[]"for arguments) provide robust handling of NULL or missing data.
138-163: LGTM! Explicit typing and proper NULL handling.The explicit parameter casts and
declaration.description or Noneensure proper NULL insertion for missing descriptions rather than empty strings.src/draive/postgres/memory.py (4)
14-33: Schema normalization looks appropriate.The split into
memories,memories_variables, andmemories_elementswith foreign key references provides a clean normalized structure. The absence of primary keys on the child tables allows multiple snapshots over time, which appears intentional.
145-162: LGTM! Defensive FK reference initialization.The
ON CONFLICT DO NOTHINGpattern ensures the base memory record exists before inserting child records, preventing FK violations. This is the correct approach for this normalized schema.
192-246: LGTM! Consistent TEXT casting for JSON retrieval.The
content::TEXTcast enables safejson.loads()decoding. The reversal logic correctly restores chronological order after the DESC sort.
266-294: LGTM! Correct latest-snapshot retrieval.The
DISTINCT ON (identifier)withORDER BY created DESCcorrectly retrieves the most recent variables snapshot. Thevariables::TEXTcast enables safe JSON decoding.
KaQuMiQ
force-pushed
the
feature/migrations
branch
2 times, most recently
from
99e6373 to
b11fcf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/draive/postgres/memory.py (1)
95-134: Fix missing base row insert and reuse the scoped connection.The first
remember()call inserts intomemories_elementswithout guaranteeing a parent row exists inmemories, so Line 118 will hit a foreign-key violation. In the same block (and again inmaintenance()),PostgresConnection.execute(...)opens a separate connection, so any failure rolls back the child rows but leaves the parent write committed—this is the same bug previously highlighted. Please ensure the root row is created inside the current transaction and issue all inserts through the acquiredconnection.@@ - async with Postgres.acquire_connection() as connection: - async with connection.transaction(): - if variables is not None: + async with Postgres.acquire_connection() as connection: + async with connection.transaction(): + ctx.log_info("...ensuring memory entry exists...") + await connection.execute( + """ + INSERT INTO + memories ( + identifier + ) + + VALUES ( + $1::TEXT + ) + + ON CONFLICT (identifier) + DO NOTHING; + """, + identifier, + ) + + if variables is not None: ctx.log_info(f"...remembering {len(variables)} variables...") - await PostgresConnection.execute( + await connection.execute( @@ - if variables is not None: - ctx.log_info(f"...remembering {len(variables)} variables...") - await PostgresConnection.execute( + if variables is not None: + ctx.log_info(f"...remembering {len(variables)} variables...") + await connection.execute(Also applies to: 165-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/guides/EvaluatorCatalog.md(8 hunks)docs/guides/Postgres.md(1 hunks)mkdocs.yml(1 hunks)pyproject.toml(1 hunks)src/draive/postgres/configuration.py(5 hunks)src/draive/postgres/instructions.py(5 hunks)src/draive/postgres/memory.py(8 hunks)tests/test_specification.py(2 hunks)tools/generate_instruction.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
mkdocs.yml
📄 CodeRabbit inference engine (AGENTS.md)
Register documentation navigation in
mkdocs.ymlunder thenav:section when adding pages
Files:
mkdocs.yml
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports from the draive package (e.g., from draive import ...), not relative imports
Follow Ruff import ordering: standard library, third-party, then local imports
Use Python 3.12+ typing features (e.g., | unions, PEP 695 generics)
Prefer abstract/base collection types (Sequence, Mapping, Set, Iterable) over concrete list, dict, set in type hints
Define and raise custom exception types for specific error cases
Use NumPy docstring convention for all functions, classes, and methods
Skip module-level docstrings unless explicitly requested
Docstrings should include Parameters, Returns, Raises, and Notes (if needed) sectionsFormat all Python code with Ruff formatter (run via
make format); do not use any other formatter
Files:
tests/test_specification.pysrc/draive/postgres/instructions.pytools/generate_instruction.pysrc/draive/postgres/configuration.pysrc/draive/postgres/memory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Place tests in the tests/ directory and use pytest (with async support)
Mark asynchronous tests with pytest.mark.asyncio
Mock dependencies within test scope using stubbed functionality state (e.g., via ctx.scope)
tests/**/*.py: Unit tests must not perform network I/O; mock providers/HTTP
Usepytest-asyncio(@pytest.mark.asyncio) for coroutine tests
Prefer scoping withctx.scope(...)in tests and bind requiredStateinstances explicitly
Avoid real I/O in tests; stub provider calls and HTTP
Files:
tests/test_specification.py
src/draive/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/draive/**/*.py: Import Haiway symbols directly asfrom haiway import State, ctx
Usectx.scope(...)to bind activeStateinstances; avoid global state
Route all logging throughctx.log_*and never use
Enforce strict typing on public APIs; avoid untyped definitions andAnyexcept at verified third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamicgetattrexcept at narrow boundaries
Use abstract immutable protocols (Mapping,Sequence,Iterable) instead of concretedict/list/setin public types
Usefinalwhere applicable; prefer composition over complex inheritance
Use precise unions (|), narrow withmatch/isinstance, and avoidcastunless provably safe and localized
Model immutable configuration/data ashaiway.State; avoid in‑place mutation and preferState.updated(...)/functional builders
Access active state viahaiway.ctxwithin async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance must use@statemethod
Add structured, concise logs around generation calls, tool dispatch, and provider requests/responses (without secrets); add metrics viactx.recordwhere applicable
All I/O is async; keep boundaries async, usectx.spawnfor detached tasks, and avoid custom threading
Translate provider/SDK errors into typed exceptions; never raise bareException; preserve context
Compose multimodal content withMultimodalContent.of(...); useResourceContent/Referencefor blobs; wrap custom data inArtifactContent(use hidden when needed)
Public symbols require NumPy-style docstrings with Parameters/Returns/Raises and rationale when non-obvious
Avoid docstrings for internal helpers; prefer self-explanatory names; skip module-level docstrings
Never log secrets or full request bodies containing keys/tokens
Read credentials from environment variables; resolve via helpers likegetenv_str
Files:
src/draive/postgres/instructions.pysrc/draive/postgres/configuration.pysrc/draive/postgres/memory.py
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Author documentation pages belong under
docs/
Files:
docs/guides/EvaluatorCatalog.mddocs/guides/Postgres.md
🧬 Code graph analysis (3)
src/draive/postgres/instructions.py (1)
src/draive/parameters/model.py (1)
from_json(705-720)
tools/generate_instruction.py (7)
src/draive/anthropic/client.py (1)
Anthropic(15-90)src/draive/anthropic/config.py (1)
AnthropicConfig(9-21)src/draive/gemini/client.py (1)
Gemini(18-66)src/draive/gemini/config.py (1)
GeminiConfig(25-36)src/draive/helpers/instruction_preparation.py (2)
InstructionPreparationAmbiguity(15-22)prepare_instructions(25-64)src/draive/openai/client.py (1)
OpenAI(22-99)src/draive/openai/config.py (1)
OpenAIResponsesConfig(21-32)
src/draive/postgres/memory.py (1)
src/draive/stages/stage.py (1)
execute(1978-2042)
🔇 Additional comments (11)
pyproject.toml (1)
8-8: LGTM! Patch version bump for postgres migrations fix.The version bump from 0.87.0 to 0.87.1 appropriately reflects the patch nature of the postgres migration fixes included in this PR.
tests/test_specification.py (1)
259-259: LGTM! Cosmetic quote style normalization.The change from single-quoted to double-quoted forward references is purely stylistic and has no functional impact.
Also applies to: 292-293
src/draive/postgres/configuration.py (2)
15-16: LGTM! Composite primary key enables versioned configuration storage.The updated schema with
PRIMARY KEY (identifier, created)allows storing multiple versions of each configuration, which aligns well with theDISTINCT ON (identifier)queries that retrieve the latest version.
51-51: LGTM! Explicit type casts improve query safety.The explicit
::TEXTand::JSONBcasts in SELECT and INSERT statements prevent implicit type conversion issues and make the query contract clearer. This is a PostgreSQL best practice.Also applies to: 77-78, 120-121
docs/guides/EvaluatorCatalog.md (1)
398-398: LGTM! Type hint updated to follow best practices.The change from
list[str]toSequence[str]aligns with the coding guidelines requiring abstract collection types in public APIs. This promotes better API design by accepting any sequence-like input.As per coding guidelines.
Also applies to: 416-416
tools/generate_instruction.py (6)
77-89: LGTM! Well-designed provider abstraction.The
ProviderSpecfrozen dataclass effectively encapsulates provider-specific configuration and disposable creation. The use offrozen=Trueensures immutability, and the factory methods provide a clean interface for creating provider contexts.
92-114: LGTM! Provider registry enables extensible configuration.The
PROVIDERSregistry cleanly maps provider keys to their specifications. Default models are consistent with the documentation comments (lines 46-48), and the factory lambdas correctly instantiate provider-specific config classes.
117-125: LGTM! Robust provider validation with helpful error messages.The
_parse_providerfunction normalizes input, validates against the registry, and provides clear error messages listing available providers when an invalid choice is given.
128-140: LGTM! Provider-agnostic instruction generation.The refactored
generate_with_providerfunction eliminates provider-specific branching by using theProviderSpecabstraction. The guidelines parameter is correctly propagated toprepare_instructions, and the context scope correctly initializes both config and disposable.
174-177: LGTM! Guidelines support added to CLI.The
--guidelinesargument enables users to inject custom guidelines during instruction preparation, which is correctly propagated to theprepare_instructionscall.
221-224: LGTM! Proper handling of ambiguous instruction preparation.The new exception handling for
InstructionPreparationAmbiguitycorrectly writes follow-up questions to stderr and exits with code 2, distinguishing this case from general errors (exit code 1).
KaQuMiQ
force-pushed
the
feature/migrations
branch
from
b11fcf0 to
26b16b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/draive/postgres/configuration.py (1)
69-103: Verify loading query handles composite key correctly.The
loadingquery fetches the latest configuration for a given identifier usingDISTINCT ON (identifier)andORDER BY identifier, created DESC. This should work correctly, but consider whetherLIMIT 1is still necessary givenDISTINCT ONalready returns one row per identifier.Consider simplifying the query by removing the redundant
LIMIT 1:loaded: PostgresRow | None = await Postgres.fetch_one( """ SELECT DISTINCT ON (identifier) identifier::TEXT, content::TEXT FROM configurations WHERE identifier = $1 ORDER BY identifier, created DESC - - LIMIT 1; + ; """, identifier, )src/draive/postgres/memory.py (1)
99-114: Critical: Inconsistent connection usage in remember function.Line 99 calls
PostgresConnection.execute()instead of using the acquiredconnection.execute(). This bypasses the explicit connection and transaction context, potentially causing the operation to run outside the transaction or on a different connection.Apply this diff to use the acquired connection:
if variables is not None: ctx.log_info(f"...remembering {len(variables)} variables...") - await PostgresConnection.execute( + await connection.execute( """ INSERT INTO memories_variables (src/draive/postgres/instructions.py (2)
97-113: Remove redundant LIMIT 1 clause.
DISTINCT ON (name)withORDER BY name, created DESCalready returns exactly one row per name (the most recent). TheLIMIT 1on line 113 is redundant.ORDER BY name, created DESC - - LIMIT 1; + ;
97-113: Add explicit ::TEXT cast for consistency.The
contentcolumn in the SELECT should be cast to::TEXTto match the explicit casting pattern used in thelistingfunction (lines 60-63).SELECT DISTINCT ON (name) - content + content::TEXT FROM
♻️ Duplicate comments (3)
src/draive/helpers/instruction_preparation.py (1)
189-189: Template example placeholder escaping needs clarification.Line 189 changes
{{topic}}to{{{{topic}}}}. Same concern as above—verify this is the intended user-facing documentation syntax.src/draive/postgres/memory.py (1)
164-181: Critical: Inconsistent connection usage in maintenance function - same issue as flagged in past review.Line 166 calls
PostgresConnection.execute()instead ofconnection.execute(). This is the exact issue flagged in the past review comment. All database operations within the explicit connection/transaction context should use the acquiredconnectioninstance.Apply this diff to fix the inconsistency:
if variables is not None: ctx.log_info(f"...remembering {len(variables)} variables...") - await PostgresConnection.execute( + await connection.execute( """ INSERT INTO memories_variables (Based on past review comments.
docs/guides/Postgres.md (1)
21-24: Fix typo in inline comment.The inline comment contains "confgiurations" which should be "configurations".
- PostgresConfigurationRepository(), # use postgres confgiurations + PostgresConfigurationRepository(), # use postgres configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/guides/EvaluatorCatalog.md(8 hunks)docs/guides/Postgres.md(1 hunks)mkdocs.yml(1 hunks)pyproject.toml(1 hunks)src/draive/helpers/instruction_preparation.py(4 hunks)src/draive/postgres/configuration.py(5 hunks)src/draive/postgres/instructions.py(5 hunks)src/draive/postgres/memory.py(8 hunks)tests/test_specification.py(2 hunks)tools/generate_instruction.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports from the draive package (e.g., from draive import ...), not relative imports
Follow Ruff import ordering: standard library, third-party, then local imports
Use Python 3.12+ typing features (e.g., | unions, PEP 695 generics)
Prefer abstract/base collection types (Sequence, Mapping, Set, Iterable) over concrete list, dict, set in type hints
Define and raise custom exception types for specific error cases
Use NumPy docstring convention for all functions, classes, and methods
Skip module-level docstrings unless explicitly requested
Docstrings should include Parameters, Returns, Raises, and Notes (if needed) sectionsFormat all Python code with Ruff formatter (run via
make format); do not use any other formatter
Files:
src/draive/postgres/configuration.pytools/generate_instruction.pysrc/draive/postgres/memory.pysrc/draive/postgres/instructions.pysrc/draive/helpers/instruction_preparation.pytests/test_specification.py
src/draive/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/draive/**/*.py: Import Haiway symbols directly asfrom haiway import State, ctx
Usectx.scope(...)to bind activeStateinstances; avoid global state
Route all logging throughctx.log_*and never use
Enforce strict typing on public APIs; avoid untyped definitions andAnyexcept at verified third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamicgetattrexcept at narrow boundaries
Use abstract immutable protocols (Mapping,Sequence,Iterable) instead of concretedict/list/setin public types
Usefinalwhere applicable; prefer composition over complex inheritance
Use precise unions (|), narrow withmatch/isinstance, and avoidcastunless provably safe and localized
Model immutable configuration/data ashaiway.State; avoid in‑place mutation and preferState.updated(...)/functional builders
Access active state viahaiway.ctxwithin async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance must use@statemethod
Add structured, concise logs around generation calls, tool dispatch, and provider requests/responses (without secrets); add metrics viactx.recordwhere applicable
All I/O is async; keep boundaries async, usectx.spawnfor detached tasks, and avoid custom threading
Translate provider/SDK errors into typed exceptions; never raise bareException; preserve context
Compose multimodal content withMultimodalContent.of(...); useResourceContent/Referencefor blobs; wrap custom data inArtifactContent(use hidden when needed)
Public symbols require NumPy-style docstrings with Parameters/Returns/Raises and rationale when non-obvious
Avoid docstrings for internal helpers; prefer self-explanatory names; skip module-level docstrings
Never log secrets or full request bodies containing keys/tokens
Read credentials from environment variables; resolve via helpers likegetenv_str
Files:
src/draive/postgres/configuration.pysrc/draive/postgres/memory.pysrc/draive/postgres/instructions.pysrc/draive/helpers/instruction_preparation.py
mkdocs.yml
📄 CodeRabbit inference engine (AGENTS.md)
Register documentation navigation in
mkdocs.ymlunder thenav:section when adding pages
Files:
mkdocs.yml
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Author documentation pages belong under
docs/
Files:
docs/guides/EvaluatorCatalog.mddocs/guides/Postgres.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Place tests in the tests/ directory and use pytest (with async support)
Mark asynchronous tests with pytest.mark.asyncio
Mock dependencies within test scope using stubbed functionality state (e.g., via ctx.scope)
tests/**/*.py: Unit tests must not perform network I/O; mock providers/HTTP
Usepytest-asyncio(@pytest.mark.asyncio) for coroutine tests
Prefer scoping withctx.scope(...)in tests and bind requiredStateinstances explicitly
Avoid real I/O in tests; stub provider calls and HTTP
Files:
tests/test_specification.py
🧬 Code graph analysis (3)
tools/generate_instruction.py (7)
src/draive/anthropic/client.py (1)
Anthropic(15-90)src/draive/anthropic/config.py (1)
AnthropicConfig(9-21)src/draive/gemini/client.py (1)
Gemini(18-66)src/draive/gemini/config.py (1)
GeminiConfig(25-36)src/draive/helpers/instruction_preparation.py (2)
InstructionPreparationAmbiguity(15-22)prepare_instructions(25-65)src/draive/openai/client.py (1)
OpenAI(22-99)src/draive/openai/config.py (1)
OpenAIResponsesConfig(21-32)
src/draive/postgres/memory.py (1)
src/draive/stages/stage.py (1)
execute(1978-2042)
src/draive/postgres/instructions.py (1)
src/draive/parameters/model.py (1)
from_json(705-720)
🔇 Additional comments (26)
tools/generate_instruction.py (4)
77-114: LGTM! Clean provider abstraction pattern.The
ProviderSpecdataclass andPROVIDERSregistry provide a maintainable, extensible way to support multiple AI providers. The separation of config/disposable factories follows the draive pattern shown in the relevant snippets.
128-140: Verify that prepare_instructions accepts guidelines parameter.The
generate_with_providerfunction now passesguidelines=guidelinestoprepare_instructions. Ensure this parameter is supported by the function signature.Based on the relevant snippets from
src/draive/helpers/instruction_preparation.py(lines 24-64), theprepare_instructionsfunction does accept aguidelinesparameter. The implementation is correct.
221-225: Proper exception handling for InstructionPreparationAmbiguity.The exception handling correctly catches
InstructionPreparationAmbiguity, extracts the questions, and exits with code 2 to distinguish from other errors. This aligns with the exception class definition shown in the relevant snippets.
117-126: _parse_provider keys align with registered draive clients. The provider keys 'anthropic', 'openai', and 'gemini' correctly match the imported client classes and PROVIDERS definitions.mkdocs.yml (1)
101-101: LGTM! Documentation navigation properly registered.The Postgres guide is correctly added to the navigation section, following the existing pattern and placement within the Guides section. As per coding guidelines, documentation pages must be registered in
mkdocs.yml.pyproject.toml (1)
8-8: LGTM! Appropriate patch version bump.The version increment from 0.87.0 to 0.87.1 is appropriate for the postgres migration fixes described in this PR. Patch versions typically cover bug fixes and backward-compatible changes.
tests/test_specification.py (1)
259-259: LGTM! Consistent quote style in forward references.The change from single to double quotes in forward-referenced type hints (
"NodeDict | None") is purely stylistic and has no functional impact. This likely aligns with Ruff formatter conventions for consistency.Also applies to: 292-293
src/draive/postgres/configuration.py (3)
120-121: LGTM! Explicit type casts improve query safety.The parameter bindings now use explicit PostgreSQL type casts (
$1::TEXT,$2::JSONB), which ensures type safety and prevents potential type coercion issues. This pattern is consistent with the changes mentioned in the AI summary for other postgres modules.
44-63: Ignore incorrect DISTINCT ON concern.The
DISTINCT ON (identifier)+ORDER BY identifier, created DESCis intentional and consistent with other modules (e.g. instructions, memory) for selecting the latest configuration per identifier; a composite PK on (identifier, created) doesn’t eliminate the need to group by identifier to fetch only the most recent entry.Likely an incorrect or invalid review comment.
132-150: Verify delete scope against composite PK
Theremovingfunction’sDELETE … WHERE identifier = $1will remove every version (composite PK(identifier, created)). Confirm if this cleanup of all history is intended or should target only the latest record.docs/guides/EvaluatorCatalog.md (1)
397-460: LGTM! Excellent alignment with best practices.The change from
list[str]toSequence[str]properly follows the coding guidelines requiring abstract collection types in type hints. The import is correctly added and usage is consistent throughout the examples.src/draive/helpers/instruction_preparation.py (3)
85-94: Helper functions are correct and well-structured.The
_format_guidelinesand_escape_curly_braceshelpers properly encapsulate the formatting logic. The escape function correctly doubles curly braces to prevent template interference.
157-159: Verify variable placeholder escaping in documentation is intentional.The template now shows
{{{{time}}}}and{{{{variable}}}}(four braces) instead of the typical two-brace placeholder syntax. This appears to be documenting the escaped form after_escape_curly_bracesis applied, but may confuse users who expect standard{variable}syntax.Confirm that the quadruple-brace syntax is the intended documentation. If these are meant to be literal examples of the final instruction format (post-escaping), consider adding a clarifying note. If these should be user-facing placeholders, they should remain as
{{variable}}.
49-52: I’ve gathered the relevant code sections around the replace call and helper functions; please review the output for confirmation of whether the literal"{guidelines}"can appear in user’s content and warrant a more robust placeholder.src/draive/postgres/memory.py (7)
14-33: Schema documentation is clear and well-structured.The new three-table schema with proper foreign key constraints and primary keys is well-documented. The separation of concerns (memories → memories_variables, memories_elements) is a good design.
95-96: Connection handling is correct.The switch from
PostgresConnection.transaction()toPostgres.acquire_connection()with explicitconnection.transaction()provides better control over the connection lifecycle.
118-133: Connection usage is correct for elements insertion.The loop correctly uses
connection.execute()for inserting memory elements within the transaction context.
143-162: Connection handling refactored correctly.The maintenance function now properly acquires a connection and uses explicit transaction context. The new approach of ensuring the memory entry exists via
INSERT ... ON CONFLICT DO NOTHINGis cleaner than checking existence separately.
192-246: Load context function correctly uses explicit TEXT casting.The addition of
::TEXTcasting on the content column is good for type safety. The function correctly handles both limited and unlimited queries and properly reverses the results to maintain chronological order.
249-263: Context decoding logic is correct.The
_decode_contextgenerator properly handles the JSON decoding and type discrimination forModelInputandModelOutputelements.
266-294: Variables loading updated correctly for new schema.The function now queries
memories_variablesinstead of the oldmemoriestable, with proper::TEXTcasting. The DISTINCT ON pattern with ORDER BY ensures the most recent variables are loaded. The fallback to emptyMap()when no row exists is appropriate.src/draive/postgres/instructions.py (3)
49-52: Verify cache limit=1 is intentional for listing function.The listing cache uses
limit=1, meaning only one result set is cached regardless of parameters. Sincelisting()accepts**extrabut doesn't use it to vary the query, this may be intentional. However, if future changes introduce filtering parameters, you'll need to adjust the cache configuration.Can you confirm that
limit=1is the desired behavior, and that no per-parameter caching is needed?
151-162: Explicit casts and null handling look good.The explicit
::TEXTand::JSONBcasts align with the schema migration requirements. Thedeclaration.description or Nonepattern correctly handles empty descriptions by storing NULL.
169-189: LGTM!The removing function correctly deletes all instruction versions for the given name and properly clears both caches.
docs/guides/Postgres.md (2)
79-108: LGTM!The ModelMemory documentation is comprehensive and accurately describes the three-table schema with proper foreign key constraints. The CASCADE delete semantics are appropriate for cleaning up related records.
71-75: No changes needed: documentation method names accurately reflect theInstructionsRepositoryAPI.
| ``` | ||
|
|
||
| Key capabilities: | ||
| - `configurations()` returns every known identifier using cached results (limit 1, default 10 minute TTL). |
There was a problem hiding this comment.
🧩 Analysis chain
Verify method name "configurations()".
The documentation refers to configurations() as returning "every known identifier". Confirm this matches the actual method name in PostgresConfigurationRepository. Based on typical repository patterns, it might be named something like list(), identifiers(), or available_configurations().
🏁 Script executed:
#!/bin/bash
# Search for the actual method name in PostgresConfigurationRepository
rg -n "def (configurations|list|identifiers|available)" src/draive/postgres/configuration.pyLength of output: 25
Use the correct method name listing() in documentation
Replace configurations() with listing() in docs/guides/Postgres.md to match the actual method in PostgresConfigurationRepository.
🤖 Prompt for AI Agents
In docs/guides/Postgres.md around line 46, the docs incorrectly reference
configurations() — update that method name to listing() to match
PostgresConfigurationRepository; ensure the sentence reads that listing()
returns every known identifier using cached results (limit 1, default 10 minute
TTL) and run a quick search in the file for any other occurrences of
configurations() to replace with listing().
| # created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| # PRIMARY KEY (identifier, created) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify migration path for composite primary key change.
The schema now uses a composite primary key (identifier, created) instead of a single-column primary key. This is a breaking change that requires data migration. Ensure:
- A migration script exists to alter existing tables
- Existing data has the
createdcolumn populated (theDEFAULT CURRENT_TIMESTAMPonly applies to new rows) - Applications expecting a single-column PK are updated
Run the following script to check for migration scripts and schema documentation:
🏁 Script executed:
#!/bin/bash
# Description: Search for postgres migration scripts and schema setup documentation
# Look for migration scripts
fd -e sql -e py 'migrat' --exec echo "Found migration file: {}"
# Search for schema setup or migration documentation
rg -n "CREATE TABLE configurations|ALTER TABLE configurations" --type=sql --type=py
# Check if there are other references to the old schema
rg -n "PRIMARY KEY.*identifier[^,]" --type=sql --type=pyLength of output: 135
Provide a migration for the new composite primary key.
No migration script exists to add the created TIMESTAMPTZ column, backfill its values, or replace the old single-column PK with (identifier, created). Create and commit a database migration (SQL or migration‐tool script) to:
- ALTER TABLE configurations ADD COLUMN created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP
- UPDATE existing rows to set
createdwhere NULL - DROP the old primary key on
identifierand ADD PRIMARY KEY (identifier, created)
🤖 Prompt for AI Agents
In src/draive/postgres/configuration.py around lines 15 to 16, there is no
migration to add the new created TIMESTAMPTZ column and replace the
single-column primary key; create and commit a migration that: ALTER TABLE
configurations ADD COLUMN created TIMESTAMPTZ NOT NULL DEFAULT
CURRENT_TIMESTAMP; UPDATE existing rows to set created for preexisting records
(e.g. set to CURRENT_TIMESTAMP or an appropriate historical value) to avoid
NULLs; DROP the existing PRIMARY KEY on identifier and ADD a composite PRIMARY
KEY (identifier, created); ensure the migration is idempotent, runs inside a
transaction, and includes a rollback or safe ordering (add column and backfill
before dropping the old PK) and add tests or a migration checksum if your
migration tool requires it.
| # created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| # PRIMARY KEY (name, created) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove trailing comma for SQL style consistency.
The trailing comma after TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP is valid but inconsistent with line 23, which has no comma. SQL style guides typically omit trailing commas in column definitions.
-# created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP,
+# created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP
# PRIMARY KEY (name, created)📝 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.
| # created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| # PRIMARY KEY (name, created) | |
| # created TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| # PRIMARY KEY (name, created) |
🤖 Prompt for AI Agents
In src/draive/postgres/instructions.py around lines 24 to 25, remove the
trailing comma after the column definition "created TIMESTAMPTZ NOT NULL DEFAULT
CURRENT_TIMESTAMP," so it matches the style used on the previous line; update
the SQL block to omit that trailing comma before the PRIMARY KEY line to
maintain consistency in column definition formatting.
| description, | ||
| declaration.description or None, | ||
| content, | ||
| f"[{','.join(argument.to_json() for argument in declaration.arguments)}]", |
There was a problem hiding this comment.
Use proper JSON serialization instead of manual string construction.
Manually constructing a JSON array string with f"[{','.join(...)}]" is fragile and potentially unsafe. If argument.to_json() doesn't properly escape special characters, this could lead to malformed JSON or injection issues.
Use json.dumps() to serialize the arguments array safely:
+import json
+
...
declaration.name,
declaration.description or None,
content,
- f"[{','.join(argument.to_json() for argument in declaration.arguments)}]",
+ json.dumps([json.loads(argument.to_json()) for argument in declaration.arguments]),
declaration.meta.to_json(),Or if declaration.arguments can be serialized directly:
+import json
+
...
- f"[{','.join(argument.to_json() for argument in declaration.arguments)}]",
+ json.dumps([arg for arg in declaration.arguments], default=lambda o: o.__dict__ if hasattr(o, '__dict__') else str(o)),📝 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.
| f"[{','.join(argument.to_json() for argument in declaration.arguments)}]", | |
| # At the top of src/draive/postgres/instructions.py, ensure you have: | |
| import json | |
| # …later, where the arguments array is constructed: | |
| declaration.name, | |
| declaration.description or None, | |
| content, | |
| json.dumps([json.loads(argument.to_json()) for argument in declaration.arguments]), | |
| declaration.meta.to_json(), |
🤖 Prompt for AI Agents
In src/draive/postgres/instructions.py around line 161, the code builds a JSON
array string via f"[{','.join(argument.to_json() for argument in
declaration.arguments)}]" which is fragile and unsafe; replace this manual
construction with proper JSON serialization by creating a Python list of the
argument JSON values (or serializable objects) and calling json.dumps(...) on
that list (import json at top if missing) so special characters are escaped and
the output is valid JSON.
26b16b5 to
4bc9747
Compare
KaQuMiQ
force-pushed
the
feature/migrations
branch
from
4bc9747 to
24ab898
Compare
1 participant
No description provided.