feat(analyst): add deterministic control integrity checks - #487
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 74389245
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-29T20:27:59Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 113.0s (2 bridge agents) |
| Total | 113.0s |
💰 Value — sound
Adds a deterministic, zero-cost Analyst that turns SupervisorRunSources/SupervisorRunTree into structured integrity findings (duplicate IDs, dangling parents, cycles, temporal impossibilities, missing captures, steer-delivery gaps) without inventing uncaptured authority or intent.
- What it does: Introduces
ControlIntegrityAnalyst(src/analyst/kinds/control-integrity.ts:908) plus the pureemitControlIntegrityFindingsfunction (src/analyst/kinds/control-integrity.ts:888). It accepts eitherSupervisorRunSourcesorSupervisorRunTree, validates each rollout row, then emitsAnalystFinding[]for mechanically demonstrable defects: duplicated or danglingrollout_ids, parent cycles, - Goals it achieves: Gives consumers a cheap, repeatable, model-agnostic way to catch supervision-tree data-quality problems before they feed training, dashboards, or release gates. It makes the existing supervisor-run contracts auditable: instead of silently trusting the tree structure, a deterministic pass surfaces identity, parentage, timestamp, terminal-event, and capture defects with exact field evidence. It also
- Assessment: Good on its merits. The change is coherent with the codebase: it follows the same deterministic-analyst pattern as
behavioralAnalyst(src/analyst/behavioral-analyst.ts:89) andSkillUsageAnalyst(src/analyst/kinds/skill-usage.ts), reuses existing supervisor-run primitives (parseSupervisorTreeinsrc/supervisor-run/analyze.ts:182,supervisorRunRolloutLinesin `src/supervisor-run/rollou - Better / existing approach: none — this is the right approach. I checked for existing supervisor-run validators, rollout integrity checks, and deterministic analysts (
src/supervisor-run/analyze.ts,src/rollout/schema.ts,src/analyst/behavioral-analyst.ts,src/analyst/kinds/skill-usage.ts,src/analyst/kinds/index.ts). No existing module emitsAnalystFinding[]over supervisor-run structure; the supervisor-run modul - Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound-with-nits
A well-fitting deterministic analyst that adds supervisor-tree integrity checks (identity, parentage, cycles, timestamps, terminal events, steer-delivery) into the established AnalystFinding pipeline, with no unreachable surface and one minor efficiency nit.
- Integration: Reachable through two real paths. (1) Pure function
emitControlIntegrityFindings(input, producedAt)at src/analyst/kinds/control-integrity.ts:888 — any caller holding aSupervisorRunSources(produced by readLoopsSupervisorRun src/supervisor-run/loops-reader.ts:68 or readClaudeCodeSupervisorRun src/supervisor-run/claude-code-reader.ts:325) or aSupervisorRunTree(produced by supervisorRunRoll - Fit with existing patterns: Fits the codebase grain precisely. It reuses three existing contracts unchanged: the
Analyst<TInput>interface (types.ts:192),makeFinding/computeFindingIdfor stable finding ids (types.ts:227-277), and the existingSupervisorRunSources/SupervisorRunTree/parseSupervisorTree/analyzeSupervisorRunSourcesmachinery — it adds no new input type. Its unavailable-vs-zero discipline (`metadat - Real-world viability: Robust beyond the happy path. The ancestry walk is iterative with a resolved-set memo (control-integrity.ts:438-482), proven against a 10,000-node chain (test line ~walks 10k nodes). Cycle detection is gated on unique id resolution so ambiguous ids never produce guessed cycle claims (control-integrity.ts:435-437). Malformed rows are validated via
validateRolloutLineand excluded from relational - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🎯 Usefulness Audit
🟡 Steer-delivery check recomputes the full supervisor report to read two scalars [ergonomics] ``
src/analyst/kinds/control-integrity.ts:831 calls
analyzeSupervisorRunSources(source, () => 0)to obtainreport.orchestration.steersandreport.orchestration.steersDelivered. That call re-runs the entire analyzer — journal/state/result/judge/worker parse, waves, concurrency, idle, economics, outcome, patch stats — to read two numbers. The same function already calledparseSupervisorTree(source)at line 599, whoseworkerLogsmap carriessteersQueuedandsteersDeliveredper worker (an
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 5 non-blocking findings — 74389245
Full multi-shot audit completed 5/5 planned shots over 10 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-29T20:36:27Z · immutable trace
# Conflicts: # .evolve/skill-runs.jsonl # scripts/verify-package-exports.mjs
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — a23b3828
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-29T23:48:05Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 86.1s (2 bridge agents) |
| Total | 86.1s |
💰 Value — sound-with-nits
Adds a deterministic supervisor-run control-integrity checker (tree topology + steer-delivery defects) over the existing SupervisorRunSources/Tree contracts, plus a thin analyst adapter; it shares one freshly-extracted parser with the metrics analyzer and rollout minter, and does not duplicate any e
- What it does: Introduces analyzeSupervisorRunIntegrity(): a pure, deterministic pass over SupervisorRunSources or SupervisorRunTree that reports mechanically demonstrable defects — duplicate rollout ids, detached/missing parents, cross-run parent edges, time-ordering violations (child-before-parent, close-before-start), parent cycles, orphan/duplicate spawn+terminal events, missing terminal outcomes, and per-wo
- Goals it achieves: Give supervisor runs the same unavailable-vs-zero integrity discipline the codebase already enforces for single rollouts, but at the TREE level: detect producer defects in parentage, identity, timing, terminal events, and steer delivery that a metrics report cannot surface, and expose them as stable, diffable, zero-cost findings through the analyst registry so a campaign can gate or triage on them
- Assessment: A good change, built in the grain of the codebase. It (1) extends the established Measured/unavailable discipline rather than inventing a new one — tree-input honestly reports 'source-checks-unavailable' instead of silently skipping source-only checks; (2) reuses the existing Analyst contract and makeFinding envelope exactly as skill-usage and the other kinds do; (3) does the right DRY refactor
- Better / existing approach: none — this is the right approach. I checked for overlap with the three existing integrity surfaces (src/trace/integrity.ts run-capture checks, src/integrity/preflight.ts model preflight, and the SupervisorRunTreeGap codes produced by rollout-nodes.ts). The gaps are coarse mint-time summaries the integrity module consumes (e.g. integrity-tree.ts:106-108 reads gap code 'source-row-malformed'); the
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound
A deterministic control-integrity analyst that fits the established skill-usage grain exactly, adds a genuinely new integrity dimension (no duplication of the economics/orchestration report), and is reachable from three barrels plus exercised by the packed-export runtime check.
- Integration: Fully wired.
ControlIntegrityAnalyst(control-integrity.ts:62-83) declaresinputKind='custom'+id='control-integrity', which the registry routes viainputs.custom?.[analyst.id](registry.ts:465-466); control-integrity.test.ts:16-23 proves the registry runs it end-to-end.analyzeSupervisorRunIntegrityis exported from the supervisor-run subpath (index.ts:36) AND root index (src/index.ts: - Fit with existing patterns: Matches the established deterministic-analyst grain precisely. It is a near-exact structural twin of SkillUsageAnalyst (skill-usage.ts:387-402): same
inputKind='custom', samecost={kind:'deterministic',est_usd_per_run:0}, sameemitXxxFindingspure function + class + singleton-export triad. The failure/improvement/knowledge-gap kinds are the LLM trace-store pattern; skill-usage and control-in - Real-world viability: Holds up. The core
analyzeSupervisorRunIntegrity(integrity.ts:46-103) is pure/synchronous and handles the two input shapes with a sound discriminator:SupervisorRunSourceshas nonodesfield (types.ts:60-90) whileSupervisorRunTreedoes, soisTree/'nodes' in inputcannot misroute. Missing evidence is reported as explicit*-unavailableissue codes rather than collapsed to zero — the - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Barrel export ordering is inconsistent with neighboring kinds [maintenance] ``
In src/analyst/kinds/index.ts:12, src/analyst/index.ts:81, and src/index.ts:97 the control-integrity export block is placed BEFORE failure-mode/improvement/knowledge-* rather than in alphabetical or topical order with the other kinds. skill-usage is exported separately too, so there is no single enforced convention, but inserting the new block in a consistent position would keep these barrels scannable as more kinds land. Cosmetic only; does not gate shipping.
🟡 integrity-issues.ts is a 15-line two-helper file [maintenance] ``
src/supervisor-run/integrity-issues.ts contains only the
evidence()andissue()helpers plus the MAX_EXAMPLES constant. It could fold into integrity-types.ts (which already owns the issue/evidence type definitions) without loss of clarity, reducing the new file count by one. A /simplify pass already ran on this PR per .evolve/skill-runs.jsonl, so this is residual rather than unconsidered; minor.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Adds a deterministic analyst over the existing SupervisorRunSources and SupervisorRunTree contracts. It reports only mechanically demonstrable identity, parentage, timestamp, terminal-event, and capture defects; missing evidence remains explicitly unavailable.
Proof:
Known follow-up: PR #485 adds stable worker IDs. Until that lands, repeated display labels are reported as unjoinable rather than guessed.