Skip to content

Replace PS1 scripts with native Go/Makefile tooling#49

Merged
ziembor merged 1 commit intomainfrom
b3.1.1
Apr 12, 2026
Merged

Replace PS1 scripts with native Go/Makefile tooling#49
ziembor merged 1 commit intomainfrom
b3.1.1

Conversation

@ziembor
Copy link
Copy Markdown
Owner

@ziembor ziembor commented Apr 12, 2026

Summary

  • Makefile replaces build-all.ps1 and run-integration-tests.ps1 with build, build-verbose, test, integration-test, clean, and help targets
  • scripts/check-integration-env.sh validates all four MSGRAPH* env vars before integration tests run, exits 1 with clear per-variable error messages
  • internal/devtools/release replaces run-interactive-release.ps1 with a portable Go implementation exposed as gomailtest devtools release (version bump, changelog, git commit/tag/push, dev-branch creation, optional PR via gh)
  • internal/devtools/env exposes gomailtest devtools env for managing MSGRAPH* env vars interactively
  • tests/integration/sendmail_test.go is a Go integration test (build-tagged) that replaces the broken Pester test
  • tests/Test-SendMail.ps1 deleted — hardcoded author's OneDrive path, referenced legacy msgraphtool.exe binary

Test plan

  • make build — binary appears at bin/gomailtest.exe; version line printed
  • make test — all unit tests pass
  • make help — targets listed with descriptions
  • make integration-test without env vars — exits 1 with "Missing required environment variable(s)" message
  • gomailtest devtools release --dry-run — walks all 13 steps without writing anything
  • Confirm tests/Test-SendMail.ps1 is absent

🤖 Generated with Claude Code

- Add Makefile: build, build-verbose, test, integration-test, clean, help
- Add scripts/check-integration-env.sh: validates MSGRAPH* env vars before
  integration tests, exits 1 with per-variable error messages
- Add internal/devtools/release: interactive release automation in Go
  (version bump, changelog, git commit/tag/push, dev-branch creation)
  replaces run-interactive-release.ps1; exposed as 'gomailtest devtools release'
- Add internal/devtools/env: MSGRAPH* env var management commands
  exposed as 'gomailtest devtools env'
- Add tests/integration/sendmail_test.go: Go integration test replacing
  the Pester-based Test-SendMail.ps1
- Wire devtools subcommand into root cobra command
- Delete tests/Test-SendMail.ps1: broken (hardcoded OneDrive path,
  references legacy msgraphtool.exe binary)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@ziembor ziembor merged commit 0faec74 into main Apr 12, 2026
5 checks passed
@claude
Copy link
Copy Markdown

claude bot commented Apr 12, 2026

Code Review

Overall this is a well-structured PR that meaningfully improves the developer toolchain. The move from PowerShell scripts to a Makefile + portable Go implementation is the right call for a cross-platform tool. Below are issues worth addressing before merge.


Bugs

1. Buffered input loss across prompt calls (prompt.go)

Every prompt helper (PromptLine, PromptYesNo, PromptLines, PromptVersion) creates its own bufio.Scanner on the shared io.Reader. bufio.Scanner maintains an internal 4 KB read-ahead buffer. When a user types ahead, the first scanner may buffer more than one line; those extra bytes are gone when the function returns and the scanner is dropped. The next call creates a fresh scanner and reads from the OS fd — missing whatever was buffered.

// Each of these creates a new bufio.Scanner on opts.In
PromptYesNo(w, r, "Continue anyway?", false)    // scanner 1 drops buffer on return
PromptVersion(w, r, currentVer)                  // scanner 2 — may miss buffered bytes

Fix: accept a *bufio.Reader instead of io.Reader so the buffer is shared across calls, or create the scanner once in Run and thread it through.


2. Silent error discard for CurrentBranch() (release.go:849)

branch, _ := CurrentBranch()   // error silently discarded
if branch != "main" && branch != "master" && ghAvailable() {

If git rev-parse fails (detached HEAD, no repo), branch is "". The condition "" != "main" && "" != "master" is true, so the PR-creation prompt fires on an empty branch name, and gh pr create will fail with a confusing error. Should be if err == nil && branch != "main" && ....


3. projectRoot() fallback points to bin/ (release_cmd.go:1038)

// Fallback: use the directory of the running binary
exe, _ := os.Executable()
return filepath.Dir(exe), nil

With make build, the binary lives at bin/gomailtest[.exe]. filepath.Dir returns bin/, not the project root. Every subsequent file operation (version file at internal/..., ChangeLog/..., etc.) will fail silently or panic. The fallback should walk up until it finds go.mod, or just return an error so the user knows to run from within the repo.


Code Quality

4. Hardcoded AI attribution in user commits (release.go:792, :912)

commitMsg := fmt.Sprintf("Bump version to %s\n\nCo-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>", newVer)

This silently adds an AI co-author trailer to every release commit and dev-cycle commit the tool creates on behalf of the user. A developer running gomailtest devtools release has no reason to expect AI attribution in their git history. This should be removed (or at most be an opt-in --ai-attribution flag).


5. newCheckCmd uses os.Stderr directly instead of cmd.ErrOrStderr() (env_cmd.go:285)

fmt.Fprintf(os.Stderr, "missing: %s\n", name)

Every other command in this PR correctly uses cmd.ErrOrStderr() so output can be redirected in tests. This one leaks directly to the process stderr and breaks testability.


6. BUILD.md still refers to the deleted build-all.ps1

The Quick Build section still shows .\build-all.ps1 and all code blocks use PowerShell syntax. Since this PR explicitly replaces those scripts with a Makefile, the doc should be updated to reflect make build / make test.


Minor / Suggestions

7. append(RequiredVars, OptionalVars...) repeated in two places (env.go:133, :155)

Both ShowVars and ClearCommands construct allVars the same way. A small unexported allVars() func or a package-level var allVars = append(...) would DRY this up.

8. ChangeLog/ directory missing from skipDirs (security_scan.go:1094)

The scanner walks ChangeLog/*.md, which is full of version numbers, author emails in changelog entries, and similar patterns. These will generate many false-positive GUID/email findings and force maintainers to confirm through noisy warnings on every release. Adding "ChangeLog": true to skipDirs would eliminate the noise.

9. Integration test uses a relative binary path (sendmail_test.go:1477)

binary := filepath.Join("..", "..", "bin", "gomailtest")

This assumes the test is invoked from tests/integration/. go test sets the working directory to the package directory, so this happens to work with go test ./tests/integration/, but it's fragile. Using os.Getenv("GOPATH") or anchoring from the module root via runtime.Caller would be more robust.

10. scripts/check-integration-env.sh documents a non-existent subcommand

echo "  gomailtest devtools env set"

There is no set subcommand in env_cmd.go — the subcommands are show, clear, and check. Should be gomailtest devtools env check or gomailtest devtools env.


What's Good

  • The dry() / dry2() pattern in release.go is clean and makes --dry-run easy to verify.
  • maskVar dispatching via suffix matching is a nice pattern that avoids repeating masking logic.
  • Using //go:build integration for the integration test keeps it properly gate-kept from regular go test ./....
  • Deleting Test-SendMail.ps1 (hardcoded OneDrive path, legacy binary name) is unambiguously the right move.

@ziembor ziembor deleted the b3.1.1 branch April 12, 2026 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant