test: pin existing behaviour as a test contract across charts and core packages#154
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds comprehensive contract-test coverage across the codebase and a CI job to run tests with coverage and upload a coverage profile. ChangesContract Test Suite Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 an extensive suite of contract tests across multiple packages to ensure the stability of core functionalities such as configuration rendering, project detection, and key management. These tests document and enforce intended behaviors for both legacy and multi-doc Talos schemas. A review comment correctly identified a documentation error in the network rendering tests where a comment contradicted the actual test implementation for VIP address stripping.
Comment on lines
+338
to
+339
| // Picking a VIP that does not match any discovery address makes the | ||
| // test robust against future discovery fixture changes. |
Contributor
There was a problem hiding this comment.
The comment here is contradictory to the comment on line 341 and the actual test implementation. The test intentionally picks a VIP that does match the discovery address to verify that it is stripped from the LinkConfig. Please update this comment to accurately reflect the test's purpose.
Contributor
Author
There was a problem hiding this comment.
Good catch — the leading comment was a leftover from an earlier iteration that picked a non-matching VIP. The test deliberately reuses the discovery address so the strip path actually fires. Comment rewritten in 43c9eb6 to describe that intent.
![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
🧹 Nitpick comments (5)
pkg/commands/root_detection_test.go (1)
305-347: ⚡ Quick win
filesDiffertest doesn't cover the(false, err)I/O-error path its own docstring documents.The contract comment at lines 312–313 says the function returns
(false, err)on read errors other than not-exist, but no test case exercises this path. The consequence is that a regression that silently returns(false, nil)on permission errors would pass undetected.This is a low-priority gap — but since the contract text is already written and the pattern for adding a case is straightforward (use a path inside a
0000-mode directory), it's worth closing.🤖 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_test.go` around lines 305 - 347, Add a test case to TestContract_FilesDiffer that exercises the "(false, err)" path of filesDiffer by creating a directory with restrictive permissions (e.g., mode 0000), placing the target filename inside it (or attempting to read a file path within it) and calling filesDiffer to provoke a permission/read error; assert that filesDiffer returns false and a non-nil error, and restore permissions before cleanup so the temp dir can be removed. Reference the existing TestContract_FilesDiffer and the filesDiffer function when locating where to insert this case.pkg/engine/contract_loadvalues_test.go (1)
36-51: 💤 Low valueContract docstring contradicts the actual assertion.
The function comment says
"no aliasing — mutating the result must not affect base", but the code usest.Logf(nott.Errorf) when aliasing is detected, meaning aliasing is silently accepted. A future maintainer reading the docstring would reasonably expect this to be an enforced invariant.Either update the comment to describe the actual pinned behaviour (aliasing is possible for pure-
akeys) and drop the misleading "no aliasing" framing, or changet.Logf→t.Errorfif the intent really is to enforce copy semantics.✏️ Suggested comment clarification
-// Contract: merging an empty map onto base returns a copy of base -// (no aliasing — mutating the result must not affect base). +// Contract: merging an empty map onto base returns a result with +// the same key-value pairs as base. Known quirk: keys present only +// in `a` (not present in `b`) are NOT deep-copied; they are shared +// by reference. This is the upstream Helm behaviour and is pinned +// here — do not assume copy semantics for pure-base keys.🤖 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/contract_loadvalues_test.go` around lines 36 - 51, The test TestContract_MergeMaps_EmptyOverlay documents "no aliasing" but then only logs (t.Logf) when aliasing is observed; fix the mismatch in the test for mergeMaps by either (A) enforcing the invariant: replace the t.Logf call with t.Errorf so the test fails when aliasing occurs (keeps the comment as-is), or (B) clarifying the docstring to state the pinned behaviour (that pure-`a` keys may be aliased) and keep t.Logf; locate the changes in TestContract_MergeMaps_EmptyOverlay and reference mergeMaps to ensure the test and its comment are consistent.pkg/engine/contract_errors_test.go (1)
49-74: 💤 Low value
helmEngine.LookupFuncglobal mutation is not parallel-safe.
renderExpectingErrorcorrectly guards witht.Cleanup, but becausehelmEngine.LookupFuncis a package-level variable, any future addition oft.Parallel()to these subtests would introduce a data race. Consider accepting the lookup as a parameter to the engine or wrapping it in a mutex-protected pattern now that this helper is established.This is a test-only concern with no current impact.
🤖 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/contract_errors_test.go` around lines 49 - 74, The test mutates the package-global helmEngine.LookupFunc without concurrency protection; add a package-level sync.Mutex (e.g., helmEngineLookupMu) and lock it around the mutation in renderExpectingError: call helmEngineLookupMu.Lock() before assigning helmEngine.LookupFunc, and in the existing t.Cleanup restore origLookup and call helmEngineLookupMu.Unlock(); keep the existing use of helmEngineEmptyLookup when lookup is nil and preserve t.Helper(), loader.LoadDir, cloneValues, maps.Copy, and eng.Render behavior.pkg/age/contract_internals_test.go (1)
166-172: ⚡ Quick winPrefer guarded type assertions with
t.Fatalfover bare panics in test helpers.The assertions at lines 166–167, 171–172, 235–236, and 274–275 will
panicon a type mismatch rather than callingt.Fatalf, which produces a raw stack trace instead of a clear test-failure message when the contract unexpectedly breaks.♻️ Pattern to apply at each assertion site
-firstTop := first["top"].(map[string]any) -secondTop := second["top"].(map[string]any) +firstTop, ok := first["top"].(map[string]any) +if !ok { + t.Fatalf("expected first[\"top\"] to be map[string]any, got %T", first["top"]) +} +secondTop, ok := second["top"].(map[string]any) +if !ok { + t.Fatalf("expected second[\"top\"] to be map[string]any, got %T", second["top"]) +}Apply the same pattern to the
["sub"],["items"]assertions in the other tests.Also applies to: 235-236, 274-275
🤖 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_internals_test.go` around lines 166 - 172, The current unguarded type assertions like firstTop := first["top"].(map[string]any) and secondTop := second["top"].(map[string]any) (and similar sites for firstSub/secondSub and ["items"]) can panic on mismatch; change them to guarded assertions using the comma-ok form (e.g., v, ok := first["top"].(map[string]any)) and call t.Fatalf with a clear message when ok is false so the test fails cleanly instead of panicking; apply the same pattern for firstSub/secondSub and the items assertions mentioned in the review.pkg/engine/contract_cluster_test.go (1)
351-370: ⚡ Quick winLoose substring assertions for
"disabled: true"and"enabled: false"may yield false positives.
TestContract_Cluster_ProxyDisabled_Cozystackasserts"disabled: true"independently of its parent key, andTestContract_Cluster_DiscoveryDisabled_Cozystackdoes the same for"enabled: false". If any other rendered YAML block in the output (e.g., another component withdisabled: true) happens to match, the test passes even if the targeted section is missing or structurally changed.Tightening to a combined pattern or using a multi-line assertion would make these contracts more precise:
♻️ Proposed tightening
- assertContains(t, out, "proxy:") - assertContains(t, out, "disabled: true") + assertContains(t, out, "proxy:\n disabled: true")- assertContains(t, out, "discovery:") - assertContains(t, out, "enabled: false") + assertContains(t, out, "discovery:\n enabled: false")Note: the exact indentation in the multiline string depends on the rendered YAML indentation level — verify against actual chart output before applying.
🤖 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/contract_cluster_test.go` around lines 351 - 370, Tests TestContract_Cluster_ProxyDisabled_Cozystack and TestContract_Cluster_DiscoveryDisabled_Cozystack use loose substring checks and can false-positive; update each test (referencing renderChartTemplate and assertContains) to assert the parent key and child value together (e.g., match a multi-line block or use a regex anchored to the parent key) so you verify "proxy" with its nested "disabled: true" and "discovery" with its nested "enabled: false" rather than any standalone "disabled: true" or "enabled: false"; ensure the pattern accounts for the YAML indentation used by the chart output.
🤖 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/contract_test.go`:
- Around line 357-366: The test currently swallows errors from os.ReadFile when
assigning first and second, which can mask I/O failures; update the two reads
(the calls that set first and second) to check the returned error and call
t.Fatal(err) (or t.Fatalf) if non-nil so failures surface immediately — locate
the os.ReadFile calls that produce variables first and second in the test and
add error handling for each.
In `@pkg/commands/contract_chart_gitignore_test.go`:
- Around line 215-235: Each of the three tests
(TestContract_WriteGitignoreFile_PreservesExistingEntries,
TestContract_WriteGitignoreFile_IdempotentOnFullList,
TestContract_WriteGitignoreFile_TolerantOfAnnotatedEntries) must capture the
current Config.GlobalOptions.Kubeconfig into a local variable at test start and
restore it in t.Cleanup instead of unconditionally setting it to ""; change the
t.Cleanup(func() { Config.GlobalOptions.Kubeconfig = "" }) calls to save :=
Config.GlobalOptions.Kubeconfig and t.Cleanup(func() {
Config.GlobalOptions.Kubeconfig = save }) so the original kubeconfig is
preserved across tests.
In `@pkg/commands/contract_helpers_test.go`:
- Around line 40-61: Test and implementation are producing a malformed IPv6 URL
(missing square brackets); update normalizeEndpoint to use
net.JoinHostPort(host, "6443") when composing host:port so IPv6 addresses get
bracketed correctly, and update the test case in TestContract_NormalizeEndpoint
to expect "https://[2001:db8::1]:6443"; also add or reference an issue note in
the comment if this was intentionally tracked as known-broken behavior.
- Around line 40-61: The normalizeEndpoint result must return valid
URI-bracketed IPv6 literals; update the normalizeEndpoint function to detect
when the host is an IPv6 literal (e.g., contains ':' after net.SplitHostPort or
when the input had brackets) and re-add surrounding square brackets before
formatting the final "https://<host>:<port>" string; ensure you still normalize
scheme to https and default port to 6443, handle inputs that already include a
scheme or port, and use the same host/port symbols returned by net.SplitHostPort
(or url parsing) so tests like TestContract_NormalizeEndpoint (which calls
normalizeEndpoint) produce "https://[2001:db8::1]:6443" not an unbracketed IPv6
address.
In `@pkg/engine/contract_cluster_test.go`:
- Around line 215-227: The test
TestContract_Cluster_NoControlplaneBlocksOnWorker is missing an assertion that
the controlplane-only block "discovery:" is absent from worker templates; inside
the t.Run body of that function (alongside the existing assertNotContains checks
for "apiServer:", "controllerManager:", "scheduler:", "etcd:",
"allowSchedulingOnControlPlanes", and "proxy:"), add an assertNotContains(t,
out, "discovery:") to enforce the contract.
---
Nitpick comments:
In `@pkg/age/contract_internals_test.go`:
- Around line 166-172: The current unguarded type assertions like firstTop :=
first["top"].(map[string]any) and secondTop := second["top"].(map[string]any)
(and similar sites for firstSub/secondSub and ["items"]) can panic on mismatch;
change them to guarded assertions using the comma-ok form (e.g., v, ok :=
first["top"].(map[string]any)) and call t.Fatalf with a clear message when ok is
false so the test fails cleanly instead of panicking; apply the same pattern for
firstSub/secondSub and the items assertions mentioned in the review.
In `@pkg/commands/root_detection_test.go`:
- Around line 305-347: Add a test case to TestContract_FilesDiffer that
exercises the "(false, err)" path of filesDiffer by creating a directory with
restrictive permissions (e.g., mode 0000), placing the target filename inside it
(or attempting to read a file path within it) and calling filesDiffer to provoke
a permission/read error; assert that filesDiffer returns false and a non-nil
error, and restore permissions before cleanup so the temp dir can be removed.
Reference the existing TestContract_FilesDiffer and the filesDiffer function
when locating where to insert this case.
In `@pkg/engine/contract_cluster_test.go`:
- Around line 351-370: Tests TestContract_Cluster_ProxyDisabled_Cozystack and
TestContract_Cluster_DiscoveryDisabled_Cozystack use loose substring checks and
can false-positive; update each test (referencing renderChartTemplate and
assertContains) to assert the parent key and child value together (e.g., match a
multi-line block or use a regex anchored to the parent key) so you verify
"proxy" with its nested "disabled: true" and "discovery" with its nested
"enabled: false" rather than any standalone "disabled: true" or "enabled:
false"; ensure the pattern accounts for the YAML indentation used by the chart
output.
In `@pkg/engine/contract_errors_test.go`:
- Around line 49-74: The test mutates the package-global helmEngine.LookupFunc
without concurrency protection; add a package-level sync.Mutex (e.g.,
helmEngineLookupMu) and lock it around the mutation in renderExpectingError:
call helmEngineLookupMu.Lock() before assigning helmEngine.LookupFunc, and in
the existing t.Cleanup restore origLookup and call helmEngineLookupMu.Unlock();
keep the existing use of helmEngineEmptyLookup when lookup is nil and preserve
t.Helper(), loader.LoadDir, cloneValues, maps.Copy, and eng.Render behavior.
In `@pkg/engine/contract_loadvalues_test.go`:
- Around line 36-51: The test TestContract_MergeMaps_EmptyOverlay documents "no
aliasing" but then only logs (t.Logf) when aliasing is observed; fix the
mismatch in the test for mergeMaps by either (A) enforcing the invariant:
replace the t.Logf call with t.Errorf so the test fails when aliasing occurs
(keeps the comment as-is), or (B) clarifying the docstring to state the pinned
behaviour (that pure-`a` keys may be aliased) and keep t.Logf; locate the
changes in TestContract_MergeMaps_EmptyOverlay and reference mergeMaps to ensure
the test and its comment are consistent.
🪄 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: 1a3152f6-804b-4aaa-87c7-e26e8c9b8411
📒 Files selected for processing (18)
.github/workflows/pr.ymlpkg/age/contract_internals_test.gopkg/age/contract_test.gopkg/commands/contract_chart_gitignore_test.gopkg/commands/contract_helpers_test.gopkg/commands/contract_root_detection_test.gopkg/commands/root_detection_test.gopkg/engine/contract_cluster_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/contract_schema_test.gopkg/modeline/contract_test.gopkg/secureperm/contract_unix_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/commands/contract_root_dispatch_test.go (1)
491-527: ⚡ Quick winAdd an explicit IPv6 contract case for
updateKubeconfigEndpoint(known defect pin).The rewrite tests only assert IPv4 behavior right now. Please add a contract test for IPv6 endpoint input and pin current behavior with
FIXME#155`` (per repo convention for known-broken contracts), so regressions/fixes are tracked intentionally.Based on learnings: contract tests in this repo intentionally pin known-broken behavior with
FIXME #<issue>comments, including the IPv6 bracketing defect tracked as#155.🤖 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/contract_root_dispatch_test.go` around lines 491 - 527, Add a new contract test that exercises updateKubeconfigEndpoint with an IPv6 endpoint input (e.g. "2001:db8::1:50000") to pin current behavior; create a test named TestContract_UpdateKubeconfigEndpoint_IPv6 that calls updateKubeconfigEndpoint with a kubeconfig containing cluster servers and asserts the rewritten server contains the unbracketed IPv6 + port (or current observed behavior), and annotate the test with a comment "FIXME `#155`" to mark the known IPv6 bracketing defect; ensure you reference updateKubeconfigEndpoint and mirror the existing IPv4 test structure for parsing and error checks so the contract captures the current behavior.
🤖 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.
Nitpick comments:
In `@pkg/commands/contract_root_dispatch_test.go`:
- Around line 491-527: Add a new contract test that exercises
updateKubeconfigEndpoint with an IPv6 endpoint input (e.g. "2001:db8::1:50000")
to pin current behavior; create a test named
TestContract_UpdateKubeconfigEndpoint_IPv6 that calls updateKubeconfigEndpoint
with a kubeconfig containing cluster servers and asserts the rewritten server
contains the unbracketed IPv6 + port (or current observed behavior), and
annotate the test with a comment "FIXME `#155`" to mark the known IPv6 bracketing
defect; ensure you reference updateKubeconfigEndpoint and mirror the existing
IPv4 test structure for parsing and error checks so the contract captures the
current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7fa8114-e2e2-423c-a6a8-0b6e033c3461
📒 Files selected for processing (1)
pkg/commands/contract_root_dispatch_test.go
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 `@pkg/commands/contract_template_test.go`:
- Around line 256-271: The test TestContract_GenerateOutput_NoTemplatesError
currently only checks that generateOutput returns a non-nil error; update it to
also assert that the error string has the expected wrapped-prefix "failed to
render templates" (e.g., using strings.HasPrefix or errors.As to inspect the
error message) so the contract described in the comment is enforced; locate the
assertion in TestContract_GenerateOutput_NoTemplatesError and add the prefix
check against the err returned from generateOutput.
- Around line 391-414: The captureStdout helper leaves os.Stdout pointed at a
closed pipe and never closes the read end; change captureStdout so it restores
os.Stdout and closes the writer/reader immediately with defers scoped to the
helper instead of using t.Cleanup: save original := os.Stdout, set os.Stdout =
w, then defer func() { os.Stdout = original }(), defer w.Close(), and ensure r
is closed after the reader goroutine finishes (or closed in a defer after the
goroutine signals completion); remove the t.Cleanup usage and ensure the
goroutine reads until EOF then signals a done channel so you close r/w
deterministically within captureStdout.
🪄 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: 7065bfbd-d0da-4928-813c-f3a2cd90de65
📒 Files selected for processing (5)
pkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_test.gopkg/commands/root_detection_test.gopkg/engine/contract_loadvalues_test.gopkg/engine/contract_schema_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/engine/contract_loadvalues_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/engine/contract_schema_test.go
- pkg/commands/contract_root_dispatch_test.go
- pkg/commands/root_detection_test.go
Comment on lines
+256
to
+271
| // Contract: an empty templates list surfaces the engine-side | ||
| // 'templates are not set' error wrapped with the same prefix. | ||
| func TestContract_GenerateOutput_NoTemplatesError(t *testing.T) { | ||
| withTemplateFlagsSnapshot(t) | ||
|
|
||
| chartRoot := makeMinimalChart(t) | ||
| Config.RootDir = chartRoot | ||
| templateCmdFlags.offline = true | ||
| templateCmdFlags.templateFiles = nil | ||
| GlobalArgs.Nodes = []string{"10.0.0.1"} | ||
|
|
||
| _, err := generateOutput(context.Background(), nil, nil) | ||
| if err == nil { | ||
| t.Fatal("expected error for empty templates") | ||
| } | ||
| } |
There was a problem hiding this comment.
Contract check is incomplete for the “no templates” path.
Line 257 states the wrapped-prefix contract, but the test only asserts err != nil. Please also assert the expected "failed to render templates" prefix so this behavior is actually pinned.
Suggested test assertion update
func TestContract_GenerateOutput_NoTemplatesError(t *testing.T) {
@@
_, err := generateOutput(context.Background(), nil, nil)
if err == nil {
t.Fatal("expected error for empty templates")
}
+ if !strings.Contains(err.Error(), "failed to render templates") {
+ t.Errorf("error must wrap with 'failed to render templates', got: %v", err)
+ }
}📝 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
| // Contract: an empty templates list surfaces the engine-side | |
| // 'templates are not set' error wrapped with the same prefix. | |
| func TestContract_GenerateOutput_NoTemplatesError(t *testing.T) { | |
| withTemplateFlagsSnapshot(t) | |
| chartRoot := makeMinimalChart(t) | |
| Config.RootDir = chartRoot | |
| templateCmdFlags.offline = true | |
| templateCmdFlags.templateFiles = nil | |
| GlobalArgs.Nodes = []string{"10.0.0.1"} | |
| _, err := generateOutput(context.Background(), nil, nil) | |
| if err == nil { | |
| t.Fatal("expected error for empty templates") | |
| } | |
| } | |
| // Contract: an empty templates list surfaces the engine-side | |
| // 'templates are not set' error wrapped with the same prefix. | |
| func TestContract_GenerateOutput_NoTemplatesError(t *testing.T) { | |
| withTemplateFlagsSnapshot(t) | |
| chartRoot := makeMinimalChart(t) | |
| Config.RootDir = chartRoot | |
| templateCmdFlags.offline = true | |
| templateCmdFlags.templateFiles = nil | |
| GlobalArgs.Nodes = []string{"10.0.0.1"} | |
| _, err := generateOutput(context.Background(), nil, nil) | |
| if err == nil { | |
| t.Fatal("expected error for empty templates") | |
| } | |
| if !strings.Contains(err.Error(), "failed to render templates") { | |
| t.Errorf("error must wrap with 'failed to render templates', got: %v", err) | |
| } | |
| } |
🤖 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/contract_template_test.go` around lines 256 - 271, The test
TestContract_GenerateOutput_NoTemplatesError currently only checks that
generateOutput returns a non-nil error; update it to also assert that the error
string has the expected wrapped-prefix "failed to render templates" (e.g., using
strings.HasPrefix or errors.As to inspect the error message) so the contract
described in the comment is enforced; locate the assertion in
TestContract_GenerateOutput_NoTemplatesError and add the prefix check against
the err returned from generateOutput.
Comment on lines
+391
to
+414
| original := os.Stdout | ||
| os.Stdout = w | ||
| t.Cleanup(func() { os.Stdout = original }) | ||
|
|
||
| done := make(chan string, 1) | ||
| go func() { | ||
| buf := make([]byte, 0, 64*1024) | ||
| readBuf := make([]byte, 4096) | ||
| for { | ||
| n, err := r.Read(readBuf) | ||
| if n > 0 { | ||
| buf = append(buf, readBuf[:n]...) | ||
| } | ||
| if err != nil { | ||
| break | ||
| } | ||
| } | ||
| done <- string(buf) | ||
| }() | ||
|
|
||
| fn() | ||
| _ = w.Close() | ||
| return <-done | ||
| } |
There was a problem hiding this comment.
captureStdout leaves global stdout redirected too long and leaks pipe lifecycle handling.
On Line 393, restoring os.Stdout in t.Cleanup defers restoration until test end, so after Line 412 closes w, stdout can remain pointed to a closed pipe for the rest of the test. Also, r is never explicitly closed on the success path. Restore/close immediately with defers scoped to the helper.
Suggested helper hardening
func captureStdout(t *testing.T, fn func()) string {
t.Helper()
r, w, err := os.Pipe()
if err != nil {
t.Fatal(err)
}
original := os.Stdout
os.Stdout = w
- t.Cleanup(func() { os.Stdout = original })
+ defer func() {
+ os.Stdout = original
+ _ = r.Close()
+ _ = w.Close()
+ }()
@@
fn()
_ = w.Close()
return <-done
}🤖 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/contract_template_test.go` around lines 391 - 414, The
captureStdout helper leaves os.Stdout pointed at a closed pipe and never closes
the read end; change captureStdout so it restores os.Stdout and closes the
writer/reader immediately with defers scoped to the helper instead of using
t.Cleanup: save original := os.Stdout, set os.Stdout = w, then defer func() {
os.Stdout = original }(), defer w.Close(), and ensure r is closed after the
reader goroutine finishes (or closed in a defer after the goroutine signals
completion); remove the t.Cleanup usage and ensure the goroutine reads until EOF
then signals a done channel so you close r/w deterministically within
captureStdout.
IvanHunters
previously approved these changes
May 9, 2026
…charts Add contract_cluster_test.go documenting the user-facing semantics of every values.yaml knob that surfaces under cluster.* across the chart matrix (cozystack/generic, legacy/multidoc, controlplane/worker). The tests are intentionally repetitive across the matrix: when one combination drifts, the test name points directly at the broken cell. Each test starts with a Contract: comment describing what is pinned and why — the comment is the user-facing documentation, the body is the executable enforcement. Coverage: - Shared (both charts): clusterName defaults to Chart.Name, podSubnets default 10.244.0.0/16, serviceSubnets default 10.96.0.0/16, endpoint always quoted. - Worker invariant (both charts): no apiServer/controllerManager/ scheduler/etcd/proxy/allowSchedulingOnControlPlanes leakage. - Cozystack-only: clusterDomain=cozy.local, unconditional 127.0.0.1 in apiServer.certSANs, certSANs append behaviour, OIDC absent by default and present when issuer set, allocateNodeCIDRs default-on with cluster-cidr emission, allocateNodeCIDRs disabled drops cluster-cidr, allowSchedulingOnControlPlanes, proxy disabled, discovery disabled. - Generic-only (pin minimalism): no clusterDomain, no unconditional loopback, no cozystack-specific defaults, apiServer block emitted but empty without certSANs, certSANs appended verbatim. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…charts Add contract_machine_test.go documenting machine.* semantics across the chart matrix. Continues the contract-as-documentation pattern from contract_cluster_test.go. Coverage: - Shared (both charts): machine.type matches template name (cp vs worker), kubelet.nodeIP.validSubnets always emitted, install.disk always emitted. - Controlplane-only: cozystack emits nodeLabels with $patch:delete on exclude-from-external-load-balancers; generic and worker never do. - Cozystack-only: kubelet extraConfig (cpuManagerPolicy:static, maxPods:512), gc_thresh sysctls trio (4096/8192/16384), vm.nr_hugepages opt-in (absent at default 0, emitted as quoted string when set), six pinned kernel modules with drbd usermode_helper=disabled parameter, machine.certSANs unconditional 127.0.0.1, two hardcoded files (containerd device-ownership patch and lvm.conf with global_filter for drbd/dm/zd), install.image default to ghcr.io/cozystack/cozystack/talos, install.image override path, registries.mirrors docker.io->mirror.gcr.io (legacy schema only — multi-doc uses RegistryMirrorConfig). - Generic-only (pin minimalism): no extraConfig, no sysctls, no kernel modules, no machine-level files, no install.image, no unconditional 127.0.0.1, no nodeLabels, no registries; certSANs appended verbatim on both machine and apiServer levels. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_schema_test.go pinning how templateOptions.talosVersion controls schema selection in talos.config (_helpers.tpl). Coverage: - Empty version → legacy (default behaviour, single document with machine.network). - v1.10/v1.11/partial "1.11" → legacy. - v1.12.0+/v1.13/v2.0.0/partial "1.12" → multi-doc (HostnameConfig + ResolverConfig as separate documents joined by ---). - v1.12.0-rc/-alpha/-beta pre-releases satisfy the >=1.12.0-0 anchor and render multi-doc (cluster-bootstrap path: nodes booted off pre-release maintenance images must get the new schema). - Legacy is always exactly one document — no internal --- separator. - Multi-doc emits HostnameConfig + ResolverConfig unconditionally on controlplane regardless of discovery state. - RegistryMirrorConfig (multi-doc) emitted only on cozystack chart; generic does not ship a default mirror. - Schema selection is per-render, not cached: alternating legacy and multi-doc renders in one process produce distinct outputs. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_errors_test.go pinning every `fail` and `required` directive in cozystack and generic _helpers.tpl. Error messages appear in talm template stderr and CI logs; users grep for the talm: prefix and specific substrings to debug bad inputs. Pinning the wording makes changing an error a reviewable act, not a silent break for everyone with a matching alert rule. Coverage: - endpoint required guard (cluster-wide, no auto-derive, URL example). - advertisedSubnets empty + empty discovery (names the values key, the missing default-gateway-bearing link, and operator recourse). - multi-doc + legacy machine.network.interfaces[] in running MachineConfig (lists LinkConfig/VLANConfig/BondConfig as recourse, v1.11 version pin as fallback). - multi-doc + bridge as IPv4-default link (names the link, missing BridgeConfig feature, vipLink workaround). - multi-doc + VLAN with unresolvable parent (spec.linkIndex hint). - multi-doc + VLAN with no vlanID (spec.vlan.vlanID hint). Add renderExpectingError helper that calls helm Render directly and returns the raw error so tests can assert on the message body. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_network_multidoc_test.go pinning the Talos v1.12+ multi-doc network output for both shipped charts. The renderer reconstructs network configuration from COSI discovery resources and emits one typed document per configurable link plus HostnameConfig + ResolverConfig. Coverage: - HostnameConfig: discovered hostname surfaced when 'real'; fallback to synthesized 'talos-<hash>' for placeholder names (rescue, talos, localhost, localhost.localdomain) or empty discovery. - ResolverConfig: populated from discovery dnsServers; YAML empty list fallback when resolvers unknown. - LinkConfig: single physical NIC produces one document (name + addresses + routes for gateway link). - Multi-NIC routing rule: only the gateway-bearing link's LinkConfig carries 'routes:'; non-gateway links emit addresses without routes (otherwise duplicate default routes shadow each other). - BondConfig: bondMaster fields surfaced verbatim (bondMode, xmitHashPolicy, lacpRate, miimon); slaves listed under links:. - Bond slaves: never emitted as standalone LinkConfig (configurable_link_names filters them by spec.slaveKind). - VLANConfig: emitted for VLAN with resolvable parent + vlanID; bond-as-parent supported. - Bridge non-gateway: skipped silently (no BridgeConfig emission yet). - Layer2VIPConfig discovery path: emitted on controlplane with link=<discovered-default-link>; never on worker. - Layer2VIPConfig vipLink override: emits with link=<vipLink>, suppresses discovery-derived emission, works on fresh-boot when no default link is discoverable yet. - floatingIP stripping: VIP not duplicated under LinkConfig.addresses even when discovery sees it on the link. Reuses existing lookup factories (simpleNicLookup, multiNicLookup, bondTopologyLookup, vlanOnBondTopologyLookup, bridgeLookup, freshNicLookup) — those fixtures are themselves part of the contract. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_network_legacy_test.go pinning the pre-v1.12 / Talos legacy network output for cozystack and generic charts. The legacy schema collapses everything into a single machine.network block: hostname, nameservers, and an interfaces[] list carrying addresses, routes, optional vlans[], optional bond block, and optional inline vip block. Coverage: - machine.network always emits hostname (quoted, with placeholder/ fallback parity with multi-doc) and nameservers (JSON array form). - Fresh-boot case: no discovery → no interfaces: mapping (only the '# -- Discovered interfaces:' debug comment from physical_links_info, asserted with indent-aware substring). - Plain physical NIC: emits - interface, addresses, routes (network/gateway pair). - floatingIP on controlplane → inline vip block on the discovered interface; never on worker. - VLAN: nested under parent's vlans: list with vlanId/addresses/ routes (legacy schema cannot represent a VLAN at top level). - Bond: top-level - interface: <bond>, nested bond: with interfaces (slaves), mode, and optional bondMaster fields. - vipLink override differs from default link → second interfaces[] entry with only vip:; inline vip on default suppressed (no double pinning of the same VIP). - vipLink override equal to default link → no extra entry. - Existing machine.network.interfaces[] in running MachineConfig → copied verbatim via toYaml (alphabetic key reorder is the YAML contract; chart trusts what the operator already declared). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add root_detection_test.go covering the pure-helper functions used during command-line argument parsing and project-root detection. These functions decide where talm finds chart files based on user- supplied -f/--file/-t/--template flags — directly observable CLI behaviour worth pinning as documentation. Coverage: - parseCommaSeparatedValues: comma-split with whitespace trim and empty-entry drop, single value, only-commas / only-whitespace edge. - parseFlagFromArgs: short and long form, space-separated and equal-sign forms, comma-separated values, absent flag, flag followed by another flag (no value), first-occurrence-wins semantics, both -f/--file and -t/--template flag pairs. - ResolveSecretsPath: empty defaults to secrets.yaml, relative paths anchored to Config.RootDir, absolute returned verbatim. - ExpandFilePaths: file passes through, directory expanded recursively to .yaml/.yml children (non-YAML siblings ignored), empty directory is an explicit error (operator typo guard), non-existent path propagated as-is so callers produce precise downstream errors. - isValidPreset: exact-match-only (case sensitive, no fuzzy match). - fileExists: thin os.Stat wrapper, true/false only. - filesDiffer: missing file differs from any content; equal content does not differ; changed content differs (drives the conditional 'do you want to overwrite?' prompt during talm init --update). Coverage rises from 16.8% to 20.8% on pkg/commands. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…ntract Add contract_test.go raising pkg/modeline coverage from 40.9% to 90.9%. The modeline is the first line of every per-node values file talm consumes — '# talm: nodes=[...], endpoints=[...], templates=[...]'. The format is contractual: editors highlight it, talm parses it on every apply, GenerateModeline writes it. Round-trip stability is the contract. Coverage: - ParseModeline rejects without '# talm: ' prefix (variants tested: empty, foreign comment, vim modeline, missing colon, missing space before/after talm:). - TrimSpace tolerance: indented modelines accepted (operators may indent for readability). - Malformed key-value rejected with precise errors (missing equals, non-JSON value, JSON value not an array, empty value). - Canonical separator '\, ' (comma + space) — matches GenerateModeline output, ensures generated modelines parse back. - Empty JSON arrays are valid (express 'no nodes' without dropping the key, useful for round-trip tooling). - ReadAndParseModeline: only the first line is read (subsequent lines are YAML the engine consumes); missing file error mentions path; empty file surfaces 'empty' substring; non-modeline first line returns the parse error verbatim. - GenerateModeline round-trip: all-populated, all-empty, partial, special characters in paths/URLs. - Generated modeline starts with '# talm: ' and emits keys in fixed order (nodes, endpoints, templates) for editor tooling. GenerateModeline's remaining 27% are unreachable json.Marshal error paths on []string inputs (json.Marshal cannot fail there). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_unix_test.go documenting operator-observable contracts that complement existing secureperm_unix_test.go. The package writes secrets material (age private keys, secrets.yaml, talosconfig, kubeconfig) atomically with mode 0o600 — losing one of these costs a cluster PKI reissue, so the contract surface is worth pinning explicitly. Coverage: - WriteFile bytes integrity: NUL, high-bit, newlines round-trip unchanged (the helper is a byte-pipe, not a serializer). - WriteFile empty payload: zero-length file with mode 0o600. - No tmp leftover on success: the .secureperm-* sibling is renamed over the target, never left behind cluttering the project root. - No tmp leftover on failure (non-existent parent dir): cleanup contract holds even when tmp creation fails before any tmp exists. - Idempotency: three repeat writes leave one file at mode 0o600 with the expected payload (atomic overwrite path stable on existing target). - LockDown on missing file: returns os.IsNotExist error (no silent skip — silently skipping would leave secrets world-readable). - LockDown idempotency: file already at 0o600 stays at 0o600. Coverage stays at 73.1% — these are contract documentation tests, not new code paths. Remaining 27% are error returns inside WriteFile (chmod / write / sync / close / rename) that require kernel-level fault injection to exercise. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_test.go raising pkg/age coverage from 3.5% to 62.9%. The package implements 'talm init --encrypt' / '--decrypt' flows: generate an age X25519 keypair, persist to talm.key (mode 0600), encrypt/decrypt secrets.yaml round-trip, support key rotation. Encryption is per-string-value with an 'ENC[AGE,data:<base64>]' envelope so encrypted files remain diffable in git. Coverage: - GenerateKey on empty dir produces age keygen layout: '# created:' comment, '# public key:' comment, AGE-SECRET-KEY-1 line. - GenerateKey is idempotent (load-or-create): a second call returns created=false and the same identity. - LoadKey accepts both age keygen format (with comments) and the legacy plain format (just the secret-key line) — backward-compat. - LoadKey errors precisely when the file is malformed (no AGE-SECRET-KEY marker) or missing. - GetPublicKey returns a string starting with 'age1'. - GetPublicKeyFromFile prefers the comment line and falls back to LoadKey when the comment is absent. - EncryptSecretsFile/DecryptSecretsFile round-trip integrity. - Encrypted file uses the ENC[AGE,data:...] envelope on each string value, keys remain plaintext (diffability), and the original plaintext does NOT leak into the encrypted output. - Incremental re-encryption: re-encrypting unchanged plaintext produces byte-identical ciphertext (so unchanged secrets stay byte-stable in git history). - Localized diff: changing one value re-encrypts only that key's envelope; siblings stay byte-stable. - RotateKeys preserves round-trip integrity. NOTE: the function contains a known bug (calls GenerateKey, which is load-or-create, so the on-disk key does not actually rotate). The test pins the minimum integrity guarantee and explicitly documents the bug without asserting on the broken side, to avoid locking in the bug as a contract. - EncryptYAMLFile/DecryptYAMLFile: generic file-pair version round-trips for arbitrary file names (kubeconfig, etc.). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_root_detection_test.go pinning the rules talm uses to
decide which directory is 'the project'. Walking up from a file path
or CWD until both Chart.yaml AND a secrets file (secrets.yaml or
secrets.encrypted.yaml) are present.
Coverage:
- DetectProjectRoot: direct match (Chart.yaml + secrets.yaml in same
dir), encrypted variant accepted as substitute for plain secrets,
walking up from a deep sub-directory, no-match returns empty
string with no error (callers fall through to other strategies),
Chart.yaml alone is NOT enough (rules out random helm charts on
disk being mistaken for talm projects).
- DetectProjectRootForFile: derives directory from file path then
delegates to DetectProjectRoot.
- ValidateAndDetectRootsForFiles: empty input returns ('', nil);
same-root files return that root; files spanning two roots is an
explicit error (talm refuses inconsistent applies); orphan file
(no markers up the tree) errors with the file name.
- DetectRootForTemplate: behavioral alias of
DetectProjectRootForFile (pin equivalence so future divergence is
intentional).
Coverage rises from 20.8% to 23.4% on pkg/commands.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add a 'coverage' job to .github/workflows/pr.yml that runs go test -coverprofile and surfaces a per-package summary in the GitHub Actions step-summary block. Profile is also uploaded as an artifact (14-day retention) so reviewers can download and explore. The job is REPORT-ONLY — it does NOT gate merges. Rationale: a threshold gate would force every PR to also touch tests, which penalises small focused PRs (a one-line bugfix should not need a synthetic test to clear an arbitrary number). Reviewers eyeball the summary; sustained drift gets addressed in dedicated coverage commits like this branch. The summary uses an awk one-liner over 'go tool cover -func' output to collapse per-function coverage into per-package averages. The overall total comes from cover's last line. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_loadvalues_test.go covering the values-aggregation pipeline that backs every 'talm template' / 'talm apply' invocation. loadValues mirrors Helm's flag precedence (-f / --set / --set-string / --set-file / --set-json / --set-literal); mergeMaps performs the deep merge. Coverage: - mergeMaps: empty overlay preserves base, top-level scalar overwrite, nested recursive merge keeping siblings, type-mismatch overwrite (map replaces scalar and vice versa, no auto-promotion), slice-replaces-not-appends contract (lists are replaced wholesale, matching Helm's --set semantics — operators relying on this for arrays of subnets/SANs must restate every element). - loadValues: -f file ordering precedence (later wins on shared keys), --set-json merges JSON object onto tree (numbers as float64), malformed JSON errors precisely, --set with dotted keys builds nested maps (strvals contract), --set-string forces string type regardless of numeric/bool appearance (required for Talos sysctls), --set-file reads file content and feeds <path>=<content> through strvals (path becomes the key when dot-free), missing --set-file path errors, missing -f file errors, malformed YAML in -f errors with file name, empty Options returns non-nil empty map, --set wins over -f, --set-string wins over --set (loadValues pipeline order). Coverage: pkg/engine 54.4% -> 59.6%. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_render_test.go covering the entry point that backs
'talm template' and 'talm apply'. Render glues chart load + values
aggregation + helm engine + applyPatchesAndRenderConfig into a
single byte stream; FullConfigProcess / InitializeConfigBundle /
SerializeConfiguration are the trio that reload the bundle once
machine type / cluster name / endpoint are known from patches.
Coverage:
- Render error surfaces:
* empty TemplateFiles -> mentions --file/--template
* missing template -> names the template
* bad chart Root -> chart-load error
* bad ValueFile -> loadValues error propagated
* malformed TalosVersion -> fast-fail BEFORE chart load (pin
order so a refactor that defers validation surfaces here)
- Render happy path: minimal chart with one worker template ->
bytes contain machine: and type: worker.
- --set values reach rendered template (end-to-end values pipeline).
- Render with Offline=true skips the multi-node check (the online-
only FailIfMultiNodes guard).
- InitializeConfigBundle: empty Options returns usable bundle with
ControlPlaneCfg + WorkerCfg; bad TalosVersion errors; missing
WithSecrets path errors with 'secrets bundle' substring.
- SerializeConfiguration: controlplane and worker outputs differ
(cluster section presence) and contain expected 'type:' lines.
- FullConfigProcess: no patches uses bundle default (controlplane);
patch with type:controlplane is detected; malformed patch errors
via LoadPatches; bad TalosVersion errors via InitializeConfigBundle.
Coverage: pkg/engine 59.6% -> 76.6%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
…ment
Add contract_chart_gitignore_test.go pinning two user-facing helpers
that ship with every 'talm init' invocation: readChartYamlPreset
(determines the active preset from Chart.yaml dependencies) and
writeGitignoreFile (manages .gitignore so a fresh 'git add .' does
not commit private cluster material).
Coverage:
- readChartYamlPreset:
* Picks first non-talm dependency as the preset (talm itself is
the always-present library chart, not a preset).
* Order in Chart.yaml matters — first non-talm wins. Pin so a
refactor returning the LAST entry surfaces here.
* No preset (talm-only deps) errors with 'preset not found'.
* Missing Chart.yaml errors mentioning the file.
* Malformed YAML errors (without this guard, unmarshal returns
nil deps and the operator gets a misleading 'preset not found'
instead of 'malformed Chart.yaml').
- writeGitignoreFile:
* Creates .gitignore from scratch with the four secrets-bearing
files: secrets.yaml, talosconfig, talm.key, kubeconfig.
* When Config.GlobalOptions.Kubeconfig is an absolute path, only
the base name lands in .gitignore (paths there are repo-relative
— absolute paths would be useless).
* Existing operator entries are preserved; the function appends
missing required entries, never rewrites the file.
* Idempotent: when all required entries are already present, no
write occurs (mtime stable across repeat 'talm init' invocations).
* Tolerant of annotated entries: 'secrets.yaml # never commit'
and 'talosconfig#TODO' both count as present, no duplicate
append.
Coverage: pkg/commands 23.4% -> 26.3%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_helpers_test.go for the pure helpers across pkg/commands that do not require a live Talos client. Each pin captures operator-observable behaviour exposed via talm CLI flows. Coverage: - normalizeEndpoint: every variant collapses to canonical https://<host>:6443 (host plus optional protocol/port stripped, port hardcoded to 6443). Includes IPv6-in-brackets and explicit-port forms. - updateKubeconfigServer: rewrites every cluster's server field, no-op when already normalised (mtime stable), missing file errors. - addToGitignore: creates .gitignore on first call, appends new entries on top of existing operator-supplied content, idempotent on repeat with same entry, treats path-prefix matches as already- present (existing 'dist' covers a request to add 'dist'). - getClusterNameFromChart: reads top-level Chart.yaml name (NOT dependencies — distinct from readChartYamlPreset). Missing or malformed Chart.yaml returns empty string by contract (caller chains a fallback default). - updateFileWithConfirmation no-prompt happy paths: creates new file with parent directories materialised; skips silently when target exists with byte-identical content (mtime stable). The prompt path requires stdin and is tested separately. Coverage: pkg/commands 26.3% -> 29.7%. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_extractresource_test.go covering the helper that
projects a COSI resource (as returned by Talos's gRPC
ResourceService) into the plain-data shape that helm template's
lookup() function consumes — {metadata: {...}, spec: <yaml-decoded>}.
Coverage:
- Metadata projection: namespace, type, id, version, phase, owner
surfaced as flat string-valued map. The chart helpers reference
these keys verbatim — renaming any one would break every helper
that does lookup().metadata.X.
- Spec is yaml-decoded: scalar strings, ints, and nested maps all
round-trip with their Go-typed values intact.
- Empty yaml spec returns nil at .spec, no error (yaml.Unmarshal('')
yields nil). Chart helpers handle missing-spec via standard 'with'
/ 'if .spec' patterns.
- Malformed yaml surfaces a clean error so a corrupted resource on
the wire fails the chart render explicitly.
Test fixture uses cosiproto.Unmarshal() to construct a real
*protobuf.Resource — same code path Talos's COSI client uses on the
client side, so the test exercises the actual unexported-yaml-field
reflection machinery that readUnexportedField wraps.
Coverage: pkg/engine 76.6% -> 79.0%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_internals_test.go covering edge cases of the
incremental encryption machinery (mergeAndEncryptYAMLValues +
decryptYAMLValuesString) that the round-trip pin in contract_test.go
does not touch.
Coverage:
- decryptYAMLValuesString:
* envelope (ENC[AGE,data:...]) round-trip back to plaintext.
* Strings without the envelope pass through verbatim (lets the
merge logic feed both raw and encrypted values through one
helper without a type switch at the call site).
* Corrupted base64 inside an envelope errors precisely.
- mergeAndEncryptYAMLValues (through EncryptSecretsFile):
* New key added to plain secrets.yaml gets encrypted on the next
round; existing keys keep byte-stable ciphertext.
* Deeply-nested change re-encrypts only the changed leaf;
siblings up and down the tree stay byte-stable.
* Type change at a key (scalar -> map) falls back to full
re-encrypt of that branch — the chart's incremental rule
degrades gracefully when the structure shifts.
* Slice length change re-encrypts the full list (per-element
merge requires stable index mapping, which a length change
invalidates).
* Slice same-length value change re-encrypts only the changed
element; siblings byte-stable.
Coverage: pkg/age 62.9% -> 69.5%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from gemini-code-assist on pkg/engine/contract_network_multidoc_test.go:339: the previous comment claimed the test picked a VIP that did NOT match any discovery address, contradicting both the trailing comment on line 341 and the test body — which intentionally sets the VIP to the same host as the discovered address so the strip path actually fires. Comment rewritten to describe the real intent. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from coderabbitai on pkg/engine/contract_cluster_test.go:227: the Contract: comment listed 'discovery' in the controlplane-only block enumeration but the test body lacked the assertion. Add assertNotContains(t, out, "discovery:") so the documented contract is also enforced executable. Signed-off-by: Aleksei Sviridkin <f@lex.la>
…gitignore tests Address review feedback from coderabbitai on pkg/commands/contract_chart_gitignore_test.go:235 (and lines 246, 282): three writeGitignoreFile tests unconditionally reset Config.GlobalOptions.Kubeconfig to '' on cleanup, which would clobber any non-empty value an earlier test had set. Capture the original value into a local at test start and restore it from cleanup, matching the pattern already in TestContract_WriteGitignoreFile_CreatesWithRequiredEntries and TestContract_WriteGitignoreFile_KubeconfigBaseNameOnly. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from coderabbitai on pkg/age/contract_test.go:357,366: the test assigned the result of os.ReadFile to first/second with the error discarded, so a failed read would manifest as the misleading 'could not isolate a's ciphertext line' assertion later instead of the real I/O error. Add explicit error checks with t.Fatal. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from coderabbitai on pkg/commands/contract_helpers_test.go:61: the IPv6 test case pinned a malformed URL (https://2001:db8::1:6443 — no brackets, not URL-parseable). Keep the case but document it as a known-bug pin via FIXME(#155); when the fix lands the expected value becomes https://[2001:db8::1]:6443 and the test starts asserting the canonical bracketed form. Signed-off-by: Aleksei Sviridkin <f@lex.la>
…racts
Add contract_root_dispatch_test.go pinning the layer that decides
which directory becomes Config.RootDir at command-startup time
(file flag -> template flag -> CWD), the talosconfig-path resolution,
and the in-memory kubeconfig endpoint rewriter used by rotate-CA.
Coverage:
- getFlagValues: non-persistent and persistent StringSlice flags,
absent flag returns non-nil empty slice (so callers can range
without a guard), registered-but-unset returns empty.
- detectRootFromFiles / detectRootFromTemplates: empty input yields
('', nil) so callers fall through; positive case under a real
project root resolves to that root's abs path.
- detectRootFromCWD walks up from CWD; macOS /var/folders symlink
resolution handled via filepath.EvalSymlinks fallback.
- checkRootConflict: no-op when --root not explicit, no error when
explicit matches detected, error names BOTH paths on conflict
(guides operator to either drop --root or move files).
- DetectAndSetRoot: -f flag wins over CWD; files in two different
roots is an explicit error; no flags + no markers leaves
Config.RootDir untouched.
- DetectAndSetRootFromFiles: happy-path file under root sets
Config.RootDir; empty input falls back to CWD detection;
--root explicit + files in a different root errors with
'conflicting' substring.
- EnsureTalosconfigPath: no-op when --talosconfig is explicit;
defaults to <RootDir>/talosconfig when nothing else set;
Chart.yaml-supplied relative path anchored to RootDir; absolute
path preserved as-is.
- updateKubeconfigEndpoint: rewrites every cluster's server: to
https://<host>:6443 (drops protocol/port from input, hardcodes
6443); malformed bytes surface parse error.
Each test that mutates package-level Config / GlobalArgs uses
withConfigSnapshot to capture and restore prior state, so tests
can run in any order without leaking.
Coverage: pkg/commands 29.7% -> 35.0%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add contract_template_test.go pinning the talm template flow above
engine.Render: resolveEngineTemplatePaths converts user-supplied
paths into the forward-slash relative form helm engine indexes by;
generateOutput composes the modeline + AUTOGENERATED warning
banner + rendered config bytes; template(args) prints the result
to stdout.
Coverage:
- resolveEngineTemplatePaths:
* absolute path inside root is returned as forward-slash relative
(helm engine map keys use forward slashes regardless of OS).
* relative-from-CWD that lands inside root is returned relative.
* outside-root path with matching <root>/templates/<basename>
falls back to the templates/<basename> form (documented
fallback so an operator running from a sibling directory still
hits the canonical location).
* outside-root path with no fallback returns the input
normalized through forward slashes.
* empty input yields a non-nil zero-length slice.
- generateOutput:
* happy path composes three sections in order — modeline,
AUTOGENERATED warning, engine-rendered body — separated by
newlines. The modeline is the first line so subsequent talm
invocations against the file pick up nodes/endpoints/templates
without explicit flags.
* missing template surfaces 'failed to render templates' wrap.
* empty templates list errors via engine.Render's 'templates are
not set' guard.
- template(args): prints the result of generateOutput to stdout
unchanged (modeline header + warning + body); propagates the
wrapped error from generateOutput without double-wrapping.
Helpers:
- withTemplateFlagsSnapshot saves and restores templateCmdFlags +
GlobalArgs.{Nodes,Endpoints} + Config.RootDir so tests can run
in any order.
- makeMinimalChart writes a chart fixture with Chart.yaml,
values.yaml, templates/config.yaml emitting a worker machine
config patch, plus a real serialized secrets bundle so
engine.Render's bundle.NewBundle path completes.
- loadSharedSecretsYAML generates the secrets bundle once per
test process via sync.Once — secrets.NewBundle populates a full
PKI tree (~half a second), generating it per-test would dominate
suite runtime.
- captureStdout redirects os.Stdout to a pipe for the duration of
a function, returns whatever was printed.
Coverage: pkg/commands 35.0% -> 38.0%.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address Windows CI failures introduced by previous contract-test
commits. None of the contracts themselves were wrong — only the
test expectations baked in POSIX assumptions (forward-slash paths,
LF-only multi-doc separators, and strvals.ParseInto behaviour on
drive-letter paths).
Fixes:
- pkg/commands/root_detection_test.go: TestContract_ResolveSecretsPath
builds expected values via filepath.Join so the OS-native
separator matches on Windows.
- pkg/commands/contract_root_dispatch_test.go:
* TestContract_CheckRootConflict_ExplicitConflict: uses
filepath.Abs of the path arguments for the substring assertion
(Windows resolves /explicit/root to D:\explicit\root).
* TestContract_EnsureTalosconfigPath_*: builds expected via
filepath.Join.
- pkg/engine/contract_loadvalues_test.go:
TestContract_LoadValues_SetFileReadsContent skips on Windows.
strvals.ParseInto interprets ':' and '\\' in Windows temp
paths as separators, splitting the path into multiple keys.
The contract holds; the test fixture cannot be expressed
cross-platform.
- pkg/engine/contract_schema_test.go:
* TestContract_Schema_Versions112AndLaterRenderMultidoc matches
the literal '---' token instead of '\n---\n' so CRLF-rendered
output on Windows still satisfies the contract.
* TestContract_Schema_LegacyIsSingleDocument scans line-by-line
with TrimRight on \r so a CRLF '---' line is also caught.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Follow-up to the previous Windows-fix commit. The crossPlatformAbs
helper in contract_root_dispatch_test.go takes care of building an
absolute path that satisfies filepath.IsAbs on both POSIX and
Windows: filepath.Join("\", "etc", ...) on Windows yields a
backslash-rooted path that lacks a drive letter and is therefore
NOT IsAbs, so ResolveSecretsPath / EnsureTalosconfigPath wrongly
anchor it to RootDir. The helper prepends the volume of the
current working directory (typically C: on a CI runner) so the
final string is treated as absolute by the OS.
Sites updated:
- TestContract_CheckRootConflict_ExplicitConflict: uses
crossPlatformAbs for both explicit and detected paths.
- TestContract_EnsureTalosconfigPath_*: uses crossPlatformAbs for
Config.RootDir and the absolute talosconfig fixture.
- TestContract_ResolveSecretsPath/absolute_returned_verbatim:
uses crossPlatformAbs for the absolute /etc/secrets.yaml fixture.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
…priority Add four new tests covering the values.yaml-takes-precedence behaviour added in #148. The chart-side tests in pkg/engine/render_test.go exercise the template rendering path; this test pins the regenerate-talosconfig flow which reads the cluster name through getClusterNameFromChart in pkg/commands. Coverage: - ValuesYamlOverridesChartYaml: both files exist, values.yaml has a non-empty clusterName -> values.yaml wins. The talosconfig regenerate path picks up the operator's override instead of silently using Chart.Name. - EmptyValuesClusterNameFallsBack: values.yaml present but clusterName: "" (the shipped cozystack/generic default) falls through to Chart.yaml.name. Without this short-circuit a fresh install would resolve to empty and downstream callers would substitute placeholders silently. - AbsentValuesKeyFallsBack: values.yaml has no clusterName key at all (other shape) -> falls through to Chart.yaml. yaml.Unmarshal into the typed struct yields the zero string for a missing field; pin so the resolver treats it the same as an explicit empty. - MalformedValuesFallsBack: a syntax error in values.yaml does NOT poison the lookup; the function silently moves on to Chart.yaml. Operators with a half-edited values.yaml still get a usable regenerate path. The existing ReadsTopLevelName / MissingReturnsEmpty / MalformedReturnsEmpty tests are preserved; their comments updated to clarify they exercise the Chart.yaml fallback when no values.yaml override is in play. Coverage: pkg/commands 38.0% -> 38.2%. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
test/chart-contract
branch
from
0a34271 to
ba98494
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_helpers_test.go`:
- Around line 196-210: The current test
TestContract_AddToGitignore_PathPrefixMatch only checks exact-match idempotence;
add a case that verifies path-prefix behavior by writing "dist\n" to .gitignore,
calling addToGitignore("dist/"), and asserting the file still contains exactly
one "dist" entry. Update the test (or add a new subtest) to use setRoot and
t.TempDir, create gitignore with "dist\n", call addToGitignore("dist/"), then
read the file and assert strings.Count(..., "dist") == 1 to ensure no duplicate
entry is added for the path-prefix variant; reference the existing
TestContract_AddToGitignore_PathPrefixMatch and the addToGitignore function when
locating where to add this case.
🪄 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: 133677c3-9ae7-4fe6-a2cd-7070cbb32e7a
📒 Files selected for processing (20)
.github/workflows/pr.ymlpkg/age/contract_internals_test.gopkg/age/contract_test.gopkg/commands/contract_chart_gitignore_test.gopkg/commands/contract_helpers_test.gopkg/commands/contract_root_detection_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_test.gopkg/commands/root_detection_test.gopkg/engine/contract_cluster_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/contract_schema_test.gopkg/modeline/contract_test.gopkg/secureperm/contract_unix_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- .github/workflows/pr.yml
- pkg/engine/contract_extractresource_test.go
- pkg/engine/contract_schema_test.go
- pkg/modeline/contract_test.go
- pkg/secureperm/contract_unix_test.go
- pkg/commands/contract_template_test.go
- pkg/age/contract_test.go
- pkg/commands/contract_root_detection_test.go
- pkg/commands/contract_root_dispatch_test.go
- pkg/engine/contract_machine_test.go
- pkg/age/contract_internals_test.go
- pkg/commands/root_detection_test.go
- pkg/engine/contract_cluster_test.go
- pkg/engine/contract_network_legacy_test.go
- pkg/engine/contract_errors_test.go
- pkg/commands/contract_chart_gitignore_test.go
- pkg/engine/contract_network_multidoc_test.go
- pkg/engine/contract_loadvalues_test.go
- pkg/engine/contract_render_test.go
Comment on lines
+196
to
+210
| func TestContract_AddToGitignore_PathPrefixMatch(t *testing.T) { | ||
| dir := t.TempDir() | ||
| setRoot(t, dir) | ||
| gitignore := filepath.Join(dir, ".gitignore") | ||
| if err := os.WriteFile(gitignore, []byte("dist\n"), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := addToGitignore("dist"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| got, _ := os.ReadFile(gitignore) | ||
| if strings.Count(string(got), "dist") != 1 { | ||
| t.Errorf("expected exactly one 'dist' entry, got:\n%s", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test only covers exact match, not the path-prefix behavior described in the contract.
The comment at lines 191-195 promises that "existing dist matches a request to add dist/" (path-prefix awareness), but the test only verifies that adding "dist" when "dist" already exists is idempotent (exact match). To fully enforce the contract, add a test case where the file contains "dist" and the request is addToGitignore("dist/"), expecting no duplicate entry.
📋 Suggested additional test case
Add after line 210:
+// Contract: verify the reverse case — existing `dist` (no trailing
+// slash) should match a request to add `dist/` (with slash).
+func TestContract_AddToGitignore_PathPrefixMatch_WithSlash(t *testing.T) {
+ dir := t.TempDir()
+ setRoot(t, dir)
+ gitignore := filepath.Join(dir, ".gitignore")
+ if err := os.WriteFile(gitignore, []byte("dist\n"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ if err := addToGitignore("dist/"); err != nil {
+ t.Fatal(err)
+ }
+ got, _ := os.ReadFile(gitignore)
+ if strings.Count(string(got), "dist") != 1 {
+ t.Errorf("expected exactly one 'dist' entry (prefix match), got:\n%s", got)
+ }
+}🤖 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/contract_helpers_test.go` around lines 196 - 210, The current
test TestContract_AddToGitignore_PathPrefixMatch only checks exact-match
idempotence; add a case that verifies path-prefix behavior by writing "dist\n"
to .gitignore, calling addToGitignore("dist/"), and asserting the file still
contains exactly one "dist" entry. Update the test (or add a new subtest) to use
setRoot and t.TempDir, create gitignore with "dist\n", call
addToGitignore("dist/"), then read the file and assert strings.Count(...,
"dist") == 1 to ensure no duplicate entry is added for the path-prefix variant;
reference the existing TestContract_AddToGitignore_PathPrefixMatch and the
addToGitignore function when locating where to add this case.
IvanHunters
approved these changes
May 9, 2026
This was referenced May 10, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pin the current behaviour of the cozystack/generic charts and the core Go packages as a layer of contract documentation that lives alongside the code. Tests are written as the second documentation surface — each test starts with a
Contract:comment describing the user-visible guarantee being captured and why it matters; the body is the executable enforcement of that guarantee. When something drifts in a future change, the failing test names point directly at the broken cell of the matrix.Scope
Charts (
pkg/engine/contract_*_test.go)Roughly 95 contract tests covering the rendered shape of both shipped charts across the matrix
chart × schema × machineType:templateOptions.talosVersionroutes legacy vs multi-doc, including pre-release tags (v1.12.0-rc.1satisfies>=1.12.0-0).failandrequireddirective across both charts (endpoint required, advertisedSubnets+empty discovery, multi-doc legacy interfaces guard, bridge-as-gateway, VLAN no parent, VLAN no vlanID).Core packages
pkg/engine(54.4% → 79.0%): loadValues / mergeMaps Helm precedence and slice-replace-not-append, Render/FullConfigProcess/InitializeConfigBundle/SerializeConfiguration error surfaces and happy paths, extractResourceData metadata projection and yaml-spec round-trip.pkg/age(3.5% → 69.5%): key generation/loading (both age keygen format and legacy plain), envelopeENC[AGE,data:...]shape, secrets round-trip, incremental encryption (new keys, nested change locality, type change fallback, list length change full re-encrypt, same-length localised re-encrypt).pkg/modeline(40.9% → 90.9%): parse/read/generate round-trip stability, prefix discipline (# talm:), trim tolerance, key order in generated lines.pkg/commands(16.8% → 29.7%): CLI flag parsing, project-root detection (two-marker rule with secrets.yaml or secrets.encrypted.yaml), Chart.yaml preset detection, .gitignore management, endpoint normalisation, kubeconfig server rewrite, no-prompt file write paths.pkg/secureperm: atomic-write contract (mode 0600, no tmp leftover on success or failure, idempotency, bytes-integrity).CI
Adds a report-only
coveragejob to.github/workflows/pr.ymlthat runsgo test -coverprofile, prints a per-package summary in the GitHub Actions step-summary, and uploads the profile as an artifact. No threshold gate — the goal is visibility into drift, not forcing every PR to also touch tests.Coverage delta
pkg/agepkg/modelinepkg/enginepkg/commands90% on
pkg/commandsis reachable only with a fake Talos gRPC server (apply,WithClient*,cosiVersionReader, etc.). That is a separate project; this PR stops at what is testable without one.Bugs surfaced during this work
Filed as separate issues so this PR stays test-only:
pkg/age.RotateKeysdoes not actually rotate the on-disk key (security-relevant).pkg/engine.debugPhasepanics on empty patch in--debugoutput loop.golangci-lintconfig refresh and resulting cleanup backlog (extended scope).The contract test for
RotateKeysdeliberately does NOT pin the broken behaviour — it only asserts round-trip integrity and documents the bug in a comment, so the test can be tightened once the fix lands.Out of scope
Summary by CodeRabbit