Conversation
Implement the p0-enforce-security-boundary change end-to-end across runtime wiring, policy mapping, auditability, and release gates. Key changes: - Add default security boundary implementations (no-op + policy) and structured security error codes. - Add DareAgentBuilder security boundary injection via with_security_boundary() and config-driven resolution (default policy, optional noop). - Enforce security preflight in DareAgent tool loop: verify_trust -> check_policy before any gateway invoke. - Map policy decisions deterministically: ALLOW continues, APPROVE_REQUIRED enters approval memory flow, DENY blocks with stable status/code/message payload. - Extend GovernedToolGateway with force_approval, approval_reason, and approval_observer so policy-triggered approval reuses existing approval memory control plane. - Emit structured security audit events (security.trust_verified, security.policy_checked, security.policy_denied, security.policy_approval) with capability/tool correlation fields. - Tighten RegistryPlanValidator to fail when trusted risk metadata is missing/invalid rather than silently defaulting. - Add security config surface to Config (from_dict/to_dict) and builder tests for config/override behavior. - Expand tests: unit coverage for allow/deny/approve_required paths, boundary defaults, gateway observer integration, trusted metadata validation; add integration flow proving high-risk tools are gated before invocation. - Add the new security gate test group to scripts/ci/run_risk_matrix.sh as a release gate. Additional fix included: - Harden event_trace_bridge OpenTelemetry compatibility by supporting both property- and method-style is_valid APIs to avoid runtime crashes during event append. Validation executed locally: - .venv/bin/python -m pytest -q tests/unit/test_security_boundary.py tests/unit/test_dare_agent_security_policy_gate.py tests/unit/test_builder_security_boundary.py tests/unit/test_governed_tool_gateway.py tests/unit/test_registry_plan_validator.py tests/unit/test_config_model.py tests/integration/test_security_policy_gate_flow.py - .venv/bin/python -m pytest -q tests/unit/test_five_layer_agent.py
…, and step-driven execution Add OpenSpec change artifacts to drive the remaining P0 workstreams in a structured, reviewable workflow. Scope: - Add full change artifacts (.openspec.yaml, proposal, design, specs, tasks) for p0-conformance-gate. - Add full change artifacts for p0-default-eventlog. - Add missing OpenSpec metadata/design/spec docs for p0-enforce-security-boundary so implementation and tasks are fully traceable. - Add full change artifacts for p0-step-driven-execution. Rationale: - Capture explicit requirements and contracts before implementation to reduce ambiguity and regression risk. - Keep P0 tracks independently auditable with per-change design, requirement deltas, and executable task checklists. - Enable deterministic follow-up implementation via openspec apply/verify/archive workflow. Notes: - This commit is documentation/spec only; no runtime source code behavior changes are introduced.
Resolve the GitHub conflict on opt_dare by merging origin/main into the branch in an isolated worktree. Key changes: - keep the opt_dare security-boundary builder and runtime preflight work intact - keep main's tool disable propagation, approval/session_id CLI flow, and runtime cleanup changes - resolve the event_trace_bridge conflict by preserving the OpenTelemetry is_valid compatibility handling from main - drop trailing blank-line noise from the newly added mailbox docs brought in from main Verification: - .venv/bin/python -m pytest tests/unit/test_builder_security_boundary.py tests/unit/test_config_model.py tests/unit/test_dare_agent_security_policy_gate.py tests/unit/test_governed_tool_gateway.py tests/unit/test_security_boundary.py tests/unit/test_builder_manager_resolution.py tests/unit/test_client_cli.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 -q - git diff --cached --check
Collaborator
Author
|
@codex review |
![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: 40fbdc3587
ℹ️ 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 the P1 review finding on PR #120. The tool preflight path was passing envelope risk defaults into verify_trust. Because ToolLoopRequest creates a default Envelope with read_only risk, strict PolicySecurityBoundary mode could incorrectly derive a trusted risk level even when the capability descriptor had no trusted risk metadata. Changes: - stop forwarding envelope risk fields into the trust-derivation context in DareAgent._evaluate_tool_security - add a regression test proving strict trust mode rejects a capability that lacks trusted risk metadata even when the request uses the default envelope Why this shape: - policy evaluation still receives the trusted risk derived from the boundary result - strict mode now behaves consistently with RegistryPlanValidator and PolicySecurityBoundary.require_trusted_metadata expectations Verification: - .venv/bin/python -m pytest tests/unit/test_dare_agent_security_policy_gate.py::test_strict_policy_boundary_rejects_missing_trusted_risk_metadata_even_with_default_envelope -q - .venv/bin/python -m pytest tests/unit/test_dare_agent_security_policy_gate.py tests/unit/test_security_boundary.py tests/unit/test_builder_security_boundary.py tests/unit/test_config_model.py tests/unit/test_governed_tool_gateway.py tests/unit/test_registry_plan_validator.py -q - git diff --check
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f8c2439fc
ℹ️ 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".
Resolve the new merge conflicts caused by origin/main advancing after the prior PR sync. Keep the step-driven execution and execute_safe wiring from main while preserving the security-boundary policy gate behavior from opt_dare. Key conflict resolutions: - keep DareAgentBuilder execution_mode and step_executor wiring while retaining config-driven security boundary resolution - preserve strict tool security preflight and structured policy errors in DareAgent - stop trusting default tool-loop envelope risk metadata, but continue trusting validated step risk metadata in step-driven execution - expose DefaultSecurityBoundary, PolicySecurityBoundary, NoOpSecurityBoundary, and security errors from the security facade for compatibility - add a regression test covering strict trusted-metadata enforcement in step-driven mode
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7a68090ad
ℹ️ 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".
Merge origin/main into the PR branch while preserving the policy-gated tool loop behavior added on opt_dare.
Key changes:
- keep the new DareAgent orchestration split and output envelope normalization from main
- move the reviewed tool-loop security preflight flow into the new internal tool executor so trust verification, policy decisions, approval escalation, and step-driven trusted risk metadata keep working after the refactor
- treat config.security.boundary=null as unset so templated/null config values do not silently select the noop security boundary
- add regression coverage for null boundary config, orchestration delegation, step-driven trusted risk propagation, and output envelope behavior
Verification:
- .venv/bin/python -m pytest tests/unit/test_builder_security_boundary.py tests/unit/test_dare_agent_security_policy_gate.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_dare_agent_orchestration_split.py tests/unit/test_agent_output_envelope.py tests/unit/test_output_normalizer.py -q
- python3 -m py_compile dare_framework/agent/dare_agent.py dare_framework/agent/_internal/tool_executor.py dare_framework/agent/builder.py tests/unit/test_builder_security_boundary.py tests/unit/test_dare_agent_security_policy_gate.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_dare_agent_orchestration_split.py
Notes:
- git diff --cached --check still reports pre-existing trailing whitespace in openspec/changes/unify-agent-output-envelope/{design,proposal}.md from origin/main; no code files fail the check.
Collaborator
Author
|
Follow-up on the latest review round after merging
Local verification run on this head:
Note: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db0adcaf0
ℹ️ 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".
Comment on lines
+94
to
+96
| before_tool_dispatch = await agent._emit_hook( | ||
| HookPhase.BEFORE_TOOL, | ||
| { |
There was a problem hiding this comment.
BEFORE_TOOL is now emitted before _evaluate_tool_security, so the hook payload can report requires_approval=False and a stale risk level for calls that policy later upgrades to APPROVE_REQUIRED (for example high-risk tools with requires_approval false in descriptor metadata). This regresses hook consumers that rely on these fields for correctness, including the tracing hook (dare_framework/observability/_internal/tracing_hook.py) which records dare.tool.requires_approval from BEFORE_TOOL; those traces/metrics become inaccurate for policy-gated calls.
Useful? React with 👍 / 👎.
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
1 participant
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.
Summary
Scope (One PR One Thing)
Changed Files (required)
List all touched files and why each file changed.
If this PR is a large diff (>300 changed lines), explain why split PRs are not possible and provide a split follow-up plan.
Acceptance Criteria
Test Evidence (required)
.........If this PR changes high-risk runtime paths (auth/concurrency/execution control), include
risk-matrixevidence.Risk and Rollback
Dependency and Lockfile Changes
requirements.txt/pyproject.toml/package.json): [ ] Yes [ ] No [ ] N/AAgent Rules Checklist (required)
Reference:
docs/agent_rules.mdskip/only/excludeusage is explained and reviewed