WT-1-run-schema: D1 per-run schema, writer, and CSV/JSONL export#56
Merged
Conversation
WT-1-run-schema (Phase 1 #4). Lands the per-run D1 evaluation row schema as a Go package + the runs table DDL + a CLI exporter. What - internal/evalrun: 24-field Schema struct matching /root/paper_writing/docs/intermediate/08_evaluation_plan_v3.md lines 256-279 verbatim. SQLWriter binds every value via parameterised ?-placeholders (no fmt.Sprintf into SQL anywhere). Schema-drift guard at writer construction compares ordered (cid, name, type, notnull, dflt_value, pk) descriptors against an in-Go expected list; refuses to construct on any mismatch (ErrSchemaDrift), surfacing missing/extra column names in the message. Validation pipeline returns typed sentinels with field-name / index context wrapping (errors.Is preserved). - internal/evalrun: NoObserver ablation flag registered via ablation.Default.Register(ablation.NoObserver, &DisableTelemetry) in init(). When DisableTelemetry=true, Insert still validates the row and then logs `[ablation] NoObserver: dropped run_id=<id>` before returning nil — this audit pointer is the non-negotiable counterweight for the silent-drop failure mode. - cmd/evalrun-export: --format=csv|jsonl, --out, --db (with OBSERVER_DB env fallback; --db wins), --filter-experiment. CSV cells whose first byte is one of `= + - @ \t \r \n` are apostrophe-prefixed to neutralise Excel/Calc formula injection (CWE-1236). Empty cells are NOT prefixed. SQLite is opened read-only (mode=ro) as defence in depth. --out close failures bubble up as runtime errors instead of being swallowed. - internal/observerstore/schema.sql: append runs DDL (24 columns + CHECK on success_oracle_result + index on experiment_id). - docs/specs/wt1-run-schema.{spec,plan}.md: full spec including security mitigations (a)-(f) and the 26-test matrix. Security - (a) Parameterised SQL only. Tested via injection payloads in workload_id, machine_topology, failure_category, AND --filter-experiment. - (b) ArtifactHashes entries gated by ^[a-f0-9]{64}$. Rejected: non-hex, length 63, empty string, uppercase hex, path-disguised-as-hash. - (c) NoObserver runs validation AND logs every dropped run_id. - (d) Drift guard catches missing column, extra column, wrong type, wrong NOT NULL, wrong DEFAULT, wrong PK, renamed column, swapped column order, missing table. - (e) CSV formula escape covers =, +, -, @, \t, \r, \n. Underlying DB row is unchanged (escape is export-only). - (f) RunID gated by ^[A-Za-z0-9_-]{8,128}$. Other string fields capped at 8 KiB. Tests - go test ./internal/evalrun/... ./cmd/evalrun-export/... \ -count=1 -shuffle=on -race passes (16 + 18 tests). - go vet ./... clean. - gofmt -l internal/evalrun cmd/evalrun-export clean. - Smoke: OBSERVER_DB=/tmp/empty.db evalrun-export --format=csv byte-identical to --db /tmp/empty.db form, prints 24-column header, exit 0. Codex review across spec / plan / code: 3-round CLEAN convergence. Base: origin/paper/v3-integration @ 17f2c3c (WT-1-ablation-registry PR #51 already merged; NoObserver registered against the typed FlagName registry). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial fresh-session review surfaced gaps the 3-round Codex loop
missed. Findings + fixes:
P1 — ArtifactHashes slice length unbounded
- The per-string 8 KiB cap protected every column except the JSON-array
serialisation of ArtifactHashes, where a caller could pass len=10M
valid 64-byte hashes and produce a ~680 MB row.
- Added const maxArtifactHashes = 256 (~17 KiB encoded ceiling) plus
ErrTooManyArtifactHashes sentinel. New test
TestInsert_RejectsTooManyArtifactHashes covers cap + boundary.
P1 — schema-drift guard blind to CHECK constraints
- PRAGMA table_info returns (cid,name,type,notnull,dflt,pk) only — no
CHECK / COLLATE / UNIQUE info. A DDL that dropped the
CHECK(success_oracle_result IN ('pass','fail','timeout')) passed
silently, eroding the last-line-of-defence story for any future
caller that bypassed Go-side validation.
- Added expectedCheckSubstrings sourced from sqlite_master.sql (which
preserves the verbatim CREATE TABLE text in modernc.org/sqlite v1.50.0).
Drift guard now string-matches the CHECK fragment after the
descriptor walk and reports a CHECK-mention error on miss. New test
TestNewSQLWriter_DetectsSchemaDriftMissingCheck applies a CHECK-less
CREATE TABLE and asserts ErrSchemaDrift + "CHECK" in the message.
P1 — NoObserver init-failure silently inert with weak signal
- If another package races us to ablation.Default.Register(NoObserver),
this package's DisableTelemetry stays inert; the only signal was a
single log.Printf at startup that operators routinely miss. The CLI
binder calling SetByName(NoObserver, true) would flip somebody else's
*bool while operator sees --ablation NoObserver accepted.
- Added InitError() public accessor and sticky initRegistrationErr.
First Insert call re-emits a louder WARNING line via sync.Once so a
stderr-suppressed start still surfaces the divergence. New test
TestRegistrationCollision_DetectableByOtherPackage proves the
registry rejects a second Register, which is the surface initial
collisions actually present at.
P2 fixes:
- failure_category ↔ result invariant now enforced (must be empty iff
result==pass). New sentinel ErrFailureCategoryOnPass, new test
TestInsert_RejectsFailureCategoryPassMismatch (4 sub-cases), and the
pre-existing TestInsert_RejectsBadOracleResult updated to supply a
matching category.
- Removed dead runRow.humanCountIdx field; replaced the two literal
20s in main.go with const humanCountIdx = 20.
- TestMain now captures m.Run() and explicitly cleans the temp build
dir before os.Exit (defer was unreachable past os.Exit).
- TestExportCSV_OutFile mode assertion is now umask-independent
(checks owner-rw + no-exec instead of literal 0644).
- Writer interface methods now carry contract docstrings: Insert
documents the both-modes-validate guarantee, Close documents that
the underlying *sql.DB is caller-owned (so `defer w.Close()` does
NOT release the DB).
- Spec §4.4 dropped the unimplemented _journal=OFF DSN fragment;
read-only SQLite open already disables journal writes.
Tests: 39 functions / 77 sub-cases total, all pass under
-count=1 -shuffle=on -race; go vet + gofmt clean; smoke
OBSERVER_DB=... evalrun-export --format=csv still emits the 24-column
header.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Fresh-Claude review applied (commit
|
| Finding | Fix |
|---|---|
ArtifactHashes slice length unbounded (DoS via 10M-entry slice → ~680 MB encoded row) |
maxArtifactHashes = 256 cap + ErrTooManyArtifactHashes; new test asserts cap + boundary |
Drift guard blind to CHECK constraints (PRAGMA table_info doesn't surface them; a DDL that drops the CHECK(success_oracle_result IN ...) would pass silently) |
Added expectedCheckSubstrings cross-check against verbatim sqlite_master.sql text; new test asserts ErrSchemaDrift + "CHECK" in error message when CHECK is missing |
NoObserver init-failure silently inert (a colliding Register in another package's init leaves DisableTelemetry wired to nobody; the only signal was a single startup log.Printf operators routinely miss) |
Added InitError() public accessor + sticky initRegistrationErr; first Insert re-emits a louder WARNING via sync.Once; new test proves the collision is observable to a second caller |
P2 fixes
failure_category↔resultinvariant now enforced (must be empty iffpass); new sentinelErrFailureCategoryOnPass, 4 sub-test cases.- Removed dead
runRow.humanCountIdxfield; the two literal20s replaced byconst humanCountIdx = 20. TestMaincleanup is now reachable pastos.Exit(capturedrc := m.Run()).TestExportCSV_OutFilemode assertion now umask-independent.Writerinterface docstrings now document the both-modes-validate guarantee (Insert) and the caller-owns-DB semantics (Close).- Spec §4.4 dropped the unimplemented
_journal=OFFDSN fragment.
Deliberately not fixed
- P2 "leading whitespace +
=slips formula escape in LibreOffice Calc." Real but bounded — the row producers are all trusted (eval-runner / driver / observer), the 8 KiB cap limits payload, and the OWASP recommendation cited would change the §7(e) spec from "explicit allowlist" to "unconditionally prefix every non-empty cell with'", which is a spec-level design call rather than a code bug. Filed mentally as a follow-up for the spec authors; not blocking this PR.
Verification
go test ./internal/evalrun/... ./cmd/evalrun-export/... -count=1 -shuffle=on -race— 77 sub-cases across 39 test functions, all passgo vet ./...cleangofmt -l internal/evalrun cmd/evalrun-exportclean- Smoke
OBSERVER_DB=/tmp/empty.db evalrun-export --format=csvstill emits the 24-column header, exit 0
+273 / -26 over the original commit; net diff to base is +3183.
🤖 Generated with Claude Code
A second adversarial fresh-session review interrogated the round-1 fix
and found that several "fixes" were either over-stated (NoObserver
collision test) or introduced new bugs (failure_category invariant
broke the timeout case; the round-1 fix added the invariant before
checking what the canonical taxonomy actually requires). This commit
addresses every finding.
P1 — failure_category accepts arbitrary strings; spec calls for 11-class enum
- internal/observerstore/failure_category.go ships AllCategories() +
IsKnown() + FailUnknown sentinel. The round-1 fix added a "must be
empty iff pass" invariant but never consulted the canonical set —
the author's OWN tests used "FailNetwork", a value that is NOT in
the 11-class taxonomy.
- Added isAcceptedFailureCategory() that delegates to
observerstore.IsKnown(), plus the "unknown" sentinel escape hatch.
- Renamed ErrFailureCategoryOnPass → ErrInvalidFailureCategory (the
sentinel now covers BOTH the compatibility invariant AND the
taxonomy membership). The wrapped error text always names which of
the two conditions failed.
- New test TestInsert_RejectsBadFailureCategory (9 sub-cases) covers
all combinations: bad-on-pass, missing-on-fail, missing-on-timeout,
off-taxonomy value, wrong case, plus accepted cases including
"unknown" for both fail and timeout.
P1 — result=="timeout" + empty failure_category was over-rejected
- The round-1 invariant required failure_category non-empty for
ANY non-pass result. The taxonomy ships FailUnknown precisely for
unclassifiable failure sites — the operator should be able to set
"unknown" instead of being blocked at validation. The fix above
accepts "unknown" universally; the new sub-cases prove it.
P1 — maxArtifactHashes = 256 violated spec §7(f) 8 KiB column cap
- Round-1 picked 256 (encoded ~17 KiB) which is more than 2x the 8
KiB per-string cap the spec itself stipulates. Reduced to 120
(encoded fits in 8 KiB with slack). Code comment now derives the
number from the cap arithmetic, so a future cap change keeps the
invariant.
P1 — ErrSchemaDrift error text drifted from spec without spec update
- Code said "expected layout"; spec line 179 still said "expected
24-column descriptor". Updated spec to match code.
P1 — CHECK-clause drift guard fragile to whitespace variants
- Round-1 used strings.Fields which only collapses whitespace runs to
single spaces. Semantically-identical reformatting like
"CHECK ( success_oracle_result IN ('pass', 'fail', 'timeout') )"
would false-positive ErrSchemaDrift because the needle had no
inner whitespace. Replaced with stripASCIIWhitespace() that drops
ALL whitespace from both sides, making the match invariant to any
reformatting that doesn't change tokens.
- New test TestNewSQLWriter_AllowsCheckClauseWhitespaceVariants (4
sub-cases) covers spaces-inside-parens, spaces-after-commas,
newlines-inside, and both-mixed.
P2 fixes:
- Time format pinned to 9-digit nanoseconds. time.RFC3339Nano strips
trailing zeros producing strings that sort inconsistently
(".5Z" < "Z" lexicographically but later chronologically), breaking
ORDER BY start_time downstream. New test
TestInsert_TimeFormatPreservesLexOrder proves lex == chrono for
exact-second / half-second / next-second values.
- Invalid UTF-8 now rejected by validate() (new ErrInvalidUTF8
sentinel + per-field check). Without this, encoding/csv preserves
raw bytes while encoding/json silently substitutes U+FFFD,
asymmetrising the two export paths. New test
TestInsert_RejectsInvalidUTF8 (3 sub-cases) confirms.
- evalrun-export now runs evalrun.CheckSchema() at startup.
Exposed CheckSchema as the read-only-friendly variant of
NewSQLWriter's guard. Without this a drifted DB would crash export
mid-stream with a cryptic "converting NULL to int" driver error.
New test TestExportCSV_DriftedSchema_Exit1AtStart confirms exit 1
+ "schema check" in stderr.
- Unexported InitError(): it had no production caller, only a
self-test, and the first-Insert WARNING is the primary mitigation.
Adding exported API for hypothetical Phase 2 consumers is YAGNI.
If a later worktree needs programmatic detection, lift it then.
- Renamed TestRegistrationCollision_DetectableByOtherPackage →
TestRegistrationCollision_RecoveryPathLogsOnFirstInsert, and made
it actually test the recovery path: pokes initRegistrationErr to
a non-nil value, resets initWarnOnce (now *sync.Once so a fresh
swap doesn't trip vet's noCopy), and asserts the loud WARNING
fires on first Insert and DOES NOT repeat on the second.
P3 — TestMain cleanup chain de-duplicated
- Dropped the redundant `defer os.RemoveAll(tmp)` and kept the two
explicit calls (build-failure + happy path). The comment now
states why no defer fallback: it would obscure the os.Exit
interaction the explicit calls handle.
P2 not addressed: CSV escape for leading-whitespace + `=` (LibreOffice
formula trigger). Real but bounded — would require flipping spec §7(e)
from explicit allowlist to unconditional prefix, which is a spec
design call not a code bug. Documented in the PR thread.
Tests: 39 functions / 93 sub-cases (up from 77), all pass under
-count=1 -shuffle=on -race; go vet + gofmt clean; smoke
OBSERVER_DB=... evalrun-export --format=csv still emits the
24-column header.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round 2 fresh-Claude review applied (commit
|
| Finding | Fix |
|---|---|
failure_category accepted arbitrary strings (the author's own tests used "FailNetwork" — not in the 11-class enum) |
isAcceptedFailureCategory() now consults observerstore.IsKnown() + the "unknown" (FailUnknown) sentinel; new sentinel ErrInvalidFailureCategory covers both invariants; 9 sub-test cases |
timeout + empty failure_category was over-rejected (no escape hatch) |
Accept "unknown" for any non-pass result; sub-cases prove timeout+unknown and fail+unknown both pass |
maxArtifactHashes = 256 permitted 17 KiB rows, exceeding spec §7(f)'s 8 KiB cap |
Reduced to 120 (encoded fits in 8 KiB with slack); code comment derives the number from the cap arithmetic |
ErrSchemaDrift text drifted from spec line 179 |
Spec updated to match: "expected layout" |
CHECK-clause drift guard used strings.Fields which false-positives on semantically-identical reformatting |
Replaced with stripASCIIWhitespace() — drops ALL whitespace from both sides; new test TestNewSQLWriter_AllowsCheckClauseWhitespaceVariants (4 sub-cases) |
P2 fixes
| Finding | Fix |
|---|---|
time.RFC3339Nano strips trailing zero nanoseconds → strings sort inconsistently across precisions |
Pinned to fixed-9-digit format "2006-01-02T15:04:05.000000000Z07:00"; new test TestInsert_TimeFormatPreservesLexOrder proves lex == chrono for exact-second / half-second / next-second |
CSV / JSONL UTF-8 asymmetry (json.Marshal silently substitutes �) |
New ErrInvalidUTF8 sentinel; validate() rejects invalid UTF-8 on all string fields; new test TestInsert_RejectsInvalidUTF8 (3 sub-cases) |
Export CLI lacked drift guard → NULL human_intervention_count crashed mid-stream |
New evalrun.CheckSchema() public function called at CLI startup; new test TestExportCSV_DriftedSchema_Exit1AtStart proves exit 1 + "schema check" in stderr |
InitError() was exported API with only a self-test caller (YAGNI) |
Unexported; first-Insert WARNING is the primary signal; TestRegistrationCollision_RecoveryPathLogsOnFirstInsert now actually exercises the recovery path (pokes initRegistrationErr and asserts the WARNING fires once via sync.Once, not twice) |
P3
TestMaincleanup chain de-duplicated: dropped the redundantdefer, kept two explicit calls.
Deliberately deferred
- P2 "leading whitespace +
=slips formula escape in LibreOffice." Real but spec-level: would change §7(e) from explicit allowlist to unconditional prefix. The 8 KiB cap + trusted row producers bound the practical risk. Flagged for the spec authors; not blocking.
Verification
go test ./internal/evalrun/... ./cmd/evalrun-export/... -count=1 -shuffle=on -race— 93 sub-cases across 39 test functions, all pass (up from 77)go vet ./...cleangofmt -l internal/evalrun cmd/evalrun-exportclean- Smoke
OBSERVER_DB=/tmp/empty.db evalrun-export --format=csvstill emits the 24-column header, exit 0
+431 / -84 over the round-1 commit; PR total +3530 over base.
🤖 Generated with Claude Code
Round-3 adversarial review found mostly cleanup items (0 P0, 0 P1) plus two real spec/code consistency hazards. Trajectory clearly converging — most remaining items are YAGNI / naming hygiene rather than substantive defects. P2 fixes: - failureCategoryUnknown pinned to observerstore.FailUnknown: Round-2's hard-coded `const failureCategoryUnknown = "unknown"` was decoupled from the canonical observerstore.FailUnknown by string value only — a future rename of FailUnknown would silently drift evalrun's accepted set. Changed to `var failureCategoryUnknown = string(observerstore.FailUnknown)` + new test TestFailureCategoryUnknown_PinnedToObserverstore that trips if anyone accidentally re-hardcodes it. - Spec §2.2 sentinel block re-synced to code: Rounds 1 + 2 added three new sentinels (ErrTooManyArtifactHashes, ErrInvalidUTF8, ErrInvalidFailureCategory) but never updated the spec block. A Phase 2 consumer writing `switch errors.Is(...)` chains from the spec would have missed those branches. Spec now lists all 9 sentinels + documents the two-part failure_category invariant. - stripASCIIWhitespace renamed to stripAllWhitespace: the implementation uses unicode.IsSpace (catches NBSP, ideographic space, etc.) but the name said "ASCII only". Name now matches behaviour; comment explains the Unicode coverage. P3 cleanup: - CheckSchema wrapper deleted; export checkSchemaDrift directly as CheckSchemaDrift. One name for one concept (was three: CheckSchema / checkSchemaDrift / ErrSchemaDrift). CLI updated. - expectedCheckSubstrings (1-entry slice) demoted to a single const expectedCheckSubstring. YAGNI; future second CHECK clause is a 2-line lift. - humanCount: int → int64 in the CLI scanner. SQLite INTEGER is up to 64-bit signed; on 32-bit Go builds the previous int would silently truncate a row with human_intervention_count > 2^31 to a negative number. Bounded practical relevance, but a free fix. - Writer interface doc cleaned: removed the DisableTelemetry reference from the interface contract (it's package-level state, not Writer state; alternative implementations aren't required to honour it). Kept the detailed contract on (*SQLWriter).Insert. - Cross-package column-name SOT pinning: exposed evalrun.ColumnNames() returning a copy derived from the expectedColumns descriptor list (same SOT the drift guard uses). New test TestColumnNames_MatchProductionSOT asserts the CLI's columnNames equals evalrun.ColumnNames(); a future schema column add that updates only one half now trips a test instead of silently scrambling CSV headers. P2 not addressed (round-3 reviewer agreed defer): CSV escape for leading-whitespace + `=` (LibreOffice formula trigger; spec design call, not a code bug). Tests: 41 functions / 95 sub-cases (up from 93), all pass under -count=1 -shuffle=on -race; go vet + gofmt clean; smoke OBSERVER_DB=... evalrun-export --format=csv still emits the 24-column header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round 3 fresh-Claude review applied (commit
|
| Finding | Fix |
|---|---|
NEW-1: failureCategoryUnknown decoupled from observerstore.FailUnknown by string value only |
Pinned via var failureCategoryUnknown = string(observerstore.FailUnknown); new test TestFailureCategoryUnknown_PinnedToObserverstore |
| NEW-2: Spec §2.2 sentinel block missed 3 sentinels added in rounds 1+2 | Block re-synced (now lists all 9); doc'd the two-part failure_category invariant |
NEW-3: stripASCIIWhitespace mis-named (uses unicode.IsSpace) |
Renamed to stripAllWhitespace; comment matches behaviour |
P3 cleanup
| Finding | Fix |
|---|---|
| NEW-4: Two SOTs for column names, never cross-checked | Exposed evalrun.ColumnNames() from descriptor SOT; new test TestColumnNames_MatchProductionSOT pins the CLI's columnNames to it |
NEW-5: humanCount int could under-range on 32-bit builds |
Changed to int64 + strconv.FormatInt |
NEW-6: Writer interface doc leaked DisableTelemetry (package state) |
Removed from interface doc; kept on (*SQLWriter).Insert |
NEW-7: expectedCheckSubstrings was a 1-entry slice (YAGNI) |
Demoted to single const expectedCheckSubstring |
NEW-8: CheckSchema was a one-line wrapper around checkSchemaDrift |
Deleted wrapper; renamed checkSchemaDrift → exported CheckSchemaDrift; one name for one concept |
Re-confirmed (round-3 reviewer interrogated, found round-1/2 fixes solid)
maxArtifactHashes=120arithmetic: verified (67*N+1 ≤ 8192 → N ≤ 122)- Fixed-9-digit time format on non-UTC input: verified safe (
t.UTC().Formatalways emitsZ) TestInsert_TimeFormatPreservesLexOrderactually catches the regression: confirmed via probe (RFC3339Nano sorts B,A,C; fixed format sorts A,B,C)TestRegistrationCollision_RecoveryPathLogsOnFirstInsertorder-stability under-shuffle=on: confirmed via-count=5 -shuffle=on -raceALTER TABLE ADD COLUMNupdatessqlite_master.sqlin modernc.org/sqlite — column-count branch fires correctly
Deferred (round-3 reviewer agreed)
- P2 CSV escape for leading-whitespace +
=(LibreOffice formula trigger). Spec design call, not a code bug.
Verification
go test ./internal/evalrun/... ./cmd/evalrun-export/... -count=1 -shuffle=on -race— 95 sub-cases across 41 test functions, all pass (up from 93)go vet ./...cleangofmt -l internal/evalrun cmd/evalrun-exportclean- Smoke
OBSERVER_DB=/tmp/empty.db evalrun-export --format=csvstill emits the 24-column header, exit 0
+115 / -50 over round-2; PR total +3595 over base.
Convergence summary across all 3 review rounds
| Round | P0 | P1 | P2 | P3 | Verdict |
|---|---|---|---|---|---|
| 1 | 0 | 3 | 6 | 0 | CHANGES REQUIRED |
| 2 | 0 | 5 | 4 | 1 | CHANGES REQUIRED |
| 3 | 0 | 0 | 3 | 5 | CHANGES REQUIRED (minor, reviewer would unblock on 2) |
Clear trajectory: severity profile dropping each round, no P0 ever, P1 → 0 by round 3. Suggesting the round-4 fresh review would either find pure P3 nits or VERDICT: CLEAN. Happy to run another round if the human wants further validation.
🤖 Generated with Claude Code
Round-4 adversarial review returned 0 P0, 0 P1, 0 P2, 4 P3 — convergence trajectory clear (rounds 1→4: 9 → 10 → 8 → 4 findings, each round lower severity). Reviewer explicitly: "code is production-ready as-is" and would only block merge on F1. All 4 P3 hygiene items addressed: F1 — Stale comment in CSV/JSONL emit path - main.go:349 still referenced strconv.Itoa even though round-3 switched to strconv.FormatInt(_, 10) on int64. Reviewer flagged as security-relevant doc rot (a future audit grepping strconv.Itoa would lose the trust trail justifying the raw byte append into the JSON stream). - Updated comment to name the actual producer and the JSON-grammar conformance guarantee. F2 — expectedColumnNames was a third independent SOT - Round-3 added evalrun.ColumnNames() and pinned the CLI's columnNames to it, but the test's expectedColumnNames was still a hand-maintained copy. A future column add that updated only expectedColumns would cause TestSchema_AllFieldsRoundtrip to fail with confusing per-column errors rather than a clean SOT- drift signal. - Replaced the 26-line literal slice with `var expectedColumnNames = ColumnNames()`. Now SOTs are exactly TWO: expectedColumns (production descriptor list) and the CLI's columnNames (cross-checked by TestColumnNames_MatchProductionSOT). F3 — failureCategoryUnknown was var, could be const - Reviewer correctly noted: observerstore.FailUnknown is a typed- string const, so `string(FailUnknown)` IS a constant expression; Go accepts `const` here. The round-2 author chose `var` reflexively, leaving the value mutable from any test in the package — defeating the "pinned to FailUnknown" intent. - Changed to const. Immutability is now compile-enforced; a future test assigning to it would fail to compile. F4 — Latent t.Parallel() footgun - The package tests mutate several package-level globals (DisableTelemetry, initRegistrationErr, initWarnOnce, log.Writer) and rely on the default sequential test scheduling for safety. A future contributor adding t.Parallel() would introduce races invisible to the prior reviewer. - Added a file-header comment stating the no-parallel contract and pointing at the alternatives (per-Writer state / accessor struct). Verification: 41 functions / 95 sub-cases pass under -count=10 -shuffle=on -race (10× the usual count to flush any shuffle-order flakes — none surfaced); go vet + gofmt clean; smoke OBSERVER_DB=... evalrun-export --format=csv still emits the 24-column header. Convergence table (cumulative across all 4 rounds): Round 1: 0 P0, 3 P1, 6 P2, 0 P3 Round 2: 0 P0, 5 P1, 4 P2, 1 P3 Round 3: 0 P0, 0 P1, 3 P2, 5 P3 Round 4: 0 P0, 0 P1, 0 P2, 4 P3 Round-4 reviewer's round-5 prediction: VERDICT: CLEAN or 1-2 pure-style nits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round 4 fresh-Claude review applied (commit
|
| Finding | Fix |
|---|---|
F1 — Stale comment referenced strconv.Itoa after round-3 switched to strconv.FormatInt; security-relevant doc rot (future audit grepping for the producer loses the trust trail) |
Comment now names the actual producer + JSON-grammar conformance guarantee |
F2 — expectedColumnNames in schema_test.go was a third hand-maintained SOT; round-3's ColumnNames() pin only covered the CLI side |
Replaced 26-line literal with var expectedColumnNames = ColumnNames(); SOTs now exactly two (descriptor + CLI), both cross-checked |
F3 — failureCategoryUnknown was var (mutable from any test) but COULD be const because observerstore.FailUnknown is itself a typed-string const |
Changed to const — immutability now compile-enforced |
F4 — Latent t.Parallel() footgun: package tests mutate globals; safety relies on default sequential scheduling |
Added file-header contract comment stating no-parallel + pointing at remediation paths |
Re-verified clean from round 3 (no regressions)
var failureCategoryUnknown = string(...)pinning — still works as advertised (now promoted toconst)- Spec §2.2 sentinel block — 9 sentinels, exact 1:1 name/order/text match
stripAllWhitespaceUnicode coverage — claims accurate (NBSP/ideographic/separators yes; ZWSP/BOM correctly NOT)CheckSchemaDriftrename — no stale callershumanCount int64— Scan + FormatInt path safeWriterinterface doc — no implementation-state leakage- CLI
columnNames↔ColumnNames()pin viaTestColumnNames_MatchProductionSOT— works as designed
Convergence trajectory
| Round | P0 | P1 | P2 | P3 | Total |
|---|---|---|---|---|---|
| 1 | 0 | 3 | 6 | 0 | 9 |
| 2 | 0 | 5 | 4 | 1 | 10 |
| 3 | 0 | 0 | 3 | 5 | 8 |
| 4 | 0 | 0 | 0 | 4 | 4 |
Clear monotone convergence: P1 → 0 by round 3, P2 → 0 by round 4, all remaining findings P3 hygiene. No P0 across any round. Round-4 reviewer's round-5 prediction: VERDICT: CLEAN or 1–2 pure-style nits.
Verification
go test ./internal/evalrun/... ./cmd/evalrun-export/... -count=10 -shuffle=on -race— 41 functions / 95 sub-cases, all pass 10× under shuffle (no shuffle-order flakes)go vet ./...cleangofmt -l internal/evalrun cmd/evalrun-exportclean- Smoke
OBSERVER_DB=/tmp/empty.db evalrun-export --format=csvstill emits the 24-column header, exit 0
+30 / -33 over round-3 (the F2 SOT consolidation deleted more than it added); PR total +3592 over base.
🤖 Generated with Claude Code
Round-5 adversarial review verdict: CLEAN. Prior tests only exercised two accepted failure_category values (slave_disconnect + "unknown"), leaving the other 10 canonical observerstore.AllCategories() entries — including the reserved-for-future tags (wrong_version, forbidden_cred, driver_restart) — with no round-trip coverage. A future refactor of isAcceptedFailureCategory that replaced observerstore.IsKnown with a hand-written switch missing a case would silently reject those values and only surface at first production use. Added a t.Run loop over observerstore.AllCategories() that inserts a row with each canonical taxonomy value and success_oracle_result="fail", proving all 11 pass validation. This closes the highest-leverage of the 3 P3 hygiene items round-5 noted. The other two (insertSampleRow using RFC3339Nano instead of fixed-9-digit; run_id lacks DB-side NOT NULL — a pre-existing SQLite quirk not touched by this PR) remain as follow-ups noted in the PR thread. Tests: 41 functions / 107 sub-cases (up from 95) pass under -count=1 -shuffle=on -race; go vet + gofmt clean. Convergence table (5 rounds complete): Round 1: 0 P0, 3 P1, 6 P2, 0 P3 (CHANGES) Round 2: 0 P0, 5 P1, 4 P2, 1 P3 (CHANGES) Round 3: 0 P0, 0 P1, 3 P2, 5 P3 (CHANGES minor) Round 4: 0 P0, 0 P1, 0 P2, 4 P3 (production-ready) Round 5: 0 P0, 0 P1, 0 P2, 3 P3 (VERDICT: CLEAN) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Round 5 fresh-Claude review — VERDICT: CLEAN (commit
|
| Round | P0 | P1 | P2 | P3 | Total | Verdict |
|---|---|---|---|---|---|---|
| 1 | 0 | 3 | 6 | 0 | 9 | CHANGES REQUIRED |
| 2 | 0 | 5 | 4 | 1 | 10 | CHANGES REQUIRED |
| 3 | 0 | 0 | 3 | 5 | 8 | CHANGES REQUIRED (minor) |
| 4 | 0 | 0 | 0 | 4 | 4 | production-ready |
| 5 | 0 | 0 | 0 | 3 | 3 | CLEAN |
Monotone P1-severity descent (3 → 5 → 0), monotone P2-severity descent (6 → 4 → 3 → 0 → 0), findings total settling around 3-4 P3 hygiene items across the last two rounds. This is the stability signature the trajectory promised.
Verification
go test ./internal/evalrun/... ./cmd/evalrun-export/... -count=1 -shuffle=on -race— 41 functions / 107 sub-cases (up from 95), all passgo vet ./...cleangofmt -l internal/evalrun cmd/evalrun-exportclean- Smoke
OBSERVER_DB=/tmp/empty.db evalrun-export --format=csv— 24-column header, exit 0
PR HEAD aa597be, 6 commits, +3614 / -0 against base. Ready for merge.
🤖 Generated with Claude Code
This was referenced Jul 1, 2026
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.
Phase 1 #4 of
docs/final/todo_list.md. Lands the per-run D1 evaluation row schema as a Go package, therunstable DDL, and a CLI exporter.Base & dependencies
origin/paper/v3-integration @ 17f2c3c(WT-1-ablation-registry PR WT-1-ablation-registry: typed FlagName registry for paper-v3 ablation flags #51 already merged;NoObserveris one of the eight canonicalFlagNameconstants ininternal/ablation)./root/paper_writing/docs/intermediate/08_evaluation_plan_v3.mdlines 256–279 (24 fields — the todo_list row's "全部 24 字段" is correct; the kickoff prompt's "27" is a typo and was reconciled to 24 with explicit user sign-off).What this PR adds
multi-agent/internal/evalrun/—Schemastruct (24 fields in 08 order), typed sentinel errors with%w-wrapped context,SQLWriterwith ordered descriptor schema-drift guard, validation pipeline,DisableTelemetrybool registered againstablation.Defaultviainit()(non-panic — logs and proceeds if registration fails).multi-agent/cmd/evalrun-export/— CLI with--format=csv|jsonl,--out,--db(envOBSERVER_DBfallback;--dbwins),--filter-experiment(parameterised). Opens SQLite read-only as defence in depth. Reports--outclose failures.multi-agent/internal/observerstore/schema.sql— appendedrunsDDL (24 columns +CHECK(success_oracle_result IN ('pass','fail','timeout'))+idx_runs_experiment) at end of file so the parallelWT-1-capability-snapshotworktree's append doesn't collide on line numbers.docs/specs/wt1-run-schema.{spec,plan}.md— full spec + 26-test plan + security cross-check.Security mitigations (spec §7 (a)–(f))
fmt.Sprintfinto SQLTestInsert_Parameterized_SQLInjection,TestExportCSV_FilterExperiment_SQLInjectionArtifactHashesentries gated by^[a-f0-9]{64}$TestInsert_RejectsBadArtifactHash(non-hex, length 63, empty, uppercase, path-disguised)NoObserverablation logs every droppedrun_idAND still runs validationTestNoObserver_DroppedRunLogged,TestNoObserver_StillRejectsInvalid(cid, name, type, notnull, dflt_value, pk)=,+,-,@,\t,\r,\n(CWE-1236); DB row unchangedTestExportCSV_FormulaInjectionEscaped,TestExportCSV_EmptyStringNotPrefixedRunID ^[A-Za-z0-9_-]{8,128}$; other string fields capped at 8 KiBTestInsert_RejectsBadRunIDFormat,TestInsert_RejectsOversizedFieldThe most important guarantee is (c):
--ablation NoObserver(Phase 2 binding) MUST NOT silently drop runs. The audit log line[ablation] NoObserver: dropped run_id=<id>is the only trail a post-hoc investigator has for distinguishing "ablation was on" from "DB was offline".Tests
go test ./internal/evalrun/... ./cmd/evalrun-export/... -count=1 -shuffle=on -race— 16 + 18 tests, all passgo vet ./...cleangofmt -l internal/evalrun cmd/evalrun-exportcleanOBSERVER_DB=/tmp/empty.db evalrun-export --format=csv→ exactly the 24-column header, exit 0; byte-identical to the--db /tmp/empty.dbformReview process
Three-phase Codex review loop (spec → plan → code), iterated to
VERDICT: CLEANon each:artifact_hashes_json→artifact_hashes), P1 drift guard too narrow, P1 CSV escape missed\n, P1 ambiguous validation errors, P1 acceptance smoke command mismatch, P2 file-scope divergence, P2 SQLite rationale wording.cid, P1 CSV escape still missed\n.--dbprecedence not tested.--outclose errors swallowed, P1 unknown-filter test ignored exit code, P2 comment "six → seven".Deviations from kickoff prompt
Schemastruct also lists 24. User confirmed "24 (match 08 literally)".schema.sql, notmigrations/*_runs.sql— there is nomigrations/subdir at base; prompt §前置条件 fix(driver): clarify list_agents roles #3 explicitly authorises this.17f2c3cnot1332327— used the actual current head oforigin/paper/v3-integration(which already contains WT-1-ablation-registry PR WT-1-ablation-registry: typed FlagName registry for paper-v3 ablation flags #51); user confirmed.Out of scope (downstream worktrees own)
--ablation NoObserverCLI binding on consumer binaries → Phase 2WT-2-flag-integrationcapability_snapshot_hash/task_contract_hash/dynamic_mcp_registry_hash→ respective Phase 1/2 worktreesWT-1-eval-runner-skeleton13_workload_spec.mdWT-2-metric-extract🤖 Generated with Claude Code