fix(codex): support repository-scoped pre-push opt-out#2557
Conversation
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (15)**/*.sh📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{scripts,bin}/**⚙️ CodeRabbit configuration file
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
Files:
**/*.{test,spec}.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,json,env*}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{jsx,tsx,js,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,env*}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-07-17T04:35:58.631ZApplied to files:
🪛 ast-grep (0.44.1)tests/scripts/codex-hooks.test.js[warning] 112-112: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 114-114: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 400-403: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 433-433: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 516-516: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) [warning] 967-967: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) 🔇 Additional comments (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pre-push hook now resolves the repository root, supports tracked ChangesPre-push opt-out handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Git
participant PrePushHook
participant VerificationSuite
Git->>PrePushHook: Resolve repository root and git directory
PrePushHook->>Git: Read tracked .ecc-prepush-disable metadata
PrePushHook->>VerificationSuite: Skip or run pre-push checks
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 |
|
| Filename | Overview |
|---|---|
| scripts/codex-git-hooks/pre-push | Adds repository-scoped .ecc-prepush-disable opt-out with index mode/stage guards and symlink rejection; replaces --is-inside-work-tree guard with --show-toplevel to capture repo_root, and moves cd "$repo_root" after the new checks. Implementation is sound, but the opt-out triggers on staged (not yet committed) index entries, which may surprise developers. |
| scripts/codex/install-global-git-hooks.sh | Adds three new log lines that describe the pre-push-only opt-out and the ECC_PREPUSH_FULL=1 override; cleanly expands the existing guidance without changing any install logic. |
| tests/scripts/codex-hooks.test.js | Adds 7 focused tests covering the happy path, untracked marker, symlink-mode marker (core.symlinks=false), deletion push with ECC_PREPUSH_FULL, pre-commit still blocking secrets, and default-unchanged behavior; helper functions runGit, runHook, createLinkedWorktreeFixture, and cleanupLinkedWorktreeFixture are well-isolated and tidy. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[git push triggered] --> B{ECC_SKIP_GIT_HOOKS=1\nor ECC_SKIP_PREPUSH=1?}
B -- yes --> Z[exit 0]
B -- no --> C{git rev-parse\n--show-toplevel succeeds?}
C -- no --> Z
C -- yes --> D{.ecc-hooks-disable\nor git_dir/ecc-hooks-disable\nexists?}
D -- yes --> Z
D -- no --> E[Read git index entry\nfor .ecc-prepush-disable\nvia ls-files --stage]
E --> F{ECC_PREPUSH_FULL=1?}
F -- yes --> I
F -- no --> G{File on disk,\nnot a symlink,\nstage==0,\nmode 100644 or 100755?}
G -- yes --> H[Print opt-out message\nexit 0]
G -- no --> I[cd repo_root]
I --> J{All refs are\ndeletion pushes?}
J -- yes --> Z
J -- no --> K[Run lint / typecheck /\ntest / build / go test / pytest]
K --> L{All checks pass?}
L -- yes --> M[exit 0]
L -- no --> N[fail + exit 1]
Reviews (1): Last reviewed commit: "fix: validate pre-push marker index mode" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| prepush_marker_entry="$(git -C "$repo_root" ls-files --stage -- ".ecc-prepush-disable" 2>/dev/null || true)" | ||
| if [[ -n "$prepush_marker_entry" ]]; then | ||
| read -r prepush_marker_mode _prepush_marker_oid prepush_marker_stage _prepush_marker_path <<< "$prepush_marker_entry" | ||
| fi |
There was a problem hiding this comment.
git ls-files --stage reflects the index, not committed history. If a developer runs git add .ecc-prepush-disable (e.g., via git add .) without yet committing, the very next git push will be bypassed with the opt-out message, even though the file has never been committed and the developer may not realize what happened. The installer guidance says "creating and tracking a regular .ecc-prepush-disable file" which most developers interpret as requiring a commit. Consider adding a comment here noting that staged-but-uncommitted entries are also accepted, and potentially document this in the installer log.
| if [[ "${ECC_PREPUSH_FULL:-0}" != "1" && -f "$prepush_marker" && ! -L "$prepush_marker" ]] \ | ||
| && [[ "$prepush_marker_stage" == "0" ]] \ | ||
| && [[ "$prepush_marker_mode" == "100644" || "$prepush_marker_mode" == "100755" ]]; then | ||
| printf '[ECC pre-push] Skipping: repository opt-out .ecc-prepush-disable is tracked; CI/remote checks remain authoritative.\n' | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
When a developer clones a repository that has .ecc-prepush-disable committed at the root, ECC will silently skip lint, typecheck, test, and build on every subsequent push from that repo. The log message is a one-liner that appears in push output and is easy to miss, especially in CI-like terminal environments or automated push scripts. Per SECURITY.md, "Local developer tools can still be valid security issues when untrusted repository content... can trigger execution without clear user intent." Here the effect is suppression rather than execution, but the net result is the same: a third-party repository author can modify a developer's local security posture without explicit per-user consent. Pre-commit checks remain active, which limits exposure, but the gap is worth documenting in a user-facing security note.
Rule Used: Treat CLI inputs, URLs, file paths, and subprocess... (source)
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
DECISION: APPROVE
This PR improves the pre-push hook functionality by adding repository-scoped opt-out options, such as .ecc-prepush-disable, making the workflow more flexible. The changes respect existing disabling methods and add clear documentation for opting out. The tests also cover the new functionality comprehensively, ensuring robustness. Everything aligns well with the repo's contribution guidelines, particularly around enhancing hooks and adding utility for developers.
2 participants
What Changed
.ecc-prepush-disablemarker. When it is a tracked, regular index entry at the real checkout root, ECC skips only its pre-push suite.ECC_PREPUSH_FULL=1forces the full pre-push suite even when the marker is present..ecc-hooks-disable.Why This Change
The global pre-push suite can be disproportionate for repositories whose authoritative validation is protected remote CI. The existing
.ecc-hooks-disablemarker is too broad because it also disables pre-commit checks.Security and Defaults
120000, or conflicted marker runs the full suite.Testing Done
node tests/run-all.js) — the full Windows run used a noncanonical Node 24/npm 12/global profile; after dependencies materialized, 3099/3105 tests passed. The six remaining failures are in unchanged environment/harness surfaces (WSL bash discovery, npm 12 pack JSON, global HOME/USERPROFILE state, and Windows buffer handling). The focused changed surface passes 27/27. Remote upstream CI remains mandatory.TDD evidence: initial RED 22/2 -> GREEN 24/0; review RED 24/2 -> GREEN 26/0; hardening RED 25/2 -> final GREEN 27/0.
bash -nandgit diff --checkpass. Code review, security review, and final Ultra audit found no P0-P3 findings.Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
If you changed dependencies or
package.json(bin/files/ deps)Not applicable; dependencies and
package.jsonare unchanged.If you added a skill, command, agent, hook, or CLI tool
Not applicable; this changes an existing hook and does not add a distributable component.
Documentation
Tracking
Tracks vex1br/metaread#69.
Rollback
Revert this PR. No migration or persistent state is involved.