Skip to content

feat: CLI audit + local/api parity: open-hooks - #14

Closed
andrei-hasna wants to merge 5 commits into
mainfrom
factory/632dc176-187e-4220-b7bb-6d72c3bf-a12a59f6
Closed

feat: CLI audit + local/api parity: open-hooks#14
andrei-hasna wants to merge 5 commits into
mainfrom
factory/632dc176-187e-4220-b7bb-6d72c3bf-a12a59f6

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Jul 27, 2026 •
edited by blacksmith-sh Bot
Loading

Copy link
Copy Markdown
Contributor

Objective

CLI audit + local/api parity: open-hooks

Repo: hasna/hooks (local: /home/hasna/workspace/hasna/opensource/open-hooks).

Audit this repo's CLI: (1) Find every DEAD command — commands that are broken, error out, reference removed code, or are declared but non-functional — and fix them or remove them with justification. (2) Find every command that only works in 'local' mode (on-box SQLite/files) and does NOT work against the 'api' (self-hosted/remote) path; implement full api-mode support for each so every command works identically against local AND api backends (route through the cloud-router/stage-A/HTTP path per this repo's existing pattern). Ensure both modes are fully implemented and tested. Add regression tests for the fixed commands and for local-vs-api parity. Open a PR; do not merge.

Verification

  • policy source: base 735983b (immutable commit — agent-proof)
  • ⚠️ GATE-INTEGRITY: agent touched verify-bearing config — REVIEW (src/cli/index.tsx, src/index.ts, src/storage.ts)
  • install: pass
  • typecheck: pass
  • build: pass
  • test: pass

Run run_c6bbd99b9b6c · backend codewith · task 632dc176-187e-4220-b7bb-6d72c3bf8263
🏭 Generated by @hasnaxyz/factory

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna
feat: CLI audit + local/api parity: open-hooks
8164445
@andrei-hasna
fix: route hook ingestion through the API authority and harden /v1 ex…
761f648 
…posure

Addresses three P1 review findings on the local/api parity work.

1. Hook event ingestion was never ported to API mode, so `hooks log` read the
   remote authority while every hook kept writing to local SQLite that no api-
   mode command could see. `writeHookEvent` now resolves the same router the
   read path uses and POSTs to `/v1/log/events`; on an unreachable or
   incompletely configured authority the event is spooled to local SQLite (and
   reported on stderr) instead of dropped, and `hooks storage push` drains that
   spool idempotently. Adds the `POST /v1/log/events` route and a shared
   `buildHookEventRow`/`insertHookEvent` pair so both write paths persist the
   identical row.

2. `apiConfigPresent()` treated a bare API key as "the user wants HTTP", so a
   stray `HOOKS_API_KEY` hijacked the PostgreSQL remote/hybrid path and then
   failed with REMOTE_API_URL_MISSING. Routing now keys on the API URL alone,
   and a remote/hybrid environment carrying both a database URL and an API URL
   emits an explicit precedence warning instead of silently misrouting.

3. `hooks mcp --http` mounted the destructive `/v1` data API unconditionally and
   accepted the client-side `HASNA_HOOKS_API_KEY` as its admin credential. The
   mount is now behind an explicit `--api` flag that defaults to off, and the
   server authenticates against the separate `HASNA_HOOKS_API_SERVER_KEY` so one
   secret no longer serves both trust roles.

Each fix is covered by a test that fails when the fix is reverted.
@andrei-hasna
fix: keep bookkeeping tables off /v1 sync and bound every API request
9f6eac0 
Two reviewer findings on the new HTTP storage/ingestion transport.

schema_migrations transport (P1): `hooks storage push` sent the client's
migration ledger to the authority, so a machine on a newer release could mark
a migration as applied on an authority that never ran its DDL — permanently
suppressing it, including the CHECK-constraint rebuilds that admit
SessionStart/SessionEnd/UserPromptSubmit. The /v1 transport now carries a
DATA_SYNC_TABLES allowlist (hook_events, feedback): storageExportRows defaults
to it and refuses an explicitly named bookkeeping table, and storageImportRows
returns an error in the SyncResult instead of upserting schema_migrations or
_meta. The PostgreSQL sync path is unchanged.

Unbounded fetch (P1): no request carried a deadline, so an authority that
accepted the connection and never answered blocked writeHookEvent forever —
every agent tool call stalled until the agent's own hook timeout killed the
process and the event was dropped, contradicting the documented
spool-rather-than-drop guarantee. Every /v1 request now carries an
AbortSignal.timeout: 3s on the hook write path
(HASNA_HOOKS_API_WRITE_TIMEOUT_MS) and 30s for interactive CLI commands
(HASNA_HOOKS_API_TIMEOUT_MS). The abort surfaces as the existing
REMOTE_API_UNREACHABLE classification, so db-writer's catch spools as before;
a malformed override falls back to the default rather than disabling the
deadline.

Regression tests, each verified to fail with its fix reverted: the authority
refuses a schema_migrations payload and its ledger is unchanged, the default
push payload carries data tables only, and writeHookEvent resolves within the
deadline against a never-responding Bun.serve with the row landing in local
SQLite.
@andrei-hasna
fix: make hooks log clear purge the local mirror in API mode
1457760 
Under an API authority the local SQLite file is a spool and a pull mirror,
not a second source of truth: `hooks storage pull` writes authority rows
into it and `hooks storage push` uploads everything it still holds. `log
clear` deleted only on the authority, so the documented drain workflow
(`hooks storage push`) re-uploaded the events an operator had just purged
and `hooks log list` showed them again. `hooks log list` in local mode also
kept showing rows the operator believed were gone.

