fix(continuous-learning-v2): warn when the observer never survives a hook invocation (#2489) - #2606
Conversation
…hook invocation (affaan-m#2489) The observer is lazy-started from a hook process that exits immediately afterwards. start-observer.sh's liveness check runs inside that still-living process tree, so it always sees a healthy observer and prints "Observer started (PID: N)". On native Windows (Git Bash/MSYS2) the reap happens later, when the hook's Job Object closes, so no self-check placed in start-observer.sh can ever observe the failure. The next hook invocation is the only place the death is visible, and _CHECK_OBSERVER_RUNNING already found it there -- then discarded it, deleting the stale PID file and restarting silently, once per tool call, forever. Users were left with an observer-start.log full of success lines and an observer that never completed a single analysis cycle. Record the "well-formed PID that is no longer alive" case, count consecutive non-survivals in ${PROJECT_DIR}/.observer-nosurvive-count, and log one explanatory warning when the streak reaches ECC_OBSERVER_NOSURVIVE_WARN_AFTER (default 3). Warning fires on equality so a persistent failure logs once per streak rather than once per tool call; finding the observer alive resets the streak. The Windows-specific explanation is gated on uname so Linux/macOS users are pointed at observer.log instead of a wrong diagnosis. Counting happens in the caller, not inside _CHECK_OBSERVER_RUNNING, because that function is invoked once per PID file and again under the start lock. The PowerShell backgrounding rewrite is deliberately not included: it cannot be exercised on a non-Windows machine, and untested process-spawning code is a worse outcome than an accurate diagnostic.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesObserver non-survival tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Heads-up on the red checks: they are inherited from Both failures are in
The new test behaves as intended across the matrix — static cases pass everywhere, and the bash-driven cases skip on |
… new warn threshold The observer's Windows limitation was only discoverable by hitting it. Record it next to observer.enabled, where it is read before the flag is set, and document ECC_OBSERVER_NOSURVIVE_WARN_AFTER so the knob added alongside the warning does not repeat the undocumented-env-var problem tracked in affaan-m#2573. zh-TW is intentionally left alone: translation parity is not enforced here and the repo rejects blind translation imports without translator review.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
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 `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 413-442: Protect the streak counter read-modify-write in
_NOTE_OBSERVER_NOSURVIVE with the existing $LAZY_START_LOCK mechanism, using
flock when available and the same graceful fallback pattern as
_START_OBSERVER_LOGGED. Keep the increment, persistence, exact-threshold warning
check, and warning block within the lock, then release file descriptor 8
afterward; preserve behavior on systems without flock.
In `@tests/hooks/observe-nosurvive-warning.test.js`:
- Around line 73-74: Update the prerequisite handling in the test setup around
hasPython and the affected asyncTest calls in
tests/hooks/observe-nosurvive-warning.test.js so a missing python3 on
non-Windows platforms fails the test suite explicitly instead of skipping or
exiting successfully. Preserve the existing Windows behavior and ensure all
three shell-execution tests use the failing prerequisite path.
🪄 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 Plus
Run ID: 8dc6461e-a9c0-4ab1-b676-2f705340a281
📒 Files selected for processing (2)
skills/continuous-learning-v2/hooks/observe.shtests/hooks/observe-nosurvive-warning.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Test (macos-latest, Node 20.x, bun)
- GitHub Check: Test (macos-latest, Node 20.x, yarn)
- GitHub Check: Test (macos-latest, Node 18.x, npm)
- GitHub Check: Test (macos-latest, Node 22.x, pnpm)
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use specialized agents proactively for planning, implementation review, testing, security review, build resolution, and domain-specific tasks; run independent operations in parallel.
Write tests before implementation, follow the RED-GREEN-IMPROVE TDD workflow, and maintain at least 80% coverage.
Never compromise security: validate all inputs, prevent injection and XSS, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive error details.
Never hardcode secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and search for similar vulnerabilities.
Always create new objects and never mutate existing ones.
Organize code by feature or domain with high cohesion and low coupling; prefer many small files over a few large files, typically 200–400 lines and no more than 800 lines.
Handle errors at every level, show user-friendly messages in UI code, log detailed context server-side, and never silently swallow errors.
Validate all user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, avoid hardcoded values, and use readable, well-named identifiers.
All required tests include unit tests for functions, utilities, and components; integration tests for APIs and databases; and E2E tests for critical user flows.
Troubleshoot test failures by checking isolation, verifying mocks, and fixing implementation rather than tests unless the tests are incorrect.
Before committing, use Conventional Commits format:<type>: <description>, with types such as feat, fix, refactor, docs, test, chore, perf, and ci.
For pull requests, analyze the full commit history, draf...
Files:
tests/hooks/observe-nosurvive-warning.test.jsskills/continuous-learning-v2/hooks/observe.sh
**/*.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/hooks/observe.sh
skills/**
📄 CodeRabbit inference engine (AGENTS.md)
Treat
skills/as the canonical workflow surface; add new workflow contributions there first.
Files:
skills/continuous-learning-v2/hooks/observe.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/hooks/observe.sh
🧠 Learnings (2)
📚 Learning: 2026-06-27T23:49:19.839Z
Learnt from: gaurav0107
Repo: affaan-m/ECC PR: 2373
File: tests/hooks/observe-signal-timeout.test.js:0-0
Timestamp: 2026-06-27T23:49:19.839Z
Learning: In tests under tests/hooks that require a Python runtime to run, the test should fail fast when Python isn’t available (or prerequisites aren’t met). Do not treat a missing Python runtime as test.skip, as an expected/allowed condition, or as a passing state; instead, explicitly fail (e.g., throw/return a rejected promise or use a test runner fail/expect that marks the test as failed) so reviewers can’t accidentally mask environment issues.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
📚 Learning: 2026-07-14T03:26:12.530Z
Learnt from: thejesh23
Repo: affaan-m/ECC PR: 2517
File: tests/hooks/pre-bash-tmux-reminder.test.js:21-25
Timestamp: 2026-07-14T03:26:12.530Z
Learning: In this repository, do not flag `console.log` usage as a guideline violation in hook test files under `tests/hooks/*.test.js`. These tests intentionally use `console.log` for pass/fail output because the repo’s console-based runner (`tests/run-all.js`) is used and there is no Jest/Mocha dependency. Outside this specific hook-test path, follow the normal logging guidelines.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
🪛 ast-grep (0.44.1)
tests/hooks/observe-nosurvive-warning.test.js
[warning] 93-104: 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.writeFileSync(
path.join(scriptsDir, 'detect-project.sh'),
[
'#!/bin/bash',
'PROJECT_ID="test-project"',
'PROJECT_NAME="test-project"',
PROJECT_ROOT="${projectDir}",
PROJECT_DIR="${projectDir}",
'CLV2_PYTHON_CMD="python3"',
''
].join('\n')
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 110-113: 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.writeFileSync(
path.join(configDir, 'config.json'),
JSON.stringify({ observer: { enabled: true } })
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 114-121: 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.writeFileSync(
path.join(scriptsLibDir, 'homunculus-dir.sh'),
[
'#!/bin/bash',
'_clv2_resolve_homunculus_dir() { printf "%s\n" "$HOME/.local/share/ecc-homunculus"; }',
''
].join('\n')
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 123-123: 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(observeShPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 133-133: 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.writeFileSync(testObserve, observeContent, { mode: 0o755 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 197-197: 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(streakFile, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 202-202: 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(logFile, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 208-208: 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(observeShPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 220-220: 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(observeShPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 236-236: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 259-259: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 269-269: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 287-287: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 292-292: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${process.pid}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 28-28: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require('child_process')
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
🔇 Additional comments (2)
tests/hooks/observe-nosurvive-warning.test.js (1)
25-322: Static analysis path-traversal andchild_processhints on this file are false positives — all paths originate fromfs.mkdtempSync/path.joinon sandbox-controlled temp dirs, not external input, andspawn/spawnSyncare called with fixed argument arrays.skills/continuous-learning-v2/hooks/observe.sh (1)
381-404: LGTM!Also applies to: 444-447, 476-490
…the lazy-start lock observe.sh runs on every tool call, so the streak read-modify-write could race between concurrent invocations -- losing an increment or logging the warning twice. That is the same class of bug the signal counter hit in affaan-m#2296, and this repo's rule is to never fall back to an unlocked read-modify-write. Rather than add a second lock, move the increment into _START_OBSERVER_LOGGED. All three of its call sites already run inside the lazy-start lock (flock / lockfile / mkdir), so the update is serialized with no new machinery. Counting at the restart instead of at detection also means N racing hooks record one death rather than N. The reset stays in the caller: it is an idempotent unlink, not a read-modify-write, so it needs no lock. Adds a regression case pinning the increment inside _START_OBSERVER_LOGGED and asserting all three call sites remain locked.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/hooks/observe-nosurvive-warning.test.js (1)
303-322: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid using
process.pidas an unconditional live-PID fixture.In containers where Node runs as PID 1,
_CHECK_OBSERVER_RUNNINGintentionally rejects this value, so the test will not reset the streak. Use a verified live PID greater than 1, such as a short-lived child process, and clean it up afterward.🤖 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 `@tests/hooks/observe-nosurvive-warning.test.js` around lines 303 - 322, The runResetWhenAlive test uses process.pid, which can be rejected when Node runs as PID 1. Replace that fixture with a verified live PID greater than 1, such as a short-lived child process, write it to .observer.pid before the second runObserve call, and ensure the child is cleaned up in the existing finally path.skills/continuous-learning-v2/hooks/observe.sh (2)
428-428: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject zero-valued threshold strings.
Line 428 rejects only literal
0; values such as00or000pass validation and compare as zero, so the positive streak can never equal the configured threshold. Normalize or reject all-zero values before comparing the threshold.🤖 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 `@skills/continuous-learning-v2/hooks/observe.sh` at line 428, Update the warn_after validation in the case statement so strings composed entirely of zeros, including 00 and 000, are normalized to the default or rejected before threshold comparisons. Preserve acceptance of valid positive numeric thresholds and the existing default behavior for invalid values.Source: Coding guidelines
426-430: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not silently discard streak and log persistence errors.
Lines 426–430 turn read/write failures into a normal success path, and Line 447 suppresses warning-log failures. A read error can reset the in-memory streak, while an unwritable counter or log silently disables this diagnostic. Surface a clear fallback/error and preserve state when reads fail.
Also applies to: 447-447
🤖 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 `@skills/continuous-learning-v2/hooks/observe.sh` around lines 426 - 430, Update the streak persistence flow around streak_file to distinguish missing or invalid data from read failures, preserving the existing streak when reads cannot be completed and surfacing a clear fallback/error instead of defaulting silently. Handle write failures from printf and warning-log failures near the warning output similarly, reporting them explicitly rather than suppressing them with “|| true”.Source: Coding guidelines
🤖 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 `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 492-498: Update the liveness handling around
_RESET_OBSERVER_NOSURVIVE_STREAK and _NOTE_OBSERVER_NOSURVIVE so the alive/dead
decision and streak mutation are serialized under the same lazy-start lock. Move
the reset into the lock-protected flow used by _START_OBSERVER_LOGGED,
preserving idempotent reset behavior while preventing a concurrent reset from
erasing a newly recorded increment.
In `@tests/hooks/observe-nosurvive-warning.test.js`:
- Around line 220-236: Strengthen the test around `_START_OBSERVER_LOGGED` so it
validates lock context for every call site, not just that exactly three exist.
Inspect the surrounding `observe.sh` control flow and assert each invocation is
within the established `flock`, `lockfile`, or `mkdir` lazy-start lock branch,
while preserving the existing count and `_NOTE_OBSERVER_NOSURVIVE` checks.
---
Outside diff comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Line 428: Update the warn_after validation in the case statement so strings
composed entirely of zeros, including 00 and 000, are normalized to the default
or rejected before threshold comparisons. Preserve acceptance of valid positive
numeric thresholds and the existing default behavior for invalid values.
- Around line 426-430: Update the streak persistence flow around streak_file to
distinguish missing or invalid data from read failures, preserving the existing
streak when reads cannot be completed and surfacing a clear fallback/error
instead of defaulting silently. Handle write failures from printf and
warning-log failures near the warning output similarly, reporting them
explicitly rather than suppressing them with “|| true”.
In `@tests/hooks/observe-nosurvive-warning.test.js`:
- Around line 303-322: The runResetWhenAlive test uses process.pid, which can be
rejected when Node runs as PID 1. Replace that fixture with a verified live PID
greater than 1, such as a short-lived child process, write it to .observer.pid
before the second runObserve call, and ensure the child is cleaned up in the
existing finally path.
🪄 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 Plus
Run ID: 7cee5aeb-8ca3-4be7-9f2a-ec207e7c4cea
📒 Files selected for processing (2)
skills/continuous-learning-v2/hooks/observe.shtests/hooks/observe-nosurvive-warning.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (23)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use specialized agents proactively for planning, implementation review, testing, security review, build resolution, and domain-specific tasks; run independent operations in parallel.
Write tests before implementation, follow the RED-GREEN-IMPROVE TDD workflow, and maintain at least 80% coverage.
Never compromise security: validate all inputs, prevent injection and XSS, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive error details.
Never hardcode secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and search for similar vulnerabilities.
Always create new objects and never mutate existing ones.
Organize code by feature or domain with high cohesion and low coupling; prefer many small files over a few large files, typically 200–400 lines and no more than 800 lines.
Handle errors at every level, show user-friendly messages in UI code, log detailed context server-side, and never silently swallow errors.
Validate all user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, avoid hardcoded values, and use readable, well-named identifiers.
All required tests include unit tests for functions, utilities, and components; integration tests for APIs and databases; and E2E tests for critical user flows.
Troubleshoot test failures by checking isolation, verifying mocks, and fixing implementation rather than tests unless the tests are incorrect.
Before committing, use Conventional Commits format:<type>: <description>, with types such as feat, fix, refactor, docs, test, chore, perf, and ci.
For pull requests, analyze the full commit history, draf...
Files:
tests/hooks/observe-nosurvive-warning.test.jsskills/continuous-learning-v2/hooks/observe.sh
**/*.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/hooks/observe.sh
skills/**
📄 CodeRabbit inference engine (AGENTS.md)
Treat
skills/as the canonical workflow surface; add new workflow contributions there first.
Files:
skills/continuous-learning-v2/hooks/observe.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/hooks/observe.sh
🧠 Learnings (2)
📚 Learning: 2026-06-27T23:49:19.839Z
Learnt from: gaurav0107
Repo: affaan-m/ECC PR: 2373
File: tests/hooks/observe-signal-timeout.test.js:0-0
Timestamp: 2026-06-27T23:49:19.839Z
Learning: In tests under tests/hooks that require a Python runtime to run, the test should fail fast when Python isn’t available (or prerequisites aren’t met). Do not treat a missing Python runtime as test.skip, as an expected/allowed condition, or as a passing state; instead, explicitly fail (e.g., throw/return a rejected promise or use a test runner fail/expect that marks the test as failed) so reviewers can’t accidentally mask environment issues.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
📚 Learning: 2026-07-14T03:26:12.530Z
Learnt from: thejesh23
Repo: affaan-m/ECC PR: 2517
File: tests/hooks/pre-bash-tmux-reminder.test.js:21-25
Timestamp: 2026-07-14T03:26:12.530Z
Learning: In this repository, do not flag `console.log` usage as a guideline violation in hook test files under `tests/hooks/*.test.js`. These tests intentionally use `console.log` for pass/fail output because the repo’s console-based runner (`tests/run-all.js`) is used and there is no Jest/Mocha dependency. Outside this specific hook-test path, follow the normal logging guidelines.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
🪛 ast-grep (0.44.1)
tests/hooks/observe-nosurvive-warning.test.js
[warning] 220-220: 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(observeShPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔇 Additional comments (4)
tests/hooks/observe-nosurvive-warning.test.js (3)
324-331: Missing Python still reports the test suite as successful.On non-Windows hosts,
hasPython === falseskips all behavioral tests whilefailedremains zero. This repeats the prior prerequisite-handling finding; fail explicitly or apply the policy consistently across the sibling hook tests.Source: Learnings
208-218: LGTM!
252-271: LGTM!skills/continuous-learning-v2/hooks/observe.sh (1)
378-385: LGTM!Also applies to: 404-411
| test('the streak increment runs under the lazy-start lock, never unlocked', () => { | ||
| const content = fs.readFileSync(observeShPath, 'utf8'); | ||
| // observe.sh fires on every tool call, so an unlocked read-modify-write on | ||
| // the streak file would lose increments or double-log the warning -- the same | ||
| // race the signal counter hit in #2296. The increment must therefore live in | ||
| // _START_OBSERVER_LOGGED, which every call site invokes inside the | ||
| // flock/lockfile/mkdir lazy-start lock. | ||
| const starter = content.match(/_START_OBSERVER_LOGGED\(\)\s*\{[\s\S]*?\n\}/); | ||
| assert.ok(starter, 'observe.sh should still define _START_OBSERVER_LOGGED'); | ||
| assert.ok( | ||
| starter[0].includes('_NOTE_OBSERVER_NOSURVIVE'), | ||
| 'the streak increment should run inside _START_OBSERVER_LOGGED, under the lazy-start lock' | ||
| ); | ||
| // Every _START_OBSERVER_LOGGED call site must be inside a lock branch. | ||
| const callSites = content.split('\n').filter((line) => /^\s+_START_OBSERVER_LOGGED\s*$/.test(line)); | ||
| assert.strictEqual(callSites.length, 3, 'expected the three locked lazy-start call sites'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify lock context, not only call-site count.
The test asserts exactly three _START_OBSERVER_LOGGED call sites but never checks that each is inside a flock/lockfile/mkdir lock branch. An unlocked call site could be introduced without changing the count, while the race regression still passes.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 220-220: 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(observeShPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 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 `@tests/hooks/observe-nosurvive-warning.test.js` around lines 220 - 236,
Strengthen the test around `_START_OBSERVER_LOGGED` so it validates lock context
for every call site, not just that exactly three exist. Inspect the surrounding
`observe.sh` control flow and assert each invocation is within the established
`flock`, `lockfile`, or `mkdir` lazy-start lock branch, while preserving the
existing count and `_NOTE_OBSERVER_NOSURVIVE` checks.
…rning output Three review findings on the affaan-m#2489 diagnostic: - An all-zero threshold silently disabled it. `00` passes a digits-only check but compares as zero, and the streak only grows, so the warning could never fire. Normalize with base-10 arithmetic and fall back to the default for anything below 1. Base 10 is forced explicitly because a leading zero would otherwise be read as octal, and `08` is an arithmetic error that would abort the hook under `set -e`. The same normalization now guards the streak read. - An unwritable log silently swallowed the diagnostic. Build the message once and fall back to stderr when the append fails. This cannot spam: the block runs once per streak, not once per tool call. The counter write keeps its `|| true` -- observe.sh runs on every tool call and the repo rule is that hooks exit 0 on non-critical errors, so a full disk must not break tool use. - The live-PID test fixture used process.pid, which is 1 in a container and is deliberately rejected by _CHECK_OBSERVER_RUNNING; the reset case would then fail for the wrong reason. Use a spawned child and clean it up. Adds a regression case for the all-zero threshold. Verified on bash 3.2 (the macOS CI runner shell) as well as bash 5.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Round 2 addressed in 05fe037 — thanks, two of these were real bugs. Zero-valued threshold strings — fixed.
Persistence errors — partially applied. The half that mattered is fixed: an unwritable I kept Also verified the new shell constructs on bash 3.2 (the macOS runner shell), not just bash 5. |
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 `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 440-466: The warning block after persisting streak state must run
only when the write to streak_file succeeds. Update the printf operation before
the equality check so its success status gates the existing warn_after logic,
while keeping write failures non-fatal and avoiding repeated warnings when
persistence fails.
In `@tests/hooks/observe-nosurvive-warning.test.js`:
- Around line 301-324: Add an “08” threshold scenario alongside
runRejectsZeroThreshold that invokes runObserve with
ECC_OBSERVER_NOSURVIVE_WARN_AFTER set to "08", asserts the call succeeds, and
verifies the recorded streak is 1. Keep the existing "00" fallback assertions
unchanged.
🪄 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 Plus
Run ID: a8a3d63b-3ba2-4e82-927b-cd2b38180c80
📒 Files selected for processing (2)
skills/continuous-learning-v2/hooks/observe.shtests/hooks/observe-nosurvive-warning.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (27)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (macos-latest, Node 20.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (macos-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 20.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Test (macos-latest, Node 20.x, yarn)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use specialized agents proactively for planning, implementation review, testing, security review, build resolution, and domain-specific tasks; run independent operations in parallel.
Write tests before implementation, follow the RED-GREEN-IMPROVE TDD workflow, and maintain at least 80% coverage.
Never compromise security: validate all inputs, prevent injection and XSS, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive error details.
Never hardcode secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and search for similar vulnerabilities.
Always create new objects and never mutate existing ones.
Organize code by feature or domain with high cohesion and low coupling; prefer many small files over a few large files, typically 200–400 lines and no more than 800 lines.
Handle errors at every level, show user-friendly messages in UI code, log detailed context server-side, and never silently swallow errors.
Validate all user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, avoid hardcoded values, and use readable, well-named identifiers.
All required tests include unit tests for functions, utilities, and components; integration tests for APIs and databases; and E2E tests for critical user flows.
Troubleshoot test failures by checking isolation, verifying mocks, and fixing implementation rather than tests unless the tests are incorrect.
Before committing, use Conventional Commits format:<type>: <description>, with types such as feat, fix, refactor, docs, test, chore, perf, and ci.
For pull requests, analyze the full commit history, draf...
Files:
tests/hooks/observe-nosurvive-warning.test.jsskills/continuous-learning-v2/hooks/observe.sh
**/*.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/hooks/observe.sh
skills/**
📄 CodeRabbit inference engine (AGENTS.md)
Treat
skills/as the canonical workflow surface; add new workflow contributions there first.
Files:
skills/continuous-learning-v2/hooks/observe.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/hooks/observe.sh
🧠 Learnings (2)
📚 Learning: 2026-06-27T23:49:19.839Z
Learnt from: gaurav0107
Repo: affaan-m/ECC PR: 2373
File: tests/hooks/observe-signal-timeout.test.js:0-0
Timestamp: 2026-06-27T23:49:19.839Z
Learning: In tests under tests/hooks that require a Python runtime to run, the test should fail fast when Python isn’t available (or prerequisites aren’t met). Do not treat a missing Python runtime as test.skip, as an expected/allowed condition, or as a passing state; instead, explicitly fail (e.g., throw/return a rejected promise or use a test runner fail/expect that marks the test as failed) so reviewers can’t accidentally mask environment issues.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
📚 Learning: 2026-07-14T03:26:12.530Z
Learnt from: thejesh23
Repo: affaan-m/ECC PR: 2517
File: tests/hooks/pre-bash-tmux-reminder.test.js:21-25
Timestamp: 2026-07-14T03:26:12.530Z
Learning: In this repository, do not flag `console.log` usage as a guideline violation in hook test files under `tests/hooks/*.test.js`. These tests intentionally use `console.log` for pass/fail output because the repo’s console-based runner (`tests/run-all.js`) is used and there is no Jest/Mocha dependency. Outside this specific hook-test path, follow the normal logging guidelines.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
🪛 ast-grep (0.44.1)
tests/hooks/observe-nosurvive-warning.test.js
[warning] 307-307: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 331-331: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 340-340: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${live.pid}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔇 Additional comments (3)
skills/continuous-learning-v2/hooks/observe.sh (2)
472-474: Serialize reset with the counter update.The previously reported alive/dead interleaving remains: an unlocked reset can erase a just-persisted increment, or a delayed increment can overwrite a later live-observer reset.
Also applies to: 512-518
378-385: LGTM!Also applies to: 404-409, 427-435
tests/hooks/observe-nosurvive-warning.test.js (1)
326-361: LGTM!
If the counter write fails, the file stays below the threshold, so every later
hook invocation rereads it, re-increments in memory, hits the equality check
and warns again -- turning the once-per-streak diagnostic into once-per-tool-
call spam. That is worse in exactly the case the stderr fallback added in the
previous commit was meant to cover, since a disk that cannot take the log
usually cannot take the counter either.
Gate the warning on the write succeeding. The write stays non-fatal: it runs
as an `if` condition, so `set -e` is satisfied and an unwritable counter costs
a delayed diagnostic rather than a broken tool call.
Tests: an unwritable counter must stay silent across repeated invocations while
the hook still exits 0, and a leading-zero threshold ("08") must be read as
decimal -- "00" alone did not exercise the base-10 conversion, since it is zero
either way.
|
Round 3 in 12ea937 — both correct, and the first one was a regression I introduced in the previous commit. Only warn after the streak is persisted — good catch. With a stuck counter file the value stays at Non-octal leading zero — also right, Both are pinned by new tests (9 cases total, still 0/9 against pre-fix Full suite 3360/3361 — the one failure is the pre-existing |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
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/observe-nosurvive-warning.test.js`:
- Around line 329-340: Update runLeadingZeroThreshold to seed the streak at 7
before invoking runObserve with ECC_OBSERVER_NOSURVIVE_WARN_AFTER set to "08",
then assert the warning is emitted and reports threshold 8. Keep the test
focused on proving that the configured leading-zero value is parsed as decimal
eight rather than falling back to the default threshold.
🪄 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 Plus
Run ID: c18b50cc-5aff-44aa-802f-e0985a3b01f2
📒 Files selected for processing (2)
skills/continuous-learning-v2/hooks/observe.shtests/hooks/observe-nosurvive-warning.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (27)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (macos-latest, Node 20.x, bun)
- GitHub Check: Test (macos-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (macos-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 22.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (17)
**/*.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/hooks/observe.sh
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Use specialized agents proactively for planning, implementation review, testing, security review, build resolution, and domain-specific tasks; run independent operations in parallel.
Write tests before implementation, follow the RED-GREEN-IMPROVE TDD workflow, and maintain at least 80% coverage.
Never compromise security: validate all inputs, prevent injection and XSS, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive error details.
Never hardcode secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and search for similar vulnerabilities.
Always create new objects and never mutate existing ones.
Organize code by feature or domain with high cohesion and low coupling; prefer many small files over a few large files, typically 200–400 lines and no more than 800 lines.
Handle errors at every level, show user-friendly messages in UI code, log detailed context server-side, and never silently swallow errors.
Validate all user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, avoid hardcoded values, and use readable, well-named identifiers.
All required tests include unit tests for functions, utilities, and components; integration tests for APIs and databases; and E2E tests for critical user flows.
Troubleshoot test failures by checking isolation, verifying mocks, and fixing implementation rather than tests unless the tests are incorrect.
Before committing, use Conventional Commits format:<type>: <description>, with types such as feat, fix, refactor, docs, test, chore, perf, and ci.
For pull requests, analyze the full commit history, draf...
Files:
skills/continuous-learning-v2/hooks/observe.shtests/hooks/observe-nosurvive-warning.test.js
skills/**
📄 CodeRabbit inference engine (AGENTS.md)
Treat
skills/as the canonical workflow surface; add new workflow contributions there first.
Files:
skills/continuous-learning-v2/hooks/observe.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/hooks/observe.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.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/observe-nosurvive-warning.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/observe-nosurvive-warning.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/observe-nosurvive-warning.test.js
🧠 Learnings (2)
📚 Learning: 2026-06-27T23:49:19.839Z
Learnt from: gaurav0107
Repo: affaan-m/ECC PR: 2373
File: tests/hooks/observe-signal-timeout.test.js:0-0
Timestamp: 2026-06-27T23:49:19.839Z
Learning: In tests under tests/hooks that require a Python runtime to run, the test should fail fast when Python isn’t available (or prerequisites aren’t met). Do not treat a missing Python runtime as test.skip, as an expected/allowed condition, or as a passing state; instead, explicitly fail (e.g., throw/return a rejected promise or use a test runner fail/expect that marks the test as failed) so reviewers can’t accidentally mask environment issues.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
📚 Learning: 2026-07-14T03:26:12.530Z
Learnt from: thejesh23
Repo: affaan-m/ECC PR: 2517
File: tests/hooks/pre-bash-tmux-reminder.test.js:21-25
Timestamp: 2026-07-14T03:26:12.530Z
Learning: In this repository, do not flag `console.log` usage as a guideline violation in hook test files under `tests/hooks/*.test.js`. These tests intentionally use `console.log` for pass/fail output because the repo’s console-based runner (`tests/run-all.js`) is used and there is no Jest/Mocha dependency. Outside this specific hook-test path, follow the normal logging guidelines.
Applied to files:
tests/hooks/observe-nosurvive-warning.test.js
🪛 ast-grep (0.44.1)
tests/hooks/observe-nosurvive-warning.test.js
[warning] 331-331: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 356-356: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔇 Additional comments (2)
skills/continuous-learning-v2/hooks/observe.sh (1)
437-470: LGTM!tests/hooks/observe-nosurvive-warning.test.js (1)
346-370: LGTM!Also applies to: 408-409
| async function runLeadingZeroThreshold() { | ||
| const { testDir, projectDir, testObserve } = buildSandbox(); | ||
| try { | ||
| fs.writeFileSync(path.join(projectDir, '.observer.pid'), `${deadPid()}\n`); | ||
| // runObserve rejects on a non-zero exit, so an octal abort fails here. | ||
| await runObserve(testObserve, projectDir, { ECC_OBSERVER_NOSURVIVE_WARN_AFTER: '08' }); | ||
|
|
||
| assert.strictEqual(readStreak(projectDir), 1, 'streak should advance under an "08" threshold'); | ||
| assert.ok( | ||
| !readStartLog(projectDir).includes(WARN_MARKER), | ||
| '"08" should be read as decimal 8, so a streak of 1 must not warn yet' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the 08 case prove the threshold is eight.
One invocation only proves it does not abort; falling back to default 3 passes these assertions too. Seed the streak at 7, then require the warning and reported threshold 8.
Proposed fix
async function runLeadingZeroThreshold() {
const { testDir, projectDir, testObserve } = buildSandbox();
try {
+ fs.writeFileSync(path.join(projectDir, STREAK_FILE), '7\n');
fs.writeFileSync(path.join(projectDir, '.observer.pid'), `${deadPid()}\n`);
- // runObserve rejects on a non-zero exit, so an octal abort fails here.
await runObserve(testObserve, projectDir, { ECC_OBSERVER_NOSURVIVE_WARN_AFTER: '08' });
- assert.strictEqual(readStreak(projectDir), 1, 'streak should advance under an "08" threshold');
+ assert.strictEqual(readStreak(projectDir), 8, 'streak should reach the decimal threshold');
assert.ok(
- !readStartLog(projectDir).includes(WARN_MARKER),
- '"08" should be read as decimal 8, so a streak of 1 must not warn yet'
+ readStartLog(projectDir).includes(WARN_MARKER),
+ '"08" should trigger once the streak reaches decimal 8'
);
+ assert.ok(readStartLog(projectDir).includes('currently 8'));
} finally {As per coding guidelines, required tests must cover configured behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function runLeadingZeroThreshold() { | |
| const { testDir, projectDir, testObserve } = buildSandbox(); | |
| try { | |
| fs.writeFileSync(path.join(projectDir, '.observer.pid'), `${deadPid()}\n`); | |
| // runObserve rejects on a non-zero exit, so an octal abort fails here. | |
| await runObserve(testObserve, projectDir, { ECC_OBSERVER_NOSURVIVE_WARN_AFTER: '08' }); | |
| assert.strictEqual(readStreak(projectDir), 1, 'streak should advance under an "08" threshold'); | |
| assert.ok( | |
| !readStartLog(projectDir).includes(WARN_MARKER), | |
| '"08" should be read as decimal 8, so a streak of 1 must not warn yet' | |
| ); | |
| async function runLeadingZeroThreshold() { | |
| const { testDir, projectDir, testObserve } = buildSandbox(); | |
| try { | |
| fs.writeFileSync(path.join(projectDir, STREAK_FILE), '7\n'); | |
| fs.writeFileSync(path.join(projectDir, '.observer.pid'), `${deadPid()}\n`); | |
| await runObserve(testObserve, projectDir, { ECC_OBSERVER_NOSURVIVE_WARN_AFTER: '08' }); | |
| assert.strictEqual(readStreak(projectDir), 8, 'streak should reach the decimal threshold'); | |
| assert.ok( | |
| readStartLog(projectDir).includes(WARN_MARKER), | |
| '"08" should trigger once the streak reaches decimal 8' | |
| ); | |
| assert.ok(readStartLog(projectDir).includes('currently 8')); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 331-331: 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.writeFileSync(path.join(projectDir, '.observer.pid'), ${deadPid()}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 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 `@tests/hooks/observe-nosurvive-warning.test.js` around lines 329 - 340, Update
runLeadingZeroThreshold to seed the streak at 7 before invoking runObserve with
ECC_OBSERVER_NOSURVIVE_WARN_AFTER set to "08", then assert the warning is
emitted and reports threshold 8. Keep the test focused on proving that the
configured leading-zero value is parsed as decimal eight rather than falling
back to the default threshold.
Source: Coding guidelines
daltino
left a comment
There was a problem hiding this comment.
This PR resolves a critical issue (#2489) where the observer fails silently on unsupported platforms like native Windows. The updates include clear documentation in SKILL.md warning users about platform limitations, robust code changes in observe.sh to count and log observer failures correctly, and comprehensive tests in observe-nosurvive-warning.test.js ensuring reliable behavior. The changes align well with the contribution guidelines and thoroughly address the problem.
|
Thank you for chasing this subtle observer lifecycle failure through the next hook invocation, and sorry it waited behind unrelated main-branch failures. The bounded, once-per-streak warning is the right user experience: it makes silent observer death diagnosable without turning every tool call into log spam. I merged current main into bf10629 and reran the behavior against today’s observer. The 9 non-survival tests, 4 counter-race tests, 5 timeout tests, 6 project-detection tests, 5 dispatch tests, and 7 entrypoint tests all pass. Bash syntax, skill validation, Markdown lint, focused ESLint, and diff checks are also clean. Hosted CI is now rerunning on the exact refreshed head; once green, this is ready for final approval and the serial merge gate. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
| # tree, so it always sees a healthy observer and reports success -- on native | ||
| # Windows the reap happens later, when the hook's Job Object closes. The next | ||
| # hook invocation is therefore the only place the death is observable, and | ||
| # before #2489 it silently deleted the stale PID and restarted, once per tool |
There was a problem hiding this comment.
_NOTE_OBSERVER_NOSURVIVE combines threshold parsing, persistence, platform detection, message construction, and logging across approximately 62 lines, exceeding the repository's 50-line function limit and making these behaviors harder to review and test independently.
File Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # needs no lock. The matching increment runs inside the lazy-start lock, in | ||
| # _START_OBSERVER_LOGGED. | ||
| if [ "$OBSERVER_ALIVE" = "true" ]; then | ||
| _RESET_OBSERVER_NOSURVIVE_STREAK |
There was a problem hiding this comment.
An invocation can observe a live observer and pause before this reset. If that observer then exits, another invocation detects the stale PID and persists a no-survival increment while holding the lazy-start lock. The first invocation subsequently unlinks the counter here, erasing the newer increment. Since the warning fires only when the persisted count equals the threshold, the next failure starts the count over and delays or suppresses the warning. Serialize the reset with the counter increment using the same lock, and revalidate liveness after acquiring it.
Artifacts
Deterministic observer no-survival reset race harness
- Runs the candidate helper definitions and schedules live observation, stale-PID increment, and reset in the failing order, proving the unlocked reset can delete the streak.
Baseline threshold warning execution output
- Runs two locked stale-PID increments from count one and shows count three with one equality warning, establishing the expected non-racing behavior.
Concurrent liveness reset race execution output
- Runs the deterministic liveness-straddle interleaving and shows B persisted count two before A removed it, after which the next death is only count one with no warning.
Twenty repeated observer reset race executions
- Repeats the deterministic race twenty times and records the same count deletion and warning suppression on every execution, confirming the failure is reproducible.
haelyra
left a comment
There was a problem hiding this comment.
The current-main reconciliation preserves the focused non-survival warning and passes the observer non-survival, counter-race, timeout, project-detection, dispatch, and entrypoint suites plus syntax, validation, lint, and diff checks locally. Approved, with merge still held for the hosted gate.
Summary
On native Windows (Git Bash / MSYS2) the continuous-learning-v2 observer is
reaped when the hook process exits and its Job Object closes, so it never runs
a single analysis cycle — but the user is told it started successfully, every
time. This makes the repeated failure visible instead of silent.
Closes #2489
Why the existing check could not catch it
agents/start-observer.shalready polls for the PID file and thenkill -0sit. That check runs inside the process tree that is about to be reaped, so at
the moment it runs the observer is genuinely alive and it correctly prints
Observer started (PID: N). The kill is triggered later, by the hook processexiting. No self-check placed in
start-observer.sh— including thesleep 2re-check suggested in the issue — can observe the failure.
The only process that can see it is the next hook invocation, and that path
already existed:
_CHECK_OBSERVER_RUNNINGinhooks/observe.shdetects thedead PID, deletes the PID file, and lazy-starts again — silently, once per tool
call, forever. Discarding that detection is the defect this PR fixes.
What changed
skills/continuous-learning-v2/hooks/observe.sh:_CHECK_OBSERVER_RUNNINGnow records that awell-formed-but-dead PID was cleared. Return values and signature are
unchanged; all eight call sites are unaffected.
${PROJECT_DIR}/.observer-nosurvive-count(alongside the existing.observer-signal-counter; both live under the homunculus dir, not the repo).ECC_OBSERVER_NOSURVIVE_WARN_AFTER(default 3), oneexplanatory block is appended to
observer-start.log— the file the issuereporter actually inspected. It fires on equality, not
>=, so a persistentfailure logs once per streak rather than once per tool call.
not inherit an old count and warn spuriously.
uname; Linux/macOS users arepointed at
observer.loginstead of being given a wrong diagnosis._CHECK_OBSERVER_RUNNING,which is invoked once per PID file and again under the start lock — counting
inside the function would double-count.
No new locking and no PID-based stale reclaim is introduced; this only
records a death the existing code already detected.
Scope note
This is planned fix 2 from the maintainer triage on #2489. Fix 1 (replacing
the background launch with a PowerShell
Start-Processdetach) is deliberatelynot included: it cannot be exercised on a non-Windows machine, and shipping
untested process-spawning code is a worse outcome than an accurate diagnostic.
Happy to follow up on it if someone can validate on a Windows host.
Validation
node tests/run-all.js→ 3356/3357. The single failure,tests/scripts/manual-hook-install-docs.test.js("README should call out thecorrect Windows Claude config root"), reproduces identically on an unmodified
maincheckout and is unrelated to this diff.tests/hooks/observe-nosurvive-warning.test.js(5 cases:2 static, 3 behavioural) drives the real
observe.shthrough the sandboxharness established by
observe-signal-counter-race.test.js. Verified itfails 0/5 against pre-fix
observe.shand passes 5/5 after — it is a realregression test, not a tautology.
tests/hooks/observer-memory.test.js31/31, including its"avoids persistence-looking signatures" guard (the first draft of the warning
text tripped its
nohupassertion; the wording was changed rather than theguard).
scripts/ci/validate-*.js,check-unicode-safety.js,validate-no-personal-paths.js,catalog.js --text,command-registry:check,eslint, andbash -npass.process.platform === 'win32'skip and apython3precondition per the convention in
observer-memory.test.js, and the runnerrejects on non-zero exit / signal / timeout with captured stderr.