refactor(broker): replace helpers.rs with focused modules#907
Merged
Conversation
Contributor
|
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 (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors the monolithic ChangesRefactor broker helpers into focused utility modules by domain ownership
Estimated code review effort: Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 4
🧹 Nitpick comments (2)
.trajectories/completed/2026-05/traj_jbo2x14y7ovt.json (1)
51-51: ⚡ Quick winPrefer repo-relative
projectIdover absolute local paths.Line 51 stores a machine-specific absolute path (
/Users/will/...), which leaks local environment details and makes trajectory artifacts less portable. Prefer a repo-relative identifier (or a stable logical project key) for committed metadata.Proposed adjustment
- "projectId": "/Users/will/Projects/AgentWorkforce/relay", + "projectId": ".",🤖 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 @.trajectories/completed/2026-05/traj_jbo2x14y7ovt.json at line 51, The committed trajectory JSON contains a machine-specific absolute path in the "projectId" field; update the projectId value to a repo-relative identifier or stable logical key (e.g., "relay" or "./") instead of "/Users/will/Projects/AgentWorkforce/relay". Locate the "projectId" key in the .trajectories/completed JSON entry (the "projectId" property) and replace the absolute local filesystem path with a portable repo-relative or canonical project identifier, then re-save the artifact..trajectories/index.json (1)
985-990: ⚡ Quick winUse a repository-relative path for the new trajectory index entry.
Line 990 records an absolute local filesystem path. For committed index metadata, a repo-relative path is safer and portable across contributors/machines.
Proposed adjustment
- "path": "/Users/will/Projects/AgentWorkforce/relay/.trajectories/completed/2026-05/traj_jbo2x14y7ovt.json" + "path": ".trajectories/completed/2026-05/traj_jbo2x14y7ovt.json"🤖 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 @.trajectories/index.json around lines 985 - 990, The "traj_jbo2x14y7ovt" index entry contains an absolute filesystem path in its "path" property; replace that absolute path with a repository-relative path (for example ".trajectories/completed/2026-05/traj_jbo2x14y7ovt.json" or "./.trajectories/..." consistent with other entries) so the index is portable across machines, and ensure the "path" value format matches the rest of the entries in the JSON.
🤖 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 `@crates/broker/src/broker/continuity.rs`:
- Around line 55-58: The parser currently falls back to unwrap_or(i + 2) when
computing bytes_consumed (assumes ACTION: is immediately after KIND:) which
under-consumes if blank/metadata lines appear; change the logic that computes
bytes_consumed so it uses the actual index found in the loop instead of i+2—use
the loop index j (or the index where you set body_start_line/action) as the
fallback. Concretely, in the code that iterates with for (j, line_at_j) in
lines.iter().enumerate().skip(i + 1) and sets action: Option<ContinuityAction>
and body_start_line: Option<usize>, replace the unwrap_or(i + 2) fallback with a
value derived from j or the discovered body_start_line/action index (e.g.,
bytes_consumed = body_start_line.unwrap_or(action_index_or_j)) so the
consumed-line count reflects where ACTION or the body actually begins rather
than assuming the next line.
In `@crates/broker/src/relaycast/dm_participants.rs`:
- Around line 9-10: get_dm_participants currently never caches failures so
repeated calls for the same cache_key trigger upstream requests on every
message; add a failure-backoff entry keyed by cache_key (separate from
DM_PARTICIPANT_CACHE_TTL) to throttle retries. Implement a new constant (e.g.
DM_PARTICIPANT_FAILURE_TTL) and on upstream failure insert a lightweight failure
marker into the same cache (respecting MAX_DM_CACHE_ENTRIES) so subsequent calls
check the cache first, return a failure/empty result or short-circuit until the
failure TTL expires, and ensure the failure path in get_dm_participants and
related logic (the cache lookup/insert points) use this marker rather than
always attempting the upstream call.
In `@crates/broker/src/util/terminal.rs`:
- Around line 61-63: The check trimmed.starts_with('\n') is unreachable because
trimmed was produced with trim_start() which removes leading newlines; change
the condition to inspect the original string before trimming (use
after_pattern.starts_with('\n')) so the editor-mode detection correctly detects
a leading newline and prevents incorrect auto-enter; update the if condition
that currently reads `if trimmed.is_empty() || trimmed.starts_with('\n')` to use
`after_pattern.starts_with('\n')` instead of checking trimmed for the newline.
In `@crates/broker/src/worker/detection.rs`:
- Around line 26-32: The current logic in detection.rs using the
relevant_output/clean_output/expected_echo flow only removes the first
occurrence of expected_echo, leaving duplicates that can still trigger activity;
change the handling so all echoed relay lines are removed (e.g., replace all
occurrences of expected_echo from clean_output or iteratively strip every match)
and return the fully-cleaned string into relevant_output so no remaining echoes
remain to cause activity markers.
---
Nitpick comments:
In @.trajectories/completed/2026-05/traj_jbo2x14y7ovt.json:
- Line 51: The committed trajectory JSON contains a machine-specific absolute
path in the "projectId" field; update the projectId value to a repo-relative
identifier or stable logical key (e.g., "relay" or "./") instead of
"/Users/will/Projects/AgentWorkforce/relay". Locate the "projectId" key in the
.trajectories/completed JSON entry (the "projectId" property) and replace the
absolute local filesystem path with a portable repo-relative or canonical
project identifier, then re-save the artifact.
In @.trajectories/index.json:
- Around line 985-990: The "traj_jbo2x14y7ovt" index entry contains an absolute
filesystem path in its "path" property; replace that absolute path with a
repository-relative path (for example
".trajectories/completed/2026-05/traj_jbo2x14y7ovt.json" or
"./.trajectories/..." consistent with other entries) so the index is portable
across machines, and ensure the "path" value format matches the rest of the
entries in the JSON.
🪄 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: 23da9c40-1506-47d6-8a9b-d32038560847
📒 Files selected for processing (27)
.trajectories/completed/2026-05/traj_jbo2x14y7ovt.json.trajectories/completed/2026-05/traj_jbo2x14y7ovt.md.trajectories/index.jsoncrates/broker/src/broker.rscrates/broker/src/broker/continuity.rscrates/broker/src/broker/delivery_verification.rscrates/broker/src/broker/injection_format.rscrates/broker/src/cli/command_parse.rscrates/broker/src/cli/mod.rscrates/broker/src/helpers.rscrates/broker/src/main.rscrates/broker/src/pty_worker.rscrates/broker/src/readiness.rscrates/broker/src/relaycast/dm_participants.rscrates/broker/src/relaycast/identity.rscrates/broker/src/relaycast/mod.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/tests.rscrates/broker/src/spawner.rscrates/broker/src/swarm.rscrates/broker/src/util/ansi.rscrates/broker/src/util/mod.rscrates/broker/src/util/terminal.rscrates/broker/src/wait.rscrates/broker/src/worker.rscrates/broker/src/worker/detection.rscrates/broker/src/wrap.rs
💤 Files with no reviewable changes (1)
- crates/broker/src/helpers.rs
barryollama
approved these changes
May 19, 2026
barryollama
left a comment
barryollama
left a comment
There was a problem hiding this comment.
Code Review: refactor(broker): replace helpers.rs with focused modules
Verdict: ✅ Approve
Summary
This PR successfully decomposes the monolithic helpers.rs (1,631 lines) into well-organized, domain-focused modules. This is a pure refactoring with no behavioral changes — code is moved as-is with tests co-located.
Structural Changes Reviewed
| New Module | Purpose | Lines | Tests |
|---|---|---|---|
cli/command_parse.rs |
CLI command parsing, cursor→agent mapping | 137 | ✅ |
broker/continuity.rs |
Continuity command block parsing | 135 | ✅ |
broker/delivery_verification.rs |
Delivery verification, throttling | 275 | ✅ |
broker/injection_format.rs |
Message injection formatting, MCP hints | 338 | ✅ |
relaycast/dm_participants.rs |
DM participant caching with TTL | 145 | ✅ |
relaycast/identity.rs |
Case-insensitive agent name comparison | 51 | ✅ |
util/ansi.rs |
ANSI escape code stripping | 97 | ✅ |
util/terminal.rs |
Terminal prompt detection (vim, permissions, etc.) | 371 | ✅ |
worker/detection.rs |
Activity detection for Claude/Codex/Gemini | 160 | ✅ |
Verification Performed
- ✅
cargo check -p agent-relay-broker— clean compilation - ✅
cargo test -p agent-relay-broker— 360 tests passed - ✅
cargo test --r(continuity tests) — 12 tests passed
Code Quality Observations
- No logic changes: All code moved verbatim from
helpers.rs - Tests co-located: Unit tests moved alongside implementation (e.g.,
cli/command_parse.rstests validate the parsing) - Clean imports: All consuming modules updated to use new paths (
use crate::cli::command_parse::..., etc.) - Module structure: Clean
mod.rsfiles forcli/,broker/,relaycast/,util/,worker/
Suggestions (non-blocking)
- The trajectory files (
.trajectories/) are included — these provide audit trail of the refactoring process via thetrailtool
Bottom Line
Clean, well-scoped refactoring that improves code organization without changing behavior. The module boundaries align with domain concepts (CLI parsing, delivery verification, terminal detection, etc.).
Reviewed by Hermes Agent
This was referenced May 19, 2026
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.
Closes #876
Summary
Validation