From ec9a33c0f6a872e2c36ce6f20a96d531ecc10875 Mon Sep 17 00:00:00 2001 From: bravew Date: Tue, 13 Oct 2026 20:27:16 -0400 Subject: [PATCH 01/16] docs(maintenance): add regression review and cleanup plan Recent feature PRs merged while the automatic quality and test jobs were skipped, so contracts such as FFmpeg publication, caption schema validation, chat-dock handoff, and permission precedence reached main without an integration check. The review records what is verified, what stays conditional, and which PRs should have been held, so cleanup starts from evidence rather than file size. Contributor blocking is defined as an evidence-based policy. The blocklist stays empty until attribution and review gates can support it. --- doc/README.md | 4 + .../2026-10-13-regression/01-findings.md | 89 ++++++++++ .../02-development-plan.md | 162 ++++++++++++++++++ .../03-lessons-and-contributors.md | 106 ++++++++++++ .../2026-10-13-regression/README.md | 48 ++++++ 5 files changed, 409 insertions(+) create mode 100644 doc/maintenance/2026-10-13-regression/01-findings.md create mode 100644 doc/maintenance/2026-10-13-regression/02-development-plan.md create mode 100644 doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md create mode 100644 doc/maintenance/2026-10-13-regression/README.md diff --git a/doc/README.md b/doc/README.md index be341e76..1c5b8e67 100644 --- a/doc/README.md +++ b/doc/README.md @@ -14,6 +14,10 @@ This is the architecture documentation for a cross-platform desktop AI agent app The documentation has been split into focused, topic-based pages for easier reading and maintenance. Each page covers a specific domain and can be read independently. +## Maintenance and regression reviews + +- [2026-10-13 regression review and development plan](maintenance/2026-10-13-regression/README.md): verified findings, cleanup checkpoints, PR lessons, and contributor quality policy. + ## Documentation Map ### System Overview diff --git a/doc/maintenance/2026-10-13-regression/01-findings.md b/doc/maintenance/2026-10-13-regression/01-findings.md new file mode 100644 index 00000000..5784740a --- /dev/null +++ b/doc/maintenance/2026-10-13-regression/01-findings.md @@ -0,0 +1,89 @@ +# Findings and evidence + +Baseline: `c0926618`, reviewed 2026-10-13. Line numbers below refer to that commit. P0 means urgent containment, P1 means fix before expanding the affected feature, and P2 means planned cleanup. “Confirmed” means the stated code behavior is visible in source or a recorded probe; it does not mean an exploit or full application scenario was executed. + +## Act on: security and execution boundaries + +| ID | Priority and evidence | Failure scenario | Fix and regression proof | +| --- | --- | --- | --- | +| S1 | P0, confirmed source chain. [http-daemon.ts](../../../src-api/src/http-daemon.ts):478; [jwt.ts](../../../src-api/src/app/middleware/jwt.ts):12; [sandbox.ts](../../../src-api/src/app/api/sandbox.ts):179 | HTTP listener omits `hostname`; Node listens on unspecified interfaces. Ordinary routes skip JWT unless `WEBUI_AUTH=true`. `/sandbox/exec` accepts a native provider and executes on the host. A network client able to reach the port can reach host-execution functionality in the default unauthenticated mode. Actual reachability depends on firewall and launch configuration; no remote exploit was attempted. | Bind local mode explicitly to loopback, make remote mode explicit and authenticated, and protect privileged operations. Test socket binding and unauthenticated/authenticated requests against an isolated daemon. Preserve authorized remote-access behavior. Inherited from the initial snapshot; #27 moved the listener rather than creating this policy. | +| S2 | P1, confirmed. [files.ts](../../../src-api/src/app/api/files.ts):1085,1188,1204 | File and editor open routes interpolate paths into shell command strings. Double quotes do not make filenames literal shell arguments. A permitted filename can cause command interpretation. | Use shell-free executable/argument calls or a platform opener. Test quotes, spaces, metacharacters, leading dashes, and platform-specific paths with process invocation mocked. Inherited. | +| S3 | P1, confirmed. [claude sandbox](../../../src-api/src/extensions/sandbox/claude.ts):249-267 | Script dependency names are joined into pip/npm command strings and executed outside the sandbox. Shell metacharacters and package install hooks can execute on the host. | Validate package specifications, remove shell interpolation, and keep installation/build hooks inside the intended execution boundary. Argument arrays alone do not isolate Python build backends or package lifecycle scripts. Inherited. | +| S4 | P1, confirmed code gap. [FFmpeg handoff](../../../src-api/src/shared/services/ffmpeg-skill/handoff.ts):284-287,328-332 | Publication chooses a deterministic destination and uses ordinary `copyFile`. A second task/design output can replace the first. A task output symlink can redirect the final write after the runner validated its staged output. | Exclusive output creation, unique allocation, real-parent containment and symlink protection at publication, plus cleanup on persistence failure. Test repeat publication, existing files, output-directory/file symlinks, and source-byte preservation. Introduced in #175. | +| S5 | P1, confirmed missing wiring. [ffmpeg-skill-server.ts](../../../src-api/src/shared/mcp/ffmpeg-skill-server.ts):122-129; [runner.ts](../../../src-api/src/shared/services/ffmpeg-skill/runner.ts):139-160 | The MCP handler never passes a cancellation signal into the runner. Direct runner tests supply one themselves. A cancelled request can continue processing and reach publication. | Carry the transport/run abort signal through execution and check it again before publication. Test the real handler/bridge boundary, child-process termination, and absence of output/file records after Stop. Introduced in #175. | +| S6 | P1, confirmed precedence. [permissions.ts](../../../src-api/src/extensions/agent/claude/permissions.ts):413-420 | Registry `deny` is respected, but a pre-approved tool returns allow even when an explicit user rule evaluated to `ask`. | Distinguish explicit ask from default classification, then enforce the agreed priority. Test allow/ask/deny crossed with pre-approved/not-pre-approved and interactive/headless runs. #90 introduced this path; #101 repaired separate headless gaps. | +| S7 | P1, confirmed policy gaps. [channel-manager.ts](../../../src-api/src/shared/channels/channel-manager.ts):1922-1942; [media.ts](../../../src-api/src/extensions/agent/openai-compat/media.ts):68-86 | MCP/provider-returned URLs reach raw fetch. The channel path has a synchronous URL pre-check, follows redirects, and buffers the body before enforcing size. The image path has no visible timeout or byte bound. | Use policy-bound `safeFetch` with DNS/redirect protection and streaming byte limits. Test redirects to denied networks, private resolved addresses, cancellation, and oversized bodies. Do not rewrite fixed-provider/local integrations indiscriminately. | +| S8 | P2, confirmed defense gap. [external MCP auth](../../../src-api/src/shared/services/external-mcp/auth.ts):30-32 | Missing remote-address metadata is accepted as loopback. Bearer-secret verification still applies, so this is not an unauthenticated MCP bypass. | Fail closed on absent socket metadata in production and supply explicit request context in tests. Verify valid loopback+secret, missing peer, non-loopback, missing secret, and invalid secret. | +| S9 | P1, confirmed build integrity gap. [API build](../../../src-api/scripts/build.mjs):246-275 | The binary build downloads/extracts a native SQLite addon without a reviewed digest. Cache reuse checks existence and does not include package version in its key. A changed asset or stale/poisoned cache can enter a sidecar build. | Version/digest-keyed cache, checksum validation before extraction and reuse, bounded download, fresh extraction directory. Test tampered archive/cache rejection without network access. Inherited. | + +Additional security work remains scoped to checkpoint 2. The file API containment helper at `files.ts:92` uses a lexical prefix and later reads can follow symlinks. Preserve its legitimate home/external-volume roots while checking resolved boundaries. MCP OAuth discovery in `shared/mcp/remote-oauth.ts` and provider media downloads in `shared/mcp/media-server.ts` need the same redirect/DNS review as S7. Tauri's `csp: null` amplifies the impact of any frontend injection but does not prove an XSS entry point. Treat CSP and capability narrowing as compatibility-tested hardening. + +The security pass found no malicious behavior in the inspected skill payload or hooks. FFmpeg payload hashes matched the checked-in provenance; upstream bytes were not independently fetched. All 63 inspected GitHub Action references were full SHA pins, with no active `pull_request_target` workflow found. A declared development signing key is intentionally untrusted; it is not evidence of a leaked production credential. Optional install analytics permission for `@scarf/scarf` should be reviewed as an explicit dependency policy choice, not labeled malware. + +## Act on: recent behavior regressions + +| ID | Priority and evidence | Failure scenario | Required test | +| --- | --- | --- | --- | +| R1 | P1, confirmed schema rejection. [timeline-schema.ts](../../../packages/video-ir/src/timeline-schema.ts):483; [timeline-types.ts](../../../packages/video-ir/src/timeline-types.ts):305; [video route](../../../src-api/src/app/api/video.ts):697 | Caption types/compiler output include `words` and `sourceAnchor`; the strict caption schema omits them. Valid generated caption snapshots fail operation/API validation. The reviewer reproduced rejection with an in-memory schema probe. #187. | Round-trip generated captions through the actual operation schema and HTTP request, persist/reload, then undo/redo without dropping timing or source anchors. | +| R2 | P2, confirmed remount logic. [SourceTranscriptPanel.tsx](../../../src/components/video/transcript/SourceTranscriptPanel.tsx):80,134 | Parent key changes remount `SourceWords`. Its ref starts with the new transcript key, so the effect intended to clear the old global word selection returns early. #187. | Replace a transcript for the selected source, assert selection clears; retain selection when mounting the same transcript. | +| R3 | P1, source-backed integration gap. [ChatDock.tsx](../../../src/components/chat-dock/ChatDock.tsx):112-122,256; [InitialMessageSender.tsx](../../../src/components/task/InitialMessageSender.tsx):152 | A task is persisted before execution, while the first prompt handoff is component state. Closing/switching before lazy thread connection or opening the full view can leave an unstarted task. #155. | Delay thread connection, send once, close/reopen and navigate to full view; assert exactly one user message and one run. | +| R4 | P2, source-backed event race. [OutcomeAutomationLists.tsx](../../../src/components/automation/OutcomeAutomationLists.tsx):35; [ChatDock.tsx](../../../src/components/chat-dock/ChatDock.tsx) | Two synchronous events open a closed dock and send scope before its listener mounts. The scope handler also omits the selected category. #155. | Open each category from a closed dock and submit; assert category and labeled answers survive into the run. | +| R5 | P2, confirmed overwrite. [OnboardingFlow.tsx](../../../src/components/onboarding/OnboardingFlow.tsx):51 | Completion unconditionally saves `planMode: 'on'`, overwriting existing `off`/`auto` choices during setup reruns. #155. | Rerun setup with each existing setting; preserve explicit choices and test fresh-install defaults separately. | +| R6 | P2, confirmed baseline test isolation. [resolve.test.ts](../../../src-api/test/unit/agent-runtimes/resolve.test.ts):239-251; [resolve.ts](../../../src-api/src/shared/agent-runtimes/resolve.ts):124 | Test expects a fixed OpenCode fixture despite real Homebrew binaries taking precedence. `pnpm test:fast` fails on this host. | Use existing path discovery injection, test npm-only discovery and Homebrew precedence independently. Inherited from `83c425bb`. | + +## Consider: unresolved requirements and architecture debt + +These are follow-up investigations, not deletion or implementation orders. + +- **Restore semantics.** `packages/video-ir/src/transcript-edit.ts:366` rejects restore when an unrelated empty caption track is present. The reviewer reproduced the difference. Confirm whether this conservative rule is intended before narrowing it. +- **Translation budgets.** `src-api/src/shared/video/caption-translate.ts:483-490` documents a deliberate choice: translation runs on the already-selected Video Mode model with no separate spend approval or budget debit. That may be correct. Confirm the user-facing claim matches this choice, then either keep the documented behavior with a test or add the agreed approval/accounting path. +- **Fork cost.** `branch-sdk-session.ts:134` forks from a parent; `claude/stream-signals.ts:516` settles against the new session's baseline. Double-counting is conditional on SDK fork cost semantics. Reproduce with a controlled SDK fixture/live trace before labeling a confirmed billing defect. +- **Migration drift.** `shared/db/migrations/utils.ts:20` does not distinguish a missing table from a missing column. #44 repaired `video_intent_log` drift and added full-registry regression coverage. Audit other ALTER callers against supported upgrade histories. Silently skipping every absent table would hide corruption and is not the fix. File numbering and migration versions already differ; do not renumber applied migrations for cosmetic consistency. +- **Tool typing.** `shared/mcp/video-edit-server.ts:2562-2625` resolves tool references through `args: never` and broad casts. #102/#103 repaired test compilation but left this type boundary hard to use. Introduce a typed wrapper with valid-input compile checks and intentional invalid-input runtime tests. Keep test casts only where they deliberately exercise an invalid boundary. +- **App-data isolation.** `shared/services/git-workspace.ts:27-30` derives app-owned repositories/sessions from `homedir()`; `publish/destinations/local-archive.ts:30` defaults to the old brand directory. Trace callers and injected roots, then test `NEUMAR_APP_DATA_DIR`. Plan compatibility for existing archives before changing paths. +- **Unsupported routes.** `/plugins/:id/apply` with `surface: 'chat'` returns 501; `/memory/entities/extract` and `/memory/consolidate` return 501 and explicitly direct callers to supported alternatives. Trace UI/external callers before deciding whether to implement, hide, document, or retire them. Platform-native dialog 501 responses can be correct capability behavior. +- **Deferred SDK requirements.** #96 explicitly deferred ambient-task filtering; profile delegates receive `omitClaudeMd: true`. Confirm coding-profile instruction policy and track deferred behavior separately from the completed timeout work. + +## Dead-code candidates and live compatibility paths + +The reachability review used static imports, literal dynamic imports/requires, and targeted searches across consumers. Knip emitted leads but exited 2 while resolving `vite.config.ts`/`pdfjs-dist/package.json`. Its 516 file candidates are not a trustworthy deletion count. Manifest-loaded design-system data and branding templates were major false positives. Never delete these from a generic unused-file report. + +| Candidate batch | Evidence and safe action | +| --- | --- | +| Old root app island | `src/app/App.tsx` imports `components/home/TaskInput.tsx`, which imports `AgentMessages.tsx`. Live entry uses `src/main.tsx` and the router. Reviewer found no reachable importer for the island. Recheck build/test/dynamic entry points and delete it as one small batch. | +| Gateway settings island | `components/settings/tabs/GatewaySettings.tsx` and its private gateway panels have no settings-host/navigation consumer in the reviewed graph. Confirm route/feature parity before deleting UI; this is not permission to delete backend gateways. | +| API orphan candidates | `extensions/mcp/sandbox-server.ts`, `core/config/hierarchy.ts`, `shared/services/pipeline/cli-dispatch.ts`, and `shared/services/video-analysis.ts` had no callers in the review. Check CLI/build/plugin entry points by exact path, then remove independently. | +| Test-only modules | Session-budget, delegation, artifact-events, MCP shim, and some provider/plugin helpers are reached only by tests or dormant code. Establish whether integration was abandoned or never completed. Delete tests only with a documented retirement decision; test-only is not synonymous with useless. | +| Dependencies | Root `@assistant-ui/react-markdown`, `@copilotkit/shared`, `tw-animate-css`, `vite-plugin-static-copy`, and API `@types/pdf-parse` are candidates. Check CSS, configs, generated builds, peer/binary usage, and workspace manifests before changing the lockfile. | +| Allowlist entries | All 68 files exist. Nine actual files now fit the 350-line default: ExportsDrawer, NewProjectPanel, ProjectChatSidebar, DesignSystemsTab, ProfileDetailSidebar, ConfigureStep, DesignModeSettings, RemotionTimelineComposition, StepPreviewCanvas. Remove obsolete allowances after measuring with the checker's own line-count convention. | + +Both `/task/:taskId` and `/task-v2/:taskId` are mounted at `src/app/router.tsx:57,65`. The V1 page is not dead even if in-app links prefer V2. Old bookmarks, API flows, and test clients need a retirement decision and redirects/parity proof. Both settings shells are live because `ui.simpleShell` defaults false at `src/shared/db/settings.ts:1570`. The manager, gateway registries, and legacy Slack routes all have live consumers. Social publishing adapters, the inactive egress proxy, and dormant DeepAgents code require explicit retirement/revival decisions. DeepAgents is deliberately excluded from public exports and built-ins at `src-api/src/core/agent/index.ts:119,185`; it is not an advertised working adapter. + +Locale deletion follows proven caller removal across all six locales. Dynamic keys must be checked before removing an apparently unused translation. + +## Prior architecture review: what persisted + +The June audit's hotspots still exist. Historical sizes below come from that local audit; present sizes were measured with `wc -l` at HEAD. Growth is a prioritization signal, not a correctness finding. + +| File | June reported lines | Current lines | +| --- | ---: | ---: | +| `src-api/src/shared/mcp/video-edit-server.ts` | 3,330 | 7,149 | +| `src-api/src/app/api/video.ts` | 3,248 | 5,200 | +| `src-api/src/extensions/agent/claude/index.ts` | 6,362 | 6,626 | +| `src-api/src/app/api/design.ts` | 4,814 | 5,068 | +| `src/shared/hooks/useAgent.ts` | 2,973 | 3,011 | +| `src/components/task/RightSidebar.tsx` | 1,675 | 838 | + +The sidebar reduction is progress. The allowlist also shrank from 69 to 68 entries; do not claim uniformly increasing debt. The current size gate only scans frontend components, leaving API routes, hooks, and stores outside its coverage. Prefer extraction along verified responsibilities to introducing a new universal engine during regression work. + +Tracked docs still claim browser IndexedDB fallback in `doc/system/overview.md:41,119` and `doc/frontend/state-management.md:25,88`, while the current frontend database abstraction uses the API-backed store. They also list Vite 7/TypeScript 5.8 and Node 20 build targets. Update these from actual manifests, database call paths, and CI, not by changing date/version headings alone. + +## Dismissed or not established + +- Size, zero review records, a non-conventional title, casts, or a grep match alone do not establish a defective PR or malicious intent. +- Empty-string defaults, raw fetch, home-directory calls, prefix checks, and cleanup catches each have legitimate uses. Review invalid-state masking and trust boundaries, not blanket substitutions. +- React lint warnings identify review work, not 453 independently reproduced defects. The baseline is 254 set-state-in-effect, 137 refs, 22 static-components, 16 exhaustive-deps, 14 preserve-manual-memoization, 6 incompatible-library, 2 purity, and 2 immutability warnings. +- #102's apparent bulk is mostly a newly tracked Cargo.lock; stacked PR #103 statistics include earlier work. Use squash-commit diffs to attribute changes. +- #176 retains its meaningful failure assertions. Adding namespaces to a translation mock and an explicit undefined call argument is not assertion weakening. +- `getByText(...).toBeTruthy()` can be meaningful because `getByText` throws on absence. Route tests mocking service dependencies are not necessarily mocking their own subject. +- No full reachability proof, exploit exercise, packaged-app run, live SDK cost test, or complete supply-chain audit was performed. Suspected issues remain separate until their proposed regressions establish them. diff --git a/doc/maintenance/2026-10-13-regression/02-development-plan.md b/doc/maintenance/2026-10-13-regression/02-development-plan.md new file mode 100644 index 00000000..30f13cd8 --- /dev/null +++ b/doc/maintenance/2026-10-13-regression/02-development-plan.md @@ -0,0 +1,162 @@ +# Development plan + +Status: ready for checkpoint review, not yet implemented. Baseline `c0926618`. Finding IDs refer to [the evidence register](01-findings.md). The first objective is containment and reliable validation; deletion and restructuring follow verified caller maps. + +## Throughput and ownership + +1. **Blocking first steps.** Record the baseline and repair the host-dependent test in checkpoint 1. Security containment in checkpoint 2 should begin immediately in parallel; do not wait for a broad cleanup to fix S1. Required repository checks must be made executable before making them merge requirements. +2. **Independent workstreams.** CI/test infrastructure, API network/process boundaries, FFmpeg lifecycle, transcript contracts, and shell/onboarding fixes can be investigated in parallel. Keep each implementation PR narrow and independently testable. +3. **Shared mutable state.** Serialize edits to `package.json`, `pnpm-lock.yaml`, locale modules, the size allowlist, `http-daemon.ts`, `video-edit-server.ts`, and common video IR schemas. Assign one owner per shared file during each batch. Rebase and rerun affected integration tests after another stream changes a shared contract. +4. **Smallest safe decomposition.** Seven checkpoints below; several contain small independent PRs. No single “cleanup everything” PR. No new framework, dependency upgrade, or generic abstraction is required just to start this work. + +## 1. Establish a reproducible merge gate + +**Likely files:** `.github/workflows/ci.yml`, existing workflow checks, root/API package scripts and configs, `src-api/test/unit/agent-runtimes/resolve.test.ts`, repository rulesets through a separately approved settings change. + +**Work:** + +- Isolate runtime discovery tests through the existing `PathDiscoveryOptions` seam. Cover npm-only discovery and Homebrew precedence independently. +- Run quality and test jobs on ordinary non-draft PRs and the final integration branch. Include dependency-bot changes; do not silently exempt the very changes that can alter behavior. +- Run `pnpm validate`, API lint, and API test-file typechecking. Add changed-surface gates for video IR and Rust. Ensure required jobs report a stable aggregate result even when a workspace-specific job is skipped legitimately. +- Use baseline-aware checks for existing lint/size debt. Reject new suppressions or warning growth in changed code unless a reviewed exception has an owner, reason, expiry, and regression coverage. Avoid turning all existing warnings into an immediate unrelated cleanup requirement. +- After proving check names and triggers on a harmless PR, propose required status checks and an independent review rule. Repository settings are not changed by this plan. A solo-maintainer exception must be explicit and retain review evidence, rather than pretending self-review is independent approval. + +**Observable result:** a feature PR cannot appear ready while quality/test jobs are absent or skipped; local and CI test discovery use the same workspace scripts. + +**Verification:** + +```bash +pnpm test:api test/unit/agent-runtimes/resolve.test.ts +pnpm validate +pnpm --filter neumar-api lint +pnpm --filter neumar-api exec tsc -p tsconfig.json --noEmit +pnpm test:fast +pnpm test:gate +``` + +Inspect Actions on the exact PR head. Deliberately introduce a reversible failure on a temporary validation branch to prove the required aggregate blocks merge, then remove it and confirm success. Use actionlint for edited workflows with pinned Actions and quoted env inputs. Do not merge the failing probe. + +**Exit:** R6 fixed; repeatable check results recorded with Node/pnpm versions; gate and ruleset proposal reviewed. Record unavailable credentials/platform checks explicitly. + +## 2. Contain host execution and external I/O + +**Likely files:** `src-api/src/http-daemon.ts`, `app/middleware/jwt.ts`, `app/api/{sandbox,files}.ts`, `extensions/sandbox/claude.ts`, `shared/network-policy/`, identified URL consumers, `shared/services/external-mcp/auth.ts`, `src-api/scripts/build.mjs`, Tauri configuration where necessary. + +**Work:** + +- Address S1 first in a small compatibility-tested change. Default daemon mode binds loopback; explicit remote mode requires authentication before privileged routes are available. Account for packaged desktop and intended WebUI clients. +- Remove shell interpolation from file opening and sandbox package operations, keeping package execution inside the stated isolation boundary. +- Fix actual filesystem containment gaps with existing policy helpers and operation-specific trusted roots. Handle symlinks and nonexistent write targets; do not reduce legitimate desktop roots to a blanket workspace-only rule. +- Move untrusted downloads and OAuth discovery to the existing network policy, with DNS-bound connections, redirect checks, timeout and byte limits. Preserve intentionally local integrations through explicit policy. +- Fail closed on missing MCP socket metadata and verify native binary cache/download integrity. Stage CSP/capability changes separately if they affect supported preview workflows. + +**Observable result:** default launch exposes no unauthenticated LAN host-execution surface; filenames remain literal; sandbox preparation stays isolated; untrusted targets cannot bypass network policy; downloaded native code has verified provenance. + +**Verification:** focused route, sandbox, network-policy, external-MCP and file-policy tests through `pnpm test:api `; isolated real daemon socket/auth smoke; mock opener assertions for hostile filenames; symlink/redirect/private-address/oversize tests; tampered download/cache fixtures; then `pnpm validate`, API lint and full API typecheck. For Tauri changes run `cargo fmt --manifest-path src-tauri/Cargo.toml --check`, `cargo check --manifest-path src-tauri/Cargo.toml --locked`, relevant Cargo tests, and a packaged/local desktop smoke. Never test against the user's active data directory. + +**Exit:** S1-S3 and verified containment/network/build-integrity findings fixed or explicitly held with owner and evidence. No exploit payloads or credentials committed into public documentation. + +## 3. Repair FFmpeg publication, cancellation, and permission priority + +**Likely files:** `shared/services/ffmpeg-skill/{runner,handoff,policy}.ts`, `shared/mcp/ffmpeg-skill-server.ts`, relevant bridge/run cancellation wiring, `extensions/agent/claude/permissions.ts`, corresponding API tests. + +**Work:** + +- Allocate unique, exclusive final outputs inside the owner's validated root. Apply checks at final publication, not only staging. Persist file/output records only after successful publication; define cleanup on DB failure. +- Connect Stop to the process supervisor through MCP and bridge execution. Prevent delayed publication after cancellation, including cancellation between process completion and handoff. +- Preserve explicit deny/ask policies over general pre-approval. Represent explicit ask separately from default classification if the current result type loses that information. + +**Observable result:** repeating an operation cannot replace previous/source files; Stop ends processing and creates no deliverable; explicit permission rules are respected in interactive and headless runs. + +**Verification:** real FFmpeg fixture tests for repeated outputs and cancellation; handler-level/bridge tests with delayed children; destination symlink/collision tests; permission matrix including allowedTools + explicit ask/deny. Run targeted API tests, `pnpm check:ffmpeg-skill`, API lint/typechecks, `pnpm validate`, then relevant broader tests. Preserve the vendored payload and pin unless a separately reviewed upstream fix is required. + +**Exit:** S4-S6 closed with tests that fail on the reviewed baseline and pass after the fix. Direct runner unit tests alone do not satisfy cancellation acceptance. + +## 4. Restore transcript and shell integration contracts + +**Likely files:** `packages/video-ir/src/{timeline-schema,timeline-types,transcript-edit}.ts`, transcript UI/store and API route tests, `ChatDock.tsx`, `InitialMessageSender.tsx`, `OutcomeAutomationLists.tsx`, `OnboardingFlow.tsx`, relevant locales. + +**Work:** + +- Align caption runtime schemas with the typed/compiler contract; test the real API operation boundary and persisted reload. +- Invalidate selection when source transcript identity changes without losing valid selection merely because the panel mounts. +- Make initial prompt delivery durable and exactly once across dock close, slow load, navigation, retries, and reopened tasks. Use existing run/message contracts rather than adding a second execution path. +- Carry automation category and labeled scope answers through dock mounting and submission. +- Preserve explicit plan-mode settings on onboarding reruns. Apply fresh-install defaults only when no explicit preference exists. +- Resolve empty-caption restore and translation-budget decisions before implementing their behavior changes. Reproduce fork-cost behavior separately before changing accounting. + +**Observable result:** generated captions can be edited/saved/reloaded, transcript replacement cannot edit old indices, the dock cannot lose or duplicate a send, automation retains scope, and setup preserves user choices. + +**Verification:** workspace-specific frontend/API regression files; `pnpm --filter @neumar/video-ir test` and `validate`; `pnpm validate`; API test typecheck; `pnpm test:fast`. Add a focused browser scenario for dock lifecycle and transcript editing against an isolated temporary API, and explicitly run it using the project's Playwright configuration. Seeding a transcript is acceptable for editing tests but is not evidence of working live ASR or translation. + +**Exit:** R1-R5 closed; intentional exclusions and live-provider checks recorded. Six-locale parity holds for any new UI text. + +## 5. Remove proven orphans and obsolete allowances + +**Likely files:** the dead-code candidate islands in `01-findings.md`, tests/locales used exclusively by retired code, `scripts/component-size-allowlist.json`; manifests/lockfile only after a dependency is proven unused. + +**Work:** + +- Build a checked inventory of static/dynamic imports, registration, CLI/build entry points, desktop consumers, and plugin manifests. Start with the old root app island and a small API orphan batch. +- Remove each proven island and its exclusive tests/exports/locales in one coherent PR. Do not use Knip's incomplete scan or untracked WIP as deletion authority. +- Remove the nine allowances whose files already satisfy the default limit. Ratchet remaining ceilings downward when extraction genuinely reduces a file. +- Evaluate dependency candidates after caller removal. Preserve lockfile resolution for unrelated packages and check packaged assets/peer/binary use. +- Treat test-only code as an implementation/retirement question. Keep the live V1 route, both shells, channels, publish feature gates, and extension APIs until their contracts are decided. + +**Observable result:** each deletion has a verified former entry graph and no dangling runtime/locale/config reference; ordinary files no longer receive unnecessary size exemptions. + +**Verification:** exact-path/symbol searches across all workspaces and registration/config sources; affected tests; production frontend/API builds for changed entry graphs; locale parity, plugin registry, component size and `pnpm validate`; inspect generated/build diffs. Compare exported tool/route inventories where relevant. Do not commit generated files incidentally. + +**Exit:** a small documented removal batch passes checks. Counts of deleted lines are secondary to preserved behavior. + +## 6. Resolve architecture ownership and reduce the highest-risk modules + +**Likely files:** channel/runtime contract docs, `core/agent/`, `shared/channels/`, `shared/services/gateway/`, legacy Slack wiring, video tool wrapper and schemas, DB migration utilities/tests, shared catalogs, selected hooks/components, tracked `doc/` architecture pages. + +**Work:** + +- Produce a channel responsibility matrix covering each provider, inbound webhook, credentials, user identity, permissions, persistence, scheduling and stop behavior. Choose a canonical runtime only after reviewing this matrix. +- Decide V1 task compatibility and unsupported-route visibility. Where a feature is deliberately unavailable, make the capability response/UI truthful. Keep revival work separate from deletion work. +- Fix the video reference wrapper's generic type contract before extracting tool groups. Migrate callers and remove the superseded internal helper in the same PR. Preserve tool names and wire schemas. +- Audit supported migration upgrade histories and missing-table behavior. Add drift fixtures that execute the complete registry; preserve applied version identities. +- Split modules by current responsibility, starting with tested seams in `video-edit-server.ts`, the Claude adapter, or `useAgent`. Shared catalog extraction is warranted only when it removes a real duplicated contract and has no runtime-specific dependencies. +- Update architecture docs from current code, including storage, entry points, supported runtimes, and check coverage. Historical audit recommendations must not be described as implemented architecture. + +**Observable result:** each live subsystem has an explicit owner and contract; selected type/upgrade hazards are covered; one large module is smaller without a second compatibility implementation left behind. + +**Verification:** old/new route, tool and event behavior comparison; typed valid/invalid input checks; channel contract fixtures and approved live-provider smoke where available; full migration registry tests on fresh/current/drifted snapshots; source/runtime tests and `pnpm validate`; review docs links against actual call paths. For unavailable provider credentials state exactly what remains unverified. + +**Exit:** reviewed architecture decisions and at least one independently verified extraction. No broad rewrite is justified solely by file size. + +## 7. Make maintenance recurring and contributor quality reviewable + +**Likely files/settings:** `doc/maintenance/`, contribution/PR templates if adopted, existing validation scripts, narrowly scoped CI reports, repository rulesets after separate approval. + +**Work:** + +- Every PR records intent, affected contracts, regression evidence, exact tested commit, unverified cases, and tool/session attribution when known. A model name in product code does not identify the code author. +- Every corrective PR records the introducing change if proven, failing scenario, why the existing gate missed it, and the prevention test/check. Link the original finding and close it only when verified. +- Once per week, and before each release, review new warnings/suppressions, size-ceiling increases, orphan candidates, unsupported feature surfaces, migration changes, security findings, and unresolved regressions. Publish a small repository report and select bounded cleanup PRs. Scheduling this process is a separate implementation action; this plan does not claim a live recurring job. +- Apply the probation/block policy in `03-lessons-and-contributors.md` only to verified, attributable repeated defects. Keep appeals, expiry, and remediation criteria. Treat malicious intent as a separate evidence question. +- Measure escaped regressions, time to repair, required-check execution, unresolved high-priority findings and expired exceptions. Do not reward deletion count or punish every follow-up fix. + +**Observable result:** maintenance has an owner, cadence, open findings, verification artifacts and expiring exceptions; repeated quality failures lead to reviewable action rather than unsupported accusations. + +**Verification:** replay the policy against the documented historical cases, ensuring stacked PRs/manual CI/different authors are handled correctly; test changed checks with clean and failing fixtures; inspect attribution/privacy and blocklist schema; verify reports contain source/PR links and exact check outcomes. + +**Exit:** agreed cadence and owner; enforceable gate definitions; policy accepted with no speculative entries. + +## Open decisions before dependent implementation + +- Is remote HTTP operation required by default launchers, or only explicit remote mode? Inventory callers first; default loopback is the proposed containment. +- Should explicit `alwaysAsk` override run pre-approval? Recommended yes, consistent with explicit user choice. Preserve explicit deny regardless. +- Should transcript restore ignore unrelated empty caption tracks, and which translation operations consume project budgets? +- Retire `/task/:taskId` with tested redirects, or maintain V1 compatibility? Both routes are currently live. +- Keep, implement, or retire test-only services, social publishing adapters and dormant plugin APIs after caller/consumer review? +- Who owns weekly maintenance and security follow-up, and what solo-maintainer review exception is acceptable? + +The user selected contract mapping before channel consolidation and visibility review before changing placeholder features. Those decisions permit the investigations above; they do not authorize silently choosing a new channel runtime or implementing all stubs. + +## Plan readiness + +The checkpoints define observable results and verification. Start with checkpoint 1 and urgent S1 containment after selecting implementation scope. Revise any checkpoint whose caller inventory changes its boundaries. Stop only the dependent branch of work when a behavior decision or unavailable platform/provider blocks it; continue independent authorized work. No checkpoint is complete just because its unit tests pass while its required integration boundary remains untested. diff --git a/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md b/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md new file mode 100644 index 00000000..326eb3aa --- /dev/null +++ b/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md @@ -0,0 +1,106 @@ +# PR lessons and contributor quality policy + +Reviewed 2026-10-13 against `c0926618`. This is a retrospective assessment of changes and gates. It does not infer malicious intent, identify an agent from product references, or recommend reverting large PRs wholesale. + +## What the merge system actually required + +The successful GitHub sample contained 84 merged PRs. The active `main` and `protect main` rulesets prevent deletion/force pushes and require PRs, but specify zero approving reviewers and no required status checks. Legacy branch protection returned 404; active rulesets still exist, so “main is completely unprotected” would be wrong. + +| Metric | Observed result | +| --- | ---: | +| PRs without an executed automatic quality/Test gate | 77/84 | +| PRs with zero check-rollup records | 0/84 | +| PRs with zero review records, including bots | 67/84 | +| PRs with zero human review records | 80/84 | +| Approving review records in the sample | 0 | +| PRs with a failure completed before merge on the matching final PR head | 7/84 | + +The automatic CI jobs in `.github/workflows/ci.yml` run only on release-style branches or manual dispatch. Every PR having check records therefore says little about quality/test execution. Manual validation also matters: #176 had a successful manual CI run on its exact commit despite skipped automatic jobs. Local testing described in a PR body is useful evidence, but it is not independently observed CI. + +The seven matching-head premerge failure cases were #176, #106, #26, #24, #3, #2 and #1. Four release PRs had a recorded quality or Test failure before merge. Reviewer/audit failures must be classified from their logs before being called code defects. This review compared check completion timestamps and run head SHAs; it did not reconstruct every historical rerun attempt. + +| PR | Premerge failed check evidence | Actions run | +| --- | --- | --- | +| [#176](https://github.com/bravew/Neumar/pull/176) | npm Audit; separate exact-head manual CI passed | [37505633375](https://github.com/bravew/Neumar/actions/runs/37505633375) | +| [#106](https://github.com/bravew/Neumar/pull/106) | Test | [36664980682](https://github.com/bravew/Neumar/actions/runs/36664980682) | +| [#26](https://github.com/bravew/Neumar/pull/26) | Lint, Types & Format; Test | [33110938945](https://github.com/bravew/Neumar/actions/runs/33110938945) | +| [#24](https://github.com/bravew/Neumar/pull/24) | Lint, Types & Format | [32870755627](https://github.com/bravew/Neumar/actions/runs/32870755627) | +| [#3](https://github.com/bravew/Neumar/pull/3) | npm Audit | [32331502583](https://github.com/bravew/Neumar/actions/runs/32331502583) | +| [#2](https://github.com/bravew/Neumar/pull/2) | Quality, review, audit; Test completed after merge | [32308922780](https://github.com/bravew/Neumar/actions/runs/32308922780), [32308922807](https://github.com/bravew/Neumar/actions/runs/32308922807), [32308922844](https://github.com/bravew/Neumar/actions/runs/32308922844) | +| [#1](https://github.com/bravew/Neumar/pull/1) | Review; npm Audit | [32217384426](https://github.com/bravew/Neumar/actions/runs/32217384426), [32217384317](https://github.com/bravew/Neumar/actions/runs/32217384317) | + +#104's audit failure completed after its merge and must not be described as an already-red merge. A current rollup alone cannot establish what was known at merge time. + +## PRs that should have been held + +“Hold” means address the stated defect or document an explicit acceptance decision before merging the affected change. It does not mean every line of the PR was bad. See finding IDs in [the evidence register](01-findings.md). + +| PR | Retrospective verdict | Evidence and missing validation | +| --- | --- | --- | +| [#187 transcript layer](https://github.com/bravew/Neumar/pull/187) | Hold for editing correctness | R1 caption schema rejects fields generated by the compiler; R2 replacement remount defeats selection clearing. Tests cover in-memory logic but miss the API/schema and replacement boundaries. A bot review raised schema drift without closing it. | +| [#175 FFmpeg skill](https://github.com/bravew/Neumar/pull/175) | Hold for execution/publication safety | S4 final publication bypasses the promised no-overwrite behavior; S5 cancellation never enters the handler's runner call. Direct runner/policy tests do not prove the entire MCP-to-publication lifecycle. Attachment/planning/error work broadened scope and caused the #176 test repair. | +| [#155 UX epic](https://github.com/bravew/Neumar/pull/155) | Hold for lifecycle/upgrade behavior | R3 initial prompt handoff is transient, R4 scope event can precede dock mount, R5 onboarding overwrites an existing preference. Component tests and locale checks do not cover close-before-connect, full-view navigation, or setup reruns. Both shells remain intentional opt-in paths. | +| [#90 permission changes](https://github.com/bravew/Neumar/pull/90) | Hold for complete policy precedence | S6 explicit interactive ask is bypassed by pre-approval. #101 repaired separate headless safety-hook omissions, showing the earlier safety claim was incomplete. Test explicit allow/ask/deny at the actual dispatch boundary. | +| [#93 SDK branches](https://github.com/bravew/Neumar/pull/93) | Hold for acknowledged branch persistence gap | Its branch-output-on-main issue needed #101. Cost inheritance remains a conditional follow-up requiring SDK evidence; do not count it as a second confirmed regression yet. | +| [#103 API test types](https://github.com/bravew/Neumar/pull/103) | Hold for a clean actual integration diff | Squash `10edc9f3` added a duplicate `toolResultText` import after #102 had already introduced it. #105 repaired formatting and API test type errors. A correct type-cleanup intent does not excuse a broken final branch. | +| [#25 durable video plans](https://github.com/bravew/Neumar/pull/25) | Hold for supported migration histories | Added another unguarded ALTER on `video_intent_log` where drifted installs may lack the table. #44 repaired the failure class. Earlier v89/v96 history is in the initial snapshot; avoid attributing every drift case to this PR. | + +#43 and #29 are review-depth concerns because of wide changes and skipped quality/test jobs. This review did not establish a specific new current defect in each solely from their size. Require additional inspection before saying either “should never have merged.” The same distinction applies to #23, whose review records and test work differ from unreviewed PRs. + +## Corrections worth preserving + +- **#102 is mostly a legitimate lockfile change.** The squash adds roughly 7,734 Cargo.lock lines to pin the desktop dependency resolution. Its size is not evidence of generated malicious code or a dishonest fix. +- **#103 is smaller than its stacked PR totals suggest.** The squash is about +486/-230 across 93 files. It adds test casts around existing handler typing, but no added `as any` or `ts-ignore`. Some casts deliberately exercise invalid runtime inputs. Review the source wrapper separately rather than calling all casts cheating. +- **#176 preserves the assertions.** It adds translation namespaces used by rendered children and the explicit undefined toast option. It does not remove the checks preventing a message/run when attachment staging fails. +- **#44 is a useful repair.** It added guards, fixed DB initialization behavior, and exercised the full registry on a drifted database. General missing-table semantics remain follow-up work. Do not remove that coverage because it is inconvenient. +- **Dependency security pins are not automatically hacks.** Inspected overrides document CVEs/compatibility constraints. Review resolved runtime consistency and lifecycle permissions, rather than treating all transitive overrides as suspicious. +- **Fixing a bug is responsible work.** An introducing defect can count toward quality review; the corrective PR itself should not receive another strike for repairing it. + +## Lessons turned into checks + +| Failure chain | Why validation was insufficient | Prevention to encode | +| --- | --- | --- | +| #175 → #176 | Translation mocks and toast call signatures drifted; normal feature CI skipped tests. | Execute frontend tests for feature PRs and final epic integration heads. | +| #102 → #103 → #105 | A stacked branch reapplied an existing import; actual final formatting/API test types were not clean. | Validate the final integrated SHA; include API format and test-file typecheck. Inspect squash diffs when attributing changes. | +| Initial migration assumptions + #25 → #44 | Upgrade fixtures did not cover drifted DB histories; missing-table ALTER stopped subsequent migrations. | Execute the full registry against fresh and supported drifted snapshots; assert initialization fails clearly rather than exposing partial state. | +| #25 → #35 | Audio capability helper returned boolean without narrowing an optional asset; API source typecheck failure reached main. | Required API source typecheck on every relevant PR. | +| Prior changes → PR #5 unblock commit `dd346af1` | Size ceilings drifted; settings background requests consumed call-order fetch mocks. | Required component check; route mocks by endpoint/purpose; isolate environment-specific Blob behavior. Ceiling increases need a real reviewed exception, not a pass-bar adjustment. | +| #90/#93 → #101 | Permission hooks and branch persistence were not tested across the full runtime integration. | Contract tests through dispatch/persistence, including headless/interactive and resumed/forked states. | +| Initial snapshot → current OpenCode baseline failure | PATH stubs left hardcoded host search roots live. | Inject filesystem discovery and assert precedence with controlled fixtures. | +| June architecture warnings → current large tool modules | The component-only size gate misses API tools, routes, hooks and stores. | Baseline-aware size/complexity review outside components; extract a tested responsibility instead of raising ceilings. | + +`pnpm validate` is not the test suite, API lint, API test-file typecheck, Rust validation, or video IR tests. CI also runs a smaller quality subset than the full root gate. “Validate passed” must always name the checks it actually represents. + +The current 453 frontend warnings pass the root gate. Track their baseline and reject newly introduced correctness warnings/suppressions in changed files, then retire existing ones in bounded batches. Avoid a bulk warning cleanup that changes behavior without regression tests. + +The AI review workflow excludes Markdown and dependency lockfiles from its default diff. A separate bounded supply-chain pass should include skill instructions, executable payloads, manifests, and resolution changes. Hash drift checks verify local consistency; they do not prove that the pinned upstream code is trustworthy. + +## Contributor policy selected for this plan + +The user selected an agent policy and attribution gate, with no speculative named blacklist. All sampled merged PRs use the same GitHub identity; tool/session attribution is incomplete. Commit sessions, coauthor trailers, and PR branches can help, but a reference to Codex/Claude in product code or a PR title is not authorship evidence. This review cannot fairly assign repeated defects to a named human, vendor, model family, or agent session. + +**Current blocklist: empty.** No GitHub access settings, bot permissions, or local agent configuration have been changed. The policy below is proposed enforcement work in checkpoint 7. + +### Attribution and incident records + +For every new contribution, record the human accountable for landing it, the authoring tool/session when known, intended scope, exact verified commit, and checks omitted with reasons. Store private session URLs/identifiers outside public documentation; public PR metadata may use an opaque local reference. Record `unknown` when attribution is unavailable rather than guessing. + +A quality incident record contains the introducing PR/commit, reproducible scenario, severity, review/check gap, attribution evidence, repair PR, verification, date, maintainer disposition, and appeal status. Two reviewers or a maintainer with an independent verification artifact confirm the incident before it counts. Attach code evidence, not opinions about style or individuals. + +### Proposed thresholds + +- **Ordinary review:** all contributors meet the same exact-head checks and contract-test requirements. Large diffs, missing attribution, warnings, or skipped checks hold a PR for evidence; they do not by themselves prove a defective author. +- **Probation:** two confirmed escaped material defects attributable to the same contributor/tool configuration within 90 days. Require smaller scoped PRs, independent review and explicit regression demonstrations for the next contributions. +- **Block from autonomous submission/merge:** three confirmed material incidents within 90 days, including at least one repeated failure class after its documented correction, or a verified deliberate bypass of required validation. A maintainer confirms identity, evidence, scope and expiry. This is a restriction on the demonstrated contributor/configuration, not an unsupported blacklist of an entire vendor or model family. +- **Security incident:** verified malicious behavior or credential disclosure gets immediate incident containment and a separate maintainer decision. A vulnerability alone does not prove intent. +- **Reinstatement:** repair outstanding incidents, demonstrate the failed contracts under the new gate, and pass three independently reviewed scoped contributions. Review a block after 30 days; renew it only with stated evidence. An appeal can correct attribution, severity, or causation at any time. + +Thresholds are policy proposals, not historical facts or automatic penalties. Do not count the same incident twice across an epic and its sub-PRs, blame corrective authors for preexisting defects, or count a transient infrastructure failure as low-quality code. Independent reviewers need access to the actual artifact and final check results. + +### Why the policy starts without names + +The code review can establish introducing changes and missing validation. It cannot reliably identify which agent instance authored each line, whether a human changed the result, or who accepted known tradeoffs at merge. Improve attribution and review gates first. Populate a blocklist only when the policy's repeated, verified incidents and maintainer decision exist. Each entry must include the evidence, affected scope, effective date, expiry, remediation and appeal link. + +## Remaining questions + +Which permission precedence, transcript restore/budget semantics, V1 compatibility, remote-mode contract and weekly owner should the implementation adopt? These are listed in [the development plan](02-development-plan.md). Channel ownership and unsupported-surface handling have already been scoped to contract/visibility review first. Document the answers with the implementing PRs so future agents see decisions beside the checks that enforce them. diff --git a/doc/maintenance/2026-10-13-regression/README.md b/doc/maintenance/2026-10-13-regression/README.md new file mode 100644 index 00000000..e6398153 --- /dev/null +++ b/doc/maintenance/2026-10-13-regression/README.md @@ -0,0 +1,48 @@ +# Regression review and maintenance plan + +Date: 2026-10-13. Reviewed baseline: `c0926618`, version 26.10.1. Status: review completed; remediation checkpoints proposed, not executed. This document records a sampled architecture and regression review, not certification of every file or dependency. + +## Start here + +1. [Findings and evidence](01-findings.md) records current defects, dead-code candidates, architecture drift, and verification limits. +2. [Development plan](02-development-plan.md) gives seven independently verifiable checkpoints, their dependencies, and decisions needed before implementation. +3. [PR lessons and contributor policy](03-lessons-and-contributors.md) records merge failures, validation gaps, and the evidence required for probation or blocking. + +The main problem is that changes can merge without exercising their contracts. Recent examples include FFmpeg publication and cancellation, transcript schema validation, chat-dock prompt handoff, and permission precedence. Earlier security boundaries also need review. Removing unused code will help, but it cannot substitute for fixing these paths and preventing another unchecked merge. + +## Baseline verification + +| Check | Result at the reviewed baseline | +| --- | --- | +| `pnpm validate` | Passed, exit 0. It emitted 453 frontend lint warnings. | +| Frontend portion of `pnpm test:fast` | 402 files, 1,789 tests passed. | +| API portion of `pnpm test:fast` | 594 files passed, 1 failed, 3 skipped; 3,860 tests passed, 1 failed, 7 skipped. Overall command exited 1. | +| `pnpm --filter neumar-api exec tsc -p tsconfig.json --noEmit` | Passed in the delegated validation review. Includes API tests. | +| `pnpm --filter neumar-api lint` | Passed with warnings in the delegated validation review. | +| `pnpm check:ffmpeg-skill` | Passed in the delegated recent-PR review. Local pin/hash integrity only. | +| Rust, packaged desktop, browser E2E, live providers, gate evals, independent upstream payload comparison | Not run in this review. | + +The failed API test is `test/unit/agent-runtimes/resolve.test.ts:251`. It expects the temporary npm-global OpenCode fixture, but the resolver finds the host's Homebrew OpenCode first. Source inspection confirms that the test uses the real filesystem despite stubbing HOME/PATH. Both the test and that search order originate in initial commit `83c425bb`. Fix test isolation and assert precedence separately; do not change production precedence to satisfy this fixture. + +The local runner used Node v26.11.1 and the pinned pnpm 11.22.0. Repeat the failing case under the supported Node 22 CI baseline when fixing it. Do not interpret the local failure as proof that production runtime discovery is broken. + +## Decisions made during this review + +- Use an evidence-based agent policy and attribution gate. No contributor names have been added to a blocklist. +- Map channel ownership, capabilities, permissions, persistence, and webhooks before proposing a canonical runtime. Preserve all live consumers during that work. +- Trace callers and visibility of unsupported API actions first. Hide misleading controls or document intentional capability limitations; feature implementation requires a separate behavior decision. +- Keep the review and plan in `doc/`. Existing local work is outside this deliverable. + +The requested recurring cleanup is defined as a maintenance process in checkpoint 7. No scheduler, repository setting, contributor access restriction, issue, or external post has been created by this documentation work. + +## Review coverage and prior architecture work + +Six bounded reviews covered dead-code reachability, recent feature PRs, older video/dependency PRs, architecture conventions, security/supply chain, and validation quality. The coordinator checked load-bearing source excerpts and corrected several overbroad scan conclusions. Codegraph was unavailable for this checkout; no index was installed or regenerated. Knip's exploratory run did not finish cleanly, so its totals are leads rather than an unused-code inventory. + +Prior material inspected includes the local June architecture audit at `dev-doc/Revamp/06-21/01-current-state-audit.md` and May channel rollout audit at `dev-doc/channels/rollout-audit-2026-05-03.md`, plus tracked architecture docs. Those local historical audits are not portable dependencies of this plan. Their observations were rechecked against current code where used. The June audit already identified runtime/tool duplication and large modules. Its proposed new shared engines are design ideas, not evidence that those engines now exist. + +No malicious code or live service credential was identified in the inspected areas. Security defects and missing controls are documented as defects, without claims about contributor intent. The static scans do not prove the absence of malicious dependencies, undiscovered vulnerabilities, or removed credentials throughout history. + +## Completion for this deliverable + +The documentation review is complete when the files and source links have been checked, the root gate result and test failure are reported, and only these docs are committed locally. Implementation is complete only after the relevant checkpoints have their own before/after regression evidence and verification. The current plan does not claim those fixes have landed. From 1bb20406de946723795f7f9fed20777b7fa00d58 Mon Sep 17 00:00:00 2001 From: bravew Date: Tue, 13 Oct 2026 21:28:11 -0400 Subject: [PATCH 02/16] docs(maintenance): add LESSONS.md and track the regression plan as epics LESSONS.md records what the 2026-10-13 review found merged work got wrong, as rules with evidence and a review-day count, so agents read the rule before repeating the mistake. AGENTS.md and CLAUDE.md point to it. The regression docs now cite reference practice for each fix, link the lessons, and index the seven epics (#188 to #194) and their 37 sub-issues. Each sub-issue is worked in its own worktree and merged into its epic branch, and the epic merges into main in one reviewed PR. --- AGENTS.md | 8 + CLAUDE.md | 1 + LESSONS.md | 243 ++++++++++++++++++ .../2026-10-13-regression/01-findings.md | 20 +- .../02-development-plan.md | 44 +++- .../03-lessons-and-contributors.md | 6 +- .../04-epics-and-issues.md | 143 +++++++++++ .../2026-10-13-regression/README.md | 9 +- 8 files changed, 461 insertions(+), 13 deletions(-) create mode 100644 LESSONS.md create mode 100644 doc/maintenance/2026-10-13-regression/04-epics-and-issues.md diff --git a/AGENTS.md b/AGENTS.md index 39107762..030beb53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,6 +22,14 @@ as one contributor's local setup, not shared project context. `.cursor/` is trac but currently holds only [codacy.mdc](.cursor/rules/codacy.mdc); shared rules belong in this file, so agents reading either entry point see them. +## Lessons + +Before changing code, read [LESSONS.md](LESSONS.md): what recent merged work got +wrong and the rule that would have caught it. Each lesson carries its evidence and +a "Seen" count of review days. A lesson seen on three review days moves into this +file. When a review or a corrective PR finds a new failure pattern, add it there +with a PR, commit, or `path:line` as evidence. + ## Working agreement ### 1. Read the structure before you change it diff --git a/CLAUDE.md b/CLAUDE.md index 12d7d841..9606307a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,6 +1,7 @@ # CLAUDE.md @AGENTS.md +@LESSONS.md The shared working agreement, commands, architecture, and conventions live in [AGENTS.md](AGENTS.md). Update that file for rules shared by all coding agents; diff --git a/LESSONS.md b/LESSONS.md new file mode 100644 index 00000000..d6f7f6db --- /dev/null +++ b/LESSONS.md @@ -0,0 +1,243 @@ + +# Lessons from merged work + +Neumar's code is written, reviewed, merged, and released by agents and by the +maintainer. Each review of merged work records what had to be fixed again, what +should not have merged, and what was verified too thinly. Read this before +changing code. + +Each lesson is a rule, then why, then the evidence. "Seen" counts the separate +review days that found it. A lesson seen on three review days moves into +[AGENTS.md](AGENTS.md), so every agent applies it without a separate read. + +The first review is 2026-10-13, covering merged PRs #1 to #187 and the tree at +`c0926618`. It is the only review so far, so every lesson below is "Seen 1x". +Several lessons have many instances inside that one review, and the instance +count is part of the evidence. The full review lives in +[doc/maintenance/2026-10-13-regression](doc/maintenance/2026-10-13-regression/README.md). + +## Validation + +**A check that skips your change is not validation. Run the closest check on the +exact commit you want merged, and write down what ran.** +- The automatic quality and test jobs in `.github/workflows/ci.yml` run only on + `release/*` or `release-v*` heads. 77 of 84 merged PRs never ran them. Every PR + still showed green check names, because audit and manifest jobs do run. +- #176 repaired frontend tests that #175 had broken. The tests were never run for + #175. #176 passed only because someone dispatched CI by hand on its commit. +- Four release PRs merged with a quality or Test failure already completed on the + matching head: #2, #24, #26, #106. A cancelled run is not a pass either. +- Seen 1x (2026-10-13). + +**A unit test that stops short of the real boundary does not prove the feature.** +- #175 proves the runner cancels when handed a signal. The MCP handler at + `src-api/src/shared/mcp/ffmpeg-skill-server.ts:122` never passes one, so Stop + cannot cancel a running operation. +- #187 tests caption edits in memory. The strict operation schema at + `packages/video-ir/src/timeline-schema.ts:483` rejects the `words` and + `sourceAnchor` fields the compiler itself emits, so the API path fails. +- #90 tests deny and default classification. It misses an explicit interactive + `alwaysAsk`, which `permissions.ts:419` lets a pre-approved tool bypass. +- #155 covers component rendering. It misses closing the chat dock before its + lazy thread connects, which loses the first prompt. +- Test the transport, schema, handler, or lifecycle the user actually reaches. +- Seen 1x (2026-10-13). + +**A claim in a PR body is a claim to verify at the final path.** +- #175 promised no overwrite and bounded, cancellable runs. Final publication at + `shared/services/ffmpeg-skill/handoff.ts:328` copies to a deterministic + destination with plain `copyFile`, so a repeat run replaces the deliverable. +- The runner refuses to overwrite its staged output, which is where the tests + looked. The unprotected write is one function later. +- Seen 1x (2026-10-13). + +**Validate the final integrated head, not each branch in a stack.** +- #102 introduced the `toolResultText` helper and one import. #103, stacked on the + same branch, added the import again. The duplicate import and two type errors + reached `main`, and #105 existed only to clean them up. +- Squash-commit stats are the real change. Stacked PR totals double-count. +- Seen 1x (2026-10-13). + +**Never raise a threshold or allowlist ceiling to make a check pass.** +- `scripts/component-size-allowlist.json` ceilings were raised three times in one + commit (`dd346af1`: RightSidebar 1675 to 1726, TaskV2MessageBubble 724 to 739, + MediaLightbox 549 to 554). The raised value then becomes the new baseline. +- `pnpm validate` passes with 453 frontend correctness warnings under it. The + pass bar is not the quality bar. +- Extract the responsibility, or record an exception with an owner, a reason, and + an expiry. Raising a number is neither. +- Seen 1x (2026-10-13). + +**When a check fails in your area, fix it or file it. Do not label it "fails on +main" and move on.** +- `pnpm test:fast` fails at `src-api/test/unit/agent-runtimes/resolve.test.ts:251` + on a machine with Homebrew OpenCode installed. It is a real test-isolation bug + that hid behind "environment specific" for the life of the repo. +- "Fails on main" means on freshly fetched `origin/main`, and you name the sha you + ran. A red under load is a timing assumption a user will meet on a busy machine. +- Seen 1x (2026-10-13). + +## Contracts and integration boundaries + +**Reproduce on the surface the caller uses: the HTTP route, the MCP handler, the +bridge, the packaged build, the chosen runtime.** +- #187, #175, #90 and #155 above are all one shape: the tested layer was not the + layer that failed. The compiler was right and the schema was wrong; the runner + was right and the handler was wrong; the classifier was right and the + precedence was wrong. +- Run the API test that exercises the route, or the integration test that crosses + the bridge. If no such test exists, that is the missing work, not a reason to + test the inner function twice. +- Seen 1x (2026-10-13). + +**Test isolation: stub the seam, not the environment.** +- `resolve.test.ts:239` stubs HOME and PATH, then calls a resolver that reads the + real filesystem and checks `/opt/homebrew/bin` first (`resolve.ts:124`). The + fixture never wins on a developer machine. +- Inject the discovery function through the existing `PathDiscoveryOptions` seam, + and assert precedence with controlled fixtures. Do not change production search + order to satisfy a test. +- Seen 1x (2026-10-13). + +**A test never reaches a real third-party service, and a fake process a test +starts ends when the test does.** +- Tests that read the real filesystem, real home directory, or a real network + target pass or fail based on the machine. That is not a signal about the code. +- Seen 1x (2026-10-13). + +## Types, state, and data + +**Fix the type, do not cast around it in the tests.** +- `shared/mcp/video-edit-server.ts:2562` types every tool handler as + `(args: never, ...)` and round-trips results with `as unknown as`. #102 and #103 + cleared the resulting errors by adding `as never` and `as unknown as` at more + than 20 test call sites. +- Parameterize the wrapper so valid input compiles. Keep a cast only where the + test deliberately exercises an invalid runtime input. +- Seen 1x (2026-10-13). + +**A default must never overwrite a value the user set.** +- `src/components/onboarding/OnboardingFlow.tsx:51` saves `planMode: 'on'` on + every completion, including a setup rerun, so an explicit `off` or `auto` is + lost. +- Before writing a setting, find every place the user can set it. Test with a + user-owned value present. +- Seen 1x (2026-10-13). + +**A failed read, parse, or migration means "unknown". It never means "empty".** +- `shared/db/migrations/utils.ts:20` cannot tell a missing table from a missing + column, so an unguarded `ALTER TABLE` on `video_intent_log` aborted ten later + migrations. The failure was silent because the database handle was published + before migrations finished. #44 repaired the case and added registry coverage. +- Skipping every absent table would be the opposite mistake, because it hides real + corruption. Decide per migration, and assert the expected schema after upgrade. +- Seen 1x (2026-10-13). + +**A generated contract and its runtime validator must be derived from one source.** +- Caption types at `packages/video-ir/src/timeline-types.ts:305` include `words` + and `sourceAnchor`. The strict schema at `timeline-schema.ts:483` does not. The + compiler emits what the schema rejects. +- Duplicated catalogs across `src/` and `src-api/src/` are pinned only by parity + tests. Prefer one package export over two copies and a test that keeps them + equal. +- Seen 1x (2026-10-13). + +## Security and filesystem boundaries + +**A name check is not a network policy. Untrusted URLs go through `safeFetch` +with resolved-address and redirect validation.** +- `shared/channels/channel-manager.ts:1937` fetches a URL taken from MCP tool + output after only a synchronous `validateBaseUrl` pre-check, follows redirects, + and buffers the body before checking its size. +- `extensions/agent/openai-compat/media.ts:78` fetches a provider-returned image + URL with no visible target policy, timeout, or byte bound. +- Keep an explicitly local integration on an explicitly local policy. Do not + replace every fixed-endpoint fetch. +- Seen 1x (2026-10-13). + +**Never build a shell command from a path, package name, or any other value the +agent or the user controls.** +- `app/api/files.ts:1085` interpolates a file path into a quoted command string + passed to `execAsync`. Quotes do not make a filename literal. +- `extensions/sandbox/claude.ts:255` joins package names into `pip install` and + `npm install` strings and runs them outside the sandbox. +- Use argument arrays with fixed binaries. Package installation belongs inside the + same isolation boundary as execution. +- Seen 1x (2026-10-13). + +**Validate containment on the resolved path, and check the final write, not only +the staging write.** +- `app/api/files.ts:92` compares a normalized path with a root by string prefix, + which accepts a sibling such as `/tmp-other` for `/tmp`, and later reads follow + symlinks. +- FFmpeg publication validates the staged output, then writes to a destination + that can be a symlink. +- Seen 1x (2026-10-13). + +**App-owned data goes through `getAppDir()`, never a home-directory path built by +hand.** +- `shared/services/git-workspace.ts:27` derives app repositories and sessions from + `homedir()`, ignoring `NEUMAR_APP_DATA_DIR`. +- `shared/services/publish/destinations/local-archive.ts:30` defaults to a + `.neuma` directory, which is the previous brand and breaks isolation. +- A path that ignores the app-data override makes tests and packaging write to the + developer's real home. +- Seen 1x (2026-10-13). + +**A security gap a document names is an open bug, not a description. File it in +the same change that writes the sentence.** +- Tracked docs still claim a browser IndexedDB fallback, Vite 7, and Node 20 + targets, so a reader cannot tell current behavior from history. +- Seen 1x (2026-10-13). + +## Deleting, duplicating, and merging + +**When you replace an internal API, migrate every caller and delete the old one in +the same wave.** +- Live parallel paths today: `/task/:taskId` and `/task-v2/:taskId`, both settings + shells behind `ui.simpleShell` (default false), two channel registries, two + Slack stacks, and mirrored video catalogs. +- A compatibility layer left behind becomes the thing the next change has to + update twice. +- Seen 1x (2026-10-13). + +**One commit, one goal. A fix found on the way gets its own commit.** +- #175 shipped the FFmpeg skill plus attachment upload, planning settings, and + error presentation, and the test repair arrived in #176. +- Seen 1x (2026-10-13). + +**Merge only the reviewed head, and record what was run at that head.** +- #103 merged a branch whose own formatting and type checks were red, and it was + found later. +- The reviewed head is the sha you ran. If you push after review, review again. +- Seen 1x (2026-10-13). + +## Working with parallel agents + +These rules are adopted for the epic work in +[doc/maintenance/2026-10-13-regression/02-development-plan.md](doc/maintenance/2026-10-13-regression/02-development-plan.md), +which is indexed with its issues and commands in +[04-epics-and-issues.md](doc/maintenance/2026-10-13-regression/04-epics-and-issues.md). +No local failure has been recorded for them yet, so they are unproven here and +carry no "Seen" count. + +- **One agent, one worktree, one branch.** Two sessions committing from one + checkout ship each other's staged files under the wrong subject. +- **After committing, `git show --stat HEAD` lists your files and only yours.** An + empty commit means another session took your index. +- **Serialize edits to shared files**: `package.json`, `pnpm-lock.yaml`, locale + modules, the size allowlist, `http-daemon.ts`, `video-edit-server.ts`, and the + shared video IR schemas. Rebase and rerun affected integration tests when + another stream changes a shared contract. +- **Sub-issues merge into the epic branch, never into `main`.** The epic closes + with one reviewed PR into `main` after every sub-issue is done. + +## Adding to this file + +- Add a lesson only with evidence: a PR number, a commit, or a file and line, plus + what was run and what was missed. +- Increment "Seen" when a later review finds the same rule again. Say which review + day, not only the count. +- After three review days, move the rule into [AGENTS.md](AGENTS.md) and leave a + pointer here. +- Keep the rule first. If it needs a paragraph of setup, it is not a rule yet. diff --git a/doc/maintenance/2026-10-13-regression/01-findings.md b/doc/maintenance/2026-10-13-regression/01-findings.md index 5784740a..c8d64fac 100644 --- a/doc/maintenance/2026-10-13-regression/01-findings.md +++ b/doc/maintenance/2026-10-13-regression/01-findings.md @@ -1,6 +1,6 @@ # Findings and evidence -Baseline: `c0926618`, reviewed 2026-10-13. Line numbers below refer to that commit. P0 means urgent containment, P1 means fix before expanding the affected feature, and P2 means planned cleanup. “Confirmed” means the stated code behavior is visible in source or a recorded probe; it does not mean an exploit or full application scenario was executed. +Baseline: `c0926618`, reviewed 2026-10-13. Line numbers below refer to that commit. P0 means urgent containment, P1 means fix before expanding the affected feature, and P2 means planned cleanup. “Confirmed” means the stated code behavior is visible in source or a recorded probe; it does not mean an exploit or full application scenario was executed. Every “Act on” finding and every “Consider” item except the deferred SDK requirements has an issue in [the issue index](04-epics-and-issues.md), and the rules behind them are in [LESSONS.md](../../../LESSONS.md). ## Act on: security and execution boundaries @@ -78,6 +78,24 @@ The sidebar reduction is progress. The allowlist also shrank from 69 to 68 entri Tracked docs still claim browser IndexedDB fallback in `doc/system/overview.md:41,119` and `doc/frontend/state-management.md:25,88`, while the current frontend database abstraction uses the API-backed store. They also list Vite 7/TypeScript 5.8 and Node 20 build targets. Update these from actual manifests, database call paths, and CI, not by changing date/version headings alone. +## Reference practice behind each fix + +These references describe the class of fix, so the proposed change is not only this review's opinion. They do not replace the repository's own helpers: use `safeFetch`, `path-validator.ts`, and `getAppDir()` where they apply. Each fix is tracked in [the issue index](04-epics-and-issues.md). + +| Finding | Practice the fix follows | +| --- | --- | +| S1 | Bind to a specific interface instead of the unrestricted address ([CWE-1327](https://cwe.mitre.org/data/definitions/1327.html)). Node listens on the unspecified address when `host` is omitted ([`server.listen`](https://nodejs.org/api/net.html)). Authenticate critical functions ([CWE-306](https://cwe.mitre.org/data/definitions/306.html)). | +| S2, S3 | Avoid the shell and pass arguments as an array to a fixed binary; validate what remains ([OWASP OS Command Injection Defense](https://cheatsheetseries.owasp.org/cheatsheets/OS_Command_Injection_Defense_Cheat_Sheet.html), [CWE-78](https://cwe.mitre.org/data/definitions/78.html)). [`execFile`](https://nodejs.org/api/child_process.html) does not spawn a shell by default. | +| S4, containment at `files.ts:92` | Resolve real paths and refuse link following where it escapes the root ([CWE-22](https://cwe.mitre.org/data/definitions/22.html), [CWE-59](https://cwe.mitre.org/data/definitions/59.html)). Create the final file exclusively instead of checking then writing ([CWE-367](https://cwe.mitre.org/data/definitions/367.html), [`COPYFILE_EXCL`](https://nodejs.org/api/fs.html)). | +| S5 | Pass an `AbortSignal` into the child process and check it again before the next side effect ([`child_process` `signal` option](https://nodejs.org/api/child_process.html)). | +| S6 | An explicit deny or ask outranks a broad grant ([CWE-863](https://cwe.mitre.org/data/definitions/863.html)). | +| S7 | Validate resolved addresses and every redirect, bind DNS to the connection, and bound time and size ([OWASP SSRF Prevention](https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html), [CWE-918](https://cwe.mitre.org/data/definitions/918.html)). | +| S8 | Treat missing peer metadata as untrusted ([CWE-636](https://cwe.mitre.org/data/definitions/636.html)). | +| S9 | Verify a digest before using downloaded code ([CWE-494](https://cwe.mitre.org/data/definitions/494.html)). | +| Tauri `csp: null` | Stage a restrictive policy after checking what the app loads ([Tauri CSP](https://v2.tauri.app/security/csp/)). | +| Merge gate | A required check that is skipped can report success, so require one stable aggregate job ([GitHub, skipped but required checks](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks)). Lint workflow files with [actionlint](https://github.com/rhysd/actionlint). | +| R1 to R6 | No external standard applies. The practice is the project's own: test the real boundary, derive the schema from the type, and inject the seam ([LESSONS.md](../../../LESSONS.md)). | + ## Dismissed or not established - Size, zero review records, a non-conventional title, casts, or a grep match alone do not establish a defective PR or malicious intent. diff --git a/doc/maintenance/2026-10-13-regression/02-development-plan.md b/doc/maintenance/2026-10-13-regression/02-development-plan.md index 30f13cd8..c6fa7748 100644 --- a/doc/maintenance/2026-10-13-regression/02-development-plan.md +++ b/doc/maintenance/2026-10-13-regression/02-development-plan.md @@ -1,6 +1,6 @@ # Development plan -Status: ready for checkpoint review, not yet implemented. Baseline `c0926618`. Finding IDs refer to [the evidence register](01-findings.md). The first objective is containment and reliable validation; deletion and restructuring follow verified caller maps. +Status: tracked as epics #188 to #194, not yet implemented. Baseline `c0926618`. Finding IDs refer to [the evidence register](01-findings.md). Issue numbers and the epic workflow are in [the issue index](04-epics-and-issues.md). The first objective is containment and reliable validation; deletion and restructuring follow verified caller maps. ## Throughput and ownership @@ -9,8 +9,22 @@ Status: ready for checkpoint review, not yet implemented. Baseline `c0926618`. F 3. **Shared mutable state.** Serialize edits to `package.json`, `pnpm-lock.yaml`, locale modules, the size allowlist, `http-daemon.ts`, `video-edit-server.ts`, and common video IR schemas. Assign one owner per shared file during each batch. Rebase and rerun affected integration tests after another stream changes a shared contract. 4. **Smallest safe decomposition.** Seven checkpoints below; several contain small independent PRs. No single “cleanup everything” PR. No new framework, dependency upgrade, or generic abstraction is required just to start this work. +## Execution model + +Each checkpoint is an epic with a branch `epic/-`. Each unit of work is a sub-issue that one agent implements in its own git worktree, as a PR into the epic branch with `Closes #`. When every sub-issue is closed, one epic PR merges the branch into `main`. [The issue index](04-epics-and-issues.md) has the commands, the blockers between sub-issues, and the merge order across epics. + +Every implementation PR follows the same rules, taken from [LESSONS.md](../../../LESSONS.md). + +- Write the regression test first at the boundary the user reaches (route, MCP handler, bridge, lifecycle). A test of the inner function alone does not close a finding. +- Show that the test fails on the reviewed baseline and passes after the change. +- Run the checks on the exact head to be merged and record its sha, plus the Node and pnpm versions. +- One goal per commit, with only the owned files staged. Migrate every caller and delete the superseded path in the same PR. +- Never raise a threshold or allowlist ceiling to make a check pass. + ## 1. Establish a reproducible merge gate +**Tracking:** epic #188, sub-issues #195 to #199. **Lessons:** a check that skips your change is not validation; never raise a ceiling to pass; stub the seam, not the environment. + **Likely files:** `.github/workflows/ci.yml`, existing workflow checks, root/API package scripts and configs, `src-api/test/unit/agent-runtimes/resolve.test.ts`, repository rulesets through a separately approved settings change. **Work:** @@ -40,6 +54,8 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## 2. Contain host execution and external I/O +**Tracking:** epic #189, sub-issues #200 to #207. **Lessons:** never build a shell command from a controlled value; a name check is not a network policy; validate containment on the resolved path. Reference practice is listed in [the findings](01-findings.md#reference-practice-behind-each-fix). + **Likely files:** `src-api/src/http-daemon.ts`, `app/middleware/jwt.ts`, `app/api/{sandbox,files}.ts`, `extensions/sandbox/claude.ts`, `shared/network-policy/`, identified URL consumers, `shared/services/external-mcp/auth.ts`, `src-api/scripts/build.mjs`, Tauri configuration where necessary. **Work:** @@ -58,6 +74,8 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## 3. Repair FFmpeg publication, cancellation, and permission priority +**Tracking:** epic #190, sub-issues #208 to #210. **Lessons:** a unit test that stops short of the real boundary does not prove the feature; a claim in a PR body is a claim to verify at the final path. + **Likely files:** `shared/services/ffmpeg-skill/{runner,handoff,policy}.ts`, `shared/mcp/ffmpeg-skill-server.ts`, relevant bridge/run cancellation wiring, `extensions/agent/claude/permissions.ts`, corresponding API tests. **Work:** @@ -74,6 +92,8 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## 4. Restore transcript and shell integration contracts +**Tracking:** epic #191, sub-issues #211 to #216. **Lessons:** derive a contract and its validator from one source; reproduce on the surface the caller uses; a default must never overwrite a value the user set. + **Likely files:** `packages/video-ir/src/{timeline-schema,timeline-types,transcript-edit}.ts`, transcript UI/store and API route tests, `ChatDock.tsx`, `InitialMessageSender.tsx`, `OutcomeAutomationLists.tsx`, `OnboardingFlow.tsx`, relevant locales. **Work:** @@ -93,6 +113,8 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## 5. Remove proven orphans and obsolete allowances +**Tracking:** epic #192, sub-issues #217 to #221. **Lessons:** migrate every caller and delete the old path in the same wave; never raise an allowlist ceiling. + **Likely files:** the dead-code candidate islands in `01-findings.md`, tests/locales used exclusively by retired code, `scripts/component-size-allowlist.json`; manifests/lockfile only after a dependency is proven unused. **Work:** @@ -111,6 +133,8 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## 6. Resolve architecture ownership and reduce the highest-risk modules +**Tracking:** epic #193, sub-issues #222 to #228. **Lessons:** fix the type instead of casting around it in tests; a failed read or migration means unknown, not empty; app-owned data goes through `getAppDir()`. + **Likely files:** channel/runtime contract docs, `core/agent/`, `shared/channels/`, `shared/services/gateway/`, legacy Slack wiring, video tool wrapper and schemas, DB migration utilities/tests, shared catalogs, selected hooks/components, tracked `doc/` architecture pages. **Work:** @@ -130,6 +154,8 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## 7. Make maintenance recurring and contributor quality reviewable +**Tracking:** epic #194, sub-issues #229 to #231. **Lessons:** merge only the reviewed head; a claim in a PR body is a claim to verify. The incident format follows blameless postmortem practice ([Google SRE](https://sre.google/sre-book/postmortem-culture/)), recording the failed contract and the gate that missed it instead of judging a person. + **Likely files/settings:** `doc/maintenance/`, contribution/PR templates if adopted, existing validation scripts, narrowly scoped CI reports, repository rulesets after separate approval. **Work:** @@ -148,12 +174,16 @@ Inspect Actions on the exact PR head. Deliberately introduce a reversible failur ## Open decisions before dependent implementation -- Is remote HTTP operation required by default launchers, or only explicit remote mode? Inventory callers first; default loopback is the proposed containment. -- Should explicit `alwaysAsk` override run pre-approval? Recommended yes, consistent with explicit user choice. Preserve explicit deny regardless. -- Should transcript restore ignore unrelated empty caption tracks, and which translation operations consume project budgets? -- Retire `/task/:taskId` with tested redirects, or maintain V1 compatibility? Both routes are currently live. -- Keep, implement, or retire test-only services, social publishing adapters and dormant plugin APIs after caller/consumer review? -- Who owns weekly maintenance and security follow-up, and what solo-maintainer review exception is acceptable? +Each decision has an issue. Record the answer there, before the dependent change merges. + +- Is remote HTTP operation required by default launchers, or only explicit remote mode? Inventory callers first; default loopback is the proposed containment. (#200) +- Should explicit `alwaysAsk` override run pre-approval? Recommended yes, consistent with explicit user choice. Preserve explicit deny regardless. (#210) +- Should transcript restore ignore unrelated empty caption tracks, and which translation operations consume project budgets? (#216) +- Retire `/task/:taskId` with tested redirects, or maintain V1 compatibility? Both routes are currently live. (#223) +- Keep, implement, or retire test-only services, social publishing adapters and dormant plugin APIs after caller/consumer review? (#221) +- Who owns weekly maintenance and security follow-up, and what solo-maintainer review exception is acceptable? (#199 and #230) +- Which channel runtime becomes canonical once the responsibility matrix exists? (#222) +- Is a restrictive Tauri CSP compatible with the preview, WebUI, and sandboxed content workflows? (#207) The user selected contract mapping before channel consolidation and visibility review before changing placeholder features. Those decisions permit the investigations above; they do not authorize silently choosing a new channel runtime or implementing all stubs. diff --git a/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md b/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md index 326eb3aa..6fbc7c6c 100644 --- a/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md +++ b/doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md @@ -1,6 +1,6 @@ # PR lessons and contributor quality policy -Reviewed 2026-10-13 against `c0926618`. This is a retrospective assessment of changes and gates. It does not infer malicious intent, identify an agent from product references, or recommend reverting large PRs wholesale. +Reviewed 2026-10-13 against `c0926618`. The rules that came out of this review are in [LESSONS.md](../../../LESSONS.md) at the repository root, where agents read them. This page keeps the PR-level evidence behind them. This is a retrospective assessment of changes and gates. It does not infer malicious intent, identify an agent from product references, or recommend reverting large PRs wholesale. ## What the merge system actually required @@ -69,6 +69,8 @@ The seven matching-head premerge failure cases were #176, #106, #26, #24, #3, #2 | Initial snapshot → current OpenCode baseline failure | PATH stubs left hardcoded host search roots live. | Inject filesystem discovery and assert precedence with controlled fixtures. | | June architecture warnings → current large tool modules | The component-only size gate misses API tools, routes, hooks and stores. | Baseline-aware size/complexity review outside components; extract a tested responsibility instead of raising ceilings. | +The checks above are built in epic #188 (merge gate, sub-issues #195 to #199) and epic #194 (PR template, weekly review, policy records). Each row corresponds to a rule in [LESSONS.md](../../../LESSONS.md). + `pnpm validate` is not the test suite, API lint, API test-file typecheck, Rust validation, or video IR tests. CI also runs a smaller quality subset than the full root gate. “Validate passed” must always name the checks it actually represents. The current 453 frontend warnings pass the root gate. Track their baseline and reject newly introduced correctness warnings/suppressions in changed files, then retire existing ones in bounded batches. Avoid a bulk warning cleanup that changes behavior without regression tests. @@ -85,7 +87,7 @@ The user selected an agent policy and attribution gate, with no speculative name For every new contribution, record the human accountable for landing it, the authoring tool/session when known, intended scope, exact verified commit, and checks omitted with reasons. Store private session URLs/identifiers outside public documentation; public PR metadata may use an opaque local reference. Record `unknown` when attribution is unavailable rather than guessing. -A quality incident record contains the introducing PR/commit, reproducible scenario, severity, review/check gap, attribution evidence, repair PR, verification, date, maintainer disposition, and appeal status. Two reviewers or a maintainer with an independent verification artifact confirm the incident before it counts. Attach code evidence, not opinions about style or individuals. +A quality incident record contains the introducing PR/commit, reproducible scenario, severity, review/check gap, attribution evidence, repair PR, verification, date, maintainer disposition, and appeal status. Two reviewers or a maintainer with an independent verification artifact confirm the incident before it counts. Attach code evidence, not opinions about style or individuals. The record follows blameless postmortem practice ([Google SRE](https://sre.google/sre-book/postmortem-culture/)). It names the contract that failed and the gate that missed it. The template and the empty blocklist schema are issue #231. ### Proposed thresholds diff --git a/doc/maintenance/2026-10-13-regression/04-epics-and-issues.md b/doc/maintenance/2026-10-13-regression/04-epics-and-issues.md new file mode 100644 index 00000000..0c4ac87d --- /dev/null +++ b/doc/maintenance/2026-10-13-regression/04-epics-and-issues.md @@ -0,0 +1,143 @@ +# Epics and issues + +The development plan is tracked as seven epics, one per checkpoint, and 37 sub-issues. Each sub-issue is written so one agent can implement it alone in its own git worktree. This page is the index. The issue bodies hold the scope, owned files, acceptance criteria, and verification commands, and they are the working copy. If an issue and this page disagree, fix the page. + +## How the work flows + +Each epic has a long-lived branch named `epic/-`, created from `main` by the maintainer. Sub-issues never target `main`. This follows the stacked-integration model recorded in [LESSONS.md](../../../LESSONS.md): one agent, one worktree, one branch, and validation of the final integrated head. + +1. An agent picks a sub-issue with no open blocker and creates a worktree from the epic branch. +2. It commits only the files the issue owns, with a regression test that fails before the change and passes after it. +3. It opens a PR whose base is the epic branch and whose body says `Closes #`. +4. A merged PR closes the sub-issue through [epic-close-issues.yml](../../../.github/workflows/epic-close-issues.yml), which acts on PRs merged into `epic/**` and skips the epic's own number. +5. When every sub-issue is closed, one epic PR merges the epic branch into `main` with `Closes #`. The PR records the head sha that `pnpm validate`, `pnpm test:fast` and the epic's surface checks ran on. + +```bash +git fetch origin +git worktree add ../neumar-wt/- -b issue/- origin/epic/- +``` + +Worktrees give each agent its own index, so two sessions cannot commit each other's staged files ([git worktree](https://git-scm.com/docs/git-worktree)). A sub-issue lists its blockers. Do not start one until its blocker has merged into the epic branch. + +## Epics + +| Epic | Checkpoint | Branch | Sub-issues | +| --- | ---: | --- | ---: | +| [#188](https://github.com/bravew/Neumar/issues/188) reproducible merge gate and test isolation | 1 | `epic/188-merge-gate` | 5 | +| [#189](https://github.com/bravew/Neumar/issues/189) contain host execution and external I/O | 2 | `epic/189-containment` | 8 | +| [#190](https://github.com/bravew/Neumar/issues/190) repair FFmpeg publication, cancellation, and permission priority | 3 | `epic/190-ffmpeg-permissions` | 3 | +| [#191](https://github.com/bravew/Neumar/issues/191) restore transcript and shell integration contracts | 4 | `epic/191-transcript-shell` | 6 | +| [#192](https://github.com/bravew/Neumar/issues/192) remove proven orphans and obsolete allowances | 5 | `epic/192-dead-code` | 5 | +| [#193](https://github.com/bravew/Neumar/issues/193) resolve architecture ownership and reduce the highest-risk modules | 6 | `epic/193-architecture-ownership` | 7 | +| [#194](https://github.com/bravew/Neumar/issues/194) recurring maintenance and contributor quality policy | 7 | `epic/194-maintenance-process` | 3 | + +Merge the epics into `main` in this order. E1 first, so later epic PRs run the repaired gate. Then E2 and E3, which fix the highest-risk defects. Then E4, E5, E6, and E7. Rebase an epic branch onto `main` before its final PR. + +Each epic branch was created from the commit that added these documents, so the links in the issue bodies resolve on the epic branch. + +## Sub-issues + +### Epic #188 reproducible merge gate and test isolation + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#195](https://github.com/bravew/Neumar/issues/195) | fix(test): isolate runtime discovery in resolve.test.ts (R6) | R6 | fix | none | +| [#196](https://github.com/bravew/Neumar/issues/196) | ci: run quality and test jobs on every non-draft PR, including PRs into epic branches | merge-gate metrics in 03-lessons-and-contributors.md | change | none | +| [#197](https://github.com/bravew/Neumar/issues/197) | ci: add API lint, API test typecheck, video-ir validate, and changed-surface Rust checks | validate scope gap (README baseline table) | change | #196 | +| [#198](https://github.com/bravew/Neumar/issues/198) | chore(quality): baseline-aware gate for lint warnings, suppressions, and size ceilings | 453 warnings; allowlist ceiling raises (dd346af1) | change | none | +| [#199](https://github.com/bravew/Neumar/issues/199) | decide: required status checks and solo-maintainer review rule for main and epic branches | merge metrics: zero required checks, zero approving reviews | decision | #196 | + +### Epic #189 contain host execution and external I/O + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#200](https://github.com/bravew/Neumar/issues/200) | fix(security): bind the daemon to loopback by default and require auth for remote mode (S1, P0) | S1 (P0) | fix | none | +| [#201](https://github.com/bravew/Neumar/issues/201) | fix(security): open files and editors without a shell command string (S2) | S2 | fix | none | +| [#202](https://github.com/bravew/Neumar/issues/202) | fix(security): validate and isolate sandbox package installation (S3) | S3 | fix | none | +| [#203](https://github.com/bravew/Neumar/issues/203) | fix(security): check file containment on the resolved path, including symlinks | files.ts:92 (additional work in 01-findings.md) | fix | #201 | +| [#204](https://github.com/bravew/Neumar/issues/204) | fix(security): route untrusted URL fetches through safeFetch (S7) | S7 (plus remote-oauth.ts and media-server.ts review) | fix | none | +| [#205](https://github.com/bravew/Neumar/issues/205) | fix(security): external MCP auth fails closed on missing peer address (S8) | S8 (P2) | fix | none | +| [#206](https://github.com/bravew/Neumar/issues/206) | fix(build): verify the native SQLite addon digest and key the cache by version (S9) | S9 | fix | none | +| [#207](https://github.com/bravew/Neumar/issues/207) | decide: Tauri CSP and capability narrowing, compatibility-tested | csp: null note in 01-findings.md | decision | none | + +### Epic #190 repair FFmpeg publication, cancellation, and permission priority + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#208](https://github.com/bravew/Neumar/issues/208) | fix(ffmpeg): publish outputs exclusively inside the validated root (S4) | S4 | fix | none | +| [#209](https://github.com/bravew/Neumar/issues/209) | fix(ffmpeg): carry Stop through the MCP handler to the runner and the publication step (S5) | S5 | fix | #208 | +| [#210](https://github.com/bravew/Neumar/issues/210) | fix(permissions): an explicit ask or deny outranks run pre-approval (S6) | S6 | fix | none | + +### Epic #191 restore transcript and shell integration contracts + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#211](https://github.com/bravew/Neumar/issues/211) | fix(video-ir): caption operation schema accepts the fields the compiler emits (R1) | R1 | fix | none | +| [#212](https://github.com/bravew/Neumar/issues/212) | fix(video): replacing a source transcript clears the word selection (R2) | R2 | fix | none | +| [#213](https://github.com/bravew/Neumar/issues/213) | fix(chat-dock): deliver the first prompt exactly once across close, slow load, and navigation (R3) | R3 | fix | none | +| [#214](https://github.com/bravew/Neumar/issues/214) | fix(automation): scope event waits for the dock and carries its category (R4) | R4 | fix | #213 | +| [#215](https://github.com/bravew/Neumar/issues/215) | fix(onboarding): setup reruns preserve an explicit plan-mode setting (R5) | R5 | fix | none | +| [#216](https://github.com/bravew/Neumar/issues/216) | decide: empty-caption restore, translation budget, and fork cost semantics | Consider items in 01-findings.md | decision | none | + +### Epic #192 remove proven orphans and obsolete allowances + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#217](https://github.com/bravew/Neumar/issues/217) | chore(frontend): delete the old root app island and the nine obsolete size allowances | dead-code batch 1 and allowlist entries | change | none | +| [#218](https://github.com/bravew/Neumar/issues/218) | chore(api): remove proven orphan modules, one at a time | dead-code batch (API orphan candidates) | change | none | +| [#219](https://github.com/bravew/Neumar/issues/219) | chore(frontend): remove the gateway settings island after a route and feature parity check | dead-code batch (gateway settings island) | change | #217 | +| [#220](https://github.com/bravew/Neumar/issues/220) | chore(deps): remove dependencies proven unused after caller removal | dead-code batch (dependencies) | change | #217, #218, #219 | +| [#221](https://github.com/bravew/Neumar/issues/221) | decide: keep, finish, or retire test-only modules and dormant adapters | dead-code batch (test-only modules) | decision | none | + +### Epic #193 resolve architecture ownership and reduce the highest-risk modules + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#222](https://github.com/bravew/Neumar/issues/222) | docs: channel responsibility matrix before choosing a canonical runtime | architecture debt (two channel registries, two Slack stacks) | decision | none | +| [#223](https://github.com/bravew/Neumar/issues/223) | chore: decide V1 task route retirement and make 501 surfaces truthful | unsupported routes; V1/V2 task routes | decision | none | +| [#224](https://github.com/bravew/Neumar/issues/224) | refactor(video): type the video tool wrapper so valid input compiles, then remove the test casts | tool typing (01-findings.md) | change | none | +| [#225](https://github.com/bravew/Neumar/issues/225) | test(db): run the full migration registry against fresh, current, and drifted snapshots | migration drift (01-findings.md) | change | none | +| [#226](https://github.com/bravew/Neumar/issues/226) | fix(api): app-owned data resolves through getAppDir() and honors NEUMAR_APP_DATA_DIR | app-data isolation (01-findings.md) | fix | none | +| [#227](https://github.com/bravew/Neumar/issues/227) | docs: update the architecture pages from the manifests, database call paths, and CI | docs drift (01-findings.md, prior architecture review) | docs | none | +| [#228](https://github.com/bravew/Neumar/issues/228) | refactor(video): extract one tested tool group out of video-edit-server.ts | hotspot growth table (01-findings.md) | change | #224 | + +### Epic #194 recurring maintenance and contributor quality policy + +| Issue | Title | Findings | Kind | Blocked by | +| --- | --- | --- | --- | --- | +| [#229](https://github.com/bravew/Neumar/issues/229) | docs(process): PR template that records intent, exact tested commit, and attribution | checkpoint 7 | docs | none | +| [#230](https://github.com/bravew/Neumar/issues/230) | docs(process): weekly and pre-release maintenance review with a report format | checkpoint 7 | docs | none | +| [#231](https://github.com/bravew/Neumar/issues/231) | docs(process): incident record template, empty blocklist schema, and policy replay | checkpoint 7; contributor policy in 03-lessons-and-contributors.md | docs | none | + +## Files two sub-issues must not edit at once + +| File | Order | +| --- | --- | +| `.github/workflows/ci.yml` | #196, then #197 | +| `src-api/src/app/api/files.ts` | #201, then #203 | +| `src-api/src/shared/services/ffmpeg-skill/handoff.ts` | #208, then #209 (abort check only) | +| `src/components/chat-dock/ChatDock.tsx` | #213, then #214 | +| `packages/video-ir/src/timeline-schema.ts` | #211 owns it for the epic, other streams rebase after it | +| `src-api/src/shared/mcp/video-edit-server.ts` | #224, then #228 | +| `src-api/src/http-daemon.ts` | #200 only | +| `scripts/component-size-allowlist.json` and locale modules | #217, then #219, then #223 if UI text changes | +| `package.json` and `pnpm-lock.yaml` | #220 runs after #217, #218, #219. #197 may add scripts first. | + +Across epics the same files can collide when the epic PRs reach `main`. The merge order above settles that. If two open epics need the same file, comment on both issues and let the earlier epic merge first. + +## Decisions that need the maintainer + +These issues carry the `needs-decision` label. Record the decision as a comment on the issue before any dependent change merges. + +- [#199](https://github.com/bravew/Neumar/issues/199) decide: required status checks and solo-maintainer review rule for main and epic branches +- [#207](https://github.com/bravew/Neumar/issues/207) decide: Tauri CSP and capability narrowing, compatibility-tested +- [#216](https://github.com/bravew/Neumar/issues/216) decide: empty-caption restore, translation budget, and fork cost semantics +- [#221](https://github.com/bravew/Neumar/issues/221) decide: keep, finish, or retire test-only modules and dormant adapters +- [#222](https://github.com/bravew/Neumar/issues/222) docs: channel responsibility matrix before choosing a canonical runtime +- [#223](https://github.com/bravew/Neumar/issues/223) chore: decide V1 task route retirement and make 501 surfaces truthful + +Two fix issues also carry a decision. The permission priority in [#210](https://github.com/bravew/Neumar/issues/210) has a recommended default on the issue. The remote-mode contract in [#200](https://github.com/bravew/Neumar/issues/200) starts with a caller inventory. + +## Known baseline + +`pnpm test:fast` fails at `src-api/test/unit/agent-runtimes/resolve.test.ts:251` on hosts with Homebrew OpenCode until [#195](https://github.com/bravew/Neumar/issues/195) lands. A PR that hits only that failure names it, with the sha it ran on, instead of calling it a regression. diff --git a/doc/maintenance/2026-10-13-regression/README.md b/doc/maintenance/2026-10-13-regression/README.md index e6398153..d682c88c 100644 --- a/doc/maintenance/2026-10-13-regression/README.md +++ b/doc/maintenance/2026-10-13-regression/README.md @@ -1,12 +1,15 @@ # Regression review and maintenance plan -Date: 2026-10-13. Reviewed baseline: `c0926618`, version 26.10.1. Status: review completed; remediation checkpoints proposed, not executed. This document records a sampled architecture and regression review, not certification of every file or dependency. +Date: 2026-10-13. Reviewed baseline: `c0926618`, version 26.10.1. Status: review completed; remediation tracked as GitHub epics #188 to #194, not yet executed. This document records a sampled architecture and regression review, not certification of every file or dependency. ## Start here 1. [Findings and evidence](01-findings.md) records current defects, dead-code candidates, architecture drift, and verification limits. 2. [Development plan](02-development-plan.md) gives seven independently verifiable checkpoints, their dependencies, and decisions needed before implementation. 3. [PR lessons and contributor policy](03-lessons-and-contributors.md) records merge failures, validation gaps, and the evidence required for probation or blocking. +4. [Epics and issues](04-epics-and-issues.md) indexes the seven epics and 37 sub-issues, the worktree and epic-branch workflow, and the files that two streams must not edit at once. + +The rules this review produced live in [LESSONS.md](../../../LESSONS.md) at the repository root, which [AGENTS.md](../../../AGENTS.md) tells every agent to read before changing code. The documents here hold the evidence behind those rules. Add new lessons to the root file, not to these pages. The main problem is that changes can merge without exercising their contracts. Recent examples include FFmpeg publication and cancellation, transcript schema validation, chat-dock prompt handoff, and permission precedence. Earlier security boundaries also need review. Removing unused code will help, but it cannot substitute for fixing these paths and preventing another unchecked merge. @@ -33,7 +36,7 @@ The local runner used Node v26.11.1 and the pinned pnpm 11.22.0. Repeat the fail - Trace callers and visibility of unsupported API actions first. Hide misleading controls or document intentional capability limitations; feature implementation requires a separate behavior decision. - Keep the review and plan in `doc/`. Existing local work is outside this deliverable. -The requested recurring cleanup is defined as a maintenance process in checkpoint 7. No scheduler, repository setting, contributor access restriction, issue, or external post has been created by this documentation work. +The requested recurring cleanup is defined as a maintenance process in checkpoint 7. Each checkpoint became an epic with its own branch, and each unit of work became a sub-issue ([index](04-epics-and-issues.md)). No scheduler, repository setting, contributor access restriction, or external post has been created. The epic branches are pushed, but no implementation has started. ## Review coverage and prior architecture work @@ -45,4 +48,4 @@ No malicious code or live service credential was identified in the inspected are ## Completion for this deliverable -The documentation review is complete when the files and source links have been checked, the root gate result and test failure are reported, and only these docs are committed locally. Implementation is complete only after the relevant checkpoints have their own before/after regression evidence and verification. The current plan does not claim those fixes have landed. +The documentation review is complete when the files and source links have been checked, the root gate result and test failure are reported, and the documents, `LESSONS.md`, and the `AGENTS.md` pointer are committed locally. Implementation is complete only after each epic's sub-issues have merged into its epic branch with before/after regression evidence, and the epic PR has merged into `main` on a head that passed the epic's checks. The current plan does not claim those fixes have landed. From d182f23f27100a18066d5804f2ec4f774f41dd98 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 00:16:07 -0400 Subject: [PATCH 03/16] docs(process): require PR verification and attribution evidence (#235) --- .github/PULL_REQUEST_TEMPLATE.md | 59 +++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 42273602..0f934e3e 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,15 +1,56 @@ -## Summary +## Intent and scope - + -## Test plan +Intent and resulting behavior: - +Affected contracts and consumers: -- [ ] `pnpm validate` -- [ ] `pnpm test:fast` -- [ ] Manually tested in the app (describe the flow) +## Verification evidence -## Related issues +Exact checked commit SHA: - +Node and pnpm versions: + + + +| Command or check | Outcome and evidence | +| --- | --- | +| `pnpm validate` | | +| Closest changed-surface check | | + +Regression evidence: + + + +Unverified cases and skipped checks, with reasons: + +## Attribution + +Accountable contributor: + +Authoring tool/configuration, if known (otherwise `unknown`): + +Session reference, if known (otherwise `unknown`): + + + +## Corrective change + + + +Original finding link: + +Introducing PR or commit, with causal evidence (or `unknown`): + +Reproducible failing scenario: + +Review/check gap that let it escape: + +Prevention test or check and its result: + +## Related issue + + + +Closes #N From 5f367700326d9a0f2e0c2b2c9ca15b08a6386c60 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 00:15:56 -0400 Subject: [PATCH 04/16] ci: run quality and test jobs on every non-draft PR (#232) The quality and test jobs were gated on a release/* or release-v* head branch, so an ordinary PR, including a PR into an epic branch, reported green check names without running anything. LESSONS.md records 77 of 84 merged PRs never running them. Run quality and test on every non-draft pull request regardless of base branch, keep them off drafts (which cannot be merged), and stop excluding dependency-bot actors. Add a push trigger for main and epic/** so the integrated head is validated too. Add a ci-gate job named "CI Gate" that needs quality, test, browser-e2e and build and runs with if: always(). It succeeds only when quality, test and build all succeeded; a skipped, cancelled or failed required job is a failure, so a skipped job cannot read as green. Browser E2E is accepted only when it was not requested, and a draft PR is accepted because GitHub cannot merge it. Values reach the script through env, and every action reference remains a full commit SHA. Refs #196 (cherry picked from commit 952c9258e7a9c4c88b0384bce53689bc96fc1e1d) --- .github/workflows/ci.yml | 76 ++++++++--- src/__tests__/ci-workflow-gate.test.ts | 180 +++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 19 deletions(-) create mode 100644 src/__tests__/ci-workflow-gate.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 357efec7..7f98847c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,11 +1,16 @@ name: CI # ABOUT THIS WORKFLOW -# - Purpose: Validate code quality, type safety, formatting, and tests on release PRs. -# - Trigger: Automatic only on release PRs (head branch `release/*` or `*release-v*`), -# e.g. release/v26.5.17, chore/release-v26.6.14, codex/release-v26.6.16. -# Regular PRs skip CI by default — run it manually via workflow_dispatch when needed. -# - Jobs: Three parallel tracks — quality (lint/types/format), test (frontend+API), build verification. +# - Purpose: Validate code quality, type safety, formatting, and tests before merge. +# - Trigger: Every non-draft pull request regardless of base branch (main, release/*, +# epic/**), including dependency-bot PRs; pushes to main and epic/** so the integrated +# head is validated; and manual workflow_dispatch. Draft PRs skip the heavy jobs until +# they are marked ready for review. +# - Jobs: quality (lint/types/format), test (frontend+API), build verification, and +# `CI Gate`, the single aggregate job that branch protection should require. It passes +# only when quality, test, and build all succeeded. A skipped or cancelled required job +# is a failure. The only accepted skips are a draft PR (GitHub cannot merge drafts) and +# Browser E2E when it was not requested via workflow_dispatch. # - Caching: pnpm store cached via actions/setup-node built-in cache support. # # CUSTOMIZATION TIPS @@ -15,6 +20,8 @@ name: CI on: pull_request: types: [opened, synchronize, reopened, ready_for_review] + push: + branches: [main, 'epic/**'] workflow_dispatch: inputs: run_e2e: @@ -35,13 +42,7 @@ jobs: quality: name: Lint, Types & Format runs-on: ubuntu-latest - if: | - (github.event_name == 'workflow_dispatch' || - (!github.event.pull_request.draft && - (startsWith(github.head_ref, 'release/') || contains(github.head_ref, 'release-v')))) && - !contains(github.actor, 'dependabot') && - !contains(github.actor, 'renovate') && - !contains(github.actor, '[bot]') + if: github.event_name != 'pull_request' || !github.event.pull_request.draft steps: - name: Checkout @@ -78,13 +79,7 @@ jobs: test: name: Test runs-on: ubuntu-latest - if: | - (github.event_name == 'workflow_dispatch' || - (!github.event.pull_request.draft && - (startsWith(github.head_ref, 'release/') || contains(github.head_ref, 'release-v')))) && - !contains(github.actor, 'dependabot') && - !contains(github.actor, 'renovate') && - !contains(github.actor, '[bot]') + if: github.event_name != 'pull_request' || !github.event.pull_request.draft steps: - name: Checkout @@ -200,3 +195,46 @@ jobs: run: pnpm build env: NODE_OPTIONS: '--max-old-space-size=4096' + + # ─── Aggregate Gate ────────────────────────────────────────── + # Stable required check. Runs even when upstream jobs fail, skip, or are cancelled. + ci-gate: + name: CI Gate + runs-on: ubuntu-latest + needs: [quality, test, browser-e2e, build] + if: always() + + steps: + - name: Evaluate required jobs + env: + IS_DRAFT_PR: ${{ github.event_name == 'pull_request' && github.event.pull_request.draft }} + RUN_E2E: ${{ github.event.inputs.run_e2e }} + QUALITY_RESULT: ${{ needs.quality.result }} + TEST_RESULT: ${{ needs.test.result }} + BUILD_RESULT: ${{ needs.build.result }} + BROWSER_E2E_RESULT: ${{ needs.browser-e2e.result }} + run: | + if [ "$IS_DRAFT_PR" = "true" ]; then + echo "::notice::Draft pull request: required jobs are skipped until it is marked ready for review. GitHub cannot merge a draft." + exit 0 + fi + + status=0 + require() { + if [ "$2" != "$3" ]; then + echo "::error::$1 finished as '$2', expected '$3'." + status=1 + fi + } + + require "Lint, Types & Format" "$QUALITY_RESULT" success + require "Test" "$TEST_RESULT" success + require "Build" "$BUILD_RESULT" success + + if [ "$RUN_E2E" = "true" ]; then + require "Browser E2E" "$BROWSER_E2E_RESULT" success + else + require "Browser E2E (not requested)" "$BROWSER_E2E_RESULT" skipped + fi + + exit "$status" diff --git a/src/__tests__/ci-workflow-gate.test.ts b/src/__tests__/ci-workflow-gate.test.ts new file mode 100644 index 00000000..f0489ad4 --- /dev/null +++ b/src/__tests__/ci-workflow-gate.test.ts @@ -0,0 +1,180 @@ +import { describe, expect, it } from 'vitest'; + +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; + +/** + * Regression coverage for issue #196: the quality and test jobs used to run only + * on `release/*` heads, so ordinary PRs (and PRs into `epic/**`) showed green + * check names without running anything. These tests pin the workflow text and + * exercise the aggregate gate's decision logic the way GitHub runs it. + */ +const workflowPath = path.resolve(process.cwd(), '.github/workflows/ci.yml'); +const workflow = readFileSync(workflowPath, 'utf-8'); + +function extractJobBlock(jobId: string): string { + const lines = workflow.split('\n'); + const start = lines.findIndex((line) => line === ` ${jobId}:`); + if (start === -1) + throw new Error(`job "${jobId}" not found in ${workflowPath}`); + + const block: string[] = []; + for (let i = start + 1; i < lines.length; i += 1) { + const line = lines[i]; + if (/^ {2}\S/.test(line)) break; + block.push(line); + } + return block.join('\n'); +} + +function extractRunScript(jobId: string): string { + const block = extractJobBlock(jobId); + const lines = block.split('\n'); + const runIndex = lines.findIndex((line) => /^\s+run: \|$/.test(line)); + if (runIndex === -1) throw new Error(`job "${jobId}" has no "run: |" step`); + + const body: string[] = []; + for (let i = runIndex + 1; i < lines.length; i += 1) { + const line = lines[i]; + if (line.trim() !== '' && !/^\s{8,}/.test(line)) break; + body.push(line); + } + return body + .map((line) => line.replace(/^ {8}/, '')) + .join('\n') + .trim(); +} + +const GATE_SCRIPT = extractRunScript('ci-gate'); + +function runGate(env: Record): { + status: number; + stdout: string; +} { + try { + const stdout = execFileSync('bash', ['-c', GATE_SCRIPT], { + env: { PATH: process.env.PATH ?? '', ...env }, + encoding: 'utf-8', + }); + return { status: 0, stdout }; + } catch (error) { + const failure = error as { status?: number; stdout?: string }; + return { status: failure.status ?? 1, stdout: failure.stdout ?? '' }; + } +} + +const passingResults = { + IS_DRAFT_PR: 'false', + RUN_E2E: 'false', + QUALITY_RESULT: 'success', + TEST_RESULT: 'success', + BUILD_RESULT: 'success', + BROWSER_E2E_RESULT: 'skipped', +}; + +describe('ci.yml triggers', () => { + it('runs on pull requests and on pushes to main and epic branches', () => { + expect(workflow).toMatch(/^on:\n {2}pull_request:/m); + expect(workflow).toMatch( + /^ {2}push:\n {4}branches: \[main, 'epic\/\*\*'\]$/m, + ); + }); + + it('does not restrict the quality or test jobs to release heads', () => { + expect(workflow).not.toContain("startsWith(github.head_ref, 'release/')"); + expect(workflow).not.toContain("contains(github.head_ref, 'release-v')"); + const draftGate = + "github.event_name != 'pull_request' || !github.event.pull_request.draft"; + expect(workflow.split(draftGate)).toHaveLength(3); + }); + + it('does not exempt dependency-bot actors', () => { + expect(workflow).not.toContain("contains(github.actor, 'dependabot')"); + expect(workflow).not.toContain("contains(github.actor, 'renovate')"); + }); + + it('pins every action reference to a full commit SHA', () => { + const uses = workflow + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.startsWith('uses:')); + expect(uses.length).toBeGreaterThan(0); + for (const use of uses) { + expect(use).toMatch(/^uses: [^@\s]+@[0-9a-f]{40}( # v\d+)?$/); + } + }); +}); + +describe('CI Gate aggregate job', () => { + it('depends on every required job and always evaluates', () => { + const block = extractJobBlock('ci-gate'); + expect(block).toMatch( + /^ {4}needs: \[quality, test, browser-e2e, build\]$/m, + ); + expect(block).toMatch(/^ {4}if: always\(\)$/m); + }); + + it('passes when every required job succeeded', () => { + expect(runGate(passingResults).status).toBe(0); + }); + + it('passes a draft pull request, which GitHub cannot merge', () => { + const result = runGate({ + ...passingResults, + IS_DRAFT_PR: 'true', + QUALITY_RESULT: 'skipped', + TEST_RESULT: 'skipped', + BUILD_RESULT: 'skipped', + }); + expect(result.status).toBe(0); + expect(result.stdout).toContain('Draft pull request'); + }); + + it('fails when a required job fails or is cancelled', () => { + for (const key of [ + 'QUALITY_RESULT', + 'TEST_RESULT', + 'BUILD_RESULT', + ] as const) { + expect(runGate({ ...passingResults, [key]: 'failure' }).status).not.toBe( + 0, + ); + expect( + runGate({ ...passingResults, [key]: 'cancelled' }).status, + ).not.toBe(0); + } + }); + + it('fails when a required job was skipped, which is not a pass', () => { + for (const key of [ + 'QUALITY_RESULT', + 'TEST_RESULT', + 'BUILD_RESULT', + ] as const) { + const result = runGate({ ...passingResults, [key]: 'skipped' }); + expect(result.status).not.toBe(0); + expect(result.stdout).toContain("expected 'success'"); + } + }); + + it('requires browser E2E only when it was requested', () => { + expect( + runGate({ + ...passingResults, + RUN_E2E: 'true', + BROWSER_E2E_RESULT: 'failure', + }).status, + ).not.toBe(0); + expect( + runGate({ + ...passingResults, + RUN_E2E: 'true', + BROWSER_E2E_RESULT: 'success', + }).status, + ).toBe(0); + expect( + runGate({ ...passingResults, BROWSER_E2E_RESULT: 'failure' }).status, + ).not.toBe(0); + }); +}); From 0fad805f5bf2d01b9b9eafb376666059f1469c1d Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 00:15:56 -0400 Subject: [PATCH 05/16] ci: run quality and test jobs on every non-draft PR (#232) The quality and test jobs were gated on a release/* or release-v* head branch, so an ordinary PR, including a PR into an epic branch, reported green check names without running anything. LESSONS.md records 77 of 84 merged PRs never running them. Run quality and test on every non-draft pull request regardless of base branch, keep them off drafts (which cannot be merged), and stop excluding dependency-bot actors. Add a push trigger for main and epic/** so the integrated head is validated too. Add a ci-gate job named "CI Gate" that needs quality, test, browser-e2e and build and runs with if: always(). It succeeds only when quality, test and build all succeeded; a skipped, cancelled or failed required job is a failure, so a skipped job cannot read as green. Browser E2E is accepted only when it was not requested, and a draft PR is accepted because GitHub cannot merge it. Values reach the script through env, and every action reference remains a full commit SHA. Refs #196 (cherry picked from commit 952c9258e7a9c4c88b0384bce53689bc96fc1e1d) --- .github/workflows/ci.yml | 76 ++++++++--- src/__tests__/ci-workflow-gate.test.ts | 180 +++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 19 deletions(-) create mode 100644 src/__tests__/ci-workflow-gate.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 357efec7..7f98847c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,11 +1,16 @@ name: CI # ABOUT THIS WORKFLOW -# - Purpose: Validate code quality, type safety, formatting, and tests on release PRs. -# - Trigger: Automatic only on release PRs (head branch `release/*` or `*release-v*`), -# e.g. release/v26.5.17, chore/release-v26.6.14, codex/release-v26.6.16. -# Regular PRs skip CI by default — run it manually via workflow_dispatch when needed. -# - Jobs: Three parallel tracks — quality (lint/types/format), test (frontend+API), build verification. +# - Purpose: Validate code quality, type safety, formatting, and tests before merge. +# - Trigger: Every non-draft pull request regardless of base branch (main, release/*, +# epic/**), including dependency-bot PRs; pushes to main and epic/** so the integrated +# head is validated; and manual workflow_dispatch. Draft PRs skip the heavy jobs until +# they are marked ready for review. +# - Jobs: quality (lint/types/format), test (frontend+API), build verification, and +# `CI Gate`, the single aggregate job that branch protection should require. It passes +# only when quality, test, and build all succeeded. A skipped or cancelled required job +# is a failure. The only accepted skips are a draft PR (GitHub cannot merge drafts) and +# Browser E2E when it was not requested via workflow_dispatch. # - Caching: pnpm store cached via actions/setup-node built-in cache support. # # CUSTOMIZATION TIPS @@ -15,6 +20,8 @@ name: CI on: pull_request: types: [opened, synchronize, reopened, ready_for_review] + push: + branches: [main, 'epic/**'] workflow_dispatch: inputs: run_e2e: @@ -35,13 +42,7 @@ jobs: quality: name: Lint, Types & Format runs-on: ubuntu-latest - if: | - (github.event_name == 'workflow_dispatch' || - (!github.event.pull_request.draft && - (startsWith(github.head_ref, 'release/') || contains(github.head_ref, 'release-v')))) && - !contains(github.actor, 'dependabot') && - !contains(github.actor, 'renovate') && - !contains(github.actor, '[bot]') + if: github.event_name != 'pull_request' || !github.event.pull_request.draft steps: - name: Checkout @@ -78,13 +79,7 @@ jobs: test: name: Test runs-on: ubuntu-latest - if: | - (github.event_name == 'workflow_dispatch' || - (!github.event.pull_request.draft && - (startsWith(github.head_ref, 'release/') || contains(github.head_ref, 'release-v')))) && - !contains(github.actor, 'dependabot') && - !contains(github.actor, 'renovate') && - !contains(github.actor, '[bot]') + if: github.event_name != 'pull_request' || !github.event.pull_request.draft steps: - name: Checkout @@ -200,3 +195,46 @@ jobs: run: pnpm build env: NODE_OPTIONS: '--max-old-space-size=4096' + + # ─── Aggregate Gate ────────────────────────────────────────── + # Stable required check. Runs even when upstream jobs fail, skip, or are cancelled. + ci-gate: + name: CI Gate + runs-on: ubuntu-latest + needs: [quality, test, browser-e2e, build] + if: always() + + steps: + - name: Evaluate required jobs + env: + IS_DRAFT_PR: ${{ github.event_name == 'pull_request' && github.event.pull_request.draft }} + RUN_E2E: ${{ github.event.inputs.run_e2e }} + QUALITY_RESULT: ${{ needs.quality.result }} + TEST_RESULT: ${{ needs.test.result }} + BUILD_RESULT: ${{ needs.build.result }} + BROWSER_E2E_RESULT: ${{ needs.browser-e2e.result }} + run: | + if [ "$IS_DRAFT_PR" = "true" ]; then + echo "::notice::Draft pull request: required jobs are skipped until it is marked ready for review. GitHub cannot merge a draft." + exit 0 + fi + + status=0 + require() { + if [ "$2" != "$3" ]; then + echo "::error::$1 finished as '$2', expected '$3'." + status=1 + fi + } + + require "Lint, Types & Format" "$QUALITY_RESULT" success + require "Test" "$TEST_RESULT" success + require "Build" "$BUILD_RESULT" success + + if [ "$RUN_E2E" = "true" ]; then + require "Browser E2E" "$BROWSER_E2E_RESULT" success + else + require "Browser E2E (not requested)" "$BROWSER_E2E_RESULT" skipped + fi + + exit "$status" diff --git a/src/__tests__/ci-workflow-gate.test.ts b/src/__tests__/ci-workflow-gate.test.ts new file mode 100644 index 00000000..f0489ad4 --- /dev/null +++ b/src/__tests__/ci-workflow-gate.test.ts @@ -0,0 +1,180 @@ +import { describe, expect, it } from 'vitest'; + +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; + +/** + * Regression coverage for issue #196: the quality and test jobs used to run only + * on `release/*` heads, so ordinary PRs (and PRs into `epic/**`) showed green + * check names without running anything. These tests pin the workflow text and + * exercise the aggregate gate's decision logic the way GitHub runs it. + */ +const workflowPath = path.resolve(process.cwd(), '.github/workflows/ci.yml'); +const workflow = readFileSync(workflowPath, 'utf-8'); + +function extractJobBlock(jobId: string): string { + const lines = workflow.split('\n'); + const start = lines.findIndex((line) => line === ` ${jobId}:`); + if (start === -1) + throw new Error(`job "${jobId}" not found in ${workflowPath}`); + + const block: string[] = []; + for (let i = start + 1; i < lines.length; i += 1) { + const line = lines[i]; + if (/^ {2}\S/.test(line)) break; + block.push(line); + } + return block.join('\n'); +} + +function extractRunScript(jobId: string): string { + const block = extractJobBlock(jobId); + const lines = block.split('\n'); + const runIndex = lines.findIndex((line) => /^\s+run: \|$/.test(line)); + if (runIndex === -1) throw new Error(`job "${jobId}" has no "run: |" step`); + + const body: string[] = []; + for (let i = runIndex + 1; i < lines.length; i += 1) { + const line = lines[i]; + if (line.trim() !== '' && !/^\s{8,}/.test(line)) break; + body.push(line); + } + return body + .map((line) => line.replace(/^ {8}/, '')) + .join('\n') + .trim(); +} + +const GATE_SCRIPT = extractRunScript('ci-gate'); + +function runGate(env: Record): { + status: number; + stdout: string; +} { + try { + const stdout = execFileSync('bash', ['-c', GATE_SCRIPT], { + env: { PATH: process.env.PATH ?? '', ...env }, + encoding: 'utf-8', + }); + return { status: 0, stdout }; + } catch (error) { + const failure = error as { status?: number; stdout?: string }; + return { status: failure.status ?? 1, stdout: failure.stdout ?? '' }; + } +} + +const passingResults = { + IS_DRAFT_PR: 'false', + RUN_E2E: 'false', + QUALITY_RESULT: 'success', + TEST_RESULT: 'success', + BUILD_RESULT: 'success', + BROWSER_E2E_RESULT: 'skipped', +}; + +describe('ci.yml triggers', () => { + it('runs on pull requests and on pushes to main and epic branches', () => { + expect(workflow).toMatch(/^on:\n {2}pull_request:/m); + expect(workflow).toMatch( + /^ {2}push:\n {4}branches: \[main, 'epic\/\*\*'\]$/m, + ); + }); + + it('does not restrict the quality or test jobs to release heads', () => { + expect(workflow).not.toContain("startsWith(github.head_ref, 'release/')"); + expect(workflow).not.toContain("contains(github.head_ref, 'release-v')"); + const draftGate = + "github.event_name != 'pull_request' || !github.event.pull_request.draft"; + expect(workflow.split(draftGate)).toHaveLength(3); + }); + + it('does not exempt dependency-bot actors', () => { + expect(workflow).not.toContain("contains(github.actor, 'dependabot')"); + expect(workflow).not.toContain("contains(github.actor, 'renovate')"); + }); + + it('pins every action reference to a full commit SHA', () => { + const uses = workflow + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.startsWith('uses:')); + expect(uses.length).toBeGreaterThan(0); + for (const use of uses) { + expect(use).toMatch(/^uses: [^@\s]+@[0-9a-f]{40}( # v\d+)?$/); + } + }); +}); + +describe('CI Gate aggregate job', () => { + it('depends on every required job and always evaluates', () => { + const block = extractJobBlock('ci-gate'); + expect(block).toMatch( + /^ {4}needs: \[quality, test, browser-e2e, build\]$/m, + ); + expect(block).toMatch(/^ {4}if: always\(\)$/m); + }); + + it('passes when every required job succeeded', () => { + expect(runGate(passingResults).status).toBe(0); + }); + + it('passes a draft pull request, which GitHub cannot merge', () => { + const result = runGate({ + ...passingResults, + IS_DRAFT_PR: 'true', + QUALITY_RESULT: 'skipped', + TEST_RESULT: 'skipped', + BUILD_RESULT: 'skipped', + }); + expect(result.status).toBe(0); + expect(result.stdout).toContain('Draft pull request'); + }); + + it('fails when a required job fails or is cancelled', () => { + for (const key of [ + 'QUALITY_RESULT', + 'TEST_RESULT', + 'BUILD_RESULT', + ] as const) { + expect(runGate({ ...passingResults, [key]: 'failure' }).status).not.toBe( + 0, + ); + expect( + runGate({ ...passingResults, [key]: 'cancelled' }).status, + ).not.toBe(0); + } + }); + + it('fails when a required job was skipped, which is not a pass', () => { + for (const key of [ + 'QUALITY_RESULT', + 'TEST_RESULT', + 'BUILD_RESULT', + ] as const) { + const result = runGate({ ...passingResults, [key]: 'skipped' }); + expect(result.status).not.toBe(0); + expect(result.stdout).toContain("expected 'success'"); + } + }); + + it('requires browser E2E only when it was requested', () => { + expect( + runGate({ + ...passingResults, + RUN_E2E: 'true', + BROWSER_E2E_RESULT: 'failure', + }).status, + ).not.toBe(0); + expect( + runGate({ + ...passingResults, + RUN_E2E: 'true', + BROWSER_E2E_RESULT: 'success', + }).status, + ).toBe(0); + expect( + runGate({ ...passingResults, BROWSER_E2E_RESULT: 'failure' }).status, + ).not.toBe(0); + }); +}); From 9b67b60ad57d8331ec2efd9df3f0defed4ee1f06 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 01:06:43 -0400 Subject: [PATCH 06/16] docs(process): add the weekly and pre-release maintenance review runbook (#238) Define the recurring maintenance review in doc/maintenance/review-process.md: cadence and owner, the review scope, the report format, how bounded cleanup pull requests are selected, and the LESSONS.md step for adding, incrementing, and promoting a lesson. Record the first dry run in doc/maintenance/2026-10-14-weekly-dry-run/README.md against head d182f23f, in the documented format, with the real check output and the checks that could not run. Scheduling a recurring job is a separate maintainer decision and is not live. Closes #230 --- .../2026-10-14-weekly-dry-run/README.md | 153 ++++++++++++++ doc/maintenance/review-process.md | 194 ++++++++++++++++++ 2 files changed, 347 insertions(+) create mode 100644 doc/maintenance/2026-10-14-weekly-dry-run/README.md create mode 100644 doc/maintenance/review-process.md diff --git a/doc/maintenance/2026-10-14-weekly-dry-run/README.md b/doc/maintenance/2026-10-14-weekly-dry-run/README.md new file mode 100644 index 00000000..ed6c79eb --- /dev/null +++ b/doc/maintenance/2026-10-14-weekly-dry-run/README.md @@ -0,0 +1,153 @@ +# Weekly maintenance review, 2026-10-14 + +Dry run of the process in [review-process.md](../review-process.md), produced +against this worktree's checked head. It exercises the report format and records +real check output. It is not a scheduled run; no recurring job exists. + +Owner: unassigned. The owner field is defined in +[review-process.md](../review-process.md#cadence-and-owner). This dry run was +produced by an implementing agent, and assigning the recurring owner is a +maintainer decision tracked in [#199](https://github.com/bravew/Neumar/issues/199). +Reviewer: implementing agent for #230. +Head: `d182f23f27100a18066d5804f2ec4f774f41dd98` (`issue/230-weekly-maintenance-review`, +rebased onto `origin/epic/194-maintenance-process`). +Node and pnpm: Node v26.11.1, pnpm 11.22.0. The Node baseline CI uses is 22; see +Unverified. +Previous report: none. This is the first run, so every trend is a baseline. + +## Comparability with the earlier review + +The earlier review was measured at `c0926618`. This head differs from it only in +tracked documentation: `git diff --stat c0926618 HEAD -- src src-api packages` +is empty, so the source, API, and shared-package trees are byte-identical. The +warning counts, allowlist entries, suppression counts, orphan candidates, and +migration files below are therefore directly comparable to the baseline. No +product behavior changed between the two heads. + +## Check outcomes + +Every command below was run in this worktree on the head above. The exit code and +counts are the real output. + +| Command | Result | Notes | +| --- | --- | --- | +| `pnpm validate` | Pass, exit 0 | Emitted 453 frontend lint warnings and 0 lint errors. All sub-checks passed: brand, critique-i18n, locale parity, design-system flags, design media catalog, uuid routing, ag-ui version (0.0.59 lockstep), plugin registry (226 entries), HyperFrames skill, FFmpeg skill (2.4.2, 42 tools), HyperFrames upgrade check, frontend and API typecheck, root and API format, component size (897 files scanned, 68 allowlisted). The HyperFrames upgrade check reports an available update 0.8.80 to 0.8.155 and changed no files. | +| `pnpm test:fast` | Fail, exit 1 | Frontend: 399 files, 1776 tests, all passed. API: 2 failed, 593 passed, 3 skipped files; 2 failed, 3859 passed, 7 skipped tests. Both failures are detailed below. | +| `pnpm --filter neumar-api lint` | Pass, exit 0 | 10 warnings, 0 errors. | +| `pnpm --filter neumar-api exec tsc -p tsconfig.json --noEmit` | Pass, exit 0 | Includes API test files. | +| `pnpm lint` (frontend) | Pass, exit 0 | 453 warnings, 0 errors. | +| `cargo check --manifest-path src-tauri/Cargo.toml --locked` | Fail, exit 101 | Failed in the `neumar` build script because the sidecar resource `src-api/dist/neumar-api-aarch64-apple-darwin` does not exist. This is a missing build artifact, not a source defect; see Unverified. | + +### `pnpm test:fast` failures + +1. `test/unit/agent-runtimes/resolve.test.ts` line 251, "resolves + npm-installed OpenCode through the shared extended PATH". This is the known + test-isolation failure tracked in [#195](https://github.com/bravew/Neumar/issues/195) + and named in [LESSONS.md](../../../LESSONS.md). The resolver finds the host's + Homebrew OpenCode before the temporary npm fixture. It reproduces on this host + because Homebrew OpenCode is installed. +2. `test/unit/plugins/loader.test.ts`, "invalidates loader generation when + watched skill files change". The failure is `Error: Hook timed out in + 30000ms` in the file's `afterEach`, which tears down the plugin hot-reload + watcher. Re-running the file alone passed: 7 tests passed in 22.00s. This is a + load-sensitive timing failure under the full parallel run, not a reproduction + in isolation. It is a new observation with no tracking issue, and it is + selected for a bounded cleanup pull request below. It is not the `#195` + failure and should not be reported as one. + +## Findings + +Severity follows the earlier review: P0 urgent containment, P1 fix before +expanding the affected feature, P2 planned cleanup. A finding carried from the +earlier review names its source review and commit. + +| ID | Surface | Finding | Evidence | Owner | Status | +| --- | --- | --- | --- | --- | --- | +| W1 | New warnings and suppressions | The frontend warning baseline is unchanged at 453, but 28 `eslint-disable-next-line` directives exist under `src/`, of which 25 suppress `react-hooks/exhaustive-deps`. API code adds 71 more, and 5 `@ts-expect-error` directives exist. No new suppression was introduced in this window. | `pnpm lint`, `grep -rn eslint-disable-next-line src/` | unassigned | Open, carried from the 2026-10-13 review | +| C1 | Size ceilings | The allowlist still holds 68 entries and no ceiling changed in this window. Nine entries are obsolete: the file is at or below the 350-line default, so the allowance is unnecessary. | `scripts/component-size-allowlist.json`; `node scripts/check-component-size.mjs` | unassigned | Open, carried from the 2026-10-13 review; tracked in [#217](https://github.com/bravew/Neumar/issues/217) | +| O1 | Orphan candidates | Four API modules named in the earlier review remain present with no importer found by exact-path search: `extensions/mcp/sandbox-server.ts`, `core/config/hierarchy.ts`, `shared/services/pipeline/cli-dispatch.ts`, `shared/services/video-analysis.ts`. This run confirmed the absence of importers, not their removal. | `grep -rn` for each path under `src-api/src` | unassigned | Open, carried from the 2026-10-13 review; tracked in [#218](https://github.com/bravew/Neumar/issues/218) | +| U1 | Unsupported surfaces | The chat plugin apply route returns 501, and the memory extract and consolidate routes return 501. All three messages are truthful and direct callers elsewhere. Both `/task/:taskId` and `/task-v2/:taskId` remain mounted, and both settings shells remain live. No change in this window. | `src-api/src/app/api/plugins.ts:330`, `src-api/src/app/api/memory.ts:497`, `:524`, `src/app/router.tsx:57,65` | unassigned | Open, carried from the 2026-10-13 review; tracked in [#223](https://github.com/bravew/Neumar/issues/223) | +| M1 | Migrations | The migrations directory holds 60 files: 58 numbered migrations plus `runner.ts` and `utils.ts`. None changed in this window. The migration-drift finding from the earlier review is unchanged and still needs the full-registry run against fresh, current, and drifted snapshots. | `git diff c0926618 HEAD -- src-api/src/shared/db` is empty | unassigned | Open, carried from the 2026-10-13 review; tracked in [#225](https://github.com/bravew/Neumar/issues/225) | +| S1 | Security | The P0 daemon binding finding is unchanged and unfixed. The default listener still omits `hostname`, and privileged routes are reachable without JWT unless `WEBUI_AUTH=true`. | `src-api/src/http-daemon.ts:478`, `src-api/src/app/middleware/jwt.ts:12`, `src-api/src/app/api/sandbox.ts:179` | unassigned | Open, carried from the 2026-10-13 review; tracked in [#200](https://github.com/bravew/Neumar/issues/200) | +| S2-S9 | Security | The remaining security findings S2 through S9 are unchanged. The source trees are identical to the reviewed baseline, so none has been fixed. | [01-findings.md](../2026-10-13-regression/01-findings.md) | unassigned | Open, carried from the 2026-10-13 review; tracked in [#201](https://github.com/bravew/Neumar/issues/201) to [#207](https://github.com/bravew/Neumar/issues/207) | +| R1-R6 | Unresolved regressions | All six regressions from the earlier review are unchanged. R6 (the `resolve.test.ts` isolation bug) reproduced in this run. | `pnpm test:fast`; [01-findings.md](../2026-10-13-regression/01-findings.md) | unassigned | Open, carried from the 2026-10-13 review; tracked in [#211](https://github.com/bravew/Neumar/issues/211) to [#216](https://github.com/bravew/Neumar/issues/216) and [#195](https://github.com/bravew/Neumar/issues/195) | +| T1 | Unresolved regression (new) | The plugin loader test times out under the full parallel run and passes alone. This is a load-sensitive timing assumption a user meets on a busy machine, and it is the class [LESSONS.md](../../../LESSONS.md) calls out. | `pnpm test:fast` (2 failed), isolated `pnpm test:api test/unit/plugins/loader.test.ts` (7 passed, 22.00s) | unassigned | New, observed in this run | +| E1 | Expired exceptions | No exception registry exists yet. Exceptions are named in the earlier review's findings, but none carries an owner, a reason, and an expiry in a machine-readable place, so none can be listed as expired. | [review-process.md](../review-process.md#review-scope) surface 8 | unassigned | New, observed in this run | + +## Metrics + +The previous column is the value the 2026-10-13 review recorded, carried from +[README.md](../2026-10-13-regression/README.md) and +[03-lessons-and-contributors.md](../2026-10-13-regression/03-lessons-and-contributors.md). +This run is the baseline for the metrics the earlier review did not record. + +| Metric | This run | Previous | Trend | +| --- | ---: | ---: | --- | +| Escaped regressions introduced since the last report | 1 (T1, new) | 6 (R1 to R6) | Baseline. R1 to R6 predate this run and are not counted again. | +| Time to repair, escaped regressions closed since the last report | Not applicable, none closed | R6 introduced `83c425bb`, still open | Baseline. R6 has been open since the initial snapshot. | +| Required-check execution on merged heads | Not measured. No merges in the window, and the CI job conditions were read rather than exercised. | 77 of 84 merged PRs never ran the automatic quality and test jobs | Unchanged by this run. The `quality` and `test` jobs in `.github/workflows/ci.yml` still require a `release/` or `release-v` head or a manual dispatch. | +| Unresolved P0 findings | 1 (S1) | 1 (S1) | No change. | +| Unresolved P1 findings | 9 (S2, S3, S4, S5, S6, S7, S9, R1, R3) | same set | No change. Source trees are identical to the baseline. | +| Expired exceptions | 0, because no registry exists | not recorded | Baseline. | + +Deletion count is not a metric and is not recorded. + +## Cleanup selected + +These are the bounded cleanup pull requests this run selects. Each names one +finding and the check that will show the change. None is started by this report. + +| Scope | Finding | Closing check | +| --- | --- | --- | +| Isolate the plugin hot-reload teardown in `src-api/test/unit/plugins/loader.test.ts` so the watcher closes within the hook timeout, and assert the watcher stops deterministically instead of relying on a 30s hook budget. | T1 | `pnpm test:api test/unit/plugins/loader.test.ts` passes, and `pnpm test:fast` shows no hook timeout. | +| Remove the nine obsolete allowlist entries whose files already fit the 350-line default. | C1 | `pnpm check:component-size` passes with 59 allowlisted entries. | +| Remove one proven API orphan, starting with the smallest, after an exact-path caller search across the workspaces and the CLI and plugin entry points. | O1 | `pnpm --filter neumar-api lint`, API typecheck, and the affected API tests pass. | + +The other findings are carried, not selected this run. S1 remains the highest +priority and is already tracked in [#200](https://github.com/bravew/Neumar/issues/200). + +## LESSONS.md + +No change. This run found no failure pattern that is not already a lesson. The +two failures it observed map to existing rules: the `resolve.test.ts` failure is +the "test isolation: stub the seam, not the environment" lesson, and the plugin +loader timeout is the "a red under load is a timing assumption a user will meet +on a busy machine" clause of the "when a check fails in your area, fix it or file +it" lesson. Neither adds a new pattern, so neither "Seen" count is incremented +here; incrementing requires a review day, and this is a dry run. + +The promoted lesson, seen on three review days and moved to +[AGENTS.md](../../../AGENTS.md), is the same process the runbook defines. This dry +run is the first review day for the new process itself, so nothing is promoted +yet. + +## Unverified + +- **Node 22 baseline.** This run used Node v26.11.1, the same local runner the + earlier review used. CI uses Node 22. The `resolve.test.ts` and plugin loader + failures were not reproduced under Node 22. +- **`cargo check`.** The command failed in the Tauri build script because the + sidecar resource `src-api/dist/neumar-api-aarch64-apple-darwin` is absent. The + Rust source was not checked. Producing that artifact needs the API binary build + step, which was not run. This is not evidence of a Rust defect. +- **Browser end-to-end tests.** `pnpm test:e2e:browser` was not run. It starts its + own API and a browser, and no browser scenario was in scope for a + documentation change. +- **Gate evals.** `pnpm test:gate` was not run. It needs `EVALS_TIER=gate` and + the eval fixtures, and no eval surface changed. +- **Live providers and packaged desktop.** No provider credentials were used and + no packaged build was produced, so no live-provider or desktop behavior is + verified. +- **Required-check execution on merged heads.** Read from + `.github/workflows/ci.yml` job conditions, not exercised against a pull request + in this window. The metric is carried from the earlier review, not re-measured. +- **Exception registry.** No machine-readable exception registry exists, so the + expired-exception metric cannot be measured beyond "none recorded". +- **Link check.** Every relative link in the runbook and this report was resolved + against the checkout by hand; no automated link checker was run. + +## Scheduling + +Scheduling the review is a separate maintainer decision and is not live. No +workflow, cron, or external job runs this process. This report is a manual dry +run. diff --git a/doc/maintenance/review-process.md b/doc/maintenance/review-process.md new file mode 100644 index 00000000..b966ef2d --- /dev/null +++ b/doc/maintenance/review-process.md @@ -0,0 +1,194 @@ +# Weekly and pre-release maintenance review + +This runbook defines the recurring maintenance review. It is written so one +person or one agent can run it from a checkout, publish a report, and select +bounded cleanup work without inventing a process each time. + +The review exists because a fix that nothing rechecks decays. The first review +([2026-10-13 regression review](2026-10-13-regression/README.md)) found that 77 +of 84 merged pull requests never ran the automatic quality and test jobs, that +four release pull requests merged with a quality or test failure already +completed on the matching head, and that four recent feature pull requests +proved the wrong layer. Repeating the review is how the repository notices the +same class of failure coming back. + +Scheduling is not part of this document. A recurring job, a scheduled workflow, +or an external reminder is a separate maintainer decision, and none is live. The +process below is run by hand until the maintainer decides otherwise. + +## Cadence and owner + +- **Weekly.** Run the review once a week against the current default branch head. +- **Pre-release.** Run the same review against the exact head to be released + before any `release:new` or `release:publish` step. The pre-release run is the + gate, so it does not wait for the weekly slot. +- **Owner.** Every report names one accountable owner in its `Owner` field, plus + the reviewer who produced it when they differ. The owner is the person + answerable for the open findings and the cleanup selections in that report, not + every contributor named in it. When no owner is recorded, the report is + incomplete and the review did not happen. + +A weekly run that finds nothing new still publishes a report. The report records +what was checked and the exact check outcomes, so a quiet week is visible +evidence rather than an absent one. + +## Review scope + +Each run inspects these surfaces. A surface with no change is reported as +unchanged with the command or query that showed it. + +1. **New lint warnings and suppressions.** Compare the warning count and the + per-rule breakdown against the previous report. Count new `eslint-disable`, + `oxlint-disable`, `@ts-ignore`, `@ts-expect-error`, and `@ts-nocheck` + directives. A new suppression in changed code is a finding unless a reviewed + exception with an owner, a reason, and an expiry exists. See + [LESSONS.md](../../LESSONS.md), "Never raise a threshold or allowlist ceiling + to make a check pass." +2. **Size-ceiling changes.** Diff + [component-size-allowlist.json](../../scripts/component-size-allowlist.json). + A raised `current` ceiling is a finding. A lowered ceiling or a removed entry + is progress. Do not treat a deletion count as a quality measure on its own. +3. **Orphan candidates.** Recheck the dead-code candidates from the previous + report and the list in [01-findings.md](2026-10-13-regression/01-findings.md). + A candidate is only removed after an exact-path and symbol search across every + workspace, the build entry points, the plugin manifests, and the desktop + consumers. Knip output is a lead, never a deletion authority. +4. **Unsupported feature surfaces.** Find routes that return 501 or an explicit + "not implemented" and confirm each one is intentional and truthful. A control + that offers a feature the API refuses is a finding. The current known surfaces + are the chat plugin apply route and the memory extract and consolidate routes. +5. **Migration changes.** Any change under `src-api/src/shared/db/migrations/` + gets a full-registry run against fresh, current, and drifted snapshots. A + missing table or column means unknown, never empty. Do not renumber applied + migrations. +6. **Security findings.** Recheck the open findings S1 to S9 in + [01-findings.md](2026-10-13-regression/01-findings.md). A security gap a + document names is an open bug, not a description, so a finding that is + documented but unfixed stays open until an issue tracks it and the fix lands. +7. **Unresolved regressions.** Recheck R1 to R6 and any regression added since. + A regression closes only when a test at the caller boundary fails before the + fix and passes after it. +8. **Expired exceptions.** Every exception carries an owner, a reason, and an + expiry. List the ones that have expired and the ones that expire before the + next scheduled run. An expired exception is an open finding until it is + renewed with a new reason or resolved. + +## Report format + +Publish each run as a new dated directory under `doc/maintenance/`, for example +`doc/maintenance/2026-10-14-weekly/README.md`. Use these headings in this order. +Every row that names a check records the exact command, its exit status, and the +counts it printed. Every finding links the pull request or the source path and +line that shows it. + +``` +# maintenance review, + +Owner: +Reviewer: +Head: +Node and pnpm: +Previous report: + +## Check outcomes +| Command | Result | Notes | +| --- | --- | --- | +| pnpm validate | | | +| pnpm test:fast | | | +| pnpm --filter neumar-api lint | | | +| pnpm --filter neumar-api exec tsc -p tsconfig.json --noEmit | | | +| closest changed-surface checks | | | + +## Findings +| ID | Surface | Finding | Evidence | Owner | Status | +| --- | --- | --- | --- | --- | --- | + +## Metrics +| Metric | This run | Previous | Trend | +| --- | ---: | ---: | --- | + +## Cleanup selected + + +## LESSONS.md + + +## Unverified + +``` + +The report distinguishes what this run observed from evidence carried over from +an earlier review. A carried-over line names the review and the commit it came +from. A check that could not run is listed under Unverified with the reason; it +is never reported as a pass. + +## Selecting bounded cleanup pull requests + +The review selects cleanup work, it does not perform it in the report. A cleanup +pull request is bounded when it meets all of these: + +- It has one goal and one subject line. A fix found on the way gets its own pull + request. +- It names the finding or the metric it moves, and the exact check that will show + the change. +- It migrates every caller and deletes the superseded path in the same change. +- It does not raise a threshold or an allowlist ceiling to pass a check. An + exception is recorded with an owner, a reason, and an expiry instead. +- It is small enough to review against the finding in one sitting. When a + candidate is larger, split it and select the first slice. + +The review does not reward deletion count, and it does not punish every +follow-up fix. A corrective pull request that repairs an escaped regression is +responsible work; the escaped regression is the finding, not the repair. The +review selects work by the risk the finding carries and the evidence that will +close it, not by how many lines it removes. + +## The LESSONS.md step + +Every run ends with one of these outcomes for [LESSONS.md](../../LESSONS.md): + +- **Add a lesson.** When a run finds a failure pattern that is not already a + lesson, add it with its evidence: a pull request number, a commit, or a file + and line, plus what was run and what was missed. Keep the rule first. +- **Increment a lesson.** When a run finds the same pattern again, increment that + lesson's "Seen" count and record the review date. Say which review day, not + only the count. +- **No change.** When no new pattern and no repeat was found, record that in the + report. This is a valid outcome. + +When a lesson reaches three review days, move the rule into +[AGENTS.md](../../AGENTS.md) so every agent applies it without a separate read, +and leave a pointer to it in LESSONS.md. The pointer keeps the evidence +findable; the rule lives in AGENTS.md from then on. + +## Metrics + +Record these each run. The trend across runs is the point; a single run's number +is only a baseline. + +- **Escaped regressions.** Regressions found after merge that a required check + should have caught. Count the ones introduced since the last report. +- **Time to repair.** From the merge that introduced a regression to the merge + that fixed it, for each escaped regression closed since the last report. +- **Required-check execution.** The share of merged pull requests in the window + whose required checks actually ran on the merged head. Skipped, cancelled, and + absent checks do not count as executed. +- **Unresolved high-priority findings.** The count of open P0 and P1 findings + from [01-findings.md](2026-10-13-regression/01-findings.md) plus any added + since, and how many are older than the last two runs. +- **Expired exceptions.** The count of exceptions past their expiry, and the + count expiring before the next run. + +Deletion count is not a metric. It is not recorded as progress, and a run with +more deletions is not a better run than one with fewer. + +## Related documents + +- [Regression review and maintenance plan](2026-10-13-regression/README.md): the + evidence, the plan, and the baseline this process rechecks. +- [Findings and evidence](2026-10-13-regression/01-findings.md): the S1 to S9 and + R1 to R6 findings the review rechecks. +- [PR lessons and contributor policy](2026-10-13-regression/03-lessons-and-contributors.md): + the merge metrics and the contributor policy. +- [LESSONS.md](../../LESSONS.md): the rules the review adds to and increments. +- [AGENTS.md](../../AGENTS.md): where a lesson moves after three review days. From d5ce9098e723f752e0fbe1c4f1ee07257367bf6c Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 01:06:51 -0400 Subject: [PATCH 07/16] docs(channels): add per-provider ownership and contract matrix (#243) Records what each live channel provider actually does, per provider and per runtime, with a path:line citation for every cell and the live consumer named. Covers the plugin runtime (shared/channels, Slack/Discord/Telegram/Lark), the gateway runtime (shared/services/gateway, WhatsApp/iMessage webhooks and the feishu, linear, sms adapters), and the legacy slack-gateway path. Documents inbound transport, credentials source, user identity, permissions, persistence, scheduling, and stop behavior for each. Findings recorded: the gateway runtime has no production caller of startGateway(), so its adapter map is empty and the WhatsApp and iMessage webhook routes return 503; the gateway_* tables have no migration; the legacy Slack gateway runs with no permission or persistence layer; and the two live Slack stacks are uncoordinated. Ends with a decision-needed section listing candidate canonical runtimes and stating that migration is a separate issue. No runtime is chosen here. --- doc/backend/channel-ownership.md | 391 +++++++++++++++++++++++++++++++ 1 file changed, 391 insertions(+) create mode 100644 doc/backend/channel-ownership.md diff --git a/doc/backend/channel-ownership.md b/doc/backend/channel-ownership.md new file mode 100644 index 00000000..0460cca7 --- /dev/null +++ b/doc/backend/channel-ownership.md @@ -0,0 +1,391 @@ +--- +summary: "Per-provider responsibility matrix for the two live channel runtimes and the legacy Slack gateway: inbound transport, credentials, identity, permissions, persistence, scheduling, and stop behavior, each cell cited to code, plus the decision needed on a canonical runtime" +read_when: + - Choosing or reviewing a canonical channel runtime + - Migrating a provider between the plugin runtime and the gateway runtime + - Tracing where a channel message is authenticated, persisted, or scheduled + - Reviewing the Slack duplication (ChannelManager Slack plugin vs legacy slack-gateway) +title: "Channel Ownership and Contracts" +--- + +# Channel Ownership and Contracts + +This document is the responsibility matrix requested by issue #222, part of epic +#193. It records what each live channel provider actually does today, with a +`path:line` citation for every cell and the live consumer named. It is a +pre-migration inventory, not a design. No runtime is chosen here; see +[Decision needed](#decision-needed). + +Evidence was read from the tree at `1bb20406de946723795f7f9fed20777b7fa00d58`, +which is the head of `epic/193-architecture-ownership` and the commit this branch +started from. Node v26.11.1, pnpm 11.22.0. Line numbers are valid for that +commit only; confirm them again if the file has moved since. + +## 1. Runtimes, not providers + +There are three channel code paths in the API, and they overlap by provider. + +| # | Runtime | Code | Loaded by | Live consumers | +|---|---|---|---|---| +| A | Plugin runtime ("ChannelManager") | `src-api/src/shared/channels/` | `http-daemon.ts:589-590` calls `getChannelManager().loadAndStartAll()` | `app/api/channels.ts`, `shared/automation/delivery.ts`, `shared/video/share.ts`, `shared/automation/engine.ts` | +| B | Gateway runtime | `src-api/src/shared/services/gateway/` | No production caller of `startGateway()` (`src-api/src/shared/services/gateway/index.ts:22`) | `app/api/channels.ts` (`getGateway()` at `:46`), `app/api/channels.ts:1198`, `:1263` webhook routes | +| C | Legacy Slack gateway | `src-api/src/shared/services/slack-gateway.ts` | `app/api/slack.ts:305` on `POST /slack/gateway/start`; no startup call site | `app/api/slack.ts`, `shared/services/slack-cowork-handler.ts`, `components/settings/SlackGatewaySettings.tsx` | + +Runtime B is only partially live. `getGateway()` is constructed lazily +(`core/gateway.ts:466-471`) but nothing calls `Gateway.start()` +(`core/gateway.ts:49`) in the shipped API; the only wrapper that would, +`startGateway()` (`shared/services/gateway/index.ts:22-37`), has no caller +anywhere in `src/`, `src-api/src/`, or `src-tauri/src/`. The consequence is +load-bearing for the whole matrix: + +- `getGateway().getAdapter(id)` (`core/gateway.ts:294-296`) reads + `this.adapters`, which `start()` populates at `core/gateway.ts:77-92`. With no + `start()`, that map is empty and the two webhook routes below return their + "adapter not running" responses. +- Gateway adapters receive inbound messages only through + `adapter.onMessage(...)`, which is wired in `connectChannel()` + (`core/gateway.ts:173-177`) and therefore also never runs. +- The gateway's own tables (`gateway_channels`, `gateway_identities`, + `gateway_sessions`, `gateway_messages`, `gateway_subscriptions`, + `gateway_audit_log`) are created only by `initializeGatewaySchema()` + (`shared/services/gateway/shared/db/schema.ts:13`), which has no caller. Only + `gateway_channels` and `routing_rules` exist in the shipped database, created + by migration 008 (`shared/db/migrations/008_gateway_routing_rules.ts:18`, + `:29`). Runtime A never reads the `gateway_*` tables. + +So the two webhook routes in `app/api/channels.ts` are the live part of runtime +B: they are mounted (`http-daemon.ts:236`) and reachable even though the +adapters they dispatch into are not running. That is a functional gap, not a +documentation gap, and it is recorded in [Unconfirmed and gaps](#4-unconfirmed-and-gaps) below. + +### Runtime A lifecycle + +- Startup: `loadAndStartAll()` (`channel-manager.ts:442`) reads every + `channel_config` row, instantiates the matching `BasePlugin` from + `getPluginCtors()` (`channel-manager.ts:82-98`, which imports + `./telegram/index`, `./discord/index`, `./slack/index`, `./lark/index`), then + calls `startAll()` (`:457`). +- `startAll()` requires an enabled config **and** a vault token + (`channel-manager.ts:460-467`), then takes a lease (`:474`) before + `plugin.start()` (`:481`). +- Plugin lifecycle is `BasePlugin.start/stop/restart/scheduleRestart` + (`base-plugin.ts:78`, `:95`, `:110`, `:125`). +- Shutdown: `stopAll()` (`channel-manager.ts:502`) is called from + `http-daemon.ts:403`. +- Dev gating: `channelsDisabled` (`http-daemon.ts:146-148`) skips the whole + runtime unless `NEUMA_DEV_CHANNELS=1`, and `NEUMA_DISABLE_CHANNELS=1` is a hard + kill switch. + +## 2. Per-provider matrix + +Legend: "A" plugin runtime, "B" gateway runtime, "C" legacy Slack gateway. +`shared/channels` is relative to `src-api/src/`; the gateway tree is +`src-api/src/shared/services/gateway/`. + +### 2.1 Slack + +| Field | Runtime A (live) | Runtime C (live, manual start) | Runtime B (opt-out) | +|---|---|---|---| +| Inbound | Bolt.js Socket Mode. `slack/index.ts:233-237` builds the `App` with `socketMode: true`; handlers at `:245` (`message`) and `:384` (`app_mention`) | Bolt.js Socket Mode. `slack-gateway.ts:99-103` builds the `App`; handlers at `:195` (`message`), `:230` (`app_mention`), `:258` (`assistant_thread_started`) | Deliberately disabled. `gateway/channels/slack/adapter.ts:17-25` registers a factory that logs and returns `null` | +| Live consumer | `channel-manager.ts` `handleIncomingMessage()` (`:563`); settings and admin via `app/api/channels.ts` | `app/api/slack.ts:79` subscribes `onInboundMessage`, which calls `slackCoworkHandler.handleInboundMessage()` (`slack.ts:93-97`, `slack-cowork-handler.ts:99`) | `app/api/channels.ts` registry listing (`:1142-1159`) | +| Credentials | AES-256-GCM vault, keyed by config UUID. `getChannelToken()` at `channel-manager.ts:461`; `mergeAndSaveCredential()` at `app/api/channels.ts:381`; vault at `credential-vault.ts:262` (`initCredentialVault`), `:308` (`getChannelToken`). Token shape `{botToken, appToken}`, parsed at `slack/index.ts:205-215` | Separate encrypted file `slack-config.enc.json` in the app dir. `slack-config.ts:124` (`getSlackConfigPath`), `:217` (`loadSlackConfig`), decrypted fields `botToken`/`appToken` at `:251-263`; `toGatewayConfig()` (`slack-gateway.ts:291-302`) maps them in | Gateway `channels.slack` config in `gateway.json` (`gateway/shared/config/loader.ts:21`, schema `gateway/shared/config/types.ts:77-85`) | +| User identity | Platform user ID, qualified per workspace: `buildQualifiedUserId('slack', userId, {teamId})` (`workspace.ts:35-48`, called at `channel-manager.ts:1057`). `channel_users` keyed by `(config_id, platform_user_id)` (migration `003_multi_bot.ts:58-59`) | Platform user ID plus team, qualified the same way at `slack-cowork-handler.ts:157` and `:245`. Sessions are in-memory, keyed by `teamId/channelId/threadTs` (`:103-107`, `:50-61`) | `resolveIdentity()` keyed on `(channel_id, sender_id)` (`gateway/shared/auth/identity-resolver.ts:19-46`) | +| Permissions | Pairing plus tier. `SecurityPipeline.run()` (`security/pipeline.ts:39`) checks duplicate (`:44`), pairing (`:51-66`), viewer tier (`:69`), rate limit (`:74`), token budget (`:84`), guardrails (`:98`). Slash commands handled at `channel-manager.ts:2015`, `/pair` at `:1980`, `/stop` at `:2069` | None in the gateway pipeline. `handleInboundMessage()` debounces (`slack-cowork-handler.ts:109-131`) and dispatches to the agent at `:253` with no pairing, tier, rate-limit, or budget check. The cowork handler is not in the ChannelManager path | Tier gate in the router: `hasPermission(tier, 'send_message')` (`message-router.ts:591`), command gate at `:568` | +| Persistence | SQLite: `channel_messages` (`001_init.ts:780`) written at `channel-manager.ts:700` (inbound) and `:1651` (outbound), `channel_sessions` (`001_init.ts:763`) via `session-manager.ts:19-38`, `channel_audit_log` (`001_init.ts:797`) via `audit-log.ts:3-4`. Workspace per user and thread at `resolveChannelWorkDir()` (`workspace.ts:83-109`). Slack-scoped per-user credentials and MCP overlay loaded at `channel-manager.ts:1159` and `:1171` | In-memory `Map` only (`slack-cowork-handler.ts:88-90`); 24 h TTL and hourly cleanup (`:43-44`, `startCleanup()` at `:734`). Thread history is re-fetched from Slack each turn | `gateway_messages` and `gateway_sessions` (`gateway/shared/db/schema.ts:55`, `:81`), written at `message-router.ts:548` and `:982`, via `getOrCreateSession()` (`gateway/core/session-manager.ts:18`) | +| Scheduling | Delivery target for automations: `deliverToChannel()` resolves the plugin by `configId` then platform (`automation/delivery.ts:266-270`). Channel context for schedule tools built at `agent-context.ts:24` and passed at `channel-manager.ts` run site | `channelContext` with `platform: 'slack'` passed to `runAgent()` at `slack-cowork-handler.ts:262-269`; it omits `permissionTier` and `identityId`, and a missing tier denies at `connector-policy.ts:329-331` via `tierMeets()` (`:98-105`) | `gateway_subscriptions` and `/subscribe` (`gateway/core/command-handlers.ts:91`) and task lifecycle notifications (`gateway/core/event-listener.ts:31`, `:44`) | +| Stop | `/stop` command (`channel-manager.ts:2069-2085`) and sentiment stop (`:739-747`) abort the active run and delete the agent session; queued message cleared; plugin shutdown `slack/index.ts:765` sets presence away and calls `app.stop()` | New message aborts the previous run (`slack-cowork-handler.ts:145-150`). `POST /slack/gateway/stop` (`slack.ts:318`) unwires the handler and calls `slackGateway.stop()` (`slack-gateway.ts:124`), which calls `app.stop()` at `:130` | `Gateway.stop()` disconnects adapters (`gateway/core/gateway.ts:139-148`); `stopChannel()` at `:338-346`. Not reachable while the gateway is not started | + +The two live Slack stacks both open a Socket Mode connection and both parse the +same `{botToken, appToken}` pair, but from different stores. They differ on +permissions (A has a pipeline, C has none), persistence (A writes SQLite, C +keeps a `Map`), and command handling (A has `/pair`, `/stop`, `/new`; C has +`/forget` only, at `slack-cowork-handler.ts:153`). + +### 2.2 Telegram + +| Field | Runtime A (live) | Runtime B (not started) | +|---|---|---| +| Inbound | grammy `Bot` plus `@grammyjs/runner` long polling. `telegram/index.ts:100` builds the bot; `:402-416` starts the runner with an `allowed_updates` list | grammy `Bot`. `gateway/channels/telegram/adapter.ts:82` builds the bot; `connect()` at `:79`, `disconnect()` at `:163` | +| Live consumer | `channel-manager.ts:563` via `registerMessageHandler` (`:365-367`) | `gateway/core/gateway.ts:173-177`; `createChannel('telegram', ...)` factory at `gateway/channels/telegram/adapter.ts:701-715` | +| Credentials | Vault. `channel-manager.ts:461`; plain bot token in `channel_config.token` replaced by the `__vault__` sentinel (`credential-vault.ts:36`) | `gateway.json` `channels.telegram.botToken` (schema `gateway/shared/config/types.ts:60-66`) | +| User identity | Telegram user ID; group chats get `chatId:threadId` and `chatId:threadId:userId` session keys (`telegram/index.ts:114-125`). Telegram IDs are globally unique, so `buildQualifiedUserId` adds no qualifier (`workspace.ts:59-61`) | `senderId` on the normalized message; gateway `gateway_identity_channels` keyed `(channel_id, channel_user_id)` | +| Permissions | Same pipeline as Slack (`security/pipeline.ts:39-125`), including an in-process token claim guard: `claimTelegramToken()` at `telegram/index.ts:394-400` prevents a duplicate long-poll for the same token | Router gate at `message-router.ts:591`; tier from `defaultPermissionTier`, default `viewer` (`gateway/shared/config/types.ts:135`, default `:332`) | +| Persistence | `channel_messages`/`channel_sessions`/`channel_audit_log` as above; session key from `telegram/index.ts:123-125` | `gateway_messages`/`gateway_sessions` | +| Scheduling | Same automation delivery path (`automation/delivery.ts:266-270`) | `gateway_subscriptions` | +| Stop | `/stop` (`channel-manager.ts:2069`); plugin stop at `telegram/index.ts:424-437` stops the runner and releases the token claim | `gateway.ts:338-346` | + +### 2.3 Discord + +| Field | Runtime A (live) | Runtime B (not started) | +|---|---|---| +| Inbound | discord.js WebSocket. `discord/index.ts:97-112` builds the `Client` with message, reaction, and DM intents; `Events.MessageCreate` at `:138`; `login()` at `:231` | discord.js. `gateway/channels/discord/adapter.ts:104` builds the `Client`; `login()` at `:146`; `disconnect()` at `:150` | +| Live consumer | `channel-manager.ts:563` | `gateway/core/gateway.ts:173-177`; factory at `gateway/channels/discord/adapter.ts:689-698` | +| Credentials | Vault, plain bot token | `gateway.json` `channels.discord.botToken` (schema `gateway/shared/config/types.ts:67-73`) | +| User identity | Discord user ID with guild qualifier: `buildQualifiedUserId('discord', userId, {guildId})` (`workspace.ts:52-58`) | `senderId`; `gateway_identity_channels` | +| Permissions | Pipeline as above; `mention_only` gate in guild channels at `discord/index.ts:142-150` | Router gate at `message-router.ts:591` | +| Persistence | Same SQLite tables; Discord display name read from message metadata at `channel-manager.ts:2160-2161` | `gateway_messages`/`gateway_sessions` | +| Scheduling | Same automation delivery path | `gateway_subscriptions` | +| Stop | `/stop`; plugin stop at `discord/index.ts:243-247` calls `client.destroy()` | `gateway.ts:338-346` | + +### 2.4 Lark / Feishu + +Runtime A treats Lark as one plugin. Runtime B splits the same platform into two +registered adapters, `feishu` and `lark`, and only `feishu` has an adapter file +(`gateway/channels/index.ts:12` imports `./feishu/adapter`; there is no +`gateway/channels/lark/` directory). Runtime A's plugin is registered as +`lark` (`channel-manager.ts:94`). + +| Field | Runtime A (live) | Runtime B (not started) | +|---|---|---| +| Inbound | Lark SDK `WSClient` long connection with an `EventDispatcher` for `im.message.receive_v1`, reactions, and `card.action.trigger`. `lark/index.ts:79-80` builds client and WS client; dispatcher at `:93-118`; `wsClient.start()` at `:121` | Same SDK, channel id `feishu`. `gateway/channels/feishu/adapter.ts:89-112` builds client, dispatcher, and WS client; `start()` at `:116`; `disconnect()` at `:130` | +| Live consumer | `channel-manager.ts:563` | `gateway/core/gateway.ts:173-177`; factory at `gateway/channels/feishu/adapter.ts:269` | +| Credentials | Vault, JSON `{appId, appSecret, verificationToken, encryptKey}` parsed by `parseLarkTokenConfig` at `lark/index.ts:64`. Startup probe at `:83` | `gateway.json` `channels.feishu.appId`/`appSecret` plus `encryptKey`/`verificationToken` (`gateway/channels/feishu/adapter.ts:29-36`, schema `gateway/shared/config/types.ts:94-99`) | +| User identity | Lark `open_id`/`user_id` with tenant qualifier: `buildQualifiedUserId('lark', userId, {tenantKey})` (`workspace.ts:46-54`) | `senderId` from `sender.sender_id.open_id` (`gateway/channels/feishu/adapter.ts:230-233`) | +| Permissions | Pipeline as above. Dedup by `event_id` via `shouldProcessEvent()` (`lark/index.ts:55`, used at `:153`) | Router gate at `message-router.ts:591` | +| Persistence | Same SQLite tables | `gateway_messages`/`gateway_sessions` | +| Scheduling | Same automation delivery path | `gateway_subscriptions` | +| Stop | `/stop`; plugin stop at `lark/index.ts:131-140` closes the WS client with `force: true` | `gateway.ts:338-346` | + +### 2.5 WhatsApp (runtime B only) + +| Field | Value | +|---|---| +| Inbound | Meta WhatsApp Business Cloud API webhook, not a socket. `POST /channels/whatsapp/webhook` (`app/api/channels.ts:1263`) verifies the HMAC and calls `adapter.handleWebhookEvent()` (`:1293`). Verification handshake on `GET /channels/whatsapp/webhook` (`:1247`). Normalization at `gateway/channels/whatsapp/cloud.ts:91` | +| Live consumer | The HTTP route itself. `getGateway().getAdapter('whatsapp')` at `app/api/channels.ts:1284`, guarded by a 503 when the adapter is absent (`:1287-1292`). With no `Gateway.start()`, that is the current response | +| Credentials | `gateway_channels.config` JSON for id `whatsapp`, read at `app/api/channels.ts:1264` and parsed by `parseWhatsAppCloudConfig()` (`cloud.ts:23-47`): `phoneNumberId`, `accessToken`, `webhookVerifyToken`, `appSecret`, optional `wabaId`/`graphVersion`. Not the vault. `validateWhatsapp()` (`app/api/channels.ts:289-295`) only returns a warning | +| User identity | WhatsApp phone number, normalized to E.164 digits by `normalizeWhatsAppTarget()` (`cloud.ts:49-57`); `senderId` set during webhook normalization (`cloud.ts:91-114`) | +| Permissions | Router gate at `message-router.ts:591` with `defaultPermissionTier` (`viewer`). Not reachable while the gateway is not started | +| Persistence | `gateway_messages`/`gateway_sessions` via `message-router.ts:548`/`:982` | +| Scheduling | `gateway_subscriptions` (`gateway/shared/db/schema.ts:105`) | +| Stop | `disconnect()` (`gateway/channels/whatsapp/adapter.ts:87-90`) only flips a flag; there is no socket to close. The webhook route stops answering when the adapter is removed from the map (`gateway.ts:338-346`) | +| Runtime class | `official` (`gateway/channels/whatsapp/adapter.ts:48`) | + +### 2.6 iMessage (runtime B only, BlueBubbles bridge) + +| Field | Value | +|---|---| +| Inbound | BlueBubbles posts webhooks. `POST /channels/imessage/webhook` (`app/api/channels.ts:1198`) rejects non-macOS hosts (`:1199-1201`), verifies an HMAC when a secret is configured (`:1231`, `verifyBlueBubblesWebhook` at `gateway/channels/imessage/webhook.ts:38-55`), then calls `adapter.handleWebhookEvent()` (`:1243`). Normalization at `webhook.ts:57-90` | +| Live consumer | The HTTP route. `getGateway().getAdapter('imessage')` at `app/api/channels.ts:1203`, 503 when absent (`:1212-1217`). Same not-started caveat as WhatsApp | +| Credentials | `gateway_channels.config` for id `imessage`: `serverUrl` and `password`, plus optional `webhookSecret` (`gateway/channels/imessage/adapter.ts:39-46`). The route falls back to the adapter's `webhookSecret()` then `webhookSecret`/`password` from config (`app/api/channels.ts:1222-1226`). `connect()` also requires a consent setting `imessage.consent.acceptedAt` (`gateway/channels/imessage/adapter.ts:37`, checked at `:81-88`) | +| User identity | iMessage handle address, or `uniqueId` fallback (`webhook.ts:101`) | +| Permissions | Router gate at `message-router.ts:591`. Consent gating happens in `connect()`, not per message | +| Persistence | `gateway_messages`/`gateway_sessions` | +| Scheduling | `gateway_subscriptions` | +| Stop | `disconnect()` (`gateway/channels/imessage/adapter.ts:110-114`) flips a flag. Webhooks keep arriving at the route until the adapter is removed | +| Runtime class | `bridge` (`gateway/channels/imessage/adapter.ts:61`) | + +### 2.7 Linear (runtime B only, no live inbound) + +| Field | Value | +|---|---| +| Inbound | Designed as a Linear comment webhook. `handleCommentWebhook()` exists at `gateway/channels/linear/adapter.ts:143-171` and normalizes a `Comment`/`create` payload. No caller: the live Linear webhook route `POST /linear/webhook` (`app/api/linear.ts:68`) uses `shared/services/linear.ts` directly and never touches this adapter | +| Live consumer | `app/api/channels.ts:1142-1159` (registry listing) and `:1165-1195` (enable/disable/reconnect). `connect()` at `linear/adapter.ts:72` only validates that a Linear API key exists | +| Credentials | Linear API key via `getLinearClientAsync()` (`linear/adapter.ts:76`); no gateway config fields | +| User identity | `payload.data.user.id`, fallback `'linear-user'` (`linear/adapter.ts:153`) | +| Permissions | Router gate at `message-router.ts:591` if a message were ever dispatched | +| Persistence | `gateway_messages`/`gateway_sessions`; none today because nothing dispatches | +| Scheduling | `gateway_subscriptions` | +| Stop | `disconnect()` (`linear/adapter.ts:90-94`) flips a flag | +| Runtime class | `official` (`linear/adapter.ts:63`) | + +### 2.8 SMS (runtime B only, placeholder) + +| Field | Value | +|---|---| +| Inbound | None implemented. `onMessage` is a no-op (`gateway/channels/sms/adapter.ts:92`) | +| Live consumer | Registry listing and enable/disable only (`app/api/channels.ts:1142-1195`) | +| Credentials | `gateway.json` `channels.sms` (`accountSid`, `authToken`, `fromNumber`, `webhookUrl`; schema `gateway/shared/config/types.ts:86-92`). `connect()` throws the setup banner when any is missing (`sms/adapter.ts:55-67`) | +| User identity | None; no inbound path | +| Permissions | None reachable | +| Persistence | None | +| Scheduling | `gateway_subscriptions` exists but nothing produces events | +| Stop | `disconnect()` (`sms/adapter.ts:69-71`) flips a flag; `isConnected()` always returns `false` (`:73-75`) | +| Runtime class | `experimental` (`sms/adapter.ts:46`) | + +## 3. Cross-cutting ownership + +### 3.1 Credentials + +Two stores, no overlap: + +- Runtime A uses the AES-256-GCM vault at `/channel-creds.enc.json` + (`credential-vault.ts:262`, `getChannelCredsPath()`), keyed by the + `channel_config.id` UUID, with the sentinel `__vault__` left in the SQLite + `token` column (`credential-vault.ts:36`). `channel_config.token` is defined at + `001_init.ts:725-728`. +- Runtime B uses `gateway.json` in the app dir (`gateway/shared/config/loader.ts:21`), + with `env:VAR` indirection resolved by `resolveEnvRefs()` + (`gateway/shared/config/loader.ts:28-51`) and Zod validation at `:78-84`. Per-channel config lives under `channels.`. +- Runtime C uses `/slack-config.enc.json` (`slack-config.ts:124-126`), + AES-256-GCM per field, key derived from hostname, username, and a nonce + (`slack-config.ts:177-192`). + +### 3.2 Identity and permissions + +- Runtime A: `channel_users` (migration `001_init.ts:742`), unique on + `(config_id, platform_user_id)` (`003_multi_bot.ts:58-59`). Tier defaults to + `operator` in the table (`001_init.ts:748`). `SecurityPipeline` auto-approves + in `access_mode: 'open'` (`security/pipeline.ts:51-66`), rejects viewers for + message sending (`:69-71`), rate-limits per `channel_users.id` (`:74`), + enforces a token budget (`:84`), and runs guardrails (`:98`). Pairing codes + live in `channel_pairing_codes` (`001_init.ts:755`) with the service at + `pairing-service.ts:12`, `:24`, `:52`. +- Runtime B: `gateway_identities` and `gateway_identity_channels` + (`gateway/shared/db/schema.ts:29-39`, `:41-52`), tier default `viewer` in the + table (`gateway/shared/db/schema.ts:33`), auto-created on first message by `resolveIdentity()` + (`identity-resolver.ts:19-46`). Gating happens in the router: command gate at + `message-router.ts:568`, `send_message` gate at `:591`, concurrency gate at + `:625`. Tier vocabulary `viewer`/`operator`/`admin` is duplicated between the + two runtimes (`shared/db/types.ts:922` and + `gateway/channels/types.ts:149`). +- Runtime C: no identity store. The cowork handler keeps an in-memory session + per thread and does not check pairing, tier, rate, or budget. + +### 3.3 Persistence + +- Runtime A writes `channel_config`, `channel_users`, `channel_pairing_codes`, + `channel_sessions`, `channel_messages`, `channel_audit_log` (all created in + `001_init.ts:725-806`), and leases in `channel_leases` + (`024_channel_leases.ts:10`). +- Runtime B is designed around `gateway_*` tables created only by + `initializeGatewaySchema()` (`gateway/shared/db/schema.ts:13`), which nothing + calls. Only `gateway_channels` and `routing_rules` exist in practice + (`008_gateway_routing_rules.ts:18`, `:29`). +- Runtime C persists nothing; sessions are in-memory with a 24 h TTL + (`slack-cowork-handler.ts:43`, `:88-90`). + +### 3.4 Scheduling and stop + +- Outbound scheduling for runtime A is `deliverToChannel()` + (`automation/delivery.ts:206`), reached from `deliver()` (`:110`, branch at + `:115`), resolving a plugin by `configId` then platform (`:266-270`) and + sending through `plugin.sendMessage` (`:290`). The automation engine holds the + manager at `automation/engine.ts:968`. Video share delivery uses the same + manager (`video/share.ts:163`). +- Runtime B scheduling is subscription based: `gateway_subscriptions` + (`gateway/shared/db/schema.ts:105`), `/subscribe` at + `gateway/core/command-handlers.ts:91`, task lifecycle notifications in + `gateway/core/event-listener.ts:31` and `:44`. +- Runtime C scheduling is indirect: it passes `channelContext` with + `platform: 'slack'` to `runAgent()` (`slack-cowork-handler.ts:262-269`), which + schedule MCP tools use to resolve a delivery target. That context omits + `permissionTier` and `identityId`, which are optional in the type + (`core/agent/types.ts:575-603`). +- Stop behavior is covered per provider above. The one shared piece is the + in-process lease: `ChannelManager` uses `NopLeaser` by default + (`channel-manager.ts:360`), which never denies a second instance, while + `DbLeaser` (`_shared/db-lease.ts:15`) would use `channel_leases`. The default + is `NopLeaser`, so two API processes can start the same bot. + +## 4. Unconfirmed and gaps + +These are things I could not confirm from the tree. They are stated as open, not +as behavior. + +- **Gateway runtime is not started by the shipped API.** I searched `src/`, + `src-api/src/`, `src-api/test/`, and `src-tauri/src/` for `startGateway`, + `stopGateway`, and `new Gateway(`. The only hits are the definitions in + `shared/services/gateway/index.ts` and the lazy singleton in + `core/gateway.ts:466`. I could not find any caller. If a packaged or desktop + entry point starts it outside these trees, that caller is not in this + repository. +- **`gateway_identities`, `gateway_sessions`, `gateway_messages`, + `gateway_subscriptions`, `gateway_identity_channels`, and `gateway_audit_log` + have no migration.** `initializeGatewaySchema()` would create them but has no + caller. Whether they exist in a shipped database depends on a code path I + could not find. +- **`POST /channels/imessage/webhook` and `POST /channels/whatsapp/webhook` + return 503 today.** That follows from the adapter map being empty without + `Gateway.start()`. I did not run the server to observe it, so this is a + reading of the code, not a runtime observation. +- **Linear inbound is unwired.** `handleCommentWebhook()` has no caller; the + live route at `app/api/linear.ts:68` bypasses it. Confirmed by search, not by + observing a Linear webhook. +- **Whether the legacy Slack gateway starts at boot.** `gateway.autoStart` + exists in the config type (`slack-config.ts:96-102`) and the API accepts it + (`app/api/slack.ts:57`), but I found no code that reads `autoStart` to start + the gateway at startup. Only the manual `POST /slack/gateway/start` route + (`slack.ts:305`) starts it. +- **`NopLeaser` is the production default.** The constructor takes an optional + `Leaser` (`channel-manager.ts:341-363`) and falls back to `NopLeaser` + (`:360`). I found no call site that injects `DbLeaser`, so the multi-instance + protection the `channel_leases` table implies is not active by default. +- **Slack Socket Mode duplication.** Both runtime A (`slack/index.ts:233`) and + runtime C (`slack-gateway.ts:99`) can open a Socket Mode connection for the + same workspace with different tokens and no coordination. What happens when + both run is a platform-side question I did not test. +- **`doc/backend/gateway.md:159`** describes Slack in the gateway as using + `slack-gateway.ts` through the unified pipeline. The current + `gateway/channels/slack/adapter.ts:17-25` returns `null` and routes nothing. + The document and the code disagree; I am reporting the mismatch rather than + changing either. + +## 5. Decision needed + +The matrix shows three runtimes for one product surface, two live Slack stacks, +and a gateway runtime whose lifecycle entry point has no caller. Choosing a +canonical runtime changes the data model (vault vs `gateway.json` vs +`slack-config.enc.json`), the permission model (pipeline vs router vs none), and +the persistence model (SQLite vs in-memory), so it is a maintainer decision, not +an implementation detail. This document does not pick one. + +Candidate canonical runtimes, with what each would imply. Listed for review, not +recommended: + +1. **Runtime A, the plugin runtime (`shared/channels/`).** Today it is the only + runtime with a startup call site, a credential vault, a permission pipeline, + and durable persistence, and it already covers Slack, Discord, Telegram, and + Lark. Making it canonical means porting WhatsApp and iMessage inbound into + plugins, retiring the gateway adapters, and moving Slack off + `slack-gateway.ts`. +2. **Runtime B, the gateway runtime (`shared/services/gateway/`).** It has the + richer abstractions (identity, subscriptions, routing rules, circuit + breaker) and the two webhook providers that runtime A lacks, but it is not + started today and its core tables are not migrated. Making it canonical means + giving it a real startup path, migrating the `gateway_*` schema, and porting + the four plugin platforms. +3. **Runtime C, the legacy Slack gateway.** It has no permissions, no + persistence, and no other provider. It is the smallest surface and the + likeliest thing to retire rather than promote. +4. **A new unified runtime.** Neither existing runtime's interfaces + (`BasePlugin` vs `ChannelAdapter`) match the other's message shape, so a + third option is to define one adapter contract and migrate both. This is the + largest change and the only one that avoids a partial overlap. + +Migration itself is a separate sub-issue. Whichever candidate is chosen, the +work that follows is: pick the contract, port providers one at a time, migrate +callers, then delete the losing runtime in the same wave, per the LESSONS.md rule +"When you replace an internal API, migrate every caller and delete the old one +in the same wave". The maintainer's choice should be recorded on issue #222 +before any migration PR is opened. + +## Appendix: live consumers of runtime A + +The manager is a singleton (`channel-manager.ts:2406`). Callers: + +- `http-daemon.ts:84` (import), `:589-590` (startup), `:403` (shutdown) +- `app/api/channels.ts:15` (import), `:392` (`addConfig`), `:472` (bulk start), + `:527` (`removeConfig`), `:547`, `:561` (`refreshConfig`), `:577`, `:757`, + `:962` (`getStatus`), `:981`, `:991`, `:1018` +- `shared/automation/delivery.ts:267` and `:348` +- `shared/automation/engine.ts:968` +- `shared/video/share.ts:163` + +## Appendix: live consumers of runtime B and runtime C + +Runtime B: + +- `app/api/channels.ts:45` (side-effect import that triggers adapter + self-registration), `:46` (`getGateway`), `:1142-1159` (registry listing via + `getRegisteredChannelIds()`), `:1165-1195` (enable/disable/reconnect), + `:1203` (iMessage adapter lookup), `:1284` (WhatsApp adapter lookup) +- `http-daemon.ts:22` and `:238` (`/gateway` audit-log routes) + +Runtime C: + +- `app/api/slack.ts:22-23` (imports), `:79` (event subscription), `:93` + (cowork handler dispatch), `:301-352` (status, start, stop, restart, test) +- `shared/services/slack-cowork-handler.ts:34` (imports `SlackInboundMessage`) +- `components/settings/SlackGatewaySettings.tsx:46` and `:78` (status and action + calls to `/slack/gateway/*`), rendered from + `components/settings/SlackConnectionSection.tsx:218` From e238ae604f054de607508c808da3509a2429a039 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 01:10:02 -0400 Subject: [PATCH 08/16] docs(process): add incident records, empty blocklist schema, and policy replay (#239) Checkpoint 7 of the regression maintenance plan asks for the records behind the contributor quality policy. This adds the incident record template, an empty blocklist with its entry schema, a schema check with fixtures, and the replay of the thresholds against the documented historical cases. The incident template follows blameless postmortem practice: it records the failed contract and the gate that missed it, not a judgement of a person. The blocklist is empty and names no contributor, vendor, model family, or agent session. An entry requires evidence, affected scope, effective date, expiry, remediation, and an appeal link. The check validates the committed blocklist, accepts the valid fixture, and rejects the fixture that is missing a required field. The replay places stacked PRs #102, #103 and #105, manual CI #176, and the different-authors case below the probation threshold of two. It counts the stacked chain once, does not blame the corrective author, and does not count the environment-specific resolve.test.ts failure. --- doc/maintenance/incidents/README.md | 43 ++++++ .../incidents/blocklist-schema.mjs | 136 ++++++++++++++++++ doc/maintenance/incidents/blocklist.json | 37 +++++ doc/maintenance/incidents/check-blocklist.mjs | 42 ++++++ .../incidents/check-blocklist.test.mjs | 57 ++++++++ .../incidents/fixtures/blocklist-invalid.json | 33 +++++ .../incidents/fixtures/blocklist-valid.json | 35 +++++ .../incidents/incident-record-template.md | 93 ++++++++++++ doc/maintenance/incidents/policy-replay.md | 96 +++++++++++++ 9 files changed, 572 insertions(+) create mode 100644 doc/maintenance/incidents/README.md create mode 100644 doc/maintenance/incidents/blocklist-schema.mjs create mode 100644 doc/maintenance/incidents/blocklist.json create mode 100644 doc/maintenance/incidents/check-blocklist.mjs create mode 100644 doc/maintenance/incidents/check-blocklist.test.mjs create mode 100644 doc/maintenance/incidents/fixtures/blocklist-invalid.json create mode 100644 doc/maintenance/incidents/fixtures/blocklist-valid.json create mode 100644 doc/maintenance/incidents/incident-record-template.md create mode 100644 doc/maintenance/incidents/policy-replay.md diff --git a/doc/maintenance/incidents/README.md b/doc/maintenance/incidents/README.md new file mode 100644 index 00000000..36c97f6e --- /dev/null +++ b/doc/maintenance/incidents/README.md @@ -0,0 +1,43 @@ +# Contributor policy records + +Date: 2026-10-14. This directory holds the records that the contributor quality +policy in +[03-lessons-and-contributors.md](../2026-10-13-regression/03-lessons-and-contributors.md) +asks for: the incident record template, the empty blocklist, and the replay of +the thresholds against the documented historical cases. + +The policy starts without names. No contributor, vendor, model family, or agent +session is identified here, and the blocklist is empty. + +| File | Purpose | +| --- | --- | +| [incident-record-template.md](incident-record-template.md) | Blameless incident record format | +| [blocklist.json](blocklist.json) | Empty blocklist with the entry schema | +| [blocklist-schema.mjs](blocklist-schema.mjs) | Schema constants and validation, shared by the check and its tests | +| [fixtures/blocklist-valid.json](fixtures/blocklist-valid.json) | A valid entry used only to exercise the schema | +| [fixtures/blocklist-invalid.json](fixtures/blocklist-invalid.json) | An entry missing a required field, used only to exercise rejection | +| [policy-replay.md](policy-replay.md) | Each documented historical case placed against the thresholds | + +## Rules this directory follows + +- An incident record names the contract that failed and the gate that missed it. + It does not judge a person. The practice is blameless postmortem + ([Google SRE](https://sre.google/sre-book/postmortem-culture/)). +- A record counts only after two reviewers or a maintainer with an independent + verification artifact confirm it. +- The blocklist stays empty until the policy's own evidence bar is met and a + maintainer records a decision. Each entry must carry evidence, affected scope, + effective date, expiry, remediation, and an appeal link. +- A block applies to a demonstrated contributor or tool configuration. It is not + a blacklist of a vendor or a model family. + +## Checks + +```bash +node doc/maintenance/incidents/check-blocklist.mjs doc/maintenance/incidents/blocklist.json +node --test doc/maintenance/incidents/check-blocklist.test.mjs +``` + +The first command validates the committed blocklist. The test runs the same +validator against both fixtures and asserts the valid entry is accepted and the +entry missing a required field is rejected. diff --git a/doc/maintenance/incidents/blocklist-schema.mjs b/doc/maintenance/incidents/blocklist-schema.mjs new file mode 100644 index 00000000..2f5a77a4 --- /dev/null +++ b/doc/maintenance/incidents/blocklist-schema.mjs @@ -0,0 +1,136 @@ +// Single source for the blocklist entry contract. The committed +// doc/maintenance/incidents/blocklist.json documents this schema and must +// declare the same required fields, which check-blocklist.mjs enforces. Keep +// the validator and the documentation derived from these constants. + +export const BLOCKLIST_VERSION = 1; + +// Required on every entry. The contributor policy in +// doc/maintenance/2026-10-13-regression/03-lessons-and-contributors.md names +// evidence, affected scope, effective date, expiry, remediation and an appeal +// link; id, contributorReference and incidentRecords identify the entry and the +// confirmed records behind it. +export const REQUIRED_FIELDS = [ + 'id', + 'contributorReference', + 'incidentRecords', + 'evidence', + 'affectedScope', + 'effectiveDate', + 'expiry', + 'remediation', + 'appealLink', +]; + +const ISO_DATE = /^\d{4}-\d{2}-\d{2}$/; + +// kind is one of string, date, stringList. date validates the ISO form. +export const FIELD_SPECS = { + id: { + kind: 'string', + description: + 'Stable record id for this entry, used by the incident records and the weekly review.', + }, + contributorReference: { + kind: 'string', + description: + 'Opaque reference to the demonstrated contributor or tool configuration. Never a vendor, model family, or agent session name.', + }, + incidentRecords: { + kind: 'stringList', + description: + 'Ids of the confirmed incident records behind this entry. Each must meet the evidence bar.', + }, + evidence: { + kind: 'stringList', + description: + 'Links to the confirming artifacts, such as incident records and verification runs.', + }, + affectedScope: { + kind: 'string', + description: + 'What is restricted and where, such as autonomous submission or merge for a named repository surface.', + }, + effectiveDate: { + kind: 'date', + description: 'ISO date the restriction starts.', + }, + expiry: { + kind: 'date', + description: + 'ISO date the entry is reviewed. A block is renewed only with stated evidence.', + }, + remediation: { + kind: 'string', + description: + 'What must be done to clear the entry, such as repairing outstanding incidents and passing reviewed scoped contributions.', + }, + appealLink: { + kind: 'string', + description: + 'Where an appeal is filed. An appeal can correct attribution, severity, or causation at any time.', + }, +}; + +function checkValue(kind, value) { + if (kind === 'string') { + return typeof value === 'string' && value.trim().length > 0; + } + if (kind === 'date') { + return typeof value === 'string' && ISO_DATE.test(value); + } + if (kind === 'stringList') { + return ( + Array.isArray(value) && + value.length > 0 && + value.every((item) => typeof item === 'string' && item.trim().length > 0) + ); + } + return false; +} + +// Returns { ok, errors }. errors is a list of human-readable strings. +export function validateBlocklist(doc) { + const errors = []; + if (doc === null || typeof doc !== 'object' || Array.isArray(doc)) { + return { ok: false, errors: ['blocklist must be a JSON object'] }; + } + + const declaredRequired = doc.schema?.entry?.required; + if (!Array.isArray(declaredRequired)) { + errors.push('schema.entry.required must be an array of field names'); + } else { + const declared = [...declaredRequired].sort().join(','); + const expected = [...REQUIRED_FIELDS].sort().join(','); + if (declared !== expected) { + errors.push( + `schema.entry.required does not match the validator contract: expected ${expected}`, + ); + } + } + + if (!Array.isArray(doc.entries)) { + errors.push('entries must be an array'); + return { ok: errors.length === 0, errors }; + } + + doc.entries.forEach((entry, index) => { + const where = `entries[${index}]`; + if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) { + errors.push(`${where} must be an object`); + return; + } + for (const field of REQUIRED_FIELDS) { + if (!(field in entry)) { + errors.push(`${where} is missing required field "${field}"`); + continue; + } + const spec = FIELD_SPECS[field]; + if (!checkValue(spec.kind, entry[field])) { + errors.push(`${where}.${field} must be a non-empty ${spec.kind}`); + } + } + }); + + return { ok: errors.length === 0, errors }; +} diff --git a/doc/maintenance/incidents/blocklist.json b/doc/maintenance/incidents/blocklist.json new file mode 100644 index 00000000..190d6b5c --- /dev/null +++ b/doc/maintenance/incidents/blocklist.json @@ -0,0 +1,37 @@ +{ + "version": 1, + "description": "Contributor policy blocklist. Empty by design. No contributor, vendor, model family, or agent session is named here. Entries are added only when the policy's evidence bar is met and a maintainer records a decision. See README.md and the contributor policy in ../2026-10-13-regression/03-lessons-and-contributors.md.", + "policy": { + "probation": "Two confirmed escaped material defects attributable to the same contributor or tool configuration within 90 days.", + "block": "Three confirmed material incidents within 90 days, including at least one repeated failure class after its documented correction, or a verified deliberate bypass of required validation.", + "reinstatement": "Repair outstanding incidents, demonstrate the failed contracts under the new gate, and pass three independently reviewed scoped contributions. A block is reviewed after 30 days and renewed only with stated evidence.", + "appeal": "An appeal can correct attribution, severity, or causation at any time. Link it from the entry's appealLink." + }, + "schema": { + "entry": { + "required": [ + "id", + "contributorReference", + "incidentRecords", + "evidence", + "affectedScope", + "effectiveDate", + "expiry", + "remediation", + "appealLink" + ], + "fields": { + "id": "Stable record id for this entry.", + "contributorReference": "Opaque reference to the demonstrated contributor or tool configuration. Never a vendor, model family, or agent session name.", + "incidentRecords": "Ids of the confirmed incident records behind this entry.", + "evidence": "Links to the confirming artifacts, such as incident records and verification runs.", + "affectedScope": "What is restricted and where.", + "effectiveDate": "ISO date the restriction starts.", + "expiry": "ISO date the entry is reviewed.", + "remediation": "What must be done to clear the entry.", + "appealLink": "Where an appeal is filed." + } + } + }, + "entries": [] +} diff --git a/doc/maintenance/incidents/check-blocklist.mjs b/doc/maintenance/incidents/check-blocklist.mjs new file mode 100644 index 00000000..87e66634 --- /dev/null +++ b/doc/maintenance/incidents/check-blocklist.mjs @@ -0,0 +1,42 @@ +// Validates a contributor policy blocklist against the entry contract in +// blocklist-schema.mjs. +// +// node doc/maintenance/incidents/check-blocklist.mjs +// +// Exits 0 when the document and every entry match the schema, 1 otherwise. An +// empty blocklist is valid: the policy starts with no entries. + +import { validateBlocklist } from './blocklist-schema.mjs'; + +import { readFileSync } from 'node:fs'; +import process from 'node:process'; + +const file = process.argv[2]; +if (!file) { + console.error( + 'Usage: node doc/maintenance/incidents/check-blocklist.mjs ', + ); + process.exit(1); +} + +let doc; +try { + doc = JSON.parse(readFileSync(file, 'utf-8')); +} catch (error) { + console.error(`Cannot read ${file}: ${error.message}`); + process.exit(1); +} + +const { ok, errors } = validateBlocklist(doc); +if (!ok) { + console.error(`${file} is not a valid blocklist:`); + for (const error of errors) { + console.error(`- ${error}`); + } + process.exit(1); +} + +const count = doc.entries.length; +console.log( + `ok: ${file} is a valid blocklist with ${count} entr${count === 1 ? 'y' : 'ies'}`, +); diff --git a/doc/maintenance/incidents/check-blocklist.test.mjs b/doc/maintenance/incidents/check-blocklist.test.mjs new file mode 100644 index 00000000..a9fb4864 --- /dev/null +++ b/doc/maintenance/incidents/check-blocklist.test.mjs @@ -0,0 +1,57 @@ +import { REQUIRED_FIELDS, validateBlocklist } from './blocklist-schema.mjs'; + +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { test } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +const read = (name) => + JSON.parse( + readFileSync(fileURLToPath(new URL(name, import.meta.url)), 'utf-8'), + ); + +test('committed blocklist is valid and empty', () => { + const { ok, errors } = validateBlocklist(read('./blocklist.json')); + assert.equal(ok, true, errors.join('; ')); +}); + +test('valid fixture entry passes', () => { + const { ok, errors } = validateBlocklist( + read('./fixtures/blocklist-valid.json'), + ); + assert.equal(ok, true, errors.join('; ')); +}); + +test('fixture missing a required field is rejected', () => { + const { ok, errors } = validateBlocklist( + read('./fixtures/blocklist-invalid.json'), + ); + assert.equal(ok, false); + assert.match(errors.join('\n'), /missing required field "remediation"/); +}); + +test('an entry missing any required field is rejected', () => { + for (const field of REQUIRED_FIELDS) { + const entry = read('./fixtures/blocklist-valid.json').entries[0]; + delete entry[field]; + const { ok, errors } = validateBlocklist({ + schema: { entry: { required: REQUIRED_FIELDS } }, + entries: [entry], + }); + assert.equal(ok, false, `expected ${field} to be required`); + assert.match( + errors.join('\n'), + new RegExp(`missing required field "${field}"`), + ); + } +}); + +test('a documented schema that drops a required field is rejected', () => { + const doc = read('./blocklist.json'); + doc.schema.entry.required = doc.schema.entry.required.filter( + (field) => field !== 'appealLink', + ); + const { ok, errors } = validateBlocklist(doc); + assert.equal(ok, false); + assert.match(errors.join('\n'), /does not match the validator contract/); +}); diff --git a/doc/maintenance/incidents/fixtures/blocklist-invalid.json b/doc/maintenance/incidents/fixtures/blocklist-invalid.json new file mode 100644 index 00000000..f342397f --- /dev/null +++ b/doc/maintenance/incidents/fixtures/blocklist-invalid.json @@ -0,0 +1,33 @@ +{ + "version": 1, + "description": "Schema fixture. This entry is intentionally missing the required remediation field, so the check must reject it. It is not a real record.", + "schema": { + "entry": { + "required": [ + "id", + "contributorReference", + "incidentRecords", + "evidence", + "affectedScope", + "effectiveDate", + "expiry", + "remediation", + "appealLink" + ] + } + }, + "entries": [ + { + "id": "example-0002", + "contributorReference": "opaque-ref-example", + "incidentRecords": ["incident-example-0003"], + "evidence": [ + "doc/maintenance/incidents/records/incident-example-0003.md" + ], + "affectedScope": "Autonomous merge for the named repository surface.", + "effectiveDate": "2026-10-14", + "expiry": "2026-11-13", + "appealLink": "https://github.com/bravew/Neumar/issues/231" + } + ] +} diff --git a/doc/maintenance/incidents/fixtures/blocklist-valid.json b/doc/maintenance/incidents/fixtures/blocklist-valid.json new file mode 100644 index 00000000..28eac794 --- /dev/null +++ b/doc/maintenance/incidents/fixtures/blocklist-valid.json @@ -0,0 +1,35 @@ +{ + "version": 1, + "description": "Schema fixture. The entry is a placeholder that exercises the blocklist schema; it is not a real record and names no contributor.", + "schema": { + "entry": { + "required": [ + "id", + "contributorReference", + "incidentRecords", + "evidence", + "affectedScope", + "effectiveDate", + "expiry", + "remediation", + "appealLink" + ] + } + }, + "entries": [ + { + "id": "example-0001", + "contributorReference": "opaque-ref-example", + "incidentRecords": ["incident-example-0001", "incident-example-0002"], + "evidence": [ + "doc/maintenance/incidents/records/incident-example-0001.md", + "doc/maintenance/incidents/records/incident-example-0002.md" + ], + "affectedScope": "Autonomous submission for the named repository surface.", + "effectiveDate": "2026-10-14", + "expiry": "2026-11-13", + "remediation": "Repair the outstanding incidents, demonstrate the failed contracts under the current gate, and pass three independently reviewed scoped contributions.", + "appealLink": "https://github.com/bravew/Neumar/issues/231" + } + ] +} diff --git a/doc/maintenance/incidents/incident-record-template.md b/doc/maintenance/incidents/incident-record-template.md new file mode 100644 index 00000000..4068cddb --- /dev/null +++ b/doc/maintenance/incidents/incident-record-template.md @@ -0,0 +1,93 @@ +# Incident record template + +One record per confirmed incident. Copy this file, fill every field, and link it +from the weekly review. A record is a description of a system failure, not a +judgement of a person. Write what contract failed and which gate let it through. + +An incident counts toward the policy thresholds only when two reviewers, or a +maintainer holding an independent verification artifact, confirm it. Until then +it is a candidate, and the `Maintainer disposition` field says so. + +Attribution fields use `unknown` when evidence is unavailable. Do not guess. +Do not name a vendor, model family, or agent session from a product reference in +code or a title; that is not authorship evidence. Keep private session URLs and +identifiers out of this file and put an opaque reference here instead. + +- **Record ID:** +- **Status:** candidate | confirmed | repaired | closed | appealed +- **Date recorded:** +- **Recorded by:** + +## Introducing change + +- **Introducing PR or commit:** +- **Causal evidence:** the specific diff, line, or squash that introduced the + behavior. A stacked PR total is not causal evidence; use the squash diff. +- **Attribution evidence:** the human accountable for landing it, and the + authoring tool or session when known. Use an opaque local reference. Write + `unknown` when the evidence does not exist, and say what would settle it. + +## Reproducible scenario + +- **Failed contract:** the behavior, schema, or invariant that was expected. +- **Steps:** the shortest sequence that reproduces the failure on the real + surface a caller uses, such as the HTTP route, MCP handler, bridge, packaged + build, or chosen runtime. +- **Observed result:** +- **Expected result:** +- **Artifact:** a command, test name, or log that shows the result. + +## Severity + +- **Level:** P0 urgent containment | P1 fix before expanding the feature | + P2 planned cleanup. Use the same scale as + [01-findings.md](../2026-10-13-regression/01-findings.md). +- **Impact:** what a user or system could not do, or what was put at risk. +- **Breadth:** how many callers, routes, platforms, or installs are affected. +- **Reversibility:** whether existing data or published output was changed. + +## Review and check gap + +- **Gate that should have caught it:** +- **Why it did not:** the check skipped, the boundary the test stopped short of, + the fixture that did not match the environment, or the job that only ran on + release heads. +- **What was observed instead:** the green check names, a PR-body claim, or a + manual run that did not cover this path. + +## Repair + +- **Repair PR:** +- **Fixing change:** the cause that was fixed, not the symptom that was hidden. +- **Prevention test or check:** the new or changed test at the boundary above, + and where it runs. + +## Verification + +- **Verified commit:** the exact head the checks ran on. +- **Commands and results:** each command, its outcome, and any failure, including + Node and pnpm versions for a local run. +- **Independent artifact:** the reviewer or maintainer artifact that confirms it. +- **Unverified:** what remains untested, and why. + +## Disposition + +- **Maintainer disposition:** accepted | disputed | no action | probation | + block | reinstatement. Record who decided and when. +- **Policy effect:** does this record count toward the 90-day thresholds, and + why. Note if it is one incident across an epic and its sub-PRs, a repair by a + different author, or an infrastructure failure that does not count. +- **Effective date:** for a block or probation, the date the restriction starts. +- **Expiry:** the date it is reviewed, or `not applicable`. + +## Appeal status + +- **Appeal status:** none | open | upheld | corrected | withdrawn +- **Appeal link:** the issue or record where an appeal is filed. +- **Appeal outcome:** for a closed appeal, what was corrected, such as + attribution, severity, or causation. + +## Notes + +Anything else that helps the next reviewer, including related records and links +to the weekly review that raised it. diff --git a/doc/maintenance/incidents/policy-replay.md b/doc/maintenance/incidents/policy-replay.md new file mode 100644 index 00000000..070c5c28 --- /dev/null +++ b/doc/maintenance/incidents/policy-replay.md @@ -0,0 +1,96 @@ +# Policy replay against the documented historical cases + +Date: 2026-10-14. This note applies the contributor quality policy in +[03-lessons-and-contributors.md](../2026-10-13-regression/03-lessons-and-contributors.md) +to the cases that review recorded, and shows each one landing on the intended +side of the thresholds. It is a paper replay of the policy, not a new +investigation. It adds no names and confirms no new incident. + +## The thresholds under test + +- **Probation:** two confirmed escaped material defects attributable to the same + contributor or tool configuration within 90 days. +- **Block from autonomous submission or merge:** three confirmed material + incidents within 90 days, including at least one repeated failure class after + its documented correction, or a verified deliberate bypass of required + validation. +- **Reinstatement:** repair outstanding incidents, demonstrate the failed + contracts under the new gate, and pass three independently reviewed scoped + contributions. Review a block after 30 days; renew only with stated evidence. + An appeal can correct attribution, severity, or causation at any time. + +A record counts only after two reviewers, or a maintainer with an independent +verification artifact, confirm it. The blocklist stays empty until that evidence +bar and a maintainer decision both exist. + +## Counting rules the replay applies + +1. **One incident, once.** A defect introduced by an epic and carried by its + sub-PRs is a single incident, counted at the introducing change. Do not count + it again at each sub-PR. +2. **A repair is not a strike.** A corrective PR that fixes a preexisting defect + does not receive its own incident. The defect counts where it was introduced. +3. **A transient infrastructure failure is not a code-quality incident.** + Environment-specific or flaky failures do not count against a contributor. +4. **Attribution must be proven.** A reference to a tool or vendor in code, a + title, or a session trailer is not authorship evidence. Use `unknown` and do + not count when the evidence does not exist. +5. **A current check rollup does not establish what was known at merge.** Only a + check that completed before merge, on the matching head, is merge-time + evidence. + +## Case replay + +| Case | Candidate incident | Decision | Policy landing | +| --- | --- | --- | --- | +| Stacked PRs #102, #103, #105 | The duplicate `toolResultText` import in #103 squash `10edc9f3`, after #102 introduced the symbol, plus the API test type and format errors #105 repaired. | One incident. It is one failure chain across a stacked branch, counted once at the integration defect, not three times. #102 is mostly a legitimate lockfile pin; #103 is smaller than its stacked totals suggest; #105 is the corrective author and gets no strike. | 1 confirmed incident, below probation (2) and block (3). | +| Manual CI #176 | The mock and toast drift that #175 introduced and #176 repaired, which automatic PR CI did not catch because the quality and Test jobs run only on release heads. | One incident, counted at #175, the introducing change. #176 is the repair and is not counted. The missing automatic PR gate is the check gap, recorded as a process fix, not a second strike. | 1 confirmed incident for the introducing configuration, below probation and block. The gate gap is a process item, not a contributor incident. | +| Different authors | Two or more contributors are said to have authored the held PRs, so the same defect pattern might accumulate per contributor. | No count. All sampled merged PRs share one GitHub identity and tool or session attribution is incomplete, so the policy cannot fairly attribute repeated defects to a named human, vendor, model family, or agent session. Even with attribution, each documented case above is a single incident. | 0 counted, below probation and block. | +| Infrastructure failure: `resolve.test.ts:251` | The `pnpm test:fast` failure on a host with Homebrew OpenCode installed, tracked in #195. | No count. It is an environment-specific test-isolation defect, not a contributor's code-quality incident. | 0 counted. Handled as test-isolation work, not a strike. | +| Timing case: #104 audit | An audit check that completed after #104 merged. | No count. A current rollup alone cannot establish what was known at merge, and this failure completed after merge, so it is not an already-red merge. | 0 counted. | + +Every documented case lands below the probation threshold of two, so the replay +produces no probation and no block. The intended side of each threshold is the +side with fewer than two confirmed, attributable incidents. + +## Per-configuration check + +The policy counts per contributor or tool configuration within 90 days, not per +repository. + +| Configuration | Confirmed incidents in the replay | Threshold reached | +| --- | ---: | --- | +| Introducing configuration for the stacked-PR chain | 1 | None; below probation. | +| Introducing configuration for the #175 to #176 chain | 1 | None; below probation. | +| Every other sampled configuration | 0 | None. | + +No configuration reaches two incidents, so no probation entry is created. With no +probation entries, no configuration can reach the third incident that would +trigger a block, including the repeated-failure-class condition. The blocklist +stays empty. + +## Reinstatement path, if an entry existed + +For a hypothetical block created from this policy, clearing it follows the same +reinstatement path the policy states, and the entry's own fields carry the +details: + +1. Repair the outstanding incidents named in the entry's `incidentRecords`. +2. Demonstrate the failed contracts under the current gate, using the boundary + the incident record names. +3. Pass three independently reviewed scoped contributions. +4. At the entry's `expiry`, review the block and renew it only with stated + evidence, or let it lapse. +5. At any time, an appeal filed at `appealLink` can correct attribution, + severity, or causation, which can remove the entry. + +## What this replay does not establish + +- It does not confirm any new incident. It reuses the review's recorded findings + and its attribution limits. +- It does not identify an author. Attribution stays `unknown` where the review + could not prove it. +- It does not evaluate the thresholds against code that changed after the + reviewed baseline `c0926618`. Re-run it when a new confirmed record exists. +- It does not create, extend, or lift a restriction. The blocklist is empty and + no repository setting has changed. From d084a8b52bf8e4a261b9c209ae14fb7264ed991d Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 01:10:06 -0400 Subject: [PATCH 09/16] fix(api): resolve app-owned data through getAppDir() (#242) git-workspace derived its repos and sessions directories from homedir() at import time, and the local archive adapter defaulted to the previous brand directory .neuma. Both ignored NEUMAR_APP_DATA_DIR, so tests and packaging could write into the developer's real home directory. Resolve the repos and sessions roots through getAppDir() per call, and default the archive root to {app data dir}/archive. A pre-rebrand .neuma/archive tree is renamed to the new root on the first write, so archives published before the rebrand stay reachable; an injected rootDir is left untouched. Refs #226 --- src-api/src/shared/services/git-workspace.ts | 34 ++-- .../publish/destinations/local-archive.ts | 55 ++++++- .../test/unit/services/git-workspace.test.ts | 146 ++++++++++++++++++ .../test/unit/services/local-archive.test.ts | 136 ++++++++++++++++ 4 files changed, 355 insertions(+), 16 deletions(-) create mode 100644 src-api/test/unit/services/git-workspace.test.ts create mode 100644 src-api/test/unit/services/local-archive.test.ts diff --git a/src-api/src/shared/services/git-workspace.ts b/src-api/src/shared/services/git-workspace.ts index 493ce9e1..336b4ef9 100644 --- a/src-api/src/shared/services/git-workspace.ts +++ b/src-api/src/shared/services/git-workspace.ts @@ -8,11 +8,10 @@ import { exec } from 'child_process'; import { existsSync, mkdirSync, readdirSync, rmSync, statSync } from 'fs'; import { copyFile, readdir, writeFile } from 'fs/promises'; -import { homedir } from 'os'; import { basename, dirname, join } from 'path'; import { promisify } from 'util'; -import { APP_DIR_NAME } from '@/config/constants'; +import { APP_DIR_NAME, getAppDir } from '@/config/constants'; import { createLogger } from '@/shared/utils/logger'; @@ -23,11 +22,19 @@ const logger = createLogger('GitWorkspace'); // Constants // ============================================================================ -/** Base directory for cached repos */ -const REPOS_DIR = join(homedir(), APP_DIR_NAME, 'repos'); +/** + * Base directory for cached repos. Resolved per call rather than at import + * time so a `NEUMAR_APP_DATA_DIR` override is honoured and tests or packaging + * never touch the developer's real home. + */ +function getReposDir(): string { + return join(getAppDir(), 'repos'); +} -/** Base directory for task session worktrees */ -const SESSIONS_DIR = join(homedir(), APP_DIR_NAME, 'sessions'); +/** Base directory for task session worktrees. */ +function getSessionsDir(): string { + return join(getAppDir(), 'sessions'); +} // ============================================================================ // Validation Helpers @@ -57,7 +64,7 @@ function isValidRepoIdentifier(identifier: string): boolean { /** * Ensure base repo exists in cache and is up-to-date. - * Location: ~/{APP_DIR_NAME}/repos/{owner}/{repo} + * Location: {app data dir}/repos/{owner}/{repo} * * - If repo doesn't exist: full clone with optional auth * - If exists: git fetch --all --prune @@ -72,7 +79,7 @@ export async function ensureBaseRepo( throw new Error(`Invalid repo identifier: ${owner}/${repo}`); } - const baseRepoDir = join(REPOS_DIR, owner, repo); + const baseRepoDir = join(getReposDir(), owner, repo); const gitDir = join(baseRepoDir, '.git'); if (existsSync(gitDir)) { @@ -125,7 +132,7 @@ export async function ensureBaseRepo( /** * Create git worktree for a task with dedicated branch. - * Location: ~/{APP_DIR_NAME}/sessions/{issueId}/{repoName} + * Location: {app data dir}/sessions/{issueId}/{repoName} * * Uses `git worktree add` to create an isolated working directory. * Does NOT checkout/pull on the base repo (avoids race conditions @@ -146,7 +153,7 @@ export async function createTaskWorktree( } const repoName = basename(baseRepoPath); - const sessionPath = join(SESSIONS_DIR, issueIdentifier.toLowerCase()); + const sessionPath = join(getSessionsDir(), issueIdentifier.toLowerCase()); const worktreePath = join(sessionPath, repoName); logger.info(`Creating worktree: ${issueIdentifier} (branch: ${branchName})`); @@ -355,15 +362,16 @@ export async function listActiveWorktrees( * Errors are logged but never block pipeline execution. */ export async function refreshBaseRepos(): Promise { - if (!existsSync(REPOS_DIR)) { + const reposDir = getReposDir(); + if (!existsSync(reposDir)) { logger.info('No base repos to refresh'); return; } - const owners = readdirSync(REPOS_DIR); + const owners = readdirSync(reposDir); for (const owner of owners) { - const ownerPath = join(REPOS_DIR, owner); + const ownerPath = join(reposDir, owner); if (!statSync(ownerPath).isDirectory()) continue; const repos = readdirSync(ownerPath); diff --git a/src-api/src/shared/services/publish/destinations/local-archive.ts b/src-api/src/shared/services/publish/destinations/local-archive.ts index 1dd365e6..282eb989 100644 --- a/src-api/src/shared/services/publish/destinations/local-archive.ts +++ b/src-api/src/shared/services/publish/destinations/local-archive.ts @@ -1,7 +1,18 @@ -import { copyFile, link, mkdir, readFile, writeFile } from 'fs/promises'; -import { homedir } from 'os'; +import { existsSync } from 'fs'; +import { + copyFile, + link, + mkdir, + readFile, + rename, + writeFile, +} from 'fs/promises'; import path from 'path'; +import { getAppDir } from '@/config/constants'; + +import { createLogger } from '@/shared/utils/logger'; + import type { DestinationCapabilities, LegContext, @@ -19,17 +30,26 @@ export interface LocalArchiveAdapterOptions { indexFile?: (filePath: string) => Promise; } +/** Pre-rebrand archive location, read once and migrated on first use. */ +const LEGACY_ARCHIVE_DIR_NAME = '.neuma'; + +const logger = createLogger('PublishLocalArchive'); + export class LocalArchiveAdapter implements PublishDestinationAdapter { readonly kind = 'local-archive' as const; private readonly rootDir: string; private readonly now: () => Date; private readonly indexFile?: (filePath: string) => Promise; + private readonly migrateLegacyRoot: boolean; constructor(options: LocalArchiveAdapterOptions = {}) { - this.rootDir = options.rootDir ?? path.join(homedir(), '.neuma', 'archive'); + this.rootDir = options.rootDir ?? path.join(getAppDir(), 'archive'); this.now = options.now ?? (() => new Date()); this.indexFile = options.indexFile; + // Only the default root has a legacy predecessor. An injected root is a + // test or caller's own directory and must not be touched. + this.migrateLegacyRoot = options.rootDir === undefined; } capabilities(): DestinationCapabilities { @@ -69,6 +89,7 @@ export class LocalArchiveAdapter implements PublishDestinationAdapter { async upload(input: PublishLegInput, ctx: LegContext): Promise { const plan = await this.plan(input); const targetDir = plan.targetPath ?? this.rootDir; + if (this.migrateLegacyRoot) await this.adoptLegacyArchive(); await mkdir(targetDir, { recursive: true }); const artifactName = path.basename(input.source.path); @@ -124,6 +145,34 @@ export class LocalArchiveAdapter implements PublishDestinationAdapter { async abort(): Promise { return; } + + /** + * Move a pre-rebrand `~/.neuma/archive` tree to the current archive root so + * archives published before the rebrand stay reachable. Idempotent: it does + * nothing once the new root exists. A failed move is logged and leaves the + * legacy tree untouched rather than deleting it. + */ + private async adoptLegacyArchive(): Promise { + const legacyRoot = path.join( + getAppDir(), + LEGACY_ARCHIVE_DIR_NAME, + 'archive', + ); + if (legacyRoot === this.rootDir) return; + if (existsSync(this.rootDir) || !existsSync(legacyRoot)) return; + + try { + await mkdir(path.dirname(this.rootDir), { recursive: true }); + await rename(legacyRoot, this.rootDir); + logger.info(`Migrated legacy archive: ${legacyRoot} -> ${this.rootDir}`); + } catch (error) { + logger.warn( + `Failed to migrate legacy archive ${legacyRoot}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + } } async function writeManifestSidecar( diff --git a/src-api/test/unit/services/git-workspace.test.ts b/src-api/test/unit/services/git-workspace.test.ts new file mode 100644 index 00000000..b4932583 --- /dev/null +++ b/src-api/test/unit/services/git-workspace.test.ts @@ -0,0 +1,146 @@ +/** + * Regression coverage for #226. App-owned repositories and task worktrees must + * resolve through getAppDir(), so NEUMAR_APP_DATA_DIR moves them and tests or + * packaging never write into the developer's real home directory. + * + * The clone URL is redirected to a local bare repository with git's + * `url.*.insteadOf` so the test exercises the real `git clone` and + * `git worktree add` paths without reaching github.com. + */ +import { execFileSync } from 'node:child_process'; +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import os from 'node:os'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + createTaskWorktree, + ensureBaseRepo, +} from '@/shared/services/git-workspace'; + +function git(args: string[], cwd?: string): void { + execFileSync('git', args, { cwd, stdio: 'pipe' }); +} + +/** Create a bare repository on `main` with one commit, ready to clone. */ +function seedBareRepo(bareRepo: string, scratch: string): void { + mkdirSync(join(bareRepo, '..'), { recursive: true }); + git(['init', '--quiet', '--bare', '--initial-branch=main', bareRepo]); + + const seed = join(scratch, 'seed'); + mkdirSync(seed, { recursive: true }); + git(['init', '--quiet', '--initial-branch=main', seed]); + writeFileSync(join(seed, 'README.md'), 'hello\n'); + git(['add', 'README.md'], seed); + git( + [ + '-c', + 'user.email=test@example.com', + '-c', + 'user.name=Test', + 'commit', + '--quiet', + '-m', + 'init', + ], + seed, + ); + git(['remote', 'add', 'origin', bareRepo], seed); + git(['push', '--quiet', 'origin', 'main'], seed); +} + +describe('git-workspace honours NEUMAR_APP_DATA_DIR (#226)', () => { + let dataDir: string; + let scratch: string; + let mirrorRoot: string; + /** A stand-in for the developer's real home; nothing may land here. */ + let fakeHome: string; + let homedirSpy: ReturnType; + const originalHome = process.env.HOME; + const originalOverride = process.env.NEUMAR_APP_DATA_DIR; + const originalGitConfig = { + count: process.env.GIT_CONFIG_COUNT, + key: process.env.GIT_CONFIG_KEY_0, + value: process.env.GIT_CONFIG_VALUE_0, + }; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), 'neumar-226-data-')); + scratch = mkdtempSync(join(tmpdir(), 'neumar-226-scratch-')); + fakeHome = mkdtempSync(join(tmpdir(), 'neumar-226-home-')); + mirrorRoot = join(scratch, 'mirror'); + process.env.HOME = fakeHome; + process.env.NEUMAR_APP_DATA_DIR = dataDir; + // The spy both fixes the value and records that the home directory was + // consulted, so a regression back to homedir() is visible. + homedirSpy = vi.spyOn(os, 'homedir').mockReturnValue(fakeHome); + // Redirect https://github.com/... to the local bare repo. + process.env.GIT_CONFIG_COUNT = '1'; + process.env.GIT_CONFIG_KEY_0 = `url.file://${mirrorRoot}/.insteadOf`; + process.env.GIT_CONFIG_VALUE_0 = 'https://github.com/'; + }); + + afterEach(() => { + homedirSpy.mockRestore(); + + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalOverride === undefined) delete process.env.NEUMAR_APP_DATA_DIR; + else process.env.NEUMAR_APP_DATA_DIR = originalOverride; + + if (originalGitConfig.count === undefined) + delete process.env.GIT_CONFIG_COUNT; + else process.env.GIT_CONFIG_COUNT = originalGitConfig.count; + if (originalGitConfig.key === undefined) + delete process.env.GIT_CONFIG_KEY_0; + else process.env.GIT_CONFIG_KEY_0 = originalGitConfig.key; + if (originalGitConfig.value === undefined) { + delete process.env.GIT_CONFIG_VALUE_0; + } else { + process.env.GIT_CONFIG_VALUE_0 = originalGitConfig.value; + } + + rmSync(dataDir, { recursive: true, force: true }); + rmSync(scratch, { recursive: true, force: true }); + rmSync(fakeHome, { recursive: true, force: true }); + }); + + it('clones the base repo under the override, not the home directory', async () => { + seedBareRepo(join(mirrorRoot, 'acme', 'demo.git'), scratch); + + const baseRepoPath = await ensureBaseRepo('acme', 'demo'); + + expect(baseRepoPath).toBe(join(dataDir, 'repos', 'acme', 'demo')); + expect(existsSync(join(baseRepoPath, '.git'))).toBe(true); + expect(readdirSync(fakeHome)).toEqual([]); + }); + + it('creates the task worktree under the override, not the home directory', async () => { + const bareRepo = join(mirrorRoot, 'acme', 'demo.git'); + seedBareRepo(bareRepo, scratch); + + const baseRepoPath = join(dataDir, 'repos', 'acme', 'demo'); + mkdirSync(join(baseRepoPath, '..'), { recursive: true }); + git(['clone', '--quiet', bareRepo, baseRepoPath]); + + const worktreePath = await createTaskWorktree( + baseRepoPath, + 'issue-226-work', + 'main', + 'ISSUE-226', + ); + + expect(worktreePath).toBe(join(dataDir, 'sessions', 'issue-226', 'demo')); + expect(existsSync(join(worktreePath, 'README.md'))).toBe(true); + expect(readdirSync(fakeHome)).toEqual([]); + }); +}); diff --git a/src-api/test/unit/services/local-archive.test.ts b/src-api/test/unit/services/local-archive.test.ts new file mode 100644 index 00000000..83d8cecb --- /dev/null +++ b/src-api/test/unit/services/local-archive.test.ts @@ -0,0 +1,136 @@ +/** + * Regression coverage for #226. The local archive default root must resolve + * through getAppDir() so NEUMAR_APP_DATA_DIR moves it, and an archive written + * under the pre-rebrand `.neuma` directory must still be found. + */ +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import os from 'node:os'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { LocalArchiveAdapter } from '@/shared/services/publish/destinations/local-archive'; +import type { + LegContext, + PublishLegInput, +} from '@/shared/services/publish/types'; + +const SHA = 'a'.repeat(64); + +function legInput(sourcePath: string, sizeBytes: number): PublishLegInput { + return { + jobId: 'job-226', + legId: 'leg-226', + source: { path: sourcePath, sha256: SHA, sizeBytes, mime: 'video/mp4' }, + metadata: { title: 'Launch Video' }, + destination: { + kind: 'local-archive', + connectionId: 'local', + approvalRequired: false, + }, + }; +} + +const legContext: LegContext = { recordChunkProgress: () => undefined }; + +function archiveTargetDir(rootDir: string): string { + return join(rootDir, '2026', '05', '06', `${SHA.slice(0, 8)}-launch-video`); +} + +describe('LocalArchiveAdapter honours NEUMAR_APP_DATA_DIR (#226)', () => { + let dataDir: string; + let scratch: string; + let sourcePath: string; + /** A stand-in for the developer's real home; nothing may land here. */ + let fakeHome: string; + let homedirSpy: ReturnType; + const originalHome = process.env.HOME; + const originalOverride = process.env.NEUMAR_APP_DATA_DIR; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), 'neumar-226-archive-data-')); + scratch = mkdtempSync(join(tmpdir(), 'neumar-226-archive-src-')); + fakeHome = mkdtempSync(join(tmpdir(), 'neumar-226-archive-home-')); + process.env.HOME = fakeHome; + process.env.NEUMAR_APP_DATA_DIR = dataDir; + homedirSpy = vi.spyOn(os, 'homedir').mockReturnValue(fakeHome); + sourcePath = join(scratch, 'video.mp4'); + writeFileSync(sourcePath, 'hello'); + }); + + afterEach(() => { + homedirSpy.mockRestore(); + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalOverride === undefined) delete process.env.NEUMAR_APP_DATA_DIR; + else process.env.NEUMAR_APP_DATA_DIR = originalOverride; + rmSync(dataDir, { recursive: true, force: true }); + rmSync(scratch, { recursive: true, force: true }); + rmSync(fakeHome, { recursive: true, force: true }); + }); + + it('writes the archive under the override, not the home directory', async () => { + const adapter = new LocalArchiveAdapter({ + now: () => new Date('2026-05-06T12:00:00.000Z'), + }); + + const plan = await adapter.plan(legInput(sourcePath, 5)); + expect(plan.targetPath).toBe(archiveTargetDir(join(dataDir, 'archive'))); + + await adapter.upload(legInput(sourcePath, 5), legContext); + + expect( + existsSync(join(archiveTargetDir(join(dataDir, 'archive')), 'video.mp4')), + ).toBe(true); + expect(readdirSync(fakeHome)).toEqual([]); + }); + + it('migrates an existing .neuma archive to the new root on first write', async () => { + const legacyRoot = join(dataDir, '.neuma', 'archive'); + const legacyArtifactDir = archiveTargetDir(legacyRoot); + mkdirSync(legacyArtifactDir, { recursive: true }); + writeFileSync(join(legacyArtifactDir, 'old.mp4'), 'old'); + + const adapter = new LocalArchiveAdapter({ + now: () => new Date('2026-05-06T12:00:00.000Z'), + }); + await adapter.upload(legInput(sourcePath, 5), legContext); + + // The legacy tree moved intact, so an archive published before the + // rebrand is still reachable at the new root. + expect(existsSync(join(legacyRoot, 'old.mp4'))).toBe(false); + expect( + existsSync(join(archiveTargetDir(join(dataDir, 'archive')), 'old.mp4')), + ).toBe(true); + expect( + existsSync(join(archiveTargetDir(join(dataDir, 'archive')), 'video.mp4')), + ).toBe(true); + }); + + it('never touches a caller-injected root', async () => { + const injectedRoot = join(scratch, 'injected'); + const legacyRoot = join(dataDir, '.neuma', 'archive'); + mkdirSync(archiveTargetDir(legacyRoot), { recursive: true }); + writeFileSync(join(archiveTargetDir(legacyRoot), 'old.mp4'), 'old'); + + const adapter = new LocalArchiveAdapter({ + rootDir: injectedRoot, + now: () => new Date('2026-05-06T12:00:00.000Z'), + }); + await adapter.upload(legInput(sourcePath, 5), legContext); + + expect(existsSync(join(archiveTargetDir(legacyRoot), 'old.mp4'))).toBe( + true, + ); + expect(existsSync(join(injectedRoot, '2026'))).toBe(true); + expect(readdirSync(fakeHome)).toEqual([]); + }); +}); From f016cd628d4fa8e9ebd6fb59c0086da82fd6c482 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 01:10:10 -0400 Subject: [PATCH 10/16] docs: update architecture pages from manifests, database call path, and CI (#244) The tracked architecture pages still described a browser IndexedDB fallback, Vite 7, TypeScript 5.8, Node 20 build targets, a Bun-compiled API sidecar, and a tauri-action release step. None of those match the current tree. Take each statement from its source of truth: src/shared/db/database.ts (an API client for the /db routes), the manifests in package.json, src-api/package.json, and src-tauri/Cargo.toml, and .github/workflows/ci.yml plus build.yml. Also correct stale details found while verifying: react-markdown replaced by streamdown, the Tauri migration list, the workspace member list, the pnpm and oxlint/oxfmt claims, and the validate script summary. --- doc/build/index.md | 26 ++++++++++------ doc/deployment/macos.md | 37 ++++++++++++---------- doc/desktop/index.md | 12 +++++--- doc/frontend/index.md | 11 ++++--- doc/frontend/state-management.md | 53 +++++++++++++++++++------------- doc/system/design-decisions.md | 15 ++++++--- doc/system/overview.md | 18 +++++++---- 7 files changed, 104 insertions(+), 68 deletions(-) diff --git a/doc/build/index.md b/doc/build/index.md index 931e4637..876c356a 100644 --- a/doc/build/index.md +++ b/doc/build/index.md @@ -87,8 +87,9 @@ pnpm dev:web # Web-only frontend via Vite (port 3420) 3. **Tauri config update** — `update_tauri_config()` auto-detects native addon directories and adds their resource glob patterns to `tauri.conf.json`; the original config is backed up before modification and restored on exit via a shell trap 4. **@yao-pkg/pkg** compiles → platform-specific native binary -*CI builds* (`.github/workflows/build.yml`): -1. **Bun compile** (`bun build --compile`) → platform-specific native binary directly +*CI builds* (`.github/workflows/build.yml`): CI runs the same `./scripts/build.sh ` +entry point on its runners, so local and CI sidecars come from the same esbuild plus +`@yao-pkg/pkg` path. Both produce the binary at `src-api/dist/-{target}` @@ -133,10 +134,13 @@ The project uses **oxlint** for linting and **oxfmt** for formatting (migrated f ### Validation ```bash -pnpm validate # lint + typecheck:all + format:check +pnpm validate # root quality gate, no test suite ``` -Runs linting, TypeScript type checking for both frontend and backend, and formatting checks in sequence. +Runs branding, linting, the locale/design/routing/plugin/skill consistency checks, +TypeScript type checking for both frontend and backend, formatting checks, and the +component-size check. It does not run a test suite. The exact script is in the +[root `package.json`](../../package.json). ## Testing @@ -289,7 +293,9 @@ Every test run starts with a clean environment: ## Release Process -Releases follow a **tag-driven** model: CI builds only run when a `v*` tag is pushed to `main`. Normal commits to `main` never trigger a build. +Releases follow a **tag-driven** model: the build workflow runs when a `v*` tag is pushed +and on manual dispatch. Normal commits to `main` never trigger a build. A release workflow +creates the version bump and tag; see [`.github/workflows/release.yml`](../../.github/workflows/release.yml). ### Cutting a Release @@ -366,17 +372,17 @@ Commits that don't match the pattern are silently omitted from the auto-generate | Step | Description | |------|-------------| -| Matrix strategy | Linux, Windows, macOS (Intel + ARM) | +| Matrix strategy | `macos-arm64` is the only uncommented matrix entry; Linux, Windows, and macOS Intel entries are present but commented out | | System deps | WebKit GTK (Linux) | -| Toolchains | Node.js 20, pnpm (latest via action-setup@v4), Bun, Rust | -| API sidecar | Bun compile for each platform | +| Toolchains | Node.js 22 (setup-node), pnpm via `pnpm/action-setup`, Rust | +| API sidecar | `./scripts/build.sh ` runs esbuild plus `@yao-pkg/pkg` with `node22` targets | | CLI bundle | Optional Node.js + Claude Code + Codex — **off by default** (~100 MB extra); enable via `with_cli: true` input | | Windows signing | Azure Key Vault + [relic](https://github.com/sassoftware/relic) — installs Go binary when `AZURE_CLIENT_ID` is set | | macOS signing | Certificate import → `security import` into temp keychain | | macOS notarization | API key (`APPLE_API_KEY` + `APPLE_API_ISSUER` + `APPLE_API_KEY_CONTENT`) preferred; Apple ID fallback | | Frontend | `pnpm build` (Vite) | -| Desktop app | `tauri-action` (builds + signs) | -| Artifacts | DEB/RPM, MSI/EXE/NSIS ZIP, DMG/APP tar.gz (includes `.sig` files for auto-updater) | +| Desktop app | `./scripts/build.sh` runs `pnpm tauri build` (builds + signs) | +| Artifacts | DEB/RPM/AppImage, MSI/EXE/NSIS ZIP, DMG/APP tar.gz (includes `.sig` files for auto-updater) | | Release | GitHub release (draft) with all artifacts; repo name read from `branding.json` | | R2 publish | Uploads installers + `latest.json` to Cloudflare R2 CDN via `scripts/r2-upload.sh` | diff --git a/doc/deployment/macos.md b/doc/deployment/macos.md index 964dbf10..8c105eb7 100644 --- a/doc/deployment/macos.md +++ b/doc/deployment/macos.md @@ -42,7 +42,7 @@ This is a three-layer desktop AI agent application: | Layer | Technology | Purpose | | -------------- | ----------------------------------- | ------------------------------ | -| **Frontend** | React 19 + Vite 7 + Tailwind CSS 4 | Desktop UI (Tauri webview) | +| **Frontend** | React 19 + Vite 8 + Tailwind CSS 4 | Desktop UI (Tauri webview) | | **Backend API** | Node.js + Hono 4 + Claude Agent SDK | AI agent orchestration | | **Desktop Shell** | Tauri 2 + Rust + SQLite | Native wrapper & sidecar mgmt | @@ -64,12 +64,14 @@ This is a three-layer desktop AI agent application: | ------------------ | ---------- | ---------------------------------------------- | | Xcode CLI Tools | Latest | C/C++ compiler, linker, macOS SDK | | Homebrew | Latest | Package manager | -| Node.js | 20 or 22 LTS | JavaScript runtime for the API server | -| pnpm | 9+ | Fast, disk-efficient package manager | +| Node.js | 22 LTS | JavaScript runtime for the API server | +| pnpm | 11.x (pinned via `packageManager`) | Fast, disk-efficient package manager | | Rust (via rustup) | Stable | Compile Tauri shell & native binaries | | Git | Latest | Source control (ships with Xcode CLI Tools) | -> **Note on Node.js versions:** The CI pipeline and `@yao-pkg/pkg` binary targets use Node 20. Node 22 LTS works fine for local development and builds. Either version is supported. +> **Note on Node.js versions:** CI (`.github/workflows/ci.yml` and `build.yml`) uses Node 22, +> and the `@yao-pkg/pkg` binary targets in `src-api/package.json` are `node22-*`. `.nvmrc` +> currently selects Node 25, which is end of life; use a supported Node 22 patch to match CI. ### API Keys (required for runtime) @@ -165,7 +167,8 @@ brew --version ### 4.2 Install Node.js (LTS) -Node.js 20 or 22 LTS are both supported. The project's `pkg` binary targets use `node20`. +Node.js 22 LTS is the supported runtime. CI and the project's `pkg` binary targets use +`node22`. **Option A — Direct via Homebrew (simplest):** @@ -201,15 +204,16 @@ npm --version ### 4.3 Install pnpm -pnpm is required — the project uses pnpm workspaces. +pnpm is required. The project uses pnpm workspaces and pins the version in the root +`package.json` `packageManager` field (currently `pnpm@11.22.0`). ```bash -# Option A — via Homebrew +# Option A: via Homebrew brew install pnpm -# Option B — via Corepack (built into Node.js) +# Option B: via Corepack (built into Node.js), which honours the pinned version corepack enable -corepack prepare pnpm@latest --activate +corepack prepare pnpm@11.22.0 --activate # Option C — via npm npm install -g pnpm @@ -219,7 +223,7 @@ npm install -g pnpm ```bash pnpm --version -# 9.x.x or 10.x.x +# 11.x.x ``` ### 4.4 Install Rust (via rustup) @@ -292,7 +296,7 @@ Expected output: Xcode CLT: /Library/Developer/CommandLineTools Homebrew: Homebrew 4.x.x Node.js: v22.x.x -pnpm: 9.x.x +pnpm: 11.22.0 Rust: rustc 1.8x.x (...) Cargo: cargo 1.8x.x (...) Git: git version 2.x.x @@ -320,13 +324,14 @@ cd ### 5.2 Install All Dependencies -The project uses pnpm workspaces with two packages: root (frontend) and `src-api` (backend). +The project uses pnpm workspaces. `pnpm-workspace.yaml` lists the root package, `src-api`, +`src-video`, and everything under `packages/*` (including `packages/video-ir`). ```bash pnpm install ``` -This installs dependencies for both packages in one go. Expected output: ~1000+ packages, takes ~30-60 seconds. +This installs dependencies for every workspace member in one go. Expected output: ~1000+ packages, takes ~30-60 seconds. **Verify the workspace structure:** @@ -477,13 +482,13 @@ pnpm dev:web ### 7.4 Code Quality Checks ```bash -# Run all checks (lint + typecheck + format) +# Run the root quality gate pnpm validate # Individual checks -pnpm lint # ESLint (frontend) +pnpm lint # oxlint (frontend) pnpm typecheck:all # TypeScript (frontend + API) -pnpm format:check # Prettier format check +pnpm format:check # oxfmt format check ``` --- diff --git a/doc/desktop/index.md b/doc/desktop/index.md index c6c4b59c..ad735566 100644 --- a/doc/desktop/index.md +++ b/doc/desktop/index.md @@ -40,13 +40,15 @@ Migrations run at app startup via the Tauri SQL plugin: | # | Migration | Description | |---|-----------|-------------| | 1 | `initial_schema` | `sessions`, `tasks` (including `work_dir`), `messages` (with `message_id` unique index), `files`, `media_versions`, `settings`, plus all indexes | -| 2 | `add_message_cost_usage_model` | Adds `cost` (REAL), `usage_input` (INTEGER), `usage_output` (INTEGER), `model` (TEXT) columns to `messages` | -| 3 | `add_message_cache_usage` | Adds `usage_cache_read` (INTEGER), `usage_cache_creation` (INTEGER) columns to `messages` | +| 2–5 | `add_message_cost`, `add_message_usage_input`, `add_message_usage_output`, `add_message_model` | Adds `cost` (REAL), `usage_input` (INTEGER), `usage_output` (INTEGER), `model` (TEXT) columns to `messages` | +| 6–7 | `add_message_usage_cache_read`, `add_message_usage_cache_creation` | Adds the cache token columns to `messages` | +| 8–10 | `add agui_type to messages`, `add run_id to messages`, `add step_name to messages` | Adds the AG-UI columns to `messages` | All tables use `IF NOT EXISTS` for idempotent execution. The backend database module -(`src-api/src/shared/db/index.ts`) uses a consolidated two-migration system (001 init + 002 catchup) -that produces the same schema. The Tauri SQLite plugin and backend `better-sqlite3` -instance produce identical schemas. +(`src-api/src/shared/db/index.ts`) owns the same schema through its own versioned migrations +(`src-api/src/shared/db/migrations/`) on `better-sqlite3`. The Tauri SQLite plugin holds the +desktop-local `settings` copy that `src/shared/db/settings.ts` reads directly; the rest of +the application data is served by the API through the `/db` routes. ## Sidecar Management diff --git a/doc/frontend/index.md b/doc/frontend/index.md index dac954ff..6621607c 100644 --- a/doc/frontend/index.md +++ b/doc/frontend/index.md @@ -98,7 +98,7 @@ src/ │ ├── shared/ # Shared logic and utilities │ ├── db/ # Database abstraction layer -│ │ ├── database.ts # Dual-backend DB (SQLite / IndexedDB) +│ │ ├── database.ts # API client for the backend /db routes │ │ ├── settings.ts # Settings persistence (cache + DB) │ │ └── types.ts # TypeScript interfaces for all entities │ ├── hooks/ # Custom React hooks @@ -142,7 +142,7 @@ src/ │ └── i18n/translations.ts # Translation type definitions │ ├── types/ # Global type declarations -│ └── react-markdown.d.ts # Module augmentation +│ └── hyperframes-player.d.ts # Module augmentation for the HyperFrames player ├── vite-env.d.ts # Vite env types (ImportMetaEnv: VITE_API_PORT) │ └── main.tsx # Application entry point @@ -155,12 +155,13 @@ The application bootstraps through the following chain: ``` index.html └── main.tsx - ├── Initialize settings from database (await) + ├── initWorkspaces() ← registers workspace panel components + ├── initializeSettings() (await) ├── ErrorBoundary ← catches unhandled React errors, shows recovery UI │ └── LanguageProvider ← detects system language, loads translations │ └── ThemeProvider ← applies CSS variables, listens to OS theme │ └── RouterProvider ← renders matched route (lazy-loaded pages) - └── Database migration (automatic on first load) + └── startWorkspaceWatcher() ← background file watcher, Tauri only ``` Key bootstrap steps: @@ -169,7 +170,7 @@ Key bootstrap steps: 2. **Error boundary** — `react-error-boundary` wraps the entire tree with a fallback UI 3. **Provider wrapping** — language and theme providers wrap the entire tree 4. **Lazy routing** — page components are code-split via `React.lazy()` with `Suspense` fallbacks -5. **Database migration** — creates/updates tables on first load (SQLite or IndexedDB) +5. **Database migration** runs on the API server, which owns the SQLite database ## Routing diff --git a/doc/frontend/state-management.md b/doc/frontend/state-management.md index 08c2ba76..f0ea02e1 100644 --- a/doc/frontend/state-management.md +++ b/doc/frontend/state-management.md @@ -1,5 +1,5 @@ --- -summary: "React state patterns (Context API, hooks, local state), database abstraction layer (SQLite + IndexedDB), and data flow architecture" +summary: "React state patterns (Context API, hooks, local state), the API-backed database client, and data flow architecture" read_when: - Understanding how state flows through the application - Working with the database layer @@ -22,7 +22,7 @@ The frontend uses a mix of React built-in state management and lightweight exter | **useSyncExternalStore** | OS-level | System theme detection (dark/light mode media query) | | **Custom hooks** | Feature-scoped | `useAgent` (V1), `useProviders`, `useVitePreview` | | **Local state** | Component-scoped | `useState`, `useRef` for per-component data | -| **Database** | Persistent | SQLite/IndexedDB for sessions, tasks, messages, settings | +| **Database** | Persistent | API-backed SQLite for sessions, tasks, messages, settings | | **In-memory cache** | Performance | `settingsCache` map for synchronous settings access | Context providers use React 19's `` syntax (replacing the legacy @@ -74,25 +74,37 @@ persistence. The frontend only displays messages from the SSE stream; it does no agent messages to the database. This eliminates duplicate writes and ensures consistency across multiple clients observing the same task. -## Database Abstraction Layer +## Database Client -The `src/shared/db/database.ts` module implements a **dual-backend storage abstraction**: +The `src/shared/db/database.ts` module is an **API client**, not a storage engine. Every +entity operation is an HTTP call to the API server's `/db` routes: ``` -┌─────────────────────────────────────────────┐ -│ Unified Database API │ -│ (createSession, saveTasks, getMessages...) │ -└────────────┬───────────────┬────────────────┘ - │ │ - ┌───────▼───────┐ ┌────▼──────────┐ - │ SQLite │ │ IndexedDB │ - │ (Tauri plugin) │ │ (Browser) │ - └───────────────┘ └───────────────┘ +┌──────────────────────────────────────────────────┐ +│ Frontend database client │ +│ (createSession, saveTasks, getMessages, ...) │ +│ src/shared/db/database.ts │ +└────────────────────────┬─────────────────────────┘ + │ fetch(`${API_BASE_URL}/db`) + ▼ +┌──────────────────────────────────────────────────┐ +│ API server (src-api/src/app/api/db.ts) │ +│ better-sqlite3, one SQLite database │ +└──────────────────────────────────────────────────┘ ``` -**Environment detection:** checks for `window.__TAURI_INTERNALS__` to select the backend. +The same client serves the browser and the desktop webview, so there is no environment +detection and no second storage backend. The API server owns the SQLite file and the +versioned migrations under `src-api/src/shared/db/migrations/`. -**Schema** (9 tables + 3 virtual tables): +Settings are the one exception: `src/shared/db/settings.ts` reads the `settings` table +through the Tauri SQL plugin when it is available, and mirrors a JSON copy into +`localStorage` for synchronous reads. Backend-synced keys fall back to +`POST /providers/settings/sync` when the local copy is missing a key. The only other +browser-side storage in the app is a small IndexedDB cache of video waveform peaks +(`src/components/video/timeline/waveformCache.ts`), which is unrelated to application data. + +**Schema** (the tables this client reaches through `/db`): | Table | Key Columns | Purpose | |-------|-------------|---------| @@ -102,12 +114,11 @@ The `src/shared/db/database.ts` module implements a **dual-backend storage abstr | `files` | id, task_id, name, type, path, is_favorite | Generated file library | | `media_versions` | id, task_id, artifact_id, version_number, path, type | Artifact version tracking | | `settings` | key, value, updated_at | User preferences | -| `memories` | id, content, category, importance, source, has_embedding | Long-term memory records | -| `embedding_cache` | content_hash, model, embedding (BLOB), dim | Embedding dedup cache | -| `session_memory_chunks` | id, task_id, chunk_index, content, has_embedding | Session transcript chunks | -| `vec_memories` | memory_id, embedding (virtual — sqlite-vec) | Vector ANN index for memories | -| `vec_session_chunks` | chunk_id, embedding (virtual — sqlite-vec) | Vector ANN index for session chunks | -| `memories_fts` | content (virtual — FTS5) | Full-text search index for memories | + +The memory system (`memories`, `embedding_cache`, `session_memory_chunks`, and the +`sqlite-vec` / FTS5 virtual tables) lives in the same database but is reached through the +`/memory` routes rather than `/db`; see [Memory System](../backend/memory.md) and the +[Database Schema](../reference/database-schema.md) for the full table list. ## Settings Layer diff --git a/doc/system/design-decisions.md b/doc/system/design-decisions.md index cc349377..425ff75c 100644 --- a/doc/system/design-decisions.md +++ b/doc/system/design-decisions.md @@ -36,11 +36,16 @@ The i18n needs are modest (6 languages, ~300 keys). A custom system provides typ translations, zero bundle overhead, and simple nested object access without the complexity of libraries like react-intl or i18next. -## Why Dual Database Backend? - -SQLite (via Tauri plugin) is the primary persistence layer for the desktop app. IndexedDB -fallback enables running the frontend as a standalone web app during development without -Tauri, preserving the full development experience. +## Why the API Owns the Database? + +The API server is the single writer of one SQLite database, opened with `better-sqlite3` in +`src-api/src/shared/db/index.ts`. The frontend reaches it through the `/db` REST routes, so a +browser client, the Tauri webview, and any second client all read and write the same data +instead of keeping a private copy. `src/shared/db/database.ts` is that API client in both +modes and holds no browser storage engine of its own. Settings are the exception: they read +the Tauri SQL plugin directly and mirror a copy to `localStorage`, with the API copy +authoritative for the keys it syncs. The only other browser storage is an IndexedDB cache +of video waveform peaks, which holds no application data. ## Why pnpm Workspaces? diff --git a/doc/system/overview.md b/doc/system/overview.md index 77f6199d..06e71777 100644 --- a/doc/system/overview.md +++ b/doc/system/overview.md @@ -38,7 +38,7 @@ all without manual intervention until final merge approval. | **Session safety** | Per-session cost caps and agent loop detection via SessionBudgetGuard | | **Workspace isolation** | All file operations are confined to user-configured workspace directories | | **Dual runtime** | Development runs processes separately; production bundles everything into a single app | -| **Offline-capable persistence** | SQLite (desktop) with IndexedDB fallback (browser) | +| **API-owned persistence** | One SQLite database owned by the API server; the frontend reaches it through the `/db` API in both browser and desktop modes | | **Long-term memory** | Persistent cross-session memory with hybrid vector + keyword search (sqlite-vec + FTS5) | | **Autonomous operation** | Linear integration pipeline runs end-to-end without human intervention (except final merge) | | **Voice I/O** | Provider-agnostic TTS/STT with streaming WebSocket STT, sentence-by-sentence TTS, and offline local models | @@ -115,18 +115,24 @@ The application operates differently in development and production environments: |--------|-------------|------------| | **Frontend** | Vite dev server (port 3420) | Bundled into Tauri webview (`dist/`) | | **API Server** | Node.js process (port 5126) | Native sidecar binary (port 2620) | -| **Database** | SQLite via Tauri plugin | SQLite via Tauri plugin | -| **Browser fallback** | IndexedDB + localStorage | N/A (always Tauri) | +| **Database** | SQLite through the API `/db` routes | SQLite through the API `/db` routes | +| **Browser mode** | Same frontend and API, opened in a browser instead of the webview | N/A (always Tauri) | | **Hot reload** | Full HMR via Vite | Not applicable | +The frontend has no separate browser database for application data. +`src/shared/db/database.ts` is an API client for `${API_BASE_URL}/db` in both modes, and the +API server owns the single SQLite database through `better-sqlite3`. Settings are the one +layer that still reads the Tauri SQL plugin directly and mirrors a copy to `localStorage`; +see [State Management](../frontend/state-management.md). + ## Technology Stack | Layer | Technologies | |-------|-------------| -| **Frontend** | React 19, Vite 7, TypeScript 5.8, Tailwind CSS 4, Radix UI, react-router-dom 7, react-markdown, react-error-boundary, @copilotkit/react-core, @assistant-ui/react, @ag-ui/client | -| **Backend API** | Hono 4, Node.js, @anthropic-ai/claude-agent-sdk, @hono/zod-validator, @modelcontextprotocol/sdk, @linear/sdk, Zod 4, better-sqlite3, sqlite-vec, @huggingface/transformers, @hono/node-ws, sherpa-onnx-node, @ag-ui/encoder, @copilotkit/runtime | +| **Frontend** | React 19, Vite 8, TypeScript 6, Tailwind CSS 4, Radix UI primitives, react-router-dom 7, streamdown, react-error-boundary, @copilotkit/react-core, @assistant-ui/react, @ag-ui/client | +| **Backend API** | Hono 4, Node.js 22, @anthropic-ai/claude-agent-sdk, @hono/zod-validator, @modelcontextprotocol/sdk, @linear/sdk, Zod 4, better-sqlite3, sqlite-vec, @huggingface/transformers, @hono/node-ws, sherpa-onnx-node, @ag-ui/encoder, @copilotkit/runtime | | **Desktop Shell** | Tauri 2, Rust, tauri-plugin-sql (SQLite), tauri-plugin-shell, tauri-plugin-fs, tauri-plugin-dialog | -| **Build Tools** | pnpm workspaces, esbuild, @yao-pkg/pkg (local), Bun compile (CI), TypeScript | +| **Build Tools** | pnpm workspaces, esbuild, @yao-pkg/pkg (`node22` binary targets), TypeScript | | **CI/CD** | GitHub Actions, matrix builds (Linux/Windows/macOS) | --- From f56cc0c658c80366eade8e0e61866f9e8a72ece0 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 14:55:36 -0400 Subject: [PATCH 11/16] test(api): prove every migration runs against its own schema (#261) A failed read used to mean "empty", so an unguarded ALTER TABLE on a table that does not exist aborted ten later migrations and left the database half-upgraded with no error. `addColumnIfMissing` now names the missing table instead of surfacing SQLite's generic message. The new suite drops each table a migration alters, reruns the registry, and asserts the resulting schema and applied versions match a database built from scratch. Migrations that repair a missing video intent table are listed explicitly; every other missing prerequisite is corruption and must throw without advancing the version. --- src-api/src/shared/db/migrations/utils.ts | 7 +- src-api/test/unit/db/migrations-drift.test.ts | 230 ++++++++++++++++++ 2 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 src-api/test/unit/db/migrations-drift.test.ts diff --git a/src-api/src/shared/db/migrations/utils.ts b/src-api/src/shared/db/migrations/utils.ts index 528a0698..1c006bb4 100644 --- a/src-api/src/shared/db/migrations/utils.ts +++ b/src-api/src/shared/db/migrations/utils.ts @@ -16,13 +16,18 @@ export function hasColumn( return cols.some((c) => c.name === column); } -/** Conditionally add a column if it doesn't exist yet. */ +/** Add a missing column; an absent required table is schema corruption. */ export function addColumnIfMissing( db: Database.Database, table: string, column: string, definition: string, ): void { + if (!hasTable(db, table)) { + throw new Error( + `Cannot add column ${table}.${column}: required table ${table} does not exist`, + ); + } if (!hasColumn(db, table, column)) { db.exec(`ALTER TABLE ${table} ADD COLUMN ${column} ${definition}`); } diff --git a/src-api/test/unit/db/migrations-drift.test.ts b/src-api/test/unit/db/migrations-drift.test.ts new file mode 100644 index 00000000..3756c56b --- /dev/null +++ b/src-api/test/unit/db/migrations-drift.test.ts @@ -0,0 +1,230 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { readFileSync, readdirSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import * as sqliteVec from 'sqlite-vec'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + closeDatabase, + Database, + DATABASE_MIGRATIONS, + getDatabase, +} from '@/shared/db'; +import { runMigrations } from '@/shared/db/migrations/runner'; +import { addColumnIfMissing } from '@/shared/db/migrations/utils'; + +const migrationsDir = fileURLToPath( + new URL('../../../src/shared/db/migrations/', import.meta.url), +); +const sortedMigrations = [...DATABASE_MIGRATIONS].sort( + (a, b) => a.version - b.version, +); +// Every migration that adds columns or rebuilds a table. Missing video intent +// tables are supported release-line drift; other missing prerequisites are corruption. +const alterCallers = [ + { version: 28, table: 'agent_profiles' }, + { version: 29, table: 'channel_config' }, + { version: 30, table: 'files' }, + { version: 44, table: 'tasks' }, + { version: 45, table: 'installed_plugins' }, + { version: 50, table: 'messages' }, + { version: 61, table: 'agent_profiles' }, + { version: 70, table: 'webui_sessions' }, + { version: 71, table: 'channel_config' }, + { version: 77, table: 'publish_destination_legs' }, + { version: 83, table: 'publish_jobs' }, + { version: 86, table: 'linked_assets' }, + { version: 88, table: 'embedding_cache' }, + { version: 93, table: 'tasks' }, + { version: 94, table: 'video_projects' }, + { version: 95, table: 'installed_plugins' }, + { version: 96, table: 'video_intent_log', repairable: true }, + { version: 97, table: 'video_plugin_candidates' }, + { version: 99, table: 'agent_runs' }, + { version: 101, table: 'agent_runs' }, + { version: 102, table: 'trace_events' }, + { version: 103, table: 'agent_runs' }, + { version: 104, table: 'agent_runs' }, + { version: 105, table: 'video_intent_log', repairable: true }, + { version: 106, table: 'messages' }, + { version: 108, table: 'video_intent_log', repairable: true }, +]; + +function schema(database: Database.Database) { + sqliteVec.load(database); + const tables = database + .prepare( + "SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%' ORDER BY name", + ) + .all() as Array<{ name: string }>; + return tables.map(({ name }) => ({ + name, + columns: database + .prepare( + 'SELECT name, type, "notnull", dflt_value, pk FROM pragma_table_info(?) ORDER BY name', + ) + .all(name), + indexes: database + .prepare( + 'SELECT name, "unique", partial FROM pragma_index_list(?) ORDER BY name', + ) + .all(name), + })); +} + +function versions(database: Database.Database) { + return database + .prepare('SELECT version FROM _migrations ORDER BY version') + .all(); +} + +let appDir: string; +const connections: Database.Database[] = []; +function memoryDatabase() { + const database = new Database(':memory:'); + connections.push(database); + return database; +} + +beforeEach(() => { + closeDatabase(); + appDir = mkdtempSync(join(tmpdir(), 'neumar-migration-drift-')); + vi.stubEnv('NEUMAR_APP_DATA_DIR', appDir); +}); +afterEach(async () => { + await new Promise((resolve) => setImmediate(resolve)); + closeDatabase(); + for (const database of connections.splice(0)) database.close(); + vi.unstubAllEnvs(); + rmSync(appDir, { recursive: true, force: true }); +}); + +describe('full migration registry upgrade histories', () => { + it('audits every migration with an ALTER caller', () => { + const audited = readdirSync(migrationsDir) + .filter((file) => /^\d{3}_.*\.ts$/.test(file)) + .flatMap((file) => { + const source = readFileSync(join(migrationsDir, file), 'utf8'); + if (!/addColumnIfMissing\(|ALTER TABLE/.test(source)) return []; + const match = /version:\s*(\d+),/.exec(source); + if (!match) throw new Error(`Missing version in ${file}`); + return [Number(match[1])]; + }) + .sort((a, b) => a - b); + expect(alterCallers.map(({ version }) => version)).toEqual(audited); + }); + + it('initializes a fresh database and reopens a current one without changing schema or user data', () => { + const database = getDatabase(); + const expectedSchema = schema(database); + const expectedVersions = versions(database); + expect(expectedVersions).toEqual( + sortedMigrations.map(({ version }) => ({ version })), + ); + database + .prepare('INSERT INTO settings (key, value) VALUES (?, ?)') + .run('migration-sentinel', 'keep'); + closeDatabase(); + const reopened = getDatabase(); + expect(schema(reopened)).toEqual(expectedSchema); + expect(versions(reopened)).toEqual(expectedVersions); + expect( + reopened + .prepare('SELECT value FROM settings WHERE key = ?') + .get('migration-sentinel'), + ).toEqual({ value: 'keep' }); + expect(reopened.pragma('integrity_check', { simple: true })).toBe('ok'); + }); + + it.each(alterCallers)( + 'finishes the entire registry when migration $version has schema but no tracking record', + ({ version }) => { + const database = memoryDatabase(); + const prefix = sortedMigrations.filter( + (migration) => migration.version <= version, + ); + runMigrations(database, prefix); + database + .prepare('DELETE FROM _migrations WHERE version = ?') + .run(version); + database + .prepare('INSERT INTO settings (key, value) VALUES (?, ?)') + .run('migration-sentinel', 'keep'); + runMigrations(database, DATABASE_MIGRATIONS); + expect(schema(database)).toEqual(schema(getDatabase())); + expect(versions(database)).toEqual(versions(getDatabase())); + expect( + database + .prepare('SELECT value FROM settings WHERE key = ?') + .get('migration-sentinel'), + ).toEqual({ value: 'keep' }); + expect(database.pragma('integrity_check', { simple: true })).toBe('ok'); + }, + ); + + it.each(alterCallers)( + 'handles the missing prerequisite for migration $version according to its policy', + ({ version, table, repairable }) => { + const database = memoryDatabase(); + runMigrations( + database, + sortedMigrations.filter((migration) => migration.version < version), + ); + database.exec(`DROP TABLE ${table}`); + if (repairable) { + runMigrations(database, DATABASE_MIGRATIONS); + expect(schema(database)).toEqual(schema(getDatabase())); + expect(versions(database)).toEqual(versions(getDatabase())); + } else { + const before = versions(database); + expect(() => runMigrations(database, DATABASE_MIGRATIONS)).toThrow( + new RegExp(table), + ); + expect(versions(database)).toEqual(before); + } + }, + ); +}); + +describe('migration failure publication boundary', () => { + it('distinguishes a missing required table from a missing column', () => { + const database = memoryDatabase(); + expect(() => + addColumnIfMissing(database, 'missing_tasks', 'mode', 'TEXT'), + ).toThrow( + 'Cannot add column missing_tasks.mode: required table missing_tasks does not exist', + ); + }); + + it('refuses a current database missing a required column on every initialization attempt', () => { + getDatabase().exec('ALTER TABLE messages DROP COLUMN is_error'); + closeDatabase(); + expect(() => getDatabase()).toThrow( + /missing required column\(s\): messages.is_error/, + ); + expect(() => getDatabase()).toThrow( + /missing required column\(s\): messages.is_error/, + ); + const repaired = new Database(join(appDir, 'database.db')); + try { + repaired.exec( + 'ALTER TABLE messages ADD COLUMN is_error INTEGER NOT NULL DEFAULT 0', + ); + } finally { + repaired.close(); + } + expect(() => getDatabase()).not.toThrow(); + }); + + it('rejects a damaged SQLite file and retries after the file is repaired', () => { + const dbPath = join(appDir, 'database.db'); + writeFileSync(dbPath, 'damaged database bytes'); + expect(() => getDatabase()).toThrow(/not a database/i); + expect(() => getDatabase()).toThrow(/not a database/i); + rmSync(dbPath); + expect(() => getDatabase()).not.toThrow(); + }); +}); From c253fd82b235906d6f809d9e093d9ce30ac99b4f Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 14:55:39 -0400 Subject: [PATCH 12/16] refactor(video): type withVideoRefResolution so tests need no casts (#262) The wrapper typed every handler as (args: never) and round-tripped the tool list and resolved args through `as unknown as` / `as never`. Give the handler a single bivariant input type (projectId: string, rest unknown) so each tool's own typed handler is assignable without a cast and valid calls compile at the call site. Tool names, descriptions and input schemas are unchanged on the wire. Add a compile-time check that the wrapped handler types projectId. Refs #224 --- src-api/src/shared/mcp/video-edit-server.ts | 40 +++++++++++-------- .../video/video-tool-refs-dispatcher.test.ts | 17 +++++++- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src-api/src/shared/mcp/video-edit-server.ts b/src-api/src/shared/mcp/video-edit-server.ts index 3c76cde6..8f65254c 100644 --- a/src-api/src/shared/mcp/video-edit-server.ts +++ b/src-api/src/shared/mcp/video-edit-server.ts @@ -2613,10 +2613,17 @@ export function createVideoEditTools(options: VideoEditServerOptions = {}) { */ const SELF_RESOLVING_TOOLS = new Set(['video_apply_timeline_ops']); -type CallableVideoToolHandler = ( - args: Record, - extra: unknown, -) => Promise; +/** + * Tools are looked up by name, so a caller cannot know which input shape it + * is passing. The handler is declared with method syntax so each tool's own + * handler (`args` inferred from its zod shape) stays assignable here through + * parameter bivariance, and valid input compiles without a cast. + */ +type VideoToolArgs = { projectId?: string } & Record; + +type CallableVideoToolHandler = { + bivarianceHack(args: VideoToolArgs, extra: unknown): Promise; +}['bivarianceHack']; type ResolvedVideoTool = Omit< Tool, @@ -2624,10 +2631,7 @@ type ResolvedVideoTool = Omit< > & { handler: CallableVideoToolHandler }; function withVideoRefResolution< - Tool extends { - name: string; - handler: (args: never, extra: unknown) => Promise; - }, + Tool extends { name: string; handler: CallableVideoToolHandler }, >( tools: readonly Tool[], options: VideoEditServerOptions, @@ -2636,8 +2640,10 @@ function withVideoRefResolution< const inner = definition.handler; return { ...definition, - handler: async (args: never, extra: unknown): Promise => { - const input = args as unknown as { projectId?: string }; + handler: async ( + args: VideoToolArgs, + extra: unknown, + ): Promise => { const execute = async () => { if ( SELF_RESOLVING_TOOLS.has(definition.name) || @@ -2645,14 +2651,16 @@ function withVideoRefResolution< ) { return inner(args, extra); } - let resolved: unknown; + let resolved: VideoToolArgs; try { - const project = await loadProjectForTool(input.projectId, options); + const project = await loadProjectForTool(args.projectId, options); + // Ref resolution only swaps ref strings for ids, so an object + // payload stays an object of the same shape. resolved = resolveVideoRefs({ value: args, project, refs: selectionResolverRefs(options), - }); + }) as VideoToolArgs; } catch (error) { return errorResult( error instanceof VideoRefResolutionError @@ -2662,9 +2670,9 @@ function withVideoRefResolution< : String(error), ); } - return inner(resolved as never, extra); + return inner(resolved, extra); }; - const projectId = input.projectId ?? activeContext(options).projectId; + const projectId = args.projectId ?? activeContext(options).projectId; if (!projectId) return execute(); return runLoggedVideoOperation({ projectId, @@ -2674,7 +2682,7 @@ function withVideoRefResolution< }); }, }; - }) as unknown as Array>; + }); } function buildVideoEditTools(options: VideoEditServerOptions) { diff --git a/src-api/test/unit/video/video-tool-refs-dispatcher.test.ts b/src-api/test/unit/video/video-tool-refs-dispatcher.test.ts index fc946040..ec3a7f77 100644 --- a/src-api/test/unit/video/video-tool-refs-dispatcher.test.ts +++ b/src-api/test/unit/video/video-tool-refs-dispatcher.test.ts @@ -2,7 +2,15 @@ import fs from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + afterEach, + beforeEach, + describe, + expect, + expectTypeOf, + it, + vi, +} from 'vitest'; import { createVideoEditTools } from '@/shared/mcp/video-edit-server'; import { getProject, writeProject } from '@/shared/video/store'; @@ -41,6 +49,13 @@ describe('video-edit dispatcher ref resolution', () => { return found; } + it('types the wrapped handler input so valid calls need no cast', () => { + type Handler = ReturnType['handler']; + expectTypeOf[0]['projectId']>().toEqualTypeOf< + string | undefined + >(); + }); + it('resolves clipIndex for a named edit tool, not just the ops batch', async () => { const result = await findTool('video_set_clip_speed').handler( { clipIds: ['clipIndex:1'], speed: 2, applyMode: 'propose' }, From 092816ae9dadd0c3c747605826784e56cd964c2e Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 18:37:03 -0400 Subject: [PATCH 13/16] fix(api): make unsupported 501 routes truthful and pin V1 task route (#263) * fix(api): make unsupported 501 routes truthful and pin V1 task route POST /memory/entities/extract, /memory/consolidate and the chat surface of POST /plugins/:id/apply now answer 501 not_available_over_http with a reason that names no unreachable alternative. No UI or external caller exists for any of them. Keep /task/:taskId mounted as the reversible assumption pending the open V1 retirement decision, and pin both task routes with a wiring test. Refs #223 * docs(task): reflect approved V1 route retention Refs #223 --- doc/backend/api-routes.md | 4 +- src-api/src/app/api/memory.ts | 24 ++++++-- src-api/src/app/api/plugins.ts | 10 +++- src-api/test/integration/api/memory.test.ts | 35 +++++++++++ .../integration/api/plugins-apply.test.ts | 60 +++++++++++++++++++ src/__tests__/app/router-task-routes.test.ts | 32 ++++++++++ src/app/router.tsx | 4 ++ 7 files changed, 160 insertions(+), 9 deletions(-) create mode 100644 src-api/test/integration/api/plugins-apply.test.ts create mode 100644 src/__tests__/app/router-task-routes.test.ts diff --git a/doc/backend/api-routes.md b/doc/backend/api-routes.md index 1e3f8277..4b3c3c21 100644 --- a/doc/backend/api-routes.md +++ b/doc/backend/api-routes.md @@ -158,7 +158,7 @@ DesignMode, task flows, and VideoMode. Plugin ids contain slashes | `GET` | `/plugins/:id/config` | Read public config values from `metadata.neuma.configSchema` | | `PUT` | `/plugins/:id/config` | Validate and save plugin config; secret fields use the secret store | | `GET` | `/plugins/:id/preview` | Serve a design-system plugin's `components.html` preview for sandboxed iframes | -| `POST` | `/plugins/:id/apply` | Apply a plugin to `task`, `design`, or `video`; `chat` returns 501 | +| `POST` | `/plugins/:id/apply` | Apply a plugin to `task`, `design`, or `video`; `chat` returns 501 `not_available_over_http` (no apply step) | | `POST` | `/plugins/install` | Install from `local`, `github`, `url`, or `marketplace` source | | `POST` | `/plugins/:id/enable` | Enable an installed or bundled plugin | | `POST` | `/plugins/:id/disable` | Disable an installed or bundled plugin | @@ -572,6 +572,8 @@ The feedback API is local-first: feedback is written to SQLite before any extern | `GET` | `/memory/:id` | Get a specific memory by ID | | `PUT` | `/memory/:id` | Update a memory (Zod-validated) | | `DELETE` | `/memory/:id` | Delete a memory | +| `POST` | `/memory/entities/extract` | Always 501 `not_available_over_http`: extraction needs an LLM callback and has no HTTP or MCP trigger | +| `POST` | `/memory/consolidate` | Always 501 `not_available_over_http`: consolidation needs an LLM callback and is not started automatically | ## Soul diff --git a/src-api/src/app/api/memory.ts b/src-api/src/app/api/memory.ts index 89d1d4a9..8c693dd6 100644 --- a/src-api/src/app/api/memory.ts +++ b/src-api/src/app/api/memory.ts @@ -487,12 +487,19 @@ memory.get('/entities/:id/graph', async (c) => { return c.json(graph); }); -// POST /memory/entities/extract — force entity extraction from recent memories +// POST /memory/entities/extract: not available over HTTP. +// +// Extraction needs an LLM callback, and an HTTP request has no agent context +// to supply one. The response names no workaround because none exists today: +// no MCP tool triggers extraction, and nothing calls the exported +// extractAndStoreEntities service. Naming an unreachable alternative would +// repeat the problem this message replaces. memory.post('/entities/extract', async (c) => { return c.json( { - error: - 'Entity extraction requires an LLM callback — use the MCP tool instead', + error: 'not_available_over_http', + message: + 'Entity extraction needs an LLM callback and has no HTTP or MCP trigger; it runs only from a caller that supplies one.', }, 501, ); @@ -514,12 +521,17 @@ memory.post('/:id/unpin', async (c) => { return c.json(mem); }); -// POST /memory/consolidate — trigger consolidation run +// POST /memory/consolidate: not available over HTTP. +// +// Consolidation needs an LLM callback, and startConsolidation has no caller, +// so the previous advice ("use the settings UI or startup scheduling") named +// two paths that cannot run. State the limit instead of the phantom fix. memory.post('/consolidate', async (c) => { return c.json( { - error: - 'Consolidation requires an LLM callback — use the settings UI or startup scheduling', + error: 'not_available_over_http', + message: + 'Consolidation needs an LLM callback and is not started automatically; it runs only from a caller that supplies one.', }, 501, ); diff --git a/src-api/src/app/api/plugins.ts b/src-api/src/app/api/plugins.ts index 020ede05..89245f13 100644 --- a/src-api/src/app/api/plugins.ts +++ b/src-api/src/app/api/plugins.ts @@ -321,11 +321,17 @@ pluginsRoutes.post( const id = c.req.param('id'); const body = c.req.valid('json'); + // Chat has no apply step: an enabled plugin's skills reach the agent on + // every run, and the marketplace "Use" action routes to Home with the + // plugin pre-attached. There is no UI caller for this branch, so the + // message names the supported path instead of a "follow-up checkpoint" + // that is not scheduled anywhere. if (body.surface === 'chat') { return c.json( { - error: 'not implemented', - message: 'Chat plugin apply ships in a follow-up checkpoint.', + error: 'not_available_over_http', + message: + 'Chat has no apply step. Enable the plugin and start the task from Home; its skills are included in the run.', }, 501, ); diff --git a/src-api/test/integration/api/memory.test.ts b/src-api/test/integration/api/memory.test.ts index 9af9f25d..c20b7744 100644 --- a/src-api/test/integration/api/memory.test.ts +++ b/src-api/test/integration/api/memory.test.ts @@ -536,4 +536,39 @@ describe('Memory API', () => { expect(body).toHaveProperty('consolidation'); }); }); + + // ── HTTP-only unavailable routes (issue #223) ── + // + // Both routes need an LLM callback an HTTP request cannot supply, so they + // answer 501. The regression this guards is the message: it previously + // named alternatives that do not exist ("use the MCP tool", "startup + // scheduling"), which sent callers to another dead end. + + describe('POST /entities/extract', () => { + it('answers 501 with a reason that names no unreachable alternative', async () => { + const { memoryRoutes } = await import('@/app/api/memory'); + const res = await memoryRoutes.request('/entities/extract', { + method: 'POST', + }); + expect(res.status).toBe(501); + const body = (await res.json()) as { error: string; message: string }; + expect(body.error).toBe('not_available_over_http'); + expect(body.message).toContain('LLM callback'); + expect(body.message).not.toMatch(/MCP tool/i); + }); + }); + + describe('POST /consolidate', () => { + it('answers 501 with a reason that names no unreachable alternative', async () => { + const { memoryRoutes } = await import('@/app/api/memory'); + const res = await memoryRoutes.request('/consolidate', { + method: 'POST', + }); + expect(res.status).toBe(501); + const body = (await res.json()) as { error: string; message: string }; + expect(body.error).toBe('not_available_over_http'); + expect(body.message).toContain('LLM callback'); + expect(body.message).not.toMatch(/settings UI|startup scheduling/i); + }); + }); }); diff --git a/src-api/test/integration/api/plugins-apply.test.ts b/src-api/test/integration/api/plugins-apply.test.ts new file mode 100644 index 00000000..05bca248 --- /dev/null +++ b/src-api/test/integration/api/plugins-apply.test.ts @@ -0,0 +1,60 @@ +/** + * POST /plugins/:id/apply over the chat surface (issue #223). + * + * Chat has no apply step: an enabled plugin's skills reach every run, and the + * marketplace "Use" action routes to Home with the plugin pre-attached. The + * route therefore answers 501, and this test pins the message so it keeps + * naming the supported path instead of a follow-up checkpoint that does not + * exist. + * + * The route is exercised through `pluginsRoutes.request()`, the same boundary + * the HTTP daemon mounts at `/plugins`, so a change to the route registration + * or the surface branch fails here rather than only in a unit of the apply + * helper. + */ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { closeDatabase } from '@/shared/db'; + +vi.mock('@/shared/utils/logger', () => ({ + createLogger: () => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }), +})); + +afterEach(() => { + closeDatabase(); +}); + +function applyChatSurface(pluginId: string): Promise { + return import('@/app/api/plugins').then(({ pluginsRoutes }) => + pluginsRoutes.request(`/${encodeURIComponent(pluginId)}/apply`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ surface: 'chat' }), + }), + ); +} + +describe('POST /plugins/:id/apply (chat surface)', () => { + it('answers 501 and points at the path that actually works', async () => { + const res = await applyChatSurface('user/hallmark'); + + expect(res.status).toBe(501); + const body = (await res.json()) as { error: string; message: string }; + expect(body.error).toBe('not_available_over_http'); + expect(body.message).toMatch(/Home/); + expect(body.message).not.toMatch(/follow-up checkpoint/i); + }); + + it('answers 501 before it looks the plugin up', async () => { + // The branch must not depend on an installed plugin: the request below + // names an id that is not installed, and a 404 here would mean the surface + // check moved behind the lookup. + const res = await applyChatSurface('does-not-exist'); + expect(res.status).toBe(501); + }); +}); diff --git a/src/__tests__/app/router-task-routes.test.ts b/src/__tests__/app/router-task-routes.test.ts new file mode 100644 index 00000000..0e4e089b --- /dev/null +++ b/src/__tests__/app/router-task-routes.test.ts @@ -0,0 +1,32 @@ +import { matchRoutes } from 'react-router-dom'; + +/** + * Task route wiring (issue #223). + * + * The maintainer approved keeping `/task/:taskId` alongside `/task-v2/:taskId`: + * https://github.com/bravew/Neumar/issues/223#issuecomment-6176446031 + * Video docs/capture tooling still uses V1, and complete V2 parity has not been + * established. These assertions guard the approved wiring against accidental + * deletion; no redirect or retirement is included. + */ +import { describe, expect, it } from 'vitest'; + +import { routeLoaderId } from '@/app/route-preload'; +import { router } from '@/app/router'; + +function matchedPaths(pathname: string): (string | undefined)[] { + const matches = matchRoutes(router.routes as never, pathname); + return (matches ?? []).map((m) => (m.route as { path?: string }).path); +} + +describe('task route wiring', () => { + it('serves both task routes', () => { + expect(matchedPaths('/task/abc')).toContain('task/:taskId'); + expect(matchedPaths('/task-v2/abc')).toContain('task-v2/:taskId'); + }); + + it('preloads the matching lazy chunk for each route', () => { + expect(routeLoaderId('/task/abc')).toBe('task'); + expect(routeLoaderId('/task-v2/abc')).toBe('task-v2'); + }); +}); diff --git a/src/app/router.tsx b/src/app/router.tsx index 396d169d..7cd7b6e3 100644 --- a/src/app/router.tsx +++ b/src/app/router.tsx @@ -53,6 +53,10 @@ export const router = createBrowserRouter([ ), }, + // Keep V1 alongside V2 per the maintainer's decision on issue #223: + // https://github.com/bravew/Neumar/issues/223#issuecomment-6176446031 + // Video docs/capture tooling still uses V1, and complete V2 parity + // has not been established. No redirect or retirement is included. { path: 'task/:taskId', element: ( From 331e2d82554d481366d869c17c67342dd97277df Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 18:38:25 -0400 Subject: [PATCH 14/16] refactor(video): extract overlay library tools from video-edit-server (#273) Move the four vivid-overlay library tools (list presets, save preset, save style from template, save user document) plus their exclusive schemas and color-normalization helper into src-api/src/shared/mcp/video-overlay-library-tools.ts. The factory receives session resolution, the proposal-only policy, and the result helpers from the host, so registration still passes through the typed wrapper and each tool name, description, schema, and registration position is unchanged. The real MCP tools/list inventory is byte-identical before and after for all four transcript/multicam flag combinations. video-edit-server.ts drops from 7,157 to 6,879 lines. Refs #228 --- src-api/src/shared/mcp/video-edit-server.ts | 298 +---------------- .../shared/mcp/video-overlay-library-tools.ts | 314 ++++++++++++++++++ .../test/unit/video/video-edit-server.test.ts | 52 +++ 3 files changed, 376 insertions(+), 288 deletions(-) create mode 100644 src-api/src/shared/mcp/video-overlay-library-tools.ts diff --git a/src-api/src/shared/mcp/video-edit-server.ts b/src-api/src/shared/mcp/video-edit-server.ts index 8f65254c..c225f895 100644 --- a/src-api/src/shared/mcp/video-edit-server.ts +++ b/src-api/src/shared/mcp/video-edit-server.ts @@ -21,7 +21,6 @@ import { deriveTimelineClipFrameFields, durationMsToFrames, KeyframeSchema, - KeyframeTrackSchema, KeyframeablePropertySchema, normalizeFrameRate, TimelineOpSchema, @@ -34,7 +33,6 @@ import type { ClipEffectStack, FrameRate, TimelineOp, - VividOverlayPresetDef, } from '@neumar/video-ir'; import { z } from 'zod'; @@ -51,6 +49,7 @@ import { multicamToolSchemas, shouldRegisterMulticamTools, } from '@/shared/mcp/video-multicam-tools'; +import { createVideoOverlayLibraryTools } from '@/shared/mcp/video-overlay-library-tools'; import { validatePath } from '@/shared/services/ffmpeg'; import { getSessionContext } from '@/shared/services/session-context'; import { createLogger } from '@/shared/utils/logger'; @@ -115,7 +114,6 @@ import { VideoBudgetExceededError, VideoCostApprovalError, } from '@/shared/video/cost-approval'; -import { normalizeCssColor } from '@/shared/video/css-colors'; import { buildCurrentVideoContext, CURRENT_VIDEO_CONTEXT_INCLUDES, @@ -160,19 +158,6 @@ import { NarrationDraftError, } from '@/shared/video/narration-draft'; import { vividOverlayContextSummary } from '@/shared/video/overlays/context-summary'; -import { - findVividOverlayPreset, - VIDEO_OVERLAY_REGISTRY, -} from '@/shared/video/overlays/registry'; -import { - saveUserOverlayDocument, - UserOverlayDocumentError, -} from '@/shared/video/overlays/user-documents'; -import { saveUserOverlayPreset } from '@/shared/video/overlays/user-presets'; -import { - saveUserOverlayStyle, - UserOverlayStyleError, -} from '@/shared/video/overlays/user-styles'; import { AI_CLIP_POLL_TIMEOUT_MS, cancelRender, @@ -296,37 +281,6 @@ import type { const logger = createLogger('VideoEditMCP'); const DEFAULT_TIMELINE_FPS = 30; -const OVERLAY_CONTROL_VALUE_SCHEMA = z.union([ - z.string(), - z.number(), - z.boolean(), -]); - -const OVERLAY_STYLE_TRANSFORM_SCHEMA = z - .object({ - scale: z.number().finite().optional(), - scaleX: z.number().finite().optional(), - scaleY: z.number().finite().optional(), - positionX: z.number().finite().optional(), - positionY: z.number().finite().optional(), - opacity: z.number().finite().optional(), - rotation: z.number().finite().optional(), - }) - .strict(); - -const USER_OVERLAY_DOCUMENT_CONTROL_SCHEMA = z - .object({ - id: z.string().min(1).max(80), - type: z.enum(['number', 'color', 'text', 'select', 'toggle']), - label: z.string().min(1).max(120), - defaultValue: OVERLAY_CONTROL_VALUE_SCHEMA, - min: z.number().finite().optional(), - max: z.number().finite().optional(), - step: z.number().positive().optional(), - options: z.array(z.string().min(1).max(80)).max(40).optional(), - }) - .strict(); - export const VIDEO_EDIT_TOOL_NAMES = [ 'video_get_plan', 'video_get_plan_progress', @@ -3091,230 +3045,15 @@ function buildVideoEditTools(options: VideoEditServerOptions) { } }, ), - tool( - 'video_list_overlay_presets', - 'Return the vivid-overlay preset catalog: id, category, tags (the video-to-template classifier label space), control schema, durations, placement metadata, and optional taste metadata for routing choices. Use this to pick the closest preset when recreating an overlay seen in reference footage. This tool is read-only.', - { - projectId: PROJECT_ID_SCHEMA, - category: z.string().min(1).optional(), - }, - async ({ projectId, category }) => { - try { - const resolvedProjectId = resolveProjectId(projectId, options); - const catalog: readonly VividOverlayPresetDef[] = - VIDEO_OVERLAY_REGISTRY; - const presets = catalog - .filter((preset) => !category || preset.category === category) - .map((preset) => ({ - id: preset.id, - backend: preset.backend, - category: preset.category, - tags: preset.tags ?? [], - controls: preset.controls.map((control) => ({ - id: control.id, - type: control.type, - defaultValue: control.defaultValue, - min: control.min, - max: control.max, - step: control.step, - options: control.options, - })), - requiresSourceAsset: preset.requiresSourceAsset ?? false, - defaultDurationMs: preset.defaultDurationMs, - minDurationMs: preset.minDurationMs, - aspectAffinity: preset.aspectAffinity ?? 'any', - anchor: preset.anchor ?? 'center', - taste: preset.taste, - })); - return jsonResult({ - schema: 'neuma.video.overlay-presets.v1', - projectId: resolvedProjectId, - presets, - }); - } catch (error) { - return errorResult( - error instanceof Error ? error.message : String(error), - ); - } - }, - ), - tool( - 'video_save_overlay_preset', - 'Save a derived overlay preset to the user\'s "My overlays" library: a built-in base preset plus control values (extracted text, colors, sizes) and loop mode. Use after matching reference footage to the closest catalog preset via video_list_overlay_presets. Color values accept CSS names and are normalized to hex.', - { - projectId: PROJECT_ID_SCHEMA, - reasoning: REASONING_SCHEMA, - name: z.string().min(1).max(80), - basePresetId: z.string().min(1), - controls: z.record( - z.string(), - z.union([z.string(), z.number(), z.boolean()]), - ), - loop: z.enum(['loop', 'hold', 'none']).optional(), - }, - async ({ projectId, name, basePresetId, controls, loop }) => { - try { - const proposal = await proposalOnlyServiceMutationResult( - projectId, - options, - 'video_save_overlay_preset', - ); - if (proposal) return proposal; - const resolvedProjectId = resolveProjectId(projectId, options); - const normalized = normalizeOverlayControlValues( - basePresetId, - controls, - ); - const preset = await saveUserOverlayPreset({ - name, - basePresetId, - controls: normalized, - loop, - }); - return jsonResult({ - schema: 'neuma.video.user-overlay-preset.v1', - projectId: resolvedProjectId, - preset, - }); - } catch (error) { - return errorResult( - error instanceof Error ? error.message : String(error), - ); - } - }, - ), - tool( - 'video_save_overlay_style_from_template', - 'Save the video-to-template v2 result as a reusable overlay style: closest built-in preset plus extracted controls, optional transform/keyframes/tags, and video-to-template provenance. Prefer this over video_save_overlay_preset when the reference overlay includes placement, scale, opacity, or motion that should remain editable.', - { - projectId: PROJECT_ID_SCHEMA, - reasoning: REASONING_SCHEMA, - name: z.string().min(1).max(80), - basePresetId: z.string().min(1), - controls: z.record(z.string(), OVERLAY_CONTROL_VALUE_SCHEMA), - loop: z.enum(['loop', 'hold', 'none']).optional(), - transform: OVERLAY_STYLE_TRANSFORM_SCHEMA.optional(), - keyframes: z.array(KeyframeTrackSchema).max(50).optional(), - tags: z.array(z.string().min(1).max(40)).max(24).optional(), - sourceId: z.string().min(1).optional(), - }, - async ({ - projectId, - name, - basePresetId, - controls, - loop, - transform, - keyframes, - tags, - sourceId, - }) => { - try { - const proposal = await proposalOnlyServiceMutationResult( - projectId, - options, - 'video_save_overlay_style_from_template', - ); - if (proposal) return proposal; - const resolvedProjectId = resolveProjectId(projectId, options); - const style = await saveUserOverlayStyle({ - name, - basePresetId, - controls: normalizeOverlayControlValues(basePresetId, controls), - loop, - transform, - keyframes, - tags, - provenance: { - kind: 'video-to-template', - ...(sourceId ? { sourceId } : {}), - }, - }); - return jsonResult({ - schema: 'neuma.video.user-overlay-style.v1', - projectId: resolvedProjectId, - style, - }); - } catch (error) { - return errorResult( - error instanceof UserOverlayStyleError - ? error.message - : error instanceof Error - ? error.message - : String(error), - ); - } - }, - ), - tool( - 'video_save_user_overlay_document', - "Save an explicitly approved custom vivid-overlay HTML document after linting it with Neuma's deterministic overlay authoring contract. Only call when the user has opted into custom document synthesis; otherwise use video_save_overlay_style_from_template.", - { - projectId: PROJECT_ID_SCHEMA, - reasoning: REASONING_SCHEMA, - userConfirmed: z.literal(true), - name: z.string().min(1).max(80), - html: z.string().min(1).max(200_000), - controls: z - .array(USER_OVERLAY_DOCUMENT_CONTROL_SCHEMA) - .max(40) - .optional(), - tags: z.array(z.string().min(1).max(40)).max(24).optional(), - provenanceKind: z - .enum(['agent', 'video-to-template']) - .default('video-to-template'), - sourceId: z.string().min(1).optional(), - }, - async ({ - projectId, - userConfirmed, - name, - html, - controls, - tags, - provenanceKind, - sourceId, - }) => { - try { - const proposal = await proposalOnlyServiceMutationResult( - projectId, - options, - 'video_save_user_overlay_document', - ); - if (proposal) return proposal; - const resolvedProjectId = resolveProjectId(projectId, options); - const document = await saveUserOverlayDocument({ - name, - html, - controls, - tags, - userConfirmed, - provenance: { - kind: provenanceKind ?? 'video-to-template', - ...(sourceId ? { sourceId } : {}), - }, - }); - return jsonResult({ - schema: 'neuma.video.user-overlay-document.v1', - projectId: resolvedProjectId, - document, - }); - } catch (error) { - if (error instanceof UserOverlayDocumentError) { - return errorResult( - JSON.stringify({ - error: error.message, - code: error.code, - issues: error.issues, - }), - ); - } - return errorResult( - error instanceof Error ? error.message : String(error), - ); - } - }, - ), + ...createVideoOverlayLibraryTools({ + projectIdSchema: PROJECT_ID_SCHEMA, + reasoningSchema: REASONING_SCHEMA, + resolveProjectId: (projectId) => resolveProjectId(projectId, options), + proposalOnlyServiceMutationResult: (projectId, toolName) => + proposalOnlyServiceMutationResult(projectId, options, toolName), + jsonResult, + errorResult, + }), tool( 'video_get_transition_seams', 'Return editable timeline transition seams with seam IDs, adjacent clip context, duration constraints, current transitions, and blocked reasons. This tool is read-only.', @@ -7127,23 +6866,6 @@ function createVideoEditMutationTools(options: VideoEditServerOptions = {}) { ]; } -function normalizeOverlayControlValues( - basePresetId: string, - controls: Record, -): Record { - const base = findVividOverlayPreset(basePresetId); - if (!base) return controls; - return Object.fromEntries( - Object.entries(controls).map(([id, value]) => [ - id, - base.controls.find((control) => control.id === id)?.type === 'color' && - typeof value === 'string' - ? normalizeCssColor(value) - : value, - ]), - ); -} - export function createVideoEditServer(options: VideoEditServerOptions = {}) { return createSdkMcpServer({ name: 'video-edit', diff --git a/src-api/src/shared/mcp/video-overlay-library-tools.ts b/src-api/src/shared/mcp/video-overlay-library-tools.ts new file mode 100644 index 00000000..f8c7cc5e --- /dev/null +++ b/src-api/src/shared/mcp/video-overlay-library-tools.ts @@ -0,0 +1,314 @@ +import { tool } from '@anthropic-ai/claude-agent-sdk/core'; +import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import { KeyframeTrackSchema } from '@neumar/video-ir'; +import type { VividOverlayPresetDef } from '@neumar/video-ir'; +import { z } from 'zod'; + +import { normalizeCssColor } from '@/shared/video/css-colors'; +import { + findVividOverlayPreset, + VIDEO_OVERLAY_REGISTRY, +} from '@/shared/video/overlays/registry'; +import { + saveUserOverlayDocument, + UserOverlayDocumentError, +} from '@/shared/video/overlays/user-documents'; +import { saveUserOverlayPreset } from '@/shared/video/overlays/user-presets'; +import { + saveUserOverlayStyle, + UserOverlayStyleError, +} from '@/shared/video/overlays/user-styles'; + +const OVERLAY_CONTROL_VALUE_SCHEMA = z.union([ + z.string(), + z.number(), + z.boolean(), +]); + +const OVERLAY_STYLE_TRANSFORM_SCHEMA = z + .object({ + scale: z.number().finite().optional(), + scaleX: z.number().finite().optional(), + scaleY: z.number().finite().optional(), + positionX: z.number().finite().optional(), + positionY: z.number().finite().optional(), + opacity: z.number().finite().optional(), + rotation: z.number().finite().optional(), + }) + .strict(); + +const USER_OVERLAY_DOCUMENT_CONTROL_SCHEMA = z + .object({ + id: z.string().min(1).max(80), + type: z.enum(['number', 'color', 'text', 'select', 'toggle']), + label: z.string().min(1).max(120), + defaultValue: OVERLAY_CONTROL_VALUE_SCHEMA, + min: z.number().finite().optional(), + max: z.number().finite().optional(), + step: z.number().positive().optional(), + options: z.array(z.string().min(1).max(80)).max(40).optional(), + }) + .strict(); + +/** The host supplies session resolution and mutation policy; handlers stay under its typed wrapper. */ +interface OverlayLibraryToolContext { + projectIdSchema: z.ZodOptional; + reasoningSchema: z.ZodOptional; + resolveProjectId: (projectId: string | undefined) => string; + proposalOnlyServiceMutationResult: ( + projectId: string | undefined, + toolName: string, + ) => Promise; + jsonResult: (value: unknown) => CallToolResult; + errorResult: (message: string) => CallToolResult; +} + +export function createVideoOverlayLibraryTools({ + projectIdSchema: PROJECT_ID_SCHEMA, + reasoningSchema: REASONING_SCHEMA, + resolveProjectId, + proposalOnlyServiceMutationResult, + jsonResult, + errorResult, +}: OverlayLibraryToolContext) { + return [ + tool( + 'video_list_overlay_presets', + 'Return the vivid-overlay preset catalog: id, category, tags (the video-to-template classifier label space), control schema, durations, placement metadata, and optional taste metadata for routing choices. Use this to pick the closest preset when recreating an overlay seen in reference footage. This tool is read-only.', + { + projectId: PROJECT_ID_SCHEMA, + category: z.string().min(1).optional(), + }, + async ({ projectId, category }) => { + try { + const resolvedProjectId = resolveProjectId(projectId); + const catalog: readonly VividOverlayPresetDef[] = + VIDEO_OVERLAY_REGISTRY; + const presets = catalog + .filter((preset) => !category || preset.category === category) + .map((preset) => ({ + id: preset.id, + backend: preset.backend, + category: preset.category, + tags: preset.tags ?? [], + controls: preset.controls.map((control) => ({ + id: control.id, + type: control.type, + defaultValue: control.defaultValue, + min: control.min, + max: control.max, + step: control.step, + options: control.options, + })), + requiresSourceAsset: preset.requiresSourceAsset ?? false, + defaultDurationMs: preset.defaultDurationMs, + minDurationMs: preset.minDurationMs, + aspectAffinity: preset.aspectAffinity ?? 'any', + anchor: preset.anchor ?? 'center', + taste: preset.taste, + })); + return jsonResult({ + schema: 'neuma.video.overlay-presets.v1', + projectId: resolvedProjectId, + presets, + }); + } catch (error) { + return errorResult( + error instanceof Error ? error.message : String(error), + ); + } + }, + ), + tool( + 'video_save_overlay_preset', + 'Save a derived overlay preset to the user\'s "My overlays" library: a built-in base preset plus control values (extracted text, colors, sizes) and loop mode. Use after matching reference footage to the closest catalog preset via video_list_overlay_presets. Color values accept CSS names and are normalized to hex.', + { + projectId: PROJECT_ID_SCHEMA, + reasoning: REASONING_SCHEMA, + name: z.string().min(1).max(80), + basePresetId: z.string().min(1), + controls: z.record( + z.string(), + z.union([z.string(), z.number(), z.boolean()]), + ), + loop: z.enum(['loop', 'hold', 'none']).optional(), + }, + async ({ projectId, name, basePresetId, controls, loop }) => { + try { + const proposal = await proposalOnlyServiceMutationResult( + projectId, + 'video_save_overlay_preset', + ); + if (proposal) return proposal; + const resolvedProjectId = resolveProjectId(projectId); + const normalized = normalizeOverlayControlValues( + basePresetId, + controls, + ); + const preset = await saveUserOverlayPreset({ + name, + basePresetId, + controls: normalized, + loop, + }); + return jsonResult({ + schema: 'neuma.video.user-overlay-preset.v1', + projectId: resolvedProjectId, + preset, + }); + } catch (error) { + return errorResult( + error instanceof Error ? error.message : String(error), + ); + } + }, + ), + tool( + 'video_save_overlay_style_from_template', + 'Save the video-to-template v2 result as a reusable overlay style: closest built-in preset plus extracted controls, optional transform/keyframes/tags, and video-to-template provenance. Prefer this over video_save_overlay_preset when the reference overlay includes placement, scale, opacity, or motion that should remain editable.', + { + projectId: PROJECT_ID_SCHEMA, + reasoning: REASONING_SCHEMA, + name: z.string().min(1).max(80), + basePresetId: z.string().min(1), + controls: z.record(z.string(), OVERLAY_CONTROL_VALUE_SCHEMA), + loop: z.enum(['loop', 'hold', 'none']).optional(), + transform: OVERLAY_STYLE_TRANSFORM_SCHEMA.optional(), + keyframes: z.array(KeyframeTrackSchema).max(50).optional(), + tags: z.array(z.string().min(1).max(40)).max(24).optional(), + sourceId: z.string().min(1).optional(), + }, + async ({ + projectId, + name, + basePresetId, + controls, + loop, + transform, + keyframes, + tags, + sourceId, + }) => { + try { + const proposal = await proposalOnlyServiceMutationResult( + projectId, + 'video_save_overlay_style_from_template', + ); + if (proposal) return proposal; + const resolvedProjectId = resolveProjectId(projectId); + const style = await saveUserOverlayStyle({ + name, + basePresetId, + controls: normalizeOverlayControlValues(basePresetId, controls), + loop, + transform, + keyframes, + tags, + provenance: { + kind: 'video-to-template', + ...(sourceId ? { sourceId } : {}), + }, + }); + return jsonResult({ + schema: 'neuma.video.user-overlay-style.v1', + projectId: resolvedProjectId, + style, + }); + } catch (error) { + return errorResult( + error instanceof UserOverlayStyleError + ? error.message + : error instanceof Error + ? error.message + : String(error), + ); + } + }, + ), + tool( + 'video_save_user_overlay_document', + "Save an explicitly approved custom vivid-overlay HTML document after linting it with Neuma's deterministic overlay authoring contract. Only call when the user has opted into custom document synthesis; otherwise use video_save_overlay_style_from_template.", + { + projectId: PROJECT_ID_SCHEMA, + reasoning: REASONING_SCHEMA, + userConfirmed: z.literal(true), + name: z.string().min(1).max(80), + html: z.string().min(1).max(200_000), + controls: z + .array(USER_OVERLAY_DOCUMENT_CONTROL_SCHEMA) + .max(40) + .optional(), + tags: z.array(z.string().min(1).max(40)).max(24).optional(), + provenanceKind: z + .enum(['agent', 'video-to-template']) + .default('video-to-template'), + sourceId: z.string().min(1).optional(), + }, + async ({ + projectId, + userConfirmed, + name, + html, + controls, + tags, + provenanceKind, + sourceId, + }) => { + try { + const proposal = await proposalOnlyServiceMutationResult( + projectId, + 'video_save_user_overlay_document', + ); + if (proposal) return proposal; + const resolvedProjectId = resolveProjectId(projectId); + const document = await saveUserOverlayDocument({ + name, + html, + controls, + tags, + userConfirmed, + provenance: { + kind: provenanceKind ?? 'video-to-template', + ...(sourceId ? { sourceId } : {}), + }, + }); + return jsonResult({ + schema: 'neuma.video.user-overlay-document.v1', + projectId: resolvedProjectId, + document, + }); + } catch (error) { + if (error instanceof UserOverlayDocumentError) { + return errorResult( + JSON.stringify({ + error: error.message, + code: error.code, + issues: error.issues, + }), + ); + } + return errorResult( + error instanceof Error ? error.message : String(error), + ); + } + }, + ), + ]; +} + +function normalizeOverlayControlValues( + basePresetId: string, + controls: Record, +): Record { + const base = findVividOverlayPreset(basePresetId); + if (!base) return controls; + return Object.fromEntries( + Object.entries(controls).map(([id, value]) => [ + id, + base.controls.find((control) => control.id === id)?.type === 'color' && + typeof value === 'string' + ? normalizeCssColor(value) + : value, + ]), + ); +} diff --git a/src-api/test/unit/video/video-edit-server.test.ts b/src-api/test/unit/video/video-edit-server.test.ts index 4c0a1c19..2a76b5f4 100644 --- a/src-api/test/unit/video/video-edit-server.test.ts +++ b/src-api/test/unit/video/video-edit-server.test.ts @@ -2565,6 +2565,58 @@ describe('overlay preset catalog + save tools (video-to-template MVP)', () => { expect(await listUserOverlayPresets()).toHaveLength(0); }); + it.each([ + { + toolName: 'video_save_overlay_preset', + input: { + name: 'External preset', + basePresetId: 'html.marker-highlight', + controls: { text: 'Important', color: 'green' }, + }, + }, + { + toolName: 'video_save_overlay_style_from_template', + input: { + name: 'External style', + basePresetId: 'html.marker-highlight', + controls: { text: 'Important', color: 'green' }, + sourceId: 'reference-1', + }, + }, + { + toolName: 'video_save_user_overlay_document', + input: { + name: 'External document', + userConfirmed: true, + html: 'Live', + }, + }, + ])( + 'keeps $toolName proposal-only through the registered tool boundary', + async ({ toolName, input }) => { + const result = await findTool(toolName, { + clientKind: 'external-mcp', + }).handler(input, {}); + + expect(result.isError).not.toBe(true); + expect(JSON.parse(toolResultText(result) ?? '{}')).toMatchObject({ + schema: 'neuma.video.mcp-proposal-required.v1', + projectId: 'project-1', + mode: 'proposal-only', + tool: toolName, + }); + const { listUserOverlayPresets } = + await import('@/shared/video/overlays/user-presets'); + const { listUserOverlayStyles } = + await import('@/shared/video/overlays/user-styles'); + const { listUserOverlayDocuments } = + await import('@/shared/video/overlays/user-documents'); + expect(await listUserOverlayPresets()).toEqual([]); + expect(await listUserOverlayStyles()).toEqual([]); + expect(await listUserOverlayDocuments()).toEqual([]); + }, + ); + it('rejects saves against unknown base presets', async () => { const tool = findTool('video_save_overlay_preset'); const result = await tool.handler( From 3c4fe7926c5a56108d4630d66004afcb8dca5b96 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 23:20:01 -0400 Subject: [PATCH 15/16] docs(channels): fix ownership decision link fragment (#288) --- doc/backend/channel-ownership.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/backend/channel-ownership.md b/doc/backend/channel-ownership.md index 0460cca7..336ed6ea 100644 --- a/doc/backend/channel-ownership.md +++ b/doc/backend/channel-ownership.md @@ -14,7 +14,7 @@ This document is the responsibility matrix requested by issue #222, part of epic #193. It records what each live channel provider actually does today, with a `path:line` citation for every cell and the live consumer named. It is a pre-migration inventory, not a design. No runtime is chosen here; see -[Decision needed](#decision-needed). +[Decision needed](#5-decision-needed). Evidence was read from the tree at `1bb20406de946723795f7f9fed20777b7fa00d58`, which is the head of `epic/193-architecture-ownership` and the commit this branch From 6a4cc42364475b977ca969672bca0178054a2186 Mon Sep 17 00:00:00 2001 From: Yong Date: Wed, 14 Oct 2026 23:29:12 -0400 Subject: [PATCH 16/16] fix(publish): migrate archives from the actual legacy home path (#287) The predecessor wrote to ~/.neuma/archive, but migration searched inside the current app directory and its test repeated that incorrect path. Adopt the legacy sibling only for the default app-data root and preserve isolation for overrides and caller-injected roots. Refs #226, #283 --- .../publish/destinations/local-archive.ts | 9 +-- .../test/unit/services/local-archive.test.ts | 58 ++++++++++++++----- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src-api/src/shared/services/publish/destinations/local-archive.ts b/src-api/src/shared/services/publish/destinations/local-archive.ts index 282eb989..2c6a5878 100644 --- a/src-api/src/shared/services/publish/destinations/local-archive.ts +++ b/src-api/src/shared/services/publish/destinations/local-archive.ts @@ -47,9 +47,10 @@ export class LocalArchiveAdapter implements PublishDestinationAdapter { this.rootDir = options.rootDir ?? path.join(getAppDir(), 'archive'); this.now = options.now ?? (() => new Date()); this.indexFile = options.indexFile; - // Only the default root has a legacy predecessor. An injected root is a - // test or caller's own directory and must not be touched. - this.migrateLegacyRoot = options.rootDir === undefined; + // Only the default app directory can adopt the old home archive. An + // app-data override or caller-injected root must stay isolated from it. + this.migrateLegacyRoot = + options.rootDir === undefined && !process.env.NEUMAR_APP_DATA_DIR?.trim(); } capabilities(): DestinationCapabilities { @@ -154,7 +155,7 @@ export class LocalArchiveAdapter implements PublishDestinationAdapter { */ private async adoptLegacyArchive(): Promise { const legacyRoot = path.join( - getAppDir(), + path.dirname(path.dirname(this.rootDir)), LEGACY_ARCHIVE_DIR_NAME, 'archive', ); diff --git a/src-api/test/unit/services/local-archive.test.ts b/src-api/test/unit/services/local-archive.test.ts index 83d8cecb..726b24fb 100644 --- a/src-api/test/unit/services/local-archive.test.ts +++ b/src-api/test/unit/services/local-archive.test.ts @@ -17,6 +17,8 @@ import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { APP_DIR_NAME } from '@/config/constants'; + import { LocalArchiveAdapter } from '@/shared/services/publish/destinations/local-archive'; import type { LegContext, @@ -45,7 +47,7 @@ function archiveTargetDir(rootDir: string): string { return join(rootDir, '2026', '05', '06', `${SHA.slice(0, 8)}-launch-video`); } -describe('LocalArchiveAdapter honours NEUMAR_APP_DATA_DIR (#226)', () => { +describe('LocalArchiveAdapter app-data isolation and legacy migration (#226)', () => { let dataDir: string; let scratch: string; let sourcePath: string; @@ -93,33 +95,58 @@ describe('LocalArchiveAdapter honours NEUMAR_APP_DATA_DIR (#226)', () => { expect(readdirSync(fakeHome)).toEqual([]); }); - it('migrates an existing .neuma archive to the new root on first write', async () => { - const legacyRoot = join(dataDir, '.neuma', 'archive'); + it('migrates the predecessor ~/.neuma/archive on first write with no override', async () => { + delete process.env.NEUMAR_APP_DATA_DIR; + const legacyRoot = join(fakeHome, '.neuma', 'archive'); + const legacyArtifactDir = archiveTargetDir(legacyRoot); + mkdirSync(legacyArtifactDir, { recursive: true }); + writeFileSync(join(legacyArtifactDir, 'old.mp4'), 'old'); + const targetRoot = join(fakeHome, APP_DIR_NAME, 'archive'); + + const adapter = new LocalArchiveAdapter({ + now: () => new Date('2026-05-06T12:00:00.000Z'), + }); + await adapter.upload(legInput(sourcePath, 5), legContext); + + expect(existsSync(join(legacyArtifactDir, 'old.mp4'))).toBe(false); + expect(existsSync(join(archiveTargetDir(targetRoot), 'old.mp4'))).toBe( + true, + ); + expect(existsSync(join(archiveTargetDir(targetRoot), 'video.mp4'))).toBe( + true, + ); + }); + + it('leaves the home archive untouched when an app-data override is set', async () => { + const legacyRoot = join(fakeHome, '.neuma', 'archive'); const legacyArtifactDir = archiveTargetDir(legacyRoot); mkdirSync(legacyArtifactDir, { recursive: true }); writeFileSync(join(legacyArtifactDir, 'old.mp4'), 'old'); + homedirSpy.mockClear(); const adapter = new LocalArchiveAdapter({ now: () => new Date('2026-05-06T12:00:00.000Z'), }); await adapter.upload(legInput(sourcePath, 5), legContext); - // The legacy tree moved intact, so an archive published before the - // rebrand is still reachable at the new root. - expect(existsSync(join(legacyRoot, 'old.mp4'))).toBe(false); + expect(homedirSpy).not.toHaveBeenCalled(); + expect(existsSync(join(legacyArtifactDir, 'old.mp4'))).toBe(true); expect( existsSync(join(archiveTargetDir(join(dataDir, 'archive')), 'old.mp4')), - ).toBe(true); + ).toBe(false); expect( existsSync(join(archiveTargetDir(join(dataDir, 'archive')), 'video.mp4')), ).toBe(true); }); - it('never touches a caller-injected root', async () => { + it('leaves the home archive untouched when a caller injects a root', async () => { + delete process.env.NEUMAR_APP_DATA_DIR; const injectedRoot = join(scratch, 'injected'); - const legacyRoot = join(dataDir, '.neuma', 'archive'); - mkdirSync(archiveTargetDir(legacyRoot), { recursive: true }); - writeFileSync(join(archiveTargetDir(legacyRoot), 'old.mp4'), 'old'); + const legacyRoot = join(fakeHome, '.neuma', 'archive'); + const legacyArtifactDir = archiveTargetDir(legacyRoot); + mkdirSync(legacyArtifactDir, { recursive: true }); + writeFileSync(join(legacyArtifactDir, 'old.mp4'), 'old'); + homedirSpy.mockClear(); const adapter = new LocalArchiveAdapter({ rootDir: injectedRoot, @@ -127,10 +154,13 @@ describe('LocalArchiveAdapter honours NEUMAR_APP_DATA_DIR (#226)', () => { }); await adapter.upload(legInput(sourcePath, 5), legContext); - expect(existsSync(join(archiveTargetDir(legacyRoot), 'old.mp4'))).toBe( + expect(homedirSpy).not.toHaveBeenCalled(); + expect(existsSync(join(legacyArtifactDir, 'old.mp4'))).toBe(true); + expect(existsSync(join(archiveTargetDir(injectedRoot), 'old.mp4'))).toBe( + false, + ); + expect(existsSync(join(archiveTargetDir(injectedRoot), 'video.mp4'))).toBe( true, ); - expect(existsSync(join(injectedRoot, '2026'))).toBe(true); - expect(readdirSync(fakeHome)).toEqual([]); }); });