fix: Update AWS SDK dependencies and improve signal handling in SSM manager#150
Conversation
📝 WalkthroughWalkthroughAdds RDS CLI commands and implementations, SSM SSH/SSH‑config/RDP features, extends ClientPool with an RDS client, forwards parent SIGINT/SIGTERM to AWS CLI subprocesses in SSM manager, upgrades many Go module dependencies, and adds comprehensive unit tests for RDS and SSM SSH behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as ztictl (process)
participant Pool as ClientPool
participant SSM as AWS SSM
participant Sub as aws-cli / ssh / rdp
User->>CLI: ztictl ssm ssh <instance>
CLI->>Pool: GetSSMClient(ctx, region)
Pool-->>CLI: SSM client
CLI->>SSM: StartSession API (StartSSHSession)
SSM-->>CLI: session payload / proxy details
CLI->>Sub: spawn ssh subprocess with ProxyCommand
Note right of CLI: parent ignores SIGINT/SIGTERM and forwards them to subprocess
User->>Sub: Ctrl+C
Sub->>CLI: subprocess exits
CLI->>SSM: cleanup / close session
Sub-->>User: ssh exits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ztictl/internal/ssm/manager.go (1)
408-413: Critical: Same signal handling issue as in StartSession.This code has the identical problem as lines 137-142.
signal.Notify()alone does not prevent default signal handling. Please apply the same fix here.🔎 Recommended fix
- // Ignore SIGINT in the parent process so it passes through to the AWS CLI subprocess. - // This fixes Ctrl+C handling in WSL2/Windows Terminal where the signal would otherwise - // kill the parent Go process and disconnect the port forwarding session. - sigChan := make(chan os.Signal, 1) - signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) - defer signal.Stop(sigChan) + // Ignore SIGINT/SIGTERM in the parent process so they pass through to the AWS CLI subprocess. + // This fixes Ctrl+C handling in WSL2/Windows Terminal where the signal would otherwise + // kill the parent Go process and disconnect the port forwarding session. + signal.Ignore(syscall.SIGINT, syscall.SIGTERM) + defer signal.Reset(syscall.SIGINT, syscall.SIGTERM)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ztictl/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
ztictl/go.modztictl/internal/ssm/manager.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go Code (macos-latest)
- GitHub Check: Test Go Code (windows-latest)
- GitHub Check: Test Go Code (ubuntu-latest)
🔇 Additional comments (2)
ztictl/go.mod (2)
20-20: spf13/cobra v1.10.2 is a valid stable release. The version exists and includes standard maintenance work (CI/linter fixes, YAML migration, refactors). No action required.
8-16: No issues found. All AWS SDK v2 versions (v1.41.0 and related service packages) are legitimate releases from AWS with no known security advisories.
- Updated go.mod to include the RDS service from AWS SDK v2. - Modified client_pool.go to initialize and expose an RDS client in the ClientPool struct. - Added a new method GetRDSClient to retrieve the RDS client from the pool.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
ztictl/cmd/ztictl/ssm_ssh_test.go (1)
62-118: Tests create mock commands instead of testing actual command implementation.The test creates a new
cobra.Commandwith mocked Run function instead of testingssmSSHCmddirectly. While this tests the expected behavior, it doesn't verify that the actual command has the correct Args validator set. Consider testing the actual command's Execute method or at minimum verify thatssmSSHCmd.Argsbehaves correctly (which you do inTestSsmSSHCmdStructure).ztictl/cmd/ztictl/rds_power.go (3)
126-130: Consider reusing ClientPool across commands.A new
ClientPoolis created for each command invocation. For a CLI tool, this is acceptable, but if performance becomes a concern, consider making the pool a package-level singleton or passing it via context.
287-316:waitForRDSStatusdoesn't respect context cancellation during polling.The function uses
time.Sleepwhich doesn't respond to context cancellation. If the user presses Ctrl+C during the wait, the function won't exit until the current sleep completes.🔎 Proposed fix to respect context cancellation
func waitForRDSStatus(ctx context.Context, rdsClient *rds.Client, dbIdentifier, targetStatus string) error { maxWait := 30 * time.Minute pollInterval := 30 * time.Second deadline := time.Now().Add(maxWait) + ticker := time.NewTicker(pollInterval) + defer ticker.Stop() for time.Now().Before(deadline) { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + resp, err := rdsClient.DescribeDBInstances(ctx, &rds.DescribeDBInstancesInput{ DBInstanceIdentifier: aws.String(dbIdentifier), }) if err != nil { return fmt.Errorf("failed to check instance status: %w", err) } if len(resp.DBInstances) == 0 { return fmt.Errorf("instance %s not found", dbIdentifier) } status := aws.ToString(resp.DBInstances[0].DBInstanceStatus) fmt.Printf(" Current status: %s\n", status) if status == targetStatus { return nil } - time.Sleep(pollInterval) + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + } } return fmt.Errorf("timeout waiting for instance to reach %s status", targetStatus) }
150-166: Consider using a table writer for cleaner output formatting.The manual
fmt.Printfwith fixed-width formatting may misalign if values exceed the column widths. Consider using a tabwriter or ensuring endpoint strings are truncated.ztictl/cmd/ztictl/ssm_ssh.go (2)
369-375: Hardcoded/u:Administratormay not be appropriate for all Windows instances.The xfreerdp command hardcodes the Administrator username, which may not be the desired user for all Windows instances. Consider making this configurable or omitting it to let the user enter credentials.
🔎 Proposed fix to make username configurable or remove hardcoding
- if _, err := exec.LookPath("xfreerdp"); err == nil { - cmd = exec.Command("xfreerdp", fmt.Sprintf("/v:localhost:%d", port), "/u:Administrator") + if _, err := exec.LookPath("xfreerdp"); err == nil { + cmd = exec.Command("xfreerdp", fmt.Sprintf("/v:localhost:%d", port)) } else if _, err := exec.LookPath("rdesktop"); err == nil {Alternatively, add a
--rdp-userflag to the RDP command and pass it through.
217-221: Duplicate detection uses simple string matching.The check
strings.Contains(string(existingConfig), fmt.Sprintf("Host %s\n", name))could have false positives if the name appears as a substring in another Host entry or in a comment. Consider using a more robust pattern match.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ztictl/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
ztictl/cmd/ztictl/rds.goztictl/cmd/ztictl/rds_power.goztictl/cmd/ztictl/rds_test.goztictl/cmd/ztictl/ssm.goztictl/cmd/ztictl/ssm_ssh.goztictl/cmd/ztictl/ssm_ssh_test.goztictl/go.modztictl/internal/ssm/client_pool.go
🧰 Additional context used
🧬 Code graph analysis (3)
ztictl/internal/ssm/client_pool.go (1)
ztictl/pkg/aws/client.go (1)
Client(18-25)
ztictl/cmd/ztictl/rds_test.go (2)
ztictl/cmd/ztictl/root.go (1)
Execute(200-202)ztictl/pkg/colors/colors.go (1)
Error(19-19)
ztictl/cmd/ztictl/ssm_ssh.go (2)
ztictl/pkg/logging/logging.go (3)
LogError(162-166)LogInfo(148-152)LogWarn(155-159)ztictl/cmd/ztictl/ssm_validation.go (2)
ValidateInstanceState(31-64)InstanceValidationRequirements(15-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go Code (ubuntu-latest)
- GitHub Check: Test Go Code (macos-latest)
- GitHub Check: Test Go Code (windows-latest)
🔇 Additional comments (17)
ztictl/cmd/ztictl/ssm.go (2)
11-31: LGTM - Updated documentation and examples are clear.The Long description properly documents the new SSH, SSH-config, and RDP subcommands alongside existing commands.
56-58: LGTM - Subcommand wiring follows established patterns.The new SSH, SSH-config, and RDP commands are registered consistently with the existing subcommand pattern.
ztictl/internal/ssm/client_pool.go (2)
12-12: LGTM - RDS client integration follows established patterns.The RDS client is added consistently with the existing client pool architecture: import, struct field, initialization, and accessor all follow the same pattern as other AWS service clients.
Also applies to: 29-29, 85-85
131-137: LGTM - GetRDSClient accessor is consistent with other accessors.The implementation correctly reuses
GetClientsfor connection pooling and thread safety.ztictl/cmd/ztictl/rds.go (1)
1-29: LGTM - Well-structured RDS command entry point.The command follows the established CLI patterns in this codebase with clear documentation and examples.
ztictl/cmd/ztictl/ssm_ssh_test.go (3)
492-529: LGTM - Good unit tests forbuildProxyCommandhelper.The tests verify that the proxy command contains the expected components for SSM SSH session establishment.
531-543: LGTM - Platform-specific SSH command test.The test correctly validates platform-specific behavior using
runtime.GOOS.
545-580: LGTM - Region example consistency tests.These tests ensure documentation examples use consistent region references, which helps maintain documentation quality.
ztictl/cmd/ztictl/rds_test.go (3)
56-85: LGTM - Good subcommand registration verification.This test ensures all expected RDS subcommands are properly registered.
612-642: LGTM - Comprehensive status colorization tests.Tests cover all expected RDS status values and verify the output contains the original status text.
663-685: LGTM - Consolidated argument validation tests.Good approach to test argument validation for multiple commands in a single test function, reducing code duplication.
ztictl/cmd/ztictl/rds_power.go (1)
318-332: LGTM - Comprehensive status colorization.The
colorizeRDSStatusfunction handles all common RDS states with appropriate color coding for visibility.ztictl/cmd/ztictl/ssm_ssh.go (3)
1-16: LGTM - Clean imports and package declaration.The imports are well-organized and include only necessary dependencies.
388-398: LGTM - Well-structured ProxyCommand builder.The function correctly handles Windows vs Unix command naming and constructs a proper SSM start-session command for SSH tunneling.
408-426: LGTM - Flag initialization follows established patterns.All flags are properly defined with shorthands, defaults, and descriptive help text.
ztictl/go.mod (2)
3-3: Go 1.24.5 is a valid, released version. However, consider updating to Go 1.25.5 (the latest stable release as of December 2025) if your project requires the most recent features or security updates.Likely an incorrect or invalid review comment.
7-26: Dependency updates look consistent and well-structured.The AWS SDK v2 packages are updated coherently, and the RDS service dependency aligns with the RDS command functionality. The uniseg addition is expected as an indirect dependency from tcell.
- Updated QA_SSM_TESTS.md to include additional formatting and clarity improvements. - Refined README.md in tools directory for better option descriptions. - Modernized .golangci.yml configuration for improved linting practices. - Modified Makefile to auto-detect version from git tags and added formatting target. - Enhanced README.md in ztictl with clearer installation and feature descriptions. - Optimized RDS command handling by reusing client pool for efficiency. - Improved SSH command handling by adding a delay for RDP client launch. - Updated fuzzy finder documentation to clarify usage and validation rules. - Enhanced validation documentation for instance states and SSM agent statuses. - Implemented signal draining in session management to prevent channel overflow.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
CONTRIBUTING.md (1)
132-159: Update release process to match actual CI/CD pipeline and correct file/version references.The documented release process has several critical inaccuracies:
- File naming: Change
RELEASE_NOTES.txttoRELEASE_NOTES.md- Version source: Update VERSION variable location from "ssm/authaws scripts" to
src/00_version.sh(the single source of truth per the file's own comments)- Release process flow: The current instructions skip the release branch step required for automated documentation generation. Per
docs/CI_CD_PIPELINE.md, the correct flow is:
- Create release branch:
git checkout -b release/vX.Y.Z(triggers auto-doc generation)- Pull generated CHANGELOG.md:
git pull origin release/vX.Y.Z- Then tag and push:
git tag vX.Y.Z && git push origin vX.Y.Z(triggers build/release)- CHANGELOG generation: CHANGELOG.md is auto-generated by the
auto-generate-docs.ymlworkflow (not manually edited), making the current instruction to "add new entry" unnecessary and potentially conflicting.Refer to
docs/CI_CD_PIPELINE.mdanddocs/development/RELEASE.mdfor the complete, automated release workflow.docs/COMMANDS.md (2)
186-242: Add RDS Commands documentation section to COMMANDS.md.The RDS command implementation (list, start, stop, reboot) exists in the codebase but is not documented in COMMANDS.md. A dedicated "RDS Commands" section should be added with usage examples for:
ztictl rds list- List RDS instancesztictl rds start <db-identifier>- Start a stopped instanceztictl rds stop <db-identifier>- Stop a running instanceztictl rds reboot <db-identifier>- Reboot an instanceInclude available flags (--region, --wait, --force-failover) and example commands for each operation.
100-184: Add documentation for missing SSM SSH/RDP commands.The PR implements three new SSM commands (
ssm ssh,ssm ssh-config,ssm rdp) that are fully functional but missing from the COMMANDS.md documentation.Add new subsections to the SSM Operations section documenting:
ztictl ssm ssh- SSH to instances via SSM tunnel (no port 22 required)ztictl ssm ssh-config- Generate SSH config entries for native SSH access through SSMztictl ssm rdp- RDP to Windows instances via SSM tunnel (no port 3389 required)Each should include available flags and usage examples consistent with existing command documentation.
🧹 Nitpick comments (5)
docs/CONFIGURATION.md (1)
323-326: Add language specifier to the code block.The fenced code block at line 323 should specify a language for proper syntax highlighting.
🔎 Proposed fix
- ``` + ```text Error: SSO start URL must begin with https:// Fix: ztictl config repair ```docs/ztiaws-demo.md (1)
278-286: Add blank lines around the table.The table should be surrounded by blank lines for proper markdown formatting and better readability.
🔎 Proposed fix
Summary of Benefits + | Feature | Traditional AWS CLI | With ZTiAWS | | ---------------- | ------------------- | ----------------------------- | | Authentication | Manual SSO setup | Guided or auto-detected login | | Instance Access | Long IDs, SSH | Interactive fuzzy finder | | OS Detection | Manual scripts | Auto-detect and adapt | | Multi-Region Ops | Loops & scripts | Single command | | File Transfers | Manual S3 upload | Smart routing with S3 | | Power Control | Console or SDK | Tag-based automation | + ---scripts/install.ps1 (1)
44-54: Consider adding checksum verification for security.The download logic uses HTTPS and TLS 1.2, which is good. However, for security best practices, consider adding checksum verification of the downloaded binary to ensure integrity and detect any tampering or corruption during download.
Verify if the GitHub releases include checksums (e.g., SHA256SUMS file). If available, the script could download and verify the checksum before proceeding:
# Example enhancement (if checksums are published) $checksumUrl = "https://github.com/$repo/releases/latest/download/SHA256SUMS" $expectedHash = (Invoke-WebRequest -Uri $checksumUrl -UseBasicParsing).Content | Select-String $assetName | ForEach-Object { $_.Line.Split()[0] } $actualHash = (Get-FileHash $destPath -Algorithm SHA256).Hash if ($actualHash -ne $expectedHash) { Write-Host " [ERROR] Checksum verification failed" -ForegroundColor Red Remove-Item $destPath exit 1 }This is a recommended security improvement but not critical if the releases don't currently publish checksums.
ztictl/Makefile (1)
98-108: Consider documenting Prettier auto-install behavior.The
npx prettiercommand will auto-install Prettier on first run if not already cached, which may cause a delay. The graceful fallback for missing Node.js is good, but users might appreciate knowing thatnpxhandles installation automatically.Optional enhancement:
🔎 Add a note to the error message
- @cd .. && npx prettier --write "**/*.md" "**/*.json" "**/*.yaml" "**/*.yml" 2>/dev/null || echo "Install Node.js for Prettier support: npm install -g prettier" + @cd .. && npx prettier --write "**/*.md" "**/*.json" "**/*.yaml" "**/*.yml" 2>/dev/null || echo "Install Node.js for Prettier support (npx will auto-install on first run)"ztictl/cmd/ztictl/ssm_ssh.go (1)
330-350: Consider adding tunnel readiness check before launching RDP client.The 2-second delay is an improvement, but it's still a race condition—the tunnel established by
ForwardPort(line 350) might not be ready whenlaunchRDPClientexecutes. A more robust approach would poll the local port for connectivity before launching the client.🔎 Proposed enhancement with readiness check
if launch { - // Launch RDP client in background after giving tunnel time to establish + // Launch RDP client after verifying tunnel is ready go func() { - time.Sleep(2 * time.Second) // Wait for tunnel to be ready + // Poll local port to ensure tunnel is established + timeout := time.After(30 * time.Second) + ticker := time.NewTicker(500 * time.Millisecond) + defer ticker.Stop() + + for { + select { + case <-timeout: + logging.LogWarn("Timeout waiting for RDP tunnel to be ready") + return + case <-ticker.C: + conn, err := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", localPort), time.Second) + if err == nil { + conn.Close() + // Tunnel is ready + launchRDPClient(localPort) + return + } + } + } launchRDPClient(localPort) }() } else {This ensures the RDP client only launches after confirming the tunnel is accepting connections.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/ISSUE_TEMPLATE/region_request.md.github/workflows/auto-generate-docs.yml.github/workflows/build.yml.prettierignore.prettierrcCHANGELOG.mdCLAUDE.mdCONTRIBUTING.mdINSTALLATION.mdMakefileREADME.mdRELEASE_NOTES.mdRELEASE_NOTES.txtdocs/CI_CD_AUTHENTICATION.mddocs/CI_CD_PIPELINE.mddocs/COMMANDS.mddocs/CONFIGURATION.mddocs/IAM_PERMISSIONS.mddocs/LOGGING.mddocs/MULTI_REGION.mddocs/NOTIFICATIONS.mddocs/PRODUCTION_VERSION_FIX.mddocs/QUICK_START.mddocs/REGIONS.mddocs/TROUBLESHOOTING.mddocs/VERSION_CHECKING.mddocs/development/BUILD_ARTIFACTS.mddocs/development/RELEASE.mddocs/examples/github-actions-oidc.ymldocs/examples/gitlab-ci-oidc.ymldocs/ztiaws-demo.mdscripts/README.mdscripts/install.ps1scripts/install.shtests/QA_AUTHAWS_TESTS.mdtests/QA_SSM_TESTS.mdtools/README.mdztictl/.golangci.ymlztictl/Makefileztictl/README.mdztictl/cmd/ztictl/rds_power.goztictl/cmd/ztictl/ssm_ssh.goztictl/docs/FUZZY_FINDER_FEATURES.mdztictl/docs/VALIDATION.mdztictl/internal/ssm/manager.go
💤 Files with no reviewable changes (1)
- RELEASE_NOTES.txt
✅ Files skipped from review due to trivial changes (13)
- tools/README.md
- docs/IAM_PERMISSIONS.md
- docs/examples/github-actions-oidc.yml
- README.md
- .prettierrc
- docs/QUICK_START.md
- docs/NOTIFICATIONS.md
- .github/workflows/auto-generate-docs.yml
- docs/TROUBLESHOOTING.md
- scripts/README.md
- ztictl/.golangci.yml
- .prettierignore
- docs/REGIONS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- ztictl/internal/ssm/manager.go
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/build.yml
92-92: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
188-188: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
255-255: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🪛 checkmake (0.2.2)
Makefile
[warning] 7-7: Missing required phony target "all"
(minphony)
[warning] 70-70: Target body for "fmt" exceeds allowed length of 5 (8).
(maxbodylength)
ztictl/Makefile
[warning] 99-99: Target body for "fmt" exceeds allowed length of 5 (8).
(maxbodylength)
🪛 Checkov (3.2.334)
.github/workflows/build.yml
[high] 132-133: AWS Access Key
(CKV_SECRET_2)
[high] 133-134: AWS Access Key
(CKV_SECRET_2)
🪛 LanguageTool
docs/ztiaws-demo.md
[style] ~272-~272: Consider a different adjective to strengthen your wording.
Context: ...rs** — interact with AWS safely without deep CLI experience. Adoption Insight: Used...
(DEEP_PROFOUND)
docs/PRODUCTION_VERSION_FIX.md
[uncategorized] ~7-~7: The official name of this software platform is spelled with a capital “H”.
Context: ... support (corporate firewalls block api.github.com) - No TLS certificate handling (cor...
(GITHUB)
docs/LOGGING.md
[grammar] ~23-~23: Ensure spelling is correct
Context: ...essages (cyan) ## Script Behavior ### authaws - Purpose: Authentication workflows requ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/CI_CD_PIPELINE.md
[uncategorized] ~29-~29: The official name of this software platform is spelled with a capital “H”.
Context: ...lized workflows: ### Primary Workflow: .github/workflows/build.yml (Unified CI/CD) *...
(GITHUB)
CHANGELOG.md
[grammar] ~299-~299: Use a hyphen to join words.
Context: ...d security package with Windows and Unix specific validation scenarios ## [v2.5....
(QB_NEW_EN_HYPHEN)
RELEASE_NOTES.md
[uncategorized] ~33-~33: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ther Changes - Wrap commands in proper markdown code blocks - docs: finalize ZTiAWS dem...
(MARKDOWN_NNP)
docs/CI_CD_AUTHENTICATION.md
[style] ~92-~92: The double modal “required Stored” is nonstandard (only accepted in certain dialects). Consider “to be Stored”.
Context: ...rity risk) - Manual rotation required - Stored in secrets manager (additional exposure...
(NEEDS_FIXED)
🪛 markdownlint-cli2 (0.18.1)
docs/ztiaws-demo.md
278-278: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
.github/ISSUE_TEMPLATE/bug_report.md
29-29: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
INSTALLATION.md
79-79: Bare URL used
(MD034, no-bare-urls)
83-83: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
457-457: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
ztictl/README.md
274-274: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.github/ISSUE_TEMPLATE/region_request.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
CHANGELOG.md
72-72: Bare URL used
(MD034, no-bare-urls)
CLAUDE.md
264-264: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
RELEASE_NOTES.md
45-45: Bare URL used
(MD034, no-bare-urls)
docs/CONFIGURATION.md
323-323: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/VERSION_CHECKING.md
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 2b. Test (macos-latest)
- GitHub Check: 2b. Test (windows-latest)
- GitHub Check: 2b. Test (ubuntu-latest)
🔇 Additional comments (61)
.github/ISSUE_TEMPLATE/feature_request.md (1)
8-8: ✅ Template improvements align with PR goals and best practices.The changes meaningfully enhance the feature request template:
- Blank line after YAML frontmatter (line 8): Improves readability and follows standard markdown conventions.
- New descriptive sections (lines 12–27): The addition of "Describe the solution you'd like," "Describe alternatives you've considered," and "Screenshots or Examples" sections follow GitHub's recommended patterns for issue templates and will help requesters provide more complete context.
- Expanded "Additional context" (lines 19–24): Including AWS CLI Version and SSM Plugin Version fields is particularly relevant for a tool that integrates deeply with AWS/SSM, and aligns well with the PR's focus on dependency management and signal handling improvements.
- Checklist update (lines 33–38): Adding "This feature requires AWS CLI/SSM plugin changes" as a checklist item provides explicit tracking for dependencies that may affect this project.
The template is now more structured and will help contributors provide richer context for feature discussions.
Also applies to: 12-27, 33-38
CONTRIBUTING.md (3)
22-28: Well-structured additions to region contribution guidelines.The new "Region Code Guidelines" and "Required Information" sections provide clear direction for contributors adding new regions. The examples are concrete and helpful.
Also applies to: 30-37
42-57: Good expansion of Development Process with actionable test commands.Adding explicit test execution commands (
make testfor Go, shell script path) removes ambiguity and makes it easier for new contributors to validate changes locally.
92-118: Clear commit message guidelines with body and footer examples.The expanded format with optional body/footer and concrete examples will help contributors write more informative commit messages. The Types section is now more complete.
ztictl/docs/VALIDATION.md (1)
1-301: All documentation references and code examples verified successfully. The files, functions, and implementation details in the VALIDATION.md documentation are accurate and match the actual codebase in ztictl/cmd/ztictl/ssm_validation.go. All six helper functions exist with correct signatures, the InstanceValidationRequirements struct matches the documented definition, all code examples follow correct patterns used throughout the codebase, and all referenced documentation files and GitHub issues are valid.ztictl/README.md (4)
19-48: Enhanced feature documentation is comprehensive and well-structured.The expanded sections on multi-OS support, cross-platform coverage, and performance benefits clearly articulate the advantages of ztictl. The formatting with emojis and visual hierarchy makes the content approachable and easy to scan.
81-110: Fuzzy finder and interactive instance selection documentation is clear and actionable.The examples showing both traditional and interactive workflows, combined with keyboard shortcuts and validation details, provide good guidance for users. Cross-references to detailed documentation in separate files follow best practices.
154-160: Authentication table with security ratings is valuable for operator decision-making.The comparison of IAM authentication methods (OIDC, EC2 Instance Profile, ECS Task Role, IAM Keys) with security ratings and setup complexity helps users choose the right approach for their environment. Cross-platform examples for each method add practical value.
243-273: Core operations examples effectively demonstrate cross-platform capabilities.The progression from listing instances through platform detection and adapting commands to Linux and Windows scenarios clearly illustrates the auto-detection feature. Examples are concrete and highlight real-world use cases.
.github/ISSUE_TEMPLATE/region_request.md (1)
8-25: LGTM! Improved readability.The added blank lines improve visual separation between sections in the issue template.
docs/CONFIGURATION.md (1)
7-457: LGTM! Documentation formatting improvements.The changes improve consistency and readability:
- Standardized quote style in YAML examples (single quotes)
- Improved table alignment and spacing
- Better section organization with consistent spacing
These are presentation-only changes with no impact on functionality.
docs/examples/gitlab-ci-oidc.yml (1)
39-40: LGTM! Quote style consistency.The change from double quotes to single quotes is a stylistic improvement that aligns with the rest of the configuration formatting in the PR.
CLAUDE.md (1)
1-335: LGTM! Improved documentation formatting.The spacing adjustments improve readability and section organization throughout the Claude memory file. No content or logic changes.
docs/ztiaws-demo.md (1)
1-316: LGTM! Documentation formatting improvements.The extensive markdown formatting changes improve readability and visual consistency throughout the demo document. These are presentation-only changes.
docs/LOGGING.md (2)
17-17: Good fix! Corrected list formatting.Removing the extra dash fixes the bullet point formatting for consistency with other list items.
21-46: Excellent documentation additions.The new sections (Script Behavior, Environment Variables, File Format) provide valuable context for users understanding the logging system's behavior across different tools and configurations.
Makefile (2)
4-6: Good addition! Automatic version detection.The VERSION variable using
git describeprovides useful context and will help track which version of the codebase is being used. The fallback to "dev" handles repositories without tags gracefully.
69-79: Useful formatting target with minor security consideration.The
fmttarget provides convenient code formatting across Go and documentation files. The graceful fallback when Node.js is missing is well-handled.The
npxcommand at line 76 will download and execute prettier from npm. While this is standard practice, verify that the repository's contributor guidelines document this dependency or consider pinning to a specific prettier version for reproducibility:# Current (works but unpinned) npx prettier --write ... # Alternative with version pinning (more reproducible) npx prettier@3.x --write ...This is not a security risk in the current context but may help with build reproducibility.
scripts/install.ps1 (2)
1-26: Well-structured PowerShell installer script.The script header, error handling setup, and architecture detection are well-implemented. The 64-bit check is appropriate given the available binaries.
56-80: Excellent user experience and verification.The PATH modification is scoped to the user (not system-wide), which is appropriate. The installation verification and usage guidance provide a good user experience. The fallback handling when version check fails is thoughtful.
scripts/install.sh (6)
1-9: LGTM!The script uses proper bash error handling with
set -euo pipefail, which ensures the installer fails fast on errors, undefined variables, or pipeline failures.
25-40: LGTM!OS and architecture detection is robust, with proper normalization to match GitHub release asset naming conventions. Windows is appropriately blocked with guidance to use the PowerShell installer.
45-56: LGTM!Download logic is well-implemented with:
- Secure temporary file handling via
mktempand trap-based cleanup- Proper curl/wget fallback with appropriate flags
- Downloads to temporary location before installation
60-69: LGTM!Installation logic properly handles permission scenarios:
- Direct installation when the directory is writable
- Automatic sudo elevation when needed
- Ensures executable permissions in both paths
71-85: LGTM!Verification logic is user-friendly:
- Checks if the binary is accessible in PATH
- Displays version information when available
- Provides clear guidance for updating PATH if needed
87-92: LGTM!Usage examples are clear and demonstrate key commands including authentication, SSM listing with region shortcuts, and help access.
CHANGELOG.md (1)
1-619: LGTM!The CHANGELOG follows the Keep a Changelog format and documents the project's history comprehensively. The changes in this PR improve formatting consistency (code blocks, headers, etc.) without altering the semantic content.
docs/development/BUILD_ARTIFACTS.md (1)
1-115: LGTM!Documentation clearly explains the distinction between source files (commit) and build artifacts (ignore), with updated references to ARM64 builds and comprehensive workflow guidance. The rationale for not committing binaries is well-articulated.
INSTALLATION.md (1)
1-455: LGTM!The installation guide is comprehensive and well-organized with:
- Quick install one-liners for Linux/macOS
- Platform-specific detailed instructions for all supported architectures
- Windows setup with both PowerShell and GUI guidance
- Extensive troubleshooting section
- Clear migration path from legacy bash tools
ztictl/Makefile (1)
9-10: LGTM!Version auto-detection using
git describeis robust and provides meaningful version strings for both releases ("2.11.0") and development builds ("2.11.0-3-g1a2b3c4"), with a sensible fallback to "dev".docs/development/RELEASE.md (1)
1-121: LGTM!The release documentation is comprehensive and includes:
- Step-by-step Git commands for the release workflow
- Clear emphasis on waiting for auto-generated documentation
- Review checkpoints before proceeding
- Troubleshooting guidance
- Emergency release procedures
This provides a solid foundation for consistent release processes.
ztictl/cmd/ztictl/rds_power.go (9)
1-20: LGTM!Package structure is clean with appropriate imports. The shared
rdsClientPoolat the package level is an efficient pattern for reusing AWS clients across commands.
23-122: LGTM!Command definitions are well-structured with:
- Clear help text and examples showing region shortcuts
- Proper argument validation (ExactArgs for start/stop/reboot)
- Consistent error handling pattern
- Helpful notes about RDS limitations (e.g., 7-day auto-restart for stopped instances)
124-172: LGTM!The list function is well-implemented with:
- Proper nil checking for optional endpoint fields
- Colorized status output for better UX
- Clear table formatting
- Graceful handling of empty results
174-207: LGTM!The start function provides good UX with:
- Immediate feedback that the command was sent
- Optional
--waitflag with clear messaging- Helpful guidance when not waiting
- Proper error wrapping
209-243: LGTM!The stop function includes an important user warning about RDS's 7-day auto-restart policy, which helps prevent surprises. The implementation is consistent with the start command.
245-284: LGTM!The reboot function properly handles the
--force-failoverflag for Multi-AZ instances and provides clear user feedback about the operation's impact.
286-326: LGTM!The wait function implements robust polling with:
- Reasonable timeouts (30 minutes) and intervals (30 seconds) for RDS operations
- Exponential backoff for transient errors
- Consecutive error tracking to avoid infinite retries
- Clear progress feedback to the user
- Proper deadline checking to prevent infinite loops
328-342: LGTM!Status colorization improves UX by making instance states immediately recognizable, with semantically appropriate colors (green for available, red for failed, yellow for stopped, etc.).
344-360: LGTM!Flag initialization is consistent across commands with:
- Clear descriptions including region shortcode examples
- Appropriate shorthand flags (-r, -w, -f)
- Sensible defaults (wait=false)
ztictl/cmd/ztictl/ssm_ssh.go (6)
19-93: LGTM!Both SSH and SSH-config commands are well-defined with:
- Flexible argument handling (optional instance identifier triggers fuzzy finder)
- Clear requirements (AWS CLI v2, Session Manager plugin, SSH client)
- Comprehensive examples showing region shortcuts
- Proper flag handling and error propagation
95-157: LGTM!SSH connection implementation is robust with:
- Instance state validation (running + SSM online)
- Sensible defaults (ec2-user)
- Proper ProxyCommand construction for SSM tunneling
- Full stdio forwarding for interactive sessions
- Support for identity files and extra SSH arguments
159-249: LGTM!SSH config generation is well-implemented with:
- Secure file permissions (0700 for .ssh directory, 0600 for config file)
- Duplicate entry detection to prevent configuration conflicts
- Clear usage examples for both appended and printed output
- Proper error handling throughout
357-387: LGTM!RDP client launcher provides comprehensive cross-platform support:
- Windows: mstsc with proper port syntax
- macOS: native rdp:// URL handling
- Linux: Multiple fallbacks (xfreerdp, rdesktop, remmina) with helpful messaging if none are installed
- Graceful failure handling (warns but doesn't crash)
389-407: LGTM!Platform-specific command helpers are correctly implemented:
buildProxyCommanduses proper AWS SSM document (AWS-StartSSHSession) with correct parameter syntax (%%pfor SSH port)getSSHCommandhandles Windows.exesuffix appropriately- Both functions are simple and maintainable
409-427: LGTM!Flag initialization is well-designed:
- SSH: Supports multiple
-oarguments via StringArrayP- SSH-config: Boolean append flag for automatic config file modification
- RDP: Configurable local port with sensible default (33389) to avoid conflicts
- All flags have clear descriptions and appropriate types
ztictl/docs/FUZZY_FINDER_FEATURES.md (1)
1-182: LGTM!The documentation is comprehensive and well-structured, covering fuzzy finder features, keyboard shortcuts, mouse support, and platform compatibility. The instance state validation table and error feedback examples are clear and helpful for users.
.github/workflows/build.yml (2)
130-136: Test credentials are intentional placeholder values.The AWS credentials used here are AWS's documented example credentials (
AKIAIOSFODNN7EXAMPLE), not real secrets. Combined withAWS_EC2_METADATA_DISABLED: 'true', this correctly sets up an isolated test environment. The Checkov warnings are false positives in this context.
268-336: LGTM - Build and release pipeline is well-structured.The cross-platform build matrix with proper versioning, artifact naming, and checksum generation follows best practices. The conditional execution ensures builds only run for tags or manual dispatch.
docs/VERSION_CHECKING.md (1)
1-57: LGTM!The documentation clearly explains the version checking design decisions including proxy support, TLS configuration, retry logic, and error handling. The environment variables table provides a helpful quick reference.
docs/CI_CD_AUTHENTICATION.md (1)
1-926: LGTM!This is a comprehensive and well-structured CI/CD authentication guide. It effectively covers the spectrum from recommended OIDC federation to fallback IAM access keys, with practical examples for major CI/CD platforms and clear security considerations.
tests/QA_AUTHAWS_TESTS.md (1)
1-469: LGTM!The QA test suite is comprehensive, covering backward compatibility, flag-based parameters, mixed syntax, error handling, edge cases, and performance. The inclusion of test data cleanup procedures and the warning about not committing real credentials demonstrates good practices.
docs/PRODUCTION_VERSION_FIX.md (1)
1-35: LGTM!The documentation clearly explains the production version check issues and their solutions. The lowercase "api.github.com" is correct as it's a domain name reference.
docs/MULTI_REGION.md (1)
1-317: LGTM!The multi-region operations guide is comprehensive, covering configuration, command usage, execution control, filtering, error handling, and troubleshooting. The use cases section provides practical examples for security patching, configuration auditing, and log collection.
docs/CI_CD_PIPELINE.md (1)
1-442: LGTM - Well-documented pipeline architecture.The CI/CD pipeline documentation provides excellent coverage of the design philosophy, job architecture, and workflow behavior. The mermaid diagram and behavior matrix effectively communicate the pipeline flow. Minor version inconsistencies noted separately.
docs/COMMANDS.md (1)
271-318: LGTM on Interactive Fuzzy Finder documentation.The Interactive Fuzzy Finder section is well-structured with clear feature descriptions, keyboard shortcuts, instance information details, and practical workflow examples. Documentation clearly explains when fuzzy finder launches and how to use it.
tests/QA_SSM_TESTS.md (5)
1-26: Verify test coverage for RDS and SSM SSH features mentioned in PR objectives.This QA document is scoped to "SSM Flag-Based Parameters QA Test Suite" and documents testing for flag-based parameter support. However, the AI summary states the PR "Adds RDS CLI commands and SSM SSH/SSH-config/RDP features" and includes "new unit tests for RDS and SSM SSH features."
This document does not cover:
- RDS command testing (list, start, stop, reboot operations)
- SSM SSH feature testing (SSH proxy, SSH-config generation, RDP features)
Either:
- Separate QA documents should exist for RDS and SSM SSH features and should be included in this review, or
- New test sections should be added to this document to cover these features
Please verify that all QA documentation for the PR is included and complete.
472-518: LGTM on test execution checklist and expected results.The Test Execution Checklist and Expected Results sections are well-organized with clear, measurable criteria. Success and failure criteria appropriately cover backward compatibility, flag support, syntax mixing, error handling, and performance. The checklist provides good coverage of pre-test setup, execution phases, and post-test validation.
39-81: LGTM on test data setup and validation procedures.The Test Data Setup section provides clear, practical instructions for environment configuration with safety warnings (
⚠️ WARNING note on line 46 about not committing sensitive test data). Thevalidate_instance_id()function provides validation before test execution, and test file creation is properly documented. This setup approach reduces errors and improves test reliability.
536-591: LGTM on reporting and maintenance sections.The Test Report Template provides comprehensive fields for documenting test execution (date, tester, environment details, summary counts, findings). Maintenance section appropriately covers ongoing testing procedures and test case update guidance to keep the test suite current as features evolve.
1-80: The QA test document correctly focuses on the flag-based parameter support feature for thessmbash script. The commit message mentions signal handling improvements ("Implemented signal draining in session management"), but these are in the ztictl Go tool (ztictl/cmd/ztictl/ssm_exec_multi.go), not the bashssmscript covered by this QA document. No signal handling enhancements to the bash script itself are present in this PR, so the test coverage is appropriately scoped for the documented feature.
…ributing guidelines and CI/CD documentation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/CI_CD_PIPELINE.md (1)
380-388: Approved: Past version inconsistency has been corrected.The previous review comment flagging Go version (1.25 vs 1.24) and golangci-lint (v2.4.0 vs v1.61.0) discrepancies in the Maintenance section has been properly addressed. The section now correctly reflects Go 1.24 and golangci-lint v1.61.0. The added note at line 387 instructing maintainers to keep values in sync is a good improvement.
.github/workflows/build.yml (1)
124-128: Go version 1.24 is outdated.Per previous review, Go 1.25.5 was released in December 2025. Consider updating to
1.25across all jobs (also at lines 227 and 311) to benefit from the latest improvements and security fixes.
🧹 Nitpick comments (2)
.github/workflows/build.yml (2)
252-256: Consider failing the build on vulnerability findings.The
continue-on-error: trueongovulncheckmeans known vulnerabilities in dependencies won't block the PR. While this approach allows gradual adoption, consider at least failing on HIGH/CRITICAL severity findings to prevent merging code with known exploitable vulnerabilities.🔎 Example: Fail on vulnerabilities
- name: Run Go vulnerability check working-directory: ./ztictl - run: govulncheck ./... || true - continue-on-error: true + run: govulncheck ./...
386-388: Add existence check for install scripts.The step copies
scripts/install.shandscripts/install.ps1without verifying they exist. If these files are missing, the release job will fail silently at this point.🔎 Proposed defensive check
- cp scripts/install.sh release/ - cp scripts/install.ps1 release/ + [ -f scripts/install.sh ] && cp scripts/install.sh release/ + [ -f scripts/install.ps1 ] && cp scripts/install.ps1 release/
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/ISSUE_TEMPLATE/bug_report.md.github/workflows/build.ymlCONTRIBUTING.mdINSTALLATION.mddocs/CI_CD_PIPELINE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- CONTRIBUTING.md
- .github/ISSUE_TEMPLATE/bug_report.md
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/build.yml
[high] 137-138: AWS Access Key
(CKV_SECRET_2)
[high] 138-139: AWS Access Key
(CKV_SECRET_2)
🪛 LanguageTool
docs/CI_CD_PIPELINE.md
[uncategorized] ~29-~29: The official name of this software platform is spelled with a capital “H”.
Context: ...lized workflows: ### Primary Workflow: .github/workflows/build.yml (Unified CI/CD) *...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 2b. Test (macos-latest)
- GitHub Check: 2b. Test (windows-latest)
- GitHub Check: 2b. Test (ubuntu-latest)
🔇 Additional comments (7)
INSTALLATION.md (1)
1-455: Comprehensive documentation update—well executed and complete.This extensive revision properly documents ztictl installation across platforms (Linux AMD64/ARM64, macOS Intel/Apple Silicon, Windows) with clear setup instructions, PATH configuration, usage examples, and troubleshooting guidance. The transition from deprecated bash tools to the modern Go binary is well-explained with clear migration messaging.
All code blocks are properly labeled with language specifiers (bash, powershell), which aligns with markdown linting standards. Download URLs follow the correct GitHub release pattern, and the Go 1.24+ requirement is current (as of February 2025 release). The document structure is clear and user-friendly, with appropriate sections for quick install, platform-specific setup, building from source, and legacy tool support.
.github/workflows/build.yml (6)
18-48: Well-designed change detection gate.The path-based filtering approach efficiently skips unnecessary jobs. The conditional logic correctly excludes
workflow_dispatchand tag pushes where change detection isn't applicable.
88-108: Script injection vulnerability properly remediated.The PR metadata is now passed through environment variables instead of direct interpolation, addressing the previously flagged script injection concern. The variables are properly quoted when passed to the script.
136-140: Static analysis false positive - these are well-known AWS example credentials.The flagged credentials (
AKIAIOSFODNN7EXAMPLEandwJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY) are official placeholder credentials from AWS documentation, intentionally used for testing withAWS_EC2_METADATA_DISABLED: 'true'. These are not real secrets and pose no security risk.
317-345: Build logic is well-structured.Good version derivation from tags vs commits, proper binary naming conventions per platform, and the smoke test on linux-amd64 verifies the build works before uploading artifacts.
408-437: Release notification properly implemented.Tag format validation, fail-fast settings, and
continue-on-error: trueensure notifications don't block releases while still catching invalid tag formats early.
238-246: The Trivy action is already pinned to the latest version.The workflow uses
aquasecurity/trivy-action@0.33.1, which is the current latest release (published September 3, 2025). No version update is needed.Likely an incorrect or invalid review comment.
… in CI/CD documentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/CI_CD_PIPELINE.md (1)
301-301: Minor: Markdown table syntax issue.Line 301 uses
v\*which may not render correctly in the Markdown table. Consider using inline code formatting or unescaped syntax for consistency with other table rows.🔎 Proposed fix
-| **Tag push (v\*)** | `build` → `release` → `notification` | Full release pipeline | +| **Tag push (v*)** | `build` → `release` → `notification` | Full release pipeline |
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/CI_CD_PIPELINE.md
🧰 Additional context used
🪛 LanguageTool
docs/CI_CD_PIPELINE.md
[uncategorized] ~29-~29: The official name of this software platform is spelled with a capital “H”.
Context: ...lized workflows: ### Primary Workflow: .github/workflows/build.yml (Unified CI/CD) *...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 2b. Test (windows-latest)
🔇 Additional comments (2)
docs/CI_CD_PIPELINE.md (2)
174-174: ✅ Version consistency resolved.The documentation now consistently references Go 1.24 and golangci-lint v1.61.0 across all sections (lines 174, 382, 384, 411). The addition of the sync note at line 387 is helpful for future maintenance.
Also applies to: 384-384, 411-411
391-391: Go version statement is accurate. The claim that Go 1.25.5 is the latest stable release as of December 2025 has been verified and is correct.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.