Skip to content

feat(analyst): add deterministic control integrity checks - #487

Merged
drewstone merged 4 commits into
mainfrom
feat/control-integrity-analyst
Jul 29, 2026
Merged

feat(analyst): add deterministic control integrity checks#487
drewstone merged 4 commits into
mainfrom
feat/control-integrity-analyst

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

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:

  • 4,226 tests passed, 3 skipped
  • 11 focused tests passed
  • typecheck, lint, build, package verification passed
  • clean merge against origin/main

Known follow-up: PR #485 adds stable worker IDs. Until that lands, repeated display labels are reported as unjoinable rather than guessed.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 pure emitControlIntegrityFindings function (src/analyst/kinds/control-integrity.ts:888). It accepts either SupervisorRunSources or SupervisorRunTree, validates each rollout row, then emits AnalystFinding[] for mechanically demonstrable defects: duplicated or dangling rollout_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) and SkillUsageAnalyst (src/analyst/kinds/skill-usage.ts), reuses existing supervisor-run primitives (parseSupervisorTree in src/supervisor-run/analyze.ts:182, supervisorRunRolloutLines in `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 emits AnalystFinding[] 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 a SupervisorRunSources (produced by readLoopsSupervisorRun src/supervisor-run/loops-reader.ts:68 or readClaudeCodeSupervisorRun src/supervisor-run/claude-code-reader.ts:325) or a SupervisorRunTree (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/computeFindingId for stable finding ids (types.ts:227-277), and the existing SupervisorRunSources/SupervisorRunTree/parseSupervisorTree/analyzeSupervisorRunSources machinery — 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 validateRolloutLine and 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 obtain report.orchestration.steers and report.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 called parseSupervisorTree(source) at line 599, whose workerLogs map carries steersQueued and steersDelivered per 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.

value-audit · 20260729T203013Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 74389245

Review health 100/100 · Reviewer score 80/100 · Confidence 85/100 · 5 findings (5 low)

glm: Correctness 80 · Security 80 · Testing 80 · Architecture 80

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 10 changed files. Global verifier still owns final merge decision.

🟡 LOW Snippet passes SupervisorRunTree but analyst also accepts SupervisorRunSources — docs/trace-analysis.md

The example computes tree then passes it under custom, which is valid. The ControlIntegrityAnalyst.analyze accepts SupervisorRunSources | SupervisorRunTree, so the prose could note either surface is acceptable (passing sources directly avoids the extra supervisorRunRolloutLines call for callers who only want integrity checks). Cosmetic; not blocking.

🟡 LOW New exports verified at compile-time only, not in runtime --eval blocks — scripts/verify-package-exports.mjs

emitControlIntegrityFindings and ROOT_CONTROL_INTEGRITY_ANALYST are imported and placed in the void[] array (lines 434-437), which only verifies TypeScript import resolution against .d.ts files. Unlike some other exports (e.g., RawAnalystFindingSchema at line 324 which is exercised via .parse()), these new symbols are never invoked at runtime. If a bundler tree-shaking bug dropped a value export while keeping its type declaration, tsc would pass but runtime import would fail. This is consistent with how many existing expo

🟡 LOW Orphan-close evidence collapses to last event when duplicates share an id — src/analyst/kinds/control-integrity.ts

orphanCloses is a Map<string, CloseRow> keyed by close.id. When two orphan terminal events share the same id, only the last is retained for the orphan finding's evidence (kind/at). The duplicate-terminal finding (closesById count >= 2) still reports the correct count, so multiplicity is not lost — but the orphan finding's rationale names only one of the two events' kind. The test 'detects orphan and duplicate terminal journal events' confirms both findings fire. Minor evidence fidelity gap; the count finding compensates.

🟡 LOW Per-node finding_id can collide across runs that share rollout_id values — src/analyst/kinds/control-integrity.ts

computeFindingId hashes {analyst_id, area, subject, claim}. Per-node findings use subject=<rollout_id> with no run-scoping (the namespace/runRef is used only in evidence URIs, never in subject). Two different supervisor runs that both contain a node with rollout_id 'worker-1' and the same structural defect (e.g. 'A child invocation is timestamped before its parent started') produce identical finding_ids. A cross-run diffFindings consumer would collapse them as appeared/disappeared incorrectly. In production rollout_ids are typically UUID-scoped so this is theoretical, but fixtures and short-id producers would hit it. Consistent with the computeFindingId contract, so this is informational, not a contract violation.

🟡 LOW parseSupervisorTree invoked up to 3 times for a single SupervisorRunSources input — src/analyst/kinds/control-integrity.ts

For a source input, parseSupervisorTree runs once inside supervisorRunRolloutLines (line 883), again at the top of sourceFindings (line 524: parseSupervisorTree(source)), and a third time inside analyzeSupervisorRunSources (line 683). All three are pure and idempotent so correctness is unaffected, but the journal and every worker event stream are re-parsed three times. For a large


tangletools · 2026-07-29T20:36:27Z · trace

tangletools
tangletools previously approved these changes Jul 29, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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) declares inputKind='custom' + id='control-integrity', which the registry routes via inputs.custom?.[analyst.id] (registry.ts:465-466); control-integrity.test.ts:16-23 proves the registry runs it end-to-end. analyzeSupervisorRunIntegrity is 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', same cost={kind:'deterministic',est_usd_per_run:0}, same emitXxxFindings pure 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: SupervisorRunSources has no nodes field (types.ts:60-90) while SupervisorRunTree does, so isTree/'nodes' in input cannot misroute. Missing evidence is reported as explicit *-unavailable issue 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() and issue() 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.

value-audit · 20260729T235003Z

@drewstone
drewstone merged commit 6f3933b into main Jul 29, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants