v1.11.0#51
Merged
Merged
Conversation
…es and support for gruff.hook.v1 contract in gruff-code-quality hook
…e optional gruff-code-quality hook unavailability
… optional script unavailability
…s and syntax validation
…error handling and caching
📝 WalkthroughWalkthroughThis PR updates hook launcher root resolution, deny-dangerous and gruff-code-quality behavior, dashboard Alpine fragment structure, and several audit/tooling helpers and tests. ChangesHook launchers and policy checks
Dashboard state, prompts, and terminal fragments
CLI audits, filesystem helpers, and parity scripts
Sequence Diagram(s)sequenceDiagram
participant HookConfig as Hook config
participant Git as git rev-parse
participant Launcher as hook launcher
participant HookScript as .goat-flow/hooks/*.sh
HookConfig->>Git: git rev-parse --show-toplevel
Git-->>HookConfig: repo root
HookConfig->>Launcher: cd root && run hook script
Launcher->>HookScript: execute deny-dangerous.sh or gruff-code-quality.sh
Estimated review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 706c71acb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| root="$(repo_root)" | ||
| cd "$root" || exit 0 | ||
| payload_paths="$(payload_file_paths "$payload")" | ||
| payload_paths="$(payload_supported_file_paths "$root" "$payload")" |
There was a problem hiding this comment.
When a PostToolUse payload names a real but unsupported file such as README.md, this now filters it out before deciding whether to fall back, so payload_paths becomes empty and the hook scans every git-changed supported file instead of doing nothing for that edit. I reproduced this with a pending src/a.ts change and an Edit payload for README.md; the hook reported the TypeScript finding even though the edited file was not analyzable. This makes ordinary docs/config edits surface stale/unrelated gruff diagnostics whenever there are pending code changes; distinguish “payload had paths but none supported” from “payload had no paths” before using git_changed_supported_paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates GOAT Flow’s hook execution model to consistently resolve the active worktree root (via git rev-parse --show-toplevel), tightens deny-dangerous pipeline detection (notably xargs-based destructive payloads), and refactors several dashboard/server/test utilities to improve reliability and maintainability.
Changes:
- Standardize root-resolving hook launcher commands (Claude/Codex/Antigravity) and soften optional
gruff-code-qualitymissing-hook behavior. - Expand
deny-dangerouswrite/destructive detection to cover pipeline segments (xargs, piped git operations) and add/adjust self-tests. - Refactor dashboard app fragments/readers and server route/execution helpers; add a test-only
test/src.tsfacade to reduce deep relative imports.
Reviewed changes
Copilot reviewed 78 out of 78 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/skills/goat-critique/SKILL.md | Docs wording update for critique “role-play” labels. |
| workflow/install-goat-flow.sh | Installer hook launcher generation and messaging changes. |
| workflow/hooks/README.md | Document updated hook root-resolution/runtime contracts. |
| workflow/hooks/deny-dangerous/patterns-writes.sh | Update git write detection to scan pipeline segments + xargs. |
| workflow/hooks/deny-dangerous/patterns-shell.sh | Detect destructive xargs rm -r payloads across pipeline segments. |
| workflow/hooks/deny-dangerous/deny-dangerous-self-test.sh | Update resolver tests; add real linked-worktree + new pipeline cases. |
| workflow/hooks/deny-dangerous.sh | Simplify git root resolution to --show-toplevel. |
| workflow/hooks/agent-config/codex-hooks.json | Codex template updated to git-root wrapper launcher. |
| workflow/hooks/agent-config/claude.json | Claude template updated to git-root wrapper launcher + fallback. |
| workflow/hooks/agent-config/antigravity-hooks.json | Antigravity template updated to git-root wrapper launcher + deny JSON. |
| test/unit/terminal-uploads.test.ts | Add fixture-purpose comment for symlink escape test. |
| test/unit/skill-quality/main.test.ts | Add fixture-purpose comment for symlink walk-root filtering. |
| test/unit/skill-quality/helpers.ts | Switch tests to import via test/src.ts facade. |
| test/unit/local-paths.test.ts | Add fixture-purpose comment for symlink escape test. |
| test/unit/hook-registrar.test.ts | Expand launcher assertions (Codex + gruff) and worktree expectations. |
| test/unit/dashboard-terminal-launch/launch-flow-06.test.ts | Update static assertions to match extracted helper functions. |
| test/unit/dashboard-terminal-launch/fakes.ts | Refactor deterministic fake timers into clearer helper functions/types. |
| test/unit/dashboard-reporting.test.ts | Doc/comment tweaks + fixture-purpose comment. |
| test/unit/dashboard-readers.test.ts | Strengthen assertions by requiring non-null enforcement block. |
| test/unit/dashboard-home.test.ts | Add doc comment + fixture-purpose comment. |
| test/unit/config-writer.test.ts | Add fixture-purpose comment for canonical hook id behavior. |
| test/unit/check-content-quality.test.ts | Replace magic number with named constant. |
| test/unit/audit-harness/check-evidence-before-claims.test.ts | Use test/src.ts facade for imports. |
| test/unit/audit-command/main.test.ts | Use test/src.ts facade; consolidate imports. |
| test/unit/audit-command/hook-facts.test.ts | Use test/src.ts facade; add launcher normalization test. |
| test/unit/audit-command/helpers.ts | Use test/src.ts facade; consolidate imports/types. |
| test/unit/audit-command/agent-deny-hooks.test.ts | Add nested-cwd failure test + error message refinement. |
| test/unit/agent-config-template-parity.test.ts | Refine assertions; detect unknown deny rules more cleanly. |
| test/src.ts | New test-only export facade for stable shallow imports. |
| test/smoke/dashboard-endpoints.test.ts | Minor TS cast cleanup + comments. |
| test/integration/skill-author.test.ts | Add fixture-purpose comment. |
| test/integration/setup-install.test.ts | Add fixture-purpose comments for migration coverage. |
| test/integration/setup-install-codex-config-migration.test.ts | Add fixture-purpose comments for migration coverage. |
| test/integration/path-integrity.test.ts | Add fixture-purpose comments for router/copath validations. |
| test/integration/audit-drift-checkdrift-hook-templates.test.ts | Add fixture-purpose comment for optional Copilot hooks allowlist. |
| test/fixtures/projects/index.ts | Use test/src.ts facade for shared types/constants. |
| src/dashboard/dashboard-readers.ts | Refactor audit-check provenance/enum decoding for stricter contracts. |
| src/dashboard/dashboard-prompts.ts | Refactor preset badge construction into rule list helper. |
| src/dashboard/dashboard-model-readers.ts | Refactor optional boolean/costTier decoding helpers. |
| src/dashboard/dashboard-app-state-fragments.ts | Split/rename Alpine fragments; extract terminal-state helpers. |
| src/dashboard/dashboard-app-skill-quality-fragments.ts | Rename fragments for clarity. |
| src/dashboard/dashboard-app-prompts-audit-fragments.ts | Extract audit/upload helpers; split fragment responsibilities. |
| src/dashboard/dashboard-app-project-terminal-fragments.ts | Extract skill evaluator + terminal scrollback helpers; split fragments. |
| src/dashboard/dashboard-app-init.ts | Extract xterm warmup/refit helpers; simplify watchers. |
| src/dashboard/app.ts | Update fragment composition to new fragment names/splits. |
| src/cli/server/safe-exec.ts | Refactor exec validation/runtime config/output capture into helpers. |
| src/cli/server/dashboard-audit-routes.ts | Refactor into smaller route-handler builders/helpers. |
| src/cli/server/agent-hook-writer.ts | Update hook command generation to active-worktree root wrappers. |
| src/cli/facts/fs.ts | Refactor createFS into smaller cached helper constructors. |
| src/cli/audit/harness/check-context.ts | Refactor doc-path resolution into clearer helpers/accumulators. |
| src/cli/audit/check-agent-deny-mechanism.ts | Refactor hook syntax checks; replay configured hooks from nested cwd too. |
| scripts/profile-dashboard-audit.mjs | Refactor CLI arg parsing into handler map helpers. |
| scripts/preflight-checks.sh | Run configured hook smoke checks from project root + nested .goat-flow. |
| scripts/check-instruction-parity.mjs | Refactor parity validation into per-file helper functions. |
| CHANGELOG.md | Add v1.10.2 entry + reword v1.10.1 bullets for clarity. |
| .gruff-ts.yaml | Update rule set/thresholds and expand security/test-quality rule list. |
| .goat-flow/learning-loop/lessons/verification.md | Update last_reviewed date. |
| .goat-flow/learning-loop/lessons/hook-testing.md | Update last_reviewed + adjust Codex launcher lesson guidance. |
| .goat-flow/learning-loop/lessons/gruff-cleanup.md | Update last_reviewed + add several new gruff/dashboard lessons. |
| .goat-flow/learning-loop/lessons/dashboard-testing.md | Update evidence anchor text to match refactor. |
| .goat-flow/learning-loop/footguns/hooks.md | Update last_reviewed + revise hook launcher footguns. |
| .goat-flow/learning-loop/footguns/deny-dangerous.md | Update last_reviewed + add pipeline-segment inspection footgun. |
| .goat-flow/learning-loop/footguns/auditor.md | Update evidence anchor wording for fs adapter comments. |
| .goat-flow/learning-loop/decisions/ADR-032-scope-gruff-hook-binary-discovery.md | Add update note + document explicit per-language binary overrides. |
| .goat-flow/hooks/deny-dangerous/patterns-writes.sh | Mirror writes-policy pipeline scanning changes into shipped hook copy. |
| .goat-flow/hooks/deny-dangerous/patterns-shell.sh | Mirror destructive xargs pipeline scanning into shipped hook copy. |
| .goat-flow/hooks/deny-dangerous/deny-dangerous-self-test.sh | Mirror updated self-tests into shipped hook copy. |
| .goat-flow/hooks/deny-dangerous.sh | Mirror simplified git root resolution into shipped hook copy. |
| .github/skills/goat-critique/SKILL.md | Docs wording update for critique “role-play” labels. |
| .codex/hooks.json | Update repo’s Codex hook config to use git-root wrapper launcher. |
| .claude/skills/goat-critique/SKILL.md | Docs wording update for critique “role-play” labels. |
| .claude/settings.json | Update repo’s Claude hook config to use git-root wrapper launcher + gruff soft-fail. |
| .agents/skills/goat-critique/SKILL.md | Docs wording update for critique “role-play” labels. |
| .agents/hooks.json | Update repo’s Antigravity hook config to use git-root wrapper launcher + gruff soft-fail. |
Comment on lines
+745
to
+749
| const claudeRootFallback = | ||
| agent === "codex" | ||
| ? "" | ||
| : `; [ -f "$root/.goat-flow/hooks/${script}" ] || root="\${CLAUDE_PROJECT_DIR:-}"`; | ||
| return `bash -c 'root="$(git rev-parse --show-toplevel 2>/dev/null || true)"${claudeRootFallback}; [ -f "$root/.goat-flow/hooks/${script}" ] || ${unavailable}; cd "$root" || ${unavailable}; bash "$root/.goat-flow/hooks/${script}"'`; |
Comment on lines
+140
to
+144
| const claudeRootFallback = | ||
| agent.id === "codex" | ||
| ? "" | ||
| : `; [ -f "$root/${path}" ] || root="\${CLAUDE_PROJECT_DIR:-}"`; | ||
| const ensureRoot = `[ -f "$root/${path}" ] || ${unavailable}`; |
| { | ||
| "type": "command", | ||
| "command": ".goat-flow/hooks/deny-dangerous.sh", | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'", |
| { | ||
| "type": "command", | ||
| "command": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" |
| { | ||
| "type": "command", | ||
| "command": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; cd \"$root\" || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'", | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; cd \"$root\" || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'", |
| { | ||
| "type": "command", | ||
| "command": ".goat-flow/hooks/deny-dangerous.sh", | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'", |
| { | ||
| "type": "command", | ||
| "command": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" |
| { | ||
| "type": "command", | ||
| "command": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; cd \"$root\" || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'", | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; cd \"$root\" || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'", |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/audit/harness/check-context.ts (1)
473-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't return PASS when
checkAllDocPaths()already recorded a missing architecture doc.
collectArchitectureDocPaths()can add"architecture.md does not exist"tofindings, but this branch still returns pass whenever the remaining path counts balance. A repo with valid router/core refs and no.goat-flow/architecture.mdwill therefore reportdoc-paths-resolveas green. Split hard failures from informational messages, or carry an explicit failure bit through the accumulator.🤖 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 `@src/cli/audit/harness/check-context.ts` around lines 473 - 505, checkAllDocPaths currently returns findings that may include hard failures (e.g. missing architecture.md) but the caller in check-context.ts only uses counts to decide PASS; update the contract so checkAllDocPaths returns an explicit failure bit (e.g. hasCriticalFailure or hardFailure) alongside totalPaths/resolvedCount/findings/unresolved, set that bit inside collectArchitectureDocPaths when it records "architecture.md does not exist", and then in the caller (the function in check-context.ts that currently branches on totalPaths/resolvedCount) short-circuit to return fail(...) whenever hasCriticalFailure is true (using the existing findings/details) so missing architecture.md is treated as a hard failure even if counts balance..goat-flow/learning-loop/lessons/hook-testing.md (1)
112-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the lesson heading with the updated guidance.
Line 112 states a blanket “should not depend on shell substitution,” but Line 120 now documents an approved
bash -c+git rev-parseshape. Rename the heading so it matches the new policy and avoids future misreads.Suggested wording update
-## Lesson: Codex hook commands should not depend on shell substitution +## Lesson: Codex hook commands must resolve repo root reliably across nested cwd🤖 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 @.goat-flow/learning-loop/lessons/hook-testing.md around lines 112 - 121, Rename the lesson heading "Lesson: Codex hook commands should not depend on shell substitution" to reflect the narrowed policy that allows a specific safe shape; update it to something like "Lesson: Codex hook commands must use a safe git-root resolving wrapper (allow bash -c + git rev-parse)" so the heading aligns with the body that documents the approved `bash -c` + `git rev-parse --show-toplevel` wrapper and avoids implying a blanket ban on all shell substitution.
🧹 Nitpick comments (3)
workflow/hooks/deny-dangerous/patterns-writes.sh (1)
271-296: 💤 Low valueConsolidate duplicated scan variable (mirror of .goat-flow/ script).
Lines 271 and 289 both derive an identically transformed value from
CMD_TRIMMED. Consider using a singlelocal scan="${CMD_TRIMMED//||/__GOAT_OR__}"variable for both the git and gh loops to reduce duplication. This mirrors the same optional refactor suggested for.goat-flow/hooks/deny-dangerous/patterns-writes.sh.🤖 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 `@workflow/hooks/deny-dangerous/patterns-writes.sh` around lines 271 - 296, The code duplicates the same transformed value of CMD_TRIMMED into two variables (repo_scan and gh_scan); replace both with a single local scan variable (e.g., local scan="${CMD_TRIMMED//||/__GOAT_OR__}") and reuse that variable in the git loop (where normalize_git_policy_candidate, is_git_push, is_git_commit, is_git_destructive and block are used) and in the GH loop (where is_gh_write_operation and block are used) to eliminate duplication while preserving existing logic and returns..goat-flow/hooks/deny-dangerous/patterns-writes.sh (1)
271-296: 💤 Low valueConsolidate duplicated scan variable.
Lines 271 and 289 both derive an identically transformed value from
CMD_TRIMMED. Consider using a singlelocal scan="${CMD_TRIMMED//||/__GOAT_OR__}"variable for both the git and gh loops to reduce duplication.♻️ Proposed consolidation
- local repo_scan="${CMD_TRIMMED//||/__GOAT_OR__}" + local scan="${CMD_TRIMMED//||/__GOAT_OR__}" local -a pipe_parts local pipe_part - IFS='|' read -ra pipe_parts <<< "$repo_scan" + IFS='|' read -ra pipe_parts <<< "$scan" for pipe_part in "${pipe_parts[@]}"; do local git_candidate git_candidate=$(normalize_git_policy_candidate "$pipe_part") if is_git_push "$git_candidate"; then block "git push is not allowed. Ask the user to push manually." || return $? fi if is_git_commit "$git_candidate"; then block "git commit is not allowed. Ask the user to commit manually." || return $? fi if is_git_destructive "$git_candidate"; then block "Destructive git operation (--no-verify / reset --hard / clean -f). Remove the flag, stash first, or run manually." || return $? fi done - local gh_scan="${CMD_TRIMMED//||/__GOAT_OR__}" local -a gh_pipe_parts - IFS='|' read -ra gh_pipe_parts <<< "$gh_scan" + IFS='|' read -ra gh_pipe_parts <<< "$scan" for pipe_part in "${gh_pipe_parts[@]}"; do🤖 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 @.goat-flow/hooks/deny-dangerous/patterns-writes.sh around lines 271 - 296, The code duplicates the same transformed CMD_TRIMMED into two variables (repo_scan and gh_scan); consolidate by creating a single local variable (e.g., scan="${CMD_TRIMMED//||/__GOAT_OR__}") and reuse it in both loops, replacing references to repo_scan and gh_scan with scan; ensure you still create separate arrays (pipe_parts / gh_pipe_parts or reuse a single array by re-reading into it before the second loop), preserve IFS='|' read -ra usage, and keep calls to normalize_git_policy_candidate, is_git_push, is_git_commit, is_git_destructive, is_gh_write_operation and block unchanged so behavior remains identical.workflow/install-goat-flow.sh (1)
733-750: ⚡ Quick winReuse
rootResolvingCommand()for the printed Claude recovery snippet.The install path now centralizes resolver construction in
rootResolvingCommand(), but Line 1796 still embeds a second copy of the same shell wrapper. The next resolver tweak can update the migrator while leaving the manual recovery guidance stale again.Also applies to: 1794-1796
🤖 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 `@workflow/install-goat-flow.sh` around lines 733 - 750, The Claude recovery snippet still duplicates the shell wrapper; replace the inlined shell snippet used for printed/manual recovery with a call to rootResolvingCommand(script) so the same resolver logic (including the claudeRootFallback and hookUnavailableCommand behavior) is reused; locate the print/migrator code that currently inlines the bash wrapper for the same hook script and substitute it to use the rootResolvingCommand() return value (ensure you pass the same script identifier used there and preserve any surrounding quoting/escaping for embedding in printed output).
🤖 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 @.goat-flow/hooks/deny-dangerous.sh:
- Around line 85-89: The repository's mirrored deny-dangerous hooks contain
several functions (e.g., find_matching_shell_paren, check_command_substitutions,
__goat_git_strip_globals) that ShellCheck reports as "never invoked" (SC2329);
to fix, either remove truly unused helper functions from
.goat-flow/hooks/deny-dangerous.sh, or if they are intentionally kept for
future/externally-invoked use, annotate each such function with an inline
ShellCheck suppression comment (e.g., # shellcheck disable=SC2329) or add a
small internal dispatcher that calls them so they are recognized as used; update
the file so ShellCheck exits zero while keeping the intended behavior and ensure
you reference the exact function names above when making edits.
In @.goat-flow/hooks/gruff-code-quality.sh:
- Around line 292-304: In payload_supported_file_paths, after normalizing
slashes, detect Windows drive-letter absolute paths (regex like ^[A-Za-z]:/) and
treat them as absolute instead of falling through to the repo-relative branch:
either reject them early (skip/continue) or strip/normalize them into a form
comparable to normalized_root before calling relative_path; update the logic
that currently falls into the *) branch so relative_path() is never called for
drive-letter paths and mirror the same change in
workflow/hooks/gruff-code-quality.sh and any callers such as process_file.
In `@src/cli/facts/fs.ts`:
- Around line 197-204: createPathResolver currently calls resolve(root,
relativePath) but allows absolute inputs and ../ traversal to escape the project
root; change resolveProjectPath to first canonicalize the root (rootResolved =
resolve(root)), then compute resolved = resolve(rootResolved, relativePath) and
verify the resolved path is still inside root by using
path.relative(rootResolved, resolved) (reject when it starts with '..' or equals
'.' escape) — if the path escapes, return a sentinel that makes callers treat
the file as absent (e.g., empty string or null) instead of returning a path
outside the root; update callers of ResolvePath (ctx.fs.exists, ctx.fs.read,
ctx.fs.list) to treat that sentinel as “not found” so escaped or absolute inputs
are rejected rather than followed.
In `@src/dashboard/dashboard-app-data-loading-fragments.ts`:
- Around line 206-209: The catch currently always toasts errors, which can
surface stale failures after the user switches workspaces; capture the request's
identifying state (e.g., projectPath, runner, generation) when the inventory
load starts and, in the catch block, compare those captured values to the
current context before calling ctx.showToast(err...). Only call ctx.showToast if
the captured projectPath/runner/generation still match the current context (or
use a per-request token/ID that you compare), so late failures for a previous
request are ignored.
In `@src/dashboard/dashboard-app-project-terminal-fragments.ts`:
- Around line 50-78: In dashboardRunSkillEvaluator capture the current
ctx.projectPath (e.g., const requestedPath = ctx.projectPath) before making the
fetch, and after any awaited operations (after
res.json()/readRecord/readErrorMessage or in the catch) verify that
ctx.projectPath === requestedPath before assigning to ctx.skillEvaluatorResult,
ctx.skillEvaluatorError, or ctx.skillEvaluatorLoading; if they differ, abort
applying the response (do not overwrite state) so results from a previous
project don't leak into the current evaluator pane. Ensure this check is applied
in both the success branch and the catch/finally handling.
In `@test/unit/dashboard-terminal-launch/fakes.ts`:
- Around line 80-82: The fake-clock requeues intervals at state.now +
timer.intervalMs which, when timer.intervalMs is 0 (because callers pass ms ??
0), re-schedules them at the same timestamp and lets tick() spin forever; change
the reschedule logic in the state.timers.set call to enforce a minimum positive
delay (e.g., nextAt = state.now + Math.max(1, timer.intervalMs)) and also ensure
the place that passes ms ?? 0 uses a minimum of 1 for interval scheduling so
zero-delay intervals become 1ms instead of 0ms.
In `@test/unit/hook-registrar.test.ts`:
- Around line 270-279: The current assertions using
claudeGruffCommands.every(...) can vacuously pass when claudeGruffCommands is
empty; add an explicit non-empty check before those .every checks (e.g.,
assert.ok(claudeGruffCommands.length > 0) or
assert.notStrictEqual(claudeGruffCommands.length, 0)) so the test fails if no
gruff commands were produced, then keep the existing asserts that each command
includes "gruff-code-quality: hook unavailable" and that none include "BLOCKED:
Policy hook unavailable".
In `@workflow/hooks/agent-config/claude.json`:
- Line 62: The current inline shell falls back to CLAUDE_PROJECT_DIR whenever
the hook file is missing, which can run a policy from another project; modify
the shell logic so you first attempt git rev-parse to set root, only if that
fails set root="${CLAUDE_PROJECT_DIR:-}", and after selecting the root strictly
check for the presence of "$root/.goat-flow/hooks/deny-dangerous.sh" (do not
re-run the fallback when the hook is absent). Update the command string that
references git rev-parse, CLAUDE_PROJECT_DIR, and deny-dangerous.sh to:
determine root via git rev-parse || use CLAUDE_PROJECT_DIR, then if the hook
file is missing print the BLOCKED error and exit (no further fallback), cd to
$root and execute .goat-flow/hooks/deny-dangerous.sh.
---
Outside diff comments:
In @.goat-flow/learning-loop/lessons/hook-testing.md:
- Around line 112-121: Rename the lesson heading "Lesson: Codex hook commands
should not depend on shell substitution" to reflect the narrowed policy that
allows a specific safe shape; update it to something like "Lesson: Codex hook
commands must use a safe git-root resolving wrapper (allow bash -c + git
rev-parse)" so the heading aligns with the body that documents the approved
`bash -c` + `git rev-parse --show-toplevel` wrapper and avoids implying a
blanket ban on all shell substitution.
In `@src/cli/audit/harness/check-context.ts`:
- Around line 473-505: checkAllDocPaths currently returns findings that may
include hard failures (e.g. missing architecture.md) but the caller in
check-context.ts only uses counts to decide PASS; update the contract so
checkAllDocPaths returns an explicit failure bit (e.g. hasCriticalFailure or
hardFailure) alongside totalPaths/resolvedCount/findings/unresolved, set that
bit inside collectArchitectureDocPaths when it records "architecture.md does not
exist", and then in the caller (the function in check-context.ts that currently
branches on totalPaths/resolvedCount) short-circuit to return fail(...) whenever
hasCriticalFailure is true (using the existing findings/details) so missing
architecture.md is treated as a hard failure even if counts balance.
---
Nitpick comments:
In @.goat-flow/hooks/deny-dangerous/patterns-writes.sh:
- Around line 271-296: The code duplicates the same transformed CMD_TRIMMED into
two variables (repo_scan and gh_scan); consolidate by creating a single local
variable (e.g., scan="${CMD_TRIMMED//||/__GOAT_OR__}") and reuse it in both
loops, replacing references to repo_scan and gh_scan with scan; ensure you still
create separate arrays (pipe_parts / gh_pipe_parts or reuse a single array by
re-reading into it before the second loop), preserve IFS='|' read -ra usage, and
keep calls to normalize_git_policy_candidate, is_git_push, is_git_commit,
is_git_destructive, is_gh_write_operation and block unchanged so behavior
remains identical.
In `@workflow/hooks/deny-dangerous/patterns-writes.sh`:
- Around line 271-296: The code duplicates the same transformed value of
CMD_TRIMMED into two variables (repo_scan and gh_scan); replace both with a
single local scan variable (e.g., local scan="${CMD_TRIMMED//||/__GOAT_OR__}")
and reuse that variable in the git loop (where normalize_git_policy_candidate,
is_git_push, is_git_commit, is_git_destructive and block are used) and in the GH
loop (where is_gh_write_operation and block are used) to eliminate duplication
while preserving existing logic and returns.
In `@workflow/install-goat-flow.sh`:
- Around line 733-750: The Claude recovery snippet still duplicates the shell
wrapper; replace the inlined shell snippet used for printed/manual recovery with
a call to rootResolvingCommand(script) so the same resolver logic (including the
claudeRootFallback and hookUnavailableCommand behavior) is reused; locate the
print/migrator code that currently inlines the bash wrapper for the same hook
script and substitute it to use the rootResolvingCommand() return value (ensure
you pass the same script identifier used there and preserve any surrounding
quoting/escaping for embedding in printed output).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1f7520c-7110-4015-adc0-cab792ff1bb6
📒 Files selected for processing (78)
.agents/hooks.json.agents/skills/goat-critique/SKILL.md.claude/settings.json.claude/skills/goat-critique/SKILL.md.codex/hooks.json.github/skills/goat-critique/SKILL.md.goat-flow/hooks/deny-dangerous.sh.goat-flow/hooks/deny-dangerous/deny-dangerous-self-test.sh.goat-flow/hooks/deny-dangerous/patterns-shell.sh.goat-flow/hooks/deny-dangerous/patterns-writes.sh.goat-flow/hooks/gruff-code-quality.sh.goat-flow/learning-loop/decisions/ADR-032-scope-gruff-hook-binary-discovery.md.goat-flow/learning-loop/footguns/auditor.md.goat-flow/learning-loop/footguns/deny-dangerous.md.goat-flow/learning-loop/footguns/hooks.md.goat-flow/learning-loop/lessons/dashboard-testing.md.goat-flow/learning-loop/lessons/gruff-cleanup.md.goat-flow/learning-loop/lessons/hook-testing.md.goat-flow/learning-loop/lessons/verification.md.gruff-ts.yamlCHANGELOG.mdscripts/check-instruction-parity.mjsscripts/preflight-checks.shscripts/profile-dashboard-audit.mjssrc/cli/audit/check-agent-deny-mechanism.tssrc/cli/audit/harness/check-context.tssrc/cli/facts/fs.tssrc/cli/server/agent-hook-writer.tssrc/cli/server/dashboard-audit-routes.tssrc/cli/server/safe-exec.tssrc/dashboard/app.tssrc/dashboard/dashboard-app-data-loading-fragments.tssrc/dashboard/dashboard-app-init.tssrc/dashboard/dashboard-app-project-terminal-fragments.tssrc/dashboard/dashboard-app-prompts-audit-fragments.tssrc/dashboard/dashboard-app-skill-quality-fragments.tssrc/dashboard/dashboard-app-state-fragments.tssrc/dashboard/dashboard-model-readers.tssrc/dashboard/dashboard-prompts.tssrc/dashboard/dashboard-readers.tstest/fixtures/projects/index.tstest/integration/audit-drift-checkdrift-hook-templates.test.tstest/integration/gruff-code-quality-smoke.test.tstest/integration/path-integrity.test.tstest/integration/setup-install-codex-config-migration.test.tstest/integration/setup-install.test.tstest/integration/skill-author.test.tstest/smoke/dashboard-endpoints.test.tstest/src.tstest/unit/agent-config-template-parity.test.tstest/unit/audit-command/agent-deny-hooks.test.tstest/unit/audit-command/helpers.tstest/unit/audit-command/hook-facts.test.tstest/unit/audit-command/main.test.tstest/unit/audit-harness/check-evidence-before-claims.test.tstest/unit/check-content-quality.test.tstest/unit/config-writer.test.tstest/unit/dashboard-home.test.tstest/unit/dashboard-readers.test.tstest/unit/dashboard-reporting.test.tstest/unit/dashboard-terminal-launch/fakes.tstest/unit/dashboard-terminal-launch/launch-flow-06.test.tstest/unit/hook-registrar.test.tstest/unit/local-paths.test.tstest/unit/skill-quality/helpers.tstest/unit/skill-quality/main.test.tstest/unit/terminal-uploads.test.tsworkflow/hooks/README.mdworkflow/hooks/agent-config/antigravity-hooks.jsonworkflow/hooks/agent-config/claude.jsonworkflow/hooks/agent-config/codex-hooks.jsonworkflow/hooks/deny-dangerous.shworkflow/hooks/deny-dangerous/deny-dangerous-self-test.shworkflow/hooks/deny-dangerous/patterns-shell.shworkflow/hooks/deny-dangerous/patterns-writes.shworkflow/hooks/gruff-code-quality.shworkflow/install-goat-flow.shworkflow/skills/goat-critique/SKILL.md
Comment on lines
+85
to
89
| local top_level | ||
| top_level="$(git rev-parse --show-toplevel 2>/dev/null)" || return 1 | ||
| [[ -n "$top_level" ]] || return 1 | ||
| printf '%s\n' "$top_level" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
shellcheck .goat-flow/hooks/deny-dangerous.sh workflow/hooks/deny-dangerous.shRepository: blundergoat/goat-flow
Length of output: 2332
Run ShellCheck on mirrored deny-dangerous hooks (and note current findings)
ShellCheck was run on both .goat-flow/hooks/deny-dangerous.sh and workflow/hooks/deny-dangerous.sh, but it exited non-zero due to SC2329 “function is never invoked” (e.g., find_matching_shell_paren, check_command_substitutions, __goat_git_strip_globals, etc.).
🤖 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 @.goat-flow/hooks/deny-dangerous.sh around lines 85 - 89, The repository's
mirrored deny-dangerous hooks contain several functions (e.g.,
find_matching_shell_paren, check_command_substitutions,
__goat_git_strip_globals) that ShellCheck reports as "never invoked" (SC2329);
to fix, either remove truly unused helper functions from
.goat-flow/hooks/deny-dangerous.sh, or if they are intentionally kept for
future/externally-invoked use, annotate each such function with an inline
ShellCheck suppression comment (e.g., # shellcheck disable=SC2329) or add a
small internal dispatcher that calls them so they are recognized as used; update
the file so ShellCheck exits zero while keeping the intended behavior and ensure
you reference the exact function names above when making edits.
Source: Coding guidelines
Comment on lines
+197
to
+204
| /** Build the resolver that anchors all fact reads under the selected project root. */ | ||
| function createPathResolver(root: string): ResolvePath { | ||
| /** Resolve one caller-supplied relative path under the adapter root. */ | ||
| function resolveProjectPath(relativePath: string): string { | ||
| return resolve(root, relativePath); | ||
| } | ||
| return resolveProjectPath; | ||
| } |
There was a problem hiding this comment.
createPathResolver() does not actually confine reads to the audited root.
resolve(root, relativePath) still accepts absolute inputs and ../ traversal. In this PR, check-context.ts feeds project-authored doc/router refs into ctx.fs.exists(...), so a malicious workspace can probe files outside the selected project instead of being sandboxed to it. Please reject escaped paths and keep the read/exists/list helpers treating them as absent rather than following them.
🤖 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 `@src/cli/facts/fs.ts` around lines 197 - 204, createPathResolver currently
calls resolve(root, relativePath) but allows absolute inputs and ../ traversal
to escape the project root; change resolveProjectPath to first canonicalize the
root (rootResolved = resolve(root)), then compute resolved =
resolve(rootResolved, relativePath) and verify the resolved path is still inside
root by using path.relative(rootResolved, resolved) (reject when it starts with
'..' or equals '.' escape) — if the path escapes, return a sentinel that makes
callers treat the file as absent (e.g., empty string or null) instead of
returning a path outside the root; update callers of ResolvePath (ctx.fs.exists,
ctx.fs.read, ctx.fs.list) to treat that sentinel as “not found” so escaped or
absolute inputs are rejected rather than followed.
| { | ||
| "type": "command", | ||
| "command": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" |
There was a problem hiding this comment.
Fallback currently triggers on missing hook, not only missing git root.
Line 62 falls back to CLAUDE_PROJECT_DIR whenever $root/.goat-flow/hooks/deny-dangerous.sh is absent. That can execute policy from a different project root instead of failing closed for the active checkout.
Suggested fix
- "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'"
+ "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; if [ -z \"$root\" ]; then root=\"${CLAUDE_PROJECT_DIR:-}\"; fi; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" | |
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; if [ -z \"$root\" ]; then root=\"${CLAUDE_PROJECT_DIR:-}\"; fi; [ -f \"$root/.goat-flow/hooks/deny-dangerous.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/deny-dangerous.sh\"'" |
🤖 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 `@workflow/hooks/agent-config/claude.json` at line 62, The current inline shell
falls back to CLAUDE_PROJECT_DIR whenever the hook file is missing, which can
run a policy from another project; modify the shell logic so you first attempt
git rev-parse to set root, only if that fails set
root="${CLAUDE_PROJECT_DIR:-}", and after selecting the root strictly check for
the presence of "$root/.goat-flow/hooks/deny-dangerous.sh" (do not re-run the
fallback when the hook is absent). Update the command string that references git
rev-parse, CLAUDE_PROJECT_DIR, and deny-dangerous.sh to: determine root via git
rev-parse || use CLAUDE_PROJECT_DIR, then if the hook file is missing print the
BLOCKED error and exit (no further fallback), cd to $root and execute
.goat-flow/hooks/deny-dangerous.sh.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e53df2e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SKIP_DIR_PATTERN='(^|/)(node_modules|vendor|\.goat-flow|dist|build|coverage|\.git|target|\.venv|\.mypy_cache|\.pytest_cache|\.ruff_cache)(/|$)' | ||
| GRUFF_CODE_QUALITY_TIMEOUT_SECONDS="${GRUFF_CODE_QUALITY_TIMEOUT_SECONDS:-30}" | ||
| BINARY_SEARCH_PATHS='vendor/bin, node_modules/.bin, bin, .venv/bin, ~/.local/bin, PATH' | ||
| GRUFF_CODE_QUALITY_TIMEOUT_SECONDS="${GRUFF_CODE_QUALITY_TIMEOUT_SECONDS:-60}" |
There was a problem hiding this comment.
When this hook is installed through the .agents/Antigravity config, the hook entry still has "timeout": 30 while the analyzer now defaults to 60 seconds. For analyzers that need 31–60 seconds, the runner kills the wrapper before run_gruff_json can reach its own timeout path and print the actionable diagnostic, so the new timeout is ineffective and the edit gets an abrupt hook timeout instead. Either keep the default within the registered hook timeout or bump the generated hook timeout in the installed/template configs at the same time.
Useful? React with 👍 / 👎.
…on and configuration files
…cumentation files
Comment on lines
98
to
101
| /** Cast this focused fake to the WebSocket subset TerminalManager consumes. */ | ||
| asTerminalSocket(): TerminalWebSocket { | ||
| return this as unknown as TerminalWebSocket; | ||
| return this as TerminalWebSocket; | ||
| } |
Comment on lines
+44
to
+48
| const entries = parseBucket(fs, dirPath, bucket); | ||
| writeFileSync( | ||
| join(projectPath, indexRelPath), | ||
| formatIndex(bucket, entries), | ||
| ); |
Comment on lines
1
to
4
| { | ||
| "name": "@blundergoat/goat-flow", | ||
| "version": "1.10.1", | ||
| "version": "1.11.0", | ||
| "description": "AI coding agent harness and local dashboard for Claude Code, OpenAI Codex, Google Antigravity, and GitHub Copilot - setup audits, guardrails, structured skills, deny hooks, and persistent learning loops.", |
…meout adjustments
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fc944aba8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| c=$(normalize_command_candidate "$1") | ||
|
|
||
| local xargs_rest="" | ||
| if xargs_rest=$(strip_xargs_prefix "$c"); then |
There was a problem hiding this comment.
When a repository write is wrapped with xargs -a/--arg-file, this new normalization trusts strip_xargs_prefix, but that parser treats -a as a valueless option; GNU xargs --help shows -a, --arg-file=FILE consumes the next operand. As a result xargs -a msgs git commit -F - normalizes to msgs git commit -F -, so the newly added is_git_commit/destructive checks do not see git and the deny hook exits 0, letting the exact xargs-wrapped git writes this change is trying to block slip through.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85599188f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local pipe_index | ||
| IFS='|' read -ra pipeline_parts <<< "$pipe_scan" | ||
| for ((pipe_index = 0; pipe_index < ${#pipeline_parts[@]}; pipe_index++)); do | ||
| check_xargs_destructive_payload "${pipeline_parts[$pipe_index]}" || return $? |
There was a problem hiding this comment.
When the new per-pipeline xargs check handles a segment such as printf x | xargs -a targets rm -rf, it delegates to strip_xargs_payload_command, but that parser still treats -a as a flag without an operand; xargs --help documents -a, --arg-file=FILE as reading arguments from a file. The normalized payload becomes targets rm -rf, so rm_has_recursive never sees rm, and I reproduced the hook exiting 0 for that command while blocking the same shape without -a. This leaves an easy bypass for the destructive xargs pipeline this change is trying to catch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1d2bdc1. Configure here.
| { | ||
| "type": "command", | ||
| "command": ".goat-flow/hooks/gruff-code-quality.sh", | ||
| "command": "bash -c 'root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\"; [ -f \"$root/.goat-flow/hooks/gruff-code-quality.sh\" ] || { printf '\\''gruff-code-quality: hook unavailable: git repository root or hook script unavailable; skipped.\\n'\\'' >&2; exit 0; }; cd \"$root\" || { printf '\\''gruff-code-quality: hook unavailable: git repository root or hook script unavailable; skipped.\\n'\\'' >&2; exit 0; }; bash \"$root/.goat-flow/hooks/gruff-code-quality.sh\"'", |
There was a problem hiding this comment.
Codex gruff hook lacks timeout
Low Severity
PostToolUse gruff-code-quality hooks were raised to a 90s timeout in .agents/hooks.json, .claude/settings.json, and .github/hooks/hooks.json, but .codex/hooks.json still omits a timeout on the same wrapper commands. Codex may kill the hook earlier than other runtimes during slower analyzer runs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1d2bdc1. Configure here.
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
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.
This pull request simplifies and improves the reliability of policy hook resolution logic across several configuration files and test scripts. The main focus is on removing legacy logic for determining the repository root (especially for submodules and alternate worktrees), standardizing error handling, and ensuring that policy checks correctly use the active worktree's policy store. It also updates documentation for clarity.
Policy hook resolution and error handling:
.goat-flow/hooks/deny-dangerous.shby removing thegit-common-dir-based resolution and always usinggit rev-parse --show-toplevel, ensuring the active worktree's policy store is used. (.goat-flow/hooks/deny-dangerous.sh).codex/hooks.json,.agents/hooks.json, and.claude/settings.jsonto use the new simplified root resolution logic, with standardized error and skip messages for missing policy hooks. (.codex/hooks.json,.agents/hooks.json,.claude/settings.json) [1] [2] [3] [4] [5] [6] [7] [8]Test improvements:
.goat-flow/hooks/deny-dangerous/deny-dangerous-self-test.shto match the new resolution logic, removing tests forgit-common-dirand adding new tests that verify the active worktree's policy store is used, including for real linked worktrees. (.goat-flow/hooks/deny-dangerous/deny-dangerous-self-test.sh) [1] [2] [3] [4] [5]Documentation updates:
.github/skills/goat-critique/SKILL.md,.claude/skills/goat-critique/SKILL.md,.agents/skills/goat-critique/SKILL.md)These changes improve maintainability, ensure correct policy enforcement in complex git worktree scenarios, and provide clearer documentation for users and developers.
Note
Medium Risk
Changes affect PreToolUse/PostToolUse policy hooks and command-blocking rules (agent safety surface), but are harness-focused with expanded self-tests rather than production app logic.
Overview
v1.11.0 bumps the harness and mirrored agent skills from 1.10.1 to 1.11.0, with the largest behavioral changes in policy hooks and learning-loop retrieval.
Hooks: Pre/post-tool hook launchers in
.agents,.claude,.codex, and.githubnow resolve the repo withgit rev-parse --show-toplevelinstead ofgit-common-dirheuristics, so linked worktrees use that worktree’s.goat-flowpolicy store.gruff-code-qualitymissing-hook handling skips (exit 0) with a stderr note instead of blocking edits; timeout rises to 90s.deny-dangerousdrops submodule-specific root logic; tests add real linked-worktree coverage. Shell/write guards tighten pipedxargs/rm,xargs -a/--arg-file, and pipedgit commit/reset/clean; backtick-substitution messaging is clearer.gruff-code-quality.sh: Default timeout 60s, per-languageGRUFF_*_BIN/GRUFF_*_TIMEOUT_SECONDS, stderr when config exists but the binary is missing, safer payload path filtering (no git-wide scan when the edit names only non-analyzable paths), and richer JSON config-error handling. ADR-032 documents explicit env overrides for monorepo layouts.Learning loop: Skills and Copilot instructions switch from grep-first buckets to INDEX-first reads of generated
INDEX.mdfiles;goat-reviewfootgun cross-check targetsfootguns/INDEX.md. ADR-035 and regenerateddecisions/INDEX.md(and related index machinery in the diff) formalizegoat-flow index+index-freshstaleness checks. Minor goat-critique doc fix: SKEPTIC/ANALYST/STRATEGIST naming.Docs/config:
.github/copilot-instructions.mdadds Small Feature complexity budget and updates stated audit check counts;config.yamlversion aligns with 1.11.0.Reviewed by Cursor Bugbot for commit 1d2bdc1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation