Conversation
![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: 52a5d0e473
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a8e25ec to
9a26968
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a26968538
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef5944eda
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Pushed
I audited the registered state-machine operations that can terminate a tool request against the legacy path. Recipe final output, skill loading, and unknown-tool handling now use the shared lifecycle where required. The regression tests cover Chat mode skips, hook denials, inactive final output, unknown shell and read calls, approval-pending siblings, and final-output ordering.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a63b20c2b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
53823e1 to
cf9f625
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf9f62584d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Adds the shared types and state both agent loops need to report what a
PreToolUse hook chain decided.
- `HookEvent::PreToolUseResult`, an observation-only event.
- `HookContext::tool_call_id`, the same value goose records as
`gen_ai.tool.call.id`. Tool name plus input cannot correlate the pre and
post events of one call when that call repeats in a session; this can.
- `HookContext::{decision, policy_evaluated, blocked_by, reason}`, all
skipped during serialization when unset, so no existing event payload
changes shape.
- `HookChainOutcome` and `emit_blocking_with_outcome`, which report whether
any matching hook ran to completion. `emit_blocking` keeps its exact
public signature and delegates, so no external caller is affected.
`policy_evaluated` is true only when a hook process was spawned and awaited
to completion. A hook that fails to spawn, times out, or exits non-zero
without denying hits the `continue` arm and does not count as evaluated.
Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
…loop Threads the request id, which is the value recorded as `gen_ai.tool.call.id`, into the PreToolUse, PostToolUse and PostToolUseFailure payloads, and emits the new PreToolUseResult event once the PreToolUse chain has completed. The event is emitted before the denial returns, so an observer sees the denial before the model receives the refusal. It is best effort like every other hook emission: awaited, guarded by `has_hooks`, and returning early before the payload is built when nothing subscribes. It carries no veto. A denied call dispatches nothing and fires neither post event, so a PreToolUse subscriber sees the request and a PostToolUse subscriber sees nothing; until now no subscriber could observe the outcome itself. Adds `HookContext::with_pre_tool_use_outcome` here rather than alongside the shared types, so the builder lands with its first production caller, plus a unit test pinning the payload: `decision` is "allow" or "deny" and nothing else, and `blocked_by` and `reason` are absent from an allow payload rather than null or empty. Behaviour tests cover the denial an observer could not otherwise see, correlation of two identical calls by id, and a registered but non-matching rule reporting allow with policy_evaluated false. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
…achine Mirrors the legacy loop change in the state machine path, which AGENTS.md requires until the agent loop migration completes. Without it the event would silently not fire under GOOSE_STATE_MACHINE=1. Same ordering, same best-effort emission, same three tests. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
…te machine RecipeOperation is registered ahead of ToolExecutionOperation, so it picks up a pending `recipe__final_output` request and executes it itself. That path never reached the hook wrapper, so in the state-machine loop a final-output call emitted no PreToolUse, no PreToolUseResult and no post-tool event, while the legacy loop emitted all three. The both-loop parity claim did not hold for it. Lifts the lifecycle out of ToolExecutionOperation into three functions in ops_toolcalling and calls them from RecipeOperation: - `run_pre_tool_hooks` runs the PreToolUse chain, emits PreToolUseResult on both the allow and the deny path, and returns the denial as the error the caller must return instead of executing. - `emit_post_tool_use` classifies the outcome and emits the post event. - `with_post_tool_hooks` keeps the streaming wrapper for the ordinary tool path. Sharing rather than copying keeps the policy in one place: what blocks, what `policy_evaluated` means, and which outcome counts as a failure. Behaviour for ordinary tool calls is unchanged; `emit_blocking` keeps its public signature. Two deliberate differences on the recipe path, both matching the tool path: - A denial returns before execution and emits no post event, the same shape dispatch_tool_call has, since it returns the denial before the post wrapper is applied. - The large-response rewrite is not applied to the final-output result. The recipe's structured output is the deliverable, not a payload to offload to a temp file, and adding that here would be a behaviour change beyond this fix. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
…e machine Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
The duplicate-call test sorted the recorded lifecycle ids, which proved membership while erasing execution order, and execution order is what decides which call wins. It now asserts the recorded order directly and pins postfail.log empty, since both calls succeed. The malformed-call test re-prompted to the turn cap and reached compaction, so a focused routing test depended on unrelated summarisation behaviour. It is bounded by max_turns instead, and the summarise rule it needed is gone. Two comments now say strict providers can reject an unmatched tool request. We have evidence of the risk, not a universal guarantee. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
…guide Adds PreToolUseResult to the Supported Events table and the new payload fields (tool_call_id, decision, policy_evaluated, blocked_by, reason) to the payload table. Addresses the review note that the guide's exhaustive tables omitted the new contract. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
policy_evaluated told a PreToolUseResult subscriber that a matching hook had evaluated the call whenever the hook process was reaped, including a non-zero exit that carried no decision. A hook that crashed or refused to answer was therefore reported as having made a policy judgement. A hook now counts as evaluated only when it exited 0 or returned a decision, which is exit 2 or a block object on stdout. A non-zero exit with no decision, a spawn failure, a timeout, and a hook that was never reached all leave it false. The value stays an at-least-one aggregate, so a hook that did evaluate keeps it true when a later hook in the chain fails. Nothing else moves. The chain still fails open, HookDecision is unchanged, and emit_blocking keeps its public result. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
Wrapping the missing-tool error in Ok(ToolCallResult) changed what dispatch_tool_call returns to its caller. This method has always reported an inactive recipe__final_output as the outer Err(ErrorData), and callers read that outer arm, so moving the error inside the handle was a silent contract change made only to reach the post-hook wrapper. The outer error is restored, and the failure observation is emitted directly instead: the same PostToolUseFailure event, with the same tool name, tool input, tool_call_id and working_dir the wrapper would have set. Nothing executes, so PostToolUse still never fires for this path. The installed-tool arm is untouched, and the state machine already behaved this way. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
…s delivery The matcher behaviour of PreToolUseResult was covered only indirectly, through tests that also exercised denial and correlation. Pin it directly: an anchored matcher on the tool name runs the hook for that tool and not for another. The guide also left the delivery model to inference. A reader could take "observation only" to mean the event is delivered out of band, which it is not: matching hooks are awaited before goose proceeds to the tool or returns the denial, so a slow subscriber adds its runtime to the call. State that, and state that delivery is best effort rather than durable. Signed-off-by: Tymofii Pidlisnyi <171286556+aeoess@users.noreply.github.com>
7ff41af to
0005e30
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
CI is green now and Codex came back clean. I also resolved the old review threads. @DOsinga ready for review whenever you have a chance |
Signed-off-by: Douwe Osinga <douwe.osinga@gmail.com>
* main: (70 commits) cli: remove recipe secret discovery (#11435) fix(openrouter): escape Gemini tool response ref keys (#11276) fix(security): honor MCP tool model visibility in Code Mode (#11425) fix(providers): estimate cost for Azure Foundry models via inferred catalog pricing (#11264) feat(providers): add Gondola as declarative OpenAI-compatible provider (#11421) feat(otel): add request params, response metadata, tool call parity, and agent identification (#11261) fix(providers): coalesce consecutive Thinking blocks in collect_stream (#11317) feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (#11120) add MCP conformance tests to goose CI (combines #10800 + #10801) (#10940) feat(desktop): sort configured providers to the top of the provider list (#11409) fix(cli): refuse symlink diagnostics outputs (#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (#11407) fix(config): serialize secret mutations (#11388) fix: decouple source file and tool response limits (#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (#11245) fix(security): suppress sensitive OTLP traces (#11381) feat(openrouter): forward session_id and add app category header (#10868) feat(acp): derive and forward thinking effort from the ACP harness (#10949) fix(aws_bedrock): replace flat model list with routing table, add Gemma 4 Mantle support (#10297) Add GPT-5.6 follow-up support for Codex and Responses API (#10460) ...
* main: (107 commits) fix(providers): inform user of clipboard copy and remove copilot auth retry on timeout (aaif-goose#11160) feat(desktop): select saved recipes when creating a schedule (aaif-goose#10892) More provider test scripts (aaif-goose#10515) cli: remove recipe secret discovery (aaif-goose#11435) fix(openrouter): escape Gemini tool response ref keys (aaif-goose#11276) fix(security): honor MCP tool model visibility in Code Mode (aaif-goose#11425) fix(providers): estimate cost for Azure Foundry models via inferred catalog pricing (aaif-goose#11264) feat(providers): add Gondola as declarative OpenAI-compatible provider (aaif-goose#11421) feat(otel): add request params, response metadata, tool call parity, and agent identification (aaif-goose#11261) fix(providers): coalesce consecutive Thinking blocks in collect_stream (aaif-goose#11317) feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (aaif-goose#11120) add MCP conformance tests to goose CI (combines aaif-goose#10800 + aaif-goose#10801) (aaif-goose#10940) feat(desktop): sort configured providers to the top of the provider list (aaif-goose#11409) fix(cli): refuse symlink diagnostics outputs (aaif-goose#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (aaif-goose#11407) fix(config): serialize secret mutations (aaif-goose#11388) fix: decouple source file and tool response limits (aaif-goose#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (aaif-goose#11245) fix(security): suppress sensitive OTLP traces (aaif-goose#11381) feat(openrouter): forward session_id and add app category header (aaif-goose#10868) ...
2 participants
Closes #10885.
This PR adds two pieces of hook observability:
tool_call_idonPreToolUse,PreToolUseResult,PostToolUse, andPostToolUseFailure, using the request ID Goose already records asgen_ai.tool.call.id.PreToolUseResultevent emitted after thePreToolUsechain resolves and before execution or denial returns.PreToolUseResultreports the finalallowordenydecision and whether policy was evaluated.policy_evaluatedis true when at least one matchingPreToolUsehook exited 0 or returned a decision (exit 2, or block JSON); a hook that exits non-zero without a decision, fails to spawn, or times out does not count. On denial, the event also includes the blocking plugin and reason.Delivery is synchronous and best-effort: matching
PreToolUseResulthooks are awaited before the tool runs or the denial returns, and the event has no veto. This PR does not change Goose's existing fail-open behavior for hook execution failures; the configurable fail-closed control is #10866 and stays separate.Loop parity
The event is implemented in both the legacy loop and the state machine.
Some state-machine paths execute or resolve tool requests before
ToolExecutionOperation: recipe final output,load_skill, and valid unknown-tool requests in Auto mode. Those paths now use the same shared lifecycle helpers, so the new event andtool_call_idbehave the same on both loops, and one-response-per-request transcript invariants hold. Chat-mode skips remain non-executing and emit no execution lifecycle, matching the legacy loop.The final-output regressions also cover permission denial, duplicate final-output calls, malformed calls, and delayed ordinary siblings. A skipped Chat-mode final-output call is not collected as a successful result.
For an inactive
recipe__final_outputtool,Agent::dispatch_tool_callkeeps its existing outer error contract and still emitsPostToolUseFailurefor observability.Tests
Twenty-six focused
hooks_lifecyclestate-machine tests plus the legacy-loop tests inagent.rscover ID correlation, allowed and denied outcomes, successful and failed post events, direct-handling state-machine paths, Chat-mode behavior, final-output routing, the sole-abnormal-hookpolicy_evaluatedcase on both loops, the inactive final-output error contract, and thePreToolUseResultmatcher target.The state-machine final-output tests assert that each tool request has exactly one response and that no response is orphaned.
Verification
cargo fmt --all --checkcargo clippy -p goose --all-targets -- -D warningscargo test -p goose --lib hooks_lifecycle: 26 passed, 0 failedPreToolUseResult, its payload fields, and its synchronous, best-effort delivery