Skip to content

Fix 6 bugs in hand-written files, templates, and build scripts#123

Open
luke-hagar-sp wants to merge 1 commit intomainfrom
fix/cross-sdk-bug-fixes
Open

Fix 6 bugs in hand-written files, templates, and build scripts#123
luke-hagar-sp wants to merge 1 commit intomainfrom
fix/cross-sdk-bug-fixes

Conversation

@luke-hagar-sp
Copy link
Copy Markdown
Contributor

Summary

Cross-SDK audit identified 6 bugs in the Go SDK across hand-written files, mustache templates, and build scripts. All fixes target the correct layer — hand-written code or templates/build scripts — so generated code will be corrected on next make build.

Bug Fixes

Critical

G1: Wrong error variable in localConfig() and homeConfig() silently swallows config parse errors

  • configuration.go L109, L138: if err2 := viper.ReadInConfig(); err != nil checked err (from the earlier os.Executable()/os.UserHomeDir() call) instead of err2. Since err was always nil at that point, config file parse errors were silently swallowed.
  • Fix: Changed err to err2 in both functions.

Medium

G2: Global viper instance pollution between localConfig() and homeConfig()

  • configuration.go L104-L107, L134-L136: Both functions called viper.AddConfigPath(), viper.SetConfigName(), viper.SetConfigType() on the global viper instance. Config paths from localConfig() leaked into homeConfig(), potentially causing the wrong config file to be loaded.
  • Fix: Each function now creates its own v := viper.New() instance.

Low

G3: Unused variables, dead code, and unnecessary import in client.go

  • client.go: Removed unused jsonCheck/xmlCheck regexp vars, unused cfg/common/token fields on APIClient, the dead service struct, and the "regexp" import.

G4: Generated tests don't compile — missing required ClientConfiguration argument

  • sdk-resources/resources/api_test.mustache L24: Template called NewConfiguration() with no arguments, but NewConfiguration requires a ClientConfiguration parameter.
  • Fix: Changed to NewConfiguration({{goImportAlias}}.ClientConfiguration{}).

G5: Missing await on fs.unlink() and fs.writeFile() in postscript.js

G6: Broken fs.stat() check in moveFiles()fs.stat() throws on missing path instead of returning falsy

  • sdk-resources/postscript.js L74: if (!await fs.stat(destPath)) always throws when the path doesn't exist (promises-based fs.stat throws ENOENT), so the mkdir fallback never executes.
  • Fix: Replaced with await fs.mkdir(destPath, { recursive: true }) which is a no-op if the directory already exists.

Verification

  • go build . passes clean (hand-written code compiles)
  • Template fix (G4) will take effect on next make build — existing generated test files in api_*/test/ have the pre-existing bug
  • Build script fixes (G5, G6) verified by code inspection

Test plan

  • Run go build ./... after next make build to verify generated tests compile
  • Verify config loading works correctly with local config.json and home ~/.sailpoint/config.yaml
  • Run make build to verify postscript.js fixes work during code generation

🤖 Generated with Claude Code

- Fix wrong error variable in localConfig() and homeConfig() (err -> err2)
- Fix global viper instance pollution between localConfig() and homeConfig()
- Remove unused jsonCheck/xmlCheck vars, dead service struct, and regexp import
- Fix generated test template to pass required ClientConfiguration arg
- Add missing await to fs.unlink() and fs.writeFile() in postscript.js
- Replace broken fs.stat() check with fs.mkdir({ recursive: true })

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

2 participants