Fix/skill mcp - #68
Merged
Merged
Conversation
The operator-aware ftsQuery parser (AND/OR/NOT, column filters, prefix terms, NEAR, syntax validation) was only exercised by unit tests; the runtime keyword path used the simpler localFtsQuery, so documented advanced syntax was silently ignored and syntax errors never surfaced. - local-backend: route searchLocalKeywordExact through ftsQuery so advanced operators, column filters, and syntax validation work as documented; delete the now-dead localFtsQuery - local-backend: fix NEAR passthrough — tokenizeFtsQuery now captures NEAR(...) as a unit and normalizes the forgiving comma form (NEAR(a, b, 5)) to FTS5's space-separated form (NEAR(a b, 5)) - local-backend: treat a leading hyphen as punctuation in simple mode (e.g. "sqlite -wal" searches for both terms) instead of rejecting it - local-backend: correct the FTS5 syntax hint (NEAR uses space-separated terms, not commas) - server: surface FtsQuerySyntaxError as text content instead of re-throwing, so MCP clients can read the hint and retry - index: special-case FtsQuerySyntaxError in the CLI catch blocks with a clearer "Invalid search syntax" header - search-solutions.md: show the canonical NEAR(token nft, 5) form
…gers Phase 1 — Rewrite skill descriptions to fix undertriggering: - Lead with 'BEFORE implementing' instead of 9 failure nouns - Name the real competitor: 'The search cost is near-zero; the cost of rediscovering a known gotcha is high' - Invert log default from 'only if novel' to 'if it took effort, log it' - Add 'specific technical fingerprint' test to skip guidance - Collapse three exclusion lists into one tight bar - Behavioral eval: recall 0% -> 100%, precision 60% -> 67% Phase 2 — Ambient hooks that fire on failure signals: - post-tool-use.mjs: detects error codes, stack traces, build failures; extracts fingerprint and nudges agent to search; silent on success; debounced to avoid noise during iterative debugging - session-start.mjs: primes agent at session start - hooks.json: rewritten from flat type:prompt (inert) to native type:command object schema (Claude/Codex/ZCode compatible) - clanker hook CLI: generates/installs config per harness with idempotent merge that preserves existing user hooks - setup.ts: wired to install hooks alongside MCP + skills for Claude, Codex, and Cursor Blog draft documenting methodology and results. CLI bumped 1.3.1 -> 1.4.0
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds ambient hook scripts and install wiring, updates FTS5 query parsing and syntax handling, rewrites ClankerOverflow skill guidance, bumps CLI/plugin versions to 1.4.0, and adds a blog draft about the rollout. ChangesAmbient hooks, FTS query updates, and release content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
blog/drafts/making-agents-actually-use-your-tools.md (2)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an alternative for "exactly" to add variety.
Line 43 uses "exactly" in "are exactly what ClankerOverflow remembers." This is stylistically fine, but the word is flagged as overused in the broader text. Consider "are precisely what," "capture," or "represent" for wordsmithing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blog/drafts/making-agents-actually-use-your-tools.md` at line 43, Replace the word "exactly" in the phrase "are exactly what ClankerOverflow remembers" with a less frequently used alternative to reduce word repetition and improve readability. Choose from alternatives such as "precisely," "capture," or "represent" based on which best maintains the intended meaning and flow of the sentence while adding stylistic variety to the paragraph.Source: Linters/SAST tools
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
The markdown code block is missing a language identifier. Since it appears to be a prompt/message example (not executable code), use
textas the language specifier for proper rendering.💬 Proposed fix
-``` +```text ClankerOverflow: A failure signal was detected. Before re-debugging, search for prior fixes with: search_solutions(e.g. "EADDRINUSE") A reusable verified fix may already exist. The search cost is ~2 seconds; rediscovering a known gotcha costs far more. -``` +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blog/drafts/making-agents-actually-use-your-tools.md` at line 88, The fenced code block containing the ClankerOverflow failure signal message is missing a language specifier on the opening fence. Locate the code block that starts with the message "ClankerOverflow: A failure signal was detected..." and add the language identifier `text` immediately after the opening three backticks on the first line to properly specify that this is a text block rather than executable code.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/hooks/post-tool-use.mjs`:
- Around line 50-53: Replace the overly broad string literal "at " in the
FAILURE_INDICATORS array with a more specific regex pattern that matches actual
JavaScript stack frame formats (such as patterns like "at function_name" with
proper word boundaries or stack trace line patterns). Apply this same change in
both locations where the issue appears: in the primary FAILURE_INDICATORS
definition and again in the duplicate check around lines 166-171, ensuring the
regex pattern is strict enough to avoid matching normal conversational text
while still catching legitimate error stack traces.
In `@packages/cli/src/hooks/install.ts`:
- Around line 237-239: The condition checking both "command" and "type"
properties in the if statement is too restrictive and prevents Cursor command
entries (which only have "command") from being marked as owned with the
_clankeroverflow flag. This causes removeOwn() to fail to prune prior entries on
reinstalls, breaking idempotency. Modify the condition to mark entries as owned
when they have the "command" property regardless of whether "type" exists,
ensuring both regular and Cursor entries are properly tracked and deduplicated
across repeated installations.
In `@packages/cli/src/index.ts`:
- Around line 727-749: The hook installation results are being printed but the
code always exits successfully regardless of whether any installations failed.
After the results loop that checks result.status for "configured", "skipped",
and "failed" values, add a check to see if any result has status equal to
"failed". If any failures are detected, exit the process with a non-zero exit
code (such as 1) using process.exit before the return statement, otherwise allow
the normal return to proceed. This ensures that failed hook installations are
properly signaled to the CLI caller for CI/scripting failure detection.
In `@packages/cli/src/mcp/local-backend.ts`:
- Around line 137-150: The normalizeNear function currently splits on commas
without accounting for quoted strings, which causes valid NEAR queries
containing commas or other delimiters inside quoted terms to be incorrectly
parsed or prematurely terminated. Modify the parsing logic in normalizeNear to
be quote-aware by tracking whether the parser is currently inside a quoted
string when iterating through the inner content, and only treat commas and
parentheses as structural delimiters when they appear outside of quotes. Apply
this same quote-aware parsing logic to all delimiter handling in the function,
including the sections referenced around lines 160-172.
---
Nitpick comments:
In `@blog/drafts/making-agents-actually-use-your-tools.md`:
- Line 43: Replace the word "exactly" in the phrase "are exactly what
ClankerOverflow remembers" with a less frequently used alternative to reduce
word repetition and improve readability. Choose from alternatives such as
"precisely," "capture," or "represent" based on which best maintains the
intended meaning and flow of the sentence while adding stylistic variety to the
paragraph.
- Line 88: The fenced code block containing the ClankerOverflow failure signal
message is missing a language specifier on the opening fence. Locate the code
block that starts with the message "ClankerOverflow: A failure signal was
detected..." and add the language identifier `text` immediately after the
opening three backticks on the first line to properly specify that this is a
text block rather than executable code.
🪄 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: CHILL
Plan: Pro
Run ID: 99a611b6-9c26-4dbd-8282-304ccffd3fae
📒 Files selected for processing (18)
blog/drafts/making-agents-actually-use-your-tools.mdpackages/cli/.claude-plugin/plugin.jsonpackages/cli/.codex-plugin/plugin.jsonpackages/cli/commands/search-solutions.mdpackages/cli/hooks/post-tool-use.mjspackages/cli/hooks/session-start.mjspackages/cli/openclaw.plugin.jsonpackages/cli/package.jsonpackages/cli/skills/clankeroverflow-cli/SKILL.mdpackages/cli/skills/clankeroverflow-mcp/SKILL.mdpackages/cli/src/hooks/install.tspackages/cli/src/index.tspackages/cli/src/mcp/local-backend.test.tspackages/cli/src/mcp/local-backend.tspackages/cli/src/mcp/server.test.tspackages/cli/src/mcp/server.tspackages/cli/src/setup.tsskills/clanker-overflow/SKILL.md
Comment on lines
+137
to
+150
| function normalizeNear(raw: string): string { | ||
| const match = raw.match(/^NEAR\s*\((.*)\)$/is); | ||
| if (!match) ftsSyntaxError("malformed NEAR(...) expression."); | ||
| const inner = match[1]!.trim(); | ||
| if (!inner) ftsSyntaxError("NEAR(...) needs at least one term."); | ||
| // Split on commas, drop empties, so "a, b, 5" -> ["a","b","5"] and "a b, 5" -> ["a b","5"]. | ||
| const parts = inner | ||
| .split(",") | ||
| .map((part) => part.trim()) | ||
| .filter(Boolean); | ||
| // A trailing bare integer is the NEAR distance; keep it after a comma. | ||
| const last = parts[parts.length - 1]; | ||
| const distance = parts.length >= 2 && /^\d+$/.test(last!) ? `, ${last}` : ""; | ||
| const terms = (distance ? parts.slice(0, -1) : parts).join(" ").trim(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make NEAR(...) parsing quote-aware before comma/paren handling.
Current parsing treats , and ) inside quoted NEAR terms as structural delimiters, so valid queries can be transformed incorrectly or prematurely terminated.
Proposed fix
function normalizeNear(raw: string): string {
const match = raw.match(/^NEAR\s*\((.*)\)$/is);
if (!match) ftsSyntaxError("malformed NEAR(...) expression.");
const inner = match[1]!.trim();
if (!inner) ftsSyntaxError("NEAR(...) needs at least one term.");
- const parts = inner
- .split(",")
- .map((part) => part.trim())
- .filter(Boolean);
+ const parts = splitNearParts(inner)
+ .map((part) => part.trim())
+ .filter(Boolean);
const last = parts[parts.length - 1];
const distance = parts.length >= 2 && /^\d+$/.test(last!) ? `, ${last}` : "";
const terms = (distance ? parts.slice(0, -1) : parts).join(" ").trim();
if (!terms) ftsSyntaxError("NEAR(...) needs at least one term.");
return `NEAR(${terms}${distance})`;
}
+function splitNearParts(inner: string): string[] {
+ const parts: string[] = [];
+ let current = "";
+ let inQuotes = false;
+ for (let i = 0; i < inner.length; i += 1) {
+ const ch = inner[i]!;
+ if (ch === '"' && inner[i - 1] !== "\\") inQuotes = !inQuotes;
+ if (ch === "," && !inQuotes) {
+ parts.push(current);
+ current = "";
+ continue;
+ }
+ current += ch;
+ }
+ if (inQuotes) ftsSyntaxError('unterminated double-quoted phrase in NEAR(...).');
+ parts.push(current);
+ return parts;
+}
+
function captureNear(query: string, parenIndex: number): { value: string; endIndex: number } {
let depth = 0;
+ let inQuotes = false;
let j = parenIndex;
for (; j < query.length; j += 1) {
const char = query[j]!;
+ if (char === '"' && query[j - 1] !== "\\") {
+ inQuotes = !inQuotes;
+ continue;
+ }
+ if (inQuotes) continue;
if (char === "(") depth += 1;
else if (char === ")") {
depth -= 1;
if (depth === 0) break;
}
}
+ if (inQuotes) ftsSyntaxError('unterminated double-quoted phrase in NEAR(...).');
if (depth !== 0) ftsSyntaxError('unmatched "(" in NEAR(...) expression.');
return { value: query.slice(parenIndex - 4, j + 1), endIndex: j };
}Also applies to: 160-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/mcp/local-backend.ts` around lines 137 - 150, The
normalizeNear function currently splits on commas without accounting for quoted
strings, which causes valid NEAR queries containing commas or other delimiters
inside quoted terms to be incorrectly parsed or prematurely terminated. Modify
the parsing logic in normalizeNear to be quote-aware by tracking whether the
parser is currently inside a quoted string when iterating through the inner
content, and only treat commas and parentheses as structural delimiters when
they appear outside of quotes. Apply this same quote-aware parsing logic to all
delimiter handling in the function, including the sections referenced around
lines 160-172.
- post-tool-use: replace over-broad "at " substring (matched prose like
"look at this") with structure-anchored stack-frame regexes so failure
detection no longer false-positives on user prompts.
- install: markOwn required a "type" field, which Cursor hook entries
({command, matcher}) lack, so re-installs produced duplicates. Gate on
"command" only, matching removeOwn. Export markOwn/removeOwn/
mergeCursorHooks and add a regression test that fails against the old
behavior.
- index: exit non-zero (1) when any installHooks result has a failed
status, so CI/scripts can detect failed installs.
Owner
Author
|
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
clanker hookcommand to generate and optionally install ambient hooks, plus a session-start reminder