# clide — External Consultant Review **Date:** 2026-05-14 **Scope:** Full-repository assessment of clide at `main` (commit `9030e56`). **Method:** Six independent specialist reviewers, each given read-only access and a brief covering best practice, clean code, architecture, usability, stability, expandability, style, consistency, and general quality. Reviewers did not see each other's findings; cross-cutting themes below are genuine independent agreement. **Panel:** | Lens | Reviewer | |---|---| | Architecture | Software Architect | | Tests & quality gates | Test / QA Analyst | | UX & accessibility | UX & Accessibility Expert | | Code quality & craft | Senior Dart/Flutter Engineer | | Security & supply chain | Security Engineer | | Docs, governance & DX | TPM / Developer-Experience Consultant | --- ## Overall verdict clide is, for a solo-dev pre-v2.0 project, **unusually disciplined** — every reviewer said so independently. The governance system is alive, the core subsystems are small and well-typed, the FFI/PTY layer shows real systems-programming care, and the quality gates are genuine rather than ornamental. The codebase is in good shape. The weaknesses cluster into a handful of themes, and several are **load-bearing**: a central guardrail (CLI-first IPC) has no runtime implementation, keyboard operability — the core requirement of a power-user dev tool — is largely unbuilt, an untrusted workspace can achieve code execution, and the two primary onboarding documents describe an architecture that no longer exists. None of these are fatal; all are fixable; most have quick-win first steps. But they should be addressed before a public v2.0. --- ## Cross-cutting themes (independent agreement) These were each flagged by **two or more** reviewers who did not coordinate: 1. **The IPC layer is mid-migration and contradicts itself.** The Architect found no Unix-socket *server* anywhere in `lib/` — D-56's "app hosts an in-process IPC server" and D-1's "CLI-first, not MCP" have no runtime path; three IPC clients (`DaemonClient`, `InProcessClient`, `IsolateClient`) coexist with two duplicated service-wiring sites. The Security reviewer independently noted `DaemonClient`'s socket code is still live as an unvalidated attack surface. **Pick one IPC model, implement or amend D-56, delete the other two.** 2. **The "no pre-existing excuse / clean board" guardrail is being violated right now.** `flutter analyze` reports 9 `unnecessary_import` issues in `test/`; `ci/test.sh` runs analyze with `--no-fatal-infos`, which silently tolerates them. Flagged by the Architect, Code Quality, and Test reviewers. The repo's own rules say fix-first. 3. **`lib/src/terminal/` is in an undeclared middle state.** ~7k LOC forked from xterm.dart, carrying commented-out `print`s, dangling TODOs, a 1137-line `parser.dart`, and the only `invalid_use_of_protected_member` suppression in the repo. MEMORY says "code under `lib/` is owned, not vendored" — so it must either be formally vendored (frozen, documented, decision-recorded) or cleaned to the project bar. Flagged by Code Quality; the Architect's "consistency" deduction points at the same seam. 4. **Documentation describes a dissolved architecture.** `README.md` and `docs/initial-plan.md` still describe a Go sidecar, `ptyc/` C helper, `app/` subdirectory, and a separate `clide --daemon` process — all removed by D-5, D-56, and the FFI pivot. A new contributor's first read builds a wrong mental model. --- ## Consolidated scorecard Scores are each reviewer's, 1–5, on their own dimensions. | Domain | Dimension | Score | |---|---|---| | **Architecture** | Layering & dependency direction | 4 | | | Separation of concerns | 4 | | | Expandability | 5 | | | Consistency | 4 | | | Guardrail adherence | 3 | | **Tests** | Coverage quality | 4 | | | Test reliability / flakiness | 3 | | | Gate trustworthiness | 3 | | | Test maintainability | 5 | | | Regression-catching power | 4 | | **UX / a11y** | Interaction model | 2 | | | Accessibility | 3 | | | Visual consistency | 4 | | | Discoverability | 2 | | | State coverage (loading/error/empty) | 3 | | **Code quality** | Idiomatic Dart | 4 | | | Error handling | 4 | | | Naming & readability | 4 | | | Consistency across subsystems | 3 | | | Resource / lifecycle safety | 4 | | **Security** | Subprocess safety | 2 | | | IPC input validation | 3 | | | Path / filesystem safety | 3 | | | Dependency / supply-chain hygiene | 3 | | | Secrets & sandboxing | 3 | | **Docs / governance** | Governance discipline | 4 | | | Documentation accuracy | 2 | | | Changelog hygiene | 3 | | | Contributor onboarding | 2 | | | Convention adherence | 4 | **Highest marks:** expandability (5), test maintainability (5). The extension contract and test-helper design are genuine standouts. **Lowest marks:** interaction model (2), discoverability (2), subprocess safety (2), documentation accuracy (2), contributor onboarding (2). --- ## Prioritized action list Synthesized across all six reviews. Severity is the highest any reviewer assigned. ### Critical — address before public v2.0 1. **Fix untrusted-workspace code execution.** `toolchain_paths.dart:79` resolves `native/dugite/bin/git` relative to the *workspace root*; a malicious repo can plant an executable there that clide runs on the first auto-fired `git.status`. Resolve `native/dugite` against `Platform.resolvedExecutable`'s directory, never the workspace. *(Security)* 2. **Resolve the IPC story.** Either implement the in-process Unix-socket server per D-56 so the `clide` CLI / C client actually works, or amend D-56 to make in-process direct dispatch the design and delete `DaemonClient`'s socket code, `IsolateClient`, `Backend`, and `backend_entry.dart`. Today the code claims three models and runs one, and a load-bearing guardrail (D-1/D-6) is unmet. *(Architecture, Security)* 3. **Make the tool keyboard-operable.** `ClideTappable` (base of nearly every interactive widget) is mouse-only — no `Focus`, no Enter/Space. The command palette has no arrow-key navigation and no Escape. For a keyboard-first dev tool this is a functional gap, not a polish item. *(UX)* 4. **Fix the onboarding docs.** Rewrite `README.md`'s `ptyc/` / `make ptyc-build` sections, fix its dead `decisions/` link, and banner `docs/initial-plan.md` as historical (or split out a current `docs/architecture.md`). *(Docs)* ### Major — should land soon 5. Add symlink re-resolution + containment re-check in `files.read` / `files.ls` — a repo symlink `config -> /etc/shadow` currently passes path-safety. *(Security)* 6. Add `test-integration` (and ideally `smoke-bundle`) to `make push-check` — the gate that catches "app won't boot" is currently omitted from the pre-push gate. *(Tests)* 7. Schema-validate the IPC argument surface; reject `-`-prefixed `branch`/`remote`/`path` values; add size/count bounds. *(Security)* 8. Establish a real focus-traversal model (`FocusTraversalGroup` per slot, a documented "focus next panel" keybinding) and integrate `FocusTracker` with Flutter's focus system instead of paralleling it. *(UX)* 9. Fix the `SchedulerService._startTicker` isolate-spawn race — a `_stopTicker()` before the spawn future resolves leaks a forever-ticking isolate. Mirror `NativePty`'s `_readerReady` pattern. *(Code quality)* 10. Decide the status of `lib/src/terminal/` — formally vendor (and decision-record) it, or do the cleanup sweep. *(Code quality)* 11. Replace fixed wall-clock `Future.delayed` sleeps in `watcher_test.dart` / `session_test.dart` with event-driven waits; make swallowed `onTimeout` callbacks `fail()` loudly. *(Tests)* 12. Write a human-facing `CONTRIBUTING.md`; cut an interim release to drain the ~80-commit `[Unreleased]` backlog; merge duplicate changelog subsection headings. *(Docs)* 13. Single global `KeyboardListener` → scoped `Shortcuts`/`Actions`; move `KeybindingResolver` off layout-dependent `keyLabel`. *(UX)* ### Quick wins — hours each - Clear the 9 `unnecessary_import` analyzer issues; drop `--no-fatal-infos` from `ci/test.sh`. *(Architecture, Tests, Code quality)* - Run the `forkpty` PTY tests with `--coverage` so `native_pty.dart` — the riskiest file — is honestly measured. *(Tests)* - Add a `Focus` + Enter/Space wrapper and a focus-ring inside `ClideTappable`; this fixes the keyboard gap for every button and list item at once. *(UX)* - Add arrow-key + Escape + selected-index to `ClidePalette` (copy the existing `_ProjectSwitcherDropdown` `onKeyEvent` pattern). *(UX)* - Amend D-66 to reflect the coverage floor's real location (`pubspec.yaml`), mechanism, and value (90%) — it currently disagrees with the changelog and the code. *(Docs)* - Reconcile `licenses.yaml` with `pubspec.yaml` (`test` version drift, phantom `lints` entry); add a `native/SHA256SUMS` manifest. *(Security, Docs)* - Replace silent `catch (_)` in `tree_sitter_ffi.dart` with a logged last-error. *(Code quality)* - Fix the `clide.dart` barrel leak in `file_tree_view.dart:8`; narrow the barrel (drop the `dispatcher.dart` export); move `test_app.dart` out of the production `main.dart` import graph. *(Architecture)* - Expand the contrast gate's `canonicalPairs` to cover `globalTextMuted`, the `status*` colors, and `panelActiveBorder`. *(UX)* - Triage stale governance Q-records (Q-1/2/3/25 overtaken by shipped Tier-1 work). *(Docs)* --- # Full reviews ## 1. Architecture — Software Architect ### Executive summary clide is an unusually disciplined solo-dev codebase. The governance system (67 D-records, tracked Q/R) is real and largely honored in code, the kernel/extension split is coherent, and the feature-first layout with barrel files is consistently applied. The single biggest strength is the **extension contract**: every built-in — including layout itself — passes the same `ClideExtension` + `ContributionPoint` contract, which is the best possible proof the contract is usable. The single biggest risk is **architectural drift in the IPC layer**: D-56 mandates the Flutter app host an in-process IPC server reachable by a thin C client over a unix socket, but no socket server exists anywhere in `lib/` — the "CLI-first, not MCP" guardrail (D-1) has no runtime path today. Compounding this, three parallel IPC client implementations (`DaemonClient` socket, `InProcessClient`, `IsolateClient` + `Backend`) coexist with two competing service-wiring sites (`main.dart` and `backend_entry.dart`), suggesting an unfinished migration. ### Strengths - **Extension contract is clean and scales** — `lib/extension/src/extension.dart` + `contribution.dart`: sealed `ContributionPoint` hierarchy, `ClideExtensionContext` lists services explicitly (deliberately avoiding a `KernelServices` import cycle — `extension.dart:50-52`). `ExtensionManager` does dependency topo-sort, dependency-gated activation, and contribution apply/remove symmetrically (`extensions_manager.dart:164-202`). Adding a pane = new extension file + one `register()` line in `main.dart`. - **Kernel admission rule is enforced, not aspirational** — D-12's "mandatory shared singleton" test visibly shaped `KernelServices` (`facade.dart:38-93`); ~25 services, each defensibly cross-cutting. The two-tier disable model (D-14) is honored: `default_layout` is itself an extension. - **Feature-first layout with barrel discipline** (D-8) is consistent — every `builtin//` and `kernel/` has a barrel; builtins import `package:clide/kernel/kernel.dart`, not deep paths. Only one leak found. - **Governance-to-code traceability is genuine** — `WidgetsApp` root (D-7) at `app.dart:38`, `ChangeNotifier`/`ListenableBuilder` state (D-10) everywhere, git hardcoded in toolchain/project loader (D-13), terminal correctly tagged `inlined-source` in `licenses.yaml` with modifications documented. - **Git subsystem cohesion** — `lib/src/git/` cleanly split into `client` / `status` / `diff` / `operations` (~250 lines each), each a single responsibility. ### Findings - **[Critical] No IPC socket server exists** — D-56 specifies the app hosts an in-process IPC server with a C client connecting over a unix socket. `grep` for `ServerSocket`/unix-domain `bind` in `lib/` returns nothing. `DaemonClient._connect` (`client.dart:72-94`) *connects* to a socket, but nothing *serves* one. Today the only working path is `InProcessClient` (`in_process.dart`), which calls the dispatcher directly in-process. **Claude cannot drive clide via `clide ...` — the CLI-first guardrail (D-1, D-6) has no implementation.** This is the load-bearing contract of the whole project and it is absent. - **[Major] Three IPC clients + two wiring sites = unfinished migration** — `DaemonClient` (socket), `InProcessClient`, and `IsolateClient`+`Backend`/ `backend_entry.dart` all coexist. `main.dart:76-113` wires subsystems via `buildDispatcher`; `backend_entry.dart:40-110` wires the *same* five subsystems again inside an isolate. `Backend.spawn` is referenced only by `facade.dart` but `main.dart` uses `autoStartDaemonClient: false` + `daemonClientFactory` (the in-process path). Dead-or-dormant isolate infrastructure with duplicated registration logic — pick one and delete the others. - **[Major] `main.dart` (production entry) imports `test_app.dart`** — `main.dart:2` and `:51-55`. The production binary carries the test harness and branches on `CLIDE_TESTMODE`. Test scaffolding should not be reachable from the shipping entry point; gate it behind a separate entrypoint or `kDebugMode`. - **[Minor] `flutter analyze` reports 9 issues** — all `unnecessary_import` in `test/`, but CLAUDE.md's "no pre-existing excuse" / "clean board" guardrail makes this a fix-first item. - **[Minor] Barrel leak** — `lib/builtin/files/src/file_tree_view.dart:8` imports `package:clide/src/files/listing.dart` directly instead of via `package:clide/clide.dart` (which already re-exports `FileEntry`). - **[Minor] `clide.dart` barrel exports the daemon dispatcher** — `clide.dart:15` exports `src/daemon/dispatcher.dart`. The barrel is described as "shared types"; the dispatcher is server-side machinery. - **[Minor] `ExtensionManager.activate` swallows exceptions** (`extensions_manager.dart: 141-143`) — a failed `activate()` logs and continues, leaving the extension un-activated but `_known`, with no surfaced "degraded" state for the UI. ### Recommendations **Quick wins:** clear the 9 analyzer issues; fix the `file_tree_view.dart` barrel leak (consider a CI grep gate for `package:clide/src/` imports outside their feature); move `test_app.dart` out of the production import graph; drop the `dispatcher.dart` export from `clide.dart`. **Larger efforts:** resolve the IPC story (implement the socket server per D-56, or amend D-56 and delete `DaemonClient`/`IsolateClient`/`Backend`/`backend_entry.dart`); collapse subsystem wiring into one `registerAllSubsystems(...)` function; give `ExtensionManager` a surfaced failure state so the UI can show degraded built-ins. ### Scorecard | Dimension | Score | Justification | |---|---|---| | Layering & dependency direction | 4/5 | Kernel→extension direction clean, context-vs-aggregate split avoids cycles; docked for the `src/`↔`kernel/src/` barrel leak and the dispatcher export. | | Separation of concerns | 4/5 | Feature-first layout, single-responsibility subsystems; duplicated subsystem registration is the blemish. | | Expandability | 5/5 | New pane = one extension file + one `register()` line; sealed contribution hierarchy; layout itself is data and extension-shaped. | | Consistency | 4/5 | Barrels, naming, D-record back-references uniform; three coexisting IPC clients and 9 analyzer issues break the bar. | | Guardrail adherence | 3/5 | `WidgetsApp`, single-process, no-Material, governance, zero-deps all honored — but D-1/D-6/D-56 (CLI-first via socket server) have no runtime implementation. | --- ## 2. Tests & quality gates — Test / QA Analyst ### Executive summary The clide test suite is, for a solo-dev pre-2.0 project, in genuinely good shape. ~104 test files against 276 lib files, ~92.75% line coverage, and — critically — the coverage was *not* bought with assertion-free filler. Even the alarmingly-named files (`coverage_trivials_test.dart`, `zero_coverage_widgets_test.dart`, `services_stubs_test.dart`, `mop_up_test.dart`) contain real behavioral assertions. The biggest strength is a sensibly layered pyramid with a real boot-path integration gate and a startup smoke test that catches the "tests pass but app won't launch" class. The biggest risk is **flakiness from wall-clock-dependent tests** — fixed `Future.delayed` sleeps in file-watcher and PTY tests will eventually produce intermittent CI failures, and the PTY tests are run via `dart test` so they are **excluded from the coverage measurement entirely**. ### Strengths - **Test pyramid is sound.** Pure-Dart unit, widget tests with a shared harness, golden tests (Alchemist), an a11y contract layer, and 3 real-boot `integration_test/` files — correctly separated by runner (`ci/test.sh` vs `ci/test_core.sh` vs `ci/test_integration.sh`). - **Helpers are well-designed.** `test/helpers/kernel_fixture.dart` boots a real `KernelServices` with in-memory themes/i18n and `autoStartDaemonClient: false` — no real socket, temp-dir scoped, proper `dispose()`. `FakeDaemonClient` is a clean stub. - **Error-branch discipline.** `pql_commands_errors_test.dart` / `git_commands_errors_test.dart` deliberately point the toolchain at a non-existent binary to drive catch-branches the happy path can't reach — table-driven, with `reason:` tags. - **OS-dialog avoidance is handled correctly.** `welcome/dialog_test.dart` mocks the `clide/window` MethodChannel to throw `MissingPluginException`, exercising the fallback path *without* spawning a native file picker. - **Startup gate.** `ci/smoke_bundle.sh` builds the real release bundle and runs it under xvfb for 5s, correctly interpreting `timeout` exit codes (124/143 = healthy). - **Coverage gate is honest.** `ci/coverage_gate.sh` is a self-contained awk parser (no `lcov` dependency), ratchets only upward, and `exit 2` distinguishes "missing data" from "below floor." ### Findings - **[Major] PTY tests are excluded from coverage.** `ci/test.sh:13` runs `flutter test --coverage --exclude-tags forkpty`; the `forkpty` tests run separately via `dart test` with no `--coverage`. So `lib/src/pty/native_pty.dart` — the highest-risk native code in the repo — is barely in the measured denominator. The 92.75% number overstates coverage of the riskiest file. - **[Major] Wall-clock sleeps will flake.** `test/files/watcher_test.dart:67-82` uses fixed `Future.delayed`; `test/pty/session_test.dart:71` polls 50×100ms and `:65` uses a bare `500ms` settle. `session_test.dart`'s `timeout(5s, onTimeout: () {})` (`:48`) *swallows* the timeout — a never-producing PTY proceeds to a confusing assertion failure rather than a clear timeout. - **[Major] `make push-check` does not run integration tests.** `push-check: decisions-validate test-core test test-a11y coverage-gate` — `test-integration` and `smoke-bundle` are omitted. A boot-order regression sails through. - **[Minor] `flutter analyze --no-fatal-infos` in `ci/test.sh:9`** contradicts the stated "fail-on-warning, clean board" discipline. - **[Minor] Integration tests run one-file-at-a-time** to dodge a batch "Unable to start the app" error — each invocation re-boots the engine (slow), and the workaround masks whether the batch failure is environmental or a real teardown leak. - **[Minor] Golden CI config disabled.** Only platform goldens run; a Linux-only CI never validates the macOS goldens, and stale `test/goldens/failures/*.png` artifacts are committed to the repo. - **[Minor] `test_core.sh` timeout kill is best-effort** — the `pkill -9 -f` pattern match is redundant noise next to the real `setsid` + `timeout --kill-after` safety net. - **[Minor] `git/client_test.dart` depends on the ambient `git` binary**, not the vendored dugite — the suite passes/fails on the host git version. ### Recommendations **Quick wins:** add `test-integration` (and `smoke-bundle`) to `push-check` — the single highest-value change; run the `forkpty` tests with `--coverage`; drop `--no-fatal-infos`; gitignore `test/goldens/failures/`; make `onTimeout` callbacks `fail()`. **Larger efforts:** replace fixed sleeps with event-driven waits (`expectLater(stream, emits(...))`); add a macOS golden CI matrix entry or document goldens as advisory; consider a coverage-exclusion allowlist for genuinely-unreachable defensive branches rather than chasing the last lines with filler tests. ### Scorecard | Dimension | Score | Justification | |---|---|---| | Coverage quality | 4/5 | Tests are meaningful even in "mop-up" files; docked because PTY/FFI is outside the measured number. | | Test reliability / flakiness | 3/5 | Fixed wall-clock sleeps and a swallowed timeout are latent intermittent failures. | | Gate trustworthiness | 3/5 | Coverage gate and smoke bundle are well-built, but `push-check` omits integration tests. | | Test maintainability | 5/5 | Shared fixtures, consistent structure, table-driven error suites, clear doc comments. | | Regression-catching power | 4/5 | Real boot-path integration + smoke + a11y + goldens; weakened by single-OS goldens and PTY coverage gaps. | --- ## 3. UX & accessibility — UX & Accessibility Expert ### Executive summary clide has an unusually disciplined *foundation* for a solo pre-v2.0 project: a coherent semantic design-token system, a WCAG-AA contrast gate wired into CI, and i18n/semantic contract tests. That foundation is the biggest strength. The biggest risk is that **keyboard operability is largely unimplemented below the foundation** — the project's own core interaction primitive (`ClideTappable`) is mouse-only, the command palette has no arrow-key navigation or Escape, and there is no focus-traversal wiring across panels. For a keyboard-first power-user dev tool, this is a critical gap that the a11y test suite does not catch because the tests assert *structural* presence (Semantics nodes exist) rather than *operability* (can you actually drive it from the keyboard). ### Strengths - **Semantic token system is real and enforced.** `lib/kernel/src/theme/tokens.dart` defines ~65 named surface tokens; widgets consume `ClideTheme.of(context).surface` rather than raw colors. The resolver provides defaults so partial themes still produce a complete `SurfaceTokens`. - **Contrast gate is genuine WCAG math, run per-theme.** `lib/kernel/src/theme/ contrast.dart` implements real relative-luminance ratio with alpha pre-compositing against neutral grey (`contrast.dart:31-37`) — semi-transparent tokens can't spuriously pass. - **Semantics are present on composed widgets.** `ClideButton` wraps `Semantics(button: true, enabled:, label:, hint:, onTap:)`; panels set `container: true, explicitChildNodes: true` with landmark labels. - **State coverage exists in data panels.** `git_panel_view.dart:86-104` handles error, loading, and empty ("working tree clean") states distinctly; `file_tree_view.dart` handles error + loading. - **Manual a11y discipline is documented.** `docs/testing/a11y-manual.md` prescribes a per-tier Orca/VoiceOver pass and is honest about why prose quality can't be automated. - **Disabled state is handled at the cursor level.** `clide_button.dart:41` switches to `SystemMouseCursors.forbidden` and drops the semantic `onTap` when `onPressed == null`. ### Findings - **[Critical] `ClideTappable` is mouse-only — no `Focus`, no keyboard activation.** `lib/widgets/src/clide_tappable.dart:37-54` is `MouseRegion` + `GestureDetector` only. It is the base for `ClideButton`, `_WinBtn`, `_RecentProjectRow`, `_ActionRow`, and most builtin list items. None can receive Tab focus or be activated with Enter/Space. The keyboard-traversal test only passes because it manually wraps the button in an external `Focus` node — it tests that the widget doesn't *block* focus, not that it *accepts* it. - **[Critical] Command palette is not keyboard-navigable.** `clide_palette.dart` — `onSubmitted` only ever invokes `filtered.first` (`:77-80`); no up/down handling, no selected index, no selection highlight, no Escape handler. - **[Major] No focus-traversal wiring between panels.** `FocusTracker` (`lib/kernel/src/focus.dart`) tracks an active *contribution id* for the `clide active` CLI, but is not Flutter `FocusScope`/`FocusTraversalGroup` integration. Nothing establishes Tab order across sidebar → workspace → context. - **[Major] Drag-resize handles have no keyboard equivalent — parity gap.** `drag_resize.dart` and `app.dart:870-912` are pure `Listener` pointer handlers, with no Semantics node at all. Per "User/Claude parity", panel sizing should have a CLI affordance; none is evident. - **[Major] Single global `KeyboardListener` is a fragile keybinding architecture.** `app.dart:90-148` routes all shortcuts through one root `KeyboardListener` — no per-context scoping, will conflict with text-input fields. `KeybindingResolver.fromKeyEvent` keys off layout-dependent `logicalKey.keyLabel`. - **[Major] Text scale is the *only* in-app a11y accommodation, and it's hidden.** `app.dart:122-138` implements Ctrl +/-/0 text scaling but it's undiscoverable. No high-contrast toggle, no reduced-motion handling, no focus-ring rendering anywhere. - **[Minor] Contrast gate covers only 11 token pairs** — omits `globalTextMuted` (muted text is everywhere), the `status*` foregrounds, syntax tokens on `panelBackground`, and `panelActiveBorder`. - **[Minor] ~43 hardcoded-color sites bypass the token system** — some defensible (ANSI palette), but the modal/palette shadow and window-control colors won't adapt to the `paper` light theme. - **[Minor] Hover state is inconsistent and not paired with focus** — every interactive widget reimplements its own `_hover` bool; none render a focus indicator. - **[Minor] `_LeftHatContent` is dead code** — `app.dart:281-292` always returns `SizedBox.shrink()`. ### Recommendations **Quick wins:** add a `Focus` + `Actions`/`Shortcuts` (Enter/Space → onTap) wrapper and a focus-ring inside `ClideTappable` — fixes the [Critical] for every button/list-item at once; add arrow-key + Escape + selected-index to `ClidePalette` (copy the existing `_ProjectSwitcherDropdown` `onKeyEvent` pattern at `app.dart:446-452`); expand `canonicalPairs`; surface text-zoom and theme switching in the palette; tokenize the modal shadow and window-control colors. **Larger efforts:** establish a real focus-traversal model and integrate `FocusTracker` with Flutter's focus system; replace the root `KeyboardListener` with scoped `Shortcuts`/`Actions` and move off `keyLabel`; add keyboard operability + Semantics to drag-resize handles plus a `clide panel resize` CLI; add an a11y test tier that asserts *operability*, not just Semantics presence. ### Scorecard | Dimension | Score | Justification | |---|---|---| | Interaction model | 2/5 | Coherent slot/panel structure and good drag-resize *with a mouse*, but keyboard operability is largely unbuilt. | | Accessibility | 3/5 | Genuine contrast gate, Semantics on composed widgets, i18n contract tests — but keyboard operability and focus order are not implemented. | | Visual consistency | 4/5 | Strong semantic token system consumed consistently; a few hardcoded-color sites are real theme-adaptation bugs. | | Discoverability | 2/5 | Command palette isn't keyboard-navigable; accommodations are undiscoverable; no in-app keybinding reference. | | State coverage | 3/5 | Data panels and dialogs handle loading/error/empty; but no focus states anywhere and no reduced-motion handling. | --- ## 4. Code quality & craft — Senior Dart/Flutter Engineer ### Executive summary clide is, for a solo pre-v2.0 project, in genuinely good shape. The core subsystems (IPC envelope, daemon dispatch, git client, PTY) are small, single-responsibility, well-typed, and consistent. `flutter analyze` is clean for `lib/` — the 9 reported issues are all in `test/`, none are suppressions. The biggest strength is the FFI/PTY layer: `lib/src/pty/native_pty.dart` shows real systems-programming discipline (pre-fork allocation, errno captured before `free`, isolate-teardown ordering documented and correct). The biggest risk is concentrated in two places: a genuine isolate-leak race in `SchedulerService`, and the large vendored-but-owned `lib/src/terminal/` xterm.dart fork (~7k LOC) which carries a different style, commented-out `print`s, and dangling TODOs that the project's own "lib is owned, not vendored" rule says must be held to the same bar. ### Strengths - **PTY/FFI layer is excellent.** `native_pty.dart:129-145` force-resolves FFI trampolines and pre-allocates *all* native memory before `forkpty()`. `native_pty.dart:171-177` captures `errno` before `_freeAll` because `free()` can clobber it. `close()` (367-397) documents and implements the kill→EOF→close ordering to avoid fd-reuse races. The child branch touches no Dart heap. - **IPC envelope is clean and idiomatic** — `lib/src/ipc/envelope.dart` uses a `sealed` class hierarchy, named constructors, a private unifying constructor, and conditional map keys. Decode is total over the type discriminant. - **Typed, meaningful errors.** `PtyException` carries `op` + optional `errno`; `GitException` carries `stderr`; `errnoToIpcError` maps POSIX errno to actionable IPC error kinds. Errors are values, not strings. - **Resource lifecycle is taken seriously across most subsystems.** `FileWatcher.stop()` cancels the subscription *and* closes the controller; `withBuffer`/`setWinsize` in `libc.dart` use `try/finally` around every native allocation. 45 files define `dispose`/`close`. - **The `DaemonEventSink` interface** keeps the dependency graph pointing the right way (server→subsystems) and is documented as such. - **The one `ignore_for_file` (`libc.dart:11-27`) is exemplary** — textbook FFI case, multi-paragraph justification exactly as CLAUDE.md requires. ### Findings - **[Major] Isolate-leak race in `SchedulerService._startTicker`** — `scheduler.dart:71`: `Isolate.spawn(...).then((iso) => _isolate = iso)`. If `_stopTicker()` runs before the spawn future completes, `_isolate` is still null, nothing is killed, and the just-spawned isolate (with its `Timer.periodic`) leaks. `native_pty.dart` solved exactly this with `_readerReady`. - **[Major] `lib/src/terminal/` held below the project's own bar.** Carries commented-out `print()` debugging (`custom_text_edit.dart:244-275`), dangling TODOs (`parser.dart:110-113`, `keytab.dart:91`), a 1137-line `parser.dart`, and the only `// ignore: invalid_use_of_protected_member` in the repo (`terminal_view.dart:363`). Either it's genuinely vendored (belongs in `native/` or documented as frozen) or it's owned (needs the cleanup pass). - **[Minor] Empty `catch (_) {}` swallows in `tree_sitter_ffi.dart:197,206`** — `DynamicLibrary.open` failures silently discarded; caller gets a bare `null` with no diagnostic about *why*. Syntax highlighting silently not working is a support headache. - **[Minor] Empty `catch (_) {}` in `test_app.dart:271,311`** — `:271` swallows a theme-load failure the harness exists to detect. - **[Minor] Dead alias in `libc.dart:201-202`** — `typedef Cmsghdr = CmsghdrLinux;` flagged "backward compatibility"; CLAUDE.md forbids backwards-compat hacks in a solo repo. - **[Minor] `// ignore: unused_field` in `editor_controller.dart:25`** "kept for future subscription changes" — speculative retention; the no-suppression rule wants it fixed, not silenced. - **[Minor] Magic numbers in hot FFI paths.** `native_pty.dart` inlines `0x0001 // POLLIN`, `28 /* SIGWINCH */`, `4 /* EINTR */`, `9 /* EBADF */` — but `libc.dart` already has a constants section and `errno_mapping.dart` has `PosixErrno.ebadf`. - **[Minor] `git_commands.dart` has ~16 near-identical handler bodies** — a `_guarded(req, () async {...})` helper would remove ~60 lines of structural duplication. Borderline. ### Recommendations **Quick wins:** fix the `SchedulerService` spawn race (track the spawn future like `NativePty._readerReady`); replace the three silent `catch (_)` in `tree_sitter_ffi.dart` with a logged last-error; delete the `Cmsghdr` alias and the `unused_field` suppression; have the PTY layer consume `libc.dart` constants / `PosixErrno` instead of inline hex. **Larger efforts:** decide the status of `lib/src/terminal/` — formally vendor it (freeze, document, decision-record) or do the cleanup sweep; optionally a `_guarded` helper for `git_commands.dart` (check whether `files_commands` / `editor_commands` share the shape). ### Scorecard | Dimension | Score | Justification | |---|---|---| | Idiomatic Dart | 4/5 | Sealed classes, named ctors, records, `const`, immutability used well; the vendored terminal tree pulls the average down. | | Error handling | 4/5 | Typed errors with context everywhere in core; a few silent `catch (_)` in the FFI loader and test harness cost the 5th point. | | Naming & readability | 4/5 | Clear, intention-revealing names; comments earn their place; inline magic numbers are the main blemish. | | Consistency across subsystems | 3/5 | IPC/git/files/pty are uniform; `lib/src/terminal/` is a different codebase in style; PTY duplicates constants `libc.dart` owns. | | Resource/lifecycle safety | 4/5 | `try/finally` around native allocs, controllers closed, subscriptions cancelled; the one real defect is the `SchedulerService` race. | --- ## 5. Security & supply chain — Security Engineer ### Executive summary clide's security posture is **above average for a solo pre-v2.0 project**. All subprocess calls use `Process.run`/`Process.start` with argument *lists* (no shell interpolation), the IPC transport is a per-user Unix socket (not a TCP port), and there is an explicit `path_safety` module with a containment check. The single biggest strength is the disciplined no-shell subprocess layer. The single biggest risk is **untrusted-workspace code execution via toolchain resolution** (`toolchain_paths.dart:79`): a malicious repo can ship a `native/dugite/bin/git` executable that clide will resolve and run. Secondary real issues: path-safety does not defend against symlink escape, and IPC command args are largely unvalidated/un-bounded. Supply-chain hygiene is mostly good but `licenses.yaml` has drifted from `pubspec.yaml` and native binaries are committed without SHA pinning. ### Strengths - **No-shell subprocess execution.** `GitClient._run` (`client.dart:210`), `PqlClient._run` (`client.dart:165`), and the PTY layer all pass `List` args directly. Classic command injection is structurally prevented. - **Toolchain uses resolved absolute paths** — git/pql/tmux resolved once to absolute paths and reused. - **Path containment check exists and is used.** `resolveUnderRoot` (`path_safety.dart:21`) collapses `..`/`.` without touching the filesystem and enforces a prefix check with a separator guard. `files.read`/`files.ls` both call it. - **IPC is a per-user Unix socket, not a network listener.** No `ServerSocket` over TCP anywhere; the default runtime path is in-process, eliminating the socket attack surface in the shipped app. - **PTY FFI memory discipline** — all native memory allocated before `forkpty()`, `errno` captured before `free()`, freed on every path. - **`pubspec.lock` is committed**, deps use exact pins (no carets), `licenses.yaml` exists with per-dep purpose/license. ### Findings - **[Critical] Malicious workspace can plant a git binary that clide executes.** `toolchain_paths.dart:79-84` builds `'$workspaceRoot/native/dugite/bin'` and runs `_firstExisting(['$dugite/git'])`; if that file exists it becomes the git binary for all `GitClient` calls, **before** falling back to PATH. An attacker commits an executable at `native/dugite/bin/git`; clide runs it on the first `git.status` (which fires automatically on workspace open). Arbitrary code execution from merely opening a repo. The `native/dugite` convention should resolve relative to the *clide install dir*, never the workspace root. - **[Major] Path-safety does not defend against symlink escape.** `path_safety.dart: 35-51` explicitly does not resolve symlinks, and the filesystem layer (`files_commands.dart:81-85`) never does either. A repo symlink `config -> /etc/shadow` passes the containment check (the *link path* is under root) and clide reads the target. Fix: after `resolveUnderRoot`, `resolveSymbolicLinksSync()` and re-verify containment. - **[Major] IPC command arguments are unvalidated and unbounded.** `DaemonDispatcher.dispatch` (`dispatcher.dart:26`) and `IpcRequest.fromJson` (`envelope.dart:49`) do no schema validation. No size limit on `files.read`, no count cap on `git.log`, no check that `git.checkout`'s `branch` (`git_commands.dart:240`) isn't a `-`-prefixed flag. `git diff`/`stage` use `--` separators (good), but `checkout(branch)` and `push(remote, branch)` do not — argument injection (`git checkout --upload-pack=...`) is possible. - **[Minor] macOS entitlements disable library validation.** `macos/Runner/Release.entitlements` sets `disable-library-validation` = true with no App Sandbox entitlement. Arguably needed for the `dlopen` of `libtree-sitter.so`, but combined with no sandbox a compromised process has full user-level filesystem access. - **[Minor] `licenses.yaml` has drifted from `pubspec.yaml`.** Lists dev-dep `test` at `1.25.8` but `pubspec.yaml:60` pins `1.30.0`; lists a `lints 5.0.0` not in `pubspec.yaml` at all. The two-step-commit guardrail is being violated. - **[Minor] Native binaries committed without SHA pinning.** `native/linux-x64/` has `libtree-sitter.so` (24 MB) and `ptyc` (22 KB) committed with no `SHA256SUMS` manifest. CLAUDE.md says native deps are "pinned by SHA"; that pinning is not evidenced. - **[Informational] No secrets service** — clide stores no tokens; git auth is delegated to the system credential helper. The right call; noted so the absence reads as deliberate. - **[Informational] Lua runtime is a stub** — `lib/lua/src/host.dart` is Tier-0. Design intent (strip `io`/`os.execute`/`package.loadlib`/`debug`) is sound; re-assess at Tier 6 — sandbox-escape via FFI re-entry will be the concern. ### Recommendations **Quick wins:** fix toolchain resolution to resolve `native/dugite` against `Platform.resolvedExecutable`'s directory, never `workspaceRoot` (closes the Critical); add symlink re-check in `files.read`/`files.ls`; reconcile `licenses.yaml` with `pubspec.yaml`; reject `-`-prefixed values for `branch`/`remote`/`path` args (or use `--` everywhere, including `checkout`). **Larger efforts:** schema-validate the IPC surface with typed arg schemas + size/count bounds; add a committed `native/SHA256SUMS` verified by `make` and CI; revisit macOS sandboxing (App Sandbox with explicit exceptions); security-review the Lua FFI boundary and capability table before Tier 6 ships. ### Scorecard | Area | Rating | Justification | |---|---|---| | Subprocess safety | 2/5 | No-shell arg lists are excellent, but the workspace-relative dugite path is a real RCE; argument-injection on `checkout`/`push` unmitigated. | | IPC input validation | 3/5 | Per-user Unix socket + in-process default sharply limits exposure, but zero arg-schema validation and no size/count bounds. | | Path/filesystem safety | 3/5 | Real containment check that's actually wired in, undermined by the unhandled symlink-escape gap. | | Dependency/supply-chain hygiene | 3/5 | Exact pins, committed lockfile, documented deps — but `licenses.yaml` drift and missing SHA manifest for committed native binaries. | | Secrets & sandboxing | 3/5 | Correctly delegates secrets; Lua sandbox is only a stub; macOS runs with library validation off and no App Sandbox. | --- ## 6. Docs, governance & DX — TPM / Developer-Experience Consultant ### Executive summary clide runs an unusually disciplined governance system for a solo-dev pre-v2.0 project: 67 decision records across six domains, with a parser-validated DQR structure, anchored cross-references, and a `make decisions-validate` gate wired into pre-push. The biggest strength is that the DQR system is genuinely *alive* — questions get resolved with dated amendments, superseded decisions are marked, and decisions cite the commits that implement them. The biggest risk is **documentation drift in the narrative docs**: `README.md` and `docs/initial-plan.md` describe an architecture (Go sidecar, `ptyc/` C helper, `app/` subdirectory, separate daemon) that three major decisions (D-5, D-56, the FFI pivot) have since dissolved. A new contributor reading the README first would build a wrong mental model. ### Strengths - **DQR system is maintained, not ornamental.** Resolved questions carry dated resolution lines pointing to the deciding D-record (`questions/architecture.md:39` Q-6→D-57). D-40 carries a `[SUPERSEDED]` tag and an amendment line. - **Decisions are linked to code and commits.** D-67 (`decisions/process.md:61`) cites implementing commits `01a99ed`, `d162ba2`. D-66 references `ci/test.sh` by path. - **Governance migration was done cleanly** — the `decisions/` → `governance/` restructure updated cross-references and the auto-generated index. - **Commit discipline is real.** `git log` shows imperative subjects, no Conventional Commits prefixes, ticket refs, logical scoping — exactly what `git-commit/SKILL.md` prescribes. - **`licenses.yaml` is thorough** — all six runtime Dart deps present, plus fonts/native libs, with purpose justifications. *(Note: the Security reviewer found version drift in this file — see Finding above; the two reviewers examined different rows.)* - **Makefile is self-documenting** (`##` help annotations) and matches `CLAUDE.md`. ### Findings - **[Critical] `docs/initial-plan.md` is badly stale.** The "north-star" doc (linked from `CLAUDE.md:14` and `README.md:44`) still describes a Go sidecar (`initial-plan.md:4,55,189`), `clide --daemon` long-running process (`:162-164`), `app/` subdirectory layout (`:184-204`), and `project.yaml` (`:172`) — all contradicted by D-5, D-56, and the single-package-at-root reality. Nothing flags it as historical. - **[Critical] `README.md` describes a dissolved architecture.** `README.md:10` documents `ptyc/` as a live component; `README.md:36` lists `make ptyc-build`. The `ptyc/` directory does not exist, the Makefile has no such target, and the CHANGELOG's own Unreleased section records ptyc's removal. - **[Major] `README.md:44` links to `decisions/`** — a directory that no longer exists (migrated to `governance/`). Dead link in the primary onboarding doc. - **[Major] CHANGELOG has duplicate subsection headings in `[Unreleased]`.** Three `### Changed` blocks (`CHANGELOG.md:100, 114, 168`), two `### Fixed`, two `### Removed` in the 2.0.0 section. Keep a Changelog 1.1.0 expects one of each per release. - **[Major] No `CONTRIBUTING.md` or onboarding doc.** For a project "intended to ship publicly to other developers," there is no contributor guide; the build/test story is scattered across `CLAUDE.md` (Claude-oriented), `README.md` (partly wrong), and Makefile help. - **[Major] Coverage-floor governance contradicts itself.** D-66 (`testing.md:65`) says the floor lives at `coverage/floor.txt` starting "≈35%"; `CHANGELOG.md:44-46` says it's in `pubspec.yaml` `coverage_floor:` starting at 34%; the latest commit is `9030e56 hold coverage_floor fixed at 90`. Three sources, three mechanisms/values. D-66 was never amended. - **[Minor] `ci/release.sh` is a stub that still references goreleaser/sidecar** — Go tooling for a project with no Go. - **[Minor] CHANGELOG `[2.0.0] — 2026-05-03` dating** — the v2.0.0 tag is dated 2026-05-03 but the enormous Unreleased section represents ~80 commits of post-tag work with no interim version. - **[Minor] Stale-ish open questions** — Q-25 (body text face) is de facto resolved by D-43/D-44 and the shipped impl; Q-1/Q-2/Q-3 ("defer until Tier 1 is in real use") are due for triage now that Tier 1 has shipped. - **[Minor] Skills system is coherent but undocumented as a set** — eight skills under `.claude/skills/`, no index. ### Recommendations **Quick wins:** rewrite `README.md`'s `ptyc/` sections and fix the `decisions/` link; banner `docs/initial-plan.md` as historical (or split out a current `docs/architecture.md`); merge the duplicate changelog subsection headings; amend D-66 to reflect the floor's actual location/mechanism/value with a dated amendment line; triage Q-1/2/3/25. **Larger efforts:** write a human-facing `CONTRIBUTING.md` (clone → `make hooks && flutter pub get` → `make test` → DQR workflow → commit conventions); cut an interim release to drain the ~80-commit Unreleased backlog; add a `.claude/skills/README.md` inventory; establish a periodic governance sweep (the repo even has a `clean-house` skill for exactly this). ### Scorecard | Area | Score | Justification | |---|---|---| | Governance discipline | 4/5 | DQR system genuinely maintained — but D-66 drift and untriaged Tier-1-era questions show the sweep cadence lags the code. | | Documentation accuracy | 2/5 | Both primary onboarding docs describe a dissolved Go-sidecar/ptyc/daemon architecture; `CLAUDE.md` is accurate by contrast. | | Changelog hygiene | 3/5 | Per-commit discipline is followed, but duplicate subsection headings violate the standard and an 80-commit Unreleased backlog undermines the format. | | Contributor onboarding | 2/5 | No `CONTRIBUTING.md`; build story split across three docs, one wrong; `CLAUDE.md` is Claude-addressed, not human-addressed. | | Convention adherence | 4/5 | Commit style, DQR claiming, `licenses.yaml` two-step rule demonstrably followed; docked for the changelog defects and the README gap. | --- ## Closing note The recurring pattern across all six reviews: **clide's foundations are excellent and its finishing is incomplete.** The extension contract, test helpers, FFI discipline, governance system, and token system are all things most projects never get right. The gaps — IPC not wired, keyboard not operable, docs describing a dead architecture, a workspace-relative binary path — are all the kind of thing that happens when a fast-moving solo project's implementation outruns its connective tissue. They are concentrated, not diffuse, and the quick-win column above would close most of the critical ones in a few focused days.