ci(lint): adopt strict golangci-lint config + cross-platform CI gate (#153)#162
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds CI golangci-lint job and strict configuration; replaces many fmt.Errorf sites with cockroachdb/errors hints; rewrites engine/Helm rendering and patching; overhauls AGE encryption/rotation and secure atomic writes; restructures CLI command flows and root detection; updates and adds many tests and fixtures. ChangesStrict lint and broad refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request performs a large-scale refactoring focused on improving error handling, code structure, and linting compliance. Major updates include migrating to cockroachdb/errors, centralizing shared constants, and splitting complex command logic into smaller functions. The pkg/age encryption logic was also refined for better idempotency. Review feedback identifies a potential failure in COSI preflight checks due to incorrect context metadata, suggests using atomic write patterns for critical files, and points out several opportunities to further align with project standards for error wrapping and performance.
|
|
||
| read := cosiVersionReader(c) | ||
| for _, node := range targetNodes { | ||
| preflightCheckTalosVersion(client.WithNode(ctx, node), read, applyCmdFlags.talosVersion, os.Stderr) |
There was a problem hiding this comment.
In direct patch mode, the COSI preflight check is performed in a loop over targetNodes. However, the ctx used here still contains the nodes (plural) metadata key added by wrapWithNodeContext. Talos's apid director rejects COSI methods if the plural nodes key is present, regardless of its length. This will cause the preflight check to fail silently for multi-node targets. Consider ensuring the context used for the COSI call only contains the singular node key.
There was a problem hiding this comment.
Investigated, this isn't a bug. The Talos client docs make client.WithNode(ctx, node) strip the plural nodes outgoing-metadata key and replace it with the singular node key — see the docstring on cosiPreflightContext in this file (lines 485-528) and the underlying pkg/machinery/client/context.go. The COSI router only requires the singular node key, which is what gets set after the call at line 299. The fix in commit 31b9965 ("scope template-rendering ctx to plural nodes; preflight COSI to singular node") is the deliberate single-node scoping for the preflight loop.
There was a problem hiding this comment.
Already handled. The COSI preflight in direct patch mode wraps ctx in client.WithNode(ctx, node) at apply.go:324 — client.WithNode (machinery client/context.go) copies outgoing metadata, deletes the plural nodes key, and sets the singular node key, so the COSI router sees only the singular form regardless of what the caller's outer ctx carried. The reasoning is captured in cosiPreflightContext (apply.go:506-553) and pinned by TestCosiPreflightContext_StripsPluralAndAttachesSingular and TestCosiPreflightContext_RejectsMultiNodeCtx.
| // Library chart files are public (Chart.yaml, helpers, | ||
| // templates) — 0o644 is the documented Helm convention, | ||
| // not a secret leak. | ||
| if err := os.WriteFile(file, fileContent, presetFileMode); err != nil { |
There was a problem hiding this comment.
Consider using an atomic write pattern (e.g., writing to a temporary file and then renaming) to avoid file corruption if the process is interrupted or the disk becomes full during the write operation. This is especially important for configuration files that might be left in a truncated state.
There was a problem hiding this comment.
WONTFIX — the file being written here is a chart artefact (Chart.yaml / values.yaml / template body) reproducible from the embedded preset on every run, not a secret-bearing file that needs atomic-write semantics. The secret-bearing files DO use atomic write: secureperm.WriteFile for talm.key / secrets.yaml / talosconfig (temp + rename per pkg/secureperm/secureperm_unix.go) and age.RotateKeys carries an explicit phase-by-phase backup-and-restore protocol. Adopting atomic-rename for chart artefacts adds boilerplate without operator value: a half-written values.yaml is recoverable by re-running talm init --force.
There was a problem hiding this comment.
Out of scope for this PR. The atomic-write hardening you describe is a separate concern from the strict-lint adoption that #153 calls for — it's not a finding the lint pass surfaced, and applying it consistently would touch every chart artefact write site in init.go, not just this one. Tracking as a follow-up rather than expanding this PR's scope.
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to resolve kubeconfig path: %w", err) |
There was a problem hiding this comment.
This site was missed during the migration to cockroachdb/errors. To maintain consistency with the project's error handling standard, consider using errors.Wrap here.
References
- Migration to cockroachdb/errors for error wrapping. (link)
- Project standard: migrate from fmt.Errorf to cockroachdb/errors.Wrap/Wrapf.
There was a problem hiding this comment.
Migrated to errors.Wrap/errors.Wrapf in commit 1ce809a along with the rest of the kubeconfig + rotate_ca handlers (those were the last fmt.Errorf stragglers in pkg/commands).
There was a problem hiding this comment.
Already migrated. kubeconfig_handler.go:108 uses errors.Wrap(err, "failed to resolve kubeconfig path"). The migration sweep in this PR covered every site in the file; git grep "fmt.Errorf" pkg/commands/kubeconfig_handler.go returns nothing.
| fmt.Fprintf(os.Stderr, "Warning: modeline parsing failed: %v\n", err) | ||
|
|
There was a problem hiding this comment.
The error is already being returned and will be handled by the caller (and eventually printed by the cobra runner). Printing it here to os.Stderr results in double-printing of the error message.
There was a problem hiding this comment.
Fixed in commit 43bbbb8 — dropped the explicit fmt.Fprintf to stderr; cobra surfaces the wrapped errors.Wrapf(err, "parsing modeline in %s", configFile) at the command boundary. Same site flagged by coderabbit at line 257 in this PR; one fix covers both.
There was a problem hiding this comment.
Already fixed. The fmt.Fprintln(os.Stderr, ...) print that triggered the double-printing was removed in this PR; root.go now only returns the wrapped error and lets cobra surface it once. The rationale is documented at root.go:252-256 ("Don't print the error here — cobra surfaces the wrapped return through stderr at the command level. Printing here AND returning the wrap caused the same message to appear twice with a misleading 'Warning:' prefix on the first copy").
| cpNodes, wNodes, err := discoverClusterNodes() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to auto-discover nodes: %w", err) | ||
| } |
There was a problem hiding this comment.
This file contains multiple instances where fmt.Errorf is still used for error wrapping. These should be migrated to errors.Wrap or errors.Wrapf to align with the project's error handling standard.
References
- Migration to cockroachdb/errors for error wrapping. (link)
- Project standard: migrate from fmt.Errorf to cockroachdb/errors.Wrap/Wrapf.
There was a problem hiding this comment.
Migrated all remaining fmt.Errorf sites in this file to errors.Wrap/errors.Wrapf in commit 1ce809a.
There was a problem hiding this comment.
Already migrated. git grep "fmt.Errorf" pkg/commands/rotate_ca_handler.go returns nothing — every error-return site in the file now uses errors.Wrap / errors.Wrapf, with errors.WithHint at boundaries where operator guidance is useful.
| } | ||
|
|
||
| //nolint:gocritic,varnamelen // 768-byte v1.Node copy is acceptable on rare CA rotation; loop variable name preserved for documentation. | ||
| for _, node := range nodes.Items { |
There was a problem hiding this comment.
Iterating over nodes.Items by value causes a copy of the v1.Node struct on each iteration. Since v1.Node is a large struct, it is more efficient to iterate by index and use a pointer to avoid these copies.
There was a problem hiding this comment.
Annotated with //nolint:gocritic two commits earlier in this branch (rationale inline above the loop): CA rotation is a rare, single-shot operation; iterating by index would shadow the documented per-iteration variable name. The 768-byte v1.Node copy is acceptable on this path.
There was a problem hiding this comment.
Pinned as a deliberate trade-off, not migrated. The 768-byte v1.Node copy is acceptable here because rotateCAFetchNodes is invoked at most once per CA rotation against a typical control-plane node count (single digits). Switching to index-and-pointer iteration would save the copy at the cost of less idiomatic Go and a slightly more error-prone loop body. The decision is captured by //nolint:gocritic,varnamelen at rotate_ca_handler.go:266 with the rationale in the directive 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/commands/init.go (1)
120-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject mutually exclusive mode flags up front.
PreRunEstill allows--encrypt,--decrypt, and--updateto be combined, butRunEresolves them by branch order:--updatewins over both, and--encryptwins over--decrypt. A conflicting invocation can therefore perform a different operation than the user asked for.Suggested fix
// For -e, -d, and -u flags, always check that we're in a project root + modeCount := 0 + for _, enabled := range []bool{initCmdFlags.encrypt, initCmdFlags.decrypt, initCmdFlags.update} { + if enabled { + modeCount++ + } + } + if modeCount > 1 { + return errors.WithHint( + errors.New("--encrypt, --decrypt, and --update are mutually exclusive"), + "choose exactly one of --encrypt, --decrypt, or --update", + ) + } + if initCmdFlags.encrypt || initCmdFlags.decrypt || initCmdFlags.update {🤖 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 120 - 152, Detect and reject mutually exclusive flags early in the command's PreRunE: validate initCmdFlags.encrypt, initCmdFlags.decrypt, and initCmdFlags.update so that no more than one is true, and return a clear error if multiple are set. Add this check at the start of the PreRunE (before the existing project-root verification) and use the initCmdFlags.* symbols to decide exclusivity; keep RunE logic unchanged but rely on PreRunE to prevent ambiguous combinations. Ensure the error message names the conflicting flags (e.g., "flags --encrypt, --decrypt and --update are mutually exclusive; specify only one") and return it so the command exits early.pkg/engine/engine.go (1)
2005-2055:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate doc comment to match the actual contract: function emits
metadataandspeconly, notstringSpec.The doc on line 2005 claims the function "builds resource with metadata, spec and stringSpec fields", but the code only emits
metadataandspec. The contract document incontract_extractresource_test.go(lines 15–21) explicitly defines the output shape as{metadata: {...}, spec: <yaml-decoded>}with no mention ofstringSpec. All four contract tests validate only these two fields. No references tostringSpecexist anywhere in the codebase, templates, or charts. Update the doc comment to"builds resource with metadata and spec fields"to align with the actual implementation and contract.🤖 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/engine/engine.go` around lines 2005 - 2055, Update the doc comment on the extractResourceData function to accurately describe its output: replace the current text "builds resource with metadata, spec and stringSpec fields" with "builds resource with metadata and spec fields" so the comment matches the implemented contract (function extractResourceData emits only "metadata" and "spec").pkg/commands/template.go (1)
104-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the already-opened Talos client in the
-fpath.
RunEalready wrapstemplateWithFilesinWithClient*, buttemplateWithFilesignores the supplied*client.ClientandrunTemplatedispatches throughWithClient*again for every file. That nests the selected mode and rebuilds the client per input file instead of once per command.Also applies to: 140-179, 267-277
🤖 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/template.go` around lines 104 - 123, RunE is creating a client wrapper (WithClient/WithClientMaintenance/WithClientSkipVerify) around templateWithFiles, but templateWithFiles and runTemplate ignore the supplied *client.Client and re-wrap per file; change templateWithFiles and runTemplate to accept and use the provided *client.Client (or nil for offline) rather than invoking WithClient* internally so the chosen client mode is created once per command. Concretely: update templateWithFiles to accept the client parameter supplied by templateFunc(args) and pass that client into runTemplate for each file, and remove the internal WithClient/WithClientMaintenance/WithClientSkipVerify calls inside runTemplate so it reuses the supplied client (ensure offline path still passes nil).
🧹 Nitpick comments (7)
pkg/commands/root_detection.go (1)
225-305: 💤 Low valueOptional:
applyFileBasedRootandapplyExplicitFilesRootoverlap significantly.Both helpers do: detect-root → if empty return (false, nil) → conflict-check against
Config.RootDirwhenRootDirExplicit→ assignConfig.RootDir. The only meaningful differences are (a)applyFileBasedRootuses thecheckRootConflicthelper whileapplyExplicitFilesRootinlines the conflict check, and (b) the hint/message wording differs slightly. The conflict-check could be unified by parameterising the hint string and reusingcheckRootConflictfrom both. Worth considering for a follow-up; not blocking.🤖 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/root_detection.go` around lines 225 - 305, Both applyFileBasedRoot and applyExplicitFilesRoot duplicate root-detection/conflict logic; refactor to reuse checkRootConflict by extending it to accept an optional hint/message parameter (or a boolean to choose hint variant) and have applyExplicitFilesRoot call ValidateAndDetectRootsForFiles then call checkRootConflict(detectedRoot, Config.RootDirExplicit, hintVariant) instead of inlining the absolute-path compare and error construction; update applyFileBasedRoot to keep calling checkRootConflict unchanged and remove the duplicated abs path logic from applyExplicitFilesRoot so both flows assign Config.RootDir only after a single shared conflict check..github/workflows/pr.yml (1)
44-47: ⚡ Quick winConsider upgrading
golangci/golangci-lint-actionto v9.
@v6is two majors behind the current@v9. While v6 will likely continue working withversion: v2.12.2, v9 is the currently maintained version with a Node.js node24 runtime (v6 uses node20). The newer version also includes better cache control options and config validation features via theverifyinput. A straightforward upgrade aligns the action with current upstream versions.♻️ Proposed bump
- name: Run golangci-lint - uses: golangci/golangci-lint-action@v6 + uses: golangci/golangci-lint-action@v9 with: version: v2.12.2 args: --timeout=5m🤖 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/pr.yml around lines 44 - 47, Update the GitHub Action invocation for the linter from golangci/golangci-lint-action@v6 to golangci/golangci-lint-action@v9; edit the `uses: golangci/golangci-lint-action@v6` entry to `uses: golangci/golangci-lint-action@v9` (keep the existing `with: version: v2.12.2` and `args: --timeout=5m` unless you want to enable newer inputs like `verify` or cache controls), ensuring the `uses` reference is the only change so the workflow runs on the maintained v9 action runtime.main.go (2)
130-140: 💤 Low valueOptional: consider exporting
chartYamlNameto remove the literal duplication.
pkg/commands/talosconfig.go(line 221) readsChart.yamlvia thechartYamlNameconstant, while this call site keeps the hardcoded literal because the constant is package-private. Exporting it (or hoisting to a shared location) would close the inconsistency surfaced by the rest of the const-hoisting pass in this PR.🤖 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 130 - 140, Replace the hardcoded "Chart.yaml" literal with a shared exported constant to avoid duplication: export the existing package-private chartYamlName (e.g., rename to ChartYamlName) or move it to a common package, then use that exported constant here when building configFile (where loadConfig is called using commands.Config.RootDir). Update all call sites (including pkg/commands/talosconfig.go which currently uses chartYamlName) to reference the exported symbol so the literal is no longer duplicated.
230-239: ⚡ Quick winPre-existing:
panicon bad timeout from Chart.yaml.Not introduced by this PR, but worth surfacing since the surrounding
loadConfigblock was migrated toerrors.Wrap: a malformedapplyOptions.timeoutinChart.yamlwill hard-panicthe binary instead of surfacing a structured operator error. A follow-up could replacepanic(err)withreturn errors.Wrap(err, "parsing applyOptions.timeout").Proposed refactor
if commands.Config.ApplyOptions.Timeout == "" { commands.Config.ApplyOptions.Timeout = constants.ConfigTryTimeout.String() } else { - var err error - - commands.Config.ApplyOptions.TimeoutDuration, err = time.ParseDuration(commands.Config.ApplyOptions.Timeout) - if err != nil { - panic(err) - } + d, err := time.ParseDuration(commands.Config.ApplyOptions.Timeout) + if err != nil { + return errors.Wrapf(err, "parsing applyOptions.timeout %q", commands.Config.ApplyOptions.Timeout) + } + commands.Config.ApplyOptions.TimeoutDuration = d }🤖 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 230 - 239, The code currently panics on a malformed timeout string when setting commands.Config.ApplyOptions.TimeoutDuration; instead return a wrapped error so callers can handle it. Replace the panic(err) in the block that parses commands.Config.ApplyOptions.Timeout with returning errors.Wrap(err, "parsing applyOptions.timeout") (or fmt.Errorf with context if errors.Wrap isn't available), and ensure the enclosing function (e.g., loadConfig) propagates that error to the caller rather than allowing a hard panic; reference commands.Config.ApplyOptions.Timeout and TimeoutDuration when making the change.pkg/commands/talosctl_wrapper.go (1)
165-185: 💤 Low valueOptional: hoist remaining command-name literals to constants.
defaultKubeconfigNameandtalosconfigNameare now constantized, but"upgrade"(line 170) and"rotate-ca"(line 175) remain as string literals while being referenced by name in their wrapper helpers (wrapUpgradeCommand,wrapRotateCACommand). For consistency with the surrounding hoisting pass, consider naming them too.🤖 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/talosctl_wrapper.go` around lines 165 - 185, Create named constants for the remaining command-name literals ("upgrade" and "rotate-ca") and use them instead of raw strings where baseCmdName is compared and when calling wrapUpgradeCommand and wrapRotateCACommand; update code around baseCmdName checks and the calls to wrapUpgradeCommand and wrapRotateCACommand to reference the new constants (e.g., const upgradeCmdName = "upgrade", rotateCACmdName = "rotate-ca") so the symbols wrapUpgradeCommand, wrapRotateCACommand, and baseCmdName use the hoisted names consistently with defaultKubeconfigName/talosconfigName.pkg/age/contract_test.go (1)
697-708: ⚡ Quick winAssert the exported sentinel here as well.
This contract only checks message text. Since
RotateKeysnow exposesErrLeftoverRotationBackup, please also pinerrors.Is(err, age.ErrLeftoverRotationBackup)so a regression in wrapping semantics does not slip through while the string still matches.🤖 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/age/contract_test.go` around lines 697 - 708, After the existing string assertions add an assertion that the returned error wraps the exported sentinel by using errors.Is(err, age.ErrLeftoverRotationBackup) so wrapping semantics are pinned; specifically, after the RotateKeys(dir) error checks (the block that verifies the message contains "rotation-backup", "interrupted", and "cleanup"), call errors.Is(err, age.ErrLeftoverRotationBackup) and fail the test if it returns false to ensure the error value is the exported sentinel (import the standard errors package if not present).pkg/engine/engine.go (1)
1980-1996: 💤 Low value
enc.Close()is silently discarded here while every other YAML encoder in this file checks it.The same pipeline pattern at lines 668‑674 and 877‑883 wraps
enc.Close()errors with hints; here it's_ = enc.Close()immediately followed by appending extra docs to the buffer. Writing tobytes.Bufferdoes not fail in practice, but the inconsistency makes the next reader wonder whether the omission was deliberate, and a future change to a non-buffer writer would lose the error.♻️ Optional: align with the rest of the file
- err = enc.Encode(&targetNode) - if err != nil { - return nil, errors.Wrap(err, "encoding target config") - } - - _ = enc.Close() + err = enc.Encode(&targetNode) + if err != nil { + return nil, errors.Wrap(err, "encoding target config") + } + + if err := enc.Close(); err != nil { + return nil, errors.Wrap(err, "closing target config encoder") + }🤖 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/engine/engine.go` around lines 1980 - 1996, The silent discard of enc.Close() should be replaced with an error check consistent with other YAML encoder usages: after encoding targetNode, call enc.Close() and if it returns an error wrap it (using errors.Wrap) with the same contextual hint used elsewhere (e.g., "closing yaml encoder while encoding target config") and return the wrapped error instead of ignoring it; locate the encoder and buffer around variables enc, targetNode, extraDocs and replace the `_ = enc.Close()` with the guarded error handling to match the pattern at the other encoder sites.
🤖 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 `@pkg/age/age.go`:
- Around line 545-551: refuseIfLeftoverBackups currently treats any os.Stat
error as "no leftover" which ignores permission/IO issues; update
refuseIfLeftoverBackups to check statErr and if statErr is nil return the
wrapped ErrLeftoverRotationBackup as before, but if statErr != nil then if
os.IsNotExist(statErr) continue the loop, otherwise return errors.Wrapf(statErr,
"failed checking leftover rotation-backup at %q", p) so unexpected Stat errors
fail fast; keep the original ErrLeftoverRotationBackup wrapping only for the
case where the file exists.
In `@pkg/commands/root.go`:
- Around line 251-257: In processModelineAndUpdateGlobals, remove the
fmt.Fprintf(os.Stderr, ...) that prints "Warning: modeline parsing failed" so
the function does not double-report the error; simply return the wrapped error
from modeline.ReadAndParseModeline as currently done, or if you intend parse
failures to be recoverable, handle specific error cases from
modeline.ReadAndParseModeline and return (someTemplates, nil) with a single
logged warning instead—update the branch around the
modeline.ReadAndParseModeline call accordingly.
In `@pkg/commands/template.go`:
- Around line 97-100: The PreRunE flow is overwriting the fallback
defaultLocalEndpoint when modelineConfig.Endpoints is empty; modify the
assignments that set GlobalArgs.Endpoints from modelineConfig.Endpoints
(including the occurrences around GlobalArgs.Endpoints in PreRunE and the
similar blocks at the other noted locations) so they only replace
GlobalArgs.Endpoints when modelineConfig.Endpoints is non-empty; leave
GlobalArgs.Endpoints unchanged (preserving defaultLocalEndpoint) if the
modeline-provided slice has length 0; apply the same conditional check to all
places that assign GlobalArgs.Endpoints from modelineConfig.Endpoints and to
similar WithClient* preparatory code paths.
- Around line 400-410: findOutsideRootFallback currently may return "<basename>"
while resolveEngineTemplatePaths only recognizes "templates/<basename>",
breaking modeline round-trips; update findOutsideRootFallback to prefer and
return "templates/<basename>" when that file exists (check
filepath.Join(absRootDir, "templates", templateName) first) instead of returning
the root-level basename, and apply the same fix to the other occurrence around
lines 536-547 so both findOutsideRootFallback and resolveEngineTemplatePaths
remain symmetric.
In `@pkg/engine/helm/files.go`:
- Around line 164-166: The code converts f[name] to s and then indexes
s[len(s)-1] which will panic on empty file content; update the logic in
pkg/engine/helm/files.go (around the s := string(f[name]) usage) to first check
len(s) > 0 before accessing s[len(s)-1], and handle the empty-string case
appropriately (e.g., leave s as-is or skip trimming) so the function that
manipulates s does not panic on empty files.
---
Outside diff comments:
In `@pkg/commands/init.go`:
- Around line 120-152: Detect and reject mutually exclusive flags early in the
command's PreRunE: validate initCmdFlags.encrypt, initCmdFlags.decrypt, and
initCmdFlags.update so that no more than one is true, and return a clear error
if multiple are set. Add this check at the start of the PreRunE (before the
existing project-root verification) and use the initCmdFlags.* symbols to decide
exclusivity; keep RunE logic unchanged but rely on PreRunE to prevent ambiguous
combinations. Ensure the error message names the conflicting flags (e.g., "flags
--encrypt, --decrypt and --update are mutually exclusive; specify only one") and
return it so the command exits early.
In `@pkg/commands/template.go`:
- Around line 104-123: RunE is creating a client wrapper
(WithClient/WithClientMaintenance/WithClientSkipVerify) around
templateWithFiles, but templateWithFiles and runTemplate ignore the supplied
*client.Client and re-wrap per file; change templateWithFiles and runTemplate to
accept and use the provided *client.Client (or nil for offline) rather than
invoking WithClient* internally so the chosen client mode is created once per
command. Concretely: update templateWithFiles to accept the client parameter
supplied by templateFunc(args) and pass that client into runTemplate for each
file, and remove the internal
WithClient/WithClientMaintenance/WithClientSkipVerify calls inside runTemplate
so it reuses the supplied client (ensure offline path still passes nil).
In `@pkg/engine/engine.go`:
- Around line 2005-2055: Update the doc comment on the extractResourceData
function to accurately describe its output: replace the current text "builds
resource with metadata, spec and stringSpec fields" with "builds resource with
metadata and spec fields" so the comment matches the implemented contract
(function extractResourceData emits only "metadata" and "spec").
---
Nitpick comments:
In @.github/workflows/pr.yml:
- Around line 44-47: Update the GitHub Action invocation for the linter from
golangci/golangci-lint-action@v6 to golangci/golangci-lint-action@v9; edit the
`uses: golangci/golangci-lint-action@v6` entry to `uses:
golangci/golangci-lint-action@v9` (keep the existing `with: version: v2.12.2`
and `args: --timeout=5m` unless you want to enable newer inputs like `verify` or
cache controls), ensuring the `uses` reference is the only change so the
workflow runs on the maintained v9 action runtime.
In `@main.go`:
- Around line 130-140: Replace the hardcoded "Chart.yaml" literal with a shared
exported constant to avoid duplication: export the existing package-private
chartYamlName (e.g., rename to ChartYamlName) or move it to a common package,
then use that exported constant here when building configFile (where loadConfig
is called using commands.Config.RootDir). Update all call sites (including
pkg/commands/talosconfig.go which currently uses chartYamlName) to reference the
exported symbol so the literal is no longer duplicated.
- Around line 230-239: The code currently panics on a malformed timeout string
when setting commands.Config.ApplyOptions.TimeoutDuration; instead return a
wrapped error so callers can handle it. Replace the panic(err) in the block that
parses commands.Config.ApplyOptions.Timeout with returning errors.Wrap(err,
"parsing applyOptions.timeout") (or fmt.Errorf with context if errors.Wrap isn't
available), and ensure the enclosing function (e.g., loadConfig) propagates that
error to the caller rather than allowing a hard panic; reference
commands.Config.ApplyOptions.Timeout and TimeoutDuration when making the change.
In `@pkg/age/contract_test.go`:
- Around line 697-708: After the existing string assertions add an assertion
that the returned error wraps the exported sentinel by using errors.Is(err,
age.ErrLeftoverRotationBackup) so wrapping semantics are pinned; specifically,
after the RotateKeys(dir) error checks (the block that verifies the message
contains "rotation-backup", "interrupted", and "cleanup"), call errors.Is(err,
age.ErrLeftoverRotationBackup) and fail the test if it returns false to ensure
the error value is the exported sentinel (import the standard errors package if
not present).
In `@pkg/commands/root_detection.go`:
- Around line 225-305: Both applyFileBasedRoot and applyExplicitFilesRoot
duplicate root-detection/conflict logic; refactor to reuse checkRootConflict by
extending it to accept an optional hint/message parameter (or a boolean to
choose hint variant) and have applyExplicitFilesRoot call
ValidateAndDetectRootsForFiles then call checkRootConflict(detectedRoot,
Config.RootDirExplicit, hintVariant) instead of inlining the absolute-path
compare and error construction; update applyFileBasedRoot to keep calling
checkRootConflict unchanged and remove the duplicated abs path logic from
applyExplicitFilesRoot so both flows assign Config.RootDir only after a single
shared conflict check.
In `@pkg/commands/talosctl_wrapper.go`:
- Around line 165-185: Create named constants for the remaining command-name
literals ("upgrade" and "rotate-ca") and use them instead of raw strings where
baseCmdName is compared and when calling wrapUpgradeCommand and
wrapRotateCACommand; update code around baseCmdName checks and the calls to
wrapUpgradeCommand and wrapRotateCACommand to reference the new constants (e.g.,
const upgradeCmdName = "upgrade", rotateCACmdName = "rotate-ca") so the symbols
wrapUpgradeCommand, wrapRotateCACommand, and baseCmdName use the hoisted names
consistently with defaultKubeconfigName/talosconfigName.
In `@pkg/engine/engine.go`:
- Around line 1980-1996: The silent discard of enc.Close() should be replaced
with an error check consistent with other YAML encoder usages: after encoding
targetNode, call enc.Close() and if it returns an error wrap it (using
errors.Wrap) with the same contextual hint used elsewhere (e.g., "closing yaml
encoder while encoding target config") and return the wrapped error instead of
ignoring it; locate the encoder and buffer around variables enc, targetNode,
extraDocs and replace the `_ = enc.Close()` with the guarded error handling to
match the pattern at the other encoder sites.
🪄 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: 14d28475-8333-4d68-97a4-8041eeb9736d
📒 Files selected for processing (61)
.github/workflows/pr.yml.gitignore.golangci.ymlcharts/charts.gogo.modmain.gopkg/age/age.gopkg/age/contract_internals_test.gopkg/age/contract_test.gopkg/commands/apply.gopkg/commands/apply_test.gopkg/commands/consts.gopkg/commands/consts_test.gopkg/commands/contract_chart_gitignore_test.gopkg/commands/contract_helpers_test.gopkg/commands/contract_init_ux_test.gopkg/commands/contract_init_validation_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_test.gopkg/commands/init.gopkg/commands/kubeconfig_handler.gopkg/commands/preflight.gopkg/commands/preflight_test.gopkg/commands/root.gopkg/commands/root_detection.gopkg/commands/root_detection_test.gopkg/commands/rotate_ca_handler.gopkg/commands/talosconfig.gopkg/commands/talosctl_wrapper.gopkg/commands/template.gopkg/commands/template_test.gopkg/commands/template_windows_test.gopkg/commands/upgrade_handler.gopkg/engine/contract_debugphase_test.gopkg/engine/contract_errors_test.gopkg/engine/contract_extractresource_test.gopkg/engine/contract_loadvalues_test.gopkg/engine/contract_machine_test.gopkg/engine/contract_network_legacy_test.gopkg/engine/contract_network_multidoc_test.gopkg/engine/engine.gopkg/engine/engine_test.gopkg/engine/helm/consts_test.gopkg/engine/helm/engine.gopkg/engine/helm/engine_test.gopkg/engine/helm/files.gopkg/engine/helm/files_test.gopkg/engine/helm/funcs.gopkg/engine/helm/funcs_test.gopkg/engine/render_test.gopkg/modeline/consts_test.gopkg/modeline/contract_test.gopkg/modeline/modeline.gopkg/modeline/modeline_test.gopkg/secureperm/contract_unix_test.gopkg/secureperm/secureperm_test.gopkg/secureperm/secureperm_unix.gopkg/secureperm/secureperm_unix_test.gopkg/secureperm/secureperm_windows.gopkg/yamltools/yamltools.gopkg/yamltools/yamltools_test.go
Closes #153. Adopts the `default: all` golangci-lint v2 config proposed in #153 and reconciles the project against it: zero lint findings on both linux/amd64 (host) and windows/amd64, with a CI lint matrix gate so the strict config doesn't erode silently. ## Strict-lint adoption - `.golangci.yml`: `default: all` with the relaxations from the issue (depguard, exhaustruct, gochecknoinits, wsl, lll, errchkjson, ireturn, gocheckcompilerdirectives) plus three documented additions: noinlineerr (200+ inline if-err sites; conversion would create variable-scope leaks), goconst on _test.go (test fixtures live inside backtick strings; substitution desyncs expected vs got), gomodguard (deprecated in v2.12+, neither allow nor blocklist configured). - `gomoddirectives.replace-allow-list` for github.com/siderolabs/talos and .../pkg/machinery — required while upstream declines siderolabs/talos#12652. ## CI lint gate - New `lint:` job in .github/workflows/pr.yml runs golangci-lint v2.12.2 on Ubuntu AND Windows runners (matching the existing `test:` matrix). Build-tagged Windows files (secureperm_windows.go, template_windows_test.go) are now actually evaluated on every PR instead of silently diverging. - `.gitattributes` pins LF for *.go so Windows runners don't surface spurious gofmt errors from autocrlf-converted CRLF endings. ## Bug fixes surfaced by the lint pass - pkg/commands/preflight.go: `nilerr` — two error sites swallowed errors with `return nil` after `if err != nil`. Now propagate. - pkg/commands/rotate_ca_handler.go: `nosprintfhostport` — URL constructed via fmt.Sprintf("%s:%s", host, port) broke for IPv6 endpoints. Switched to net.JoinHostPort. - pkg/age/age.go + pkg/engine/engine.go: `forcetypeassert` — type assertions without ok-form panicked on unexpected YAML shapes. Switched to ok-form with wrapped error returns. - pkg/yamltools/yamltools.go + pkg/engine/engine.go: `exhaustive` — yaml.Kind switches missed DocumentNode/AliasNode/ScalarNode/SequenceNode. Added explicit cases with documented intent. - pkg/commands/init.go: `writeGitignoreFile` reported "Updated" for files it just created (existence check ran AFTER WriteFile). Capture stat before write; pinned by TestContract_WriteGitignoreFile_CreatedVsUpdatedReporting. - pkg/age/age.go: `refuseIfLeftoverBackups` (RotateKeys Phase 0) failed open on permission/sharing/transient I/O errors — only os.IsNotExist is the "no leftover" answer; everything else now fails closed. - pkg/engine/helm/files.go: `Lines()` panicked on empty file content (s[len(s)-1] without length guard). Added early-return guard. Bug is verbatim from upstream Helm v3.20.2; we ship the code so we own the fix. - pkg/commands/template.go: modeline-omitted endpoints overwrote the PreRunE-seeded defaultLocalEndpoint; modeline path generation (root-level basename fallback) was asymmetric with resolution. Both fixed. - pkg/commands/root.go: dropped duplicate stderr print of modeline parse failures — cobra surfaces the wrapped error at the command boundary already. ## Refactors - pkg/age/age.go: EncryptSecretsFile / DecryptSecretsFile / RotateKeys decomposed into named helpers (encryptYAMLPair, decryptYAMLPair, loadOrGenerateIdentity, incrementalEncryptMap, plus rotation-phase helpers). All 19 contract tests still pass. - pkg/commands/apply.go + template.go: split monolithic per-file closures into named helpers (applyOneFileTemplateMode, applyOneFileDirectPatchMode, templateOneFile, buildTemplateRunner, etc.). - pkg/engine/engine.go: extractResourceData rewritten — was an unguarded yamlValue.(string) assertion that would panic; now returns a typed error. ## Error-handling migration ~220 sites migrated from fmt.Errorf("...: %w", err) to cockroachdb/errors.Wrap / Wrapf (project standard). Adds operator-facing hints via errors.WithHint at boundaries. ## Const hoisting ~150 repeated literals (k8s/COSI keys, Helm template names, file modes, preset/chart names, test fixtures) moved to package-level constants with documented intent — single source of truth, single rename per future change. ## Verification - go build ./... — clean (host + GOOS=windows) - go test ./... -count=1 — green (8/8 packages) - go test ./... -race — green - golangci-lint run — 0 issues (host + GOOS=windows) - Manual smoke: talm init --preset cozystack --name test-cluster creates expected files; ancestor-project refusal works; --root . opt-in works. Bulk of the diff is mechanical: const extraction, error-wrap migration, blank-line discipline, and //nolint: annotations with rationale where the rule cannot be satisfied without losing fidelity (project's "200% rule" on disabling lints). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
@coderabbitai full review |
|
/gemini review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring aimed at improving code maintainability, error reporting, and linting compliance. Key changes include the adoption of github.com/cockroachdb/errors for operator hints, the centralization of string constants, and the decomposition of complex command logic into smaller helper functions. The review identified a critical bug in the apply and template commands where the default local endpoint fallback is lost during multi-file processing. Additionally, the reviewer pointed out a discrepancy in the preflight logic where version parsing errors are suppressed despite the PR's stated intent to propagate them.
| if !applyCmdFlags.endpointsFromArgs { | ||
| GlobalArgs.Endpoints = []string{} | ||
| } |
There was a problem hiding this comment.
The fallback to defaultLocalEndpoint (127.0.0.1) established in PreRunE (lines 106-108) is lost for subsequent files during multi-file execution. When endpointsFromArgs is false, GlobalArgs.Endpoints is reset to an empty slice at the end of each iteration (line 142). Since PreRunE only runs once, any file after the first one that lacks a modeline endpoint will proceed with an empty endpoint list, which may cause client initialization errors if no endpoints are defined in talosconfig. The reset should restore the default fallback state.
| if !applyCmdFlags.endpointsFromArgs { | |
| GlobalArgs.Endpoints = []string{} | |
| } | |
| if !applyCmdFlags.endpointsFromArgs { | |
| GlobalArgs.Endpoints = []string{defaultLocalEndpoint} | |
| } |
There was a problem hiding this comment.
Re-checked against the talos client and the suggestion would regress. The fix in 349da55 keeps the empty-slice reset and adds the rationale + contract tests.
Args.WithClientNoNodes (cmd/talosctl/pkg/talos/global/client.go) registers client.WithConfig(cfg) first and then layers client.WithEndpoints(c.Endpoints...) only when len(c.Endpoints) > 0. An empty GlobalArgs.Endpoints therefore falls back to the talosconfig context's endpoints — the operator-expected behaviour when the talosconfig already names the cluster. Re-seeding [defaultLocalEndpoint] between files would unconditionally override that fallback with loopback for every file past the first whose modeline omits endpoints, silently mis-routing applies to 127.0.0.1.
The PreRunE seed at apply.go:106-108 only guards against zero-endpoint client construction on the very first file; from the second file onward, talosconfig fallback is the intended path. Pinned by TestResetGlobalArgsBetweenFiles_ClearsModelineSuppliedEndpoints, TestResetGlobalArgsBetweenFiles_PreservesCLIFlagState, and TestResetGlobalArgsBetweenFiles_DoesNotReseedLoopback.
| if !templateCmdFlags.endpointsFromArgs { | ||
| GlobalArgs.Endpoints = []string{} | ||
| } |
There was a problem hiding this comment.
Similar to the apply command, the defaultLocalEndpoint fallback is lost for subsequent files in multi-file template rendering. The reset on line 174 clears GlobalArgs.Endpoints when they are not provided via CLI, but fails to re-apply the default placeholder required for consistent client behavior across all files in the batch.
| if !templateCmdFlags.endpointsFromArgs { | |
| GlobalArgs.Endpoints = []string{} | |
| } | |
| if !templateCmdFlags.endpointsFromArgs { | |
| GlobalArgs.Endpoints = []string{defaultLocalEndpoint} | |
| } |
There was a problem hiding this comment.
Same reasoning as the apply.go thread on this PR — the suggestion would regress the talosconfig-context fallback. The empty-slice reset is the intended path because the talos client falls back to talosconfig context endpoints when GlobalArgs.Endpoints is empty (Args.WithClientNoNodes registers client.WithConfig(cfg) first and applies client.WithEndpoints only when the slice is non-empty). Re-seeding [defaultLocalEndpoint] would unconditionally override that fallback with loopback for every file past the first.
templateWithFiles now delegates the per-file reset to the consolidated resetGlobalArgsBetweenFiles helper (commit 349da55) so apply and template share one implementation, and the same contract tests cover both call sites.
| return nil //nolint:nilerr // best-effort: never block apply on parse failure | ||
| } | ||
| } | ||
|
|
||
| runningContract, err := machineryconfig.ParseContractFromVersion(runningVersion) | ||
| if err != nil { | ||
| return nil | ||
| return nil //nolint:nilerr // best-effort: never block apply on parse failure |
There was a problem hiding this comment.
There is a discrepancy between the PR description and the implementation here. The PR highlights state that these nilerr sites "Now propagate" errors, but the code continues to swallow them by returning nil (now with a //nolint:nilerr suppression). While the accompanying comment justifies this as "best-effort" to avoid blocking execution on parse failures, it contradicts the stated intent of the PR. If propagation was indeed intended to surface invalid version strings as warnings, the error should be returned instead of suppressed.
There was a problem hiding this comment.
PR description corrected. The two nilerr sites at preflight.go:145 and :151 are intentional best-effort returns: the version preflight is advisory and must not block apply on a Talos COSI version-read failure. The //nolint:nilerr directive carries the rationale ("never block apply on parse failure"); the original PR body wording incorrectly stated the errors now propagate. The "Bug fixes" section now describes the actual change — the lint exception with documented rationale, not propagation.
The per-file iteration in apply() and templateWithFiles() resets GlobalArgs.Endpoints when the operator did not supply --endpoints on the CLI. The reset must yield an EMPTY slice, not a re-seeded [defaultLocalEndpoint] placeholder. The talos client (cmd/talosctl/pkg/talos/global/client.go, WithClientNoNodes / WithClientSkipVerify) registers client.WithConfig(cfg) first and then layers client.WithEndpoints only when len(c.Endpoints) > 0. An empty GlobalArgs.Endpoints therefore falls back to the talosconfig context's endpoints — the operator-expected behaviour. Re-seeding loopback would silently override the talosconfig context for any subsequent file whose modeline omits endpoints. Consolidate the reset semantics into resetGlobalArgsBetweenFiles with the rationale documented in-file, and pin the contract with TestResetGlobalArgsBetweenFiles_* covering the modeline-supplied clear, the CLI-flag preserve, and the explicit no-loopback-re-seed regression guard. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Two sites in rotate_ca_handler.go re-implemented the trim-and-rejoin logic that normalizeEndpoint already exposes — once when seeding the upstream --k8s-endpoint flag from GlobalArgs.Endpoints, and once in updateKubeconfigEndpoint when rewriting cluster server URLs. Both inline copies omitted the bracket-stripping branch from normalizeEndpoint, so a bare bracketed IPv6 input (e.g. [2001:db8::1]) round-tripped to https://[[2001:db8::1]]:6443 — same class of bug nosprintfhostport surfaced for the talosctl wrapper. Delegate both sites to normalizeEndpoint, drop the now-unused net import, and pin the IPv6 no-port round-trip via TestContract_UpdateKubeconfigEndpoint_NormalisesAllClusters so a future inline re-implementation surfaces immediately. Signed-off-by: Aleksei Sviridkin <f@lex.la>
writeGitignoreFile branched on (statErrBefore == nil) to pick the "Updated" vs "Created" log line. That distinction collapses any non-IsNotExist stat error (EACCES on the parent directory, ENOTDIR mid-path, EIO on a flaky disk) into the "Created" bucket — and would falsely report "Created" for a file that already existed but was unreadable until WriteFile fixed permissions on the path. Branch on os.IsNotExist explicitly so ambiguous stat errors fall into the "Updated" bucket. The "Updated" wording is correct in the ambiguous case because it does not falsely promise the absence we never confirmed. Document the rationale in-file so a future "tighten this" pass does not reintroduce the stale check. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Hoist the verb-pick branch out of writeGitignoreFile into the pure gitignoreReportVerb helper and unit-test it directly. The test covers the four cases that os.Stat surfaces in practice: - nil (file existed) -> "Updated" - ErrNotExist (file absent) -> "Created" - ErrPermission (parent EACCES) -> "Updated" - *os.PathError variants of the above Without the helper extraction, exercising the EACCES branch would require fault injection on os.Stat. Keeping the production branch in-line behind a tested pure function preserves the IsNotExist contract under future refactors. Centralise the "Created"/"Updated" verbs into reportVerbCreated / reportVerbUpdated so goconst stops firing on the now-multiple- occurrence literals, and rewrite every Fprintf call in init.go to reference the constants. Signed-off-by: Aleksei Sviridkin <f@lex.la>
The hoisted initSubcommandName at main.go:26 advertises a single source of truth for the subcommand string, but the prefix check at line 190 still inlined the literal "init". Switch to the constant so a future rename surfaces both sites at once instead of leaving the literal silently behind. Signed-off-by: Aleksei Sviridkin <f@lex.la>
The PresetFiles walker was annotated with nolint:wrapcheck whose rationale claimed the inner func wrapped errors with file context. The inner func actually returned raw err on both error paths, so a downstream caller reading a *fs.PathError surfacing a single field could not tell which embedded chart file triggered it. Replace the inline raw returns with errors.Wrapf(...) calls that prepend the offending path; the outer return wraps the WalkDir result with the operation name. The nolint directives are no longer needed. Pin the success-path contract — that PresetFiles emits Chart.yaml entries with %s placeholders for name/version, and that the talm subdirectory filter under preset trees is in force — via two smoke tests that previously had no coverage at all. Signed-off-by: Aleksei Sviridkin <f@lex.la>
files.Lines() reads content[len(content)-1] unconditionally to
strip a trailing newline. When a key is present in the files map
but maps to an empty []byte value, that read used to panic with
"index out of range" — the early-return on f[name] == nil shape
caught only the unset case, not the explicitly-empty one. The
empty-content branch added in this PR returns []string{} so a
template's `{{ range .Files.Lines "empty.txt" }}` iterates zero
times instead of taking the engine down.
The fix shipped without coverage. TestLines_EmptyContent pins the
five shapes Lines() must handle: empty []byte, nil entry, missing
key, single trailing newline, content with no trailing newline.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
FullConfigProcess took a context.Context that it never observed — the function performs no I/O respecting cancellation. The "_" parameter masked an honest API: callers were passing live contexts to a function that discarded them, which is misleading review surface. Drop the parameter from FullConfigProcess and propagate to its two callers (apply.applyOneFileDirectPatchMode, upgrade_handler) plus the four FullConfigProcess contract tests. The upgrade handler also drops a now-orphaned context.Background() allocation and the unused "context" import; the apply path drops the now-unused ctx threading down to applyOneFileDirectPatchMode which had been carrying it for this single FullConfigProcess call. If a future configpatcher needs cancellation, reintroduce ctx typed as the first argument. Signed-off-by: Aleksei Sviridkin <f@lex.la>
The companion fn to PresetFiles still returned a raw fs.PathError behind a nolint:wrapcheck directive while PresetFiles wraps with errors.Wrap. Match the pattern: errors.Wrap with the operation description so an embedded-FS failure surfaces with context, not just a bare path error. Signed-off-by: Aleksei Sviridkin <f@lex.la>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/commands/template_windows_test.go (1)
43-43: 💤 Low valueUpdate the parallelism safety comment.
The comment states the test is "not safe with t.Parallel" because it "uses os.Chdir", but line 64 now uses
t.Chdirinstead. Unlikeos.Chdir,t.Chdir(introduced in Go 1.24) leverages per-goroutine working directories and is safe with parallel tests.Consider either removing this comment or updating it to reflect that the test could now be safely marked
t.Parallel()if desired.📝 Suggested comment update
-// Not safe with t.Parallel — uses os.Chdir, which is process-global. +// Safe with t.Parallel after migrating to t.Chdir (Go 1.24+), but not marked parallel to preserve test isolation guarantees.Or simply remove the comment if parallelism is not a concern.
🤖 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/template_windows_test.go` at line 43, The comment incorrectly warns that the test is "not safe with t.Parallel — uses os.Chdir" even though the test now calls t.Chdir (Go 1.24+), which is per-goroutine and safe for parallel tests; update the comment to reflect that t.Chdir is used and the test can be marked t.Parallel() if desired, or simply remove the comment entirely; locate the comment above the test function that uses t.Chdir and either delete it or change it to something like "safe with t.Parallel — uses t.Chdir (Go 1.24+)" and optionally add t.Parallel() inside the test body.pkg/commands/apply.go (1)
693-700: 💤 Low valueConsider logging the path-resolution fallback for operator visibility.
When
filepath.Abs(rootDir)orfilepath.Relfail, the function silently falls back to normalized-only paths. While the fallback is safe, an operator won't know why a modeline-referenced template resolved differently than expected (e.g., due to permission issues onrootDiror an invalid path). A debug-level log or comment surfacing the fallback reason would improve troubleshooting.💡 Optional: Surface fallback for debugging
At line 694, consider:
absRootDir, rootErr := filepath.Abs(rootDir) if rootErr != nil { + // filepath.Abs failed; falling back to normalized paths without root resolution for i, p := range templates {At line 712, consider:
relPath, relErr := filepath.Rel(absRootDir, absTemplatePath) if relErr != nil { + // filepath.Rel failed; using original template path normalized resolved[i] = engine.NormalizeTemplatePath(templatePath)Also applies to: 711-716
🤖 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/apply.go` around lines 693 - 700, The code silently falls back to normalized-only paths when filepath.Abs(rootDir) or filepath.Rel fail; update the error paths to log a debug-level message with the error and context before returning so operators see why normalization-only was used. Specifically, in the block that calls filepath.Abs(rootDir) and the block handling filepath.Rel failures, emit a debug log (using the file's existing logger variable) that includes the failing call (filepath.Abs or filepath.Rel), the rootDir or path involved, and the error, then proceed to call engine.NormalizeTemplatePath(p) as before; keep behavior unchanged except for the added debug logging.
🤖 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 `@pkg/commands/contract_chart_gitignore_test.go`:
- Around line 326-339: The helper captureStderr leaves os.Stderr pointing at a
closed pipe because it uses t.Cleanup to restore os.Stderr at test end; change
captureStderr to restore os.Stderr immediately by replacing t.Cleanup with a
local defer that sets os.Stderr = origStderr, and ensure the pipe ends are
closed inside the function (close the write end w immediately after fn()
returns, then copy from the read end r and close r) so each invocation of
captureStderr is self-contained and does not leak a closed os.Stderr into
subsequent calls.
In `@pkg/engine/engine.go`:
- Around line 1986-1995: The yaml.Encoder.Close() error is being ignored after
encoding targetNode; change the code that calls enc.Close() (the encoder
variable `enc` used to encode `targetNode` into `buf`) to capture its return
value and return a wrapped error (e.g., using errors.Wrap with context like
"closing target config encoder") instead of discarding it so that Close failures
surface like the other encoder uses in this file.
---
Nitpick comments:
In `@pkg/commands/apply.go`:
- Around line 693-700: The code silently falls back to normalized-only paths
when filepath.Abs(rootDir) or filepath.Rel fail; update the error paths to log a
debug-level message with the error and context before returning so operators see
why normalization-only was used. Specifically, in the block that calls
filepath.Abs(rootDir) and the block handling filepath.Rel failures, emit a debug
log (using the file's existing logger variable) that includes the failing call
(filepath.Abs or filepath.Rel), the rootDir or path involved, and the error,
then proceed to call engine.NormalizeTemplatePath(p) as before; keep behavior
unchanged except for the added debug logging.
In `@pkg/commands/template_windows_test.go`:
- Line 43: The comment incorrectly warns that the test is "not safe with
t.Parallel — uses os.Chdir" even though the test now calls t.Chdir (Go 1.24+),
which is per-goroutine and safe for parallel tests; update the comment to
reflect that t.Chdir is used and the test can be marked t.Parallel() if desired,
or simply remove the comment entirely; locate the comment above the test
function that uses t.Chdir and either delete it or change it to something like
"safe with t.Parallel — uses t.Chdir (Go 1.24+)" and optionally add t.Parallel()
inside the test body.
🪄 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: e8778d9d-fc69-4275-a6e0-ba31649b3205
📒 Files selected for processing (64)
.gitattributes.github/workflows/pr.yml.gitignore.golangci.ymlcharts/charts.gocharts/charts_test.gogo.modmain.gopkg/age/age.gopkg/age/contract_internals_test.gopkg/age/contract_test.gopkg/commands/apply.gopkg/commands/apply_test.gopkg/commands/consts.gopkg/commands/consts_test.gopkg/commands/contract_chart_gitignore_test.gopkg/commands/contract_helpers_test.gopkg/commands/contract_init_ux_test.gopkg/commands/contract_init_validation_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_test.gopkg/commands/init.gopkg/commands/kubeconfig_handler.gopkg/commands/preflight.gopkg/commands/preflight_test.gopkg/commands/root.gopkg/commands/root_detection.gopkg/commands/root_detection_test.gopkg/commands/rotate_ca_handler.gopkg/commands/talosconfig.gopkg/commands/talosctl_wrapper.gopkg/commands/template.gopkg/commands/template_test.gopkg/commands/template_windows_test.gopkg/commands/upgrade_handler.gopkg/engine/contract_debugphase_test.gopkg/engine/contract_errors_test.gopkg/engine/contract_extractresource_test.gopkg/engine/contract_loadvalues_test.gopkg/engine/contract_machine_test.gopkg/engine/contract_network_legacy_test.gopkg/engine/contract_network_multidoc_test.gopkg/engine/contract_render_test.gopkg/engine/engine.gopkg/engine/engine_test.gopkg/engine/helm/consts_test.gopkg/engine/helm/engine.gopkg/engine/helm/engine_test.gopkg/engine/helm/files.gopkg/engine/helm/files_test.gopkg/engine/helm/funcs.gopkg/engine/helm/funcs_test.gopkg/engine/render_test.gopkg/modeline/consts_test.gopkg/modeline/contract_test.gopkg/modeline/modeline.gopkg/modeline/modeline_test.gopkg/secureperm/contract_unix_test.gopkg/secureperm/secureperm_test.gopkg/secureperm/secureperm_unix.gopkg/secureperm/secureperm_unix_test.gopkg/secureperm/secureperm_windows.gopkg/yamltools/yamltools.gopkg/yamltools/yamltools_test.go
✅ Files skipped from review due to trivial changes (18)
- pkg/engine/contract_machine_test.go
- pkg/engine/contract_extractresource_test.go
- pkg/commands/consts.go
- .gitignore
- pkg/commands/preflight_test.go
- pkg/commands/template_test.go
- pkg/engine/helm/consts_test.go
- pkg/commands/root_detection_test.go
- pkg/engine/contract_network_legacy_test.go
- pkg/secureperm/secureperm_test.go
- go.mod
- pkg/modeline/consts_test.go
- pkg/engine/helm/funcs_test.go
- pkg/engine/contract_loadvalues_test.go
- pkg/engine/contract_network_multidoc_test.go
- pkg/commands/contract_init_ux_test.go
- pkg/engine/contract_errors_test.go
- pkg/yamltools/yamltools_test.go
🚧 Files skipped from review as they are similar to previous changes (35)
- .github/workflows/pr.yml
- pkg/engine/contract_debugphase_test.go
- pkg/commands/consts_test.go
- pkg/commands/talosconfig.go
- main.go
- pkg/engine/engine_test.go
- pkg/commands/contract_init_validation_test.go
- pkg/engine/helm/funcs.go
- pkg/commands/talosctl_wrapper.go
- pkg/commands/kubeconfig_handler.go
- pkg/modeline/modeline_test.go
- pkg/secureperm/secureperm_unix_test.go
- pkg/age/contract_internals_test.go
- pkg/commands/preflight.go
- pkg/secureperm/secureperm_unix.go
- charts/charts.go
- pkg/modeline/contract_test.go
- .golangci.yml
- pkg/commands/contract_template_test.go
- pkg/commands/root.go
- pkg/commands/contract_root_dispatch_test.go
- pkg/secureperm/contract_unix_test.go
- pkg/commands/template.go
- pkg/commands/init.go
- pkg/engine/helm/engine_test.go
- pkg/modeline/modeline.go
- pkg/engine/render_test.go
- pkg/commands/upgrade_handler.go
- pkg/age/age.go
- pkg/secureperm/secureperm_windows.go
- pkg/yamltools/yamltools.go
- pkg/commands/rotate_ca_handler.go
- pkg/commands/root_detection.go
- pkg/commands/apply_test.go
- pkg/engine/helm/engine.go
Two findings from review: 1. The captureStderr helper in contract_chart_gitignore_test.go used t.Cleanup to restore os.Stderr, deferring the restore to test end. The test invokes the helper twice sequentially, so the second call's `origStderr := os.Stderr` captured the first call's already-closed pipe writer; subsequent prints then went into a closed fd. The assertions still passed because os.Pipe's buffer absorbed the writes before the test inspected stdout, but the helper was lying about its restore semantics. Switch to a per-call defer that restores os.Stderr and closes the read end as soon as fn() returns, so each invocation is self-contained. 2. SerializeConfiguration discarded the yaml.Encoder Close error while two sister sites in the same file (engine.go:585, 765) already wrap it. Encoder.Close finalises the stream — a discarded failure here can return a partially-written buffer to the apply/template path, surfacing as malformed YAML downstream instead of an actionable error. Match the existing pattern: errors.Wrap with a descriptive operation name. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Hoist the encode-and-close pattern from applyPatchesAndRenderConfig into encodeYAMLNodeIndented and pin its error semantics. Two tests cover the reachable branches: an always-failing writer drives the encode-error wrap, and a happy- path test pins the success contract. The close-error branch — which CodeRabbit's review surfaced as the original concern — is documented in the test file as unreachable in practice: gopkg.in/yaml.v3's Encoder.Close performs no Write call on the underlying writer for any input the engine produces (verified empirically with a 1024-key mapping and a multi-doc stream — both yield zero writes during Close). The wrap stays in production code as defensive symmetry with the sister sites at engine.go:585 and :765, with a comment in the test file so a future reader does not interpret the absence of a runtime test as confidence the path is dead. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Hoist the captureStderr closure out of TestContract_WriteGitignoreFile_CreatedVsUpdatedReporting into a package-level test helper and add TestCaptureStderr_RestoresOsStderrPerCall as a focused regression guard. The previous t.Cleanup-based version passed assertions only because os.Pipe's buffer absorbed the writes — between sequential captureStderr calls os.Stderr pointed at the first call's already-closed writer, so the second call's origStderr capture preserved a closed fd. The fix's per-call defer is now pinned by asserting that os.Stderr identity matches the original after each helper invocation, and that writing to os.Stderr after the helper returns does not error. Signed-off-by: Aleksei Sviridkin <f@lex.la>
loadConfig used panic(err) on time.ParseDuration failure, which
crashed the talm process before cobra could format the error.
Switch to errors.Wrapf with errors.WithHint so the surrounding
command runner prints a cleanly-formatted diagnostic that names
the offending field, echoes the bad value, and tells the operator
the expected duration format ("30s", "2m", "1h").
Pinned by TestLoadConfig_InvalidApplyTimeoutReturnsError (error
wrap + hint chain) and TestLoadConfig_ValidApplyTimeoutParses
(success path stores TimeoutDuration).
Pre-existing on main; surfaced as a non-blocking observation
during /branch-review and now folded in rather than carried
forward.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The uint32(unsafe.Sizeof(windows.SecurityAttributes{})) idiom is
flagged at-a-glance as a potential narrowing conversion, but the
struct is 1 uint32 + 2 pointers — 24 bytes on win64, 12 on win32
— both fit uint32 trivially. Add an in-line comment naming the
struct layout, the platform sizes, and the canonical x/sys/
windows precedent so a future reader does not need to derive the
safety argument from scratch.
No behaviour change.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Three files still had fmt.Errorf("...: %w", err) sites that the
strict-lint sweep missed: talosctl_wrapper.go (3), talosconfig.go
(5), upgrade_handler.go (2). Convert all 10 to errors.Wrap so the
project's "use cockroachdb/errors not stdlib" standard is
uniform across pkg/commands.
The four remaining fmt.Errorf sites in pkg/engine/helm/engine.go
are deliberate sentinel wraps that match upstream Helm test text;
they stay as-is.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Pre-existing comment claimed the function builds a map with "metadata, spec and stringSpec fields", but stringSpec was never written by the current or prior implementations. Update to match what the code actually emits: "metadata and spec fields". Signed-off-by: Aleksei Sviridkin <f@lex.la>
pkg/generated/presets.go is mechanical glue around the embedded chart map produced by go embed; it's not hand-edited. The linguist-generated marker keeps it out of GitHub language stats and collapses it in PR diffs by default, matching the repo convention captured in CLAUDE.md for vendored/generated code markers. Signed-off-by: Aleksei Sviridkin <f@lex.la>
The block-level comment above the const () group only described completionInternal but read as if it documented all three constants. Move to per-line doc comments so each constant carries its own explanation: initSubcommandName (project Chart.yaml creator), completionSubcommand (user-facing shell completion generator), completionInternal (cobra internal Tab autocompletion). Signed-off-by: Aleksei Sviridkin <f@lex.la>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@main_test.go`:
- Around line 92-96: Snapshot commands.GlobalArgs alongside commands.Config
before calling loadConfig and restore both in the t.Cleanup closure so tests
don't leak mutated global state; specifically, save savedConfig :=
commands.Config and savedGlobalArgs := commands.GlobalArgs (near the existing
saved := commands.Config) and change the cleanup to restore both
(commands.Config = savedConfig; commands.GlobalArgs = savedGlobalArgs). Apply
the same change in the other test block where you also restore only
commands.Config.
In `@main.go`:
- Around line 236-247: The default branch sets
commands.Config.ApplyOptions.Timeout to constants.ConfigTryTimeout.String() but
never sets commands.Config.ApplyOptions.TimeoutDuration, leaving it zero; update
the default path to also set TimeoutDuration (e.g. assign
constants.ConfigTryTimeout or parse the string the same way the else branch
does) so TimeoutDuration is populated regardless of which branch runs; adjust
the logic around time.ParseDuration/use of constants.ConfigTryTimeout and ensure
any returned error handling matches the existing errors.Wrapf/WithHint pattern
used in the else branch.
In `@pkg/commands/contract_chart_gitignore_test.go`:
- Around line 338-345: The deferred cleanup currently restores os.Stderr and
closes r but misses closing the write-end w on failure paths, causing a
potential leak if fn() panics or calls t.Fatal; update the anonymous defer (the
func that restores os.Stderr and calls r.Close()) to also call _ = w.Close() so
the write-end is always closed, ensuring both r and w are closed regardless of
fn()'s outcome.
In `@pkg/engine/contract_yaml_encoder_test.go`:
- Around line 73-95: Test TestEncodeYAMLNodeIndented_HappyPath currently only
checks for "key: value" and doesn't assert the 2-space indentation; update the
test that calls encodeYAMLNodeIndented to explicitly verify indentation by
either (a) creating a nested mapping yaml.Node so the encoded output contains a
line prefixed by exactly two spaces (e.g. "\n key: value") and asserting that
substring is present, or (b) splitting buf.String() into lines and asserting the
line containing "key: value" begins with two spaces; reference
TestEncodeYAMLNodeIndented_HappyPath and encodeYAMLNodeIndented when making the
change.
🪄 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: 45baa7c1-3216-42c2-9f43-d0cdd1a805ee
📒 Files selected for processing (10)
.gitattributesmain.gomain_test.gopkg/commands/contract_chart_gitignore_test.gopkg/commands/talosconfig.gopkg/commands/talosctl_wrapper.gopkg/commands/upgrade_handler.gopkg/engine/contract_yaml_encoder_test.gopkg/engine/engine.gopkg/secureperm/secureperm_windows.go
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/commands/upgrade_handler.go
- pkg/commands/talosctl_wrapper.go
- pkg/commands/talosconfig.go
- pkg/secureperm/secureperm_windows.go
- pkg/engine/engine.go
Pre-existing on main since 2024: when applyOptions.timeout is empty in Chart.yaml, loadConfig filled the Timeout string with constants.ConfigTryTimeout.String() but never parsed it into TimeoutDuration — the parse lived only in the else branch. The field had no consumer in production, so the bug never surfaced visibly, but the post-condition "TimeoutDuration is populated after a successful loadConfig" was silently broken. Restructure the parse to run unconditionally after the default- string fill so both branches end up with TimeoutDuration set. The error wrap and operator hint stay shaped as before. Pinned by TestLoadConfig_EmptyApplyTimeoutResolvesDefault. The existing happy-path and invalid-input tests continue to cover the explicit-value branches. Also extract snapshotConfigState to capture both commands.Config AND commands.GlobalArgs across loadConfig calls — loadConfig mutates GlobalArgs.Talosconfig too, and tests that restored only Config could leak that field into siblings. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Defer previously closed only the read end. If fn() panics or calls t.Fatal, control bypasses the explicit w.Close() on the normal path and the writer fd leaks. Move w.Close() into the defer too — closing twice on the normal path is safe (the second call returns an error which we discard explicitly). Signed-off-by: Aleksei Sviridkin <f@lex.la>
Previous shape claimed in the docstring to "guard against a refactor that swaps the indent" but only checked for the substring "key: value" — flat top-level mapping has no leading spaces regardless of indent setting, so the assertion was indent-blind. Switch to a nested mapping and assert "\n inner: leaf" with the 2-space prefix, plus an explicit reject of the "\n inner: leaf" 4-space variant (yaml.v3's default when SetIndent is not called). Now a regression that drops SetIndent(2) actually fails this test. Signed-off-by: Aleksei Sviridkin <f@lex.la>
2 participants
Closes #153.
What
Adopts the
default: allgolangci-lint v2 config proposed in #153 and reconciles the project against it: zero lint findings on bothlinux/amd64(host) andwindows/amd64, with a CI lint matrix gate so the strict config doesn't erode silently.Highlights
Strict-lint adoption
.golangci.yml:default: allwith the relaxations from the issue (depguard, exhaustruct, gochecknoinits, wsl, lll, errchkjson, ireturn, gocheckcompilerdirectives) plus three documented additions:noinlineerr(200+ inline if-err sites; conversion would create variable-scope leaks),goconston_test.go(test fixtures live inside backtick strings; substitution desyncs expected vs. got),gomodguard(deprecated in v2.12+, neither allow nor blocklist configured here).gomoddirectives.replace-allow-listforgithub.tiyicn.workers.dev/siderolabs/talosand.../pkg/machinery— required while upstream declines feat(talosctl): add --skip-verify flag to skip TLS certificate verification siderolabs/talos#12652.CI lint gate
lint:job in.github/workflows/pr.ymlrunsgolangci-lint v2.12.2on Ubuntu and Windows runners (matching the existingtest:matrix). Build-tagged Windows files (secureperm_windows.go,template_windows_test.go) are now actually evaluated on every PR instead of silently diverging.Bug fixes surfaced by the lint pass
pkg/commands/preflight.go:nilerr— tworeturn nilafterif err != nilsites. Pinned as deliberate best-effort behaviour with//nolint:nilerrcarrying the rationale ("never block apply on parse failure"); the version-warn preflight is advisory and must not refuse the apply itself when a Talos COSI version read fails.pkg/commands/rotate_ca_handler.go:nosprintfhostport— URL constructed viafmt.Sprintf("%s:%s", host, port)broke for IPv6 endpoints. Switched to the package-levelnormalizeEndpointhelper which handles bracketed IPv6 round-trips. Two sister sites inrotate_ca_handler.go(--k8s-endpointflag default andupdateKubeconfigEndpoint) now also delegate tonormalizeEndpointso they cannot silently regress in isolation.pkg/age/age.go+pkg/engine/engine.go:forcetypeassert— type assertions without ok-form panicked on unexpected YAML shapes. Switched to ok-form with wrapped error returns.pkg/yamltools/yamltools.go+pkg/engine/engine.go:exhaustive— yaml.Kind switches missedDocumentNode/AliasNode/ScalarNode/SequenceNode. Added explicit cases with documented intent.pkg/commands/init.go:writeGitignoreFilereported "Updated" for files it just created (existence check ran AFTER WriteFile). Fixed by capturing existence before the write and branching onos.IsNotExist(statErrBefore)so ambiguous stat errors (e.g. EACCES on the parent directory) fall into the same bucket as "exists" instead of falsely reporting "Created". Pinned byTestContract_WriteGitignoreFile_CreatedVsUpdatedReporting.pkg/engine/helm/files.go:Lines()guarded against anindex out of rangepanic when a key exists in the files map but maps to an empty[]bytevalue. Pinned byTestLines_EmptyContent.charts/charts.go:PresetFilesnow wraps the embedded-FSWalkDirerrors with the offending file path viaerrors.Wrapf, matching the rationale carried by the surroundingnolint:wrapcheckdirectives elsewhere in the package. Pinned byTestPresetFiles_*.Refactors
pkg/age/age.go:EncryptSecretsFile/DecryptSecretsFile/RotateKeysdecomposed into named helpers (encryptYAMLPair,decryptYAMLPair,loadOrGenerateIdentity,incrementalEncryptMap, plus rotation-phase helpers). All 19 contract tests still pass.pkg/commands/apply.go+template.go: split monolithic per-file closures into named helpers (applyOneFileTemplateMode,applyOneFileDirectPatchMode,templateOneFile,buildTemplateRunner, etc.). The per-iterationGlobalArgs.Nodes/Endpointsreset is consolidated intoresetGlobalArgsBetweenFiles, with an explicit comment + contract tests pinning the reset to[]string{}(the talos client'sclient.WithConfigfalls back to talosconfig context endpoints whenGlobalArgs.Endpointsis empty; re-seeding loopback would override that fallback for any subsequent file whose modeline omits endpoints).pkg/engine/engine.go:extractResourceDatarewritten — was an unguardedyamlValue.(string)assertion that would panic; now returns a typed error.FullConfigProcesslost its unused_ context.Contextparameter (see Breaking changes below).main.go:strings.HasPrefix(cmd.Use, "init")switched to use the existinginitSubcommandNameconstant for a single source of truth.Error-handling migration
fmt.Errorf("...: %w", err)tocockroachdb/errors.Wrap/Wrapf(project standard). Adds operator-facing hints viaerrors.WithHintat boundaries.Const hoisting
Breaking changes
engine.FullConfigProcessdrops its unused_ context.Contextfirst parameter. The function performs no I/O that respects cancellation, and the underscore-named parameter was misleading API surface. Both in-tree callers (pkg/commands/apply.go,pkg/commands/upgrade_handler.go) are updated. External consumers ofpkg/enginethat vendor this package will see a compile break and need to drop theirctxargument at the call site.Verification
go build ./...— clean (host + GOOS=windows)go test ./... -count=1— green (8/8 packages)go test ./... -race— greengolangci-lint run— 0 issues (host + GOOS=windows)talm init --preset cozystack --name test-clustercreates expected files; ancestor-project refusal works;--root .opt-in works.Stats
61 files changed, +4046/-2012 lines. Bulk of the diff is mechanical: const extraction, error-wrap migration, blank-line discipline, and
//nolint:annotations with rationale where the rule cannot be satisfied without losing fidelity (project's "200% rule" on disabling lints).Summary by CodeRabbit
New Features
Chores