Used claude to fix the lint errors by chrisghill · Pull Request #219 · massdriver-cloud/mass

chrisghill · 2026-03-13T19:52:35Z

No description provided.

Copilot

Pull request overview

This PR primarily addresses golangci-lint findings across the Mass CLI by adding/adjusting GoDoc, tightening file/dir permissions, and making small refactors to satisfy specific linters (unused vars, error handling, type assertions, etc.).

Changes:

Add/normalize package/type/function documentation comments across many packages to satisfy golint-style doc requirements.
Refactor small logic patterns to satisfy linters (type assertion safety, switch style, error variable shadowing, ignoring returned values explicitly).
Adjust filesystem permission modes in multiple places and tweak devcontainer + golangci config for consistent tooling.

Reviewed changes

Copilot reviewed 130 out of 131 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
pkg/version/version.go	Add GoDoc for package/constants/functions
pkg/tui/teahelper/main.go	Add GoDoc; helper function docs
pkg/tui/components/artifacttable/model_test.go	Clarify nolint comment
pkg/tui/components/artifacttable/model.go	Add GoDoc; clarify nolint rationale
pkg/tui/components/artifacttable/keys.go	Add package/type GoDoc
pkg/tui/components/artdeftable/model_test.go	Clarify nolint comment
pkg/tui/components/artdeftable/model.go	Replace header comment w/ package GoDoc; add docs
pkg/tui/components/artdeftable/keys.go	Add GoDoc
pkg/templates/templates.go	Add GoDoc for exported items
pkg/templates/file_manager.go	Tighten perms; simplify conditional
pkg/server/version/version.go	Add GoDoc
pkg/server/server.go	Add GoDoc; context-aware slog; ListenConfig; perms
pkg/server/bundle/params.go	Exported comment capitalization fix
pkg/server/bundle/bundle.go	Add GoDoc for package/handler/methods
pkg/proxy/proxy.go	Add GoDoc
pkg/provisioners/types.go	Switch refactor; add NoopProvisioner docs/behavior
pkg/provisioners/opentofu_test.go	Remove fmt usage in test file reads
pkg/provisioners/opentofu.go	Safer type assertions; best-effort cleanup; tighten perms
pkg/provisioners/helm_test.go	Remove fmt usage in test file reads
pkg/provisioners/helm.go	Add GoDoc
pkg/provisioners/find.go	Add GoDoc; safer type assertions; nolint annotations
pkg/provisioners/bicep_test.go	Remove fmt usage in test file reads
pkg/provisioners/bicep.go	Safer type assertions; tighten perms; add GoDoc
pkg/prettylogs/main.go	Add GoDoc
pkg/params/params.go	Use errors.New; add GoDoc; import cleanup
pkg/params/merge.go	Add package/function GoDoc; clarify nolint comment
pkg/mockfilesystem/main.go	Replace block comment; path building tweaks; perms change
pkg/jsonschema/loader.go	Add GoDoc; context-based HTTP request; var rename
pkg/gqlmock/gqlmock.go	Add GoDoc; comment cleanup
pkg/files/files_test.go	testpackage nolint; add cyclo nolint; safer type assertion
pkg/files/files.go	Add GoDoc; introduce UserRW const; error var rename
pkg/definition/read.go	Add GoDoc; drop unused ctx; use errors.New for static errors
pkg/definition/publish_test.go	Ignore write result explicitly
pkg/definition/publish.go	Add GoDoc
pkg/definition/get.go	Add GoDoc
pkg/definition/dereference_test.go	Switch-based test branch logic
pkg/definition/dereference.go	Add GoDoc; errors.New for static errors; range-int loop
pkg/definition/delete.go	Add GoDoc
pkg/definition/build.go	Add package GoDoc
pkg/debuglog/debuglog.go	Convert file perms to 0600; add GoDoc
pkg/commands/project/export.go	Add GoDoc; rename projectID var
pkg/commands/preview/new_test.go	Clarify nolint comment
pkg/commands/preview/new.go	Add GoDoc
pkg/commands/preview/model.go	Add GoDoc; safer type assertion path
pkg/commands/preview/deploy_test.go	Replace ignored assertion w/ explicit checks
pkg/commands/preview/deploy.go	Improve function comment
pkg/commands/preview/decommission.go	Add package GoDoc
pkg/commands/pkg/reset.go	Improve function comment
pkg/commands/pkg/patch_test.go	Add safe type assertion on params string
pkg/commands/pkg/patch.go	Improve function comment
pkg/commands/pkg/export_test.go	Update expected no-state behavior; use require for errors
pkg/commands/pkg/export.go	Add ErrNoState; decode error handling; string concat; docs
pkg/commands/pkg/deploy_test.go	Add nolint for reassign in tests
pkg/commands/pkg/deploy.go	Add GoDoc for exported vars and RunDeploy
pkg/commands/pkg/configure_test.go	Add safe type assertion on params string
pkg/commands/pkg/configure.go	Add package GoDoc; improve function comment
pkg/commands/image/push_test.go	Remove log output capture boilerplate
pkg/commands/image/push.go	Group consts; add GoDoc
pkg/commands/image/image.go	Add GoDoc for types
pkg/commands/image/docker_client.go	Add package/type/method GoDoc
pkg/commands/environment/export.go	Add GoDoc; use errors.New for static errors; rename vars
pkg/commands/bundle/pull.go	Add GoDoc; remove blank line
pkg/commands/bundle/publish_test.go	testpackage nolint
pkg/commands/bundle/publish.go	Add GoDoc; rename orgID var; simplify conditional
pkg/commands/bundle/new_test.go	Formatting/alignment cleanup
pkg/commands/bundle/new.go	Tighten perms; use strings.Builder for connections block
pkg/commands/bundle/lint.go	Add GoDoc; switch style
pkg/commands/bundle/import.go	Add GoDoc; safer type assertions; nolint clarifications
pkg/commands/bundle/build.go	Add package GoDoc; function GoDoc
pkg/commands/artifact/update.go	Add GoDoc
pkg/commands/artifact/import.go	Add package GoDoc; add function GoDoc
pkg/cli/table.go	Add package GoDoc
pkg/bundle/write_schemas.go	Tighten perms; minor string formatting change; GoDoc
pkg/bundle/transformations_test.go	Remove blank line
pkg/bundle/transformations.go	Add GoDoc
pkg/bundle/pull.go	Add GoDoc
pkg/bundle/publish_test.go	Range simplification
pkg/bundle/publish.go	Add GoDoc
pkg/bundle/prompt_test.go	testpackage nolint
pkg/bundle/prompt.go	Add GoDoc; safer type assertions
pkg/bundle/lint_test.go	Ignore write result; assert.Len; indentation fixes
pkg/bundle/lint.go	Remove LintIssue.Error; add GoDoc; refactor message building
pkg/bundle/environment_variables.go	Add GoDoc
pkg/bundle/dereference.go	Add GoDoc
pkg/bundle/combine.go	Add safer type assertions for maps.Copy/append
pkg/bundle/bundle.go	Add GoDoc; add mapstructure tags; const grouping
pkg/bundle/build.go	Add package GoDoc; add method GoDoc
pkg/artifact/types.go	Add GoDoc
pkg/artifact/import_prompt.go	Add package/type/function GoDoc
pkg/api/urls.go	Simplify server AppURL fallback logic
pkg/api/server.go	Add mapstructure tags; add GoDoc
pkg/api/scalars/json.go	Add GoDoc for UnmarshalJSON
pkg/api/scalars/cursor.go	Add package GoDoc
pkg/api/project.go	Add mapstructure tags; build errors via strings.Builder; GoDoc
pkg/api/preview_environment.go	Add GoDoc
pkg/api/preview_config.go	Add GoDoc for types/method
pkg/api/package.go	Add mapstructure tags; add GoDoc
pkg/api/oci.go	Add mapstructure tags; add GoDoc
pkg/api/manifest.go	Add mapstructure tags; GoDoc; improve error building
pkg/api/error.go	Add GoDoc
pkg/api/environment_test.go	Rename vars to ID form (but test name remains “ByPackage”)
pkg/api/environment.go	Rename envURLTemplate; mapstructure tags; GoDoc; error builders
pkg/api/deployment.go	Add GoDoc
pkg/api/credential.go	Improve package comment; context-aware slog; GoDoc
pkg/api/cost.go	Improve package comment; mapstructure tags; GoDoc
pkg/api/container_repository.go	Add GoDoc
pkg/api/bundle_test.go	Rename vars to ID form
pkg/api/bundle.go	Add mapstructure tags; add GoDoc; rename param bundleID
pkg/api/artifact_definitions.go	Add mapstructure tags; error building; GoDoc
pkg/api/artifact.go	Add mapstructure tags; error building; GoDoc
main.go	Add package GoDoc
docs/helpdocs/helpdoc.go	Add GoDoc; remove fmt usage
cmd/version.go	Rename doc comment to match NewCmdVersion
cmd/server.go	Add GoDoc; swagger annotation alignment
cmd/schema.go	Add GoDoc; ignore MarkFlagRequired errors explicitly
cmd/project.go	Add GoDoc; rename vars; error var rename; template exec var rename
cmd/preview.go	Add GoDoc
cmd/package.go	Add GoDoc; funlen nolint; rename vars; switch for release channel
cmd/logs.go	Add GoDoc
cmd/infrastructure.go	dupl nolint; add GoDoc for deprecated command
cmd/image.go	Add GoDoc; simplify boolean expression
cmd/environment.go	Add GoDoc; rename vars; error var rename; template exec var rename
cmd/docs.go	Convert Run -> RunE with error returns; tighten perms; fmt.Fprintf
cmd/definition.go	Add GoDoc; dupl nolint; error var rename; template exec var rename
cmd/credential.go	Add GoDoc
cmd/bundle.go	Add GoDoc; funlen nolint; switch style; rename vars; error var rename
cmd/artifact.go	Add GoDoc; ignore MarkFlagRequired errors explicitly; dupl nolint; var rename
cmd/application.go	Add package GoDoc; dupl nolint; deprecated command doc
.golangci.yaml	Adjust analyzer exclusions; relax sloglint global rule; tweak exclusions
.devcontainer/devcontainer.json	Move to prebuilt image; add golangci-lint feature; VS Code settings
.devcontainer/Dockerfile	Remove custom devcontainer Dockerfile

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

pkg/commands/pkg/configure_test.go

 			vars := gqlmock.ParseInputVariables(req)
-			paramsJSON := []byte(vars["params"].(string))
+			paramsStr, ok := vars["params"].(string)
+			if !ok {
+				panic("vars[\"params\"] is not a string")
+			}


pkg/commands/pkg/patch_test.go

 			vars := gqlmock.ParseInputVariables(req)
-			paramsJSON := []byte(vars["params"].(string))
+			paramsStr, ok := vars["params"].(string)
+			if !ok {
+				panic("vars[\"params\"] is not a string")
+			}


pkg/commands/pkg/configure_test.go

 		func(req *http.Request) any {
 			vars := gqlmock.ParseInputVariables(req)
-			paramsJSON := []byte(vars["params"].(string))
+			paramsStr, ok := vars["params"].(string)
+			if !ok {
+				panic("vars[\"params\"] is not a string")
+			}


pkg/provisioners/types.go

+// ReadProvisionerInputs is a no-op for unknown provisioner types.
 func (p *NoopProvisioner) ReadProvisionerInputs(string) (map[string]any, error) {
-	return nil, nil
+	return map[string]any{}, nil


pkg/mockfilesystem/main.go

+			Path:    path.Join(rootTemplateDir, templatePath) + "/massdriver.yaml",
 			Content: massdriverYamlTemplate,
 		},
 		{
-			Path: fmt.Sprintf("%s/main.tf", path.Join(rootTemplateDir, templatePath, srcPath)),
+			Path: path.Join(rootTemplateDir, templatePath, srcPath) + "/main.tf",
 		},


pkg/commands/preview/model.go

+				currentModel, currentModelOk := m.current.(artifacttable.Model)
+				if !currentModelOk {
+					break
+				}
+				selectedArtifacts := currentModel.SelectedArtifacts


pkg/api/environment_test.go

 		},
 	})

 	mdClient := client.Client{
 		GQL: gqlClient,
 	}

-	got, err := api.GetEnvironmentsByProject(t.Context(), &mdClient, packageId)
+	got, err := api.GetEnvironmentsByProject(t.Context(), &mdClient, packageID)


pkg/commands/preview/deploy_test.go

+		if !ok {
+			panic("input is not a map[string]any")
+		}

-		paramsJSON := []byte((inputMap["packageConfigurations"]).(string))
+		pkgConfStr, pkgConfOk := inputMap["packageConfigurations"].(string)
+		if !pkgConfOk {
+			panic("inputMap[\"packageConfigurations\"] is not a string")
+		}


chrisghill · 2026-03-13T20:04:42Z

@claude review

claude · 2026-03-13T20:04:57Z

Claude finished @chrisghill's task in 1m 53s —— View job

PR Review

Read CLAUDE.md for repo guidelines (none found)
Analyze PR diff and changed files
Review key files flagged by Copilot
Provide comprehensive review feedback

This PR does a good job of systematically addressing golangci-lint findings: GoDoc comments, file permissions, error handling patterns, and linter suppressions. Most changes are mechanical and correct. Below are the issues I found, ranging from a real behavioral regression down to minor doc/style nits.

Critical: NoopProvisioner behavioral regression

pkg/provisioners/types.go:36 — ReadProvisionerInputs now returns map[string]any{} (empty map) instead of nil.

The LintInputsMatchProvisioner function in pkg/bundle/lint.go:249-252 explicitly uses nil as a sentinel to skip the check for "not implemented" provisioners:

// If this provisioner doesn't have "ReadProvisionerVariables" implemented, it returns nil
if provisionerInputs == nil {
    continue
}

With the empty-map change, this nil check is bypassed. The code then proceeds to diff an empty provisionerInputsProperties against the full set of massdriverInputsProperties, generating spurious warnings that every massdriver.yaml input is "missing from IaC" for any unknown provisioner type. This is a silent regression that will produce noisy false-positive lint output.

Fix: Either revert NoopProvisioner.ReadProvisionerInputs to return nil, nil (preserving the existing contract), or update LintInputsMatchProvisioner to treat an empty map from the noop provisioner as "no inputs to check". Fix this →

Moderate: `panic` in tests should be `t.Fatalf`

Multiple test files use panic for failed type assertions inside test handler closures. This crashes the entire test binary and can obscure other test failures:

pkg/commands/pkg/configure_test.go:26 and :72
pkg/commands/pkg/patch_test.go:29
pkg/commands/preview/deploy_test.go:79 and :84

Example (configure_test.go:24-27):

paramsStr, ok := vars["params"].(string)
if !ok {
    panic("vars[\"params\"] is not a string")
}

Should be:

paramsStr, ok := vars["params"].(string)
if !ok {
    t.Fatalf("vars[\"params\"] is not a string, got %T", vars["params"])
}

Note that t needs to be passed into the response closures for this to work. Fix this →

Moderate: preview/model.go – `break` after failed type assertion is silently unsafe

pkg/commands/preview/model.go:92-106:

case artifactSelection:
    currentModel, currentModelOk := m.current.(artifacttable.Model)
    if !currentModelOk {
        break  // exits the inner switch
    }
    // ... saves selection ...
}

