Ivan: Implement Self-Feeding Learnings Capture in Expert Mode#45
Ivan: Implement Self-Feeding Learnings Capture in Expert Mode#45erkangz wants to merge 2 commits into
Conversation
Introduces a second execution mode (--mode expert) where Ivan acts as a principal engineer rather than a one-shot dispatcher. A separate Opus architect session critiques an implementer session across a design dialogue, implementation, and code-review dialogue, all seeded with institutional learnings retrieved from the .ivan store. Defaults to simple (one-shot) mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: ivan-agent <ivan-agent@users.noreply.github.com>
|
@codex Review the new collaborative execution mode implementation, particularly the updates in |
WalkthroughThis PR introduces expert-mode execution for the Ivan agent, enabling collaborative orchestration with architect design reviews and code reviews running in parallel with implementation. The implementation standardizes turn execution across executors through shared TurnResult/TurnOptions types, adds CLI and config support for selecting between simple and expert modes, introduces a new CollaborativeExecutor that manages multi-round design and review cycles with verdict parsing, and implements a learning capture pipeline that distills architect critiques into structured knowledge records for institutional memory. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/claude-cli-executor.ts (1)
154-156:⚠️ Potential issue | 🟠 MajorFix session continuity in
claude-cli-executor(missing session_id capture)The CLI executor doesn’t parse any session identifier from Claude CLI output. It sets
currentSessionIdfrom the incomingsessionId(sessionId || '') and then returns that value without updating it from stdout (unlikeClaudeExecutor, which readssession_idfrom SDK messages). Sinceclaude --helpshows no session-related options/help, there’s no indication of a supported CLI flag to retrieve a session id, so expert-mode continuity with the CLI backend will effectively start fresh each turn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/claude-cli-executor.ts` around lines 154 - 156, The CLI executor never updates currentSessionId (it uses sessionId || '') so session continuity is lost; modify the claude-cli-executor code that reads CLI stdout (the block managing lastMessage/currentSessionId) to detect and parse a session identifier emitted by the CLI (e.g., JSON messages or a "session_id" pattern similar to ClaudeExecutor), set currentSessionId when such a token is found, and ensure the function returns this updated currentSessionId instead of the original sessionId; update any variables/readers that reference lastMessage to extract the session id and persist it across turns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 726-734: The getCollaborativeConfig() return currently trusts
persisted values and uses ?? which lets invalid values (0, negatives, strings)
through; validate and sanitize each field before returning: ensure
architectModel is a non-empty string fallback to 'claude-opus-4-8', coerce
maxDesignRounds and maxReviewRounds to integers and enforce a minimum >=1 with
default 3 and 2 respectively, and normalize captureLearnings to a boolean (treat
truthy strings/values as true, otherwise default true). Update the logic inside
getCollaborativeConfig() to perform these checks/coercions on
config?.collaborative.{architectModel,maxDesignRounds,maxReviewRounds,captureLearnings}
and return only the validated values.
In `@src/index.ts`:
- Around line 665-674: The CLI currently only normalizes `mode` when `--mode` is
provided; instead, when `mode` is undefined from `program.opts()` but
`config.mode` exists, call `resolveMode(config.mode)` and assign the normalized
value back to `config.mode` so config-file values like "EXPERT" or other-cased
strings are normalized the same way as CLI input; update the block around
`program.opts()` where `rewritePrompt`, `baseBranch`, and `mode` are handled
(references: program.opts, config.mode, resolveMode) to perform this extra
normalization path.
In `@src/learnings/critique-distiller.ts`:
- Around line 247-253: The code currently filters existing records against
newRecordIds but appends newRecords as-is, allowing duplicate ids if the
distiller emits the same id multiple times; modify the merge to deduplicate
newRecords first (e.g., create dedupedNewRecords by keeping the first occurrence
per r.id using a Set or Map) and then set mergedRecords = [...existingRecords,
...dedupedNewRecords]; update references to newRecordIds if needed (or
recompute) so loadCanonicalRecords/mergedRecords logic uses the deduped list.
In `@src/services/task-executor.ts`:
- Around line 53-75: runImplementation currently always constructs a
CollaborativeExecutor which emits console logs and ignores the global
quiet/non-interactive setting; update runImplementation to pass the configured
quiet flag (from configManager.getCollaborativeConfig() or the TaskExecutor's
runtime options) into the CollaborativeExecutor invocation and ensure
CollaborativeExecutor (in src/services/collaborative-executor.ts) gates all
informational console output behind that quiet flag so expert-mode runs honor
non-interactive/quiet mode and do not pollute stdout; reference the
runImplementation method, CollaborativeExecutor class, and the collaborative
config/quiet option when making the changes.
---
Outside diff comments:
In `@src/services/claude-cli-executor.ts`:
- Around line 154-156: The CLI executor never updates currentSessionId (it uses
sessionId || '') so session continuity is lost; modify the claude-cli-executor
code that reads CLI stdout (the block managing lastMessage/currentSessionId) to
detect and parse a session identifier emitted by the CLI (e.g., JSON messages or
a "session_id" pattern similar to ClaudeExecutor), set currentSessionId when
such a token is found, and ensure the function returns this updated
currentSessionId instead of the original sessionId; update any variables/readers
that reference lastMessage to extract the session id and persist it across
turns.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8423fb22-9b59-48ab-af66-f517691b4feb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/config.tssrc/index.tssrc/learnings/critique-distiller.tssrc/services/claude-cli-executor.tssrc/services/claude-executor.tssrc/services/collaborative-executor.tssrc/services/executor-factory.tssrc/services/task-executor.tssrc/types/non-interactive-config.ts
📜 Review details
🔇 Additional comments (21)
src/services/executor-factory.ts (1)
5-50: LGTM!src/services/claude-executor.ts (2)
6-10: LGTM!Also applies to: 36-58
100-107: LGTM!Also applies to: 150-151, 192-200
src/services/claude-cli-executor.ts (3)
6-10: LGTM!Also applies to: 29-51
95-101: LGTM!Also applies to: 144-145
168-171:--append-system-promptCLI flag is supported by the Claude CLI.The executor passes
--append-system-promptto theclaudeCLI, and the official Claude Code CLI reference documents--append-system-promptas a valid flag for appending per-invocation system instructions without replacing the default system prompt/tool guidance.src/services/collaborative-executor.ts (10)
1-6: LGTM!Also applies to: 13-21
24-44: LGTM!
59-99: LGTM!
100-142: LGTM!
144-204: LGTM!
206-233: LGTM!
241-250: LGTM!
252-277: LGTM!
279-285: LGTM!
287-369: LGTM!src/types/non-interactive-config.ts (1)
1-2: LGTM!Also applies to: 47-53
src/config.ts (1)
22-35: LGTM!src/index.ts (1)
10-13: LGTM!Also applies to: 43-46, 722-730, 738-748, 785-789, 837-837
src/services/task-executor.ts (1)
22-24: LGTM!Also applies to: 36-46, 97-114, 482-485, 785-789
src/learnings/critique-distiller.ts (1)
22-244: LGTM!Also applies to: 254-268
| getCollaborativeConfig(): CollaborativeConfig { | ||
| const config = this.getConfig(); | ||
| const c = config?.collaborative; | ||
| return { | ||
| architectModel: c?.architectModel || 'claude-opus-4-8', | ||
| maxDesignRounds: c?.maxDesignRounds ?? 3, | ||
| maxReviewRounds: c?.maxReviewRounds ?? 2, | ||
| captureLearnings: c?.captureLearnings ?? true | ||
| }; |
There was a problem hiding this comment.
Validate collaborative settings before returning them.
At Line 731–734, ?? only handles null/undefined. Invalid persisted JSON values (e.g., 0, negative numbers, non-boolean captureLearnings) pass through and can silently skip design/review loops or mis-handle learning capture downstream.
Proposed hardening
getCollaborativeConfig(): CollaborativeConfig {
const config = this.getConfig();
const c = config?.collaborative;
+
+ const maxDesignRounds =
+ Number.isInteger(c?.maxDesignRounds) && (c?.maxDesignRounds ?? 0) > 0
+ ? (c?.maxDesignRounds as number)
+ : 3;
+ const maxReviewRounds =
+ Number.isInteger(c?.maxReviewRounds) && (c?.maxReviewRounds ?? 0) > 0
+ ? (c?.maxReviewRounds as number)
+ : 2;
+ const captureLearnings =
+ typeof c?.captureLearnings === 'boolean' ? c.captureLearnings : true;
+
return {
architectModel: c?.architectModel || 'claude-opus-4-8',
- maxDesignRounds: c?.maxDesignRounds ?? 3,
- maxReviewRounds: c?.maxReviewRounds ?? 2,
- captureLearnings: c?.captureLearnings ?? true
+ maxDesignRounds,
+ maxReviewRounds,
+ captureLearnings
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getCollaborativeConfig(): CollaborativeConfig { | |
| const config = this.getConfig(); | |
| const c = config?.collaborative; | |
| return { | |
| architectModel: c?.architectModel || 'claude-opus-4-8', | |
| maxDesignRounds: c?.maxDesignRounds ?? 3, | |
| maxReviewRounds: c?.maxReviewRounds ?? 2, | |
| captureLearnings: c?.captureLearnings ?? true | |
| }; | |
| getCollaborativeConfig(): CollaborativeConfig { | |
| const config = this.getConfig(); | |
| const c = config?.collaborative; | |
| const maxDesignRounds = | |
| Number.isInteger(c?.maxDesignRounds) && (c?.maxDesignRounds ?? 0) > 0 | |
| ? (c?.maxDesignRounds as number) | |
| : 3; | |
| const maxReviewRounds = | |
| Number.isInteger(c?.maxReviewRounds) && (c?.maxReviewRounds ?? 0) > 0 | |
| ? (c?.maxReviewRounds as number) | |
| : 2; | |
| const captureLearnings = | |
| typeof c?.captureLearnings === 'boolean' ? c.captureLearnings : true; | |
| return { | |
| architectModel: c?.architectModel || 'claude-opus-4-8', | |
| maxDesignRounds, | |
| maxReviewRounds, | |
| captureLearnings | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 726 - 734, The getCollaborativeConfig() return
currently trusts persisted values and uses ?? which lets invalid values (0,
negatives, strings) through; validate and sanitize each field before returning:
ensure architectModel is a non-empty string fallback to 'claude-opus-4-8',
coerce maxDesignRounds and maxReviewRounds to integers and enforce a minimum >=1
with default 3 and 2 respectively, and normalize captureLearnings to a boolean
(treat truthy strings/values as true, otherwise default true). Update the logic
inside getCollaborativeConfig() to perform these checks/coercions on
config?.collaborative.{architectModel,maxDesignRounds,maxReviewRounds,captureLearnings}
and return only the validated values.
| const { rewritePrompt, baseBranch, mode } = program.opts<{ | ||
| rewritePrompt: boolean; | ||
| baseBranch?: string; | ||
| mode?: string; | ||
| }>(); | ||
| if (rewritePrompt) config.rewritePrompt = true; | ||
| if (baseBranch) config.baseBranch = baseBranch; | ||
| // CLI --mode overrides the config file; config value wins only if no flag. | ||
| if (mode !== undefined) config.mode = resolveMode(mode); | ||
|
|
There was a problem hiding this comment.
Normalize config-file mode even when --mode is not passed.
At Line 673, only CLI-provided mode is resolved. If mode comes from JSON config (no CLI override), values like "EXPERT" or invalid strings bypass normalization and can silently route to simple mode downstream.
Proposed fix
if (rewritePrompt) config.rewritePrompt = true;
if (baseBranch) config.baseBranch = baseBranch;
+ config.mode = resolveMode(config.mode);
// CLI --mode overrides the config file; config value wins only if no flag.
if (mode !== undefined) config.mode = resolveMode(mode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { rewritePrompt, baseBranch, mode } = program.opts<{ | |
| rewritePrompt: boolean; | |
| baseBranch?: string; | |
| mode?: string; | |
| }>(); | |
| if (rewritePrompt) config.rewritePrompt = true; | |
| if (baseBranch) config.baseBranch = baseBranch; | |
| // CLI --mode overrides the config file; config value wins only if no flag. | |
| if (mode !== undefined) config.mode = resolveMode(mode); | |
| const { rewritePrompt, baseBranch, mode } = program.opts<{ | |
| rewritePrompt: boolean; | |
| baseBranch?: string; | |
| mode?: string; | |
| }>(); | |
| if (rewritePrompt) config.rewritePrompt = true; | |
| if (baseBranch) config.baseBranch = baseBranch; | |
| config.mode = resolveMode(config.mode); | |
| // CLI --mode overrides the config file; config value wins only if no flag. | |
| if (mode !== undefined) config.mode = resolveMode(mode); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 665 - 674, The CLI currently only normalizes
`mode` when `--mode` is provided; instead, when `mode` is undefined from
`program.opts()` but `config.mode` exists, call `resolveMode(config.mode)` and
assign the normalized value back to `config.mode` so config-file values like
"EXPERT" or other-cased strings are normalized the same way as CLI input; update
the block around `program.opts()` where `rewritePrompt`, `baseBranch`, and
`mode` are handled (references: program.opts, config.mode, resolveMode) to
perform this extra normalization path.
| const newRecordIds = new Set(newRecords.map((r) => r.id)); | ||
| const dataset = loadCanonicalRecords(repoPath); | ||
| const existingRecords = dataset.learnings.filter( | ||
| (r) => !newRecordIds.has(r.id) | ||
| ); | ||
| const mergedRecords = [...existingRecords, ...newRecords]; | ||
|
|
There was a problem hiding this comment.
Deduplicate the new batch before merge to prevent duplicate canonical IDs.
On Line [247] and Line [252], existing records are deduped against newRecordIds, but newRecords itself is appended as-is. If distillation emits repeated statements, the same deterministic id can be written multiple times to lessons.jsonl.
Proposed fix
- const newRecordIds = new Set(newRecords.map((r) => r.id));
+ const dedupedNewRecords = Array.from(
+ new Map(newRecords.map((record) => [record.id, record])).values()
+ );
+ const newRecordIds = new Set(dedupedNewRecords.map((r) => r.id));
const dataset = loadCanonicalRecords(repoPath);
const existingRecords = dataset.learnings.filter(
(r) => !newRecordIds.has(r.id)
);
- const mergedRecords = [...existingRecords, ...newRecords];
+ const mergedRecords = [...existingRecords, ...dedupedNewRecords];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/learnings/critique-distiller.ts` around lines 247 - 253, The code
currently filters existing records against newRecordIds but appends newRecords
as-is, allowing duplicate ids if the distiller emits the same id multiple times;
modify the merge to deduplicate newRecords first (e.g., create dedupedNewRecords
by keeping the first occurrence per r.id using a Set or Map) and then set
mergedRecords = [...existingRecords, ...dedupedNewRecords]; update references to
newRecordIds if needed (or recompute) so loadCanonicalRecords/mergedRecords
logic uses the deduped list.
| private async runImplementation( | ||
| taskWithInstructions: string, | ||
| executionPath: string, | ||
| sessionId?: string | ||
| ): Promise<TurnResult> { | ||
| if (this.executionMode === 'expert') { | ||
| const collaborative = new CollaborativeExecutor( | ||
| this.getClaudeExecutor(), | ||
| this.configManager.getCollaborativeConfig() | ||
| ); | ||
| return collaborative.run({ | ||
| taskDescription: taskWithInstructions, | ||
| executionPath, | ||
| learningsRepoPath: this.workingDir, | ||
| getDiff: () => { | ||
| if (!this.gitManager) { | ||
| throw new Error('GitManager not initialized'); | ||
| } | ||
| return this.gitManager.getDiff(); | ||
| }, | ||
| incomingSessionId: sessionId | ||
| }); | ||
| } |
There was a problem hiding this comment.
Expert mode breaks non-interactive quiet-output expectations.
At Line 53–75, expert execution always runs through CollaborativeExecutor, which emits direct console.logs. In non-interactive flow (Line 1035), quiet mode is enabled specifically to suppress intermediate output, so expert runs can pollute stdout and break machine consumers.
Suggested direction
private async runImplementation(
taskWithInstructions: string,
executionPath: string,
sessionId?: string
): Promise<TurnResult> {
if (this.executionMode === 'expert') {
const collaborative = new CollaborativeExecutor(
this.getClaudeExecutor(),
- this.configManager.getCollaborativeConfig()
+ this.configManager.getCollaborativeConfig(),
+ { quiet: this.getClaudeExecutor().quietMode }
);And in src/services/collaborative-executor.ts, gate all informational logs behind that quiet option.
Also applies to: 1031-1035
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/task-executor.ts` around lines 53 - 75, runImplementation
currently always constructs a CollaborativeExecutor which emits console logs and
ignores the global quiet/non-interactive setting; update runImplementation to
pass the configured quiet flag (from configManager.getCollaborativeConfig() or
the TaskExecutor's runtime options) into the CollaborativeExecutor invocation
and ensure CollaborativeExecutor (in src/services/collaborative-executor.ts)
gates all informational console output behind that quiet flag so expert-mode
runs honor non-interactive/quiet mode and do not pollute stdout; reference the
runImplementation method, CollaborativeExecutor class, and the collaborative
config/quiet option when making the changes.
2 participants
This pull request introduces a mechanism to capture and persist architect critiques as institutional learnings in expert mode. After completing a task, critiques that prompted revisions will be distilled into reusable LearningRecords, enhancing the system's knowledge over time.
Main Changes:
captureLearningsboolean to the collaborative config, defaulting to true.CritiqueDistillerclass to process architect critiques and generate LearningRecords.CollaborativeExecutor.runto collect critiques and invoke the distiller after task completion.Important Notes:
source_typevalue 'collaborative_review'.Co-authored with @ivan-agent
Summary by CodeRabbit
--modeCLI flag with "simple" and "expert" execution options