Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/hooks/auto-launch.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ function isPortOpen(port) {
});
}

// spawnImpl is injectable (mirrors openBrowser's convention below) so the
// windowsHide behavior on the server-launch spawn is testable without
// actually spawning a background process.
export async function autoLaunchBusinessHub(projectRoot, opts = {}) {
if (process.env.RSTACK_NO_BUSINESS_HUB === '1') return;

const spawnImpl = opts.spawnImpl ?? spawn;
const port = Number(process.env.RSTACK_BUSINESS_PORT ?? 3008);
const already = await isPortOpen(port);
if (already) {
Expand All @@ -34,9 +38,14 @@ export async function autoLaunchBusinessHub(projectRoot, opts = {}) {
const args = ['--no-browser'];
if (projectRoot) args.push('--project', projectRoot);

const child = spawn(process.execPath, [binPath, ...args], {
const child = spawnImpl(process.execPath, [binPath, ...args], {
stdio: 'ignore',
detached: true,
// node.exe is itself a console-subsystem executable; spawning it detached
// without windowsHide flashes a visible console window on win32, same as
// the cmd.exe browser-launch case #470/#471 fixed — that fix covered only
// openBrowser's spawn, not this one.
windowsHide: true,
env: { ...process.env, RSTACK_NO_BROWSER: '1', RSTACK_BUSINESS_PORT: String(port) },
});
child.unref();
Expand Down
5 changes: 5 additions & 0 deletions src/integrations/pi/rstack-sdlc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ function tryRegisterAndLaunchHub(projectRoot: string): void {
const child = spawn(process.execPath, [binPath, "--no-browser", "--project", projectRoot], {
stdio: ["ignore", logFd, logFd],
detached: true,
// node.exe is itself a console-subsystem executable; spawning it detached
// without windowsHide flashes a visible console window on win32, same as
// the cmd.exe browser-launch case #470/#471 fixed — that fix covered only
// openUrl's spawn, not this one.
windowsHide: true,
env: { ...process.env, RSTACK_NO_BROWSER: "1", RSTACK_BUSINESS_PORT: String(port) },
});
child.unref();
Expand Down
3 changes: 2 additions & 1 deletion src/notifications/channels/discord.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ export function convertSlackToDiscord(slackPayload) {
if (block.type === 'section' && block.text) {
descriptionLines.push(block.text.text);
}
if (block.type === 'fields' && block.fields) {
// 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');
Comment on lines +24 to 28

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

Expand Down
3 changes: 2 additions & 1 deletion src/notifications/channels/teams.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ export function convertSlackToTeams(slackPayload) {
facts: []
});
}
if (block.type === 'fields' && block.fields) {
// Fields ride on a section block; 'fields' is tolerated for older payloads.
if ((block.type === 'section' || block.type === 'fields') && block.fields) {
const currentSection = sections[sections.length - 1] || { facts: [] };
if (!sections.includes(currentSection)) {
sections.push(currentSection);
Expand Down
3 changes: 2 additions & 1 deletion src/notifications/channels/text.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ export function slackPayloadToText(slackPayload) {
if (block.type === 'section' && block.text?.text) {
lines.push(stripMrkdwn(block.text.text));
}
if (block.type === 'fields' && Array.isArray(block.fields)) {
// Fields ride on a section block; 'fields' is tolerated for older payloads.
if ((block.type === 'section' || block.type === 'fields') && Array.isArray(block.fields)) {
for (const field of block.fields) {
const parts = String(field.text ?? '').split('\n');
const name = stripMrkdwn(parts[0]);
Expand Down
6 changes: 4 additions & 2 deletions src/notifications/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ export function formatSlackStageMessage(runId, stageId, status, details = {}) {
},
},
{
type: 'fields',
// Block Kit has no 'fields' block type — fields live on a section block.
type: 'section',
fields,
},
];
Expand Down Expand Up @@ -146,7 +147,8 @@ export function formatSlackTaskReportMessage(runId, taskId, trace) {
},
},
{
type: 'fields',
// Block Kit has no 'fields' block type — fields live on a section block.
type: 'section',
fields: [
{ type: 'mrkdwn', text: `*Run ID:*\n${runId}` },
{ type: 'mrkdwn', text: `*Task:*\n${taskId}` },
Expand Down
30 changes: 29 additions & 1 deletion tests/windows-console-flash-470.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@
* passes windowsHide on win32, and that non-win32 platforms are unaffected
* (the option is harmless there, but shell must stay false).
*
* 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).
*
Comment on lines +9 to +16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Informational

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

* owner: RStack developed by Richardson Gunde
*/

Expand All @@ -17,7 +25,7 @@
// call sites below instead of a direct import.
import test from 'node:test';
import assert from 'node:assert/strict';
import { openBrowser as openBrowserAutoLaunch } from '../src/hooks/auto-launch.js';
import { openBrowser as openBrowserAutoLaunch, autoLaunchBusinessHub } from '../src/hooks/auto-launch.js';
import { openUrl } from '../src/integrations/pi/rstack-sdlc.ts';
import { startContainerRuntime } from '../src/core/harness/sandbox.js';

Expand Down Expand Up @@ -94,6 +102,26 @@ test('pi/rstack-sdlc.ts openUrl: no-ops under CI (never pops a browser during au
assert.equal(calls.length, 0, 'openUrl must not spawn anything when CI is set');
});

test('auto-launch.js autoLaunchBusinessHub: windowsHide:true on the server-launch spawn (missed by #471)', async () => {
const calls = [];
// A high, effectively-never-bound port so isPortOpen() resolves false and
// the function takes the "spawn a fresh instance" branch under test.
const originalPort = process.env.RSTACK_BUSINESS_PORT;
const originalNoBrowser = process.env.RSTACK_NO_BROWSER;
process.env.RSTACK_BUSINESS_PORT = '58471';
process.env.RSTACK_NO_BROWSER = '1';
Comment on lines +109 to +112

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

4. 58471 hardcoded test port 📜 Skill insight ⚙ Maintainability

The test hardcodes the port value 58471 instead of using a named constant. This reduces
readability and makes future updates more error-prone.
Agent Prompt
## Issue description
A literal port number is used directly in the test (`'58471'`), which violates the magic-number rule.

## Issue Context
This port is intended to be an effectively-unused port to force `isPortOpen()` to return false.

## Fix Focus Areas
- tests/windows-console-flash-470.test.js[109-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

try {
await autoLaunchBusinessHub('/tmp/project', { spawnImpl: recordingSpawn(calls) });
Comment on lines +107 to +114

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Make the spawn branch deterministic.

isPortOpen() checks the real TCP port 58471, so another process can make the test skip the spawn and fail at calls.length === 1. The test also does not override an inherited RSTACK_NO_BUSINESS_HUB=1, which can return before spawning. Inject/stub the port check (or otherwise guarantee a closed port) and explicitly set/restore RSTACK_NO_BUSINESS_HUB.

🤖 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 `@tests/windows-console-flash-470.test.js` around lines 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.

} finally {
if (originalPort === undefined) delete process.env.RSTACK_BUSINESS_PORT; else process.env.RSTACK_BUSINESS_PORT = originalPort;
if (originalNoBrowser === undefined) delete process.env.RSTACK_NO_BROWSER; else process.env.RSTACK_NO_BROWSER = originalNoBrowser;
}
assert.equal(calls.length, 1);
assert.equal(calls[0].cmd, process.execPath);
assert.equal(calls[0].options.detached, true);
assert.equal(calls[0].options.windowsHide, true);
});
Comment on lines +107 to +123

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

5. Flaky port-based test 🐞 Bug ☼ Reliability

The new autoLaunchBusinessHub regression test assumes port 58471 is unused so isPortOpen()
returns false; if any process is listening on 127.0.0.1:58471, autoLaunchBusinessHub() will
early-return and never call the injected spawnImpl, causing a nondeterministic test failure.
Agent Prompt
### Issue description
`tests/windows-console-flash-470.test.js` hard-codes `RSTACK_BUSINESS_PORT=58471` to force `autoLaunchBusinessHub()` down the spawn path. Because `autoLaunchBusinessHub()` performs a real TCP probe (`isPortOpen`) before spawning, a listener on that port will flip the branch and make the test fail intermittently.

### Issue Context
This test is intended to validate `windowsHide: true` on the Business Hub *server-launch* spawn. It should be deterministic and not depend on the machine’s ambient port usage.

### Fix Focus Areas
- src/hooks/auto-launch.js[10-35]
- tests/windows-console-flash-470.test.js[105-123]

### Suggested fix
Add a second injectable dependency to `autoLaunchBusinessHub`, e.g. `opts.isPortOpenImpl ?? isPortOpen`, and use that instead of calling `isPortOpen` directly. In the test, pass `isPortOpenImpl: async () => false` so the spawn path is guaranteed without relying on any specific port.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


test('sandbox.js startContainerRuntime: windowsHide:true on the engine auto-start spawn', async () => {
const calls = [];
const result = await startContainerRuntime({
Expand Down
Loading