refactor: tidy source, tests and docs

.golangci.yml

@@ -20,6 +20,10 @@ linters:
   settings:
     gocyclo:
       min-complexity: 15
+    staticcheck:
+      # ST1005 wants lower-case error strings; the CLI prints its errors verbatim as diagnostics,
+      # so they are capitalised sentences on purpose.
+      checks: ["all", "-ST1000", "-ST1003", "-ST1005", "-ST1016", "-ST1020", "-ST1021", "-ST1022"]
     gocritic:
       enabled-tags:
         - diagnostic

CLAUDE.md

@@ -53,7 +53,7 @@ make actionlint && make shellcheck
 go build -o tf-version-bump .
 ```
 
-**golangci-lint must match CI's version, currently v2.12** (see `.github/workflows/lint.yml`; `.golangci.yml` is `version: "2"` schema). It enables a curated linter set rather than the defaults, sets `gocyclo` min-complexity to 15, and lints test files too. Bump the version here when the workflow pins a new one.
+**golangci-lint must match CI's version, currently v2.12** (see `.github/workflows/lint.yml`; `.golangci.yml` is `version: "2"` schema). It enables a curated linter set rather than the defaults, sets `gocyclo` min-complexity to 15, lints test files too, and disables staticcheck's ST1005 because the CLI prints its `Error: ...` strings verbatim. Bump the version here when the workflow pins a new one.
 
 ```bash
 curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.12
@@ -87,7 +87,7 @@ Note this means the tool cannot protect a user who `cd`s into `.terraform` and g
 
 `main()` → `validateOperationModes` → either standalone config validation or `findMatchingFiles` → `runAuditMode` (`-audit-file`: `buildAudit`, never writes Terraform files) or `runUpdateMode` → `runConfigFileMode` (YAML) / `runCLIMode` (one direct operation). Each update mode dispatches to one of three update paths: `processFiles` → `applyModuleVersion` for modules, `processTerraformVersion` → `updateTerraformVersionWithCount`, or `processProviderVersion` → `updateProviderVersionWithCount`.
 
-`processFiles` parses each file once with `readTerraformFile`, then applies the module entries to it in YAML order through `applyModuleVersion`, which writes after each change unless in a dry run. Entries for one source therefore chain — an entry whose `from` lists an earlier entry's target moves the block again in the same run — and dry runs and checks report the same chain because the parsed file carries every earlier change. A file that cannot be read or parsed counts once per entry; each failed write counts once, and the file is read again for later entries, a read that a file whose write kept its backup refuses. `updateModuleVersionWithCount` is the single-entry wrapper most module tests call.
+`processFiles` parses each file once with `readTerraformFile`, then applies the module entries to it in YAML order through `applyModuleVersion`, which writes after each change unless in a dry run. Entries for one source therefore chain (an entry whose `from` lists an earlier entry's target moves the block again in the same run), and dry runs and checks report the same chain because the parsed file carries every earlier change. A file that cannot be read or parsed counts once per entry; each failed write counts once, and the file is read again for later entries, a read that a file whose write kept its backup refuses. `updateModuleVersionWithCount` is the single-entry wrapper most module tests call.
 
 `applyModuleVersion` bundles its many parameters into a `moduleUpdateOptions` struct and delegates per-block work to `updateModuleBlockResult` → `shouldSkipModuleVersion`. Add new per-module filtering there rather than growing the parameter list.
 
@@ -158,9 +158,11 @@ File-level errors log and continue to the next file; bad flags, invalid globs, n
 
 Success is prefixed `✓`; dry-run lines use `→` with the verb "Would update". User-facing values are wrapped with `quote(s, format)`: `'vpc'` for `text` output, `` `vpc` `` for `md`. Thread `outputFormat` through rather than hardcoding quotes.
 
-Every summary ends with `failureNote`, which reports `N update(s) failed; see the errors on stderr` whenever the mode's *update* error count is non-zero, so a summary of what succeeded is never mistaken for the whole run. The note counts file-per-entry attempts, the same scale as the summaries and not the block counts `-report-file` writes, and every counted failure is logged where it happens, so the count is also the number of diagnostics. It names stderr rather than saying "above" because the summary goes to stdout, and a failure after the summary, such as a report that cannot be written, reaches only the fatal line. `skipNote` sits beside it and reports `N module(s) skipped` or `N provider(s) skipped`, `see the warnings on stderr`, counting only the entries a run matched and would not change — a module with no `version` attribute without `-force-add`, `-force-add` on a source that cannot take a version, an object-syntax provider with no `version` argument, which an update does not add even under `-force-add`, and a provider given as a bare version string, which updates do not change either. The warning names which of those it found, because a provider pinned by a version string is not unpinned; the run simply will not move it. Provider block syntax cannot be skipped, because a missing version there is simply set, and a Terraform `required_version` is set whether or not one is already there. A module excluded by `ignore_modules`, `from` or `ignore_versions`, a local module and a module already at the target version were all skipped exactly as asked, so counting them would make the line noise; the count is threaded from `updateModuleBlockResult` through `applyModuleVersion` and `processFiles` as block indexes rather than a total, because entries for one source are applied to the same file in turn and the line speaks for modules, not for the attempts that reached them; exit codes are unchanged, because the updater cannot clear a skip on its own and a check that demanded one would loop. A run that updated nothing never prints its count line at all — no summary says `updated 0 file(s)`, in either tense — because a clean zero is a result the run cannot vouch for, and config mode says nothing about the config in that case. A skip alone does not suppress the line: both no-op messages name skipping as one of the causes they cannot tell apart, so the message explains and the note quantifies, because a failed run must not send the operator to the config when the files or their permissions are at fault. A config-mode run that did nothing reports it as already applied, skipped or matching nothing, pointing at `-audit-file` for which — "skipped" covers the filters and the missing-version warning, and dropping it would repeat the fault this wording exists to fix. Direct mode says the same thing in its own words and points at nothing, because `-audit-file` needs a config and `-verbose` lists only the modules a filter skipped. A config that asks for nothing never reaches either message: `runConfigFileMode` refuses it, as `-validate-config` does, through the shared `Config.declaresNoUpdates`. That refusal lives in the runner rather than in `loadResolvedConfig`, which audit mode shares and which must keep auditing such a config rather than failing the report automation that calls it.
+Every summary ends with two notes. `failureNote` reports `N update(s) failed; see the errors on stderr` whenever the mode's update error count is non-zero; it counts file-per-entry attempts, the same scale as the summaries rather than the block counts `-report-file` writes, and every counted failure is logged where it happens, so the count is also the number of diagnostics. `skipNote` reports `N module(s) skipped` or `N provider(s) skipped`, `see the warnings on stderr`, counting only the entries a run matched and would not change: a module with no `version` attribute without `-force-add`, `-force-add` on a source that cannot take a version, an object-syntax provider with no `version` argument (never added, even under `-force-add`) and a provider given as a bare version string (never changed). The warning names which it found. Provider block syntax and `required_version` cannot be skipped, because a missing value there is simply set. A module excluded by `ignore_modules`, `from` or `ignore_versions`, a local module and a module already at the target were skipped as asked and are not counted. Skips are threaded from `updateModuleBlockResult` up to `processFiles` as block indexes, not totals, because entries for one source hit the same file in turn and the line speaks for modules; exit codes ignore skips, because the updater cannot clear one and a check that demanded it would loop.
 
-`-report-file` is the machine-readable automation contract. It writes schema version 2 JSON with exact counts of unique Terraform, module, and provider blocks whose version values changed across the complete command. A failed run writes no report, and removes any report already at the destination when — and only when — it changed files, so a consumer that reads the file without checking the exit status fails loudly instead of acting on counts for a tree that has since changed. `runUpdateMode` decides that from the update total and `-dry-run`, and the publish failure runs the same epilogue, because a run whose files changed and whose counts never landed is the case most certain to need it. A run refused for its config, a run whose every file failed and a dry run all leave the tree as they found it, so they leave the report too. Audit mode never removes its destination, for the same reason: a failed audit changes nothing on disk and the previous audit is still a true snapshot. Keep human summaries and the report separate: existing summaries count source/file operations, while the report counts individual changed blocks. Dry-run reports contain zero counts because no file values changed.
+A run that updated nothing never prints `updated 0 file(s)`, in either tense, because a clean zero is a result the run cannot vouch for. With no failures it prints a no-op message that names every cause it cannot tell apart (nothing matched, already at the target, skipped); config mode points at `-audit-file` for which, and direct mode points at nothing, because `-audit-file` needs a config and `-verbose` lists only filter skips. With failures and no updates it prints only the notes, so a file-level fault is never blamed on the config. A config that asks for nothing never reaches either message: `runConfigFileMode` refuses it, as `-validate-config` does, through the shared `Config.declaresNoUpdates`. That refusal lives in the runner rather than in `loadResolvedConfig`, which audit mode shares and which must keep auditing such a config.
+
+`-report-file` is the machine-readable automation contract. It writes schema version 2 JSON with exact counts of unique Terraform, module, and provider blocks whose version values changed across the complete command. A failed run writes no report, and removes any report already at the destination when, and only when, it changed files (including a file a write left untrusted, and including a run whose report publish failed), so a consumer that reads the file without checking the exit status fails loudly instead of acting on stale counts. A run refused for its config, a run whose every file failed and a dry run leave the tree, and so the report, as they found them. Audit mode never removes its destination: a failed audit changes nothing on disk, so the previous audit is still true. Keep human summaries and the report separate: summaries count source/file operations, the report counts changed blocks. Dry-run reports contain zero counts because no file values changed.
 
 `-audit-file` is the read-only comparison contract. In config mode it writes schema version 1 JSON listing each configured Terraform, provider and module version value the selected files declare, its current and expected values, whether they already match and, for modules, the first filter that would skip an update. The audit and the updater share `moduleVersionFilter` and the `attributeHasStringValue` comparison, and `recordModuleBlock` judges a source's entries in YAML order against the version earlier entries would write, as `processFiles` applies them; keep the audit in step with any change to update filtering or ordering. Unlike the update modes, a selected file that cannot be read or parsed stops the audit: the command exits 1 and writes nothing, leaving any existing audit untouched.
 

audit.go

@@ -74,11 +74,11 @@ func buildAudit(files []string, config *Config) (*auditReport, error) {
 func parseAuditedFile(filename string) (*hclwrite.File, error) {
 	src, err := os.ReadFile(filename)
 	if err != nil {
-		return nil, fmt.Errorf("Error auditing %s: failed to read file: %w", filename, err) //nolint:staticcheck // User-facing CLI diagnostic.
+		return nil, fmt.Errorf("Error auditing %s: failed to read file: %w", filename, err)
 	}
 	file, diags := hclwrite.ParseConfig(src, filename, hcl.Pos{Line: 1, Column: 1})
 	if diags.HasErrors() {
-		return nil, fmt.Errorf("Error auditing %s: failed to parse HCL: %s", filename, diags.Error()) //nolint:staticcheck // User-facing CLI diagnostic.
+		return nil, fmt.Errorf("Error auditing %s: failed to parse HCL: %s", filename, diags.Error())
 	}
 	return file, nil
 }
@@ -116,9 +116,9 @@ func (audit *auditReport) recordRequiredProvider(filename string, requiredProvid
 }
 
 // recordModuleBlock records the block once for each config entry with an equal source. A run
-// applies those entries in order, so each entry is judged against the value the entries before it
-// leave: the block's own version until one of them would rewrite it. -audit-file rejects -force-add,
-// so no entry gives a block without a version one.
+// applies those entries in order, so each is judged against the value the entries before it
+// leave: the block's own version until one of them would rewrite it. -audit-file rejects
+// -force-add, so no entry gives a block without a version one.
 func (audit *auditReport) recordModuleBlock(filename string, block *hclwrite.Block, updates []ModuleUpdate) {
 	source, ok := moduleSourceValue(block)
 	if !ok {
@@ -127,7 +127,6 @@ func (audit *auditReport) recordModuleBlock(filename string, block *hclwrite.Blo
 	name := moduleBlockName(block)
 	versionAttribute := block.Body().GetAttribute("version")
 	var rewrittenVersion *string
-	// Index rather than copy: gocritic's hugeParam rejects passing a ModuleUpdate by value.
 	for i := range updates {
 		update := &updates[i]
 		if update.Source != source {
@@ -169,9 +168,8 @@ func moduleAuditSkipFor(name, source string, actual *string, update *ModuleUpdat
 	return nil
 }
 
-// auditedAttribute returns an attribute's value as written, without its quotes, and whether it
-// already evaluates to expected, which is the updater's no-op test. A missing attribute has no
-// value.
+// auditedAttribute returns an attribute's value as written and whether it already evaluates to
+// expected, which is the updater's no-op test. A missing attribute has no value.
 func auditedAttribute(attr *hclwrite.Attribute, expected string) (actual *string, matches bool) {
 	if attr == nil {
 		return nil, false
@@ -180,9 +178,9 @@ func auditedAttribute(attr *hclwrite.Attribute, expected string) (actual *string
 	return &value, attributeHasStringValue(attr, expected)
 }
 
-// runAuditMode writes the audit of files against the config. It validates the destination
-// before reading Terraform files, never writes them, and replaces an existing audit only once
-// every file has been audited.
+// runAuditMode writes the audit of files against the config. It validates the destination before
+// reading Terraform files, never writes them, and replaces an existing audit only once every file
+// has been audited.
 func runAuditMode(files []string, flags *cliFlags) error {
 	config, err := loadResolvedConfig(flags.configFile, flags.branch)
 	if err != nil {
@@ -196,12 +194,12 @@ func runAuditMode(files []string, flags *cliFlags) error {
 	audit, err := buildAudit(files, config)
 	if err != nil {
 		if discardErr := prepared.discard(); discardErr != nil {
-			err = fmt.Errorf("%w; failed to discard prepared audit: %v", err, discardErr)
+			err = fmt.Errorf("%w; failed to discard prepared audit: %w", err, discardErr)
 		}
 		return err
 	}
 	if err := prepared.publish(audit); err != nil {
-		return fmt.Errorf("Error writing audit: %w", err) //nolint:staticcheck // User-facing CLI diagnostic.
+		return fmt.Errorf("Error writing audit: %w", err)
 	}
 	fmt.Printf("✓ Wrote audit to %s\n", quote(flags.auditFile, flags.output))
 	return nil
@@ -210,15 +208,14 @@ func runAuditMode(files []string, flags *cliFlags) error {
 // auditedObjectVersion reads the version item of an object-syntax provider declaration.
 func auditedObjectVersion(objExpr *hclsyntax.ObjectConsExpr, expression []byte, expected string) (actual *string, matches bool) {
 	for _, item := range objExpr.Items {
-		keyName, ok := providerObjectItemKey(item)
-		if !ok || keyName != "version" {
+		if key, ok := providerObjectItemKey(item); !ok || key != "version" {
 			continue
 		}
-		valueRange := item.ValueExpr.Range()
-		if valueRange.Start.Byte < 0 || valueRange.End.Byte > len(expression) || valueRange.Start.Byte > valueRange.End.Byte {
+		start, end, ok := objectItemValueRange(item, expression)
+		if !ok {
 			continue
 		}
-		valueExpression := expression[valueRange.Start.Byte:valueRange.End.Byte]
+		valueExpression := expression[start:end]
 		value := trimQuotes(strings.TrimSpace(string(valueExpression)))
 		return &value, expressionHasStringValue(valueExpression, expected)
 	}