Repository navigation
fix(io): migrate fmt.Fprintf/Println anti-patterns to pkg/ui, pkg/data - #2713
Merged
Merged
Conversation
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
July 10, 2026 12:40
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Contributor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Resource Changes Found for
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates output to shared ChangesOutput and interaction migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/ci/providers/generic/provider.go (1)
181-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment: says "write to stderr" but now uses
ui.Writef.The comment on line 181 still reads "No summary file configured - write to stderr." but the code now routes through
ui.Writef, which writes through the UI formatter channel, not directly to stderr. Update the comment to reflect the new output path.♻️ Update stale comment
- // No summary file configured - write to stderr. - // This makes the summary visible in local testing. + // No summary file configured - write through the UI channel. + // This makes the summary visible in local testing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ci/providers/generic/provider.go` around lines 181 - 183, Update the stale comment above the ui.Writef call in the generic provider’s summary-output logic to describe writing through the UI formatter/channel instead of directly to stderr.cmd/ai/sessions.go (1)
289-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
ui.Writelnfor human-facing session messages.These messages (e.g., "✅ Deleted N session(s)...", "Resume the session with: ...") are human-facing UI messages, not pipeable data. Per coding guidelines, human-facing UI messages should go through
ui.*functions. Usingui.Writelnwould also remove the_ =error-discarding pattern since it's void.As per coding guidelines: "Write pipeable data through data.Write/data.Writef/data.Writeln/data.WriteJSON/data.WriteYAML and human-facing UI messages through ui functions."
♻️ Suggested refactor
func cleanSessionsCommand(cmd *cobra.Command, args []string) error { // ... if count == 0 { - _ = data.Writeln("No sessions to clean.") + ui.Writeln("No sessions to clean.") } else { - _ = data.Writeln(fmt.Sprintf("✅ Deleted %d session(s) older than %s", count, olderThanStr)) + ui.Writeln(fmt.Sprintf("✅ Deleted %d session(s) older than %s", count, olderThanStr)) } // ... }Apply the same pattern to
exportSessionCommand(line 367) andimportSessionCommand(lines 408-414), replacing_ = data.Writeln(...)withui.Writeln(...).Also applies to: 367-367, 408-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ai/sessions.go` around lines 289 - 291, Replace human-facing session status and guidance messages emitted by the affected cleanup, exportSessionCommand, and importSessionCommand paths with ui.Writeln instead of data.Writeln; remove the _ = error-discarding assignments. Keep data writers only for pipeable output, including messages such as “No sessions to clean,” deletion summaries, and resume instructions.Source: Coding guidelines
pkg/devcontainer/lifecycle_config.go (1)
37-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider propagating write errors from print functions.
All print helpers silently discard
data.Writef/data.Writelnerrors with_ =. While this matches the priorfmt.Printfbehavior (which also ignored errors),ShowConfigreturnserrorand could propagate write failures (e.g., broken pipe) if the print functions returnederror. This is a pre-existing pattern made explicit by the migration, so deferring is fine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/lifecycle_config.go` around lines 37 - 153, Defer this refactor for now; retain the existing ignored data.Writef/data.Writeln errors across printBasicInfo, printBuildInfo, printWorkspaceInfo, printMounts, printPorts, printEnv, printRunArgs, and printRemoteUser. If addressed later, change each helper to return and propagate errors through ShowConfig rather than discarding them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/root.go`:
- Around line 2270-2313: Restore the command’s original output writer after
buffered help rendering so the --cast tee remains active. In both renderFlagHelp
and renderInteractiveHelp, save command.OutOrStdout() before
command.SetOut(&buf), then restore it after command.Help() (ideally with defer)
while preserving the buffered content for pager output.
In `@pkg/data/data_test.go`:
- Around line 441-477: TestWriteUnmasked_BypassesMasking leaks the package-level
I/O context and registered secret. Save the existing globalIOContext before
calling InitWriter(ioCtx), then restore it with defer immediately after setup,
while retaining direct ioCtx access for masker registration.
In `@pkg/ui/formatter.go`:
- Around line 290-294: In MarkdownMessageNoWrap, the MarkdownNoWrap error branch
overwrites its correctly formatted fallback with raw content. Remove the
rendered = content override and preserve the rendered value returned by
globalFormatter.MarkdownNoWrap, including its trimmed content and trailing
newline.
In `@tests/snapshots/TestCLICommands_atmos_support.stdout.golden`:
- Line 8: Fix the malformed GitHub URL in the snapshot output by locating the
source text or wrapping/no-wrap Markdown logic that produces “https://github.
com/cloudposse/atmos/issues” and preserving
“https://github.com/cloudposse/atmos/issues” without the inserted space; update
the golden snapshot only if the generated output is correct.
---
Nitpick comments:
In `@cmd/ai/sessions.go`:
- Around line 289-291: Replace human-facing session status and guidance messages
emitted by the affected cleanup, exportSessionCommand, and importSessionCommand
paths with ui.Writeln instead of data.Writeln; remove the _ = error-discarding
assignments. Keep data writers only for pipeable output, including messages such
as “No sessions to clean,” deletion summaries, and resume instructions.
In `@pkg/ci/providers/generic/provider.go`:
- Around line 181-183: Update the stale comment above the ui.Writef call in the
generic provider’s summary-output logic to describe writing through the UI
formatter/channel instead of directly to stderr.
In `@pkg/devcontainer/lifecycle_config.go`:
- Around line 37-153: Defer this refactor for now; retain the existing ignored
data.Writef/data.Writeln errors across printBasicInfo, printBuildInfo,
printWorkspaceInfo, printMounts, printPorts, printEnv, printRunArgs, and
printRemoteUser. If addressed later, change each helper to return and propagate
errors through ShowConfig rather than discarding them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f555c66b-e29d-42dd-8e7b-092079395710
📒 Files selected for processing (87)
cmd/ai/ask.gocmd/ai/sessions.gocmd/ai/testmain_test.gocmd/auth/env.gocmd/cmd_utils.gocmd/cmd_utils_test.gocmd/custom_command_integration_test.gocmd/devcontainer/helpers.gocmd/devcontainer/interfaces.gocmd/devcontainer/mock_interfaces_test.gocmd/devcontainer/providers.gocmd/devcontainer/providers_test.gocmd/devcontainer/service.gocmd/devcontainer/service_test.gocmd/docs.gocmd/list/settings.gocmd/list/values.gocmd/root.gocmd/support.gocmd/testing_helpers_test.gocmd/testing_main_test.gocmd/version/formatters.gocmd/version/testmain_test.gointernal/exec/atmos.gointernal/exec/docs_generate.gointernal/exec/pro.gointernal/exec/shell_utils.gointernal/exec/testmain_test.gointernal/exec/validate_schema.gointernal/exec/version.gointernal/exec/workflow_adapters.gointernal/exec/workflow_utils.gopkg/ai/context.gopkg/ai/session/manager.gopkg/ai/tools/permission/checker_test.gopkg/ai/tools/permission/prompter.gopkg/ai/tools/permission/testmain_test.gopkg/atlantis/testmain_test.gopkg/auth/cloud/kube/config.gopkg/auth/credentials/store.gopkg/auth/identities/aws/webflow_ui.gopkg/auth/providers/aws/sso.gopkg/aws/aws_eks_update_kubeconfig_test.gopkg/ci/providers/generic/provider.gopkg/ci/providers/github/annotations.gopkg/ci/providers/github/annotations_test.gopkg/ci/providers/github/log_group.gopkg/ci/providers/github/log_group_test.gopkg/component/mock/mock.gopkg/data/data.gopkg/data/data_test.gopkg/devcontainer/lifecycle_config.gopkg/devcontainer/lifecycle_list.gopkg/devcontainer/testmain_test.gopkg/env/output.gopkg/env/testmain_test.gopkg/hooks/main_test.gopkg/list/list_instances.gopkg/manifest/render.gopkg/manifest/testmain_test.gopkg/provisioner/source/cmd/testmain_test.gopkg/runner/step/cast.gopkg/runner/step/cast_simulate.gopkg/runner/step/testmain_test.gopkg/telemetry/testmain_test.gopkg/telemetry/utils.gopkg/telemetry/utils_test.gopkg/ui/formatter.gopkg/ui/formatter_test.gopkg/ui/interfaces.gopkg/ui/output_test.gopkg/utils/doc_utils.gopkg/utils/go_getter_utils_test.gopkg/utils/json_utils.gopkg/utils/log_utils.gopkg/utils/log_utils_test.gopkg/utils/markdown_utils.gopkg/utils/markdown_utils_test.gopkg/utils/version_utils.gopkg/utils/yaml_utils.gotests/snapshots/TestCLICommands_atmos.stderr.goldentests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config.stderr.goldentests/snapshots/TestCLICommands_atmos_doesnt_warn_if_in_git_repo_with_no_atmos_config.stderr.goldentests/snapshots/TestCLICommands_atmos_list_instances.tty.goldentests/snapshots/TestCLICommands_atmos_support.stdout.goldentests/snapshots/TestCLICommands_atmos_warns_if_not_in_git_repo_with_no_atmos_config.stderr.goldentests/snapshots/TestCLICommands_check_atmos_in_empty-dir.stderr.golden
💤 Files with no reviewable changes (11)
- pkg/utils/markdown_utils_test.go
- pkg/aws/aws_eks_update_kubeconfig_test.go
- cmd/devcontainer/interfaces.go
- cmd/devcontainer/service_test.go
- pkg/utils/markdown_utils.go
- cmd/devcontainer/providers.go
- pkg/utils/log_utils_test.go
- pkg/telemetry/utils_test.go
- cmd/devcontainer/providers_test.go
- cmd/devcontainer/mock_interfaces_test.go
- pkg/utils/log_utils.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/data/data_test.go`:
- Around line 721-809: Update setupTestIO to capture the current package-level
markdown renderer before tests run and restore it during cleanup, alongside
globalIOContext. Ensure all tests using SetMarkdownRenderer, including
TestMarkdownNoWrap and its fallback/error variants, leave the original renderer
intact for subsequent tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb1c9f2e-f9b8-43b3-9d01-ad0698bd5af4
📒 Files selected for processing (7)
cmd/help_template_test.gocmd/root.gocmd/support.gopkg/data/data.gopkg/data/data_test.gopkg/ui/formatter.gotests/snapshots/TestCLICommands_atmos_support.stdout.golden
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/TestCLICommands_atmos_support.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/support.go
- pkg/ui/formatter.go
- cmd/root.go
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Jul 10, 2026
… markdown renderer in data test helper - Bump the shared macOS/Windows "Acceptance tests" step timeout from 45 to 60 minutes. Windows has repeatedly run right up against the 45-minute cap and gotten force-cancelled before finishing, failing the job with no real test failure in the logs; still comfortably inside the job's 75-minute overall timeout. - pkg/data/data_test.go: setupTestIO now also snapshots and restores globalMarkdownRender (not just globalIOContext), so tests calling SetMarkdownRenderer no longer leak a mock renderer into later tests (CodeRabbit review comment on PR #2713).
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (84.11%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2713 +/- ##
==========================================
+ Coverage 81.32% 81.61% +0.29%
==========================================
Files 1641 1639 -2
Lines 154729 154739 +10
==========================================
+ Hits 125830 126290 +460
+ Misses 21974 21533 -441
+ Partials 6925 6916 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Jul 10, 2026
Adds targeted unit tests for the largest patch-coverage gaps flagged by Codecov on this branch (144 uncovered added lines across 19 files): - cmd/root.go: help-rendering cluster (initCobraConfig, rootUsageFunc, isHelpRequested, parsePagerFlagValue, renderFlagHelp, renderInteractiveHelp, renderRootHelp, rootHelpFunc) via new cmd/root_help_render_test.go. - pkg/auth/cloud/kube/config.go: debug-diff comparison helpers (configContentEqual, *MapsEqualDebug, mergeWouldChange), 100% covered. - pkg/data/data.go + pkg/ui/formatter.go: WriteUnmasked/MarkdownNoWrap error paths and MarkdownMessageNoWrap/markdownRenderWidth branches. - 8 small cmd/* files (auth/env, version/formatters, list/values, list/settings, support, ai/ask, ai/sessions, devcontainer/helpers) and 7 small pkg/*+internal/exec files (ai/context, auth/providers/aws/sso, runner/step/cast_simulate, exec/atmos, exec/workflow_utils, ai/session/manager, auth/credentials/store). pkg/ai/session/compactor.go gets a //go:generate mockgen directive (plus the generated pkg/ai/session/mock_compactor.go) so Compactor can be mocked for the manager_test.go additions - no behavior change. A handful of lines (~11) remain uncovered where testing would require faking a real TTY (boa/bubbletea usage rendering, isatty-gated devcontainer prompt) or adding a DI seam with no other use, documented inline.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ai/context_test.go`:
- Around line 16-51: Add a dedicated “closed pipe returns error” subtest
alongside the existing cases in TestPromptForConsent: replace os.Stdin with a
pipe, close the write end without writing input, invoke PromptForConsent, and
assert that an error is returned. Ensure stdin restoration and pipe
setup/cleanup match the existing test pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f1793a3-177c-48db-9e61-5358374a953f
📒 Files selected for processing (21)
cmd/ai/ask_test.gocmd/ai/sessions_test.gocmd/auth/env_test.gocmd/list/settings_test.gocmd/list/values_test.gocmd/root_help_render_test.gocmd/support_test.gocmd/version/formatters_test.gointernal/exec/atmos_test.gointernal/exec/workflow_utils_test.gopkg/ai/context_test.gopkg/ai/session/compactor.gopkg/ai/session/manager_test.gopkg/ai/session/mock_compactor.gopkg/auth/cloud/kube/config_diff_test.gopkg/auth/credentials/store_test.gopkg/auth/providers/aws/sso_test.gopkg/data/data_test.gopkg/runner/step/cast_simulate_test.gopkg/ui/formatter_test.gopkg/ui/output_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/ai/session/compactor.go
- pkg/ai/session/mock_compactor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/data/data_test.go
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Jul 10, 2026
Add a "closed pipe returns error" case to TestPromptForConsent for the reader.ReadString EOF-without-delimiter branch, per CodeRabbit review on PR #2713.
…data Audits and migrates every remaining raw fmt.Fprintf/Fprintln/Println call outside pkg/io/pkg/ui to the sanctioned ui.*/data.* API, so all output consistently goes through secret masking, TTY/color degradation, and --cast recording as CLAUDE.md requires. Several call sites (AI permission prompts, devcontainer/kubeconfig debug diagnostics, CI annotations, the Cobra help writer) previously bypassed masking entirely. - Add data.WriteUnmasked/WriteUnmaskedf, an explicit escape hatch for the one legitimate case where masking must not apply: atmos auth env / atmos env emitting real credential values for shell eval. - Add ui.MarkdownNoWrap/MarkdownMessageNoWrap for deterministic, snapshot-stable single-line notices (used by the telemetry disclosure). - Delete the pure print-forwarding shims in pkg/utils (PrintMessage/PrintfMessageToTUI/PrintfMarkdown*) and migrate all ~50 call sites directly to data.*/ui.*, consolidating onto the already-established ui.Markdown/MarkdownMessage renderer instead of a second, bespoke one. - Fix RootCmd's default output writer (cmd/root.go) to route through the masked I/O context instead of raw os.Stdout, without touching Cobra's help/usage template writer convention; extracted the oversized help/usage closures into named helpers while there. - Correct pkg/devcontainer's ShowConfig/List output to the data channel (primary command output) instead of the UI channel it was mismigrated to initially. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, stabilize CI markdown tests - data.MarkdownNoWrap/MarkdownNoWrapf render markdown without word-wrap so long inline URLs in `atmos support` no longer get hard-broken mid-domain (e.g. "https://github. com/...") when they don't fit the wrap width. - renderFlagHelp/renderInteractiveHelp restore the command's original writer after buffering into a pager, so it isn't left pointed at a discarded buffer for output written after the function returns. - cmd/help_template_test.go strips ANSI codes before asserting on rendered description text, since CI=true forces color output (see terminal.DetectColorProfile) and Glamour wraps each word in its own style span, breaking plain substring checks.
… markdown renderer in data test helper - Bump the shared macOS/Windows "Acceptance tests" step timeout from 45 to 60 minutes. Windows has repeatedly run right up against the 45-minute cap and gotten force-cancelled before finishing, failing the job with no real test failure in the logs; still comfortably inside the job's 75-minute overall timeout. - pkg/data/data_test.go: setupTestIO now also snapshots and restores globalMarkdownRender (not just globalIOContext), so tests calling SetMarkdownRenderer no longer leak a mock renderer into later tests (CodeRabbit review comment on PR #2713).
Adds targeted unit tests for the largest patch-coverage gaps flagged by Codecov on this branch (144 uncovered added lines across 19 files): - cmd/root.go: help-rendering cluster (initCobraConfig, rootUsageFunc, isHelpRequested, parsePagerFlagValue, renderFlagHelp, renderInteractiveHelp, renderRootHelp, rootHelpFunc) via new cmd/root_help_render_test.go. - pkg/auth/cloud/kube/config.go: debug-diff comparison helpers (configContentEqual, *MapsEqualDebug, mergeWouldChange), 100% covered. - pkg/data/data.go + pkg/ui/formatter.go: WriteUnmasked/MarkdownNoWrap error paths and MarkdownMessageNoWrap/markdownRenderWidth branches. - 8 small cmd/* files (auth/env, version/formatters, list/values, list/settings, support, ai/ask, ai/sessions, devcontainer/helpers) and 7 small pkg/*+internal/exec files (ai/context, auth/providers/aws/sso, runner/step/cast_simulate, exec/atmos, exec/workflow_utils, ai/session/manager, auth/credentials/store). pkg/ai/session/compactor.go gets a //go:generate mockgen directive (plus the generated pkg/ai/session/mock_compactor.go) so Compactor can be mocked for the manager_test.go additions - no behavior change. A handful of lines (~11) remain uncovered where testing would require faking a real TTY (boa/bubbletea usage rendering, isatty-gated devcontainer prompt) or adding a DI seam with no other use, documented inline.
Add a "closed pipe returns error" case to TestPromptForConsent for the reader.ReadString EOF-without-delimiter branch, per CodeRabbit review on PR #2713.
…er CI=true CI=true forces color output (see terminal.DetectColorProfile), which made Glamour wrap "test command"/" description" in separate ANSI style spans and broke the plain assert.Contains checks - same class of failure fixed earlier for TestRenderMarkdownDescription/TestPrintDescription.
Bubble Tea fails fast when it can't open a controlling TTY on Unix (openInputTTY falls back to /dev/tty and errors immediately), which this test relied on to reach ExecuteAtmosCmd's error-return path without a real interactive session. On Windows it instead reads via syscall.ReadConsole against the process's console handle, which blocks waiting for real input instead of erroring - hanging the whole internal/exec test binary until the CI job's 40-minute timeout killed it.
Erik Osterman (Cloud Posse) (osterman)
force-pushed
the
osterman/pr-instructions
branch
from
July 11, 2026 00:10
9f4f026 to
2c3574b
Compare
Every push was rerunning the full commit-time hook suite (go build, lint, go mod tidy, etc.) a second time on an already-verified tree, adding several minutes of redundant work to every push. Add default_stages: [pre-commit] (plus default_install_hook_types) so hooks only fire at commit time; three pre-commit-hooks.yaml hooks (trailing-whitespace, end-of-file-fixer, check-added-large-files) hardcode their own stages upstream and need an explicit override to respect this too. CI's own lint/build jobs still re-verify everything server-side on push, so this doesn't remove coverage for the rebase/amend/cherry-pick case where the local tree changes without a fresh commit-time run - CI is the backstop for that.
Same class of hang as TestExecuteAtmosCmd_TUIStartFailureIsReturned: Bubble Tea fails fast when it can't open a controlling TTY on Unix, which this test relied on to reach ExecuteWorkflowUI's error-return path without a real interactive session. On Windows it instead reads via syscall.ReadConsole against the process's console handle, which blocks waiting for real input instead of erroring - hanging the whole internal/exec test binary until the CI job's 40-minute timeout killed it.
…om merging independent toast lines The migration to ui.Success/Info/Warning (3faadd1) replaced hand-rolled "\n%s ...\n\n" Writef calls with the new single-line toast API, silently dropping the blank-line padding those calls used to emit around auth login, docs generate, pro lock/unlock, and the version latest-check message. Separately, tests/cli_test.go's snapshot-normalization pass was blindly merging independent icon-prefixed toast lines (e.g. the telemetry notice and the experimental-feature notice) onto one line since it had no way to tell a wrapped paragraph continuation from two distinct ui.* calls; fix it at the root by deriving the non-mergeable prefix set from pkg/ui/theme's icon constants instead of patching one glyph at a time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
# Conflicts: # internal/exec/workflow_adapters.go
…save The Build (windows) job's 30-minute timeout was tuned before this branch merged in a much larger dependency tree (AI/MCP/LSP/k8s/OPA/Helm v4), so a cold-cache Windows build now runs the job right up against that budget and gets killed mid cache-save (observed: killed at 30m10s), which cascades into downstream jobs (e.g. the k3s demo-helmfile gate) reporting a false failure since their upstream build never finished. Bump to 45 minutes for real headroom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ble width test
TestCreateVersionTable_IndicatorColumnStaysNarrow asserted the byte offset of
"VERSION" in the rendered header line directly, but under CI=true the header
cell renders bold ("\x1b[1mVERSION"), and strings.Index counted those escape
bytes as part of the column position (expected 4, got 8) — same class of bug
already fixed elsewhere in this file's captureStderr helper (bfdccd0).
Strip ANSI from the table string before splitting into lines, consistent
with that precedent.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Jul 13, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.223.0-rc.10. |
This was referenced Jul 19, 2026
This was referenced Aug 6, 2026
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 19, 2026
…ionale All 7 actions/setup-go steps in .github/workflows/test.yml had cache: false, two with a comment claiming this avoided "PR-controlled keys" poisoning the cache across PRs or into main. That rationale doesn't hold: GitHub Actions already isolates this. A pull_request run gets read/write access only to its own branch's cache scope, read-only access to the default branch's cache, and cannot restore caches from sibling PRs; only push/ workflow_dispatch/schedule/etc. can write to the default branch's cache scope at all - pull_request and merge_group both get read-only access to it. There's no cross-PR or PR->main poisoning path for this to guard against. The one real, historically-documented incident (PR #2713, 2026-07-13) was a cold-cache Windows build blowing through its then-30-minute timeout during the post-job cache-save step, not a poisoning attack - fixed at the time by bumping the timeout to 45 minutes (still in place today). cache: false was introduced separately and later (2026-08-02, commit 9343caf, a large squashed refactor(store) PR whose own commit message never mentions caching), with no documented rationale beyond the now-corrected comment. Restores cache: true explicitly (not just the implicit default) on all 7 steps so there's a concrete anchor for the corrected explanatory comment, kept once on the build job's step rather than duplicated 7x. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Michael Pursifull (arcaven)
pushed a commit
to arcaven/atmos
that referenced
this pull request
Aug 19, 2026
…dup (cloudposse#2955) * refactor(lint): replace bash lint orchestration with go-native Mage tooling Ports the bash staleness-check/build orchestration for custom-gcl, lintroller, and gomodcheck (in .atmos.d/lint.yaml and scripts/run-custom-golangci-lint.sh) to Go targets in magefiles/, invoked via `go tool mage` (Go 1.24+ tool directive). Fixes latent POSIX-only staleness checks (find -newer) that silently never worked on Windows, and replaces a hand-rolled cached-binary staleness check for gomodcheck with a simpler `go run` (the pre-commit hook already did this, unifying both call sites for the first time). The never-build-custom-gcl-in-a-pre-commit-hook invariant (previously fixed after a worktree corruption incident) is preserved exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate CodeQL allocation-size-overflow alerts Replace make(T, len(base)+len(overlay)) capacity hints with max(len(base), len(overlay)) in mergeEnvSlices to avoid the summed-len overflow pattern CodeQL flags (go/allocation-size-overflow, alerts #5326-#5329). max() of two independently-safe lengths carries no overflow risk, unlike their sum. Also fixes pre-existing EditorConfig indentation (3-space list continuations under numbered items, want multiples of 2) in tools/lintroller/README.md that was blocking this commit's affected-file validation hook. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(mage): address CodeRabbit findings on the Mage lint migration Fixes 4 review threads on PR cloudposse#2955: lintrollerIsStale ignored go.mod/go.sum changes, CustomGCL built golangci-lint in the caller's cwd instead of the repo root, Precommit only checked binary existence (not staleness), and mageRepoRoot's HasPrefix match could stop at tools/lintroller's own go.mod instead of walking to the real root. Adds unit test coverage for magefiles/ (previously invisible to `go test ./...` under the mage build tag) and a standalone CI job + Codecov wiring so it's no longer untested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(deps): bump moby/go-archive to v0.3.0 for path-traversal fix Dependabot alert cloudposse#277 (GHSA-hfg8-hc9c-6c3h, high): a crafted tar archive could write outside the extraction directory in github.com/moby/go-archive < v0.3.0. v0.2.0 -> v0.3.0 is a semver-minor bump, not blocked by .github/dependabot.yml's major-version-only ignore rule. Transitively pulls moby/sys/sequential v0.7.0 and moby/sys/user v0.4.1 via `go mod tidy`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf(lint): switch import formatter from goimports to gci goimports takes ~95-108s for a full-repo custom-gcl fmt pass; gci with explicit import-section ordering (standard/default/cloudposse-prefix) does the same pass in ~5-7s, a ~15-20x speedup, benchmarked directly against this repo. gci is a stock golangci-lint v2 formatter, so no .custom-gcl.yml plugin change is needed. Updates the two doc references (CLAUDE.md, lint-fix/test-coverage-fix agents) that named goimports as the enforced formatter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docs): correct pre-existing EditorConfig indentation in agent guardrails .claude/agents/lint-fix.md and .claude/agents/test-coverage-fix.md used a 3-space continuation indent under numbered-list items, violating this repo's .editorconfig (markdown indent_size: 2, left-padding must be a multiple of 2). Pre-existing on origin/main, but only surfaced once these files were touched by the goimports->gci commit, since `atmos validate --affected` validates whole files once they enter the affected set rather than just changed lines. CI's "Run pre-commit hooks" job on PR cloudposse#2955 caught this (Validate EditorConfig failed with the same 11 findings reproduced locally). Also adds the fix-log record for the goimports->gci swap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(magefiles): close patch coverage gaps flagged by Codecov on PR cloudposse#2955 Codecov reported this PR's patch coverage at 80.80% (48 lines missing) across magefiles/mage_lint_changed.go (0%), mage_lint_lintroller.go (65.85%), mage_lint_customgcl.go (78.57%), mage_lint_golangci_run.go (86.45%), magefile.go (85.71%), mage_lint_precommit.go (84.61%), and mage_lint_gomodcheck.go (90.00%) — below CLAUDE.md's mandatory >85%. Adds tests exercising real failure-propagation and build-orchestration paths (repo-root resolution errors, staleness-check errors via ENOTDIR, an end-to-end go-build-and-run against a trivial stand-in program, go list parse failures, gomodcheck's success path) rather than padding coverage. Remaining gaps are left uncovered with stated reasons: Windows-only branches (unreachable on this CI runner), and a few genuinely impractical-to-reproduce OS-level races/failures (fd close errors, TOCTOU on directory walks). Local coverage on the touched functions is now at or near 100% for every function Codecov flagged; magefiles/acceptance.go is untouched (not part of this branch's diff vs origin/main, correctly excluded from Codecov's patch view). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): restore Go build cache in test.yml, correct the security rationale All 7 actions/setup-go steps in .github/workflows/test.yml had cache: false, two with a comment claiming this avoided "PR-controlled keys" poisoning the cache across PRs or into main. That rationale doesn't hold: GitHub Actions already isolates this. A pull_request run gets read/write access only to its own branch's cache scope, read-only access to the default branch's cache, and cannot restore caches from sibling PRs; only push/ workflow_dispatch/schedule/etc. can write to the default branch's cache scope at all - pull_request and merge_group both get read-only access to it. There's no cross-PR or PR->main poisoning path for this to guard against. The one real, historically-documented incident (PR cloudposse#2713, 2026-07-13) was a cold-cache Windows build blowing through its then-30-minute timeout during the post-job cache-save step, not a poisoning attack - fixed at the time by bumping the timeout to 45 minutes (still in place today). cache: false was introduced separately and later (2026-08-02, commit 9343caf, a large squashed refactor(store) PR whose own commit message never mentions caching), with no documented rationale beyond the now-corrected comment. Restores cache: true explicitly (not just the implicit default) on all 7 steps so there's a concrete anchor for the corrected explanatory comment, kept once on the build job's step rather than duplicated 7x. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.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
2 participants
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.
what
fmt.Fprintf/fmt.Fprintln/fmt.Printlncall outsidepkg/io/pkg/uito the sanctionedui.*/data.*API across ~40 files (AI permission prompts, devcontainer/kubeconfig debug diagnostics, CI annotations/log groups, the Cobra root command,pkg/manifest,pkg/runner/stepcast recording, and more).pkg/utils(PrintMessage,PrintfMessageToTUI,PrintfMarkdown*) and migrate all ~50 call sites directly ontodata.*/ui.*, consolidating onto the already-establishedui.Markdown/ui.MarkdownMessagerenderer instead of a second, bespoke one.data.WriteUnmasked/WriteUnmaskedf, an explicit escape hatch for the one legitimate case where masking must not apply (atmos auth env/atmos envemitting real credential values for shell eval).ui.MarkdownNoWrap/MarkdownMessageNoWrapfor deterministic, snapshot-stable single-line notices (used by the telemetry disclosure).RootCmd's default output writer to route through the masked I/O context instead of rawos.Stdout, and extract the oversized Cobra help/usage closures into named helpers while there.pkg/devcontainer'sShowConfig/Listoutput to the data channel (primary command output) instead of the UI channel, and update the affected CLI golden snapshots to match.why
fmt.Fprintf/fmt.Printlnoutsidepkg/io/pkg/uibecause it bypasses secret masking, TTY/color degradation, and--castrecording. Several call sites found in this audit (AI permission prompts, devcontainer/kubeconfig debug output, CI annotations, the Cobra help writer) genuinely bypassed masking entirely.pkg/utilsprint helpers were pure forwarding shims duplicatingpkg/ui/pkg/data, adding an unnecessary second code path for callers to reason about; deleting them and routing directly through the canonical API reducespkg/utils's surface area per this repo's ongoing effort to empty that package out.atmos auth env/atmos envneed one legitimate, explicit way to emit unmasked output (the whole point of those commands is exporting real credentials foreval), so a dedicatedWriteUnmaskedAPI makes that intent explicit instead of ad hocfmt.Printcalls.references