docs(cloud): v2 Postgres-as-monolith schema upgrade#105
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Caution Review failedPull request was closed or merged during review 📝 Walkthrough
WalkthroughExtended documentation to describe a PostgreSQL schema upgrade from v1 to v2. The v2 migration enables the pg_trgm extension, adds generated full-text search columns with GIN indexes, vector embeddings with HNSW indexes, and introduces tenant-isolated sync_queue and tenant_cache tables with comprehensive RLS policies and a queue-claiming function. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gradata/enhancements/meta_rules_storage.py (1)
275-321: 🧹 Nitpick | 🔵 Trivial
super_meta_rulesis missingtenant_idcolumn support.The
meta_rulestable now includestenant_idandvisibility, butsuper_meta_rules(lines 227-240, 275-321) does not have these columns added. If super-meta-rules should also be tenant-scoped (they derive from meta-rules), consider adding the same columns for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gradata/enhancements/meta_rules_storage.py` around lines 275 - 321, The super_meta_rules table and its persistence logic need tenant scoping and visibility columns; update ensure_super_table to create/alter super_meta_rules with tenant_id and visibility columns, and update save_super_meta_rules to INSERT the tenant_id and visibility values (use s.tenant_id and s.visibility or s.visibility.value if enum) into the VALUES list and parameter tuple, keeping the column order consistent with the INSERT column list; ensure any JSON/serialization rules for tenant_id/visibility match meta_rules usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/cloud-apply-instructions.md`:
- Around line 17-25: Update the verification steps to reflect the new v2 schema:
change the expected migration row from `001_cloud_schema_v1` to
`002_cloud_monolith_v2`, add verification that the `pg_trgm` extension is
installed, confirm tsvector columns are populated (check the table columns
created by the migration that include `tsvector`), and replace the old policy
count expectation with the new `pg_policies` output check; ensure the guide text
(the section that references expected CREATE TABLE/ENABLE ROW LEVEL SECURITY
counts and the `cloud_migrations` row) is updated accordingly so a v1-only apply
no longer appears valid.
In `@docs/architecture/cloud-monolith-v2.md`:
- Around line 149-156: The document currently creates an UNLOGGED table
tenant_cache but doesn’t call out the durability trade-off; add a clear note
stating that tenant_cache is UNLOGGED and that all its rows are lost on Postgres
crash or unclean shutdown, and surface this prominently either in the "What v2
adds" summary table or as a highlighted note near the existing tenant_cache
description (around line 18); reference the table name tenant_cache and the
UNLOGGED keyword so operators can immediately see the risk and decide if a
persistent cache is needed.
- Around line 65-74: The generated tsvector currently applies to JSON columns
(scope, examples) which indexes JSON syntax; update the GENERATED ALWAYS AS
expression for meta_rules.search_tsv to extract and concatenate actual text
values from those JSON columns before calling to_tsvector (e.g., use
jsonb_each_text or another jsonb->> extraction and string_agg to combine values)
so principle, scope and examples feed plain text to to_tsvector; check
meta_rules_storage.py to confirm the JSON structure and choose the correct key
extraction for scope/examples, then update the ALTER TABLE CREATE INDEX
migration to use those extracted text expressions instead of raw JSON.
In `@src/gradata/_cloud_sync.py`:
- Around line 158-163: The HTTP error log currently truncates the response body
to 200 bytes in the except urllib.error.HTTPError as e handler, which can drop
useful DB/RLS constraint messages; update the handler in
src/gradata/_cloud_sync.py to either increase the truncation to a larger window
(e.g., 500–1000 bytes) or log the full e.read() at DEBUG while keeping the
WARNING message shorter; specifically adjust the _log.warning call that uses
table, e.code and e.read()[:200] and add a separate _log.debug entry that logs
the full response body (ensure you call e.read() only once or buffer it into a
variable before logging).
- Around line 200-201: The code opens a SQLite connection with
sqlite3.connect(db_path) and closes it manually in a finally block; replace this
with a context manager to ensure deterministic closing and safer exception
handling by using "with sqlite3.connect(db_path) as conn:" around the code that
uses conn (where tenant_for(brain) and subsequent DB operations occur), removing
the manual conn.close()/finally cleanup and keeping the same variable name
(conn) so existing logic using conn continues to work.
- Around line 136-163: Validate the constructed URL's scheme before making the
request: parse the value coming from ENV_URL (used to build the local variable
url) with urllib.parse.urlparse and ensure scheme == "https" (reject empty or
other schemes like file://); if validation fails, log a warning on _log and
return 0 early instead of calling urllib.request.urlopen in the cloud_sync path
so only HTTPS endpoints are allowed.
In `@src/gradata/_core.py`:
- Around line 774-786: The INSERT into lesson_transitions currently writes None
for the session column causing loss of session boundaries; update the insertion
to use the real session value (e.g., brain.session or a local current_session
hoisted above) instead of None. Locate the block that executes conn.execute(...)
which inserts into lesson_transitions and replace the None argument with
brain.session (or a captured current_session variable) so the VALUES tuple
stores the actual session identifier; ensure current_session is
defined/imported/accessible in the scope surrounding the transitions loop where
_tenant_for and now are used.
In `@src/gradata/_events.py`:
- Around line 75-77: Read/update methods query(), _detect_session(),
correction_rate(), compute_leading_indicators(), audit_trend(), and supersede()
currently scan the whole events table; update each to be tenant-aware by adding
tenant_id filtering to every SELECT/UPDATE/DELETE (e.g., include "WHERE
tenant_id = :tenant_id" or add tenant_id to existing WHERE clauses and when
selecting by id use "WHERE id = :id AND tenant_id = :tenant_id"). Ensure the
tenant_id value is taken from the instance or passed context (e.g.,
self.tenant_id or method parameter), use parameterized queries, and apply the
same tenant filter to any full-table scans or joins within those functions so no
rows from other tenants are returned or modified.
In `@src/gradata/_migrations/__init__.py`:
- Around line 117-144: Commit inline migrations before running numbered ones and
make imports/execution of each numbered migration defensive: in _apply_numbered,
call conn.commit() after ensure_migrations_table() (so inline changes are
persisted) then for each module name from _numbered_modules() perform the import
(importlib.import_module) inside a try/except and on import failure log the
error and continue (do not crash); likewise call the migration's up(conn,
tenant_id) inside a try/except, and if up() raises log the exception and skip
calling mark_applied() for that migration so partial failures don't get recorded
as applied; keep using get_or_create_tenant_id(), has_applied(), mark_applied(),
and the module NAME/up attributes to locate and process the migrations.
In `@src/gradata/_migrations/_runner.py`:
- Around line 64-66: Add a short docstring to each SQL-building helper
(column_exists, add_column_if_missing, create_index_if_missing) stating these
are internal migration utilities and that table/column/index names must be
trusted (not user-provided) because the functions construct SQL with f-strings;
update the docstring for column_exists and the other two helper functions to
explicitly mention "internal-only" usage and the trust requirement so reviewers
understand the safety assumption.
In `@src/gradata/_migrations/001_add_tenant_id.py`:
- Around line 160-246: The up() function is too branchy; extract the per-tenant
and mixed-visibility table loops into two helpers (e.g.,
handle_per_tenant_table(conn, t, tenant_id, summary) and
handle_mixed_visibility_table(conn, t, tenant_id, summary)) that encapsulate:
checking table_exists(conn, t), calling add_column_if_missing for "tenant_id"
(and "visibility" for mixed), performing the UPDATE backfills for tenant_id and
visibility, updating summary["rows_backfilled"], summary["tables_backfilled"],
and summary["visibility_backfilled"], and creating indexes via
create_index_if_missing; then replace the two for-loops in up() with calls to
these helpers, preserving existing behavior and idempotence (use
add_column_if_missing, create_index_if_missing, table_exists, and the same SQL
statements) so the returned summary remains identical.
- Around line 300-317: The migration calls up(conn, tenant_id=tid) and then
mark_applied(...), but never commits the transaction; add an explicit
conn.commit() after mark_applied(...) (and before printing "OK" / return) so the
schema/backfill changes and the applied marker are persisted atomically, while
keeping conn.close() in the finally block; use the existing up, mark_applied,
NAME, and conn symbols to locate where to insert the commit.
In `@src/gradata/_migrations/tenant_uuid.py`:
- Around line 41-65: The current temp-file then os.replace sequence races
because two processes can both see fpath missing and each replace it; instead
attempt to create the final file atomically: open fpath with
os.O_WRONLY|os.O_CREAT|os.O_EXCL (using os.open on fpath rather than tmp) and
write new_tid via os.fdopen; if os.open raises FileExistsError, read and return
the existing fpath contents (tid) and only fall back to new_tid if the read is
not a valid UUID; keep references to new_tid, fpath, tmp (remove or stop using
tmp), and the _is_valid_uuid check.
In `@src/gradata/_query.py`:
- Around line 47-51: The current use of
contextlib.suppress(sqlite3.OperationalError) around conn.execute("ALTER TABLE
brain_fts_content ADD COLUMN tenant_id TEXT") hides all OperationalError cases
(locks, I/O, schema issues); change it to an explicit try/except that catches
sqlite3.OperationalError as e and only suppresses it when the error text
indicates the column already exists (e.g. check "duplicate column name" in
str(e).lower()), otherwise re-raise the exception so real migration failures
surface.
In `@src/gradata/_tenant.py`:
- Around line 50-68: The code currently overwrites a corrupt tenant id file
(fpath) with a new UUID, silently rotating the brain into a new tenant; change
this so any invalid/truncated tenant file is treated as an error requiring
manual repair: when reading fpath (the initial tid path check) or inside the
FileExistsError handler, if _is_valid_uuid(existing) is False, raise a clear
exception (e.g., ValueError or a custom CorruptTenantError) instead of calling
fpath.write_text(tid,...); keep the successful path (return existing valid tid)
and the fresh-create path (open(..., "x") write tid and return) unchanged, but
remove the auto-overwrite step so no silent rekeying occurs.
In `@src/gradata/cli.py`:
- Around line 35-43: The fallback in _get_brain() is inconsistent with
_resolve_brain_root(): change the brain_dir resolution in _get_brain() (the line
assigning brain_dir) so it uses the same fallback as _resolve_brain_root() —
either call _resolve_brain_root() directly or replace Path.cwd() with
Path("brain") — ensuring the env_str("GRADATA_BRAIN") > getattr(args,
"brain_dir", None) > Path("brain") precedence and keeping the Brain
import/instantiation logic unchanged.
In `@src/gradata/enhancements/scoring/loop_intelligence.py`:
- Around line 154-155: The analytics functions detect_manual() and
get_activity_stats() currently query activity_log and prep_outcomes without
tenant scoping, so results aggregate across tenants; modify those query paths
(and the other occurrences around the commented ranges) to use the
already-derived _tid (from tenant_for(Path(db_path).parent)) and the same DB
handle from _get_db(db_path) and add a WHERE filter for the tenant identifier
column (e.g., tenant_id = _tid or the module’s canonical tenant column) to every
SELECT/COUNT/AVG against activity_log and prep_outcomes (and any joined
subqueries) so each operation is limited to rows for that tenant only; keep
using the existing variables _tid, conn, detect_manual(), and
get_activity_stats() to locate and update the SQL/ORM calls.
In `@tests/test_cloud_row_push.py`:
- Around line 84-90: The current truthy check "assert row and row[0]" is
ambiguous; split it into two explicit assertions: first assert that the query
returned a row (e.g., assert row is not None or assert row, "sync_state row
missing for brain_id ...") and then assert that last_push_at is not empty (e.g.,
assert row[0] is not None or != "" with a clear message). Optionally validate
the timestamp format by attempting to parse row[0] with datetime.fromisoformat
or matching a regex to ensure it is a valid ISO8601 timestamp; update the test
in tests/test_cloud_row_push.py around the conn.execute result handling
(variables: conn, row, last_push_at) accordingly.
In `@tests/test_tenant_id_inserts.py`:
- Around line 277-314: Add a regression test that calls fts_rebuild() and
asserts it persists tenant_id: create a BrainContext via
BrainContext.from_brain_dir(brain_dir), call fts_rebuild(ctx=ctx) (or with the
appropriate args used by the implementation), then query the brain_fts_content
table for an example source added by the rebuild and assert the returned
tenant_id equals TEST_TENANT; reference the fts_rebuild function and the
brain_fts_content table/column names used in existing tests to match style and
DB access.
---
Outside diff comments:
In `@src/gradata/enhancements/meta_rules_storage.py`:
- Around line 275-321: The super_meta_rules table and its persistence logic need
tenant scoping and visibility columns; update ensure_super_table to create/alter
super_meta_rules with tenant_id and visibility columns, and update
save_super_meta_rules to INSERT the tenant_id and visibility values (use
s.tenant_id and s.visibility or s.visibility.value if enum) into the VALUES list
and parameter tuple, keeping the column order consistent with the INSERT column
list; ensure any JSON/serialization rules for tenant_id/visibility match
meta_rules usage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc9cfb94-4d87-4aaf-a4ff-177be8be97ce
📒 Files selected for processing (23)
docs/architecture/cloud-apply-instructions.mddocs/architecture/cloud-monolith-v2.mddocs/architecture/multi-tenant-future-proofing.mdsrc/gradata/_cloud_sync.pysrc/gradata/_core.pysrc/gradata/_events.pysrc/gradata/_migrations/001_add_tenant_id.pysrc/gradata/_migrations/__init__.pysrc/gradata/_migrations/_runner.pysrc/gradata/_migrations/fill_null_tenant.pysrc/gradata/_migrations/tenant_uuid.pysrc/gradata/_query.pysrc/gradata/_tenant.pysrc/gradata/audit.pysrc/gradata/cli.pysrc/gradata/enhancements/meta_rules_storage.pysrc/gradata/enhancements/scoring/loop_intelligence.pysrc/gradata/enhancements/self_improvement/__init__.pysrc/gradata/hooks/rule_enforcement.pysrc/gradata/rules/rule_engine/__init__.pysrc/gradata/rules/rule_graph.pytests/test_cloud_row_push.pytests/test_tenant_id_inserts.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
src/gradata/**/*.py
⚙️ CodeRabbit configuration file
src/gradata/**/*.py: This is the core SDK. Check for: type safety (from future import annotations required), no print()
statements (use logging), all functions accepting BrainContext where DB access occurs, no hardcoded paths. Severity
scoring must clamp to [0,1]. Confidence values must be in [0.0, 1.0].
Files:
src/gradata/enhancements/self_improvement/__init__.pysrc/gradata/rules/rule_engine/__init__.pysrc/gradata/cli.pysrc/gradata/rules/rule_graph.pysrc/gradata/audit.pysrc/gradata/_query.pysrc/gradata/_core.pysrc/gradata/_migrations/fill_null_tenant.pysrc/gradata/_events.pysrc/gradata/hooks/rule_enforcement.pysrc/gradata/_tenant.pysrc/gradata/enhancements/scoring/loop_intelligence.pysrc/gradata/_migrations/tenant_uuid.pysrc/gradata/_cloud_sync.pysrc/gradata/_migrations/__init__.pysrc/gradata/enhancements/meta_rules_storage.pysrc/gradata/_migrations/001_add_tenant_id.pysrc/gradata/_migrations/_runner.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: Test files. Verify: no hardcoded paths, assertions check specific values not just truthiness,
parametrized tests preferred for boundary conditions, floating point comparisons use pytest.approx.
Files:
tests/test_tenant_id_inserts.pytests/test_cloud_row_push.py
src/gradata/hooks/**
⚙️ CodeRabbit configuration file
src/gradata/hooks/**: JavaScript hooks for Claude Code integration. Check for: no shell injection (no execSync with user
input), temp files must use per-user subdirectory, HTTP calls must have timeouts, errors must be silent (never block
the tool chain).
Files:
src/gradata/hooks/rule_enforcement.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.417Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
📚 Learning: 2026-04-17T17:18:07.417Z
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.417Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
Applied to files:
src/gradata/cli.pysrc/gradata/_query.pysrc/gradata/_core.pytests/test_tenant_id_inserts.pysrc/gradata/_migrations/fill_null_tenant.pysrc/gradata/_events.pydocs/architecture/multi-tenant-future-proofing.mdsrc/gradata/_tenant.pysrc/gradata/enhancements/scoring/loop_intelligence.pysrc/gradata/_migrations/tenant_uuid.pytests/test_cloud_row_push.pysrc/gradata/_cloud_sync.pysrc/gradata/_migrations/001_add_tenant_id.py
🪛 LanguageTool
docs/architecture/multi-tenant-future-proofing.md
[uncategorized] ~142-~142: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...yle flow. Defer until it matters. 4. Open source tension. The SDK is Apache-2.0 but `s...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~144-~144: Use a hyphen to join words.
Context: ...Apache license clean-room. 5. "Future proof now" is a trap. Most future-proo...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.22.0)
docs/architecture/multi-tenant-future-proofing.md
[warning] 21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 32-32: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 43-43: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 90-90: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
[warning] 91-91: Ordered list item prefix
Expected: 2; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 95-95: Ordered list item prefix
Expected: 3; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 104-104: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 110-110: Ordered list item prefix
Expected: 2; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 114-114: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 118-118: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/1/1
(MD029, ol-prefix)
docs/architecture/cloud-monolith-v2.md
[warning] 21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 32-32: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 43-43: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 90-90: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
[warning] 91-91: Ordered list item prefix
Expected: 2; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 95-95: Ordered list item prefix
Expected: 3; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 104-104: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 110-110: Ordered list item prefix
Expected: 2; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 114-114: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 118-118: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/1/1
(MD029, ol-prefix)
🪛 Ruff (0.15.10)
src/gradata/_core.py
[warning] 481-481: datetime.date.today() used
(DTZ011)
tests/test_tenant_id_inserts.py
[warning] 22-22: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
[warning] 31-31: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
[warning] 77-77: Unused function argument: brain_dir
(ARG001)
[warning] 104-104: Unused function argument: brain_dir
(ARG001)
[warning] 139-139: Unused function argument: brain_dir
(ARG001)
[warning] 166-166: Unused function argument: brain_dir
(ARG001)
[warning] 189-189: Unused function argument: brain_dir
(ARG001)
[warning] 214-214: Unused function argument: brain_dir
(ARG001)
[warning] 234-234: Unused function argument: brain_dir
(ARG001)
[warning] 257-257: Unused function argument: brain_dir
(ARG001)
src/gradata/_migrations/fill_null_tenant.py
[error] 112-112: Possible SQL injection vector through string-based query construction
(S608)
[error] 121-121: Possible SQL injection vector through string-based query construction
(S608)
src/gradata/_tenant.py
[warning] 30-30: Consider moving this statement to an else block
(TRY300)
[warning] 60-60: Consider moving this statement to an else block
(TRY300)
src/gradata/_migrations/tenant_uuid.py
[warning] 79-79: Consider moving this statement to an else block
(TRY300)
tests/test_cloud_row_push.py
[warning] 62-62: Missing return type annotation for private function fake_post
(ANN202)
[warning] 90-90: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
src/gradata/_cloud_sync.py
[error] 127-127: Possible SQL injection vector through string-based query construction
(S608)
[error] 139-150: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 153-153: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
src/gradata/_migrations/001_add_tenant_id.py
[error] 131-131: Possible SQL injection vector through string-based query construction
(S608)
[error] 134-134: Possible SQL injection vector through string-based query construction
(S608)
[warning] 160-160: Too many branches (14 > 12)
(PLR0912)
[error] 187-187: Possible SQL injection vector through string-based query construction
(S608)
[error] 210-210: Possible SQL injection vector through string-based query construction
(S608)
[error] 216-216: Possible SQL injection vector through string-based query construction
(S608)
🔇 Additional comments (9)
src/gradata/rules/rule_engine/__init__.py (1)
15-15: LGTM. Safe, isolated cleanup in a pure re-export module.src/gradata/enhancements/self_improvement/__init__.py (1)
1-1: LGTM. Helpful clarification of the barrel-module intent without changing import behavior.src/gradata/rules/rule_graph.py (1)
242-260: Looks good. The confidence clamp is preserved, and the guardedALTER TABLEcalls keep older brains writable while adding tenant-scoped rows.src/gradata/audit.py (1)
49-61: Looks good. This keeps provenance writes forward-compatible for pre-tenant SQLite schemas while adding the tenant-scoped column at the write boundary.src/gradata/hooks/rule_enforcement.py (1)
39-52: Good hardening of env parsing and the zero-reminder off switch.
_env_int()now clamps invalid input safely, and the early return avoids emitting an empty reminder header whenGRADATA_MAX_REMINDERS=0.Based on learnings: In PR
#102(gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup.Also applies to: 127-131
docs/architecture/multi-tenant-future-proofing.md (1)
1-161: LGTM — Comprehensive multi-tenant architecture plan.The document provides clear, actionable guidance for tenant isolation with well-defined success criteria. The explicit "lock in now" vs "defer" separation is good engineering discipline.
src/gradata/_migrations/_runner.py (1)
112-122: LGTM — Clean brain DB resolution logic.The
resolve_brain_dbfunction handles both file paths and directories, with sensible fallbacks to environment variable and./brain. The implementation is straightforward and covers the expected use cases.src/gradata/_migrations/001_add_tenant_id.py (1)
51-84: LGTM — Comprehensive per-tenant table list.The
PER_TENANT_TABLESlist aligns with the architecture document and covers all expected tenant-scoped tables. The explicit exclusion comments (lines 94-101) are helpful for maintenance.src/gradata/enhancements/meta_rules_storage.py (1)
121-121: No concern here. The code path is safe and consistent throughout the codebase.
db_pathis always constructed asbrain.db_path = self.dir / "system.db"whereself.diris the resolved brain directory. This meansPath(db_path).parentwill always correctly resolve to the brain directory, andtenant_forsafely handles the path via.resolve(). There is no scenario where the parent directory assumption breaks down.> Likely an incorrect or invalid review comment.
| ALTER TABLE meta_rules | ||
| ADD COLUMN IF NOT EXISTS search_tsv tsvector | ||
| GENERATED ALWAYS AS ( | ||
| setweight(to_tsvector('english', coalesce(principle, '')), 'A') || | ||
| setweight(to_tsvector('english', coalesce(scope, '')), 'B') || | ||
| setweight(to_tsvector('english', coalesce(examples, '')), 'C') | ||
| ) STORED; | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_meta_rules_tsv | ||
| ON meta_rules USING GIN (search_tsv); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
to_tsvector on JSON columns may produce suboptimal search results.
Lines 69-70 apply to_tsvector('english', ...) to scope and examples, which appear to be JSON columns based on meta_rules_storage.py. This will index the JSON syntax (braces, quotes) along with the actual content. Consider extracting text values before indexing, or document that search quality may vary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/cloud-monolith-v2.md` around lines 65 - 74, The generated
tsvector currently applies to JSON columns (scope, examples) which indexes JSON
syntax; update the GENERATED ALWAYS AS expression for meta_rules.search_tsv to
extract and concatenate actual text values from those JSON columns before
calling to_tsvector (e.g., use jsonb_each_text or another jsonb->> extraction
and string_agg to combine values) so principle, scope and examples feed plain
text to to_tsvector; check meta_rules_storage.py to confirm the JSON structure
and choose the correct key extraction for scope/examples, then update the ALTER
TABLE CREATE INDEX migration to use those extracted text expressions instead of
raw JSON.
| url = f"{os.environ[ENV_URL].rstrip('/')}/rest/v1/{table}" | ||
| key = os.environ[ENV_KEY] | ||
| body = json.dumps(rows).encode("utf-8") | ||
| req = urllib.request.Request( | ||
| url, | ||
| data=body, | ||
| method="POST", | ||
| headers={ | ||
| "apikey": key, | ||
| "Authorization": f"Bearer {key}", | ||
| "Content-Type": "application/json", | ||
| # Upsert on primary key so repeat pushes are idempotent. | ||
| "Prefer": "resolution=merge-duplicates,return=minimal", | ||
| }, | ||
| ) | ||
| try: | ||
| # URL is sourced from GRADATA_CLOUD_URL env; operator-controlled. | ||
| with urllib.request.urlopen(req, timeout=30) as resp: | ||
| if 200 <= resp.status < 300: | ||
| return len(rows) | ||
| _log.warning("cloud_sync: %s returned HTTP %s", table, resp.status) | ||
| return 0 | ||
| except urllib.error.HTTPError as e: | ||
| _log.warning("cloud_sync: %s HTTP %s: %s", table, e.code, e.read()[:200]) | ||
| return 0 | ||
| except urllib.error.URLError as e: | ||
| _log.warning("cloud_sync: %s network error: %s", table, e) | ||
| return 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating URL scheme before opening.
While GRADATA_CLOUD_URL is operator-controlled, validating the scheme as https:// would prevent accidental file:// or other scheme usage flagged by the static analyzer (S310).
🛡️ Proposed scheme validation
def _post(table: str, rows: list[dict[str, Any]]) -> int:
"""POST rows to Supabase PostgREST. Returns count accepted."""
if not rows:
return 0
- url = f"{os.environ[ENV_URL].rstrip('/')}/rest/v1/{table}"
+ base = os.environ[ENV_URL].rstrip('/')
+ if not base.startswith(("https://", "http://")):
+ _log.warning("cloud_sync: invalid URL scheme in %s", ENV_URL)
+ return 0
+ url = f"{base}/rest/v1/{table}"
key = os.environ[ENV_KEY]🧰 Tools
🪛 Ruff (0.15.10)
[error] 139-150: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 153-153: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gradata/_cloud_sync.py` around lines 136 - 163, Validate the constructed
URL's scheme before making the request: parse the value coming from ENV_URL
(used to build the local variable url) with urllib.parse.urlparse and ensure
scheme == "https" (reject empty or other schemes like file://); if validation
fails, log a warning on _log and return 0 early instead of calling
urllib.request.urlopen in the cloud_sync path so only HTTPS endpoints are
allowed.
| except urllib.error.HTTPError as e: | ||
| _log.warning("cloud_sync: %s HTTP %s: %s", table, e.code, e.read()[:200]) | ||
| return 0 | ||
| except urllib.error.URLError as e: | ||
| _log.warning("cloud_sync: %s network error: %s", table, e) | ||
| return 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Log truncation may cut off actionable error details.
Line 159 truncates the error body to 200 bytes. For debugging Supabase RLS or constraint errors, this may lose the actual violation message. Consider 500–1000 bytes or log the full response at DEBUG level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gradata/_cloud_sync.py` around lines 158 - 163, The HTTP error log
currently truncates the response body to 200 bytes in the except
urllib.error.HTTPError as e handler, which can drop useful DB/RLS constraint
messages; update the handler in src/gradata/_cloud_sync.py to either increase
the truncation to a larger window (e.g., 500–1000 bytes) or log the full
e.read() at DEBUG while keeping the WARNING message shorter; specifically adjust
the _log.warning call that uses table, e.code and e.read()[:200] and add a
separate _log.debug entry that logs the full response body (ensure you call
e.read() only once or buffer it into a variable before logging).
| if fpath.exists(): | ||
| tid = fpath.read_text(encoding="utf-8").strip() | ||
| if _is_valid_uuid(tid): | ||
| return tid | ||
|
|
||
| brain.mkdir(parents=True, exist_ok=True) | ||
| tid = str(uuid.uuid4()) | ||
| try: | ||
| with open(fpath, "x", encoding="utf-8") as fh: | ||
| fh.write(tid) | ||
| return tid | ||
| except FileExistsError: | ||
| existing = fpath.read_text(encoding="utf-8").strip() | ||
| if _is_valid_uuid(existing): | ||
| return existing | ||
| # Corrupt file — overwrite with our fresh UUID so subsequent reads | ||
| # are consistent. Losing the junk content is the intended behavior. | ||
| fpath.write_text(tid, encoding="utf-8") | ||
| return tid |
There was a problem hiding this comment.
Don’t silently rotate a brain into a new tenant on corrupt .tenant_id.
If the file is truncated or manually damaged, Lines 65-68 replace it with a fresh UUID, and every subsequent write/sync goes to a different tenant. That effectively orphans the existing tenant-scoped data for this brain. For an identity file, fail fast and require explicit repair instead of auto-rekeying.
Suggested fix
if fpath.exists():
tid = fpath.read_text(encoding="utf-8").strip()
if _is_valid_uuid(tid):
return tid
+ raise ValueError(f"Invalid tenant id file: {fpath}")
brain.mkdir(parents=True, exist_ok=True)
tid = str(uuid.uuid4())
try:
with open(fpath, "x", encoding="utf-8") as fh:
fh.write(tid)
return tid
except FileExistsError:
existing = fpath.read_text(encoding="utf-8").strip()
if _is_valid_uuid(existing):
return existing
- # Corrupt file — overwrite with our fresh UUID so subsequent reads
- # are consistent. Losing the junk content is the intended behavior.
- fpath.write_text(tid, encoding="utf-8")
- return tid
+ raise ValueError(f"Invalid tenant id file: {fpath}")🧰 Tools
🪛 Ruff (0.15.10)
[warning] 60-60: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gradata/_tenant.py` around lines 50 - 68, The code currently overwrites a
corrupt tenant id file (fpath) with a new UUID, silently rotating the brain into
a new tenant; change this so any invalid/truncated tenant file is treated as an
error requiring manual repair: when reading fpath (the initial tid path check)
or inside the FileExistsError handler, if _is_valid_uuid(existing) is False,
raise a clear exception (e.g., ValueError or a custom CorruptTenantError)
instead of calling fpath.write_text(tid,...); keep the successful path (return
existing valid tid) and the fresh-create path (open(..., "x") write tid and
return) unchanged, but remove the auto-overwrite step so no silent rekeying
occurs.
| """Resolve brain directory from env, args, or cwd. | ||
|
|
||
| Precedence mirrors :func:`_resolve_brain_root` exactly — | ||
| ``GRADATA_BRAIN`` env > ``--brain-dir`` arg > cwd — so both helpers | ||
| always target the same brain (important for export, tests with tmp | ||
| brains, etc.). | ||
| """ | ||
| from gradata import Brain | ||
| brain_dir = getattr(args, "brain_dir", None) or Path.cwd() | ||
| brain_dir = env_str("GRADATA_BRAIN") or getattr(args, "brain_dir", None) or Path.cwd() |
There was a problem hiding this comment.
Align _get_brain() with _resolve_brain_root() before claiming they mirror exactly.
Line 43 falls back to Path.cwd(), but src/gradata/cli.py:685-693 still falls back to Path("brain"). Commands that mix these helpers can still read and write different brains when neither GRADATA_BRAIN nor --brain-dir is set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gradata/cli.py` around lines 35 - 43, The fallback in _get_brain() is
inconsistent with _resolve_brain_root(): change the brain_dir resolution in
_get_brain() (the line assigning brain_dir) so it uses the same fallback as
_resolve_brain_root() — either call _resolve_brain_root() directly or replace
Path.cwd() with Path("brain") — ensuring the env_str("GRADATA_BRAIN") >
getattr(args, "brain_dir", None) > Path("brain") precedence and keeping the
Brain import/instantiation logic unchanged.
| _tid = tenant_for(Path(db_path).parent) | ||
| conn = _get_db(db_path) |
There was a problem hiding this comment.
Finish tenant-scoping the analytics reads in this module.
These write paths now derive _tid, but detect_manual() and get_activity_stats() below still aggregate across the full activity_log / prep_outcomes tables. In a shared DB, one tenant's rows will skew another tenant's manual-detection and prep-effectiveness results.
Also applies to: 207-208, 238-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gradata/enhancements/scoring/loop_intelligence.py` around lines 154 -
155, The analytics functions detect_manual() and get_activity_stats() currently
query activity_log and prep_outcomes without tenant scoping, so results
aggregate across tenants; modify those query paths (and the other occurrences
around the commented ranges) to use the already-derived _tid (from
tenant_for(Path(db_path).parent)) and the same DB handle from _get_db(db_path)
and add a WHERE filter for the tenant identifier column (e.g., tenant_id = _tid
or the module’s canonical tenant column) to every SELECT/COUNT/AVG against
activity_log and prep_outcomes (and any joined subqueries) so each operation is
limited to rows for that tenant only; keep using the existing variables _tid,
conn, detect_manual(), and get_activity_stats() to locate and update the SQL/ORM
calls.
| conn = sqlite3.connect(brain / "system.db") | ||
| row = conn.execute( | ||
| "SELECT last_push_at FROM sync_state WHERE brain_id = ?", | ||
| ("11111111-2222-3333-4444-555555555555",), | ||
| ).fetchone() | ||
| conn.close() | ||
| assert row and row[0] # timestamp was recorded |
There was a problem hiding this comment.
Make the sync-state assertion explicit.
assert row and row[0] only checks truthiness, so the failure won't tell you whether the row was missing or last_push_at stayed empty. Split this into two asserts, and optionally validate the timestamp format.
As per coding guidelines: tests/**: assertions check specific values not just truthiness.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 90-90: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_cloud_row_push.py` around lines 84 - 90, The current truthy check
"assert row and row[0]" is ambiguous; split it into two explicit assertions:
first assert that the query returned a row (e.g., assert row is not None or
assert row, "sync_state row missing for brain_id ...") and then assert that
last_push_at is not empty (e.g., assert row[0] is not None or != "" with a clear
message). Optionally validate the timestamp format by attempting to parse row[0]
with datetime.fromisoformat or matching a regex to ensure it is a valid ISO8601
timestamp; update the test in tests/test_cloud_row_push.py around the
conn.execute result handling (variables: conn, row, last_push_at) accordingly.
| def test_fts_index_sets_tenant_id(brain_dir: Path, db_path: Path) -> None: | ||
| from gradata._paths import BrainContext | ||
| from gradata._query import fts_index | ||
|
|
||
| ctx = BrainContext.from_brain_dir(brain_dir) | ||
| fts_index("test/source.md", "general", "hello world", embed_date="2026-04-17", ctx=ctx) | ||
|
|
||
| conn = sqlite3.connect(str(db_path)) | ||
| row = conn.execute( | ||
| "SELECT tenant_id FROM brain_fts_content WHERE source = 'test/source.md'" | ||
| ).fetchone() | ||
| conn.close() | ||
| assert row is not None | ||
| assert row[0] == TEST_TENANT | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # _query.py — brain_fts_content (fts_index_batch) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_fts_index_batch_sets_tenant_id(brain_dir: Path, db_path: Path) -> None: | ||
| from gradata._paths import BrainContext | ||
| from gradata._query import fts_index_batch | ||
|
|
||
| ctx = BrainContext.from_brain_dir(brain_dir) | ||
| fts_index_batch( | ||
| [{"source": "batch/file.md", "file_type": "general", "text": "batch text", "embed_date": "2026-04-17"}], | ||
| ctx=ctx, | ||
| ) | ||
|
|
||
| conn = sqlite3.connect(str(db_path)) | ||
| row = conn.execute( | ||
| "SELECT tenant_id FROM brain_fts_content WHERE source = 'batch/file.md'" | ||
| ).fetchone() | ||
| conn.close() | ||
| assert row is not None | ||
| assert row[0] == TEST_TENANT |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression test for fts_rebuild().
This suite covers fts_index() and fts_index_batch(), but src/gradata/_query.py::fts_rebuild() was also changed to persist tenant_id and can regress independently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_tenant_id_inserts.py` around lines 277 - 314, Add a regression
test that calls fts_rebuild() and asserts it persists tenant_id: create a
BrainContext via BrainContext.from_brain_dir(brain_dir), call
fts_rebuild(ctx=ctx) (or with the appropriate args used by the implementation),
then query the brain_fts_content table for an example source added by the
rebuild and assert the returned tenant_id equals TEST_TENANT; reference the
fts_rebuild function and the brain_fts_content table/column names used in
existing tests to match style and DB access.
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Addressed the 3 in-diff review items in commit e53c648:
The remaining 16 comments are outside this PR's diff (pre-existing code shipped in #102). Not treating them as blocking for this docs-only PR. |
Adds pg_trgm + tsvector search, pgvector HNSW index, SKIP LOCKED queue + UNLOGGED cache to let Supabase replace Redis/Kafka/ Elasticsearch/Pinecone for gradata-cloud workloads. All idempotent, RLS-scoped per tenant. Schema forward-compatible — can land and sit idle until features opt in. Co-Authored-By: Gradata <noreply@gradata.ai>
… JSON tsvector caveat Co-Authored-By: Gradata <noreply@gradata.ai>
Gradata
force-pushed
the
feat/postgres-monolith-v2
branch
from
e53c648 to
235bfb1
Compare
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
Adds the cloud schema v2 doc that lets Supabase replace Redis/Kafka/Elasticsearch/Pinecone in one DB — no new vendors, RLS-scoped per tenant.
pg_trgm+tsvectorGIN indexes oneventsandmeta_rules→ native full-text searchpgvectorHNSW index onmeta_rules.embedding→ semantic retrievalsync_queuetable +sync_queue_claim(...)withFOR UPDATE SKIP LOCKED→ work queueUNLOGGED tenant_cachetable → ephemeral per-tenant cacheAll idempotent, all RLS-scoped. Schema is forward-compatible — it can land and sit idle until specific features opt in.
Reference: inspired by Thom's "I replaced my entire stack with Postgres" (https://www.tiktok.com/t/ZP8g5Fqrc/).
Test plan
002_cloud_monolith_v2row,pg_trgmextension, tsvector column populates, new policies visible inpg_policiescloud_rls_test.sqlGenerated with Gradata