Skip to content

feat(parser): add charm crush agent harness support - #1724

Draft
scross01 wants to merge 17 commits into
kenn-io:mainfrom
scross01:agent/charm-crush
Draft

feat(parser): add charm crush agent harness support#1724
scross01 wants to merge 17 commits into
kenn-io:mainfrom
scross01:agent/charm-crush

Conversation

@scross01

@scross01 scross01 commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Adds Charm Crush (charmbracelet/crush) as a supported agent.

Crush keeps one SQLite store per project at <project>/.crush/crush.db, with a project registry (projects.json) in its global data directory. The provider expands registry entries into roots, parses the parts-based message JSON (text, reasoning, tool_call, tool_result, finish), pairs tool results into system messages keyed by call ID, links subagent sessions through parent_session_id and the <uuid>$$<spawning-tool-call-id> child-ID convention, and emits one aggregate usage event per session from cumulative token totals and the recorded provider cost.

Notable decisions:

  • Timestamps are Unix seconds. Crush's schema comments claim milliseconds, but its own update triggers write seconds. Decoding uses seconds with a magnitude fallback for forward compatibility, covered by an era regression test.
  • Freshness hashes rows, not just mtimes. Second-resolution timestamps make same-second edits invisible to stat-based freshness, so the provider fingerprints the session and message rows and requires the hash for freshness.
  • Watcher work is bounded. Rowid cursors over sessions and messages mean a database event re-parses only sessions with new rows; a scheduled reconciliation pass covers metadata-only edits and deletes.
  • Format detection is schema-shape based. The messages.parts column identifies a Crush store; the vendored goose migration table (goose_db_version) proves nothing and must not be mistaken for the Goose agent's format.

Limits: Crush stores no per-request usage, so there is no per-message token attribution or cache-token breakdown; projects registered after startup are picked up by scheduled reconciliation; unknown future parts types are skipped rather than failing the session.

Where to look: internal/parser/crush.go (parser), internal/parser/crush_provider.go (discovery, cursors, fingerprint), internal/parser/crush_test.go (16 tests incl. provider-path usage and watcher-bounds coverage), the wiring in types.go/provider.go/taxonomy.go, frontend color/label, and the pinned provenance entry in docs/internal/session-format-sources.md.

