Add RL rollout substrate and refresh kernel contracts - #27
Add RL rollout substrate and refresh kernel contracts#27kmccleary3301 wants to merge 24 commits into
Conversation
![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: 279e0dcd2e
ℹ️ 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".
|
|
||
|
|
||
| @ray.remote | ||
| class DockerSandboxV2(DevSandboxV2): | ||
| class DockerSandboxV2: |
There was a problem hiding this comment.
When BREADBOARD_SANDBOX_DRIVER=docker or RAY_USE_DOCKER_SANDBOX=1 selects this actor, it no longer inherits DevSandboxV2 but only reimplements part of that API. Existing agent paths still call methods such as self.sandbox.vcs.remote(...) and self.sandbox.edit_replace.remote(...) for VCS and search/replace tools, so those tools now fail with a Ray actor missing-method error under Docker. Please copy or compose the remaining sandbox methods (edit_replace, multiedit, list_files, vcs, etc.) before dropping the base class.
Useful? React with 👍 / 👎.
| engine_version = (os.environ.get("BREADBOARD_ENGINE_VERSION") or "0.1.0").strip() or "0.1.0" | ||
| app = FastAPI(title="BreadBoard CLI Bridge", version=engine_version) | ||
| _service = service or SessionService() | ||
| rl_service = LiveRLRunService(Path(os.environ.get("BREADBOARD_RL_RUN_STORE", ":memory:"))) |
There was a problem hiding this comment.
With BREADBOARD_RL_RUN_STORE unset, wrapping ':memory:' in Path(...) turns SQLite's in-memory DSN into a filesystem path; SQLiteRLRunStore then calls sqlite3.connect(self.path), creating a persistent file literally named :memory: and sharing default app state across instances. Keep ':memory:' as a string unless an env path is provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75e88e6495
ℹ️ 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".
| function connect() { | ||
| if (closed) return | ||
| closeSource() | ||
| source = new EventSourceCtor(buildEventSourceUrl()) |
There was a problem hiding this comment.
When BREADBOARD_API_TOKEN is set, this SDK path cannot open a stream even if config.authToken is provided: openEventStream constructs a native EventSource directly, but never resolves or sends the token, and the bridge auth middleware requires an Authorization header for /v1/sessions/{id}/events. In authenticated deployments, callers using CliSdkProvider.openEventStream will just reconnect on repeated 401s while the fetch-based streamSessionEvents path works.
Useful? React with 👍 / 👎.
| if (options.lastEventId) { | ||
| url.searchParams.set("from_id", options.lastEventId) | ||
| } |
There was a problem hiding this comment.
On a transient SSE error after events have already been delivered, reconnects still use only the original options.lastEventId (or replay=true from initial connection) because handleMessage never stores event.lastEventId for buildEventSourceUrl. In the common case with no initial last id, every reconnect replays the whole session log and duplicates already-applied events in the live UI; update a mutable last-seen id and use it as from_id on reconnect.
Useful? React with 👍 / 👎.
75e88e6 to
4729ac3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4729ac38b6
ℹ️ 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".
| from breadboard.rl.phase3.store import SQLiteRLRunStore | ||
|
|
||
| DEFAULT_CAPS = ResourceCaps(max_tasks=128, max_gpus=8, max_budget_usd=500.0, max_duration_seconds=7200, max_artifact_bytes=512 * 1024 * 1024) | ||
| DEFAULT_EGRESS_POLICY = EgressPolicy(allowed_prefixes=("ws",), max_artifact_bytes=DEFAULT_CAPS.max_artifact_bytes) |
There was a problem hiding this comment.
For any tenant whose workspace_id is not exactly ws, artifact collection/replay rejects otherwise valid artifacts: add_artifact accepts paths like workspace-a/replay/a.json after enforce_workspace_path, but collect/replay then apply this static policy and fail with path prefix is not egress-approved. Derive the allowed prefix from the run's workspace_id (or apply egress after workspace isolation without a hard-coded workspace name).
Useful? React with 👍 / 👎.
| { | ||
| "input_ids": _tensor(torch, tensors.get("input_ids"), device=device, dtype=getattr(torch, "long", None)), | ||
| "attention_mask": _tensor(torch, tensors.get("attention_mask"), device=device, dtype=getattr(torch, "long", None)), | ||
| "responses": _tensor(torch, tensors.get("input_ids"), device=device, dtype=getattr(torch, "long", None)), |
There was a problem hiding this comment.
When the Phase 3 live trainer builds a real VeRL DataProto, responses is set to the full prompt+completion input_ids while response_mask and old_log_probs are completion-length tensors. For any row with a non-empty prompt, this treats prompt tokens as generated responses and misaligns them with completion logprobs, corrupting PPO/GRPO updates. Use the padded completion_ids for responses so it matches the response mask/logprobs.
Useful? React with 👍 / 👎.
| last = self._conn.execute("select max(sequence) as seq from rl_events where run_id=?", (run_id,)).fetchone()["seq"] | ||
| sequence = int(last or 0) + 1 |
There was a problem hiding this comment.
In the FastAPI app a single SQLiteRLRunStore is shared while sync routes run in worker threads, so two operations appending events for the same run can both read the same max(sequence) before either insert commits. In that scenario one insert hits the (run_id, sequence) primary key and the request fails/drops an event; allocate the next sequence under a lock or an atomic SQLite transaction/counter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff90bc6596
ℹ️ 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 _row_ref(row: Mapping[str, Any]) -> dict[str, Any]: | ||
| return { |
There was a problem hiding this comment.
When projection rows use group_id to group multiple completions for GRPO, _row_ref drops that field before the batch reaches build_phase3_dataproto; the trainer then falls back to each row's unique task_id for uid and raises GRPO uid groups must contain at least two rows even though the input rows were correctly grouped. Include the normalized group_id in row_refs so valid GRPO batches are not rejected.
Useful? React with 👍 / 👎.
| verifier = _require_mapping(payload, "verifier", errors) | ||
| renderer = _require_mapping(payload, "renderer", errors) | ||
| replay = _require_mapping(payload, "replay", errors) | ||
| exports = _require_mapping(payload, "exports", errors) |
There was a problem hiding this comment.
When callers use lint_env_package/validate_env_package_mapping directly, a package with reward: {} and a matching package_hash receives no validation error because this required-mapping block skips reward and there is no later reward_id/kind check. That lets invalid env packages pass the lint gate and fail later in EnvPackage.from_dict, so require the reward mapping and its non-empty fields here.
Useful? React with 👍 / 👎.
ff90bc6 to
cba0bcc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cba0bcc7a0
ℹ️ 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 start(self, run_id: str) -> RunStatus: | ||
| status = self.store.status(run_id) | ||
| self.store.update_state(run_id, state="running") |
There was a problem hiding this comment.
When a run has already been rejected by submit due to resource-cap failures, start() still rewrites it to running because it does not check the current state before calling update_state; the same live service is what the scheduler/API bridge shares, so a stale or unconditional scheduler start by run_id can revive a rejected run and later mark it successful. Preserve the Phase 2 state-machine checks here so only queued runs can start.
Useful? React with 👍 / 👎.
|
|
||
| def resource_cap_rejections(submission: RunSubmission, caps: ResourceCaps) -> list[str]: | ||
| rejections: list[str] = [] | ||
| if submission.requested_tasks > caps.max_tasks: |
There was a problem hiding this comment.
If a client submits requested_tasks=0 or a negative value, this helper only checks the upper bound and returns no rejection, so the live /runs route queues a run that has no meaningful task count; the same max-only pattern applies to GPUs, budget, and duration because the API model does not set lower bounds. Add lower-bound validation before accepting the run.
Useful? React with 👍 / 👎.
| if graph.edges and len(graph.edges) != max(0, len(graph.nodes) - 1): | ||
| errors.append("session_order graph must have node_count - 1 edges") |
There was a problem hiding this comment.
For any trajectory graph with multiple nodes and zero edges, this condition is skipped entirely because it is guarded by graph.edges, so validate_graph_invariants accepts a disconnected session even though replay/credit code treats the node sequence as ordered by session_order edges. Remove the truthiness guard so multi-node graphs missing edges are rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 137608213a
ℹ️ 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 ReplayParityReport( | ||
| parity_tier="T2_deterministic_runtime_verifier", | ||
| passed=not mismatches, | ||
| mismatches=mismatches, |
There was a problem hiding this comment.
For a replay with the same node/edge kinds and terminal reward but a different step payload, such as a different submitted action or observation, mismatches remains empty and this returns passed=True. decide_export_admission can then mark a non-reproduced trajectory exportable, so please include a stable comparison/hash of the node payloads or at least the action/observation/evidence fields before reporting parity success.
Useful? React with 👍 / 👎.
| record = RenderedTurnRecord.from_dict(record_payload) | ||
| except ValueError as exc: | ||
| return [*errors, str(exc)] | ||
| errors.extend(validate_rendered_turn(record)) |
There was a problem hiding this comment.
When validating an exported payload loaded from disk, the supplied trainability section is never checked against classify_rendered_turn_trainability(record). If a record becomes non-trainable due to finish_reason='length', overlong_prompt, or a bridge failure but the old or tampered trainability.sft_trainable=true remains, validation still returns no error and downstream export gates can train on blocked rows; recompute and compare the trainability block here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e16ef5aa0e
ℹ️ 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".
| "artifact_paths": {key.removesuffix(".json"): str(OUT / key) for key in artifacts}, | ||
| "artifact_payloads": {key.removesuffix(".json"): {k: v for k, v in payload.items() if k != "body"} for key, payload in artifacts.items()}, | ||
| "input_hashes": {"probe_source": _source_sha256()}, | ||
| "required_artifact_keys": [key.removesuffix(".json") for key in artifacts], |
There was a problem hiding this comment.
When this probe emits a passing P3-M11 component report, required_artifact_keys lists all seven metric artifacts, but input_hashes only contains probe_source. The final-report validator iterates each required artifact key and requires a matching hash, so otherwise-valid live endpoint evidence is rejected with input_hashes.<artifact> must be present; compute and include the SHA256 for every artifact written by _write_artifacts.
Useful? React with 👍 / 👎.
| request = urllib.request.Request(url, data=body, headers=headers, method=method) | ||
| with urllib.request.urlopen(request, timeout=REQUEST_TIMEOUT_SECONDS) as response: # noqa: S310 - explicit target endpoint probe. |
There was a problem hiding this comment.
In deployments where a configured verifier, object-store, or scheduler probe endpoint responds with a redirect, this default urlopen path follows it after _token_headers has added the bearer token, and the local-endpoint check only examined the original URL. That can forward probe credentials to a redirected host or a local address and record metrics from an unapproved endpoint; use a no-redirect opener or revalidate the redirect target before preserving auth headers.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR adds
breadboard/rl/, a package that takes recorded agent sessions and turns them into structured records other tools can train on, replay, and audit. It also refreshes the kernel contract layer those records depend on: schemas, conformance fixtures, generated client bindings, and the CLI and TUI code that reads them.The split is deliberate. Agent execution keeps doing one job, recording what happened during a session. The new package reads those recordings and decides what is eligible to export, how to replay it, and how to serve it through a typed run API. RL logic stays out of the generic agent runner.
If you are reviewing this cold, start with the contract layer, then read
breadboard/rl/from the data model up to the API.What's in the package
breadboard/rl/is grouped by concern:The data model is the part to understand first. A recorded session becomes a trajectory graph with credit frames. The graph is validated, projected into token-level turns, and either marked trainable or held back. Replay and state references stay stable, so a run can be rebuilt and checked later.
Run service and API
A run service sits on top of the package, backed by a durable store and a typed web router mounted by the CLI bridge. The API covers the run lifecycle: submit a run, look up its status, stream its events, cancel it, list its artifacts, replay an artifact, and read its audit trail. Every request carries tenant and workspace identifiers, and the router refuses cross-workspace or cross-artifact access through the security layer.
Artifact writes stay inside the service and scheduler path. The public API has no general upload endpoint.
Kernel contracts and conformance
The kernel contract surface moves together with the code that uses it: event kinds, tool definitions, terminal records, effective tool surfaces, environment selectors, and session transcripts. This PR updates the schemas and adds example records, registries, fixtures, comparators, generated client bindings, and validator scripts.
Keeping them together means a schema, its example, its fixture, and its client consumer are reviewable in one place, so a contract change and its downstream effect land in the same diff.
CLI bridge and TUI
The CLI bridge and TUI client were updated to read the new contracts: event normalization and runtime emission, typed client responses, auth and upload handling, stream normalization for session events, and the auth, connect, doctor, and REPL command paths. Generated client checks are included.
Safety checks
The package adds local hardening for anything that runs exported data or verifiers: import-hook and symlink-escape checks, process-cleanup validation, reward-hack probes, and quarantine decisions for suspect runs. These protect the export and verification paths against obvious local tampering.
How to review
The branch is split into small commits. A workable order:
The boundary worth checking closely: RL logic should stay inside
breadboard/rl/and the typed API. If it turns up in the generic agent runner, flag it.Validation
Local checks that passed during branch prep:
Notes
Generated and fixture-heavy files are committed next to the code that produces or consumes them. Local capture caches and machine-specific run output are kept off the branch, and deployment-specific execution detail is out of scope for this PR.
Compat reviewed: non-breaking