feat: ORACLE — AI-assisted graph intelligence (#193)#194
Merged
flyingrobots merged 11 commits intomainfrom Feb 12, 2026
Merged
Conversation
added 7 commits
February 12, 2026 03:55
Four composable detectors (dangling edges, orphan milestones, orphan nodes, low-confidence edges), orchestrator, and fixer. 10 tests.
Extracts file, commit, and graph context to build bounded LLM prompts. Language inference from extensions, size-bounded output. 9 tests.
`git mind doctor [--json] [--fix]` runs graph integrity checks. Exit code 1 when errors found. --fix removes dangling edges.
callAgent shells out to GITMIND_AGENT, parseSuggestions handles JSON and code fences, filterRejected excludes previously-rejected edges. 10 tests.
getPendingSuggestions, acceptSuggestion, rejectSuggestion, adjustSuggestion, skipSuggestion, getReviewHistory, batchDecision. Decision nodes (decision: prefix) store provenance. 9 tests.
Replace suggest/review stubs with full implementations. suggest: --agent, --context, --json flags. Shells out to GITMIND_AGENT. review: --batch accept|reject for non-interactive, interactive readline mode with [a]ccept/[r]eject/[s]kip prompts. formatSuggestions, formatReviewItem, formatDecisionSummary formatters.
Export doctor, context, suggest, and review modules from public API. Add ORACLE section to changelog: 200 tests across 13 files.
Contributor
📝 WalkthroughWalkthroughAdds AI-driven Doctor, Context, Suggest, and Review modules; extends CLI flag parsing and new command signatures; adds formatting helpers; implements graph diagnostics and fixes, agent-driven suggestion generation, review persistence, and comprehensive unit tests. Changes
Sequence DiagramssequenceDiagram
actor User
participant CLI as CLI (bin/git-mind.js)
participant Cmd as commands.js
participant Doctor as doctor.js
participant Graph as Graph Instance
User->>CLI: doctor [--json] [--fix]
CLI->>Cmd: doctor(cwd, { json, fix })
Cmd->>Doctor: runDoctor(graph)
Doctor->>Graph: fetch nodes & edges
Graph-->>Doctor: nodes, edges
Doctor->>Doctor: run detectors
Doctor-->>Cmd: { issues, summary, clean }
alt fix enabled
Cmd->>Doctor: fixIssues(graph, issues)
Doctor->>Graph: removeEdge (dangling)
Graph-->>Doctor: removal results
Doctor-->>Cmd: { fixed, skipped, details }
end
Cmd->>CLI: return JSON/formatted output
CLI->>User: display result
sequenceDiagram
actor User
participant CLI as CLI (bin/git-mind.js)
participant Cmd as commands.js
participant Suggest as suggest.js
participant Context as context.js
participant Agent as External Agent
participant Graph as Graph Instance
User->>CLI: suggest [--agent X] [--context Y] [--json]
CLI->>Cmd: suggest(cwd, { agent, context, json })
Cmd->>Suggest: generateSuggestions(cwd, graph, opts)
Suggest->>Context: extractContext(cwd, graph)
Context->>Graph: extractGraphContext()
Graph-->>Context: nodes, edges
Context-->>Suggest: { files, commits, graph, prompt }
Suggest->>Agent: callAgent(prompt)
Agent-->>Suggest: response text
Suggest->>Suggest: parseSuggestions(response)
Suggest->>Graph: filterRejected(suggestions, graph)
Graph-->>Suggest: decisions
Suggest-->>Cmd: { suggestions, errors, prompt }
Cmd->>CLI: JSON/formatted suggestions
CLI->>User: display suggestions
sequenceDiagram
actor User
participant CLI as CLI (bin/git-mind.js)
participant Cmd as commands.js
participant Review as review.js
participant Graph as Graph Instance
User->>CLI: review [--batch accept|reject] [--json]
CLI->>Cmd: review(cwd, { batch, json })
alt batch mode
Cmd->>Review: batchDecision(graph, action)
Review->>Graph: getPendingSuggestions()
Graph-->>Review: pending suggestions
Review->>Graph: apply accept/reject to each
Graph-->>Review: updates
Review->>Graph: recordDecision() x n
Review-->>Cmd: summary
else interactive
Cmd->>Review: getPendingSuggestions(graph)
Review->>Graph: pending suggestions
loop for each suggestion
Review->>User: prompt accept/reject/skip
User-->>Review: decision
Review->>Graph: apply decision (update/remove)
Review->>Graph: recordDecision()
end
Review-->>Cmd: summary
end
Cmd->>CLI: formatted summary / JSON
CLI->>User: display summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try { | ||
| // Get commits with short sha and first line of message | ||
| const logOutput = execSync( | ||
| `git log --format="%h %s" ${range} 2>/dev/null || git log --format="%h %s" -${limit}`, |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, to fix this class of problem you should: (1) avoid constructing shell command strings with untrusted data; (2) prefer execFile/spawn with argument arrays so the shell never interprets those arguments; or (3) if you must use a shell, safely quote or escape any interpolated data using a robust library such as shell-quote.
For this specific code, the safest fix without changing external behavior is to remove opts.range from any shell command string and only ever pass it as a non-shell-processed argument. We can refactor extractCommitContext to avoid using shell features (2>/dev/null and || ...) by doing the fallback logic in JavaScript: first try one git log invocation with the user-supplied range, catching any error, and on failure fall back to a second git log invocation that uses only the numeric limit. Both invocations will use execSync with an argument array (['log', '--format=%h %s', range] and ['log', '--format=%h %s', -${limit}]) so no shell is involved at all. This entirely eliminates the tainted string concatenation and addresses all three CodeQL variants: the data still flows from opts in generateSuggestions and extractContext, but it no longer influences a shell command string.
Concretely in src/context.js:
- Replace the single
execSynccall on lines 118–120 with:- A first
execSyncusing['log', ... , range]. - A
catchthat then triesexecSyncwith['log', ... , -limit]. - If the fallback also fails, just return
[]fromextractCommitContext.
- A first
- Use
execSyncfrom'node:child_process'in array form, which is already imported. - Keep
sanitizeGitArgunchanged, as it is now an extra guard but no longer the primary shell-safety mechanism.
No changes are needed in src/suggest.js beyond this, since opts continues to be forwarded but no longer taints a shell command string.
Suggested changeset
1
src/context.js
| @@ -114,11 +114,23 @@ | ||
| const range = sanitizeGitArg(opts.range ?? `HEAD~${limit}..HEAD`); | ||
|
|
||
| try { | ||
| // Get commits with short sha and first line of message | ||
| const logOutput = execSync( | ||
| `git log --format="%h %s" ${range} 2>/dev/null || git log --format="%h %s" -${limit}`, | ||
| { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] } | ||
| ); | ||
| // Get commits with short sha and first line of message. | ||
| // Avoid shell interpretation by using argument arrays and JS-level fallback. | ||
| let logOutput; | ||
| try { | ||
| logOutput = execSync( | ||
| 'git', | ||
| ['log', '--format=%h %s', range], | ||
| { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] } | ||
| ); | ||
| } catch { | ||
| // Fallback: use a simple limit-based log if the range is invalid. | ||
| logOutput = execSync( | ||
| 'git', | ||
| ['log', '--format=%h %s', `-${limit}`], | ||
| { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] } | ||
| ); | ||
| } | ||
| const lines = logOutput.trim().split('\n').filter(Boolean); | ||
|
|
||
| return lines.map(line => { |
Copilot is powered by AI and may make mistakes. Always verify output.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/git-mind.js (1)
59-68:⚠️ Potential issue | 🔴 Critical
parseFlagstreats all flags as key-value pairs — breaks boolean flags like--jsonand--fix.When
--jsonprecedes a valued flag (e.g.,git mind suggest --json --agent my-cmd),parseFlagsconsumes--agentas the value of--json, losing the--agentflag entirely. This affects bothsuggestandreviewcommands since they useparseFlagsalongsideargs.includes('--json').Example:
git mind suggest --json --agent my-cmd
parseFlagssees['--json', '--agent', 'my-cmd']- Sets
flags.json = '--agent', skips ahead →flags.agentis never setThe
doctorcommand avoids this by not usingparseFlags, butsuggestandrevieware affected.Proposed fix: skip known boolean flags in parseFlags
+const BOOLEAN_FLAGS = new Set(['json', 'fix']); + function parseFlags(args) { const flags = {}; for (let i = 0; i < args.length; i++) { - if (args[i].startsWith('--') && i + 1 < args.length) { - flags[args[i].slice(2)] = args[i + 1]; + if (args[i].startsWith('--')) { + const name = args[i].slice(2); + if (BOOLEAN_FLAGS.has(name)) { + flags[name] = true; + } else if (i + 1 < args.length) { + flags[name] = args[i + 1]; + i++; + } - i++; } } return flags; }Alternatively, filter out
--jsonand--fixfrom the args before passing toparseFlags, similar to hownodesdoes it on Line 120.
🤖 Fix all issues with AI agents
In `@src/cli/commands.js`:
- Around line 375-411: The readline interface `rl` created by createInterface is
not closed if acceptSuggestion or rejectSuggestion throws, causing a
terminal/input leak; wrap the interactive loop that uses `ask` and calls
`acceptSuggestion`, `rejectSuggestion`, and `skipSuggestion` in a try/finally
(or ensure a finally block) so that `rl.close()` is always called regardless of
errors, then keep the existing catch to set process.exitCode and log the error.
In `@src/cli/format.js`:
- Around line 214-222: The code assumes fixResult.details is iterable and will
throw if it's undefined; update the block that builds the "Fix Results" output
(the if (fixResult) { ... } section using lines.push and the for...of over
fixResult.details) to guard against missing/non-array details by either checking
Array.isArray(fixResult.details) before iterating or defaulting to an empty
array (e.g., const details = Array.isArray(fixResult.details) ?
fixResult.details : []; then loop over details).
In `@src/context.js`:
- Around line 155-163: The current loop uses node.includes(fp) which can produce
false positives; change the match in the file node collection to a precise
comparison: inside the loop that iterates filePaths and nodes (where you check
node.startsWith('file:')), replace node.includes(fp) with a deterministic check
such as node === `file:${fp}` or a normalized suffix check like
node.endsWith(`/${fp}`) after normalizing paths, so fileNodeSet only contains
exact file nodes (refer to fileNodeSet, filePaths, nodes,
node.startsWith('file:'), node.includes(fp)).
- Around line 104-138: The extractCommitContext function is vulnerable to shell
injection because opts.range and the derived range are interpolated into
execSync, and sha is later interpolated into the git diff-tree command; fix by
validating/sanitizing opts.range before building the git log command (allow only
a safe whitelist/regex for range formats like HEAD, HEAD~N, <sha>..HEAD, or
numeric limits) and avoid shell interpolation by passing arguments as an array
to execSync/spawn when possible; additionally validate the extracted sha against
a strict hex pattern (e.g., /^[0-9a-f]{7,40}$/) before using it in the git
diff-tree invocation (or pass sha as an execSync argument rather than inline),
and ensure all execSync calls use safe argument forms rather than string
interpolation.
In `@src/doctor.js`:
- Around line 160-183: fixIssues currently assumes issue.affected is an array
and uses positional destructuring ([source, target, type]) which is brittle;
update fixIssues to read named properties instead (e.g., prefer issue.source /
issue.target / issue.edgeType or issue.affected.source etc.) and adjust the
call-sites or detectDanglingEdges output to emit an object with those keys so
removeEdge(graph, source, target, type) receives explicitly named values; ensure
you still handle legacy array shape by adding a small fallback that maps [a,b,c]
-> {source:a,target:b,edgeType:c} and update references in fixIssues to use the
named variables.
In `@src/review.js`:
- Around line 249-275: getReviewHistory repeats the same N+1 pattern as
getPendingSuggestions by calling graph.getNodeProps in a loop; extract a shared
helper (e.g., getDecisionNodes or fetchDecisionProps) that takes graph and
optional filter, moves the logic that lists nodes, filters by prefix 'decision:'
and bulk-fetches/collects props into a reusable function, then have
getReviewHistory call that helper to build the decisions array and apply the
action filter and sorting; update references in getPendingSuggestions and
getReviewHistory to use the new helper (identify by function names
getReviewHistory, getPendingSuggestions and the new helper name
getDecisionNodes/fetchDecisionProps).
- Around line 82-113: The current getPendingSuggestions function performs
sequential awaits for graph.getNodeProps inside the for-loop over decisionNodes
causing an N+1 waterfall; change this to fetch node props concurrently by
replacing the loop with a Promise.all over decisionNodes (e.g.,
Promise.all(decisionNodes.map(id => graph.getNodeProps(id)))) and then iterate
the resolved props array to populate reviewedKeys, or if the graph API exposes a
batch method (e.g., graph.getNodesProps(decisionNodes)) use that instead; ensure
you still guard for null/undefined propsMap and build the same
`${source}|${target}|${edgeType}` keys before filtering lowConf results.
- Around line 34-49: The JSDoc for makeDecisionId is misleading because the
function appends a time-based epoch (Date.now()), so it is not deterministic;
either remove the epoch from makeDecisionId (keep only the sha256 of
`${source}|${target}|${type}`) to make IDs truly deterministic for the same
inputs, or, if the goal is per-review uniqueness, update the JSDoc to say
"unique decision node ID" (or similar) and keep the epoch — modify the function
comment or remove the epoch accordingly in the makeDecisionId implementation.
- Around line 285-297: In batchDecision, validate the action parameter before
processing pending suggestions: ensure action is exactly 'accept' or 'reject'
(e.g., compare against a small set or use a switch) and throw a clear TypeError
if it is not; then proceed to call acceptSuggestion or rejectSuggestion for each
suggestion (using getPendingSuggestions as before) so there’s no silent
fallthrough to rejectSuggestion when an invalid string is passed.
- Around line 182-221: The current adjustSuggestion implementation deletes the
old edge with removeEdge before creating the new one, risking data loss if
createEdge fails; update adjustSuggestion to perform the change atomically by
either (A) creating the new edge first and only removing the old one after
successful creation (use createEdge then removeEdge, ensuring any errors abort
without deleting the original), or preferably (B) use a single graph-level patch
(graph.createPatch) to create the new edge properties and remove the old edge in
the same patch/commit so both operations succeed or fail together; reference
adjustSuggestion, removeEdge, createEdge and graph.createPatch when applying the
fix.
- Line 184: The code currently sets newConf to a magic 0.8 when
adjustments.confidence is missing; change the assignment so it preserves the
original confidence instead of silently overwriting it — i.e., compute newConf
from adjustments.confidence if present, otherwise use original.confidence (and
if you want a final fallback keep 0.8 only when both are undefined). Update the
assignment for newConf in src/review.js (referencing adjustments.confidence and
original.confidence) to implement this precedence.
In `@src/suggest.js`:
- Around line 36-68: In callAgent add a configurable timeout (e.g., opts.timeout
in ms) and enforce it by starting a timer when the child is spawned; on timeout
call child.kill('SIGTERM') (or SIGKILL if needed) and reject the Promise with a
clear timeout Error, clear the timer when the child emits 'close' or on
successful resolve/reject, and guard against double resolution by tracking a
settled flag; also ensure the timer is cleared on the child's 'error' handler so
the process cannot hang indefinitely.
- Around line 84-108: The markdown fence and array-extraction regexes (used on
responseText to set fenceMatch/arrayMatch) are vulnerable to ReDoS; replace the
fence regex with a non-backtracking pattern like /```(?:json)?[
\t]*\n([\s\S]*?)\n```/ (use explicit horizontal-space then required newline) and
remove the fallback /\[[\s\S]*\]/ regex entirely — instead locate the first '['
and matching ']' with indexOf/lastIndexOf (or a simple loop to find the closing
bracket) and substring that slice before calling JSON.parse (or apply a length
cap on the slice) to avoid catastrophic backtracking when parsing in suggest.js
where fenceMatch, arrayMatch and parsed are used.
Security: - Sanitize git range args against shell injection (context.js) - Fix ReDoS in code fence and array extraction regexes (suggest.js) - Add agent subprocess timeout with configurable limit (suggest.js) - Fix parseFlags boolean flag handling for --json/--fix (git-mind.js) Data safety: - Create-before-remove in adjustSuggestion for atomicity (review.js) - Preserve original confidence instead of magic 0.8 default (review.js) - Validate batchDecision action parameter (review.js) - Wrap interactive review readline in try/finally (commands.js) Correctness: - Use exact match for file node association (context.js) - Guard fixResult.details with nullish coalescing (format.js) - Use named properties in fixIssues instead of positional destructuring (doctor.js) - Fix makeDecisionId JSDoc (deterministic → unique) (review.js) Performance: - Promise.all for concurrent node prop fetches in review.js Tests: 205 passing (5 new regression tests)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@bin/git-mind.js`:
- Around line 169-194: The flag handling is inconsistent: some cases read --json
via args.includes('--json') while suggest and review already call parseFlags;
update the doctor, suggest, and review branches to consistently read the json
boolean from parseFlags (use parseFlags(args.slice(1)) to produce a flags object
and pass flags.json into the options for doctor, suggest, and review), and
remove direct uses of args.includes('--json'); reference the doctor, suggest,
review cases and the parseFlags function to locate and change the flag reads.
In `@src/cli/format.js`:
- Around line 232-260: formatSuggestions currently assumes result.errors is
always iterable and uses result.errors.length and for (const err of
result.errors), which throws if result.errors is undefined; update
formatSuggestions to defensively default result.errors to an empty array
(consistent with how fixResult.details is handled) before checking length and
iterating — e.g., normalize with a local const errors = result.errors || [] and
then use errors.length and for (const err of errors) to avoid TypeError when
callers pass a result without an errors property.
In `@src/context.js`:
- Around line 112-121: extractCommitContext currently interpolates opts.limit
into the shell fallback (`-${limit}`) without coercion, allowing a caller to
inject a non-numeric string; fix by coercing and validating opts.limit into a
safe integer before use (e.g., parseInt/Number and clamp to a positive max with
fallback to 10) and use that validated variable (limit) when building the git
command; reference symbols: extractCommitContext, opts.limit, limit, and the git
log fallback invocation so you replace the unchecked value with the sanitized
integer.
- Around line 104-110: The sanitizeGitArg function's blacklist is missing
redirection and newline characters, so update the pattern in sanitizeGitArg to
also reject '<', '>', '\n', and '\r' (e.g. expand the regex character class to
include <>\\n\\r) and throw the same Error for inputs containing them; ensure
the updated check remains in the sanitizeGitArg function and add/adjust unit
tests to cover cases like "HEAD > /tmp/output" and strings containing newlines.
In `@src/doctor.js`:
- Around line 106-125: The file imports isLowConfidence from validators.js but
detectLowConfidenceEdges reimplements the same check; either remove the unused
import or refactor detectLowConfidenceEdges to call isLowConfidence to avoid
duplication—if you need a configurable threshold, update isLowConfidence to
accept a threshold argument (e.g., isLowConfidence(edge, threshold)) and then
replace the inline check in detectLowConfidenceEdges with edges.filter(e =>
isLowConfidence(e, threshold)); otherwise remove the isLowConfidence import and
keep the existing thresholded implementation.
- Around line 89-104: detectOrphanNodes is treating review-created decision
nodes as orphans; update detectOrphanNodes to skip nodes whose ids match the
internal review prefix (e.g. starting with "decision:") before checking
connectivity. In the function (detectOrphanNodes) filter out nodes with
n.startsWith("decision:") or other known internal prefixes so they are not
included in the orphan detection and returned list; preserve existing output
shape for genuine orphans.
- Line 7: The import extractPrefix from './validators.js' in src/doctor.js is
unused; remove the unused symbol from the import statement so only
isLowConfidence is imported (i.e., update the import declaration that currently
reads import { extractPrefix, isLowConfidence } from './validators.js' to import
only isLowConfidence). Ensure no other references to extractPrefix remain in
this file after the change.
In `@src/review.js`:
- Around line 229-240: skipSuggestion currently builds a decision object but
does not persist it, so getPendingSuggestions (which only checks persisted
decision nodes) will continue to surface skipped suggestions; change
skipSuggestion to persist the decision by calling recordDecision with the
created decision (use makeDecisionId to build the id as now), make the function
async if needed and await recordDecision, propagate or handle errors from
recordDecision, and return the persisted decision object; alternatively, if the
intended behavior is "defer" not "dismiss", add a JSDoc comment to
skipSuggestion explaining that it purposely does not call recordDecision so
skipped items remain pending.
- Around line 124-129: The acceptSuggestion and adjustSuggestion functions must
handle the race where setEdgeProperty throws if the edge was deleted after
suggestion retrieval: wrap the patch operations (graph.createPatch(),
patch.setEdgeProperty(...), and await patch.commit()) in a try-catch around the
calls in acceptSuggestion and adjustSuggestion, catch the specific error thrown
when the edge does not exist, and handle it gracefully (e.g., log/return a clear
"edge already deleted" result or noop) instead of letting the exception bubble;
alternatively, before calling patch.setEdgeProperty check edge existence via the
graph's read API (e.g., a getEdge/getMaterializedEdge call) and skip/return if
missing—ensure you reference and update acceptSuggestion, adjustSuggestion,
setEdgeProperty, graph.createPatch, and patch.commit accordingly.
In `@src/suggest.js`:
- Around line 187-209: The loop in filterRejected performs sequential
graph.getNodeProps calls causing latency; change it to fetch all node props
concurrently using Promise.all on decisionNodes.map(nodeId =>
graph.getNodeProps(nodeId)), then iterate the resulting array of propsMaps to
build rejectedKeys exactly as before (skip falsy propsMaps, check action ===
'reject', read source/target/edgeType and add
`${source}|${target}|${edgeType}`), preserving the final filter that returns
suggestions excluding rejected keys.
In `@test/suggest.test.js`:
- Around line 26-34: The test restores process.env.GITMIND_AGENT using a falsy
check which fails for empty-string values; update the restoration in the test
case (the "throws when GITMIND_AGENT is not set" spec in test/suggest.test.js)
to check for undefined explicitly (use if (original !== undefined)) so that an
original value of "" is restored correctly after the test that calls
callAgent('test prompt').
Comment on lines
+124
to
+129
| export async function acceptSuggestion(graph, suggestion, opts = {}) { | ||
| // Update edge confidence to 1.0 and add reviewedAt | ||
| const patch = await graph.createPatch(); | ||
| patch.setEdgeProperty(suggestion.source, suggestion.target, suggestion.type, 'confidence', 1.0); | ||
| patch.setEdgeProperty(suggestion.source, suggestion.target, suggestion.type, 'reviewedAt', new Date().toISOString()); | ||
| await patch.commit(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
git-warp setEdgeProperty non-existent edge behavior
💡 Result:
In @git-stunts/git-warp, setEdgeProperty(from, to, label, key, value) does not silently create the edge.
It validates that the edge already exists, either:
- added earlier in the same patch via
addEdge(...)(tracked in_edgesAdded), or - present in the current materialized state (
state.edgeAliveOR-Set)
If the edge exists in neither place, it throws an Error with the message:
Cannot set property on unknown edge (from → to [label]): add the edge first[1]
So the correct pattern is to addEdge(...) first (possibly in the same patch), or ensure the edge is already in materialized state before calling setEdgeProperty(...). [1]
Sources
[1] PatchBuilderV2 implementation (setEdgeProperty validation + thrown error message) (jsr.io)
Citations:
🏁 Script executed:
# First, let's find and examine the src/review.js file
find . -name "review.js" -type fRepository: neuroglyph/git-mind
Length of output: 78
🏁 Script executed:
# Check the repository structure
git ls-files | head -20Repository: neuroglyph/git-mind
Length of output: 344
🏁 Script executed:
# Read the acceptSuggestion function and surrounding context
wc -l src/review.jsRepository: neuroglyph/git-mind
Length of output: 80
🏁 Script executed:
# Read around the acceptSuggestion function (lines 124-129)
sed -n '115,140p' src/review.jsRepository: neuroglyph/git-mind
Length of output: 1087
🏁 Script executed:
# Read the adjustSuggestion function (lines 198-204)
sed -n '190,215p' src/review.jsRepository: neuroglyph/git-mind
Length of output: 1100
🏁 Script executed:
# Search for edge existence checks or loadEdge patterns in the file
rg -n "loadEdge|edgeExists|hasEdge" src/review.jsRepository: neuroglyph/git-mind
Length of output: 45
🏁 Script executed:
# Find where acceptSuggestion and adjustSuggestion are called
rg -n "acceptSuggestion|adjustSuggestion" src/Repository: neuroglyph/git-mind
Length of output: 706
🏁 Script executed:
# Look for PendingSuggestion definition
rg -n "PendingSuggestion" src/Repository: neuroglyph/git-mind
Length of output: 935
🏁 Script executed:
# Check if there's any edge validation before calling setEdgeProperty
rg -n "loadEdge\(" src/Repository: neuroglyph/git-mind
Length of output: 45
🏁 Script executed:
# Search for error handling patterns or try-catch blocks around setEdgeProperty
rg -B 3 -A 3 "setEdgeProperty" src/review.jsRepository: neuroglyph/git-mind
Length of output: 981
🏁 Script executed:
# Read the getPendingSuggestions function
sed -n '82,120p' src/review.jsRepository: neuroglyph/git-mind
Length of output: 1316
🏁 Script executed:
# Look at the PendingSuggestion typedef
sed -n '12,25p' src/review.jsRepository: neuroglyph/git-mind
Length of output: 508
🏁 Script executed:
# Check if there's any loadEdge or edge existence validation in the entire file
rg -n "state\." src/review.js | head -20Repository: neuroglyph/git-mind
Length of output: 45
Correct the edge existence check and error handling for concurrent edge deletions in acceptSuggestion and adjustSuggestion.
setEdgeProperty() will throw an Error if the edge no longer exists in the materialized state. While getPendingSuggestions retrieves edges that currently exist, a race condition occurs if the edge is deleted (e.g., by a parallel rejection) between suggestion retrieval and setEdgeProperty execution. The functions currently have no guard or error handling for this scenario. Either validate edge existence before calling setEdgeProperty, wrap the patch operations in try-catch with appropriate recovery, or document the single-writer assumption.
🤖 Prompt for AI Agents
In `@src/review.js` around lines 124 - 129, The acceptSuggestion and
adjustSuggestion functions must handle the race where setEdgeProperty throws if
the edge was deleted after suggestion retrieval: wrap the patch operations
(graph.createPatch(), patch.setEdgeProperty(...), and await patch.commit()) in a
try-catch around the calls in acceptSuggestion and adjustSuggestion, catch the
specific error thrown when the edge does not exist, and handle it gracefully
(e.g., log/return a clear "edge already deleted" result or noop) instead of
letting the exception bubble; alternatively, before calling
patch.setEdgeProperty check edge existence via the graph's read API (e.g., a
getEdge/getMaterializedEdge call) and skip/return if missing—ensure you
reference and update acceptSuggestion, adjustSuggestion, setEdgeProperty,
graph.createPatch, and patch.commit accordingly.
Security: - Expand sanitizeGitArg blocklist with <, >, \n, \r (context.js) - Coerce opts.limit to safe integer 1-100 (context.js) - Eliminate ReDoS fence regex entirely, use indexOf (suggest.js) Consistency: - Unify --json/--fix flag handling via parseFlags (git-mind.js) - Promise.all in filterRejected for concurrent prop fetches (suggest.js) - Guard result.errors with optional chaining (format.js) Correctness: - Exclude decision: nodes from orphan detection (doctor.js) - Remove unused extractPrefix import (doctor.js) - Fix env var restoration in test (suggest.test.js) Documentation: - skipSuggestion: document intentional defer behavior (review.js) - acceptSuggestion/adjustSuggestion: document single-writer assumption (review.js) Tests: 206 passing (1 new regression test)
Owner
Author
|
@coderabbitai review please |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/cli/format.js`:
- Around line 286-310: formatDecisionSummary currently does a for...of over
result.decisions without guarding for undefined or non-iterable values; change
it to treat decisions as an empty array when missing/invalid (e.g., assign a
safe local like const decisions = Array.isArray(result.decisions) ?
result.decisions : [] or use result.decisions ?? []) and iterate over that
instead so the counts accumulation (counts[d.action]) never throws; keep the
rest of the function unchanged and still use result.processed for the
conditional.
In `@src/doctor.js`:
- Around line 32-55: The dangling-edge issues produced by detectDanglingEdges
include additional properties (source, target, edgeType) that are not declared
on the DoctorIssue typedef, so update the DoctorIssue typedef to include
optional properties source:string, target:string and edgeType:string (or mark
them optional) and document that the 'dangling-edge' type will set them; also
ensure fixIssues (which destructures those properties) is left intact or amended
to safely handle undefined values if you prefer not to make them required.
In `@src/review.js`:
- Around line 189-208: When handling a type change (newType !== original.type),
ensure the new edge records the review timestamp and correct the misleading
comment: modify the call path so createEdge records 'reviewedAt' (either by
passing reviewedAt to createEdge or by immediately applying a patch to set
reviewedAt after creation) using graph.createPatch()/patch.setEdgeProperty for
the new edge and then commit; also update the comment to say this ordering
provides safer ordering (not atomic) because createEdge and removeEdge commit
separately. Refer to createEdge/removeEdge in src/edges.js and the patch logic
that uses graph.createPatch(), patch.setEdgeProperty(..., 'reviewedAt', ...),
and patch.commit() to implement the change.
In `@src/suggest.js`:
- Around line 223-230: generateSuggestions currently calls parseSuggestions to
get { suggestions, errors } and then filterRejected(suggestions, graph) but if
parseSuggestions returned suggestions that were all rejected the final return
can be { suggestions: [], errors: [], prompt } with no explanation; update
generateSuggestions to detect when parseSuggestions produced non-empty
suggestions but filterRejected returns an empty array and errors is empty, and
in that case add a diagnostic (e.g., push a message into errors or add a
rejectedCount field) so the caller/CLI knows suggestions were filtered out;
refer to generateSuggestions, parseSuggestions and filterRejected when
implementing the check and augmenting the returned object accordingly.
- Around line 86-87: Add a no-op 'error' listener on the child process stdin to
prevent uncaught exceptions when writing: before calling
child.stdin.write(prompt) (in the code around the child.stdin.write /
child.stdin.end in src/suggest.js) attach child.stdin.on('error', () => {}) and
optionally remove the listener after the stream finishes (e.g., on 'finish' or
'close') to avoid leaking the handler; ensure the listener is attached before
write and cleaned up afterward.
In `@test/doctor.test.js`:
- Around line 150-176: The dangling-edge test issue is missing the properties
fixIssues expects; update the constructed dangling-edge issue in the "removes
dangling edges and skips non-fixable issues" test to include source, target, and
edgeType (e.g. source: 'task:a', target: 'spec:b', edgeType: 'implements') so
that fixIssues can destructure and call removeEdge correctly (see
detectDanglingEdges, fixIssues, removeEdge).
- Guard result.decisions with nullish coalescing in formatDecisionSummary - Add source/target/edgeType optional properties to DoctorIssue typedef - Set reviewedAt on new edge after type change in adjustSuggestion - Add rejectedCount diagnostic to generateSuggestions - Add no-op error listener on child.stdin in callAgent - Fix doctor test fixture to include named properties for fixIssues - Update adjustSuggestion comment from "atomic" to "safer ordering"
Owner
Author
|
@coderabbitai review please |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/doctor.js`:
- Around line 137-156: runDoctor currently aggregates issues from
detectDanglingEdges, detectOrphanMilestones, detectOrphanNodes, and
detectLowConfidenceEdges but omits duplicate-edge detection; if duplicate-edge
detection is intended, add the detector by importing/implementing
detectDuplicateEdges and include it in the issues array (e.g.,
...detectDuplicateEdges(nodes, edges)) so duplicates are reported alongside
other detectors in runDoctor and counted in the summary logic.
Owner
Author
|
@coderabbitai review please |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/context.js`:
- Around line 200-281: buildPrompt currently assumes context.graph,
context.commits, and context.files are non-null and will throw when they're
undefined; update buildPrompt to defensively handle partial contexts by
normalizing those fields at the top (e.g., ensure context.graph = context.graph
?? { nodes: [], edges: [] }, context.graph.nodes = context.graph.nodes ?? [],
context.graph.edges = context.graph.edges ?? [], context.commits =
context.commits ?? [], context.files = context.files ?? []) or throw a clear,
descriptive error if context itself is falsy; apply these guards before any
.length checks so references in the existing code paths (the
node/edge/commit/file sections and the truncation logic) never access .length on
undefined.
In `@src/review.js`:
- Around line 82-114: The logic that fetches decision-node props is duplicated
in getPendingSuggestions and getReviewHistory; extract a shared helper (e.g.,
fetchDecisionProps or fetchDecisionReviewedKeys) that takes graph, finds nodes
starting with 'decision:', calls graph.getNodeProps via Promise.all, and returns
the consolidated result needed by both callers (either an array/map of props or
a Set of reviewed keys); then update getPendingSuggestions and getReviewHistory
to call this helper (use the same key format `${source}|${target}|${edgeType}`
or expose reviewed keys) to remove duplicated code and ensure consistent
behavior across both functions.
In `@test/context.test.js`:
- Around line 34-49: The test "extracts tracked files with inferred languages"
uses a loose assertion expect(files.length).toBeGreaterThanOrEqual(2) which
allows unexpected files; update the assertion to expect(files.length).toBe(2) to
precisely verify only the two committed files are returned by extractFileContext
(or, if .gitmind/ metadata files are expected, add a clarifying comment
explaining why >= 2 is necessary). Ensure you update the assertion near the
jsFile/mdFile checks and keep references to extractFileContext, jsFile and
mdFile intact.
- Around line 68-80: Add a stronger assertion to the test that at least one
returned commit from extractCommitContext contains the changed filename: verify
that the commits array (variable commits) includes an entry where files is an
array and includes 'app.js' (e.g. assert some commit satisfies
Array.isArray(c.files) && c.files.includes('app.js')). This will ensure
extractCommitContext returns the files field correctly in addition to sha and
message.
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
- Add defensive guards in buildPrompt for nullish context fields - Extract shared fetchDecisionProps helper to deduplicate review.js - Tighten context.test.js file count assertion from >= to exact - Add commit files assertion to extractCommitContext test
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/cli/format.js`:
- Around line 232-260: formatSuggestions currently interpolates (s.confidence *
100).toFixed(0) without guarding against non-finite values, which can produce
"NaN%"; mirror formatEdge's approach by normalizing s.confidence first (e.g.,
const conf = Number.isFinite(s.confidence) ? s.confidence : 0) and then use conf
when building confStr and rendering lines so a sensible fallback percentage is
shown instead of NaN.
- Around line 269-279: The formatReviewItem function assumes item.confidence is
numeric and does (item.confidence * 100).toFixed(0), which yields "NaN%” for
null/undefined; update the confStr computation in formatReviewItem to mirror
other formatters: first check typeof item.confidence === 'number' and only then
compute and format `${(item.confidence * 100).toFixed(0)}%` (wrapped with
chalk.dim as before); if not a number, use a safe fallback (e.g.,
chalk.dim('(n/a)') or an empty dim string) so the printed line never shows NaN.
In `@src/context.js`:
- Around line 158-191: In extractGraphContext, getNodes() and getEdges() are
fetched sequentially causing unnecessary latency; change the code in
extractGraphContext to call both graph.getNodes() and graph.getEdges()
concurrently using Promise.all (or equivalent) and destructure the results into
nodes and edges before proceeding with the existing file-node filtering and
relatedEdges logic so all downstream uses (e.g., fileNodeSet, relatedNodes,
filteredEdges) remain unchanged.
In `@src/doctor.js`:
- Around line 137-156: runDoctor currently awaits graph.getNodes() and
graph.getEdges() sequentially; fetch them concurrently (same as
extractGraphContext) to improve performance by using Promise.all to run
graph.getNodes() and graph.getEdges() in parallel, then destructure the results
into nodes and edges before continuing; update any variable names accordingly in
runDoctor so detectDanglingEdges, detectOrphanMilestones, detectOrphanNodes, and
detectLowConfidenceEdges receive the same nodes/edges values as before.
In `@src/suggest.js`:
- Around line 164-168: The code silently replaces non-number item.confidence
with 0.5 and only records validateConfidence failures; change this so when
item.confidence is not a number you also push a warning into itemErrors (or a
new warnings array) indicating a default was applied and the original invalid
value, then continue to call validateConfidence on the default; update the logic
around validateConfidence, item.confidence and itemErrors (and create a warnings
accumulator if needed) so callers can detect misconfigured agent outputs.
Comment on lines
+232
to
+260
| export function formatSuggestions(result) { | ||
| const lines = []; | ||
|
|
||
| lines.push(chalk.bold('Suggestions')); | ||
| lines.push(chalk.dim('═'.repeat(32))); | ||
| lines.push(''); | ||
|
|
||
| if (result.suggestions.length === 0) { | ||
| lines.push(chalk.dim(' No suggestions generated')); | ||
| } else { | ||
| for (const s of result.suggestions) { | ||
| const confStr = chalk.dim(`(${(s.confidence * 100).toFixed(0)}%)`); | ||
| lines.push(` ${chalk.cyan(s.source)} ${chalk.dim('--[')}${chalk.yellow(s.type)}${chalk.dim(']-->')} ${chalk.cyan(s.target)} ${confStr}`); | ||
| if (s.rationale) { | ||
| lines.push(` ${chalk.dim(s.rationale)}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (result.errors?.length > 0) { | ||
| lines.push(''); | ||
| lines.push(chalk.yellow(`${result.errors.length} parse error(s):`)); | ||
| for (const err of result.errors) { | ||
| lines.push(` ${chalk.yellow(figures.warning)} ${err}`); | ||
| } | ||
| } | ||
|
|
||
| return lines.join('\n'); | ||
| } |
Contributor
There was a problem hiding this comment.
Same NaN% risk in formatSuggestions for s.confidence.
Line 243 has the same unguarded (s.confidence * 100).toFixed(0) pattern. parseSuggestions defaults confidence to 0.5, so this is unlikely to trigger in normal flows, but for consistency with formatEdge's guard it's worth protecting.
Proposed fix
- const confStr = chalk.dim(`(${(s.confidence * 100).toFixed(0)}%)`);
+ const confStr = typeof s.confidence === 'number'
+ ? chalk.dim(`(${(s.confidence * 100).toFixed(0)}%)`)
+ : '';🤖 Prompt for AI Agents
In `@src/cli/format.js` around lines 232 - 260, formatSuggestions currently
interpolates (s.confidence * 100).toFixed(0) without guarding against non-finite
values, which can produce "NaN%"; mirror formatEdge's approach by normalizing
s.confidence first (e.g., const conf = Number.isFinite(s.confidence) ?
s.confidence : 0) and then use conf when building confStr and rendering lines so
a sensible fallback percentage is shown instead of NaN.
Comment on lines
+269
to
+279
| export function formatReviewItem(item, index, total) { | ||
| const lines = []; | ||
| lines.push(chalk.bold(`Review [${index + 1}/${total}]`)); | ||
| lines.push(chalk.dim('─'.repeat(32))); | ||
| const confStr = chalk.dim(`(${(item.confidence * 100).toFixed(0)}%)`); | ||
| lines.push(` ${chalk.cyan(item.source)} ${chalk.dim('--[')}${chalk.yellow(item.type)}${chalk.dim(']-->')} ${chalk.cyan(item.target)} ${confStr}`); | ||
| if (item.rationale) { | ||
| lines.push(` ${chalk.dim('Rationale:')} ${item.rationale}`); | ||
| } | ||
| return lines.join('\n'); | ||
| } |
Contributor
There was a problem hiding this comment.
formatReviewItem assumes item.confidence is always a number.
If item.confidence is undefined or null, the expression (item.confidence * 100).toFixed(0) on line 273 would render "NaN%". Other formatters in this file (e.g., formatEdge at line 51–52) guard with a typeof check. Consider the same pattern here for consistency.
Proposed fix
- const confStr = chalk.dim(`(${(item.confidence * 100).toFixed(0)}%)`);
+ const confStr = typeof item.confidence === 'number'
+ ? chalk.dim(`(${(item.confidence * 100).toFixed(0)}%)`)
+ : '';🤖 Prompt for AI Agents
In `@src/cli/format.js` around lines 269 - 279, The formatReviewItem function
assumes item.confidence is numeric and does (item.confidence * 100).toFixed(0),
which yields "NaN%” for null/undefined; update the confStr computation in
formatReviewItem to mirror other formatters: first check typeof item.confidence
=== 'number' and only then compute and format `${(item.confidence *
100).toFixed(0)}%` (wrapped with chalk.dim as before); if not a number, use a
safe fallback (e.g., chalk.dim('(n/a)') or an empty dim string) so the printed
line never shows NaN.
Comment on lines
+158
to
+191
| export async function extractGraphContext(graph, filePaths) { | ||
| const nodes = await graph.getNodes(); | ||
| const edges = await graph.getEdges(); | ||
|
|
||
| if (!filePaths || filePaths.length === 0) { | ||
| return { nodes, edges }; | ||
| } | ||
|
|
||
| // Find nodes related to the given file paths | ||
| const fileNodeSet = new Set(); | ||
| for (const fp of filePaths) { | ||
| for (const node of nodes) { | ||
| if (node === `file:${fp}` || (node.startsWith('file:') && node.endsWith(`/${fp}`))) { | ||
| fileNodeSet.add(node); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Include nodes connected to file nodes | ||
| const relatedNodes = new Set(fileNodeSet); | ||
| for (const e of edges) { | ||
| if (fileNodeSet.has(e.from)) relatedNodes.add(e.to); | ||
| if (fileNodeSet.has(e.to)) relatedNodes.add(e.from); | ||
| } | ||
|
|
||
| const filteredEdges = edges.filter( | ||
| e => relatedNodes.has(e.from) || relatedNodes.has(e.to) | ||
| ); | ||
|
|
||
| return { | ||
| nodes: nodes.filter(n => relatedNodes.has(n)), | ||
| edges: filteredEdges, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
getNodes() and getEdges() can be fetched concurrently.
Lines 159–160 await two independent graph reads sequentially. This was already addressed in review.js via Promise.all for decision-node props; the same pattern applies here.
♻️ Proposed refactor
export async function extractGraphContext(graph, filePaths) {
- const nodes = await graph.getNodes();
- const edges = await graph.getEdges();
+ const [nodes, edges] = await Promise.all([graph.getNodes(), graph.getEdges()]);🤖 Prompt for AI Agents
In `@src/context.js` around lines 158 - 191, In extractGraphContext, getNodes()
and getEdges() are fetched sequentially causing unnecessary latency; change the
code in extractGraphContext to call both graph.getNodes() and graph.getEdges()
concurrently using Promise.all (or equivalent) and destructure the results into
nodes and edges before proceeding with the existing file-node filtering and
relatedEdges logic so all downstream uses (e.g., fileNodeSet, relatedNodes,
filteredEdges) remain unchanged.
Comment on lines
+137
to
+156
| export async function runDoctor(graph, opts = {}) { | ||
| const nodes = await graph.getNodes(); | ||
| const edges = await graph.getEdges(); | ||
|
|
||
| const issues = [ | ||
| ...detectDanglingEdges(nodes, edges), | ||
| ...detectOrphanMilestones(nodes, edges), | ||
| ...detectOrphanNodes(nodes, edges), | ||
| ...detectLowConfidenceEdges(edges, opts.threshold), | ||
| ]; | ||
|
|
||
| const summary = { errors: 0, warnings: 0, info: 0 }; | ||
| for (const issue of issues) { | ||
| if (issue.severity === 'error') summary.errors++; | ||
| else if (issue.severity === 'warning') summary.warnings++; | ||
| else if (issue.severity === 'info') summary.info++; | ||
| } | ||
|
|
||
| return { issues, summary, clean: issues.length === 0 }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
getNodes() and getEdges() can be fetched concurrently here too.
Same pattern as extractGraphContext in src/context.js — two independent async reads executed sequentially.
♻️ Proposed refactor
export async function runDoctor(graph, opts = {}) {
- const nodes = await graph.getNodes();
- const edges = await graph.getEdges();
+ const [nodes, edges] = await Promise.all([graph.getNodes(), graph.getEdges()]);📝 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
| export async function runDoctor(graph, opts = {}) { | |
| const nodes = await graph.getNodes(); | |
| const edges = await graph.getEdges(); | |
| const issues = [ | |
| ...detectDanglingEdges(nodes, edges), | |
| ...detectOrphanMilestones(nodes, edges), | |
| ...detectOrphanNodes(nodes, edges), | |
| ...detectLowConfidenceEdges(edges, opts.threshold), | |
| ]; | |
| const summary = { errors: 0, warnings: 0, info: 0 }; | |
| for (const issue of issues) { | |
| if (issue.severity === 'error') summary.errors++; | |
| else if (issue.severity === 'warning') summary.warnings++; | |
| else if (issue.severity === 'info') summary.info++; | |
| } | |
| return { issues, summary, clean: issues.length === 0 }; | |
| } | |
| export async function runDoctor(graph, opts = {}) { | |
| const [nodes, edges] = await Promise.all([graph.getNodes(), graph.getEdges()]); | |
| const issues = [ | |
| ...detectDanglingEdges(nodes, edges), | |
| ...detectOrphanMilestones(nodes, edges), | |
| ...detectOrphanNodes(nodes, edges), | |
| ...detectLowConfidenceEdges(edges, opts.threshold), | |
| ]; | |
| const summary = { errors: 0, warnings: 0, info: 0 }; | |
| for (const issue of issues) { | |
| if (issue.severity === 'error') summary.errors++; | |
| else if (issue.severity === 'warning') summary.warnings++; | |
| else if (issue.severity === 'info') summary.info++; | |
| } | |
| return { issues, summary, clean: issues.length === 0 }; | |
| } |
🤖 Prompt for AI Agents
In `@src/doctor.js` around lines 137 - 156, runDoctor currently awaits
graph.getNodes() and graph.getEdges() sequentially; fetch them concurrently
(same as extractGraphContext) to improve performance by using Promise.all to run
graph.getNodes() and graph.getEdges() in parallel, then destructure the results
into nodes and edges before continuing; update any variable names accordingly in
runDoctor so detectDanglingEdges, detectOrphanMilestones, detectOrphanNodes, and
detectLowConfidenceEdges receive the same nodes/edges values as before.
Comment on lines
+164
to
+168
| // Validate confidence | ||
| const conf = typeof item.confidence === 'number' ? item.confidence : 0.5; | ||
| const confResult = validateConfidence(conf); | ||
| if (!confResult.valid) itemErrors.push(confResult.error); | ||
|
|
Contributor
There was a problem hiding this comment.
Silent confidence default masks agent misconfiguration.
When item.confidence is not a number (e.g. "high", null, undefined), it silently defaults to 0.5 and passes validation without any error surfaced. The caller has no way to know the agent is returning invalid confidence values.
Consider pushing a warning into errors (or a separate warnings array) when the default is applied, so users can fix their agent's output format.
Proposed fix
// Validate confidence
- const conf = typeof item.confidence === 'number' ? item.confidence : 0.5;
+ const hasConfidence = typeof item.confidence === 'number';
+ const conf = hasConfidence ? item.confidence : 0.5;
+ if (!hasConfidence) {
+ errors.push(`Item ${i}: confidence missing or not a number, defaulting to 0.5`);
+ }
const confResult = validateConfidence(conf);
if (!confResult.valid) itemErrors.push(confResult.error);📝 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
| // Validate confidence | |
| const conf = typeof item.confidence === 'number' ? item.confidence : 0.5; | |
| const confResult = validateConfidence(conf); | |
| if (!confResult.valid) itemErrors.push(confResult.error); | |
| // Validate confidence | |
| const hasConfidence = typeof item.confidence === 'number'; | |
| const conf = hasConfidence ? item.confidence : 0.5; | |
| if (!hasConfidence) { | |
| errors.push(`Item ${i}: confidence missing or not a number, defaulting to 0.5`); | |
| } | |
| const confResult = validateConfidence(conf); | |
| if (!confResult.valid) itemErrors.push(confResult.error); |
🤖 Prompt for AI Agents
In `@src/suggest.js` around lines 164 - 168, The code silently replaces non-number
item.confidence with 0.5 and only records validateConfidence failures; change
this so when item.confidence is not a number you also push a warning into
itemErrors (or a new warnings array) indicating a default was applied and the
original invalid value, then continue to call validateConfidence on the default;
update the logic around validateConfidence, item.confidence and itemErrors (and
create a warnings accumulator if needed) so callers can detect misconfigured
agent outputs.
This was referenced Feb 13, 2026
This was referenced Feb 24, 2026
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
GITMIND_AGENTenv var with accept/reject/re-suggest workflowdecision:prefix nodes)doctor,context,suggest,reviewcommands with terminal formattingsrc/index.js7 commits, 1929 lines added, 200 tests passing across 13 files.
Test plan
npx vitest run— 200 tests passing (13 files)Summary by CodeRabbit