@scross01
feat(parser): add charm crush agent harness support
488ca9b 
Charm Crush stores one SQLite database per project under
<project>/.crush/crush.db with a projects.json registry at its global
data directory. The new provider expands that registry into roots,
parses the parts-based message JSON (text, reasoning, tool_call,
tool_result, finish), pairs tool results into system messages, links
subagent sessions through parent_session_id, and emits one aggregate
usage event per session from the cumulative token totals and recorded
cost. Timestamps are decoded as Unix seconds, matching what Crush
actually writes despite its schema comments claiming milliseconds.
Windows users get the confirmed %LOCALAPPDATA%/crush data dir.
plans/ records the plan this implementation followed.
@scross01
fix(parser): harden charm crush provider after skill review
a5c015b 
Review against the add-new-agent checklist found the provider claimed
summary-message handling in the provenance doc without implementing it.
Summary rows (is_summary_message=1) now emit as compact-boundary system
messages with the original row's model attribution cleared. Freshness
gains a per-session row hash with FingerprintHashRequiredForFreshness so
same-second edits are not missed by the store's second-resolution
timestamps, and watcher events use bounded rowid cursors over sessions
and messages so work stays proportional to inserted rows instead of
re-enumerating the whole project database. The malformed parts policy
(fail that session's parse, matching goose) and cursor behavior are now
documented in the provenance entry.
@scross01
fix(parser): emit charm crush usage events on the parse result
2a1f1c7 
The sync engine writes usage rows only from ParseResult.UsageEvents,
but the crush provider left the aggregate session events on the
ParsedSession field, which feeds ID validation only, so crush sessions
synced with no token usage or cost on the usage page. The provider now
carries the events on the ParseResult, verified end to end against a
scratch archive.

Also wires subagent tool-call linking: crush names subagent sessions
<own-uuid>$$<spawning-tool-call-id>, so a parent's tool call now gets
SubagentSessionID and renders the inline child transcript.
@scross01
fix(parser): harden crush provider after review findings
5234248
@scross01
scross01 marked this pull request as draft September 11, 2026 12:42
@scross01
chore: remove planning notes from the branch
9126700 
The plans/ directory is a local advisor workspace documenting design
and review decisions; it does not belong in the upstream history.
@scross01
Merge branch 'main' into agent/charm-crush
9936265
@scross01
fix(parser): adapt crush provider to post-rebase parser APIs
8070af1 
The branch's rebase onto main changed the shared parser APIs the crush
provider predates: dbBackedProviderSpec.parse takes a context.Context,
provider specs are built per StableSourceSnapshots flag, SQLite stores
open with immutable=1 for stable snapshots, and inferToolSkillName
takes a context. Thread all of them through the crush parser and
provider, preserving the WAL-corruption fallback (a stable-snapshot
open now fails instead of silently degrading the snapshot guarantee)
and fixing the test call sites.
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (8070af1)

Verdict: Changes require fixes for 4 findings.

High

  • internal/parser/crush_provider.go:39-49: Registry roots are expanded into per-project directories before reconciliation scope resolution, so scheduled reconciliation of the original global registry directory usually resolves no scopes and misses new projects and metadata or deletion changes. Preserve the logical registry root for scope resolution and map it to expanded project data directories, including matching physical proof scopes.

    Reported by: codex

  • internal/parser/crush_provider.go:338-348: Fingerprinting unconditionally selects messages.provider, messages.is_summary_message, and finished_at, so valid older Crush stores lacking documented newer columns fail fingerprinting before parsing. Feature-detect optional columns and build SELECT expressions with safe defaults, or explicitly support each schema version without failing fingerprints for valid older stores.

    Reported by: codex

Medium

  • internal/parser/crush.go:47-49, internal/parser/crush.go:331-376: The registry parser discards each project's path and derives the project directory as two levels above crush.db, producing incorrect Cwd or project attribution for custom per-project data directories and potentially incorrect CWD filtering. Retain the registry path alongside each data directory and carry that mapping into parsing; use the filesystem-derived parent only as a fallback for direct .crush roots.

    Reported by: codex

  • internal/parser/crush_paths.go:8-14: The default directory list does not honor XDG_DATA_HOME or CRUSH_GLOBAL_DATA, so users relying on either documented environment variable receive no default discovery. Resolve the default Crush registry directory using the upstream XDG and CRUSH_GLOBAL_DATA rules when constructing configuration defaults.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 11s | Total: 8m15s

@scross01
fix(parser): address crush provider review findings
5f86ce6 
- Preserve registry root for reconciliation scope resolution by
  overriding ResolveReconciliationScopes to expand registry roots to
  their data directories before scope matching
- Feature-detect optional message columns (provider, finished_at,
  is_summary_message) in crushSessionFingerprint so valid older Crush
  stores without newer columns do not fail fingerprinting
- Retain registry path from projects.json for correct project
  attribution when data directories do not follow the default layout
- Honor CRUSH_GLOBAL_DATA and XDG_DATA_HOME environment variables for
  Crush default directory resolution
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (5f86ce6)

Verdict: Changes require fixes for 2 findings.

High

  • internal/parser/crush.go:475-486; internal/parser/crush_provider.go:682-689: Queries and watcher cursor logic reference optional Crush columns unconditionally, so older valid stores can fail parsing when provider or is_summary_message is absent and change tracking can fail when finished_at is absent. Feature-detect optional columns and generate queries and cursor expressions with safe defaults when they are absent.

    Reported by: codex

Medium

  • internal/parser/crush.go:374: Session parsing calls ExtractProjectFromCwd(projectDir) directly, bypassing the filesystem-discovery guard in the stable-snapshot context; remote or foreign project paths can therefore trigger local filesystem or Git probing. Use the context-aware project extraction function and pass the parse context through.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 13s | Total: 13m56s

@scross01
fix(parser): address crush review findings and CI failures
d149d89 
- Feature-detect optional columns (provider, is_summary_message,
  finished_at) in loadCrushMessages and crushRowIdentityExpression so
  older valid Crush stores without these columns do not fail parsing
  or change tracking
- Use ExtractProjectFromCwdWithBranchContext in parseCrushSession so
  stable-snapshot contexts skip local filesystem probing for foreign
  project paths
- Replace for-range loop with maps.Copy in normalizeCrushRoots to
  satisfy the modernize linter
- Fix TestCrushProviderDiscoveryAndRoots on Windows by using
  filepath.ToSlash to avoid invalid JSON escape sequences in test
  registry paths
- Add TestCrushParseSessionWithoutOptionalColumns to verify parsing
  works with minimal schema
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (d149d89)

Verdict: Changes require fixes for 2 findings.

High

  • internal/sync/engine.go:7741-7789, 13532-13539: AgentCrush is not included in the sync engine's dedicated DB-backed provider paths, so full syncs fall back to generic discovery, materialize all sessions, and cannot use Crush fingerprints for persisted freshness; unchanged sessions are reparsed on every full sync. Add AgentCrush to the DB-backed sync dispatch and related provider/freshness predicates, with a behavioral regression test proving unchanged sessions are skipped.

    Reported by: codex

  • internal/parser/crush_provider.go:40-59, 91-109: A configured root ending in crush.db is normalized to its parent data directory, but ResolveReconciliationScopes does not translate the original database-file root. Reconciliation can create a scope that excludes virtual session members, preventing complete coverage and leaving deleted sessions stale. Map direct crush.db request roots to the normalized data-directory root and include virtual members in the reconciliation scope. Add a test that configures a database-file root, deletes a session, and verifies reconciliation removes or tombstones it.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 10s | Total: 25m36s

@mariusvniekerk mariusvniekerk self-assigned this Sep 11, 2026
mariusvniekerk and others added 2 commits September 11, 2026 13:21
@mariusvniekerk
fix(sync): skip unchanged crush sessions
ce9a10b 
Crush keeps many sessions in one SQLite database. The generic sync path
parsed every session again even when its stored fingerprint still matched.

Use the database-backed sync path so repeated scans leave unchanged archive
rows untouched.
@scross01
fix(sync): wire crush into DB-backed sync and reconciliation scopes
4b32b04 
AgentCrush was provider-authoritative but missing from the dedicated
DB-backed full-sync dispatch and persisted-freshness predicates, so
unchanged sessions were reparsed on every full sync. Reconciliation
also failed to map configured crush.db roots onto the normalized data
directory, leaving deleted sessions stale when the database file was
the request root.
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (4b32b04)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/parser/crush.go:103-129: When the normal read-only SQLite connection fails, openCrushDB falls back to immutable mode and may read only the main database while ignoring an existing WAL, allowing stale session contents to overwrite the archive. Do not silently downgrade live-store reads to immutable mode; surface the original error, or require an explicit request for immutable snapshots.

    Reported by: codex

  • internal/parser/crush_provider.go:NewProvider and SourcesForChangedPath: Crush sources discovered from a registry root or configured crush.db file normalize to different data-directory roots without preserving SourceRef.ConfiguredRoot, so source-machine mapping falls back to path-based or default attribution. Preserve the original configured registry/database root on every generated SourceRef, including full-discovery and changed-path sources.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m58s

@scross01
fix(parser): stop crush immutable fallback and preserve ConfiguredRoot
88dc98f 
Live crush.db opens must not silently downgrade to immutable mode,
which ignores WAL content and can overwrite the archive with stale
sessions. Crush sources discovered from a registry or crush.db root
also now keep the original configured spelling on ConfiguredRoot so
source-machine mapping does not fall back to the expanded data dir.
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (88dc98f)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/parser/crush_provider.go:44-60,160-184: Reconciliation expands registry or database-file roots to data directories, then reconstructs providers from those expanded roots, losing the original registry mapping, project path, and configured machine association. Custom registry layouts can therefore be parsed with the wrong project/CWD and sessions can be attributed to the local machine. Preserve the original-root metadata through reconciliation scopes, or construct scoped providers with an alias mapping back to the configured registry/database root.

    Reported by: codex

  • internal/parser/crush_provider.go:506-520: The fingerprint query unconditionally selects messages.updated_at even though validateCrushSchema does not require that column and the parser's minimal accepted schema omits it. Because fingerprint freshness is mandatory, such stores parse directly but fail every provider fingerprint/sync operation. Feature-detect messages.updated_at when building the fingerprint query, or require it consistently in schema validation and remove the claimed compatibility.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 10s | Total: 12m29s

scross01 and others added 3 commits September 11, 2026 15:31
@scross01
fix(parser): keep crush roots through reconciliation and drop optiona…
1771c53 
…l updated_at

Scoped reconciliation providers were rebuilt from expanded data
directories, losing registry expansion, project mapping, and the
original configured root for machine attribution. TraversalRoots now
carry the original configured spelling so NewProvider reconstruction
restores that metadata.

Fingerprint queries no longer require messages.updated_at, which
validateCrushSchema does not demand; minimal accepted stores parse
but previously failed every fingerprint.
@mariusvniekerk @codex
Merge upstream Crush fixes
bc0b8cb 
Bring the PR branch's newer Crush parser and sync work into the local fix
before publication.

The combined history preserves Crush database roots for reconciliation and
uses the database-backed sync path. Missing source sessions remain in the
archive.

Generated with Codex
Co-authored-by: Codex <codex@openai.com>
@mariusvniekerk @codex
Merge latest upstream Crush parser fix
73e50e0 
Bring the contributor branch's newest reconciliation and fingerprint fixes
into the combined branch before publication.

Scoped reconciliation keeps the configured Crush root when it rebuilds a
provider. Fingerprints also work for valid stores without messages.updated_at.

Generated with Codex
Co-authored-by: Codex <codex@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (73e50e0)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/parser/crush_provider.go:348-366: The Crush fingerprint hashes only SQLite rows, while parsing derives Project and Cwd from projects.json. If a registry entry changes its project Path while retaining DataDir, sync sees the same fingerprint and skips reparsing, leaving stale project metadata. Include the resolved project path or registry mapping generation in the fingerprint, or invalidate affected sources when registry metadata changes; add a behavioral regression test.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 9s | Total: 16m23s

mariusvniekerk and others added 2 commits September 11, 2026 18:11
@mariusvniekerk @codex
fix(sync): refresh crush project metadata
09149f1 
A Crush registry can change a project's path without changing its data
directory or SQLite rows. The old fingerprint treated that session as
unchanged and left its archived project metadata stale.

Include the resolved project directory in the fingerprint so the next sync
updates the session while unchanged registry entries still skip parsing.

Generated with Codex
Co-authored-by: Codex <codex@openai.com>
@mariusvniekerk
fix(parser): refresh crush derived session data
13204b5 
Crush derives parent tool-call links from child session rows. The old fingerprint skipped the parent after a child appeared, so the archive kept an empty link.

A registry can also enumerate sibling data directories during scoped reconciliation. Keep those directories inside the traversal boundary while proof remains limited to the requested project.
@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (13204b5)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/parser/crush.go:375-378: Tool rows converted to system RoleUser messages can still be counted as human messages when tool_call_id is missing or unparseable, and CountsAuthoritative prevents correction. Exclude system messages from the count, such as by requiring !message.IsSystem.

    Reported by: codex

Reviewers: 2 done | Synthesis: codex, 9s | Total: 9m10s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@mariusvniekerk mariusvniekerk

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@scross01 @mariusvniekerk