Repository navigation
fix: normalize debate wake tool response envelopes - #19
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The change reworks fail-closed security/privacy-sensitive hook parsing with many subtle branches, so final human review is warranted despite no blocking defects found.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR hardens the debate_post_with_recipients PostToolUse wake hook so that it resolves the canonical tool-response identity (msg_id/topic_id/schema_version) conservatively across the many wrapper shapes MCP/Claude hooks can emit. The previous _unwrap_tool_response greedily returned the first dict containing msg_id/schema_version, which could pick a stale or error-wrapped candidate. The new _normalize_tool_response traverses all recognized wrapper branches, fails closed on isError, conflicting identities, non-scalar identity fields, and over-sized/over-deep structures, and selects the most complete original candidate. It also tightens diagnostics so missing-response logging only emits allowlisted shape sketches (no unknown keys/values or raw tool names), and adds a non-object payload guard in _run_hook.
Changes:
- Replace greedy unwrapping with a bounded, fail-closed
_normalize_tool_responseplus alias-aware_extract_tool_response. - Add privacy-preserving
_describe_shapeformissing_tool_responselogging and stop logging raw payload keys / exception strings / full tool names. - Add comprehensive regression tests covering wrappers, legacy aliases, conflict/error/bounds rejection, privacy, fail-closed payloads, and a real DAO dry-run integration path.
| File | Description |
|---|---|
| hooks/debate_wake.py | Adds bounded fail-closed envelope normalization, allowlisted shape logging, and a non-object payload guard in _run_hook. |
| tests/test_debate_wake_payload_normalization.py | New tests for normalization, privacy diagnostics, fail-closed handling, and an end-to-end stdin→resolver→dry-run DAO regression. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return out | ||
| if "tool_response" in hook_payload: | ||
| return _normalize_tool_response(hook_payload["tool_response"]) | ||
| aliases = [hook_payload[key] for key in _HOOK_RESPONSE_KEYS[1:] if key in hook_payload] |

Summary
Verification
hooks/debate_wake.pyand addstests/test_debate_wake_payload_normalization.py.