fix(core): harden the eval harness from live A/B and BEAM runs - #1416
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d43c69b6e5
ℹ️ 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".
This was referenced Aug 31, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 628b027058
ℹ️ 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".
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
An unterminated slice concatenated the stderr footer onto the final stdout line in terminals and merged captures. Lead with the newline on stderr when content lacks one; stdout stays byte-exact for pipes. Found during live smoke testing on the 1398 eval branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
The live 100K tier mixes ints with one level of int-list groups in event_ordering source_chat_ids ([116, ..., [136, 138]]) when a single event's evidence spans chats — a shape the fail-fast loader rejected. Flatten one level of int lists into the union; anything deeper or non-int still fails fast. Fixture and tests carry the live shape. Found by the loader's own schema guard during the first real dataset fetch on the 1398 eval branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
The live 100K tier carries a third marker variant: '->-> 2,22, 24' — an id list with a space after a comma. The pattern now matches an int followed by comma-separated ints or N/A with optional spaces, staying narrow so unknown '->->'-bearing content still trips the fail-fast instead of being silently stripped. Fixture and tests carry all three observed variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Surveyed every marker in the live 100K tier (2,199 across five shapes) and closed the last gap: six markers carry a trailing ')' that is generator junk (zero opening parens in every affected message). The pattern now covers the complete observed population while unknown '->->' content still fails fast; comment records the survey. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Anthropic's OpenAI-compat endpoint rejects identity-linked API keys without an anthropic-workspace-id header, which the first live A/B run hit as an opaque 400 on all 24 tasks. Add a repeatable --model-header Name=value option that rides only in the model-factory closure — never in AgentTasksConfig, so header values can never leak into run artifacts — and include the response body in openai-compat errors so the next rejection names itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Claude 5 models reject the temperature parameter outright ('temperature
is deprecated for this model'), which the second live A/B hit on all 24
tasks — named precisely by the new error-body reporting. Add
--model-temperature (number, or 'omit' to drop the parameter), default
unchanged at 0 so local openai-compat servers stay pinned; the choice is
recorded in the run config for provenance.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
Signed-off-by: phernandez <paul@basicmachines.co>
project add registers a project but does not index it, so the DB stayed empty, status reported ready vacuously (zero pending work was ever queued), and every retrieval tool saw an empty project — the first real-model A/B ran 24 tasks against nothing. Run an explicit reindex --full --search after registration, before the settle wait. The scripted smoke masked this because canned answers never consult the index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Three fixes from the first real-model A/B run: - Answer-set graders now strip the task's OWN project prefix from answer-extracted permalinks (agents faithfully quote tool output, which is project-prefixed); a different project's prefix still fails, so cross-project leakage keeps failing. Six of the run's failures were this comparison artifact. - State-graded tasks run reindex --search (whose completion includes forward-reference relation resolution) before settling and grading; the rich curate-connect 'write was verified correct by argument fingerprint but grading saw 0 resolved relations' race is closed. - Task project names no longer double the at- prefix. Also diagnosed, agent-side by design (measured behavior, no harness change): agents sometimes omit the project argument and the warm MCP session routes calls to the previously named project — including writes, with the agent then 'verifying' in the wrong project. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Live-run DBs store entity permalinks project-prefixed
('at-<run>-<task>/notes/...'), so RelationResolves' relative-permalink
SQL could never match — curate-connect was ungradeable on every run for
every agent. Match both forms for the source and strip the task's own
project prefix from resolved targets before comparison; a target under
a different project's prefix still fails.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
Signed-off-by: phernandez <paul@basicmachines.co>
A programmatic caller constructing AgentTasksConfig and calling run_agent_tasks directly got the factory's default temperature while manifest.json recorded the configured value — a silent provenance lie in an artifact whose whole job is provenance. The CLI was unaffected because it pre-binds temperature into its factory. The default is now a None sentinel rather than the function itself, so the branch reads the module attribute at call time; an injected factory still owns its own configuration and is called unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
phernandez
force-pushed
the
1398-eval-test-fixes
branch
from
September 1, 2026 18:51
628b027 to
65de3a4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65de3a498e
ℹ️ 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".
An OpenAI-compatible gateway that rejects a request often quotes the offending request back in its 4xx/5xx body — a 401 can echo the bearer token, and proxies name the header they refused. `_post` appended that raw body to `LLMRunnerError`, the driver stored `str(exc)` on the errored `AgentTaskResult`, and that row is written to `per-task-agent.jsonl` and `summary.md`, which `publish` copies into the public results bundle. So the values accepted through `--model-header` and the resolved API key could land in a committed artifact, contradicting the promise those options carry that they never enter run artifacts. Redact the body against the exact values we hold (bearer token + every operator-supplied header value) before it reaches the error. Value-based rather than pattern-based masking is deterministic and cannot be beaten by an unfamiliar credential format. Redaction runs before the 300-char truncation, since a secret straddling the cut would otherwise survive as an unmatched prefix; secrets are masked longest-first so a value containing another is replaced whole. Regression tests cover the seam and the artifact: one writes the errored row through the driver's own `_write_jsonl` and asserts neither secret appears in the saved file while the rejection reason still does. Signed-off-by: phernandez <paul@basicmachines.co>
`--model-temperature nan|inf|-inf` parses cleanly through `float()` and passes `AgentTasksConfig` validation, but JSON has no encoding for those values: httpx raises a bare `ValueError: Out of range float values are not JSON compliant` when it serializes the request body. That is not one of `_post`'s handled transport failures, so it escapes the retry loop and the driver's task-level handling, aborting the whole run mid-flight — after surface setup, with no provenance artifacts written. Validate finiteness alongside the existing parse, so a bad value is a `BadParameter` before any setup cost is paid. Verified against httpx 0.28.1; a test pins the underlying httpx rejection so the guard's rationale does not silently rot. Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 943f2b33fa
ℹ️ 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".
HTTP allows any visible ASCII character in a header value, so `"` (0x22) and `\` (0x5C) are legal in a value passed through `--model-header`. When a gateway echoes such a value back inside a JSON error body it writes the *escaped* spelling — `"` as `\"`, `\` as `\\` — while `redact_secrets` searched only for the original plaintext. The replacement found nothing, and the credential rode the body through `LLMRunnerError` into `per-task-agent.jsonl` and `summary.md`, which `publish` copies into the public results bundle. The leak hid from the existing artifact test because that test scans the raw file text for the plaintext: the saved JSONL re-escapes the body, so the plaintext genuinely is absent while the secret stays recoverable verbatim from the decoded row. Mask every spelling the value can take instead of just the plaintext: the raw form, the form inside a JSON string, and the form inside a JSON string nested in another (a proxy wrapping an upstream body in a string field). A value with nothing to escape produces the plaintext at every level, so ordinary alphanumeric keys are unaffected. The existing properties hold: masking is still value-based rather than pattern-based, still runs before the 300-char truncation, and is still longest-first so a value containing another is replaced whole. Ordering now breaks ties on the form itself, since the forms are collected into a set and equal-length secrets would otherwise redact differently per run. Regression tests cover the seam and the artifact. The artifact test asserts on the decoded row rather than the raw file text, which is what makes it fail before the fix. Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf18a1fd38
ℹ️ 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".
Two input-validation gaps that only the CLI guarded, leaving the
explicitly supported `run_agent_tasks(config)` path open.
`model_temperature` now sets `allow_inf_nan=False`. nan/inf survive both
`float()` and Pydantic's plain float schema, so a programmatic caller
reached the request body where httpx raises a bare ValueError mid-run.
The substantive half is provenance: `model_dump_json()` encodes nan/inf
as `null`, which is exactly this field's "temperature omitted" sentinel,
so the recorded run config silently misreported what was sent — a run
that looks valid afterwards is worse than a crash.
`run_id` now must be argv- and path-safe. The driver splices it into a
`bm project add` argv token (project names are `{run_id}-{task.id}`) and
into two filesystem paths. `--run-id=-trial` made the name `-trial-<task>`,
which the BM CLI parses as options. That failed late — after the benchmark
home, a warm `bm mcp` subprocess, and a full corpus copy — and illegibly:
`run_command` captures stderr, so the operator saw a bare
`CalledProcessError ... exit status 2`, not the CLI's own "No such option:
-t". The abandoned home then blocked re-running the same run_id.
Both rules live on the model so the CLI and direct callers share one
source of truth. The CLI's existing finiteness check stays: it fires
before `create_tool_agent_model`, which is upstream of config
construction, and renders a `BadParameter` naming the actual flag rather
than a Pydantic traceback. Config construction is wrapped for the same
reason, without duplicating any rule.
Signed-off-by: phernandez <paul@basicmemory.com>
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cef513e92
ℹ️ 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".
`redact_secrets` masked each secret in the spellings `json.dumps` produces, but a JSON encoder may spell any character as `\uXXXX` and encoders disagree about which: Go's html-safe default emits `\u003c` for `<`, PHP emits `\/` for `/`. A `--model-header` value containing `<`, `>` or `&`, echoed by a Go-based gateway, therefore rode the 401 body into `per-task-agent.jsonl` and the public results bundle intact. Enumerating encoder-specific spellings only lengthens an incomplete list. Instead the precise masking is kept for the common case -- it is why the body stays readable at all -- and backed by a safety net: the masked text is unescaped into a *view* by regex, never `json.loads`, so a body truncated mid-string cannot raise, and a secret still visible in that view costs the whole body rather than the credential. Unescaping repeats to a fixpoint (each pass strictly shortens the text, so it terminates) to reach a secret escaped once upstream and again by the proxy that wrapped it, and the check runs after every pass because a secret containing a backslash can be revealed by one pass and consumed as an escape prefix by the next. Control escapes decode to their characters so a multi-line diagnostic is not falsely withheld. The rejection still reaches the operator: the HTTP status and URL come from the underlying exception, not the body. Reported by Codex on #1416. Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 946dd862af
ℹ️ 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".
… name Two credential-disclosure paths in the agent-task tool seam, from PR #1416 review. The first is the fourth round of the same leak. Each earlier round masked one origin -- an echoed 401 body, its JSON-escaped spelling, its unicode-escaped spelling -- and the next origin arrived unredacted anyway. This one is not a response body at all: an operator header value containing CR or LF survives --model-header (which strips only surrounding whitespace), h11 refuses to serialize it, and its LocalProtocolError quotes the whole value, which the harness interpolates straight into LLMRunnerError and thence into per-task-agent.jsonl. Observed against a live socket: openai-compat call to http://127.0.0.1:57035/v1 failed after 1 attempts: Illegal header value b'wrkspc_secret\nleak' Patching per-origin is losing, so this moves the scrub to the choke point. OpenAICompatToolAgent is the only object that holds the credentials, and it emits text to the outside world through exactly one channel: the message of an LLMRunnerError. A new _error method is now the single place in the class that constructs one, and it runs the foreign half of every message through the existing redact_secrets. That is upstream of every sink -- artifact, console and summary alike -- which the artifact writer in driver.py is not; putting it there would also have meant threading the secret set into a module that never sees it today. Messages are split into a harness-authored summary and a foreign detail, and only the detail is redacted, because redact_secrets replaces its whole input when masking provably failed. That keeps the trade the withhold marker documents: the operator still learns which endpoint failed and how, even when the diagnostic itself has to be dropped. The second defect is header merge semantics. HTTP header names are case-insensitive, but the merge used a plain dict, so an operator passing --model-header authorization=... alongside an exported OPENAI_API_KEY produced both an Authorization and an authorization entry, and httpx serialized both: [(b'Authorization', b'Bearer sk-ambient-DO-NOT-SEND'), (b'authorization', b'Bearer op-token-intended')] The endpoint then picks one, and the ambient credential is disclosed to an endpoint the operator never meant to hand it to. Headers are now merged through httpx.Headers, whose item assignment drops every existing entry with that name -- the merge HTTP actually describes -- with operator headers applied last, one at a time so a repeated --model-header cannot smuggle two spellings through as a mapping. Operator headers override rather than being refused: --model-header is a deliberate choice made for this run, while the bearer is derived from whatever the shell happens to export, so the explicit value is the one that should survive, and refusing the pair would strand the common case of a shell that exports OPENAI_API_KEY for unrelated tools. Five regression tests, each failing on the pre-fix source: the CR/LF header value at the seam and again at the saved artifact, the withheld-detail split, the ambient-versus-operator authorization collision, and two operator spellings of one name. Signed-off-by: phernandez <paul@basicmachines.co>
phernandez
added a commit
that referenced
this pull request
Sep 1, 2026
`redact_secrets` masked each secret in the spellings `json.dumps` produces, but a JSON encoder may spell any character as `\uXXXX` and encoders disagree about which: Go's html-safe default emits `\u003c` for `<`, PHP emits `\/` for `/`. A `--model-header` value containing `<`, `>` or `&`, echoed by a Go-based gateway, therefore rode the 401 body into `per-task-agent.jsonl` and the public results bundle intact. Enumerating encoder-specific spellings only lengthens an incomplete list. Instead the precise masking is kept for the common case -- it is why the body stays readable at all -- and backed by a safety net: the masked text is unescaped into a *view* by regex, never `json.loads`, so a body truncated mid-string cannot raise, and a secret still visible in that view costs the whole body rather than the credential. Unescaping repeats to a fixpoint (each pass strictly shortens the text, so it terminates) to reach a secret escaped once upstream and again by the proxy that wrapped it, and the check runs after every pass because a secret containing a backslash can be revealed by one pass and consumed as an escape prefix by the next. Control escapes decode to their characters so a multi-line diagnostic is not falsely withheld. The rejection still reaches the operator: the HTTP status and URL come from the underlying exception, not the body. Reported by Codex on #1416. Signed-off-by: phernandez <paul@basicmachines.co>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Live testing of the #1398 eval stack — five real-model A/B runs and a full BEAM 100K pass — against merged
main. Every commit on this branch was bought by a real failure; the runs that followed each fix validated it. Fixes #1400's follow-through and produces the tracker's first quotable numbers.What changed (12 commits, in failure order)
BEAM live-data drift (all caught by the deliberate fail-fasts):
event_orderingchat-id groups mix ints with int-lists ([116, …, [136, 138]]) — flatten one level, deeper still fails->-> 2,22, 24and->-> 1,5)— final pattern covers the complete surveyed population (2,199 markers, five shapes, survey in the comment); the)verified as generator junk; unknown->->content still fails fastAgent-task harness correctness:
project addregisters only,statusreported ready vacuously, and all 24 tasks of run 3 saw empty projects (masked by the scripted smoke, whose canned answers never consult the index) — explicitreindex --full --searchbefore settleRelationResolvesSQL both now match the task's own prefix while foreign prefixes still fail (cross-project leakage keeps failing, verified against live run DBs)at-at-double project prefix removedModel-endpoint integration (Anthropic compat, verified live):
--model-header Name=value(identity-linked keys needanthropic-workspace-id); values ride only in the model-factory closure, structurally excluded from run artifacts--model-temperaturewithomit(Claude 5 rejects the parameter); recorded in the run config for provenanceCLI: plain-slice footer no longer visually concatenates onto unterminated content (stdout stays byte-exact for pipes).
What the validated runs produced
4dedb8c85318): bm-local vs mem0-local at retrieval parity (R@10 0.657 vs 0.648 answerable) with half the latency (379 ms vs 746 ms); QA macro 0.498 vs 0.513; BM's only meaningful per-ability deficits are temporal reasoning (−7.2pp) and contradiction resolution (−6.6pp) — exactly SPEC-82 and SPEC-89, now recorded pre-implementation as measurable deltas.Product issues driven by these runs
#1414 (index-on-add + readiness contract + refuse-on-never-indexed), #1415 (project-qualified paths — round-trip asymmetry evidence), plus tracker findings on #1398.
🤖 Generated with Claude Code
https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp