fix+refactor: 12-expert review — bugs, dead code, safety, quality#34
Merged
Conversation
- kubectl quiet mode now captures stderr for transient error detection - kubectl retry with non-seekable stdin returns error instead of replaying empty input - kubectl error messages include actual executed args (with injected flags) - deployRemote gateway timeout returns error instead of exit 0 - runQuiet uses context.WithTimeout instead of leaking goroutines - buggy contains()/indexOf() test helpers replaced with strings.Contains Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove SandboxUpload/SandboxExec from Gateway interface and CLI impl (zero callers) - Remove status.Warnf (zero callers) - Inline decodeBase64 (trivial stdlib wrapper, one caller) - Delete internal/util package (single FileExists function, inline at call site) - Flatten Gateway sub-interfaces into single interface (no consumer used the narrow types) - Remove no-op cmd.Stderr = nil in runOutput Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Check os.UserHomeDir and GatewaySelect errors in deployRemote - Add failure guidance on deploy error (suggests harness teardown --k8s) - Teardown requires at least one flag instead of defaulting to delete-all - Validate --local/--remote mutual exclusion on new command - Use os.MkdirTemp for staging dir instead of hardcoded /tmp/openshell - Defer cleanup of staging dir on all exit paths - Quote values in BuildSandboxEnv (%q) to prevent shell injection - Write sandbox.env with 0600 instead of 0644 - Fix stale ./setup-providers.sh reference to harness providers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ature - Extract parseFirstColumn to deduplicate ProviderList/SandboxList parsing - Merge parse.go into profile.go (arbitrary split, 30 lines) - Merge check.go into preflight.go (arbitrary split, check.go depended on preflight.go types) - Move shared test mock (mockGW) to cmd/helpers_test.go - Change RunHelm to return error only (string return was always empty) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…le lint - Deduplicate directory walk loop in detectHarnessDir (roots slice) - Fail fast with clear error instead of silently falling back to "." - Add default case in preflight subcommand switch (rejects unknown args) - Add make vet and make lint targets to Makefile Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Net: -51 lines across 21 files. All tests passing.
Test plan
go test ./...passes (both modules)go vet ./...cleanmake validate(full integration — podman + OCP)🤖 Generated with Claude Code