port: hook + validator fixes from ECC#68
Merged
Conversation
Second port PR from the 2026-05-12 upstream sync round (audit log:
upstream/sync-rounds/2026-05-12.md).
scripts/hooks/block-no-verify.js (0dcde13)
Rewrite flag detection to use shell-words tokenization instead of
string-stripping regex. EGC's previous stripQuotedStrings approach
had two real bypass holes: (1) "git -c \"core.hooksPath=/dev/null\"
commit -m \"msg\"" stripped the quoted -c value to "" and silently
allowed the hook override; (2) "git commit \"--no-verify\" -m
\"msg\"" similarly stripped the quoted flag to "". The tokenizer
preserves shell quoting structure, so quoted args occupying the
flag position are still detected as flags while flags MENTIONED
inside a commit message body remain false-positive-safe.
Also gains: combined short options like `git commit -am` are now
parsed correctly (-am with a message arg does not consume the
next token as a value for -n), and chained git commands like
"git commit -m 'msg' && git push --no-verify" detect the push
bypass instead of stopping at the first git invocation.
tests/hooks/block-no-verify.test.js (0dcde13)
+6 new tests: quoted core.hooksPath override; -am combined short
opt + quoted --no-verify / -n message; git bypass phrase in
quoted message; real quoted --no-verify flag still blocked; later
chained git commands still blocked. 23/23 pass.
scripts/ci/validate-skills.js (e196f8a)
Extend the validator to inspect SKILL.md frontmatter:
- require name: field (non-empty after trim + comment-strip)
- reject literal block-scalar description: (|, |-, |+, |2, |-2,
|- # note, etc.) — preserves internal newlines and breaks
flat-table renderers keyed off description.
Frontmatter findings default to WARN (exit 0) and promote to ERROR
with --strict or CI_STRICT_SKILLS=1. Structural findings (missing
or empty SKILL.md) remain hard errors.
skills/openclaw-persona-forge/SKILL.md (data fix, paired with e196f8a)
description: was a literal block scalar (|-). Inlined to a quoted
string preserving the original Chinese text verbatim.
skills/skill-stocktake/SKILL.md (data fix, paired with e196f8a)
Frontmatter was missing name: entirely. Added "name: skill-stocktake".
.github/workflows/reusable-validate.yml
Enable --strict on validate-skills now that both data defects are
fixed. Future SKILL.md additions cannot reintroduce the same
class of frontmatter defect without failing CI.
Four ECC commits evaluated and not ported (architecture mismatch):
- d26d66f / b1456bd / 03108be: ECC's session-start hook emits an
additional_context block carrying learned-instinct content. EGC's
session-start.js only manages session/learned-skills directories
and command shims — there is no additional_context surface to
inject into. These are session-start design changes, not bug
fixes.
- 3fadc37: ECC moves continuous-learning observe from a bash hook
to a Node observe-runner registered in hooks/hooks.json. EGC keeps
the observe hook inside the skill bundle at
skills/continuous-learning-v2/hooks/observe.sh. Porting would
require restructuring EGC's skill-local hook layout.
Lint clean. 270/264 -> 270 total tests pass (+6 new in
block-no-verify). validate-skills reports 0 warnings against the
current skills/ tree under --strict.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a strict-mode SKILL.md validator (parses frontmatter, enforces ChangesSkill Directory Validation System
Git Hook Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/ci/validate-skills.js`:
- Around line 151-154: The current empty-name check (else if (values.name ===
'')) misses quoted empty strings and YAML nulls; update the conditional after
the hasOwnProperty check to treat null/undefined and any string that's empty
after trimming as invalid (e.g. if (values.name == null ||
String(values.name).trim() === '')), and call reportFrontmatterFinding the same
way (referencing values, reportFrontmatterFinding and the SKILL.md message) so
"" / '' / ~ / null all fail CI.
In `@scripts/hooks/block-no-verify.js`:
- Around line 240-242: The isCommitNoVerifyShortFlag function fails to detect
combined short flags where 'n' is not the first char (e.g., '-an'); update its
detection to match any short-option token that starts with a single dash
followed by letters and contains 'n' anywhere (but not long flags starting with
'--'). Modify isCommitNoVerifyShortFlag to return true for tokens like '-n' or
any /^-[a-zA-Z]*n[a-zA-Z]*$/ so combined flags such as '-an', '-na', '-abcn' are
caught while preserving rejection of long flags.
In `@tests/hooks/block-no-verify.test.js`:
- Around line 144-177: Add a test case that ensures combined short options with
-n not in first position are blocked: use the existing test harness/runHook and
add a test named "blocks -n in combined short options like -an" which runs 'git
commit -an -m "msg"' and asserts exit code 2; then fix the parsing bug in
isCommitNoVerifyShortFlag so it detects 'n' anywhere inside a combined
short-flag token (e.g., treat '-an' and '-na' equivalently) by scanning the
characters of combined flags rather than only checking first position.
🪄 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: 5813702e-cbe9-4190-9ae5-fb20b88b9678
📒 Files selected for processing (6)
.github/workflows/reusable-validate.ymlscripts/ci/validate-skills.jsscripts/hooks/block-no-verify.jsskills/openclaw-persona-forge/SKILL.mdskills/skill-stocktake/SKILL.mdtests/hooks/block-no-verify.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
P0 — combined short option bypass in block-no-verify (CodeRabbit):
isCommitNoVerifyShortFlag only matched when 'n' was the first
character after the dash. Tokens like '-an' (== '-a -n' in git
semantics) or '-na' slipped through because the regex was
/^-n[a-zA-Z]/. Rewritten to scan every character of a combined
short-option token until either an 'n' is found (bypass) or a
value-consuming flag like -m / -F / -C / -c is hit (which ends
the option chain, the rest becomes the inline value). So '-mn'
still allows ('n' is part of the -m message), '-am' still allows
(no 'n' before the message-consuming 'm'), but '-an' / '-na' /
'-abcn' are now caught.
P1 — quoted-empty / YAML-null name bypass (CodeRabbit):
inspectFrontmatter leaves raw scalar values as literal strings,
so `name: ""` produced values.name === '""' (length 2, not 0)
and slipped past the `values.name === ''` check. Same for `''`,
`null`, and `~`. Added normalizeYamlScalar() that strips matched
outer quotes and treats `null` / `~` as empty before the check.
Tests (+3 in block-no-verify, 26 total):
- blocks -n hidden inside combined short option (-an)
- blocks -n hidden inside combined short option (-na)
- still allows -mn (n is inside the -m message, not a flag)
Lint clean. 273/270 -> 273 total tests pass.
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 (1)
scripts/hooks/block-no-verify.js (1)
333-352:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
tokenizeShellWords()to properly handle quoted arguments before the subcommand.The current code splits on whitespace:
gap.trim().split(/\s+/)is not quote-aware. A quoted global-flag value with spaces can hide the real subcommand. For example,git --git-dir "/tmp/repo dir/.git" commit --no-verifycauses the parser to incorrectly tokenize the gap into['--git-dir', '"/tmp/repo', 'dir/.git"']and fail to identifycommitas the subcommand, allowing the--no-verifybypass.Suggested fix
- const gap = input.slice(git.idx + git.len, cmdIdx); - const tokens = gap.trim().split(/\s+/).filter(Boolean); + const tokens = tokenizeShellWords(input, git.idx + git.len, cmdIdx) + .map(token => token.value);Add a test case with a quoted path containing spaces to prevent regression.
🤖 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 `@scripts/hooks/block-no-verify.js` around lines 333 - 352, Replace the naive split (gap.trim().split(/\s+/)) with a quote-aware tokenizer by calling tokenizeShellWords on the gap (e.g., tokens = tokenizeShellWords(gap.trim())), then keep the existing loop logic that uses expectFlagArg/onlyFlagsAndArgs and the same git global-flag list so quoted flag values (like "--git-dir \"/tmp/repo dir/.git\"") are treated as single tokens; ensure any whitespace-trimming still occurs before tokenization and leave the fallback that advances searchPos unchanged. Also add a unit/test case that runs the affected parsing path with a quoted path containing spaces (for example a --git-dir value with spaces) to prevent regression.
🤖 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 `@scripts/hooks/block-no-verify.js`:
- Line 70: The COMMIT_SHORT_OPTIONS_WITH_VALUE set is missing the short option
't', causing isCommitNoVerifyShortFlag() to mis-parse combined flags like -tn;
update the COMMIT_SHORT_OPTIONS_WITH_VALUE constant (the Set declared as
COMMIT_SHORT_OPTIONS_WITH_VALUE) to include 't' so the parser treats -t as
consuming the next character as its value and stops scanning before checking for
'n'.
- Line 38: The guard compares git config keys case-sensitively using
GIT_CONFIG_KEY_PREFIX ('core.hooksPath=') which allows bypass via case
variations; update the logic to normalize to lowercase before matching: either
set GIT_CONFIG_KEY_PREFIX to all-lowercase ('core.hookspath=') and call
.toLowerCase() on the incoming config key before doing startsWith, or always
compare configKey.toLowerCase().startsWith(GIT_CONFIG_KEY_PREFIX.toLowerCase());
apply the same normalization to the other matching block that checks git config
keys (the checks referencing GIT_CONFIG_KEY_PREFIX and the config key parsing in
the later hook-checking logic).
---
Outside diff comments:
In `@scripts/hooks/block-no-verify.js`:
- Around line 333-352: Replace the naive split (gap.trim().split(/\s+/)) with a
quote-aware tokenizer by calling tokenizeShellWords on the gap (e.g., tokens =
tokenizeShellWords(gap.trim())), then keep the existing loop logic that uses
expectFlagArg/onlyFlagsAndArgs and the same git global-flag list so quoted flag
values (like "--git-dir \"/tmp/repo dir/.git\"") are treated as single tokens;
ensure any whitespace-trimming still occurs before tokenization and leave the
fallback that advances searchPos unchanged. Also add a unit/test case that runs
the affected parsing path with a quoted path containing spaces (for example a
--git-dir value with spaces) to prevent regression.
🪄 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: 2a96321a-7580-4ddf-8627-bb11c55a088f
📒 Files selected for processing (3)
scripts/ci/validate-skills.jsscripts/hooks/block-no-verify.jstests/hooks/block-no-verify.test.js
P0 — core.hooksPath case-insensitivity bypass (CodeRabbit): Git config section and variable names are case-insensitive per git-scm.com/docs/git-config, so 'git -c core.hookspath=/dev/null commit ...' or 'core.HOOKSPATH=...' bypassed the previous guard which compared against the literal 'core.hooksPath=' constant. Switched GIT_CONFIG_KEY_PREFIX to the lowercase form and lowercase the candidate tokens in hasHooksPathOverride before matching, in both the '-c <key>' and '-c<key>' code paths. P1 — -t template short option missing (CodeRabbit): COMMIT_SHORT_OPTIONS_WITH_VALUE was missing 't'. Git parses 'git commit -tn' as -t with template path 'n' (stuck-arg form), not '-t -n'. Without 't' in the set, the scanner reached the 'n' check and falsely flagged the command as a --no-verify bypass. Added 't' to the set with a comment explaining the stuck-arg semantics. Tests (+3, 29 total in this file): - still allows -tn (n is the -t template path, not a flag) - blocks case-variant core.hooksPath (lowercase) - blocks case-variant core.hooksPath (uppercase) Lint clean. 276/276 across the suite.
This was referenced May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second port PR from the 2026-05-12 upstream sync round (audit log:
upstream/sync-rounds/2026-05-12.md).Lands two real correctness fixes plus the matching CI guard:
scripts/hooks/block-no-verify.js(ECC0dcde13) — close two bypass holes in the git-hook protection hook.scripts/ci/validate-skills.js(ECCe196f8a) — flag SKILL.md frontmatter defects in CI.openclaw-persona-forge+skill-stocktakeSKILL.md so the new validator passes under--strict.--strictin CI so future SKILL.md additions can't regress the same defects.Bypass holes closed in
block-no-verify.jsEGC's previous detection ran
stripQuotedStrings(regex) before the flag regexes. Two real holes:git -c "core.hooksPath=/dev/null" commit -m "msg"— the quoted-cvalue got stripped to"", so the override slipped through.git commit "--no-verify" -m "msg"— quoted--no-verifyflag stripped to"".The upstream rewrite tokenizes by shell words instead, so quoted args occupying the flag position are still detected as flags while flags mentioned inside a commit message body stay false-positive-safe.
Additional fixes that come along:
git commit -amparse correctly (-amwith a message arg does not consume the next token as a-nvalue).git commit -m 'msg' && git push --no-verifydetect the second-segment bypass instead of stopping at the first git invocation.-an/-na/-abcnstyle combined short options are now detected — CodeRabbit caught that the upstream regex^-n[a-zA-Z]only matched whennwas the first character.Validator extension
validate-skills.jsnow inspects SKILL.md frontmatter:name:field. Quoted-empty ("",'') and YAML null (~,null) are normalised to empty before the check (round 2 fix from CodeRabbit).description:indicators (|,|-,|+,|2,|-2,|- # note, etc. per YAML 1.2). Inline strings and folded>scalars are fine.Frontmatter findings default to WARN;
--strictorCI_STRICT_SKILLS=1promotes them to ERROR. Structural findings (missing/empty SKILL.md) remain hard errors as before.Data fixes
The validator surfaced exactly the two defects ECC's PR #1664 fixed upstream:
skills/openclaw-persona-forge/SKILL.mdhaddescription: |-(multi-line Chinese). Inlined to a single quoted string preserving the original text verbatim.skills/skill-stocktake/SKILL.mdwas missingname:entirely. Addedname: skill-stocktake(kept the "Gemini skills" wording — not reverted to ECC's "Claude skills").After the data fixes, the validator reports zero warnings under
--strict, so the CI workflow now runs it in strict mode.Files (6, +580 / -65)
scripts/hooks/block-no-verify.js0dcde13+ round-2 fix-an/-nanow caughttests/hooks/block-no-verify.test.js0dcde13+ round-2 fixscripts/ci/validate-skills.jse196f8a+ round-2 fix--strictmode + YAML scalar normalisationskills/openclaw-persona-forge/SKILL.mdskills/skill-stocktake/SKILL.mdname:field.github/workflows/reusable-validate.yml--strictto validate-skillsECC commits evaluated and not ported (architecture mismatch)
d26d66f/b1456bd/03108be— ECC's session-start hook emits anadditional_contextblock carrying learned-instinct content. EGC's session-start.js only manages session/learned-skills directories and command shims; there is noadditional_contextsurface to inject into. These are session-start design changes, not portable fixes.3fadc37— ECC moves continuous-learning observe from a bash hook to a Nodeobserve-runner.jsregistered in top-levelhooks/hooks.json. EGC keeps the observe hook inside the skill bundle atskills/continuous-learning-v2/hooks/observe.sh. Porting would mean restructuring EGC's skill-local hook layout.These skips are also recorded against the audit log for future-round provenance.
Test plan
npm run lintcleannpm test273/273node scripts/ci/validate-skills.js0 warnings against the current treenode scripts/ci/validate-skills.js --strictexits 0core.hooksPath, quoted--no-verify, chainedgit push --no-verify, combined-an/-na--no-verify/-n/core.hooksPath=mentioned inside commit messages;-mn(n is the-mmessage body, not a flag)Summary by CodeRabbit
Tests
Chores