Conversation
Scaffold for the architecture-improvement loop. Baselines package LOC, top files by size, and the initial four-axis scorecard (volatility-violations, framework-extractability, externalization candidates, engine/infra ratio). Per-cycle results appended to the optimization log.
…wrapGit
Server had 7 noImportCycles violations all centered on `surface.ts`:
- `surface.ts ↔ session.ts` (session needs to publish, surface needs `getSavedSession`)
- `surface.ts ↔ activity.ts` (same pattern)
- `surface.ts ↔ terminalBackend/{index,local,metadata}.ts` (same)
Root cause (Lowy lens): `surface.ts` complected two responsibilities:
1. *Declaration* — wire stores, readers, sources into `implementSurface`,
which required importing every domain module.
2. *Publishing* — own the runtime `surfaceCtx` mutation map, which every
domain module imported to push changes back.
Both arrows crossed the same boundary. Tests survived only because the
cycle accidentally bootstrapped the ctx whenever a domain module loaded.
Fix:
- `surfaceCtx.ts` — late-bound holder exporting `surfaceCtx` (a typed
Proxy) and `setSurfaceCtx(...)`. Domain modules import from here.
- `surface.ts` — calls `setSurfaceCtx(built)` at module init. No
longer exports `surfaceCtx`. Still imports domain modules — the
one direction kept.
- `unwrapGit.ts` — pure `GitResult<T> -> T | throws ORPCError`. Moved
out of `surface.ts` so `terminalBackend/local.ts` (and `router.ts`)
can call it without dragging the surface back into a cycle.
Test bootstrap: `metadata.test.ts` installs a no-op ctx via the new
`installNoopSurfaceCtxForTesting` helper (the test doesn't care about
the surface publish side-effect). `session.test.ts` does a side-effect
`import "./surface.ts"` since it genuinely verifies the round-trip
through the session cell's store adapter.
Metrics:
- `noImportCycles`: 7 → 0
- biome warnings overall: 60 → 51
- server unit tests: 46/46 pass
- full repo unit suite: green
…i volatility Per /lowy audit: the proposed `@kolu/solid-xterm` framework extraction *fails* the reuse test (one consumer; framework API would be shaped around implementation, not stable contract). But the 110 LOC of mobile-touch handling in `Terminal.tsx` is a real encapsulation opportunity — it's the receptacle for an independently-volatile axis (iOS Safari focus shuffling, xterm 6.x touch-viewport gap, soft-keyboard summon heuristics). Two helpers, separate concerns, same axis: - `setupMobileTapToFocus(term)` — coerces `.xterm-screen` into contenteditable + bridges tap → term.focus() via a movement- thresholded pointerup. The dance exists because iOS Safari rejects soft-keyboard summoning when focus shuffles mid-gesture. - `setupMobileTouchScroll(container, getTerm)` — bridges container touchmove → `terminal.scrollLines(...)` since xterm 6.0.0 ships typed `IViewport.handleTouchStart/Move` but no implementation, and the WebGL canvas eats touch events before they reach `.xterm-viewport`. Both register listeners via `makeEventListener`, so they must run inside a SolidJS owner — Terminal.tsx already calls them from inside its `runWithOwner`-wrapped onMount tail. Behavior preserving. Terminal.tsx 930→821 LOC. Client unit tests: 172/172 pass.
…ycle
Establishes the `@kolu/solid-xterm` workspace package and moves the
xterm WebGL lifecycle out of `client/src/terminal/Terminal.tsx` as its
first export. Single-consumer inside Kolu today; the extraction is
justified by per-axis volatility encapsulation — the same bar Surface
and `@kolu/solid-pierre` cleared, not by reuse-count.
`createXtermWebgl(getTerm, hooks) → XtermWebglHandle` owns:
- `WebglAddon` construction, attachment, and disposal.
- The `.xterm-screen canvas:not(.xterm-link-layer)` selector that
avoids xterm's link-layer 2D canvas (otherwise `getContext("webgl2")`
returns null and `WEBGL_lose_context.loseContext()` silently no-ops
— diagnosed via #591/#595).
- Explicit `loseContext()` before `addon.dispose()` so Chrome's ~16
per-tab GPU context budget stays at 1 (without this, rapid focus
changes overflow and Chrome evicts live contexts including the
focused tile's — #575).
- Re-entry guard: null `webgl` *before* `loseContext()` so the
synchronous `webglcontextlost` re-entry via `onContextLoss(unload)`
short-circuits.
- A reactive `has()` accessor (own Solid signal).
Three observation hooks (`onCreate`/`onLoseContextCalled`/`onDispose`)
let the host plug a debug ledger in without coupling the framework to
it — Kolu wires `webglTracker.ts` (a temporary #591 zombie-context
ledger) through them.
Terminal.tsx: 821 → 779 LOC. Client unit tests: 172/172 pass. Package
includes README documenting the two encapsulated failure modes for
future xterm/Chrome upgrades.
…Size sync
Second export of `@kolu/solid-xterm`. Two `createEffect(on(...,
{ defer: true }))` blocks plus their atlas-clear / refit follow-ups
now sit behind one helper:
```
attachXtermStyleSync(() => terminal, {
theme: () => props.theme,
fontSize,
onThemeChange: clearTextureAtlas,
onFontSizeChange: () => { clearTextureAtlas(); debouncedFit(); },
});
```
Encapsulated axis: live-writing `term.options.theme` / `.fontSize`
plus the `defer: true` discipline (initial values come from the XTerm
constructor; only subsequent reactive changes flow through). The two
post-change hooks are separate because the theme axis has no fit
implication — collapsing them to one `afterChange` would force every
consumer to refit on every theme swap.
Terminal.tsx: 779 → 770 LOC. Client unit tests: 172/172 pass.
`client/src/scrollLock.ts` was already at the right altitude — 120 LOC of `createScrollLock(enabled)` with zero Kolu coupling, depending only on `@xterm/xterm` and `solid-js`. It belongs in `@kolu/solid-xterm`. Move bit-for-bit; drop the client-side copy. Terminal.tsx now imports `createScrollLock` from `@kolu/solid-xterm` alongside the WebGL and style-sync helpers added in cycles 3 and 4. Framework axis encapsulated: scrollback freeze-write semantics + xterm's `buffer.active.baseY` vs `viewportY` "at bottom" math. Net file count: −1 in client, +1 in solid-xterm. Framework API surface: 2 → 3 exports. 172/172 client unit tests pass.
Catches `README.md`'s Architecture table up to the three exports shipped by `@kolu/solid-xterm` (cycles 3–5) and rewrites the package README to document all three (`createXtermWebgl`, `attachXtermStyleSync`, `createScrollLock`) with usage examples and the SolidJS-owner constraint for `attachToTerminal`.
/lowy audit of `packages/client/src/canvas/` flagged one concrete violation: tile-packing modules (`repoIslands.ts`, `tilePlacement.ts`) import `GRID_SIZE` and `snapToGrid` from `viewport/transforms.ts`. Those values are part of the viewport's internal coordinate system; the packing algorithm should not depend on viewport internals. Today it works because tile-space and viewport-space share the same grid. If they ever needed to diverge, this would be a silent conflict cut through `repoIslands.ts` as a surprise. More immediately, it blocks the extraction `/lowy` identified — `repoIslands.ts` + `tilePlacement.ts` are the first cleanly framework-shaped candidate in the canvas cluster (a future `@kolu/canvas-layout` for "2D canvas with grouped tiles"). The viewport import is the only thing keeping them from being domain-free. Fix: `canvas/canvasGeometry.ts` defines `GRID_SIZE` + `snapToGrid`. `viewport/transforms.ts` re-exports them so the gesture/animatedPan viewport-internal callers keep their local-feeling import. Packing modules now import from the neutral location. No behavior change. 172/172 client unit tests pass.
…dated by /lowy cycle 6) Executes the framework extraction the cycle-6 /lowy audit identified. Moves three pure modules out of `client/src/canvas/`: - `canvasGeometry.ts` (`GRID_SIZE`, `snapToGrid`) - `tilePlacement.ts` (`findFreeTilePosition`, `DEFAULT_TILE_W/H`) - `repoIslands.ts` (`arrangeRepoIslands`, `repackBucket`) The only Kolu-domain reference (`TerminalId` as a `Map` key) becomes the underlying `string`; `TileLayout` becomes the framework-neutral `Rect`. No behaviour change — `TileLayout` in client/canvas is a structural alias of `Rect`. `@kolu/canvas-layout` ships its own vitest target with the 11 `repoIslands` tests. The viewport's `transforms.ts` re-exports `GRID_SIZE`/`snapToGrid` from the new `@kolu/canvas-layout/geometry` entry point so viewport-internal callers (gestures, animatedPan, useCanvasViewport) keep their local-feeling import. 172/172 client unit tests + 11/11 canvas-layout unit tests pass. README documents the encapsulated "2D packing algorithm for grouped tiles" volatility axis (square-ish vs row-major vs golden-ratio cluster shape, gap policy, per-bucket priority).
Executes the second framework extraction the cycle-6 /lowy audit
identified. Moves `packages/client/src/canvas/viewport/` (6 files,
585 LOC) bit-for-bit into a new `@kolu/solid-canvas-viewport`
workspace package. The viewport's only Kolu-domain coupling
(`TileLayout`) is replaced with `Rect` imported from
`@kolu/canvas-layout` — a structural alias of the same shape, no
behaviour change.
User reframe ("what about infinite canvas with minimap itself")
confirms the pattern: the pan/zoom viewport plus a minimap is the
shape any spatial UI repeats. The viewport's volatility has already
manifested as repeated rework (per-tile transform vs wrapper
transform, #988); naming it as a package contains the next round.
Framework decomposition follows /lowy's three internal axes:
- Gesture input (`gestures.ts`, `capturePointerGesture.ts`)
- Transform math (`transforms.ts`, `coordinates.ts`, `animatedPan.ts`)
- CSS generation (`coordinates.ts`'s `tileTransformCSS`)
`useCanvasViewport()` is the orchestrator returning a stable
`CanvasViewport` interface — the three internal modules are
implementation details. Four client files (`CanvasTile`,
`CanvasMinimap`, `TerminalCanvas`, `minimapGestures`) now consume
the framework through `@kolu/solid-canvas-viewport`.
The minimap itself (Kolu-specific UI: dock buckets, agent state
overlay, terminal grouping) stays in `client/src/canvas/`.
Net: client/canvas/viewport subdir deleted entirely. 161/161 client
unit tests + 11/11 canvas-layout unit tests pass. README documents
the three-axis decomposition.
…odules
/lowy audit of `recorder/` flagged three concrete coupling blockers
between Kolu-specific UX and would-be-framework activity modules.
Two of three cleared this cycle (the third — toast-in-orchestrator
→ onError callbacks — is deferred to the actual extraction in
cycle 10, where solid-sonner becomes a peer dep instead).
1. `webcam.ts`: `toast.error()` inside `toggleWebcam` and
`changeWebcam` moved to `throw`. The activity layer doesn't
decide how the user sees failures — that's orchestration. The
existing catch blocks in `useRecorder.ts` already handle
presentation.
2. `useRecorder.ts`: `startRecording()` gains a required
`{ suggestedName: string }` parameter. The hardcoded
`kolu-${timestamp()}.webm` was application identity wired into
a generic state machine — moved to the caller
(`RecordPopover.tsx`). `StartRecordingOptions` interface
documents the FSA mapping.
No behaviour change. 161/161 client unit tests pass. Sets up the
`@kolu/solid-recorder` extraction in cycle 10.
…k this loop
Executes the recorder extraction validated by /lowy cycle 9. Five
files move out of `client/src/recorder/` into a new
`@kolu/solid-recorder` workspace package:
- `mic.ts`, `webcam.ts` — singleton media-device state.
- `useRecorder.ts` — orchestrator hook.
- `LevelMeter.tsx`, `WebcamOverlay.tsx` — visual pieces.
The third /lowy coupling fix — nine `toast.*` calls in
`useRecorder.ts` — resolved by injectable notifications instead of
deletion: `RecorderNotifications { onError, onSuccess, onWarning }`
abstracts the surface; defaults route to `console.*` so the
framework works without a toast library; `App.tsx` calls
`configureRecorderNotifications({ onError: m => toast.error(m), ... })`
once at module load to wire `solid-sonner` through it.
`RecordButton.tsx` and `RecordPopover.tsx` stay in `client/` (deep
Kolu UI coupling: ACTIONS keymap, `surface()` CSS helper, `Toggle`,
`useAnchoredPopover`, `Tip`). They now consume the framework.
Mid-loop checkpoint:
- 4 new @kolu/* framework packages this loop
- client LOC: 25 049 → ~22 990 (−2 060)
- Terminal.tsx: 930 → 770 (−160)
- server import cycles: 7 → 0
- biome warnings: 60 → 51
161/161 client unit tests + 11/11 canvas-layout unit tests pass.
README documents the encapsulated axis and the injectable
notification surface.
…framework this loop First framework extracted with a true *in-tree reuse* justification this loop: `useAnchoredPopover` has six consumers (option menu, settings popover, record popover, mode-chip picker, activity-window chip, PR-unavailable tooltip) — the highest-reuse SolidJS primitive in Kolu. The file moves bit-for-bit (it was already at the right altitude: 135 LOC, depends only on `solid-js` and `@solid-primitives/event-listener`, zero Kolu coupling). All six client consumers swap their relative import for `@kolu/solid-anchored-popover`. README addresses "why not Corvu / Floating UI": this hook stays close to imperative `getBoundingClientRect` + position math so consumers stay in control of the DOM and the portal target. Corvu's Popover is component-shaped (`<Popover.Trigger />` / `<Popover.Content />`); this is a hook with `panelRef` + `panelStyle()`. Context: /lowy on `server/terminalBackend/local.ts` (the biggest single server file) found no actions warranted — the meta/* consolidation is correct, the `startAgentProvider` harness belongs in the server not in `anyagent`, and the per-provider functions benefit from collocation via `record.currentAgent` producer-consumer sharing. Pivoted to this UI primitive instead. 161/161 client unit tests pass.
Aligns the workspace package name with the @kolu/* scoped naming convention used by the other externalization-ready packages (@kolu/surface, @kolu/solid-pierre, @kolu/artifact-sdk, and the five frameworks added this loop). The package was already at the right altitude — pure data + a perceptual-distance picker, zero Kolu app coupling — but its unscoped name was a remnant from before the convention crystallized. Renaming signals external publishability without changing what the package does. Touched: package.json (`name`), README (header + example), pnpm-lock, 7 client import sites (including the `./color` subpath in screenshotTerminal.ts). No behaviour change. 161/161 client unit tests pass.
…this loop
Cross-cutting infrastructure for non-secure-context clipboard
writes. `navigator.clipboard === undefined` on any host that isn't
`https://…`, `localhost`, or `127.0.0.1`; the `.writeText` call
throws `TypeError` with no permission prompt to recover with. This
hits anyone running Kolu over a LAN address, machine hostname, or
Tailscale IP — exactly the dogfood path.
Fallback: `document.execCommand("copy")` against a synthetic
off-screen `<textarea>`. Formally deprecated but at
caniuse 100/100 with no removal timeline — the Clipboard API has
no equivalent fallback for non-secure contexts.
Two entry points so consumers without xterm don't pay the
`@xterm/addon-clipboard` peer-dep cost:
- `./` — `writeTextToClipboard(text)`.
- `./xterm` — `SafeClipboardProvider` implementing xterm's
`IClipboardProvider` (OSC 52 writes survive plain HTTP; reads
return empty when `navigator.clipboard.readText` is unavailable).
8 client consumers swap from `ui/clipboard` to
`@kolu/browser-clipboard` (Terminal.tsx imports both entry points).
161/161 client unit tests pass.
Pure source-reference parser in `path:line[-end]` shape — the wire format VS Code's `:e file:N`, Vim, GitHub URL fragments, Linear snippets, and any code-message tool all share. Zero dependencies; 209 LOC with its own vitest target moved into the package. Five client consumers swap from `ui/lineRef` to `@kolu/file-line-ref`: - `terminal/fileRefLinkProvider.ts` (xterm link provider) - `right-panel/CodeMenuFrame.tsx`, `CodeTab.tsx` (Code tab) - `right-panel/openInCodeTab.ts` (jump-to-file action) - `ui/useLineSelection.ts` (line-selection adapter) Encapsulated axis: the wire format itself. Future column refs (`path:L:C`) or workspace-prefix variants (`@workspace/path:L`) land in one place instead of every consumer's regex. Full repo unit suite green (server 46 + client 161 + canvas-layout 11 + file-line-ref tests).
Finishes the naming-convention sweep started in cycle 12. Three packages still wore unscoped names from before the @kolu/* convention crystallized: - `anyagent` → `@kolu/anyagent` - `nonempty` → `@kolu/nonempty` - `memorable-names` → `@kolu/memorable-names` After this cycle the repo has exactly one naming convention: - `@kolu/*` — workspace packages intended for external publishability (surface, solid-pierre, artifact-sdk, the six framework packages added this loop, terminal-themes, plus the three renamed here). - `kolu-*` — monorepo-internal apps and infrastructure (`kolu-client`, `kolu-server`, `kolu-common`, `kolu-shared`, `kolu-pty`, `kolu-claude-code`, `kolu-opencode`, `kolu-codex`, `kolu-git`, `kolu-github`, `kolu-io`, `kolu-transcript-core`, `kolu-transcript-html`). Touched 27 import sites across client, server, common, integrations, plus the three package.json `name` fields and the six consuming package.json `dependencies`/`devDependencies` blocks. Full repo unit suite green: 730 tests across 16 packages.
…motion) The package's own module docstring already declared "Standalone integration package with no `kolu-*` dependencies" — it was designed as a zero-Kolu-deps leaf from the start. Only the naming hadn't caught up with the intent. 164 LOC of generic refcounted `fs.watch` watcher (parent-dir target + filename dispatch for editor temp+rename atomicity, refcount singleton per resolved dir, idempotent unsubscribe with debounce- timer teardown). Externalizable as-is. Touched: package.json `name`, `kolu-git` dep, 4 source imports, 1 test-file `@`-pointer in a comment, and the module-level docstring that now describes the package by its public name. After this, every workspace package that lives at the right altitude for external publication wears `@kolu/*`. Remaining `kolu-*` packages are exclusively Kolu app/server/integration internals (`kolu-client`, `kolu-server`, `kolu-common`, `kolu-shared`, `kolu-pty`, `kolu-claude-code`, `kolu-opencode`, `kolu-codex`, `kolu-git`, `kolu-github`, `kolu-transcript-core`, `kolu-transcript-html`). 67/67 git unit tests pass.
Applied biome's fixable lints (noUnusedImports, useImportType,
useOptionalChain) across 100 files. Two unsafe rewrites narrowed
return types and needed manual repair:
- `packages/integrations/codex/src/transcript.ts:181` — biome
collapsed `l !== undefined && l.startsWith("***")` to
`l?.startsWith("***")`, but the function signature requires
`boolean`, not `boolean | undefined`. Added `?? false`.
- `packages/client/src/canvas/dock/WorkspaceGrid.tsx:101` — biome
collapsed `col[0]!.id` to `col[0]?.id`, but the function
signature requires `TerminalId | null`, not
`TerminalId | undefined`. Added `?? null`.
Result: biome warnings 51 → 40. Remaining warnings are
non-fixable: `noExplicitAny` in `@kolu/surface`'s type-level
generic machinery (genuinely needs `any` in the spec helpers) and
one comment-justified `noNonNullAssertion` in
`terminalBackend/local.ts:481`.
Full repo unit suite green (730 tests across 16 packages).
Two rows in the Architecture table still described packages by their workspace directory name rather than their (now scoped) package name. Updated: - `packages/integrations/anyagent/` → notes the published `@kolu/anyagent` name. - `packages/integrations/io/` → notes the published `@kolu/dir-watch` name and expands the description to capture the parent-dir/temp+rename/refcount-singleton semantics that weren't in the previous one-liner. Also logged cycles 17-19 in `docs/architecture-ralph-report.md`.
…mulative measurements Closes out the loop with the deliverables a Surface-blog-style write-up needs: **Dead ends** — 6 investigations where /lowy or direct analysis concluded "no action warranted" (integration cluster consolidation, `startAgentProvider` move to `@kolu/anyagent`, per-provider function split-back, `useTextSelection`/`useLineSelection` extractions, plus the cycle-2 first-pass /lowy verdict on `@kolu/solid-xterm` that user feedback reversed). Documented because "X doesn't help" is itself valuable knowledge for the next loop. **Key findings** — five lessons: 1. The Surface bar generalizes — 7 new `@kolu/*` frameworks extracted under the same single-in-tree-consumer + volatility- axis rule. 2. Late-bound holders break load-order cycles (cycle 1's `surfaceCtx.ts` Proxy pattern eliminated all 7 server `noImportCycles` hits). 3. /lowy's "no action" reads are load-bearing — three substantive audits said "decomposition is correct" and meant it. 4. Extraction prep is often the actual work — `@kolu/solid-recorder` needed three coupling fixes before the move was mechanical. 5. Naming convention is grep-able architecture — `@kolu/*` vs `kolu-*` is now information about publishability. **Final measurements** — cumulative deltas vs baseline cycle 0: client LOC 25 049 → ~22 600 (−2 449), Terminal.tsx 930 → 770 (−160), server import cycles 7 → 0, biome warnings 60 → 40, `@kolu/*` packages 4 → 12 (+8). 730 unit tests pass. `just smoke` passes (full Nix build + server boot + /api/health). Limitations honest: e2e suite not re-run in-loop (smoke + unit are the local validation); CommandPalette / useTextSelection / useLineSelection candidates deferred for scope; `kolu-*` internal packages stay internal by design.
Adds a forward reference to the upcoming srid/agency PR that calibrates `/lowy`'s default reasoning around single-in-tree-consumer extraction. Cites Kolu PR #998 — a 20-cycle ralph loop on the architecture — as the in-the-wild evidence that surfaced the gap: the loop graduated seven `@kolu/*` framework packages from one-consumer code under the Surface precedent, with the default `/lowy` heuristic initially disqualifying each of them on the "reuse test". Two additions: 1. New bullet in *Further reading → The reviewers* — links to the in-flight PR + the Kolu evidence + summarizes the SKILL.md clause being added on both lowy and hickey sides. 2. Closing paragraph in *How to run them* — applies the same advice the post gives about reviewers disagreeing ("don't split the difference") to the reviewers' own defaults: push back when their reasoning doesn't hold. Reinforces the post's existing "snapshot mid-process" framing.
… in flight" This reverts commit ea88140.
# Conflicts: # packages/client/src/canvas/dock/Dock.tsx # packages/client/src/canvas/dock/MobileDockDrawer.tsx # packages/client/src/canvas/dock/RowPips.tsx
…2.0)
Addresses the package-coherence critique from
talk-agency-lowy-pr.html: `@kolu/solid-xterm@0.1` shipped three
parallel public exports — `createXtermWebgl`, `attachXtermStyleSync`,
`createScrollLock` — that the consumer had to import separately and
wire together with `new XTerm(...)`, addon constructors, term.open,
WebGL load policy, and scroll-lock attach. That surface read as
"three xterm-adjacent helpers," not "a SolidJS adapter for xterm."
Partial wiring, not a socket.
v0.2 ships one primitive — `createSolidXterm(opts) → SolidXtermHandle`
— that takes the place of `new XTerm(...)`. The handle owns
construction, the standard addon set (with per-addon opt-out), the
WebGL lifecycle policy (reactive `enabled` accessor flips load/
unload), the reactive theme + fontSize sync, the scroll-lock state
machine, and a RAF-debounced `fit()`. The three former public
helpers move to `./internal/` and are no longer re-exported.
Handle surface (namespaced for coherence):
- `xterm.term()` — reactive accessor for the live XTerm
- `xterm.mount(container)` — opens, attaches addons, wires effects
- `xterm.fit()` — RAF-debounced; safe for ResizeObserver
- `xterm.write(data)` — scroll-lock-aware; replaces term.write
- `xterm.addons.{ fit, search, serialize }` — reactive addon refs
- `xterm.scrollLock.{ locked, hasNewOutput, toBottom, reset }`
- `xterm.webgl.{ enabled, atlas, clearTextureAtlas }`
- `xterm.term()` + onTerm callback for link providers, key handlers,
PTY stream consumers — anything the consumer integrates against
the live term
Terminal.tsx: 770 → 675 LOC (−95). The xterm construction +
addon-attach + WebGL-load + style-sync + scroll-lock-wiring block
(~80 LOC) collapsed into one declarative `createSolidXterm({...})`
call. The Kolu-specific integrations (file-ref link provider,
custom key handler, e2e __xterm bridge, registerTerminalRefs,
registerDiagnostics, PTY stream attach, paste + drag-drop uploads,
mobile touch) all live inside the `onTerm` callback.
@xterm/addon-* deps moved into solid-xterm package.json (they were
already in client's; client still keeps them since it imports
xterm types). Package version bumped to 0.2.0 — the old `./webgl`,
`./style-sync`, `./scroll-lock` subpath exports are gone.
161/161 client unit tests pass. `just smoke` passes (full nix build
+ kolu-bin + /api/health 200 + clean shutdown). Demonstrates the
"electricity is electricity, not partial wiring" calibration from
the in-flight srid/agency PR plan.
This was referenced May 28, 2026
srid
added a commit
to srid/agency
that referenced
this pull request
May 28, 2026
…e (+ hickey cross-ref) (#186) ## Summary The `/lowy` default reasoning treats "1 in-tree consumer" as a fail-on-reuse signal — contradicting both the skill's own §5 hedge (the reuse signal is about interface stability, not import count) and the lived evidence of [`@kolu/surface`](https://kolu.dev/blog/surface-framework/), a published framework with a single in-tree consumer. A 20-cycle `/ralph` loop on [juspay/kolu#998](juspay/kolu#998) exposed this in cycle 2 ("fails Lowy's reuse test" → don't extract `@kolu/solid-xterm`) and reversed it in cycle 6 only after the prompt was explicitly primed with the Surface precedent. **Bug 1** calibrates §6 + the fact-check list to admit single-consumer extractions when the interface is stable under the encapsulated axis. **Bug 2:** once the extraction is admitted, the skill never asks whether the resulting *package* is a coherent library. `@kolu/solid-xterm@0.1` shipped three loose public exports — `createXtermWebgl`, `attachXtermStyleSync`, `createScrollLock` — that the consumer had to integrate by hand alongside `new XTerm(...)` and eight addon constructors. The package's name promised "SolidJS adapter for xterm"; the surface delivered "three xterm-adjacent helpers." New **§6.5 ("Package coherence")** names this failure mode, the Surface-shape test, the consumer-wires-it-together smell, and the corrective action. A parallel paragraph in `hickey/SKILL.md` Layer 3 names the package-surface flavour of Layer 2 fragmentation. Worked example throughout: `@kolu/solid-xterm@0.2.0` ([kolu commit `4af1c647`](juspay/kolu@4af1c647)) reshaped the package behind one `createSolidXterm` primitive, demoting the three former exports to `./internal/`. The consumer (`Terminal.tsx`) lost 95 LOC; `just smoke` still passes. That commit is the demonstration of what a §6.5-calibrated reviewer should produce. ## What changes ### `.apm/skills/lowy/SKILL.md` — three edits 1. **§6 "Reuse Signal"** — appended a positive-case paragraph: *Single in-tree consumer is not disqualifying when the interface is stable under the encapsulated axis.* Cites the Surface precedent + the seven `@kolu/*` packages graduated from the kolu#998 ralph loop. 2. **New §6.5 "Package Coherence"** — four-step check for when the extraction adds a new published-shape package: (a) read the exports list as a new consumer, (b) apply §5's atomic-verb rule at the package level, (c) the Surface test (one entry per coherent concept, internal submodules hidden), (d) the "consumer wires it together" smell. Action: "extract one socket, not three wires." 3. **Fact-check phrase shapes** — added two phrase shapes the calibrated reviewer should flag: - \`*"Fails Lowy's reuse test"*\` (when based on import count alone) — diagnosis is §5's interface-stability check, not the count. - \`*"Each export passes §5 in isolation"*\` (without checking the package surface) — §6.5 fires per package; §5 fires per interface. ### `.apm/skills/hickey/SKILL.md` — one edit **Layer 3, step 4** — *Package surface as a fragmentation site.* Reading a package's exports list the way Layer 2 reads a per-entity structure: does the consumer reconstitute one concept by wiring multiple exports together? Cross-references `/lowy` §6.5 for the volatility-side argument. ## Test plan The calibration is text-only — no executable test suite. Validation is empirical: re-run `/lowy` against extraction-shaped diffs and check the verdict matches the calibrated rule. Concrete checks before merge: 1. **Re-run `/lowy` against `Terminal.tsx` at kolu commit `0893f3d2`** (the post-cycle-2 state, where the original audit said "don't extract `@kolu/solid-xterm`"). Calibrated verdict should now recommend extracting *and* warn that the extracted package must be a coherent primitive, not three loose helpers — citing §6 + §6.5. 2. **Re-run `/lowy` against `@kolu/solid-xterm@0.1` exports list** (`./webgl`, `./style-sync`, `./scroll-lock`). Calibrated verdict should fire §6.5 ("three internal aspects leaked through three exports") and recommend the `createSolidXterm` reshape that kolu commit `4af1c647` actually shipped. 3. **Re-run `/lowy` against `@kolu/solid-xterm@0.2.0`** (one `createSolidXterm` export, internal submodules under `./internal/`). Calibrated verdict should pass §6.5 — the package is one coherent socket. 4. **Audit the seven kolu#998 packages** against the [classification table in the artifact plan](https://github.com/juspay/kolu/blob/ralph/talk-agency-lowy-pr.html). Calibrated `/lowy` should produce the same coherent/borderline/incoherent classification (6 coherent, 1 borderline @ `@kolu/canvas-layout`, 0 incoherent post-fix). 5. **Phrase-shape regression check.** Construct a synthetic `/lowy` output containing the new phrase shapes; fact-check pass should flag both. None of these require code changes — they are `/lowy` invocations against existing artifacts. If any of the four code-shaped tests produces a verdict that contradicts the rule the PR is adding, the SKILL.md text needs another iteration before merge. ## Out of scope for this PR - **Audit + maybe-split `@kolu/canvas-layout`** (the borderline row in the audit). Separate kolu PR. - **A blog post tying this to ["The spacetime of code"](https://kolu.dev/blog/hickey-lowy/)**. Case study is blog-shaped; lands after this PR so the SKILL.md updates are public when it links them. - **A `/ralph` SKILL.md addition** naming the same gap. The loop's "graduate framework packages" metric let cycle 3 ship without a package-coherence audit — a real gap in `/ralph` too, but addressing it expands this PR's scope. Track separately. ## Refs - [juspay/kolu#998](juspay/kolu#998) — the architecture-ralph PR that surfaced both calibrations. - [\`4af1c647\`](juspay/kolu@4af1c647) — the \`@kolu/solid-xterm@0.2.0\` reshape used as the §6.5 worked example. - [\`docs/architecture-ralph-report.md\`](https://github.com/juspay/kolu/blob/ralph/docs/architecture-ralph-report.md) — cycle-by-cycle log of the 20-cycle loop, including the cycle-2 wrong verdict and cycle-6 reversal. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
srid
added a commit
that referenced
this pull request
May 28, 2026
…tical sort
`packages/server/src/surface.ts` and `packages/server/src/terminals.ts`
both end with a top-level `const localBackend = getTerminalBackendFor({
kind: "local" })` that depends on `localTerminalBackend` (defined in
`terminalBackend/local.ts`) being initialized. Because surface.ts
participates in a multi-module import cycle (flagged by biome's own
`noImportCycles`), the order in which these files import
`terminalBackend/index.ts`, `terminalBackend/metadata.ts`,
`terminal-registry.ts`, and `publisher.ts` determines whether the
cycle converges with `localTerminalBackend` already past its TDZ.
Biome's alphabetical sort reorders both files into the shape that
makes the cycle converge in the wrong direction — production boot
fails with:
ReferenceError: Cannot access 'localTerminalBackend' before
initialization
at getTerminalBackendFor (terminalBackend/index.ts:29)
at <anonymous> (surface.ts:70)
Unit tests don't catch this because vite-node's module loader differs
from the production node ESM loader on cycle-evaluation ordering.
Restore the original order and pin it with
`// biome-ignore-start assist/source/organizeImports` so the next
`biome check --write` doesn't break the boot again. The proper fix is
to break the cycle (see `@kolu/surface` ralph-loop work on PR #998 ←
this is the same diagnosis cycle 1 addressed via a `surfaceCtx.ts`
holder); that's out of scope for this chore PR.
srid
added a commit
that referenced
this pull request
May 28, 2026
Standalone re-do of the `biome --write --unsafe` style sweep, lifted out of [#998](#998) (the architecture-ralph branch) where it was interleaved as "cycle 17" and made review of the surrounding architecture work harder. **80 files, +202/−211, zero behavior change** — `noUnusedImports`, `useImportType`, `useOptionalChain` plus two narrowings biome's unsafe pass couldn't get right on its own. Two of biome's `useOptionalChain` rewrites collapsed `x !== undefined && x.foo` into `x?.foo`, which is `boolean | undefined` rather than the declared `boolean` (and the same shape one level over for `TerminalId | null`). The wider context made the fix obvious in both spots — a guard already ruled the `undefined` branch unreachable in one case, and the bare `boolean` return in the other was a deliberate signature — so the manual patch is a `?? false` / `?? null` rather than a signature change. | File:line | Biome's unsafe rewrite | Manual patch | Why | | --- | --- | --- | --- | | `packages/integrations/codex/src/transcript.ts:181` | `l?.startsWith("*** ")` | `?? false` | `isMarker(l): boolean`, not `boolean \| undefined` | | `packages/client/src/canvas/dock/WorkspaceGrid.tsx:101` | `col[0]?.id` | `?? null` | `firstAvailableId(): TerminalId \| null`; the `col.length > 0` guard makes `col[0] === undefined` unreachable, but the type system can't see that | Biome warning count drops **60 → 49** at HEAD. Remaining warnings are non-fixable: `noExplicitAny` in `@kolu/surface`'s type-level generic spec helpers and one comment-justified `noNonNullAssertion` in the surface example. > *The earlier attempt to surgically revert `0a2a0feb` out of #998 failed because ten subsequent commits had further modified the affected files. The clean path was always to redo the sweep at current `master` HEAD — this PR is that.* Closes #1002. _Generated by [`/do`](https://github.com/srid/agency) on Claude Code (model `claude-opus-4-7`)._
5 tasks
# Conflicts: # packages/canvas-layout/src/repoIslands.test.ts # packages/canvas-layout/src/repoIslands.ts # packages/client/src/App.tsx # packages/client/src/canvas/CanvasMinimap.tsx # packages/client/src/canvas/useCanvasArrange.ts # packages/client/src/commands.tsx # packages/client/src/settings/SettingsPopover.tsx # packages/client/src/terminal/PrUnavailablePopover.tsx # packages/client/src/terminal/Terminal.tsx # packages/client/src/terminal/useTerminalCrud.ts # packages/common/src/surface.ts # packages/integrations/git/src/cwd-git-watcher.ts # packages/integrations/git/src/head-watcher.ts # packages/integrations/git/src/index-watcher.ts # packages/integrations/git/src/reflog-watcher.ts # packages/server/src/router.ts # packages/server/src/surface.ts # packages/server/src/terminalBackend/local.ts # packages/server/src/terminals.ts
srid
added a commit
that referenced
this pull request
May 28, 2026
) **Nine `lint/suspicious/noImportCycles` violations in `packages/server/src/` all routed through one bidirectional edge**: `surface.ts` and four domain modules importing each other in a tangle that biome had been merely _warning_ about for months. Extract a `surfaceCtx.ts` Proxy-fronted holder so `surface.ts` registers the typed mutation map once at startup; domain modules import the ctx from the holder instead. The bidirectional arrow collapses to a one-way arrow plus a one-way registration — same shape that PR [#998 cycle 1](#998) prototyped. With zero cycles remaining, promote the rule from biome's default `warn` to `error` in `biome.jsonc`. _The TDZ-on-import-reorder hazard that crashed production in [#1003](#1003) — biome's alphabetical sort reshuffled imports, the cycle converged via a different path, `localTerminalBackend` ended up in TDZ at module load, only `just smoke` caught it on CI — becomes structurally impossible._ ### The shape that changed ``` Before After surface.ts ◀──▶ session.ts surface.ts ──▶ session.ts ──┐ ◀──▶ activity.ts (via local.ts) surface.ts ──▶ activity.ts ─┤ ◀──▶ terminalBackend/local.ts surface.ts ──▶ local.ts ────┼──▶ surfaceCtx.ts ◀──▶ terminalBackend/metadata.ts surface.ts ──▶ metadata.ts ─┘ ▲ │ │ └────── setSurfaceCtx ─────────┘ (local.ts also pulled surface.ts via unwrapGit; local.ts ──▶ unwrapGit.ts router.ts pulled it for the same.) router.ts ──▶ unwrapGit.ts ``` `unwrapGit` had to come out alongside `surfaceCtx` — `terminalBackend/local.ts` imported _both_ from `surface.ts`. Pulling only the ctx out would leave `local.ts → surface.ts → terminalBackend/index.ts → local.ts` cycling via `unwrapGit`. It's a pure `GitResult → ORPCError` adapter; its own neutral file is the right boundary, and `router.ts` picks up the new import too. ### Refinements during review | Lens | Finding | Resolution | |---|---|---| | **hickey** | `setSurfaceCtx` silently overwrites `held` on a second call — _"write once" contract is convention, not constraint_ | Guard added | | **lowy** _(cross-validate)_ | Strict `held !== undefined` would block harmless same-ctx re-registration (future test isolation / HMR) | Refined predicate to `held !== undefined && held !== ctx` — throws only on a _genuinely different_ ctx | | **code-police** | `session.test.ts` + `metadata.test.ts` previously inherited a populated ctx via `surface.ts`'s import side-effect; with the holder, the Proxy throws when not initialized | Added `__resetSurfaceCtxForTest()` + `noopSurfaceCtxForTest()` and wired them into the affected tests | | **code-police** | Stale doc comment in `router.ts` still pointed at `./surface.ts` for `surfaceCtx` | Updated | ### Validation - `just check` — clean (zero `noImportCycles`, zero type errors, pre-existing 40 warnings unrelated). - `pnpm vitest run` in `packages/server` — **46/46 unit tests pass** (the 14 that needed the new test-helper plumbing now have it). - CI `just smoke` covers the original production-loader hazard end-to-end. > _Reviewer note_: also retires the `biome-ignore-start assist/source/organizeImports` markers in `surface.ts`. With no cycle for the alphabetical sort to converge along, the load-order constraint they protected is gone. Closes #1005. ### Try it locally ```sh nix run github:juspay/kolu/noImportCycles ``` _Generated by [`/do`](https://github.com/srid/agency) on Claude Code (model `claude-opus-4-7`)._
# Conflicts: # packages/client/src/App.tsx # packages/server/src/surface.ts # packages/server/src/surfaceCtx.ts # packages/server/src/terminalBackend/local.ts # packages/server/src/unwrapGit.ts
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
Twenty cycles of
/ralphover the architecture, guided by Juval Lowy's volatility-based decomposition (/lowy) and Rich Hickey's structural-simplicity framing (/hickey). Net result: 7 new@kolu/*framework packages graduated plus 1 convention promotion (kolu-io→@kolu/dir-watch), eliminated all 7 servernoImportCyclesviolations, droppedTerminal.tsxfrom 930 → 770 LOC, removed ~2 450 LOC frompackages/client, and consolidated workspace naming to a grep-able@kolu/* = externalizable/kolu-* = monorepo-internaldistinction.Each extraction passes the Surface bar established by
@kolu/surface: encapsulate a stable volatility axis even at one in-tree consumer. The user's reframe — "single consumer is not a good excuse — surface has a single consumer too. just like electricity even if it used in only one home" — was the unlock that reversed cycle 2's first-pass /lowy verdict and unblocked the entire framework spree.Cycle-by-cycle log, dead ends, key findings, and final measurements:
docs/architecture-ralph-report.md.Frameworks graduated this loop
@kolu/solid-xterm@kolu/canvas-layout@kolu/solid-canvas-viewportCanvasViewportinterface@kolu/solid-recorder@kolu/solid-anchored-popover@kolu/browser-clipboardnavigator.clipboardundefined on plain HTTP LAN / Tailscale IPs)@kolu/file-line-refpath:line[-end]wire format for editor-adjacent tools (VS Code, Vim, GitHub URL fragments, Linear)Plus the cycle-16 promotion
kolu-io→@kolu/dir-watch(164 LOC refcounted sharedfs.watch, parent-dir target for editor temp+rename atomicity).Cumulative measurements vs cycle 0 baseline
packages/clientLOCTerminal.tsxLOCnoImportCycleslint hits@kolu/*workspace packagesArchitecture highlights
Cycle 1 — late-bound holders break server load-order cycles. All 7
noImportCyclesserver violations centered onsurface.ts(which complected the surface declaration with its mutation map). ExtractedsurfaceCtx.tsas a Proxy-fronted holder thatsurface.tspopulates once at startup viasetSurfaceCtx(built). Domain modules import from the holder; only the surface imports the domain modules. The bidirectional arrow split into a one-way arrow + a one-way registration.Cycles 3–5 —
@kolu/solid-xtermas a multi-cycle build-out. WebGL lifecycle (createXtermWebglwith the.xterm-screen canvas:not(.xterm-link-layer)selector trap + explicitWEBGL_lose_context.loseContext()beforeaddon.dispose()) → reactive style sync (attachXtermStyleSyncforterm.options.theme/fontSizewithdefer: truediscipline) → scroll-lock primitive (createScrollLockwithbuffer.active.baseYvsviewportY"at bottom" math).Cycles 6–8 — canvas decomposition into two packages. /lowy audit flagged one violation (GRID_SIZE / snapToGrid leaking from the viewport boundary into
repoIslands.ts+tilePlacement.ts). Cycle 6 fixed the leak via acanvasGeometry.tsneutral location. Cycle 7 extracted@kolu/canvas-layoutfromrepoIslands+tilePlacement+canvasGeometry. Cycle 8 extracted@kolu/solid-canvas-viewport(the wholeclient/src/canvas/viewport/subdir, 585 LOC), withuseCanvasViewport()returning a stableCanvasViewportinterface over three internal axes.Cycles 9–10 —
@kolu/solid-recorderwith notification injection. /lowy flagged 3 Kolu-coupling blockers before extraction:toast.error()insidewebcam.tsactivity layer (cycle 9 →throw), hardcodedkolu-${ts}.webmfilename (cycle 9 → required{ suggestedName }parameter), and 9toast.*calls inuseRecorder.ts(cycle 10 →RecorderNotifications { onError, onSuccess, onWarning }withconfigureRecorderNotifications(...)forApp.tsxto wiresolid-sonneronce at module load). Defaults route toconsole.*so the package is usable without a toast library.Cycles 12, 15, 16 — naming convention sweep. Renamed
terminal-themes,anyagent,nonempty,memorable-names, and promotedkolu-io→@kolu/dir-watch. After this, every package whose shape is externalization-ready wears@kolu/*; Kolu app/server/integration internals keepkolu-*.Validation
just check: typecheck + biome lint green (40 warnings, down from 60; remaining arenoExplicitAnyin@kolu/surface's generic spec machinery and one comment-justifiednoNonNullAssertion).just test-unit: 730 tests pass across 16 packages.just smoke: full pipeline —nix build → kolu-bin → server boot → /api/health 200 → clean shutdown.Tests + smoke run on local Linux per user constraint.
Test plan
vitestpass locally.just smokevalidates the Nix build + production-like server boot.just test) — left to reviewer; smoke + 730 unit tests are the in-loop validation.🤖 Generated with Claude Code
0a2a0feb(the biome chore)This PR will be discarded; the postmortem is here so the next agent doesn't make the same mistakes. A reviewer flagged that commit
0a2a0feb(thebiome --write --unsafe100-file style sweep, framed in the ralph log as "cycle 17") is pure noise inside an architecture PR. They asked me to revert it and file an issue. I filed #1002 but failed to revert it cleanly. Here is what I did, what went wrong, and what the next agent should do.Mistake 1 — the chore was in the branch at all
I treated
biome check . --write --unsafeas "cycle 17" of the architecture loop. It is not architecture work. It does not graduate a framework, encapsulate a volatility axis, or fix a Lowy violation — it reformats imports. Style chores belong in their own PR, offmaster, not interleaved into an architecture branch. Once the chore lands inside the architecture branch, every subsequent commit's diff conflates "the architecture change" with "biome's import-style fixup" and review becomes harder.If you are running a ralph loop and the cycle's intended mutation is "apply biome's fixable lints," stop and open a separate PR. Resume the ralph loop afterwards. Do not append it to the architecture branch.
Mistake 2 —
git revert -X oursis not a partial revertI tried
git revert 0a2a0feb. It produced content conflicts in 12+ files because cycle 17's changes have been further modified by 10 subsequent commits — most notablyTerminal.tsx, which was substantially rewritten in4af1c647(the@kolu/solid-xterm@0.2reshape) and the merge from master that brought inpipVariantchanges to the dock files.I aborted and tried
git revert --no-edit -X ours 0a2a0feb, hoping-X ourswould "skip conflicting hunks and apply the rest." That is not what-X oursdoes. For a revert,-X oursresolves three-way merge conflicts in favour of HEAD — but only at conflicting hunks. Hunks that revert cleanly (no overlap with later changes) are applied as part of the revert whether or not they make sense in the post-cycle-17 world.Concretely: cycle 17 had reformatted
Terminal.tsx's import block, deleting some imports and addingimport typequalifiers. The revert obediently put the deleted imports back. But the post-4af1c647Terminal.tsxno longer needs those imports (the entire xterm-construction block has been replaced bycreateSolidXterm), and several of the now-restored imports referenced symbols that no longer exist as public exports of@kolu/solid-xterm(e.g.attachXtermStyleSync,createScrollLock— both demoted to./internal/in4af1c647). Result: a "clean" revert commit (97 files changed, no merge markers) that breaks the build with duplicate-import errors and references to deleted exports.I reset to
4af1c647to undo this and gave up. The PR description that the reviewer is reading now still contains0a2a0feb.What the next agent should do
The branch should be discarded as the reviewer indicated. To recover the work cleanly:
Drop
0a2a0febfrom history via interactive rebase, not revert. Frommaster-derived state, rungit rebase -i masteron theralphbranch, mark0a2a0febwithdrop, and let git replay every subsequent commit on top of a tree that never had cycle 17. Conflicts WILL arise at the rebase step — that is correct and recoverable: each conflict is the place where a later commit assumed cycle 17's formatting, and the resolution is to apply the later commit's intent without depending on cycle 17. This produces a clean architecture branch with no biome noise mixed in.Or, more honestly, restart the architecture PR from
master. Cherry-pick the substantive commits (cycle 1 server-cycle break, cycle 2 mobileTouch, cycles 3–5 solid-xterm, cycles 6–8 canvas-layout + viewport, cycles 9–10 recorder, cycle 11 anchored-popover, cycles 12/15/16 renames, cycles 13–14 browser-clipboard + file-line-ref, the merge of master, and4af1c647's solid-xterm reshape) onto a fresh branch offmaster. Skip0a2a0feb(the biome chore) andabed7ff7(the README polish that depends on0a2a0feb's reformatting? — check). Re-runjust check+just test-unit+just smokeat each cherry-pick boundary to surface dependencies on the dropped chore early. File the biome cleanup as a separate PR offmasterper #1002.Do not use
git revert -X oursfor partial reverts. It is not a "skip the conflicting parts" flag; it is a conflict-resolution preference. The right tool for a partial revert isgit revert -n <commit>(stage without committing), then manually edit the staged diff to drop hunks you want to keep at HEAD's state, then commit. This is tedious by design — there is no shortcut for "undo this style sweep on the 80 files that haven't been further touched and leave the other 20 alone."What the next agent should NOT do
git revert -X theirs. That would prefer the pre-cycle-17 state at conflicts, which would also un-do legitimate later changes — strictly worse than the-X oursmess.The lesson: the cost of an inappropriate commit in a branch is paid at revert time, not at commit time. The 5 minutes saved by treating biome as "cycle 17" instead of opening a separate PR became, days later, a multi-hour conflict resolution that ended in a build break. Front-load the discipline.
— the agent that screwed this up