chore: rebase main and reconcile docs-first runtime changes - #113
Conversation
- Rebased local work onto latest origin/main and resolved DareAgent merge conflicts by combining governed tool gateway flow with security boundary trust/policy enforcement. - Preserved the in-progress documentation-first governance updates across docs/guides, design modules, TODO ledgers, and OpenSpec specs/archives. - Kept security-boundary and step-driven execution runtime changes in agent/context/event layers and carried forward corresponding unit tests. - Included archive/sync outputs for completed OpenSpec changes so active change set is clean after the rebase.
![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: bcd235906c
ℹ️ 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".
| if step_executor is None: | ||
| from dare_framework.agent._internal.step_executor import DefaultStepExecutor | ||
|
|
||
| step_executor = DefaultStepExecutor(self._tool_gateway) |
There was a problem hiding this comment.
In step_driven mode, the default executor is built with self._tool_gateway and then executes steps directly, which bypasses _run_tool_loop and therefore skips verify_trust/check_policy/execute_safe plus approval-gateway handling. This means a policy that would deny (or require approval for) invoke_tool in model-driven mode is not enforced for the same capability in step-driven mode, so high-risk tools can run without the intended security gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 44969e6. Step-driven default execution no longer invokes tools directly via the gateway; it now routes each validated step through _run_tool_loop using _execute_step_via_tool_loop, so trust verification, policy checks, approval handling, and governed execution are applied consistently with model-driven mode. Added regression coverage in test_step_driven_default_executor_routes_through_security_policy.
| return MilestoneResult( | ||
| success=False, | ||
| outputs=[], | ||
| errors=[plan_policy_error], | ||
| verify_result=VerifyResult(success=False, errors=[plan_policy_error]), | ||
| ) |
There was a problem hiding this comment.
When plan policy check fails, this early return exits _run_milestone_loop before emitting the normal failure lifecycle (milestone.failed event and HookPhase.AFTER_MILESTONE). Any hook-based cleanup/metrics logic relying on AFTER_MILESTONE will not run for security-denied milestones, causing inconsistent observability and lifecycle behavior specifically on policy-failure paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 44969e6. On plan-policy failure we now emit milestone.failed and HookPhase.AFTER_MILESTONE before returning, preserving lifecycle consistency for hook-based cleanup/metrics. This is covered by test_plan_policy_failure_emits_after_milestone_and_precise_decision.
- Remove and . - Keep a single canonical implementation/export path via and . - Align runtime package layout with trust-boundary spec requirement that legacy shim path is no longer supported. - Verified no runtime/test imports rely on after this change.
- Delete dare_framework/event/impl shim modules that only provided legacy re-export paths. - Keep event log default implementation on canonical path dare_framework.event backed by dare_framework/event/_internal/sqlite_event_log.py. - Remove compatibility alias test that imported SQLiteEventLog from dare_framework.event.impl. - Re-run targeted event persistence tests to confirm append/query/replay/verify behavior remains intact without shim paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cc7233735
ℹ️ 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".
| trusted_params, trust_error = await self._resolve_tool_security( | ||
| capability_id=request.capability_id, | ||
| params=request.params, | ||
| tool_name=tool_name, | ||
| risk_level=risk_level, |
There was a problem hiding this comment.
_resolve_tool_security(...) runs before the try block that normalizes tool invocation failures, so exceptions thrown by verify_trust/check_policy escape _run_tool_loop and abort the whole run. This is user-visible when a custom boundary rejects malformed input or has transient backend errors: instead of returning a structured tool failure (with normal loop/hook behavior), execution raises and exits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 44969e6. _resolve_tool_security(...) is now wrapped inside _run_tool_loop error handling, so boundary exceptions are normalized into structured tool-loop failures (including tool.error event and AFTER_TOOL hook emission) instead of aborting the run. Added regression test test_tool_loop_security_boundary_exception_returns_structured_failure.
| self._context.budget_check() | ||
| if self._exec_ctl is not None: | ||
| self._poll_or_raise() | ||
|
|
||
| step_result = await step_executor.execute_step( |
There was a problem hiding this comment.
Step-driven execution checks budget each step but never increments tool_calls, while the default step executor invokes one tool per step through the gateway. As a result, configured max_tool_calls limits are not enforced in step-driven mode, so long plans can exceed the intended tool-call budget without tripping guards.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 44969e6. Tool-call accounting is now enforced in step-driven mode for both paths: default executor goes through _run_tool_loop (which charges tool_calls), and custom step executors explicitly charge one tool_calls unit per step. Added regression coverage in test_step_driven_custom_executor_still_tracks_tool_call_budget.
| "milestone_id": milestone.milestone_id, | ||
| "decision": "deny", | ||
| "error": plan_policy_error, |
There was a problem hiding this comment.
When plan policy blocks milestone execution, the emitted security.plan.policy event hardcodes "decision": "deny" even if the policy result was APPROVE_REQUIRED. This misclassifies approval-required outcomes in audit/metrics pipelines, making policy diagnostics and operational reporting inaccurate for that path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 44969e6. _check_plan_policy now returns both error and concrete decision, and security.plan.policy now logs the actual result (allow / approve_required / deny) instead of hardcoding deny. Verified in test_plan_policy_failure_emits_after_milestone_and_precise_decision.
Route default step-driven execution through the governed tool loop so verify_trust/check_policy/execute_safe and approval handling are applied consistently with model-driven execution. Normalize policy-denied milestone exits by emitting milestone.failed and AFTER_MILESTONE hook payloads before returning, and log the actual plan-policy decision (allow/approve_required/deny) instead of a fixed deny string. Wrap security-resolution exceptions in _run_tool_loop so boundary failures become structured tool-loop failures with tool.error events and AFTER_TOOL hooks rather than aborting the run. Add regression tests for: step-driven security policy enforcement, tool-call budget accounting for custom step executors, precise plan-policy decision logging plus milestone lifecycle hooks, and structured handling of security boundary trust failures.
|
Addressed the current review findings and pushed What was fixed:
Added regression tests:
Local verification run:
(ruff is not installed in this local environment, but CI installs and runs it in |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/zts212653/Deterministic-Agent-Runtime-Engine/blob/44969e62f67800348fbaf7636bbd70c75142e9a1/dare_framework/agent/builder.py#L737-L741
DareAgentBuilder instantiates DareAgent without any security_boundary argument, and the builder has no setter for one, so builder-created agents always fall back to DefaultSecurityBoundary (permissive ALLOW policy). In practice this means applications using the standard BaseAgent.dare_agent_builder(...) path cannot enforce custom trust/policy boundaries added in this change.
ℹ️ 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".
| trusted_params, trust_error = await self._resolve_tool_security( | ||
| capability_id=request.capability_id, | ||
| params=request.params, | ||
| tool_name=tool_name, | ||
| risk_level=risk_level, | ||
| requires_approval=requires_approval, |
There was a problem hiding this comment.
In step-driven execution, each ValidatedStep risk is placed on ToolLoopRequest.envelope, but _run_tool_loop passes risk_level derived only from the capability descriptor into _resolve_tool_security; when descriptors are missing/stale (or report lower risk), policy is evaluated as read-only and high-risk steps can be incorrectly allowed. This bypass is reproducible with a boundary that denies high risk and a gateway that can invoke tools but returns no descriptor metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 80dbe93. _run_tool_loop now derives risk using the stricter value between descriptor metadata and ToolLoopRequest.envelope.risk_level, then passes that into trust/policy evaluation. This prevents risk downgrade when descriptor metadata is missing/stale in step-driven execution. Added regression test test_step_driven_uses_step_risk_level_when_descriptor_missing (gateway descriptors empty, validated step high risk, policy denies as expected).
Combine tool risk from descriptor metadata and request envelope by taking the stricter level before calling security boundary trust/policy checks. This closes the gap where step-driven execution could downgrade risk to read-only when capability descriptors were missing or stale, despite validated step risk being higher. Add regression coverage in test_step_driven_uses_step_risk_level_when_descriptor_missing to assert high-risk validated steps are denied by policy even when list_capabilities() returns no descriptor metadata.
|
Follow-up fix pushed in
Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80dbe93ec2
ℹ️ 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".
| result_payload = tool_result.get("result") | ||
| if result_payload is None: |
There was a problem hiding this comment.
In _execute_step_via_tool_loop, successful calls prefer tool_result["result"], but _run_tool_loop populates that key with the internal ToolResult wrapper while the actual payload is in tool_result["output"]. This makes step-driven outputs (and _previous_output passed to later steps) carry ToolResult objects instead of plain tool output data, which breaks multi-step chaining when downstream steps expect JSON-like values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fedc715. _execute_step_via_tool_loop now uses tool_result["output"] as the step output payload (falling back to result only when output is absent), so step outputs and _previous_output carry plain tool payloads instead of internal ToolResult wrappers. Added regression test test_step_driven_passes_plain_previous_output_between_steps.
| validated_plan = await self._run_plan_loop(milestone) | ||
| self._log(f"Plan loop done, validated_plan={validated_plan is not None}") | ||
|
|
||
| plan_policy_error, plan_policy_decision = await self._check_plan_policy( |
There was a problem hiding this comment.
The direct call to _check_plan_policy(...) is not guarded, so if a custom ISecurityBoundary.check_policy raises (for example, transient policy backend failures), the exception aborts the run instead of returning a structured milestone failure path. This skips normal milestone failure handling for that path and turns a policy-system outage into an unhandled runtime exception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fedc715. Plan-policy evaluation is now guarded in _run_milestone_loop; exceptions from _check_plan_policy are converted into structured milestone failure (security.plan.policy with decision: error, milestone.failed, and AFTER_MILESTONE) instead of escaping and aborting run execution. Added regression test test_plan_policy_exception_returns_structured_milestone_failure.
Add and persist the injected boundary on the builder so builder-based construction can use explicit trust/policy enforcement instead of always falling back to the default boundary. Thread the configured security boundary through into to keep behavior parity with direct DareAgent construction paths. Add regression coverage in and verify with step-driven/security-boundary/builder-focused test suite to prevent future wiring regressions.
Use tool-loop output payload as step result output in step-driven execution so step chaining passes plain JSON-like values instead of internal ToolResult wrappers. Guard plan-policy evaluation in milestone loop; when security boundary policy checks raise, convert to structured milestone failure with security.plan.policy decision=error plus normal milestone.failed and AFTER_MILESTONE lifecycle emission. Add regression tests for step-to-step previous output typing and for structured handling of plan-policy backend exceptions.
|
Replying to review note in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fedc71538a
ℹ️ 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".
| ltm_budget = retrieval_budget * normalized_ltm_ratio | ||
| knowledge_budget = retrieval_budget * normalized_knowledge_ratio |
There was a problem hiding this comment.
The budget split here always reserves a share for both LTM and knowledge, even when one source is absent (or configured with top_k=0), so single-source retrieval can be truncated unnecessarily under finite token limits. In DefaultAssembledContext.assemble, this causes false token_budget_low degradation where a candidate would fit in the full retrieval budget but is dropped because it only receives a fraction (often 50%) of that budget. Recompute ratios using only active retrieval sources before applying ltm_budget/knowledge_budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b68e4d. Retrieval budget split now only considers active sources (source != None and top_k > 0), so single-source retrieval can use the full retrieval budget instead of being implicitly truncated by a 50/50 split. I also scoped the token_budget_low pre-check to active sources. Added regression test test_context_assemble_single_source_uses_full_retrieval_budget.
When assembling context with finite token budget, compute retrieval ratio splits using only active sources (source exists and top_k > 0) so single-source retrieval is not unnecessarily truncated by an implicit 50/50 split. Also scope token_budget_low pre-check to active retrieval sources, preserving expected degradation behavior while avoiding false positives when a source is disabled. Add regression test test_context_assemble_single_source_uses_full_retrieval_budget to ensure single-source retrieval can use the full retrieval budget.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b68e4db9c
ℹ️ 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".
| "risk_level": trusted_input.risk_level.value, | ||
| "requires_approval": requires_approval, | ||
| **trusted_input.metadata, |
There was a problem hiding this comment.
Build the policy context so that trusted canonical fields cannot be overwritten by TrustedInput.metadata. Right now **trusted_input.metadata is appended after risk_level/capability_id/tool_name, so a boundary that returns metadata containing one of those keys silently overrides the computed values and can bypass policy decisions (for example, a high-risk tool can be evaluated as read_only if metadata carries risk_level: "read_only"). Merge metadata first or filter reserved keys before calling check_policy to avoid this downgrade path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a8759c8. In _resolve_tool_security, policy context now merges trusted_input.metadata first and writes canonical keys (capability_id, tool_name, risk_level, requires_approval) last, so metadata cannot override them. Added regression test test_tool_policy_context_preserves_canonical_fields_over_metadata to cover this spoofing path.
In _resolve_tool_security, build check_policy context so trusted metadata is merged first and canonical fields (capability_id/tool_name/risk_level/requires_approval) are written last, preventing metadata spoofing from downgrading policy evaluation. Add regression test test_tool_policy_context_preserves_canonical_fields_over_metadata to verify metadata cannot override canonical policy context values.
The design-doc-drift gate previously hard depended on ripgrep (`rg`). On runners where `rg` is absent, the script produced false negatives for required patterns and failed the CI job even when docs were correct. Add search helpers that prefer `rg` when available and automatically fall back to `grep -E`/`grep -En` when not. Keep existing pattern semantics and stale-claim detection behavior unchanged. Verified both execution paths locally: standard environment and simulated no-rg environment (`PATH=/usr/bin:/bin`).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdc74265e1
ℹ️ 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".
| if not plan.steps: | ||
| return await self._finalize_execute(execute_start, { |
There was a problem hiding this comment.
This check causes a hard failure for any step-driven run where a planner is configured but no validator is provided: _run_plan_loop still returns ValidatedPlan(success=True, steps=[]) in its no-validator branch, so execute() reaches this guard and fails every time with "step-driven execution requires validated plan steps". Because validator is currently optional in the builder/API, this introduces a deterministic runtime break for a supported configuration; either hydrate steps in the no-validator path or fail fast during construction when execution_mode="step_driven" lacks a validator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d5db2e.
Applied fail-fast validation during agent construction: when execution_mode="step_driven" and a planner is configured, a validator is now mandatory; otherwise DareAgent.__init__ raises ValueError("step_driven execution with planner requires validator").
This removes the runtime dead-end path where planner-only step-driven configs reached execute and failed later with "step-driven execution requires validated plan steps".
Added regression coverage for both constructor and builder paths:
test_constructor_rejects_step_driven_planner_without_validatortest_builder_rejects_step_driven_planner_without_validator
Also updated design docs to reflect this invariant:
docs/design/modules/plan/README.mddocs/design/Architecture.md
Introduce strict DareAgent construction guards for step-driven execution to prevent deterministic runtime failure paths discovered in PR review. Key changes: - Validate and normalize execution_mode in DareAgent.__init__, rejecting unsupported values. - Enforce fail-fast rule: when execution_mode is step_driven and planner is configured, validator is now mandatory; otherwise construction raises ValueError. - Add regression coverage in tests/unit/test_dare_agent_step_driven_mode.py for both direct constructor and builder path to ensure invalid planner-only step-driven configs fail before execution. - Update design docs (docs/design/modules/plan/README.md and docs/design/Architecture.md) to document the new step-driven planner+validator invariant so docs remain reconstructable and aligned with runtime behavior. Rationale: this resolves the review finding where planner-only step-driven agents reached execute() with empty validated steps and failed mid-run with a generic error instead of surfacing a clear configuration contract at build time.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d5db2eb9b
ℹ️ 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".
| if result_payload is None: | ||
| result_payload = tool_result.get("result") |
There was a problem hiding this comment.
When a step-driven tool call succeeds but returns output=None, _execute_step_via_tool_loop replaces that with tool_result["result"] (the internal wrapper object). This leaks runtime internals into step outputs despite the nearby comment, and downstream steps then receive a ToolResult-shaped object in _previous_output instead of the tool’s actual output value, which can break chaining/serialization for tools that intentionally use None as a valid success payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 60c2370.
Adjusted _execute_step_via_tool_loop to preserve explicit output=None payloads for successful tool calls. We now fallback to tool_result["result"] only when the output key is absent, not when it is present with a None value.
Added regression coverage:
test_step_driven_preserves_none_output_between_steps
This verifies both step outputs and _previous_output chaining keep None semantics and do not leak internal ToolResult wrappers.
Prevent internal ToolResult wrapper leakage when a successful step-driven tool call intentionally returns output=None. Key changes: - Update _execute_step_via_tool_loop to preserve explicit None payloads when the tool-loop response includes an output key. - Keep fallback to tool_result['result'] only for legacy responses where output key is absent. - Add regression test test_step_driven_preserves_none_output_between_steps to verify step outputs and _previous_output propagation keep None semantics across chained steps. Rationale: review feedback identified that treating output=None as missing caused fallback to internal wrapper objects, breaking downstream chaining/serialization contracts for tools that use None as a valid success payload.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60c2370d59
ℹ️ 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".
| else: | ||
| # Custom step executors may bypass tool-loop accounting; keep | ||
| # tool-call budgets aligned with one-step-one-tool execution. | ||
| self._context.budget_use("tool_calls", 1) | ||
| step_result = await step_executor.execute_step( |
There was a problem hiding this comment.
When execution_mode="step_driven" is used with a configured step_executor, this branch calls step_executor.execute_step(...) directly and skips _run_tool_loop, so verify_trust, check_policy, and execute_safe are never applied. In practice this lets step-driven tool execution bypass deny/approval policies (including when using DefaultStepExecutor, which invokes the tool gateway directly), which undermines the security boundary added in this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fe9be0.
Applied security gating for custom step executors in step-driven mode:
- Added
_execute_step_via_custom_executor(...)and routed the custom-executor branch through it. - Before invoking
step_executor.execute_step(...), runtime now runs_resolve_tool_security(...)with strict risk derivation (max of descriptor + envelope risk) and policy/trust checks. - Execution now runs inside
security_boundary.execute_safe(...)with sandbox metadata. - Policy deny/approval-required and trust failures now return structured failed
StepResult, and executor is not called.
Regression test added:
test_step_driven_custom_executor_respects_security_policy
| plan_policy_error, plan_policy_decision = await self._check_plan_policy( | ||
| milestone, | ||
| validated_plan, |
There was a problem hiding this comment.
This invokes _check_plan_policy(...) unconditionally even when validated_plan is None (e.g., model-driven runs without a planner). A security boundary that denies unknown actions or requires plan presence will now fail every such run before execution starts, which is a behavioral regression from the previous flow where no plan-policy gate existed for planless execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fe9be0.
Scoped plan-policy checks to plan-present flows only:
- In
_run_milestone_loop,_check_plan_policy(...)is now called only whenvalidated_plan is not None. - This prevents planless/model-driven runs from being blocked by
execute_planpolicy decisions when no plan was generated.
Regression test added:
test_plan_policy_check_skipped_when_plan_is_absent
Address newly surfaced PR review findings around step-driven security enforcement and plan-policy regression paths. Key changes: - Scope plan policy checks to validated-plan flows only: milestone loop now skips _check_plan_policy when no validated plan exists, preventing false deny/approve_required failures for model-driven runs without planner output. - Route custom step_executor execution through security boundary checks in step-driven mode: run trust/policy evaluation via _resolve_tool_security before execution, then execute through security_boundary.execute_safe with sandbox metadata. - Preserve strict risk semantics for custom step executors by deriving risk from the stricter of descriptor and envelope values before policy evaluation. - Keep failure behavior structured: trust/policy exceptions and denials now return failed StepResult instead of silently bypassing security controls. - Add regression tests: * test_step_driven_custom_executor_respects_security_policy * test_plan_policy_check_skipped_when_plan_is_absent Rationale: custom step executors previously bypassed invoke_tool security gates, and plan-policy checks were applied unconditionally even when no plan existed. These changes restore policy correctness while preserving existing execution contracts.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fe9be0269
ℹ️ 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".
| if decision is PolicyDecision.APPROVE_REQUIRED: | ||
| return {}, "tool invocation requires security approval" |
There was a problem hiding this comment.
_resolve_tool_security maps PolicyDecision.APPROVE_REQUIRED to an immediate error, and the tool loop treats that as a terminal failure path. In deployments where the security boundary intentionally returns APPROVE_REQUIRED for risky capabilities, tool calls are denied outright instead of entering the approval-memory/human-resolution flow, even if an approval manager is configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5804c5b.
PolicyDecision.APPROVE_REQUIRED from _resolve_tool_security is no longer treated as immediate terminal denial. The tool path now routes this outcome through _resolve_tool_approval(...) (including pending/wait resolution) when descriptor-level approval is not already active. This keeps approval-memory/human-resolution behavior available for boundary-driven approval-required outcomes.
Also adjusted _resolve_tool_security to retain trusted params on APPROVE_REQUIRED so approval evaluation uses the actual invocation payload.
Regression coverage:
test_tool_loop_approve_required_routes_through_approval_workflow
| if normalized_execution_mode == "step_driven" and planner is not None and validator is None: | ||
| raise ValueError("step_driven execution with planner requires validator") |
There was a problem hiding this comment.
The constructor validation allows execution_mode="step_driven" when planner is None, but the public execute() path cannot produce a validated plan in that configuration (_run_plan_loop returns None and step-driven execute requires validated steps). This creates a build-valid runtime-invalid mode that predictably fails only after execution starts instead of failing fast during construction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5804c5b.
Added constructor-level fail-fast validation for this runtime-invalid configuration:
DareAgent.__init__now raisesValueError("step_driven execution requires planner")whenexecution_mode="step_driven"andplanner is None.
Regression coverage:
test_constructor_rejects_step_driven_without_plannertest_builder_rejects_step_driven_without_planner
Approval handling in two step-driven paths happened before BEFORE_TOOL hook dispatch, which could trigger approval-manager failures or waits even when hook policy should block the tool call immediately.\n\nThis change enforces hook-first ordering in both locations:\n- _run_tool_loop (default step-driven tool path)\n- _execute_step_via_custom_executor (custom step executor path)\n\nNow non-approval trust/policy failures still short-circuit first, then BEFORE_TOOL hook is evaluated, and approval resolution runs only if hook allows execution.\n\nTests:\n- add tool-loop regression for metadata approval + blocking hook ordering\n- add custom step-executor regression for metadata approval + blocking hook ordering\n- run tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c02d90323a
ℹ️ 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".
| return { | ||
| "success": False, | ||
| "error": trust_error, | ||
| "output": {}, | ||
| } |
There was a problem hiding this comment.
When verify_trust/check_policy blocks a tool call, this branch returns a failure payload without a status field, unlike other governed-tool failures. Downstream handling in the execute loop uses status to distinguish policy denials from generic execution errors, so these security denials are misclassified as plain failures (e.g., no not_allow signal for policy-aware handling and user-facing messaging). This occurs whenever ISecurityBoundary.check_policy(action="invoke_tool") returns DENY.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1f09d6b.
Security-policy trust denials in _run_tool_loop now return an explicit status:
status: "not_allow"
This aligns deny-path classification with downstream execute-loop handling and preserves policy-aware messaging behavior.
Updated test:
test_tool_loop_denied_by_security_policynow assertsstatus == "not_allow"
Verified with:
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
| parsed = int(value) | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
_safe_int catches TypeError/ValueError but not OverflowError, so values like YAML .inf (parsed as float('inf')) for assemble_top_k or assemble_reserve_tokens will raise and crash context assembly instead of safely defaulting. Because these options are read from untyped config dicts, this makes retrieval assembly brittle for malformed or extreme config inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1f09d6b.
DefaultAssembledContext._safe_int now catches OverflowError (in addition to TypeError/ValueError) so .inf-style numeric inputs safely fall back to defaults instead of crashing assembly.
Added regression test:
test_context_assemble_handles_overflowing_numeric_retrieval_config
Verified with:
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
…c config Two review follow-ups: 1) _run_tool_loop now returns status='not_allow' for security-policy trust denials so execute-loop consumers can classify policy rejects consistently. 2) DefaultAssembledContext._safe_int now catches OverflowError in addition to TypeError/ValueError, preventing .inf-like config values from crashing context assembly. Tests: - assert status='not_allow' in tool-loop deny path test - add overflow-config regression test for context assembly - run tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f09d6b2e5
ℹ️ 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".
| if math.isnan(parsed) or parsed < 0: | ||
| return self._DEFAULT_SOURCE_RATIO | ||
| return parsed |
There was a problem hiding this comment.
_safe_ratio currently accepts float('inf') as a valid ratio, because it only rejects NaN and negative values. With configs like assemble_ratio: .inf, later normalization (ltm_ratio / ratio_total) produces NaN budgets, and _take_with_budget no longer enforces token limits (x > NaN is always false), so oversized retrieval hits are included even when token budget is low. This breaks the new budgeted-assemble guardrail and can overrun prompt budgets instead of degrading retrieval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 713f584.
DefaultAssembledContext._safe_ratio now rejects non-finite values (inf, -inf, nan) and falls back to the default ratio. This prevents NaN budget splits from bypassing _take_with_budget guardrails.
Added regression test:
test_context_assemble_rejects_infinite_ratio_and_keeps_budget_guardrails
Verified with:
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
_safe_ratio previously accepted float('inf'), which could produce NaN normalized budgets and disable budget filtering in _take_with_budget.\n\nTreat non-finite values (inf/-inf/nan) as invalid and fall back to default ratio, preserving token-budget guardrails.\n\nTests:\n- add regression for assemble_ratio=float('inf') budget guardrail behavior\n- keep overflow coercion regression coverage\n- run tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 713f584cfc
ℹ️ 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".
| def _safe_ratio(self, value: Any) -> float: | ||
| try: | ||
| parsed = float(value) | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
_safe_ratio only catches TypeError/ValueError, but float() can also raise OverflowError for very large integer config values (for example assemble_ratio: 10**10000 from parsed YAML/JSON). In that case context assembly raises and aborts the run instead of falling back to the default ratio, which defeats the defensive parsing this method is meant to provide.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82fabfa.
DefaultAssembledContext._safe_ratio now also catches OverflowError (besides TypeError/ValueError), so huge numeric ratio inputs no longer crash assembly and correctly fall back to default ratio handling.
Added regression test:
test_context_assemble_handles_overflowing_numeric_ratio_config
Verified with:
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
_safe_ratio now catches OverflowError in addition to TypeError/ValueError so very large numeric ratio inputs (e.g. huge integers from config) cannot crash context assembly.\n\nAlso added regression coverage for overflowing ratio inputs to ensure fallback normalization stays defensive.\n\nTests:\n- add test_context_assemble_handles_overflowing_numeric_ratio_config\n- run tests/unit/test_dare_agent_security_boundary.py tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_context_implementation.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82fabfa29b
ℹ️ 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".
| tool_result = await self._run_tool_loop( | ||
| request, | ||
| transport=transport, | ||
| tool_name=step.capability_id, |
There was a problem hiding this comment.
In step-driven mode, validated steps are sent straight to _run_tool_loop without checking whether the step is a plan tool (plan: / CapabilityKind.PLAN_TOOL). Unlike the model-driven path (which sets encountered_plan_tool to trigger milestone retry/replan), this path tries to invoke the plan tool as a normal capability, so plan-driven replan signals become hard tool failures when the capability is not registered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1caa6d2.
Step-driven execution now handles plan-tool steps before invoking tool execution. In _run_step_driven_execute_loop, each validated step is checked for plan-tool semantics via both:
plan:prefix / descriptor-based detection (_is_plan_tool_call)ValidatedStep.metadata["capability_kind"] == "plan_tool"
When matched, execute loop returns encountered_plan_tool + plan_tool_name and exits early, so milestone loop can replan instead of attempting normal tool invocation.
Added regression test:
test_step_driven_plan_tool_step_triggers_replan_signal_without_tool_invocation
Verified with:
pytest -q tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_dare_agent_security_boundary.py
| return { | ||
| "success": False, | ||
| "error": approval_error_text, | ||
| "output": {}, |
There was a problem hiding this comment.
When policy-driven approval is denied, this return payload omits status, so downstream handling treats it as a generic failure (fail) instead of an explicit denial (not_allow). In executions where check_policy(...)=APPROVE_REQUIRED and approval resolves to deny, this loses the denial semantic used elsewhere for tool outcomes and can misclassify blocked actions in runtime messages/telemetry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1caa6d2.
_run_tool_loop now preserves explicit deny semantics for approval-rejected calls by attaching a status field on the approval-failure return path:
- approval denial (
...denied...) ->status: "not_allow" - non-denial approval errors ->
status: "fail"
This keeps policy-aware downstream handling aligned with deny classification.
Added regression test:
test_tool_loop_policy_approval_deny_preserves_not_allow_status
Verified with:
pytest -q tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_dare_agent_security_boundary.py
Address two review findings in step-driven/tool-loop governance.\n\n- Step-driven execute loop now detects plan-tool steps before invoking tool execution. Detection covers both capability IDs and metadata, then returns / so milestone loop can replan instead of failing on missing capability registration.\n- Tool-loop approval rejection path now includes an explicit field. Denial outcomes preserve , while non-denial approval errors keep , maintaining downstream policy-aware classification.\n\nAdded regression tests:\n- test_step_driven_plan_tool_step_triggers_replan_signal_without_tool_invocation\n- test_tool_loop_policy_approval_deny_preserves_not_allow_status\n\nVerification:\n- pytest -q tests/unit/test_dare_agent_step_driven_mode.py tests/unit/test_dare_agent_security_boundary.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1caa6d2c83
ℹ️ 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".
| reserve_tokens_raw = ltm_config.get("assemble_reserve_tokens") | ||
| if reserve_tokens_raw is None: | ||
| reserve_tokens_raw = knowledge_config.get("assemble_reserve_tokens") |
There was a problem hiding this comment.
The reserve-token selection always prefers long_term_memory.assemble_reserve_tokens when present, even if LTM retrieval is disabled (assemble_top_k=0 or long_term_memory is None). In that configuration, knowledge-only retrieval can be incorrectly starved (or fully skipped) because the inactive LTM reserve is subtracted from the budget first. This causes avoidable token_budget_low degradation and drops knowledge hits that would otherwise fit under the knowledge source’s own reserve policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89d784d.
Reserve token selection in DefaultAssembledContext.assemble now respects active retrieval sources:
- LTM-only active -> use
long_term_memory.assemble_reserve_tokens - Knowledge-only active -> use
knowledge.assemble_reserve_tokens - Both active (or neither) -> retain existing precedence fallback behavior
This prevents inactive LTM reserve values from starving knowledge-only retrieval budgets.
Added regression test:
test_context_assemble_ignores_inactive_ltm_reserve_tokens_for_knowledge_only_retrieval
Verified with:
pytest -q tests/unit/test_context_implementation.py
| if retrieval_budget != float("inf"): | ||
| ltm_budget = retrieval_budget * normalized_ltm_ratio | ||
| knowledge_budget = retrieval_budget * normalized_knowledge_ratio |
There was a problem hiding this comment.
Budgets are pre-split across LTM and knowledge before retrieval calls, but if one source fails at runtime (for example LTM raises), the other source still uses only its original fraction instead of the full remaining retrieval budget. In mixed-source setups this can drop valid candidates from the surviving source even though they fit within the total retrieval budget, degrading context quality specifically during partial backend outages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89d784d.
When LTM retrieval fails and knowledge retrieval is active, the knowledge path now gets the full remaining retrieval budget instead of staying capped at its pre-split ratio slice. This preserves retrieval quality during partial backend outages.
Added regression test:
test_context_assemble_rebalances_budget_when_ltm_retrieval_fails
Verified with:
pytest -q tests/unit/test_context_implementation.py
Address two retrieval-budget review findings in DefaultAssembledContext.\n\nChanges:\n- Reserve token selection now prefers the active retrieval source. In knowledge-only retrieval, inactive long_term_memory reserve settings no longer starve the retrieval budget.\n- When long_term_memory retrieval fails and knowledge retrieval is active, knowledge now receives the full remaining retrieval budget instead of staying capped at its pre-split ratio share.\n\nAdded regression tests:\n- test_context_assemble_ignores_inactive_ltm_reserve_tokens_for_knowledge_only_retrieval\n- test_context_assemble_rebalances_budget_when_ltm_retrieval_fails\n\nVerification:\n- pytest -q tests/unit/test_context_implementation.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89d784d8b7
ℹ️ 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".
| ltm_candidates = context.long_term_memory.get(query=query, top_k=ltm_top_k) | ||
| ltm_messages = self._take_with_budget(ltm_candidates, ltm_budget) |
There was a problem hiding this comment.
When assemble_ratio normalizes a source budget to 0 (for example, long_term_memory.assemble_ratio=0 while knowledge gets all budget), this branch still calls long_term_memory.get(...) and only drops results afterward via _take_with_budget. That means we still pay for potentially expensive retrieval backends and can incorrectly mark the request as degraded (token_budget_low) even though the source was effectively disabled by config. Add a guard to bypass retrieval entirely when the computed per-source budget is <= 0.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c26340e.
In DefaultAssembledContext.assemble, retrieval calls are now skipped when the computed per-source budget is <= 0:
- LTM path skips
long_term_memory.get(...)whenltm_budget <= 0 - Knowledge path skips
knowledge.get(...)wheneffective_knowledge_budget <= 0
This avoids unnecessary backend calls for ratio-disabled sources and prevents false token_budget_low degradation from post-filter drops.
Added regression test:
test_context_assemble_skips_zero_budget_source_retrieval_call
Verified with:
.venv/bin/pytest -q tests/unit/test_context_implementation.py
Root cause\n- Retrieval sources with normalized per-source budget <= 0 were still queried.\n- Results were then dropped by budget filtering, which incurred unnecessary retrieval calls and could incorrectly set token_budget_low degradation.\n\nChanges\n- In DefaultAssembledContext.assemble, guard per-source retrieval calls by budget.\n- Skip long-term-memory retrieval when ltm_budget <= 0.\n- Skip knowledge retrieval when effective_knowledge_budget <= 0.\n- Preserve existing degradation semantics for real truncation and retrieval failures.\n\nTests\n- Added test_context_assemble_skips_zero_budget_source_retrieval_call.\n- Verified with: .venv/bin/pytest -q tests/unit/test_context_implementation.py
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…c lifecycle Context:\nPR #113 confirmed that cross-project documentation governance philosophy should be handled in a dedicated follow-up change instead of being mixed into runtime rebase work.\n\nWhat this commit includes:\n- creates OpenSpec change \n- adds proposal defining scoped governance objectives: aggregation entry, frontmatter contract, active/archive indexing, and machine-checkable checkpoints\n- adds technical design covering decisions, risks, migration strategy, and non-goals\n- adds capability specs:\n - new capability: \n - modified capability: \n- adds implementation task breakdown (12 tasks) to drive follow-up execution in small verifiable steps\n\nRationale:\nThis commit intentionally starts with governance contracts and execution plan artifacts only, so the follow-up implementation PR can proceed under explicit OpenSpec requirements with traceable acceptance criteria.\n\nVerification:\n- openspec status --change enhance-doc-governance-traceability --json\n- openspec instructions apply --change enhance-doc-governance-traceability --json\n- openspec validate --changes enhance-doc-governance-traceability
|
Follow-up started as a standalone OpenSpec PR: #126 ( This new PR contains the governance change scaffolding (proposal/design/specs/tasks) promised in this thread, and is ready for task-by-task apply in subsequent commits. |
Context: A follow-up review highlighted that the standalone governance change mentioned automated checkpoints but did not explicitly encode the “SOP skillization” requirement raised in PR #113. Updates in this commit: - proposal: adds explicit scope for skillizing governance lifecycle stages (kickoff/execution/completion/verification) - design: adds a dedicated decision for checkpoint-to-skill mapping and skill-based governance execution semantics - specs: - documentation-lifecycle-traceability: adds requirement that key SOP stages MUST be represented by callable skills - design-reconstructability-governance: adds requirement to maintain auditable checkpoint-skill mappings - tasks: adds a dedicated SOP skillization workstream and renumbers pilot tasks accordingly Verification: - openspec validate --changes enhance-doc-governance-traceability - openspec status --change enhance-doc-governance-traceability --json
2 participants
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