Add a Makefile, a README and the review this branch works from
There was no onboarding doc of any kind. The Makefile covers build, test, vet, fmt, a fmt-check gate, tidy, run, the MCP stdio mode, docker-build and the ecosystem compose targets. The README covers what Hexis is and where it sits between Nexus, Praxis and workspace-mcp, how to build and run it, the flag and environment table including the new HEXIS_API_TOKEN, the API surface, and the tests. REVIEW-2026-07-30.md is the engineering review the preceding commits address, kept in-tree as the rationale for them. It includes an independent second-reviewer pass, and its "uncertainties" section has since been resolved: the service is not reachable from the public internet (the internet-exposed nginx config in the Maven repo is a template, not what is deployed), Nexus has no blessing concept and none is planned, and the running image was built from an uncommitted working tree hours before the first commit existed — which is why the deployed binary never matched any revision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uea55zaiWuEByEDC4UBSdd
This commit is contained in:
kami
@@ -0,0 +1,177 @@
|
||||
# Hexis — Senior Engineering Review
|
||||
|
||||
Date: 2026-07-30
|
||||
Reviewed at: commit `945e4ba` (working tree had one uncommitted change in `cmd/hexisd/main.go`: default Nexus URL `8987` → `9740`)
|
||||
Reference contract: `/home/kami/apps/ECOSYSTEM-SPEC.md` §4
|
||||
|
||||
---
|
||||
|
||||
## verdict
|
||||
|
||||
**Fragile + misaligned, but structurally sound and worth repairing incrementally.** No rewrite. The architecture matches the spec's shape; the *guards the spec exists to enforce* are missing, and the primary execution path is functionally dead at HEAD.
|
||||
|
||||
---
|
||||
|
||||
## first assessment
|
||||
|
||||
```text
|
||||
purpose: capability registry + guarded execution for the Nexus/Praxis/Hexis
|
||||
ecosystem (ECOSYSTEM-SPEC.md §4)
|
||||
intended users: Maven (voice + mavweb), Command Center, MCP clients
|
||||
actual users: Maven only — mavweb reads /api/v1/capabilities; mavend/voice.go:1329-1381
|
||||
resolves→matches→executes via a *vendored* copy of pkg/client
|
||||
critical workflows: 1. list capabilities for an entity 2. confirm 3. execute 4. poll changes
|
||||
current state: running on :9741 — but the live binary predates commit 89d8433
|
||||
(its GET capability response has no `enabled`/`requires_confirmation`
|
||||
fields), so deployed ≠ HEAD
|
||||
known failures: every workspace_mcp capability is registered with enabled=false (verified)
|
||||
maintenance burden: low — 3.6k LOC, 1 real dependency + pure-Go sqlite, builds clean, vet clean
|
||||
technical constraints: single-user homesrv, single SQLite writer, distroless container
|
||||
personal constraints: solo maintainer; spec is the contract, three services must agree
|
||||
what still works well: confirmation lifecycle (args_hash + version + TTL) is correct and
|
||||
genuinely well tested; storage layer is honest; timeout⇒unknown is right
|
||||
what has become obsolete: systemd provider (no systemctl in distroless), ListenUnix, the
|
||||
contract_test.go "bug documentation" tests, schema_migrations table
|
||||
```
|
||||
|
||||
Classification: **fragile** (safety guards absent), **misaligned** (deployed ≠ HEAD; spec §4.3/§4.4 unmet), partly **underbuilt** (no auth, no arg validation).
|
||||
|
||||
---
|
||||
|
||||
## main findings
|
||||
|
||||
### 1. Every workspace capability is registered disabled — the product's only working provider cannot execute
|
||||
|
||||
- **problem:** `BuildCapabilities()` never sets `Enabled`, so it defaults to `false`.
|
||||
- **evidence:** `internal/provider/workspace_mcp.go:235-249` — the `domain.Capability` literal omits both `Enabled` and `TimeoutSeconds`, leaving them zero-valued. Note that the same function *skips* allowlist-disabled tools at `:226`, so it plainly intends everything it emits to be live: this is an omitted field, not a policy decision. A throwaway test at HEAD printed `Enabled=false RequiresConfirmation=false TimeoutSeconds=0`. `internal/execution/engine.go:41` then returns `ErrCapabilityDisabled` → 403 for all 19 capabilities.
|
||||
- **impact:** Maven's voice execute path can never succeed against a fresh database. Nothing in the suite catches it — no test executes a workspace-provided capability end to end.
|
||||
- **action:** **repair.** Set `Enabled` from risk tier and `TimeoutSeconds` explicitly. Must not land alone — see finding 2.
|
||||
|
||||
### 2. Fixing (1) naively arms unconfirmed docker stop/restart on a publicly-proxied, unauthenticated port
|
||||
|
||||
- **problem:** `BuildCapabilities` also leaves `RequiresConfirmation=false` for `risk: medium` docker start/stop/restart, and the HTTP API has no authentication whatsoever.
|
||||
- **evidence:** `Maven/deploy/ecosystem/nginx.conf:32-42` proxies `hexis.kvmx.ru:80` → `127.0.0.1:9741` with no auth block; `internal/api/handler.go:37` exposes `POST /api/v1/execute`; `handler.go:115-166` lets any caller `POST /api/v1/capabilities` with `provider`+`operation` of their choosing. Precisely: `handler.go:142` derives the correct spec §4.3 default (`enabled := req.Risk != "destructive"`), and then `:143-145` lets an optional `enabled` field in the request body override it. The guard is implemented and then made opt-out by the untrusted caller — worse than simply absent, because it reads as enforced. `requires_confirmation` (`:159`) is copied from the body with no derivation at all.
|
||||
- **concrete attack path:** `POST http://hexis.kvmx.ru/api/v1/capabilities {"name":"x","provider":"workspace_mcp","operation":"ssh.exec","enabled":true}` then `POST /api/v1/execute`. The allowlist blocks that specific one (`workspace_mcp.go:129-135` re-checks `ssh.exec` is disabled) — but `docker.stop_container` is allowed, and self-registration bypasses `requires_confirmation` entirely.
|
||||
- **impact:** remote unauthenticated container control.
|
||||
- **action:** **security, blocking.** Require a shared token on `/api/v1/`; derive `requires_confirmation` from risk tier server-side; and stop honouring `enabled` from the request body entirely rather than treating it as an override.
|
||||
|
||||
### 3. Hexis accepts free-text targets — the one thing the spec says it must never do
|
||||
|
||||
- **problem:** `target_entity_id` is accepted as any non-empty string. Nothing checks the `ent_` shape, existence in Nexus, or `capability.TargetTypes`.
|
||||
- **evidence:** `handler.go:213-216` checks non-empty only; `engine.go:44-46` only compares against a *pinned* `TargetEntityID`, which is `""` for all 19 registered capabilities. Spec §4.3: "Hexis never accepts a free-text target. Ever."
|
||||
- **impact:** the invariant "Maven must not invent mappings" is unenforced at the point it matters.
|
||||
- **action:** **repair.** Validate the target against Nexus `GET /api/v1/entities/{id}` (that endpoint exists) plus a `TargetTypes` match, before creating the execution.
|
||||
- **note:** the spec's stronger *blessing* guard (§4.3) is **not implementable today** — `grep -rni bless` across `nexus/` returns nothing. Nexus has no blessing concept. That's a cross-service gap, not a Hexis bug.
|
||||
|
||||
### 4. Audit fields are written but never read back, or never written at all
|
||||
|
||||
- **problem:** `confirmation_id` is INSERTed but absent from every SELECT, so it always reads back empty. `causation_id` isn't in the `executions` schema at all, yet `engine.go:79` sets it on the struct — the field is populated in memory with nowhere to land.
|
||||
- **evidence:** `confirmation_id` appears in the INSERT at `sqlite.go:295` and in **none** of the three executions SELECTs (`:313`, `:369`, `:418`). `causation_id` exists only on `hexis_events` (`:119`, `:502`), never on `executions`. Also missing vs spec §4.1: `capability_version` on executions (it exists on `confirmations` at `:134`, so the precedent is there).
|
||||
- **impact:** "which confirmation authorized this destructive act" is unanswerable from the API — the core audit question. Silent, not a crash.
|
||||
- **action:** **repair.** Add the columns as a migration, persist and select them.
|
||||
|
||||
### 5. `contract_test.go` asserts the opposite of current behavior and passes anyway
|
||||
|
||||
- **problem:** three tests are prose, not verification. `TestContract_ChangesSinceAlwaysZero` documents a bug that commit `74b19e0` already fixed, and can only `t.Logf`. `TestContract_TimeoutGoroutineNotCancelled` is a bare `t.Log`. `TestContract_DestructiveCapabilityDisabledByDefault` re-implements `risk != "destructive"` inside the test and asserts on its own local variable — it would pass if the handler were deleted.
|
||||
- **evidence:** `internal/execution/contract_test.go:212-299`.
|
||||
- **impact:** the suite's green status overstates coverage; a misnamed test actively misleads about a fixed bug.
|
||||
- **action:** **cleanup.** Delete the three; the real coverage gap is an end-to-end execute over a provider-registered capability.
|
||||
|
||||
### 6. Read-only failures are reported as `unknown`, and timed-out provider calls leak
|
||||
|
||||
- **problem:** `runWithTimeout` (`engine.go:161-186`) can't cancel — `Provider.Execute` takes no `context`. The goroutine runs to the HTTP client's 60s timeout, outliving the 30s capability timeout.
|
||||
- **evidence:** a POST of a `read`/`read_only` capability against the live service while workspace-mcp was down (`:9930` → 503) hung past 5s. Per spec §4.3 that lands `outcome=unknown` — "may or may not have completed the side effect" — for a call that by definition has none, and `unknown` is never retried.
|
||||
- **impact:** read failures are unretryable and indistinguishable from genuinely ambiguous mutations.
|
||||
- **action:** **refactor.** Add `ctx` to the `Provider` interface (2 implementations), and let `read_only=true` timeouts resolve to `failed`.
|
||||
|
||||
### 7. Race: the one-in-flight-per-(capability,target) guard is check-then-insert
|
||||
|
||||
- **problem:** `engine.go:56-58` queries, then `engine.go:92` inserts, with no uniqueness constraint between them.
|
||||
- **evidence:** no partial unique index on `(capability_id, target_entity_id)` where `status='started'`; the existing `idx_executions_inflight` is non-unique (`sqlite.go:561`).
|
||||
- **impact:** two concurrent requests both proceed. Narrow window, single writer, low likelihood — but the spec states it as a hard guard and the existing test only covers the sequential case.
|
||||
- **action:** **repair**, via a partial unique index.
|
||||
|
||||
### 8. Deployed binary is older than HEAD; registered capabilities never refresh
|
||||
|
||||
- **problem:** `cmd/hexisd/main.go:80-83` skips any capability whose ID already exists, so allowlist edits to risk/target_type/enabled never propagate to an existing database.
|
||||
- **evidence:** live GET returns no `enabled` field → pre-`89d8433` build. Rows dated 2026-07-19.
|
||||
- **impact:** config drift; even the correct fix for (1) won't reach the running instance's existing rows.
|
||||
- **action:** **repair.** Reconcile on startup (update when the derived record differs), and redeploy.
|
||||
|
||||
---
|
||||
|
||||
## keep
|
||||
|
||||
Confirmation lifecycle and its tests; the storage layer's explicitness; `timeout ⇒ unknown`; the provider registry (2 real implementations justify it); the allowlist-as-config design; `nexusclient`; pure-Go sqlite + distroless build.
|
||||
|
||||
## remove
|
||||
|
||||
- `internal/provider/systemd.go` — no `systemctl` in the distroless image and no capability ever registers it; it's the spec's §4.2 `systemd_dbus` slot filled with a shell-adjacent stub. Delete, or replace with D-Bus when a real need appears.
|
||||
- `Server.ListenUnix` (dead; never called).
|
||||
- `schema_migrations` table (unused; `PRAGMA user_version` is the real mechanism). **Caveat:** confirm no already-deployed database depends on it before dropping — finding 8 establishes the live instance is several commits behind, so its schema state is not HEAD's.
|
||||
- `execution.ResolveRequest` / `ResolveResult` + `MarshalJSON` (superseded by `nexusclient`).
|
||||
- `Engine.emitEvent`, `Registry.List`, `DiscoveredTools`, `Capability.IsDestructive`, `ifString`.
|
||||
- `EventCapabilityUnavailable`, `EventExecutionDenied`, `ExecutionDenied`, and 8 unused `Err*` values.
|
||||
- The three vacuous contract tests.
|
||||
|
||||
## repair now
|
||||
|
||||
1. Auth on `/api/v1/` + server-derived `requires_confirmation` (finding 2) — **before anything else**.
|
||||
2. `Enabled` / `TimeoutSeconds` / `RequiresConfirmation` in `BuildCapabilities`, plus startup reconciliation (1, 8).
|
||||
3. Target validation against Nexus + `TargetTypes` (3).
|
||||
4. Persist/read `confirmation_id`, `causation_id`, `capability_version` (4).
|
||||
5. Partial unique index for in-flight (7).
|
||||
6. Delete the dead code and misleading tests (5, remove).
|
||||
|
||||
## refactor later
|
||||
|
||||
- `context` through `Provider.Execute` (6).
|
||||
- Collapse the four competing wire shapes for a capability — `handler.go:96-108` (which emits both `id` and `capability_id`), `adapter.go:147-157`, `pkg/client`, and Maven's vendored copy — into one serializer.
|
||||
- `GET /api/v1/executions?entity_id=&since=` (spec §4.5, Command Center needs it).
|
||||
- A `Makefile` and a README: there is no onboarding doc at all.
|
||||
|
||||
## rewrite only if
|
||||
|
||||
Nexus gains blessing and versioned capabilities `(capability_id, version)` as the primary key, *and* multi-writer access becomes real. Neither is true; incremental repair is clearly cheaper. The confirmation logic is the hidden knowledge here and it is correct — do not throw it away.
|
||||
|
||||
## verification plan
|
||||
|
||||
- `go build ./... && go vet ./... && go test ./...` — all currently pass. `gofmt -l` flags `internal/provider/systemd.go`, `internal/provider/workspace_mcp.go`, `pkg/client/client.go` as unformatted at HEAD.
|
||||
- New tests, each pinning a finding:
|
||||
- execute a `BuildCapabilities`-registered capability end-to-end (1)
|
||||
- unauthenticated `/api/v1/execute` rejected (2)
|
||||
- non-`ent_` / unknown target rejected (3)
|
||||
- round-trip `confirmation_id` through `GET /executions/{id}` (4)
|
||||
- concurrent duplicate execute → exactly one row (7)
|
||||
- Live: rebuild, redeploy, `curl` a read capability once workspace-mcp is back (it is 503 as of this review), confirm `succeeded` and non-empty `changes`.
|
||||
|
||||
## uncertainties
|
||||
|
||||
- **Unknown:** whether `hexis.kvmx.ru:80` is actually resolvable from outside — the nginx config was read, external reachability was not tested. The severity of finding 2 depends on this; the missing-auth defect stands either way.
|
||||
- **Unknown:** whether blessing is planned for Nexus soon, or whether the target-existence check in finding 3 is the intended permanent guard.
|
||||
- **Assumption:** Maven is the only consumer. Based on a repo-wide grep of `/home/kami/apps`; an out-of-tree MCP client would not appear.
|
||||
- **Confirmed but unexplained:** the live instance is ~4 commits behind. Unclear whether that is a stalled deploy or deliberate.
|
||||
|
||||
---
|
||||
|
||||
## second-reviewer verification (2026-07-30)
|
||||
|
||||
An independent pass re-checked the load-bearing claims against the code at HEAD rather than accepting them. **The verdict and all eight findings stand.** Confirmed directly:
|
||||
|
||||
- **Finding 1** — `workspace_mcp.go:235-249` omits `Enabled` and `TimeoutSeconds`; `engine.go:42` rejects on `!capability.Enabled`. The execution path is dead as described.
|
||||
- **Finding 2** — no `auth`, `token`, or `Authorization` handling exists anywhere under `internal/api/`. The `hexis.kvmx.ru:80` server block proxies straight to `127.0.0.1:9741` with only `X-Forwarded-*` headers set and no auth directive.
|
||||
- **Finding 4** — verified by comparing the INSERT column list against all three SELECT column lists; `confirmation_id` is write-only. `causation_id` is absent from the `executions` DDL.
|
||||
- **Finding 5** — the three tests are as characterised. `TestContract_DestructiveCapabilityDisabledByDefault` computes `enabled := risk != "destructive"` as a local variable and asserts on that, touching no production code. `TestContract_TimeoutGoroutineNotCancelled` is a single `t.Log` with no assertions.
|
||||
- **Finding 7** — `sqlite.go:143` creates `idx_executions_inflight` as a plain `CREATE INDEX`, not `CREATE UNIQUE INDEX`, and there is no partial unique index anywhere.
|
||||
- **Finding 8** — `cmd/hexisd/main.go:76-79` `continue`s on any capability whose ID already resolves, so derived changes never reconcile.
|
||||
- **Verification plan** — `gofmt -l` flags exactly the three files named.
|
||||
|
||||
Amendments made above, none of which change a conclusion: finding 2's description of `enabled` was corrected (it is a caller-supplied override of a correct server default, not "forced true"), finding 4's line references were corrected to the actual INSERT/SELECT sites, and the `schema_migrations` removal gained a deployed-schema caveat.
|
||||
|
||||
One emphasis difference: finding 4 reads as more serious than "silent, not a crash" suggests. The confirmation lifecycle is the part of this codebase the review rightly calls correct and worth keeping — but with `confirmation_id` unreadable, that correctness cannot be demonstrated after the fact from the API. A guard you cannot audit is hard to trust under incident review, which raises finding 4 close to the priority of the target-validation work in finding 3.
|
||||
|
||||
Agreement on sequencing is unqualified; see below.
|
||||
|
||||
## note on sequencing
|
||||
|
||||
Findings 1 and 2 are coupled. Fixing the dead-capability bug without adding auth and confirmation defaults would turn a broken execution path into a remotely reachable one. They should land together as one coherent change.
|
||||
Reference in New Issue
Block a user