feat(hooks): kill switches, tacit signals, cloud URL fix, emit refactor#134
feat(hooks): kill switches, tacit signals, cloud URL fix, emit refactor#134
Conversation
…edup The hook-overlap audit removed the JS implicit-feedback hook as redundant with the SDK version, but verifier caught that the SDK SIGNAL_MAP was missing the GAP category: "what about", "you forgot/missed/skipped/ dropped/ignored", "did you check/verify/test/review". CHALLENGE_PATTERNS already catches "you didn't/missed/forgot/failed" but lost the "what about" and "did you check" variants. Adding GAP_PATTERNS restores strict parity with the removed JS hook. Tests: 48 implicit_feedback + hooks_intelligence tests pass. Co-Authored-By: Gradata <noreply@gradata.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary
WalkthroughCentralized hook event emission via Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook.main()
participant Emitter as emit_hook_event()
participant Resolver as resolve_brain_dir()
participant BrainCtx as BrainContext.from_brain_dir()
participant Events as gradata._events.emit()
Hook->>Emitter: prepare event_type, source, data (may pass brain_dir)
Emitter->>Resolver: request brain_dir if not provided
Resolver-->>Emitter: brain_dir or None
Emitter->>BrainCtx: build context from brain_dir
BrainCtx-->>Emitter: BrainContext
Emitter->>Events: emit(event_type, source, data, ctx)
Events-->>Emitter: emit result (success/failure)
Emitter-->>Hook: return bool success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gradata/src/gradata/hooks/implicit_feedback.py`:
- Around line 77-81: GAP_PATTERNS is currently overlapping with
CHALLENGE_PATTERNS causing double-labeling in _detect_signals(); make these
markers exclusive by removing the "forgot|missed" alternatives from GAP_PATTERNS
(i.e., delete the `you (forgot|missed|skipped|dropped|ignored)` variant or at
least drop `forgot|missed`) so that ownership remains with CHALLENGE_PATTERNS
and _detect_signals() will not emit both `gap` and `challenge` for the same
phrase.
🪄 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: 938caa71-e575-4294-ae99-78b49a91fea4
📒 Files selected for processing (1)
Gradata/src/gradata/hooks/implicit_feedback.py
📜 Review details
🔇 Additional comments (1)
Gradata/src/gradata/hooks/implicit_feedback.py (1)
88-88: Good restoration ofgapregistration in signal detection.Line 88 correctly wires
GAP_PATTERNSintoSIGNAL_MAP, so the hook can emitgapwhen matched.
Users rarely type "looks good" — they just send the next task.
brain.correct() logs every CORRECTION but explicit approval words
fire 20x less, making the correction ratio look broken (2289% over
last 14 days). Emit a tacit OUTPUT_ACCEPTED when a substantive
follow-up prompt (>=30 chars) has no negation / challenge /
reminder / gap signals.
- Adds `mode: "explicit" | "tacit"` to the event payload so the
audit script can distinguish signal strength.
- Short acks ("ok", "go", "thanks") stay below the threshold —
they are too ambiguous to infer prior-turn acceptance.
Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gradata/src/gradata/hooks/implicit_feedback.py`:
- Around line 91-98: The new tacit-acceptance behavior (constants
_NEGATIVE_SIGNAL_TYPES and _TACIT_MIN_LENGTH and the code paths that emit
OUTPUT_ACCEPTED around where those constants are referenced) is outside the
stated parity goal; either remove this tacit-acceptance logic entirely (delete
the _NEGATIVE_SIGNAL_TYPES/_TACIT_MIN_LENGTH constants and the code that checks
message length/negative signals and emits OUTPUT_ACCEPTED) or gate it behind an
explicit config/feature flag (e.g., add a boolean allow_tacit_acceptance option
read where the emission occurs and skip the check when false) and then update
the PR description to explicitly document the behavioral change and rationale so
reviewers know this is intentional.
🪄 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: bfd3fd42-b016-4d6c-845a-2cebee3eb54d
📒 Files selected for processing (1)
Gradata/src/gradata/hooks/implicit_feedback.py
📜 Review details
🔇 Additional comments (5)
Gradata/src/gradata/hooks/implicit_feedback.py (5)
75-81: Double-labeling issue persists:forgot|missedappears in bothCHALLENGE_PATTERNSandGAP_PATTERNS.Line 79's
forgot|missedoverlaps with Line 62'sCHALLENGE_PATTERNS. A phrase like "you forgot" will emit bothchallengeandgapsignals, which may skew signal counts and classification semantics.Consider removing
forgot|missedfrom one of the pattern lists to make them mutually exclusive.
128-137: Logic is correct for the intended behavior.The signal detection and flag computation is well-structured. The early return condition properly gates on both explicit signals and tacit acceptance.
147-178: Event emission structure is sound.The
mode: "explicit" | "tacit"field is a useful addition for audit scripts. The conditional emission ofIMPLICIT_FEEDBACKonly when signals exist, and the separation of approval vs. tacit acceptance paths, are correctly implemented.Based on context snippet 3, downstream consumers like
session_closeonly check event type existence, so the newmodefield won't break compatibility.
182-185: Return behavior is consistent with test expectations.Returning
Nonefor tacit acceptance (no explicit signals) aligns with context snippet 4 showing tests expectNonefor neutral messages. The emittedOUTPUT_ACCEPTEDevent still triggers downstream processing while the hook's return value correctly indicates no explicit feedback was detected.
83-89: LGTM!Adding
"gap": GAP_PATTERNStoSIGNAL_MAPcorrectly wires the restored signal category into the detection pipeline, achieving parity with the brain-level implementation shown in context snippet 1.
session_close previously flagged rules as REJECTED when a corrections'
30-char prefix happened to match any substring of the lesson
description. "never hardcode secrets" and "never hardcode port numbers"
collided on "never hardcode " and quietly poisoned the graduation
pipeline. Require the shorter side to be ≥40 chars and to be a full
substring match (either direction) before rejecting.
Also remove a dead `payload.get("tool_output", "")` expression in
claude_code.py whose return value was never captured.
Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Cloud API now mounts routes under /api/v1 (gradata-cloud@04a272f). SDK was posting to /v1/telemetry/metrics — 404s. Rebases the default base to https://api.gradata.ai/api/v1 and trims the /v1 prefix from the per-call paths. Also applies ruff formatting to the security regression tests (no behavior change). Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Gradata/src/gradata/hooks/claude_code.py (1)
53-60:⚠️ Potential issue | 🟠 MajorAdd payload shape guards to keep hook truly non-blocking.
payloadandtool_inputare assumed to be dicts. If stdin contains valid JSON with another shape,.get(...)can raise and break hook execution.🛠️ Proposed fix
- tool_name = payload.get("tool_name", "") + if not isinstance(payload, dict): + return + tool_name = payload.get("tool_name", "") if tool_name not in ("Edit", "Write"): return # Extract draft (old content) and final (new content) from tool input tool_input = payload.get("tool_input", {}) + if not isinstance(tool_input, dict): + return old_string = tool_input.get("old_string", "") new_string = tool_input.get("new_string", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/hooks/claude_code.py` around lines 53 - 60, The hook assumes payload and tool_input are dicts and uses payload.get(...) which can raise if payload isn't a mapping; update the start of the hook to defensively validate shapes: safely read tool_name by checking isinstance(payload, dict) and default to "" if not, return early if tool_name not in ("Edit", "Write"); likewise ensure tool_input = payload.get("tool_input", {}) only after confirming payload is a dict, and coerce tool_input to {} when not a dict before extracting old_string and new_string (use isinstance checks and default "" values) so the hook never raises on malformed stdin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gradata/src/gradata/hooks/session_close.py`:
- Around line 306-318: The current loop compares normalized_desc and
normalized_lesson but only skips when normalized_desc is shorter than 40,
allowing false positives when lesson_desc is the short string; change the
condition to enforce the 40-character minimum on the shorter of the two strings
(i.e., before matching, check that min(len(normalized_desc),
len(normalized_lesson)) >= 40 and continue otherwise) so that matches only
proceed when the shorter normalized string meets the 40-char floor; update the
loop that iterates rejecting_descriptions and the comparisons using
normalized_lesson and normalized_desc accordingly.
---
Outside diff comments:
In `@Gradata/src/gradata/hooks/claude_code.py`:
- Around line 53-60: The hook assumes payload and tool_input are dicts and uses
payload.get(...) which can raise if payload isn't a mapping; update the start of
the hook to defensively validate shapes: safely read tool_name by checking
isinstance(payload, dict) and default to "" if not, return early if tool_name
not in ("Edit", "Write"); likewise ensure tool_input = payload.get("tool_input",
{}) only after confirming payload is a dict, and coerce tool_input to {} when
not a dict before extracting old_string and new_string (use isinstance checks
and default "" values) so the hook never raises on malformed stdin.
🪄 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: 9fbe5fba-766c-4199-ad0c-ff43037ff21f
📒 Files selected for processing (2)
Gradata/src/gradata/hooks/claude_code.pyGradata/src/gradata/hooks/session_close.py
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
🔇 Additional comments (1)
Gradata/src/gradata/hooks/claude_code.py (1)
18-36: Clean wrapper functions with lazy local imports look good.These hook-management entry points remain simple and side-effect free, and the function-scoped imports keep startup overhead minimal.
… helper Five hooks were repeating the same resolve_brain_dir() → BrainContext.from_brain_dir() → emit() dance. Centralize into emit_hook_event() so new hooks don't re-learn the pattern and failures log uniformly. - _base.py: add emit_hook_event(event_type, source, data, brain_dir=None) - implicit_feedback, agent_graduation, tool_failure_emit, self_review: migrated - Net -13 lines, identical external behavior, all 90 hook tests pass Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Gradata/src/gradata/hooks/implicit_feedback.py (2)
91-98:⚠️ Potential issue | 🟠 MajorNeutral follow-ups are being promoted to
OUTPUT_ACCEPTED.Any 30+ character message without a negative keyword now emits
OUTPUT_ACCEPTED, so prompts like “can you explain why this works?” or a brand-new task are treated as acceptance of the previous output. DownstreamGradata/src/gradata/hooks/session_close.py:40-52treatsOUTPUT_ACCEPTEDas a trigger event, so this changes behavior beyond simple telemetry labeling.Also applies to: 128-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/hooks/implicit_feedback.py` around lines 91 - 98, The current tacit-acceptance heuristic (_NEGATIVE_SIGNAL_TYPES and _TACIT_MIN_LENGTH) treats any 30+ char follow-up as OUTPUT_ACCEPTED, causing neutral follow-ups to be promoted; update the acceptance logic to require an explicit affirmative signal or clear conversational continuity instead of length alone: modify the function that emits OUTPUT_ACCEPTED to (a) check for positive/affirmative keywords (e.g., "thanks", "works", "correct", "good") and (b) verify topical continuity (e.g., reply/quoted message or conversation context) before emitting OUTPUT_ACCEPTED, while keeping negative signal checks using _NEGATIVE_SIGNAL_TYPES and retaining _TACIT_MIN_LENGTH only as a secondary heuristic. Ensure these changes touch the same module/constants and the code path that sets OUTPUT_ACCEPTED so session_close handlers no longer treat neutral 30+ char messages as acceptance.
77-80:⚠️ Potential issue | 🟠 MajorMake
challengeandgapmutually exclusive.
_detect_signals()emits one match per category, not one category per message. SinceCHALLENGE_PATTERNSstill matchesyou forgot|missedon Line 62, inputs like “you forgot to test this” now emit bothchallengeandgap, which skews classification instead of just restoring the dropped bucket.✂️ One simple fix
- re.compile(r"\byou (didn'?t|didnt|missed|forgot|failed)\b", re.I), + re.compile(r"\byou (didn'?t|didnt|failed)\b", re.I),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/hooks/implicit_feedback.py` around lines 77 - 80, _detect_signals currently can emit both "challenge" and "gap" for the same message because CHALLENGE_PATTERNS still matches phrases like "you forgot"; update _detect_signals to make these categories mutually exclusive by giving GAP_PATTERNS precedence: when a message matches any GAP_PATTERNS, do not emit a "challenge" signal (or explicitly remove "challenge" if both were collected). Locate the _detect_signals function and adjust the matching logic so you check GAP_PATTERNS (and related GAP handling) before adding CHALLENGE_PATTERNS (or filter the final signals to drop "challenge" when "gap" is present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gradata/src/gradata/cloud/sync.py`:
- Line 32: The code moved "/api/v1" into _DEFAULT_API_BASE which breaks config
compatibility for persisted api_base values; add a compatibility shim that
normalizes legacy bases (e.g., in the cloud config loader or a helper used by
request builders) by detecting if the configured api_base does not include the
"/api/v1" prefix and, if missing, append "/api/v1" (handling existing trailing
slashes to avoid duplicate slashes) before composing request URLs; reference
_DEFAULT_API_BASE and the request-building sites (the functions that POST
telemetry/metrics and corpus/contribute) and ensure the normalization runs on
load or immediately before building each request URL.
---
Duplicate comments:
In `@Gradata/src/gradata/hooks/implicit_feedback.py`:
- Around line 91-98: The current tacit-acceptance heuristic
(_NEGATIVE_SIGNAL_TYPES and _TACIT_MIN_LENGTH) treats any 30+ char follow-up as
OUTPUT_ACCEPTED, causing neutral follow-ups to be promoted; update the
acceptance logic to require an explicit affirmative signal or clear
conversational continuity instead of length alone: modify the function that
emits OUTPUT_ACCEPTED to (a) check for positive/affirmative keywords (e.g.,
"thanks", "works", "correct", "good") and (b) verify topical continuity (e.g.,
reply/quoted message or conversation context) before emitting OUTPUT_ACCEPTED,
while keeping negative signal checks using _NEGATIVE_SIGNAL_TYPES and retaining
_TACIT_MIN_LENGTH only as a secondary heuristic. Ensure these changes touch the
same module/constants and the code path that sets OUTPUT_ACCEPTED so
session_close handlers no longer treat neutral 30+ char messages as acceptance.
- Around line 77-80: _detect_signals currently can emit both "challenge" and
"gap" for the same message because CHALLENGE_PATTERNS still matches phrases like
"you forgot"; update _detect_signals to make these categories mutually exclusive
by giving GAP_PATTERNS precedence: when a message matches any GAP_PATTERNS, do
not emit a "challenge" signal (or explicitly remove "challenge" if both were
collected). Locate the _detect_signals function and adjust the matching logic so
you check GAP_PATTERNS (and related GAP handling) before adding
CHALLENGE_PATTERNS (or filter the final signals to drop "challenge" when "gap"
is present).
🪄 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: a7b9fcb3-83ed-4569-8520-6e720dd34c16
📒 Files selected for processing (8)
Gradata/src/gradata/cloud/sync.pyGradata/src/gradata/hooks/_base.pyGradata/src/gradata/hooks/agent_graduation.pyGradata/src/gradata/hooks/implicit_feedback.pyGradata/src/gradata/hooks/self_review.pyGradata/src/gradata/hooks/tool_failure_emit.pyGradata/tests/test_cloud_sync.pyGradata/tests/test_security_regressions.py
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
Gradata
changed the title
…ation, off-by-one From autoresearch audits on patterns/, enhancements/, and top-level SDK: - rag.py: two-pass query expansion was unreachable (Stage 3 unconditionally returned before Stage 4). Moved expansion inside Stage 3 gate so cfg.two_pass actually takes effect when non-empty results exist. - parallel.py: DependencyGraph.run mutated task.input_data on the shared ParallelTask instance. Re-running the graph saw stale upstream outputs. Use dataclasses.replace to scope the resolved input to the current run. - guardrails.py: two dead expressions (.lower() and str()) whose results were discarded; removed. - _confidence.py: sessions_since_fire off-by-one — reset to 0 then immediately += 1 produced a systematic overcount for fired lessons. Track via flag and skip the increment on fire. Added defensive severity default for fragile ternary on CONTRADICTING path. - meta_rules.py:685: refresh_meta_rules mutated existing_metas in place despite contract; use dataclasses.replace so callers' references stay pristine. - brain.py:_resolve_pending: held a SQLite connection open across lessons_lock. Close before acquiring the file lock; re-open only for the final UPDATE. All 670 affected tests pass. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
#133 added opt-out env vars (GRADATA_SECRET_SCAN=0, _CONFIG_PROTECTION=0, _SESSION_PERSIST=0, etc.) that disable the corresponding hook. Dev shells often leave these set, which then flips 10 hook safety/intelligence tests from green to failing locally even though the code is correct. Session-scoped autouse fixture pops the seven kill-switches for the whole test session and restores them on teardown. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…wallowed exceptions - _events.py:_ensure_table: cache schema-initialized state per db_path so the 10+ CREATE/ALTER/INDEX DDL statements run once per process instead of on every emit() call. PRAGMAs still re-run per connection. - reflection.py: CritiqueChecklist duplicate-name scan was O(n²) via list.count in a loop; use Counter once. - reporting.py: three `except Exception: pass` blocks in build_brain_briefing silently dropped rule/quality/correction extraction errors. Log at DEBUG so misconfigurations are diagnosable without changing the silent-return contract. All 180/167 affected tests pass. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
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 (1)
Gradata/src/gradata/enhancements/self_improvement/_confidence.py (1)
788-912:⚠️ Potential issue | 🟠 MajorMark reinforcing corrections as fires too.
When the reinforcing branch runs on Lines 788-809,
fire_countis incremented, but_fired_this_sessionis never set before Line 912. That meanssessions_since_firestill becomes1after a reinforcing fire, so the off-by-one fix only covers the survived+injected path.Proposed fix
if direction == "REINFORCING": # Reinforcing: correction aligns with lesson direction → BONUS lesson.alpha += 1.0 # Reset contradiction streak on reinforcement lesson._contradiction_streak = 0 @@ _bayes = max(0.0, min(1.0, _bayesian_confidence(lesson))) lesson.confidence = max(_bayes, _pre_bonus_conf) lesson.fire_count += 1 + lesson.sessions_since_fire = 0 + _fired_this_session = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/enhancements/self_improvement/_confidence.py` around lines 788 - 912, Reinforcing corrections currently increment lesson.fire_count but never set the _fired_this_session flag (or reset sessions_since_fire), causing an off-by-one—so after the reinforcing branch (where lesson.fire_count += 1) also set lesson.sessions_since_fire = 0 and set the module-scope/session-scope flag _fired_this_session = True; update the reinforcing branch that handles direction == "REINFORCING" (using the lesson local and _fired_this_session symbol) to mirror the survived+injected path's behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gradata/src/gradata/brain.py`:
- Around line 1195-1198: The SELECT-then-UPDATE flow around pending_approvals
(using approval_id) is racy because the UPDATE does not re-check resolution IS
NULL or verify that a row was actually changed; modify the UPDATE statement used
near where approval_id is handled to include "AND resolution IS NULL" (or
otherwise repeat the same predicate) and immediately check the executing
cursor/connection rowcount/changes to ensure one row was updated before
returning {"resolved": True,...}; ensure this change is applied to both the
update block around lines ~1231-1239 and the earlier block that relies on the
initial SELECT so the response only reports resolved when the UPDATE affected a
row (or use a single atomic UPDATE within a transaction if preferred).
In `@Gradata/src/gradata/contrib/patterns/guardrails.py`:
- Line 203: The current computation of block_reason builds a string from all
failing checks but GuardedResult's docstring requires explaining only the first
blocking failure; update the logic that sets block_reason (currently using
failing_input) to use only the first element (e.g., failing_input[0]) and format
it as "{name}: {details}" so block_reason describes the single primary failure
and keep the GuardedResult docstring unchanged.
In `@Gradata/src/gradata/contrib/patterns/rag.py`:
- Around line 399-412: The current pass-2 retrieval blocks around fts_fn and
vector_fn silently catch all exceptions and drop failure context; update the
try/except handling in the fts_fn and vector_fn calls (the blocks that populate
pass2_results and pass2_total using expanded_query and limit * 2) to surface
errors instead of swallowing them — either log the exception with context
(including which retriever failed, the expanded_query and limit) via the
module/process logger or attach the exception details to the response/mode
metadata, and ensure failures do not prevent returning diagnostic info; keep the
same call sites (fts_fn(...) and vector_fn(...)) but replace bare excepts with
explicit exception capture and propagation or structured logging so callers can
detect pass-2 retriever failures.
- Around line 389-423: Add regression tests that exercise the cfg.two_pass path
and assert the returned RetrievalResult includes the expanded query and two-pass
mode semantics: invoke the retrieval path so extract_expansion_terms returns
terms (you can stub/mock extract_expansion_terms or the fts_fn/vector_fn), then
assert RetrievalResult.query equals the expanded_query (original query + joined
expansion terms), RetrievalResult.mode includes "two_pass" and the "+N terms"
suffix, and RetrievalResult.total_candidates accounts for pass2_total; target
the existing test modules (tests/test_patterns.py and tests/test_bug_fixes.py)
near the two referenced areas and use the same retrieval entrypoint that
triggers rrf_merge/apply_graduation_scoring so the test covers the full two-pass
flow.
- Line 420: The code currently assigns expanded_query to the public
RetrievalResult.query field (via the RetrievalResult(...) call using
query=expanded_query); change this to preserve the original user query by
passing the original query variable (e.g. query or original_query) into
RetrievalResult.query and move expanded_query into a non-public/explicit field
(e.g. metadata["expanded_query"] or a new attribute like expansion or
expanded_query) so downstream consumers see the untouched user input in
RetrievalResult.query while still retaining the expansion data internally;
update the code that constructs RetrievalResult and any consumers expecting
expanded_query accordingly.
---
Outside diff comments:
In `@Gradata/src/gradata/enhancements/self_improvement/_confidence.py`:
- Around line 788-912: Reinforcing corrections currently increment
lesson.fire_count but never set the _fired_this_session flag (or reset
sessions_since_fire), causing an off-by-one—so after the reinforcing branch
(where lesson.fire_count += 1) also set lesson.sessions_since_fire = 0 and set
the module-scope/session-scope flag _fired_this_session = True; update the
reinforcing branch that handles direction == "REINFORCING" (using the lesson
local and _fired_this_session symbol) to mirror the survived+injected path's
behavior.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 7d505440-5f9c-468b-a3ab-18f1b2916c12
📒 Files selected for processing (6)
Gradata/src/gradata/brain.pyGradata/src/gradata/contrib/patterns/guardrails.pyGradata/src/gradata/contrib/patterns/parallel.pyGradata/src/gradata/contrib/patterns/rag.pyGradata/src/gradata/enhancements/meta_rules.pyGradata/src/gradata/enhancements/self_improvement/_confidence.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
📚 Learning: 2026-04-17T17:18:07.439Z
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
Applied to files:
Gradata/src/gradata/brain.py
🔇 Additional comments (8)
Gradata/src/gradata/enhancements/meta_rules.py (1)
682-689: Good fix: validated meta updates are now side-effect free.Line 688 correctly uses
replace(...)instead of mutatingmetain place, preventing hidden state changes for callers still holdingexisting_metas.Gradata/src/gradata/contrib/patterns/parallel.py (2)
37-37: Strong improvement: per-run task input is now isolated.Using
replace(task, input_data=resolved_input)avoids mutating the originalParallelTask, which prevents stale-input cross-run behavior.Also applies to: 315-329
173-174: Error-path/message cleanups look safe.These changes are non-functional string/format compactions; exception behavior and execution flow are preserved.
Also applies to: 195-195, 304-305, 311-312, 331-331, 385-386
Gradata/src/gradata/contrib/patterns/guardrails.py (3)
51-53: Formatting-only refactors look good.These edits are behavior-preserving and improve readability without changing runtime semantics.
Also applies to: 243-244, 325-330, 494-496, 556-558
456-467: Optional deny-list iteration hardening is solid.Using
global_deny or []andagent_tools_denied or []keeps the logic concise and safely handlesNone.
638-657: Dead-code cleanup inguards_from_graduated_rulesis a good improvement.Removing the no-op expressions and keeping the guard construction explicit improves maintainability with no functional regression.
Gradata/src/gradata/contrib/patterns/rag.py (1)
35-35: Formatting-only edits look good.No correctness or maintainability concerns in these ranges.
Also applies to: 49-49, 58-71, 133-143, 153-260, 442-445, 504-506, 524-526
Gradata/src/gradata/brain.py (1)
897-916: Backward-compatiblerules.injectedpayload expansion.Adding
taskhere looks safe: the provided listener inGradata/src/gradata/integrations/session_history.py:29-34only readspayload.get("rules", []), so this extra field should not break existing subscribers.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…, silent except, dead code CRITICAL - brain.py::review_pending: conn.close() was outside try/finally. If fetchall() or row materialization raised, the SQLite handle leaked. Switched to `with contextlib.closing(get_connection(...)) as conn:`. - _brain_manifest.py::generate: session-count cross-check did get_connection + conn.close() with close outside finally. Same fix. - _manifest_metrics.py::_quality_metrics: previously read lessons.md twice per call (once here, once inside `_lesson_distribution`). Read once, pass text through. HIGH - _manifest_helpers.py::_count_events, _get_tables: conn.close() only on happy path. Switched to contextlib.closing. - _manifest_metrics.py::_quality_metrics second conn block: same fix. - _manifest_metrics.py:221: dead list-comprehension whose result was immediately discarded — deleted. - brain.py::correct: telemetry `except Exception: pass` now debug-logs so failures are visible. - rules/rule_engine/_scoring.py::validate_assumptions: bare `except: pass` on scope_json parse now logs at debug level. Tests: 602 passed (brain + manifest + scoring + confidence scope). Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…st parse errors - brain.py::_search_events: term filter now runs in SQL (LOWER(data_json) LIKE) instead of fetching 500 rows and Python-filtering. Empty query returns [] early. - brain.py: delete dead `with contextlib.suppress(ImportError): pass` trailer. - cloud/client.py::_read_local_manifest: corrupt brain.manifest.json now logs at warning level before returning empty dict, instead of silently shipping empty payloads to the cloud. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…, scope_json helper - brain.py::_load_lessons: mtime-keyed parse cache so apply_brain_rules and other read-only callers reuse parsed lessons instead of re-parsing on every call. apply_brain_rules switched to the cached loader; enhancements import check moved to find_spec so pyright sees it as a capability probe. - _graduation.py: hoist Beta-LB env reads out of the per-lesson loop via a new _read_beta_lb_config() called once per graduate() invocation. - similarity.py: expose semantic_vector / similarity_from_vectors so callers comparing one probe against many stored strings precompute stored vectors once (O(N*M) tokenization -> O(N+M)). - _graduation.py dedup gate: precompute existing-rule vectors outside the candidate loop and use similarity_from_vectors. - _manifest_metrics.py: add _parse_ts_utc() so naive/aware timestamp mixes from SQLite coerce to aware UTC before subtraction. - _scoring.py::lesson_scope: shared scope_json parse helper; _engine.py and validate_assumptions now use it instead of inlining the try/json.loads pattern. Removes the unused logging import from _scoring.py. Full suite: 3908 passed, 2 skipped. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Removes 7 redundant `import json as _json_*` statements from hot paths (parse_lessons per-meta-line, format_lessons per-lesson). Python caches imports so the cost is modest, but the stuttered aliases obscure intent. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…ning index - brain.py: invalidate lessons + rule cache after patch_rule/forget/rollback/_resolve_pending writes; wrap _resolve_pending sqlite connections in contextlib.closing; cache self_improvement capability check in __init__; add logger.debug to silent excepts in session/manifest.proof - loop_detection.py: use Counter alongside deque for O(1) repeat detection (was O(window_size) per record) - q_learning_router.py: hoist hmac/logging/platform/time imports to module level; precompute agent_index dict for O(1) lookup (was O(N) list.index per update_reward) Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…epts - meta_rules.py: _resolve_principle_creds() hoists GRADATA_LLM_* env reads out of per-category loop; _try_llm_principle accepts precomputed creds - reporting.py: wrap health-report sqlite3 connection in contextlib.closing; replace two `except: pass` with logger.debug - router_warmstart.py: wrap warm-start sqlite connection in contextlib.closing (was leaked if exception in between connect/close) - contrib/enhancements/quality_gates.py: wrap success-report sqlite in contextlib.closing; replace `except: pass` with logger.debug - brain.py: lineage() now uses get_connection() (consistent with the rest of brain.py) instead of raw sqlite3.connect - test_agentic_synthesis.py: update mocks to accept new creds kwarg Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
… log swallowed excepts - rag.py: replace non-deterministic hash() with zlib.crc32 for RRF chunk IDs (PYTHONHASHSEED randomisation was silently breaking dedupe across processes/restarts) - rag.py: order_by_relevance_position no longer uses list.insert(0, ...) — was O(N^2) per call, now O(N) via head/tail split + reverse - rag.py: two-pass expansion + NaiveRAG.retrieve silent excepts now log at debug instead of masking misconfigured backends - tree_of_thoughts.py: precompute rule_word_sets once outside _default_scorer closure (was O(N*M) re-tokenisation per candidate x existing_rule) - rule_context_bridge.py: wrap WAL checkpoint conn in contextlib.closing; log the swallow - brain.py: hoist dataclasses import to module level (was inside health()) Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…stry - rule_graph.py: wrap add_rule_relationship and get_related_rules sqlite connections in contextlib.closing (were leaked on exception) - rule_tree.py: replace non-deterministic hash() with zlib.crc32 for lesson IDs written to persisted .md frontmatter (was changing across processes, breaking cross-run ID stability) - contrib/patterns/memory.py: use heapq.nlargest for retrieve() when limit < len(matches); full sort only when returning everything - contrib/patterns/orchestrator.py: mirror _REGISTERED_INTENT_PATTERNS into a dict index for O(1) classify_request lookup (was O(N) linear scan per classify call) Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Whitelist the user-prompt handoff-watchdog and session-start handoff-inject hooks (plus their dispatchers) so fresh clones keep context-pressure handling wired into Claude Code. Everything else under .claude/ remains ignored. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Refine the .gitignore watchdog carve-out — track only the two hook files themselves; leave dispatcher wiring machine-local. Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
- brain.py: _resolve_pending re-checks resolution IS NULL and verifies rowcount in the UPDATE; prevents lost-race overwrites returning resolved=true when another worker already resolved - rag.py: two-pass expanded retrieval returns the original user query (not expanded_query) so downstream telemetry/logging never surfaces mined corpus terms as user input - cloud/sync.py: _normalize_api_base() upgrades legacy https://api.gradata.ai bases (no /api/v1 segment) on load; older cloud-config.json files self-heal instead of silently POSTing to unversioned endpoints - hooks/session_close.py: enforce the 40-char floor on BOTH lesson_desc and rejecting desc; gating only one side let short lessons match long descriptions via prefix containment - hooks/implicit_feedback.py: drop forgot|missed from GAP_PATTERNS (already owned by CHALLENGE_PATTERNS); raise tacit threshold to 60 chars and skip messages that look like questions — "can you explain ..." no longer counts as tacit acceptance - guardrails.py: block_reason now reports only the first failing input check, aligning with the GuardedResult docstring contract Co-Authored-By: Gradata <noreply@gradata.ai>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
@coderabbitai review The GAP_PATTERNS / CHALLENGE_PATTERNS overlap flagged in your previous review was addressed in commit 7911ef4 (NOTE comment + gap variant now uses |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Gradata/src/gradata/contrib/patterns/guardrails.py (1)
640-652:⚠️ Potential issue | 🟠 MajorGraduated-rule guards are non-enforcing: generated checks always return
pass.Lines 640-652 build
check_fnthat never inspectsdataand always emitsresult="pass". This contradicts the function/docs stating graduated SECURITY/ACCURACY rules become guardrail checks, and it silently disables rule-based blocking/flagging.Proposed minimal heuristic fix
def _make_check(rule_text: str, rule_cat: str) -> Callable[[Any], GuardCheck]: """Create a check function that scans output for rule violations.""" + rule_text_l = rule_text.lower().strip() + neg_prefixes = ("never ", "do not ", "don't ") + forbidden = next( + (rule_text_l[len(p):].strip() for p in neg_prefixes if rule_text_l.startswith(p)), + "", + ) def check_fn(data: Any) -> GuardCheck: - # Simple keyword check — does the output violate the rule? - # Rules are phrased as "never X" or "always Y" - # This is a heuristic; real guardrails need LLM-backed checks + text = str(data).lower() + if forbidden and forbidden in text: + return GuardCheck( + name=f"rule_{rule_cat.lower()}", + result="fail", + details=f"Rule violated: {rule_text[:80]}", + action_taken="blocked", + ) return GuardCheck( name=f"rule_{rule_cat.lower()}", result="pass", details=f"Rule: {rule_text[:80]}", action_taken="passed", )Also applies to: 656-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/contrib/patterns/guardrails.py` around lines 640 - 652, The generated check function _make_check currently ignores its input and always returns a passing GuardCheck; change check_fn to actually scan the provided data (the output string or token stream) for the rule expressed in rule_text and set GuardCheck.result accordingly: for negative/forbidden rules (detect prefixes like "never", "do not", "don't", "no") treat any occurrence of the forbidden keyword/phrase in data as a violation and return result="fail" (with details indicating the matched substring and action_taken="blocked" or "flagged"); for positive/required rules (detect "always", "must", "ensure") treat absence of the required phrase as a violation and return result="fail" or "warn"; implement simple, case-insensitive substring and token-boundary checks using rule_text.lower() and data_text.lower(), and ensure the same fix is applied to the duplicate check factory used later so graduated rules actually enforce or flag failures instead of always passing.Gradata/src/gradata/contrib/patterns/memory.py (1)
132-147:⚠️ Potential issue | 🟡 MinorValidate
limitbefore ranking.
Line 143 currently allows negativelimitvalues to pass through silently. Reject invalid inputs explicitly to avoid hard-to-debug caller mistakes.💡 Proposed fix
def retrieve( self, query: str, types: list[str] | None = None, limit: int = 10, ) -> list[Memory]: """Return memories whose content contains ``query`` (case-insensitive).""" + if limit < 0: + raise ValueError("limit must be >= 0") + if limit == 0: + return [] query_lower = query.lower() allowed = set(types) if types is not None else VALID_TYPES now = _now_iso()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/contrib/patterns/memory.py` around lines 132 - 147, The method that builds matches and then branches on "if limit >= len(matches):" doesn't validate the incoming limit, allowing negative values; add an explicit validation at the start of this method (the function iterating self._data and computing matches, referencing variables limit and matches) to ensure limit is an int >= 0 and raise a ValueError (or TypeError for non-int) for invalid inputs; keep the existing ranking logic (matches.sort / heapq.nlargest) unchanged and only proceed after the validation.Gradata/src/gradata/contrib/patterns/orchestrator.py (1)
256-258:⚠️ Potential issue | 🟡 MinorThe
prependparameter no longer affects lookup precedence.The docstring states that
prepend=Truecauses the entry to "take precedence during lookup," but sinceclassify_requestnow uses O(1) dict lookup (line 375), list ordering is irrelevant. The dict stores exactly one entry per intent key, so there is no "precedence" to establish.Consider either:
- Removing the
prependparameter entirely (breaking change), or- Updating the docstring to clarify that
prependonly affects list ordering for iteration/inspection purposes, not lookup behavior.📝 Suggested docstring update
prepend: When ``True`` the new entry is inserted at position 0 so it - takes precedence during lookup. + appears first when iterating ``_REGISTERED_INTENT_PATTERNS``. + Note: this does not affect ``classify_request`` lookup, which + uses O(1) dict indexing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/contrib/patterns/orchestrator.py` around lines 256 - 258, The prepend parameter in the function signature (prepend: bool = False) no longer affects lookup precedence because classify_request now uses O(1) dict lookup; update the function's docstring (the method that accepts prepend) to state that prepend only affects list ordering for iteration/inspection and does not change lookup behavior, and mention classify_request as the reason; alternatively, if you prefer a breaking change, remove the prepend parameter and its uses (update callers and tests) — prefer the non-breaking docstring clarification unless you intentionally want to change the API.Gradata/src/gradata/_manifest_metrics.py (1)
328-357: 🛠️ Refactor suggestion | 🟠 MajorUse context-managed DB cleanup in the first
_quality_metricsquery block.This block can still bypass
conn.close()on mid-block exceptions. Align it with the laterclosing(get_connection(...))pattern to avoid silent connection leaks.♻️ Proposed refactor
- conn = get_connection(db) - recent_sessions = [ - r[0] - for r in conn.execute(""" + import contextlib as _ctxlib + with _ctxlib.closing(get_connection(db)) as conn: + recent_sessions = [ + r[0] + for r in conn.execute(""" SELECT session FROM events WHERE typeof(session)='integer' GROUP BY session HAVING COUNT(*) >= 2 ORDER BY session DESC LIMIT 10 - """).fetchall() - ] - if recent_sessions: - placeholders = ",".join("?" * len(recent_sessions)) - total_corrections = ( - conn.execute( - f"SELECT COUNT(*) FROM events WHERE type='CORRECTION' AND session IN ({placeholders})", - recent_sessions, - ).fetchone()[0] - or 0 - ) - total_outputs = ( - conn.execute( - f"SELECT COUNT(*) FROM events WHERE type='OUTPUT' AND session IN ({placeholders})", - recent_sessions, - ).fetchone()[0] - or 0 - ) - if total_outputs > 0: - result["correction_rate"] = round(total_corrections / total_outputs, 3) - conn.close() + """).fetchall() + ] + if recent_sessions: + placeholders = ",".join("?" * len(recent_sessions)) + total_corrections = ( + conn.execute( + f"SELECT COUNT(*) FROM events WHERE type='CORRECTION' AND session IN ({placeholders})", + recent_sessions, + ).fetchone()[0] + or 0 + ) + total_outputs = ( + conn.execute( + f"SELECT COUNT(*) FROM events WHERE type='OUTPUT' AND session IN ({placeholders})", + recent_sessions, + ).fetchone()[0] + or 0 + ) + if total_outputs > 0: + result["correction_rate"] = round(total_corrections / total_outputs, 3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/_manifest_metrics.py` around lines 328 - 357, The first query block in _quality_metrics currently uses conn = get_connection(db) and may skip conn.close() on exceptions; change it to use a context manager (e.g., with closing(get_connection(db)) as conn from contextlib) so the connection is always closed even on errors, keep the same logic that builds recent_sessions, placeholders, total_corrections, total_outputs and sets result["correction_rate"], and ensure you add the necessary import (contextlib.closing) and remove the explicit conn.close() call.Gradata/src/gradata/contrib/patterns/parallel.py (1)
173-195:⚠️ Potential issue | 🟠 MajorReject duplicate task IDs before building the graph.
task_map,_task_map, andresultsall key bytask.id, so duplicate IDs currently overwrite one task and can surface as a bogus cycle error instead of a clear validation failure. This should be rejected up front.🛠️ Proposed fix
def _topological_waves(tasks: list[ParallelTask]) -> list[list[str]]: """Sort *tasks* into dependency waves using Kahn's algorithm. @@ - task_map: dict[str, ParallelTask] = {t.id: t for t in tasks} + seen: set[str] = set() + duplicate_ids: set[str] = set() + for task in tasks: + if task.id in seen: + duplicate_ids.add(task.id) + else: + seen.add(task.id) + if duplicate_ids: + raise ValueError(f"Duplicate task ids are not allowed: {sorted(duplicate_ids)}") + + task_map: dict[str, ParallelTask] = {t.id: t for t in tasks}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/contrib/patterns/parallel.py` around lines 173 - 195, Before constructing the dependency graph, validate that no two Task objects share the same task.id by scanning the input tasks and rejecting duplicates; specifically, detect duplicate keys that would be inserted into task_map/_task_map and results, and raise a clear ValueError (e.g., "Duplicate task id: '<id>'") instead of allowing later overwrite and a misleading cycle error. Implement this check near where task_map is built (the code that populates task_map/_task_map/results from the tasks iterable), iterating tasks to record seen ids and raising immediately on a second occurrence so downstream code (in_degree, dependents, waves calculation) can assume unique ids. Ensure the error message includes the offending id for easy debugging.
♻️ Duplicate comments (1)
Gradata/src/gradata/cloud/sync.py (1)
36-50:⚠️ Potential issue | 🟠 MajorNormalization is still bypassable for non-file config paths.
_normalize_api_base()only runs inload_config().CloudConfiginstances created directly (e.g.,Gradata/src/gradata/_core.pyLine 1121-1124) can still carryhttps://api.gradata.aiand produce unversioned request URLs after this path rewrite.💡 Proposed fix (normalize at the data model boundary)
`@dataclass` class CloudConfig: @@ - api_base: str = _DEFAULT_API_BASE + api_base: str = _DEFAULT_API_BASE @@ last_sync_at: str = "" + + def __post_init__(self) -> None: + self.api_base = _normalize_api_base(str(self.api_base))Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gradata/src/gradata/cloud/sync.py` around lines 36 - 50, CloudConfig instances can bypass normalization because _normalize_api_base is only called in load_config; ensure the API base is normalized at the data-model boundary by calling _normalize_api_base inside the CloudConfig initializer (or a dedicated setter/validator) so any direct construction (e.g., where CloudConfig is instantiated in _core.py) always converts legacy "https://api.gradata.ai" to "https://api.gradata.ai/api/v1"; update CloudConfig.__init__ (or its from_dict/validate method) to call _normalize_api_base on the api_base field and use the normalized value for subsequent requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/sdk-ci.yml:
- Around line 1-3: Add an explicit top-level permissions block to this workflow
(the file starting with "name: SDK CI" and "on:") to restrict the GITHUB_TOKEN
to least-privilege scopes for the test-only job(s); create a permissions:
mapping (for example, setting only the required tokens such as contents: read
and any other minimal scopes your tests need) so the workflow no longer relies
on repo/org defaults and only grants minimal access.
In @.gitignore:
- Line 89: The TODO notes that the watchdog hook is missing from the installer
assets; update _installer.py to include the watchdog hook files in the installer
assets list (e.g., add the watchdog hook file paths to the
ASSETS/INSTALLER_ASSETS variable or the function that returns asset paths such
as get_installer_assets()), remove the TODO, and ensure any installation logic
that copies or registers hooks (e.g., install_assets(), register_hooks()) also
handles the new watchdog files so they are packaged and installed automatically;
run the installer flow locally to verify the watchdog hook is present after
installation.
- Around line 85-86: Update the .gitignore comment that currently says "two
handoff-watchdog hook files" to accurately reference both hook files by name
(handoff-watchdog.js and handoff-inject.js) or reword to "two handoff hook
files"; locate the comment near the existing mention of handoff-watchdog.js and
replace the phrasing so it clearly lists both filenames or uses the corrected
term.
In `@Gradata/src/gradata/_events.py`:
- Around line 61-81: _schema_initialized currently caches only DB paths, so
_ensure_table may skip schema setup if the file at that path was replaced;
change the cache to record a stable file identity (e.g., os.stat fingerprint
like (st_dev, st_ino, st_mtime) or a short SQLite file fingerprint) instead of
just the path: in _ensure_table, call os.stat(db_file) (or another fingerprint
method) to produce an identity, check that identity against the stored entry for
that path in _schema_initialized (convert _schema_initialized from set[str] to a
mapping str->identity or set of identity tuples), and only skip DDL when the
current identity matches the cached identity; after successfully ensuring the
schema, store/update the identity for db_file in _schema_initialized; apply the
same identity-checking change wherever _schema_initialized is referenced
(including the other usage noted around lines 127-128).
In `@Gradata/src/gradata/brain.py`:
- Around line 881-885: The current hot path in Brain._<unnamed>? uses
importlib.util.find_spec repeatedly to probe for
gradata.enhancements.self_improvement; instead use the cached boolean computed
in __init__ (self._has_self_improvement) instead of importing importlib.util
each call—change the conditional to check self._has_self_improvement and return
"" when it's False so the expensive find_spec call is removed from the hot path.
- Around line 343-350: The cached lessons returned by _load_lessons are the
original mutable Lesson objects, so callers like
apply_rules()/demote_stale_rules() can mutate the cached list and affect later
reads; change _load_lessons to keep storing the parsed lessons in
self._lessons_parse_cache but return a fresh copy of the lessons each time
(e.g., perform a deep copy or create new Lesson instances) so callers receive
independent objects; update references to _lessons_parse_cache and the
_load_lessons function to ensure the cache still holds the canonical parsed
value while all callers get a cloned list to mutate safely.
- Around line 1619-1629: The current use of LOWER(data_json) LIKE ? (built into
the local `where` and `params` used in `conn.execute`) forces a full table scan
and defeats LIMIT; replace this with an indexed text-search approach: create or
use an FTS5 virtual table for event text (e.g., events_fts) that is kept in sync
with the `events` table, then change the query so you query the FTS table with
MATCH for the joined terms (or use the FTS5 AND/OR syntax) to obtain matching
rowids/docids and then SELECT from `events` WHERE id IN (...) ORDER BY id DESC
LIMIT ?; update the code around the `where`, `params`, and `conn.execute` usage
to build an FTS MATCH query from `terms` (or join the FTS result) instead of
using LOWER(data_json) LIKE ? so the search uses the index and LIMIT is applied
efficiently.
In `@Gradata/src/gradata/contrib/patterns/loop_detection.py`:
- Around line 126-130: The eviction branch can IndexError when window size is
zero; add a guard to skip eviction when self._window.maxlen == 0 (or when
self._window is empty) before accessing self._window[0]. Update the block in
loop_detection where it checks len(self._window) == self._window.maxlen to first
ensure self._window.maxlen > 0 (or that self._window) is non-empty, and only
then perform evicted = self._window[0], decrement self._counts[evicted], and
possibly del self._counts[evicted]; this prevents reading from an empty deque
when window_size == 0.
In `@Gradata/src/gradata/contrib/patterns/parallel.py`:
- Around line 315-329: Add a regression test that ensures the
dataclasses.replace path prevents shared-state leaks: create a ParallelTask
instance with a non-trivial input_data, build a DependencyGraph that uses that
task as a dependent (so the code path that uses replace(task, input_data=...) in
parallel.py is exercised), call DependencyGraph.run() twice on the same graph
instance, and after each run assert that the original ParallelTask.input_data
(the task object you created) remains equal to its original value (unchanged) to
verify replace() was used instead of mutating the original; make sure the test
references DependencyGraph.run, ParallelTask.input_data, and the task with
depends_on to target the exact code path.
In `@Gradata/src/gradata/contrib/patterns/q_learning_router.py`:
- Around line 226-227: The cached map self._agent_index is built directly from
the potentially mutable/duplicate self.config.agents which can later cause
misrouting or KeyError; fix by (1) validating and rejecting duplicate agent
names when constructing the map (raise ValueError with a clear message if
duplicates found) and (2) use an immutable snapshot for the mapping source
(e.g., tuple(self.config.agents)) so the map remains stable; implement this in
the code that creates self._agent_index (referencing _agent_index and
config.agents) and add a short unit-test or guard to ensure duplicates are
detected and prevented.
In `@Gradata/src/gradata/enhancements/self_improvement/_confidence.py`:
- Line 745: When handling a reinforcing fire (the branch where direction ==
"REINFORCING" and you increment fire_count), also set _fired_this_session = True
so the idle-session tracking is reset; locate the REINFORCING branch in this
module (where fire_count is increment) and mirror the same _fired_this_session
assignment used in the other firing branches, and apply the same change to the
other similar fire_count increment blocks in this file so that any fired lesson
cannot also be counted as idle in the same session.
In `@Gradata/src/gradata/enhancements/similarity.py`:
- Around line 29-122: The stop-word list currently contains "might", which
causes _tokenize() to remove modal hedging tokens before _expand_synonyms() can
canonicalize hedging intent; update the code so hedging tokens are preserved by
removing "might" from the _STOP_WORDS set (or alternatively move
normalization/hedging canonicalization to run before stop-word filtering in
_tokenize/_expand_synonyms) and ensure the same change is applied to the other
stop-word definitions referenced around the file (the duplicate sets at the
other locations noted in the review).
In `@Gradata/src/gradata/hooks/implicit_feedback.py`:
- Around line 188-203: The current branch emits OUTPUT_ACCEPTED whenever
has_approval or tacit_accept is true, but mixed feedback (e.g., approval plus
gap/correction) should not count as acceptance; update the logic around
emit_hook_event to only emit explicit acceptance when approvals exist and there
are no conflicting signal types, and only emit tacit acceptance when
tacit_accept is true and there are no conflict signals. Concretely, compute a
guard like has_pure_approval = has_approval and not any(s["type"] in
("gap","rejection","correction") for s in signals) and use has_pure_approval
(instead of has_approval) for the first emit_hook_event call, and likewise
require no conflict signals before emitting the tacit acceptance event for
tacit_accept; keep using emit_hook_event and the existing payload shapes.
In `@Gradata/src/gradata/middleware/_core.py`:
- Around line 185-200: The cached entry is only keyed by mtime and stores a
mutable list, so changing min_confidence or mutating the returned list corrupts
cache; update the caching in the method that calls _load_from_brain (the
load()/loader block using self._cache, self._brain_path, and
self.min_confidence) to include min_confidence in the cache key (e.g., cache =
(mtime, self.min_confidence, ...)) and store an immutable copy of filtered
(e.g., a tuple) so callers cannot mutate cached state, and when returning to
callers return a new list (or shallow copy) derived from the cached immutable
tuple to preserve immutability and correct behavior when min_confidence changes.
In `@Gradata/src/gradata/rules/rule_graph.py`:
- Around line 277-293: The read path for rule relationships is missing tenant
scoping: update the relationship SELECTs used in the get_related_rules logic to
filter by tenant_id (the same tenant_id written by store_relationship).
Concretely, modify both queries that build rows (the ones selecting rule_a_id,
rule_b_id, relationship, confidence) to add "AND tenant_id = ?" (or "WHERE ...
AND tenant_id = ?" in the alternate branch) and append the tenant_id value to
the parameter tuples so only relationships for the current tenant are returned;
ensure you use the same tenant identifier variable used elsewhere (matching
store_relationship) and keep both the rel_type and non-rel_type branches
consistent.
In `@Gradata/src/gradata/rules/rule_tracker.py`:
- Around line 158-165: The direct SELECT bypasses schema bootstrap and PRAGMA
setup; call _ensure_table(conn) immediately after opening the sqlite3 connection
to guarantee the events table exists and restore any initialization done by
gradata._events.query(), and also set the DB busy timeout (e.g. via
conn.execute(f"PRAGMA busy_timeout = {_p.BUSY_TIMEOUT}") or the existing project
constant) before running the SELECT that uses rule_id and limit so the method
won't silently return empty on a fresh brain or under concurrent writes.
- Around line 162-165: The LIKE pattern uses rule_id directly which allows % and
_ to act as wildcards; update the SQL to escape special LIKE characters in
rule_id (escape backslash first, then replace % -> \% and _ -> \_) and build the
pattern as '%"rule:{escaped_rule_id}"%' while using the ESCAPE '\' clause in the
query; apply the same escape logic for the second occurrence noted in the file
so tags_json LIKE ? becomes tags_json LIKE ? ESCAPE '\' with the escaped
parameter substituted.
In `@Gradata/src/gradata/rules/rule_tree.py`:
- Around line 379-380: The frontmatter id generation using
zlib.crc32(lesson.description) % 10000 in the content string is too small and
ignores category/path, causing collisions; fix by generating a stable,
larger-space id that incorporates both lesson.category and lesson.description
(and optionally lesson.path) such as using full crc32 hex or a namespaced UUID
(e.g., uuid5) over the combined string, and replace the current f-string id
expression (the content variable that references lesson.category and zlib.crc32)
with the new deterministic id generator so exported ids are unique across
categories and descriptions.
In `@Gradata/tests/test_integrations.py`:
- Around line 97-102: Add a regression test that invokes patched_create using
the positional messages argument (not kwargs) to hit the positional routing
branch in patched_create in openai_adapter (the rewritten path around
patched_create), and assert that the call forwarded to original_create contains
a system message inserted at index 0 while the caller's original messages list
(e.g. messages = [{"role":"user","content":"Hello"}]) remains unchanged;
specifically call patched_create(messages) (positional) and inspect
original_create.call_args.args or the first positional arg to verify the system
role/content and that messages is still the original list.
---
Outside diff comments:
In `@Gradata/src/gradata/_manifest_metrics.py`:
- Around line 328-357: The first query block in _quality_metrics currently uses
conn = get_connection(db) and may skip conn.close() on exceptions; change it to
use a context manager (e.g., with closing(get_connection(db)) as conn from
contextlib) so the connection is always closed even on errors, keep the same
logic that builds recent_sessions, placeholders, total_corrections,
total_outputs and sets result["correction_rate"], and ensure you add the
necessary import (contextlib.closing) and remove the explicit conn.close() call.
In `@Gradata/src/gradata/contrib/patterns/guardrails.py`:
- Around line 640-652: The generated check function _make_check currently
ignores its input and always returns a passing GuardCheck; change check_fn to
actually scan the provided data (the output string or token stream) for the rule
expressed in rule_text and set GuardCheck.result accordingly: for
negative/forbidden rules (detect prefixes like "never", "do not", "don't", "no")
treat any occurrence of the forbidden keyword/phrase in data as a violation and
return result="fail" (with details indicating the matched substring and
action_taken="blocked" or "flagged"); for positive/required rules (detect
"always", "must", "ensure") treat absence of the required phrase as a violation
and return result="fail" or "warn"; implement simple, case-insensitive substring
and token-boundary checks using rule_text.lower() and data_text.lower(), and
ensure the same fix is applied to the duplicate check factory used later so
graduated rules actually enforce or flag failures instead of always passing.
In `@Gradata/src/gradata/contrib/patterns/memory.py`:
- Around line 132-147: The method that builds matches and then branches on "if
limit >= len(matches):" doesn't validate the incoming limit, allowing negative
values; add an explicit validation at the start of this method (the function
iterating self._data and computing matches, referencing variables limit and
matches) to ensure limit is an int >= 0 and raise a ValueError (or TypeError for
non-int) for invalid inputs; keep the existing ranking logic (matches.sort /
heapq.nlargest) unchanged and only proceed after the validation.
In `@Gradata/src/gradata/contrib/patterns/orchestrator.py`:
- Around line 256-258: The prepend parameter in the function signature (prepend:
bool = False) no longer affects lookup precedence because classify_request now
uses O(1) dict lookup; update the function's docstring (the method that accepts
prepend) to state that prepend only affects list ordering for
iteration/inspection and does not change lookup behavior, and mention
classify_request as the reason; alternatively, if you prefer a breaking change,
remove the prepend parameter and its uses (update callers and tests) — prefer
the non-breaking docstring clarification unless you intentionally want to change
the API.
In `@Gradata/src/gradata/contrib/patterns/parallel.py`:
- Around line 173-195: Before constructing the dependency graph, validate that
no two Task objects share the same task.id by scanning the input tasks and
rejecting duplicates; specifically, detect duplicate keys that would be inserted
into task_map/_task_map and results, and raise a clear ValueError (e.g.,
"Duplicate task id: '<id>'") instead of allowing later overwrite and a
misleading cycle error. Implement this check near where task_map is built (the
code that populates task_map/_task_map/results from the tasks iterable),
iterating tasks to record seen ids and raising immediately on a second
occurrence so downstream code (in_degree, dependents, waves calculation) can
assume unique ids. Ensure the error message includes the offending id for easy
debugging.
---
Duplicate comments:
In `@Gradata/src/gradata/cloud/sync.py`:
- Around line 36-50: CloudConfig instances can bypass normalization because
_normalize_api_base is only called in load_config; ensure the API base is
normalized at the data-model boundary by calling _normalize_api_base inside the
CloudConfig initializer (or a dedicated setter/validator) so any direct
construction (e.g., where CloudConfig is instantiated in _core.py) always
converts legacy "https://api.gradata.ai" to "https://api.gradata.ai/api/v1";
update CloudConfig.__init__ (or its from_dict/validate method) to call
_normalize_api_base on the api_base field and use the normalized value for
subsequent requests.
🪄 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: 513b8f79-9f0f-4870-9cdd-afa30c3ec77c
⛔ Files ignored due to path filters (2)
.claude/hooks/session-start/handoff-inject.jsis excluded by!.claude/**.claude/hooks/user-prompt/handoff-watchdog.jsis excluded by!.claude/**
📒 Files selected for processing (44)
.github/workflows/sdk-ci.yml.gitignoreGradata/src/gradata/_brain_manifest.pyGradata/src/gradata/_events.pyGradata/src/gradata/_manifest_helpers.pyGradata/src/gradata/_manifest_metrics.pyGradata/src/gradata/brain.pyGradata/src/gradata/cloud/client.pyGradata/src/gradata/cloud/sync.pyGradata/src/gradata/contrib/enhancements/quality_gates.pyGradata/src/gradata/contrib/patterns/guardrails.pyGradata/src/gradata/contrib/patterns/loop_detection.pyGradata/src/gradata/contrib/patterns/memory.pyGradata/src/gradata/contrib/patterns/orchestrator.pyGradata/src/gradata/contrib/patterns/parallel.pyGradata/src/gradata/contrib/patterns/q_learning_router.pyGradata/src/gradata/contrib/patterns/rag.pyGradata/src/gradata/contrib/patterns/reflection.pyGradata/src/gradata/contrib/patterns/tree_of_thoughts.pyGradata/src/gradata/enhancements/meta_rules.pyGradata/src/gradata/enhancements/reporting.pyGradata/src/gradata/enhancements/router_warmstart.pyGradata/src/gradata/enhancements/rule_context_bridge.pyGradata/src/gradata/enhancements/self_improvement/_confidence.pyGradata/src/gradata/enhancements/self_improvement/_graduation.pyGradata/src/gradata/enhancements/similarity.pyGradata/src/gradata/graph.pyGradata/src/gradata/hooks/implicit_feedback.pyGradata/src/gradata/hooks/session_close.pyGradata/src/gradata/integrations/embeddings.pyGradata/src/gradata/integrations/langchain_adapter.pyGradata/src/gradata/integrations/openai_adapter.pyGradata/src/gradata/mcp_server.pyGradata/src/gradata/middleware/_core.pyGradata/src/gradata/rules/rule_engine/_engine.pyGradata/src/gradata/rules/rule_engine/_scoring.pyGradata/src/gradata/rules/rule_graph.pyGradata/src/gradata/rules/rule_tracker.pyGradata/src/gradata/rules/rule_tree.pyGradata/src/gradata/rules/scope.pyGradata/src/gradata/sidecar/watcher.pyGradata/tests/conftest.pyGradata/tests/test_agentic_synthesis.pyGradata/tests/test_integrations.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
📚 Learning: 2026-04-17T17:18:07.439Z
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
Applied to files:
Gradata/tests/conftest.pyGradata/src/gradata/contrib/patterns/rag.pyGradata/tests/test_integrations.pyGradata/src/gradata/brain.py
🪛 Ruff (0.15.10)
Gradata/src/gradata/integrations/openai_adapter.py
[warning] 78-78: Consider (*args[:1], messages, *args[2:]) instead of concatenation
Replace with (*args[:1], messages, *args[2:])
(RUF005)
🔇 Additional comments (48)
Gradata/src/gradata/hooks/session_close.py (1)
306-326: Two-sided 40-character gating is now correctly enforced before REJECT substring matching.This update closes the prior false-positive path by requiring both normalized strings to meet the length floor before containment checks proceed.
Gradata/src/gradata/contrib/patterns/reflection.py (5)
35-35: Good refactor for duplicate criterion detection.Line 173 now uses
Counter(names)efficiently and keeps validation behavior intact.Also applies to: 173-175
68-68: Validation error messages remain clear and correct.No functional issues in these formatting updates.
Also applies to: 91-92
212-212: Evaluation/refinement flow changes are sound.Both comprehensions preserve behavior and improve readability.
Also applies to: 297-297
359-359:default_evaluatorrefactor looks behavior-preserving.No correctness regressions found in the updated string/reason and tuple formatting.
Also applies to: 363-373, 377-379, 386-388, 393-401, 404-404, 409-409
528-535: Criteria construction refactor is clean.The multiline
Criterion(...)append keeps behavior unchanged and improves readability.Gradata/src/gradata/integrations/langchain_adapter.py (3)
74-75: Good observability upgrade forapply_brain_rulesfailures.Line 74-75 now records the failure path instead of silently swallowing it, which is the right behavior for troubleshooting while preserving fallback flow.
81-82: Good parity forcontext_forexception handling.Line 81-82 consistently logs retrieval failures and keeps memory loading resilient.
104-105:brain.observeerror handling change looks good.Line 104-105 improves debuggability without changing runtime control flow in
save_context.Gradata/src/gradata/contrib/patterns/tree_of_thoughts.py (2)
18-18: No actionable concern in these non-functional line changes.Also applies to: 32-32
125-143: Efficient scorer-path optimization looks correct.Precomputing
rule_word_setsand reusingcandidate_wordsremoves repeated tokenization work while preserving scoring and overlap-penalty behavior.Gradata/src/gradata/graph.py (4)
151-159: Good complexity win on state grouping lookup.Using
nodes_by_idat Line 151 and Line 159 removes repeated linear scans while preserving the same lookup semantics.
173-203: Edge-construction refactor is clean and behavior-preserving.The multiline
GraphEdge(...)construction at Line 173-203 keeps relation/weight values unchanged and improves readability.
206-259: Set-based merged-target tracking is a solid optimization.Replacing per-meta-rule
any(...)edge scans withmerged_into_targetsmembership checks (Line 206, Line 247) keeps fallback behavior while reducing time complexity.
316-328: Mermaid rendering updates look safe.The shape map formatting and node-line f-string quoting changes at Line 316-328 are non-functional and keep output structure consistent.
Gradata/src/gradata/enhancements/reporting.py (2)
212-240: Good move replacing silent failure paths with debug logs.The new logging keeps this path non-fatal while making failures diagnosable during troubleshooting.
334-345: Connection lifecycle handling is safer here.Using
contextlib.closing(sqlite3.connect(...))removes manual close risk and keeps DB-read failures properly surfaced intoreport.issues.Gradata/src/gradata/contrib/enhancements/quality_gates.py (1)
352-370: Nice reliability improvement on DB access.The switch to context-managed SQLite cleanup plus debug logging on failures improves operational visibility without changing the report contract.
Gradata/src/gradata/contrib/patterns/guardrails.py (2)
51-53: Formatting/readability-only edits reviewed; no actionable issues.No behavior regressions found in these segments.
Also applies to: 247-248, 329-334, 460-470, 498-500, 560-562, 642-643, 653-654
203-207:block_reasonnow correctly follows the first-failure contract.Good fix: using the first failing check keeps runtime behavior aligned with the documented
GuardedResult.block_reasonsemantics..gitignore (1)
90-99: LGTM! The negation pattern is correctly implemented.The alternating ignore/negate pattern correctly whitelists the two specific hook files deep within the ignored
.claude/directory tree. All parent directories are properly negated, and intermediate re-ignores prevent unintended sibling files from being tracked. This follows Git best practices for selective whitelisting.Gradata/src/gradata/rules/rule_graph.py (1)
244-265: Connection cleanup refactor is solid.Using
contextlib.closing(...)here is a good reliability improvement; the connection is guaranteed to close while preserving the existing commit/insert behavior.Gradata/src/gradata/contrib/patterns/memory.py (7)
14-20:VALID_TYPESrefactor is clean and behavior-preserving.
This improves readability without changing semantics.
60-60: Validation message compaction looks good.
The error remains clear and preserves diagnostic value.
226-227:EpisodicMemory.decaycondition rewrite is correct.
No logic regression in the pruning predicate.
356-357:ProceduralMemory.decaypredicate remains correct after compaction.
Looks good.
393-393: Unknown-type guard is still explicit and correct.
Good simplification with no behavior change.
423-424:MemoryManager.decaycondition compaction is safe.
No correctness concerns here.
449-450:avg_reinforcementsrewrite preserves safety and output behavior.
Thetotal > 0guard still correctly prevents division by zero.Gradata/src/gradata/contrib/patterns/orchestrator.py (4)
246-249: LGTM!The O(1) lookup index is correctly initialized from the existing patterns list. Good addition of the explanatory comment.
313-315: LGTM!Index synchronization is correct. The dict assignment properly handles both new registrations and replacements of existing intents.
374-375: LGTM!Clean O(1) optimization using dict lookup with appropriate fallback. The comment documenting the change from O(N) is helpful for future maintainers.
512-520: LGTM!Formatting improvement with expanded dict literals and trailing commas. No logic changes.
Gradata/src/gradata/enhancements/router_warmstart.py (1)
78-96: Connection lifecycle refactor is solid.Moving query execution inside
contextlib.closing(...)ensures the SQLite handle is reliably closed even on failure.Gradata/src/gradata/contrib/patterns/loop_detection.py (1)
39-39: Good refactor: O(1) repeat counting with consistent reset state.Maintaining
_countsalongside_windowand clearing it inreset()is a clean performance and consistency improvement.Also applies to: 106-106, 124-125, 132-133, 156-156
Gradata/src/gradata/_manifest_helpers.py (1)
58-75: Good connection lifecycle hardening.Using
contextlib.closing(get_connection(db))here is a solid cleanup improvement and keeps DB resource handling consistent.Gradata/src/gradata/_brain_manifest.py (1)
62-71: DB cross-check cleanup looks correct.The
closing(get_connection(...))usage keeps the cross-check path safer without changing behavior.Gradata/src/gradata/cloud/client.py (1)
171-176: Nice observability improvement on manifest parse failure.Including the manifest path and exception in the warning makes cloud connect/sync failures much easier to diagnose.
Gradata/src/gradata/mcp_server.py (1)
452-454: Good failure-path instrumentation.Logging
_dispatchexceptions with traceback here is the right move for production debugging while preserving API behavior.Gradata/src/gradata/enhancements/similarity.py (1)
295-317: Nice extraction of reusable vector helpers.This keeps the probe-vs-many dedup path cheap while preserving the same empty-input behavior as
semantic_similarity.Gradata/src/gradata/enhancements/self_improvement/_graduation.py (1)
130-157: Good hot-path hoist.Reading the Beta-LB config once and reusing precomputed rule vectors removes repeated work from the promotion loop without changing the dedup short-circuit behavior.
Also applies to: 243-269
Gradata/src/gradata/integrations/embeddings.py (1)
73-77: Deterministic trigram indexing fix looks solid.This removes cross-process randomness from local embeddings and stabilizes similarity/clustering behavior.
Gradata/src/gradata/rules/rule_engine/_scoring.py (1)
40-57: Good refactor: shared scope parsing removes duplicated edge-case handling.Centralizing
scope_jsonparsing here improves consistency and reduces future drift between scoring and engine paths.Also applies to: 392-397
Gradata/tests/conftest.py (1)
119-141: Nice test isolation hardening for hook kill-switch env vars.Session-level scrub + restore is the right approach to keep hook tests deterministic across developer environments.
Gradata/src/gradata/enhancements/rule_context_bridge.py (1)
91-94: Good cleanup on WAL checkpoint connection handling.Using
contextlib.closing(...)plus debug logging on checkpoint failure is a safe resilience improvement.Gradata/src/gradata/contrib/patterns/rag.py (1)
396-434: Two-pass flow placement and query-field semantics are corrected.Moving expansion before hybrid finalization and keeping
RetrievalResult.queryas the original user input resolves the prior semantic hazard cleanly.Gradata/src/gradata/integrations/openai_adapter.py (1)
62-79: Good fix for shared-state mutation in message injection path.Cloning
messagesbefore rule injection and routing the cloned payload back into the request path is the right behavior; this prevents cross-call accumulation on caller-owned message objects.Also applies to: 89-89
Gradata/tests/test_integrations.py (1)
63-83: Nice assertion update: validating outbound payload instead of caller-owned input.These checks correctly assert behavior on the adapter-supplied
messagesand confirm caller input immutability.Also applies to: 90-102
| name: SDK CI | ||
|
|
||
| on: |
There was a problem hiding this comment.
Set explicit minimal GITHUB_TOKEN permissions in CI workflow.
Without an explicit permissions block, token scope depends on repo/org defaults and can be broader than required for test-only jobs.
🔒 Suggested least-privilege update
name: SDK CI
on:
@@
+permissions:
+ contents: read
+
jobs:
test:Also applies to: 18-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sdk-ci.yml around lines 1 - 3, Add an explicit top-level
permissions block to this workflow (the file starting with "name: SDK CI" and
"on:") to restrict the GITHUB_TOKEN to least-privilege scopes for the test-only
job(s); create a permissions: mapping (for example, setting only the required
tokens such as contents: read and any other minimal scopes your tests need) so
the workflow no longer relies on repo/org defaults and only grants minimal
access.
| # Track only the two handoff-watchdog hook files so future sessions (and | ||
| # fresh clones) keep the context-pressure watchdog code under version |
There was a problem hiding this comment.
Clarify the comment to accurately describe both hook files.
The comment refers to "two handoff-watchdog hook files," but only one file is named handoff-watchdog.js (line 97). The second file is handoff-inject.js (line 99). Consider revising to "two handoff hook files" or listing both names explicitly for clarity.
📝 Proposed fix for the comment
-# Track only the two handoff-watchdog hook files so future sessions (and
-# fresh clones) keep the context-pressure watchdog code under version
+# Track only the handoff-watchdog and handoff-inject hook files so future
+# sessions (and fresh clones) keep the hook code under version📝 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.
| # Track only the two handoff-watchdog hook files so future sessions (and | |
| # fresh clones) keep the context-pressure watchdog code under version | |
| # Track only the handoff-watchdog and handoff-inject hook files so future | |
| # sessions (and fresh clones) keep the hook code under version |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 85 - 86, Update the .gitignore comment that
currently says "two handoff-watchdog hook files" to accurately reference both
hook files by name (handoff-watchdog.js and handoff-inject.js) or reword to "two
handoff hook files"; locate the comment near the existing mention of
handoff-watchdog.js and replace the phrasing so it clearly lists both filenames
or uses the corrected term.
| # fresh clones) keep the context-pressure watchdog code under version | ||
| # control. Wiring them into the dispatcher/settings is machine-local — | ||
| # see Gradata/src/gradata/hooks/_installer.py for the portable install | ||
| # path (TODO: add watchdog to installer assets). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO comment flagged: Add watchdog to installer assets.
The inline TODO suggests adding the watchdog hook to the installer assets in _installer.py. This ensures a more automated, portable installation path for the hook files.
Would you like me to open an issue to track this task, or would you prefer to address it in a future PR?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 89, The TODO notes that the watchdog hook is missing from
the installer assets; update _installer.py to include the watchdog hook files in
the installer assets list (e.g., add the watchdog hook file paths to the
ASSETS/INSTALLER_ASSETS variable or the function that returns asset paths such
as get_installer_assets()), remove the TODO, and ensure any installation logic
that copies or registers hooks (e.g., install_assets(), register_hooks()) also
handles the new watchdog files so they are packaged and installed automatically;
run the installer flow locally to verify the watchdog hook is present after
installation.
| # Process-level cache: once we've initialized the schema on a given db path | ||
| # in this process, subsequent emit()/supersede() calls skip the 10+ DDL | ||
| # statements and single commit. SQLite parses CREATE IF NOT EXISTS etc. on | ||
| # every call otherwise, adding measurable latency to hot write paths. | ||
| _schema_initialized: set[str] = set() | ||
|
|
||
|
|
||
| def _ensure_table(conn: sqlite3.Connection): | ||
| conn.execute("PRAGMA journal_mode=WAL") | ||
| conn.execute("PRAGMA busy_timeout=5000") | ||
| # Skip DDL if this process already ensured the schema for this db file. | ||
| # The conn-level PRAGMAs above still need to run per-connection. | ||
| try: | ||
| db_file = next( | ||
| (row[2] for row in conn.execute("PRAGMA database_list").fetchall() if row[1] == "main"), | ||
| None, | ||
| ) | ||
| except sqlite3.Error: | ||
| db_file = None | ||
| if db_file and db_file in _schema_initialized: | ||
| return |
There was a problem hiding this comment.
Path-only schema caching breaks after DB replacement.
Once a path is added to _schema_initialized, _ensure_table() never verifies that the current file at that path is still the same database. If system.db is recreated or swapped in-process, the new file skips schema setup and the next SELECT/INSERT can fail with missing table/column errors.
Possible fix
-_schema_initialized: set[str] = set()
+_schema_initialized: set[tuple[str, int, int]] = set()
...
- if db_file and db_file in _schema_initialized:
- return
+ schema_key = None
+ if db_file:
+ try:
+ st = os.stat(db_file)
+ schema_key = (db_file, st.st_ino, st.st_mtime_ns)
+ except OSError:
+ schema_key = None
+ if schema_key and schema_key in _schema_initialized:
+ return
...
- if db_file:
- _schema_initialized.add(db_file)
+ if schema_key:
+ _schema_initialized.add(schema_key)Also applies to: 127-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/src/gradata/_events.py` around lines 61 - 81, _schema_initialized
currently caches only DB paths, so _ensure_table may skip schema setup if the
file at that path was replaced; change the cache to record a stable file
identity (e.g., os.stat fingerprint like (st_dev, st_ino, st_mtime) or a short
SQLite file fingerprint) instead of just the path: in _ensure_table, call
os.stat(db_file) (or another fingerprint method) to produce an identity, check
that identity against the stored entry for that path in _schema_initialized
(convert _schema_initialized from set[str] to a mapping str->identity or set of
identity tuples), and only skip DDL when the current identity matches the cached
identity; after successfully ensuring the schema, store/update the identity for
db_file in _schema_initialized; apply the same identity-checking change wherever
_schema_initialized is referenced (including the other usage noted around lines
127-128).
| mtime = path.stat().st_mtime | ||
| key = (str(path), mtime) | ||
| cache = getattr(self, "_lessons_parse_cache", None) | ||
| if cache is not None and cache[0] == key: | ||
| return cache[1] | ||
| lessons = parse_lessons(path.read_text(encoding="utf-8")) | ||
| self._lessons_parse_cache = (key, lessons) | ||
| return lessons |
There was a problem hiding this comment.
The lessons cache is sharing mutable Lesson objects across readers.
_load_lessons() returns the same cached list instance. Callers like apply_rules() mutate lessons in place (demote_stale_rules() changes state/stale), so one read can silently change what every later read sees without updating lessons.md.
Possible fix
- if cache is not None and cache[0] == key:
- return cache[1]
+ if cache is not None and cache[0] == key:
+ import copy
+ return copy.deepcopy(cache[1])
lessons = parse_lessons(path.read_text(encoding="utf-8"))
- self._lessons_parse_cache = (key, lessons)
+ import copy
+ self._lessons_parse_cache = (key, copy.deepcopy(lessons))
return lessons🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/src/gradata/brain.py` around lines 343 - 350, The cached lessons
returned by _load_lessons are the original mutable Lesson objects, so callers
like apply_rules()/demote_stale_rules() can mutate the cached list and affect
later reads; change _load_lessons to keep storing the parsed lessons in
self._lessons_parse_cache but return a fresh copy of the lessons each time
(e.g., perform a deep copy or create new Lesson instances) so callers receive
independent objects; update references to _lessons_parse_cache and the
_load_lessons function to ensure the cache still holds the canonical parsed
value while all callers get a cloned list to mutate safely.
| with contextlib.closing(sqlite3.connect(str(db_path))) as conn: | ||
| conn.row_factory = sqlite3.Row | ||
|
|
||
| if rel_type is not None: | ||
| rows = conn.execute( | ||
| "SELECT rule_a_id, rule_b_id, relationship, confidence " | ||
| "FROM rule_relationships " | ||
| "WHERE (rule_a_id = ? OR rule_b_id = ?) AND relationship = ?", | ||
| (rule_id, rule_id, rel_type.value), | ||
| ).fetchall() | ||
| else: | ||
| rows = conn.execute( | ||
| "SELECT rule_a_id, rule_b_id, relationship, confidence " | ||
| "FROM rule_relationships " | ||
| "WHERE rule_a_id = ? OR rule_b_id = ?", | ||
| (rule_id, rule_id), | ||
| ).fetchall() |
There was a problem hiding this comment.
Add tenant scoping to relationship reads.
store_relationship writes tenant_id (Line 262), but get_related_rules reads without tenant filtering (Lines 281-293). In a shared DB, this can return another tenant’s edges.
🔧 Proposed fix
def get_related_rules(
db_path: str | Path,
rule_id: str,
rel_type: RuleRelationType | None = None,
) -> list[dict]:
@@
- with contextlib.closing(sqlite3.connect(str(db_path))) as conn:
+ _tid = tenant_for(Path(db_path).parent)
+ with contextlib.closing(sqlite3.connect(str(db_path))) as conn:
conn.row_factory = sqlite3.Row
+ cols = {
+ row["name"]
+ for row in conn.execute("PRAGMA table_info(rule_relationships)").fetchall()
+ }
+ tenant_clause = " AND (tenant_id = ? OR tenant_id IS NULL)" if "tenant_id" in cols else ""
if rel_type is not None:
rows = conn.execute(
- "SELECT rule_a_id, rule_b_id, relationship, confidence "
- "FROM rule_relationships "
- "WHERE (rule_a_id = ? OR rule_b_id = ?) AND relationship = ?",
- (rule_id, rule_id, rel_type.value),
+ "SELECT rule_a_id, rule_b_id, relationship, confidence "
+ "FROM rule_relationships "
+ f"WHERE (rule_a_id = ? OR rule_b_id = ?) AND relationship = ?{tenant_clause}",
+ (rule_id, rule_id, rel_type.value, _tid)
+ if tenant_clause
+ else (rule_id, rule_id, rel_type.value),
).fetchall()
else:
rows = conn.execute(
- "SELECT rule_a_id, rule_b_id, relationship, confidence "
- "FROM rule_relationships "
- "WHERE rule_a_id = ? OR rule_b_id = ?",
- (rule_id, rule_id),
+ "SELECT rule_a_id, rule_b_id, relationship, confidence "
+ "FROM rule_relationships "
+ f"WHERE (rule_a_id = ? OR rule_b_id = ?){tenant_clause}",
+ (rule_id, rule_id, _tid) if tenant_clause else (rule_id, rule_id),
).fetchall()Based on learnings: PR #102 required a tenant-scoped fallback SELECT in gradata/_events.py, which indicates tenant-scoped reads are expected in shared persistence paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/src/gradata/rules/rule_graph.py` around lines 277 - 293, The read
path for rule relationships is missing tenant scoping: update the relationship
SELECTs used in the get_related_rules logic to filter by tenant_id (the same
tenant_id written by store_relationship). Concretely, modify both queries that
build rows (the ones selecting rule_a_id, rule_b_id, relationship, confidence)
to add "AND tenant_id = ?" (or "WHERE ... AND tenant_id = ?" in the alternate
branch) and append the tenant_id value to the parameter tuples so only
relationships for the current tenant are returned; ensure you use the same
tenant identifier variable used elsewhere (matching store_relationship) and keep
both the rel_type and non-rel_type branches consistent.
| with contextlib.closing(sqlite3.connect(str(db_path or _p.DB_PATH))) as conn: | ||
| conn.row_factory = sqlite3.Row | ||
| rows = conn.execute( | ||
| "SELECT * FROM events " | ||
| "WHERE type = 'RULE_APPLICATION' AND tags_json LIKE ? " | ||
| "ORDER BY id DESC LIMIT ?", | ||
| (f'%"rule:{rule_id}"%', limit), | ||
| ).fetchall() |
There was a problem hiding this comment.
Restore _ensure_table(conn) on the direct read path.
This bypasses the schema/bootstrap work that gradata._events.query() used to do. On a fresh brain, events may not exist yet; under concurrent writes you also lose the configured busy_timeout, so this method can now fall into the broad except and silently return an empty history.
Suggested fix
import contextlib
import json
import sqlite3
from gradata import _paths as _p
+ from gradata._events import _ensure_table
with contextlib.closing(sqlite3.connect(str(db_path or _p.DB_PATH))) as conn:
conn.row_factory = sqlite3.Row
+ _ensure_table(conn)
rows = conn.execute(
"SELECT * FROM events "
"WHERE type = 'RULE_APPLICATION' AND tags_json LIKE ? "
"ORDER BY id DESC LIMIT ?",
(f'%"rule:{rule_id}"%', limit),
).fetchall()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/src/gradata/rules/rule_tracker.py` around lines 158 - 165, The direct
SELECT bypasses schema bootstrap and PRAGMA setup; call _ensure_table(conn)
immediately after opening the sqlite3 connection to guarantee the events table
exists and restore any initialization done by gradata._events.query(), and also
set the DB busy timeout (e.g. via conn.execute(f"PRAGMA busy_timeout =
{_p.BUSY_TIMEOUT}") or the existing project constant) before running the SELECT
that uses rule_id and limit so the method won't silently return empty on a fresh
brain or under concurrent writes.
| "WHERE type = 'RULE_APPLICATION' AND tags_json LIKE ? " | ||
| "ORDER BY id DESC LIMIT ?", | ||
| (f'%"rule:{rule_id}"%', limit), | ||
| ).fetchall() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "rule_tracker.py" | head -5Repository: Gradata/gradata
Length of output: 102
🏁 Script executed:
cat -n ./Gradata/src/gradata/rules/rule_tracker.py | sed -n '150,180p'Repository: Gradata/gradata
Length of output: 1378
🏁 Script executed:
cat -n ./Gradata/src/gradata/rules/rule_tracker.py | sed -n '130,160p'Repository: Gradata/gradata
Length of output: 1184
🏁 Script executed:
grep -r "tags_json" ./Gradata/src/gradata --include="*.py" | head -20Repository: Gradata/gradata
Length of output: 1470
🏁 Script executed:
grep -B5 -A5 "tags_json" ./Gradata/src/gradata/_events.py | head -40Repository: Gradata/gradata
Length of output: 1600
🏁 Script executed:
grep -B10 -A10 "rule:" ./Gradata/src/gradata --include="*.py" -r | head -50Repository: Gradata/gradata
Length of output: 3661
🏁 Script executed:
grep -B5 -A5 '"rule:' ./Gradata/src/gradata --include="*.py" -rRepository: Gradata/gradata
Length of output: 2615
🏁 Script executed:
grep -B10 "def get_rule_history" ./Gradata/src/gradata/rules/rule_tracker.py | head -20Repository: Gradata/gradata
Length of output: 313
🏁 Script executed:
grep -B20 "emit.*RULE_APPLICATION" ./Gradata/src/gradata/rules/rule_tracker.py | head -30Repository: Gradata/gradata
Length of output: 606
🏁 Script executed:
grep -B5 -A15 "def emit" ./Gradata/src/gradata/_events.py | head -40Repository: Gradata/gradata
Length of output: 1106
🏁 Script executed:
grep -A30 "def emit" ./Gradata/src/gradata/_events.py | grep -A20 "tags_json\|json.dumps"Repository: Gradata/gradata
Length of output: 41
🏁 Script executed:
sed -n '100,200p' ./Gradata/src/gradata/_events.py | cat -nRepository: Gradata/gradata
Length of output: 4850
🏁 Script executed:
sed -n '100,150p' ./Gradata/src/gradata/_events.py | cat -nRepository: Gradata/gradata
Length of output: 2900
🏁 Script executed:
sed -n '100,200p' ./Gradata/src/gradata/_events.py | cat -n | grep -A10 "json.dumps"Repository: Gradata/gradata
Length of output: 41
🏁 Script executed:
sed -n '32,200p' ./Gradata/src/gradata/_events.py | cat -nRepository: Gradata/gradata
Length of output: 7945
🏁 Script executed:
sed -n '169,250p' ./Gradata/src/gradata/_events.py | cat -nRepository: Gradata/gradata
Length of output: 3697
Escape special characters in the LIKE pattern to prevent unintended matches on rule_id containing _ or %.
The rule_id parameter is used directly in a SQLite LIKE pattern without escaping. Since % and _ are wildcards in LIKE clauses, a rule_id like foo_bar or foo%bar would match unintended rows. The tags are stored as JSON (e.g., ["rule:foo_bar"]), so the pattern must account for JSON encoding and properly escape special characters.
Suggested fix
with contextlib.closing(sqlite3.connect(str(db_path or _p.DB_PATH))) as conn:
conn.row_factory = sqlite3.Row
+ needle = json.dumps(f"rule:{rule_id}")
+ like_pattern = (
+ "%"
+ + needle.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
+ + "%"
+ )
rows = conn.execute(
"SELECT * FROM events "
- "WHERE type = 'RULE_APPLICATION' AND tags_json LIKE ? "
+ "WHERE type = 'RULE_APPLICATION' AND tags_json LIKE ? ESCAPE '\\' "
"ORDER BY id DESC LIMIT ?",
- (f'%"rule:{rule_id}"%', limit),
+ (like_pattern, limit),
).fetchall()Also applies to: 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/src/gradata/rules/rule_tracker.py` around lines 162 - 165, The LIKE
pattern uses rule_id directly which allows % and _ to act as wildcards; update
the SQL to escape special LIKE characters in rule_id (escape backslash first,
then replace % -> \% and _ -> \_) and build the pattern as
'%"rule:{escaped_rule_id}"%' while using the ESCAPE '\' clause in the query;
apply the same escape logic for the second occurrence noted in the file so
tags_json LIKE ? becomes tags_json LIKE ? ESCAPE '\' with the escaped parameter
substituted.
| content = f"""--- | ||
| id: {lesson.category}_{hash(lesson.description) % 10000:04d} | ||
| id: {lesson.category}_{zlib.crc32(lesson.description.encode("utf-8")) % 10000:04d} |
There was a problem hiding this comment.
Frontmatter IDs still collide too easily.
zlib.crc32(lesson.description) % 10000 only gives 10k buckets and ignores category/path, so distinct lessons can export with the same id. That makes the vault’s only stable identifier unsafe for backlinks or external sync consumers.
Possible fix
-id: {lesson.category}_{zlib.crc32(lesson.description.encode("utf-8")) % 10000:04d}
+id: {lesson.category}_{zlib.crc32(f"{lesson.category}:{lesson.description}".encode("utf-8")):08x}📝 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.
| content = f"""--- | |
| id: {lesson.category}_{hash(lesson.description) % 10000:04d} | |
| id: {lesson.category}_{zlib.crc32(lesson.description.encode("utf-8")) % 10000:04d} | |
| content = f"""--- | |
| id: {lesson.category}_{zlib.crc32(f"{lesson.category}:{lesson.description}".encode("utf-8")):08x} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/src/gradata/rules/rule_tree.py` around lines 379 - 380, The
frontmatter id generation using zlib.crc32(lesson.description) % 10000 in the
content string is too small and ignores category/path, causing collisions; fix
by generating a stable, larger-space id that incorporates both lesson.category
and lesson.description (and optionally lesson.path) such as using full crc32 hex
or a namespaced UUID (e.g., uuid5) over the combined string, and replace the
current f-string id expression (the content variable that references
lesson.category and zlib.crc32) with the new deterministic id generator so
exported ids are unique across categories and descriptions.
| # A system message should have been inserted in the cloned list. | ||
| call_messages = original_create.call_args.kwargs["messages"] | ||
| assert call_messages[0]["role"] == "system" | ||
| assert "RULE: Be brief" in call_messages[0]["content"] | ||
| # Caller's own list must remain untouched. | ||
| assert messages == [{"role": "user", "content": "Hello"}] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one regression test for positional messages routing branch.
patched_create has a distinct positional rewrite path in Gradata/src/gradata/integrations/openai_adapter.py Line 77-Line 78. A dedicated test for that branch would prevent silent regressions there.
✅ Suggested test addition
class TestOpenAIAdapter:
@@
def test_creates_system_message_when_missing(self, fresh_brain):
@@
assert messages == [{"role": "user", "content": "Hello"}]
+
+ def test_rules_injected_with_positional_messages(self, fresh_brain):
+ """Rules are injected when create() receives positional messages arg."""
+ from gradata.integrations.openai_adapter import patch_openai
+
+ client, _ = self._make_client()
+ original_create = client.chat.completions.create
+ patched = patch_openai(client, brain_dir=fresh_brain.dir)
+ patched._brain.apply_brain_rules = MagicMock(return_value="RULE: Positional")
+
+ original_messages = [{"role": "user", "content": "Hello"}]
+ patched.chat.completions.create("gpt-4", original_messages)
+
+ sent_messages = original_create.call_args.args[1]
+ assert sent_messages[0]["role"] == "system"
+ assert "RULE: Positional" in sent_messages[0]["content"]
+ assert original_messages == [{"role": "user", "content": "Hello"}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gradata/tests/test_integrations.py` around lines 97 - 102, Add a regression
test that invokes patched_create using the positional messages argument (not
kwargs) to hit the positional routing branch in patched_create in openai_adapter
(the rewritten path around patched_create), and assert that the call forwarded
to original_create contains a system message inserted at index 0 while the
caller's original messages list (e.g. messages =
[{"role":"user","content":"Hello"}]) remains unchanged; specifically call
patched_create(messages) (positional) and inspect original_create.call_args.args
or the first positional arg to verify the system role/content and that messages
is still the original list.
1 participant
Summary
Stack of SDK fixes and refactors on top of #133.
fix(implicit_feedback): restore GAP signal category dropped during hook dedup (12e96d3)feat(implicit_feedback): emit tacitOUTPUT_ACCEPTEDon silent follow-ups (ab0bb62)fix(hooks): eliminate false-positive REJECTED outcomes + dead code (3981939)fix(cloud): align SDK base URL with/api/v1prefix after gradata-cloud@04a272f (1f89df4)refactor(hooks): collapse emit boilerplate into_base.emit_hook_event— 5 hooks migrated, net -13 lines (03e5363)Test plan
pytest Gradata/tests/green/api/v1/telemetry/metrics(tested intest_cloud_sync.py)api.gradata.aiafter mergeGenerated with Gradata