feat: add learned repo memory and MCP product-proof evals - #69
Open
bernoussama wants to merge 7 commits into
Open
feat: add learned repo memory and MCP product-proof evals#69bernoussama wants to merge 7 commits into
bernoussama wants to merge 7 commits into
Conversation
WalkthroughThe changes add verified solution learning across the CLI and MCP server, repository solution resources, benchmark and recorder workflows, Pi search reminders, failure-resolution hooks, debug fixtures, reports, and updated skill and plugin guidance. ChangesLearning and MCP integration
Evaluation and product-proof workflows
Hooks and agent setup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
@coderabbitai review Generated benchmark summaries and raw run captures were removed. The PR is now 148 changed files and 14,237 added lines, below the automated review limits. |
|
ヽ(・∀・)ﾉ ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
clankeroverflow-mcp-workspace/product-proof/README.md-11-12 (1)
11-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the scenario count in README.md
clankeroverflow-mcp-workspace/product-proof/README.md:11-12saysscenarios.jsonhas 30 prompts, but the file now contains 38; align the doc with the current fixture set or note that it refers to an older revision.🤖 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 `@clankeroverflow-mcp-workspace/product-proof/README.md` around lines 11 - 12, Update the scenarios.json description in the product-proof README to reflect the current 38 prompts, or explicitly label the documented count as belonging to an older revision.clankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/response.md-28-30 (1)
28-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced dotenv example.
Markdownlint MD040 flags Line 28. Use a language such as
textordotenvafter the opening fence.🤖 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 `@clankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/response.md` around lines 28 - 30, Add a language identifier to the opening fence of the dotenv example containing DIRECT_URL, using text or dotenv to satisfy Markdownlint MD040.Source: Linters/SAST tools
packages/cli/src/evals/repo-stackoverflow.ts-190-194 (1)
190-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject missing option values instead of treating the next flag as a path.
For example,
--output-json --workspace-root /tmpassigns--workspace-rootas the JSON output path. Validate that the next argument exists and does not start with--, or fail with a clear usage error.🤖 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/evals/repo-stackoverflow.ts` around lines 190 - 194, The parseArg function currently accepts another flag as an option value; validate that index + 1 exists and the next argument does not start with “--”. If validation fails, raise a clear usage error instead of returning the flag as a path.clankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r2.md-54-60 (1)
54-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix or remove the port leak-check loop.
ss -ltnonly checks listening sockets, notTIME_WAIT, and the! ... || { ... break; }condition breaks on the first successfulssinvocation regardless of whether a matching socket exists. This loop neither waits nor validates the suspected failure state.🤖 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 `@clankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r2.md` around lines 54 - 60, Fix or remove the port leak-check loop in the report methodology: replace the flawed ss -ltn condition with an explicit check that validates whether the target port is present and, if retaining the loop, waits and retries until the intended state is reached; otherwise remove the loop and document the limitation that listening-socket checks do not detect TIME_WAIT.clankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r2.md-38-42 (1)
38-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t describe this as an explicit
SO_REUSEADDRfix. The sample only callsserver.listen(...); it doesn’t set a reuse option. Since Node already enablesSO_REUSEADDRon net servers, this wording overstates the change and implies the bind issue is resolved when it isn’t.🤖 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 `@clankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r2.md` around lines 38 - 42, The report incorrectly characterizes the change as an explicit SO_REUSEADDR fix. Revise the affected wording to state only that the server uses server.listen(...) and avoid claiming reuse options were set or that the bind issue was resolved.packages/cli/src/evals/record-codex-product-proof.test.ts-54-59 (1)
54-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the final
enabledvalue forwithout_mcp.
The test only checks that bothmcp_servers.clankeroverflow.enabled=trueand...=falseare present, so it still passes if the override order changes. Assert that...=falseis the finalclankeroverflow.enabledentry.🤖 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/evals/record-codex-product-proof.test.ts` around lines 54 - 59, Update the “disables ClankerOverflow for the without_mcp config” test to verify the final clankeroverflow enabled override, rather than merely checking both values are present. Use the args produced by codexArgs and assert that the last mcp_servers.clankeroverflow.enabled entry is exactly false.clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.md-13-15 (1)
13-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
node10doesn’t supportexports
moduleResolution: "node"/"node10"is legacy and should not be described as partially resolvingpackage.jsonexports; if the package relies onexports, usenode16,nodenext, orbundlerinstead.🤖 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 `@clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.md` around lines 13 - 15, Correct the module-resolution comparison in the referenced guidance: update the description of moduleResolution "node"/"node10" to state that it does not support package.json exports, rather than partially resolving them, and direct exports-based packages to use "node16", "nodenext", or "bundler".clankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/test.mjs-3-5 (1)
3-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit the assertion to the
createBranchimplementation.The current regex matches any
return sqlappearing anywhere aftercreateBranch(), including a later function, so valid fixtures can fail. Scope the check to the function body or use a more precise structural/runtime assertion.🤖 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 `@clankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/test.mjs` around lines 3 - 5, Refine the assertion in the test fixture so it inspects only the body of the createBranch implementation, rather than any later return sql in the file. Update the regex to capture the createBranch function body or replace it with a precise structural/runtime check, and assert only that this implementation does not immediately execute a query after branch creation.clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.md-24-60 (1)
24-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the pnpm vs TypeScript boundary
Atclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.md:24-60, separate pnpm’s dependency/linking requirement from TypeScript’s module resolution. Declaringworkspace:*makes the package available to the consuming workspace; TypeScript then readsexports/types/mainfrom that package, and built.d.tsfiles are only required when the package ships compiled types.🤖 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 `@clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.md` around lines 24 - 60, Clarify the separation between pnpm and TypeScript: state that adding the workspace:* dependency in the consuming package enables pnpm linking, after which TypeScript resolves the package using its package.json fields such as exports, types, and main. Revise the guidance around built .d.ts files to say they are required only when the dependency ships compiled declarations, rather than implying TypeScript always requires a build; preserve the relevant verification steps.packages/cli/src/mcp/server.ts-484-487 (1)
484-487: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winTighten repo-solution lookup to exact matches.
readSolutionResource()only reads fromlistRepoSolutionFiles(), so this isn’t a path-traversal vector, butcandidate.includes(idOrSlug)can resolve the wrong note for partial ids/slugs. Match the basename exactly (or key by basename) instead.
packages/cli/src/learn.ts:444-450🤖 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/server.ts` around lines 484 - 487, Update readSolutionResource to select a repository solution only when its basename exactly equals the requested idOrSlug, replacing the partial candidate.includes(idOrSlug) lookup with an exact basename comparison or basename-keyed lookup; preserve the existing not-found behavior and ensure the MCP resource handler continues using readSolutionResource.packages/cli/src/evals/record-codex-product-proof.ts-400-407 (1)
400-407: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck
result.errorfor spawn failures (e.g., ENOENT).When
spawnSyncfails to spawn the process (codex binary not found),result.erroris set andresult.statusisnull. The code enters the error branch viaresult.status !== 0, but the error message is built only fromresult.stderrand event errors — both empty on ENOENT. The resulting message"Codex exec failed with status unknown."doesn't indicate the binary was missing.🛡️ Proposed fix to include spawn error in error output
if (result.status !== 0) { + const spawnError = result.error?.message ?? ""; const stderrExcerpt = (result.stderr ?? "").split(/\r?\n/).slice(-12).join("\n").trim(); const eventErrors = eventErrorMessages(eventsPath).join("\n").trim(); - const errorText = [eventErrors, stderrExcerpt].filter(Boolean).join("\n"); + const errorText = [spawnError, eventErrors, stderrExcerpt].filter(Boolean).join("\n"); return { scenario_id: scenario.id, config, repetition, status: "failed", error: errorText, - transcript: `Codex exec failed with status ${result.status ?? "unknown"}.\n${errorText}`, + transcript: `Codex exec failed with status ${result.status ?? "unknown"}.\n${errorText}`, usage,🤖 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/evals/record-codex-product-proof.ts` around lines 400 - 407, Handle spawn failures explicitly in the code surrounding the spawnSync invocation: check result.error and include its details in the constructed failure message before or alongside status and stderr handling. Update the error branch so ENOENT and other process-launch errors identify the underlying spawn error instead of reporting only an unknown status.Source: Linters/SAST tools
packages/cli/src/learn.ts-82-83 (1)
82-83: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winEnv var redaction misses quoted values.
The regex value group
[^\s"']+excludes quote characters, so common patterns likeDATABASE_URL="postgres://..."orAPI_KEY='sk_...'are not redacted. Only unquoted values likeFOO=bar` are caught.🛡️ Proposed fix to also match quoted env var values
- [/\b([A-Z][A-Z0-9_]{2,})=([^\s"'`]+)/g, "$1=<redacted>", "env var value"], + [/\b([A-Z][A-Z0-9_]{2,})=("[^"]*"|'[^']*'|[^\s"'`]+)/g, "$1=<redacted>", "env var value"],🤖 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/learn.ts` around lines 82 - 83, Update the env-var replacement regex in the `replacements` array to match both unquoted values and single- or double-quoted values, while preserving the existing variable-name matching and replacing the entire value with `<redacted>`.packages/cli/src/learn.ts-300-348 (1)
300-348: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the backend in
learnSolution.createSolutionBackend()opens a newLocalBackend/SQLite handle per call, butSolutionBackendhas no cleanup path. In the MCP server, repeatedlearn_solutioncalls can leave handles open; add aclose()/dispose()hook and call it in afinallyblock, or reuse a shared backend.🤖 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/learn.ts` around lines 300 - 348, Ensure learnSolution always releases the backend created by createSolutionBackend: add a close/dispose lifecycle method to the SolutionBackend interface and its implementations, then wrap the duplicate-checking, logging, and mirror-writing flow in a try/finally that invokes the backend cleanup method. Preserve existing return behavior while guaranteeing cleanup on success, duplicates, and errors.clankeroverflow-mcp-workspace/iteration-1/grade_and_aggregate.py-286-290 (1)
286-290: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDelta formatting breaks for negative deltas.
f"+{...:.2f}"always prepends+, so a negative delta renders as"+-0.15"instead of"-0.15". This string is written tobenchmark.jsonand consumed byreview.html'sdeltaClass(), which doesparseFloat(val)—parseFloat("+-0.15")isNaN, silently dropping the positive/negative color coding for that cell.🐛 Suggested fix
- "delta": {"pass_rate": f"+{stats(ws_runs)['mean'] - stats(wo_runs)['mean']:.2f}"}, + "delta": {"pass_rate": f"{stats(ws_runs)['mean'] - stats(wo_runs)['mean']:+.2f}"},🤖 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 `@clankeroverflow-mcp-workspace/iteration-1/grade_and_aggregate.py` around lines 286 - 290, Fix the delta formatting in the run_summary construction by replacing the unconditional “+” prefix with sign-aware formatting, so positive values render with “+”, negative values with “-”, and zero consistently; ensure the resulting string remains parseable by review.html’s deltaClass() via parseFloat.
🧹 Nitpick comments (5)
packages/cli/src/mcp/server.ts (2)
466-477: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider async file reads in the
listcallback to avoid blocking the event loop.
readFileSyncis called synchronously inside the asynclistcallback for every solution file. For repos with many learned solutions, this blocks the event loop during enumeration. Switching tofs/promisesreadFilewould be more consistent with the async handler pattern.🤖 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/server.ts` around lines 466 - 477, Update the MCP resource `list` callback to use asynchronous file reads instead of `readFileSync`. Import `readFile` from `fs/promises`, make the mapping await each file read and parse its contents, and use `Promise.all` so `list` returns all resource entries without blocking the event loop.
466-477: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
path.basenameinstead offile.split("/")for cross-platform compatibility.
file.split("/").at(-1)won't extract the filename correctly on Windows where paths use backslashes. Usepath.basename(file, ".md")for robust cross-platform behavior.♻️ Proposed fix
+import { basename } from "node:path"; + // ... in the list callback: resources: listRepoSolutionFiles().map((file) => { const parsed = parseLearnMarkdown(readFileSync(file, "utf8")); - const name = file.split("/").at(-1)?.replace(/\.md$/, "") ?? file; + const name = basename(file, ".md"); return {🤖 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/server.ts` around lines 466 - 477, Replace the manual filename extraction in the MCP resource list callback with path.basename(file, ".md"), ensuring the path module is imported and the existing fallback behavior is preserved if needed.packages/cli/src/evals/record-codex-product-proof.ts (1)
347-353: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse shell-safe single-quote escaping for path substitution in
verifyWorkspace.
spawnSyncwithshell: trueand a string command is a command-injection surface.JSON.stringifyproduces double-quoted strings, but shell double quotes still interpret$, backticks, and!. While the verification command comes from trusted fixture data andworkspacePathis frommkdtempSync,benchmarkWorkspaceDiris user-controlled via--workspace. Switching to single-quote shell escaping eliminates all metacharacter interpretation.🔒️ Proposed fix using single-quote shell escaping
function verifyWorkspace(scenario: Scenario, benchmarkWorkspaceDir: string, workspacePath: string) { if (scenario.task_type !== "debug_workspace" || !scenario.verification_command) { return {}; } - const quote = (value: string) => JSON.stringify(value); + const quote = (value: string) => `'${value.replaceAll("'", "'\\''")}'`; const command = scenario.verification_command .replaceAll("{workspace}", quote(workspacePath)) .replaceAll("{workspaceDir}", quote(benchmarkWorkspaceDir));🤖 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/evals/record-codex-product-proof.ts` around lines 347 - 353, Update verifyWorkspace and the command construction before spawnSync to shell-escape benchmarkWorkspaceDir/workspacePath with single quotes, replacing each embedded single quote with the standard '\'' sequence. Do not use JSON.stringify or double-quoted substitutions for user-controlled paths, while preserving shell: true and the existing command behavior.Source: Linters/SAST tools
packages/cli/src/learn.ts (1)
379-413: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExport loses structured fields during round-trip.
parseStructuredSolutionreturns the entire "Reusable Context" section asrepoNote, but doesn't extractframework,runtime,packageManager, orfingerprintsback into individual fields. After export → re-import viasyncRepoSolutions, these fields are empty — degrading duplicate detection (falls back to problem text instead of fingerprints) and tag normalization.♻️ Proposed fix to parse individual fields from Reusable Context
function parseStructuredSolution(solution: string) { + const context = markdownSection(solution, "Reusable Context"); + const extractItem = (label: string) => { + const match = context.match(new RegExp(`^- ${label}:\\s*(.+)$`, "m")); + return match?.[1]?.trim() ?? ""; + }; return { rootCause: markdownSection(solution, "Root Cause") || "See verified fix.", solution: markdownSection(solution, "Verified Fix") || solution, verification: markdownSection(solution, "Verification") || "Previously logged in ClankerOverflow.", - repoNote: markdownSection(solution, "Reusable Context"), + repoNote: extractItem("Repo note"), + framework: extractItem("Framework"), + runtime: extractItem("Runtime"), + packageManager: extractItem("Package manager"), + fingerprints: extractItem("Fingerprints"), }; }Then update
exportLocalSolutionsto pass the extracted fields towriteLearnMarkdown:return writeLearnMarkdown(repoRoot, row.id, { problem: row.problem, rootCause: parsed.rootCause, solution: parsed.solution, verification: parsed.verification, repoNote: parsed.repoNote, + framework: parsed.framework, + runtime: parsed.runtime, + packageManager: parsed.packageManager, + fingerprints: parsed.fingerprints, tags: row.tags ?? "clankeroverflow", });🤖 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/learn.ts` around lines 379 - 413, Update parseStructuredSolution to parse framework, runtime, packageManager, and fingerprints individually from the “Reusable Context” section, while retaining repoNote as needed. In exportLocalSolutions, pass these parsed fields to writeLearnMarkdown so export→syncRepoSolutions preserves duplicate-detection fingerprints and tag normalization metadata.packages/cli/src/evals/product-proof.ts (1)
410-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing literal-key fallback in
pricingForModel.
pricing?.modelreads the literal property named"model"on the pricing config — it is NOTpricing?.[model](the parameter). This is confirmed intentional (the test suite passespricing: { model: {...} } as a generic default rate), but the dot-notation makes it read exactly like a copy/paste bug where[model]was intended. Consider renaming the special key (e.g.pricing?.defaultis already used right after it — two different "default" mechanisms is itself confusing) or adding a comment clarifying that"model"` is a magic fallback key, not a dynamic lookup.🤖 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/evals/product-proof.ts` around lines 410 - 412, Clarify the intentional literal-key fallback in pricingForModel by adding a concise comment identifying pricing.model as the magic generic default rate, distinct from the dynamic pricing[model] lookup and pricing.default fallback; preserve the existing fallback behavior.
🤖 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
`@clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.md`:
- Around line 52-58: Clarify the package exports example so the "./src/index.ts"
target is explicitly limited to bundlers or configured TypeScript loaders. For
stock Node consumers, show runtime exports targeting compiled ".js" files and
type exports targeting ".d.ts" declarations, and update the surrounding
explanation accordingly.
In
`@clankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost-hidden-r2.md`:
- Around line 9-19: Reconcile the report’s 24-sample recall denominators with
the current scenario catalog and trigger-eval-set.json inputs. Regenerate the
metrics from those inputs, or document the exact dataset and revision that
produced 24 cases, and update the report’s headline metrics so every denominator
is reproducible.
In
`@clankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-prisma-neon.mjs`:
- Around line 4-7: Update the `directUrl` validation in the schema-checking
script to capture the environment variable used by the `directUrl = env(...)`
assignment, then validate that captured value against `DIRECT_URL`,
`DIRECT_DATABASE_URL`, or `DATABASE_DIRECT_URL`. Remove the file-wide
environment-variable regex so unrelated comments or fields cannot satisfy the
check.
In
`@clankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-react-query-rollback.mjs`:
- Line 5: Fix the regex in the verifier’s source check by grouping the
alternatives so “context” is only accepted as part of the intended return
pattern, rather than matching anywhere in the file. Update the condition around
the existing source test and preserve case-insensitive matching.
In
`@clankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-sentry-sourcemaps.mjs`:
- Around line 5-6: The verifier currently accepts unrelated text containing
configuration keywords; update the validation in the sourcemap verifier to
assert the expected property/value combinations for hidden sourcemaps,
post-upload file deletion, and release configuration. Replace the broad regex
check with targeted patterns or parsed configuration checks that cannot be
satisfied by comments, arbitrary strings, or variable names.
In
`@clankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-stripe-workers.mjs`:
- Around line 5-8: Update the verifier assertions around the fixture source
checks to validate the actual webhook implementation rather than arbitrary token
presence: inspect or execute the handler path to confirm it calls Stripe’s
asynchronous event-construction API with the raw request body, while still
rejecting synchronous constructEvent usage. Replace the broad checks in the
debug-stripe-workers verifier with assertions tied to the handler’s call and
raw-body expression.
In `@packages/cli/skills/clankeroverflow-cli/SKILL.md`:
- Around line 72-101: Pin `@clankeroverflow/cli` to an approved explicit version
in every new npx example, including the learn and log command snippets in the
SKILL.md section. Replace the unversioned package references consistently and
preserve the existing command options and guidance.
In `@packages/cli/src/evals/product-proof.ts`:
- Around line 678-701: Remove or broaden the duplicate headline metrics in the
report: update the calculations and output references around mustSearchRecall,
knownFixTriggerRecall, usefulRetrievalRate, and knownFixRetrievalRate so
identical values are not presented under different labels. Prefer widening one
metric’s scope to represent a distinct population; otherwise remove the
redundant rows and keep a single canonical label for each signal.
In `@packages/cli/src/evals/repo-stackoverflow.ts`:
- Around line 138-143: Update the leakage measurement around unsafeText to
include the generated repository note and the solution content returned by
pass-2 search, rather than only the controlled expoScenario fields. Combine
these persisted and retrieved artifacts with the scenario text before applying
the existing unsafeText regular-expression check.
In `@packages/cli/src/index.ts`:
- Around line 294-311: Propagate parent learn options into the sync subcommand
instead of allowing child defaults to override them. Update the sync command
definition and its option-reading logic to avoid redeclaring defaulted --source
and --no-dedupe values, or explicitly resolve those options from the parent
learn command, preserving values supplied to learn before sync.
---
Minor comments:
In `@clankeroverflow-mcp-workspace/iteration-1/grade_and_aggregate.py`:
- Around line 286-290: Fix the delta formatting in the run_summary construction
by replacing the unconditional “+” prefix with sign-aware formatting, so
positive values render with “+”, negative values with “-”, and zero
consistently; ensure the resulting string remains parseable by review.html’s
deltaClass() via parseFloat.
In
`@clankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/response.md`:
- Around line 28-30: Add a language identifier to the opening fence of the
dotenv example containing DIRECT_URL, using text or dotenv to satisfy
Markdownlint MD040.
In
`@clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.md`:
- Around line 24-60: Clarify the separation between pnpm and TypeScript: state
that adding the workspace:* dependency in the consuming package enables pnpm
linking, after which TypeScript resolves the package using its package.json
fields such as exports, types, and main. Revise the guidance around built .d.ts
files to say they are required only when the dependency ships compiled
declarations, rather than implying TypeScript always requires a build; preserve
the relevant verification steps.
In
`@clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.md`:
- Around line 13-15: Correct the module-resolution comparison in the referenced
guidance: update the description of moduleResolution "node"/"node10" to state
that it does not support package.json exports, rather than partially resolving
them, and direct exports-based packages to use "node16", "nodenext", or
"bundler".
In `@clankeroverflow-mcp-workspace/product-proof/README.md`:
- Around line 11-12: Update the scenarios.json description in the product-proof
README to reflect the current 38 prompts, or explicitly label the documented
count as belonging to an older revision.
In
`@clankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r2.md`:
- Around line 54-60: Fix or remove the port leak-check loop in the report
methodology: replace the flawed ss -ltn condition with an explicit check that
validates whether the target port is present and, if retaining the loop, waits
and retries until the intended state is reached; otherwise remove the loop and
document the limitation that listening-socket checks do not detect TIME_WAIT.
- Around line 38-42: The report incorrectly characterizes the change as an
explicit SO_REUSEADDR fix. Revise the affected wording to state only that the
server uses server.listen(...) and avoid claiming reuse options were set or that
the bind issue was resolved.
In
`@clankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/test.mjs`:
- Around line 3-5: Refine the assertion in the test fixture so it inspects only
the body of the createBranch implementation, rather than any later return sql in
the file. Update the regex to capture the createBranch function body or replace
it with a precise structural/runtime check, and assert only that this
implementation does not immediately execute a query after branch creation.
In `@packages/cli/src/evals/record-codex-product-proof.test.ts`:
- Around line 54-59: Update the “disables ClankerOverflow for the without_mcp
config” test to verify the final clankeroverflow enabled override, rather than
merely checking both values are present. Use the args produced by codexArgs and
assert that the last mcp_servers.clankeroverflow.enabled entry is exactly false.
In `@packages/cli/src/evals/record-codex-product-proof.ts`:
- Around line 400-407: Handle spawn failures explicitly in the code surrounding
the spawnSync invocation: check result.error and include its details in the
constructed failure message before or alongside status and stderr handling.
Update the error branch so ENOENT and other process-launch errors identify the
underlying spawn error instead of reporting only an unknown status.
In `@packages/cli/src/evals/repo-stackoverflow.ts`:
- Around line 190-194: The parseArg function currently accepts another flag as
an option value; validate that index + 1 exists and the next argument does not
start with “--”. If validation fails, raise a clear usage error instead of
returning the flag as a path.
In `@packages/cli/src/learn.ts`:
- Around line 82-83: Update the env-var replacement regex in the `replacements`
array to match both unquoted values and single- or double-quoted values, while
preserving the existing variable-name matching and replacing the entire value
with `<redacted>`.
- Around line 300-348: Ensure learnSolution always releases the backend created
by createSolutionBackend: add a close/dispose lifecycle method to the
SolutionBackend interface and its implementations, then wrap the
duplicate-checking, logging, and mirror-writing flow in a try/finally that
invokes the backend cleanup method. Preserve existing return behavior while
guaranteeing cleanup on success, duplicates, and errors.
In `@packages/cli/src/mcp/server.ts`:
- Around line 484-487: Update readSolutionResource to select a repository
solution only when its basename exactly equals the requested idOrSlug, replacing
the partial candidate.includes(idOrSlug) lookup with an exact basename
comparison or basename-keyed lookup; preserve the existing not-found behavior
and ensure the MCP resource handler continues using readSolutionResource.
---
Nitpick comments:
In `@packages/cli/src/evals/product-proof.ts`:
- Around line 410-412: Clarify the intentional literal-key fallback in
pricingForModel by adding a concise comment identifying pricing.model as the
magic generic default rate, distinct from the dynamic pricing[model] lookup and
pricing.default fallback; preserve the existing fallback behavior.
In `@packages/cli/src/evals/record-codex-product-proof.ts`:
- Around line 347-353: Update verifyWorkspace and the command construction
before spawnSync to shell-escape benchmarkWorkspaceDir/workspacePath with single
quotes, replacing each embedded single quote with the standard '\'' sequence. Do
not use JSON.stringify or double-quoted substitutions for user-controlled paths,
while preserving shell: true and the existing command behavior.
In `@packages/cli/src/learn.ts`:
- Around line 379-413: Update parseStructuredSolution to parse framework,
runtime, packageManager, and fingerprints individually from the “Reusable
Context” section, while retaining repoNote as needed. In exportLocalSolutions,
pass these parsed fields to writeLearnMarkdown so export→syncRepoSolutions
preserves duplicate-detection fingerprints and tag normalization metadata.
In `@packages/cli/src/mcp/server.ts`:
- Around line 466-477: Update the MCP resource `list` callback to use
asynchronous file reads instead of `readFileSync`. Import `readFile` from
`fs/promises`, make the mapping await each file read and parse its contents, and
use `Promise.all` so `list` returns all resource entries without blocking the
event loop.
- Around line 466-477: Replace the manual filename extraction in the MCP
resource list callback with path.basename(file, ".md"), ensuring the path module
is imported and the existing fallback behavior is preserved if needed.
🪄 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: 12b500a9-1036-44cb-9e70-f5335e93968c
📒 Files selected for processing (143)
.gitignoreclankeroverflow-mcp-workspace/description-optimization/2026-06-23_233329/results.jsonclankeroverflow-mcp-workspace/evals/evals.jsonclankeroverflow-mcp-workspace/iteration-1/benchmark.jsonclankeroverflow-mcp-workspace/iteration-1/billing-discount/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/billing-discount/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/billing-discount/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/billing-discount/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/billing-discount/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/billing-discount/without_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-1/dark-mode-toggle/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/dark-mode-toggle/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/dark-mode-toggle/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/dark-mode-toggle/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/dark-mode-toggle/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/eaddrinuse-ci/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/eaddrinuse-ci/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/eaddrinuse-ci/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/eaddrinuse-ci/with_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-1/eaddrinuse-ci/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/eaddrinuse-ci/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/event-loop-explain/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/event-loop-explain/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/event-loop-explain/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/event-loop-explain/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/event-loop-explain/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/grade_and_aggregate.pyclankeroverflow-mcp-workspace/iteration-1/inertia-noindex-ssr/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/inertia-noindex-ssr/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/inertia-noindex-ssr/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/inertia-noindex-ssr/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/inertia-noindex-ssr/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-1/promise-async-await/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/promise-async-await/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/promise-async-await/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/promise-async-await/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/promise-async-await/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/review.htmlclankeroverflow-mcp-workspace/iteration-1/stripe-cf-workers-webcrypto/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/stripe-cf-workers-webcrypto/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/stripe-cf-workers-webcrypto/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/stripe-cf-workers-webcrypto/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/stripe-cf-workers-webcrypto/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/swr-vs-reactquery/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/swr-vs-reactquery/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/swr-vs-reactquery/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/swr-vs-reactquery/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/swr-vs-reactquery/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/eval_metadata.jsonclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/grading.jsonclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-2/dark-mode-toggle/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-2/event-loop-explain/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-2/grade_iter2.pyclankeroverflow-mcp-workspace/iteration-2/inertia-noindex-ssr/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-2/swr-vs-reactquery/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/iteration-2/ts2307-pnpm-workspaces/with_skill/outputs/metrics.jsonclankeroverflow-mcp-workspace/product-proof/README.mdclankeroverflow-mcp-workspace/product-proof/fixtures.jsonclankeroverflow-mcp-workspace/product-proof/reports/report-claude-debug-cost-low-r1.mdclankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r1.mdclankeroverflow-mcp-workspace/product-proof/reports/report-claude-smoke-low-r2.mdclankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost-hidden-r2.mdclankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost-hidden-r3.mdclankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost-hidden.mdclankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost.mdclankeroverflow-mcp-workspace/product-proof/reports/report.mdclankeroverflow-mcp-workspace/product-proof/runs/sample-runs.jsonclankeroverflow-mcp-workspace/product-proof/scenarios.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-eaddrinuse-ci/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-eaddrinuse-ci/server.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-eaddrinuse-ci/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-inertia-noindex/app.blade.phpclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-inertia-noindex/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-inertia-noindex/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/db.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-prisma-neon/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-prisma-neon/schema.prismaclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-prisma-neon/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-react-query-rollback/mutation.jsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-react-query-rollback/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-react-query-rollback/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-sentry-sourcemaps/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-sentry-sourcemaps/sentry.config.jsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-sentry-sourcemaps/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-stripe-workers/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-stripe-workers/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-stripe-workers/webhook.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-ts2307-pnpm/package-under-test.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-ts2307-pnpm/package.jsonclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-ts2307-pnpm/src/app.jsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-ts2307-pnpm/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-eaddrinuse-ci.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-inertia-noindex.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-neon-first-query.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-prisma-neon.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-react-query-rollback.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-sentry-sourcemaps.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-stripe-workers.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-ts2307-pnpm.mjsclankeroverflow-mcp-workspace/repo-stackoverflow/reports/repo-stackoverflow-local-smoke.mdclankeroverflow-mcp-workspace/repo-stackoverflow/runs/repo-stackoverflow-local-smoke.jsonclankeroverflow-mcp-workspace/trigger-eval-set.jsonpackage.jsonpackages/cli/.claude-plugin/plugin.jsonpackages/cli/.codex-plugin/plugin.jsonpackages/cli/commands/learn.mdpackages/cli/commands/log-solution.mdpackages/cli/hooks/hooks.jsonpackages/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/evals/product-proof.test.tspackages/cli/src/evals/product-proof.tspackages/cli/src/evals/record-claude-product-proof.test.tspackages/cli/src/evals/record-claude-product-proof.tspackages/cli/src/evals/record-codex-product-proof.test.tspackages/cli/src/evals/record-codex-product-proof.tspackages/cli/src/evals/repo-stackoverflow.test.tspackages/cli/src/evals/repo-stackoverflow.tspackages/cli/src/hooks/install.test.tspackages/cli/src/hooks/install.tspackages/cli/src/index.test.tspackages/cli/src/index.tspackages/cli/src/learn.test.tspackages/cli/src/learn.tspackages/cli/src/mcp/server.test.tspackages/cli/src/mcp/server.tsskills/clanker-overflow/SKILL.mdskills/cli-version-bump/SKILL.md
| import { join } from "node:path"; | ||
|
|
||
| const source = readFileSync(join(process.argv[2], "mutation.js"), "utf8"); | ||
| if (!/return\s+.*previous|context/i.test(source)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Regex alternation precedence causes false-positive passes.
/return\s+.*previous|context/i matches either return\s+.*previous or the bare word context anywhere in the file. If mutation.js contains the word "context" in a comment, import, or variable name — but onMutate never returns it — the verifier still passes.
🐛 Proposed fix: group the alternation
-if (!/return\s+.*previous|context/i.test(source)) {
+if (!/return\s+.*(previous|context)/i.test(source)) {📝 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.
Suggested change
| if (!/return\s+.*previous|context/i.test(source)) { | |
| if (!/return\s+.*(previous|context)/i.test(source)) { |
🤖 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
`@clankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-react-query-rollback.mjs`
at line 5, Fix the regex in the verifier’s source check by grouping the
alternatives so “context” is only accepted as part of the intended return
pattern, rather than matching anywhere in the file. Update the condition around
the existing source test and preserve case-insensitive matching.
Owner
Author
|
@coderabbitai review\n\nPlease re-review the latest commit c5c8a05; it addresses the prior actionable findings. |
|
✅ Action performedReview finished.
|
feat(cli): improve Pi search triggering
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/cli/src/mcp/backend.ts (1)
36-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose backends created by
backendForSourceafter use.Now that
close()is part of theSolutionBackendinterface,backendForSourceinserver.tscreates new backends for non-"configured" sources (search, upvote, downvote) that are never closed. ForLocalBackend, each call opens a new SQLite connection that leaks. Consider closing backends after each operation or caching them per source.🤖 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/backend.ts` at line 36, Update backendForSource usages in server.ts so every backend created for non-"configured" sources is closed after its operation completes, including when the operation throws. Reuse the SolutionBackend.close method and ensure LocalBackend SQLite connections are released without changing the existing operation behavior.packages/cli/extensions/clankeroverflow-reminder.ts (1)
77-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the relationship between the two prompt classifiers.
The extension's
shouldRemindForPromptand the eval module'sclassifyPiPrompt(inpi-triggering.ts) are independent classifiers for the same conceptual goal. They use different pattern sets and could diverge over time, making eval results less representative of actual runtime behavior. Consider adding a brief comment noting the relationship, or eventually unifying the pattern sources.🤖 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/extensions/clankeroverflow-reminder.ts` around lines 77 - 91, Document near the runtime classifier used by shouldRemindForPrompt that the eval module’s classifyPiPrompt is an independent classifier for the same goal, with separate pattern sets that may diverge. Keep the classifiers unchanged and add only a brief relationship note.packages/cli/src/evals/pi-triggering.ts (1)
152-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the "debug" action kind for clarity.
Line 187 classifies any non-search/learn/skill_read tool call as
"debug". While the semantics are correct forsearchBeforeDebug(any non-search action should be preceded by a search), the name "debug" is misleading for tools likewriteoredit. A name like"other_action"or"non_search"would better convey the intent.🤖 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/evals/pi-triggering.ts` around lines 152 - 189, The fallback action returned by actionsForEvent should no longer be named "debug", since it also represents tools such as write and edit. Rename this action kind to a clearer non-search/other-action name and update all consumers, including searchBeforeDebug-related logic, to preserve the existing behavior.
🤖 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/src/mcp/server.ts`:
- Around line 468-480: Update the resource list callback around Promise.all to
handle per-file failures from readFile or parseLearnMarkdown without rejecting
the entire listing, omitting or otherwise safely handling failed entries. Add
error handling to the read callback around readSolutionResource so invalid or
missing ids return the established MCP error response instead of propagating an
exception.
---
Nitpick comments:
In `@packages/cli/extensions/clankeroverflow-reminder.ts`:
- Around line 77-91: Document near the runtime classifier used by
shouldRemindForPrompt that the eval module’s classifyPiPrompt is an independent
classifier for the same goal, with separate pattern sets that may diverge. Keep
the classifiers unchanged and add only a brief relationship note.
In `@packages/cli/src/evals/pi-triggering.ts`:
- Around line 152-189: The fallback action returned by actionsForEvent should no
longer be named "debug", since it also represents tools such as write and edit.
Rename this action kind to a clearer non-search/other-action name and update all
consumers, including searchBeforeDebug-related logic, to preserve the existing
behavior.
In `@packages/cli/src/mcp/backend.ts`:
- Line 36: Update backendForSource usages in server.ts so every backend created
for non-"configured" sources is closed after its operation completes, including
when the operation throws. Reuse the SolutionBackend.close method and ensure
LocalBackend SQLite connections are released without changing the existing
operation behavior.
🪄 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: d0f28e66-7ede-4a4d-894a-61eb0c8a5165
📒 Files selected for processing (45)
clankeroverflow-mcp-workspace/iteration-1/grade_and_aggregate.pyclankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.mdclankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.mdclankeroverflow-mcp-workspace/pi-triggering/cases.jsonclankeroverflow-mcp-workspace/product-proof/README.mdclankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost-hidden-r2.mdclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-prisma-neon/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-react-query-rollback/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-sentry-sourcemaps/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-stripe-workers/test.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-neon-first-query.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-prisma-neon.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-react-query-rollback.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-sentry-sourcemaps.mjsclankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-stripe-workers.mjspackage.jsonpackages/cli/.claude-plugin/plugin.jsonpackages/cli/.codex-plugin/plugin.jsonpackages/cli/extensions/clankeroverflow-reminder.tspackages/cli/openclaw.plugin.jsonpackages/cli/package.jsonpackages/cli/skills/clankeroverflow-cli/SKILL.mdpackages/cli/skills/clankeroverflow-mcp/SKILL.mdpackages/cli/src/evals/pi-triggering-run.tspackages/cli/src/evals/pi-triggering.test.tspackages/cli/src/evals/pi-triggering.tspackages/cli/src/evals/product-proof.tspackages/cli/src/evals/record-codex-product-proof.test.tspackages/cli/src/evals/record-codex-product-proof.tspackages/cli/src/evals/repo-stackoverflow.test.tspackages/cli/src/evals/repo-stackoverflow.tspackages/cli/src/index.test.tspackages/cli/src/index.tspackages/cli/src/learn.test.tspackages/cli/src/learn.tspackages/cli/src/mcp/backend.tspackages/cli/src/mcp/local-backend.tspackages/cli/src/mcp/remote-backend.tspackages/cli/src/mcp/server.test.tspackages/cli/src/mcp/server.tspackages/cli/src/pi-reminder.test.tspackages/cli/src/setup.test.tspackages/cli/src/setup.ts
✅ Files skipped from review due to trivial changes (7)
- clankeroverflow-mcp-workspace/product-proof/README.md
- packages/cli/.codex-plugin/plugin.json
- clankeroverflow-mcp-workspace/pi-triggering/cases.json
- clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/with_skill/outputs/response.md
- clankeroverflow-mcp-workspace/iteration-1/prisma-neon-timeout/without_skill/outputs/response.md
- clankeroverflow-mcp-workspace/iteration-1/ts2307-pnpm-workspaces/without_skill/outputs/response.md
- clankeroverflow-mcp-workspace/product-proof/reports/report-debug-cost-hidden-r2.md
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/cli/openclaw.plugin.json
- clankeroverflow-mcp-workspace/product-proof/workspace-verifiers/debug-prisma-neon.mjs
- clankeroverflow-mcp-workspace/product-proof/workspace-fixtures/debug-neon-first-query/test.mjs
- packages/cli/src/evals/record-codex-product-proof.test.ts
- package.json
- packages/cli/src/index.test.ts
- packages/cli/src/evals/repo-stackoverflow.ts
- packages/cli/src/index.ts
- packages/cli/skills/clankeroverflow-mcp/SKILL.md
- packages/cli/skills/clankeroverflow-cli/SKILL.md
- clankeroverflow-mcp-workspace/iteration-1/grade_and_aggregate.py
- packages/cli/src/mcp/server.test.ts
- packages/cli/src/evals/product-proof.ts
Comment on lines
+468
to
+480
| resources: await Promise.all( | ||
| listRepoSolutionFiles().map(async (file) => { | ||
| const parsed = parseLearnMarkdown(await readFile(file, "utf8")); | ||
| const name = basename(file, ".md"); | ||
| return { | ||
| uri: `clankeroverflow://repo/solutions/${name}`, | ||
| name, | ||
| title: parsed.problem, | ||
| mimeType: "text/markdown", | ||
| }; | ||
| }), | ||
| ), | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add error handling to resource callbacks.
The list callback uses Promise.all, which fails entirely if any file read rejects (e.g., file deleted between listRepoSolutionFiles() and readFile). The read callback doesn't catch errors from readSolutionResource — a non-existent id propagates unhandled.
🛡️ Proposed fix: resilient resource callbacks
list: async () => ({
- resources: await Promise.all(
+ resources: (
+ await Promise.allSettled(
listRepoSolutionFiles().map(async (file) => {
const parsed = parseLearnMarkdown(await readFile(file, "utf8"));
const name = basename(file, ".md");
return {
uri: `clankeroverflow://repo/solutions/${name}`,
name,
title: parsed.problem,
mimeType: "text/markdown",
};
}),
- ),
+ ))
+ .filter((r) => r.status === "fulfilled")
+ .map((r) => r.value),
}), async (uri, variables) => {
- const { text } = readSolutionResource(String(variables.id));
- return { contents: [{ uri: uri.href, mimeType: "text/markdown", text }] };
+ try {
+ const { text } = readSolutionResource(String(variables.id));
+ return { contents: [{ uri: uri.href, mimeType: "text/markdown", text }] };
+ } catch {
+ return { contents: [{ uri: uri.href, mimeType: "text/markdown", text: `Solution "${variables.id}" not found.` }] };
+ }
},Also applies to: 488-488
🤖 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/server.ts` around lines 468 - 480, Update the resource
list callback around Promise.all to handle per-file failures from readFile or
parseLearnMarkdown without rejecting the entire listing, omitting or otherwise
safely handling failed entries. Add error handling to the read callback around
readSolutionResource so invalid or missing ids return the established MCP error
response instead of propagating an exception.
bernoussama
commented
Aug 21, 2026
bernoussama
left a comment
bernoussama
left a comment
Owner
Author
There was a problem hiding this comment.
Nightly review — clankeroverflow#69 (feat: add learned repo memory and MCP product-proof evals)
Verdict: Comment (no blockers).
- Eval artifacts committed to git: timestamped
results.jsonfiles (1.4k lines each) underclankeroverflow-mcp-workspace/. You already gitignoreproduct-proof/runs/*— do the same fordescription-optimization/*/results.jsonand keep only curated samples, or history will bloat fast. - The committed eval itself shows
trigger_rate: 0.0on most train queries and best score 4/8 — i.e. the learned description didn't generalize. Worth a short conclusion in the README/eval notes so the artifact isn't misleading. - Mixes real feature code (learned repo memory) with bulk data files — split data from code next time so the code delta is actually reviewable.
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
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
clanker learnworkflows with local repo Markdown mirrors, sync/export, deduplication, and private-by-default storage.learn_solution, repo solution resources, stronger search triggers, and untrusted-result guidance.Validation
pnpm --filter @clankeroverflow/cli buildpnpm --filter @clankeroverflow/cli test(163 tests)pnpm run lint(passes with existing warnings)pnpm run formatuvx --from skills-ref agentskills validatefor all three ClankerOverflow skillsgit diff --cached --checkReview notes
Summary by CodeRabbit