Fix headless broker readiness#855
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
khaliqgant
force-pushed
the
codex/fix-broker-headless-reliability-doc
branch
2 times, most recently
from
28234d3 to
ccd3440
Compare
khaliqgant
force-pushed
the
codex/fix-broker-headless-reliability-doc
branch
from
ccd3440 to
4da9b34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.trajectories/index.json:
- Around line 406-453: The index currently records machine-local absolute paths
for trajectory entries (e.g., the "path" values under keys traj_v1wexlfur5zr,
traj_6sjeohtm3php, traj_4vucir4qvqa2, traj_erzd7j9nto9r, traj_7uznwzoxbao6,
traj_9fdv7hxm0b60, traj_0o6gb2wvk59t); change each "path" value to a
repo-relative path (for example
".trajectories/completed/2026-05/traj_v1wexlfur5zr.json") preserving the
filename and directory structure, and update all similar new entries in this
block to use the same normalization so no absolute /Users/... paths are
committed.
In `@src/cli/lib/broker-lifecycle.ts`:
- Around line 393-411: In childUpArgsForDetachedStart, detect if the original
CLI args (from cliUserArgs(deps)) contain both the background and foreground
flags (using matchesCliOption or hasCliOption) and fail fast instead of silently
preferring one: if both are present throw a clear error (e.g., throw new
Error('Cannot specify both --background and --foreground')) or otherwise return
a failure to the caller, before stripping flags and rebuilding the child argv;
reference the function childUpArgsForDetachedStart and helper functions
matchesCliOption/hasCliOption when adding this validation.
- Around line 1509-1510: The current logic silently coerces invalid or partial
`--wait-for` values (options?.waitFor) into 0 via Number.parseFloat, so update
the validation: before computing waitSeconds/waitMs, verify the raw string is a
valid non-negative decimal number (e.g. trim then match against a regex like
/^\d+(\.\d+)?$/ or similar) and reject anything with extra characters or a
negative sign; if invalid, emit a clear error and exit/fail instead of
continuing. Only call Number.parseFloat and compute waitMs (waitSeconds * 1000)
after the string passes this validation; update the code paths around the
existing waitSeconds and waitMs variables to use the validated value.
🪄 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 Plus
Run ID: 8d990823-71de-4847-a689-b24dda0de924
📒 Files selected for processing (19)
.trajectories/completed/2026-05/traj_0o6gb2wvk59t.json.trajectories/completed/2026-05/traj_0o6gb2wvk59t.md.trajectories/completed/2026-05/traj_4vucir4qvqa2.json.trajectories/completed/2026-05/traj_4vucir4qvqa2.md.trajectories/completed/2026-05/traj_6sjeohtm3php.json.trajectories/completed/2026-05/traj_6sjeohtm3php.md.trajectories/completed/2026-05/traj_7uznwzoxbao6.json.trajectories/completed/2026-05/traj_7uznwzoxbao6.md.trajectories/completed/2026-05/traj_9fdv7hxm0b60.json.trajectories/completed/2026-05/traj_9fdv7hxm0b60.md.trajectories/completed/2026-05/traj_erzd7j9nto9r.json.trajectories/completed/2026-05/traj_erzd7j9nto9r.md.trajectories/completed/2026-05/traj_v1wexlfur5zr.json.trajectories/completed/2026-05/traj_v1wexlfur5zr.md.trajectories/index.jsonsrc/cli/commands/core.test.tssrc/cli/commands/core.tssrc/cli/lib/broker-lifecycle.tstests/integration/broker/utils/broker-harness.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/cli/lib/broker-lifecycle.ts`:
- Around line 1584-1597: The readiness check currently treats a broker as
STARTING when readBrokerStatusDetails returns null because getSession failed,
but readiness should be decided from getStatus() alone; change
readBrokerStatusDetails (or its callers) so it returns status even if session
lookup fails (i.e., make session optional), and update the logic around
statusDetails (and the similar block at 1601-1606) to consider the broker UP
when status exists regardless of session, only attempting to log session
information if session !== undefined; reference getStatus(), getSession(),
readBrokerStatusDetails, and the readiness.statusDetails variable when making
this change.
- Around line 335-356: The orphan-matching is too fuzzy: using
path.basename(projectRoot) via brokerName and the fallback
process.command.includes(projectRoot) can match unrelated repos; tighten
matching in the broker detection logic (around brokerName, candidates,
deps.execCommand, parsePsAuxLine, isBrokerExecutableCommand,
commandHasBrokerName) so we only pick processes whose executable or
working-directory path exactly corresponds to the project root. Replace the
basename fuzzy check with a stricter check that compares the resolved
projectRoot path (path.resolve(projectRoot)) against the process command or the
command's executable path (the first token from process.command) using path
separators or an exact suffix match (e.g., endsWith or contains
`${resolvedProjectRoot}${path.sep}`) and in the fallback avoid simple
.includes(projectRoot) in favor of the same boundary-aware comparison to prevent
matching sibling directories before sending SIGTERM.
🪄 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 Plus
Run ID: c288e147-4ed9-420c-976d-d32f2fe540db
📒 Files selected for processing (5)
.trajectories/completed/2026-05/traj_4chzkm724ufo.json.trajectories/completed/2026-05/traj_4chzkm724ufo.md.trajectories/index.jsonsrc/cli/commands/core.test.tssrc/cli/lib/broker-lifecycle.ts
✅ Files skipped from review due to trivial changes (2)
- .trajectories/completed/2026-05/traj_4chzkm724ufo.json
- .trajectories/index.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/commands/core.test.ts
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
1 participant
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.
Summary
agent-relay up --no-dashboarddetach by default but return success only after broker API readinessstatus --wait-forsemantics for RUNNING, STARTING, and STOPPED timeout casesVerification
npm exec tsc -- --noEmit --pretty falsenpm exec vitest -- run src/cli/commands/core.test.tsnpm exec eslint -- src/cli/lib/broker-lifecycle.ts src/cli/commands/core.ts src/cli/commands/core.test.ts(0 errors; existing warnings only)git diff --check