Skip to content

fix(orchestrator): Drop subprocess stdout from woodpecker-apply phase result#640

Merged
stefanko-ch merged 1 commit into
mainfrom
fix/orchestrator-subprocess-output-leak
Jun 4, 2026
Merged

fix(orchestrator): Drop subprocess stdout from woodpecker-apply phase result#640
stefanko-ch merged 1 commit into
mainfrom
fix/orchestrator-subprocess-output-leak

Conversation

@stefanko-ch

@stefanko-ch stefanko-ch commented Jun 4, 2026

Copy link
Copy Markdown
Owner

Summary

_phase_woodpecker_apply was the only CalledProcessError handler in src/nexus_deploy/orchestrator.py that surfaced exc.output into the PhaseResult.detail (as a sliced last-line "tail", truncated to 120 chars). The six other handlers in the same file — infisical-bootstrap, services-configure, gitea-configure, seed, kestra-register, woodpecker-oauth — all stick to type(exc).__name__ + rc and leave docker logs / docker compose logs as the source of truth for full output.

This PR aligns the woodpecker handler with the rest of the file.

Why

exc.output here is the captured stdout of docker compose up -d for the woodpecker stack, which has a .env populated with Infisical-managed secrets. Modern compose prints mostly status lines (Container … Started), but its error branches can echo back values from the rendered env block (e.g. when interpolation fails for a malformed variable, the failing key=value can land in the error message). The risk of those landing in a PhaseResult.detail that gets logged or surfaced to the operator is non-zero — and inconsistent with the convention every other handler in this file already follows.

What this does NOT lose

The new detail still tells the operator everything actionable from inside Python:

  • which phase failed (woodpecker-apply)
  • the return code (rc=1)
  • the exception class (CalledProcessError)

For the full compose stack trace, the on-server docker compose logs woodpecker / docker logs woodpecker-server is the canonical place — which is also the convention for every other stack's deploy failure.

Test plan

  • Existing test_phase_woodpecker_apply_partial_on_compose_up_nonzero still passes — it asserts "docker compose up -d failed" in result.detail and "rc=1" in result.detail, both substrings present in the new detail.
  • All 50 woodpecker-related unit tests pass (uv run pytest tests/unit/ -q -k woodpecker50 passed, 1525 deselected).
  • No regression on a real deploy where woodpecker compose-up actually fails — operator gets the clean partial-phase signal in stderr + the full diagnostic via docker logs on the server.

Summary by CodeRabbit

  • Bug Fixes
    • Improved consistency in error reporting for Docker compose failures during deployment phases.

… result

`_phase_woodpecker_apply` was the only CalledProcessError handler in
the file that surfaced exc.output into the PhaseResult.detail (as a
sliced "tail" of the last stdout line, truncated to 120 chars). The
six other handlers (infisical-bootstrap, services-configure,
gitea-configure, seed, kestra-register, woodpecker-oauth) all stick
to `type(exc).__name__` + rc and let `docker logs <container>` /
`docker compose logs` be the source of truth for the full output.

exc.output here is the captured stdout of `docker compose up -d` for
the woodpecker stack. Modern compose tends to print only the start
sequence ("Network … Creating", "Container … Started", etc.), but
its error branches can echo back values from the rendered env block
(e.g. when interpolation fails for a malformed variable). The
woodpecker stack's .env contains Infisical-managed secrets — the
risk of those landing in a PhaseResult that gets logged / surfaced
to the operator is non-zero.

Aligned this handler with the rest of the file: report the rc and
the exception type, no stdout excerpt. Existing test
test_phase_woodpecker_apply_partial_on_compose_up_nonzero still
passes — it only asserts on "docker compose up -d failed" and
"rc=1", both of which remain in the new detail string.
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4538a5de-9401-4beb-afa7-3d9978779471

📥 Commits

Reviewing files that changed from the base of the PR and between 7c50f1a and 9c597c2.

📒 Files selected for processing (1)
  • src/nexus_deploy/orchestrator.py

📝 Walkthrough

Walkthrough

The PR simplifies error reporting in the _phase_woodpecker_apply function within the docker compose failure handler. Previously, the handler extracted and appended a stdout excerpt to the phase result detail; now it reports only the return code and exception type, consistent with other error handlers in the file.

Changes

Woodpecker Apply Error Handling

Layer / File(s) Summary
Error handler detail simplification
src/nexus_deploy/orchestrator.py
Removed custom stdout excerpt extraction from the CalledProcessError handler block (lines 1892–1907); now returns a partial result with only rc=<returncode> and type(exc).__name__, eliminating output parsing and aligning the failure reporting pattern with other phase error handlers in the file.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing subprocess stdout from the woodpecker-apply phase result in the orchestrator, which is exactly what the changeset implements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/orchestrator-subprocess-output-leak

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

coverage

Coverage report — nexus_deploy
FileStmtsMissCoverMissing
__init__.py50100% 
_remote.py150100% 
cli.py40100% 
compose_restart.py400100% 
compose_runner.py880100% 
config.py1400100% 
firewall.py2060100% 
gitea.py5875590%691–692, 697, 720–721, 733–734, 770–771, 783–784, 802–803, 828–829, 851–852, 863–864, 919–920, 928–929, 934, 940–941, 965–966, 999–1000, 1003, 1034–1035, 1076–1077, 1082–1083, 1123–1124, 1155–1156, 1179–1180, 1185–1186, 1285–1286, 1291–1292, 1766, 1770, 1791, 1819–1820, 1907
hetzner_capacity.py1260100% 
infisical.py2040100% 
kestra.py176398%223, 427, 768
orchestrator.py6187388%456, 616, 628, 798–799, 804–805, 837–839, 848, 853–855, 866, 903–904, 909–910, 930, 965–966, 971–972, 980, 1005–1006, 1014, 1036–1037, 1042–1043, 1095–1096, 1101–1102, 1335, 1338, 1408, 1414–1415, 1420–1421, 1455, 1562–1563, 1568–1569, 1618–1619, 1624–1625, 1684, 1699, 1756, 1761–1762, 1767–1768, 1775, 1781, 1956, 1963, 1975–1976, 1981–1982, 1988, 1994, 2067–2068, 2089–2090
pipeline.py2071393%165–166, 350, 388, 468, 485, 580–581, 626–627, 717–718, 765
r2_tokens.py113298%87, 150
s3_persistence.py199199%315
s3_restore.py1030100% 
secret_sync.py990100% 
seeder.py980100% 
service_env.py3973391%1142, 1144–1146, 1154–1155, 1484–1487, 1492–1498, 1527–1531, 1547–1551, 1573, 1575, 1597–1598, 1605, 1714
services.py319199%2018
setup.py1651392%238, 308–311, 319, 323–328, 344
ssh.py560100% 
stack_sync.py960100% 
tfvars.py440100% 
tofu.py860100% 
workspace_coords.py1010100% 
TOTAL429219495% 

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@stefanko-ch stefanko-ch merged commit 7a57ef7 into main Jun 4, 2026
9 checks passed
@stefanko-ch stefanko-ch deleted the fix/orchestrator-subprocess-output-leak branch June 4, 2026 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant