fix(observer): replace hardcoded sleep 2 with PID file poll in start-observer.sh#2356
fix(observer): replace hardcoded sleep 2 with PID file poll in start-observer.sh#2356mc856 wants to merge 3 commits into
Conversation
…observer.sh Fixes affaan-m#2295 The previous `sleep 2` after launching the observer loop has two problems: on slow filesystems or loaded systems 2 seconds may not be enough, producing a false-negative on the subsequent PID file check; on healthy systems it adds unnecessary latency. Replace with a poll loop that exits as soon as the PID file appears: for _i in $(seq 1 50); do [ -f "$PID_FILE" ] && break; sleep 0.2; done 50 × 0.2s = 10s max wait (vs the previous fixed 2s), but typical startup returns within the first iteration. No behavior change in the success path — only the wait strategy changes. Tests: `node tests/run-all.js` 2891 passed / 0 failed; `npm run lint`, `catalog:check`, `command-registry:check` all clean.
…aan-m#2295) Asserts start-observer.sh never reverts to the fixed `sleep 2` wait and keeps the 50 × 0.2s `$PID_FILE` poll in place. Sits next to the existing observer-loop invariant block in tests/hooks/hooks.test.js, matching the repo's pattern of guarding shell-script invariants via source-content assertions. Without this, any future "cleanup" that reintroduces a fixed sleep would silently regress the slow-filesystem fix from the previous commit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (19)**/*.sh📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
skills/**/*.{js,ts,py,sh}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{skills,commands,agents,rules}/**⚙️ CodeRabbit configuration file
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
Files:
**/*.{test,spec}.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,json,env*}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{jsx,tsx,js,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,env*}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{jsx,tsx,html,js,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesObserver startup readiness
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@tests/hooks/hooks.test.js`:
- Around line 3158-3160: The polling check in hooks.test.js is too
formatting-specific because it matches one exact shell line, so refactors can
break it unnecessarily. Update the assertion around startObserverSource to
verify the behavior instead: assert that start-observer.sh polls for $PID_FILE
with a bounded retry loop and short sleep intervals, without requiring the exact
loop syntax. Keep the existing symbols startObserverSource and the
start-observer.sh expectations, but loosen the regex so it matches equivalent
polling implementations rather than a single hard-coded line.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fee86b00-473a-4457-afe9-a2bdb047f49b
📒 Files selected for processing (2)
skills/continuous-learning-v2/agents/start-observer.shtests/hooks/hooks.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (19)
**/*.sh
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before running shell commands, explain destructive or networked actions and prefer read-only inspection first
Files:
skills/continuous-learning-v2/agents/start-observer.sh
skills/**/*.{js,ts,py,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Place new workflow contributions in
skills/directory as the canonical workflow surface
Files:
skills/continuous-learning-v2/agents/start-observer.sh
{skills,commands,agents,rules}/**
⚙️ CodeRabbit configuration file
{skills,commands,agents,rules}/**: Focus on prompt-injection resilience, tool-permission scope, destructive action guards, and secret exfiltration risks.
Files:
skills/continuous-learning-v2/agents/start-observer.sh
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/hooks/hooks.test.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/hooks/hooks.test.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/hooks/hooks.test.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/hooks/hooks.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}: Write tests before implementation using TDD workflow: write failing test (RED), implement minimal code (GREEN), then refactor (IMPROVE)
Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)
Avoid deep nesting (>4 levels)
Files:
tests/hooks/hooks.test.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}: Never mutate existing objects; always create new objects with changes applied (Immutability requirement)
Handle errors at every level; provide user-friendly messages in UI code and detailed context in server-side logs
Ensure error messages don't leak sensitive data
Files:
tests/hooks/hooks.test.js
**/*.{jsx,tsx,html,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Sanitize HTML output to prevent XSS vulnerabilities
Files:
tests/hooks/hooks.test.js
🪛 ast-grep (0.44.0)
tests/hooks/hooks.test.js
[warning] 3155-3155: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(__dirname, '..', '..', 'skills', 'continuous-learning-v2', 'agents', 'start-observer.sh'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔇 Additional comments (1)
skills/continuous-learning-v2/agents/start-observer.sh (1)
218-219: LGTM!
|
| Filename | Overview |
|---|---|
| skills/continuous-learning-v2/agents/start-observer.sh | Replaces hardcoded sleep 2 with a 50-iteration, 0.2 s-interval poll for $PID_FILE. The bash syntax, set -e interaction, and failure-path behavior are all correct. Comment documents the failure-case latency trade-off. |
| tests/hooks/hooks.test.js | Adds a four-assertion regression test guarding against reversion to sleep 2. The negative assertion is robust; the three positive assertions tie tests to one specific incantation of the loop, which the existing review thread has already flagged. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant S as start-observer.sh
participant N as nohup (observer-loop.sh)
participant FS as Filesystem ($PID_FILE)
S->>N: "spawn via nohup &"
loop Poll up to 50 × 0.2 s
S->>FS: [ -f "$PID_FILE" ]?
alt File exists
FS-->>S: true → break
else File absent
S->>S: sleep 0.2
end
end
S->>FS: final [ -f "$PID_FILE" ] check (line 233)
alt PID file present
S->>S: kill -0 $pid → echo Observer started
else PID file absent
S->>S: "echo Failed to start observer && exit 1"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant S as start-observer.sh
participant N as nohup (observer-loop.sh)
participant FS as Filesystem ($PID_FILE)
S->>N: "spawn via nohup &"
loop Poll up to 50 × 0.2 s
S->>FS: [ -f "$PID_FILE" ]?
alt File exists
FS-->>S: true → break
else File absent
S->>S: sleep 0.2
end
end
S->>FS: final [ -f "$PID_FILE" ] check (line 233)
alt PID file present
S->>S: kill -0 $pid → echo Observer started
else PID file absent
S->>S: "echo Failed to start observer && exit 1"
end
Reviews (2): Last reviewed commit: "fix(observer): loosen poll-regression as..." | Re-trigger Greptile
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…-path latency (affaan-m#2356 review) Addresses CodeRabbit + Greptile feedback on PR affaan-m#2356: - tests/hooks/hooks.test.js: split the over-specific positive assertion (which pinned the exact `for _i in $(seq 1 50); … sleep 0.2; done` line) into three intent-based assertions — bounded iteration count, early-exit on $PID_FILE, sub-second interval. Valid refactors (rename loop var, switch to `while`, retune to 100 × 0.1s) no longer false-fail while the `sleep 2` regression remains guarded. - start-observer.sh: extend the inline comment to record the trade-off Greptile flagged — a loop that crashes before writing $PID_FILE is now detected in ~10s instead of ~2s. Healthy startups still return in iteration 1. Tests: node tests/run-all.js (Node v22.18.0) → 2892 passed / 0 failed.
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
DECISION: APPROVE
This PR improves the reliability of the observer startup process by replacing a fixed sleep 2 with a PID file polling mechanism. The polling approach accounts for varying filesystem responsiveness and provides early detection of startup failures. The associated test confirms the absence of the hardcoded sleep and the presence of polling logic, adhering to the repo's guidelines for hooks. Clean implementation and well-reasoned trade-offs make this a solid improvement.
2 participants
Summary
start-observer.shwaited for the spawned observer loop to write$PID_FILEwith a hardcodedsleep 2. On slow filesystems / loaded hosts that's not always enough — the subsequent[ -f "$PID_FILE" ]check false-negatives and the script prints "Failed to start observer" even though the loop is alive. On healthy systems the 2 s is pure latency.for _i in $(seq 1 50); do [ -f "$PID_FILE" ] && break; sleep 0.2; done(10 s max wait, typically returns in iteration 1).set -einteraction is safe — LHS of&&is exempt.tests/hooks/hooks.test.js, sibling to the existingobserver-loopinvariant-guard block, asserting the script never reverts tosleep 2and keeps the poll line.Closes #2295
Test plan
node tests/run-all.json Node v22.18.0 → 2892 passed / 0 failed (including the new regression test)shellcheck skills/continuous-learning-v2/agents/start-observer.sh→ clean on the changed lines (only a pre-existing SC1091 info at line 31, unrelated)start-observer.sh start/stop/statuscycle on macOS — PID file consistently observed in iteration 1