Conversation
…T-64) Add ILiberateProgress type and optional onProgress callback to ILiberateOptions. LiberateService emits progress at triage, enriching, and complete phases. Both init and liberate CLI commands use the callback to write in-place progress to stderr. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds progress reporting to the liberate flow: a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI Command"
participant Wrapper as "liberateWithProgress"
participant Service as "LiberateService"
participant Handler as "createStderrProgressHandler / stderr"
CLI->>Wrapper: call liberateWithProgress(fn, onProgress)
Wrapper->>Service: fn() -> liberate({ onProgress })
Service->>Handler: onProgress({ phase: "triage", total })
Service->>Handler: onProgress({ phase: "processing", current, total, sha, subject, factsExtracted })
Service->>Handler: onProgress({ phase: "complete", current: total, total, factsExtracted })
Handler-->>CLI: write updates to stderr (TTY-aware)
Wrapper-->>CLI: ensure final newline / emit final complete if needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/commands/liberate.ts`:
- Around line 52-62: The onProgress callback in liberate.ts duplicates logic
found in init.ts; extract this shared formatting into a helper factory (e.g.,
createStderrProgressHandler) that returns (p: ILiberateProgress) => void and
implements the triage/enriching/complete branches (use p.sha.slice(0,7),
truncate p.subject to 60 chars, and write to process.stderr as currently done);
replace the local onProgress in liberate.ts (and the one in init.ts) by calling
createStderrProgressHandler() to get the handler and pass that to the liberate
service so both commands reuse the same implementation.
There was a problem hiding this comment.
Pull request overview
This PR adds progress reporting to the liberate workflow so the CLI can provide real-time feedback during commit triage and per-commit processing, without impacting existing service callers (progress is optional).
Changes:
- Introduces
ILiberateProgressand an optionalonProgresscallback onILiberateOptions. - Emits progress events from
LiberateServiceat triage, per-commit processing, and completion. - Updates
initandliberatecommands to render in-place progress updates tostderr.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/commands/liberate.ts | Adds an onProgress callback to print triage + per-commit progress to stderr. |
| src/commands/init.ts | Adds the same stderr progress reporting when init runs liberate with enrichment. |
| src/application/services/LiberateService.ts | Emits progress callbacks during triage/per-commit loop and on completion. |
| src/application/interfaces/ILiberateService.ts | Defines ILiberateProgress and adds onProgress to options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/commands/liberate.ts
Outdated
| const onProgress = (p: ILiberateProgress): void => { | ||
| if (p.phase === 'triage') { | ||
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | ||
| } else if (p.phase === 'enriching') { | ||
| const sha = p.sha.slice(0, 7); | ||
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | ||
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | ||
| } else if (p.phase === 'complete') { | ||
| process.stderr.write('\n'); | ||
| } | ||
| }; | ||
|
|
||
| const result = await liberateService.liberate({ | ||
| since, | ||
| maxCommits, | ||
| dryRun: options.dryRun, | ||
| threshold, | ||
| enrich: options.enrich, | ||
| onProgress, | ||
| }); |
There was a problem hiding this comment.
If liberateService.liberate() throws after writing an in-place progress line, the terminal may be left mid-line and subsequent error output will be messy. Consider wrapping the call in a try/finally that ensures a trailing newline is written if progress was started.
| const onProgress = (p: ILiberateProgress): void => { | |
| if (p.phase === 'triage') { | |
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | |
| } else if (p.phase === 'enriching') { | |
| const sha = p.sha.slice(0, 7); | |
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | |
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | |
| } else if (p.phase === 'complete') { | |
| process.stderr.write('\n'); | |
| } | |
| }; | |
| const result = await liberateService.liberate({ | |
| since, | |
| maxCommits, | |
| dryRun: options.dryRun, | |
| threshold, | |
| enrich: options.enrich, | |
| onProgress, | |
| }); | |
| let inPlaceProgressStarted = false; | |
| const onProgress = (p: ILiberateProgress): void => { | |
| if (p.phase === 'triage') { | |
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | |
| } else if (p.phase === 'enriching') { | |
| const sha = p.sha.slice(0, 7); | |
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | |
| inPlaceProgressStarted = true; | |
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | |
| } else if (p.phase === 'complete') { | |
| if (inPlaceProgressStarted) { | |
| process.stderr.write('\n'); | |
| inPlaceProgressStarted = false; | |
| } | |
| } | |
| }; | |
| const result = await (async () => { | |
| try { | |
| return await liberateService.liberate({ | |
| since, | |
| maxCommits, | |
| dryRun: options.dryRun, | |
| threshold, | |
| enrich: options.enrich, | |
| onProgress, | |
| }); | |
| } finally { | |
| if (inPlaceProgressStarted) { | |
| process.stderr.write('\n'); | |
| inPlaceProgressStarted = false; | |
| } | |
| } | |
| })(); |
src/commands/init.ts
Outdated
| const onProgress = (p: ILiberateProgress): void => { | ||
| if (p.phase === 'triage') { | ||
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | ||
| } else if (p.phase === 'enriching') { | ||
| const sha = p.sha.slice(0, 7); | ||
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | ||
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | ||
| } else if (p.phase === 'complete') { | ||
| process.stderr.write('\n'); |
There was a problem hiding this comment.
The progress output uses \r unconditionally. When stderr is not a TTY (CI logs, redirected output), this will leave raw carriage returns in logs. Consider gating the in-place update behind process.stderr.isTTY and falling back to newline-based progress (or no progress) when not a TTY; also clear the line before rewriting so shorter subjects don't leave trailing characters from the previous update.
| const onProgress = (p: ILiberateProgress): void => { | |
| if (p.phase === 'triage') { | |
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | |
| } else if (p.phase === 'enriching') { | |
| const sha = p.sha.slice(0, 7); | |
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | |
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | |
| } else if (p.phase === 'complete') { | |
| process.stderr.write('\n'); | |
| const isTTY = process.stderr.isTTY; | |
| let lastProgressLength = 0; | |
| const onProgress = (p: ILiberateProgress): void => { | |
| if (p.phase === 'triage') { | |
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | |
| } else if (p.phase === 'enriching') { | |
| const sha = p.sha.slice(0, 7); | |
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | |
| const line = ` [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`; | |
| if (isTTY) { | |
| const paddedLine = line.padEnd(lastProgressLength, ' '); | |
| lastProgressLength = line.length; | |
| process.stderr.write(`\r${paddedLine}`); | |
| } else { | |
| process.stderr.write(`${line}\n`); | |
| } | |
| } else if (p.phase === 'complete') { | |
| if (isTTY) { | |
| process.stderr.write('\n'); | |
| } |
src/commands/init.ts
Outdated
| const result = await liberateService.liberate({ | ||
| maxCommits: commitCount, | ||
| enrich: true, | ||
| onProgress, | ||
| }); |
There was a problem hiding this comment.
If liberateService.liberate() throws after writing an in-place progress line, the terminal may be left mid-line and subsequent error output will be messy. Consider wrapping the call in a try/finally that ensures a trailing newline is written if progress was started.
| export interface ILiberateProgress { | ||
| /** Current phase of processing. */ | ||
| readonly phase: 'triage' | 'enriching' | 'complete'; | ||
| /** Current commit index (1-based). */ |
There was a problem hiding this comment.
ILiberateProgress.current is documented as 1-based, but the service emits { phase: 'triage', current: 0, ... }. Either update the documentation to explicitly allow 0 for non-per-commit phases (triage/complete), or change the emitted value to match the documented contract.
| /** Current commit index (1-based). */ | |
| /** | |
| * Current commit index. | |
| * 1-based during per-commit phases (e.g. 'enriching'); 0 during 'triage' and 'complete'. | |
| */ |
| let commitIndex = 0; | ||
| for (const scored of triageResult.highInterest) { | ||
| commitIndex++; | ||
| options?.onProgress?.({ phase: 'enriching', current: commitIndex, total: highInterestTotal, sha: scored.commit.sha, subject: scored.commit.subject, factsExtracted: totalFactsExtracted }); |
There was a problem hiding this comment.
The service emits phase: 'enriching' for every high-interest commit even when options.enrich is false and no LLM enrichment occurs. This makes the phase name misleading for consumers; consider renaming the phase (e.g., to 'processing'/'extracting') or only emitting 'enriching' when enrichment is actually enabled, with a separate phase for heuristic-only processing.
| options?.onProgress?.({ phase: 'enriching', current: commitIndex, total: highInterestTotal, sha: scored.commit.sha, subject: scored.commit.subject, factsExtracted: totalFactsExtracted }); | |
| const phase = shouldEnrich ? 'enriching' : 'processing'; | |
| options?.onProgress?.({ phase, current: commitIndex, total: highInterestTotal, sha: scored.commit.sha, subject: scored.commit.subject, factsExtracted: totalFactsExtracted }); |
| const highInterestTotal = triageResult.highInterest.length; | ||
| options?.onProgress?.({ phase: 'triage', current: 0, total: highInterestTotal, sha: '', subject: '', factsExtracted: 0 }); | ||
|
|
||
| // Process each high-interest commit | ||
| let commitIndex = 0; | ||
| for (const scored of triageResult.highInterest) { | ||
| commitIndex++; | ||
| options?.onProgress?.({ phase: 'enriching', current: commitIndex, total: highInterestTotal, sha: scored.commit.sha, subject: scored.commit.subject, factsExtracted: totalFactsExtracted }); |
There was a problem hiding this comment.
New progress-callback behavior is introduced here but there are existing unit/integration tests for LiberateService and none assert the progress contract. Consider adding tests that verify onProgress is called with expected phases/order and that current/total/factsExtracted values match the documented behavior (including the zero-high-interest case).
src/commands/init.ts
Outdated
| const onProgress = (p: ILiberateProgress): void => { | ||
| if (p.phase === 'triage') { | ||
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | ||
| } else if (p.phase === 'enriching') { | ||
| const sha = p.sha.slice(0, 7); | ||
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | ||
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | ||
| } else if (p.phase === 'complete') { | ||
| process.stderr.write('\n'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The onProgress formatter logic here is duplicated in src/commands/liberate.ts. To avoid the two commands drifting (message text, truncation rules, TTY handling, etc.), consider extracting a shared helper for rendering ILiberateProgress to stderr.
src/commands/liberate.ts
Outdated
| const onProgress = (p: ILiberateProgress): void => { | ||
| if (p.phase === 'triage') { | ||
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | ||
| } else if (p.phase === 'enriching') { | ||
| const sha = p.sha.slice(0, 7); | ||
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | ||
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); |
There was a problem hiding this comment.
The progress output uses \r unconditionally. When stderr is not a TTY (CI logs, redirected output), this will leave raw carriage returns in logs. Consider gating the in-place update behind process.stderr.isTTY and falling back to newline-based progress (or no progress) when not a TTY; also clear the line before rewriting so shorter subjects don't leave trailing characters from the previous update.
| const onProgress = (p: ILiberateProgress): void => { | |
| if (p.phase === 'triage') { | |
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | |
| } else if (p.phase === 'enriching') { | |
| const sha = p.sha.slice(0, 7); | |
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | |
| process.stderr.write(`\r [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`); | |
| let lastProgressLineLength = 0; | |
| const onProgress = (p: ILiberateProgress): void => { | |
| if (p.phase === 'triage') { | |
| process.stderr.write(`Found ${p.total} high-interest commits to analyze.\n`); | |
| } else if (p.phase === 'enriching') { | |
| const sha = p.sha.slice(0, 7); | |
| const subject = p.subject.length > 60 ? p.subject.slice(0, 57) + '...' : p.subject; | |
| const line = ` [${p.current}/${p.total}] ${sha} ${subject} (${p.factsExtracted} facts)`; | |
| if (process.stderr.isTTY) { | |
| const clear = '\r' + ' '.repeat(lastProgressLineLength) + '\r'; | |
| process.stderr.write(clear + line); | |
| lastProgressLineLength = line.length; | |
| } else { | |
| process.stderr.write(line + '\n'); | |
| } |
- Extract shared createStderrProgressHandler() into src/commands/progress.ts - Add liberateWithProgress() try/finally wrapper for terminal cleanup - Gate \r updates behind process.stderr.isTTY; use newlines in non-TTY - Rename phase 'enriching' to 'processing' (accurate for heuristic-only) - Fix doc: current is 0 for triage/complete phases, 1-based for processing - Add 2 unit tests verifying onProgress contract and zero-commit edge case Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/application/interfaces/ILiberateService.ts`:
- Around line 12-15: Update the ILiberateService.docs for the current property
to match runtime behavior: state that current is 0 during 'triage', is a 1-based
commit index during 'processing', and equals total when phase is 'complete'
(instead of saying 0 for complete); reference the ILiberateService interface and
its phase and current properties so consumers see the exact semantics.
In `@tests/unit/application/services/LiberateService.test.ts`:
- Around line 98-143: The tests in LiberateService.test.ts call service.liberate
and indirectly exercise real git/FS via GitClient/MemoryRepository; change them
to be true unit tests by mocking those dependencies (or move the file to
tests/integration). Replace real GitClient/MemoryRepository usage in the test
setup with manual mocks/stubs that implement the minimal behaviors used by
LiberateService.liberate (e.g., return controlled commit lists, shas, subjects,
and file contents) so ILiberateProgress events can be asserted
deterministically; ensure the test injects the mocked client into the service
instance (or the constructor/factory used by LiberateService) rather than
invoking real git commands. Also keep the same assertions on events
(triage/processing/complete) but driven from the mocked responses to keep the
test fast and isolated.
| /** Current phase of processing. */ | ||
| readonly phase: 'triage' | 'processing' | 'complete'; | ||
| /** Current commit index (1-based during 'processing'; 0 for triage/complete). */ | ||
| readonly current: number; |
There was a problem hiding this comment.
Clarify current semantics in progress docs.
LiberateService emits current = total on the complete phase, but the comment says 0 for triage/complete. Align the doc (or the emitter) to avoid ambiguity for consumers.
✏️ Suggested doc tweak
- /** Current commit index (1-based during 'processing'; 0 for triage/complete). */
+ /** Current commit index (1-based during 'processing'; 0 for triage; equals total for complete). */📝 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.
| /** Current phase of processing. */ | |
| readonly phase: 'triage' | 'processing' | 'complete'; | |
| /** Current commit index (1-based during 'processing'; 0 for triage/complete). */ | |
| readonly current: number; | |
| /** Current phase of processing. */ | |
| readonly phase: 'triage' | 'processing' | 'complete'; | |
| /** Current commit index (1-based during 'processing'; 0 for triage; equals total for complete). */ | |
| readonly current: number; |
🤖 Prompt for AI Agents
In `@src/application/interfaces/ILiberateService.ts` around lines 12 - 15, Update
the ILiberateService.docs for the current property to match runtime behavior:
state that current is 0 during 'triage', is a 1-based commit index during
'processing', and equals total when phase is 'complete' (instead of saying 0 for
complete); reference the ILiberateService interface and its phase and current
properties so consumers see the exact semantics.
| it('should call onProgress with triage, processing, and complete phases', async () => { | ||
| const events: ILiberateProgress[] = []; | ||
|
|
||
| await service.liberate({ | ||
| cwd: repoDir, | ||
| dryRun: true, | ||
| threshold: 1, | ||
| onProgress: (p) => events.push({ ...p }), | ||
| }); | ||
|
|
||
| assert.ok(events.length >= 2, 'should emit at least triage + complete'); | ||
|
|
||
| // First event is triage | ||
| assert.equal(events[0].phase, 'triage'); | ||
| assert.equal(events[0].current, 0); | ||
| assert.ok(events[0].total >= 0); | ||
|
|
||
| // Last event is complete | ||
| const last = events[events.length - 1]; | ||
| assert.equal(last.phase, 'complete'); | ||
|
|
||
| // All middle events are processing | ||
| const processingEvents = events.filter(e => e.phase === 'processing'); | ||
| for (let i = 0; i < processingEvents.length; i++) { | ||
| assert.equal(processingEvents[i].current, i + 1, 'current should be 1-based'); | ||
| assert.ok(processingEvents[i].sha.length > 0, 'sha should be non-empty'); | ||
| assert.ok(processingEvents[i].subject.length > 0, 'subject should be non-empty'); | ||
| } | ||
| }); | ||
|
|
||
| it('should emit triage and complete even with zero high-interest commits', async () => { | ||
| const events: ILiberateProgress[] = []; | ||
|
|
||
| await service.liberate({ | ||
| cwd: repoDir, | ||
| dryRun: true, | ||
| threshold: 100, | ||
| onProgress: (p) => events.push({ ...p }), | ||
| }); | ||
|
|
||
| assert.equal(events.length, 2); | ||
| assert.equal(events[0].phase, 'triage'); | ||
| assert.equal(events[0].total, 0); | ||
| assert.equal(events[1].phase, 'complete'); | ||
| assert.equal(events[1].factsExtracted, 0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unit-test isolation: consider mocking Git/FS dependencies.
These new tests still exercise real git commands and filesystem via GitClient/MemoryRepository. For unit tests, prefer manual mocks or move this to tests/integration to keep unit tests isolated and fast.
As per coding guidelines: "tests/unit/**/*.{ts,tsx}: Create unit tests with manually mocked dependencies (no testing framework) in tests/unit/ directory".
🤖 Prompt for AI Agents
In `@tests/unit/application/services/LiberateService.test.ts` around lines 98 -
143, The tests in LiberateService.test.ts call service.liberate and indirectly
exercise real git/FS via GitClient/MemoryRepository; change them to be true unit
tests by mocking those dependencies (or move the file to tests/integration).
Replace real GitClient/MemoryRepository usage in the test setup with manual
mocks/stubs that implement the minimal behaviors used by
LiberateService.liberate (e.g., return controlled commit lists, shas, subjects,
and file contents) so ILiberateProgress events can be asserted
deterministically; ensure the test injects the mocked client into the service
instance (or the constructor/factory used by LiberateService) rather than
invoking real git commands. Also keep the same assertions on events
(triage/processing/complete) but driven from the mocked responses to keep the
test fast and isolated.
Summary
ILiberateProgresstype and optionalonProgresscallback toILiberateOptionsLiberateServiceemits progress at triage, enriching, and complete phasesinitandliberateCLI commands write in-place progress to stderr using\ronProgressis optionalCloses GIT-64
Test plan
npm run pre-commitpasses (type-check + lint)git-mem liberate --enrich --commit-count 10shows per-commit progressgit-mem initwithANTHROPIC_API_KEYin.envshows progress🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests