feat(v0.7.0): gradata_recall MCP tool + universal hook adapters + audit CLI#171
feat(v0.7.0): gradata_recall MCP tool + universal hook adapters + audit CLI#171
Conversation
…it CLI Lands the v0.7.0 sprint: - gradata_recall(situation, max_tokens, ranker) MCP tool with token-budget cutoff and meta-rule reservation (always at least 1 META if any matches) - 6 PreToolUse hook adapters under hooks/adapters/: claude-code, codex, gemini, cursor, hermes, opencode. Each idempotent, atomic, signature-detected. - gradata install --agent X|all CLI command - gradata audit / gradata audit --json (recall coverage report) - BrainConfig.max_recall_tokens (default 2000) + ranker (hybrid|flat|tree_only) plumbed through 5 cap sites - INJECTABLE_META_SOURCES filter now logs warning when dropping rules (gate kept for safety; debug visible) - README quickstart leads with 'gradata install --agent X' Tests: 4174 passed / 4 skipped / 5 deselected. 54 new tests covering recall budget, adapters, install CLI, audit, BrainConfig propagation, meta-source filter warning. Lint: ruff check + format pass. Pyright: 0 errors, 27 optional-import warnings. Layering: no Layer 0 -> 2 imports introduced. Cloud dashboard widget: gradata-cloud not adjacent on this branch; TODO comment left in _audit.py for follow-up PR.
Required for ruff format --check to pass after the v0.7.0 feature commit. Pure formatting (whitespace, quote style, line wrapping). No logic changes. Also dedupes test_correction_event_captures_draft_text in test_rule_to_hook.py (was duplicated at lines 27 and 51 in main; codex collapsed to one).
|
Too many files changed for review. ( |
📝 Walkthrough
WalkthroughAdds configurable recall ranking and token-budgeted recall (MCP tool + CLI), ranker-aware rule retrieval, a brain-level config (BrainConfig) with recall defaults, a recall/audit CLI flow, a universal hook-adapter installer framework with per-agent adapters and CLI wiring, an audit utility for recall coverage, tenant-UUID race-hardened creation, hook prompt-budgeting and meta-rule injectable filtering, many adapter/hook modules and tests, and small behavioral fixes (cloud sync, materializer, client session coercion); most other edits are formatting/refactors. ChangesRecall ranking, token-budgeted recall, and brain config
Hook adapter installers and CLI wiring
Audit utility and CLI audit switch
Tenant UUID persistence and provenance
Cloud sync, client, and schema hardening (selected behavioral edits)
Miscellaneous additions and wide refactors (formatting, logging, small fixes)
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Client
participant MCP as "MCP Server"
participant Brain as "Brain"
participant RE as "Rule Engine"
participant FS as "Filesystem/DB"
Client->>MCP: JSON-RPC gradata_recall(situation, max_tokens?, ranker?, include_all_sources?)
MCP->>FS: resolve lessons_path & meta_rules_path from brain dir
MCP->>Brain: build scope from situation
alt ranker == "flat"
Brain->>RE: apply_rules(scope, max_rules?)
else ranker in {"hybrid","tree_only"}
Brain->>RE: apply_rules_with_tree(scope, ranker=...)
end
RE-->>Brain: applied/scored rules
Brain-->>MCP: ranked rule candidates
MCP->>FS: optionally load/filter meta-rules (INJECTABLE_META_SOURCES)
MCP->>MCP: select rules under token budget, format XML
MCP-->>Client: return JSON-RPC content (brain-rules XML)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
Gradata/src/gradata/_config_paths.py (2)
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_config_filecan escape the config directory for absolute/traversal inputs.
Pathjoining does not guarantee<config_dir>/<name>: absolutenamevalues bypass the base, and..segments can escape after normalization. This breaks the function’s stated contract and opens a path-traversal footgun ifnameever comes from input.Proposed hardening diff
def get_config_file(name: str) -> Path: """Return ``<config_dir>/<name>``. Convenience wrapper.""" - return get_config_dir() / name + rel = Path(name) + if rel.is_absolute() or ".." in rel.parts: + raise ValueError("name must be a relative path inside the config directory") + return get_config_dir() / rel🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/_config_paths.py` around lines 47 - 49, get_config_file currently concatenates get_config_dir() and name which allows absolute names or “..” segments to escape the config dir; change get_config_file to enforce that name is a relative, non-anchored path and that the final resolved path is inside get_config_dir(): reject or raise on Path(name).is_absolute() and on any '..' path.parts (or normalize by using PurePath and disallow parent references), then compute final = (get_config_dir() / name).resolve() and verify final.resolve().relative_to(get_config_dir().resolve()) (or raise if that fails) before returning final; reference get_config_file and get_config_dir in your changes.
36-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the documented “absolute path override” contract.
Line 10 in the module docs says
GRADATA_CONFIG_DIRis an absolute override, but relative values are currently accepted and resolved against the process CWD. That can produce surprising, non-portable config locations.Proposed contract-aligned check
override = os.environ.get(ENV_CONFIG_DIR, "").strip() if override: - return Path(override).expanduser().resolve() + override_path = Path(override).expanduser() + if not override_path.is_absolute(): + raise ValueError(f"{ENV_CONFIG_DIR} must be an absolute path") + return override_path.resolve()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/_config_paths.py` around lines 36 - 39, The env override currently accepts relative paths; enforce the documented absolute-only contract by validating ENV_CONFIG_DIR before resolving: when override = os.environ.get(ENV_CONFIG_DIR, "").strip() is set, check that Path(override).is_absolute() (after expanduser if you prefer) and if not, raise a clear ValueError (or similar) indicating GRADATA_CONFIG_DIR must be an absolute path; only then call Path(override).expanduser().resolve() and return it. This change touches the override handling around the ENV_CONFIG_DIR/override variable in the config path resolution logic.Gradata/src/gradata/enhancements/metrics.py (1)
127-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd logging to the exception handler.
The bare
except Exception: return {}silently swallows all errors in metrics computation, which could hide database corruption, schema issues, or other critical failures. As per coding guidelines, memory products must avoid silent failures.📊 Proposed fix to add exception logging
+ except Exception as e: + # Import logger if not already available at module level + import logging + logger = logging.getLogger(__name__) + logger.warning( + "Failed to compute metrics from database", + exc_info=True + ) - except Exception: return {}As per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/enhancements/metrics.py` around lines 127 - 128, The bare exception handler "except Exception: return {}" in the metrics computation should log the error instead of silently swallowing it; update the except block in the metrics computation function that contains "except Exception: return {}" to call the module logger (or create one via logging.getLogger(__name__) if none exists) and log the exception (e.g., logger.warning or logger.exception) with exc_info=True, then still return {} to preserve behavior.Gradata/src/gradata/hooks/dispatch_post.py (1)
50-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrease exception visibility in
_invoketo avoid silent hook failures.Catching all exceptions is fine for isolation, but debug-only logging without traceback can mask persistent failures in constituent hooks. Log at warning level with
exc_info=Trueso failures are diagnosable without breaking dispatch behavior.Proposed patch
def _invoke(module_name: str, data: dict) -> dict | None: """Import and invoke a constituent hook's main(). Suppress all errors.""" try: module = __import__(f"gradata.hooks.{module_name}", fromlist=["main"]) return module.main(data) except Exception as exc: - _log.debug("dispatch_post: %s suppressed exception: %s", module_name, exc) + _log.warning( + "dispatch_post: %s suppressed exception: %s", + module_name, + exc, + exc_info=True, + ) return NoneAs per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/hooks/dispatch_post.py` around lines 50 - 55, The except block in _invoke currently suppresses all exceptions with a debug log; change it to log at warning level and include the traceback by calling _log.warning("dispatch_post: %s suppressed exception", module_name, exc_info=True) (or equivalent) so failures are visible while still returning None; keep the import/__import__ and return None behavior unchanged and only replace the _log.debug call with a warning that passes exc_info=True and a clear message referencing module_name.Gradata/src/gradata/enhancements/scoring/loop_intelligence.py (1)
182-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t silently suppress event-emission failures.
Line 182 currently swallows all exceptions with no trace. Keep the non-blocking behavior, but emit a warning with
exc_info=Trueso failures are diagnosable.Suggested fix
+import logging + +logger = logging.getLogger(__name__) ... - # Never break the logging path on emit failure - with contextlib.suppress(Exception): - _emit_event_fn( + # Never break the logging path on emit failure + try: + _emit_event_fn( "DELTA_TAG", "loop_intelligence", { "activity_type": activity_type, "prospect": prospect, "company": company, "source": source, "prep_level": prep_level, "detail": detail, "activity_log_id": row_id, }, tags=[ t for t in [ f"prospect:{prospect}" if prospect else None, f"type:{activity_type}", ] if t ], session=session, ) + except Exception: + logger.warning("Failed to emit DELTA_TAG from log_activity", exc_info=True)As per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py` around lines 182 - 204, The current contextlib.suppress(Exception) around the call to _emit_event_fn in loop_intelligence is swallowing all exceptions silently; keep the non-blocking behavior but replace the silent suppression with catching Exception and calling logger.warning(..., exc_info=True) so failures are recorded (e.g., wrap the _emit_event_fn invocation in try/except Exception as e and log with logger.warning("Failed to emit DELTA_TAG event in loop_intelligence", exc_info=True) before continuing). Ensure you reference the same parameters/session and preserve tags construction and non-raising behavior.Gradata/tests/test_edit_distance_convergence.py (1)
11-33:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse
tmp_path+ sharedBRAIN_DIRfixture for Brain test isolation (nottempfile.mkdtemp()).Line 17 introduces ad-hoc temp dirs in the helper, which bypasses the suite’s isolation contract for Brain path handling. Please refactor this helper/tests to accept
tmp_pathand rely on the conftest-managedBRAIN_DIRsetup.As per coding guidelines, "Set
BRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/tests/test_edit_distance_convergence.py` around lines 11 - 33, The helper _make_brain currently creates an ad-hoc temp dir with tempfile.mkdtemp() which breaks test isolation; change _make_brain to accept the pytest tmp_path fixture and use the test-suite BRAIN_DIR setup by setting os.environ["BRAIN_DIR"] = str(tmp_path) (or rely on the shared BRAIN_DIR fixture), create the lessons.md file inside tmp_path, instantiate Brain against that path, and remove tempfile.mkdtemp() usage; after adjusting the env var ensure the _paths.py cache is refreshed before calling Brain.init() (e.g., importlib.reload on the module that reads BRAIN_DIR) so Brain picks up the test directory.Gradata/src/gradata/onboard.py (1)
446-468:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently swallow seed bootstrap failures.
The current
except Exception: passmasks real onboarding failures and removes observability on a user-visible setup path.🔧 Proposed fix
+import logging @@ +logger = logging.getLogger("gradata") @@ - except Exception: - pass # Seed rules are optional — don't block onboarding + except (ImportError, OSError, sqlite3.Error, ValueError): + logger.warning("Seed rule bootstrap skipped during onboarding", exc_info=True)As per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/onboard.py` around lines 446 - 468, The try/except around the seed rules block (involving write_lessons_safe, format_lessons, Lesson, seed_rules and lessons_path) currently swallows all exceptions; change it to catch only expected exceptions (e.g., IOError/OSError/ValueError) or re-raise unexpected ones, and ensure failures are logged with context (use logger.warning or similar with exc_info=True and a message like "Failed to seed lessons") so onboarding errors are observable instead of silently ignored. Ensure the handler still treats seed rules as non-fatal (doesn't raise for benign errors) but does not suppress unexpected exceptions.Gradata/src/gradata/_migrations/tenant_uuid.py (1)
53-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTOCTOU race and crash recovery gap.
Two concerns with the atomic-write logic:
TOCTOU between
fpath.exists()check andos.replace()(lines 53-54): Another process could createfpathafter the check but before the replace, causing an overwrite. This is mostly benign since all callers converge on whatever value ends up on disk, but it defeats "first writer wins" semantics.Crash recovery gap: If a previous run crashed after creating the temp file (line 46) but before cleanup, the stale
.tenant_id.tmp.<PID>persists. On the next run with the same PID,os.open(..., O_EXCL)fails, we catchFileExistsErrorandpass(line 58-60), then attemptfpath.read_text()at line 62 — butfpathmay not exist yet, raising an unhandledFileNotFoundError.🛡️ Proposed fix for crash recovery
except FileExistsError: # Extremely unlikely (PID collision); fall back to reading disk. - pass + # Stale temp from a prior crash — clean it up and retry once. + try: + os.unlink(tmp) + except OSError: + pass + # If fpath doesn't exist after cleanup, re-enter to create. + if not fpath.exists(): + return get_or_create_tenant_id(brain_dir) tid = fpath.read_text(encoding="utf-8").strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/_migrations/tenant_uuid.py` around lines 53 - 66, Replace the TOCTOU check+os.replace sequence with an atomic try/except around os.replace(tmp, fpath): attempt os.replace(tmp, fpath) and on FileExistsError treat it as "lost the race" by removing tmp (os.unlink(tmp)) and proceeding to read the winner; remove the separate fpath.exists() check. Also make reading the winner robust by wrapping fpath.read_text(...) in a try/except FileNotFoundError: if fpath is missing then either try to read tmp (if still exists) or fall back to returning new_tid; ensure the FileExistsError path when creating tmp (the os.open(..., O_EXCL) case) either generates a new unique tmp name (e.g., include uuid/random) or safely unlinks a stale temp before retrying so the flow does not end up with an unhandled FileNotFoundError. Apply these changes to the variables/functions used in the diff (tmp, fpath, new_tid, and the _is_valid_uuid check) so the logic is atomic and crash-recovery safe.Gradata/src/gradata/audit.py (1)
55-66:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove the defensive
ALTER TABLEfrom the write path; migrations are guaranteed to run first.
Brain.__init__callsrun_migrations()before any business logic (line 139 ofbrain.py), which applies Migration 001 that addstenant_idtorule_provenance. The defensive schema migration on every call is redundant and masks a guideline violation: the outerexcept Exceptionswallows write errors at debug level only, risking silent data loss without warning.Remove the
ALTER TABLEblock (lines 56–58) since the column is guaranteed to exist. Also upgrade exception handling from debug-level silence towarningwithexc_info=True; provenance insert failures are critical to audit integrity and should not be hidden.Suggested fix
with get_connection(db_path) as conn: - # Defensive migration: brains created before migration 001 lack tenant_id. - with _ctx.suppress(_sqlite3.OperationalError): - conn.execute("ALTER TABLE rule_provenance ADD COLUMN tenant_id TEXT") conn.execute( "INSERT INTO rule_provenance " "(rule_id, correction_event_id, session, timestamp, user_context, tenant_id) " "VALUES (?, ?, ?, ?, ?, ?)", (rule_id, correction_event_id, session, timestamp, user_context, _tid), ) except Exception as e: - _log.debug("write_provenance failed: %s", e) + _log.warning("write_provenance failed: %s", e, exc_info=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/audit.py` around lines 55 - 66, Remove the defensive schema migration inside write_provenance: delete the with _ctx.suppress(_sqlite3.OperationalError): conn.execute("ALTER TABLE rule_provenance ADD COLUMN tenant_id TEXT") block so the write path no longer alters schema; then change the exception handler at the end of write_provenance to call _log.warning with exc_info=True instead of _log.debug (i.e., replace _log.debug("write_provenance failed: %s", e) with a warning-level log including the exception info) to ensure provenance insert failures are surfaced.Gradata/tests/test_coverage_gaps.py (1)
28-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReload
_pathsbefore propagating cached paths out of_init_brain().This helper updates
os.environ["BRAIN_DIR"], but it never refreshesgradata._pathsbefore copying_p.BRAIN_DIR/DB_PATH/...into other modules. If_pathswas already imported by an earlier test, every reassignment below can propagate stale paths and make the suite flaky.As per coding guidelines, "Set
BRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/tests/test_coverage_gaps.py` around lines 28 - 64, The helper _init_brain sets os.environ["BRAIN_DIR"] and calls Brain.init but then reads cached values from gradata._paths (_p) which may be stale; fix by reloading the gradata._paths module (using importlib.reload on the imported _p) after setting BRAIN_DIR and/or after Brain.init, then propagate the refreshed attributes from _p into gradata._events (_ev), _brain_manifest (_bm), _export_brain (_ex), _query (_q) and _tag_taxonomy (_tt) so they receive current BRAIN_DIR/DB_PATH/etc.; update the _init_brain function (reference: _init_brain, os.environ["BRAIN_DIR"], Brain.init, gradata._paths as _p) to perform the reload before copying path values.Gradata/tests/test_cli_seed.py (1)
25-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh
gradata._pathsbefore callingBrain.init()in these tests.These cases initialize a fresh brain under
tmp_path, but they never reset the process-global path cache first. If_paths.pywas imported by an earlier test,cmd_seed()can end up reading or mutating the previous brain and make this module order-dependent in CI.As per coding guidelines, "Set
BRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/tests/test_cli_seed.py` around lines 25 - 55, These tests must refresh the gradata._paths module cache before calling Brain.init() to avoid reading stale process-global paths; update each test (the ones calling Brain.init() and cmd_seed(), e.g., test_cmd_seed_adds_7_rules_first_run, test_cmd_seed_is_idempotent, test_cmd_seed_errors_without_flag, test_cmd_seed_creates_missing_brain_dir) to reload gradata._paths (for example via importlib.reload(gradata._paths) or by popping 'gradata._paths' from sys.modules and re-importing) and ensure BRAIN_DIR is set to the tmp_path-based brain before calling Brain.init().Gradata/src/gradata/_query.py (1)
48-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace broad exception suppression with schema introspection to prevent silent migration failures.
sqlite3.OperationalErrorcovers both duplicate-column errors AND operational failures likedatabase is lockedand I/O issues. Suppressing it silently can allow the migration to skip, leaving the schema half-migrated and causing FTS corruption in later writes. Check column existence first withPRAGMA table_info(...)instead.Safer approach (precedent:
_migrations/_runner.py)- with contextlib.suppress(sqlite3.OperationalError): - conn.execute("ALTER TABLE brain_fts_content ADD COLUMN tenant_id TEXT") + columns = { + row[1] for row in conn.execute("PRAGMA table_info(brain_fts_content)").fetchall() + } + if "tenant_id" not in columns: + conn.execute("ALTER TABLE brain_fts_content ADD COLUMN tenant_id TEXT")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/_query.py` around lines 48 - 52, The code currently silences sqlite3.OperationalError around conn.execute("ALTER TABLE brain_fts_content ADD COLUMN tenant_id TEXT") which can hide real operational failures; instead, query the schema with PRAGMA table_info('brain_fts_content') and inspect the returned column names for "tenant_id" (use the same DB connection object used by conn) and only run the ALTER TABLE when that column is missing; remove contextlib.suppress(sqlite3.OperationalError) and let real sqlite errors surface (or catch and re-raise after logging) so only the explicit existence check controls whether the ALTER is executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Gradata/README.md`:
- Around line 103-107: The example sequence uses "gradata init ./my-brain" but
later runs commands without the explicit brain path (e.g., "gradata install
--agent claude-code" and "gradata audit"), which can resolve to a different
default; update the README examples so every command consistently references the
same brain (e.g., append "--brain ./my-brain" to "gradata install" and "gradata
audit" or state a single DEFAULT_BRAIN variable at the top), and ensure the
other occurrence mentioned (lines 205-207) is fixed the same way; look for the
commands "gradata init", "gradata install --agent claude-code", and "gradata
audit" to apply the change.
In `@Gradata/src/gradata/_audit.py`:
- Around line 151-154: The try/except around load_meta_rules(db_path) silently
swallows errors; change it to catch Exception as e and log the failure with
exc_info=True before returning an empty list. Specifically, in the block around
load_meta_rules and metas, replace the bare except with "except Exception as e:"
and call logger.warning("Failed to load meta rules from %s", db_path,
exc_info=True) (or similar) then return []; reference the load_meta_rules call
and the metas variable to locate where to add the logging.
- Around line 7-12: Add logging support by importing the logging module and
creating a module-level logger (e.g., logger = logging.getLogger(__name__))
alongside the existing imports in gradata._audit, then update the exception
handler around line 153 to call logger.exception(...) or logger.error(...,
exc_info=True) instead of silently swallowing the error so failures are recorded
with stacktrace context.
In `@Gradata/src/gradata/_export_brain.py`:
- Around line 123-133: The try/except in build_prospect_map that reads and
parses files (variables fm_name, fm_company, name_map) currently swallows all
errors; change it to catch specific exceptions (e.g., OSError,
UnicodeDecodeError, re.error) or at minimum Exception but log the failure with
logger.warning(..., exc_info=True) including context (file path and the failing
regex groups) so parse failures are visible and won't silently omit PII
replacements used by sanitize_content; apply the same fix to the other identical
try/except block handling file parsing so both places surface errors instead of
using bare except: pass.
In `@Gradata/src/gradata/_installer.py`:
- Around line 175-196: The try/except blocks that read and parse meta_file and
manifest_file silently swallow all errors; update the handlers in the code
around meta_file and manifest_file (used by list_installed or related installer
logic in _installer.py) to catch specific exceptions (at minimum
json.JSONDecodeError and OSError/FileNotFoundError) instead of a bare except,
and emit a warning via the module logger (e.g., logger.warning(...,
exc_info=True)) when parsing/IO fails so broken install metadata is visible
while keeping existing info.update behavior intact.
In `@Gradata/src/gradata/_validator.py`:
- Around line 246-247: The bare "except Exception: pass" in the timestamp-span
validation block in gradata/_validator.py must not silently swallow errors;
replace it with catching the expected exception types (e.g., ValueError,
TypeError) or, if uncertain, log the exception with logger.warning(...,
exc_info=True) including context ("timestamp-span validation failed for
event...") and then decide whether to mark the validation as failed or continue;
do not leave a silent pass — either propagate critical exceptions or record them
to ensure the dimension score isn't artificially inflated.
In `@Gradata/src/gradata/brain.py`:
- Around line 925-929: The cache key built for rules omits the max_rules
parameter causing different max_rules calls to collide; update the
RuleCache.make_key invocation where cache_key is created (the line assigning
cache_key) to include max_rules in the first component (e.g., incorporate
max_rules into the f-string along with scope.task_type, ranker, and
max_recall_tokens) so the key reflects scope.task_type, ranker,
max_recall_tokens, and max_rules while keeping scope.domain and scope.audience
as the other key parts.
- Around line 983-989: Truncation can cut through an XML/HTML tag and appending
"</brain-rules>" may leave a dangling partial tag; update the truncation logic
that uses max_recall_tokens, budget_chars and result so that after slicing to
budget_chars you strip any trailing partial tag before appending the closing tag
(e.g., remove any trailing substring from the last unmatched '<' to the end, or
truncate back to the last '>' boundary), then ensure the string ends with a
single well-formed "</brain-rules>" instead of blindly using removesuffix.
In `@Gradata/src/gradata/cli.py`:
- Around line 277-303: The brain directory resolution in _cmd_install_agent is
inconsistent with _resolve_brain_root: _cmd_install_agent uses
env_str("BRAIN_DIR") while _resolve_brain_root prefers GRADATA_BRAIN; update
_cmd_install_agent to use the same resolution logic as _resolve_brain_root
(either call _resolve_brain_root() to obtain brain_dir or replicate its lookup
order so GRADATA_BRAIN is checked before BRAIN_DIR), ensuring the adapter
install call (get_adapter, adapter.install) uses the identical resolved Path as
other commands.
In `@Gradata/src/gradata/cloud/_credentials.py`:
- Around line 54-55: Replace the silent contextlib.suppress(OSError) around
os.chmod(KEYFILE_PATH, 0o600) with an explicit try/except that catches OSError
and emits a warning containing the error details (use logger.warning(...,
exc_info=True)); if a module logger (e.g. logger = logging.getLogger(__name__))
does not exist, create one via the logging module before use so failures to
harden KEYFILE_PATH permissions are recorded rather than silently ignored.
In `@Gradata/src/gradata/cloud/_sync_state.py`:
- Around line 54-55: The current use of
contextlib.suppress(sqlite3.OperationalError) around conn.execute(f"ALTER TABLE
sync_state ADD COLUMN {col} {decl}") hides all OperationalErrors; replace it
with a targeted check: either query the existing columns via
conn.execute("PRAGMA table_info(sync_state)") and skip the ALTER if column `col`
already exists, or wrap the execute in try/except sqlite3.OperationalError as e
and if the error text indicates a duplicate column (e.g. "duplicate column" in
str(e).lower()) then ignore, otherwise log the error with logger.warning(...,
exc_info=True) and re-raise so real migration failures are visible; keep
references to conn.execute, the ALTER TABLE sync_state ADD COLUMN statement, and
the variables col/decl in your fix.
In `@Gradata/src/gradata/cloud/client.py`:
- Around line 201-205: The loop retries forever when a single-event batch still
raises _TooLargeError; inside the except _TooLargeError block (handling
_post()), detect if current_batch == 1 and in that case advance offset by 1 (or
otherwise skip/drop that problematic event) before continuing, and log a clear
message via logger.warning that the single event was skipped; otherwise keep the
existing behavior of halving current_batch and continue. Use the existing
symbols current_batch, offset, _post(), _TooLargeError, and logger.warning to
implement this conditional skip to avoid the infinite retry.
In `@Gradata/src/gradata/correction_detector.py`:
- Line 189: Update the docs-domain regex tuple (the (re.compile(...), "docs")
entry) so it matches plural forms: include "docs" and "documentation" inside the
alternation (e.g.
(doc|docs|documentation|document|readme|spec|design|architecture|plan)) while
keeping the \b word boundaries and re.IGNORECASE flag; modify that single tuple
to avoid falling back to "general" for common plural usages.
In `@Gradata/src/gradata/enhancements/edit_classifier.py`:
- Around line 56-58: _TONE_WORDS contains "I think", "I believe", "I feel" but
matching uses old_lower/new_lower so those entries never match; update the
_TONE_WORDS entries in edit_classifier.py to their lowercase forms ("i think",
"i believe", "i feel") or normalize the list to lowercase when it's created so
comparisons against old_lower/new_lower succeed (refer to the _TONE_WORDS
variable and the places where old_lower/new_lower are used).
In `@Gradata/src/gradata/enhancements/rule_verifier.py`:
- Line 71: The list comprehension that filters all_rules calls
should_verify(tool_type, rule.get("category", "UNKNOWN")) without guarding
non-string values; if a rule has category=None this leads to a .upper() crash
inside should_verify. Change the comprehension to normalize the category before
calling should_verify—for example, extract cat = rule.get("category", "UNKNOWN")
and use cat if isinstance(cat, str) else "UNKNOWN", then call
should_verify(tool_type, cat_normalized); update the expression that references
rule.get("category", "UNKNOWN") accordingly so should_verify always receives a
string.
In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py`:
- Around line 554-556: The hard-coded "Pipeline" label in the updated row should
use the passed-in tier argument instead: in the code that assigns lines[i] (the
f-string that currently contains "Pipeline"), replace that literal with the tier
parameter (e.g., {tier}) so calls like update_markdown_table(..., tier="Cold")
correctly label rows; locate the assignment to lines[i] in loop_intelligence.py
and substitute the tier variable into the f-string.
In `@Gradata/src/gradata/hooks/_generated_runner_core.py`:
- Around line 61-65: Remove the redundant text=True kwarg from the subprocess
call that currently has capture_output=True, text=True,
timeout=per_hook_timeout, encoding="utf-8", errors="replace"; because
encoding="utf-8" already implies text mode, delete the text=True entry so the
call only passes capture_output=True, timeout=per_hook_timeout,
encoding="utf-8", errors="replace" (locate the invocation in
_generated_runner_core.py by finding the parameter list containing
capture_output=True and encoding="utf-8").
In `@Gradata/src/gradata/hooks/_installer.py`:
- Around line 293-296: The _save_settings function currently writes
SETTINGS_PATH directly which risks corruption; replace the direct
SETTINGS_PATH.write_text call in _save_settings with the project atomic
JSON-write helper (the repository helper for atomic JSON persistence) and pass
the settings dict (and same formatting options, e.g., indent=2 and trailing
newline) to it, keeping the existing SETTINGS_PATH.parent.mkdir(...) step to
ensure the directory exists; update only the write line in _save_settings,
referencing SETTINGS_PATH and the helper (e.g., atomic_write_json or the repo's
equivalent) so writes become atomic.
In `@Gradata/src/gradata/hooks/adapters/cursor.py`:
- Around line 14-22: The current code replaces servers["gradata"] wholesale,
dropping user-added keys; change the logic to compare and update only the
managed keys ("command" and "args") returned from mcp_command(brain_dir) so
existing fields like env/cwd are preserved: call command = mcp_command(...),
build desired = {"command": command[0], "args": command[1:]}, then if existing
is a mapping merge only these two keys into existing (or compare
existing.get("command") and existing.get("args") to decide idempotence) and
update servers["gradata"] with those two keys instead of assigning desired
wholesale, then call write_json(agent_config_path, data) as before so other keys
remain intact.
In `@Gradata/src/gradata/hooks/adapters/hermes.py`:
- Around line 27-42: The current assembly builds raw strings and can duplicate
or overwrite top-level hooks; instead load the existing YAML (safe_load) from
agent_config_path into a dict, ensure a hooks -> pre_tool_use list exists,
append the new hook dict {id: sig, command: hook_command(brain_dir)} only if an
entry with the same id doesn't already exist, then dump the merged YAML back via
atomic_write_text; update the code around variables agent_config_path, existing,
atomic_write_text, sig, hook_command and brain_dir to perform structured
read/merge/write (use safe_load/safe_dump) so the operation is idempotent and
preserves user custom hooks.
In `@Gradata/src/gradata/hooks/inject_brain_rules.py`:
- Around line 440-449: The duplicate warnings come from logging non-injectable
metas in two places; modify the logic so filtering and warning happen once and
reuse the filtered result: extract the source-checking + _log.warning behavior
into a single helper (e.g., filter_injectable_metas(metas) or perform the check
only when building cached_metas) that returns the injectable metas and updates
meta_covered_categories/meta_covered_lesson_ids, then have both the mutex
pre-pass and the injection pipeline use that helper or skip emitting warnings
when cached_metas is being reused (check cached_metas truthiness before
warning), ensuring INJECTABLE_META_SOURCES, cached_metas, _log.warning,
meta_covered_categories, and meta_covered_lesson_ids are only referenced once
for warning/filtering.
In `@Gradata/src/gradata/hooks/pre_compact.py`:
- Around line 42-44: The comprehension that computes snapshot["lesson_count"]
uses line.startswith("#") on the raw line so indented comments like " # note"
are treated as lessons; update the comprehension in pre_compact.py (the
snapshot["lesson_count"] assignment that iterates over text.splitlines()) to
call line.strip() once per iteration (e.g., assign stripped = line.strip()) and
use stripped both for emptiness check and for startswith("#") so indented
comment lines are excluded.
In `@Gradata/src/gradata/mcp_server.py`:
- Around line 153-161: The MCP schema is hardcoding recall defaults
("max_tokens" default 2000 and "ranker" default "hybrid") which prevents
gradata_recall() from using None to fall back to BrainConfig; update the
dispatcher so the schema does not set concrete defaults for these recall fields
(use None / omit the "default" for "max_tokens" and "ranker" or explicitly allow
null) and ensure the request body leaves them as None when the client omits
them; apply the same change to the other occurrence referenced around lines
356-357 so both dispatch paths let gradata_recall() use the brain-configured
defaults.
In `@Gradata/src/gradata/notifications.py`:
- Around line 112-116: The notification handler currently swallows all
exceptions and logs them at debug in the try/except around calling fmt(payload
or {}) and callback(notif); change this to either catch specific exceptions
(e.g., ValueError/TypeError) or at minimum call _log.warning(..., exc_info=True)
instead of _log.debug so formatter/callback failures are visible; update the
except block that surrounds fmt and callback to use typed exceptions or
_log.warning with exc_info=True referencing fmt, callback and _log.
In `@Gradata/src/gradata/rules/rule_engine/_engine.py`:
- Around line 436-439: When an unknown ranker value is being silently
normalized, add a warning log before assigning ranker = "hybrid"; specifically,
check the ranker variable in the same block where you currently call apply_rules
and reassign invalid values, and emit a warning (e.g., logger.warning or module
logger) that includes the invalid ranker value and that it will default to
"hybrid". Update the code around the ranker check in _engine.py (the branch that
currently reads if ranker not in {"hybrid", "tree_only"}:) to log the unexpected
value referencing the ranker variable and then fall back to "hybrid" as before.
In `@Gradata/src/gradata/rules/rule_engine/_formatting.py`:
- Around line 508-509: Replace the bare "except Exception: pass" in
_formatting.py (the try/except that wraps the structured example attachment)
with code that logs the full exception and stack trace instead of silently
swallowing it: capture the exception using the module or existing logger (e.g.,
logger.exception or logging.getLogger(__name__).exception), include contextual
identifiers (example id/feature name or other available variables from the
attachment code) in the log message, and then either continue gracefully or
re-raise if the caller must handle failures; do not leave the exception
suppressed.
In `@Gradata/tests/test_batch_approval.py`:
- Around line 83-85: Tests call Brain.init() directly which bypasses the
conftest isolation (tmp_path BRAIN_DIR and _paths cache rewiring) and risks
cross-test state; replace direct Brain.init(...) in
Gradata/tests/test_batch_approval.py (and the other occurrence around lines
134-135) with the conftest-provided brain fixture or helper that sets BRAIN_DIR
via tmp_path, or if you must call Brain.init() here ensure you first set
os.environ["BRAIN_DIR"] = tmp_path and reload the _paths module cache (e.g.,
importlib.reload on the module that defines path constants) before calling
Brain.init() so the path-cache is updated and tests remain isolated.
In `@Gradata/tests/test_cloud_events_push.py`:
- Line 208: The unused timeout parameter in the test double function
fake_urlopen is causing lint ARG001 warnings after removing the noqa; rename the
parameter to start with an underscore (e.g., _timeout) so it still matches
urllib.request.urlopen's signature but signals intentional non-use, and apply
the same change to the other 11 test-doubles in this test module that accept a
timeout argument to avoid ARG001 warnings.
In `@Gradata/tests/test_convergence_gate.py`:
- Around line 28-39: The test refactored nested context managers into a single
parenthesized with-statement (in tests/test_convergence_gate.py around the
patch.object(brain, "_get_convergence") usage), which conflicts with the project
lint ignore for SIM117 in pyproject.toml that states nested context managers are
preferred for clarity; either revert this change and restore the original nested
with-...: blocks around patch.object(...) and
patch("gradata.enhancements.edit_classifier.extract_behavioral_instruction") to
match the documented rationale, or update pyproject.toml (the SIM117 ignore
comment) to reflect the new style (e.g., change the comment to "allow combined
or nested context managers" or remove SIM117 from the ignore list) so the code
style and policy are consistent.
In `@Gradata/tests/test_convergence.py`:
- Around line 12-37: Replace uses of tempfile.mkdtemp() in the helper
_make_brain_with_corrections and in test_convergence_per_category with the
conftest-provided tmp_path + BRAIN_DIR pattern: create the brain directory under
tmp_path / BRAIN_DIR, set os.environ["BRAIN_DIR"] to that path (or call
Brain.init() after setting it) so _paths.py cache is refreshed, then construct
Brain(...) against that directory instead of the tempfile result; reference
Brain.init(), _paths.py, _make_brain_with_corrections, and
test_convergence_per_category when making these changes.
In `@Gradata/tests/test_dualwrite_atomicity.py`:
- Around line 133-136: Replace the process-wide mutation os.environ["BRAIN_DIR"]
= str(tmp_path) in the test with a scoped monkeypatch.setenv("BRAIN_DIR",
str(tmp_path)) so the environment change is isolated to the test; after setting
the env via monkeypatch, ensure the Brain initialization re-reads the paths by
calling Brain.init() (or otherwise reload/clear the _paths.py module cache)
before instantiating Brain() so the test uses the tmp_path-scoped BRAIN_DIR.
In `@Gradata/tests/test_inspection.py`:
- Around line 295-297: Tests should not call Brain.init() directly; instead use
the shared isolated setup that sets BRAIN_DIR via tmp_path in conftest.py so
_paths.py is refreshed and tests remain isolated. Replace the direct
Brain.init(...) calls with the provided test fixture (or set
os.environ['BRAIN_DIR']=tmp_path and reload the _paths.py module cache before
invoking Brain.init) so the conftest cache-refresh path is used; reference
Brain.init, BRAIN_DIR, _paths.py and conftest.py when making the change.
In `@Gradata/tests/test_loop_intelligence.py`:
- Around line 515-521: The test test_updates_existing_row currently only asserts
numeric content after calling update_markdown_table with SAMPLE_MD; update it to
also assert that non-Tier rows do not regain extra table cells by adding a
negative assertion such as ensuring the returned string r does not contain the
marker "Auto-updated" or any extra cell text for non-Tier rows. Locate the test
function test_updates_existing_row and the call to update_markdown_table and add
one or more assertions like assert "Auto-updated" not in r (or assert
r.count("|") matches expected columns for that row) to lock in the new non-Tier
row shape. Ensure you reference SAMPLE_MD and the updated row payload so the
negative check verifies no extra cells were reintroduced.
In `@Gradata/tests/test_mcp_recall_token_budget.py`:
- Around line 37-38: The token-budget assertion uses floor division which lets
lengths 401–403 slip; update the second assertion that currently reads "assert
len(text) // 4 <= 100" to use ceiling division so boundary overages fail (e.g.,
import math and use "assert math.ceil(len(text) / 4) <= 100" or use integer-safe
ceiling "(len(text) + 3) // 4 <= 100"), keeping the existing "text" variable and
the surrounding asserts unchanged.
---
Outside diff comments:
In `@Gradata/src/gradata/_config_paths.py`:
- Around line 47-49: get_config_file currently concatenates get_config_dir() and
name which allows absolute names or “..” segments to escape the config dir;
change get_config_file to enforce that name is a relative, non-anchored path and
that the final resolved path is inside get_config_dir(): reject or raise on
Path(name).is_absolute() and on any '..' path.parts (or normalize by using
PurePath and disallow parent references), then compute final = (get_config_dir()
/ name).resolve() and verify
final.resolve().relative_to(get_config_dir().resolve()) (or raise if that fails)
before returning final; reference get_config_file and get_config_dir in your
changes.
- Around line 36-39: The env override currently accepts relative paths; enforce
the documented absolute-only contract by validating ENV_CONFIG_DIR before
resolving: when override = os.environ.get(ENV_CONFIG_DIR, "").strip() is set,
check that Path(override).is_absolute() (after expanduser if you prefer) and if
not, raise a clear ValueError (or similar) indicating GRADATA_CONFIG_DIR must be
an absolute path; only then call Path(override).expanduser().resolve() and
return it. This change touches the override handling around the
ENV_CONFIG_DIR/override variable in the config path resolution logic.
In `@Gradata/src/gradata/_migrations/tenant_uuid.py`:
- Around line 53-66: Replace the TOCTOU check+os.replace sequence with an atomic
try/except around os.replace(tmp, fpath): attempt os.replace(tmp, fpath) and on
FileExistsError treat it as "lost the race" by removing tmp (os.unlink(tmp)) and
proceeding to read the winner; remove the separate fpath.exists() check. Also
make reading the winner robust by wrapping fpath.read_text(...) in a try/except
FileNotFoundError: if fpath is missing then either try to read tmp (if still
exists) or fall back to returning new_tid; ensure the FileExistsError path when
creating tmp (the os.open(..., O_EXCL) case) either generates a new unique tmp
name (e.g., include uuid/random) or safely unlinks a stale temp before retrying
so the flow does not end up with an unhandled FileNotFoundError. Apply these
changes to the variables/functions used in the diff (tmp, fpath, new_tid, and
the _is_valid_uuid check) so the logic is atomic and crash-recovery safe.
In `@Gradata/src/gradata/_query.py`:
- Around line 48-52: The code currently silences sqlite3.OperationalError around
conn.execute("ALTER TABLE brain_fts_content ADD COLUMN tenant_id TEXT") which
can hide real operational failures; instead, query the schema with PRAGMA
table_info('brain_fts_content') and inspect the returned column names for
"tenant_id" (use the same DB connection object used by conn) and only run the
ALTER TABLE when that column is missing; remove
contextlib.suppress(sqlite3.OperationalError) and let real sqlite errors surface
(or catch and re-raise after logging) so only the explicit existence check
controls whether the ALTER is executed.
In `@Gradata/src/gradata/audit.py`:
- Around line 55-66: Remove the defensive schema migration inside
write_provenance: delete the with _ctx.suppress(_sqlite3.OperationalError):
conn.execute("ALTER TABLE rule_provenance ADD COLUMN tenant_id TEXT") block so
the write path no longer alters schema; then change the exception handler at the
end of write_provenance to call _log.warning with exc_info=True instead of
_log.debug (i.e., replace _log.debug("write_provenance failed: %s", e) with a
warning-level log including the exception info) to ensure provenance insert
failures are surfaced.
In `@Gradata/src/gradata/enhancements/metrics.py`:
- Around line 127-128: The bare exception handler "except Exception: return {}"
in the metrics computation should log the error instead of silently swallowing
it; update the except block in the metrics computation function that contains
"except Exception: return {}" to call the module logger (or create one via
logging.getLogger(__name__) if none exists) and log the exception (e.g.,
logger.warning or logger.exception) with exc_info=True, then still return {} to
preserve behavior.
In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py`:
- Around line 182-204: The current contextlib.suppress(Exception) around the
call to _emit_event_fn in loop_intelligence is swallowing all exceptions
silently; keep the non-blocking behavior but replace the silent suppression with
catching Exception and calling logger.warning(..., exc_info=True) so failures
are recorded (e.g., wrap the _emit_event_fn invocation in try/except Exception
as e and log with logger.warning("Failed to emit DELTA_TAG event in
loop_intelligence", exc_info=True) before continuing). Ensure you reference the
same parameters/session and preserve tags construction and non-raising behavior.
In `@Gradata/src/gradata/hooks/dispatch_post.py`:
- Around line 50-55: The except block in _invoke currently suppresses all
exceptions with a debug log; change it to log at warning level and include the
traceback by calling _log.warning("dispatch_post: %s suppressed exception",
module_name, exc_info=True) (or equivalent) so failures are visible while still
returning None; keep the import/__import__ and return None behavior unchanged
and only replace the _log.debug call with a warning that passes exc_info=True
and a clear message referencing module_name.
In `@Gradata/src/gradata/onboard.py`:
- Around line 446-468: The try/except around the seed rules block (involving
write_lessons_safe, format_lessons, Lesson, seed_rules and lessons_path)
currently swallows all exceptions; change it to catch only expected exceptions
(e.g., IOError/OSError/ValueError) or re-raise unexpected ones, and ensure
failures are logged with context (use logger.warning or similar with
exc_info=True and a message like "Failed to seed lessons") so onboarding errors
are observable instead of silently ignored. Ensure the handler still treats seed
rules as non-fatal (doesn't raise for benign errors) but does not suppress
unexpected exceptions.
In `@Gradata/tests/test_cli_seed.py`:
- Around line 25-55: These tests must refresh the gradata._paths module cache
before calling Brain.init() to avoid reading stale process-global paths; update
each test (the ones calling Brain.init() and cmd_seed(), e.g.,
test_cmd_seed_adds_7_rules_first_run, test_cmd_seed_is_idempotent,
test_cmd_seed_errors_without_flag, test_cmd_seed_creates_missing_brain_dir) to
reload gradata._paths (for example via importlib.reload(gradata._paths) or by
popping 'gradata._paths' from sys.modules and re-importing) and ensure BRAIN_DIR
is set to the tmp_path-based brain before calling Brain.init().
In `@Gradata/tests/test_coverage_gaps.py`:
- Around line 28-64: The helper _init_brain sets os.environ["BRAIN_DIR"] and
calls Brain.init but then reads cached values from gradata._paths (_p) which may
be stale; fix by reloading the gradata._paths module (using importlib.reload on
the imported _p) after setting BRAIN_DIR and/or after Brain.init, then propagate
the refreshed attributes from _p into gradata._events (_ev), _brain_manifest
(_bm), _export_brain (_ex), _query (_q) and _tag_taxonomy (_tt) so they receive
current BRAIN_DIR/DB_PATH/etc.; update the _init_brain function (reference:
_init_brain, os.environ["BRAIN_DIR"], Brain.init, gradata._paths as _p) to
perform the reload before copying path values.
In `@Gradata/tests/test_edit_distance_convergence.py`:
- Around line 11-33: The helper _make_brain currently creates an ad-hoc temp dir
with tempfile.mkdtemp() which breaks test isolation; change _make_brain to
accept the pytest tmp_path fixture and use the test-suite BRAIN_DIR setup by
setting os.environ["BRAIN_DIR"] = str(tmp_path) (or rely on the shared BRAIN_DIR
fixture), create the lessons.md file inside tmp_path, instantiate Brain against
that path, and remove tempfile.mkdtemp() usage; after adjusting the env var
ensure the _paths.py cache is refreshed before calling Brain.init() (e.g.,
importlib.reload on the module that reads BRAIN_DIR) so Brain picks up the test
directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| pip install gradata | ||
| gradata init ./my-brain | ||
| gradata install --agent claude-code --brain ./my-brain | ||
| gradata audit | ||
| ``` |
There was a problem hiding this comment.
Use a consistent brain path across the example command sequence.
The snippets initialize ./my-brain but later run commands without --brain, which can resolve to a different default brain directory and confuse first-time setup.
Suggested doc fix
gradata init ./my-brain
gradata install --agent claude-code --brain ./my-brain
-gradata audit
+gradata audit --brain ./my-brain gradata init ./my-brain # create a brain
-gradata install --agent codex # install recall hook for an agent
+gradata install --agent codex --brain ./my-brain # install recall hook for an agentAlso applies to: 205-207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/README.md` around lines 103 - 107, The example sequence uses "gradata
init ./my-brain" but later runs commands without the explicit brain path (e.g.,
"gradata install --agent claude-code" and "gradata audit"), which can resolve to
a different default; update the README examples so every command consistently
references the same brain (e.g., append "--brain ./my-brain" to "gradata
install" and "gradata audit" or state a single DEFAULT_BRAIN variable at the
top), and ensure the other occurrence mentioned (lines 205-207) is fixed the
same way; look for the commands "gradata init", "gradata install --agent
claude-code", and "gradata audit" to apply the change.
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from datetime import UTC, datetime, timedelta | ||
| from pathlib import Path | ||
| from typing import Any |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add logging support for exception handling.
The module lacks a logger, but exception handling at line 153 should log failures. Add logging setup.
Proposed fix
from __future__ import annotations
import json
+import logging
from datetime import UTC, datetime, timedelta
from pathlib import Path
from typing import Any
from gradata.hooks.adapters._base import AGENTS, adapter_config_path
+
+_log = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/_audit.py` around lines 7 - 12, Add logging support by
importing the logging module and creating a module-level logger (e.g., logger =
logging.getLogger(__name__)) alongside the existing imports in gradata._audit,
then update the exception handler around line 153 to call logger.exception(...)
or logger.error(..., exc_info=True) instead of silently swallowing the error so
failures are recorded with stacktrace context.
| try: | ||
| metas = load_meta_rules(db_path) | ||
| except Exception: | ||
| return [] |
There was a problem hiding this comment.
Add logging when meta-rule loading fails.
The except Exception: block silently swallows errors. Per coding guidelines, exceptions should at minimum log with exc_info=True to avoid silent failures.
Proposed fix
try:
metas = load_meta_rules(db_path)
- except Exception:
+ except Exception as exc:
+ _log.warning("load_meta_rules failed for %s: %s", db_path, exc)
return []As per coding guidelines: "Never use bare except: pass — use typed exceptions or at minimum logger.warning(...) with exc_info=True to avoid silent failure in a memory product."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/_audit.py` around lines 151 - 154, The try/except around
load_meta_rules(db_path) silently swallows errors; change it to catch Exception
as e and log the failure with exc_info=True before returning an empty list.
Specifically, in the block around load_meta_rules and metas, replace the bare
except with "except Exception as e:" and call logger.warning("Failed to load
meta rules from %s", db_path, exc_info=True) (or similar) then return [];
reference the load_meta_rules call and the metas variable to locate where to add
the logging.
| try: | ||
| text = f.read_text(encoding="utf-8") | ||
| fm_name = re.search(r'^name:\s*(.+)$', text, re.MULTILINE) | ||
| fm_name = re.search(r"^name:\s*(.+)$", text, re.MULTILINE) | ||
| if fm_name and fm_name.group(1).strip(): | ||
| val = fm_name.group(1).strip() | ||
| name_map[val] = f"[PROSPECT_{counter}]" | ||
| fm_company = re.search(r'^company:\s*(.+)$', text, re.MULTILINE) | ||
| fm_company = re.search(r"^company:\s*(.+)$", text, re.MULTILINE) | ||
| if fm_company and fm_company.group(1).strip(): | ||
| name_map[fm_company.group(1).strip()] = f"[COMPANY_{counter}]" | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silent parse failures in build_prospect_map can leak unredacted names
Both except Exception: pass branches suppress failures that feed the PII redaction map. If parsing fails quietly, sanitize_content may miss owner/prospect replacements in export output.
Suggested fix
+import logging
@@
+_log = logging.getLogger(__name__)
@@
- except Exception:
- pass
+ except Exception:
+ _log.warning(
+ "Failed to parse prospect metadata for redaction map: %s",
+ f,
+ exc_info=True,
+ )
@@
- except Exception:
- pass
+ except Exception:
+ _log.warning(
+ "Failed to parse brain manifest owner for redaction map: %s",
+ manifest_path,
+ exc_info=True,
+ )As per coding guidelines: "Never use bare except: pass — use typed exceptions or at minimum logger.warning(...) with exc_info=True to avoid silent failure in a memory product".
Also applies to: 141-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/_export_brain.py` around lines 123 - 133, The try/except
in build_prospect_map that reads and parses files (variables fm_name,
fm_company, name_map) currently swallows all errors; change it to catch specific
exceptions (e.g., OSError, UnicodeDecodeError, re.error) or at minimum Exception
but log the failure with logger.warning(..., exc_info=True) including context
(file path and the failing regex groups) so parse failures are visible and won't
silently omit PII replacements used by sanitize_content; apply the same fix to
the other identical try/except block handling file parsing so both places
surface errors instead of using bare except: pass.
| try: | ||
| meta = json.loads(meta_file.read_text(encoding="utf-8")) | ||
| info.update({ | ||
| "version": meta.get("brain_version"), | ||
| "domain": meta.get("domain"), | ||
| "installed": meta.get("installed_at", "?")[:10], | ||
| }) | ||
| info.update( | ||
| { | ||
| "version": meta.get("brain_version"), | ||
| "domain": meta.get("domain"), | ||
| "installed": meta.get("installed_at", "?")[:10], | ||
| } | ||
| ) | ||
| except Exception: | ||
| pass | ||
| elif manifest_file.exists(): | ||
| try: | ||
| manifest = json.loads(manifest_file.read_text(encoding="utf-8")) | ||
| info.update({ | ||
| "version": manifest.get("metadata", {}).get("brain_version"), | ||
| "domain": manifest.get("metadata", {}).get("domain"), | ||
| }) | ||
| info.update( | ||
| { | ||
| "version": manifest.get("metadata", {}).get("brain_version"), | ||
| "domain": manifest.get("metadata", {}).get("domain"), | ||
| } | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Replace silent except ...: pass in install listing path.
Lines 184-185 and 195-196 swallow parse failures, making broken install metadata invisible and hard to diagnose for list_installed(). Use typed exceptions and at least warning-level logging with exc_info=True.
Proposed fix
+import logging
+
+_log = logging.getLogger(__name__)
...
- except Exception:
- pass
+ except (OSError, json.JSONDecodeError) as e:
+ _log.warning("Failed to parse %s: %s", meta_file, e, exc_info=True)
...
- except Exception:
- pass
+ except (OSError, json.JSONDecodeError) as e:
+ _log.warning("Failed to parse %s: %s", manifest_file, e, exc_info=True)As per coding guidelines, "Never use bare except: pass — use typed exceptions or at minimum logger.warning(...) with exc_info=True to avoid silent failure in a memory product".
📝 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.
| try: | |
| meta = json.loads(meta_file.read_text(encoding="utf-8")) | |
| info.update({ | |
| "version": meta.get("brain_version"), | |
| "domain": meta.get("domain"), | |
| "installed": meta.get("installed_at", "?")[:10], | |
| }) | |
| info.update( | |
| { | |
| "version": meta.get("brain_version"), | |
| "domain": meta.get("domain"), | |
| "installed": meta.get("installed_at", "?")[:10], | |
| } | |
| ) | |
| except Exception: | |
| pass | |
| elif manifest_file.exists(): | |
| try: | |
| manifest = json.loads(manifest_file.read_text(encoding="utf-8")) | |
| info.update({ | |
| "version": manifest.get("metadata", {}).get("brain_version"), | |
| "domain": manifest.get("metadata", {}).get("domain"), | |
| }) | |
| info.update( | |
| { | |
| "version": manifest.get("metadata", {}).get("brain_version"), | |
| "domain": manifest.get("metadata", {}).get("domain"), | |
| } | |
| ) | |
| except Exception: | |
| pass | |
| try: | |
| meta = json.loads(meta_file.read_text(encoding="utf-8")) | |
| info.update( | |
| { | |
| "version": meta.get("brain_version"), | |
| "domain": meta.get("domain"), | |
| "installed": meta.get("installed_at", "?")[:10], | |
| } | |
| ) | |
| except (OSError, json.JSONDecodeError) as e: | |
| _log.warning("Failed to parse %s: %s", meta_file, e, exc_info=True) | |
| elif manifest_file.exists(): | |
| try: | |
| manifest = json.loads(manifest_file.read_text(encoding="utf-8")) | |
| info.update( | |
| { | |
| "version": manifest.get("metadata", {}).get("brain_version"), | |
| "domain": manifest.get("metadata", {}).get("domain"), | |
| } | |
| ) | |
| except (OSError, json.JSONDecodeError) as e: | |
| _log.warning("Failed to parse %s: %s", manifest_file, e, exc_info=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/_installer.py` around lines 175 - 196, The try/except
blocks that read and parse meta_file and manifest_file silently swallow all
errors; update the handlers in the code around meta_file and manifest_file (used
by list_installed or related installer logic in _installer.py) to catch specific
exceptions (at minimum json.JSONDecodeError and OSError/FileNotFoundError)
instead of a bare except, and emit a warning via the module logger (e.g.,
logger.warning(..., exc_info=True)) when parsing/IO fails so broken install
metadata is visible while keeping existing info.update behavior intact.
CR findings (CodeRabbit, PR #171): - brain.py: cache key now includes max_rules (was colliding) - brain.py: token-budget truncation strips trailing partial XML before appending </brain-rules> (was leaving dangling <) - cloud/client.py: single-event _TooLargeError now skips event + warns (was infinite-retry) - _migrations/tenant_uuid.py: atomic-write race fixed with UUID tmp + FileExistsError handling - mcp_server.py: removed hardcoded recall defaults (was shadowing BrainConfig fall-back) - adapters/cursor.py: preserves user MCP server keys (env/cwd) instead of wholesale replacement - adapters/hermes.py: structured YAML read/merge/write (was raw strings, duplicated hooks) - inject_brain_rules.py: filter+warn extracted to single helper (was duplicate warnings) - edit_classifier.py: tone words lowercased to match comparison Silent except cleanup (replaced with logger.warning(..., exc_info=True)): - _validator, cloud/_credentials, cloud/_sync_state, audit, _query - notifications, rules/rule_engine/_formatting, enhancements/metrics - hooks/dispatch_post, onboard, scoring/loop_intelligence - hooks/_installer (now uses atomic_write_json) Path/env hardening: - _config_paths.get_config_dir validates absolute env override - _config_paths.get_config_file rejects path traversal (../, absolute) - cli._cmd_install_agent uses _resolve_brain_root() (consistent with rest) Windows CI fixes: - adapters/_base.py: home resolution honors HOME or USERPROFILE - adapters/_base.py: hook signatures use .as_posix() for cross-platform stability (was failing idempotency on win32) - test_dualwrite_atomicity: skipif win32 with pre-PR171 reason (existing fcntl/PermissionError issues, unrelated to this PR) Test isolation: - 6 test files migrated from direct Brain.init() to monkeypatch.setenv + importlib.reload(_paths) pattern - test_mcp_recall_token_budget: ceiling division on token budget assert (was letting 401-403 char text slip past 100-token budget) - test_convergence_gate: reverted to nested with-blocks (SIM117 policy) Tests: 4174 passed / 4 skipped / 5 deselected (parent verified, 180s) Lint: ruff check + format pass Types: pyright 0 errors, 27 existing optional-import warnings
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Gradata/src/gradata/cloud/client.py (1)
296-305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
OverflowErrorto exception handler for JSON-parsed special float values.Line 302 can raise
OverflowErrorwhenjson.loads()producesInfinityor-Infinityvalues that are then coerced to int. The current handler catches onlyValueError/TypeError, missing this case.💡 Proposed fix
- except (ValueError, TypeError): + except (ValueError, TypeError, OverflowError): session_val = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/cloud/client.py` around lines 296 - 305, The try/except around coercing session_raw to session_val (handling session_raw, session_val) currently only catches ValueError and TypeError but can raise OverflowError when json.loads yields Infinity/-Infinity and you call int() (see the block converting floats/others to int); update the exception handler for that conversion (the try block that sets session_val from session_raw) to also catch OverflowError so those cases fall back to session_val = None.Gradata/src/gradata/rules/rule_engine/_engine.py (1)
450-499:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the flat-engine safety gates after tree retrieval.
Once
has_pathsis true, this branch rankscandidatesdirectly and skips the flat pipeline’s TTL demotion and domain-disable checks. That meansranker="hybrid"can still inject stale RULE-tier lessons or rules already disabled forscope.domain, even thoughranker="flat"would suppress them. Since ranker is now a first-class config/MCP input, this becomes user-visible behavior drift.Please run the tree candidate pool through the same eligibility/scoping pipeline as
apply_rules()before sorting, or extract that shared filtering logic so both paths enforce the same policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/rules/rule_engine/_engine.py` around lines 450 - 499, The tree branch bypasses the flat-engine eligibility checks (TTL demotion and domain-disable) so candidates from RuleTree.get_rules_for_context can include stale or domain-disabled RULE-tier lessons; to fix, run the candidate list through the same eligibility/scoping pipeline used by apply_rules() before computing relevance and sorting (i.e., reuse or extract the flat pipeline logic that applies TTL demotion, domain-disable and other scope checks rather than ranking raw candidates), applying that filter to candidates (after retrieval and before building scored or before scored.sort) so RuleTree results and apply_rules() enforce identical policy; reference apply_rules(), RuleTree.get_rules_for_context, compute_scope_weight, lesson_scope, and the sorting code (_STATE_PRIORITY/_difficulty_from_lesson/_effective_conf) to locate where to plug the shared filter.Gradata/src/gradata/hooks/inject_brain_rules.py (1)
803-805:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
brain_prompt.mdbypasses the configured recall budget.This early return skips the
max_prompt_charscap computed at Lines 341-343._read_brain_prompt()still truncates against the globalMAX_BRAIN_PROMPT_CHARS, so a lowerbrain-config.jsonmax_recall_tokensis ignored wheneverbrain_prompt.mdexists.Suggested fix
bp_text = _read_brain_prompt(Path(brain_dir)) if bp_text: + if len(bp_text) > max_prompt_chars: + bp_text = bp_text[:max_prompt_chars].rstrip() return {"result": bp_text}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/hooks/inject_brain_rules.py` around lines 803 - 805, The early return that returns bp_text from inject_brain_rules.py bypasses the per-brain cap (max_prompt_chars) computed earlier; update the logic so bp_text is truncated to the computed max_prompt_chars before returning (either by passing the cap into _read_brain_prompt or by slicing bp_text after call), ensuring the final returned value respects the brain-config.json max_recall_tokens instead of only the global MAX_BRAIN_PROMPT_CHARS.
♻️ Duplicate comments (3)
Gradata/src/gradata/enhancements/scoring/loop_intelligence.py (1)
553-579:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTier scoping is still inconsistent in table updates/inserts.
Line 553 (
tier.lower() != "cold") makes non-coldupdates match all rows, and Line 578 still inserts"Pipeline"regardless of thetierargument. This can mislabel rows when callingupdate_markdown_table(..., tier="Cold")(or any non-default tier).Suggested fix
- is_pipeline = tier in line or tier.lower() != "cold" + has_tier = "Tier" in (lines[header_line] if header_line >= 0 else "") + row_tier = cells[5].strip().lower() if has_tier and len(cells) > 5 else "" + is_target_tier = (not has_tier) or (row_tier == tier.lower()) - if row_key in data and is_pipeline: + if row_key in data and is_target_tier: d = data[row_key] - has_tier = "Tier" in (lines[header_line] if header_line >= 0 else "") if has_tier: lines[i] = ( f"| {cells[0]} | {d['sent']} | {d['replies']} | {d['rate']}% | {d['confidence']} | {tier} | Auto-updated |" ) ... - if has_tier: - new_row = f"| {display} | {d['sent']} | {d['replies']} | {d['rate']}% | {d['confidence']} | Pipeline | Auto-added |" + if has_tier: + new_row = f"| {display} | {d['sent']} | {d['replies']} | {d['rate']}% | {d['confidence']} | {tier} | Auto-added |"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py` around lines 553 - 579, The tier scoping is wrong: replace the loose check is_pipeline = tier.lower() != "cold" with an explicit check for the pipeline tier (e.g., is_pipeline = tier and tier.lower() == "pipeline") so updates only target pipeline rows, and when adding new rows use the actual tier argument instead of the hardcoded "Pipeline" (update the new_row construction to insert tier or a normalized tier_label variable). Ensure you reference the same variables used in this block (tier, is_pipeline, data, header_line, last_table_line) so updated rows and inserted rows consistently reflect the provided tier.Gradata/src/gradata/_validator.py (1)
239-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord malformed timestamps as a failed
training_span_dayscheck.This still logs and skips the check instead of failing it. When parsing fails,
resultsdoes not get atraining_span_daysentry, soTRAINING_DEPTH.totalshrinks and the trust score can be overstated.Suggested fix
except (TypeError, ValueError): logger.warning("timestamp-span validation failed", exc_info=True) + results.append( + { + "check": "training_span_days", + "pass": False, + "note": "invalid timestamp format in events", + } + )As per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/_validator.py` around lines 239 - 250, When parsing timestamps fails in the training span calculation, do not skip adding a result: inside the except (TypeError, ValueError) handler for the training_span_days calculation append a failing entry to results (e.g., {"check": "training_span_days", "value": None, "pass": False, "note": "malformed timestamps — could not compute training span"}) and keep the existing logger.warning(..., exc_info=True); this ensures TRAINING_DEPTH.total reflects the failure and prevents overstating trust scores. Reference the results list, the "training_span_days" check name, and the existing logger call in _validator.py.Gradata/src/gradata/cloud/_sync_state.py (1)
48-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-raise failed
ALTER TABLEoperations after logging.
existing_colsalready eliminates the duplicate-column case, so anysqlite3.OperationalErrorhere is a real migration failure. Swallowing it can still commit/cache a partial schema, and_SCHEMA_ENSUREDthen prevents later calls from retrying the missing column repair.Suggested fix
try: conn.execute(f"ALTER TABLE sync_state ADD COLUMN {col} {decl}") except sqlite3.OperationalError: _log.warning("sync_state schema migration failed for column %s", col, exc_info=True) + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/cloud/_sync_state.py` around lines 48 - 60, The migration loop that adds missing columns (using existing_cols and conn.execute with "ALTER TABLE sync_state ADD COLUMN ...") currently logs sqlite3.OperationalError and swallows it; change the except block to log the warning (as it does) and then re-raise the caught exception (e.g., except sqlite3.OperationalError as e: _log.warning(..., exc_info=True); raise) so failed ALTER TABLE operations surface and do not leave _SCHEMA_ENSURED masking an incomplete schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Gradata/src/gradata/_config_paths.py`:
- Around line 52-59: get_config_file currently accepts empty or
current-directory names (name == "" or name == ".") because Path("")/Path(".")
resolves to the config directory; add an explicit rejection before resolving: in
the get_config_file logic check the original name (and the created requested
Path) and raise ValueError if name is empty or if requested == Path(".") (or
requested.parts == ()/represents the current directory), e.g. check if not
str(name).strip() or requested == Path(".") and raise "config file name must be
a relative path inside the config directory"; keep the existing checks using
get_config_dir, requested, resolved and the subsequent directory-escape
validation unchanged.
In `@Gradata/src/gradata/_query.py`:
- Around line 51-56: The current broad except sqlite3.OperationalError around
the ALTER TABLE for brain_fts_content can hide real migration failures; change
it to catch the OperationalError as e, check the error message (e.args[0] or
str(e)) and only suppress/ignore it if it indicates the concurrent
duplicate-column race (e.g., contains "duplicate column name" or similar SQLite
text), otherwise re-raise or log it as an error so the migration doesn't
silently fail — update the block around conn.execute("ALTER TABLE
brain_fts_content ADD COLUMN tenant_id TEXT") to implement this conditional
handling for sqlite3.OperationalError related to tenant_id.
In `@Gradata/src/gradata/brain.py`:
- Around line 887-895: The code currently calls current_brain_config() and pulls
max_recall_tokens and ranker from that process-global singleton (when
max_recall_tokens or ranker are None), which creates cross-brain leakage; change
the logic to resolve defaults from this Brain instance instead (e.g., load or
use a cached per-instance config tied to self.dir or an instance attribute)
rather than calling current_brain_config(). Specifically, replace the
current_brain_config() usage with reading the config for this brain (from
self.dir or from self._cached_config / self.resolved_config on the Brain
instance) and assign max_recall_tokens and ranker from that instance-specific
config so the rule-cache key and recall/defaults remain per-brain.
In `@Gradata/src/gradata/cli.py`:
- Around line 594-605: _resolve_brain_root currently prefers GRADATA_BRAIN and
defaults to "./brain", which conflicts with _get_brain and tests; change its
resolution order to mirror the rest of the CLI: first honor explicit CLI args
(getattr(args, "brain_dir") then getattr(args, "brain")), then prefer the
fixture-set env var BRAIN_DIR (env_str("BRAIN_DIR")), then fall back to
GRADATA_BRAIN (env_str("GRADATA_BRAIN")), and finally default to Path.cwd()
instead of Path("brain"); update the function _resolve_brain_root accordingly so
audit, recall, and agent install target the same brain as _get_brain.
- Around line 309-318: cmd_recall currently hardcodes lessons_path as brain_root
/ "lessons.md", which can miss discovered lessons; change it to reuse the
canonical lessons discovery used by cmd_export instead of hardcoding. Locate the
discovery logic that cmd_export uses (the same helper or API it calls to find
the canonical lessons file) and call that from cmd_recall to compute
lessons_path, then pass that discovered path into gradata_recall (keeping
gradata_recall, cmd_recall, cmd_export, and _resolve_brain_root names for
reference). Ensure you remove the hardcoded brain_root / "lessons.md" and
substitute the discovered lessons file path.
In `@Gradata/src/gradata/cloud/client.py`:
- Around line 181-183: The loop that iterates over pending events uses
batch_size to compute chunks and never advances offset when batch_size <= 0,
causing an infinite loop; add a guard at the start of the method (the function
that defines pending/offset/batch_size and runs while offset < len(pending)) to
validate batch_size (e.g., if batch_size is None or <= 0) and either raise a
ValueError or coerce it to a sensible minimum (1), so chunk = pending[offset:
offset + current_batch] always yields progress and offset can advance; reference
the variables batch_size, pending, offset and the while offset < len(pending)
loop when applying the fix.
In `@Gradata/src/gradata/enhancements/metrics.py`:
- Around line 95-122: The sqlite3 connection created as conn in metrics.py can
leak if an exception occurs before conn.close(); update the code in the function
that computes correction density to open the DB with a context manager (use
"with sqlite3.connect(str(db)) as conn:") or wrap the existing conn usage in
try/finally to ensure conn.close() always runs; make this change around the
block that sets max_session, min_session, outputs, corrections, and density so
the connection is reliably closed on errors.
In `@Gradata/src/gradata/hooks/adapters/_base.py`:
- Around line 70-77: The read_json function currently treats invalid or non-dict
JSON as an empty config and thus can silently overwrite user data; update
read_json to only return {} for non-existent files but to surface errors for
unreadable or invalid content by raising or logging and re-raising: keep the
initial exists() check to return {} when the file is missing, but when
path.exists() is True and json.loads(path.read_text(...)) raises
json.JSONDecodeError or OSError, log the error with exc_info=True (using the
module logger) and re-raise a descriptive exception (or re-raise the original),
and if the parsed data is not a dict raise a TypeError/ValueError instead of
returning {}; reference the read_json function and the exceptions
json.JSONDecodeError, OSError, and TypeError/ValueError so reviewers can locate
and apply the change.
- Around line 62-67: Replace the hardcoded "python" with the current interpreter
(use sys.executable) in both hook_command and mcp_command so installed agent
configs invoke the same Python that has gradata installed (update hook_command
to embed sys.executable in the returned shell string and mcp_command to return
[sys.executable, "-m", "gradata.mcp_server", "--brain-dir", str(brain_dir)]);
and update read_json to catch JSONDecodeError and OSError, log a warning using
logger.warning with exc_info=True when a file is unreadable/corrupted, and then
return {} (preserving the existing fallback) so corrupted configs are not
silently overwritten and failures are visible in logs.
In `@Gradata/src/gradata/hooks/inject_brain_rules.py`:
- Around line 807-810: The current truncation does a blind slice of the
concatenated prompt (result = mandatory_block + disposition_block + rules_block
+ meta_block) which can cut XML-like blocks mid-tag; modify inject_brain_rules
so you build and truncate by block boundaries instead: assemble blocks as a list
[mandatory_block, disposition_block, rules_block, meta_block] and concatenate
them one-by-one until adding the next block would exceed max_prompt_chars, then
return the concatenated safe result; if you must trim within a block, back up to
the last complete closing tag (e.g., use rfind('</') on the partial block) to
ensure you never return malformed markup. Ensure you reference and use the
existing variables mandatory_block, disposition_block, rules_block, meta_block,
max_prompt_chars, and the function inject_brain_rules.
In `@Gradata/src/gradata/onboard.py`:
- Around line 470-473: The except block that catches (OSError, ValueError) in
Gradata/src/gradata/onboard.py currently logs only the exception string and
loses the traceback; update the handler that logs "optional seed rules failed:
%s" (the except (OSError, ValueError) as exc block) to include exc_info=True in
the logger.warning call so the stack/context is preserved (i.e.,
logger.warning(..., exc_info=True)).
In `@Gradata/tests/test_convergence.py`:
- Around line 29-42: The test currently suppresses all exceptions from
brain.emit using contextlib.suppress, hiding fixture/setup failures; remove the
blanket suppression around the loop (or replace it with a narrow try/except that
only catches the particular expected exception type) so that errors from
brain.emit propagate and fail the test, and if you must catch an expected error,
re-raise or call pytest.fail(...) for any other exception; locate the loop using
brain.emit and the contextlib.suppress wrapper and change it accordingly.
- Around line 14-24: The helper _make_brain_with_corrections currently mutates
os.environ["BRAIN_DIR"] directly; change it to accept a monkeypatch fixture and
call monkeypatch.setenv("BRAIN_DIR", str(tmp_path / "brain")) instead of
assigning os.environ, then keep the importlib.reload(_p) and Brain.init(d,
domain="test", interactive=False) calls as-is so the refreshed _paths module
picks up the temp path; mirror the signature and usage pattern used in
tests/test_cli_seed.py so BRAIN_DIR is isolated per-test and cannot leak to
other tests.
---
Outside diff comments:
In `@Gradata/src/gradata/cloud/client.py`:
- Around line 296-305: The try/except around coercing session_raw to session_val
(handling session_raw, session_val) currently only catches ValueError and
TypeError but can raise OverflowError when json.loads yields Infinity/-Infinity
and you call int() (see the block converting floats/others to int); update the
exception handler for that conversion (the try block that sets session_val from
session_raw) to also catch OverflowError so those cases fall back to session_val
= None.
In `@Gradata/src/gradata/hooks/inject_brain_rules.py`:
- Around line 803-805: The early return that returns bp_text from
inject_brain_rules.py bypasses the per-brain cap (max_prompt_chars) computed
earlier; update the logic so bp_text is truncated to the computed
max_prompt_chars before returning (either by passing the cap into
_read_brain_prompt or by slicing bp_text after call), ensuring the final
returned value respects the brain-config.json max_recall_tokens instead of only
the global MAX_BRAIN_PROMPT_CHARS.
In `@Gradata/src/gradata/rules/rule_engine/_engine.py`:
- Around line 450-499: The tree branch bypasses the flat-engine eligibility
checks (TTL demotion and domain-disable) so candidates from
RuleTree.get_rules_for_context can include stale or domain-disabled RULE-tier
lessons; to fix, run the candidate list through the same eligibility/scoping
pipeline used by apply_rules() before computing relevance and sorting (i.e.,
reuse or extract the flat pipeline logic that applies TTL demotion,
domain-disable and other scope checks rather than ranking raw candidates),
applying that filter to candidates (after retrieval and before building scored
or before scored.sort) so RuleTree results and apply_rules() enforce identical
policy; reference apply_rules(), RuleTree.get_rules_for_context,
compute_scope_weight, lesson_scope, and the sorting code
(_STATE_PRIORITY/_difficulty_from_lesson/_effective_conf) to locate where to
plug the shared filter.
---
Duplicate comments:
In `@Gradata/src/gradata/_validator.py`:
- Around line 239-250: When parsing timestamps fails in the training span
calculation, do not skip adding a result: inside the except (TypeError,
ValueError) handler for the training_span_days calculation append a failing
entry to results (e.g., {"check": "training_span_days", "value": None, "pass":
False, "note": "malformed timestamps — could not compute training span"}) and
keep the existing logger.warning(..., exc_info=True); this ensures
TRAINING_DEPTH.total reflects the failure and prevents overstating trust scores.
Reference the results list, the "training_span_days" check name, and the
existing logger call in _validator.py.
In `@Gradata/src/gradata/cloud/_sync_state.py`:
- Around line 48-60: The migration loop that adds missing columns (using
existing_cols and conn.execute with "ALTER TABLE sync_state ADD COLUMN ...")
currently logs sqlite3.OperationalError and swallows it; change the except block
to log the warning (as it does) and then re-raise the caught exception (e.g.,
except sqlite3.OperationalError as e: _log.warning(..., exc_info=True); raise)
so failed ALTER TABLE operations surface and do not leave _SCHEMA_ENSURED
masking an incomplete schema.
In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py`:
- Around line 553-579: The tier scoping is wrong: replace the loose check
is_pipeline = tier.lower() != "cold" with an explicit check for the pipeline
tier (e.g., is_pipeline = tier and tier.lower() == "pipeline") so updates only
target pipeline rows, and when adding new rows use the actual tier argument
instead of the hardcoded "Pipeline" (update the new_row construction to insert
tier or a normalized tier_label variable). Ensure you reference the same
variables used in this block (tier, is_pipeline, data, header_line,
last_table_line) so updated rows and inserted rows consistently reflect the
provided tier.
🪄 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: 532336b6-79e1-4e41-8103-ee1634265d28
📒 Files selected for processing (40)
Gradata/src/gradata/_atomic.pyGradata/src/gradata/_config_paths.pyGradata/src/gradata/_migrations/tenant_uuid.pyGradata/src/gradata/_query.pyGradata/src/gradata/_validator.pyGradata/src/gradata/audit.pyGradata/src/gradata/brain.pyGradata/src/gradata/cli.pyGradata/src/gradata/cloud/_credentials.pyGradata/src/gradata/cloud/_sync_state.pyGradata/src/gradata/cloud/client.pyGradata/src/gradata/correction_detector.pyGradata/src/gradata/enhancements/edit_classifier.pyGradata/src/gradata/enhancements/metrics.pyGradata/src/gradata/enhancements/rule_verifier.pyGradata/src/gradata/enhancements/scoring/loop_intelligence.pyGradata/src/gradata/hooks/_generated_runner_core.pyGradata/src/gradata/hooks/_installer.pyGradata/src/gradata/hooks/adapters/_base.pyGradata/src/gradata/hooks/adapters/cursor.pyGradata/src/gradata/hooks/adapters/hermes.pyGradata/src/gradata/hooks/dispatch_post.pyGradata/src/gradata/hooks/inject_brain_rules.pyGradata/src/gradata/hooks/pre_compact.pyGradata/src/gradata/mcp_server.pyGradata/src/gradata/notifications.pyGradata/src/gradata/onboard.pyGradata/src/gradata/rules/rule_engine/_engine.pyGradata/src/gradata/rules/rule_engine/_formatting.pyGradata/tests/test_batch_approval.pyGradata/tests/test_cli_seed.pyGradata/tests/test_cloud_events_push.pyGradata/tests/test_convergence.pyGradata/tests/test_convergence_gate.pyGradata/tests/test_coverage_gaps.pyGradata/tests/test_dualwrite_atomicity.pyGradata/tests/test_edit_distance_convergence.pyGradata/tests/test_inspection.pyGradata/tests/test_loop_intelligence.pyGradata/tests/test_mcp_recall_token_budget.py
📜 Review details
⏰ 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). (6)
- GitHub Check: pytest ubuntu-latest / py3.11
- GitHub Check: pytest macos-latest / py3.12
- GitHub Check: pytest ubuntu-latest / py3.12
- GitHub Check: pytest windows-latest / py3.11
- GitHub Check: pytest windows-latest / py3.12
- GitHub Check: pytest macos-latest / py3.11
🧰 Additional context used
📓 Path-based instructions (2)
Gradata/tests/**/*.py
📄 CodeRabbit inference engine (Gradata/AGENTS.md)
Gradata/tests/**/*.py: SetBRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests
Add unit tests intests/test_*.pyfor every CI push without LLM calls (deterministic); mark integration tests with@pytest.mark.integrationand skip them by default (they hit real LLM APIs)
Files:
Gradata/tests/test_convergence_gate.pyGradata/tests/test_cli_seed.pyGradata/tests/test_batch_approval.pyGradata/tests/test_cloud_events_push.pyGradata/tests/test_edit_distance_convergence.pyGradata/tests/test_dualwrite_atomicity.pyGradata/tests/test_inspection.pyGradata/tests/test_convergence.pyGradata/tests/test_coverage_gaps.py
Gradata/src/**/*.py
📄 CodeRabbit inference engine (Gradata/AGENTS.md)
Gradata/src/**/*.py: Prefersentence-transformersfor local embeddings,google-genaifor Gemini embeddings,cryptographyfor AES-GCM encrypted system.db,bm25sfor BM25 rule ranking, andmem0aifor external memory adapters — guard all optional dependency imports withtry / except ImportErrorat the call site, never at module level
Maintain strict layering: Layer 0 (Primitives: _types.py, _db.py, _events.py, _paths.py, _file_lock.py; Patterns: contrib/patterns/) must never import from Layer 1 (Enhancements: enhancements/, rules/) or Layer 2 (Public API: brain.py, cli.py, daemon.py, mcp_server.py)
Never use bareexcept: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product
Never import from out-of-scope sibling directories../Sprites/or../Hausgem/withingradata/*code — that is a layering bug
Never leak private-sibling paths into public docs/code — no references to../Sprites/,../Hausgem/, email addresses, OneDrive paths, or Sprites-specific examples from insidegradata/*
Use atomic-write helper when writing JSON files to prevent corruption from mid-write crashes
Files:
Gradata/src/gradata/hooks/adapters/cursor.pyGradata/src/gradata/rules/rule_engine/_formatting.pyGradata/src/gradata/hooks/dispatch_post.pyGradata/src/gradata/notifications.pyGradata/src/gradata/hooks/adapters/hermes.pyGradata/src/gradata/cloud/_credentials.pyGradata/src/gradata/_atomic.pyGradata/src/gradata/mcp_server.pyGradata/src/gradata/_config_paths.pyGradata/src/gradata/hooks/_installer.pyGradata/src/gradata/brain.pyGradata/src/gradata/hooks/pre_compact.pyGradata/src/gradata/_query.pyGradata/src/gradata/hooks/_generated_runner_core.pyGradata/src/gradata/_migrations/tenant_uuid.pyGradata/src/gradata/enhancements/scoring/loop_intelligence.pyGradata/src/gradata/hooks/adapters/_base.pyGradata/src/gradata/enhancements/rule_verifier.pyGradata/src/gradata/hooks/inject_brain_rules.pyGradata/src/gradata/cli.pyGradata/src/gradata/audit.pyGradata/src/gradata/cloud/_sync_state.pyGradata/src/gradata/correction_detector.pyGradata/src/gradata/enhancements/edit_classifier.pyGradata/src/gradata/cloud/client.pyGradata/src/gradata/_validator.pyGradata/src/gradata/enhancements/metrics.pyGradata/src/gradata/rules/rule_engine/_engine.pyGradata/src/gradata/onboard.py
🔇 Additional comments (30)
Gradata/tests/test_cloud_events_push.py (1)
208-209: Good fix: lint-safe test doubles without changing behavior.Using
_ = timeoutkeeps theurlopen-compatible signature and makes intentional non-use explicit across all mocks. Looks consistent and correct.Also applies to: 240-241, 258-259, 279-280, 302-303, 324-325, 348-349, 363-364, 386-387, 415-416, 485-486, 527-528
Gradata/src/gradata/hooks/dispatch_post.py (2)
51-52: Deferred import placement is solid for handler isolation.Loading each hook module at invocation time keeps one failing constituent from blocking others.
53-55: Exception suppression path is now observable (good change).Using
warningwithexc_info=Truepreserves continuation behavior while avoiding silent failures.As per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".Gradata/src/gradata/hooks/pre_compact.py (1)
42-48: Good fix: indented comments are now excluded fromlesson_count.This resolves the earlier counting bug by evaluating both emptiness and
#prefix on the stripped line.Gradata/src/gradata/notifications.py (1)
112-117: Good fix: notification handler failures are now visible at warning level.Switching to
_log.warning(..., exc_info=True)in the exception path is the right behavior for this failure mode.Gradata/src/gradata/rules/rule_engine/_formatting.py (3)
11-21: Good module-level logger setup.This is a clean way to enable structured error reporting in this module.
425-425: Slice update is behavior-preserving and clearer.No functional regression here; this reads cleanly.
511-512: Nice fix for silent failure handling.Line 511-Line 512 now preserve graceful fallback while surfacing full failure context in logs.
Gradata/src/gradata/enhancements/rule_verifier.py (5)
157-183: No action needed.This segment is a readability-only reformat of existing
RuleVerificationconstruction paths with no behavior change.
208-208: No action needed.Import/layout change only; no functional or layering concern introduced.
228-236: No action needed.Tuple formatting in
conn.execute(...)is non-functional and keeps existing insert semantics intact.
245-247: No action needed.This is formatting-only for the
passedaggregate query and does not alter stats behavior.
72-75: Good hardening of category normalization before verification.Line 74 now guarantees
should_verify(...)receives a string, which prevents category-related crashes in pre-filtering.Gradata/src/gradata/enhancements/edit_classifier.py (2)
45-69: Good fix on tone marker normalization.Lowercasing phrase markers now aligns with
old_lower/new_lowermatching, so these tone signals are actually detectable.
555-559: Nice defensive guard for missing LLM provider.Returning early when provider resolution yields
Noneavoids null-provider calls and keeps extraction failure-safe.Gradata/src/gradata/correction_detector.py (3)
42-120: Regex pattern-table refactor looks solid.The multiline tuple layout improves maintainability without changing matching behavior, and the added correction signals are coherent with the existing confidence model.
131-156: New structured correction types are wired correctly.The added keyword patterns (
tone,format,omission,approach,scope) align withStructuredCorrectionType, so classification remains type-safe.Also applies to: 202-210
189-195: Docs-domain matching improvement is correctly implemented.Including
docsanddocumentationin the docs-domain regex with\bboundaries closes a common classification gap cleanly.Gradata/src/gradata/hooks/_generated_runner_core.py (1)
58-65: LGTM — redundanttext=Truecorrectly removed.The removal of
text=Trueis appropriate sinceencoding="utf-8"already implies text mode per Python'ssubprocesssemantics.proc.stdoutremains a string, so the write tosys.stdoutat line 72 behaves identically to before.Gradata/src/gradata/enhancements/scoring/loop_intelligence.py (1)
185-210: Good hardening of event-emit failures.Line 208–209 now logs failures with stack traces instead of silently swallowing them, while keeping activity logging non-blocking.
Gradata/src/gradata/_config_paths.py (1)
38-41: Good hardening on absolute override enforcement.Line 39-41 correctly fail fast on non-absolute
GRADATA_CONFIG_DIRvalues and avoid ambiguous relative resolution.Gradata/src/gradata/_migrations/tenant_uuid.py (5)
15-24: LGTM!Imports are minimal and appropriate for this atomic file operation utility. No external dependencies or layer violations.
60-72: LGTM!The generic exception handler properly cleans up the temp file before re-raising, and the final read has appropriate fallback handling. This follows the coding guideline of avoiding silent failures.
75-80: LGTM!Clean read-only accessor with appropriate validation.
83-88: LGTM!Standard UUID validation with appropriate exception handling.
91-110: LGTM!CLI interface is straightforward with clear --peek mode distinction.
Gradata/src/gradata/enhancements/metrics.py (1)
130-131: Excellent improvement to exception handling.The change from silent exception handling to
logger.warningwithexc_info=Truedirectly addresses the coding guideline requirement and ensures that failures in metrics computation are logged with full stack traces for debugging. This prevents silent failures in the memory product.As per coding guidelines: "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product."Gradata/src/gradata/cloud/client.py (1)
203-212: Single-event 413 handling looks correct now.The skip-and-advance path at Line 205-212 prevents the previous infinite retry behavior and keeps sync progressing.
Gradata/tests/test_inspection.py (1)
325-330: Use the same isolatedBRAIN_DIRbootstrap in this test too.This is still a raw
Brain.init(...)on a tmp directory without resettingBRAIN_DIRand reloadinggradata._paths, so it can pick up cached paths from earlier tests even though the fixture above was fixed.As per coding guidelines,
Gradata/tests/**/*.py: “SetBRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests”.Gradata/src/gradata/mcp_server.py (1)
348-362: Good fix: omitted recall args now fall back toBrainConfig.Passing
Nonethrough for absentmax_tokensandrankerkeeps MCP behavior aligned with the rest of the recall surface instead of pinning it to hardcoded defaults.
…afety gates CRITICAL fixes: - hooks/adapters/codex.py: write VALID TOML (was producing unparseable config that broke users' codex on next launch — caught by Round 2 smoke-test polluting parent's real ~/.codex/config.toml) - rules/rule_engine/_engine.py: tree retrieval now runs through shared TTL demotion + state eligibility + domain disable + scope relevance gates BEFORE ranking. Was bypassing safety gates so ranker=hybrid could inject stale RULE-tier or domain-disabled rules. - hooks/inject_brain_rules.py: brain_prompt.md respects per-brain recall budget; truncation now block-aligned w/ partial-XML repair - cloud/client.py: catches OverflowError on JSON Infinity; rejects batch_size <= 0 before chunk loop Test isolation hardening: - tests/conftest.py: autouse fixture isolates HOME/USERPROFILE/ XDG_CONFIG_HOME for adapter tests (prevents pollution of real user configs) - _base.adapter_config_path() resolves through env-aware helper - adapter commands use sys.executable (not bare 'python') Other Round 2 fixes: - _validator.py: malformed timestamps now fail training_span_days dimension instead of silent skip - _query.py / cloud/_sync_state.py: re-raise non-duplicate-column ALTER TABLE failures - enhancements/scoring/loop_intelligence.py: tier scoping uses actual Tier column from header; inserted rows use requested tier - _config_paths.py: rejects empty + '.' config file names - brain.py: recall defaults load config from current Brain instance directory (not process global) - cli.py: brain root resolution uses explicit args > BRAIN_DIR > GRADATA_BRAIN > cwd - enhancements/metrics.py: SQLite connection via context manager Tests: 4176 passed / 4 skipped / 5 deselected (+2 new isolation tests) Lint: ruff check + format pass Types: pyright 0 errors, 27 existing optional-import warnings
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Gradata/src/gradata/brain.py (1)
944-952:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
self._rule_graphthrough both retrieval paths.
Brain.__init__eagerly createsself._rule_graph, butapply_brain_rules()never passes it toapply_rules_with_tree(..., rule_graph=...)or the flat fallbackapply_rules(..., graph=...). That means the new conflict suppression and co-occurrence tracking ingradata.rules.rule_engine._enginenever run through the main public API.Suggested patch
try: from gradata.rules.rule_engine import apply_rules_with_tree applied = apply_rules_with_tree( lessons, scope, max_rules=max_rules, ranker=ranker, event_bus=_bus, + rule_graph=self._rule_graph, ) except (ImportError, Exception): - applied = apply_rules(lessons, scope, max_rules=max_rules, bus=_bus) + applied = apply_rules( + lessons, + scope, + max_rules=max_rules, + bus=_bus, + graph=self._rule_graph, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/brain.py` around lines 944 - 952, The code path in apply_brain_rules calls apply_rules_with_tree(...) and falls back to apply_rules(...), but never passes the prebuilt self._rule_graph; update both calls to include the graph: call apply_rules_with_tree(..., rule_graph=self._rule_graph, ...) and in the except branch call apply_rules(..., graph=self._rule_graph, ...) so the new conflict suppression and co-occurrence tracking in gradata.rules.rule_engine._engine are used; modify the calls where apply_rules_with_tree and apply_rules are invoked in Brain.apply_brain_rules (refer to apply_rules_with_tree and apply_rules symbols).Gradata/src/gradata/hooks/inject_brain_rules.py (1)
614-645:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCluster replacements are computed, but the rendered prompt ignores them.
cluster_lines,cluster_injected_ids, and the post-mutex budgeting are all built above, butsynth_inputis rebuilt from rawscored[:max_rules]here. That reintroduces clustered leaf rules into<brain-rules>and drops the cluster summaries entirely, while.last_injection.json/lesson_applicationsstill describe the clustered selection.
♻️ Duplicate comments (2)
Gradata/src/gradata/brain.py (1)
983-993:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBacking up to the last
>still leaves nested tags unclosed.If truncation lands after an opening inner tag but before its closing tag,
rfind(">")preserves that opener and only appends</brain-rules>.format_rules_for_prompt()emits nested XML-like markup, so this can still cache malformed output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/brain.py` around lines 983 - 993, The current truncation logic slices by character and then naively appends "</brain-rules>", which can leave inner tags unclosed; update the truncation in the block that checks max_recall_tokens/result to validate and balance the XML-like tags (e.g., scanning result after slicing for tags produced by format_rules_for_prompt()) by using a simple stack-based tag parser to remove any unmatched trailing opening tags or trim back to the last fully balanced tag sequence before appending the closing "</brain-rules>" (referencing symbols: max_recall_tokens, result, "<brain-rules>", and "</brain-rules>"); ensure the code either strips incomplete trailing tags or rebuilds a balanced tail so the cached result is well-formed.Gradata/src/gradata/hooks/inject_brain_rules.py (1)
61-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis helper can still emit malformed XML-like blocks.
When a partial chunk contains an opening block tag plus some inner closed tags,
_truncate_prompt_blocks()trims to the last</...>it can find and returns that fragment without the enclosing block’s own closing tag. That reintroduces the same malformed-markup failure the previous block-boundary fix was trying to avoid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Gradata/src/gradata/hooks/inject_brain_rules.py` around lines 61 - 87, _truncate_prompt_blocks currently slices a partial that may include an opening tag whose matching closing tag lies beyond the cut, causing malformed XML-like output; adjust the truncation logic in _truncate_prompt_blocks so that when you compute partial you ensure it ends on a balanced element boundary: after creating partial (and checking close_start/close_end), detect any unmatched opening tags (an opening "<tag" without a corresponding "</tag>" later in partial) and if found, roll partial back to before that unmatched opening tag (or drop the partial entirely) so only fully-closed elements are appended to result_parts; update the partial-handling around close_start/close_end and the final append to result_parts to enforce this balanced-tag check (use the existing variables partial, close_start, close_end, result_parts).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Gradata/src/gradata/cloud/client.py`:
- Around line 297-306: The session parsing currently truncates non-integral
floats (e.g., 1.9 -> 1); update the logic in the try/except that computes
session_val from session_raw so that when isinstance(session_raw, float) you
only coerce it to int if session_raw.is_integer() is True, otherwise treat it
like a malformed value and set session_val = None; keep the existing handling
for None, bool, int and the final fallback int(str(session_raw)) for other
types, and preserve the existing except (ValueError, TypeError, OverflowError)
behavior around the conversion.
- Around line 273-291: The code silently swallows parsing/serialization errors
when normalizing event data (in the data_val JSON parse block and the json.dumps
for data_blob); change the bare excepts to catch specific exceptions (e.g.,
json.JSONDecodeError for json.loads and TypeError/ValueError for json.dumps) and
call the module logger (use the existing logger symbol) with logger.warning(...)
including exc_info=True before falling back to the {"_raw": ...} or str(...)
behavior; update the handlers around data_val, data_blob, and keep
existing_eid/event_id logic intact.
In `@Gradata/src/gradata/enhancements/metrics.py`:
- Line 165: The f-string in Gradata/src/gradata/enhancements/metrics.py
currently classifies blandness using the test "generic if blandness > 0.7 else
varied", which treats exactly 0.700 as "varied"; update that conditional on the
f-string (the expression referencing blandness in the Blandness score line) to
use an inclusive comparison (">= 0.7") so values equal to 0.70 are classified as
"generic".
In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py`:
- Around line 47-56: The _POSITIVE_OUTCOMES constant was extended to include
hyphenated variants but get_activity_stats still checks only underscore forms;
update get_activity_stats to reference the canonical _POSITIVE_OUTCOMES set (or
normalize outcome strings before checking) so hyphenated outcomes like
"meeting-booked", "deal-advanced", "positive-reply", and "demo-completed" are
counted; locate the check in get_activity_stats and either replace the
hard-coded list with a membership test against _POSITIVE_OUTCOMES or normalize
outcomes (e.g., replace "-" with "_" or vice versa) before membership testing.
In `@Gradata/src/gradata/rules/rule_engine/_engine.py`:
- Around line 219-240: The function _score_scoped_lessons uses a hardcoded
relevance cutoff of 0.4 which mismatches the engine's documented/default 0.3
threshold; change the cutoff in _score_scoped_lessons from 0.4 to 0.3 so lessons
with relevance in [0.3,0.4) are retained, keeping current logic that multiplies
compute_scope_weight(lesson_scope(lesson)) by _CT_BOOST and logs suppressed
items via log_suppression/_make_rule_id when _ctx is present; ensure no other
implicit thresholds remain in this helper so flat/tree retrieval paths behave
consistently with apply_rules and filter_by_scope.
---
Outside diff comments:
In `@Gradata/src/gradata/brain.py`:
- Around line 944-952: The code path in apply_brain_rules calls
apply_rules_with_tree(...) and falls back to apply_rules(...), but never passes
the prebuilt self._rule_graph; update both calls to include the graph: call
apply_rules_with_tree(..., rule_graph=self._rule_graph, ...) and in the except
branch call apply_rules(..., graph=self._rule_graph, ...) so the new conflict
suppression and co-occurrence tracking in gradata.rules.rule_engine._engine are
used; modify the calls where apply_rules_with_tree and apply_rules are invoked
in Brain.apply_brain_rules (refer to apply_rules_with_tree and apply_rules
symbols).
---
Duplicate comments:
In `@Gradata/src/gradata/brain.py`:
- Around line 983-993: The current truncation logic slices by character and then
naively appends "</brain-rules>", which can leave inner tags unclosed; update
the truncation in the block that checks max_recall_tokens/result to validate and
balance the XML-like tags (e.g., scanning result after slicing for tags produced
by format_rules_for_prompt()) by using a simple stack-based tag parser to remove
any unmatched trailing opening tags or trim back to the last fully balanced tag
sequence before appending the closing "</brain-rules>" (referencing symbols:
max_recall_tokens, result, "<brain-rules>", and "</brain-rules>"); ensure the
code either strips incomplete trailing tags or rebuilds a balanced tail so the
cached result is well-formed.
In `@Gradata/src/gradata/hooks/inject_brain_rules.py`:
- Around line 61-87: _truncate_prompt_blocks currently slices a partial that may
include an opening tag whose matching closing tag lies beyond the cut, causing
malformed XML-like output; adjust the truncation logic in
_truncate_prompt_blocks so that when you compute partial you ensure it ends on a
balanced element boundary: after creating partial (and checking
close_start/close_end), detect any unmatched opening tags (an opening "<tag"
without a corresponding "</tag>" later in partial) and if found, roll partial
back to before that unmatched opening tag (or drop the partial entirely) so only
fully-closed elements are appended to result_parts; update the partial-handling
around close_start/close_end and the final append to result_parts to enforce
this balanced-tag check (use the existing variables partial, close_start,
close_end, result_parts).
🪄 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: 643ebe71-b859-49d5-9147-0776550fe94e
📒 Files selected for processing (18)
Gradata/src/gradata/_config_paths.pyGradata/src/gradata/_query.pyGradata/src/gradata/_validator.pyGradata/src/gradata/brain.pyGradata/src/gradata/cli.pyGradata/src/gradata/cloud/_sync_state.pyGradata/src/gradata/cloud/client.pyGradata/src/gradata/enhancements/metrics.pyGradata/src/gradata/enhancements/scoring/loop_intelligence.pyGradata/src/gradata/hooks/adapters/_base.pyGradata/src/gradata/hooks/adapters/codex.pyGradata/src/gradata/hooks/inject_brain_rules.pyGradata/src/gradata/onboard.pyGradata/src/gradata/rules/rule_engine/_engine.pyGradata/tests/conftest.pyGradata/tests/test_cli_install_agent.pyGradata/tests/test_convergence.pyGradata/tests/test_hook_adapters.py
📜 Review details
⏰ 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). (6)
- GitHub Check: pytest macos-latest / py3.12
- GitHub Check: pytest ubuntu-latest / py3.12
- GitHub Check: pytest windows-latest / py3.11
- GitHub Check: pytest macos-latest / py3.11
- GitHub Check: pytest windows-latest / py3.12
- GitHub Check: pytest ubuntu-latest / py3.11
🧰 Additional context used
📓 Path-based instructions (2)
Gradata/src/**/*.py
📄 CodeRabbit inference engine (Gradata/AGENTS.md)
Gradata/src/**/*.py: Prefersentence-transformersfor local embeddings,google-genaifor Gemini embeddings,cryptographyfor AES-GCM encrypted system.db,bm25sfor BM25 rule ranking, andmem0aifor external memory adapters — guard all optional dependency imports withtry / except ImportErrorat the call site, never at module level
Maintain strict layering: Layer 0 (Primitives: _types.py, _db.py, _events.py, _paths.py, _file_lock.py; Patterns: contrib/patterns/) must never import from Layer 1 (Enhancements: enhancements/, rules/) or Layer 2 (Public API: brain.py, cli.py, daemon.py, mcp_server.py)
Never use bareexcept: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product
Never import from out-of-scope sibling directories../Sprites/or../Hausgem/withingradata/*code — that is a layering bug
Never leak private-sibling paths into public docs/code — no references to../Sprites/,../Hausgem/, email addresses, OneDrive paths, or Sprites-specific examples from insidegradata/*
Use atomic-write helper when writing JSON files to prevent corruption from mid-write crashes
Files:
Gradata/src/gradata/hooks/adapters/codex.pyGradata/src/gradata/cloud/_sync_state.pyGradata/src/gradata/_config_paths.pyGradata/src/gradata/brain.pyGradata/src/gradata/hooks/adapters/_base.pyGradata/src/gradata/onboard.pyGradata/src/gradata/hooks/inject_brain_rules.pyGradata/src/gradata/_validator.pyGradata/src/gradata/enhancements/scoring/loop_intelligence.pyGradata/src/gradata/enhancements/metrics.pyGradata/src/gradata/cloud/client.pyGradata/src/gradata/_query.pyGradata/src/gradata/rules/rule_engine/_engine.pyGradata/src/gradata/cli.py
Gradata/tests/**/*.py
📄 CodeRabbit inference engine (Gradata/AGENTS.md)
Gradata/tests/**/*.py: SetBRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests
Add unit tests intests/test_*.pyfor every CI push without LLM calls (deterministic); mark integration tests with@pytest.mark.integrationand skip them by default (they hit real LLM APIs)
Files:
Gradata/tests/test_hook_adapters.pyGradata/tests/test_cli_install_agent.pyGradata/tests/conftest.pyGradata/tests/test_convergence.py
🔇 Additional comments (21)
Gradata/src/gradata/_validator.py (1)
249-258: Nice hardening of timestamp-span validation error handling.Line 249-Line 258 now uses typed exception handling, logs with
exc_info=True, and records a failed check instead of silently suppressing the failure.Gradata/src/gradata/enhancements/metrics.py (4)
16-21: Good logging setup for module-level observability.Line 20 sets a module logger cleanly and supports structured warning paths used below.
95-119: Connection lifecycle is now safe under exceptions.Using
with sqlite3.connect(...) as connon Line 95 correctly eliminates the previous connection-leak risk while keeping query logic intact.
128-130: Exception handling no longer fails silently.Line 129 now logs with
exc_info=True, which is the right behavior for diagnosability in this path.As per coding guidelines, "Never use bare
except: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product".
149-150: No-data fast path is concise and clear.Line 150’s single-return branch is readable and preserves deterministic output for empty windows.
Gradata/src/gradata/cloud/_sync_state.py (1)
48-61: Schema migration hardening looks good.The changes properly address the previous review feedback:
- PRAGMA check for existing columns minimizes unnecessary ALTER attempts
- OperationalError is now logged with
exc_info=Trueand re-raised instead of silently suppressedThis aligns with the coding guideline requiring typed exceptions with logging instead of bare
except: pass.Gradata/src/gradata/_query.py (2)
51-58: Tenant migration error handling correctly implements the suggested fix.The code now properly distinguishes between:
- The benign "duplicate column" race condition (silently ignored)
- Real migration failures (logged with
exc_info=Trueand re-raised)This addresses the previous review feedback.
270-303: Improved readability of pattern matching.The refactor from inline patterns to explicit lists makes the memory type classification easier to maintain and understand. Logic is preserved.
Gradata/src/gradata/enhancements/scoring/loop_intelligence.py (2)
185-210: Good failure-isolation on event emission.The
try/exceptaround_emit_event_fn(...)withlogger.warning(..., exc_info=True)keeps activity logging resilient while preserving diagnostics.
553-562: Tier-aware row updates look correct.Using
row_tiermatching and writing{tier}in both update and insert paths fixes the prior relabeling risk and keeps mixed-tier tables consistent.Also applies to: 579-579
Gradata/src/gradata/cli.py (4)
143-151: LGTM!The
cmd_auditrefactor cleanly delegates to the newgradata._auditmodule with proper JSON/text output formatting.
283-309: LGTM!The
_cmd_install_agentfunction now correctly uses_resolve_brain_root(args)for consistent brain directory resolution, handles multiple agents with proper error reporting, and exits with code 1 on any failure.
312-331: LGTM!The
cmd_recallfunction now properly usesBrain._find_lessons_path()for canonical lessons discovery with a fallback tobrain_root / "lessons.md", addressing the previous hardcoded path concern.
606-617: LGTM!The
_resolve_brain_rootfunction now follows the same precedence as_get_brain: explicit args first (brain_dir,brain), thenBRAIN_DIRenv var, thenGRADATA_BRAIN, and finallyPath.cwd(). This ensures consistent brain targeting across all CLI commands.Gradata/tests/test_convergence.py (2)
16-45: LGTM!The
_make_brain_with_correctionshelper now correctly usesmonkeypatch.setenv("BRAIN_DIR", ...)withimportlib.reload(_p)for proper test isolation, and the blanket exception suppression has been removed. This aligns with the coding guidelines for test isolation.
51-161: LGTM!All test functions properly accept the
tmp_pathandmonkeypatchfixtures and delegate brain creation to the refactored helper. The per-category test now properly asserts theby_categorystructure.Gradata/tests/test_cli_install_agent.py (2)
9-22: LGTM!The
_run_clihelper properly isolates the subprocess environment by redirectingHOME,USERPROFILE, andXDG_CONFIG_HOMEtotmp_path, preventing any writes to the real user's config directories.
25-50: LGTM!Both tests effectively validate the
gradata install --agentCLI flow: one verifies single-agent config generation under the isolated home, and the other verifies--agent alldetection of pre-existing configs.Gradata/tests/test_hook_adapters.py (3)
32-43: LGTM!Good edge-case coverage for paths containing quotes. The test verifies that the Codex adapter produces valid TOML and preserves the exact brain path in the hook command.
46-56: LGTM!This safety regression test correctly captures the real user's config state before the conftest fixture redirects HOME, then verifies the real file remains unchanged after adapter installation writes to the isolated location.
14-29: The conftest fixture correctly patches HOME before adapter_config_path is evaluated.The autouse fixture
_isolate_user_configs_for_adapter_tests(lines 118–129 in conftest.py) patches HOME, USERPROFILE, and XDG_CONFIG_HOME to tmp_path before the test runs. Since autouse fixtures execute before the test function body, these environment variables are set whenadapter_config_path(agent)is called on line 20, ensuring proper isolation from the real user's config directory.
| if isinstance(data_val, str): | ||
| try: | ||
| _parsed = json.loads(data_val) | ||
| data_val = _parsed if isinstance(_parsed, dict) else {"_raw": data_val} | ||
| except Exception: | ||
| data_val = {"_raw": data_val} | ||
| elif not isinstance(data_val, dict): | ||
| data_val = {"_raw": str(data_val)} | ||
| # Prefer existing event_id from local jsonl (ULID, globally unique) if | ||
| # present. Else derive from (ts,type,source,data,session) so events | ||
| # that share a timestamp + type don't collide and silently overwrite. | ||
| existing_eid = ev.get("event_id") | ||
| if isinstance(existing_eid, str) and existing_eid: | ||
| event_id = existing_eid[:64] | ||
| else: | ||
| try: | ||
| data_blob = json.dumps(ev.get("data", {}), sort_keys=True, default=str) | ||
| except Exception: | ||
| data_blob = str(ev.get("data", "")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Narrow these silent catch-alls in event normalization.
Line 277 and Line 290 currently swallow unexpected parse/serialization failures with no signal. That makes malformed event payloads hard to diagnose in a sync path; catch the expected exception types and log with exc_info=True before falling back.
Proposed fix
if isinstance(data_val, str):
try:
_parsed = json.loads(data_val)
data_val = _parsed if isinstance(_parsed, dict) else {"_raw": data_val}
- except Exception:
+ except json.JSONDecodeError:
+ logger.warning("Sync: legacy event data is not valid JSON", exc_info=True)
data_val = {"_raw": data_val}
elif not isinstance(data_val, dict):
data_val = {"_raw": str(data_val)}
@@
else:
try:
data_blob = json.dumps(ev.get("data", {}), sort_keys=True, default=str)
- except Exception:
+ except (TypeError, ValueError):
+ logger.warning(
+ "Sync: could not serialize event data for event_id derivation",
+ exc_info=True,
+ )
data_blob = str(ev.get("data", ""))As per coding guidelines, "Never use bare except: pass — use typed exceptions or at minimum logger.warning(...) with exc_info=True to avoid silent failure in a memory product".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/cloud/client.py` around lines 273 - 291, The code
silently swallows parsing/serialization errors when normalizing event data (in
the data_val JSON parse block and the json.dumps for data_blob); change the bare
excepts to catch specific exceptions (e.g., json.JSONDecodeError for json.loads
and TypeError/ValueError for json.dumps) and call the module logger (use the
existing logger symbol) with logger.warning(...) including exc_info=True before
falling back to the {"_raw": ...} or str(...) behavior; update the handlers
around data_val, data_blob, and keep existing_eid/event_id logic intact.
| try: | ||
| if session_raw is None: | ||
| session_val = None | ||
| elif isinstance(session_raw, bool): | ||
| if session_raw is None or isinstance(session_raw, bool): | ||
| session_val = None | ||
| elif isinstance(session_raw, int): | ||
| session_val = session_raw | ||
| elif isinstance(session_raw, float): | ||
| session_val = int(session_raw) | ||
| else: | ||
| session_val = int(str(session_raw)) | ||
| except (ValueError, TypeError): | ||
| except (ValueError, TypeError, OverflowError): |
There was a problem hiding this comment.
Reject non-integral float session values instead of truncating them.
Line 302-303 turns 1.9 into 1. That silently reassigns malformed events to the wrong session; only integral floats should be coerced, otherwise fall back to None.
Proposed fix
elif isinstance(session_raw, float):
- session_val = int(session_raw)
+ session_val = int(session_raw) if session_raw.is_integer() else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/cloud/client.py` around lines 297 - 306, The session
parsing currently truncates non-integral floats (e.g., 1.9 -> 1); update the
logic in the try/except that computes session_val from session_raw so that when
isinstance(session_raw, float) you only coerce it to int if
session_raw.is_integer() is True, otherwise treat it like a malformed value and
set session_val = None; keep the existing handling for None, bool, int and the
final fallback int(str(session_raw)) for other types, and preserve the existing
except (ValueError, TypeError, OverflowError) behavior around the conversion.
| f" Rule misfire rate: {float(get('rule_misfire_rate', 0.0)):.1%}", | ||
| f" Blandness score: {blandness:.3f} " | ||
| f"({'generic' if blandness > 0.7 else 'varied'})", | ||
| f" Blandness score: {blandness:.3f} ({'generic' if blandness > 0.7 else 'varied'})", |
There was a problem hiding this comment.
Use inclusive threshold for 0.70 blandness boundary.
Line 165 uses > 0.7, which classifies exactly 0.700 as varied. The file’s own threshold text states 0.70, so this should be inclusive.
🔧 Proposed fix
- f" Blandness score: {blandness:.3f} ({'generic' if blandness > 0.7 else 'varied'})",
+ f" Blandness score: {blandness:.3f} ({'generic' if blandness >= 0.7 else 'varied'})",📝 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" Blandness score: {blandness:.3f} ({'generic' if blandness > 0.7 else 'varied'})", | |
| f" Blandness score: {blandness:.3f} ({'generic' if blandness >= 0.7 else 'varied'})", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/enhancements/metrics.py` at line 165, The f-string in
Gradata/src/gradata/enhancements/metrics.py currently classifies blandness using
the test "generic if blandness > 0.7 else varied", which treats exactly 0.700 as
"varied"; update that conditional on the f-string (the expression referencing
blandness in the Blandness score line) to use an inclusive comparison (">= 0.7")
so values equal to 0.70 are classified as "generic".
| _POSITIVE_OUTCOMES: set[str] = { | ||
| "reply", "positive-reply", "meeting-booked", "demo-completed", | ||
| "deal-advanced", "meeting_booked", "deal_advanced", "closed", | ||
| "reply", | ||
| "positive-reply", | ||
| "meeting-booked", | ||
| "demo-completed", | ||
| "deal-advanced", | ||
| "meeting_booked", | ||
| "deal_advanced", | ||
| "closed", | ||
| } |
There was a problem hiding this comment.
Positive-outcome taxonomy now diverges from stats query.
You expanded _POSITIVE_OUTCOMES to include hyphenated variants, but get_activity_stats (Line 390) still hard-codes underscore-only values. This will undercount positives for outcomes like meeting-booked, deal-advanced, positive-reply, and demo-completed.
Suggested fix
- for r in conn.execute(
- """SELECT prep_level, COUNT(*) as total,
- SUM(CASE WHEN outcome IN ('reply','meeting_booked','deal_advanced','closed') THEN 1 ELSE 0 END) as positive,
- AVG(days_to_outcome) as avg_days
- FROM prep_outcomes WHERE date >= ? GROUP BY prep_level""",
- (cutoff,),
- ).fetchall():
+ positive_outcomes = tuple(_POSITIVE_OUTCOMES)
+ placeholders = ",".join("?" for _ in positive_outcomes)
+ query = f"""SELECT prep_level, COUNT(*) as total,
+ SUM(CASE WHEN outcome IN ({placeholders}) THEN 1 ELSE 0 END) as positive,
+ AVG(days_to_outcome) as avg_days
+ FROM prep_outcomes WHERE date >= ? GROUP BY prep_level"""
+ for r in conn.execute(query, (*positive_outcomes, cutoff)).fetchall():🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/enhancements/scoring/loop_intelligence.py` around lines
47 - 56, The _POSITIVE_OUTCOMES constant was extended to include hyphenated
variants but get_activity_stats still checks only underscore forms; update
get_activity_stats to reference the canonical _POSITIVE_OUTCOMES set (or
normalize outcome strings before checking) so hyphenated outcomes like
"meeting-booked", "deal-advanced", "positive-reply", and "demo-completed" are
counted; locate the check in get_activity_stats and either replace the
hard-coded list with a membership test against _POSITIVE_OUTCOMES or normalize
outcomes (e.g., replace "-" with "_" or vice versa) before membership testing.
| def _score_scoped_lessons( | ||
| lessons: list[Lesson], | ||
| scope: RuleScope, | ||
| *, | ||
| _ctx=None, | ||
| ) -> list[tuple[Lesson, float]]: | ||
| scored: list[tuple[Lesson, float]] = [] | ||
| for lesson in lessons: | ||
| relevance = compute_scope_weight(lesson_scope(lesson), scope) | ||
| relevance *= _CT_BOOST.get(lesson.correction_type, 1.0) | ||
| if relevance >= 0.4: | ||
| scored.append((lesson, relevance)) | ||
| elif _ctx: | ||
| from gradata.rules.rule_tracker import log_suppression | ||
|
|
||
| log_suppression( | ||
| rule_id=_make_rule_id(lesson), | ||
| reason="relevance_threshold", | ||
| relevance=relevance, | ||
| ctx=_ctx, | ||
| ) | ||
| return scored |
There was a problem hiding this comment.
Use the same relevance cutoff as the rest of the engine.
_score_scoped_lessons() now drops anything below 0.4, but apply_rules() still documents a 0.3 cutoff and filter_by_scope() defaults to 0.3. Since both flat and tree retrieval now funnel through this helper, rules scoring in [0.3, 0.4) disappear from recall unexpectedly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Gradata/src/gradata/rules/rule_engine/_engine.py` around lines 219 - 240, The
function _score_scoped_lessons uses a hardcoded relevance cutoff of 0.4 which
mismatches the engine's documented/default 0.3 threshold; change the cutoff in
_score_scoped_lessons from 0.4 to 0.3 so lessons with relevance in [0.3,0.4) are
retained, keeping current logic that multiplies
compute_scope_weight(lesson_scope(lesson)) by _CT_BOOST and logs suppressed
items via log_suppression/_make_rule_id when _ctx is present; ensure no other
implicit thresholds remain in this helper so flat/tree retrieval paths behave
consistently with apply_rules and filter_by_scope.
1 participant
Summary
Lands the v0.7.0 sprint — the universal recall + adapter layer that makes Gradata "drop in to any agent" rather than "claude-code only".
What's in
gradata_recall(situation, max_tokens, ranker)MCP tool — token-budgeted retrieval with meta-rule reservation (always at least 1 META if any matches; never get crowded out by individual lessons)hooks/adapters/: claude-code, codex, gemini, cursor, hermes, opencode. Each idempotent (signature-detected), atomic-write, preserves user customizations.gradata install --agent X|allCLI command with auto-detect of installed agentsgradata audit/gradata audit --json— recall coverage report, flags decayed-still-injecting rules and non-injectable meta-rule sourcesBrainConfig.max_recall_tokens(default 2000) +ranker(hybrid|flat|tree_only) plumbed through 5 cap sitesgradata install --agent Xinstead of manual hook setupTest plan
pytest tests/ -x --timeout=60 -m "not integration": 4174 passed / 4 skipped / 5 deselectedruff check src/ tests/: passedruff format --check src/ tests/: passed (473 files clean)pyright src/: 0 errors, 27 warnings (all unresolved optional/extras imports — expected)Smoke test (manual, tmp HOME):
gradata install --agent claude-code/--agent codex/--agent hermes→ all wrote configs successfullygradata auditreported "Agents configured: 3"Layering check
No Layer 0 → 2 imports introduced. Adapters live in
hooks/(Layer 2 territory, fine).Risk
recall(query, max_rules=5)behavior preserved. Newgradata_recallis additive.--include-all-sourcesdebug flag. Easier rollback than deleting outright.gradata-cloud/cloud/dashboard/components/not adjacent on this branch; TODO comment left in_audit.pyfor follow-up PR.Commit shape
Two commits on the branch:
feat(v0.7.0): gradata_recall MCP tool + universal hook adapters + audit CLI— 26 files, +1331/-72 (the actual feature)chore(format): ruff repo-wide normalization— 286 files, formatter-only churn (required forruff format --checkto pass; no logic). Also dedupestest_correction_event_captures_draft_textwhich was duplicated in main.Reviewing commit 1 alone gives the full feature surface.
Decisions made autonomously (per AGENTS.md OODA godmode)
INJECTABLE_META_SOURCES: kept filter + warn log (not deleted) — safer rollback pathhybrid|flat|tree_onlypyproject.tomlignores added:N801,SIM117— historical patterns in tests, justified inline🤖 Built by [delegate→codex/gpt-5.5], reviewed and committed by Claude opus-4-7.