Greptile flagged that the env-access guard in
gateguard-env-documented.test.js could be bypassed via reflective reads
of process.env (Reflect.get/has/set/deleteProperty/defineProperty/
getOwnPropertyDescriptor/ownKeys), since none of the existing
UNSUPPORTED_ACCESS patterns matched that form.
Add a rule that rejects Reflect.get/has/set/deleteProperty/
defineProperty/getOwnPropertyDescriptor/ownKeys(process.env, ...) and
three self-check fixture cases (Reflect.get, Reflect.has,
Reflect.ownKeys) so the guard is pinned against silently missing them
again.
Negative control: commenting out only the new rule reproduces exactly
the reported gap (the 3 new fixture cases fail with "access guard
missed: Reflect.get, Reflect.has, Reflect.ownKeys"); restoring it goes
back to 10/10.
The documentation surface test scanned the hook's raw source with two
regexes. That had two holes, both confirmed against the shipped parser:
- a GATEGUARD_* name appearing only in a comment or a string was counted
as a real read, and
- destructured, aliased and computed reads were invisible, so an
undocumented knob added in one of those forms would pass silently.
Blank comments, string literals, template-literal text and regex literals
before scanning, so only real code contributes. Blanking preserves length,
so `process.env[...]` keys are located in the blanked code and read back
from the raw source at the same offset.
Rather than chase every possible access form with regexes, the supported
forms are now enforced: destructuring, aliasing, spreading, enumerating
and computed keys fail the guard with instructions to either keep the
convention or extend the parser. Six self-checks cover the blanker and
the guard, including a regex literal containing a slash.
Refs #2573
CodeRabbit review on #2611, all four findings:
- GATEGUARD_DISABLED sat in a table introduced as 'these do not disable the gate'. Moved to its own full-disable section with ECC_GATEGUARD, and corrected the accepted values against ECC_DISABLE_VALUES (0/false/off/disabled/disable - the earlier draft would have implied 'no' works, which it does not).
- Documented that a leading **/ compiles to .*/ and so needs a preceding separator: verified by reproducing the hook's glob->regex translation, **/tests/** matches /repo/tests/foo.js but not a bare relative tests/foo.js. Docs now say so and the example carries both forms. Matcher behaviour deliberately unchanged - widening it is a behaviour change, not a docs fix.
- Reverse-drift check now compares documented names against the parsed env reads instead of hookSource.includes(), so a name surviving only in a comment or error string no longer satisfies it.
- readGateguardEnvNames builds one Set from collected matches instead of mutating via Set#add, per the repo's no-in-place-mutation guideline.
GateGuard reads five GATEGUARD_* environment variables that were absent
from skills/gateguard/SKILL.md, so the only discoverable escape hatch was
ECC_GATEGUARD=off - disabling the load-bearing destructive-Bash gate
along with the noisy ones (#2573).
Documented, with defaults and exact accepted values read from the hook:
- GATEGUARD_BASH_ROUTINE_DISABLED (was undocumented everywhere)
- GATEGUARD_EXEMPT_GLOBS (previously only in a 2.1.0 release note)
- GATEGUARD_BASH_EXTRA_DESTRUCTIVE (was undocumented)
- GATEGUARD_DISABLED (was undocumented)
- GATEGUARD_STATE_DIR (was undocumented; named in a runtime warning)
- GATEGUARD_FACT_FORCE_FULL_DENIALS (already documented; folded into the
same table for one lookup point)
Adds tests/ci/gateguard-env-documented.test.js, which asserts every
GATEGUARD_* variable the hook reads appears in the skill doc, and that the
doc names no variable the hook has stopped reading. That surface test is
what found the three knobs beyond the two the issue reported.
Docs and test only; no hook behaviour changes.
Refs #2573
haelyra