feat: distinguish dry-run copies in JSON and human output#17
Conversation
- Add DryRun bool field to Result struct (JSON: "dry_run") - Split Summary.Copied into Copied (real mode) and CopyPlanned (dry-run mode) with omitempty for mutual exclusivity - Update executePrepared to increment CopyPlanned in dry-run, Copied in real mode - Update SUMMARY human output to use copy_planned= in dry-run mode - Update jsonResult struct in CLI integration tests - Add TestApplyDryRunJSON CLI integration test - Add TestEngineApplyDryRunCopyPlanned engine integration test - Update README with separate dry-run and normal JSON output examples Agent-Logs-Url: https://github.com/satococoa/git-worktreeinclude/sessions/b43b6731-413f-4d2f-ae93-143bd52bfa32 Co-authored-by: satococoa <31448+satococoa@users.noreply.github.com>
b03c927
into
copilot/discuss-doctor-command-necessity
There was a problem hiding this comment.
Pull request overview
This PR removes ambiguity in apply --dry-run output by clearly distinguishing planned copies from performed copies in both JSON output and the human-readable SUMMARY line.
Changes:
- Added
dry_runto the JSONResultpayload and populated it from the engine. - Split
summary.copiedintosummary.copiedvssummary.copy_planned(mutually exclusive viaomitempty) and updated engine counting logic accordingly. - Updated CLI summary labeling, README examples, and added/updated integration tests to assert the new dry-run contract.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents dry_run and shows separate JSON examples/field naming for dry-run vs normal execution. |
| internal/engine/engine.go | Adds Result.DryRun, splits summary counters (Copied/CopyPlanned), and increments the correct one based on dryRun. |
| internal/engine/engine_integration_test.go | Adds engine-level dry-run test coverage for DryRun, CopyPlanned, and copy action status="planned". |
| internal/cli/cli.go | Prints copy_planned= in dry-run SUMMARY output and copied= otherwise. |
| internal/cli/cli_integration_test.go | Updates JSON decoding struct and adds CLI-level dry-run JSON/verbose assertions for the new fields/labels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ebb9ea274
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| Copied int `json:"copied,omitempty"` | ||
| CopyPlanned int `json:"copy_planned,omitempty"` |
There was a problem hiding this comment.
Preserve mode-specific summary key when count is zero
omitempty on both Summary.Copied and Summary.CopyPlanned causes both counters to disappear whenever the value is 0 (for example, apply --dry-run --json when all matched files are already identical, or a normal no-op run). That makes dry-run JSON omit copy_planned even though the CLI/docs describe it as the dry-run counter, and it also changes the previous JSON shape where copied was always emitted, which can break consumers that key off field presence.
Useful? React with 👍 / 👎.
apply --dry-runwas incrementingsummary.copiedand printingcopied=even though no files were written, making it ambiguous whether the count reflects actual copies or planned ones.Changes
engine.goDryRun booltoResult(JSON:"dry_run")Summary.Copied→Copied/CopyPlannedwithomitempty; they are mutually exclusive in JSON outputexecutePreparedincrementsCopyPlannedwhendryRun=true,Copiedotherwisecli.gocopy_planned=in dry-run mode,copied=in real modeREADME
--dry-runflag description notes the label differenceTests
jsonResultupdated withDryRunandCopyPlannedfieldsTestApplyDryRunVerboseOutputassertscopy_planned=present,copied=absentTestApplyDryRunJSON: verifiesdry_run=true,copy_planned>0,copied==0, all copy actionsstatus="planned"TestEngineApplyDryRunCopyPlanned: same contract at engine levelJSON example
Original prompt
Context
The PR branch
copilot/discuss-doctor-command-necessityalready removed thedoctorcommand. However, theapplycommand still usescopiedin both the JSON summary field and the human-readable SUMMARY line, even during--dry-runmode where no files are actually copied. This is ambiguous.Required Changes
1. Add
DryRunfield toResult(JSON:"dry_run")In
internal/engine/engine.go, add aDryRun boolfield to theResultstruct so the JSON output includes"dry_run": trueor"dry_run": false. This field should be set byexecutePreparedbased on thedryRunparameter.2. Split
Summary.CopiedintoCopiedandCopyPlannedIn
internal/engine/engine.go:Summarystruct to have two fields:CopiedandCopyPlannedare mutually exclusive viaomitempty: in dry-run mode onlyCopyPlannedis non-zero; in real execution mode onlyCopiedis non-zero.executePrepared, where it currently doesresult.Summary.Copied++, change it to:dryRunis true:result.Summary.CopyPlanned++dryRunis false:result.Summary.Copied++3. Update human-readable SUMMARY line in
internal/cli/cli.goIn the
runApplyfunction, change the SUMMARY format string to usecopy_planned=whendryRunis true andcopied=whendryRunis false:When
dryRunis true:using
result.Summary.CopyPlannedWhen
dryRunis false:using
result.Summary.Copied4. Update README.md
"dry_run": trueand"copy_planned"instead of"copied"."dry_run": falseand"copied".--dry-runflag description explaining that in dry-run mode, the summary usescopy_plannedinstead ofcopied.5. Update tests
internal/engine/engine_integration_test.goTestEngineApplyCopiesIgnoredFiles: currently checksres.Summary.Copied != 2— keep as-is since this is a real apply (not dry-run).TestEngineApplyIncludeValidationAndNoop: checksres.Summary.Copied != 0— keep as-is.TestEngineApplyUsesSourceIncludeWhenTargetIncludeMissing: checksres.Summary.Copied != 2— keep as-is.TestEngineApplyNoopWhenSourceIncludeMissingEvenIfTargetHasInclude: checksres.Summary.Copied != 0— keep as-is.CopyPlannedis set andCopiedis 0, and thatDryRunistruein the result.internal/cli/cli_integration_test.gojsonResultstruct to includeDryRun booland bothCopiedandCopyPlannedfields.TestApplyAC1AC2AC6AC7: keep checkingres.Summary.Copied(not dry-run).TestApplyAC8MissingIncludeIsNoop: update to check bothCopiedandCopyPlannedare 0.TestApplyUsesSourceIncludeWhenTargetIncludeMissing: keep checkingres.Summary.Copied(not dry-run).TestApplyDryRunVerboseOutput: update to check forcopy_planned=instead ofcopied=in the SUMMARY line.TestApplyNoopWhenSourceIncludeMissingEvenIfTargetHasInclude: update if it checksCopied.TestApplyDryRunJSONthat runsapply --dry-run --jsonand verifies:res.DryRun == trueres.Summary.CopyPlanned > 0res.Summary.Copied == 0status: "planned"(for copy ops)internal/cli/cli_unit_test.goImportant notes
main. Push only to the existing branchcopilot/discuss-doctor-command-necessity.The following is the prior conversation context from the user's chat exploration (may be truncated):
User: The README now directs users to
apply --dry-run --verbosefor diagnostics, but the human summary label iscopied=and the JSON field issummary.copied, which both still increment during dry-run (planned copies) even though no files are actually copied. This is user-facing ambiguity; consider clarifying in the docs thatcopiedrepresents copies performed or planned (dry-run), or adjust the CLI/JSON wording to distinguishcopy_plannedvscopiedwhen--dry-runis set.