Skip to content

Add Kilo parser support#679

Open
maphew wants to merge 6 commits into
kenn-io:mainfrom
maphew:add-kilo-parser
Open

Add Kilo parser support#679
maphew wants to merge 6 commits into
kenn-io:mainfrom
maphew:add-kilo-parser

Conversation

@maphew

@maphew maphew commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Add Kilo parser support

Kilo (https://github.com/Kilo-Org/kilocode) is a fork of OpenCode
that uses the identical session storage format. This PR adds Kilo as
a supported agent using the same parsing logic as OpenCode, reading
sessions from ~/.local/share/kilo/storage/.

Closes #617

@roborev-ci

roborev-ci Bot commented Jun 13, 2026

Copy link
Copy Markdown

roborev: Combined Review (27a1e1d)

Medium-risk issues remain in the Kilo sync support; no security findings were reported.

Medium

  • internal/sync/engine.go:4161
    Kilo storage writes are not forced through full message replacement, even though Kilo reuses OpenCode’s mutable storage format. After the first sync, edits or deletions in existing message/part JSON files can be ignored by the append-only writer while session metadata still advances.
    Fix: Force full replacement for Kilo storage writes, either by setting forceReplace: true in the storage branch or by adding parser.AgentKilo to both replacement predicates.

  • internal/sync/engine.go:5680
    Archive preservation remains OpenCode-only, so Kilo hybrid roots can overwrite a preserved storage transcript with older or incomplete kilo.db fallback data during normal sync or resync.
    Fix: Generalize the preservation helper for OpenCode-format agents and use Kilo’s virtual-path/fingerprint checks for AgentKilo.

  • internal/sync/engine.go:1619
    The resync empty-discovery guard subtracts OpenCode and Kiro SQLite sessions from the old file-backed count, but not Kilo SQLite sessions. A legacy SQLite-only Kilo install can sync through syncKilo successfully and still abort the DB swap because file discovery is zero.
    Fix: Subtract root Kilo SQLite sessions from oldFileSessions, for example by counting agent = 'kilo' rows with file_path matching kilo.db#.


Panel: ci_default_security | Synthesis: codex, 11s | Members: codex_default (codex/default, done, 4m59s), codex_security (codex/security, done, 1m10s) | Total: 6m20s

@maphew maphew left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Found one issue:

  • Medium: Kilo storage sync does not force message replacement at internal/sync/engine.go:4161. Kilo uses the same mutable storage format as OpenCode, where message and part JSON can be rewritten in place during streaming/tool updates. processKilo returns a normal processResult for storage files, and writeBatch only forces replacement for AgentOpenCode at internal/sync/engine.go:5340 and internal/sync/engine.go:5449. In a normal sync after a Kilo message/part changes without adding a new ordinal, writeMessages will append nothing and leave stale message content in the database. Kilo should either set forceReplace: true for parsed storage sessions or be included wherever OpenCode forces replacement.

gpt-5.5 on behalf of matt

@roborev-ci

roborev-ci Bot commented Jun 13, 2026

Copy link
Copy Markdown

roborev: Combined Review (bffcc50)

Summary verdict: one medium correctness issue remains; no security issues were reported.

Medium

  • Location: internal/sync/engine.go:1621
    • Problem: ResyncAll only subtracts SQLite-backed Kilo rows from oldFileSessions. Kilo storage rows now use the same archive-preservation path as OpenCode, so if every Kilo storage write is intentionally preserved, stats.Synced stays 0 while oldFileSessions remains >0, causing the abort guard to fire before orphan-copy can preserve and swap the rebuilt DB.
    • Fix: Treat Kilo like OpenCode in this guard by subtracting all root Kilo sessions, or otherwise allow preserved Kilo storage sessions. Add a Kilo ResyncAll storage-archive preservation test.

Panel: ci_default_security | Synthesis: codex, 21s | Members: codex_default (codex/default, done, 5m35s), codex_security (codex/security, done, 2m18s) | Total: 8m14s

@mjacobs

mjacobs commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, @maphew — a fork that reuses OpenCode's storage format wholesale should be cheap to add, and you've leaned on the existing OpenCode plumbing rather than reinventing it (isOpenCodeFormatStorageAgent, hasOpenCodeFormatStorageFingerprint, the shared replace gates). The kilo.go delegation is clean and Closes #617 is welcome. Two things in the sync lifecycle to flag, both in internal/sync/engine.go.

1. Dead isOpenCodeStoragePath is failing the lint check

isOpenCodeStoragePath is now unused — the storage-archive check that called it was generalized to isOpenCodeFormatStoragePath(agent, path), orphaning the old single-agent helper. It's the only red check on the PR:

internal/sync/engine.go:5807:6: func isOpenCodeStoragePath is unused (unused)

Deleting it should clear lint.

2. ResyncAll storage-preservation accounting: a storage-only Kilo archive can abort the swap

This is the one worth a careful look. countRootKiloSQLiteSessions is modeled on Kiro — it filters to the SQLite subset:

file_path LIKE '%kilo.db#%'

But Kilo isn't shaped like Kiro here — it's a hybrid like OpenCode, and the PR wires it that way: isOpenCodeFormatStorageAgent returns OpenCode || Kilo, so storage-mode Kilo rows ride OpenCode's archive-preservation path. When a parse is preserved against the archive it contributes 0 to Synced, so a storage-only archive where every parse is preserved resyncs to Synced == 0.

The trap is in ResyncAll. oldFileSessions starts from FileBackedSessionCount (every agent not in NonFileBackedAgents() — Kilo included), then subtracts each preserved-root count. Because the Kilo subtraction only covers the kilo.db# subset, storage-mode Kilo rows are never subtracted. So a storage-only Kilo archive ends with Synced == 0 and oldFileSessions > 0, which makes preservedOnly false (it needs oldFileSessions == 0 || trashedCopied > 0) and trips the abortSwap guard — the correctly-rebuilt temp DB is discarded before orphan-copy runs, so the archive isn't restored on that pass.

Kiro can use the filtered count because its file-backed sessions are a genuinely separate source that re-syncs through the normal pipeline; OpenCode/Kilo storage rows are not. The fix is to count Kilo like OpenCode — drop the file_path filter so the Kilo subtraction covers every root Kilo row, matching countRootOpenCodeSessions.

For context, not a knock: OpenCode already special-cases this exact oldFileSessions subtraction (no file_path filter) for the same reason, so the fix is just bringing Kilo in line with the agent whose path it reuses. The reason it's invisible in normal use is that it's data-shape-dependent — it never reproduces for anyone whose archive includes a kilo.db session, only for a storage-only archive.

On the tests

The Kilo tests are solid for what they cover, but I don't think either exercises this path:

  • TestKiloPreservesStorageArchiveAgainstSQLiteFallback tests storage preservation, but through SyncAll/SyncPaths, not ResyncAll.
  • TestResyncAllAllowsKiloSQLiteOnlySessions runs ResyncAll, but on a kilo.db SQLite session that re-parses fresh (Synced == 1) — so the abort guard is never reached via the preservation branch.

The missing case is a storage-only Kilo archive (every parse preserved, Synced == 0) run through ResyncAll, asserting stats.Aborted == false and that rows survive. OpenCode already has the exact mirror to copy — TestResyncAllOpenCodeStorageArchivePreservesStaleSQLiteFallback — which runs ResyncAll on a preserved storage archive and asserts Aborted == false with Synced == 0. That's the shape; a require.NotZero(stats.Synced) assertion would give false confidence, since a clean resync re-parses storage rows and never hits the guard.

Smaller notes / questions

  • +577 lines in engine.go for a format-identical fork. Given Kilo is byte-for-byte OpenCode's storage format, this is more engine surface than I'd have expected. A lot of it looks like agent-parameterized variants of existing helpers (isOpenCodeFormatStoragePath, the per-agent switches), which is reasonable — but is there room to make the OpenCode path itself storage-format-agent-aware (a small set of OpenCode-format agents) so the next fork is a registry add rather than another ~500 engine lines? Genuine question, no objection if the current factoring is cleanest.
  • Token usage — since Kilo shares OpenCode's storage schema, opencode.go's cache.read/cache.writecache_read_input_tokens/cache_creation_input_tokens normalization should apply unchanged via delegation. Have you eyeballed cost/usage for a real Kilo session to confirm the tokens land? (Usage rows are silently filtered on token_usage != '' AND model != '', so a key mismatch is zero cost with no error.)
  • parse-diff — if you have a real Kilo archive, agentsview parse-diff --agent kilo --fail-on-change against a freshly-resynced (quiescent) archive is the quickest empirical check that the write-path round-trips with zero drift.
  • Registry-completeness — heads up that TestRegistryCompleteness only asserts listed → registered, and its allTypes list already omits several agents, so a green run there doesn't actually gate Kilo's inclusion. Not your bug, just don't read it as coverage.

Happy to look again once the lint fix and the resync accounting land — the hard part (matching OpenCode's storage semantics) is right.

@maphew

maphew commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

@mjacobs I pushed commit 43a8b75 with the review follow-ups:

  • removed the unused isOpenCodeStoragePath helper
  • fixed ResyncAll accounting so Kilo subtracts all root OpenCode-format sessions, including storage-mode rows
  • consolidated several Kilo/OpenCode storage-format branches behind shared OpenCode-format helpers
  • added a storage-only Kilo resync preservation regression test asserting the swap does not abort and the archived rows survive

Validation passed:

  • go test ./internal/sync -run 'TestResyncAll(OpenCodeStorageArchivePreservesStaleSQLiteFallback|KiloStorageArchivePreservesStaleSQLiteFallback|OpenCodeStorageArchiveAllowsNewerSQLiteFallback|OpenCodeOnly|KiroSQLiteOnly)$'
  • go test ./internal/parser
  • go test ./...

I could not run agentsview parse-diff --agent kilo --fail-on-change because this checkout does not currently expose a parse-diff command, and there is no real Kilo archive fixture in the repo beyond parser tests.

gpt-5.5 on behalf of @maphew

@roborev-ci

roborev-ci Bot commented Jun 14, 2026

Copy link
Copy Markdown

roborev: Combined Review (6b80220)

Medium-risk parse-diff gaps remain in Kilo support.

Medium

  • internal/sync/parsediff.go:270: Kilo is now advertised as parse-diff supported, but parseDiffDatabaseSources and stripVirtualSourceSuffix only handle Kiro/Zed/OpenCode shared DB paths. SQLite-only Kilo sessions synced from kilo.db may not be re-parsed and can be reported as skipped/source-missing instead of compared. Add Kilo kilo.db synthesis and kilo.db#id suffix stripping, with coverage for SQLite-only and hybrid Kilo roots.

  • internal/sync/engine.go:4139: Kilo processing applies stored mtime/data-version skips even when e.forceParse is set. parse-diff relies on force-parse to re-parse every discovered source, so clean Kilo storage sessions can emit no parse result and be falsely treated as missing/presence-changed. Mirror OpenCode behavior by bypassing Kilo skip checks under forceParse, and guard the DB meta skip with !e.forceParse.


Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 5m51s), codex_security (codex/security, done, 53s) | Total: 6m52s

@mjacobs

mjacobs commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Re-checked the follow-up locally — the three things from last pass are all resolved cleanly (dead isOpenCodeStoragePath gone; ResyncAll now subtracts all root Kilo rows via countRootOpenCodeFormatSessions, with TestResyncAllKiloStorageArchivePreservesStaleSQLiteFallback pinning the Synced==0/Aborted==false case; full suites green here).

The newer roborev pass flagged two parse-diff gaps, and I confirmed both against the branch. They matter because Kilo is auto-advertised as parse-diff-supported (resolveParseDiffAgents is registry-derived), so shipping it half-wired makes parse-diff --agent kilo report false drift rather than just skipping Kilo:

  1. parseDiffDatabaseSources synthesizes only Kiro/OpenCode db sources and stripVirtualSourceSuffix ignores kilo.db#<id> (even though this PR defines ParseKiloSQLiteVirtualPath), so SQLite-only Kilo sessions report as source-missing.
  2. forceParse isn't honored for Kilo at two sites — the kilo.db fan-out skip in processKilo (missing !e.forceParse, which processOpenCode has) and shouldSkipKiloByPath (missing the if e.forceParse { return false } that shouldSkipOpenCodeByPath opens with).

Since parse-diff is new from our side, I've wired all four sites to mirror OpenCode, added TestParseDiffCoversMixedKiloRoot, and opened it as a PR into your branch so Kilo lands fully parse-diff-wired in one go: maphew#1. Feel free to merge, tweak, or push back. (One smaller parity item I left out of that PR: the kilo.db branch logs-and-skips per-session parse errors where OpenCode surfaces them as DiffParseError — easy follow-up if you want it.)

@roborev-ci

roborev-ci Bot commented Jun 15, 2026

Copy link
Copy Markdown

roborev: Combined Review (47c87f5)

Medium finding: Kilo parse-diff can miss per-session parse failures.

  • Severity: Medium
  • Location: internal/sync/engine.go:4148
  • Problem: Kilo kilo.db fan-out silently logs and skips per-session parse errors even when e.forceParse is set for parse-diff. Unlike Kiro/OpenCode/Zed, these errors are not returned in sessionErrs, so an unstored malformed Kilo SQLite session can be omitted from the parse-diff report and --fail-on-change can pass without reporting the parse failure.
  • Fix: Mirror processOpenCode/processKiro: collect sessionErrs when e.forceParse, attach meta.SessionID and meta.VirtualPath, and return them in processResult.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 5m29s), codex_security (codex/security, done, 2m18s) | Total: 7m55s

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Request: Kilo code support

2 participants