fix(guard): honor disabled checks in executor validation - #392
Merged
ceilf6 merged 3 commits intoJul 30, 2026
Merged
Conversation
Owner
|
@ceilf6/repo-guard |
6 similar comments
Owner
|
@ceilf6/repo-guard |
Owner
|
@ceilf6/repo-guard |
Owner
|
@ceilf6/repo-guard |
Owner
|
@ceilf6/repo-guard |
Owner
|
@ceilf6/repo-guard |
Owner
|
@ceilf6/repo-guard |
Owner
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
代码评审报告: fix(guard): honor disabled checks in executor validation
风险等级: 低
处理建议: 评论
决策摘要: 修复本身正确且最小，直接命中 #386 的 Fix 要求，可以合并；但请补齐 validateFilePath 的执行器路径测试与单标志独立性测试，并确认「禁用 fileExistence 同时关掉 guard 层路径逃逸拦截」是可接受的取舍。
级联分析
- 变更符号:
HallucinationGuard.validateFilePath、HallucinationGuard.validateCode（packages/hallucination-guard/src/guard.ts:148、:169） - 受影响流程: executor 执行前校验（
packages/core/src/executor/executor.ts:420read_file、:509create_file）与执行后代码校验（:550）；配置入口packages/core/src/agent/agent.ts:96 - 变更集外调用方: 文本搜索（text search）显示
@frontagent/hallucination-guard的这两个方法仅被packages/core/src/executor/executor.ts与两个测试文件消费，无其他包引用；默认值全部为true，未显式禁用的调用方行为不变 - 置信度: medium（PR 提供的 GitNexus impact 与本地 grep 一致；MCP 工具层与 SecurityManager 的兜底行为由代码阅读确认，未实际运行测试）
问题发现
-
[中] 禁用 fileExistence 会连带关闭 guard 层的路径逃逸拦截
- 证据:
checkFileExistence在存在性判断之前先做isInsidePath安全检查并以severity: 'block'返回（packages/hallucination-guard/src/checks/file-existence.ts:29-37）。新的提前返回（guard.ts:149）跳过了整个函数，因此fileExistence: false时../../etc/passwd这类路径在 guard 层不再被拦截，而返回pass: true。 - 受影响调用方/流程:
executor.ts:420（read_file）、executor.ts:509（create_file）。写路径仍被SecurityManager.evaluateFileWrite以file_path_outside_project硬性 deny（packages/core/src/security.ts:363-374，不可被 allow 规则绕过）；读路径仍被packages/mcp-file/src/path-safety.ts:83/packages/mcp-filesense/src/tools.ts:234拦截。所以这是纵深防御少了一层，而不是唯一防线失守——也正因如此不作为阻塞项。同时validate()慢路径本来就有相同语义（guard.ts:79），本 PR 只是让快路径与之一致。 - 最小可行修复: 在
validateFilePath提前返回之前先调用isInsidePath（checks/path-containment.ts已导出），越界时仍返回 block；让 ablation 开关只关闭“幻觉检查”，不关闭路径边界。若有意保留现状，请在 PR 或 issue 中显式记录该取舍。
- 证据:
-
[中] 执行器路径上的
validateFilePath门控实际未被测试覆盖- 证据: 新增执行器测试走
create_file分支，调用的是validateFilePath(path, false)（executor.ts:509），而src/broken.ts在临时目录中并不存在——即使没有本次修复，checkFileExistence也会返回pass: true。因此该测试只证明了validateCode的门控。 - 受影响调用方/流程: issue #386 的 Fix 明确要求「Add a test that a fully-disabled guard performs no checks on the executor path」，其中
validateFilePath的阻塞行为发生在read_file（executor.ts:420，shouldExist = true）。 - 最小可行修复: 增加一个
read_file步骤、目标为不存在的文件、guard 全禁用的用例，断言result.validation.pass === true；该用例在修复前会因Hallucination detected: File ... does not exist失败。
- 证据: 新增执行器测试走
-
[中] 三个新测试都一次性关闭全部四个开关，无法验证标志与检查的对应关系
- 证据:
guard.test.ts:224-229、:244-249、executor.test.ts:419-424均设置四个 flag 全 false。若门控接错（例如语法检查误挂在importValidity上），这些断言仍然全绿。 - 受影响调用方/流程:
validateCode中两个独立门控（guard.ts:177、:183-187）；ablation benchmark 需要逐项开关才有意义。 - 最小可行修复: 补一个
syntaxValidity: false, importValidity: true的用例（断言仍报告导入错误）和一个反向用例（断言仍报告语法错误）。
- 证据:
-
[中]
HallucinationGuardConfig.enabled仍是死配置- 证据:
packages/core/src/types.ts:411声明enabled: boolean，但全仓仅agent.ts:96读取config.hallucinationGuard?.checks，enabled没有任何读取点（text search）。即{ enabled: false }且不写checks时，guard 依旧全量执行。 - 受影响调用方/流程: issue #386 的 problem statement 是「guard 无法通过文档化的配置面关闭」；
enabled正是最直观的那个开关，ablation 若用它仍然“什么都没测到”。不过 issue 的 Fix 段只点名enabledChecks+ 两个方法，本 PR 已覆盖该范围。 - 最小可行修复: 在
agent.ts:93-97把enabled === false映射为四项全 false（或删除该字段）；也可作为独立 follow-up issue，但建议在本 PR 描述中说明它仍未生效，避免 #386 被误判为完全关闭。
- 证据:
行级发现
- [packages/hallucination-guard/src/guard.ts:149] 该提前返回同时跳过了
checkFileExistence内的isInsidePath边界拦截；建议在返回前先做路径包含性检查，越界仍返回severity: 'block'。 - [packages/hallucination-guard/src/guard.test.ts:244] 四个开关全关时无法区分是哪个 flag 在生效；补一组
syntaxValidity: false / importValidity: true（及反向）的用例来锁定门控对应关系。 - [packages/core/src/executor/executor.test.ts:454] 此断言只覆盖
validateCode；create_file走的是validateFilePath(path, false)且目标文件不存在，修复前后同样通过。请补一个不存在文件的read_file步骤来真正覆盖executor.ts:420。
Karpathy 评审
- 假设: 假设「禁用 fileExistence」只意味着关闭幻觉检查，而不包括路径边界安全语义——该假设未在代码或 PR 中言明，见发现 1。其余假设（默认全开、
setCheckEnabled运行时可变）与validate()既有语义一致。 - 简洁性: 修复方式与
validate()中已有的if (this.enabledChecks.x)模式一致，没有引入新抽象、wrapper 或配置层；门控逻辑在慢路径/快路径各写一遍属于可接受的轻微重复。 - 结构质量: 无退化。文件行数、函数复杂度、层级归属均未越界；逻辑留在拥有该概念的 guard 类内，没有泄漏到 executor。
- 变更范围: 外科手术式，3 个文件、+114/-3，无无关重构与格式噪声，全部变更可追溯到 #386 的 Fix 段。
- 验证: 方向正确（执行器路径 + guard 单元层双覆盖），但存在两处覆盖空洞（发现 2、3）。PR 列出的 typecheck / precommit / quality:local 未在本次评审中重跑，仅采信 PR 自述。
缺失覆盖
- 执行器路径上 guard 全禁用时的
read_file（不存在文件）用例，覆盖executor.ts:420→validateFilePath(path, true)。 - 单标志独立性用例：仅关语法检查、仅关导入检查各一条，确认两个门控没有接错。
- guard 禁用状态下的路径逃逸用例（
../../etc/passwd），用于固化发现 1 中「保留边界拦截还是有意放行」的决定。
| * 快速验证文件路径 | ||
| */ | ||
| async validateFilePath(path: string, shouldExist = true): Promise<HallucinationCheckResult> { | ||
| if (!this.enabledChecks.fileExistence) { |
There was a problem hiding this comment.
该提前返回同时跳过了 checkFileExistence 内的 isInsidePath 边界拦截；建议在返回前先做路径包含性检查，越界仍返回 severity: 'block'。
| it('honors disabled syntax and import checks on the fast path', async () => { | ||
| const guard = new HallucinationGuard({ | ||
| projectRoot: TEST_ROOT, | ||
| enabledChecks: { |
There was a problem hiding this comment.
四个开关全关时无法区分是哪个 flag 在生效；补一组 syntaxValidity: false / importValidity: true（及反向）的用例来锁定门控对应关系。
| content: invalidContent, | ||
| }); | ||
| expect(result.stepResult.success).toBe(true); | ||
| expect(result.validation).toEqual({ pass: true, results: [], blockedBy: undefined }); |
There was a problem hiding this comment.
此断言只覆盖 validateCode；create_file 走的是 validateFilePath(path, false) 且目标文件不存在，修复前后同样通过。请补一个不存在文件的 read_file 步骤来真正覆盖 executor.ts:420。
Owner
|
/review |
There was a problem hiding this comment.
代码评审报告: fix(guard): honor disabled checks in executor validation
风险等级: 中
处理建议: 请求修改
决策摘要: ** 核心修复方向正确且对默认配置零影响，但 validateFilePath 的提前返回同时绕过了 checkFileExistence 内的项目根目录包含性安全检查，这超出了 issue #386 的请求范围；修复只需 3 行，建议合入前处理。
级联分析
- 变更符号: 原始模型未提供结构化级联字段。
- 受影响流程: 原始模型未提供结构化级联字段。
- 变更集外调用方: unknown
- 置信度: degraded
问题发现
- [中] 禁用 fileExistence 同时禁用了项目根目录包含性安全检查
- 证据:
guard.ts:149-157在调用checkFileExistence之前返回pass: true；而packages/hallucination-guard/src/checks/file-existence.ts:29-37在存在性判断之前先执行isInsidePath并以severity: 'block'拦截 “Path is outside project root”，源码中明确注释为「安全检查」。issue #386 只要求跳过幻觉检查，未要求放开路径边界。 - 受影响调用方/流程:
executor.ts:420的read_file前置校验。security.ts:34-48把read_file归入READ_TOOLS，security.ts:324-331直接 allow 且不做任何路径解析，因此在 executor 流水线内，guard 是读路径上唯一的包含性校验。写路径不受影响（security.ts:363-374独立以 critical 拒绝越界写）。 - 最小可行修复: 在提前返回分支内保留包含性判断——复用
isInsidePath(resolve(projectRoot, path), resolve(projectRoot))，越界时仍返回pass: false, severity: 'block'，仅跳过存在性断言。
- 证据:
- [中] executor 路径测试无法区分 fileExistence 是否被真正跳过
- 证据: 新测试走
create_file，对应executor.ts:509的validateFilePath(path, false)；目标src/broken.ts在临时目录中并不存在，因此checkFileExistence在启用状态下同样返回pass: true。该断言只对validateCode具有判别力，把guard.ts:149的改动整体回退，此测试仍会通过。 - 受影响调用方/流程: issue #386 明确要求「a test that a fully-disabled guard performs no checks on the executor path」，该验收项目前只覆盖了一半。
- 最小可行修复: 增加一个
read_file步骤指向不存在的路径（命中executor.ts:419-425的shouldExist=true）。注意启用时会被getPreValidationSkip（executor.ts:250）转成 skipped step，断言应对比 skipped 与实际执行两种结果。
- 证据: 新测试走
- [低] 两个 guard 测试都把四个开关全部关闭，未覆盖开关独立性
- 证据:
guard.test.ts:224-229与244-249使用相同的全关配置。若guard.ts:184的 import 分支误用syntaxValidity（或反之），两个测试仍然通过。 - 受影响调用方/流程:
validateCode的两个独立开关缺少回归保护。 - 最小可行修复: 补一例
syntaxValidity: false, importValidity: true断言 import 失败仍拦截，及其镜像用例。
- 证据:
- [低] 兄弟配置项
hallucinationGuard.enabled仍是 dead config- 证据:
packages/core/src/types.ts:409-419定义了enabled: boolean，但agent.ts:93-97只转发checks，从不消费enabled。 - 受影响调用方/流程: issue #386 的 Impact 称「guard 无法通过文档化配置面禁用」；本 PR 修好了
checks，而最直观的enabled: false开关在合入后依旧无效。 - 最小可行修复: 超出 issue 的 Fix 段落范围，不阻塞本 PR；建议开独立 issue 跟踪，避免留下第二个同类陷阱。
- 证据:
行级发现
- [packages/hallucination-guard/src/guard.ts:149] 该提前返回同时跳过了
checkFileExistence中的项目根包含性拦截（file-existence.ts:29-37），而read_file在 core security 层不做路径校验；请在此分支内保留isInsidePath判断，越界仍返回 block，只跳过存在性断言。 - [packages/core/src/executor/executor.test.ts:413] 该用例走
create_file（shouldExist=false）且目标文件本就不存在，启用状态下结果相同，无法证明 fileExistence 被跳过；改用指向不存在路径的read_file步骤，并区分 skipped 与实际执行。 - [packages/hallucination-guard/src/guard.test.ts:241] 用例名称声称同时覆盖 syntax 与 import，但四个开关全关，无法区分两者；补一例
syntaxValidity: false, importValidity: true以锁定开关独立性。
Karpathy 评审
- 假设: 模型输出需要归一化为固定 Markdown 契约。
- 简洁性: 已提取 summary、finding、evidence 与 fix；原始 prose 不再附在评论中，避免占用下游解析与代理上下文。
- 变更范围: 原始模型未提供结构化范围字段。
- 验证: 需要查看 CI、测试或人工 CR 证据补强合并信心。
缺失覆盖
- 输出未命中 Repo Guard Markdown 契约；建议补充真实模型质量评估覆盖。
| * 快速验证文件路径 | ||
| */ | ||
| async validateFilePath(path: string, shouldExist = true): Promise<HallucinationCheckResult> { | ||
| if (!this.enabledChecks.fileExistence) { |
There was a problem hiding this comment.
该提前返回同时跳过了 checkFileExistence 中的项目根包含性拦截（file-existence.ts:29-37），而 read_file 在 core security 层不做路径校验；请在此分支内保留 isInsidePath 判断，越界仍返回 block，只跳过存在性断言。
| expect(result.needsRollback).toBe(false); | ||
| }); | ||
|
|
||
| it('honors a fully disabled hallucination guard on the executor validation path', async () => { |
There was a problem hiding this comment.
该用例走 create_file（shouldExist=false）且目标文件本就不存在，启用状态下结果相同，无法证明 fileExistence 被跳过；改用指向不存在路径的 read_file 步骤，并区分 skipped 与实际执行。
| ); | ||
| }); | ||
|
|
||
| it('honors disabled syntax and import checks on the fast path', async () => { |
There was a problem hiding this comment.
用例名称声称同时覆盖 syntax 与 import，但四个开关全关，无法区分两者；补一例 syntaxValidity: false, importValidity: true 以锁定开关独立性。
…guard-enabled-checks
Address Repo Guard review on ceilf6#392: - validateFilePath: the disabled-check fast path now still enforces the isInsidePath project-root boundary (security check, not a hallucination check) and only skips the existence assertion. - guard.test.ts: containment-under-disabled case plus single-flag independence cases (syntaxValidity off keeps import checks, and the mirror), so the two validateCode switches can no longer be crossed silently. - executor.test.ts: discriminating read_file case — a missing path executes when fileExistence is disabled and converts to a skipped step when enabled — covering the half of ceilf6#386's acceptance the create_file case could not. hallucinationGuard.enabled staying dead config is tracked in ceilf6#400. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
|
Maintainer follow-up in 822d1af addressing the Repo Guard review:
Also merged latest 🤖 Generated with Claude Code |
ceilf6
dismissed
github-actions[bot]’s stale review
July 30, 2026 13:48
All four findings addressed in 822d1af (containment kept on the disabled fast path, discriminating read_file executor test, switch-independence tests; finding 4 tracked in #400). Repo Guard cannot re-run on fork PRs under pull_request_target, so dismissing the stale review after manual verification.
ceilf6
approved these changes
Jul 30, 2026
ceilf6
left a comment
ceilf6
left a comment
Owner
There was a problem hiding this comment.
Review findings addressed in 822d1af; verified locally with full quality gates (guard 23/23, executor 28/28, quality:precommit green). Guard check failure on this PR is the pull_request_target fork-checkout refusal, not a review verdict.
This was referenced Jul 30, 2026
Merged
ceilf6
added a commit
that referenced
this pull request
Aug 1, 2026
…yaml Seventh repo-guard round on #402. The first finding is the same defect this repo has now hit four times, committed by me while fixing an instance of it. **The veto ignored `enabledChecks`.** The executor implements its own pre-write gate, and it consulted no configuration — so `syntaxValidity: false` no longer turned it off. That is exactly the mechanism by which #386 voided the guard arm of the July ablation, and #392 had just fixed it. `HallucinationGuard` gains a public `isCheckEnabled()` (any caller reimplementing a check's semantics outside the guard has to be able to ask), and both the veto and the rollback trigger are gated on it. A regression test pins that the guard stays ablatable. **The veto applied to `.yaml`.** A markdown fence inside a YAML block scalar is legitimate content; `detectLanguage` includes yaml, so those writes would have been refused. The predicate is now restricted to TypeScript/JavaScript, where a line starting with ``` is never valid. Regression test included. **`validation_failed`'s `stage` had no counter.** The event carried it but `run-eval.mjs` aggregated by `type` alone, and `report.mjs` printed a bare count under a prose note saying that count must not be used. The harness now also counts `type:stage`, and the report prints the breakdown — `pre_write` is the only real interception; `post_write` includes the forward-reference imports this PR deliberately lets land. **A write tool's failure aborts again.** Excluding plain tool failures from `needsRollback` was meant to stop one failed `read_file` from killing a plan, but it covered writes too: after an `EACCES` on a write the plan continued, and if the following steps merely *referenced* the missing module they would all pass — the run ending green with the module absent. Read-only actions keep the exclusion; writes do not. **The unreadable-after-write branch is no longer silent.** When the file cannot be read back, no rollback is attempted and nothing is recorded — three ways of looking like "nothing happened". It now warns and annotates `stepResult.error`. Deliberately *not* setting `rollbackFailed`: that drives abort semantics and is a stronger claim than "could not determine". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Linked Issue Or Context
Summary
Impact Scope
GitNexus Impact Summary
Verification
Checklist
pnpm quality:precommit, or explained why it could not run.pnpm quality:localfor critical skeleton changes, or explained why it could not run.