Clear the mirror after the remote DELETE succeeds, scoped by `--hook` the
same way the remote delete is. The purge runs even when the authority
reported nothing cleared: rows spooled while it was unreachable exist only
locally and would otherwise be pushed straight after the purge.

`clearLocalHookEventMirror` returns 0 without touching the filesystem when
no local database exists — `getDb()` would create the file and its schema,
and an API-mode client with no spool must not grow one just to empty it.

Regression coverage in src/cli/cli.test.ts drives the real CLI against a
loopback authority: pull -> clear -> push -> list returns [], an unpushed
spool is not pushed after a clear, `--hook` leaves other hooks' rows alone,
and a clear with no mirror creates no local database.
@andrei-hasna
fix: report both stores in hooks log clear and route MCP log tools …
e77a40e 
…through the API authority

`hooks log clear --yes` in API mode discarded the return value of
`clearLocalHookEventMirror()`, so purging an unpushed local spool the authority
never held printed "Nothing to clear." / `cleared: 0` while deleting every local
row — audit history destroyed behind a message saying nothing happened. The
command now reports `cleared_remote` and `cleared_local` alongside a headline
`cleared` that is the larger of the two, not their sum: `storage pull` mirrors
authority rows into the same local table, so adding the counts would report one
pulled event twice.

The four MCP log tools (`hooks_log_list`, `hooks_log_tail`, `hooks_log_errors`,
`hooks_log_summary`) still read local SQLite, which the API write path no longer
writes to, so they answered "no events" for work that had just landed on the
authority. They now route through the same client as `hooks log …` and fail
closed with the `REMOTE_*` message instead of serving an empty local result set.

Supporting parity: `since` filtering on `GET /v1/log/events`, and a new
`GET /v1/log/summary` route backed by the existing `summarizeHookEvents()`.
`send_feedback` deliberately keeps writing local SQLite — `feedback` is a
DATA_SYNC_TABLE, so the row spools and drains on the next `storage push`.
@andrei-hasna

andrei-hasna commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #14 @ e77a40e — lens: correctness+security+gates, reviewer Augustus (1 of 1)

What I read:

  • git log --oneline origin/main..HEAD and git diff origin/main...HEAD --stat.
  • Full diff and surrounding source for all changed files: README.md, the four observability hook entrypoints, src/cli/cloud-router.ts, src/cli/index.tsx, src/db/log-store.ts, src/db/storage-sync.ts, src/lib/db-writer.ts, src/mcp/http.ts, src/mcp/server.ts, src/server/api.ts, export surfaces, and the changed CLI/MCP/API/db-writer tests.

What I ran:

  • git rev-parse HEAD -> exit 0; confirmed e77a40eebb37a99732ce66e6c5ae9598de97c9fc.
  • git log --oneline origin/main..HEAD -> exit 0.
  • git diff origin/main...HEAD --stat -> exit 0.
  • bun install -> exit 0.
  • bun run typecheck -> exit 0.
  • bun test -> exit 1; 1051 pass, 51 fail.
  • bun run build -> exit 0.
  • Diagnostic only: HOME=<temporary directory under this worktree> bun test -> exit 0; 1102 pass, 0 fail.

Blocking P0/P1 findings:

  • P1 gate failure: the exact required local test gate bun test exits 1 in this worktree. The 51 failures are in installer/CLI/MCP install/update tests that read the workstation's existing ~/.claude/settings.json state; examples include tests expecting no installed hooks receiving the full installed hook list, and install tests receiving success: 0 or success: false because the hooks are already installed. The isolated diagnostic pass with HOME set to a temporary directory under the worktree passes all 1102 tests, so I did not find evidence that the new API routing code caused those failures. However, the requested merge rule requires the gates run here to be green, and the exact bun test gate is not green.

Non-blocking follow-ups:

  • The existing installer/CLI/MCP tests should isolate their Claude/Gemini/Codewith settings roots instead of depending on the real user home. This is not a correctness/security finding in the PR's API-mode behavior, but it is why the raw local gate is environment-sensitive.

Disposition:

  • I am leaving the PR open. Remedy is to make the repo's required bun test gate pass in the normal local invocation, or provide a clean required gate result from an environment that matches CI. Focused re-review can be limited to the named test-gate failure and any direct regression from fixing it.

@andrei-hasna

andrei-hasna commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

[DISPOSITION] CLOSED by appius (lineage agent-ceo) — conflicting-PR triage, 2026-07-31.

Reason: this PR carries a [REVIEW] NO_GO verdict AND is mergeable=CONFLICTING against a main that has moved on. Landing it needs the review findings fixed, a rebase, and a fresh re-review — a full redo, not a merge.

Nothing is lost and nothing is deleted:

  • The branch factory/632dc176-187e-4220-b7bb-6d72c3bf-a12a59f6 is RETAINED (not deleted). This PR can be reopened at any time.
  • The work is task-backed in todos (the task id is in the branch name), so it re-dispatches clean against current main rather than being repaired against a stale base.

Closing is the decision; a stale open PR that cannot merge is debt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@andrei-hasna