Add validate and plan workflows with ordered config parsing#1
Conversation
Clean now removes broken links based on their resolved target, directory creation reports chmod failures, and relink avoids removing links when Readlink fails.
Adds focused tests for platform shell selection, fallback behavior, exit-code propagation, and canceled-context handling at the shell execution boundary.
BREAKING CHANGE: --plugin, --plugin-dir, --disable-built-in-plugins, and the plugins directive are no longer accepted as compatibility features. The Go port now exposes only built-in directives.
Adds read-only validation and plan generation for supported directives, preserves config directive source order across YAML, JSON, JSON5, and TOML, and refactors link resolution around an explicit resolved-link model.
Updates README and guides for validate/plan usage, JSON plan output, and the supported YAML, JSON, JSON5, and TOML format set.
Drops the HOCON runtime dependency and example, records the supported-format decision, and keeps HOCON as an explicitly unsupported format in tests.
Builds now pass the resolved release version into dotbot-go so packaged binaries report the tag-derived version.
Add package documentation and godoc comments across the public Go surface. Refactor CLI help, app orchestration, JSON5 ordered parsing, link application, and shell directive option handling while preserving behavior. Add characterization tests for config normalization, link mutations, globbing, expansion, CLI help, and shell option precedence.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (45)
📝 WalkthroughWalkthroughThis PR removes HOCON support, adds order-preserving parsers for supported config formats, introduces validate/plan flows across config, core, app, and CLI layers, and updates CI, tooling, and documentation for the new command and release behavior. ChangesConfig Format Rework
Validate/Plan Workflow and Directive Refactor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (5)
internal/config/config_test.go (1)
106-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared order-preservation test scaffolding.
The five
TestReadPreserves*TaskDirectiveOrdertests repeat the same write-file/Read/assert-order pattern. A small table-driven helper (filename, content, expected directives) would reduce duplication without losing per-format clarity.🤖 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 `@internal/config/config_test.go` around lines 106 - 291, The order-preservation tests repeat the same setup/read/assert flow across multiple formats, so extract the shared scaffolding into a small table-driven helper to reduce duplication. Keep the format-specific cases in TestReadPreservesYAMLTaskDirectiveOrder, TestReadPreservesJSONTaskDirectiveOrder, TestReadPreservesJSON5TaskDirectiveOrder, TestReadPreservesJSON5OrderWithDelimitersInStringsAndComments, TestReadPreservesJSON5QuotedKeysWithDelimiters, TestReadPreservesTOMLTaskDirectiveOrder, and TestReadPreservesTOMLArrayTableTaskDirectiveOrder by passing filename, file contents, and expected directives into the helper.internal/app/app.go (2)
164-169: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
baseDirectoryindexesopts.ConfigFiles[0]without its own bounds check.This is safe today because
Runalready validateslen(opts.ConfigFiles) != 0before callingbaseDirectory(Line 94-97), but the helper itself has no defensive guard, so any future call site that skips that upstream check would panic with an index-out-of-range.🤖 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 `@internal/app/app.go` around lines 164 - 169, baseDirectory currently dereferences opts.ConfigFiles[0] without checking that ConfigFiles is non-empty, so make the helper defensive by validating opts.ConfigFiles before indexing it and handling the empty case safely. Update baseDirectory in app.go to either return a sensible fallback or surface an error, and keep the existing Run validation aligned with this helper so future call sites cannot panic when they bypass the upfront len(opts.ConfigFiles) check.
90-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWarning-suppression logic duplicates the
"json"format literal fromplan_output.go.Line 103's
!(opts.Plan && opts.Output == "json")guard exists purely to keep the "No tasks" warning from corrupting JSON plan output (confirmed byTestRunPlanJSONIsNotPrefixedByEmptyConfigWarning). This duplicates the format switch inplan_output.go'swritePlanOutput. If a new structured format (e.g."yaml") is added there later, this condition must be remembered and updated separately, or the new format's output will get silently corrupted by the warning.Consider exposing a small helper (e.g.
isStructuredOutput(format string) bool) fromplan_output.goand reusing it here instead of hardcoding the literal twice.Also applies to: 103-105
🤖 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 `@internal/app/app.go` around lines 90 - 93, The warning-suppression check in app.Run currently hardcodes the same output-format logic as plan_output.go, which risks drifting when new structured formats are added. Move that format decision into a shared helper in plan_output.go, such as isStructuredOutput(format string), and reuse it from the Run flow instead of checking opts.Output against the literal directly. Update the guard around the “No tasks” warning to call the shared helper so plan output handling stays consistent with writePlanOutput.internal/core/create.go (1)
19-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor duplication between
ValidateandPlan.Both methods independently re-derive the list/map shape from
dataafterPlanalready callsValidate. Consider extracting a shared parse step (e.g., a privateparseCreate(data) (paths []string, opts map[string]any, err error)) reused byValidate/Plan/Handleto avoid re-implementing the same type switch three times across handlers.🤖 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 `@internal/core/create.go` around lines 19 - 62, `CreateHandler.Validate` and `CreateHandler.Plan` are duplicating the same list/map parsing logic for `data`; extract the shape check and conversion into a shared helper (for example a private parse function used by `Validate`, `Plan`, and `Handle`) so the type-switch and string/map assertions live in one place and `Plan` just consumes the parsed result..github/workflows/ci-release.yml (1)
124-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
govulncheckto a specific version for reproducible CI.
go run golang.org/x/vuln/cmd/govulncheck@latestis unpinned, so the release pipeline's behavior can change without a corresponding PR when a new govulncheck version is published. Thejustfile'svulnchecktarget has the same pattern.♻️ Proposed fix
- - name: Run vulnerability scan - run: go run golang.org/x/vuln/cmd/govulncheck@latest ./... + - name: Run vulnerability scan + run: go run golang.org/x/vuln/cmd/govulncheck@v1.1.4 ./...🤖 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 @.github/workflows/ci-release.yml around lines 124 - 129, The vulnerability scan step is using an unpinned govulncheck version, which makes CI behavior non-reproducible. Update the workflow’s “Run vulnerability scan” command to use a fixed govulncheck version instead of `@latest`, and apply the same change in the justfile vulncheck target so both entry points stay consistent.
🤖 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 @.github/workflows/ci-release.yml:
- Around line 105-109: The Run golangci-lint step is pinned to an older
golangci-lint version that cannot parse Go 1.26 syntax while this workflow uses
Go 1.26.4. Update the version in the golangci/golangci-lint-action@v6
configuration to a release that supports Go 1.26, keeping the existing args and
action setup unchanged.
- Around line 84-93: The test and build jobs in the workflow are still checking
out with writable GitHub credentials and using Go caching on untrusted runs.
Update the actions/checkout step in those jobs to set persist-credentials to
false, and adjust the actions/setup-go step for those jobs to avoid writing
cache on PR/untrusted events (or disable cache there entirely). Keep the tag job
unchanged, since it is the only one that needs push access.
In `@docs/USER_GUIDE.md`:
- Around line 71-73: The validation description in USER_GUIDE.md is inaccurate
because validate does not report how many operations would be planned. Update
the wording around validate to say it checks that the config can be read and
directive payloads have supported shapes, while explicitly stating it does not
create directories, links, or run shell commands; use the validate command
description in the guide as the place to adjust this behavior text.
In `@internal/config/toml_parser.go`:
- Around line 131-132: The tomlunstable.String branch in the TOML parser is
returning raw bytes instead of decoding the string value, so escape sequences
and multiline/basic string handling are incorrect. Update the value handling in
the parser to route String nodes through tomlScalarValue rather than converting
node.Data directly in the toml parser logic. Keep the fix localized to the
scalar decoding path used by the TOML node switch.
In `@internal/expand/expand_test.go`:
- Around line 46-68: The Path tests are using Unix-style expected strings even
though NormSlash and ExpandEnv make the output OS-dependent. Update TestPath to
branch on runtime.GOOS or construct the expected values with filepath helpers so
the env-only and undefined-env cases match Windows backslash behavior as well as
other platforms. Use the Path test cases and the Path function as the reference
points when adjusting the assertions.
- Around line 9-33: The User test is only setting HOME, so the bare-tilde case
can still depend on host-specific Windows home resolution. Update TestUser in
expand_test.go to either set the Windows home environment variables used by
os.UserHomeDir (such as USERPROFILE) alongside HOME, or skip the test on
Windows, and keep the existing User/~/filepath expectations unchanged.
---
Nitpick comments:
In @.github/workflows/ci-release.yml:
- Around line 124-129: The vulnerability scan step is using an unpinned
govulncheck version, which makes CI behavior non-reproducible. Update the
workflow’s “Run vulnerability scan” command to use a fixed govulncheck version
instead of `@latest`, and apply the same change in the justfile vulncheck target
so both entry points stay consistent.
In `@internal/app/app.go`:
- Around line 164-169: baseDirectory currently dereferences opts.ConfigFiles[0]
without checking that ConfigFiles is non-empty, so make the helper defensive by
validating opts.ConfigFiles before indexing it and handling the empty case
safely. Update baseDirectory in app.go to either return a sensible fallback or
surface an error, and keep the existing Run validation aligned with this helper
so future call sites cannot panic when they bypass the upfront
len(opts.ConfigFiles) check.
- Around line 90-93: The warning-suppression check in app.Run currently
hardcodes the same output-format logic as plan_output.go, which risks drifting
when new structured formats are added. Move that format decision into a shared
helper in plan_output.go, such as isStructuredOutput(format string), and reuse
it from the Run flow instead of checking opts.Output against the literal
directly. Update the guard around the “No tasks” warning to call the shared
helper so plan output handling stays consistent with writePlanOutput.
In `@internal/config/config_test.go`:
- Around line 106-291: The order-preservation tests repeat the same
setup/read/assert flow across multiple formats, so extract the shared
scaffolding into a small table-driven helper to reduce duplication. Keep the
format-specific cases in TestReadPreservesYAMLTaskDirectiveOrder,
TestReadPreservesJSONTaskDirectiveOrder,
TestReadPreservesJSON5TaskDirectiveOrder,
TestReadPreservesJSON5OrderWithDelimitersInStringsAndComments,
TestReadPreservesJSON5QuotedKeysWithDelimiters,
TestReadPreservesTOMLTaskDirectiveOrder, and
TestReadPreservesTOMLArrayTableTaskDirectiveOrder by passing filename, file
contents, and expected directives into the helper.
In `@internal/core/create.go`:
- Around line 19-62: `CreateHandler.Validate` and `CreateHandler.Plan` are
duplicating the same list/map parsing logic for `data`; extract the shape check
and conversion into a shared helper (for example a private parse function used
by `Validate`, `Plan`, and `Handle`) so the type-switch and string/map
assertions live in one place and `Plan` just consumes the parsed result.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b5222c4-70bd-489b-ad4d-2623b79f42f7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (59)
.github/workflows/ci-release.yml.golangci.ymlREADME.mdcmd/dotbot-go/doc.godocs/CONFIG_FORMATS.mddocs/TROUBLESHOOTING.mddocs/USER_GUIDE.mddocs/decisions/ADR-001-supported-config-formats.mddocs/migration/ARCHITECTURE.mddocs/migration/CLI_ENGINEER.mddocs/migration/CORE_MIGRATION.mddocs/migration/DEPENDENCY_DECISIONS.mddocs/migration/MIGRATION_ANALYSIS.mddocs/migration/REVIEW_REPORT.mddocs/migration/TEST_ENGINEER.mdexamples/install.hocongo.modinternal/app/app.gointernal/app/app_test.gointernal/app/doc.gointernal/app/plan_output.gointernal/cli/cli.gointernal/cli/cli_test.gointernal/cli/doc.gointernal/config/config.gointernal/config/config_test.gointernal/config/doc.gointernal/config/json5_ordered.gointernal/config/json_parser.gointernal/config/normalize_test.gointernal/config/parsers.gointernal/config/toml_parser.gointernal/config/yaml_parser.gointernal/core/clean.gointernal/core/core_test.gointernal/core/create.gointernal/core/dispatcher.gointernal/core/doc.gointernal/core/helpers.gointernal/core/link.gointernal/core/link_apply.gointernal/core/link_glob.gointernal/core/link_glob_test.gointernal/core/link_mutation_test.gointernal/core/link_paths.gointernal/core/plan.gointernal/core/shell_handler.gointernal/core/types.gointernal/expand/doc.gointernal/expand/expand.gointernal/expand/expand_test.gointernal/fsops/doc.gointernal/fsops/fs.gointernal/log/doc.gointernal/log/log.gointernal/shell/doc.gointernal/shell/shell.gointernal/shell/shell_test.gojustfile
💤 Files with no reviewable changes (5)
- go.mod
- examples/install.hocon
- docs/migration/REVIEW_REPORT.md
- internal/config/parsers.go
- docs/migration/TEST_ENGINEER.md
oleksandr-balyshyn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/core/create.go (1)
26-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlan omits resolved mode from
Detail.
Planonly setsDirectiveandTarget; the resolvedos.FileMode(viacreateMode) isn't surfaced inOperation.Detail, even thoughOperation.Detailexists to carry "directive-specific context for display." Users running--planwon't see what mode will be applied to created directories.♻️ Suggested enhancement
func (h CreateHandler) Plan(ctx *Context, directive string, data any) ([]Operation, error) { entries, err := createEntries(data) if err != nil { return nil, err } + defaults, _ := asMap(ctx.Defaults["create"]) operations := make([]Operation, 0, len(entries)) for _, entry := range entries { - operations = append(operations, Operation{Directive: directive, Target: entry.path}) + mode := createMode(defaults, entry.options) + operations = append(operations, Operation{Directive: directive, Target: entry.path, Detail: fmt.Sprintf("mode=%o", mode)}) } return operations, nil }🤖 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 `@internal/core/create.go` around lines 26 - 36, The Plan method in CreateHandler is only populating Directive and Target, so the resolved file mode is missing from Operation.Detail. Update Plan to carry the createMode-derived os.FileMode into each Operation.Detail for directory entries, while keeping createEntries as the source of targets, so --plan shows the actual mode that will be applied.
🤖 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 `@internal/core/create.go`:
- Around line 20-23: Validate currently only calls createEntries, so invalid
mode values can slip through --validate and fail later in Handle. Update
CreateHandler.Validate to also resolve and check the mode for each parsed entry,
including when the default mode is used, or change createMode/parseMode so
invalid modes return an error during validation. Use the existing createEntries,
createMode, parseMode, and CreateHandler.Validate flow to keep the validation
path aligned with runtime handling.
---
Nitpick comments:
In `@internal/core/create.go`:
- Around line 26-36: The Plan method in CreateHandler is only populating
Directive and Target, so the resolved file mode is missing from
Operation.Detail. Update Plan to carry the createMode-derived os.FileMode into
each Operation.Detail for directory entries, while keeping createEntries as the
source of targets, so --plan shows the actual mode that will be applied.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f43e6e17-4a72-4daa-8450-ba6b262d6b74
📒 Files selected for processing (17)
.github/workflows/ci-release.yml.golangci.ymldocs/USER_GUIDE.mdinternal/app/app.gointernal/app/app_test.gointernal/app/plan_output.gointernal/cli/cli.gointernal/cli/cli_test.gointernal/config/config_test.gointernal/config/json5_ordered.gointernal/config/toml_parser.gointernal/core/create.gointernal/core/link_apply.gointernal/expand/expand_test.gointernal/log/log.gointernal/shell/shell.gojustfile
✅ Files skipped from review due to trivial changes (3)
- .golangci.yml
- docs/USER_GUIDE.md
- internal/log/log.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/config/toml_parser.go
- justfile
- internal/expand/expand_test.go
- internal/shell/shell.go
- internal/cli/cli.go
- internal/core/link_apply.go
- internal/app/plan_output.go
- internal/app/app.go
- .github/workflows/ci-release.yml
- internal/app/app_test.go
- internal/cli/cli_test.go
- internal/config/config_test.go
- internal/config/json5_ordered.go
Hard-Coded Secrets (24)
More info on how to fix Hard-Coded Secrets in General. 👉 Go to the dashboard for detailed results. 📥 Happy? Share your feedback with us. |
1 participant
Summary by CodeRabbit
validateandplancommands, includingplan --output jsonfor structured previews.link,create,clean,shell) with clearer errors and stricter validation.