Fix remaining #470 console flash: hub server spawn, not just browser-open - #475
Open
richard-devbot wants to merge 8 commits into
Open
Fix remaining #470 console flash: hub server spawn, not just browser-open#475richard-devbot wants to merge 8 commits into
richard-devbot wants to merge 8 commits into
Conversation
…#472) npm audit reported 5 high-severity advisories, all tracing to one root: a DoS/ReDoS bug in brace-expansion (GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg), reachable via eslint -> minimatch@3.1.5 -> brace-expansion@1.1.16. npm's only reported fix was eslint@10 (isSemVerMajor), which pulls in a new no-useless-assignment rule in @eslint/js's recommended config, flagging 15 pre-existing dead-store patterns across src/ -- too invasive for a security-only fix. Instead: add a root `overrides` entry (this repo already uses the same mechanism for esbuild/protobufjs/ws) forcing brace-expansion to the patched 5.0.8 line. Verified this reaches eslint's own tree (minimatch@3.1.5's brace-expansion now 5.0.8) while, as documented, leaving @earendil-works/pi-coding-agent's shrinkwrapped copy (5.0.6) untouched -- npm honors a shrinkwrap absolutely, so overrides can't reach it regardless. That shrinkwrapped copy is the only remaining high-severity finding, and it's exactly the node this gate's BUNDLED_ROOTS/ TOLERATED_BUNDLED_ADVISORIES allowlist already exists to tolerate -- added the second GHSA id to that allowlist (the gate is deliberately strict per-advisory-id, so a newly surfaced GHSA for an already-tolerated package still fails until consciously added). Verified: npm run lint unchanged (0 errors, same 4 pre-existing warnings -- confirms minimatch@3.1.5 works fine with the newer brace-expansion at runtime), npm run typecheck clean, gate logic simulated against real npm audit output now reports 0 blocking.
Slack notifications have never been deliverable. Both payload builders in
src/notifications/index.js emitted a block of `{ type: 'fields', fields }`,
but Block Kit has no `fields` block type — fields belong on a `section`
block. Slack rejects the entire attachment with:
400 invalid_attachments
Verified against a live Incoming Webhook: failing before this change,
delivering successfully after it.
The bogus type had propagated to every consumer, so each one is updated to
read fields off a section block while still tolerating the old shape, which
keeps any in-flight or persisted payloads converting correctly:
- channels/teams.js - facts extraction
- channels/discord.js - embed fields
- channels/text.js - plain-text flattening (covered by the existing
"slackPayloadToText renders blocks as readable
plain text" test, which caught this consumer)
Teams and Discord converters produce identical output to before the change.
tests/notifications-channels.test.js, harness-notifications.test.js and
notify-hook.test.js pass 21/21.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…open #471 fixed the cmd.exe browser-open spawn (`start <url>` via shell:true) at four call sites, but each of those files makes a SECOND spawn that #471 missed: launching the Business Hub server itself by spawning node.exe directly with `detached: true` and no `windowsHide`. node.exe is a console-subsystem executable. Spawning one detached without windowsHide flashes a visible console window on win32 independent of the shell:true/cmd.exe case #471 addressed — so the flash kept happening on every Claude Code session start (SessionStart hook -> autoLaunchBusinessHub) and every Pi session start, exactly where users would notice it most. Fixed the two call sites: - src/hooks/auto-launch.js (Claude Code SessionStart hook) - src/integrations/pi/rstack-sdlc.ts (Pi session start) autoLaunchBusinessHub now accepts an injectable `spawnImpl` (mirroring the existing openBrowser/startContainerRuntime convention) so the windowsHide behavior on the server-launch spawn is unit-testable without spawning a real background process. Added a regression test to tests/windows-console-flash-470.test.js alongside the existing #470 coverage. Full suite: 1559 pass / 51 fail, same 51 as an unmodified tree — all spawn ENOENT / temp-dir EBUSY, the documented pre-existing Windows-sandbox limitations, none touching the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughBusiness Hub spawn paths now hide detached Windows consoles and support injected spawning for tests. Slack notification formatting now emits standard section field blocks, while converters accept both section and legacy field payloads. ChangesBusiness Hub console launch
Slack field payload handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
PR Summary by QodoFix Win console flash on hub spawn; correct Slack Block Kit fields
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![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: 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 `@tests/windows-console-flash-470.test.js`:
- Around line 107-114: Make the autoLaunchBusinessHub spawn-branch test
deterministic by stubbing or injecting the isPortOpen check so the configured
port is guaranteed closed, and explicitly set RSTACK_NO_BUSINESS_HUB to enable
launching. Capture and restore the original RSTACK_NO_BUSINESS_HUB value
alongside the existing environment variables, preserving cleanup in all paths.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a68eea8f-56b8-4342-ac75-d644031ee94a
📒 Files selected for processing (7)
src/hooks/auto-launch.jssrc/integrations/pi/rstack-sdlc.tssrc/notifications/channels/discord.jssrc/notifications/channels/teams.jssrc/notifications/channels/text.jssrc/notifications/index.jstests/windows-console-flash-470.test.js
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+24
to
28
| // Fields ride on a section block; 'fields' is tolerated for older payloads. | ||
| if ((block.type === 'section' || block.type === 'fields') && block.fields) { | ||
| for (const f of block.fields) { | ||
| const raw = f.text || ''; | ||
| const parts = raw.split('\n'); |
There was a problem hiding this comment.
1. Duplicated slack fields parsing 📜 Skill insight ⚙ Maintainability
The PR repeats the same Slack Block Kit fields-handling logic (and explanatory comment) across multiple notification channel converters, creating a DRY violation and increasing maintenance risk if the parsing behavior changes again.
Agent Prompt
## Issue description
The Slack `fields` parsing logic is duplicated across multiple notification channel converters, violating DRY and making future schema tweaks easy to miss in one of the implementations.
## Issue Context
The same conditional logic and loop appear in `discord.js`, `teams.js`, and `text.js` to support both the correct Block Kit shape (`section` with `fields`) and a legacy tolerated shape (`type: 'fields'`).
## Fix Focus Areas
- src/notifications/channels/discord.js[24-33]
- src/notifications/channels/teams.js[27-40]
- src/notifications/channels/text.js[26-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+16
| * The original #470/#471 fix covered the browser-open spawn (cmd.exe /c | ||
| * start) at each call site but missed the OTHER spawn each of those files | ||
| * makes: launching the Business Hub SERVER itself by spawning node.exe | ||
| * directly with detached: true. node.exe is a console-subsystem executable, | ||
| * so that spawn flashes a console too, independent of the shell:true case — | ||
| * it kept flashing after #471 merged. Covered below for both the Claude | ||
| * Code hook (auto-launch.js) and the Pi integration (rstack-sdlc.ts). | ||
| * |
There was a problem hiding this comment.
2. Regression test lacks attribution metadata 📜 Skill insight ⚙ Maintainability
The updated regression test comment block references #470/#471 but does not include the required found-date and QA report path metadata. This reduces traceability for why/when the regression coverage was added and where the original report lives.
Agent Prompt
## Issue description
The regression test header comment is missing required attribution metadata (date found and QA report path).
## Issue Context
Compliance requires regression tests to include: issue ID, what broke, date found, and QA report path.
## Fix Focus Areas
- tests/windows-console-flash-470.test.js[1-18]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo flagged four issues on the eslint/brace-expansion security fix: - no test coverage for the TOLERATED_BUNDLED_ADVISORIES allowlist change - brace-expansion@5.0.8 declares engines.node "20 || >=22", excluding the Node 18 CI lane - the ">=5.0.8" override is unbounded and can float to a future major - the script comment mis-attributed the version bump to an eslint 9->10 bump that never happened (the real mechanism is the root override) Fix: bound the override to an exact "5.0.8" pin; export the pure tolerate/block decision logic from security-audit.mjs (evaluateAdvisories, isFullyBundled, advisoryIds) behind a main-guard so it's unit-testable without shelling out to npm audit, and add tests/security-audit.test.js covering the tolerate/block/allowlist-miss/severity-threshold paths, plus a pinned-allowlist regression test; correct the stale comment to document the real mechanism and the accepted Node 18 engines trade-off (no brace-expansion release fixes both GHSAs while keeping Node 18 support; it's a devDependency-only path and no engine-strict is set, so this doesn't fail CI in practice). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… path
CodeRabbit flagged that discord.js/teams.js/text.js all gained a branch
tolerating legacy block.type === 'fields' payloads (alongside the correct
section+fields shape), but only the section path was exercised in tests
— the compatibility guarantee could regress silently. Add a test that
feeds a legacy { type: 'fields', fields: [...] } block through all three
converters.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to fix-slack-block-kit-fields-block-type
…abbit) The regression test hardcoded RSTACK_BUSINESS_PORT=58471 assuming the port would be closed, but autoLaunchBusinessHub's real isPortOpen() does a live TCP probe — a listener on that port (however unlikely) would flip the branch and fail the test nondeterministically. It also didn't guard against an inherited RSTACK_NO_BUSINESS_HUB=1 short-circuiting the function before the spawn. Fix: add an injectable isPortOpenImpl (mirrors the existing spawnImpl convention) to autoLaunchBusinessHub, stub it in the test to guarantee the spawn branch, name the placeholder port as a constant, and explicitly clear/restore RSTACK_NO_BUSINESS_HUB alongside the other env vars. Skipped (not applicable): qodo's "regression test lacks attribution metadata" (date-found/QA-report-path) — no other test in this repo follows that format, so adding it here would be a one-off convention, not a fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-type' into fix-472-hub-server-spawn-console-flash
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.
Problem
The console-window flash from #470 is still happening — #471 fixed the wrong half of it.
Every affected file (
auto-launch.js,rstack-sdlc.ts,sandbox.js,dashboard/server.js) makes two spawns:spawn(cmd, [url], { shell: true })→ opens the browser viacmd.exe /c start— this one gotwindowsHidein Fix Windows console-window flash on browser/runtime auto-launch #471spawn(process.execPath, [binPath, ...], { detached: true })→ launches the Business Hub server itself — this one didn'tprocess.execPathisnode.exe, a console-subsystem executable. Spawning itdetached: truewithoutwindowsHideflashes a console window on win32 regardless ofshell— a separate cause from the one #471 fixed. It fires on the path users hit most: Claude CodeSessionStart→autoLaunchBusinessHub→ this spawn, and the equivalent Pi session-start path.Fix
Added
windowsHide: trueto the two missed call sites:src/hooks/auto-launch.js— Claude Code hub launchsrc/integrations/pi/rstack-sdlc.ts— Pi hub launchautoLaunchBusinessHubnow accepts an injectablespawnImpl(same convention asopenBrowser/startContainerRuntime) so this is unit-testable without spawning a real process.Verification
tests/windows-console-flash-470.test.js, alongside the existing Windows: browser/runtime auto-launch briefly flashes a visible console window #470 coverage — assertswindowsHide: trueon the server-launch spawn.tests/windows-console-flash-470.test.js: 7/7 pass.spawn npx ENOENT, Windows temp-dirEBUSY), the documented pre-existing sandbox limitations. None touch the changed files.🤖 Generated with Claude Code
Summary by CodeRabbit