Redesign checksum config and add migration tool - #342
Merged
Conversation
Contributor
Reviewer's GuideRedesigns command checksum configuration to support a new Sequence diagram for the new lets self fix migration commandsequenceDiagram
actor User
participant CobraSelf as self_cmd.fix
participant Migrate as migrate
participant ChecksumMigration
User->>CobraSelf: lets self fix [--dry-run]
CobraSelf->>CobraSelf: parse flags (--dry-run, --config)
CobraSelf->>Migrate: Fix(configName, LETS_CONFIG_DIR, dryRun, out)
Migrate->>Migrate: FindConfig(configName, configDir)
Migrate->>Migrate: collectConfigPaths(rootPath, workDir)
loop each config path
Migrate->>ChecksumMigration: Apply(root *yaml.Node)
alt checksum or persist_checksum present
ChecksumMigration->>ChecksumMigration: migrateCommandChecksum(command)
ChecksumMigration-->>Migrate: changed = true
else no migration needed
ChecksumMigration-->>Migrate: changed = false
end
end
alt dry-run
Migrate-->>CobraSelf: Result with Previews
else write files
Migrate-->>CobraSelf: Result with ChangedFiles, Applied
end
CobraSelf-->>User: migrated config output / status messages
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
kindermax
force-pushed
the
codex/redesign-checksums-and-migration
branch
from
July 29, 2026 06:58
41a40d1 to
1d376fd
Compare
kindermax
marked this pull request as ready for review
July 29, 2026 07:17
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 5 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- In
Executor.initCmd, you computechecksumWorkDirbased on command-specific overrides but still callcmd.ChecksumCalculatorwithe.cfg.WorkDir; this should passchecksumWorkDirso checksum scripts and file scans respect per-commandwork_dir. - The migration code in
fixFilewrites updated configs with a hard-coded0o644mode; consider preserving the original file permissions (viaos.Stat/FileMode) to avoid unintentionally changing executable or read-only bits.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Executor.initCmd`, you compute `checksumWorkDir` based on command-specific overrides but still call `cmd.ChecksumCalculator` with `e.cfg.WorkDir`; this should pass `checksumWorkDir` so checksum scripts and file scans respect per-command `work_dir`.
- The migration code in `fixFile` writes updated configs with a hard-coded `0o644` mode; consider preserving the original file permissions (via `os.Stat`/`FileMode`) to avoid unintentionally changing executable or read-only bits.
## Individual Comments
### Comment 1
<location path="internal/executor/executor.go" line_range="193-202" />
<code_context>
+ checksumShell = cmd.Shell
+ }
+
+ checksumWorkDir := e.cfg.WorkDir
+ if cmd.WorkDir != "" {
+ checksumWorkDir = cmd.WorkDir
+ }
+
+ checksumEnv := e.cfg.CommandBuiltinEnv(cmd, checksumShell, checksumWorkDir)
+ maps.Copy(checksumEnv, e.cfg.GetEnv())
+
// calculate checksum if needed
- if err := cmd.ChecksumCalculator(e.cfg.WorkDir); err != nil {
+ if err := cmd.ChecksumCalculator(e.cfg.WorkDir, checksumShell, checksumEnv); err != nil {
return fmt.Errorf("failed to calculate checksum for command '%s': %w", cmd.Name, err)
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Checksum calculation ignores overridden command work_dir
`checksumWorkDir` correctly reflects the command’s effective working directory, but `ChecksumCalculator` still receives `e.cfg.WorkDir`. This means checksums won’t respect per-command `work_dir` overrides and can be wrong or fail. Pass `checksumWorkDir` (and the corresponding env) into `ChecksumCalculator` instead.
</issue_to_address>
### Comment 2
<location path="internal/config/migrate/migrate.go" line_range="115-116" />
<code_context>
+
+ preview := ""
+
+ if !dryRun {
+ if err := os.WriteFile(path, updated, 0o644); err != nil {
+ return false, nil, "", fmt.Errorf("can not write config %s: %w", path, err)
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Migration rewrites config files with fixed permissions, potentially changing original modes
`os.WriteFile(path, updated, 0o644)` always forces 0644, which may differ from the existing file’s permissions and unintentionally relax or change them. Please capture the current mode with `os.Stat` and pass that `FileMode` into `os.WriteFile` so the migration preserves existing permissions.
Suggested implementation:
```golang
if !dryRun {
mode := fs.FileMode(0o644)
if info, err := os.Stat(path); err == nil {
mode = info.Mode()
} else if !os.IsNotExist(err) {
return false, nil, "", fmt.Errorf("can not stat config %s: %w", path, err)
}
if err := os.WriteFile(path, updated, mode); err != nil {
return false, nil, "", fmt.Errorf("can not write config %s: %w", path, err)
}
```
To compile successfully, ensure the file imports the `io/fs` package:
- In the import block of `internal/config/migrate/migrate.go`, add:
`import "io/fs"` (or add `fs` to an existing grouped import).
If `fs` is already imported elsewhere in this file, no further changes are needed.
</issue_to_address>
### Comment 3
<location path="internal/config/config/command_test.go" line_range="49" />
<code_context>
})
}
+
+func TestParseCommandChecksum(t *testing.T) {
+ t.Run("old list syntax", func(t *testing.T) {
+ text := dedent.Dedent(`
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for invalid checksum configurations (conflicting or incomplete settings) to exercise new validation branches
`UnmarshalYAML` now has several validation paths that `TestParseCommandChecksum` doesn’t cover: (1) both `checksum.files` and `checksum.sh` set; (2) `persist_checksum` set without `checksum`; (3) `persist_checksum` / `checksum.persist` set without any `checksum.files` or `checksum.sh`; and (4) conflicting `persist_checksum` vs `checksum.persist`. Please add table-driven tests asserting `yaml.Unmarshal` fails with the expected error for each case to cover these branches and prevent regressions.
Suggested implementation:
```golang
"testing"
"github.com/lets-cli/lets/internal/checksum"
"github.com/lithammer/dedent"
"gopkg.in/yaml.v3"
"strings"
)
```
```golang
func TestParseCommandChecksum(t *testing.T) {
t.Run("old list syntax", func(t *testing.T) {
text := dedent.Dedent(`
checksum:
- foo.txt
persist_checksum: true
cmd: echo ok
`)
command := CommandFixture(t, text)
if !command.PersistChecksum {
t.Fatal("expected persisted checksum")
}
})
t.Run("invalid configurations", func(t *testing.T) {
tests := []struct {
name string
yamlText string
wantErrSubstring string
}{
{
name: "both checksum.files and checksum.sh set",
yamlText: dedent.Dedent(`
cmd: echo ok
checksum:
files:
- foo.txt
sh: echo "checksum"
`),
// Adjust this substring to whatever the UnmarshalYAML implementation returns
wantErrSubstring: "checksum.files and checksum.sh",
},
{
name: "persist_checksum set without checksum",
yamlText: dedent.Dedent(`
cmd: echo ok
persist_checksum: true
`),
// Adjust this substring to match actual error wording
wantErrSubstring: "persist_checksum requires checksum",
},
{
name: "checksum.persist set without files or sh",
yamlText: dedent.Dedent(`
cmd: echo ok
checksum:
persist: true
`),
// Adjust this substring to match actual error wording
wantErrSubstring: "checksum.persist requires",
},
{
name: "conflicting persist_checksum and checksum.persist",
yamlText: dedent.Dedent(`
cmd: echo ok
persist_checksum: true
checksum:
files:
- foo.txt
persist: false
`),
// Adjust this substring to match actual error wording
wantErrSubstring: "conflict" ,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var cmd Command
err := yaml.Unmarshal([]byte(tt.yamlText), &cmd)
if err == nil {
t.Fatalf("expected error for invalid checksum configuration, got nil")
}
if tt.wantErrSubstring != "" && !strings.Contains(err.Error(), tt.wantErrSubstring) {
t.Fatalf("expected error to contain %q, got %q", tt.wantErrSubstring, err.Error())
}
})
}
})
}
```
1. Ensure the type name `Command` in the new test matches the actual type that implements `UnmarshalYAML` for commands. If your type is named differently (e.g. `command`, `CommandConfig`), update the declaration `var cmd Command` accordingly.
2. The `wantErrSubstring` values are placeholders based on typical error messages; align each substring with the exact error strings returned by your `UnmarshalYAML` implementation so the assertions are stable.
3. If `UnmarshalYAML` is implemented on a wrapper struct (e.g. a higher-level config object containing commands), you may need to unmarshal into that type instead and adjust the YAML snippets to match the surrounding structure.
</issue_to_address>
### Comment 4
<location path="docs/docs/config.md" line_range="894" />
<code_context>
This feature is useful when you want to know that something has changed between two executions of a command.
-`persist_checksum` can be used only if `checksum` declared for command.
+`checksum.persist` can be used only if `checksum.files` or `checksum.sh` declared for command.
If set to `true`, each run all calculated checksums will be stored to disk.
</code_context>
<issue_to_address>
**suggestion (typo):** Grammar tweak: add auxiliary verb "is" before "declared".
You could say: "`checksum.persist` can be used only if `checksum.files` or `checksum.sh` is declared for the command" (or "are declared" if you treat them jointly) to improve grammatical clarity.
```suggestion
`checksum.persist` can be used only if `checksum.files` or `checksum.sh` is declared for the command.
```
</issue_to_address>
### Comment 5
<location path="docs/docs/advanced_usage.md" line_range="100" />
<code_context>
We then can store this checksum somewhere in the file and check that stored checksum with a checksum from env.
-Fortunately, `lets` have an option for that - `persist_checksum`.
+Fortunately, `lets` have an option for that - `checksum.persist`.
-If `persist_cheksum` used with `checksum` `lets` will store new checksum to `.lets` dir and each time you run a command `lets` will check if stored checksum changed from the one from env.
</code_context>
<issue_to_address>
**issue (typo):** Subject–verb agreement: "lets" should "have" vs "has".
Rewrite this sentence as: “Fortunately, `lets` has an option for that - `checksum.persist`.”
```suggestion
Fortunately, `lets` has an option for that - `checksum.persist`.
```
</issue_to_address>
### Comment 6
<location path="internal/checksum/checksum.go" line_range="172" />
<code_context>
cmd := exec.Command(shell, "-c", script)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }) | ||
| } | ||
|
|
||
| func TestParseCommandChecksum(t *testing.T) { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for invalid checksum configurations (conflicting or incomplete settings) to exercise new validation branches
UnmarshalYAML now has several validation paths that TestParseCommandChecksum doesn’t cover: (1) both checksum.files and checksum.sh set; (2) persist_checksum set without checksum; (3) persist_checksum / checksum.persist set without any checksum.files or checksum.sh; and (4) conflicting persist_checksum vs checksum.persist. Please add table-driven tests asserting yaml.Unmarshal fails with the expected error for each case to cover these branches and prevent regressions.
Suggested implementation:
"testing"
"github.com/lets-cli/lets/internal/checksum"
"github.com/lithammer/dedent"
"gopkg.in/yaml.v3"
"strings"
)func TestParseCommandChecksum(t *testing.T) {
t.Run("old list syntax", func(t *testing.T) {
text := dedent.Dedent(`
checksum:
- foo.txt
persist_checksum: true
cmd: echo ok
`)
command := CommandFixture(t, text)
if !command.PersistChecksum {
t.Fatal("expected persisted checksum")
}
})
t.Run("invalid configurations", func(t *testing.T) {
tests := []struct {
name string
yamlText string
wantErrSubstring string
}{
{
name: "both checksum.files and checksum.sh set",
yamlText: dedent.Dedent(`
cmd: echo ok
checksum:
files:
- foo.txt
sh: echo "checksum"
`),
// Adjust this substring to whatever the UnmarshalYAML implementation returns
wantErrSubstring: "checksum.files and checksum.sh",
},
{
name: "persist_checksum set without checksum",
yamlText: dedent.Dedent(`
cmd: echo ok
persist_checksum: true
`),
// Adjust this substring to match actual error wording
wantErrSubstring: "persist_checksum requires checksum",
},
{
name: "checksum.persist set without files or sh",
yamlText: dedent.Dedent(`
cmd: echo ok
checksum:
persist: true
`),
// Adjust this substring to match actual error wording
wantErrSubstring: "checksum.persist requires",
},
{
name: "conflicting persist_checksum and checksum.persist",
yamlText: dedent.Dedent(`
cmd: echo ok
persist_checksum: true
checksum:
files:
- foo.txt
persist: false
`),
// Adjust this substring to match actual error wording
wantErrSubstring: "conflict" ,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var cmd Command
err := yaml.Unmarshal([]byte(tt.yamlText), &cmd)
if err == nil {
t.Fatalf("expected error for invalid checksum configuration, got nil")
}
if tt.wantErrSubstring != "" && !strings.Contains(err.Error(), tt.wantErrSubstring) {
t.Fatalf("expected error to contain %q, got %q", tt.wantErrSubstring, err.Error())
}
})
}
})
}- Ensure the type name
Commandin the new test matches the actual type that implementsUnmarshalYAMLfor commands. If your type is named differently (e.g.command,CommandConfig), update the declarationvar cmd Commandaccordingly. - The
wantErrSubstringvalues are placeholders based on typical error messages; align each substring with the exact error strings returned by yourUnmarshalYAMLimplementation so the assertions are stable. - If
UnmarshalYAMLis implemented on a wrapper struct (e.g. a higher-level config object containing commands), you may need to unmarshal into that type instead and adjust the YAML snippets to match the surrounding structure.
CI installs the latest released lets before running project commands. Keep the repo test-bats command on checksum syntax that the released binary can parse while this branch adds the new checksum.persist syntax.
Document checksum.sh as an intentional Project config execution boundary and suppress the gosec audit at that call site. Also pass the effective command work_dir into checksum calculation while preserving Project config workdir fallback for commands without an explicit work_dir.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Preserve config file modes when applying migrations and add coverage for invalid checksum configuration validation branches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
checksum.files,checksum.sh, andchecksum.persistwhile keeping legacy checksum syntax workinglets self fixto migrate local configs and mixins, with dry-run preview outputpersist_checksumTesting
go test ./...lets lintlets test-bats command_checksum.batslets test-bats command_persist_checksum.batslets test-bats self_fix.batsSummary by Sourcery
Redesign command checksum configuration to support file- and script-based checksums with a new persisted checksum schema, and introduce a self-fix migration command to upgrade existing configs while preserving backward compatibility.
New Features:
checksum.files,checksum.sh, andchecksum.persistsupport.lets self fixCLI command with optional--dry-runto automatically migrate local configs and mixins from deprecated checksum syntax.Enhancements:
persist_checksumand emit colored warnings at runtime.Documentation:
checksum.files,checksum.sh,checksum.persist) and deprecate the legacy checksum list/map andpersist_checksumsyntax.lets self fixCLI usage and its--dry-runpreview mode.Tests:
Chores: