chore: add ktlint and apply one-shot format pass - #58
Merged
Merged
Conversation
- ktlint Gradle plugin 14.2.0 via version catalog, applied to all modules; ktlintCheck runs as part of check (CI enforcement lands in a follow-up PR) - .editorconfig: ktlint_official style, trailing commas, max line 140; @Composable/@ObjCName functions exempt from function-naming - ktlintFormat across the codebase: 67 files, mechanical changes only (indentation, trailing commas, import order); no logic changes
- .editorconfig: ij_kotlin_code_style = intellij_idea (JetBrains coding conventions from kotlinlang.org), trailing commas and 140-col limit kept - ktlint comment mechanics: comment-wrapping, no-single-line-block-comment - AGENTS.md: comments policy for agents - comments only for 'why', never narrating 'what'; AI-style narration comments prohibited
- ktlint-rules module: sharingan-comments ruleset (ktlint 1.5.0 API)
- todo-without-issue: TODO/FIXME must reference an issue number
- no-narration-comment: flags AI-style change notes ('// added ...')
- wired into every ktlint-enabled module via ktlintRuleset; excluded
from BCV; generated code (SQLDelight) excluded from all ktlint runs
- rules are heuristics with a documented ceiling (see ponytail note in
SharinganRules.kt); suppress with // ktlint-disable if a false
positive appears
The member is deprecated in ktlint's 1.5.x API but remains the abstract member RuleSetProviderV3 requires; no replacement exists on this version.
- ktlint-rules: no-multi-line-comment flags runs of 2+ adjacent // lines and non-KDoc block comments spanning lines; KDoc is exempt (docs) - existing multi-line comments condensed to single lines, keeping the 'why' (tests, descriptors, build scripts, notification guard) - AGENTS.md: comments policy updated to cover the new rule
Audit of all remaining inline comments against the AGENTS.md policy: - deleted 9 pure banner dividers (test names and function names already say what follows) and one what-narration comment (bottom fade) - rewrote the DetailScreen banner to a plain single-line pointer - everything else kept: issue-linked why-comments, invariants, workarounds, seam pointers
mibrahimdev
added a commit
that referenced
this pull request
Aug 31, 2026
develop gained a ktlint ruleset (#58) after this branch forked: standard formatting rules plus a custom no-multi-line-comment rule. Apply ktlintFormat and condense every multi-line // run this branch introduced into a single line, per the AGENTS.md comments policy. KDoc is exempt, so the Persistence lifecycle docblock is unchanged. Also applies the ktlint plugin to :sharingan-db. The module was created on this branch before the ruleset existed, so it was escaping the lint gate the same way it was escaping the BCV gate — adding it surfaced eight violations that were previously invisible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBhi6Qx8rdo1aX9EnmtEJS
mibrahimdev
added a commit
that referenced
this pull request
Sep 1, 2026
* feat: write-behind capture — events survive process death (#50) Slice 2 of the flight-recorder epic. Events now flow off the in-memory ring buffer into the on-device DB, so logs survive process death. - SharinganStore gains an internal `onRecord` seam, invoked after the unchanged lock-free CAS append (only while recording). No public change. - PersistenceController owns CoroutineScope(SupervisorJob()+Dispatchers.Default), wires `onRecord = { channel.trySend(it) }` on a bounded channel, and drains it with a single flusher coroutine that writes each batch in one SQLDelight transaction (size ~50 / 250 ms, whichever first). - Lazy session row created on the first flushed event of the launch. - internal @serializable EventDto (Http/Mqtt/Ble) with fromEvent() encode-only; public event ABI untouched. Stored as the event payload_json blob. - Persisted event PK is globally unique across sessions: session id is timestamp+random, and the event row id prefixes the raw EventIds value with the session id (EventIds resets each launch, so the raw value is not safe as a cross-session PK). - Hands-free on Android via the existing manifest ContentProvider. - Tests: burst > ring capacity all persisted, batching (not one-write-per-event), record() unchanged when persistence off, cross-session id uniqueness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: review follow-ups — DROP_OLDEST, idempotent start, drain-on-stop, session commit (#50) Addresses both reviewers' REQUEST-CHANGES on #56. - Channel now uses BufferOverflow.DROP_OLDEST so the crash-tail (newest events) survives backpressure, never the oldest. Pinned by a direct-channel test. - start() guarded by an AtomicBoolean (idempotent — no double flusher). - Persistence bootstrap made thread-safe with an AtomicBoolean CAS (`synchronized` is JVM-only, unavailable in commonMain). - stop() drains the pending channel + in-flight batch (channel.close() then join), then cancels the scope and closes the SqlDriver. - sessionId memoized only AFTER the batch transaction commits; reset on rollback so a later batch re-derives a rolled-back session row. - Per-batch transaction wrapped in try/catch so one failure never kills the flusher coroutine. - Tests: DROP_OLDEST overflow semantics, stop() drains pending batch; two-session test no longer stop()s (stop now closes the shared driver). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: use a real UUID for session ids (#50) timestamp+Random collided on Kotlin/Native (same millis + Native Random.Default returning the same value on rapid successive calls), so the two sessions in the test produced one PK and the second insert hit a UNIQUE violation (swallowed by the flusher's try/catch). kotlin.uuid.Uuid.random() is collision-free on every target; started_at stays currentTimeMillis(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ios): unique per-driver in-memory DB name (#50) The constant "sharingan-test.db" with inMemory=true mapped to a single file:...?cache=shared in-memory DB on Kotlin/Native, so every createTestDriver() in the test binary shared one database and earlier tests leaked ~700 rows into the two-sessions test. A per-driver UUID name gives each test a private DB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: enforce foreign keys so session deletes cascade (#50) Enable PRAGMA foreign_keys / setForeignKeyConstraintsEnabled on Android, iOS driver configuration, and JVM test drivers. Add deleteSession query and a cascade test that fails when FKs are off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: flush on a real latency deadline not an idle timeout (#50) Stamp a deadline when the first event enters an empty batch and use the remaining time in withTimeoutOrNull. A slow steady stream now flushes within the configured interval instead of waiting for the batch size. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor: controller owns the driver it opens (#50) Give PersistenceController a three-method lifecycle: start() wires the seam, stop() drains and leaves the driver open for readers, close() stop()s then cancels the scope and closes the driver. Tests inject the driver through an internal constructor; production uses the no-driver constructor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor: extract :sharingan-db so persistence leaves the public ABI (#50) Move SQLDelight schema, drivers, persistence controller and tests into a new :sharingan-db module. The controller becomes a generic PersistenceController<T> that receives a (T) -> EventRow mapping, keeping JSON encoding off the hot record() path. :sharingan consumes :sharingan-db as an implementation dependency, so the SQLDelight-generated types are no longer exported to the iOS framework header. - EventDto and toRow stay in :sharingan, package dev.sharingan.internal - Add SharinganDbContext for the Android context seam - Add Now.* equivalents for :sharingan-db's own time source - Update CI, BCV config, and release docs for the third artifact Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: warn that PersistenceController.stop() does not detach the seam (#50) Add KDoc warnings to stop() and close() so slice 4's configure()/shutdown path knows that events submitted after stop() are silently dropped unless the caller detaches store.onRecord first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: add EventDto encode coverage (#50) Pin the JSON encoding of HttpEvent / MqttEvent / BleEvent via EventDto in :sharingan, including the redacted-header value. Uses the same json instance that toRow() uses (now internal for test visibility). No decode round-trip — toEvent() is slice 3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: swallow toRow mapper throws inside flush so the flusher survives (#50) The caller-supplied (T) -> EventRow mapping can throw (e.g. JSON encode of an unbounded body). Move it inside the existing batch try/catch so the exception drops one batch instead of escaping runFlusher and crashing the host process. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: use atomic select instead of withTimeoutOrNull in flusher (#50) withTimeoutOrNull can prompt-cancel after taking an element from the channel, losing events under backpressure. Replace the timeout path with select { channel.onReceiveCatching { ... }; onTimeout { ... } } so the receive and timeout are atomic. The race itself is not deterministic in a unit test; the new focused test pins the deadline-flush path that the select exercises. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: hold flusherJob in an AtomicReference to avoid a null race with stop (#50) A concurrent stop() could read flusherJob before start() assigned it, skip the join, and tear down the scope while a transaction was in flight. Replace the AtomicBoolean + var pair with a single AtomicReference<Job?> used as the start/stop gate; stop atomically reads-and-clears it before joining. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor: make channel capacity an internal constructor param and test DROP_OLDEST (#50) Expose channelCapacity through the internal constructor so tests can exercise backpressure without relying on production defaults. Rename the 500-event burst test to reflect that it stays within the channel, and add a focused DROP_OLDEST test that submits before start() so the flusher cannot drain during the burst. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: document SharinganStore onRecord seam contract (#50) Add focused contract tests showing that the in-memory store forwards every accepted event to the internal persistence seam, skips forwarding while paused, and still forwards events that are later evicted by the ring buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: make PersistenceController single-use and start flusher LAZY (#50) Reviewers found that the finding-#3 AtomicReference rewrite reintroduced finding-#2 one method up: start() launched the flusher eagerly before the CAS, so a redundant start() cancelled a coroutine that could already be suspended in channel.receive(), losing events. - Fix A: launch the flusher with CoroutineStart.LAZY and call job.start() only inside the CAS-won branch. Drop the losing job's cancel() entirely. - Fix B: add a terminal `stopped` state. stop() sets it; start() after stop() throws IllegalStateException. Update KDoc on start()/stop() to state single-use semantics and remove the now-wrong 'Idempotent' label. - Fix C: document that the `select`/`onTimeout` usage is ExperimentalCoroutinesApi and load-bearing for the published module. - Optional nit: collapse readAndClearFlusherJob() to flusherJob.exchange(null). Add a focused TDD test proving a redundant start() dispatches exactly one flusher and loses no events, and rewrite the vacuous start/stop-cycle test into an assertion that start()-after-stop() throws. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: PersistenceController native compile blocker, stop() race, and deterministic test (#50) - Fix 1 (iOS CI blocker): replace the JVM-only CountingDispatcher/Runnable approach in PersistenceControllerTest with an internal `flusherStartCount()` seam backed by an AtomicInt. This removes the `java.lang.Runnable` reference that broke Kotlin/Native compilation. - Fix 2 (single-use hole in stop()): always close the channel before reading and joining the flusher job. A concurrent start() that wins the CAS after stop() sets the terminal flag now starts on a closed channel and exits cleanly instead of leaving a live flusher behind. - Fix 3 (deterministic double-start test): assert `flusherStartCount() == 1` after two start() calls instead of string-matching a kotlinx.coroutines internal class name. - Optional nit: use `error(...)` for the single-use guard (still throws IllegalStateException). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: move flusherStartCount assertion after flush to remove race (#50) The assertion that only one flusher started was read before the flusher had demonstrably entered its body. Move it after the flushed.await() so the count is deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: trim over-explained comments in the persistence layer (#50) Cut comments that restated the code or narrated review history; kept the non-obvious why-notes (LAZY start, atomic select, map-inside-try, DROP_OLDEST, UUID-not-Random, the stop() seam WARNING). Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: apply reviewers clarity/minimalism cut-list (#50) Comment-only edits in PersistenceController, EventDto, Persistence, and PersistenceControllerTest per Vigil + Sage cut-list. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: BCV-guard :sharingan-db's published ABI (#50) :sharingan-db ships to Maven Central, so its public surface must be guarded by apiCheck like any other published module. Remove the ignoredProjects exemption, commit the generated api/*.api dumps, and verify apiCheck passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: describe shipped persistence (flight recorder) accurately (#50) Three statements predate the flight recorder: ARCHITECTURE's overview claimed nothing is persisted, section 5.4 described persistence as hypothetical/opt-in, and CONTEXT.md's Store entry claimed memory-only. All three now describe what ships: an in-memory ring buffer (300) that is also mirrored to a SQLite flight recorder via the write-behind seam (SharinganStore.onRecord -> PersistenceController), on by default in debug, with request/response bodies never written to disk. CONTEXT.md gains Flight Recorder, Write-Behind Seam, and Run entries, and the Capture entry notes redaction applies before disk too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: fix remaining memory-only claims across README, AGENTS, llms.txt (#50) Five more places still claimed "memory-only / never persisted": the README feature bullet and SharinganStore note, ARCHITECTURE's design properties and known-limitations entries, and AGENTS.md lines 3 and 106. All now say what ships: the ring buffer stays memory-only and is still cleared on process death; events are additionally mirrored to the on-disk SQLite flight recorder (debug only), which survives, and bodies are never written to disk. AGENTS.md and llms.txt are a hand-maintained mirror — regenerated llms.txt from AGENTS.md and confirmed byte-identical before committing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: keep CONTEXT glossary formatting uniform (#50) The Capture entry gained a stray blank line before its _Avoid_ line; every other entry has them adjacent. Backtick `session` in the Run entry so it reads as the schema table name rather than the term reserved for the v2 epic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBhi6Qx8rdo1aX9EnmtEJS * fix: clear() drops persisted rows and pending batch; bodies stay off disk (#50) B1: SharinganStore.clear() now invokes an onClear seam; Persistence wires it to a new PersistenceController.clear(). The clear travels as a command on the same channel the writes use, so the single flusher coroutine deletes rows at an exact point in the write order — a pending in-flight batch cannot resurrect events after a clear. B2: toRow() strips HttpDto request/response bodies and Mqtt/Ble payloads on the persistence path only (design default persistBodies = false); EventDto stays capable of carrying bodies for the slice-5 opt-in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: give the write-behind seam an owner (#50) The seam had no lifecycle. Persistence.start() wired store.onRecord and forgot both the controller and the store, so nothing could unwire it and 'started' latched true for the process lifetime. - Wire the seams BEFORE starting the flusher. start() is LAZY so the old order was harmless, but the dependency is now encoded rather than implied. - Add Persistence.stop(): unwires onRecord/onClear, closes the controller, and resets 'started' so a later start() works. Retains the store it wired. - @volatile on both seam properties (kotlin.concurrent.Volatile, so it applies on Native as well as the JVM). They are written at process start and read on the capture path. Left deliberately: the dropped-batch report stays a println, since the project has no logging facility and a debug-only recorder does not justify inventing one. Both simplifications carry ponytail: comments naming the ceiling. Not unit-tested, and the docblock says why: start() builds a real driver, and DriverFactory.create() on Android needs the ContentProvider-installed Context that a JVM unit test cannot supply. The alternative was Robolectric or a controller-injection seam, neither justified by straight-line wiring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBhi6Qx8rdo1aX9EnmtEJS * chore: refresh :sharingan-db API dump for clear() (#50) The BCV gate added in 87f9e71 caught the ABI widening from the clear() work: PersistenceController.clear() (called cross-module by :sharingan) and the SQLDelight-generated deleteAllEvents(). Both are intended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBhi6Qx8rdo1aX9EnmtEJS * style: conform to the ktlint comments policy from develop (#50) develop gained a ktlint ruleset (#58) after this branch forked: standard formatting rules plus a custom no-multi-line-comment rule. Apply ktlintFormat and condense every multi-line // run this branch introduced into a single line, per the AGENTS.md comments policy. KDoc is exempt, so the Persistence lifecycle docblock is unchanged. Also applies the ktlint plugin to :sharingan-db. The module was created on this branch before the ruleset existed, so it was escaping the lint gate the same way it was escaping the BCV gate — adding it surfaced eight violations that were previously invisible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBhi6Qx8rdo1aX9EnmtEJS --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Adds ktlint (Gradle plugin
org.jlleitschuh.gradle.ktlint14.2.0) and applies a one-shot mechanical format pass across the codebase. CI enforcement is intentionally not in this PR — it lands in a follow-up so this PR's diff stays purely mechanical.What's here
:sharingan,:sharingan-noop,:sample:composeApp.ktlintCheckruns as part ofcheck..editorconfig:ktlint_officialstyle, trailing commas on (matches the Compose convention), max line 140.@Composable/@ObjCNamefunctions are exempt fromfunction-naming— the capitalizedSharinganViewController()factory is public API and must keep its name.PreviewData.kt; no renames, no logic changes.Verification
./gradlew ktlintCheck— green./gradlew check— green (unit tests +apiCheck+checkApiParityall pass; public API surface unchanged)Follow-up
Enforcement in CI: a
lintjob (ubuntu,./gradlew ktlintCheck) added first inbuild.ymlwithjvm/ios/androidgated behindneeds: lint.