Repository navigation
fix(output): render concurrent carriage-return updates safely - #2860
Andriy Knysh (aknysh) merged 20 commits into
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTerraform output handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TerraformExecutions
participant TerraformAdapter
participant SpinnerOutput
participant Provisioner
participant ComponentHooks
participant PrefixedWriters
TerraformExecutions->>TerraformAdapter: Run concurrent invocations
TerraformAdapter->>SpinnerOutput: Suppress spinners
TerraformAdapter->>Provisioner: Provision workdirs with suppressed output
TerraformAdapter->>ComponentHooks: Pass component stdout and stderr writers
ComponentHooks->>PrefixedWriters: Write hook output
TerraformAdapter->>PrefixedWriters: Stream Terraform output
PrefixedWriters->>PrefixedWriters: Normalize carriage-return lines
TerraformAdapter->>SpinnerOutput: Restore spinner state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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/terraform/output/spinner_test.go`:
- Around line 19-31: Update TestSuppressSpinnersRestoresNestedScopes to register
both restoreOuter and restoreInner with t.Cleanup immediately after creating
them, ensuring cleanup runs after assertion failures. Invoke one restoration
function twice and verify suppression remains enabled while the other scope is
active, then restore the remaining scope and verify spinnersSuppressed() is
false.
🪄 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 Plus
Run ID: e3d6ddba-69f8-4f0e-af45-6e4c97dc7314
📒 Files selected for processing (4)
pkg/scheduler/adapters/terraform.gopkg/terraform/output/executor_utils.gopkg/terraform/output/spinner.gopkg/terraform/output/spinner_test.go
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/terraform/output/executor_utils.go`:
- Around line 105-116: Update finishSpinner to call clearSpinnerLine() during
cleanup instead of using the captured clearsLine flag and directly invoking
ui.ClearLine(), so suppression state is checked at cleanup time. Add a
regression test covering suppression beginning after spinner creation but before
the returned cleanup function runs.
🪄 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 Plus
Run ID: 23e55a89-3bf3-49f6-9450-6fc75310ae83
📒 Files selected for processing (4)
pkg/terraform/output/executor.gopkg/terraform/output/executor_test.gopkg/terraform/output/executor_utils.gopkg/terraform/output/executor_workdir.go
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/scheduler/adapters/terraform_test.go`:
- Around line 27-32: Remove the duplicate adapterWriterFunc type and its Write
method from the test file, and reuse the existing package-level
adapterWriterFunc defined in terraform.go.
🪄 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 Plus
Run ID: 153502a1-d45b-4c1d-a438-6bcf5c9eeb60
📒 Files selected for processing (2)
pkg/scheduler/adapters/terraform.gopkg/scheduler/adapters/terraform_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/adapters/terraform.go
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
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/terraform/output/executor_utils.go`:
- Around line 118-122: Update clearSpinnerLine to hold spinnerSuppression while
checking the suppression count and calling ui.ClearLine, rather than using
spinnersSuppressed() which releases the lock before clearing. Preserve the
existing behavior of skipping the clear when suppression is active and ensure
SuppressSpinners cannot interleave between the check and line clear.
🪄 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 Plus
Run ID: a0aa78bf-3282-4899-9d79-7b20fc4eea61
📒 Files selected for processing (10)
pkg/io/line_prefix_writer.gopkg/io/line_prefix_writer_test.gopkg/scheduler/adapters/terraform.gopkg/scheduler/adapters/terraform_test.gopkg/terraform/output/executor.gopkg/terraform/output/executor_test.gopkg/terraform/output/executor_utils.gopkg/terraform/output/executor_workdir.gopkg/terraform/output/spinner.gopkg/terraform/output/spinner_test.go
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 29 minutes. |
|
CodeRabbit (@coderabbitai) my concern is that Atmos should be automatically downgrading things like spinners when running in parallel mode so that we don't need to worry about carriage returns like this. I suspect this was always a problem, but a recent pull request that was merged fixed our TTY detection, and that's why we see this now. While I agree we should gracefully handle carriage returns from sub-processes that are not Atmos, if it's Atmos itself, then it should just not be emitting the spinners, for example. With that in mind, is this the correct fix? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed files ---'
git diff --name-only HEAD^ HEAD
printf '%s\n' '--- outlines ---'
ast-grep outline pkg/scheduler/adapters/terraform.go --items all
ast-grep outline pkg/terraform/output/spinner.go --items all
ast-grep outline pkg/terraform/output/executor_utils.go --items all
ast-grep outline pkg/io/line_prefix_writer.go --items all
printf '%s\n' '--- scheduler adapter relevant code ---'
rg -n -C 12 'SuppressSpinners|newSerializedWriter|LinePrefixWriter|parallel|concurrent|mutex|Mutex' pkg/scheduler/adapters/terraform.go pkg/scheduler/adapters/terraform_test.go
printf '%s\n' '--- spinner and executor relevant code ---'
rg -n -C 10 'SuppressSpinners|spinnersSuppressed|startSpinnerOrLog|finishSpinner|clearSpinnerLine|StartSpinner|ClearLine' \
pkg/terraform/output/spinner.go \
pkg/terraform/output/executor.go \
pkg/terraform/output/executor_utils.go \
pkg/terraform/output/executor_workdir.go \
pkg/terraform/output/executor_test.go \
pkg/terraform/output/spinner_test.go
printf '%s\n' '--- line writer relevant code ---'
rg -n -C 12 'func \(.*LinePrefixWriter.*\) (Write|Flush)|\\r|\\n' \
pkg/io/line_prefix_writer.go \
pkg/io/line_prefix_writer_test.goLength of output: 90323 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- ExecuteTerraform control flow ---'
sed -n '133,235p' pkg/scheduler/adapters/terraform.go
printf '%s\n' '--- output lifecycle and dispatch path ---'
sed -n '608,715p' pkg/scheduler/adapters/terraform.go
sed -n '1238,1463p' pkg/scheduler/adapters/terraform.go
printf '%s\n' '--- all spinner construction and suppression call sites ---'
rg -n -C 4 'NewSpinner\s*\(|RunSpinner\s*\(|SuppressSpinners\s*\(|spinnersSuppressed\s*\(' --type go .
printf '%s\n' '--- adapter tests that exercise suppression or output lookups ---'
rg -n -C 8 'SuppressSpinners|spinnersSuppressed|GetOutput|GetAllOutputs|MaxConcurrency:\s*[2-9]|MaxConcurrency:\s*[1-9][0-9]' \
pkg/scheduler/adapters/terraform_test.go \
pkg/terraform/output/executor_test.go \
pkg/terraform/output/spinner_test.go
printf '%s\n' '--- complete PR diff from merge base ---'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)"
if [ -n "$base" ]; then
git diff --stat "$base"..HEAD
git diff -- "$base"..HEAD -- pkg/scheduler/adapters/terraform.go pkg/terraform/output/spinner.go pkg/terraform/output/executor_utils.go pkg/io/line_prefix_writer.go
else
echo 'Merge base is unavailable.'
fiLength of output: 102371
The fix now has two layers:
So, carriage-return normalization should not be the primary solution for Atmos spinners. It is the required safety boundary for child-process output. The spinner suppression is the primary solution for Atmos output. I found one coverage gap. The tests verify 🐇🔎 ✏️ Learnings added
You are interacting with an AI system. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2860 +/- ##
=======================================
Coverage ? 82.75%
=======================================
Files ? 1860
Lines ? 180337
Branches ? 0
=======================================
Hits ? 149243
Misses ? 23309
Partials ? 7785
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
the
patch
|
Added the requested adapter-level regression in 0ecff7a. It verifies that a |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/hooks/command_engine_test.go`:
- Around line 191-222: The test
TestCommandEngine_RoutesSubprocessOutputToContextWriters currently exercises
only stdout routing. Update its child-process fixture to emit a distinct stderr
value, configure the corresponding test environment input, and assert that the
value appears only in the context’s stderr buffer while stdout remains
unchanged, providing coverage for both writer paths.
In `@pkg/scheduler/adapters/terraform_test.go`:
- Around line 374-386: Update testNodeHooks.BeforeWithWriters and the
corresponding hook-writing path so n.mu protects only state recording and error
lookup, not fmt.Fprint; release the lock before writing and add separate
synchronization that coordinates fragmented writes from concurrent hooks through
their node writers. Strengthen TestExecuteTerraformConcurrentHooksUseNodeWriters
with behavior-focused assertions proving concurrent writes contend correctly and
cannot pass merely because shared writer synchronization serializes them.
🪄 Autofix
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 Plus
Run ID: 7bf9eed3-5c8a-4d25-ad68-43d168f70533
📒 Files selected for processing (16)
cmd/helmfile/helmfile.gocmd/terraform/utils.gopkg/hooks/command_engine.gopkg/hooks/command_engine_test.gopkg/hooks/hooks.gopkg/hooks/kind.gopkg/hooks/kinds/tfmigrate/kind.gopkg/io/line_prefix_writer.gopkg/io/line_prefix_writer_test.gopkg/scheduler/adapters/terraform.gopkg/scheduler/adapters/terraform_test.gopkg/schema/schema.gopkg/terraform/output/executor.gopkg/terraform/output/executor_utils.gopkg/terraform/output/executor_workdir.gopkg/terraform/output/spinner_test.go
💤 Files with no reviewable changes (1)
- pkg/terraform/output/executor.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/terraform/output/executor_workdir.go
- pkg/terraform/output/executor_utils.go
- pkg/io/line_prefix_writer_test.go
- pkg/io/line_prefix_writer.go
|
CodeRabbit (@coderabbitai) Anything else? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null)"
printf '%s\n' '--- PR commits and changed files ---'
git log --oneline "${base}..HEAD"
git diff --name-only "${base}..HEAD"
printf '%s\n' '--- final hook-output commit ---'
git show --format=fuller --stat 8cf80fbd0
git show --format= --unified=80 8cf80fbd0 -- \
pkg/scheduler/adapters/terraform.go \
pkg/scheduler/adapters/terraform_test.go \
pkg/hooks \
pkg/schema/schema.go \
cmd/terraform/utils.go \
cmd/helmfile/helmfile.go \
pkg/io/line_prefix_writer.go \
pkg/io/line_prefix_writer_test.go
printf '%s\n' '--- relevant outlines ---'
for f in \
pkg/scheduler/adapters/terraform.go \
pkg/scheduler/adapters/terraform_test.go \
pkg/hooks/hooks.go \
pkg/hooks/command_engine.go \
pkg/hooks/kind.go \
pkg/hooks/kinds/tfmigrate/kind.go \
pkg/schema/schema.go \
cmd/terraform/utils.go \
cmd/helmfile/helmfile.go \
pkg/io/line_prefix_writer.go \
pkg/io/line_prefix_writer_test.go
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all
fi
done
printf '%s\n' '--- hook contracts, implementations, and writer propagation ---'
rg -n -C 12 --type go \
'ComponentNodeHooksWithOutput|ComponentNodeHookWriters|BeforeWithWriters|AfterWithWriters|RunPerComponentHooksOptions|ExecContext|subprocessPrep|Stdout:|Stderr:' \
pkg cmd
printf '%s\n' '--- serialized writer and line handling ---'
sed -n '1,260p' pkg/io/line_prefix_writer.go
sed -n '1,380p' pkg/io/line_prefix_writer_test.go
rg -n -C 8 --type go \
'newSerializedWriter|NewLinePrefixWriter|\.Flush\(\)|outputMu|nodeWriters|LinePrefixWriter' \
pkg/scheduler/adapters pkg/io
printf '%s\n' '--- direct output from hook and scheduler paths ---'
rg -n -C 8 --type go \
'os\.(Stdout|Stderr)|fmt\.(Fprint|Print)|ui\.[A-Za-z0-9_]+|iolib\.(UI|Data)|process\.Streams|renderTerminal' \
pkg/hooks pkg/scheduler/adapters cmd/terraform cmd/helmfileLength of output: 151407 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- lifecycle hook dispatch ---'
sed -n '640,715p' pkg/scheduler/adapters/terraform.go
rg -n -C 20 --type go \
'func \(.*\) run(Before|After)NodeHooks|BeforeWithWriters|AfterWithWriters|RunPerComponentHooks' \
pkg/scheduler/adapters/terraform.go \
cmd/terraform/utils.go \
cmd/helmfile/helmfile.go
printf '%s\n' '--- writer option propagation and resolved-hook execution ---'
rg -n -C 20 --type go \
'type RunPerComponentHooksOptions|func RunPerComponentHooks|func \(.*Hooks.*\) runResolvedHook|ExecContext\{' \
pkg/hooks/hooks.go
sed -n '80,145p' pkg/hooks/command_engine.go
sed -n '240,370p' pkg/hooks/command_engine.go
printf '%s\n' '--- structured and markdown hook output configuration ---'
rg -n -C 12 --type go \
'CaptureOutput|capture.output|FormatMarkdown|Format.*Markdown|Summary|Artifact|renderTerminal' \
pkg/hooks cmd/terraform pkg/scheduler/adapters
printf '%s\n' '--- tests for concurrent node hooks and captured hook output ---'
sed -n '450,560p' pkg/scheduler/adapters/terraform_test.go
rg -n -C 12 --type go \
'RunPerComponentHooks|CaptureOutput|FormatMarkdown|renderTerminal|Stdout.*Stderr' \
pkg/hooks/*_test.go pkg/hooks/**/*_test.go pkg/scheduler/adapters/terraform_test.goLength of output: 246793
P1 — Serialize structured hook summaries during concurrent execution
Built-in scanner hooks and Infracost hooks can produce a non-empty Route The subprocess stream path is now correct. The structured hook-summary path remains outside the serialized path. You are interacting with an AI system. |
GitHub Actions
Inactive
|
CodeRabbit (@coderabbitai) why do you keep finding "one remaining blocking gap." Can you just give it to us all at once? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null)"
printf '%s\n' '--- PR files and complete diff ---'
git diff --name-only "${base}..HEAD"
git diff --find-renames --unified=70 "${base}..HEAD" -- \
pkg/scheduler/adapters \
pkg/terraform/output \
pkg/provisioner/workdir \
pkg/provisioner/source \
pkg/hooks \
pkg/io/line_prefix_writer.go \
pkg/schema/schema.go \
cmd/terraform \
cmd/helmfile
printf '%s\n' '--- maps of concurrent scheduler and output-relevant packages ---'
for path in \
pkg/scheduler/adapters/terraform.go \
pkg/terraform/output \
pkg/provisioner/workdir \
pkg/provisioner/source \
pkg/hooks \
pkg/io/line_prefix_writer.go \
cmd/terraform/utils.go \
cmd/helmfile/helmfile.go
do
if [ -e "$path" ]; then
echo "--- $path ---"
ast-grep outline "$path" --items all || true
fi
done
printf '%s\n' '--- all direct terminal/UI write sites in reachable packages ---'
rg -n -C 8 --type go \
'(ui\.[A-Za-z0-9_]+\s*\(|iolib\.(UI|Data)\.(Write|WriteString)\s*\(|os\.(Stdout|Stderr)\.(Write|WriteString)\s*\(|fmt\.Fprint[a-z]*\s*\(|process\.Streams\s*\{|Stdout:|Stderr:)' \
pkg/scheduler/adapters pkg/terraform/output pkg/provisioner/workdir pkg/provisioner/source pkg/hooks cmd/terraform cmd/helmfile
printf '%s\n' '--- suppression and component-writer propagation ---'
rg -n -C 12 --type go \
'(SuppressSpinners|spinnersSuppressed|WithOutputSuppressed|OutputSuppressed|ComponentNodeHooksWithOutput|BeforeWithWriters|AfterWithWriters|ComponentNodeHookWriters|RunPerComponentHooksOptions|ExecContext|newSerializedWriter|NewLinePrefixWriter|renderTerminal)' \
pkg/scheduler/adapters pkg/terraform/output pkg/provisioner/workdir pkg/provisioner/source pkg/hooks pkg/schema cmd/terraform cmd/helmfile
printf '%s\n' '--- complete implementations at output boundaries ---'
sed -n '120,250p' pkg/scheduler/adapters/terraform.go
sed -n '620,735p' pkg/scheduler/adapters/terraform.go
sed -n '1220,1490p' pkg/scheduler/adapters/terraform.go
sed -n '1,430p' pkg/hooks/command_engine.go
sed -n '1,280p' pkg/hooks/hooks.go
sed -n '1,260p' pkg/hooks/kind.go
sed -n '1,300p' pkg/hooks/kinds/tfmigrate/kind.go
sed -n '1,260p' pkg/io/line_prefix_writer.goLength of output: 115411 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- non-test hook engine implementations and direct terminal writes ---'
rg -n -C 6 --glob '!**/*_test.go' --type go \
'func \(.*\) Run\(ctx \*hooks?\.?ExecContext\)|func \(.*\) Run\(ctx \*ExecContext\)|ui\.(Writeln|MarkdownMessage|Infof?|Successf?|Warningf?|Errorf?|ClearLine)\s*\(|os\.(Stdout|Stderr)|fmt\.(Print|Fprint)' \
pkg/hooks
printf '%s\n' '--- registered hook kinds and their engines ---'
rg -n -C 5 --glob '!**/*_test.go' --type go \
'RegisterKind\s*\(|Engine:\s*|type Engine struct' \
pkg/hooks
printf '%s\n' '--- concurrent dispatcher paths and direct UI writes ---'
rg -n -C 12 --glob '!**/*_test.go' --type go \
'func \(d \*TerraformDispatcher\) Dispatch|ui\.[A-Za-z0-9_]+\s*\(|runBeforeNodeHooks|runAfterNodeHooks|nodeWriters|finishNode|writeTerraformSummary' \
pkg/scheduler/adapters/terraform.go
printf '%s\n' '--- UI and CI log-group output implementations ---'
ast-grep outline pkg/ui --items all
rg -n -C 10 --glob '!**/*_test.go' --type go \
'func (Writeln|MarkdownMessage|Info|Infof|Success|Successf|Warning|Warningf|Error|Errorf|ClearLine)\s*\(|func (Group|StartLogGroup)\s*\(' \
pkg/ui pkg/ci
printf '%s\n' '--- focused source slices ---'
sed -n '300,390p' pkg/hooks/command_engine.go
sed -n '1,240p' pkg/hooks/step_engine.go
sed -n '1,220p' pkg/hooks/kinds/tfmigrate/kind.go
sed -n '650,730p' pkg/scheduler/adapters/terraform.go
sed -n '1,220p' pkg/scheduler/adapters/terraform_test.goLength of output: 112342 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- step executor: process and UI output boundaries ---'
rg -n -C 10 --glob '!**/*_test.go' --type go \
'(type StepExecutor|func \(.*StepExecutor.*\) Execute|exec\.Command|process\.|os\.(Stdout|Stderr)|ui\.[A-Za-z0-9_]+\s*\(|fmt\.(Fprint|Print))' \
pkg/runner/step pkg/runner
printf '%s\n' '--- store hook execution: terminal and subprocess output boundaries ---'
ast-grep outline pkg/hooks/store_cmd.go --items all
rg -n -C 10 --glob '!**/*_test.go' --type go \
'(func \(.*StoreCommand.*\) RunE|func .*RunE|exec\.Command|process\.|os\.(Stdout|Stderr)|ui\.[A-Za-z0-9_]+\s*\(|fmt\.(Fprint|Print))' \
pkg/hooks pkg/store
printf '%s\n' '--- CI log-group write path ---'
sed -n '1,140p' pkg/ci/log_group.go
sed -n '145,230p' pkg/ci/loggroup.go
rg -n -C 8 --glob '!**/*_test.go' --type go \
'(func .*StartGroup|func .*EndGroup|func .*Group|os\.(Stdout|Stderr)|fmt\.(Fprint|Print)|WriteString)' \
pkg/ci
printf '%s\n' '--- every ComponentNodeHooks implementation and the legacy fallback ---'
rg -n -C 10 --glob '!**/*_test.go' --type go \
'(ComponentNodeHooks|BeforeWithWriters|AfterWithWriters|func \(.*\) Before\(|func \(.*\) After\()' \
. -g '*.go'
printf '%s\n' '--- output and integration tests that cover non-command lifecycle hooks ---'
rg -n -C 8 --type go \
'(step.*hook|store.*hook|CI.*group|DryRun|dry run|MarkdownMessage|Summary\.Body|ComponentNodeHooksWithOutput)' \
pkg/hooks pkg/runner pkg/scheduler/adapters cmdLength of output: 164527 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- runner step candidate files ---'
fd -e go . pkg/runner/step | sort
printf '%s\n' '--- store hook candidate files ---'
fd -e go . pkg/hooks | rg '(store|command)' | sort
printf '%s\n' '--- exact runner-step execution and output calls ---'
rg -n -C 16 --glob '!**/*_test.go' --type go \
'type StepExecutor|func \(.*StepExecutor.*\) Execute|func \(.*StepExecutor.*\) execute|os\.(Stdout|Stderr)|ui\.[A-Za-z0-9_]+\s*\(|exec\.Command|process\.' \
pkg/runner/step
printf '%s\n' '--- exact store-hook execution and output calls ---'
rg -n -C 16 --glob '!**/*_test.go' --type go \
'func \(.*StoreCommand.*\) RunE|func \(.*StoreCommand.*\) Run|os\.(Stdout|Stderr)|ui\.[A-Za-z0-9_]+\s*\(|exec\.Command|process\.' \
pkg/hooks
printf '%s\n' '--- CI group marker implementation ---'
sed -n '1,120p' pkg/ci/log_group.go
sed -n '157,215p' pkg/ci/loggroup.go
rg -n -C 12 --glob '!**/*_test.go' --type go \
'(StartGroup|EndGroup|StartLogGroup|EndLogGroup|WriteString|os\.Stdout|os\.Stderr)' \
pkg/ci/internal pkg/ci/providers pkg/ciLength of output: 150372
Here is the complete result from the built-in concurrent Terraform paths. Blocking paths
These are the two remaining blocking issues for the concurrent terminal-rendering contract. Non-blocking follow-ups
I found no additional built-in direct terminal-output path in I will use this full-path audit method for this class of review. I will not report another isolated “one remaining blocking gap” unless a code change creates a new output path. (ᵔᴥᵔ) ✏️ Learnings added
You are interacting with an AI system. |
Agreed: this is a genuine concurrent-output gap. The contained structured-summary path can be routed through the existing node writer, but step hooks need an execution-scoped output contract propagated through A scoped redirect of the global UI writer would not be safe under parallel node execution, because it is process-wide and concurrent hooks could overwrite each other's destination. I recommend handling step-hook output as a dedicated follow-up rather than expanding this PR with a partial workaround. The dry-run, CI-log-group, and legacy-interface items are non-blocking as described. |
|
Tip For best results, initiate chat on the files or code changes.
A process-wide UI-writer redirect is unsafe during parallel execution. The follow-up should add execution-scoped writers to The current PR can treat the structured-summary path as the contained remaining change. The dry-run, CI log-group, and legacy-interface items remain non-blocking. Would you like me to create a follow-up issue? GitHub Issues and Linear are available. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
…al-output' into fix/concurrent-terminal-output
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.225.0. |
This branch was previously deployed
3 participants
what
why
references
validation
go test ./pkg/scheduler/adapters ./pkg/io -count=1go test ./pkg/terraform/output -run '^TestSuppressSpinnersRestoresNestedScopes$' -count=1pre-commit run --files pkg/scheduler/adapters/terraform.go pkg/terraform/output/executor_utils.go pkg/terraform/output/spinner.go pkg/terraform/output/spinner_test.goSummary by CodeRabbit
Bug Fixes
Improvements