m.promptCursor++  // always runs, even after the break

When the type assertion fails, break exits the switch m.mode block but execution falls through to m.promptCursor++ on line 106. This advances the wizard cursor with an unsaved selection, potentially causing an index-out-of-range panic on the next m.prompts[m.promptCursor] access or leaving the model in an inconsistent state.

Fix: Return early without advancing when the type assertion fails, or surface the error. Fix this →

Minor: Misleading test name/variable in environment_test.go

pkg/api/environment_test.go:253 — The test is named TestGetEnvironmentsByPackage, the variable is packageID, but the function being tested is api.GetEnvironmentsByProject(...). The variable name and the function being tested are mismatched. Either the function name changed but the test name/variable wasn't updated, or this test was intended to cover a different API. Fix this →

Minor: Misleading doc comment on `HasIssues()`

pkg/bundle/lint.go:69-72:

// HasIssues returns true if the result contains any error-level issues
func (r *LintResult) HasIssues() bool {
    return len(r.Issues) > 0
}

The comment says "any error-level issues" but the implementation returns true for any issues (including warnings). The comment should say "any issues (warnings or errors)". This is inconsistent with HasErrors() which has the same doc text but actually only checks for errors.

Minor: String concatenation instead of `filepath.Join` in mockfilesystem

pkg/mockfilesystem/main.go:39-44 — Paths are constructed by appending string literals:

Path: path.Join(rootTemplateDir, templatePath) + "/massdriver.yaml",
Path: path.Join(rootTemplateDir, templatePath, srcPath) + "/main.tf",

Prefer filepath.Join(rootTemplateDir, templatePath, "massdriver.yaml") for filesystem paths. Also note path (URL paths) is used throughout this file but filepath is imported for projectRoot() — mixing the two is inconsistent. On Linux they behave identically, but the convention matters.

Overall

The bulk of this PR (GoDoc comments, permission tightening, errors.New for static errors, strings.Builder usage) is straightforward and correct. The one functional regression to address before merging is the NoopProvisioner nil→empty-map change, which will cause false-positive lint warnings. The panic-in-tests and the model.go type assertion issue are also worth fixing. The rest can be addressed in follow-up.

Used claude to fix the lint errors

0ce026a

chrisghill requested a review from coryodaniel as a code owner March 13, 2026 19:52

chrisghill requested review from Copilot and coryodaniel and removed request for coryodaniel March 13, 2026 19:52

Copilot started reviewing on behalf of chrisghill March 13, 2026 19:53 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

PR comment fixes

98ac687

ChristensenJoe approved these changes Mar 13, 2026

View reviewed changes

chrisghill merged commit 834b1dc into main Mar 13, 2026
5 of 6 checks passed

chrisghill deleted the fix-linter-20260313 branch March 13, 2026 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Used claude to fix the lint errors#219

Used claude to fix the lint errors#219
chrisghill merged 2 commits intomainfrom
fix-linter-20260313

chrisghill commented Mar 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

chrisghill commented Mar 13, 2026

Uh oh!

claude bot commented Mar 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

chrisghill commented Mar 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

chrisghill commented Mar 13, 2026

Uh oh!

claude bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Critical: NoopProvisioner behavioral regression

Moderate: panic in tests should be t.Fatalf

Moderate: preview/model.go – break after failed type assertion is silently unsafe

Minor: Misleading test name/variable in environment_test.go

Minor: Misleading doc comment on HasIssues()

Minor: String concatenation instead of filepath.Join in mockfilesystem

Overall

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

claude bot commented Mar 13, 2026 •

edited

Loading

Moderate: `panic` in tests should be `t.Fatalf`

Moderate: preview/model.go – `break` after failed type assertion is silently unsafe

Minor: Misleading doc comment on `HasIssues()`

Minor: String concatenation instead of `filepath.Join` in mockfilesystem