feat(version): warn when project charts drift from the talm binary#216
Open
Aleksei Sviridkin (lexfrei) wants to merge 5 commits into
Open
feat(version): warn when project charts drift from the talm binary#216Aleksei Sviridkin (lexfrei) wants to merge 5 commits into
Aleksei Sviridkin (lexfrei) wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughAdds Chart.yaml normalization, embedded talm library export, and deterministic hashing; implements vendored-vs-embedded chart drift checks and preset lock pinning; wires checks into root command with a --strict-charts flag and Config.StrictCharts to escalate warnings to hard errors; updates init to write/update preset locks and adds tests/docs. ChangesChart drift detection and strict-mode enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a content-drift detection mechanism to verify that the project's vendored charts/talm/ library matches the copy embedded in the talm binary, ignoring pure version stamp changes. It also adds a strict mode (--strict-charts or strictCharts: true) to treat drift as a hard error. The review feedback highlights two important issues: a regular expression in charts/charts.go that could incorrectly match nested keys or comments in Chart.yaml leading to formatting errors, and a version prefix check in main.go that might prematurely skip the drift check on non-standard release builds.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Chart.yaml. Both are rewritten to a %s placeholder so the init flow can | ||
| // substitute the real project name/version, and so the drift check can | ||
| // compare two chart trees without a version stamp counting as content. | ||
| var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`) |
Contributor
There was a problem hiding this comment.
The chartMetaRegex regular expression matches any occurrence of name: or version: globally in the Chart.yaml file.
If the Chart.yaml contains nested fields with these keys (for example, under dependencies, maintainers, or annotations), or if these words appear in descriptions or comments, they will also be replaced with %s placeholders. When talm init subsequently formats this content using fmt.Sprintf with only two arguments (project name and version), the extra placeholders will result in formatting errors or %!s(MISSING) being written to the output.
To prevent this, anchor the regular expression to the start of the line using the multi-line flag (?m)^ so that only top-level name: and version: fields are matched.
Suggested change
| var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`) | |
| var chartMetaRegex = regexp.MustCompile(`(?m)^(name|version): \S+`) |
Comment on lines
+241
to
+244
| version, ok := strings.CutPrefix(Version, "v") | ||
| if !ok { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
The drift check is silently skipped if the binary Version does not start with a v prefix (e.g., if a package manager or custom build script injects a version like 0.30.0 instead of v0.30.0).
Instead of using strings.CutPrefix and skipping when ok is false, consider explicitly checking if Version == "dev" to identify development builds, and then trimming the v prefix if present.
Suggested change
| version, ok := strings.CutPrefix(Version, "v") | |
| if !ok { | |
| return nil | |
| } | |
| if Version == "dev" { | |
| return nil | |
| } | |
| version := strings.TrimPrefix(Version, "v") |
Expose the binary's built-in talm library chart as a normalized path->content map (TalmLibraryFiles) plus a deterministic, order- independent digest over a chart tree (HashChartFiles). Chart.yaml name/version lines are folded to %s placeholders through a shared NormalizeChartMeta helper — also adopted by PresetFiles — so a version stamp is not counted as content. This is the comparison primitive for detecting when a project's vendored charts/talm/ has drifted from the binary that renders it: equal digests mean identical library bytes, independent of the stamped version. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
CheckChartDrift compares a project's vendored charts/talm/ against the binary's built-in copy via the chart-tree digest. The vendored tree is read through an os.Root scoped to charts/talm/, confining traversal to that subtree so a symlink inside it cannot redirect a read elsewhere. The match is by content, with the Chart.yaml version stamp normalized away on both sides: a binary version bump that left the library byte- identical is not reported as drift. A project with no vendored library stays silent rather than erroring, and a charts/talm that is a file rather than a directory surfaces as an error (callers degrade it to a non-fatal warning) instead of a crash. The drift message points operators at `talm init --update --preset <preset>` — the bare form cannot resolve the preset from an init'd project's Chart.yaml and would not re-vendor. Config gains an opt-in StrictCharts field (Chart.yaml strictCharts) for turning drift from a warning into a hard error; absent keeps today's behavior. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
talm renders from the project's vendored charts/talm/, never the binary's built-in charts, so upgrading the binary leaves the library frozen at the version that last ran init — drift that was previously invisible. On every config-loading command a release build now compares the two and, by default, prints a non-fatal warning to stderr; stdout and the exit code are unchanged. A project (strictCharts: true in Chart.yaml) or an operator (--strict-charts) can promote the warning to a hard error raised before the command body runs. The build version is interpreted through releaseVersion, which accepts both release forms — goreleaser injects it without a leading "v" (0.30.0), the Makefile's `git describe --tags` keeps it (v0.30.0) — and treats only "dev"/empty as a non-release. Gating on the "v" prefix alone would disable the check on every downloaded release and stamp the dev sentinel 0.1.0 into the vendored charts at init time; releaseVersion is used in both places. The decision logic is factored into a pure evaluateChartDrift for direct test coverage of the warn/strict/silent branches. The --strict-charts usage string carries no backticks, so pflag does not misrender the bool flag as taking a value. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add a README section explaining the embed -> vendor -> render model: how init vendors charts/talm/, why a binary upgrade does not touch it, the `talm init --update --preset <preset>` re-sync workflow (the preset is required because an init'd Chart.yaml does not record it), and the content-based drift warning with its strictCharts / --strict-charts enforcement, including that strict mode also blocks read-only commands. Rework manual-test-plan B5 into a runnable drift-detection script: default warning is stderr-only with the exit code unchanged, the re-vendor step that clears it, strict mode short-circuiting before the command body, and the no-false-alarm cases (version-only difference, dev build, freshly synced project) that must stay silent. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
feat/chart-drift-detection
branch
from
21065e3 to
ad56f0e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/charts.go`:
- Around line 31-35: The regex in chartMetaRegex currently matches any
`name:`/`version:` anywhere, including indented dependency entries; update
chartMetaRegex to only match top-level keys by anchoring to the start of a line
and enabling multiline mode (e.g., use a pattern like
`(?m)^(name|version):\s+\S+`) so only top-level Chart.yaml `name` and `version`
lines are normalized; update the declaration of chartMetaRegex accordingly and
run tests that use the rewrite/drift logic to confirm dependency entries are no
longer rewritten.
In `@main.go`:
- Around line 140-145: The help text for the Bool flag created via
cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", ...) points
users to "talm init --update" which is inconsistent with the actual remediation
that requires specifying a preset; update the usage string to reference the
correct recovery command (for example "run 'talm init --update --preset
<preset>'" or otherwise instruct to supply --preset <preset>) so the guidance
aligns with the rest of the codebase; modify the third argument (the usage
string) passed to BoolVar where strictChartsFlag is declared to contain the
corrected remediation text.
🪄 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: 8b52ff39-6fbd-43e1-893d-d9903a898ef7
⛔ Files ignored due to path filters (1)
pkg/generated/presets.gois excluded by!**/generated/**
📒 Files selected for processing (9)
README.mdcharts/charts.gocharts/library_test.godocs/manual-test-plan.mdmain.gomain_test.gopkg/commands/chart_drift.gopkg/commands/chart_drift_test.gopkg/commands/root.go
Comment on lines
+31
to
+35
| // chartMetaRegex matches the `name:`/`version:` metadata lines of a | ||
| // Chart.yaml. Both are rewritten to a %s placeholder so the init flow can | ||
| // substitute the real project name/version, and so the drift check can | ||
| // compare two chart trees without a version stamp counting as content. | ||
| var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`) |
There was a problem hiding this comment.
Normalize only top-level name/version keys in Chart.yaml.
Line 35 uses an unanchored pattern, so Line 47 can also rewrite dependency entries like dependencies[].name / dependencies[].version. That can suppress real drift signals by normalizing meaningful changes away.
Suggested fix
-var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`)
+var chartMetaRegex = regexp.MustCompile(`(?m)^(name|version):\s+\S+\s*$`)Also applies to: 47-47
🤖 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 `@charts/charts.go` around lines 31 - 35, The regex in chartMetaRegex currently
matches any `name:`/`version:` anywhere, including indented dependency entries;
update chartMetaRegex to only match top-level keys by anchoring to the start of
a line and enabling multiline mode (e.g., use a pattern like
`(?m)^(name|version):\s+\S+`) so only top-level Chart.yaml `name` and `version`
lines are normalized; update the declaration of chartMetaRegex accordingly and
run tests that use the rewrite/drift logic to confirm dependency entries are no
longer rewritten.
Comment on lines
+140
to
145
| // No backticks in this usage string: pflag's UnquoteUsage treats the | ||
| // first backtick-quoted word as the flag's value-placeholder name, which | ||
| // on a bool flag misrenders --help as `--strict-charts talm init --update` | ||
| // (as if it took an argument). | ||
| cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update to re-sync)") | ||
|
|
There was a problem hiding this comment.
Align --strict-charts help remediation with actual required command.
Line 144 points users to talm init --update, but drift remediation elsewhere requires --preset <preset>. This inconsistency can send operators to a non-working recovery path.
Suggested fix
-cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update to re-sync)")
+cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update --preset <preset> to re-sync)")📝 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.
Suggested change
| // No backticks in this usage string: pflag's UnquoteUsage treats the | |
| // first backtick-quoted word as the flag's value-placeholder name, which | |
| // on a bool flag misrenders --help as `--strict-charts talm init --update` | |
| // (as if it took an argument). | |
| cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update to re-sync)") | |
| // No backticks in this usage string: pflag's UnquoteUsage treats the | |
| // first backtick-quoted word as the flag's value-placeholder name, which | |
| // on a bool flag misrenders --help as `--strict-charts talm init --update` | |
| // (as if it took an argument). | |
| cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update --preset <preset> to re-sync)") |
🤖 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 `@main.go` around lines 140 - 145, The help text for the Bool flag created via
cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", ...) points
users to "talm init --update" which is inconsistent with the actual remediation
that requires specifying a preset; update the usage string to reference the
correct recovery command (for example "run 'talm init --update --preset
<preset>'" or otherwise instruct to supply --preset <preset>) so the guidance
aligns with the rest of the codebase; modify the third argument (the usage
string) passed to BoolVar where strictChartsFlag is declared to contain the
corrected remediation text.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@charts/charts.go`:
- Around line 35-47: The current NormalizeChartMeta and chartMetaRegex replace
any occurrences of "name:"/ "version:" including indented/nested ones; update
chartMetaRegex to only match top-level fields by using a multiline-anchored
pattern (e.g. use ^ in multiline mode) so it matches only lines that start with
name/version with no indentation, then keep NormalizeChartMeta (function) using
that regex to ReplaceAllString with "$1: %s"; reference chartMetaRegex,
NormalizeChartMeta, and chartYamlName when making the change.
In `@docs/manual-test-plan.md`:
- Around line 239-241: The drift-clear step hardcodes "--preset generic" in the
"talm init --update --preset generic --force" and should use the same preset as
the baseline project created earlier; change the hardcoded preset to the
consistent preset used in setup (e.g., replace "--preset generic" with "--preset
cozystack" or a reusable variable like "--preset ${PRESET}") and ensure the same
preset value is used in the "talm template -f nodes/node0.yaml --offline" flow
so the "drift cleared" assertion is valid.
In `@main.go`:
- Around line 140-145: Update the --strict-charts flag help text set in
cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", ...) so it
references the correct remediation (--preset <preset>) instead of suggesting
"talm init --update"; modify the message string to mention using the --preset
<preset> option to re-sync/remediate vendored charts/talm to match the binary,
keeping the rest of the explanatory text intact for context.
🪄 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: bc549dde-ff97-422c-8522-52bfc581ee2f
⛔ Files ignored due to path filters (1)
pkg/generated/presets.gois excluded by!**/generated/**
📒 Files selected for processing (9)
README.mdcharts/charts.gocharts/library_test.godocs/manual-test-plan.mdmain.gomain_test.gopkg/commands/chart_drift.gopkg/commands/chart_drift_test.gopkg/commands/root.go
Comment on lines
+35
to
+47
| var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`) | ||
|
|
||
| // NormalizeChartMeta rewrites the name/version lines of a Chart.yaml to %s | ||
| // placeholders. Files other than Chart.yaml pass through unchanged. base is | ||
| // the file's base name (e.g. path.Base(p)). Keeping a single normalizer | ||
| // means the init-time substitution and the content-drift comparison treat | ||
| // chart metadata identically. | ||
| func NormalizeChartMeta(base, content string) string { | ||
| if base != chartYamlName { | ||
| return content | ||
| } | ||
|
|
||
| return chartMetaRegex.ReplaceAllString(content, "$1: %s") |
There was a problem hiding this comment.
Normalize only top-level Chart.yaml name/version fields.
The current regex also matches nested name: / version: entries (for example inside dependencies:), which can hide real drift in chart content.
Suggested fix
-var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`)
+var chartMetaRegex = regexp.MustCompile(`(?m)^(name|version):\s+.*$`)📝 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.
Suggested change
| var chartMetaRegex = regexp.MustCompile(`(name|version): \S+`) | |
| // NormalizeChartMeta rewrites the name/version lines of a Chart.yaml to %s | |
| // placeholders. Files other than Chart.yaml pass through unchanged. base is | |
| // the file's base name (e.g. path.Base(p)). Keeping a single normalizer | |
| // means the init-time substitution and the content-drift comparison treat | |
| // chart metadata identically. | |
| func NormalizeChartMeta(base, content string) string { | |
| if base != chartYamlName { | |
| return content | |
| } | |
| return chartMetaRegex.ReplaceAllString(content, "$1: %s") | |
| var chartMetaRegex = regexp.MustCompile(`(?m)^(name|version):\s+.*$`) | |
| // NormalizeChartMeta rewrites the name/version lines of a Chart.yaml to %s | |
| // placeholders. Files other than Chart.yaml pass through unchanged. base is | |
| // the file's base name (e.g. path.Base(p)). Keeping a single normalizer | |
| // means the init-time substitution and the content-drift comparison treat | |
| // chart metadata identically. | |
| func NormalizeChartMeta(base, content string) string { | |
| if base != chartYamlName { | |
| return content | |
| } | |
| return chartMetaRegex.ReplaceAllString(content, "$1: %s") | |
| } |
🤖 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 `@charts/charts.go` around lines 35 - 47, The current NormalizeChartMeta and
chartMetaRegex replace any occurrences of "name:"/ "version:" including
indented/nested ones; update chartMetaRegex to only match top-level fields by
using a multiline-anchored pattern (e.g. use ^ in multiline mode) so it matches
only lines that start with name/version with no indentation, then keep
NormalizeChartMeta (function) using that regex to ReplaceAllString with "$1:
%s"; reference chartMetaRegex, NormalizeChartMeta, and chartYamlName when making
the change.
Comment on lines
+239
to
+241
| talm init --update --preset generic --force | ||
| talm template -f nodes/node0.yaml --offline 2>&1 >/dev/null | grep '^WARN:' && echo "FAIL: drift not cleared" || echo "OK: drift cleared" | ||
| ``` |
There was a problem hiding this comment.
Use the same preset as project bootstrap in the drift-clear step.
Line 239 hardcodes --preset generic, but the plan’s baseline project is created with --preset cozystack (Line 39). This can invalidate the “drift cleared” assertion for the documented flow. Replace it with the actual preset used in setup (or a placeholder variable reused consistently).
Suggested doc fix
-talm init --update --preset generic --force
+talm init --update --preset cozystack --force(or use --preset <your-preset> consistently throughout B5)
🤖 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 `@docs/manual-test-plan.md` around lines 239 - 241, The drift-clear step
hardcodes "--preset generic" in the "talm init --update --preset generic
--force" and should use the same preset as the baseline project created earlier;
change the hardcoded preset to the consistent preset used in setup (e.g.,
replace "--preset generic" with "--preset cozystack" or a reusable variable like
"--preset ${PRESET}") and ensure the same preset value is used in the "talm
template -f nodes/node0.yaml --offline" flow so the "drift cleared" assertion is
valid.
Comment on lines
+140
to
145
| // No backticks in this usage string: pflag's UnquoteUsage treats the | ||
| // first backtick-quoted word as the flag's value-placeholder name, which | ||
| // on a bool flag misrenders --help as `--strict-charts talm init --update` | ||
| // (as if it took an argument). | ||
| cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update to re-sync)") | ||
|
|
There was a problem hiding this comment.
Use the same actionable remediation in --strict-charts help text.
The flag help currently points to talm init --update, but drift remediation elsewhere requires --preset <preset>. Keeping these consistent avoids sending users to a non-remediating command path.
Suggested fix
-cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update to re-sync)")
+cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update --preset <preset> to re-sync)")📝 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.
Suggested change
| // No backticks in this usage string: pflag's UnquoteUsage treats the | |
| // first backtick-quoted word as the flag's value-placeholder name, which | |
| // on a bool flag misrenders --help as `--strict-charts talm init --update` | |
| // (as if it took an argument). | |
| cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update to re-sync)") | |
| // No backticks in this usage string: pflag's UnquoteUsage treats the | |
| // first backtick-quoted word as the flag's value-placeholder name, which | |
| // on a bool flag misrenders --help as `--strict-charts talm init --update` | |
| // (as if it took an argument). | |
| cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", false, "fail if the project's vendored charts/talm/ differs from the talm binary's built-in copy (run talm init --update --preset <preset> to re-sync)") |
🤖 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 `@main.go` around lines 140 - 145, Update the --strict-charts flag help text
set in cmd.PersistentFlags().BoolVar(&strictChartsFlag, "strict-charts", ...) so
it references the correct remediation (--preset <preset>) instead of suggesting
"talm init --update"; modify the message string to mention using the --preset
<preset> option to re-sync/remediate vendored charts/talm to match the binary,
keeping the rest of the explanatory text intact for context.
The vendored library check (charts/talm/) only covers code the operator never edits. The preset templates (templates/) ARE operator-editable, so content-comparing them would warn on every legitimate customization. Pin the pristine preset hash into .talm-preset.lock at init / init --update time and compare the binary's current preset hash against that baseline — never against the edited templates/ — so operator edits are never reported as drift. A newer binary shipping changed preset defaults no longer matches the baseline and warns (or hard-errors under --strict-charts), pointing at `talm init --update --preset <preset>`. init --update advances the baseline, clearing the warning even when individual file diffs are declined. Projects with no lock (generated before pinning) stay silent. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/commands/init.go (1)
1335-1346:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate inferred preset before any
init --updatewrites.When
presetNameis inferred from Chart.yaml (Line 1335) and not embedded in the current binary,updateTalmLibraryChartcan still mutatecharts/talmand only fail later at Line 1432 inWritePresetLock(unknown preset). That returns an error after partial writes.Fail fast by validating inferred
presetNameagainstgenerated.AvailablePresets()immediately afterreadChartYamlPreset()and before Step 1 writes.Suggested fix
@@ } else { // Try to read from Chart.yaml var err error presetName, err = readChartYamlPreset() if err != nil { @@ return err } + + availablePresets, err := generated.AvailablePresets() + if err != nil { + return errors.Wrap(err, "failed to get available presets") + } + if !isValidPreset(presetName, availablePresets) { + return errors.WithHint( + errors.Newf("preset %q from Chart.yaml is not embedded in this talm binary. Valid presets are: %v", presetName, availablePresets), + "pass --preset with one of the embedded presets, or use a talm version that ships your project preset", + ) + } }Also applies to: 1428-1434
🤖 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 `@pkg/commands/init.go` around lines 1335 - 1346, The code currently infers presetName via readChartYamlPreset() but does not validate it before performing writes in updateTalmLibraryChart, which can produce partial changes and later fail in WritePresetLock; after readChartYamlPreset() returns (where presetName is set), immediately check that presetName exists in generated.AvailablePresets() and return an error if not found so init --update fails fast; apply the same validation before any code path that calls updateTalmLibraryChart or reaches WritePresetLock to avoid partial writes.
🤖 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.
Outside diff comments:
In `@pkg/commands/init.go`:
- Around line 1335-1346: The code currently infers presetName via
readChartYamlPreset() but does not validate it before performing writes in
updateTalmLibraryChart, which can produce partial changes and later fail in
WritePresetLock; after readChartYamlPreset() returns (where presetName is set),
immediately check that presetName exists in generated.AvailablePresets() and
return an error if not found so init --update fails fast; apply the same
validation before any code path that calls updateTalmLibraryChart or reaches
WritePresetLock to avoid partial writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65a2d9f2-8698-482e-b3cc-1e50a3c37067
📒 Files selected for processing (7)
README.mddocs/manual-test-plan.mdmain.gomain_test.gopkg/commands/chart_drift.gopkg/commands/chart_drift_test.gopkg/commands/init.go
✅ Files skipped from review due to trivial changes (1)
- docs/manual-test-plan.md
🚧 Files skipped from review as they are similar to previous changes (1)
- main_test.go
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.
What
A
talm initproject vendors two things from the binary, and both silently fall behind when you upgradetalm:charts/talm/— the library chart. Render commands read this local copy, never the binary's embedded charts, so a binary upgrade leaves it frozen at whatever version last raninit. You render with stale chart logic and nothing tells you.templates/— the preset (the machine-config opinions: sysctls, etcd args, the cozystack defaults). Same staleness on a binary upgrade, but with a twist: you are expected to edit these, so they cannot be content-compared without flagging every legitimate customization.Previously both were invisible — operators had to eyeball
talm versionagainst the project. This surfaces both, each with the comparison that fits what it is.How
Library (
charts/talm/) — content comparison. On every config-loading command, compare the vendored tree against the binary's built-in copy by content digest. TheChart.yamlversion stamp is normalized away on both sides, so a binary bump that left the library byte-identical is never flagged — no false positives on version-only differences.Preset (
templates/) — pinned baseline. Because the preset is operator-editable, comparing live content would warn on every customization. Insteadtalm init(andinit --update) pin the hash of the pristine preset — as shipped in the binary — into.talm-preset.lock:A release build compares the binary's current preset hash against that pinned baseline — never against your edited
templates/. So operator customizations are never drift; a newer binary shipping changed preset defaults is.init --updaterewrites the baseline, clearing the warning even when you decline individual file diffs to keep your edits. Projects with no lock (generated before pinning) stay silent.Shared contract for both:
WARN:on stderr pointing attalm init --update --preset <preset>. Rendered output (stdout) and the exit code are unchanged.strictCharts: trueinChart.yaml(committed, so a team/CI inherits it) or--strict-chartsturns either mismatch into a hard error (exit 1), raised before the command body runs.dev/source builds (embedded charts are a moving target the developer controls), projects with nothing vendored / no lock, and matching content.The remediation carries
--presetbecausetalm init --updateresolves the preset fromChart.yaml, which an init'd project does not record — pass the preset you rantalm initwith.Testing
templates/false-positive guard, the no-lock silent case, and the warn-vs-fail decision for both checks.-ldflagsbuilds: matching → silent; older-vendored library under a newer binary with identical content → silent; library content edit → warn (exit 0);initwrites.talm-preset.lock; matching preset → silent; operator edit totemplates/→ silent (no false positive); corrupted preset baseline → warn; strict via flag and viaChart.yaml→ exit 1; dev build → silent.Docs
talm init --updateworkflow, and a dedicated preset-drift section.docs/manual-test-plan.md: B5 (library drift) and B5a (preset drift, incl. the false-positive guard) as runnable scripts.Supersedes #214: same goal, but that approach compared the project's top-level
Chart.yamlversion:(the wrong file, owned by the operator) and matched on version number rather than content.Summary by CodeRabbit
New Features
--strict-chartsorChart.yamlconfiguration to escalate drift to a hard error.talm-preset.lockbaseline tracking andtalm init --update --preset <preset>workflow for re-syncing charts while preserving custom configurationsDocumentation