Skip to content

refactor: extract runWithSignalHandling helper#2495

Merged
lpcox merged 1 commit intomainfrom
fix/dedup-log-streamer-signal-handling
May 4, 2026
Merged

refactor: extract runWithSignalHandling helper#2495
lpcox merged 1 commit intomainfrom
fix/dedup-log-streamer-signal-handling

Conversation

@lpcox
Copy link
Copy Markdown
Collaborator

@lpcox lpcox commented May 4, 2026

Summary

Deduplicate the identical signal-handler + readline loop from streamFromContainer and tailFile in src/logs/log-streamer.ts into a shared runWithSignalHandling helper.

Before

Both functions contained an identical 14-line block that:

  1. Attaches SIGINT/SIGTERM cleanup handlers
  2. Runs a readline interface over a child process stdout
  3. Silently swallows SIGTERM errors
  4. Removes the cleanup handlers in a finally block

After

runWithSignalHandling(proc, formatter, parse, withPid) encapsulates this pattern. Both callers reduce to spawning the child process and delegating to the helper.

Testing

  • All 13 log-streamer tests pass
  • TypeScript compilation clean

Closes #2481

Deduplicate the identical signal-handler + readline loop
from streamFromContainer and tailFile into a shared
runWithSignalHandling helper.

Closes #2481

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 4, 2026 14:53
@lpcox lpcox requested a review from Mossaka as a code owner May 4, 2026 14:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors src/logs/log-streamer.ts to remove duplicated signal-handling + readline streaming logic by extracting it into a shared internal helper, improving maintainability and ensuring future changes to the cleanup lifecycle are made in one place.

Changes:

  • Introduced runWithSignalHandling(proc, formatter, parse, withPid) to encapsulate SIGINT/SIGTERM cleanup and stdout line streaming.
  • Updated streamFromContainer and tailFile to delegate to the new helper after spawning the appropriate child process.
Show a summary per file
File Description
src/logs/log-streamer.ts Extracts duplicated signal-handler/readline loop into runWithSignalHandling and reuses it from both container and file tailing paths.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Smoke Test Results

✅ GitHub MCP: Last 2 merged PRs retrieved

  • [Test Coverage] Add tests for parseDifcProxyHost and preserveIptablesAudit
  • refactor: remove unused exports from internal modules

✅ Playwright: github.com title verified ("GitHub" detected)
✅ File Writing: /tmp/gh-aw/agent/smoke-test-claude-25326038150.txt created
✅ Bash Tool: File verification successful

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🔥 Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP (list PRs)
GitHub.com connectivity
File write/read ✅ (smoke-test-copilot-byok-25326038147.txt confirmed)
BYOK inference (this response)

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com

Overall: PASS — authored by @lpcox, reviewer @Mossaka

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🔥 Smoke Test Results

Test Result
GitHub MCP connectivity ✅ PR listed successfully
GitHub.com HTTP connectivity ❌ Template var unresolved
File write/read ❌ Template var unresolved

PR: refactor: extract runWithSignalHandling helper — author: @lpcox

Overall: FAIL — smoke-data outputs were not substituted (workflow template vars unresolved), so HTTP and file tests could not be verified.

📰 BREAKING: Report filed by Smoke Copilot

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

refactor: extract runWithSignalHandling helper
[Test Coverage] Add tests for parseDifcProxyHost and preserveIptablesAudit
refactor: remove unused exports from internal modules
GitHub merged PR review ✅ | safeinputs-gh query ❌ | Playwright ✅ | Tavily search ❌
File write ✅ | Bash cat ✅ | Discussion comment ✅ | Build AWF ✅
Overall: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ NO
Node.js v24.14.1 v20.20.2 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ALL_TESTS_PASSED=false — Python and Node.js versions differ between host and chroot.

Tested by Smoke Chroot

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🏗️ Build Test Suite Results

⚠️ ALL CLONES FAILEDgh CLI is not authenticated in this environment (GH_TOKEN not set), so no test repositories could be cloned.

Ecosystem Project Build/Install Tests Status
Bun elysia N/A ❌ CLONE_FAILED
Bun hono N/A ❌ CLONE_FAILED
C++ fmt N/A ❌ CLONE_FAILED
C++ json N/A ❌ CLONE_FAILED
Deno oak N/A ❌ CLONE_FAILED
Deno std N/A ❌ CLONE_FAILED
.NET hello-world N/A ❌ CLONE_FAILED
.NET json-parse N/A ❌ CLONE_FAILED
Go color N/A ❌ CLONE_FAILED
Go env N/A ❌ CLONE_FAILED
Go uuid N/A ❌ CLONE_FAILED
Java gson N/A ❌ CLONE_FAILED
Java caffeine N/A ❌ CLONE_FAILED
Node.js clsx N/A ❌ CLONE_FAILED
Node.js execa N/A ❌ CLONE_FAILED
Node.js p-limit N/A ❌ CLONE_FAILED
Rust fd N/A ❌ CLONE_FAILED
Rust zoxide N/A ❌ CLONE_FAILED

Overall: 0/8 ecosystems passed — ❌ FAIL

Error: gh CLI is not authenticated. All gh repo clone commands failed with:

gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.

No tests were run. The workflow requires a valid GH_TOKEN to clone the test repositories.

Generated by Build Test Suite for issue #2495 · ● 243.4K ·

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Smoke Test Results

  • Redis PING: ❌ timeout/no response
  • PostgreSQL pg_isready: ❌ no response (host.docker.internal:5432 - no response)
  • PostgreSQL SELECT 1: ❌ skipped (pg_isready failed)

Overall: FAILhost.docker.internal is not reachable from this runner environment. Service containers may not be running or the hostname is not resolvable.

🔌 Service connectivity validated by Smoke Services

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Smoke Test: Gemini Engine Validation

  • test: add coverage for parseDifcProxyHost and preserveIptablesAudit ✅
  • refactor: remove unused exports from internal modules ✅
  • GitHub.com Connectivity ✅
  • File Writing Testing ✅
  • Bash Tool Testing ✅

Overall status: PASS

💎 Faceted by Smoke Gemini

@lpcox lpcox merged commit f3f2418 into main May 4, 2026
67 of 72 checks passed
@lpcox lpcox deleted the fix/dedup-log-streamer-signal-handling branch May 4, 2026 15:20
Copilot AI added a commit that referenced this pull request May 4, 2026
- Resolve rename conflict in docker-manager-compose.test.ts (keep split version)
- Remove unused imports from docker-manager-compose.test.ts
- Add parseDifcProxyHost tests to docker-manager-utils.test.ts (from #2471)
- Add preserveIptablesAudit tests to docker-manager-cleanup.test.ts (from #2471)
- Auto-merge log-streamer.ts refactor from #2495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] streamFromContainer and tailFile duplicate identical signal-handler + readline loop

2 participants