Skip to content

fix(fp-check): scope hooks and make Stop path-aware - #190

Closed
GotenJBZ wants to merge 1 commit into
trailofbits:mainfrom
GotenJBZ:fix/fp-check-hooks-skill-frontmatter
Closed

fix(fp-check): scope hooks and make Stop path-aware#190
GotenJBZ wants to merge 1 commit into
trailofbits:mainfrom
GotenJBZ:fix/fp-check-hooks-skill-frontmatter

Conversation

@GotenJBZ

@GotenJBZ GotenJBZ commented Jun 24, 2026

Copy link
Copy Markdown

Fixes #143. Supersedes #188.

Problems (in main)

  1. Stop and SubagentStop both use matcher: "*", so they run on every subagent/agent stop in any session where fp-check is installed.
  2. The Stop prompt demands the full deep-path output (5 phases, 13 questions, full PoC) for every bug. Standard-path verifications are rejected as "incomplete".
  3. Hook loops in the main agent when running fp-check using subagents/ultracode.

Changes

  1. Stop hook moved into skill frontmatter. This way, the stop hooks will be executed only when the skill is loaded, avoiding the firing in unrelated sessions.
  2. Changed the Stop hook prompt as ordered rules: (1) skip if not fp-check (2) skip if delegated to subagents/workflow, (3) check only the path actually used (standard vs deep)
  3. SubagentStop remains in hooks.json, narrowing the matcher to only support specific subagents data-flow-analyzer|exploitability-verifier|poc-builder

@CLAassistant

CLAassistant commented Jun 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@GotenJBZ
GotenJBZ marked this pull request as ready for review June 24, 2026 15:19
@GotenJBZ
GotenJBZ requested review from ahpaleus and dguido as code owners June 24, 2026 15:19

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@GotenJBZ

GotenJBZ commented Jun 24, 2026

Copy link
Copy Markdown
Author

Adding as a reviewer both @remiforall and @computerality since they both tried to solve this exact problem.

EDIT: I cannot do that :ops:

@dguido

dguido commented Jul 29, 2026

Copy link
Copy Markdown
Member

Thanks for this @GotenJBZ — the diagnosis is right on all three counts, and #205 now tracks the part that isn't covered elsewhere.

Closing in favour of #188, and I want to be straight about why, because it isn't "ours over yours":

Where #188 goes further. Your PR narrows the SubagentStop matcher to data-flow-analyzer|exploitability-verifier|poc-builder. #188 deletes plugins/fp-check/hooks/hooks.json entirely and moves each completeness check into the frontmatter of the agent it belongs to. Same insight as yours — frontmatter scoping — pushed one step further: with no hooks file there is no global matcher left to get wrong later.

Where your PR goes further. #188 moves the Stop prompt but does not change it, so your problem 2 is real and still open: a standard-path verification is still measured against the deep path and reported incomplete. That is #205, with your rule ordering quoted and credited.

Why #188 goes first. It is CI-validated and bumps 1.0.2 → 1.0.3 in both plugin.json and marketplace.json. #190 has run only the CLA check (fork PRs need maintainer approval to run workflows) and carries no version bump, so clients would not receive it even if merged.

If you want to take #205, it's yours — and it's the more interesting half of the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fp-check Stop hook with matcher "*" fires on every unrelated session, causing forced LLM turns

3 participants