NS09A: executable replay plan and runtime core - #44
Conversation
7b6f9b7 to
d26674c
Compare
|
@codex review |
|
Base corrected to the campaign integration branch @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: d26674c258
ℹ️ 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 timed_out_code is not None: | ||
| detail = "replay exceeded its total deadline" if timed_out_code == "replay.total_timeout" else "tool call exceeded its deadline" | ||
| raise _RuntimeFailure("timed_out", timed_out_code, detail) | ||
| messages.append({"role": "tool", "tool_call_id": call.id, "content": json.dumps(_redact(outcome, secrets=secret_values, workspace=workspace.root, counter=redaction_count), sort_keys=True)}) |
There was a problem hiding this comment.
When a successful tool returns a value that matches a secret or the workspace path, this appends the redacted copy to the in-memory messages transcript that is used for the next provider request. The persisted tool_outcome is already redacted via put(...), so redacting here changes what the model sees on subsequent turns (<redacted> instead of the real tool result) and can make replay behavior diverge from the execution it is supposed to reproduce. Keep the original outcome in the live transcript and redact only the artifact copy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f4ed99f. The live provider transcript now receives the original canonical tool outcome; only persisted artifact copies pass through redaction. Added an isolated-provider regression proving the next turn sees SECRET while the stored tool_outcome contains <redacted>. Focused acceptance: 147 passed.
|
Fixed the Codex P1 live-tool-result finding in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4ed99f10c
ℹ️ 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".
| provider_tools, reverse_tool_aliases = _provider_tool_wire_schemas(scenario.tool_schemas) | ||
| provider_messages = _provider_wire_messages(messages, reverse_tool_aliases) |
There was a problem hiding this comment.
When a replay uses a provider config that forces host.execute (for example provider_tools.anthropic.tool_choice = {"type": "tool", "name": "host.execute"}), this aliases the schema/messages to host_execute before invoking the provider but leaves the copied provider_context.agent_config unchanged. The Anthropic runtime only builds its own alias map from the tools it receives, so after this pre-aliasing it sends a tool_choice for host.execute that is not present in the tools list, causing the provider call to fail before the host tool can run. Please rewrite forced tool choices with the same alias map before invoking the provider.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 624b2f9654
ℹ️ 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".
| method = getattr(self.sandbox, "replay_process_containment", None) | ||
| if not callable(method): | ||
| raise RuntimeError("sandbox does not attest detached-descendant containment") |
There was a problem hiding this comment.
Because replay_process_containment is now part of the host port, leaving this as a lazy runtime lookup means SandboxHostAdapter still constructs and probe() still reports available for a sandbox that has get_workspace()/execute() but no containment attestation; run_replay() later rejects the same host in _host_containment_identity, so catalog/preflight flows can pass with an unusable replay host. Please validate this attestation during construction or probing when the adapter declares the replay-capable host contract.
Useful? React with 👍 / 👎.
|
Fixed the exact-head Codex P2 host-containment preflight finding in
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4037922b18
ℹ️ 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 isinstance(value, (list, tuple, set, frozenset)): | ||
| rows = [_identity_value(item, next_seen) for item in value] | ||
| return sorted(rows, key=canonical_json) if isinstance(value, (set, frozenset)) else rows | ||
| transient = {"calls", "_calls", "polls", "_polls", "messages_seen", "tools_seen", "_client_identities"} |
There was a problem hiding this comment.
When provider is a ProviderRuntimeAdapter, _provider_route_binding() hashes _runtime_type_identity(provider), and this state walk excludes _client_identities but still includes _client_replay_specs. After create_client(), that list contains the raw api_key plus every client spec the adapter has ever created, so the frozen provider-route lock can change because of unrelated prior clients and can pull secret material into the binding input. Treat _client_replay_specs as transient here, or project only the sanitized active client identity that is already passed separately.
Useful? React with 👍 / 👎.
|
Fixed the exact-head Codex P2 provider-route identity finding in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e32d9c970a
ℹ️ 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 get_provider_metadata(self, key: str, default: Any = None) -> Any: | ||
| return self.provider_metadata.get(key, default) | ||
| def provider_metadata_snapshot(self) -> Dict[str, Any]: | ||
| return dict(self.provider_metadata) |
There was a problem hiding this comment.
When run_replay receives a real SessionState as provider_context.session_state, _provider_context_snapshot now detects this new method and canonicalizes the entire returned dict; normal sessions store non-JSON runtime objects such as control_queue (and permission_queue for interactive permissions) in provider metadata, so this dict(self.provider_metadata) path raises ReplayRunError("provider context must contain only canonical provider data") before replay starts. Please snapshot only the JSON replay state the provider runtimes need, e.g. continuation IDs/rate-limit metadata, or filter/validate per key instead of returning every metadata entry.
Useful? React with 👍 / 👎.
|
Fixed the exact-head Codex P1 provider-metadata snapshot finding in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66eeec79af
ℹ️ 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".
| host_worker = _ExecToolWorker(host, fresh_workspace_descriptor, capability_kind="host") | ||
| policy_worker = _ExecToolWorker(_policy_capability(authorize), fresh_workspace_descriptor, capability_kind="policy") | ||
| for name in sorted(tools): | ||
| tool_workers[name] = _ExecToolWorker(tools[name], fresh_workspace_descriptor) | ||
| before = _snapshot(fresh_workspace, fresh_workspace_identity, fresh_workspace_descriptor); before_ref = put("workspace_before", before, None) |
There was a problem hiding this comment.
When a host/tool executor module has import-time or restore-time filesystem side effects, the worker bootstrap runs it with cwd set to the fresh workspace and write permission before this snapshot is captured. Those writes or deletes then become part of workspace_before even though they were not in the plan's initial_workspace_sha256, so a replay can publish completed evidence whose baseline already includes unplanned mutations. Capture workspace_before immediately after _materialize_initial, or keep capability workers read-only until after the baseline snapshot.
Useful? React with 👍 / 👎.
| if row["location_kind"] == "object_ref" and row["location"] != row["sha256"]: | ||
| raise ReplayExecutionError("CAS object_ref locations must equal the content digest") |
There was a problem hiding this comment.
For manifests that use location_kind: "object_ref" with a remote object-store URI, this validator rejects records that the public bb.replay_artifact_manifest.v1 schema allows. That means any replay artifacts stored outside the local CAS cannot be loaded through ReplayArtifactManifest.from_dict; either accept the schema's URI form here or tighten the public schema to CAS-only.
Useful? React with 👍 / 👎.
|
Fixed both exact-head Codex P2 findings in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e42531c7e
ℹ️ 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 allow_process: | ||
| candidates.append("/") |
There was a problem hiding this comment.
When allow_process=True for the host worker, adding / to the sandbox read roots causes the Linux Landlock rules built from read_roots to grant read access to the entire filesystem; the Darwin branch similarly skips the file-read deny when process execution is allowed. In the inspected replay host path, an approved host.execute command can therefore read arbitrary files outside the fresh workspace and feed their contents into tool outcomes/provider messages, bypassing the intended capability isolation and redaction boundary.
Useful? React with 👍 / 👎.
|
Fixed the exact-head Codex P1 host-subprocess read isolation finding in
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f56c0f297a
ℹ️ 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".
| ] | ||
| verify_paths = ssl.get_default_verify_paths() | ||
| candidates.extend((verify_paths.cafile, verify_paths.capath)) | ||
| candidates.extend(path for path in sys.path if isinstance(path, str) and path) |
There was a problem hiding this comment.
When replay runs from a source checkout or with the project root on PYTHONPATH, adding every sys.path entry to the Landlock/macOS read allowlist grants the whole checkout as a readable subtree. Since the fresh replay workspace lives under .breadboard/replays/.../workspace, a host subprocess such as /bin/cat can still read sibling files under that checkout (for example ./secret.txt) even though they are outside the fresh workspace; _ExecToolWorker explicitly propagates the parent sys.path via PYTHONPATH, so this affects the normal dev/source-run path. Please narrow the runtime allowlist to import/runtime directories that are not ancestors of the replay workspace, or import what is needed before applying the filesystem sandbox.
Useful? React with 👍 / 👎.
|
Exact-head review requested for Provider route identities now derive dependencies from the same full, transient-sanitized provider state envelope used for worker reconstruction. Replay-affecting collaborators in external modules are content-bound; transient replay client caches remain excluded. Acceptance: @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95893358e9
ℹ️ 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".
| "model_id": provider_model, "model_revision": model_revision, "started_at_utc": call_started, "completed_at_utc": active_clock.now(), | ||
| "duration_ms": call_duration, "status": exchange_status, "request_payload_sha256": request_payload_sha256, | ||
| "response_payload_sha256": response_payload_sha256 if exchange_status == "completed" else None, "finish_reason": finish_reason, | ||
| "usage": usage, "evidence_refs": [response_ref.digest], "fallback_used": False, "problem": exchange_problem, |
There was a problem hiding this comment.
When a replay has multiple provider turns, each provider_request artifact is only added to the global manifest and this exchange records only the response artifact, so consumers cannot reliably tell which redacted request artifact belongs to this request_id/attempt_id without depending on manifest insertion order. Include request_ref.digest in the exchange evidence refs (or another explicit request reference) so each exchange remains self-contained and auditable.
Useful? React with 👍 / 👎.
|
Exact-head review requested for Each provider exchange now explicitly references both its redacted request artifact and response/error artifact. Multi-turn exchanges can be audited by request/attempt ID without manifest-order assumptions. Acceptance: @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact-head review requested for This bounded remediation closes the prior exact-head findings: provider context is frozen into the model-policy binding; post-rename rollback removes the published tree before CAS rollback; worker payloads use a pre-populated descriptor with bounded startup; final snapshots check cancellation/total deadline between reads; module access follows a content-bound closure; local host process creation is kernel-denied; unsupported Windows execution fails before storage; exchanges link request and response evidence explicitly. Acceptance: @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e9907adf
ℹ️ 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".
| paths.update((location, os.path.realpath(location))) | ||
| if dependency_files is not None: | ||
| dependency_files.add((f"{module_name}:{candidate.name}/{package_init.name}", location)) | ||
| _add_package_module_allowlist(package_name, runtime_roots, paths, exact_specs, dependency_files) |
There was a problem hiding this comment.
Fresh evidence after the sibling-file fix is that a top-level capability module still recurses into every sibling importable package here. When the capability lives in a PYTHONPATH directory that also contains an unrelated package with resource files or credentials, _add_package_module_allowlist() grants and binds reads for that whole package, so a provider/tool worker can import or read resources that were never part of the frozen capability; limit this to discovered imports or explicitly declared resource roots.
Useful? React with 👍 / 👎.
|
Governance escalation: PR #44 is now draft and must not merge. Frozen packet bb-06u.11 (sha256:f1de966ab784004e37910dbde83c591ac0838a1c9175a19643fba61c3f03862b) permits <=800 non-generated changed lines, 3 implementation attempts, and 3 independent review rounds. Exact base e1f5d84 to remote head b4e9907 is +6290/-32 over 35 commits; the PR has 36 COMMENTED reviews and zero APPROVED. Unpushed local work raises the effective diff to +7997/-33 and overlaps NS09B deadline/cancellation/provider-conformance scope. Under governance sha256:d7aa8facdf15621aee0f4ecd77bbc1203e5b0a5a64e1807da0a44ab5dff8cd61, this is BLOCKED_BUDGET/HUMAN_DECISION. A human A3 must authorize a versioned split/new packet or budget before more NS09A mutation. All candidate-dependent evidence/reviews must then rerun; no quality waiver or QC/envelope change is proposed. |
1 participant
Tracker: bb-06u.11
Implements the evidence-backed executable replay core, fresh capability-isolated workers, provider alias/client-state handling, process cleanup, and replay artifact validation.
Compat reviewed: non-breaking
Verification: 149 focused acceptance tests passed.