jpmschweitzerandClaude Opus 4.7
6e0aa459d1
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>
777 lines
46 KiB
Markdown
777 lines
46 KiB
Markdown
# 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.
|