jpmschweitzer/clide
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

add 2026-05-14 consultant review

Browse Source 
Six-reviewer independent assessment at commit 9030e56. Cross-cutting
themes: IPC server unimplemented (D-1/D-6/D-56 drift), keyboard
operability largely unbuilt, workspace-relative dugite path is a
real RCE, onboarding docs describe a dissolved architecture.

Followup tickets land alongside this commit (see pql backlog).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-05-17 20:46:22 +02:00
co-authored by Claude Opus 4.7
parent 8391dd15c2
commit 6e0aa459d1
1 changed files with 776 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
consultants.md
+776
View File
@@ -0,0 +1,776 @@
# 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/<name>/` 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<String>` 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.