feat(autoresearch): autonomous experiment engine - #922
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA comprehensive autonomous research engine (AutoResearch) has been added to NeuroLink, enabling unattended AI-driven experiment loops with git-backed safety, deterministic regex-based metric evaluation, and phase-gated tool access. The implementation includes TypeScript libraries, CLI commands, TaskManager integration, type definitions, documentation, examples, and extensive test fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client/CLI
participant W as ResearchWorker
participant PM as PromptCompiler
participant E as ExperimentRunner
participant RR as ResultRecorder
participant RP as RepoPolicy
C->>W: initialize(tag)
W->>W: create/checkout autoresearch/<br>branch
W->>W: init ResearchStateStore
loop Each Experiment Cycle
W->>W: runExperimentCycle()
W->>PM: buildCyclePrompt(state)
PM-->>W: cycle prompt with state/results
W->>E: run() command
E->>E: spawn process with<br>timeout/capture
E-->>W: ExperimentSummary<br>(metric, crashed, timedOut)
W->>RR: appendTsv(record)
RR-->>W: recorded
W->>RP: validateCommit()
RP-->>W: {valid, violations}
W->>W: decide outcome<br>(keep/discard/crash)
alt Keep
W->>W: update bestMetric
W->>W: git commit candidate
else Discard/Crash
W->>W: git reset acceptedCommit
end
W-->>C: ExperimentRecord
end
sequenceDiagram
participant TM as TaskManager
participant TE as TaskExecutor
participant AE as AutoresearchTaskExecutor
participant W as ResearchWorker
participant NL as NeuroLink.generate()
TM->>TM: create(definition)
TM->>TM: validate autoresearch config
TM->>TE: construct with emitter
loop Task Loop
TE->>AE: executeAutoresearchTick(task)
AE->>W: getOrCreateWorker()
W->>W: resume() or initialize()
AE->>W: getTools(),<br>getCyclePrompt()
AE->>NL: generate(prompt,<br>systemPrompt, tools)
NL-->>AE: result with toolCalls
AE->>W: advancePhase(nextPhase)
AE-->>TE: TaskRunResult
TE->>TE: append to continuation<br>history
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
Introduces an “AutoResearch” autonomous experiment engine to NeuroLink, enabling an AI-driven propose→edit→run→evaluate loop while keeping metric evaluation and repo safety decisions deterministic in TypeScript.
Changes:
- Adds a new
src/lib/autoresearch/module (config, state store, repo policy, runner, parser, recorder, tools, worker, phase policy, exports). - Adds a new
neurolink autoresearchCLI command group plus command registration. - Adds an end-to-end continuous test suite with fixture scripts and an accompanying design doc/demo.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/types/autoresearchTypes.ts | Adds core AutoResearch types and defaults. |
| src/lib/types/index.ts | Re-exports AutoResearch types/defaults through the types barrel. |
| src/lib/autoresearch/config.ts | Resolves defaults and validates AutoResearch configuration. |
| src/lib/autoresearch/errors.ts | Defines AutoResearch error codes via the shared error factory. |
| src/lib/autoresearch/stateStore.ts | Implements file-backed JSON state persistence with atomic writes. |
| src/lib/autoresearch/repoPolicy.ts | Enforces read/write boundaries and validates git operations. |
| src/lib/autoresearch/summaryParser.ts | Parses run logs deterministically into structured summaries. |
| src/lib/autoresearch/resultRecorder.ts | Records experiment results to TSV and optional JSONL audit trail. |
| src/lib/autoresearch/runner.ts | Runs experiments with timeout, captures output, writes run.log. |
| src/lib/autoresearch/tools.ts | Adds 12 Vercel AI SDK tools for repo/experiment lifecycle. |
| src/lib/autoresearch/phasePolicy.ts | Defines phase-based tool access policies. |
| src/lib/autoresearch/promptCompiler.ts | Builds system + cycle prompts from program/state/results. |
| src/lib/autoresearch/worker.ts | Orchestrates deterministic experiment cycles and state updates. |
| src/lib/autoresearch/index.ts | Public API exports for the AutoResearch module. |
| src/cli/commands/autoresearch.ts | Adds neurolink autoresearch CLI (init/status/results/run-once). |
| src/cli/parser.ts | Registers the new CLI command group. |
| test/continuous-test-suite-autoresearch.ts | Adds an end-to-end continuous test suite (24 tests) for the feature. |
| test/fixtures/autoresearch/train.py | Fixture “training” script emitting deterministic metrics. |
| test/fixtures/autoresearch/slow.py | Fixture script used to verify timeout handling. |
| test/fixtures/autoresearch/crash.py | Fixture script used to verify crash detection/handling. |
| test/fixtures/autoresearch/program.md | Fixture research program text (constraints/instructions). |
| examples/autoresearch/run-demo.ts | Demo script to run multiple AI-driven cycles on a local repo. |
| docs/2026-04-03-autoresearch-design.md | Design specification for the feature and intended workflows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+31
| isWriteAllowed(filePath: string): boolean { | ||
| const resolved = path.resolve(this.config.repoPath, filePath); | ||
| // Must be inside repoPath | ||
| if (!resolved.startsWith(this.config.repoPath)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isWriteAllowed()/isReadAllowed() use resolved.startsWith(this.config.repoPath) to enforce “inside repo”. This is vulnerable to prefix collisions (e.g. repoPath /tmp/repo will match /tmp/repo2/...). Use a path-boundary check (e.g. path.relative() and ensure it doesn’t start with .. and isn’t absolute) or compare against repoPath + path.sep.
Comment on lines
+104
to
+110
| try { | ||
| const output = execSync("git diff --cached --name-only", { | ||
| cwd: this.config.repoPath, | ||
| encoding: "utf-8", | ||
| }); | ||
| return output.trim().split("\n").filter(Boolean); | ||
| } catch { |
There was a problem hiding this comment.
This file uses execSync("git ...") which invokes a shell. The PR description calls out shell-safe git ops via execFileSync argument arrays; consider switching these to execFileSync("git", [...]) for consistency and to avoid shell invocation.
Comment on lines
+81
to
+90
| // Verify metric pattern is valid regex with one capture group | ||
| try { | ||
| const _regex = new RegExp(config.metric.pattern); | ||
| const match = new RegExp(config.metric.pattern + "|").exec(""); | ||
| const groupCount = (match?.length ?? 1) - 1; | ||
| if (groupCount < 1) { | ||
| throw AutoresearchError.create( | ||
| "CONFIG_INVALID", | ||
| `metric.pattern must have at least one capture group: ${config.metric.pattern}`, | ||
| ); |
There was a problem hiding this comment.
The comment says metric.pattern is a regex with one capture group, but validation only checks for at least one capture group (groupCount < 1). Either enforce exactly one group (since parsing uses only match[1]) or update the comment/docs to avoid misleading config authors.
| const timedOut = metric === null && !crashed; | ||
|
|
||
| return { | ||
| crashed: crashed || (metric === null && !timedOut), |
There was a problem hiding this comment.
crashed: crashed || (metric === null && !timedOut) is redundant because timedOut is defined as metric === null && !crashed, making the second clause always false. This looks like a logic bug / leftover and should be simplified (and/or adjust the timeout/crash derivation if different behavior was intended).
Suggested change
| crashed: crashed || (metric === null && !timedOut), | |
| crashed, |
Comment on lines
+114
to
+117
| execSync("git init && git add -A && git commit -m 'init'", { | ||
| cwd: FIXTURE_REPO, | ||
| stdio: "ignore", | ||
| }); |
There was a problem hiding this comment.
The fixture repo setup runs git commit without configuring user.name/user.email. In many CI environments this causes the commit to fail, breaking the suite. Consider setting local config in the fixture repo before committing (or using git -c user.name=... -c user.email=... commit ...).
Suggested change
| execSync("git init && git add -A && git commit -m 'init'", { | |
| cwd: FIXTURE_REPO, | |
| stdio: "ignore", | |
| }); | |
| execSync( | |
| "git init && git config user.name 'Test Fixture' && git config user.email 'test-fixture@example.com' && git add -A && git commit -m 'init'", | |
| { | |
| cwd: FIXTURE_REPO, | |
| stdio: "ignore", | |
| }, | |
| ); |
| import { ResultRecorder } from "../../src/lib/autoresearch/resultRecorder.js"; | ||
| import { PromptCompiler } from "../../src/lib/autoresearch/promptCompiler.js"; | ||
| import { createResearchTools } from "../../src/lib/autoresearch/tools.js"; | ||
| import { parseExperimentSummary } from "../../src/lib/autoresearch/summaryParser.js"; |
There was a problem hiding this comment.
parseExperimentSummary is imported but never used in this demo file. Please remove the unused import to avoid lint failures and keep the example minimal.
Suggested change
| import { parseExperimentSummary } from "../../src/lib/autoresearch/summaryParser.js"; |
Comment on lines
+112
to
+113
| export type ResearchWorkerConfig = { | ||
| config: ResearchConfig; |
There was a problem hiding this comment.
ResearchWorkerConfig is exported as { config: ResearchConfig }, but ResearchWorker’s constructor actually accepts a Partial<ResearchConfig> & { repoPath; mutablePaths; runCommand; metric; }. This mismatch can confuse API consumers and weakens typing. Either update ResearchWorkerConfig to match the constructor/public API, or change ResearchWorker to accept ResearchWorkerConfig.
Suggested change
| export type ResearchWorkerConfig = { | |
| config: ResearchConfig; | |
| export type ResearchWorkerConfig = Omit< | |
| Partial<ResearchConfig>, | |
| "repoPath" | "mutablePaths" | "runCommand" | "metric" | |
| > & { | |
| repoPath: string; | |
| mutablePaths: string[]; | |
| runCommand: string; | |
| metric: MetricConfig; |
Comment on lines
+35
to
+39
| const proc = spawn(this.config.runCommand, { | ||
| shell: true, | ||
| cwd: this.config.repoPath, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); |
There was a problem hiding this comment.
spawn(this.config.runCommand, { shell: true }) runs the command through a shell. If runCommand comes from a config file (or other untrusted input), this is a command-injection vector. Consider representing the command as { command: string; args: string[] } (similar to src/lib/mcp/mcpClientFactory.ts:345) and spawning without shell: true, or at least document that runCommand must be trusted.
Comment on lines
+251
to
+256
| const resultsPath = resolve(argv.repoPath, "results.tsv"); | ||
| if (!existsSync(resultsPath)) { | ||
| console.info(chalk.yellow("No results file.")); | ||
| return; | ||
| } | ||
| const lines = readFileSync(resultsPath, "utf-8").trim().split("\n"); |
There was a problem hiding this comment.
results subcommand hard-codes results.tsv at the repo root. The library supports configurable resultsPath, and init writes .autoresearch/config.json; consider reading resultsPath from that config so neurolink autoresearch results works with non-default paths.
murdore
force-pushed
the
feat/auto-research
branch
from
April 3, 2026 21:21
8bc1075 to
e293416
Compare
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
examples/autoresearch/run-demo.ts (2)
88-88: Wrap long async operations withwithTimeout.Lines 88, 104-106, 111-119, and 140 perform core async calls without the repository-standard timeout wrapper.
As per coding guidelines "Use
withTimeoututility to wrap async calls for error handling".Also applies to: 104-106, 111-119, 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/autoresearch/run-demo.ts` at line 88, The async calls in this file (notably promptCompiler.buildSystemPrompt and the other core async operations around lines 104-106, 111-119, and 140) are not wrapped with the repository-standard withTimeout utility; update each long-running promise (e.g., promptCompiler.buildSystemPrompt and the other async call sites in this file) to be invoked as await withTimeout(originalPromise, SOME_TIMEOUT_MS) (import withTimeout if missing), and ensure you handle the possible timeout rejection in the surrounding try/catch or promise handling so errors are logged/propagated consistently.
24-25: UseexecFileSyncwith argv arrays for git calls in the demo.At Lines 62/68/186, switching from
execSync("git ...")toexecFileSync("git", [...])improves shell-safety and keeps process execution style consistent.💡 Proposed fix
-import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; @@ - execSync("git checkout -b autoresearch/ai-demo", { + execFileSync("git", ["checkout", "-b", "autoresearch/ai-demo"], { cwd: REPO, stdio: "ignore", }); @@ - execSync("git checkout autoresearch/ai-demo", { + execFileSync("git", ["checkout", "autoresearch/ai-demo"], { cwd: REPO, stdio: "ignore", }); @@ - execSync("git log --oneline -10", { cwd: REPO, encoding: "utf-8" }), + execFileSync("git", ["log", "--oneline", "-10"], { + cwd: REPO, + encoding: "utf-8", + }),Also applies to: 62-71, 185-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/autoresearch/run-demo.ts` around lines 24 - 25, Replace unsafe execSync("git ...") calls with execFileSync("git", [...]) and import execFileSync from "node:child_process"; specifically, update the three places that call execSync for git operations (the invocations currently using execSync in run-demo.ts) to use execFileSync with the git executable as the first arg and each git token as separate array elements (e.g., execFileSync("git", ["clone", "url"], { stdio: "inherit" })). Ensure you remove shell interpolation, pass args as an array, and preserve options like stdio so behavior remains identical.src/lib/autoresearch/runner.ts (1)
32-57: PreferwithTimeoututility over custom timer logic.Line 32 currently builds a bespoke timeout wrapper. Please standardize this path with
withTimeoutso timeout/error handling stays consistent with the rest of the TS codebase.As per coding guidelines "Use
withTimeoututility to wrap async calls for error handling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/runner.ts` around lines 32 - 57, Replace the bespoke Promise+setTimeout logic in the runner (the block creating `proc` via spawn and using `timer`, `timedOut`, and manual SIGKILL) with the shared withTimeout utility: wrap the async operation that collects stdout/stderr from `spawn(this.config.runCommand, { cwd: this.config.repoPath, shell: true, stdio: [...] })` in `withTimeout(..., this.config.timeoutMs)` so the utility drives timeout rejection; ensure you still attach `proc.stdout`/`proc.stderr` handlers to accumulate `output`, call `proc.kill("SIGKILL")` when the wrapped promise rejects for timeout, clear any local cleanup, and import `withTimeout` where `logContent` is assigned to maintain consistent timeout/error handling across the codebase.src/lib/autoresearch/resultRecorder.ts (3)
61-64: Wrap debug logging withlogger.shouldLog('debug')check.As per coding guidelines, expensive serialization should be guarded with a log level check to avoid unnecessary object creation when debug logging is disabled.
♻️ Suggested fix
- logger.debug("[Autoresearch] Appended TSV record", { - commit: record.commit, - status: record.status, - }); + if (logger.shouldLog('debug')) { + logger.debug("[Autoresearch] Appended TSV record", { + commit: record.commit, + status: record.status, + }); + }As per coding guidelines: "Always wrap expensive serialization with
logger.shouldLog('debug')before calling it".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/resultRecorder.ts` around lines 61 - 64, The debug log call in resultRecorder.ts that calls logger.debug("[Autoresearch] Appended TSV record", { commit: record.commit, status: record.status }) should be guarded by logger.shouldLog('debug') to avoid building the metadata object when debug is disabled; wrap the creation of the object and the logger.debug invocation in an if (logger.shouldLog('debug')) { ... } block (referencing the logger.debug call and the record.commit/record.status usage) so serialization only happens when debug logging is enabled.
141-150: Minor inefficiency: multiple filter passes over records.
getStats()iterates overrecordsfour times (for discard, crash, timeout counts). Consider a single-pass approach for better performance with large result sets.♻️ Optional single-pass approach
async getStats(): Promise<ExperimentStats> { const records = await this.readAll(); - const keeps = records.filter((r) => r.status === "keep"); - const bestKeep = keeps.reduce<ExperimentRecord | null>((best, r) => { + let keepCount = 0; + let discardCount = 0; + let crashCount = 0; + let timeoutCount = 0; + let bestKeep: ExperimentRecord | null = null; + + for (const r of records) { + switch (r.status) { + case "keep": + keepCount++; + if (r.metric !== null) { + if (bestKeep === null || bestKeep.metric === null) { + bestKeep = r; + } else if (this.config.metric.direction === "lower") { + if (r.metric < bestKeep.metric) bestKeep = r; + } else { + if (r.metric > bestKeep.metric) bestKeep = r; + } + } + break; + case "discard": discardCount++; break; + case "crash": crashCount++; break; + case "timeout": timeoutCount++; break; + } + } + + return { + total: records.length, + keepCount, + discardCount, + crashCount, + timeoutCount, + keepRate: records.length > 0 ? keepCount / records.length : 0, + bestMetric: bestKeep?.metric ?? null, + bestCommit: bestKeep?.commit ?? null, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/resultRecorder.ts` around lines 141 - 150, The getStats() implementation currently calls records.filter(...) multiple times to compute discardCount, crashCount, and timeoutCount; change it to a single-pass tally: iterate once over records (e.g., forEach or for...of inside getStats) and increment counters for discard, crash, timeout, and optionally build keeps/track bestKeep during that same pass so you avoid multiple scans; then compute total, keepCount, keepRate, bestMetric and bestCommit from the accumulated counters/keeps. Ensure you reference and update the same symbols used now (getStats, records, keeps, bestKeep) so behavior remains identical.
119-121: Silent error swallowing may mask legitimate failures.
readAll()catches all exceptions and returns an empty array, making it impossible to distinguish between an empty file and a read/parse failure. Consider logging a warning on parse errors so issues are discoverable.♻️ Suggested improvement
} catch { + logger.warn("[Autoresearch] Failed to read results file", { + path: this.tsvPath, + }); return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/resultRecorder.ts` around lines 119 - 121, The readAll() function currently swallows all exceptions and returns an empty array, hiding parse/read failures; update its catch block to capture the error (e.g. catch (err)) and log a warning with the error details and context (use the module's logger or console.warn) before returning an empty array so parse errors are discoverable; locate the readAll function in resultRecorder.ts (and any surrounding ResultRecorder class or helper) and replace the bare catch with one that logs the error message and stack along with a short contextual message identifying the file/operation.test/continuous-test-suite-autoresearch.ts (1)
1191-1195: Potential test fragility: git commit may fail if no staged changes.If
train.pycontent is identical to what's already in the repo (e.g., due to a previous failed test run leaving artifacts), thegit add+git commitwill fail with "nothing to commit". Consider using--allow-emptyor checking if there are staged changes first.♻️ More robust commit
writeFileSync(originalTrainPath, worseTrainPy, "utf-8"); - execSync("git add train.py && git commit -m 'worse experiment'", { + execSync("git add train.py && git diff --cached --quiet || git commit -m 'worse experiment'", { cwd: FIXTURE_REPO, stdio: "ignore", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-autoresearch.ts` around lines 1191 - 1195, The git commit in the test is fragile because execSync runs "git add train.py && git commit -m 'worse experiment'" which fails if there are no staged changes; modify the test around writeFileSync(originalTrainPath, worseTrainPy, "utf-8") and the execSync call to handle an empty commit case by either using "git commit --allow-empty -m 'worse experiment'" or by detecting staged changes (e.g., via "git diff --staged --quiet" or "git status --porcelain") before committing and only running git commit when there are changes, so the execSync call against FIXTURE_REPO never errors when train.py is unchanged.src/lib/autoresearch/promptCompiler.ts (2)
21-80: Consider makingbuildSystemPrompt()synchronous.The method is declared
asyncbut performs only synchronous operations (readFileSync). Either:
- Make it synchronous and return
stringdirectly, or- If you want to support async file reading in the future, use
fs/promises.readFilewithawait♻️ Suggested refactor (sync version)
- async buildSystemPrompt(): Promise<string> { + buildSystemPrompt(): string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/promptCompiler.ts` around lines 21 - 80, The method buildSystemPrompt is declared async but only uses synchronous IO (readFileSync); change it to a synchronous function by removing the async keyword and updating the signature from buildSystemPrompt(): Promise<string> to buildSystemPrompt(): string, keep the readFileSync usage and the same return value, and then update any call sites that currently await buildSystemPrompt() to treat it as a synchronous call; alternatively, if you prefer async behavior instead, replace readFileSync with fs/promises.readFile and await it inside buildSystemPrompt while keeping the Promise<string> signature.
83-114:buildCyclePrompt()is also synchronous internally.Similar to
buildSystemPrompt(), this method is declaredasyncbut contains noawaitexpressions. The method body is purely synchronous string manipulation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/promptCompiler.ts` around lines 83 - 114, The buildCyclePrompt function is declared async but performs only synchronous work; remove the async keyword and change its signature from async buildCyclePrompt(...): Promise<string> to buildCyclePrompt(...): string, updating any callers that await its result (either remove the await or wrap calls with Promise.resolve(...) if they must still receive a promise); locate the function by name buildCyclePrompt in promptCompiler.ts and mirror the same synchronous signature used by buildSystemPrompt.src/lib/autoresearch/tools.ts (1)
54-653: Function exceeds maximum line limit (600 lines vs 300 allowed).The static analysis correctly identifies that
createResearchToolsis too large. Consider splitting the tools into logical groups in separate files and re-exporting from an index:
contextTools.ts:research_get_context,research_checkpointfileTools.ts:research_read_file,research_write_candidate,research_diffgitTools.ts:research_commit_candidate,research_revertexperimentTools.ts:research_run_experiment,research_parse_log,research_inspect_failurerecordTools.ts:research_record,research_accept♻️ Example structure
// tools/index.ts export function createResearchTools(deps: ResearchToolsDeps) { return { ...createContextTools(deps), ...createFileTools(deps), ...createGitTools(deps), ...createExperimentTools(deps), ...createRecordTools(deps), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/tools.ts` around lines 54 - 653, createResearchTools is too large (over 600 lines); split its tool implementations into smaller modules and recompose them to stay under the 300-line limit. Create separate factories like createContextTools (research_get_context, research_checkpoint), createFileTools (research_read_file, research_write_candidate, research_diff), createGitTools (research_commit_candidate, research_revert), createExperimentTools (research_run_experiment, research_parse_log, research_inspect_failure), and createRecordTools (research_record, research_accept) that each accept ResearchToolsDeps and return an object of tool(...) entries, then modify createResearchTools to import and merge those with object spread (e.g., return { ...createContextTools(deps), ...createFileTools(deps), ... }) preserving existing function names and behavior and reusing stateStore, config, repoPolicy, runner, recorder.src/cli/commands/autoresearch.ts (1)
293-295: WraprunExperimentCyclecall withwithTimeoutfor timeout protection.Per coding guidelines, async calls should use
withTimeoutfor error handling. The experiment cycle performs multiple operations (state management, experiment execution, git operations, file I/O) and could hang if the runner encounters issues. Add timeout wrapping or verify the internal timeout is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/autoresearch.ts` around lines 293 - 295, Wrap the call to ResearchWorker.runExperimentCycle with the withTimeout helper to guard against hangs: replace the direct await worker.runExperimentCycle(argv.description) with an awaited withTimeout invocation that calls runExperimentCycle (e.g., withTimeout(() => worker.runExperimentCycle(argv.description), timeoutMs) or withTimeout(worker.runExperimentCycle.bind(worker, argv.description), timeoutMs)); pick a reasonable timeout value (use an existing config value like configRaw.experimentTimeoutMs if available, otherwise define one) and ensure the function still throws on timeout so caller error handling remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/autoresearch.ts`:
- Around line 251-255: The code hardcodes "results.tsv" when building
resultsPath in the autoresearch command; update the logic in the module (the
const resultsPath declaration in src/cli/commands/autoresearch.ts) to use the
configured value (e.g. config.resultsPath) instead of the literal string,
falling back to "results.tsv" if config.resultsPath is missing/empty, and ensure
resolve(argv.repoPath, ...) uses that variable so the CLI looks in the
configured results location.
In `@src/lib/autoresearch/config.ts`:
- Around line 81-99: The metric.pattern validation currently allows multiple
capture groups and compiles the user regex unsafely; update the validation
around config.metric.pattern to require exactly one capture group (i.e., enforce
groupCount === 1, not < 1) and replace the unsafe RegExp compilation with a
ReDoS-safe engine (for example use the RE2 constructor or a timeout-wrapped
compile) when creating and testing the pattern; keep using
AutoresearchError.create for error cases and preserve the existing error branch
that rethrows AUTORESEARCH errors, but throw CONFIG_INVALID with distinct
messages for "must have exactly one capture group" and "pattern is not a
valid/safe regex" when appropriate.
In `@src/lib/autoresearch/repoPolicy.ts`:
- Around line 71-79: The current staged-file checks hard-code results.tsv and
".autoresearch/" instead of using the configured transient paths, so custom
resultsPath/statePath can be bypassed or over-blocked; update the checks in
repoPolicy.ts to compare the staged file against this.config.resultsPath (exact
match) and this.config.statePath (contains or prefix check as appropriate)
instead of file.endsWith("results.tsv") and file.includes(".autoresearch/"), and
push violations to the same violations array when those configured paths are
matched.
- Around line 103-133: Replace unsafe execSync string invocations in
getStagedFiles, getCurrentBranch, and getHeadCommit with execFileSync("git",
[...args...]) calls using argument arrays; keep the same options (cwd:
this.config.repoPath, encoding: "utf-8") and preserve the existing try/catch
behavior and return values (empty array or empty string) so behavior is
unchanged while avoiding shell invocation and injection risk.
- Around line 16-23: The boundary checks using startsWith are unsafe; update the
validation in the constructor and wherever startsWith(this.config.repoPath) is
used (e.g., in the methods that check paths at lines around 29 and 48) to
normalize and validate with path.relative: for each resolved path in
resolvedMutablePaths and resolvedImmutablePaths and for any path-checked in
methods, compute const rel = path.relative(this.config.repoPath, resolvedPath)
and treat the path as inside the repo only if rel is not absolute and does not
start with '..' (and also reject absolute resolvedPath); apply the same
path.relative-based validation to user-configured mutablePaths/immutablePaths
before accepting them.
In `@src/lib/autoresearch/runner.ts`:
- Around line 59-66: The process "close" handlers (the proc.on("close"...)
blocks in runner.ts) currently ignore non-zero exit codes; update both handlers
(the one around the earlier close callback and the second at the 99-104 range)
to treat any code !== 0 as a crash: set the run result's crashed flag (e.g.,
output.crashed = true), record the exit code (e.g., output.exitCode = code) and
log a warning including the code, then resolve/return the output as before so
downstream logic can detect the crash.
In `@src/lib/autoresearch/stateStore.ts`:
- Around line 28-30: The current read path in the state loader uses
readFileSync(this.filePath) and JSON.parse(...) then blindly casts to
ResearchState, which can allow malformed but syntactically valid JSON to pass;
update the loader that reads the file (the code around readFileSync, JSON.parse,
and the method that returns ResearchState) to validate the parsed object has
required fields (e.g., currentPhase, runCount and any other mandatory properties
of ResearchState) and their expected types, and if validation fails throw an
error with the code STATE_CORRUPT; ensure the validation occurs immediately
after parsing and before casting/returning so any truncated/older/hand-edited
state is rejected.
In `@src/lib/autoresearch/summaryParser.ts`:
- Around line 87-97: The parser currently infers timeout by setting timedOut =
metric === null && !crashed and then using that to compute crashed, which
mislabels other failures; update the parser (the function that builds
timedOut/crashed in summaryParser.ts) to accept an explicit timeout/exit signal
parameter from runner.ts (e.g., exitSignal or timedOutFlag) and use that value
for timedOut instead of metric === null, and compute crashed as crashed ||
exitSignal (or similar) while removing the metric-based timeout inference so
only the runner-supplied signal determines timeouts.
In `@src/lib/autoresearch/worker.ts`:
- Around line 163-167: Wrap the call to this.runner.run() with the withTimeout
utility to add a defensive per-call timeout: import/use withTimeout and replace
const summary = await this.runner.run(); with something like awaiting
withTimeout(this.runner.run(), <appropriateTimeoutMs>) (choose the same timeout
value used by ExperimentRunner or a configured/default timeout) and handle the
timeout rejection so logger/cleanup still runs; keep the surrounding
logger.info(...) and assign the result to summary as before.
---
Nitpick comments:
In `@examples/autoresearch/run-demo.ts`:
- Line 88: The async calls in this file (notably
promptCompiler.buildSystemPrompt and the other core async operations around
lines 104-106, 111-119, and 140) are not wrapped with the repository-standard
withTimeout utility; update each long-running promise (e.g.,
promptCompiler.buildSystemPrompt and the other async call sites in this file) to
be invoked as await withTimeout(originalPromise, SOME_TIMEOUT_MS) (import
withTimeout if missing), and ensure you handle the possible timeout rejection in
the surrounding try/catch or promise handling so errors are logged/propagated
consistently.
- Around line 24-25: Replace unsafe execSync("git ...") calls with
execFileSync("git", [...]) and import execFileSync from "node:child_process";
specifically, update the three places that call execSync for git operations (the
invocations currently using execSync in run-demo.ts) to use execFileSync with
the git executable as the first arg and each git token as separate array
elements (e.g., execFileSync("git", ["clone", "url"], { stdio: "inherit" })).
Ensure you remove shell interpolation, pass args as an array, and preserve
options like stdio so behavior remains identical.
In `@src/cli/commands/autoresearch.ts`:
- Around line 293-295: Wrap the call to ResearchWorker.runExperimentCycle with
the withTimeout helper to guard against hangs: replace the direct await
worker.runExperimentCycle(argv.description) with an awaited withTimeout
invocation that calls runExperimentCycle (e.g., withTimeout(() =>
worker.runExperimentCycle(argv.description), timeoutMs) or
withTimeout(worker.runExperimentCycle.bind(worker, argv.description),
timeoutMs)); pick a reasonable timeout value (use an existing config value like
configRaw.experimentTimeoutMs if available, otherwise define one) and ensure the
function still throws on timeout so caller error handling remains intact.
In `@src/lib/autoresearch/promptCompiler.ts`:
- Around line 21-80: The method buildSystemPrompt is declared async but only
uses synchronous IO (readFileSync); change it to a synchronous function by
removing the async keyword and updating the signature from buildSystemPrompt():
Promise<string> to buildSystemPrompt(): string, keep the readFileSync usage and
the same return value, and then update any call sites that currently await
buildSystemPrompt() to treat it as a synchronous call; alternatively, if you
prefer async behavior instead, replace readFileSync with fs/promises.readFile
and await it inside buildSystemPrompt while keeping the Promise<string>
signature.
- Around line 83-114: The buildCyclePrompt function is declared async but
performs only synchronous work; remove the async keyword and change its
signature from async buildCyclePrompt(...): Promise<string> to
buildCyclePrompt(...): string, updating any callers that await its result
(either remove the await or wrap calls with Promise.resolve(...) if they must
still receive a promise); locate the function by name buildCyclePrompt in
promptCompiler.ts and mirror the same synchronous signature used by
buildSystemPrompt.
In `@src/lib/autoresearch/resultRecorder.ts`:
- Around line 61-64: The debug log call in resultRecorder.ts that calls
logger.debug("[Autoresearch] Appended TSV record", { commit: record.commit,
status: record.status }) should be guarded by logger.shouldLog('debug') to avoid
building the metadata object when debug is disabled; wrap the creation of the
object and the logger.debug invocation in an if (logger.shouldLog('debug')) {
... } block (referencing the logger.debug call and the
record.commit/record.status usage) so serialization only happens when debug
logging is enabled.
- Around line 141-150: The getStats() implementation currently calls
records.filter(...) multiple times to compute discardCount, crashCount, and
timeoutCount; change it to a single-pass tally: iterate once over records (e.g.,
forEach or for...of inside getStats) and increment counters for discard, crash,
timeout, and optionally build keeps/track bestKeep during that same pass so you
avoid multiple scans; then compute total, keepCount, keepRate, bestMetric and
bestCommit from the accumulated counters/keeps. Ensure you reference and update
the same symbols used now (getStats, records, keeps, bestKeep) so behavior
remains identical.
- Around line 119-121: The readAll() function currently swallows all exceptions
and returns an empty array, hiding parse/read failures; update its catch block
to capture the error (e.g. catch (err)) and log a warning with the error details
and context (use the module's logger or console.warn) before returning an empty
array so parse errors are discoverable; locate the readAll function in
resultRecorder.ts (and any surrounding ResultRecorder class or helper) and
replace the bare catch with one that logs the error message and stack along with
a short contextual message identifying the file/operation.
In `@src/lib/autoresearch/runner.ts`:
- Around line 32-57: Replace the bespoke Promise+setTimeout logic in the runner
(the block creating `proc` via spawn and using `timer`, `timedOut`, and manual
SIGKILL) with the shared withTimeout utility: wrap the async operation that
collects stdout/stderr from `spawn(this.config.runCommand, { cwd:
this.config.repoPath, shell: true, stdio: [...] })` in `withTimeout(...,
this.config.timeoutMs)` so the utility drives timeout rejection; ensure you
still attach `proc.stdout`/`proc.stderr` handlers to accumulate `output`, call
`proc.kill("SIGKILL")` when the wrapped promise rejects for timeout, clear any
local cleanup, and import `withTimeout` where `logContent` is assigned to
maintain consistent timeout/error handling across the codebase.
In `@src/lib/autoresearch/tools.ts`:
- Around line 54-653: createResearchTools is too large (over 600 lines); split
its tool implementations into smaller modules and recompose them to stay under
the 300-line limit. Create separate factories like createContextTools
(research_get_context, research_checkpoint), createFileTools
(research_read_file, research_write_candidate, research_diff), createGitTools
(research_commit_candidate, research_revert), createExperimentTools
(research_run_experiment, research_parse_log, research_inspect_failure), and
createRecordTools (research_record, research_accept) that each accept
ResearchToolsDeps and return an object of tool(...) entries, then modify
createResearchTools to import and merge those with object spread (e.g., return {
...createContextTools(deps), ...createFileTools(deps), ... }) preserving
existing function names and behavior and reusing stateStore, config, repoPolicy,
runner, recorder.
In `@test/continuous-test-suite-autoresearch.ts`:
- Around line 1191-1195: The git commit in the test is fragile because execSync
runs "git add train.py && git commit -m 'worse experiment'" which fails if there
are no staged changes; modify the test around writeFileSync(originalTrainPath,
worseTrainPy, "utf-8") and the execSync call to handle an empty commit case by
either using "git commit --allow-empty -m 'worse experiment'" or by detecting
staged changes (e.g., via "git diff --staged --quiet" or "git status
--porcelain") before committing and only running git commit when there are
changes, so the execSync call against FIXTURE_REPO never errors when train.py is
unchanged.
🪄 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: 93298150-f6ca-4cdb-b613-3d751c83b41a
📒 Files selected for processing (23)
docs/2026-04-03-autoresearch-design.mdexamples/autoresearch/run-demo.tssrc/cli/commands/autoresearch.tssrc/cli/parser.tssrc/lib/autoresearch/config.tssrc/lib/autoresearch/errors.tssrc/lib/autoresearch/index.tssrc/lib/autoresearch/phasePolicy.tssrc/lib/autoresearch/promptCompiler.tssrc/lib/autoresearch/repoPolicy.tssrc/lib/autoresearch/resultRecorder.tssrc/lib/autoresearch/runner.tssrc/lib/autoresearch/stateStore.tssrc/lib/autoresearch/summaryParser.tssrc/lib/autoresearch/tools.tssrc/lib/autoresearch/worker.tssrc/lib/types/autoresearchTypes.tssrc/lib/types/index.tstest/continuous-test-suite-autoresearch.tstest/fixtures/autoresearch/crash.pytest/fixtures/autoresearch/program.mdtest/fixtures/autoresearch/slow.pytest/fixtures/autoresearch/train.py
Comment on lines
+81
to
+99
| // Verify metric pattern is valid regex with one capture group | ||
| try { | ||
| const _regex = new RegExp(config.metric.pattern); | ||
| const match = new RegExp(config.metric.pattern + "|").exec(""); | ||
| const groupCount = (match?.length ?? 1) - 1; | ||
| if (groupCount < 1) { | ||
| throw AutoresearchError.create( | ||
| "CONFIG_INVALID", | ||
| `metric.pattern must have at least one capture group: ${config.metric.pattern}`, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.includes("AUTORESEARCH")) { | ||
| throw error; | ||
| } | ||
| throw AutoresearchError.create( | ||
| "CONFIG_INVALID", | ||
| `metric.pattern is not a valid regex: ${config.metric.pattern}`, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the currently recommended Node.js-compatible approach to prevent ReDoS for user-supplied regular expressions (e.g., safe-regex2, re2), and what are the trade-offs?
💡 Result:
The currently recommended Node.js-compatible approach to prevent ReDoS when using user-supplied regular expressions is to use the 're2' npm package, which provides bindings to Google's RE2 regex engine. RE2 guarantees linear-time matching, avoiding catastrophic backtracking entirely. Alternative approaches include: - Static analysis with 'safe-regex2' to detect potentially vulnerable patterns before use. - Input length limits, simplified patterns avoiding nested quantifiers, and trusted validation libraries like Joi or Zod. For user-supplied regex specifically, replace native RegExp with RE2: npm install re2 const RE2 = require('re2'); const userRegex = new RE2(userPattern, 'u'); // 'u' flag for unicode Trade-offs of RE2: Pros: Completely prevents ReDoS; fast linear time; supports most JS regex features. Cons: Lacks backreferences, lookarounds, and some advanced features (RE2 avoids context-free features); C++ native module requiring compilation/prebuilds (may fail on some platforms); always Unicode mode. 'safe-regex2' trade-offs: Pros: Pure JS; lightweight; detects many catastrophic patterns. Cons: Heuristic-based with false positives/negatives; doesn't execute the regex, just checks. Official Node.js docs do not specify a single library but emphasize general security practices like input validation. RE2 is widely recommended in security contexts for ReDoS protection.
Citations:
- 1: https://stackoverflow.com/questions/63127145/safe-regex-patterns-from-redos-attack
- 2: https://github.com/uhop/node-re2
- 3: https://www.npmjs.com/package/re2?activeTab=code
- 4: https://www.aikido.dev/code-quality/rules/guard-against-slow-regular-expressions-preventing-redos-attacks
- 5: https://www.herodevs.com/blog-posts/what-are-redos-attacks-how-regular-expressions-can-take-down-your-application
- 6: https://www.npmjs.com/package/safe-regex2
- 7: https://registry.npmjs.org/safe-regex2
🏁 Script executed:
# First, let's examine the file to understand its structure and the specific lines mentioned
head -120 src/lib/autoresearch/config.ts | tail -50Repository: juspay/neurolink
Length of output: 1509
🏁 Script executed:
# Check the full function containing lines 81-99
sed -n '70,110p' src/lib/autoresearch/config.tsRepository: juspay/neurolink
Length of output: 1206
🏁 Script executed:
# Search for import statements and utility usage in the file to understand available utilities
head -30 src/lib/autoresearch/config.tsRepository: juspay/neurolink
Length of output: 1198
🏁 Script executed:
# Check if withTimeout or logger are used elsewhere in this file
rg -n "withTimeout|logger|shouldLog" src/lib/autoresearch/config.tsRepository: juspay/neurolink
Length of output: 161
🏁 Script executed:
# Search for the metric type definition to understand the contract
rg -n "metric.*pattern" src/lib/autoresearch/config.ts -A 3 -B 3Repository: juspay/neurolink
Length of output: 749
🏁 Script executed:
# Check for any existing ReDoS mitigation or regex validation patterns in the codebase
rg -n "re2|safe-regex|ReDoS" --type tsRepository: juspay/neurolink
Length of output: 1950
🏁 Script executed:
# Find and examine the ResearchConfig type definition
rg -n "type ResearchConfig|interface ResearchConfig" --type ts -A 20Repository: juspay/neurolink
Length of output: 1341
🏁 Script executed:
# Search for metric property definition in the types
rg -n "metric.*:" --type ts -B 2 -A 2 | grep -A 5 -B 5 "pattern"Repository: juspay/neurolink
Length of output: 1650
🏁 Script executed:
# Check the autoresearchTypes file directly
cat -n src/lib/types/autoresearchTypes.ts | grep -A 30 "metric"Repository: juspay/neurolink
Length of output: 3012
Fix capture group contract violation and add ReDoS protection to metric.pattern validation.
Line 18 of autoresearchTypes.ts specifies the type contract as "regex with one capture group" (singular), but the validation at lines 86 only rejects patterns with zero capture groups (if (groupCount < 1)), allowing multiple capture groups. Additionally, lines 83-84 compile user-supplied regex patterns synchronously without timeout protection, creating a ReDoS vulnerability.
The validation must enforce exactly one capture group and use a timeout-safe regex library (e.g., RE2) or wrap compilation with a protective mechanism.
🧰 Tools
🪛 ast-grep (0.42.0)
[warning] 82-82: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(config.metric.pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/autoresearch/config.ts` around lines 81 - 99, The metric.pattern
validation currently allows multiple capture groups and compiles the user regex
unsafely; update the validation around config.metric.pattern to require exactly
one capture group (i.e., enforce groupCount === 1, not < 1) and replace the
unsafe RegExp compilation with a ReDoS-safe engine (for example use the RE2
constructor or a timeout-wrapped compile) when creating and testing the pattern;
keep using AutoresearchError.create for error cases and preserve the existing
error branch that rethrows AUTORESEARCH errors, but throw CONFIG_INVALID with
distinct messages for "must have exactly one capture group" and "pattern is not
a valid/safe regex" when appropriate.
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 4, 2026 07:25
960651b to
15a5ae7
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/lib/autoresearch/summaryParser.ts (1)
94-98:⚠️ Potential issue | 🟡 MinorTimeout inference fallback still present when explicit signal not provided.
Line 95 falls back to
metric === null && !crashedwhenoptions?.timedOutisundefined. This means callers that omit the explicit signal (e.g., direct tool invocations ofresearch_parse_log) will still mislabel regex mismatches or spawn failures as timeouts.Consider removing the inference fallback and requiring the
timedOutparameter, or defaulting tofalseinstead of inferring:- const timedOut = options?.timedOut ?? (metric === null && !crashed); + const timedOut = options?.timedOut ?? false;This way, unknown failures surface as
crashed: truerather thantimedOut: true, which is a safer default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/summaryParser.ts` around lines 94 - 98, The current timeout inference uses options?.timedOut ?? (metric === null && !crashed) which treats missing timedOut as a timeout; change this so absence of options.timedOut does not infer a timeout. Replace the fallback with a strict default (treat undefined as false) or require the caller to supply timedOut: specifically update the timedOut computation in summaryParser.ts (the variable computed from options?.timedOut) to use options?.timedOut === true ? true : false (or throw/validate if undefined), and adjust finalCrashed (the finalCrashed = crashed || (metric === null && !timedOut) logic) accordingly so unknown/missing signals mark failures as crashed rather than timedOut; ensure callers of research_parse_log are updated or validated to pass timedOut when necessary.
🧹 Nitpick comments (3)
src/lib/autoresearch/summaryParser.ts (1)
71-74: MB→GB conversion heuristic is fragile.The conversion triggers if
memoryConfig.name.toLowerCase().includes("mb"), which could false-positive on names like"memory_limb_count"or miss variants like"MegaBytes". Consider using a more explicit configuration flag or pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/summaryParser.ts` around lines 71 - 74, The MB→GB heuristic in summaryParser.ts currently checks memoryConfig.name.toLowerCase().includes("mb"), which can false-positive or miss variants; update the logic in the memory parsing section (referencing memoryConfig and memoryValue) to rely on an explicit unit indicator or stricter pattern match: prefer checking a new memoryConfig.unit or memoryConfig.isInMB flag if available, otherwise match whole-word unit variants with a regex like /\bmb\b/i or full words like "megabytes"/"megabyte" before converting memoryValue from MB to GB; ensure any new config key is read defensively and fall back to no conversion if neither explicit flag nor strict pattern is present.src/lib/autoresearch/stateStore.ts (1)
24-133: Consider whether async signatures on sync operations are intentional.All methods (
load,save,initialize,update) are declaredasyncbut use synchronousreadFileSync/writeFileSync. This works but is misleading—callersawaitoperations that never actually yield. If this is intentional for future migration to async fs, consider adding a comment. Otherwise, removeasyncor switch tofs.promises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/stateStore.ts` around lines 24 - 133, The four methods load, save, initialize, and update are declared async but use synchronous fs APIs (readFileSync/writeFileSync), which is misleading; either remove the async keywords and return synchronously, or convert implementations to use fs.promises (readFile, writeFile, mkdir, rename) so they truly await I/O, or keep async and add a clear comment explaining the intentional synchronous choice for now. Update the function signatures and call sites accordingly (or replace sync calls with promises inside load, save) and ensure errors are still wrapped with AutoresearchError.create as before.src/lib/autoresearch/tools.ts (1)
30-36: Move the exported tool types intosrc/lib/types.
ResearchToolsDepsandResearchToolsare reusable source types, but they live inside the implementation module that already owns the 600-line factory. Pulling them intosrc/lib/types/autoresearchTypes.tswill make this file easier to split by tool/phase and matches the repo's type-placement convention.Based on learnings:
Project standard: Place reusable/shared types under src/lib/types/*.ts; test-only helper types under test/types/*.ts; avoid declaring local types inside source implementation files.Also applies to: 54-58, 655-658
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/tools.ts` around lines 30 - 36, Extract the exported types ResearchToolsDeps and ResearchTools into a new shared types module (e.g., autoresearchTypes) under the project types area, then replace their declarations in src/lib/autoresearch/tools.ts with imports of those types; update all usages (including the other occurrences you noted in the file) to import from the new module and run the type-checker to ensure no broken imports remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/2026-04-03-autoresearch-design.md`:
- Around line 246-253: The documentation for ResearchStateStore lists
initialize(tag) but the implementation in stateStore.ts defines initialize(tag:
string, branch: string); update the docs to match the implementation by changing
the signature to initialize(tag: string, branch: string) and document the
additional branch parameter (its purpose and expected values), keeping the other
method signatures (load, save, update) unchanged and referencing
ResearchStateStore.initialize to ensure parity between doc and code.
- Around line 297-312: Update the docs to reflect the actual
parseExperimentSummary signature and behavior: note that parseExperimentSummary
accepts a fourth optional parameter options?: { timedOut?: boolean; exitCode?:
number } in addition to (logContent, metricConfig, memoryConfig?), and update
the "Key behaviors" list to state that explicit timeout and exitCode signals in
options override/augment regex-based detection (e.g., timedOut true forces
timeout state, exitCode can mark non-zero exits), while preserving existing
behaviors (pattern capture, crash detection via final "FAIL" or missing metric,
rawTail last 50 lines, never throws). Mention the new options parameter name and
its fields (timedOut, exitCode) and how they influence returned
ExperimentSummary.
In `@src/lib/autoresearch/promptCompiler.ts`:
- Around line 108-110: The prompt currently hardcodes "Start by calling
research_get_context" (in promptCompiler.ts where parts.push adds the cycle
instruction), which can point the model to a tool that is disabled for many
phases; change this to be phase-aware by consulting the phasePolicy
(src/lib/autoresearch/phasePolicy.ts) or the runtime's current phase and
building the prompt to instruct the model to begin with an action that is
actually enabled for that phase instead of always research_get_context. Locate
the string added in promptCompiler.ts (the parts.push that references
research_get_context), replace it with logic that looks up enabled tools/actions
for the currentPhase (using the phasePolicy.getEnabledTools or equivalent) and
compose a sentence telling the model to start by calling one of the enabled
actions for that phase (or a generic "use an enabled tool for <currentPhase>"
fallback) so the first suggested action will not be unavailable.
In `@src/lib/autoresearch/repoPolicy.ts`:
- Around line 82-86: The dirname check using path.dirname(this.config.statePath)
can return "." and cause file.startsWith(".") to incorrectly block dotfiles;
update the check in the block that pushes `violations.push(\`State file staged:
${file}\`)` so it resolves both paths (use path.resolve) and compares either
exact filename when dirname is "." or uses a directory-prefix check that appends
path.sep (e.g., resolvedDir + path.sep) before testing startsWith; reference the
symbols `this.config.statePath`, `path.dirname`, `path.resolve`,
`file.startsWith`, and `violations.push` to locate and replace the current
brittle logic with the robust absolute-path or exact-match logic.
In `@src/lib/autoresearch/tools.ts`:
- Around line 196-207: The current per-path try/catch around execFileSync("git",
["diff", "--", mutablePath], ...) swallows all git errors; change it to only
suppress the case where the target path truly doesn't exist and let other git
failures bubble up: inside the catch, check existence of the path (use
fs.existsSync(path.join(config.repoPath, mutablePath)) or equivalent) and if the
file is absent skip that path, otherwise rethrow the caught error so real git
failures surface; keep references to config.mutablePaths, config.repoPath, diffs
and the execFileSync call to locate the change.
- Around line 390-430: The code reparses run.log in research_record() and
research_accept(), which loses exit/timeout context and allows acceptance
without a proven metric; instead persist the structured ExperimentSummary
produced by parseExperimentSummary (or at least the derived status/recorded
metric/memory and timedOut/crashed flags) into the ExperimentRecord when
creating/updating records in research_record() (use the existing
parseExperimentSummary call there to populate a new field like summary or
recordedStatus), and change research_accept() and any other code paths
(including the other occurrence noted) to read that persisted
ExperimentSummary/recordedStatus on the ExperimentRecord rather than reparsing
the log; finally, make research_accept() refuse to promote anything unless the
persisted summary indicates a definitive "keep" with a non-null metric and not
timedOut/crashed.
- Around line 311-315: The execute handler is awaiting runner.run() directly
which can hang; wrap the call using the shared withTimeout utility: replace
await runner.run() with await withTimeout(runner.run(), config.timeoutMs +
30_000) (or similar) inside the execute function so the experiment path uses the
same safety timeout as the worker path; ensure the withTimeout symbol is
imported/available and propagate or log the timeout error consistently with
existing error handling in execute.
In `@src/lib/autoresearch/worker.ts`:
- Around line 215-240: The revert error handling around execFileSync("git",
["reset", "--hard", state.acceptedCommit"]) currently only logs and then
proceeds to update state (this.stateStore.update) and advance currentPhase to
"propose"; change it so that a failed revert prevents any further state
transitions: in the catch block (where logger.warn is used) either rethrow the
error or set a revertFailed flag and return early from the surrounding method so
you do not call this.stateStore.update to clear candidateCommit, increment
runCount, or update currentPhase to "propose"; ensure the persisted state
remains unchanged when execFileSync fails (reference state.acceptedCommit,
execFileSync, logger.warn, and this.stateStore.update/currentPhase).
---
Duplicate comments:
In `@src/lib/autoresearch/summaryParser.ts`:
- Around line 94-98: The current timeout inference uses options?.timedOut ??
(metric === null && !crashed) which treats missing timedOut as a timeout; change
this so absence of options.timedOut does not infer a timeout. Replace the
fallback with a strict default (treat undefined as false) or require the caller
to supply timedOut: specifically update the timedOut computation in
summaryParser.ts (the variable computed from options?.timedOut) to use
options?.timedOut === true ? true : false (or throw/validate if undefined), and
adjust finalCrashed (the finalCrashed = crashed || (metric === null &&
!timedOut) logic) accordingly so unknown/missing signals mark failures as
crashed rather than timedOut; ensure callers of research_parse_log are updated
or validated to pass timedOut when necessary.
---
Nitpick comments:
In `@src/lib/autoresearch/stateStore.ts`:
- Around line 24-133: The four methods load, save, initialize, and update are
declared async but use synchronous fs APIs (readFileSync/writeFileSync), which
is misleading; either remove the async keywords and return synchronously, or
convert implementations to use fs.promises (readFile, writeFile, mkdir, rename)
so they truly await I/O, or keep async and add a clear comment explaining the
intentional synchronous choice for now. Update the function signatures and call
sites accordingly (or replace sync calls with promises inside load, save) and
ensure errors are still wrapped with AutoresearchError.create as before.
In `@src/lib/autoresearch/summaryParser.ts`:
- Around line 71-74: The MB→GB heuristic in summaryParser.ts currently checks
memoryConfig.name.toLowerCase().includes("mb"), which can false-positive or miss
variants; update the logic in the memory parsing section (referencing
memoryConfig and memoryValue) to rely on an explicit unit indicator or stricter
pattern match: prefer checking a new memoryConfig.unit or memoryConfig.isInMB
flag if available, otherwise match whole-word unit variants with a regex like
/\bmb\b/i or full words like "megabytes"/"megabyte" before converting
memoryValue from MB to GB; ensure any new config key is read defensively and
fall back to no conversion if neither explicit flag nor strict pattern is
present.
In `@src/lib/autoresearch/tools.ts`:
- Around line 30-36: Extract the exported types ResearchToolsDeps and
ResearchTools into a new shared types module (e.g., autoresearchTypes) under the
project types area, then replace their declarations in
src/lib/autoresearch/tools.ts with imports of those types; update all usages
(including the other occurrences you noted in the file) to import from the new
module and run the type-checker to ensure no broken imports remain.
🪄 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: 4986bcb0-6a84-47ea-9762-f757feb82200
📒 Files selected for processing (24)
docs/2026-04-03-autoresearch-design.mdexamples/autoresearch/run-demo.tssrc/cli/commands/autoresearch.tssrc/cli/parser.tssrc/cli/utils/envManager.tssrc/lib/autoresearch/config.tssrc/lib/autoresearch/errors.tssrc/lib/autoresearch/index.tssrc/lib/autoresearch/phasePolicy.tssrc/lib/autoresearch/promptCompiler.tssrc/lib/autoresearch/repoPolicy.tssrc/lib/autoresearch/resultRecorder.tssrc/lib/autoresearch/runner.tssrc/lib/autoresearch/stateStore.tssrc/lib/autoresearch/summaryParser.tssrc/lib/autoresearch/tools.tssrc/lib/autoresearch/worker.tssrc/lib/types/autoresearchTypes.tssrc/lib/types/index.tstest/continuous-test-suite-autoresearch.tstest/fixtures/autoresearch/crash.pytest/fixtures/autoresearch/program.mdtest/fixtures/autoresearch/slow.pytest/fixtures/autoresearch/train.py
✅ Files skipped from review due to trivial changes (8)
- test/fixtures/autoresearch/program.md
- src/cli/utils/envManager.ts
- test/fixtures/autoresearch/crash.py
- src/cli/parser.ts
- src/lib/types/index.ts
- src/lib/autoresearch/errors.ts
- test/fixtures/autoresearch/slow.py
- examples/autoresearch/run-demo.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/fixtures/autoresearch/train.py
- src/lib/autoresearch/runner.ts
- test/continuous-test-suite-autoresearch.ts
- src/lib/autoresearch/resultRecorder.ts
murdore
force-pushed
the
feat/auto-research
branch
from
April 4, 2026 08:48
15a5ae7 to
3a1c49e
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 4, 2026 08:56
3a1c49e to
56a077b
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/lib/autoresearch/tools.ts (1)
476-500:⚠️ Potential issue | 🟠 MajorOnly accept a run that was already classified as
keep.This gate only requires a non-null metric. A timed-out or crashed experiment can still emit a metric before failing, and this code will promote it if the number looks better. Require
!summary.timedOut,!summary.crashed, and ideallystate.lastStatus === "keep"before updatingacceptedCommit.💡 Suggested fix
- if (!summary || summary.metric === null) { + if ( + !summary || + summary.metric === null || + summary.timedOut || + summary.crashed || + state.lastStatus !== "keep" + ) { return { success: false, error: - "No valid experiment summary to accept. Run an experiment first.", + "Latest run is not a proven keep. Record the run first and only accept successful improvements.", }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/tools.ts` around lines 476 - 500, Update the acceptance gate to only accept summaries that were explicitly marked to keep and did not fail: check summary.timedOut === false and summary.crashed === false and that state.lastStatus === "keep" before comparing/setting bestMetric and updating acceptedCommit; if any of these checks fail, return a failure with a clear error (similar style to the existing messages) referencing summary.metric and state.bestMetric (and include config.metric.direction where relevant). Ensure you perform these guards before the isImprovement check and assignment to bestMetric/acceptedCommit so timed-out/crashed or non-kept runs are never promoted.src/lib/autoresearch/stateStore.ts (1)
32-60:⚠️ Potential issue | 🟠 MajorValidate required field types before casting to
ResearchState.This only checks presence plus two numeric fields. A parsed object with
currentPhase: {},branch: 123, orstartedAt: nullstill passes here and then fails later in the worker/prompt flow. Reject malformed-but-JSON-valid state before the cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/stateStore.ts` around lines 32 - 60, The current validation only checks presence and two numeric fields on the parsed object before casting to ResearchState; expand this to validate types for each required field (use the existing parsed variable and requiredFields concept) and throw AutoresearchError.create("STATE_CORRUPT", ...) when a field has the wrong type. Specifically enforce branch and tag are strings, currentPhase is a valid phase string/enum (or at least a string), runCount and keepCount are numbers (already checked), and startedAt/updatedAt are non-null string/number timestamps (or parseable dates); use clear error messages naming the offending field (e.g., "State file has invalid type for field: currentPhase") and keep the validation inside the same function before returning parsed as ResearchState so the cast only happens after all type checks pass.src/lib/autoresearch/promptCompiler.ts (1)
66-74:⚠️ Potential issue | 🟡 MinorKeep the system workflow phase-neutral.
buildCyclePrompt()is phase-aware now, but the system prompt still hardcodesresearch_get_contextas step 1 and the full 1→8 sequence on every turn. Inedit,commit,run,evaluate,record, andaccept_or_revert, that conflicts with the enabled-tool policy and can waste a turn on an unavailable action.
🧹 Nitpick comments (6)
src/lib/autoresearch/worker.ts (3)
271-278: Async calls ingetCyclePromptlack timeout protection.The
stateStore.load()andrecorder.readAll()calls could hang on filesystem issues. Consider adding timeout wrappers for robustness, especially since this method may be called in user-facing CLI flows.As per coding guidelines: "Use
withTimeoututility to wrap async calls for error handling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/worker.ts` around lines 271 - 278, getCyclePrompt currently calls this.stateStore.load() and this.recorder.readAll() without timeouts; wrap both calls with the withTimeout utility (e.g., withTimeout(this.stateStore.load(), SOME_TIMEOUT_MS) and withTimeout(this.recorder.readAll(), SOME_TIMEOUT_MS)) to prevent hangs, propagate or convert timeout rejections into an AutoresearchError (e.g., AutoresearchError.create("TIMEOUT", "...")) so callers get a clear error, and then pass the loaded state and results into this.promptCompiler.buildCyclePrompt as before; ensure to pick or wire a sensible timeout constant and handle/annotate errors from withTimeout consistently inside getCyclePrompt.
203-250: ConsolidatecurrentPhaseupdates to reduce redundancy.Line 240 sets
currentPhase: "propose"for the non-keep path, and line 250 sets it again unconditionally. For the "keep" path (lines 203-212),currentPhaseis only set at line 250. Consider moving the phase update inside the "keep" branch or removing the duplicate from line 250.♻️ Cleaner state update pattern
if (status === "keep") { await this.stateStore.update({ acceptedCommit: commit, bestMetric: summary.metric, baselineMetric: state.baselineMetric ?? summary.metric, keepCount: state.keepCount + 1, runCount: state.runCount + 1, lastStatus: status, candidateCommit: null, + currentPhase: "propose", }); } else { // ... revert logic ... await this.stateStore.update({ runCount: state.runCount + 1, lastStatus: status, candidateCommit: null, currentPhase: "propose", }); } logger.info("[Autoresearch] Experiment complete", { status, metric: summary.metric, runCount: state.runCount + 1, }); - await this.stateStore.update({ currentPhase: "propose" }); return record;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/worker.ts` around lines 203 - 250, The code updates currentPhase twice; remove the redundant unconditional update and ensure currentPhase is set in both branches by adding currentPhase: "propose" into the "keep" branch's stateStore.update call (the update inside the if (status === "keep") block) so both the keep and non-keep paths set currentPhase in their respective stateStore.update calls (refer to stateStore.update, currentPhase, and the if (status === "keep") branch); then delete the final await this.stateStore.update({ currentPhase: "propose" }) at the end.
124-127: Consider wrapping async calls withwithTimeout.The
stateStore.initialize()andrecorder.ensureResultsFile()calls are not protected by timeouts. If these operations hang (e.g., due to filesystem issues), the initialization will block indefinitely.♻️ Suggested fix
+ import { withTimeout } from "../utils/errorHandling.js"; + // ... in initialize(): - const state = await this.stateStore.initialize(tag, branch); - await this.recorder.ensureResultsFile(); + const state = await withTimeout( + this.stateStore.initialize(tag, branch), + 30_000, + new Error("State initialization timed out"), + ); + await withTimeout( + this.recorder.ensureResultsFile(), + 10_000, + new Error("Results file creation timed out"), + );As per coding guidelines: "Use
withTimeoututility to wrap async calls for error handling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/worker.ts` around lines 124 - 127, Wrap the potentially hanging async calls in initialize() with the withTimeout utility: replace direct calls to this.stateStore.initialize(tag, branch) and this.recorder.ensureResultsFile() with withTimeout(...) invocations so both calls fail fast on timeout and propagate a clear error; use the appropriate timeout value used elsewhere in the module (e.g., INIT_TIMEOUT / this.config.initTimeout or a shared constant) and ensure errors are caught/propagated the same way as other withTimeout usages in this codebase.src/cli/commands/autoresearch.ts (2)
302-307: Validate config structure before creating worker.The parsed
configRawis spread directly into theResearchWorkerconstructor without validation. If the config file is malformed or missing required fields, the error message may be cryptic.♻️ Add minimal validation
const configRaw = JSON.parse(readFileSync(configPath, "utf-8")); + if (!configRaw.mutablePaths || !configRaw.runCommand || !configRaw.metric) { + spinner.fail(chalk.red("Invalid config. Missing required fields.")); + process.exit(1); + } const { ResearchWorker } = await import("../../lib/autoresearch/worker.js"); const worker = new ResearchWorker({ ...configRaw, repoPath });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/autoresearch.ts` around lines 302 - 307, Parse and validate the structure of configRaw before passing it to ResearchWorker: check that required fields (e.g., the keys your ResearchWorker expects) exist and have correct types, and throw or log a clear error if validation fails; only after successful validation construct new ResearchWorker({ ...configRaw, repoPath }) and proceed to await worker.resume() and worker.runExperimentCycle(argv.description). Locate the parsing and instantiation around configRaw, ResearchWorker, repoPath, worker.resume, and worker.runExperimentCycle to add this minimal validation and user-friendly error handling.
179-179: Consider timeout forworker.initialize()call.The initialization involves git operations and I/O that could hang. Consider adding a timeout to prevent the CLI from blocking indefinitely.
+ const initTimeout = 60_000; // 60 seconds - const state = await worker.initialize(argv.tag); + const state = await Promise.race([ + worker.initialize(argv.tag), + new Promise<never>((_, reject) => + setTimeout(() => reject(new Error("Initialization timed out")), initTimeout) + ), + ]);As per coding guidelines: "Use
withTimeoututility to wrap async calls for error handling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/autoresearch.ts` at line 179, Wrap the call to worker.initialize(argv.tag) with the withTimeout utility to avoid indefinite hangs (e.g., replace direct await worker.initialize(...) with await withTimeout(worker.initialize(argv.tag), TIMEOUT_MS)); choose an appropriate TIMEOUT_MS constant (or configurable flag) and ensure you catch and handle the timeout rejection (the thrown TimeoutError or custom error) to log a clear message and exit/return gracefully; update any surrounding logic that expects the returned state so a timeout path does not continue with undefined state.docs/2026-04-03-autoresearch-design.md (1)
489-505: Minor markdown formatting.The static analysis flags missing blank lines around fenced code blocks (lines 490, 503). These are within a numbered list context where the formatting is acceptable, but adding blank lines would improve some markdown renderers' compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/2026-04-03-autoresearch-design.md` around lines 489 - 505, Add blank lines before and after the fenced TypeScript code block in the numbered list so Markdown renderers and linters accept it; specifically, insert an empty line above the ```typescript that precedes the block beginning with "if (summary.crashed..." and an empty line after the closing ``` so the surrounding items (steps referencing summary.crashed, isBetter, accept(), revert(), and the return of ExperimentRecord) render correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/2026-04-03-autoresearch-design.md`:
- Around line 544-562: The CLI docs currently list both implemented and
unimplemented subcommands without differentiation; update the section to add
explicit phase labels so readers know which are available now: mark the header
(currently "Phase 1+2 combined") to call out Phase 1 and Phase 2 separately, put
the implemented commands (init, status, results, run-once) under "Phase 1" and
move the not-yet-implemented commands (start, pause, resume, stop, reset) under
"Phase 2" (or "Phase 2+"), and add a brief note that Phase 2 commands are
planned/not yet implemented so users aren’t misled.
In `@src/cli/commands/autoresearch.ts`:
- Line 233: Wrap the JSON.parse(readFileSync(statePath, "utf-8")) call in a
try-catch and validate the resulting object before asserting it as
ResearchState: catch JSON parse errors and log/exit or recreate a safe default;
after parsing, check required properties (e.g., expected keys on the
ResearchState interface) and their types, and if validation fails, handle by
logging the corruption and using a fallback defaultState or removing the broken
file. Update the code around statePath/readFileSync and the const state:
ResearchState assignment to use a temporary parsed variable, perform validation,
and only then assign to state or handle the error path.
In `@src/lib/autoresearch/repoPolicy.ts`:
- Around line 38-40: Update the path-checks to treat immutable paths as a hard
deny: before returning whether a candidate path (`resolved`) matches any entry
in `this.resolvedMutablePaths`, first check `this.resolvedImmutablePaths` and
return false if `resolved` is equal to or startsWith any immutable entry +
`path.sep`; only if it passes the immutable check should you evaluate
`this.resolvedMutablePaths`. Apply the same change to the other similar check
(the block that currently uses `this.resolvedMutablePaths.some(...)` around the
later section), using the same `resolved`/`path.sep` equality and startsWith
tests against `this.resolvedImmutablePaths` to ensure immutable paths are never
allowed even under mutable parents.
In `@src/lib/autoresearch/resultRecorder.ts`:
- Around line 30-35: ensureResultsFile() writes the TSV header without ensuring
the parent directory exists, causing ENOENT for custom results paths; before
calling writeFileSync for this.tsvPath (in ensureResultsFile), create the parent
directory using the dirname of this.tsvPath with a recursive mkdir (same
approach used in appendJsonl()), then proceed to write the header so the file
creation won't fail when the directory is missing.
In `@src/lib/autoresearch/runner.ts`:
- Around line 60-67: Update the proc.on("close") handler to accept both (code,
signal) instead of just code, call clearTimeout(timer) and set exitCode = code
?? 0 as before but also pass the signal into parseExperimentSummary so
signal-terminated processes can be marked as crashes; additionally include the
signal field in the logger.debug call (refer to the proc.on("close", ...)
handler and parseExperimentSummary) so downstream logic can treat non-null
signal terminations appropriately.
In `@src/lib/autoresearch/tools.ts`:
- Around line 250-276: Stage the mutable paths before calling
repoPolicy.validateCommit so the policy validates the actual index that will be
committed: move the execFileSync("git", ["add", ...]) loop to run before calling
repoPolicy.validateCommit(state.branch). If validation then fails, unstage the
same paths (e.g., execFileSync("git", ["reset", "--", ...]) or
execFileSync("git", ["restore", "--staged", ...]) for each mutablePath) and
return the error/violations as before; only call execFileSync("git", ["commit",
"-m", message]) after validation succeeds. Use the existing symbols
config.mutablePaths, repoPolicy.validateCommit, state.branch, and the git
execFileSync calls to locate and update the code.
---
Duplicate comments:
In `@src/lib/autoresearch/stateStore.ts`:
- Around line 32-60: The current validation only checks presence and two numeric
fields on the parsed object before casting to ResearchState; expand this to
validate types for each required field (use the existing parsed variable and
requiredFields concept) and throw AutoresearchError.create("STATE_CORRUPT", ...)
when a field has the wrong type. Specifically enforce branch and tag are
strings, currentPhase is a valid phase string/enum (or at least a string),
runCount and keepCount are numbers (already checked), and startedAt/updatedAt
are non-null string/number timestamps (or parseable dates); use clear error
messages naming the offending field (e.g., "State file has invalid type for
field: currentPhase") and keep the validation inside the same function before
returning parsed as ResearchState so the cast only happens after all type checks
pass.
In `@src/lib/autoresearch/tools.ts`:
- Around line 476-500: Update the acceptance gate to only accept summaries that
were explicitly marked to keep and did not fail: check summary.timedOut ===
false and summary.crashed === false and that state.lastStatus === "keep" before
comparing/setting bestMetric and updating acceptedCommit; if any of these checks
fail, return a failure with a clear error (similar style to the existing
messages) referencing summary.metric and state.bestMetric (and include
config.metric.direction where relevant). Ensure you perform these guards before
the isImprovement check and assignment to bestMetric/acceptedCommit so
timed-out/crashed or non-kept runs are never promoted.
---
Nitpick comments:
In `@docs/2026-04-03-autoresearch-design.md`:
- Around line 489-505: Add blank lines before and after the fenced TypeScript
code block in the numbered list so Markdown renderers and linters accept it;
specifically, insert an empty line above the ```typescript that precedes the
block beginning with "if (summary.crashed..." and an empty line after the
closing ``` so the surrounding items (steps referencing summary.crashed,
isBetter, accept(), revert(), and the return of ExperimentRecord) render
correctly.
In `@src/cli/commands/autoresearch.ts`:
- Around line 302-307: Parse and validate the structure of configRaw before
passing it to ResearchWorker: check that required fields (e.g., the keys your
ResearchWorker expects) exist and have correct types, and throw or log a clear
error if validation fails; only after successful validation construct new
ResearchWorker({ ...configRaw, repoPath }) and proceed to await worker.resume()
and worker.runExperimentCycle(argv.description). Locate the parsing and
instantiation around configRaw, ResearchWorker, repoPath, worker.resume, and
worker.runExperimentCycle to add this minimal validation and user-friendly error
handling.
- Line 179: Wrap the call to worker.initialize(argv.tag) with the withTimeout
utility to avoid indefinite hangs (e.g., replace direct await
worker.initialize(...) with await withTimeout(worker.initialize(argv.tag),
TIMEOUT_MS)); choose an appropriate TIMEOUT_MS constant (or configurable flag)
and ensure you catch and handle the timeout rejection (the thrown TimeoutError
or custom error) to log a clear message and exit/return gracefully; update any
surrounding logic that expects the returned state so a timeout path does not
continue with undefined state.
In `@src/lib/autoresearch/worker.ts`:
- Around line 271-278: getCyclePrompt currently calls this.stateStore.load() and
this.recorder.readAll() without timeouts; wrap both calls with the withTimeout
utility (e.g., withTimeout(this.stateStore.load(), SOME_TIMEOUT_MS) and
withTimeout(this.recorder.readAll(), SOME_TIMEOUT_MS)) to prevent hangs,
propagate or convert timeout rejections into an AutoresearchError (e.g.,
AutoresearchError.create("TIMEOUT", "...")) so callers get a clear error, and
then pass the loaded state and results into this.promptCompiler.buildCyclePrompt
as before; ensure to pick or wire a sensible timeout constant and
handle/annotate errors from withTimeout consistently inside getCyclePrompt.
- Around line 203-250: The code updates currentPhase twice; remove the redundant
unconditional update and ensure currentPhase is set in both branches by adding
currentPhase: "propose" into the "keep" branch's stateStore.update call (the
update inside the if (status === "keep") block) so both the keep and non-keep
paths set currentPhase in their respective stateStore.update calls (refer to
stateStore.update, currentPhase, and the if (status === "keep") branch); then
delete the final await this.stateStore.update({ currentPhase: "propose" }) at
the end.
- Around line 124-127: Wrap the potentially hanging async calls in initialize()
with the withTimeout utility: replace direct calls to
this.stateStore.initialize(tag, branch) and this.recorder.ensureResultsFile()
with withTimeout(...) invocations so both calls fail fast on timeout and
propagate a clear error; use the appropriate timeout value used elsewhere in the
module (e.g., INIT_TIMEOUT / this.config.initTimeout or a shared constant) and
ensure errors are caught/propagated the same way as other withTimeout usages in
this codebase.
🪄 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: 267069f7-34f6-4cfd-9de4-6cf232718fc3
📒 Files selected for processing (23)
docs/2026-04-03-autoresearch-design.mdexamples/autoresearch/run-demo.tssrc/cli/commands/autoresearch.tssrc/cli/parser.tssrc/lib/autoresearch/config.tssrc/lib/autoresearch/errors.tssrc/lib/autoresearch/index.tssrc/lib/autoresearch/phasePolicy.tssrc/lib/autoresearch/promptCompiler.tssrc/lib/autoresearch/repoPolicy.tssrc/lib/autoresearch/resultRecorder.tssrc/lib/autoresearch/runner.tssrc/lib/autoresearch/stateStore.tssrc/lib/autoresearch/summaryParser.tssrc/lib/autoresearch/tools.tssrc/lib/autoresearch/worker.tssrc/lib/types/autoresearchTypes.tssrc/lib/types/index.tstest/continuous-test-suite-autoresearch.tstest/fixtures/autoresearch/crash.pytest/fixtures/autoresearch/program.mdtest/fixtures/autoresearch/slow.pytest/fixtures/autoresearch/train.py
✅ Files skipped from review due to trivial changes (11)
- test/fixtures/autoresearch/crash.py
- test/fixtures/autoresearch/train.py
- test/fixtures/autoresearch/program.md
- test/fixtures/autoresearch/slow.py
- src/lib/autoresearch/errors.ts
- src/cli/parser.ts
- src/lib/types/autoresearchTypes.ts
- src/lib/autoresearch/index.ts
- src/lib/types/index.ts
- test/continuous-test-suite-autoresearch.ts
- examples/autoresearch/run-demo.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/autoresearch/phasePolicy.ts
murdore
force-pushed
the
feat/auto-research
branch
from
April 4, 2026 20:59
56a077b to
6f6e100
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 11, 2026 18:18
7fdb81f to
38f3a49
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 11, 2026 18:28
38f3a49 to
f548bc7
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 12, 2026 02:41
f548bc7 to
4c11e8c
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 12, 2026 02:50
4c11e8c to
907ff15
Compare
Contributor
Author
Review Response — Round 12Fixed (907ff15):Inline:
Won't fix (with reasons):
@coderabbitai review |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Acknowledged fixes (907ff15):
Won't-fix decisions accepted:
Triggering a fresh incremental review now. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
test/continuous-test-suite-autoresearch-redis.ts (1)
151-163:⚠️ Potential issue | 🟠 MajorBound the Redis calls with
withTimeout.The probe and the actual Redis integration steps still await network operations directly. If Redis is slow or half-open, this continuous suite can hang indefinitely instead of skipping/failing fast. Please route these awaits through the shared timeout helper, ideally via a small wrapper so the probe and all RedisTaskStore operations use the same bound.
As per coding guidelines,
**/*.ts: UsewithTimeoututility to wrap async calls for error handling.Also applies to: 830-902, 905-1047
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-autoresearch-redis.ts` around lines 151 - 163, The probe function is awaiting Redis network calls directly—wrap those awaits with the shared withTimeout helper by creating a small wrapper (e.g., redisWithTimeout or timedRedisCall) and use it inside isRedisAvailable to bound client.connect(), client.ping(), and client.quit(); then replace direct awaits in the RedisTaskStore methods (the store's connect/ping/get/set/delete operations) to call the same wrapper so all Redis interactions share the same timeout behavior and fail fast.docs/2026-04-03-autoresearch-design.md (1)
546-564:⚠️ Potential issue | 🟡 MinorCLI command signatures are still stale here.
This section still uses
[configPath],--max-experiments, and omits the required<taskId>forpause,resume, andstop. Please update it to match the implementedsrc/cli/commands/autoresearch.tssurface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/2026-04-03-autoresearch-design.md` around lines 546 - 564, Update the CLI command signatures in this docs block to exactly match the implemented surface in src/cli/commands/autoresearch.ts: replace optional bracketed [configPath] with the actual parameter syntax used in the implementation (e.g., <configPath>), rename the stale flag --max-experiments to the exact flag name used in the code, ensure the pause, resume, and stop commands include the required <taskId> parameter, and verify each command’s --format/default options (status, results) and other flags match the implemented signatures for start, status, results, run-once, pause, resume, stop, and reset.src/lib/autoresearch/stateStore.ts (1)
35-91:⚠️ Potential issue | 🟠 MajorValidate the full persisted state before casting or saving.
load()still accepts any shape that has the required fields, andupdate()only validates a few patch keys before merging. That means corrupted values in persisted fields likeacceptedCommit,candidateCommit,lastStatus,baselineMetric, orlastSummarycan survive and break revert/status handling later. Please centralize fullResearchStatevalidation and run it both immediately afterJSON.parse()and again onupdatedbeforesave().Also applies to: 170-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/autoresearch/stateStore.ts` around lines 35 - 91, Centralize full ResearchState validation into a new validator (e.g., validateResearchState(parsed: unknown)) and invoke it in load() immediately after JSON.parse() and again in update() after merging the patch and before calling save(); the validator should check all required primitive/enum fields already validated (branch, currentPhase, runCount, keepCount, tag, startedAt, updatedAt) plus optional/persisted fields that can break logic (acceptedCommit, candidateCommit, lastStatus, baselineMetric, lastSummary, etc.) for correct types/shape and valid values, and throw AutoresearchError.create("STATE_CORRUPT", ...) on failure so no corrupted state is cast or persisted.test/demo-autoresearch/run-demo.ts (1)
301-320:⚠️ Potential issue | 🟠 MajorReplace the manual
Promise.racetimeout with the shared timeout helper.This still fails the outer await without guaranteeing the in-flight generation/tool loop stops, so the demo can keep mutating the repo after the cycle is marked failed. Please switch this call site to the repo’s
withTimeoutpattern instead of the ad-hoc race.#!/bin/bash rg -n "Promise\\.race|withTimeout|generateText\\(" test/demo-autoresearch/run-demo.ts src/lib/utils/errorHandling.tsAs per coding guidelines, "Use
withTimeoututility to wrap async calls for error handling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/demo-autoresearch/run-demo.ts` around lines 301 - 320, Replace the ad-hoc Promise.race timeout with the shared withTimeout helper: call withTimeout(generateText({...}) , GENERATE_TIMEOUT_MS, `generateText exceeded ${GENERATE_TIMEOUT_MS}ms`) instead of the manual new Promise reject branch so the in-flight generateText/tools loop is cancelled correctly; ensure you import withTimeout from src/lib/utils/errorHandling and keep the same args (model, system: systemPrompt, prompt: fullPrompt, tools, stopWhen: stepCountIs(20), maxRetries: 1) and existing GENERATE_TIMEOUT_MS constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/2026-04-03-autoresearch-design.md`:
- Around line 266-284: The documentation for RepoPolicy is out of sync with the
implementation; update the API block to reflect the actual public surface:
change validateCommit() to validateCommit(expectedBranch: string) (document that
it validates staged files and branch), remove the now-nonexistent
validateBranch() entry, and add entries for getCurrentBranch(): Promise<string>
and getHeadCommit(): Promise<string> (with brief descriptions matching their
behavior). Ensure the method signatures and short descriptions exactly match the
implemented symbols: RepoPolicy.validateCommit, RepoPolicy.getCurrentBranch, and
RepoPolicy.getHeadCommit.
- Around line 395-415: Update the docs to match the actual exports and tool
lists in phasePolicy.ts: replace the incorrect export names
(`createPrepareStepCallback`) with the real exported functions
`getPhaseToolPolicy` and `getAllResearchToolNames`, and sync the Phase → Tool
mapping so it reflects the runtime policy (e.g., add `research_checkpoint` to
the `baseline` and `accept_or_revert` rows and verify every phase’s Active Tools
and Forced First Tool match what getPhaseToolPolicy returns). Ensure the table
header/names and any textual references mention the correct symbols
`getPhaseToolPolicy` and `getAllResearchToolNames` so the documented contract
aligns with the module implementation.
In `@src/lib/autoresearch/config.ts`:
- Around line 42-60: validateConfig currently checks repoPath and .git but
doesn't reject programPath, resultsPath, statePath, or logPath that escape the
repository; update validateConfig to ensure each of these fields is a
repo-relative path (reject absolute paths and any path that resolves outside
repoPath). For each field (programPath, resultsPath, statePath, logPath) resolve
the candidate against config.repoPath (use path.resolve or equivalent), compute
a safe check (e.g., path.relative(repoPath, resolved).startsWith('..') or verify
resolved startsWith repoPath + path.sep) and throw
AutoresearchError.create("CONFIG_INVALID", ...) if the path is absolute or
resolves outside repoPath; keep the existing repoPath/.git checks intact. Ensure
error messages mention the offending field name and value so the caller can fix
configuration.
In `@src/lib/autoresearch/tools.ts`:
- Around line 567-573: When updating state after accepting or reverting a
candidate (the stateStore.update call that sets candidateCommit: null and
updates acceptedCommit/baselineMetric/keepCount), also clear any run-derived
fields lastSummary and lastStatus to prevent phantom results; modify the
stateStore.update payload(s) (the one shown and the similar call around line
619) to include lastSummary: null and lastStatus: null alongside
candidateCommit: null.
In `@src/lib/tasks/autoresearchTaskExecutor.ts`:
- Around line 316-327: calledTools currently includes every toolExecution
regardless of success, causing inferNextPhase to advance state on failed runs;
update the mapping that builds calledTools (from toolExecutions / the generate()
normalization) to filter only executions with a successful output (e.g.,
non-null/defined and/or a success marker) before extracting the name, so
inferNextPhase receives only successful tool names; keep references to
calledTools, inferNextPhase, worker.getState, and worker.advancePhase when
locating and updating the logic.
---
Duplicate comments:
In `@docs/2026-04-03-autoresearch-design.md`:
- Around line 546-564: Update the CLI command signatures in this docs block to
exactly match the implemented surface in src/cli/commands/autoresearch.ts:
replace optional bracketed [configPath] with the actual parameter syntax used in
the implementation (e.g., <configPath>), rename the stale flag --max-experiments
to the exact flag name used in the code, ensure the pause, resume, and stop
commands include the required <taskId> parameter, and verify each command’s
--format/default options (status, results) and other flags match the implemented
signatures for start, status, results, run-once, pause, resume, stop, and reset.
In `@src/lib/autoresearch/stateStore.ts`:
- Around line 35-91: Centralize full ResearchState validation into a new
validator (e.g., validateResearchState(parsed: unknown)) and invoke it in load()
immediately after JSON.parse() and again in update() after merging the patch and
before calling save(); the validator should check all required primitive/enum
fields already validated (branch, currentPhase, runCount, keepCount, tag,
startedAt, updatedAt) plus optional/persisted fields that can break logic
(acceptedCommit, candidateCommit, lastStatus, baselineMetric, lastSummary, etc.)
for correct types/shape and valid values, and throw
AutoresearchError.create("STATE_CORRUPT", ...) on failure so no corrupted state
is cast or persisted.
In `@test/continuous-test-suite-autoresearch-redis.ts`:
- Around line 151-163: The probe function is awaiting Redis network calls
directly—wrap those awaits with the shared withTimeout helper by creating a
small wrapper (e.g., redisWithTimeout or timedRedisCall) and use it inside
isRedisAvailable to bound client.connect(), client.ping(), and client.quit();
then replace direct awaits in the RedisTaskStore methods (the store's
connect/ping/get/set/delete operations) to call the same wrapper so all Redis
interactions share the same timeout behavior and fail fast.
In `@test/demo-autoresearch/run-demo.ts`:
- Around line 301-320: Replace the ad-hoc Promise.race timeout with the shared
withTimeout helper: call withTimeout(generateText({...}) , GENERATE_TIMEOUT_MS,
`generateText exceeded ${GENERATE_TIMEOUT_MS}ms`) instead of the manual new
Promise reject branch so the in-flight generateText/tools loop is cancelled
correctly; ensure you import withTimeout from src/lib/utils/errorHandling and
keep the same args (model, system: systemPrompt, prompt: fullPrompt, tools,
stopWhen: stepCountIs(20), maxRetries: 1) and existing GENERATE_TIMEOUT_MS
constant.
🪄 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: 4bd0db80-7d64-4004-b822-9bb5ee827367
📒 Files selected for processing (60)
README.mddocs-site/scripts/sync-docs.tsdocs-site/sidebars.tsdocs/2026-04-03-autoresearch-design.mddocs/cookbook/autoresearch-quickstart.mddocs/cookbook/index.mddocs/features/autoresearch.mddocs/features/index.mddocs/plans/autoresearch-gap-analysis-and-completion-prompt.mdexamples/autoresearch/README.mdexamples/autoresearch/run-demo.tsexamples/autoresearch/sample-config.jsonexamples/autoresearch/sample-program.mdscripts/build-browser.mjssrc/cli/commands/autoresearch.tssrc/cli/commands/task.tssrc/cli/parser.tssrc/lib/autoresearch/config.tssrc/lib/autoresearch/errors.tssrc/lib/autoresearch/index.tssrc/lib/autoresearch/phasePolicy.tssrc/lib/autoresearch/promptCompiler.tssrc/lib/autoresearch/repoPolicy.tssrc/lib/autoresearch/resultRecorder.tssrc/lib/autoresearch/runner.tssrc/lib/autoresearch/stateStore.tssrc/lib/autoresearch/summaryParser.tssrc/lib/autoresearch/tools.tssrc/lib/autoresearch/worker.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/litellm.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/tasks/autoresearchTaskExecutor.tssrc/lib/tasks/errors.tssrc/lib/tasks/taskExecutor.tssrc/lib/tasks/taskManager.tssrc/lib/telemetry/attributes.tssrc/lib/telemetry/tracers.tssrc/lib/types/autoresearchTypes.tssrc/lib/types/common.tssrc/lib/types/index.tssrc/lib/types/taskTypes.tstest/continuous-test-suite-autoresearch-e2e.tstest/continuous-test-suite-autoresearch-live.tstest/continuous-test-suite-autoresearch-redis.tstest/demo-autoresearch-real/experiment.shtest/demo-autoresearch-real/program.mdtest/demo-autoresearch-real/run-demo.tstest/demo-autoresearch-real/tsconfig.providers-check.jsontest/demo-autoresearch/.demo-repotest/demo-autoresearch/experiment.pytest/demo-autoresearch/program.mdtest/demo-autoresearch/run-demo.tstest/fixtures/autoresearch/crash.pytest/fixtures/autoresearch/program-live.mdtest/fixtures/autoresearch/program.mdtest/fixtures/autoresearch/slow.pytest/fixtures/autoresearch/train.py
✅ Files skipped from review due to trivial changes (28)
- test/fixtures/autoresearch/program.md
- docs/cookbook/index.md
- test/demo-autoresearch/.demo-repo
- docs-site/sidebars.ts
- src/lib/telemetry/tracers.ts
- test/fixtures/autoresearch/slow.py
- docs-site/scripts/sync-docs.ts
- examples/autoresearch/sample-config.json
- docs/features/index.md
- test/fixtures/autoresearch/program-live.md
- examples/autoresearch/sample-program.md
- docs/plans/autoresearch-gap-analysis-and-completion-prompt.md
- test/demo-autoresearch-real/tsconfig.providers-check.json
- README.md
- examples/autoresearch/README.md
- test/demo-autoresearch/program.md
- test/demo-autoresearch-real/experiment.sh
- docs/cookbook/autoresearch-quickstart.md
- src/lib/telemetry/attributes.ts
- src/lib/autoresearch/errors.ts
- examples/autoresearch/run-demo.ts
- docs/features/autoresearch.md
- src/lib/autoresearch/index.ts
- src/lib/autoresearch/promptCompiler.ts
- test/demo-autoresearch-real/run-demo.ts
- src/lib/types/taskTypes.ts
- src/lib/autoresearch/resultRecorder.ts
- src/lib/types/autoresearchTypes.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/cli/parser.ts
- src/lib/providers/openRouter.ts
- src/lib/tasks/errors.ts
- test/fixtures/autoresearch/crash.py
- test/fixtures/autoresearch/train.py
- src/lib/tasks/taskManager.ts
- src/lib/tasks/taskExecutor.ts
- scripts/build-browser.mjs
- src/lib/providers/litellm.ts
- src/cli/commands/task.ts
- src/lib/core/baseProvider.ts
- src/lib/providers/openaiCompatible.ts
- src/lib/autoresearch/runner.ts
- src/lib/neurolink.ts
- test/continuous-test-suite-autoresearch-e2e.ts
Comment on lines
+266
to
+284
| ```typescript | ||
| export class RepoPolicy { | ||
| constructor(private config: ResearchConfig) | ||
|
|
||
| /** Returns true if path is within mutablePaths */ | ||
| isWriteAllowed(filePath: string): boolean | ||
|
|
||
| /** Returns true if path is in immutablePaths */ | ||
| isProtected(filePath: string): boolean | ||
|
|
||
| /** Validates staged files are all in mutablePaths and on the right branch */ | ||
| async validateCommit(): Promise<{ valid: boolean; violations: string[] }> | ||
|
|
||
| /** Validates the repo is on the research branch */ | ||
| async validateBranch(expectedBranch: string): Promise<boolean> | ||
|
|
||
| /** Returns list of staged file paths */ | ||
| async getStagedFiles(): Promise<string[]> | ||
| ``` |
There was a problem hiding this comment.
RepoPolicy API section is out of sync with the implementation.
This block documents validateCommit() without an expectedBranch argument and a validateBranch() method that the current class does not expose. Please update the section to match the implemented surface (validateCommit(expectedBranch: string), getCurrentBranch(), getHeadCommit()), otherwise readers will be pointed at a nonexistent API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/2026-04-03-autoresearch-design.md` around lines 266 - 284, The
documentation for RepoPolicy is out of sync with the implementation; update the
API block to reflect the actual public surface: change validateCommit() to
validateCommit(expectedBranch: string) (document that it validates staged files
and branch), remove the now-nonexistent validateBranch() entry, and add entries
for getCurrentBranch(): Promise<string> and getHeadCommit(): Promise<string>
(with brief descriptions matching their behavior). Ensure the method signatures
and short descriptions exactly match the implemented symbols:
RepoPolicy.validateCommit, RepoPolicy.getCurrentBranch, and
RepoPolicy.getHeadCommit.
Comment on lines
+395
to
+415
| ```typescript | ||
| export function getPhaseToolPolicy(phase: ExperimentPhase): PhaseToolPolicy; | ||
|
|
||
| export function createPrepareStepCallback( | ||
| stateStore: ResearchStateStore, | ||
| ): PrepareStepFunction; | ||
| ``` | ||
|
|
||
| Phase → Tool mapping: | ||
|
|
||
| | Phase | Active Tools | Forced First Tool | | ||
| | ------------------ | ------------------------------------------------------------------------------------- | --------------------------- | | ||
| | `bootstrap` | `research_get_context`, `research_read_file`, `research_checkpoint` | `research_get_context` | | ||
| | `baseline` | `research_run_experiment`, `research_parse_log`, `research_record`, `research_accept` | `research_run_experiment` | | ||
| | `propose` | `research_get_context`, `research_read_file` | `research_get_context` | | ||
| | `edit` | `research_read_file`, `research_write_candidate`, `research_diff` | — | | ||
| | `commit` | `research_commit_candidate` | `research_commit_candidate` | | ||
| | `run` | `research_run_experiment` | `research_run_experiment` | | ||
| | `evaluate` | `research_parse_log`, `research_inspect_failure` | `research_parse_log` | | ||
| | `record` | `research_record`, `research_checkpoint` | `research_record` | | ||
| | `accept_or_revert` | `research_accept`, `research_revert` | — | |
There was a problem hiding this comment.
PhasePolicy docs mention the wrong export and stale tool lists.
phasePolicy.ts in this PR exposes getPhaseToolPolicy() / getAllResearchToolNames(), not createPrepareStepCallback(), and the table is missing research_checkpoint for at least baseline and accept_or_revert. Please sync this section with the actual module so the documented phase contract matches runtime behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/2026-04-03-autoresearch-design.md` around lines 395 - 415, Update the
docs to match the actual exports and tool lists in phasePolicy.ts: replace the
incorrect export names (`createPrepareStepCallback`) with the real exported
functions `getPhaseToolPolicy` and `getAllResearchToolNames`, and sync the Phase
→ Tool mapping so it reflects the runtime policy (e.g., add
`research_checkpoint` to the `baseline` and `accept_or_revert` rows and verify
every phase’s Active Tools and Forced First Tool match what getPhaseToolPolicy
returns). Ensure the table header/names and any textual references mention the
correct symbols `getPhaseToolPolicy` and `getAllResearchToolNames` so the
documented contract aligns with the module implementation.
Comment on lines
+42
to
+60
| export function validateConfig(config: ResearchConfig): void { | ||
| // Verify repoPath exists and is a directory | ||
| if ( | ||
| !existsSync(config.repoPath) || | ||
| !statSync(config.repoPath).isDirectory() | ||
| ) { | ||
| throw AutoresearchError.create( | ||
| "CONFIG_INVALID", | ||
| `repoPath does not exist or is not a directory: ${config.repoPath}`, | ||
| ); | ||
| } | ||
|
|
||
| // Verify it's a git repo | ||
| if (!existsSync(path.join(config.repoPath, ".git"))) { | ||
| throw AutoresearchError.create( | ||
| "REPO_NOT_FOUND", | ||
| `repoPath is not a git repository: ${config.repoPath}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Reject internal artifact paths that escape repoPath.
programPath, resultsPath, statePath, and logPath are treated as repo-relative everywhere else, but validateConfig() never enforces that. A config like statePath: "../../tmp/state.json" or an absolute logPath will make the worker read/write outside the target repo, which breaks the safety boundary this feature is trying to provide.
Suggested fix
export function validateConfig(config: ResearchConfig): void {
+ const assertInsideRepo = (field: string, targetPath: string): void => {
+ const resolved = path.resolve(config.repoPath, targetPath);
+ const rel = path.relative(config.repoPath, resolved);
+ if (rel.startsWith("..") || path.isAbsolute(rel)) {
+ throw AutoresearchError.create(
+ "CONFIG_INVALID",
+ `${field} must stay inside repoPath: ${targetPath}`,
+ );
+ }
+ };
+
// Verify repoPath exists and is a directory
if (
!existsSync(config.repoPath) ||
!statSync(config.repoPath).isDirectory()
) {
@@
if (!existsSync(path.join(config.repoPath, ".git"))) {
throw AutoresearchError.create(
"REPO_NOT_FOUND",
`repoPath is not a git repository: ${config.repoPath}`,
);
}
+
+ assertInsideRepo("programPath", config.programPath);
+ assertInsideRepo("resultsPath", config.resultsPath);
+ assertInsideRepo("statePath", config.statePath);
+ assertInsideRepo("logPath", config.logPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/autoresearch/config.ts` around lines 42 - 60, validateConfig
currently checks repoPath and .git but doesn't reject programPath, resultsPath,
statePath, or logPath that escape the repository; update validateConfig to
ensure each of these fields is a repo-relative path (reject absolute paths and
any path that resolves outside repoPath). For each field (programPath,
resultsPath, statePath, logPath) resolve the candidate against config.repoPath
(use path.resolve or equivalent), compute a safe check (e.g.,
path.relative(repoPath, resolved).startsWith('..') or verify resolved startsWith
repoPath + path.sep) and throw AutoresearchError.create("CONFIG_INVALID", ...)
if the path is absolute or resolves outside repoPath; keep the existing
repoPath/.git checks intact. Ensure error messages mention the offending field
name and value so the caller can fix configuration.
Comment on lines
+567
to
+573
| await stateStore.update({ | ||
| acceptedCommit: state.candidateCommit, | ||
| bestMetric, | ||
| baselineMetric: state.baselineMetric ?? bestMetric, | ||
| keepCount: state.keepCount + 1, | ||
| candidateCommit: null, | ||
| }); |
There was a problem hiding this comment.
Clear run-derived state when a candidate is accepted or reverted.
Both terminal paths clear candidateCommit, but they leave lastSummary and lastStatus behind. A later research_record() can then append a phantom result for the accepted/reverted commit without a fresh experiment. Please clear those fields alongside candidateCommit in both paths.
Also applies to: 619-619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/autoresearch/tools.ts` around lines 567 - 573, When updating state
after accepting or reverting a candidate (the stateStore.update call that sets
candidateCommit: null and updates acceptedCommit/baselineMetric/keepCount), also
clear any run-derived fields lastSummary and lastStatus to prevent phantom
results; modify the stateStore.update payload(s) (the one shown and the similar
call around line 619) to include lastSummary: null and lastStatus: null
alongside candidateCommit: null.
Comment on lines
+316
to
+327
| const calledTools: string[] = (result.toolExecutions ?? []) | ||
| .map((te) => { | ||
| const t = te as Record<string, unknown>; | ||
| return typeof t.name === "string" ? t.name : ""; | ||
| }) | ||
| .filter(Boolean); | ||
|
|
||
| const currentState = await worker.getState(); | ||
| const currentPhase = currentState?.currentPhase ?? "bootstrap"; | ||
| const nextPhase = inferNextPhase(currentPhase, calledTools); | ||
| if (nextPhase) { | ||
| await worker.advancePhase(nextPhase); |
There was a problem hiding this comment.
Advance the phase only for successful tool executions.
calledTools is derived from every toolExecution, so a failed research_commit_candidate, research_run_experiment, or research_record still pushes the state machine forward. That can leave the scheduled worker stuck in a later phase with no candidate commit or recorded result. Please inspect each execution’s output and only feed successful tool runs into inferNextPhase.
Based on learnings, the normal generate() flow returns toolExecutions normalized as { name: string, input: StandardRecord, output: unknown, duration: number }.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tasks/autoresearchTaskExecutor.ts` around lines 316 - 327,
calledTools currently includes every toolExecution regardless of success,
causing inferNextPhase to advance state on failed runs; update the mapping that
builds calledTools (from toolExecutions / the generate() normalization) to
filter only executions with a successful output (e.g., non-null/defined and/or a
success marker) before extracting the name, so inferNextPhase receives only
successful tool names; keep references to calledTools, inferNextPhase,
worker.getState, and worker.advancePhase when locating and updating the logic.
murdore
force-pushed
the
feat/auto-research
branch
from
April 12, 2026 03:32
907ff15 to
86c0edf
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
feat/auto-research
branch
from
April 12, 2026 03:59
86c0edf to
32ab464
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
…and docs Autonomous research loop that proposes code changes via AI, executes experiments, evaluates results against a deterministic metric, and keeps or discards each change — running unattended for hours. Phase 1 — Core engine: - 12 research tools (Vercel AI SDK tool() + Zod) - 9-phase state machine with tool filtering per phase - Deterministic keep/discard policy (no AI judgment on outcomes) - Shell-safe git ops via execFileSync with argument arrays - path.relative() boundary checks - File-backed JSON state with atomic writes and field validation - Explicit timedOut/exitCode signal from runner to parser - withTimeout defense-in-depth on runner.run() - TSV results compatible with upstream analysis.ipynb Phase 2 — Scheduling, docs, operations: - CLI: neurolink autoresearch init/start/pause/resume/stop/reset/status/results/run-once - TaskManager integration via autoresearchTaskExecutor - OpenTelemetry spans on all worker operations - 10 lifecycle events (initialized, experiment-completed, metric-improved, etc.) - Feature guide: docs/features/autoresearch.md - Cookbook quickstart: docs/cookbook/autoresearch-quickstart.md - Example README, sample-config.json, sample-program.md - Package export: @juspay/neurolink/autoresearch - Browser build: add missing fs sync stubs (appendFileSync, cpSync) - E2E, live provider, and Redis/BullMQ test suites
murdore
force-pushed
the
feat/auto-research
branch
from
April 12, 2026 04:07
32ab464 to
a7bcb26
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
|
🎉 This PR is included in version 9.53.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This branch was successfully deployed
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
Adds an autonomous research experiment engine to NeuroLink, inspired by Karpathy's autoresearch. An AI agent proposes code changes, runs experiments, evaluates results against a deterministic metric, and keeps or discards each change — running unattended for hours.
tool()+ Zod — read/write code, run experiments, record results, manage git lifecycleexecFileSyncwith argument arrays, no string interpolationanalysis.ipynbneurolink autoresearch init/status/results/run-onceFiles (23)
src/lib/types/autoresearchTypes.tssrc/lib/autoresearch/— config, errors, stateStore, repoPolicy, summaryParser, resultRecorder, runner, tools, phasePolicy, promptCompiler, worker, indexsrc/cli/commands/autoresearch.tstest/continuous-test-suite-autoresearch.ts+ 4 fixturesexamples/autoresearch/run-demo.tsdocs/2026-04-03-autoresearch-design.mdsrc/lib/types/index.ts(barrel exports),src/cli/parser.ts(command registration)Test plan
npx tsx test/continuous-test-suite-autoresearch.ts— 24/24 passingpnpm run check— 0 type errors from autoresearch filesneurolink autoresearch init, verify state + config writtenneurolink autoresearch run-once, verify results.tsv + git commitnpx tsx examples/autoresearch/run-demo.tswith responsive training scriptSummary by CodeRabbit
Release Notes
New Features
autoresearch init,run-once,start) or SDK with deterministic metric extraction, git-backed revert safety, and TaskManager integration.Documentation