feat: add secure public trajectory upload - #989
Conversation
|
CI note: manifest-parity is failing on external agents@ce26cbc because OpenClaw manifest main contains the GPT-5.4 shim update while benchflow main does not yet. This reproduces locally and is unrelated to this PR. The focused sync is #986, whose full CI including parity is green. I am intentionally not folding that unrelated OpenClaw change into this trajectory-upload PR; I will refresh this branch from main after #986 merges and rerun all checks. |
![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: 8b4d21f3f5
ℹ️ 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 redact_value(value: Any, *, field_name: str | None = None) -> tuple[Any, int]: | ||
| """Return a structurally redacted JSON value and replacement count.""" | ||
| if isinstance(value, Mapping): | ||
| redacted: dict[Any, Any] = {} |
There was a problem hiding this comment.
When a denylisted field contains a mapping, sequence, or non-string value, these early branches run before the sensitive-key check, so inputs such as {"credentials":{"token":"opaque-secret"}}, {"secret":["opaque-secret"]}, or {"password":123456} receive zero redactions. The validator uses the same function for its final scan, so these secrets are accepted and promoted into the public trajectory dataset; redact the entire value of a sensitive field before dispatching on its type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf0c95c. Sensitive-key handling now runs before mapping, sequence, string, or scalar dispatch and replaces the entire value with [REDACTED]. The regression covers mapping, list, and integer sensitive values, and the validator reuses the same fail-closed function.
| if path.name == "llm_trajectory.jsonl": | ||
| try: | ||
| records = load_llm_trajectory_jsonl(path, strict=True) | ||
| except PrimeSftTrajectoryJsonlError as exc: | ||
| raise CaptureRejected(str(exc)) from exc |
There was a problem hiding this comment.
For an anonymously uploaded artifact named llm_trajectory.jsonl, load_llm_trajectory_jsonl() calls Path.read_text() and lets UnicodeDecodeError escape rather than wrapping it in PrimeSftTrajectoryJsonlError. Since run_once() catches only CaptureRejected, a client can upload correctly hashed invalid UTF-8 and make every validator execution crash without deleting the queue message or quarantine prefix, repeatedly consuming the public validator job until the message expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf0c95c. Upload validation no longer special-cases llm_trajectory.jsonl through the whole-file loader; all JSONL now uses the streaming validator, whose UnicodeDecodeError path becomes a terminal CaptureRejected. Client and service regressions use invalid UTF-8 in an llm_trajectory.jsonl file.
| if self._capture_status(digest) in {"ingested", "rejected"}: | ||
| self._delete_message(message) | ||
| return True |
There was a problem hiding this comment.
If a contributor reuses a still-valid SAS after a capture becomes ingested or rejected, the new manifest event takes this branch and only deletes the queue message. The grant includes create/write permission, and the returned If-None-Match header is controlled by the anonymous client, so it can be omitted to recreate or repeatedly overwrite every scoped blob after validation; those unvalidated blobs and versions then remain until lifecycle expiry, enabling substantial storage-cost abuse. Clean the terminal prefix before acknowledging such events and prevent further writes where possible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf0c95c. Terminal duplicate/replay events now clean the exact inbox// prefix before acknowledging the queue message. The regression seeds replayed quarantine blobs for both ingested and rejected states and requires the prefix to be empty.
| if path.name == "llm_trajectory.jsonl": | ||
| try: | ||
| records = load_llm_trajectory_jsonl(path, strict=True) |
There was a problem hiding this comment.
For llm_trajectory.jsonl, this loads the whole file as text, splits it into another full set of strings, and retains every parsed record in a list. The public contract accepts artifacts up to 1 GiB while the deployed validator is limited to 1 GiB of memory, so a valid capture well below the advertised limit can OOM both staging and server validation and remain unpromoted; validate this format incrementally or lower the accepted size to a bound the job can process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf0c95c. LLM trajectories now take the same line-by-line validation and secret-scan path as other JSONL rather than read_text/splitlines/list retention. The public per-artifact bound is also reduced from 1 GiB to 128 MiB (256 MiB per capture), and the event job is raised to 2 GiB for a defensible parsing envelope.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/benchflow-ai/benchflow/blob/cf0c95cee71683776e7b385290e6537b45f26aa3/src/benchflow/publish/azure_backend.py#L203-L205
The returned If-None-Match header is only advisory, so an anonymous caller can omit it and continue using the write-enabled SAS to recreate or repeatedly overwrite artifact blobs after validation, retaining large current blobs and versions until lifecycle expiry. The terminal cleanup added for the earlier report does not close this path: the deployed Event Grid filter at deploy-azure.sh:295-298 delivers only manifest.json events, so artifact-only replays never invoke the cleanup branch; enforce create-only/size-bounded writes or arrange cleanup for every post-terminal inbox write.
ℹ️ 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".
| blob = self.container.get_blob_client(prefix + relname) | ||
| expected_size = item.get("bytes") | ||
| if blob.get_blob_properties().size != expected_size: | ||
| raise CaptureRejected(f"size mismatch for {relname}") |
There was a problem hiding this comment.
When an anonymous client uploads a syntactically valid manifest but omits any declared artifact, get_blob_properties() raises Azure's ResourceNotFoundError, which is not converted to CaptureRejected and therefore escapes run_once() without deleting the queue message. Because the deployed queue retains messages for seven days, each such manifest repeatedly crashes validator executions and enough incomplete captures can starve legitimate uploads; treat a missing declared blob as a terminal capture rejection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 741dddb. ResourceNotFoundError from any declared manifest/artifact lookup is converted inside the download phase to CaptureRejected, so the exact quarantine prefix is cleaned, the ledger records rejected, and the queue message is deleted. The regression omits a declared artifact and requires terminal cleanup/no promotion.
|
Addressed the review-level artifact-only replay finding in 741dddb: SAS grants now use Azure BlobSasPermissions(create=True) with no write permission. This makes create-only enforcement a storage-service property rather than relying on the client-supplied If-None-Match header; omitting that header cannot overwrite an existing blob. A regression inspects the generated permission and requires create=true/write=false. I will also live-probe the second PUT without the precondition after deployment. |
|
Closed the remaining artifact-only replay path in 46b6a71. Event Grid now delivers every BlobCreated under Live production proof on the exact deployed image
Regression coverage requires pending artifact events to wait for the manifest and terminal artifact replays to be cleaned. The public stock CLI also completed a fresh anonymous demo upload at this image, digest |
|
@codex review |
|
@kywch @xdotli This is ready for the required independent human review: full local suite is green ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46b6a71256
ℹ️ 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: dict[str, Path] = {} | ||
| for item in artifacts: |
There was a problem hiding this comment.
An anonymous client can replace the granted manifest with one containing thousands of duplicate entries for the same valid, up-to-128 MiB artifact. This loop downloads every entry before validate_local_capture() applies ContributionManifest's eight-artifact and uniqueness checks, so a sub-1 MiB manifest can amplify into hundreds of gigabytes of repeated downloads, time out the public validator, and retry from the queue. Parse and validate the complete manifest contract before issuing any artifact storage requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca0df6b. The validator now parses and validates the complete ContributionManifest contract immediately after the bounded manifest download and before issuing any artifact get-properties or download call. A nine-entry duplicate manifest regression requires rejection with exactly one blob client request: manifest.json.
| manifest_data = json.loads(manifest_bytes) | ||
| artifacts = manifest_data["artifacts"] | ||
| except (json.JSONDecodeError, KeyError, TypeError) as exc: |
There was a problem hiding this comment.
Although invalid UTF-8 artifacts are now converted to CaptureRejected, this separate manifest decode still lets UnicodeDecodeError escape because the handler catches only JSONDecodeError, KeyError, and TypeError. An anonymous client can upload arbitrary manifest bytes through its scoped SAS, causing the validator job to fail without cleaning the prefix, recording rejection, or deleting the seven-day queue message, so the same event is retried repeatedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca0df6b. Manifest parsing is centralized in validate_manifest_bytes(), which converts UnicodeDecodeError, JSONDecodeError, and Pydantic ValidationError to terminal CaptureRejected. The invalid-UTF-8 regression requires rejected ledger state and queue acknowledgement.
| self._delete_message(message) | ||
| return True | ||
|
|
||
| if self._capture_status(digest) in {"ingested", "rejected"}: |
There was a problem hiding this comment.
With at-least-once Event Grid delivery and up to five concurrent validator executions configured in infra/trajectory-upload/deploy-azure.sh:278, two manifest events can both observe pending here. If one promotes and cleans quarantine while the other is still downloading, the second converts the resulting missing blob into rejection and overwrites the first worker's ingested ledger status, allowing the broker to reopen an already-promoted digest. Atomically claim the pending row, or otherwise prevent a later rejection from downgrading an ingested capture.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca0df6b. Manifest workers acquire an optimistic Azure Table ETag lease by transitioning pending to validating; only one worker owns a digest, active-lease duplicates leave their queue message for retry, and expired leases are reclaimable after a crash. The 35-minute lease exceeds the Job's 30-minute timeout. Broker re-grants preserve pending/validating and rejected digests cannot reopen. Terminal state is recorded before cleanup so replay events always observe the terminal state.
|
Production validation for
Focused service/CLI tests: 51 passed; full Ruff/format/type checks green. Requesting another exact-head pass after these fixes: @codex review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c67d4e630
ℹ️ 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".
| manifest_path.write_text( | ||
| json.dumps(manifest, indent=2, sort_keys=True, allow_nan=False) + "\n", | ||
| encoding="utf-8", | ||
| ) |
There was a problem hiding this comment.
When result.json or config.json contains a scalar run-metadata field large enough to make this generated file exceed MAX_MANIFEST_BYTES, staging still succeeds because the broker handshake omits run and therefore accepts the request. The validator then terminally rejects the oversized manifest, and because the digest covers only artifacts, even retrying after removing the large metadata is blocked by the rejected ledger row; direct mode instead publishes a manifest the shared contract cannot consume. Fresh evidence beyond the earlier artifact-limit fix is that the final serialized manifest is never checked against its 1 MiB bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| replacement_count += replacements | ||
| else: | ||
| shutil.copyfile(source, target) | ||
| payloads.append(_staged_file(target, relname, "application/jsonl")) |
There was a problem hiding this comment.
For inputs near the file or capture limit, structural redaction can substantially increase the staged size—for example, many short password values become [REDACTED]. The limits at lines 117–124 apply only to the source bytes, so this appended StagedFile may exceed the 128 MiB artifact or 256 MiB capture contract; broker mode then rejects output produced by the client itself, while direct mode uploads an out-of-contract capture. Fresh evidence after the prior direct-limit fix is that its checks still run before this size-expanding transformation.
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: d124e18b87
ℹ️ 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 _is_sensitive_key(field_name: str) -> bool: | ||
| normalized = re.sub(r"([A-Z]+)([A-Z][a-z])", r"\1_\2", field_name) | ||
| normalized = re.sub(r"([a-z0-9])([A-Z])", r"\1_\2", normalized) | ||
| normalized = normalized.casefold().replace("-", "_") | ||
| return normalized in DENYLISTED_KEYS or normalized.endswith( |
There was a problem hiding this comment.
After the camelCase/hyphen hardening, separators such as spaces and dots are still left intact, so common records like {"api key":"opaque-secret"}, {"client secret":"opaque-secret"}, or {"access.token":"opaque-secret"} produce zero replacements. Because the validator uses the same redact_value function, these prefixless credentials pass the final scan and can be promoted publicly; normalize non-alphanumeric separators to underscores before applying the denylist and suffix checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 939cf025afce. Sensitive field normalization now converts every non-alphanumeric separator run to _ after camelCase/acronym splitting, so api key, client secret, and access.token match the same denylist/suffix rules as their snake/camel/kebab forms. Client redaction and validator-side raw rejection share this function; regressions cover the new separator shapes in both paths.
| status = self._capture_status(digest) | ||
| if status == "ingested": | ||
| raise AlreadyUploaded( | ||
| base_url=self.container_url, | ||
| prefix=f"sources/community/{digest}/", |
There was a problem hiding this comment.
Requests for an ingested digest return here before _consume_rate_limit() runs, and the rejected branch has the same behavior. An anonymous caller can therefore upload or reject one tiny capture and then issue unlimited valid handshakes for that digest, each forcing a serialized Azure Table lookup on the single-replica public broker while bypassing its advertised 20-request hourly limit; consume the rate-limit allowance before these terminal-status returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 939cf025afce. The durable per-IP quota is now consumed under the broker lock before any capture-ledger lookup, including terminal ingested and rejected digests. The regression sets a one-request quota for each terminal status, verifies the first request returns its terminal response, and requires the second to raise RateLimited before another terminal lookup.
| raw_rewards = result.get("rewards") | ||
| rewards: Mapping[str, Any] = raw_rewards if isinstance(raw_rewards, Mapping) else {} | ||
| return { |
There was a problem hiding this comment.
When a selected trial contains an agent-produced result.json or config.json that is very large, read_text() materializes the entire file even though only six scalar metadata fields can reach the bounded manifest. A malformed trial can therefore make bench traj upload exhaust local memory before the final manifest-size check; inspect the file size and reject or ignore metadata beyond a small bound before reading it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 939cf025afce. Optional result.json and config.json metadata is now read through a bounded binary stream capped at 1 MiB plus one sentinel byte; oversized metadata is ignored before parsing, so no unbounded read_text/read_bytes allocation occurs. The regression shrinks the bound, makes unbounded Path.read_text fail if called, and verifies the oversized agent field never enters the manifest.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2304df85c
ℹ️ 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".
| payloads: list[StagedFile] = [] | ||
| replacement_count = 0 | ||
| for source in resolved.files: | ||
| relname = f"trajectory/{source.name}" |
There was a problem hiding this comment.
When an input is named TRACE.JSONL, my trace.jsonl, or uses other non-allowlisted characters, _resolve_input() accepts it and this line preserves the filename in the manifest. The broker subsequently rejects it because services/trajectory_upload/contract.py only permits trajectory/[A-Za-z0-9._-]{1,128}.jsonl, while --direct bypasses that validation and publishes a manifest that the shared contribution contract cannot parse. Apply the artifact-name constraint, or normalize the name, during staging for both transports.
Useful? React with 👍 / 👎.
| def test_broker_validation_errors_are_fail_closed_and_json_safe( | ||
| tmp_path: Path, | ||
| ) -> None: | ||
| """Guards the live Azure fix after malformed handshakes returned HTTP 500.""" |
There was a problem hiding this comment.
This docstring explicitly describes a regression—malformed handshakes previously returning HTTP 500—but identifies neither the PR nor the commit containing that behavior or its fix. Name the relevant PR/commit so the test complies with the repository's regression-test traceability requirement.
AGENTS.md reference: AGENTS.md:L17-L17
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: 939cf025af
ℹ️ 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".
| "_token", | ||
| "_secret", | ||
| "_password", | ||
| "_passwd", |
There was a problem hiding this comment.
After the earlier field-name hardening, the current predicate still recognizes only names ending in _passwd, not the common bare passwd field. A record such as {"passwd":"opaque-prefixless-value"} therefore receives zero replacements in both staging and validator scans, causing the original credential to be promoted publicly; include unambiguous bare credential names such as passwd in the denylist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1dba71bb052. Bare passwd is now explicitly sensitive, and the audit generalized the predicate to compact lowercase variants and every existing credential suffix family, so shapes such as clientsecret, accesskey, and servicecredentials cannot bypass separators/case normalization. Client and validator share the predicate; focused regressions cover these forms.
| .replace("+00:00", "Z"), | ||
| "source_id": source_id, | ||
| "traj_digest": f"sha256:{traj_digest}", | ||
| "uploaded_by": uploaded_by, |
There was a problem hiding this comment.
When BENCHFLOW_TRAJ_UPLOADED_BY exceeds 256 characters, staging accepts it even though UploadRequest.uploaded_by has max_length=256. Broker mode consequently rejects client-generated output, while --direct publishes a manifest that ContributionManifest cannot parse; apply the shared bound before either transport is selected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1dba71bb052. MAX_UPLOADED_BY_LENGTH = 256 now lives in shared staging and is imported by the service contract. Staging rejects an oversized contributor label before manifest construction for both broker and direct paths. The same parity audit moved canonical artifact-name validation into the shared module and added direct-path regressions for spaces, uppercase extensions, and overlong names.
| if len(payload) > MAX_RUN_METADATA_BYTES: | ||
| return {} | ||
| value = strict_json_loads(payload) | ||
| except (OSError, UnicodeDecodeError, ValueError): |
There was a problem hiding this comment.
If an agent-produced result.json or config.json contains deeply nested but sub-1 MiB JSON, strict_json_loads() raises RecursionError, which this handler does not catch. The optional metadata then aborts bench traj upload instead of being ignored like other malformed metadata; catch recursion failures here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1dba71bb052. Optional metadata parsing now catches RecursionError alongside other malformed/invalid metadata failures and ignores the file. A regression supplies a 1,200-level sub-1 MiB result.json and requires staging to finish without importing metadata or leaking the exception.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1dba71bb0
ℹ️ 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 hasattr(app.state, "backend"): | ||
| app.state.backend = app.state.backend_factory() |
There was a problem hiding this comment.
When the scale-to-zero broker receives simultaneous first requests, FastAPI can run these synchronous handlers in separate worker threads, allowing this unsynchronized lazy check to construct multiple AzureUploadBroker instances. Each instance then has a different _state_lock, so concurrent _consume_rate_limit() read/upsert cycles can lose increments and let a cold-start burst bypass the durable 20-request hourly limit; initialize the backend during app startup or guard this initialization with a shared lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae2c32756987. Each FastAPI app now owns one shared initialization lock, and _backend() uses a double-checked critical section before calling the factory. All first-request threads therefore receive the same AzureUploadBroker and its single quota lock. An 8-thread barrier regression widens factory construction deliberately and requires exactly one construction plus identical backend identity for every caller.
| if isinstance(value, Sequence) and not isinstance(value, (str, bytes, bytearray)): | ||
| redacted_items = [] | ||
| replacements = 0 | ||
| for item in value: | ||
| clean, count = redact_value(item) |
There was a problem hiding this comment.
When a command is represented as an argv-style array such as {"command":["tool","--api-key","opaque-prefixless-value"]}, this branch scans each element independently, so neither the flag nor its following prefixless credential produces a replacement. Because client staging and the validator both use redact_value, the original secret can pass the final gate and be promoted publicly; recognize sensitive option elements and redact their following argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae2c32756987. Structural redaction now recognizes sensitive long-option flags in argv arrays, redacts the following value, handles inline --secret=value, and applies the same logic to shell-command strings via shlex while leaving strings unchanged when no replacement occurs. Regressions cover --api-key, --client-secret, --access_token=value, shell strings, and validator-side rejection of an unredacted argv capture.
| if len(manifest_bytes) > MAX_MANIFEST_BYTES: | ||
| raise CaptureRejected("manifest exceeds the 1 MiB limit") | ||
| try: | ||
| raw_manifest = strict_json_loads(manifest_bytes) |
There was a problem hiding this comment.
When an anonymous uploader supplies a manifest encoded as UTF-16 or UTF-32, Python's json.loads() auto-detects the byte encoding here, so no UnicodeDecodeError is raised and the validator promotes the original non-UTF-8 manifest bytes. Fresh evidence beyond the prior invalid-UTF-8 fix is that a BOM-prefixed UTF-16 serialization successfully reaches ContributionManifest.model_validate; explicitly decode with UTF-8 before calling the JSON parser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae2c32756987. Manifest bytes are explicitly decoded as UTF-8 before strict JSON parsing; decoding failures remain terminal invalid manifest rejections. Parametrized regressions require UTF-16 and UTF-32 JSON manifests to fail, while the existing invalid-byte queue test continues to prove terminal ledger rejection and cleanup.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae2c327569
ℹ️ 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".
| clean, count = redact_value( | ||
| item, | ||
| field_name=key if isinstance(key, str) else None, | ||
| ) |
There was a problem hiding this comment.
When a trajectory uses BenchFlow's own ACP header representation, such as {"headers":[{"name":"Authorization","value":"opaque-prefixless-value"}]} (produced by src/benchflow/acp/types.py:163), these recursive calls treat name and value independently, so the credential label is never applied to its sibling value and the record receives zero replacements. The validator reuses redact_value, allowing the prefixless authorization value to be promoted; recognize structured name/value and key/value carriers when propagating sensitive-field context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 671ee89. Structured name/value and key/value carriers now propagate sensitive label context to value/values fields case-insensitively. Client and validator regressions cover a prefixless Authorization value.
| except CaptureRejected as exc: | ||
| logger.warning("capture %s rejected: %s", digest, exc) | ||
| self._record_status(digest, "rejected", detail=str(exc)[:512]) |
There was a problem hiding this comment.
When an anonymous caller knows a capture's artifact names and hashes, they can obtain its manifest SAS and upload an invalid manifest such as {} without possessing any artifact bytes. This branch then records the content digest as rejected; subsequent handshakes hit AzureUploadBroker.create_upload's terminal rejected branch, and there is no expiry or reset, so the legitimate exact capture is blocked indefinitely. Scope rejection to an upload attempt or otherwise allow a safe new attempt instead of creating a global digest tombstone from unauthenticated manifest bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 671ee89. Quarantine grants are now attempt-scoped, rejected digests reopen under a fresh isolated attempt, and stale Event Grid events clean only their old prefix without mutating the active ledger. Legacy in-flight prefixes remain supported. Added broker retry, valid promotion, and stale-event regressions.
| if redact: | ||
| redacted_manifest, manifest_replacements = redact_value(manifest) |
There was a problem hiding this comment.
When an allowed filename itself resembles a credential, such as sk-abcdefghijklmnop.jsonl, whole-manifest redaction changes its manifest entry to trajectory/***REDACTED***.jsonl while the corresponding StagedFile retains the original name. Broker mode then rejects the generated handshake because the rewritten name violates the artifact-name contract, while direct mode uploads the original object alongside a manifest that cannot describe or validate it; exempt structural artifact fields from free-text redaction or reject such names before staging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 671ee89. Shared artifact-name validation now rejects credential-like filenames before staging and is reused by the broker contract, so manifest redaction cannot diverge from the staged blob name. Client and HTTP contract regressions cover this.
GitHub Actions
Failure
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 671ee89909
ℹ️ 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".
| raise CaptureRejected("invalid manifest: content must be UTF-8") from exc | ||
| try: | ||
| raw_manifest = strict_json_loads(manifest_text) | ||
| manifest = ContributionManifest.model_validate(raw_manifest) |
There was a problem hiding this comment.
When an anonymous manifest uses coercible but schema-invalid values such as "bytes":"123", "bytes":123.0, or a numeric created_at, Pydantic's default lax validation normalizes them in the returned model, so integrity checks succeed, but the validator later promotes the original manifest_bytes. The public manifest therefore retains types that violate the declared JSON contract and can fail strict downstream consumers; validate these fields strictly or promote a canonical serialization of the validated model.
Useful? React with 👍 / 👎.
|
@kywch @xdotli @Galius5136 — exact head |
|
@bingran-you Thanks for tagging me! I went through Overall the structure looks solid, and I checked a bunch of the security-sensitive parts around SAS scope, path handling, logging/redaction, validator claiming, packaging, and the new tests. I did find three things I’d fix before merge. 1. Manifest validation vs promoted bytes
The checks then run against that normalized model, but the promotion path writes the original I reproduced cases where validation passes, but the promoted manifest still contains the original incompatible types and fails a strict re-parse of the contract. I think the cleanest options are either:
2. If That means something as normal as an apostrophe can cause an opaque secret passed through a sensitive CLI flag to survive redaction. For example:
I reproduced this on the current head with a few malformed/unbalanced command strings. I’d make that path fail closed, or add a conservative fallback redactor when 3. Delegation-key refresh can leave a stale key cached
In a controlled probe, a transient refresh failure left the old key cached with a future expiry timestamp, so the broker kept returning apparently valid handshakes for a while even though the later Azure PUT failed. I’d update the expiry only after a successful fetch and keep that refresh transition under the existing lock. A couple of smaller notes:
I also checked the surrounding paths and the SAS scoping, object-name validation, anonymous-read settings, logging behavior, validator claiming, filename allowlisting, packaging, and the targeted tests all look good from my side. I couldn’t independently reproduce the live Azure deployment/recovery flow because that needs the author’s Azure environment. I’d request changes for #1 and #2; I’d also fix #3 before merge, although I’m fine with maintainers deciding whether that one can be a fast-follow. Once you push a new head, I’ll re-check the exact commit and run a full local E2E on my machine before approving. |
2 participants
Summary
Production evidence
Security probes
Verification
Review focus
Please verify the one-command CLI boundary, manifest/redaction contract, broker SAS permissions, validator fail-closed promotion, duplicate-event behavior, and Azure IAM scopes. The branch has also passed the requested thermo-nuclear structural review; unrelated uv.lock resolver churn was removed.