Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Broker signal handling and validation packages/cli/src/cli/lib/core-maintenance.ts, packages/cli/src/cli/commands/core.test.ts, CHANGELOG.md |
The command logs the prospective SIGTERM during dry-run and attempts the signal during normal uninstall. Tests verify the process and project files remain unchanged during dry-run, and verify that normal uninstall sends SIGTERM once. The changelog records this behavior and updates the unreleased heading. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- AgentWorkforce/relay#1345: Adds CLI maintenance dry-run verification procedures related to the uninstall dry-run behavior.
Merge Risk: 
2a34b
-
The uninstall dry-run behavior is ready to merge with no identified blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description clearly explains the change, testing, and verification results. However, it omits the required RelayFlow Proof section and does not provide the required Test Plan checklist or manual-t… Add the required ## Test Plansection with checked statuses for tests and manual testing. Add the## RelayFlow Proofsection with exactly one valid change type and RelayFlow case, or usenon-functionalandn/aonly if applicable.Out of Scope Changes check The core-maintenance.ts change and the new Fixed changelog entry are in scope for #1986.
CHANGELOG.md also changes the heading## [Unreleased]to## [Unreleased - Major]. The PR description does …Restore the heading to ## [Unreleased], or remove the unrelated heading change from this PR. If theUnreleased - Majorheading is intended, explain its purpose in the PR description and link it to a separate issue.Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: preventing uninstall --dry-runfrom signaling the broker.Linked Issues check [ #1986] The issue requires thatuninstall --dry-rundoes not send SIGTERM to the broker PID in a valid connection.json, and that ordinary uninstall keeps broker shutdown.
At head, `runUninstallComm…Full details: Linked Issues check
-
Explanation
[
#1986] The issue requires thatuninstall --dry-rundoes not send SIGTERM to the broker PID in a valid connection.json, and that ordinary uninstall keeps broker shutdown.At head,
runUninstallCommandin packages/cli/src/cli/lib/core-maintenance.ts computesisDryRun = options.dryRun === truebefore the broker block. Whenconn.pid > 0andisDryRunis true, the code logs[dry-run] Would send SIGTERM to broker (pid: ...)and does not calldeps.killProcess. When not a dry run, the originaldeps.killProcess(conn.pid, 'SIGTERM')call runs inside the same try/catch. The existing dry-run branches for data and project cleanup are unchanged in the diff hunk shown.The PR summary and raw summary describe regression tests in packages/cli/src/cli/commands/core.test.ts. These cover dry-run with no signal and no file mutation, and ordinary uninstall with one SIGTERM to the recorded PID. The test diff could not be read, so the test content is taken from the summaries only. The implementation result is confirmed from the source diff.
-
Full details: Description check
-
Explanation
The description clearly explains the change, testing, and verification results. However, it omits the required RelayFlow Proof section and does not provide the required Test Plan checklist or manual-testing status.
Resolution
Add the required
## Test Plansection with checked statuses for tests and manual testing. Add the## RelayFlow Proofsection with exactly one valid change type and RelayFlow case, or usenon-functionalandn/aonly if applicable.
Full details: Out of Scope Changes check
-
Explanation
The core-maintenance.ts change and the new Fixed changelog entry are in scope for
#1986.CHANGELOG.md also changes the heading
## [Unreleased]to## [Unreleased - Major]. The PR description does not mention this change. The heading rename does not implement the dry-run fix, the regression coverage, or the ordinary-uninstall restoration check. It changes release metadata that the issue does not request.The Removed entry for Python workflows and the Python SDK is present in the base. The diff shows it only as unchanged context, so it is not a new change in this PR.
The test diff in core.test.ts could not be read. Its scope is therefore based on the summaries only.
Resolution
Restore the heading to
## [Unreleased], or remove the unrelated heading change from this PR. If theUnreleased - Majorheading is intended, explain its purpose in the PR description and link it to a separate issue.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the broker’s door,
“No signals sent” is what we’re here for.
The preview leaves the process sound,
While normal shutdown brings it down.
The changelog hops along the way.
Comment @coderabbitai help to get the list of available commands.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
 [Medium impact] This PR appears safe to merge; dry-run no longer signals the broker, and ordinary uninstall keeps its shutdown behavior.
|
Fixes #1986.
What changed
connection.jsoncontains a PID,uninstall --dry-runnow previews the broker shutdown instead of sendingSIGTERM.connection.jsonand mocked process-signalling: dry-run causes no signals or filesystem mutations, while ordinary uninstall still signals the recorded PID.Verification
vitest run packages/cli/src/cli/commands/core.test.ts: 171 passed.npm run typecheck: passed.runUninstallCommand).git diff --check: passed.The tests do not kill a real broker; process signalling is dependency-injected and asserted explicitly. The original issue contains an isolated reproduction against the published CLI.
Need help on this PR? Tag
@codesmith-botwith what you need.