Release 0.30.0 - #547
Release 0.30.0#547
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMadar 0.30.0 adds workspace-aware artifact routing for linked Git worktrees, automatic graph refresh for stdio MCP servers, atomic graph publication, updated MCP installation and validation, and corresponding CLI, infrastructure, test, documentation, package, and SBOM changes. ChangesWorkspace-aware artifact routing
Automatic MCP graph refresh
Release and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant MCPServer
participant WorkspaceWatcher
participant GraphArtifact
Agent->>MCPServer: send stdio request
MCPServer->>WorkspaceWatcher: reconcile active workspace
WorkspaceWatcher->>GraphArtifact: publish refreshed graph
MCPServer->>Agent: return graph-backed response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/mcp-response-evidence.ts (1)
129-171: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winShort-circuit the fast path before reading
graph.json.
readGraphSourceRoot(graphPath)still does a synchronous read/parse, but the cheap path check can already return high confidence in the common case. Move the string-only match ahead of the file read, and passgraph.graph.root_paththrough from the caller when it’s already available instead of rereading the artifact.🤖 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 `@src/runtime/mcp-response-evidence.ts` around lines 129 - 171, The scope check currently reads and parses graph.json before evaluating the cheap path match. In the relevant scope-validation function, evaluate normalizedGraphPath against expectedGraphPath first and return high confidence immediately when it matches; otherwise use an optional caller-provided graph.graph.root_path for source-root validation, falling back to readGraphSourceRoot only when that value is unavailable. Update the caller to pass the existing root_path through without rereading the artifact.
🧹 Nitpick comments (4)
src/infrastructure/watch.ts (1)
167-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
sameFilesystemPathproduction helper across infrastructure and runtime layers. Both files independently implement identical filesystem-identity comparison logic used to validate that a graph artifact belongs to the expected workspace; the shared root cause is that this utility has no single source of truth, risking silent drift between the two copies for a security/correctness-relevant check.
src/infrastructure/watch.ts#L167-L173: keep the canonical implementation here (or move it intosrc/shared/workspace.ts, which both files already import) and export it.src/runtime/stdio-server.ts#L154-L160: remove this local copy and importsameFilesystemPathfromsrc/infrastructure/watch.ts(or the shared module) instead.🤖 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 `@src/infrastructure/watch.ts` around lines 167 - 173, Make sameFilesystemPath a single exported implementation in src/infrastructure/watch.ts (or shared/workspace.ts), preserving its realpathSync/resolve behavior; in src/runtime/stdio-server.ts, remove the duplicate helper and import the shared symbol instead.tests/unit/watch.test.ts (1)
644-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
waitForpolling helper across two test files. Both files independently define a byte-for-byte equivalent async condition-poller for waiting on graph-refresh side effects; the shared root cause is that this test utility has no common home.
tests/unit/watch.test.ts#L644-L654: remove this localwaitForand import it from a shared test-utils module instead.tests/unit/stdio-server.test.ts#L12-L22: remove this localwaitForand import it from the same shared test-utils module instead.🤖 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 `@tests/unit/watch.test.ts` around lines 644 - 654, The duplicated async waitFor polling helper should live in a shared test-utils module. Remove the local waitFor definition from tests/unit/watch.test.ts at lines 644-654 and import the shared helper; likewise remove the local definition from tests/unit/stdio-server.test.ts at lines 12-22 and import that same helper, preserving existing call behavior.src/runtime/task-applicability.ts (1)
364-396: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSame worktree/graph-detection heuristic hand-duplicated in three places.
hasMadarGraph/WORKSPACE_GRAPH_CHECKindependently re-implement "walk up from cwd checking forout/graph.jsonor a.gitfile (linked worktree)" in three different syntaxes (CommonJS, ESM, and a compactnode -eone-liner). A future change to this detection logic (e.g. handling git submodules that also use a.gitfile, or adding another worktree signal) risks being applied to only one or two of the three copies, silently diverging hook behavior across agent platforms.
src/runtime/task-applicability.ts#L364-L396: treat this as the canonical multi-line implementation; extract it into a single shared string-template builder that the other two sites derive their compacted/ESM variants from.src/infrastructure/install.ts#L131-L142: generateWORKSPACE_GRAPH_CHECK's compact one-liner from the same shared template instead of a hand-minified copy.src/infrastructure/install.ts#L619-L644: generateOPENCODE_PLUGIN_JS'shasMadarGraphfrom the same shared template (ESM-flavored) instead of a hand-written copy.🤖 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 `@src/runtime/task-applicability.ts` around lines 364 - 396, Extract the canonical worktree/graph-detection logic from task-applicability.ts#L364-L396 into a shared string-template builder, then derive all variants from it. Update src/runtime/task-applicability.ts#L364-L396, src/infrastructure/install.ts#L131-L142 (WORKSPACE_GRAPH_CHECK), and src/infrastructure/install.ts#L619-L644 (OPENCODE_PLUGIN_JS) so their CommonJS, compact, and ESM forms share the same walk-up detection behavior.src/pipeline/federate.ts (1)
39-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-reading the graph file just to infer the repo name.
loadSourceGraphalready parses the JSON; threadparsed.root_paththrough toinferRepoName(or pass a pre-parsed carrier intoresolveGraphSourceRoot) instead of callingreadGraphSourceRoot(graphPath)again. That removes an extra disk read and JSON parse on the federate path.🤖 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 `@src/pipeline/federate.ts` around lines 39 - 64, Update loadSourceGraph to return the parsed root_path alongside the KnowledgeGraph, then change inferRepoName to use that parsed value instead of calling readGraphSourceRoot(graphPath). Propagate the new value through the federate path so repository-name inference reuses the existing parse and avoids another file read.
🤖 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 `@src/infrastructure/watch.ts`:
- Around line 175-193: Merge graphBelongsToWorkspace and graphUsesSpi into a
single graph metadata read that performs one readFileSync and JSON.parse, then
returns both the workspace-membership and SPI-mode values. Update rebuildCode’s
generateGraph options construction to consume those results instead of invoking
both independent helpers, while preserving the existing false behavior for
unreadable or invalid graph files.
In `@src/pipeline/export.ts`:
- Around line 381-394: Update writeFileAtomically to retry renameSync when
Windows transient lock errors EPERM or EBUSY occur, using a small bounded
backoff between attempts. Preserve same-directory atomic replacement, rethrow
non-transient errors and the lock error after retries are exhausted, and retain
temporary-file cleanup in the finally block.
In `@src/runtime/stdio-server.ts`:
- Around line 162-170: Update graphRootPath to avoid the uncached loadGraph and
KnowledgeGraph rebuild on each tools/list request; reuse the existing cached
graph-loading path, such as loadGraphCached, while preserving validation and the
current null-on-failure behavior when reading graph.graph.root_path.
In `@src/shared/workspace.ts`:
- Around line 30-41: Memoize resolveMadarWorkspace by resolved rootPath so
repeated cacheDir lookups reuse the previously computed workspace instead of
rerunning its git commands. Add caching at the resolveMadarWorkspace boundary,
preserve its existing result and failure behavior, and ensure different rootPath
values remain independently cached.
---
Outside diff comments:
In `@src/runtime/mcp-response-evidence.ts`:
- Around line 129-171: The scope check currently reads and parses graph.json
before evaluating the cheap path match. In the relevant scope-validation
function, evaluate normalizedGraphPath against expectedGraphPath first and
return high confidence immediately when it matches; otherwise use an optional
caller-provided graph.graph.root_path for source-root validation, falling back
to readGraphSourceRoot only when that value is unavailable. Update the caller to
pass the existing root_path through without rereading the artifact.
---
Nitpick comments:
In `@src/infrastructure/watch.ts`:
- Around line 167-173: Make sameFilesystemPath a single exported implementation
in src/infrastructure/watch.ts (or shared/workspace.ts), preserving its
realpathSync/resolve behavior; in src/runtime/stdio-server.ts, remove the
duplicate helper and import the shared symbol instead.
In `@src/pipeline/federate.ts`:
- Around line 39-64: Update loadSourceGraph to return the parsed root_path
alongside the KnowledgeGraph, then change inferRepoName to use that parsed value
instead of calling readGraphSourceRoot(graphPath). Propagate the new value
through the federate path so repository-name inference reuses the existing parse
and avoids another file read.
In `@src/runtime/task-applicability.ts`:
- Around line 364-396: Extract the canonical worktree/graph-detection logic from
task-applicability.ts#L364-L396 into a shared string-template builder, then
derive all variants from it. Update src/runtime/task-applicability.ts#L364-L396,
src/infrastructure/install.ts#L131-L142 (WORKSPACE_GRAPH_CHECK), and
src/infrastructure/install.ts#L619-L644 (OPENCODE_PLUGIN_JS) so their CommonJS,
compact, and ESM forms share the same walk-up detection behavior.
In `@tests/unit/watch.test.ts`:
- Around line 644-654: The duplicated async waitFor polling helper should live
in a shared test-utils module. Remove the local waitFor definition from
tests/unit/watch.test.ts at lines 644-654 and import the shared helper; likewise
remove the local definition from tests/unit/stdio-server.test.ts at lines 12-22
and import that same helper, preserving existing call 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4987768d-b542-4c17-b404-8dc8ff4cc841
📒 Files selected for processing (47)
.github/scripts/validate-mcp-registry.mjsCHANGELOG.mdREADME.mddocs/mcp-registry/server.jsondocs/reference/cli-and-mcp.mdpackage-lock.jsonpackage.jsonsbom.cdx.jsonsrc/cli/main.tssrc/cli/parser.tssrc/infrastructure/benchmark.tssrc/infrastructure/benchmark/quality.tssrc/infrastructure/benchmark/runner.tssrc/infrastructure/cache.tssrc/infrastructure/compare.tssrc/infrastructure/doctor.tssrc/infrastructure/generate.tssrc/infrastructure/handoff-command.tssrc/infrastructure/install.tssrc/infrastructure/proof-report.tssrc/infrastructure/review-compare.tssrc/infrastructure/time-travel.tssrc/infrastructure/try-command.tssrc/infrastructure/watch.tssrc/pipeline/export.tssrc/pipeline/federate.tssrc/pipeline/spi/cache.tssrc/runtime/mcp-response-evidence.tssrc/runtime/stdio-server.tssrc/runtime/stdio/tools.tssrc/runtime/task-applicability.tssrc/shared/graph-source-root.tssrc/shared/security.tssrc/shared/workspace.tstests/unit/cli.test.tstests/unit/compare-native-agent.test.tstests/unit/doctor.test.tstests/unit/install.test.tstests/unit/mcp-registry-metadata.test.tstests/unit/mcp-response-evidence.test.tstests/unit/pipeline.test.tstests/unit/stdio-pr-impact.test.tstests/unit/stdio-server.test.tstests/unit/time-travel-infrastructure.test.tstests/unit/watch.test.tstests/unit/workspace.test.tstests/unit/worktree-cli-artifacts.test.ts
| function graphBelongsToWorkspace(graphPath: string, workspaceRoot: string): boolean { | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(graphPath, 'utf8')) as { root_path?: unknown } | ||
| return typeof parsed.root_path === 'string' | ||
| && parsed.root_path.trim().length > 0 | ||
| && sameFilesystemPath(parsed.root_path, workspaceRoot) | ||
| } catch { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| function graphUsesSpi(graphPath: string): boolean { | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(graphPath, 'utf8')) as { spi_mode?: unknown } | ||
| return parsed.spi_mode === true | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Two full, separate graph.json parses per rebuild just to read two scalar fields.
graphBelongsToWorkspace and graphUsesSpi each independently readFileSync + JSON.parse the entire graph.json. Both are called back-to-back while building generateGraph's options in rebuildCode (lines 449-451), so every single debounced rebuild during an active watch/auto-refresh session fully re-parses the same large file twice just to read root_path and spi_mode. For the large repos this tool targets, that's a repeatable, avoidable I/O+CPU cost on a hot path.
🐛 Proposed fix: merge into one read
-function graphBelongsToWorkspace(graphPath: string, workspaceRoot: string): boolean {
- try {
- const parsed = JSON.parse(readFileSync(graphPath, 'utf8')) as { root_path?: unknown }
- return typeof parsed.root_path === 'string'
- && parsed.root_path.trim().length > 0
- && sameFilesystemPath(parsed.root_path, workspaceRoot)
- } catch {
- return false
- }
-}
-
-function graphUsesSpi(graphPath: string): boolean {
- try {
- const parsed = JSON.parse(readFileSync(graphPath, 'utf8')) as { spi_mode?: unknown }
- return parsed.spi_mode === true
- } catch {
- return false
- }
-}
+function readGraphRefreshMetadata(graphPath: string): { rootPath: string | null; usesSpi: boolean } {
+ try {
+ const parsed = JSON.parse(readFileSync(graphPath, 'utf8')) as { root_path?: unknown; spi_mode?: unknown }
+ return {
+ rootPath: typeof parsed.root_path === 'string' && parsed.root_path.trim().length > 0 ? parsed.root_path.trim() : null,
+ usesSpi: parsed.spi_mode === true,
+ }
+ } catch {
+ return { rootPath: null, usesSpi: false }
+ }
+}And at the call site:
- result = generateGraph(resolvedWatchPath, {
- ...(existsSync(manifestPath) && existsSync(graphPath) && graphBelongsToWorkspace(graphPath, resolvedWatchPath) ? { update: true } : {}),
- ...(graphUsesSpi(graphPath) ? { useSpi: true } : {}),
+ const existingGraphMetadata = existsSync(manifestPath) && existsSync(graphPath) ? readGraphRefreshMetadata(graphPath) : null
+ result = generateGraph(resolvedWatchPath, {
+ ...(existingGraphMetadata?.rootPath && sameFilesystemPath(existingGraphMetadata.rootPath, resolvedWatchPath) ? { update: true } : {}),
+ ...(existingGraphMetadata?.usesSpi ? { useSpi: true } : {}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/infrastructure/watch.ts` around lines 175 - 193, Merge
graphBelongsToWorkspace and graphUsesSpi into a single graph metadata read that
performs one readFileSync and JSON.parse, then returns both the
workspace-membership and SPI-mode values. Update rebuildCode’s generateGraph
options construction to consume those results instead of invoking both
independent helpers, while preserving the existing false behavior for unreadable
or invalid graph files.
| function writeFileAtomically(outputPath: string, content: string): void { | ||
| const temporaryPath = join( | ||
| dirname(outputPath), | ||
| `.madar-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}.tmp`, | ||
| ) | ||
|
|
||
| try { | ||
| writeFileSync(temporaryPath, content, 'utf8') | ||
| renameSync(temporaryPath, outputPath) | ||
| } finally { | ||
| rmSync(temporaryPath, { force: true }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.renameSync throw EPERM or EBUSY on Windows when the destination file is open for reading in another process, and is there a recommended retry pattern?
💡 Result:
Yes, Node.js fs.renameSync (and its asynchronous counterpart fs.rename) will frequently throw EPERM or EBUSY errors on Windows when the destination file or source file is held open by another process [1][2][3][4]. Unlike POSIX systems (Linux/macOS), where renaming is generally an atomic operation that can proceed even if a file is open, Windows enforces mandatory file locking [2][4]. If any process—including antivirus software, the Windows Search indexer, a text editor, or another instance of Node.js—has an active handle on the file, the operating system will block the rename operation, and Node.js will surface this as an EPERM or EBUSY error [2][5][6][4]. Recommended Retry Pattern Because these locks are often transient (e.g., an antivirus scan finishing or a file being released by a background process), a common and recommended solution is to implement a retry mechanism with exponential backoff [2]. 1. Manual Implementation: You can wrap the fs.renameSync call in a loop with a small delay [6]. Example (Conceptual): javascript const fs = require('fs'); function renameWithRetry(src, dest, retries = 5, delay = 100) { for (let i = 0; i < retries; i++) { try { fs.renameSync(src, dest); return; // Success } catch (err) { if ((err.code === 'EPERM' || err.code === 'EBUSY') && i < retries - 1) { // Wait before retrying const start = Date.now(); while (Date.now() - start < delay) {} delay *= 2; // Exponential backoff } else { throw err; // Rethrow if not a transient error or retries exhausted } } } } 2. Using graceful-fs: The community-maintained library graceful-fs is the standard approach to resolve this issue [3][6][4]. It automatically patches the Node.js fs module on Windows to include an internal retry mechanism for EPERM, EACCES, and EBUSY errors, sparing you from implementing manual logic [3][4]. Best Practice: - If the file lock is held by your own application (e.g., a file stream that hasn't finished writing), ensure the stream is fully closed—typically by waiting for the 'close' or 'end' event—before attempting to rename [2]. - Use the graceful-fs library if you are working in an environment where third-party file locks (like antivirus) are unpredictable [3][4].
Citations:
- 1: EPERM when renaming files on Windows nodejs/node#29481
- 2: https://errornotes.dev/en/errors/nodejs/fix-eperm-operation-not-permitted-rename-error-in-nodejs-on-windows
- 3: [BUG] EPERM on Windows with
install-strategy=linked:fs.renamefails inwrite-file-atomicnpm/cli#9021 - 4: Improved workarounds for win32 isaacs/node-graceful-fs#119
- 5: https://stackoverflow.com/questions/56646016/nodejs-itself-keeps-file-ebusy-on-windows
- 6: https://stackoverflow.com/questions/32457363/eperm-while-renaming-directory-in-node-js-randomly
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols.
ast-grep outline src/pipeline/export.ts --view expanded || true
# Show the relevant section with line numbers.
sed -n '350,430p' src/pipeline/export.ts | cat -n
# Search for other rename/write atomic helpers or retry logic in the repo.
rg -n "renameSync|writeFileAtomically|EBUSY|EPERM|retry|backoff|graceful-fs" src . --glob '!**/node_modules/**' || trueRepository: mohanagy/madar
Length of output: 41645
Retry renameSync on Windows transient lock errors. The same-directory rename keeps the JSON write atomic, but it can still throw EPERM/EBUSY on Windows when another process briefly holds the target file. A small retry/backoff here would keep exports from failing on a transient lock.
🤖 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 `@src/pipeline/export.ts` around lines 381 - 394, Update writeFileAtomically to
retry renameSync when Windows transient lock errors EPERM or EBUSY occur, using
a small bounded backoff between attempts. Preserve same-directory atomic
replacement, rethrow non-transient errors and the lock error after retries are
exhausted, and retain temporary-file cleanup in the finally block.
| function graphRootPath(graphPath: string): string | null { | ||
| try { | ||
| const graph = loadGraph(validateGraphPath(graphPath)) | ||
| const rootPath = graph.graph.root_path | ||
| return typeof rootPath === 'string' && rootPath.trim().length > 0 ? rootPath.trim() : null | ||
| } catch { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
graphRootPath does a full uncached graph load on every tools/list call.
graphRootPath calls loadGraph(validateGraphPath(graphPath)) - a full JSON parse + KnowledgeGraph rebuild - purely to read the root_path string field. It's invoked on every tools/list request (line 669), whereas the same diff's compareRefs override (line 696) correctly reuses loadGraphCached. For large graphs (this tool's stated target: large TS/Node repos) this adds unnecessary parse/build latency to a request that previously only did a cheap dirname(graphPath) lookup.
🐛 Proposed fix
function graphRootPath(graphPath: string): string | null {
try {
- const graph = loadGraph(validateGraphPath(graphPath))
+ const graph = loadGraphCached(graphPath)
const rootPath = graph.graph.root_path
return typeof rootPath === 'string' && rootPath.trim().length > 0 ? rootPath.trim() : null
} catch {
return null
}
}Also applies to: 669-669
🤖 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 `@src/runtime/stdio-server.ts` around lines 162 - 170, Update graphRootPath to
avoid the uncached loadGraph and KnowledgeGraph rebuild on each tools/list
request; reuse the existing cached graph-loading path, such as loadGraphCached,
while preserving validation and the current null-on-failure behavior when
reading graph.graph.root_path.
| function gitPath(rootPath: string, args: string[]): string | null { | ||
| try { | ||
| const value = execFileSync('git', ['-C', rootPath, ...args], { | ||
| encoding: 'utf8', | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| windowsHide: true, | ||
| }).trim() | ||
| return value.length > 0 ? value : null | ||
| } catch { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files src/shared/workspace.ts src/infrastructure/cache.ts
printf '\n## outline workspace.ts\n'
ast-grep outline src/shared/workspace.ts --view expanded || true
printf '\n## outline cache.ts\n'
ast-grep outline src/infrastructure/cache.ts --view expanded || true
printf '\n## relevant excerpts\n'
sed -n '1,220p' src/shared/workspace.ts | cat -n
printf '\n--- cache.ts ---\n'
sed -n '1,240p' src/infrastructure/cache.ts | cat -nRepository: mohanagy/madar
Length of output: 11944
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- workspace.ts lines 1-180 ---'
sed -n '1,180p' src/shared/workspace.ts | cat -n
echo '--- cache.ts lines 1-220 ---'
sed -n '1,220p' src/infrastructure/cache.ts | cat -nRepository: mohanagy/madar
Length of output: 10305
Memoize resolveMadarWorkspace
resolveMadarWorkspace() runs up to three synchronous git commands on every call, and src/infrastructure/cache.ts’s cacheDir() reaches it on each cache lookup. Cache the workspace by resolved rootPath to avoid repeated subprocess spawns on large repos.
🤖 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 `@src/shared/workspace.ts` around lines 30 - 41, Memoize resolveMadarWorkspace
by resolved rootPath so repeated cacheDir lookups reuse the previously computed
workspace instead of rerunning its git commands. Add caching at the
resolveMadarWorkspace boundary, preserve its existing result and failure
behavior, and ensure different rootPath values remain independently cached.
1 participant
Summary
Verification
After this PR merges and npm is published, push annotated tag v0.30.0 to trigger the GitHub Release workflow.
Summary by CodeRabbit