Conversation
Three scripts under skills/continuous-learning-v2/ used the hardcoded `#!/bin/bash` shebang while the other four already used the portable `#!/usr/bin/env bash`: - hooks/observe.sh (runs on every hook invocation) - scripts/detect-project.sh - agents/start-observer.sh The hardcoded interpreter path fails to execute on systems where bash is not installed at /bin/bash (NixOS, some Homebrew layouts, FreeBSD). Standardize all three to `#!/usr/bin/env bash`, matching the repo-wide majority convention, and add a regression test that asserts shebang uniformity for every shell script in this skill so the inconsistency cannot reappear. Fixes affaan-m#2303
|
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 (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (22)
🧰 Additional context used📓 Path-based instructions (16)**/*.{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:
🧠 Learnings (1)📚 Learning: 2026-06-27T23:49:19.839ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThree shell scripts in Shebang portability fix and enforcement
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
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 `@tests/hooks/continuous-learning-shebang-consistency.test.js`:
- Around line 52-54: The firstLine helper in the shebang consistency test is not
normalizing CRLF, so its result can include a trailing carriage return and break
comparisons for scripts checked out with Windows line endings. Update firstLine
to normalize line endings before returning the first line, and keep the fix
localized to this helper so the shebang checks in
continuous-learning-shebang-consistency.test.js compare the logical first line
consistently.
- Around line 23-32: The custom test runner’s test() catch block is losing
failure context by logging only err.message and using console.log in edited test
code. Update the failure handling in test() to surface the full error object so
stack traces and assertion diffs are preserved, and replace the direct
console.log usage with the repo-preferred error/reporting approach while keeping
the passed/failed accounting intact.
🪄 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: fe41af24-52b0-4b13-8445-5524cd66e98d
📒 Files selected for processing (4)
skills/continuous-learning-v2/agents/start-observer.shskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shtests/hooks/continuous-learning-shebang-consistency.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Greptile Review
- GitHub Check: Test (macos-latest, Node 18.x, pnpm)
- GitHub Check: Test (macos-latest, Node 22.x, bun)
- GitHub Check: Test (macos-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, bun)
- GitHub Check: Test (macos-latest, Node 20.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
🧰 Additional context used
📓 Path-based instructions (20)
**/*.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.shskills/continuous-learning-v2/scripts/detect-project.shskills/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/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shskills/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/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shskills/continuous-learning-v2/agents/start-observer.sh
skills/**/scripts/**
⚙️ CodeRabbit configuration file
skills/**/scripts/**: Review generated or imported scripts as untrusted-input tooling. Flag RCE, path traversal, network fetches without validation, and writes outside the expected workspace.
Files:
skills/continuous-learning-v2/scripts/detect-project.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/hooks/continuous-learning-shebang-consistency.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/hooks/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.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/continuous-learning-shebang-consistency.test.js
**/*.{jsx,tsx,html,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Sanitize HTML output to prevent XSS vulnerabilities
Files:
tests/hooks/continuous-learning-shebang-consistency.test.js
🧠 Learnings (1)
📚 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/continuous-learning-shebang-consistency.test.js
🪛 ast-grep (0.44.0)
tests/hooks/continuous-learning-shebang-consistency.test.js
[error] 41-41: An archive entry path (e.g. entry.path / entry.fileName / header.name) is joined to an output directory without validating that the resolved path stays inside that directory. A malicious archive can use "../" sequences to escape the extraction directory and overwrite arbitrary files (Zip Slip). Resolve the path and verify it starts with the normalized output directory, or strip traversal with path.basename, before writing the entry.
Context: path.join(dir, entry.name)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(zip-slip-archive-extraction-javascript)
[warning] 52-52: 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(filePath, 'utf8')
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/agents/start-observer.sh (1)
1-1: LGTM!skills/continuous-learning-v2/hooks/observe.sh (1)
1-1: LGTM!skills/continuous-learning-v2/scripts/detect-project.sh (1)
1-1: LGTM!
|
| Filename | Overview |
|---|---|
| skills/continuous-learning-v2/agents/start-observer.sh | Updates the observer launcher shebang to the portable bash form. |
| skills/continuous-learning-v2/hooks/observe.sh | Updates the observation hook shebang to the portable bash form. |
| skills/continuous-learning-v2/scripts/detect-project.sh | Updates the project detection helper shebang to the portable bash form. |
| tests/hooks/continuous-learning-shebang-consistency.test.js | Adds shebang consistency coverage for continuous-learning-v2 shell scripts. |
Reviews (3): Last reviewed commit: "test(continuous-learning-v2): skip hidde..." | Re-trigger Greptile
Address review feedback on the shebang-consistency regression test: - firstLine() now splits on /\r?\n/ so a script checked out with CRLF line endings does not leave a trailing carriage return that would break the shebang comparison on Windows. - The test() helper now surfaces the full error (stack trace, not just the message) on failure and writes pass/fail lines via process.stdout/stderr so diagnostics are preserved.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
The recursive shell-script scan now skips hidden directories (e.g. the observer's runtime `.observer-tmp`). This keeps the shebang-consistency check deterministic: only committed skill scripts are examined, and an untracked local artifact left over from an observer run can no longer cause a false failure.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Quick note: these CI reds aren't from this PR. The leftover Windows and Python Tests reds are separate, already-existing issues (a bash-detection test and a missing pyyaml), not related to this change. |
daltino
left a comment
There was a problem hiding this comment.
This PR makes a small but important improvement by standardizing the shell script shebangs to #!/usr/bin/env bash, which enhances portability across systems like NixOS, Homebrew environments, and others where bash may not be at /bin/bash. It also adds a test to ensure this standard is maintained going forward — a thoughtful addition that aligns well with the repo's guidelines for automation and consistency. 🚀
What Changed
Standardized the shebang line in three
skills/continuous-learning-v2/shellscripts from the hardcoded
#!/bin/bashto the portable#!/usr/bin/env bash:hooks/observe.shscripts/detect-project.shagents/start-observer.shThe other four scripts in this skill already used
#!/usr/bin/env bash, so thisbrings the whole directory to a single convention. Also added a regression test
(
tests/hooks/continuous-learning-shebang-consistency.test.js) that assertsevery
*.shunder the skill uses the portable shebang.Why This Change
The hardcoded
#!/bin/bashpath fails to execute on systems where bash is notinstalled at
/bin/bash— NixOS, some Homebrew layouts, and FreeBSD.observe.shruns on every PreToolUse/PostToolUse hook invocation, so it is the highest-impact
offender.
#!/usr/bin/env bashresolves bash viaPATH, which is the portableform and already the repo-wide majority convention.
Testing Done
node tests/run-all.js— 2946 passed, 0 failed (includes the new test).continuous-learning-shebang-consistency.test.jsstandalone: 9/9 pass.npx eslint scripts/**/*.js tests/**/*.js— clean.node scripts/ci/validate-skills.js— 277 skill dirs validated.node scripts/ci/check-unicode-safety.js— passed.node scripts/ci/validate-no-personal-paths.js— passed.Type of Change
/bin/bash)Security & Quality Checklist
Documentation
No documentation changes required — shebang normalization is internal to the
skill's shell scripts.
Fixes #2303