ci(eval-combined): bump writ + add combined WRIT+Tier2 qa-gate lane (#1737) - #1741
Merged
Merged
Conversation
…Tier2 qa-gate lane (#1737) Makes the combined layered matrix the CI qa-gate surface (QA-evals QE2). Three parts, now that the WRIT adapter rebuild is merged (writ#1): - Bump the writ submodule 40f55e2 -> 3c0900a (memory_events-driven adapter; aggregate WRIT recall 16% -> 57% against neotoma). - eval-combined imports writ/dist (built) not the stale untracked writ/src/*.js CommonJS artifacts (#1738 follow-up). - New eval_combined CI lane: checkout submodules:recursive, build writ, run npm run eval:combined. Emits the per-category WRIT(state) x Tier-2(agent) coverage matrix; fails the lane on a Tier-2 failure, an errored WRIT run (fail-closed, #1739), or aggregate WRIT recall < 0.5. Keyless (WRIT substring judge); OPENAI_API_KEY/WRIT_JUDGE_API_KEY enables llm_judge fidelity. Verified locally: full run exits 0 — Tier-2 26/43 passed 0 failed, WRIT 77 scenarios recall=57% (clears the 0.5 floor with margin). Note: the 0.5 aggregate floor is a coarse minimum, not a regression ratchet; several categories (abstention/constraint/extraction_drift) are still 0% and are tracked for WRIT capability work. A per-category ratchet is future hardening. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude encountered an error —— View job PR Review (#1741) — in progress
|
Collaborator
|
🤖 Vanellus — Ateles swarm, PR steward
Fix (host-side): set a long-lived Posted by the Apis dispatcher; the operator has been paged to re-auth. |
submodules:recursive fails in CI — other submodules (foundation) use SSH URLs with no key in the runner. writ is public HTTPS, so checkout normally and 'git submodule update --init writ' explicitly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @markmhendrickson's task —— View job PR Review (#1741) — submitted
Verdict: APPROVED-WITH-NOTES (Blocking: 0, Advisory: 3, Nit: 2) Formal review submitted via Top-level notes (all non-blocking):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed commit: c5e5be4
Scope
Reviewing main..feat/writ-submodule-bump-combined-lane — 4 files, +43/−2. Surfaces: CI workflow, npm scripts, eval-combined runner, writ submodule pointer. High-risk: no (no auth, no contract, no data-layer, no schema changes).
Findings
[ADVISORY] silent-behavior / error-handling
File: packages/eval-combined/src/cli.ts:73-78 (interacts with packages/eval-combined/src/runner.ts:231-248, untouched by this diff but newly load-bearing)
Rule: fail-closed parity between layers — the PR description claims the lane "fails on a Tier-2 failure"
Finding: The Tier-2 path in runner.ts catches errors and logs them but does NOT set any error flag analogous to `writError`. If Tier-2 throws during `loadScenariosFromDir` / `runScenarios` (loader bug, missing fixture, isolated-server crash), `tier2Summary` stays `null` and the cli's exit-code ladder
`result.writError ? 1 : result.tier2Summary && result.tier2Summary.failed > 0 ? 1 : ... : 0`
falls through to 0. A Tier-2 *crash* therefore exits 0 (passes the lane). The WRIT layer was hardened for this in #1738/#1739; the Tier-2 layer was not. This PR promotes the runner to the qa-gate surface, so the asymmetry is now load-bearing.
Fix: Mirror the WRIT pattern in runner.ts — capture the caught error into a `tier2Error` field on CombinedResult, and add `result.tier2Error ? 1 :` to the exit-code ladder in cli.ts. Acceptable to land the bump+lane first, but track explicitly as a follow-up — the PR body's "fails on a Tier-2 failure" claim only holds for `failed > 0`, not for `threw`.
[ADVISORY] portability / ci
File: .github/workflows/ci_test_lanes.yml:201-235
Rule: lanes that run sub-builds + a real Neotoma server should bound runtime
Finding: New `eval_combined` job has no `timeout-minutes`. The PR notes Tier-2 runs 43 scenarios against a real isolated server plus 77 WRIT scenarios; a stuck server or slow LLM call can hang the runner for the GitHub default (6h). Other lanes in this file are similarly unbounded, so this is consistent with house style — flagging because this lane is the qa-gate.
Fix: Add `timeout-minutes: 30` (or whatever the observed p99 is) to the job.
[ADVISORY] discoverability / docs
File: docs/developer/package_scripts.md
Rule: skill Phase 5c — new npm scripts should be referenced where operators look them up
Finding: `eval:combined` is added to `package.json` but `docs/developer/package_scripts.md` is not updated. `eval:tier1` and `eval:scenarios` are likewise undocumented there, so this is consistent with current state rather than a regression — but it propagates the gap on the *new* qa-gate surface.
Fix: Add a one-line entry under the `eval:*` group describing the combined lane and the WRIT submodule prerequisite (`git submodule update --init writ && npm --prefix writ ci && npm --prefix writ run build`).
[NIT] ci
File: .github/workflows/ci_test_lanes.yml:223-232
Rule: keep CI install steps explicit
Finding: The job runs `npm ci` for the root workspace and then `npm ci --prefix writ` followed by `npm run build --prefix writ`. The intent is clear from the comments, but the two `npm ci` calls happen back-to-back across boundaries with no cache key for `writ/package-lock.json`.
Fix: (optional) Add a second `actions/setup-node` cache step keyed on `writ/package-lock.json`, or accept the cold install since `writ` is small.
[NIT] hygiene
File: packages/eval-combined/src/runner.ts:190
Rule: prefer letting Node resolve `package.json#main`
Finding: The new import path is correct and the inline comment is excellent (explains exactly why `src/*.js` was wrong). Optionally collapse to `await import(writPath)` and rely on `writ`'s `package.json#main`, which is already `dist/index.js` — that keeps the consumer agnostic to writ's internal layout if it ever moves to `dist/esm/index.js`. The explicit path makes the #1738 regression more obviously test-pinned, so this is purely cosmetic.
Fix: (optional) `await import(writPath)` instead of `join(writPath, "dist", "index.js")`.
Checklist (Phase 4)
| # | Item | Status |
|---|---|---|
| 1–3 | openapi.yaml / contract_mappings / contract tests | — N/A (no OpenAPI/contract change) |
| 4 | new CLI commands in cli_command_coverage_guard | — N/A (no top-level CLI) |
| 5 | MCP↔CLI parity | — N/A |
| 6–7 | runtime overrides / env vars | — N/A |
| 8–9 | error hints / legacy-payload fixture | — N/A (no validation change) |
| 10–11 | bc-diff / legacy-payloads replay | — N/A (not a release PR) |
| 12–13 | additionalProperties / new response fields | — N/A |
| 14 | release supplement | — N/A (no release-visible behavior) |
| 15 | schema-agnostic design re-read | — N/A |
| 16 | determinism preserved | — N/A (no data-layer change) |
| 17 | idempotency_key semantics | — N/A |
| 18 | no new PII in logs/metrics | ✓ (runner logs scenario counts only) |
| 19 | renamed files snake_case | — N/A |
| 20 | security gates / classify-diff | ✓ (no sensitive surface; will run on PR) |
| 21 | new Express routes in protected_routes_manifest | — N/A |
| 22 | no bare req.socket.remoteAddress / XFF / Host | — N/A |
| 23 | user-facing-surface coverage tests | ~ advisory — new CI lane is the test; no separate unit test for the dist-vs-src import branch (the #1738 recurrence is now caught only by the lane itself, which is acceptable for an eval surface) |
| 24 | npm script prefix convention | ✓ — eval: is an established category (eval:tier1, eval:scenarios) |
| 25 | no unstable iteration in stored-output paths | — N/A |
Architectural review (Phase 5)
- State-layer boundaries (10.8): N/A — eval tooling, no production code path.
- Schema-agnostic design: N/A — no per-type branches added.
- Determinism: N/A — eval orchestration, not stored-output.
- Immutability: N/A.
- Auth surface: N/A.
- Error handling: see ADVISORY (Tier-2 fail-open asymmetry).
- Portability: workflow has no hardcoded developer paths; comment correctly explains the
submodules: recursiveavoidance. ✓ - Search/ranking:
entity_handlers.tsuntouched. — N/A.
Product/UX + principles (Phase 5b)
- Explicit-over-implicit (10.2): the WRIT 0.5 aggregate floor is an explicit numeric gate; the PR body explicitly flags it as a minimum, not a regression ratchet, with category-level zeros enumerated. Honest framing. ✓
- Silent behavior: the lane EMITS a per-category matrix to stdout — coverage is surfaced, not hidden. ✓
- Provenance: WRIT report shape preserves
scenario_id/category/scores. ✓ - Discoverability: see ADVISORY (
package_scripts.mdnot updated foreval:combined). - Naming:
eval:combinedconsistent with siblingeval:tier1/eval:scenarios. ✓
Summary
--- Review Summary ---
Base..Head: main..c5e5be4 (feat/writ-submodule-bump-combined-lane)
Files reviewed: 4
Blocking: 0
Advisory: 3
Nit: 2
Verdict: APPROVED-WITH-NOTES
Should address in follow-up:
- Mirror WRIT's fail-closed handling for Tier-2 errors in eval-combined/runner.ts + cli.ts (currently a Tier-2 crash exits 0).
- Add `timeout-minutes` to the new `eval_combined` job.
- Add `eval:combined` (and ideally `eval:tier1` / `eval:scenarios`) to docs/developer/package_scripts.md with the writ submodule prerequisite.
Nothing in the diff blocks merge. The WRIT submodule bump is correctly paired with the import-path fix (now hitting writ/dist, the package "main") and a CI lane that builds writ before running. The fail-closed contract is satisfied for the WRIT layer (the layer this PR explicitly fixes via #1738/#1739), but is incomplete for the Tier-2 layer — track as a follow-up so the qa-gate claim holds end-to-end.
…r (no 429s)
CI eval_combined failed: WRIT replays 5-20 sessions x 77 scenarios = bursty
back-to-back writes that trip the default write rate limit ('429 Write rate
limit exceeded'), which (via the #1739 fail-closed behavior) errored the WRIT
run and failed the lane. The runner now starts the isolated server with
NEOTOMA_WRITE_RATE_LIMIT_PER_MIN=1000000 (benchmark harness, not production
traffic) via the env option the harness already supports.
Verified: 'npm run eval:combined' with no manual env now exits 0 — WRIT 77
scenarios recall=57%, Tier-2 0 failed. (The #1739 fail-closed worked as intended
— it caught the 429 instead of silently passing.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @markmhendrickson's task —— View job PR Review (#1741) — submitted
Verdict: APPROVED-WITH-NOTES (Blocking: 0, Advisory: 3, Nit: 3) Formal review submitted against head Top-level notes (all non-blocking, carried forward from the prior review + one new):
|
There was a problem hiding this comment.
Reviewed commit: 130847b
Scope
Reviewing main..feat/writ-submodule-bump-combined-lane — 4 files, +51/−3. Surfaces: CI workflow, npm scripts, eval-combined runner, writ submodule pointer. High-risk: no (no auth, no contract, no data layer, no schema changes, no production code path).
New since the prior review (c5e5be4 → 130847b9b): one commit fix(eval-combined): raise write rate limit on the WRIT isolated server, +8/−1 in packages/eval-combined/src/runner.ts only. Passes env: { NEOTOMA_WRITE_RATE_LIMIT_PER_MIN: "1000000" } through the existing IsolatedServerOptions.env pass-through (packages/eval-harness/src/isolated_server.ts:177-188); the var is read at src/actions.ts:477-481 with a Math.max(1, …) floor, so the high value is well-formed and bounded. No effect on production traffic — the override only applies to the in-process startIsolatedNeotomaServer spawn inside the WRIT lane.
Findings
[ADVISORY] silent-behavior / error-handling
File: packages/eval-combined/src/runner.ts:252-254 (interacts with packages/eval-combined/src/cli.ts:73-77)
Rule: fail-closed parity between layers — the PR description claims the lane "fails on a Tier-2 failure"
Finding: The Tier-2 path in runner.ts catches errors and logs them but does NOT set any error flag analogous to `writError`. If Tier-2 throws during `loadScenariosFromDir` / `runScenarios` (loader bug, missing fixture, isolated-server crash), `tier2Summary` stays `null` and cli.ts's exit-code ladder
`result.writError ? 1 : result.tier2Summary && result.tier2Summary.failed > 0 ? 1 : ... : 0`
falls through to 0. A Tier-2 *crash* therefore exits 0 (passes the lane). The WRIT layer was hardened for this exact pattern in #1738/#1739; the Tier-2 layer was not. This PR promotes the runner to the qa-gate surface, so the asymmetry is now load-bearing — and the rate-limit fix in 130847b9b demonstrates how easily a benchmark-host fault would have masked itself (the WRIT layer caught it because writError is wired; an equivalent fault on the Tier-2 side would have silently exited 0).
Fix: Mirror the WRIT pattern in runner.ts — capture the caught error into a `tier2Error` field on `CombinedResult`, and add `result.tier2Error ? 1 :` to the exit-code ladder in cli.ts. Acceptable to land this PR first, but track explicitly as a follow-up — the PR body's "fails on a Tier-2 failure" claim only holds for `failed > 0`, not for `threw`.
[ADVISORY] portability / ci
File: .github/workflows/ci_test_lanes.yml:201-235
Rule: lanes that run sub-builds + a real Neotoma server should bound runtime
Finding: New `eval_combined` job has no `timeout-minutes`. The PR notes Tier-2 runs 43 scenarios against a real isolated server plus 77 WRIT scenarios; a stuck server or slow LLM call can hang the runner for the GitHub default (6h). Other lanes in this file are similarly unbounded, so this is consistent with house style — flagging because this lane is the qa-gate.
Fix: Add `timeout-minutes: 30` (or whatever the observed p99 is) to the job.
[ADVISORY] discoverability / docs
File: docs/developer/package_scripts.md
Rule: skill Phase 5c — new npm scripts should be referenced where operators look them up
Finding: `eval:combined` is added to `package.json:88` but `docs/developer/package_scripts.md` is not updated. `eval:tier1` and `eval:scenarios` are likewise undocumented there, so this is consistent with current state rather than a regression — but it propagates the gap on the *new* qa-gate surface, including the writ submodule prerequisite (`git submodule update --init writ && npm --prefix writ ci && npm --prefix writ run build`).
Fix: Add a one-line entry under the `eval:*` group describing the combined lane and the writ submodule prerequisite.
[NIT] ci
File: .github/workflows/ci_test_lanes.yml:223-232
Rule: keep CI install steps explicit
Finding: The job runs `npm ci` for the root workspace and then `npm ci --prefix writ` followed by `npm run build --prefix writ`. The intent is clear from the comments, but the two `npm ci` calls happen back-to-back across boundaries with no cache key for `writ/package-lock.json`.
Fix: (optional) Add a second `actions/setup-node` cache step keyed on `writ/package-lock.json`, or accept the cold install since `writ` is small.
[NIT] hygiene
File: packages/eval-combined/src/runner.ts:190
Rule: prefer letting Node resolve `package.json#main`
Finding: The new import path is correct and the inline comment is excellent (explains exactly why `src/*.js` was wrong). Optionally collapse to `await import(writPath)` and rely on `writ`'s `package.json#main`, which is already `dist/index.js` — that keeps the consumer agnostic to writ's internal layout if it ever moves to `dist/esm/index.js`. The explicit path makes the #1738 regression more obviously test-pinned, so this is purely cosmetic.
Fix: (optional) `await import(writPath)` instead of `join(writPath, "dist", "index.js")`.
[NIT] hygiene
File: packages/eval-combined/src/runner.ts:208-215
Rule: hardcoded "1000000" as an effective-off rate limit is a magic number
Finding: The new `NEOTOMA_WRITE_RATE_LIMIT_PER_MIN: "1000000"` is the documented escape hatch and the inline comment explains the bursty replay pattern correctly. The choice of value is sensible (functionally unbounded, while still being a finite integer the `Math.max(1, …)` guard at `src/actions.ts:478` happily accepts). Marginal: a named constant or a comment citing "effectively unlimited" would let a future reader skip the math.
Fix: (optional) `const WRIT_BENCH_WRITE_LIMIT = "1000000"; // effectively unlimited for the 5–20 sessions × 77 scenarios burst pattern` next to the call site.
Checklist (Phase 4)
| # | Item | Status |
|---|---|---|
| 1–3 | openapi.yaml / contract_mappings / contract tests | — N/A (no OpenAPI/contract change) |
| 4 | new CLI commands in cli_command_coverage_guard | — N/A (no top-level CLI) |
| 5 | MCP↔CLI parity | — N/A |
| 6–7 | runtime overrides / env vars | — N/A (the env var passed in 130847b is consumed by an existing documented env, NEOTOMA_WRITE_RATE_LIMIT_PER_MIN, not a new one) |
| 8–9 | error hints / legacy-payload fixture | — N/A (no validation change) |
| 10–11 | bc-diff / legacy-payloads replay | — N/A (not a release PR) |
| 12–13 | additionalProperties / new response fields | — N/A |
| 14 | release supplement | — N/A (no release-visible behavior) |
| 15 | schema-agnostic design re-read | — N/A |
| 16 | determinism preserved | — N/A (no data-layer change) |
| 17 | idempotency_key semantics | — N/A |
| 18 | no new PII in logs/metrics | ✓ (runner logs scenario counts only) |
| 19 | renamed files snake_case | — N/A |
| 20 | security gates / classify-diff | ✓ (no sensitive surface; CI will gate) |
| 21 | new Express routes in protected_routes_manifest | — N/A |
| 22 | no bare req.socket.remoteAddress / XFF / Host | — N/A |
| 23 | user-facing-surface coverage tests | ~ advisory — new CI lane is the test; no separate unit test for the dist-vs-src import branch or the rate-limit override (the #1738 recurrence is now caught only by the lane itself, which is acceptable for an eval surface) |
| 24 | npm script prefix convention | ✓ — eval: is an established category (eval:tier1, eval:scenarios) |
| 25 | no unstable iteration in stored-output paths | — N/A |
Architectural review (Phase 5)
- State-layer boundaries (10.8): N/A — eval tooling, no production code path.
- Schema-agnostic design: N/A — no per-type branches added.
- Determinism: N/A — eval orchestration, not stored-output. The rate-limit override is config-only and does not affect ID derivation or reducer ordering.
- Immutability: N/A.
- Auth surface: N/A — the WRIT adapter uses the existing isolated-server bearer token plumbing.
- Error handling: see ADVISORY (Tier-2 fail-open asymmetry); the rate-limit fix correctly closes the
429 → writError → exit 1loop on the WRIT side. - Portability: workflow has no hardcoded developer paths; comment correctly explains the
submodules: recursiveavoidance. ✓ - Search/ranking:
entity_handlers.tsuntouched. — N/A.
Product/UX + principles (Phase 5b)
- Explicit-over-implicit (10.2): the WRIT 0.5 aggregate floor is an explicit numeric gate; the PR body explicitly flags it as a minimum, not a regression ratchet, with category-level zeros enumerated. Honest framing. ✓
- Silent behavior: the lane EMITS a per-category matrix to stdout — coverage is surfaced, not hidden. The 130847b rate-limit override is comment-documented at the call site and bounded to the benchmark process. ✓
- Provenance: WRIT report shape preserves
scenario_id/category/scores. ✓ - Discoverability: see ADVISORY (
package_scripts.mdnot updated foreval:combined). - Naming:
eval:combinedconsistent with siblingeval:tier1/eval:scenarios. ✓
Summary
--- Review Summary ---
Base..Head: main..130847b9b (feat/writ-submodule-bump-combined-lane)
Files reviewed: 4
Blocking: 0
Advisory: 3
Nit: 3
Verdict: APPROVED-WITH-NOTES
Should address in follow-up:
- Mirror WRIT's fail-closed handling for Tier-2 errors in eval-combined/runner.ts + cli.ts (currently a Tier-2 crash exits 0).
- Add `timeout-minutes` to the new `eval_combined` job.
- Add `eval:combined` (and ideally `eval:tier1` / `eval:scenarios`) to docs/developer/package_scripts.md with the writ submodule prerequisite.
Nothing in the diff blocks merge. The WRIT submodule bump is correctly paired with the import-path fix (now hitting writ/dist, the package "main"), a CI lane that builds writ before running, and the 130847b rate-limit raise that unblocks the burst-write pattern at the harness boundary without touching production defaults. The fail-closed contract is satisfied for the WRIT layer (the layer this PR explicitly fixes via #1738/#1739/130847b9b), but is incomplete for the Tier-2 layer — track as a follow-up so the qa-gate claim holds end-to-end.
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
2 participants
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.
Makes the combined layered matrix the CI qa-gate surface (QA-evals QE2), now that the WRIT adapter rebuild is merged (writ#1). Part of umbrella #230.
What
40f55e2 → 3c0900a— the memory_events-driven adapter (aggregate WRIT recall 16% → 57% against neotoma; drift 0→60%, update 0→100%, forgetting 20→80%, temporal 20→100%, provenance 0→80%).writ/dist(built) not the stale untrackedwrit/src/*.jsCommonJS artifacts (eval-combined is broken against current WRIT (scenarios.filter is not a function) AND fails open — blocks wiring the combined WRIT+Tier2 matrix as the qa-gate #1738 follow-up).eval_combinedCI lane:checkout submodules:recursive→ build writ →npm run eval:combined. Emits the per-category WRIT(state) × Tier-2(agent) coverage matrix; fails on a Tier-2 failure, an errored WRIT run (fail-closed, fix(eval-combined): await WRIT loader + fail closed on WRIT error (#1738) #1739), or aggregate WRIT recall < 0.5. Keyless (WRIT substring judge);OPENAI_API_KEY/WRIT_JUDGE_API_KEYenables llm_judge fidelity.Verified locally
Full
eval:combinedexits 0 — Tier-2 26/43 passed, 0 failed; WRIT 77 scenarios, recall=57% (clears the 0.5 floor with margin). The layered matrix renders per-category.Honest note
The 0.5 aggregate floor is a coarse minimum, not a regression ratchet — several categories (abstention/constraint/extraction_drift) are still 0% and tracked for WRIT capability work. A per-category ratchet is future hardening.
Advisory review GHA + Vanellus error on Anthropic-API auth (known); merge on
security_gates+ substantive lanes green.