GH-4282: resolve pulse-wrapper worker count review feedback#4357
GH-4282: resolve pulse-wrapper worker count review feedback#4357alex-solovyev merged 3 commits intomainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR refines the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 unit tests (beta)
📝 Coding Plan
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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Fri Mar 13 06:01:03 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.agents/scripts/pulse-wrapper.sh (1)
1920-1923: Harden supervisor exclusion to argument boundaries.The current
index()checks are substring-based, so a worker line containing both tokens in free-form text can be excluded accidentally. Prefer token-boundary matching for--role pulseand--session-key supervisor-pulse.♻️ Proposed refinement
count=$(ps axo command | awk ' - index($0, ".opencode run") > 0 && - index($0, "/full-loop") > 0 && - !(index($0, "--role pulse") > 0 && index($0, "--session-key supervisor-pulse") > 0) { + index($0, ".opencode run") > 0 && + index($0, "/full-loop") > 0 && + !( + $0 ~ /(^|[[:space:]])--role([=[:space:]])pulse([[:space:]]|$)/ && + $0 ~ /(^|[[:space:]])--session-key([=[:space:]])supervisor-pulse([[:space:]]|$)/ + ) { count++ } END { print count + 0 } ') || count=0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/pulse-wrapper.sh around lines 1920 - 1923, The substring checks in the conditional using index($0, "--role pulse") and index($0, "--session-key supervisor-pulse") can falsely match inside other text; update the condition that currently checks index($0, ".opencode run"), "/full-loop", and the two index(...) checks to use token-boundary regex matches instead (e.g. replace the index(...) checks with match($0, /(^|[[:space:]])--role[[:space:]]*pulse($|[[:space:]])/) and match($0, /(^|[[:space:]])--session-key[[:space:]]*supervisor-pulse($|[[:space:]])/)) so only exact argument tokens for "--role pulse" and "--session-key supervisor-pulse" trigger the exclusion; keep the rest of the condition (the .opencode run and /full-loop checks and the count++ action) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/tests/test-pulse-wrapper-worker-count.sh:
- Around line 109-112: The trap for cleanup is registered after calling
setup_test_env which can leave temp artifacts if setup_test_env fails; move the
trap registration so trap teardown_test_env EXIT is executed before calling
setup_test_env in main(), ensuring teardown_test_env is always registered (with
the same trap command and EXIT signal) prior to invoking setup_test_env.
---
Nitpick comments:
In @.agents/scripts/pulse-wrapper.sh:
- Around line 1920-1923: The substring checks in the conditional using index($0,
"--role pulse") and index($0, "--session-key supervisor-pulse") can falsely
match inside other text; update the condition that currently checks index($0,
".opencode run"), "/full-loop", and the two index(...) checks to use
token-boundary regex matches instead (e.g. replace the index(...) checks with
match($0, /(^|[[:space:]])--role[[:space:]]*pulse($|[[:space:]])/) and match($0,
/(^|[[:space:]])--session-key[[:space:]]*supervisor-pulse($|[[:space:]])/)) so
only exact argument tokens for "--role pulse" and "--session-key
supervisor-pulse" trigger the exclusion; keep the rest of the condition (the
.opencode run and /full-loop checks and the count++ action) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5235b3a1-60f2-424d-b454-bf99988cbb25
📒 Files selected for processing (2)
.agents/scripts/pulse-wrapper.sh.agents/scripts/tests/test-pulse-wrapper-worker-count.sh
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Fri Mar 13 07:02:33 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
…ve_workers Use regex token-boundary matching for --role pulse and --session-key supervisor-pulse instead of substring index() checks, preventing accidental exclusion of workers whose command text contains these tokens in free-form context. Addresses CodeRabbit review feedback on PR #4357.
|
Dispatching worker to address unresolved CodeRabbit suggestion.
|
|
Addressed CodeRabbit feedback: replaced substring |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Fri Mar 13 07:04:27 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Review feedback addressedBoth CodeRabbit suggestions have been applied: 1. Harden supervisor exclusion to argument boundaries (nitpick on $0 ~ /(^|[[:space:]])--role([=[:space:]])pulse([[:space:]]|$)/ &&
$0 ~ /(^|[[:space:]])--session-key([=[:space:]])supervisor-pulse([[:space:]]|$)/This prevents accidental exclusion of workers whose command text contains these tokens in free-form context (e.g. a worker prompt that mentions 2. Register cleanup trap before setup (actionable comment on |
Three issues from CodeRabbit review on PR #4357 (GH#4282): 1. Unify global capacity counting: prefetch_active_workers() used a grep pipeline that diverged from count_active_workers(). Replace with the same awk filter so the active-worker snapshot shown to the pulse agent is always consistent with the global capacity counter. 2. Narrow /pulse exclusion to argument boundaries: count_active_workers() had a !( newline ) awk pattern that caused a GNU awk parse error (unexpected newline after '!'). Collapse to a single-line !(...) guard. prefetch_active_workers() now uses the same token-boundary regex (--role pulse + --session-key supervisor-pulse as whole arguments) instead of a broad substring grep that could hide legitimate workers. 3. Exact --dir matching in has_worker_for_repo_issue(): replace index($0, path) > 0 (substring match) with a regex that anchors path as a whole --dir argument (space or = form, optional trailing slash). Prevents sibling-path false positives such as /tmp/aidevops matching /tmp/aidevops-tools. Add four regression tests covering the new behaviours: - prefetch_active_workers excludes supervisor pulse - prefetch_active_workers count matches count_active_workers - has_worker_for_repo_issue rejects sibling-path match - has_worker_for_repo_issue accepts exact path match Closes #4282
Three issues from CodeRabbit review on PR #4357 (GH#4282): 1. Unify global capacity counting: prefetch_active_workers() used a grep pipeline that diverged from count_active_workers(). Replace with the same awk filter so the active-worker snapshot shown to the pulse agent is always consistent with the global capacity counter. 2. Narrow /pulse exclusion to argument boundaries: count_active_workers() had a !( newline ) awk pattern that caused a GNU awk parse error (unexpected newline after '!'). Collapse to a single-line !(...) guard. prefetch_active_workers() now uses the same token-boundary regex (--role pulse + --session-key supervisor-pulse as whole arguments) instead of a broad substring grep that could hide legitimate workers. 3. Exact --dir matching in has_worker_for_repo_issue(): replace index($0, path) > 0 (substring match) with a regex that anchors path as a whole --dir argument (space or = form, optional trailing slash). Prevents sibling-path false positives such as /tmp/aidevops matching /tmp/aidevops-tools. Add four regression tests covering the new behaviours: - prefetch_active_workers excludes supervisor pulse - prefetch_active_workers count matches count_active_workers - has_worker_for_repo_issue rejects sibling-path match - has_worker_for_repo_issue accepts exact path match Closes #4282
Three issues from CodeRabbit review on PR #4357 (GH#4282): 1. Unify global capacity counting: prefetch_active_workers() used a grep pipeline that diverged from count_active_workers(). Replace with the same awk filter so the active-worker snapshot shown to the pulse agent is always consistent with the global capacity counter. 2. Narrow /pulse exclusion to argument boundaries: count_active_workers() had a !( newline ) awk pattern that caused a GNU awk parse error (unexpected newline after '!'). Collapse to a single-line !(...) guard. prefetch_active_workers() now uses the same token-boundary regex (--role pulse + --session-key supervisor-pulse as whole arguments) instead of a broad substring grep that could hide legitimate workers. 3. Exact --dir matching in has_worker_for_repo_issue(): replace index($0, path) > 0 (substring match) with a regex that anchors path as a whole --dir argument (space or = form, optional trailing slash). Prevents sibling-path false positives such as /tmp/aidevops matching /tmp/aidevops-tools. Add four regression tests covering the new behaviours: - prefetch_active_workers excludes supervisor pulse - prefetch_active_workers count matches count_active_workers - has_worker_for_repo_issue rejects sibling-path match - has_worker_for_repo_issue accepts exact path match Closes #4282
#4456) Three issues from CodeRabbit review on PR #4357 (GH#4282): 1. Unify global capacity counting: prefetch_active_workers() used a grep pipeline that diverged from count_active_workers(). Replace with the same awk filter so the active-worker snapshot shown to the pulse agent is always consistent with the global capacity counter. 2. Narrow /pulse exclusion to argument boundaries: count_active_workers() had a !( newline ) awk pattern that caused a GNU awk parse error (unexpected newline after '!'). Collapse to a single-line !(...) guard. prefetch_active_workers() now uses the same token-boundary regex (--role pulse + --session-key supervisor-pulse as whole arguments) instead of a broad substring grep that could hide legitimate workers. 3. Exact --dir matching in has_worker_for_repo_issue(): replace index($0, path) > 0 (substring match) with a regex that anchors path as a whole --dir argument (space or = form, optional trailing slash). Prevents sibling-path false positives such as /tmp/aidevops matching /tmp/aidevops-tools. Add four regression tests covering the new behaviours: - prefetch_active_workers excludes supervisor pulse - prefetch_active_workers count matches count_active_workers - has_worker_for_repo_issue rejects sibling-path match - has_worker_for_repo_issue accepts exact path match Closes #4282
2 participants
Summary
grepactive worker count pipeline with a singleawkpass for lower process overhead.--role pulse+--session-key supervisor-pulsesignature so/full-loopworkers mentioning/pulseare still counted.Verification
shellcheck .agents/scripts/pulse-wrapper.shshellcheck .agents/scripts/tests/test-pulse-wrapper-worker-count.shbash .agents/scripts/tests/test-pulse-wrapper-worker-count.shCloses #4282
Summary by CodeRabbit
Refactor
Tests