feat: add dry-run mode to suggest#93
Conversation
|
This PR has merge conflicts needs to be resolved😊 |
|
Thanks for the heads-up — I merged latest |
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
Thanks for the PR — the dry-run mode is a great addition. I found a couple of blocking correctness issues that should be fixed before merge:
- Dry-run prompt construction diverges from real generation
- In
suggestCommanddry-run usesbuildSystemPrompt(profile)andbuildUserPrompt(diff)directly. - Actual generation uses
resolveSystemPrompt(...)/resolveUserPrompt(...)with template vars (diff,profile,branch) andconfig. - Result: when users set
systemPromptTemplateoruserPromptTemplate, dry-run output does not reflect the real LLM payload.
- Dry-run truncation output diverges from real generation
- Dry-run currently prints full diff and hardcodes truncation as none/sent in full.
- Actual generation runs
truncateDiff(diff, config.maxDiffSize)before prompting. - Result: dry-run can mislead users about what is actually sent to the model.
- Tests should cover parity behavior
- Please add/adjust tests to assert dry-run matches real pipeline behavior for:
- custom prompt templates (template var substitution path), and
- truncation (large diff shows truncated payload + accurate truncation status).
Once these are aligned, this should be in good shape.
|
Aligned dry-run with the real generation pipeline in the latest commit: it now applies |
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
Request changes.
The new dry-run mode is useful, but the PR changes the user-facing CLI contract without preserving compatibility. The repository README still documents commit-echo suggest --no-commit, while the implementation now exposes --dry-run only. That means existing users following the published docs will hit an unknown-option failure, and there is no alias to keep the old invocation working.
Please either keep --no-commit as a compatibility alias or update the documentation and any related examples/tests so the published command surface matches the implementation.
|
Addressed in 4c4fb25.\n\nChanges:\n- restored as a deprecated compatibility alias so existing documented invocations no longer fail with an unknown option\n- updated the README example to show the current form while explicitly noting the compatibility alias\n- added an e2e regression test that exercises end-to-end\n\nValidation:\n- \n- |
|
Follow-up with the correctly formatted details: addressed in 4c4fb25. Changes:
Validation:
|
|
And resolve merge conflicts |
|
Resolved the remaining merge conflicts on Re-ran:
|
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
Overview
This PR adds a --dry-run / -n flag to commit-echo suggest. When invoked, it prints the diff, style profile, resolved system/user prompts, and truncation status without making any LLM API call. It also retroactively documents --no-commit as a deprecated compatibility alias. Closes #90.
Files changed: 5 (387 additions, 91 deletions)
✅ What is good
-
Clean architecture — The dry-run path reuses existing infrastructure (
truncateDiff,resolveSystemPrompt,resolveUserPrompt,buildProfile,formatProfile) so its output is guaranteed to match what the real LLM call would receive. No duplication, no drift. -
Early return — The dry-run check returns before any spinner or LLM call, so
--dry-runis instantaneous and has zero chance of accidentally hitting the API. -
formatDryRunOutputis well-factored — Pure function, exported, independently unit-testable. The output is clear and informative, showing each section under bold headers. -
Backward compatibility —
--no-commitis preserved and now explicitly documented as a deprecated alias. The new test confirms it still works. -
Unit tests cover the key paths — The three tests in
tests/suggest-dry-run.test.mjscover basic formatting, template variable substitution, and truncation display.
🚩 Required changes
1. String quote style contradicts Prettier config
The project's .prettierrc sets "singleQuote": true, but this PR converts all string literals in suggest.ts and index.ts from single quotes to double quotes. This means running npx prettier --check on these files will now fail.
The formatting changes also add a lot of noise to the diff (~200 of the 387 additions are just quote/style changes), making the actual logic harder to review.
Fix: Run npx prettier --write src/commands/suggest.ts src/index.ts to restore single-quote consistency, then force-push the corrected branch.
2. Missing E2E test for --dry-run itself
The dry-run path exercises the entire pipeline — config loading, git diff gathering, profile building, diff truncation, and prompt resolution — but there is no E2E test that actually runs commit-echo suggest --dry-run. The existing E2E test for --no-commit tests the CLI bootstrap but still hits the mock LLM server.
A single E2E test for --dry-run would validate that all the pieces wire together correctly without needing a mock HTTP server:
test("suggest --dry-run prints LLM inputs without calling the API", async (t) => {
// ... setup ...
const result = await runSuggestUntil(
["suggest", "--dry-run"],
{ cwd: repo, env, text: "Dry run: no LLM API call will be made." },
);
assert.match(result.stdout, /Style profile:/);
assert.match(result.stdout, /System prompt:/);
assert.match(result.stdout, /User prompt:/);
assert.match(result.stdout, /Truncation:/);
assert.doesNotMatch(result.stdout, /Suggestions generated:/);
});🔍 Minor suggestions (not blocking)
-
Consider showing the model name in dry-run output — The non-dry-run
--verboseoutput prints the configured model. Includingconfig.modelin the dry-run output would give a complete picture of what the LLM call would look like:Model: gpt-4.1 -
README could use a
--dry-runexample — Consider adding a--dry-runusage line near the existingsuggestexamples to make the new flag more discoverable.
📋 Summary
| Category | Verdict |
|---|---|
| Correctness | Solid |
| Test coverage | Good for unit tests, needs an E2E test |
| Code quality | Clean, well-structured |
| Style/formatting | Prettier incompatibility - must fix |
| Backward compat | --no-commit preserved and documented |
| Security | No concerns |
Overall: A well-implemented feature with a clean architecture. The two required changes are: (1) run Prettier to fix the quote style to match the project config, and (2) add an E2E test for the --dry-run flag.
|
Note: This PR also has merge conflicts with |
|
Addressed the latest review feedback in e6949fe. Changes:
Validation:
|
There was a problem hiding this comment.
Thanks for the implementation and test coverage expansion here. I’m requesting changes before merge due to one blocking process concern and a couple of important follow-ups.
Important (non-blocking but should be addressed in this PR)
- There is significant formatting-only churn mixed with behavior changes in
src/commands/suggest.tsandsrc/llm/client.ts. Please split/reduce unrelated formatting edits to make the functional diff easier to validate. - If available in your workflow, include test evidence in the PR description (or paste a short run summary) for the new dry-run and no-commit compatibility scenarios.
|
Addressed the latest follow-up in 7307d44.\n\nChanges:\n- merged the latest upstream |
|
Rechecked this follow-up today: the branch is up to date with current |
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
Review: feat: add dry-run mode to suggest
What's done well
- Dry-run reuses the real prompt pipeline —
truncateDiff→resolveSystemPrompt→resolveUserPromptguarantees the output exactly matches what would be sent to the API. Great design. formatDryRunOutputis a pure, exported, well-tested function — easy to unit test, no side effects, clear contract.--no-commitbackward compat is correctly handled via Commander's--no-prefix.- Test coverage hits unit, template-parity, truncation-parity, and E2E scenarios.
- README updated to reflect current usage.
Changes requested
1. Merge conflicts must be resolved (blocking)
The mergeable_state is currently blocked. Please rebase on latest main and resolve the conflicts before this can be merged.
2. Add a deprecation warning for --no-commit
--no-commit is silently accepted with no signal to users that they should migrate. When someone runs commit-echo suggest --no-commit, they should see a brief note. Something like:
if (options.noCommit) {
console.warn(pc.yellow("Note: --no-commit is deprecated; 'commit-echo suggest' already skips committing."));
}The --no-commit option currently maps to Commander's built-in --no- prefix on the --commit boolean, so detecting it requires either:
- Adding a separate
--no-commitoption and checking it explicitly, or - Using
program.parseOptionshooks, or - A simpler approach: add a dedicated
--no-commitoption that sets a flag you can check.
This is a minor UX improvement but worth addressing for a clean deprecation path.
3. Move tests/suggest-dry-run.test.mjs to tests/e2e/
This file spawns the CLI process (process.execPath + dist/index.js) — it's functionally an E2E test. The existing convention places E2E tests under tests/e2e/. Please move it to tests/e2e/suggest-dry-run.test.mjs for consistency with the rest of the test suite.
4. Separate formatting changes from the feature
The diff includes many Prettier-driven whitespace reformatting changes in tests/e2e/suggest-smoke.test.mjs (wrapping long objects into multi-line, reformatting arrow functions). These are cosmetic and inflate the diff. Consider:
- Run
npx prettier --write .in a standalone commit onmainfirst - Rebase this PR on top of that formatting commit
This keeps the feature diff clean and reviewable.
|
Thanks for the review — I pushed an update addressing the requested items:
Validation run locally: Both passed (7 tests). I also ran |
nightcityblade
force-pushed
the
fix/issue-90-dry-run
branch
from
fd5784d to
ca993b1
Compare
|
Addressed the latest review feedback and force-pushed an updated branch. Changes in this push:
Validation run:
|
|
Conflict resolution for Two files conflict because
|
nightcityblade
force-pushed
the
fix/issue-90-dry-run
branch
from
ca993b1 to
7c26a53
Compare
|
Thanks — I resolved the latest What I aligned with your notes:
Validation:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements a ChangesDry-Run Flag Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
Comment |
2 participants
Fixes #90
Summary
-n, --dry-runtocommit-echo suggest--no-commitas a deprecated compatibility aliasTests
npm run build✅npm test✅node --test tests/e2e/suggest-smoke.test.mjs tests/suggest-dry-run.test.mjs tests/cli-help.test.mjs✅npm run format:checksrc/commands/init.ts,src/commands/suggest.ts,src/git/hook.ts, andsrc/llm/client.ts; left unformatted to avoid reintroducing unrelated churn.Summary by CodeRabbit
New Features
--dry-runflag to thesuggestcommand to preview LLM input without generating suggestions--no-commitflag deprecated with compatibility warningDocumentation
suggestcommandTests