fix(approval): align PR77 review feedback on transport ownership - #102
Merged
Conversation
Collaborator
Author
Collaborator
Author
|
补充：重点核查 approval deny 路径的 tool result 分类为 not_allow，以及 agent 侧不再通过实例状态持有 transport。 |
![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: 41f0d6e048
ℹ️ 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".
added 3 commits
February 26, 2026 01:12
Implement ADR v2 end-to-end for transport event typing and approval control-plane consistency. Key changes: - Introduce and propagate canonical handling via + and expose normalizer from transport facade. - Keep minimal and add for channels that support unsolicited event polling. - Remove legacy transport payload dead fields () from runtime payload builders and hook emission. - Route CLI approval flows through channel/action dispatch paths and require explicit in approval responses. - Add filtering in approval pending polling and align pending-state notifications with condition-based synchronization. - Expand and tighten tests for protocol normalization, adapter behavior, action handler responses, and example CLIs. Rationale: - Preserve transport boundary abstraction and avoid direct internal manager coupling. - Make event routing explicit and auditable without relying on payload-shape heuristics. - Reduce concurrency ambiguity in approval pending wakeups and prevent cross-session polling bleed. - Match strict review feedback by removing protocol dead fields and consistency gaps before PR.
…semantics Address the actionable file-review comments on PR #77 with focused behavioral fixes. Key changes: - Remove mutable instance-level state from . - Thread explicitly through execution/session/milestone/tool loops and approval emission paths. - Update to require explicit transport input, avoiding hidden channel selection. - Stop emitting transport events after resolution. - Represent denied approval outcomes as in tool-loop results and tool messages. - Preserve serialization stability for tool messages via . Tests: - Add regression test that transport payload send requires explicit transport argument. - Add regression test that denied approvals produce and do not emit . - Run approval/agent regression suite (55 tests) covering five-layer flow, CLI approval paths, and approval manager/action handler behavior. Rationale: - Prevent transport cross-talk/concurrency risk from shared mutable state. - Keep approval denial semantics explicit for downstream consumers and model-facing tool messages. - Align transport behavior with reviewer guidance that non-transport-loop paths should rely on explicit transport parameters.
Cloud Codex reported two regressions on PR #102: session-filtered polling missed deduplicated pending approvals, and the CLI hard-capped approval action transport waits at 30s.\n\nThis commit fixes both issues and adds regression coverage:\n- Track all sessions waiting on a deduplicated pending approval request and include them in session-filtered poll matching.\n- Keep approval poll transport timeout aligned with user-provided timeout_seconds/timeout_ms, with a small client-side cushion so ask() does not expire first.\n- Apply the timeout fix in both example CLIs (05 enhanced and 06 mcp) to keep behavior consistent.\n- Add manager/action-handler tests for deduplicated cross-session polling and CLI tests that assert poll timeout values are propagated to transport waits.\n\nValidation:\n- Targeted regressions: 4 passed\n- Affected unit suites: 33 passed\n- Full suite currently has unrelated pre-existing failures in builder_manager_resolution and example_10 compatibility tests.
zts212653
force-pushed
the
fix/pr77-comment-alignment
branch
from
February 26, 2026 09:17
41f0d6e to
b4603f9
Compare
Collaborator
|
@codex 严格一点的review下。必须充分考虑功能职责和代码实现 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4603f96fd
ℹ️ 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".
mindfn
reviewed
Feb 26, 2026
Move approval evaluation/pending transport emission out of DareAgent and into a dedicated GovernedToolGateway wrapper so agent orchestration only performs tool-loop control and consumes tool outcomes.\n\nKey changes:\n- add dare_framework/tool/_internal/governed_tool_gateway.py to centralize approval evaluate/wait/send at tool invoke boundary\n- switch DareAgent tool invocation to governed gateway and treat denied calls as normal tool result status=not_allow\n- remove legacy agent-level approval helpers (_resolve_tool_approval/_emit_approval_pending_message/_send_transport_payload)\n- extend EnvelopeKind with SELECT and emit approval.pending envelopes as kind=select with explicit allow/deny options\n- update example 10 response parsing to prefer envelope.event_type with payload.type fallback so removing payload type aliases does not misclassify errors\n- refresh unit tests to assert event_type/select semantics and updated boundary behavior\n\nRationale:\n- addresses PR feedback about clear responsibility boundaries (agent vs tool gateway)\n- keeps interaction protocol explicit at envelope layer\n- preserves shipped consumer correctness under event_type-first transport routing
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3fedb2b02
ℹ️ 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".
Address two blocking P1 regressions reported on PR #102.\n\nKey fixes:\n- restore approval lifecycle event emission in GovernedToolGateway while waiting for human resolution\n - emit exec.waiting_human before wait_for_resolution\n - emit exec.resume and tool.approval after decision\n - preserve tool.approval events for rule-based allow/deny paths\n- wire DareAgent tool loop to pass its event logger into governed gateway so event-log-driven approval flows keep functioning\n- fix deduplicated pending request wakeups in ToolApprovalManager.evaluate\n - when a dedup hit adds a new session subscriber, notify condition waiters immediately\n - update _track_pending_session_locked to return whether subscriber set changed\n\nRegression coverage (Red->Green):\n- test_poll_pending_session_waiter_wakes_when_dedup_adds_matching_session\n- test_no_planner_emits_approval_lifecycle_events_for_event_log_auto_resolution\n\nVerification run:\n- targeted approval/event-flow suite: 82 passed\n- full pytest baseline remains unchanged except known pre-existing builder-manager failures (7 failed / 330 passed / 11 skipped).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acfb1463b7
ℹ️ 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".
Address Codex P2 on PR #102: governed gateway invocation previously passed governance metadata as explicit keyword arguments (, , ) and then expanded . If a tool legitimately used one of those keys, Python raised before approval evaluation or tool execution.\n\nImplementation:\n- introduce in governed gateway to carry approval metadata out-of-band from tool params\n- change to accept the approval context as a positional argument and derive governance fields internally\n- update tool loop to construct/pass and keep untouched for downstream tool invocation\n\nRegression coverage:\n- add to verify tool-level arguments execute successfully through the approval path\n\nVerification:\n- targeted approval/event suites pass (83 passed)\n- full project baseline unchanged except known pre-existing builder-manager failures (7 failed / 331 passed / 11 skipped).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 212daff3f8
ℹ️ 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".
…failures Address the two new Codex P2 comments on PR #102. Changes: - prevent governed invoke collisions when tool args include `context` - add `runtime_context` to `ApprovalInvokeContext` - stop passing `context=` explicitly from DareAgent into governed invoke - in `GovernedToolGateway`, route runtime context out-of-band using internal key `__dare_runtime_context__` when tool params already contain `context` - in `ToolGateway.invoke`, consume that internal key, restore colliding tool arg `context`, and build `RunContext` from runtime context override - differentiate approval denial from approval infrastructure/configuration failures - add `ApprovalResolution` with verdicts: allow/deny/error - map deny => output.status `not_allow` - map infrastructure/configuration errors => output.status `fail` Red-Green regression coverage: - test_no_planner_tool_params_context_does_not_collide_with_runtime_context - test_no_planner_missing_approval_manager_reports_fail_status Verification: - targeted approval/transport/gateway suites: 94 passed - full baseline unchanged except known pre-existing builder-manager failures: 7 failed / 333 passed / 11 skipped
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d35a7a7f8b
ℹ️ 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".
Address PR #102 review feedback (P1 + P2) in one pass. Key changes: - Added RuntimeContextOverride internal wrapper and shared reserved key constant. - Updated ToolGateway to ignore caller-supplied __dare_runtime_context__ values unless they carry the trusted internal wrapper type. - Kept tool-argument/context collision recovery while preventing user-controlled runtime context replacement. - Updated GovernedToolGateway to pass effective execution params (including reinjected context arg) into approval evaluation. - Updated governed runtime-context forwarding to use the internal wrapper type. Tests: - Added regression test ensuring untrusted __dare_runtime_context__ does not override run_context deps. - Added unit test ensuring approval evaluation uses effective params when context collides with invoke signature. - Verified relevant gateway/five-layer tests pass after changes.
Collaborator
|
这条评论只讨论“如何一次性把授权/权限链路做对”（当前项目仍在开发阶段，不需要兼容历史协议）。 我先给结论：
1) 架构层（职责边界）问题 A：审批结果语义仍通过
|
Collaborator
|
补充一条更“可执行”的 review（按初级同学可落地的方式写），目标是把这个特性从“勉强可用”做到“易用 + 好用 + 可维护 + 可验证”。 先说原则：
A. 当前问题分层清单（按严重级别）P0 Correctness
这会直接破坏“session 级授权”的可信性。 P1 语义一致性
结果：审计和 hook 看到的是“已批准后执行失败”，但真实是“审批系统失败/未配置”。
这是典型 contract drift。 P1 易用性
这意味着审批 pending 事件无法实时推送到 CLI（只能靠用户手动 P2 健壮性/维护性
当前只拦截
B. 该怎么分析（先分析再改）建议先写一页“授权链路 invariants”，至少包含以下 8 条：
然后把现状映射到 invariants，看每条谁违背了。 C. 该怎么设计（目标架构）1) 结果模型类型化当前把治理状态塞进 建议：
2) 授权模型分离两个角色
规则绑定（scope=session）必须用 3) 协议生命周期定版明确选择：
选一个后删除另一套的文档、分支、helper、枚举，避免半迁移状态。 4) CLI 产品化目标
D. 该怎么开发（建议提交顺序）
E. 测试应覆盖到什么程度（最小但完整）单测
集成测试
示例回归
F. 文档应达到什么标准
如果按上面收敛，你这个特性会从“可以跑”变成“可预测、可解释、可维护”。这才是授权系统真正可长期演进的状态。 |
Collaborator
|
为了推动这条 PR 实际落地，我再给一个“合并前收敛清单”（请作者在本线程逐项回复 Merge Gate（建议按此执行）
交付格式（请按这个模板回复）
并在最终更新时附上：
这样这条 PR 就能从“讨论层面”进入“可验收层面”。 |
Collaborator
Author
|
已按 PR 上的收敛方向直接落盘：\n- 互动分发按 （message/action/control）收口，去耦消息/动作/控制路径；\n- Builder 侧注入 完整链路；\n- DefaultAgentChannel 的分发与超时/错误回包行为已统一；\n- stdio 入口按 命令映射 -> 并区分本地生命周期命令。\n如果需要，我再补一轮本地验证（本消息为同步状态同步/收敛确认）。 |
Collaborator
Author
|
已按 PR 收敛方向落盘同步：
以上是本次收敛结果；如要，我再补一轮本地验证（我先前这条消息作为阶段同步/合入确认）。 |
Collaborator
|
二次完整 review（基于当前 head Blocking Findings1) [P0]
|
This was referenced Feb 27, 2026
Collaborator
Author
|
已按你说的把 #102 收束完毕并已合入。 本次我同步落盘了当前遗留修复（runtime context 派发参数去重污染）并补了回归测试，另起一个追踪：
本次 follow-up PR 先做一项 unblock（审批去重/持久化中剥离 dare_runtime_context），其余 3 个阻塞项与 2 个建议项我已在 issue 111 按清单跟进。 |
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.
What
This PR is a targeted follow-up for file-level review comments on PR #77.
Changes aligned to review feedback
DareAgent._active_transportstate.transportexplicitly through execute/session/milestone/execute/tool/approval paths._send_transport_payloadnow requires explicittransportargument.not_allowfor approval-denied paths.statusso downstream consumers can distinguish deny vs generic failure.approval.resolvedtransport event emission from agent approval flow.approval.pendingas the async wait signal.Why
Review-comment mapping (PR #77)
#discussion_r2796634176-> addressed by explicit transport threading + removal of_active_transport.#discussion_r2796936639-> addressed by removingapproval.resolvedemission and introducingstatus=not_allow.#discussion_r2796667747/#discussion_r2796940677-> partially addressed in this PR (agent-side approval output semantics and transport ownership). The larger architecture refactor (tool-owned approval pipeline/risk-policy redesign) is intentionally scoped out of this patch set.Test Evidence
/Users/lysander/projects/dare-framework/.venv/bin/python -m pytest \ tests/unit/test_five_layer_agent.py \ tests/unit/test_dare_agent_hook_transport_boundary.py \ tests/unit/test_examples_cli.py \ tests/unit/test_examples_cli_mcp.py \ tests/unit/test_tool_approval_action_handler.py \ tests/unit/test_tool_approval_manager.py # 55 passedOpen Questions
Supersedes: #100