fix: wrap Makefile subshell cd commands in parentheses

[danpiths]

Summary

Wraps cd ui && ... commands in subshells (cd ui && ...) in the dev and
dev-pulse Make targets to prevent the cd from leaking into the parent shell
process when backgrounded with &.

Changes

• Wrapped 4 cd ui && npm run dev invocations in parentheses for both dev and
  dev-pulse targets
• Without parentheses, cd ui changes the working directory of the parent shell
  when the command is backgrounded, which can cause subsequent commands in the
  recipe to run from the wrong directory

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

make dev
# Verify both UI dev server and API server start correctly

Screenshots/Recordings

N/A

Breaking changes

• Yes
• No

Related issues

N/A

Security considerations

None.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[danpiths]

• feat: improve realtime log detail UI with voice, transport, and audio tokens #3336
• feat: enrich realtime routing, logging, cost, and session tracking #3335
• feat: add Azure realtime provider and nested model normalization #3334
• fix: wrap Makefile subshell cd commands in parentheses #3333 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

🧪 Test Suite Available

This PR can be tested by a repository admin.

Run tests for PR #3333

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Chores
  • More reliable UI dev server startup when the profiler is disabled.
  • Added explicit tracking of background server PIDs and a robust shutdown that terminates those processes and their children.
  • Reduced risk of background-process race conditions during dev startup.
  • Preserves the overall startup flow (including API server and pulse variant behavior).

Walkthrough

The Makefile's dev and dev-pulse targets run the UI dev server under DISABLE_PROFILER in a grouped subshell, and dev now explicitly records ui_pid/api_pid and uses PID-based cleanup/traps to terminate processes and their children (TERM then KILL) before waiting.

Changes

Development Build Targets

Layer / File(s)	Summary
Cleanup and PID tracking Makefile	dev disables job-control, introduces ui_pid/api_pid, and replaces job-control kill %1 %2 with cleanup()/stop_dev() that terminates tracked PIDs and their child processes (TERM then KILL) and waits.
UI subshell startup and API launch Makefile	In dev DISABLE_PROFILER path, UI is started as (cd ui && BIFROST_DISABLE_PROFILER=1 npm run dev) & with its PID saved to ui_pid; API server is started and api_pid saved; Makefile waits for either to exit to trigger cleanup.
dev-pulse subshell change Makefile	dev-pulse DISABLE_PROFILER UI startup changed to (cd ui && BIFROST_DISABLE_PROFILER=1 npm run dev) & (no other behavior changes).

🎯 3 (Moderate) | ⏱️ ~20 minutes

I hopped through Makefile lines with a grin,
Wrapped the UI start so the background can spin,
Profiler muted, dev server tucked in a shell,
PIDs tracked, signals sent, processes bid farewell,
A rabbit's small cheer for builds that run and chill. 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: wrapping Makefile subshell cd commands in parentheses to fix a shell behavior issue.
Description check	✅ Passed	The description includes all major template sections with appropriate content: summary explaining the problem, detailed changes, bug fix classification, testing instructions, and completed checklist items.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/05-08-fix_wrap_makefile_subshell_cd_commands_in_parentheses

---

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

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the subshell wrapping correctly scopes directory changes, and the new PID-based cleanup in dev is a straightforward improvement.

The core fix (subshell wrapping) is correct and non-breaking. The additional dev process management refactor is well-scoped and uses standard POSIX primitives. The only gap is that dev-pulse was not brought in line with the new approach, which is a consistency nit rather than a functional defect.

No files require special attention.

Important Files Changed

Filename	Overview
Makefile	Wraps cd commands in subshells for both dev and dev-pulse targets; additionally refactors dev from job-control (set -m / kill %1 %2) to explicit PID-based process management

_{Reviews (5): Last reviewed commit: "fix: wrap Makefile subshell cd commands ..." | Re-trigger Greptile}

The base branch was changed.

The merge-base changed after approval.

The merge-base changed after approval.

The merge-base changed after approval.

[coderabbitai]

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 `@Makefile`:
- Around line 159-178: The cleanup() target currently kills only direct children
found via pgrep -P and leaves deeper descendants running; update it to terminate
entire process trees by targeting process groups or recursive descendants for
both ui_pid and api_pid (the variables ui_pid/api_pid and the cleanup() function
are the relevant symbols). Specifically, when sending TERM/KILL, prefer killing
the whole process group (send signals to negative pid for the process group id)
and/or recursively collect all descendant PIDs before sending signals so no deep
child is left running; keep the existing logging messages and fall back to the
pgrep -P approach only if process-group signaling is unavailable.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b969f6eb-2d30-479b-a699-72d34a8a61db

📥 Commits

Reviewing files that changed from the base of the PR and between 49f495f and 4b8c081.

📒 Files selected for processing (1)

• Makefile

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Kill the full process tree during make dev cleanup.

This cleanup only targets direct children of the tracked subshell. With the process chain started at Line 225, that can kill the shell and its first child while leaving a deeper long-lived child behind, so the next make dev can come up with a stale process still holding $(PORT).

♻️ Suggested fix

 cleanup() { \
 	$(ECHO) "$(YELLOW)[make dev] cleanup started; ui_pid=$$ui_pid api_pid=$$api_pid$(NC)"; \
 	trap - EXIT INT TERM HUP; \
+	kill_tree() { \
+		local sig="$$1" pid="$$2" child; \
+		for child in $$(pgrep -P "$$pid" 2>/dev/null || true); do \
+			kill_tree "$$sig" "$$child"; \
+		done; \
+		kill -s "$$sig" "$$pid" 2>/dev/null || true; \
+	}; \
 	for pid in "$$ui_pid" "$$api_pid"; do \
 		if [ -n "$$pid" ]; then \
-			children="$$(pgrep -P "$$pid" 2>/dev/null || true)"; \
-			$(ECHO) "$(YELLOW)[make dev] sending TERM to pid $$pid and children: $${children:-none}$(NC)"; \
-			kill -TERM $$children "$$pid" 2>/dev/null || true; \
+			$(ECHO) "$(YELLOW)[make dev] sending TERM to pid $$pid tree$(NC)"; \
+			kill_tree TERM "$$pid"; \
 		fi; \
 	done; \
 	sleep 1; \
 	for pid in "$$ui_pid" "$$api_pid"; do \
 		if [ -n "$$pid" ]; then \
-			children="$$(pgrep -P "$$pid" 2>/dev/null || true)"; \
-			$(ECHO) "$(YELLOW)[make dev] sending KILL to pid $$pid and remaining children: $${children:-none}$(NC)"; \
-			kill -KILL $$children "$$pid" 2>/dev/null || true; \
+			$(ECHO) "$(YELLOW)[make dev] sending KILL to pid $$pid tree$(NC)"; \
+			kill_tree KILL "$$pid"; \
 		fi; \
 	done; \

🤖 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 `@Makefile` around lines 159 - 178, The cleanup() target currently kills only
direct children found via pgrep -P and leaves deeper descendants running; update
it to terminate entire process trees by targeting process groups or recursive
descendants for both ui_pid and api_pid (the variables ui_pid/api_pid and the
cleanup() function are the relevant symbols). Specifically, when sending
TERM/KILL, prefer killing the whole process group (send signals to negative pid
for the process group id) and/or recursively collect all descendant PIDs before
sending signals so no deep child is left running; keep the existing logging
messages and fall back to the pgrep -P approach only if process-group signaling
is unavailable.

[akshaydeo]

Merge activity

• May 14, 1:58 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 14, 1:59 PM UTC: @akshaydeo merged this pull request with Graphite.