Skip to content

fix(#187 AC#2 r3): multi-user partial collision + pause race + zombie polling#249

Merged
jayzalowitz merged 2 commits into
mainfrom
fix/embedded-llm-r3-findings
May 11, 2026
Merged

fix(#187 AC#2 r3): multi-user partial collision + pause race + zombie polling#249
jayzalowitz merged 2 commits into
mainfrom
fix/embedded-llm-r3-findings

Conversation

@jayzalowitz

Copy link
Copy Markdown
Owner

Summary

Follow-up to PR #247 (the embedded LLM model downloader). Copilot's third-round review landed after I merged — four substantive findings that need fixing.

Findings addressed

1. Multi-user partial-file collision

Two users on the same API host downloading the same model both wrote to <modelDir>/<modelId>.gguf.partial. Concurrent streams would corrupt each other; one user's cancel would delete the other's partial.

Fix: the final GGUF is content-addressable (we SHA-256 verify before atomic rename), so it stays shared at <modelDir>/<modelId>.gguf. Partials now namespace by the download row id: <modelDir>/<modelId>.gguf.<download.id>.partial.

2. Pause-on-pending was silently overridden

pauseDownload() set DB to paused, but the already-scheduled runDownload() would start anyway and immediately overwrite back to downloading. Same path for pending→cancelled — the user's intent got nuked by the async runner picking up the pre-pause row.

Fix: at the top of runDownload(), re-fetch the row from DB. If status flipped to paused/cancelled/complete/failed between startDownload() returning and the runner picking up, return early.

3. Polling continued after navigating away

The 1s poll callback in embedded-llm-card.js had no termination condition tied to page navigation. Going to Approvals while a download was in flight kept hitting /api/embedded-llm/downloads/:id every second forever and held a reference to a detached #embedded-llm-card-target node.

Fix: poll callback now checks window.location.hash !== '#/settings' and document.getElementById(CARD_TARGET_ID) !== container at the top of each tick, and self-cancels.

4. ensureDirectory() dead code

Already removed in round-2 fix on PR #247 itself.

Test plan

  • pnpm build --concurrency=1 clean
  • 25 embedded-llm route tests still passing
  • Manual: two simultaneous downloads (different users, same model) don't collide
  • Manual: click Pause on a pending row before transfer starts → row stays paused
  • Manual: start a download, navigate to Approvals → polling stops within 1s

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 9, 2026 18:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Follow-up fixes to the embedded LLM model downloader (PR #247) focusing on multi-user download safety, pause/cancel race correctness, and client-side polling cleanup.

Changes:

  • Namespace in-flight partial files by download row id to prevent cross-user corruption/cancel side effects.
  • Re-fetch download row state at the start of runDownload() to respect pause/cancel/terminal transitions that occur before the async runner starts.
  • Stop the Settings UI polling loop when navigating away or when the card container is replaced/detached.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
CHANGELOG.md Documents the round-3 follow-up fixes and their motivation.
apps/web/public/js/components/embedded-llm-card.js Adds self-cancelling polling when leaving Settings or when the DOM container is replaced.
apps/api/src/embedded-llm/downloader.ts Namespaces partial paths by download id and re-checks DB status to avoid pause/cancel races.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +43 to +47
* Final paths are *not* namespaced by user — the GGUF is content-
* addressable (we verify SHA-256 before rename), so two users on the
* same host downloading the same model land identical bytes at the
* same path. The race-prone bit is the in-flight `.partial`, which
* `partialPathFor()` namespaces by download row id below.
Comment on lines +58 to +60
*/
function partialPathFor(targetPath: string, downloadId: string): string {
return `${targetPath}.${downloadId}.partial`;
Comment on lines 197 to 203
inFlight.delete(downloadId);
}

const partial = partialPathFor(row.target_path);
const partial = partialPathFor(row.target_path, downloadId);
if (existsSync(partial)) {
try {
unlinkSync(partial);
jayzalowitz added a commit that referenced this pull request May 9, 2026
…ration

Three findings from Copilot's review of PR #249:

- **Install destroys good shared file on rename failure**: the previous
  "unlink target → renameSync" sequence had a window where, if rename
  threw or got cancelled mid-flight, the host was left with no GGUF
  at all — even though a perfectly good copy had been deleted moments
  before. Since the final path is now content-addressable (SHA-256
  verified before this point), an existing target IS the model. New
  install path: if target exists → just discard our partial, treat
  as installed; otherwise renameSync directly without unlinking; if
  the rename fails AND target now exists (race lost to another row),
  treat as installed.

- **Legacy non-namespaced partials orphaned by migration**: PR #247
  shipped with `<target>.partial`; PR #249 namespaces by row id.
  Existing pre-#249 partials would sit on disk indefinitely. Added
  unconditional cleanup at the top of runDownload — if a legacy
  `<target>.partial` exists, delete it before the row's namespaced
  partial is created.

- **Stale comment in cancelDownload**: docstring still mentioned
  `<target_path>.partial`. Updated to the namespaced format.

Build clean. 25 route tests still passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jayzalowitz

Copy link
Copy Markdown
Owner Author

Pushed f5e567e addressing all 3 findings:

  • Install idempotency (downloader.ts:417): replaced the unlink-then-rename dance with: if target exists → discard our partial; else renameSync directly; if the rename throws AND target now exists (race lost) → treat as installed. No more way to destroy a known-good shared file.

  • Legacy partial migration (downloader.ts:60): added unconditional cleanup at the top of runDownload() — any pre-PR-fix(#187 AC#2 r3): multi-user partial collision + pause race + zombie polling #249 <target>.partial on disk is deleted before this row's namespaced partial is created. Avoids orphan junk after the upgrade.

  • Stale cancelDownload comment (downloader.ts:203): docstring updated to mention the namespaced format and the fact that the shared final GGUF is left alone.

Build clean. 25 route tests still passing.

jayzalowitz and others added 2 commits May 9, 2026 14:57
… polling

Copilot's third-round review of PR #247 landed post-merge. Four
substantive findings, all addressed:

- **Multi-user partial-file collision**: two users on the same API
  host downloading the same model both wrote to
  <modelDir>/<modelId>.gguf.partial — concurrent streams could
  corrupt each other and one user's cancel could delete the other's
  partial. Final GGUF stays shared at <modelDir>/<modelId>.gguf
  (content-addressable, SHA-256 verified before rename), but partials
  now namespace by download row id.

- **Pause-on-pending was silently overridden**: pauseDownload() set
  DB to 'paused', but the already-kicked-off runDownload() would
  start anyway and overwrite back to 'downloading'. Same path for
  pending→cancelled. Fixed by re-fetching the row at the top of
  runDownload() and bailing early if status flipped to paused /
  cancelled / complete / failed between startDownload returning
  and the async runner picking up.

- **Polling continued after navigating away from Settings**: the 1s
  poll callback in embedded-llm-card.js had no termination tied to
  page navigation. Going to Approvals while a download was in
  flight kept hitting /api/embedded-llm/downloads/:id every second
  forever and held a reference to a detached
  #embedded-llm-card-target node. Now checks window.location.hash
  !== '#/settings' and document.getElementById(CARD_TARGET_ID) !==
  container at the top of each tick and stops itself.

- **ensureDirectory dead code**: already removed in the round-2 fix.

Build clean. 25 route tests still passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ration

Three findings from Copilot's review of PR #249:

- **Install destroys good shared file on rename failure**: the previous
  "unlink target → renameSync" sequence had a window where, if rename
  threw or got cancelled mid-flight, the host was left with no GGUF
  at all — even though a perfectly good copy had been deleted moments
  before. Since the final path is now content-addressable (SHA-256
  verified before this point), an existing target IS the model. New
  install path: if target exists → just discard our partial, treat
  as installed; otherwise renameSync directly without unlinking; if
  the rename fails AND target now exists (race lost to another row),
  treat as installed.

- **Legacy non-namespaced partials orphaned by migration**: PR #247
  shipped with `<target>.partial`; PR #249 namespaces by row id.
  Existing pre-#249 partials would sit on disk indefinitely. Added
  unconditional cleanup at the top of runDownload — if a legacy
  `<target>.partial` exists, delete it before the row's namespaced
  partial is created.

- **Stale comment in cancelDownload**: docstring still mentioned
  `<target_path>.partial`. Updated to the namespaced format.

Build clean. 25 route tests still passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jayzalowitz jayzalowitz force-pushed the fix/embedded-llm-r3-findings branch from f5e567e to c80d401 Compare May 9, 2026 18:57
@jayzalowitz jayzalowitz merged commit c6e93de into main May 11, 2026
8 checks passed
jayzalowitz added a commit that referenced this pull request May 11, 2026
…ELOG entry

The "Embedded LLM downloader: round-3 review fixes (#187 AC#2 follow-up)"
entry landed on main via PR #249 (commit c6e93de) after this branch
last rebased. The branch did not pull it in, so squash-merging would
have deleted it from main as a side effect.

Restored verbatim from origin/main:CHANGELOG.md so the squash diff is
purely additive. Caught by the /document-release cross-doc consistency
pass — exactly the kind of silent regression that motivated adding the
pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jayzalowitz added a commit that referenced this pull request May 11, 2026
…efault) (#250)

* feat(#197): gbrain memory backend + CRDB adapter + hybrid composer (default)

Promotes @skytwin/memory-gbrain from a CLI-shellout skeleton (PR #215) to a
real, in-process, CockroachDB-backed memory layer. Default MemoryPort for
new installs is now gbrain — vector embeddings + tsvector full-text search
fused via Reciprocal Rank Fusion. No separate Postgres process, no external
CLI install — gbrain runs against the SkyTwin DB stack directly.

Per user direction: gbrain is the default, mempalace is the second option,
and everything works against CRDB where possible.

Ships:
- 040-gbrain-memory.sql: brain_pages (FLOAT8[] embedding + TSVECTOR with
  inverted index), brain_entities, brain_triples, brain_episodes,
  brain_signals, brain_settings, brain_embedding_jobs (FOR UPDATE SKIP
  LOCKED queue).
- @skytwin/memory-gbrain-crdb-adapter (NEW): repository.ts (CRDB-backed +
  hybridSearch), in-memory-repository.ts (test-friendly mirror),
  embedding.ts (HashEmbeddingProvider deterministic fallback +
  OpenAiEmbeddingProvider for any /v1/embeddings endpoint), rrf.ts.
- @skytwin/memory-gbrain: EmbeddedGbrainMemoryPort with the full MemoryPort
  surface (semantic_search, code_aware_search, temporal_triples, episodic,
  graph_walk); searchCodeAware boost; hasExternalGbrainConfig() detection.
- @skytwin/memory-hybrid: diagnostics counters + capability-aware fallback.
- apps/api/src/memory-setup.ts: per-user backend factory (default 'gbrain';
  MEMORY_BACKEND env override; per-user brain_settings.backend wins).
- apps/api/src/routes/memory-config.ts: GET/POST /api/memory-config,
  /dismiss-notification, /diagnostics.
- apps/web memory-settings page with the "your twin got smarter" notice.
- docs/memory-swap.md: backends-at-a-glance, env knobs, rollback path.

Tests: 145+ new (49 CRDB adapter + 50 memory-gbrain + 9 hybrid diagnostics
+ 21 api memory-setup/routes + 6 DB-gated integration). Full suite: 70/70
turbo tasks pass.

Closes #197.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(#197 post-/review): brain_settings default = 'gbrain' (was 'hybrid')

/review caught a real bug: the migration's `brain_settings.backend DEFAULT 'hybrid'`
disagreed with the factory's `'gbrain'` default in `apps/api/src/memory-setup.ts`.

Failure mode: a fresh user (no brain_settings row) hitting
POST /api/memory-config/dismiss-notification triggered upsertSettings({hybrid_notification_dismissed:true}).
The COALESCE in INSERT defaulted backend → 'hybrid' even though the factory
considers a missing row to mean 'gbrain'. Result: dismissing the notification
silently flipped the user's backend.

Fixed in three places (must stay in sync — comment links the others):
- packages/db/src/migrations/040-gbrain-memory.sql: column DEFAULT 'gbrain'
- packages/memory-gbrain-crdb-adapter/src/repository.ts: upsertSettings COALESCE 'gbrain'
- packages/memory-gbrain-crdb-adapter/src/in-memory-repository.ts: upsertSettings fallback 'gbrain'

Plus a regression test on the in-memory store.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(#197): persona-driven E2E + realistic-corpus + robustness + migration

Adds 50+ tests that drive the gbrain memory layer with realistic data and
edge cases — addressing the user's "deeply test this with realish examples"
request. The point isn't unit coverage (we already had that); it's
"does the system actually build a profile when fed real-life data?"

New test files:
- realistic-corpus.ts fixture: ~30 labeled signals (Gmail, calendar, notes,
  code, chat) modelled after a real twin's first month, plus deterministic
  noise generators to scale to 500.
- realistic-retrieval.test.ts: R@5/P@5 floor with labeled relevance,
  hybrid-vs-text-only ablation, multi-user isolation under load (6 users,
  500 signals each, no cross-talk).
- persona-sam-patel.ts fixture: a 6-week storyline for a Series A founder
  (fundraise prep → VC meetings → term sheet → hiring loops → close →
  vacation), with tagged signals + entities + triples + episodes.
- persona-simulation.test.ts: drives the full storyline end-to-end and
  checks every load-bearing twin behaviour: entity recognition, graph
  walks (Mahesh → Anchor VC → Beacon Series A), triple filters,
  time-bounded episode lookup, semantic search on natural-language
  founder questions, profile summarisation, full export → import round
  trip with answer parity, and week-by-week incremental emergence.
- concurrent-worker.test.ts: 200 parallel recordSignal calls; failed
  embeddings get queued; worker drains the queue with FOR UPDATE SKIP
  LOCKED semantics; failed jobs exhaust retries and stop blocking.
- migration.test.ts: mempalace-flavoured export → gbrain importAll →
  imported content is searchable; idempotent re-import skips dupes;
  export → import → re-export histogram parity.
- robustness.test.ts: every degraded mode — embedding throws / times out /
  returns junk, queries empty / oversize / punctuation-only, mixed-dim
  vector corpus (model migration), pages with null embedding, OpenAI HTTP
  abort/timeout, multi-tenant safety under partial failure.
- memory-config-roundtrip.test.ts: real Express + real factory + real
  EmbeddedGbrainMemoryPort + real HybridMemoryPort end-to-end. Stubs only
  the @skytwin/db query layer. Verifies the dismiss-notification fix
  (default backend STAYS gbrain on a fresh user).

Test totals: 86 memory-gbrain (was 18) + 50 CRDB adapter + 19 hybrid +
26 api = 181 tests across the new memory subsystem. Full suite: 70/70
turbo tasks pass.

Honest about hash-trick limits: the persona test asserts ≥80% recall
across founder questions rather than 100%, because the deterministic
fallback embedding is intentionally weak. With OpenAI text-embedding-3-small
the same test suite runs at materially higher recall — but the floor here
catches retrieval-pipeline regressions without flaking on embedding quality.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(#197): full E2E fake-user → DecisionMaker — surfaces real safety finding

Constructs a complete fake user (Bob Patel, Series A SaaS founder) with
realistic preferences, behavioural patterns, traits, and trust tier
MODERATE_AUTONOMY. Wires the actual DecisionMaker + TwinService +
PolicyEvaluator against in-memory ports and feeds a realistic inbox
through the pipeline.

This is the user's "would the system actually do the email" check.

Surfaces a real finding: with the rule-based fallback CandidateGenerator,
the DecisionMaker auto-archives BOARD CHAIR and CFO emails because the
candidates are content-blind — `archive_email` is generated for every
EMAIL_TRIAGE situation regardless of sender. Bob's high-confidence
preference "board threads always require approval" doesn't gate the
candidate; it just informs scoring. Result at MODERATE_AUTONOMY:

  [AUTO-EXECUTE  ] archive_email — Stratechery newsletter        ← right
  [AUTO-EXECUTE  ] archive_email — Board chair: May meeting       ← WRONG
  [AUTO-EXECUTE  ] archive_email — CFO: Q2 forecast review        ← WRONG
  [NEEDS APPROVAL] accept_invite — Eng leadership 1:1             ← right
  [AUTO-EXECUTE  ] snooze_reminder — Adobe Creative Cloud         ← right
  [AUTO-EXECUTE  ] escalate_to_user — Friendly check-in           ← right

Production safeguards against this:
  1. OBSERVER / SUGGEST trust tier always gates everything (test asserts).
  2. A sender-aware `CandidateGenerator` reads sender + content and
     produces an irreversible `flag_for_manual_review` candidate for
     protected senders. The included `protectiveGenerator` demonstrates
     this — same shape as the LLM strategy that runs in production.

With the protective generator wired in:

  [AUTO-EXECUTE  ] archive_email           — Stratechery newsletter
  [NEEDS APPROVAL] flag_for_manual_review  — Board chair
  [NEEDS APPROVAL] flag_for_manual_review  — CFO

16 tests across two describe blocks. Also exports LabelInferencePort and
SenderLabelHint from @skytwin/decision-engine so tests can build the
custom Gmail-history-aware label hint port (#122).

Full suite: 70/70 turbo tasks pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197): SenderAwareCandidateGenerator + memory-enriched DecisionContext

Closes the safety gap surfaced by the fake-user E2E: the rule-based
candidate generator was content-blind, so at MODERATE_AUTONOMY the twin
auto-archived board chair / CFO / legal emails the same way it auto-archived
newsletters. This patch lands two production wirings:

1. New @skytwin/decision-engine export `SenderAwareCandidateGenerator` —
   a CandidateGenerator that wraps the rule-based generator with a
   pre-pass on `decision.rawData.from` and decision content. When the
   sender or subject matches a protected pattern (board/chair/cfo/coo/
   ceo/founder/partner/investor/legal/counsel/attorney/sec/audit/
   compliance/tax) or content mentions a protected topic
   (term sheet/wire transfer/signed/nda/equity/cap table/board deck/
   earnings/payroll), the generator SUPPRESSES the rule-based candidate
   set entirely and emits ONLY a `flag_for_manual_review` candidate
   (irreversible, CONFIDENCE: CONFIRMED). The built-in policy
   NO_IRREVERSIBLE_WITHOUT_APPROVAL gates this through the approval
   queue at every trust tier.

   Suppressing the base set (rather than just prepending the flag) is
   load-bearing: if archive_email is in the candidate list it scores
   higher than flag (lower risk because reversible) and would auto-execute
   anyway — the very bug we are fixing.

   Configurable via `protectedPattern` and `protectedSubjectPattern`
   constructor options; defaults match common corporate email surface area.

2. Wired SenderAwareCandidateGenerator into events.ts as the rule-based
   fallback. Used both:
     - directly as the DecisionMaker's CandidateGenerator when no LLM
       client is configured
     - as the inner RuleBasedCandidateGenerator that LLM strategies fall
       back to when LLM calls fail

   This means the safety improvement applies both to users without LLM
   keys (rule-based by default) and to LLM users when their LLM call
   fails — there is no path through events.ts that auto-archives a board
   email at MODERATE_AUTONOMY+.

3. Wired episodicMemories into DecisionContext. mempalaceRepository
   .getEpisodes is fetched in parallel with patterns/traits/temporal
   profile, mapped onto the EpisodicMemory shape, and passed to
   DecisionMaker.evaluate. The existing scoreCandidate boost
   (decision-maker.ts:1285+) consumes this field to weight candidates
   that match historically-positive past decisions. Closes the
   "twin's memory of past decisions affects current decisions" loop
   that was structurally present (the field existed) but unwired.

Tests:
- 12 unit tests for SenderAwareCandidateGenerator covering: protected
  senders (board/CFO/legal/investor), protected subjects (term sheet,
  wire transfer, cap table), routine email passthrough, non-email
  situations passthrough, custom pattern overrides.
- 3 integration tests for the events.ts wiring: board chair email
  selects flag_for_manual_review and does not auto-execute; routine
  newsletter selects archive/label; mempalaceRepository.getEpisodes
  is called with the right (userId, {domain, situationType, limit}).
- Updated existing events-routes.test.ts mock to include
  SenderAwareCandidateGenerator + emailLabelRepository + mempalaceRepository.

Full suite: 70/70 turbo tasks pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197): embedding-backfill worker — drains brain_embedding_jobs queue

Required when an external embedding provider (OpenAI, Ollama, vLLM) is
configured: the synchronous embed call inside recordSignal can fail
(rate limit, network, timeout). The write path persists the page row
unembedded and queues a job to brain_embedding_jobs. Without this worker
the queue never drains and search recall silently degrades — pages exist
in tsvector index but not the vector index, so RRF gives them only the
text-rank contribution.

What ships:

- apps/worker/src/jobs/embedding-backfill.ts:
  - `runEmbeddingBackfillJob({ batchSize, embedding })` — single-cycle
    drain. Leases up to batchSize jobs via SELECT FOR UPDATE SKIP LOCKED,
    embeds, persists, marks done. Failed jobs go through markJobFailed
    which auto-retries up to 3 times (the brain_embedding_jobs CHECK
    constraint flips status to 'failed' on the 4th attempt).
  - `getWorkerEmbeddingProvider()` — env-driven provider selection that
    mirrors `apps/api/src/memory-setup.ts` exactly. Same selection logic
    on both sides is load-bearing: if API embeds rows with OpenAI but
    worker embeds with hash-trick, cosine across them collapses.
  - Returns a structured `EmbeddingBackfillSummary` with attempted /
    succeeded / failed / pendingAfter counters that the worker loop
    logs on each non-empty cycle.

- apps/worker/src/index.ts: scheduled at 30s intervals alongside the
  existing metrics-rollup / changelog-poll / domain-extraction /
  federation-sync jobs. SKIP LOCKED makes it safe under multiple worker
  instances simultaneously.

Tests: 12 cases in apps/worker/src/__tests__/embedding-backfill.test.ts:
- happy path (drains queue, marks each done, respects batchSize)
- failure handling (embedding throws → markJobFailed; lease throws →
  cycle stops cleanly; markJobFailed itself throws → run continues)
- pendingAfter from DB and graceful pending-query failure
- env-driven provider choice (hash default, OpenAI when key set,
  OPENAI_EMBEDDING_MODEL override, fallback to OPENAI_API_KEY)

Full suite: 70/70 turbo tasks pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197): feedback → episode loop closes the learning circle

When a user approves or rejects an action via POST /api/approvals/:id/respond,
also persist an Episode into the memory layer. The next time a similar
decision is evaluated, mempalaceRepository.getEpisodes (already wired in
events.ts as part of this PR) pulls that episode into
DecisionContext.episodicMemories. DecisionMaker.calculateEpisodicBoost
(decision-maker.ts:1285+) consumes the episode's `actionTaken` +
`utilityScore` to tilt the candidate score:

  - approve → utility 0.9 → next time the same candidate appears it
    gets a positive boost on score, making auto-execute more likely.
  - reject → utility 0.0 → next time the same candidate's score gets
    no boost (and other candidates with non-zero utility from past
    approvals leapfrog it).

This closes the loop on the memory architecture: the twin's memory of
what the user *actually decided* feeds back into the next decision,
without any manual preference editing. The previous behaviour was that
approvals only updated the TwinService preferences (which influence
candidate confidence); episodes are a different signal — they record
the SPECIFIC action that won, not just the user's domain-level pref.

Implementation:

- apps/api/src/routes/approvals.ts: after `processFeedback` returns and
  before the (optional) execution branch, lookup the originating
  decision and call `mempalaceRepository.createEpisode` with the
  approval outcome. Wrapped in a try/catch — episode persistence is
  best-effort; never blocks the approval response on a memory-layer
  hiccup.

- The episode shape carries the full breadcrumb: userId, situationSummary
  (from the decision's interpreted summary, with a synthetic fallback),
  domain, situationType, actionTaken (from the candidate that the user
  approved/rejected), feedbackType, feedbackDetail (the user's reason),
  decisionId (so callers can join back), and utilityScore.

Tests: 4 cases in apps/api/src/__tests__/feedback-loop.test.ts:
  - approve path → createEpisode called with utility 0.9
  - reject path → createEpisode called with utility 0.0 + reason text
  - createEpisode throws → approval still returns 200 (best-effort)
  - decision row missing interpreted summary → synthetic fallback

Full suite: 70/70 turbo tasks pass; api 535 / worker 83 / memory-gbrain 86.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197): gbrain MemoryPort writes on ingest + corrections E2E

Two improvements that close the remaining loop in the gbrain memory layer:

1. **events.ts now writes inbound signals to gbrain.** Previously, every
   inbound event landed in the legacy `signals` / `decisions` tables but
   `brain_pages` stayed empty in production — meaning searchSemantic
   returned nothing, even with the gbrain backend explicitly selected.
   The new `recordSignalToMemory` helper calls
   `getMemoryPortForUser(userId).port.recordSignal(...)` on every ingest
   (fire-and-forget, so memory hiccups don't block the decision pipeline).

2. **approvals.ts now writes the resulting episode to gbrain too.** The
   prior commit added the legacy mempalaceRepository.createEpisode call;
   this layer adds a parallel `port.recordEpisode` so the gbrain backend's
   semantic index covers approved/rejected outcomes. Future similar
   signals' searchSemantic queries surface these episodes directly.

Tests:
- apps/api/src/__tests__/gbrain-write-on-events.test.ts: real Express
  round-trip with a stubbed @skytwin/db query layer; asserts that an
  inbound /api/events/ingest results in INSERT INTO brain_pages firing
  via the MemoryPort path (not just brain_signals).

- packages/decision-engine/src/__tests__/twin-learns-from-corrections.test.ts:
  5 cases proving DecisionMaker.calculateEpisodicBoost actually shifts
  outcomes when episodicMemories carry feedback:
    * baseline (no memory) selects deterministically
    * rejection episode does not improve the rejected action's rank
    * heavy rejections cannot improve the rejected action's rank
    * approval reinforcement keeps the approved winner
    * memory only matters when episode.actionTaken matches the candidate

  These tests run the REAL DecisionMaker.evaluate against in-memory
  TwinService + PolicyEvaluator ports — so the assertions exercise the
  exact production scoring code path, not a mock.

Full suite: 70/70 turbo tasks pass; api 536, decision-engine 109.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197): assistant chat uses MemoryPort.searchSemantic alongside legacy ILIKE

Wires the chat assistant's `MemoryContextProvider` to call the user's
selected MemoryPort (`getMemoryPortForUser`) in parallel with the legacy
`mempalaceRepository.searchEpisodes` ILIKE path, dedupes by summary, and
returns the merged top-K. This means chat answers automatically benefit
from gbrain's vector + tsvector RRF retrieval when the gbrain backend
has indexed pages — without losing the cold-install behavior where
mempalace's ILIKE returns recent episodes immediately.

Why both:
  - Hot install with gbrain: the semantic side surfaces vector-relevant
    pages the ILIKE keyword search would miss (e.g. "what did the CFO
    say?" returns CFO threads even when the user didn't type the literal
    word "CFO" in their question). Mempalace ILIKE then catches anything
    in the legacy table that hasn't been re-indexed yet.
  - Cold install: brain_pages is empty so semantic returns []. The
    mempalace path serves chat answers without a wait for the worker to
    backfill embeddings.
  - Both run in parallel; the slower of the two does not gate the chat
    response. Per-side errors are caller-swallowed.

The dedupe is by lowercased summary text — same episode often surfaces
from both sources, especially after `recordEpisode` has dual-written it.

Full suite: 70/70 turbo tasks pass; api 536 passing.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(#197): full-loop E2E — signal → approval → next signal carries the episode

Drives the entire memory-feedback loop through real Express route handlers:

  POST /api/events/ingest         (board chair email — sender-aware path)
  → flag_for_manual_review candidate, autoExecute=false, approval created

  POST /api/approvals/:id/respond (user rejects)
  → mempalaceRepository.createEpisode called with utility 0.0,
    feedback_type='reject', action_taken='flag_for_manual_review'
  → episodeStore now has the rejection row

  POST /api/events/ingest         (similar board email)
  → mempalaceRepository.getEpisodes called, returns the rejection
    episode → DecisionContext.episodicMemories carries it →
    DecisionMaker.calculateEpisodicBoost weighs it

This proves the wiring intact across all three route handlers and the
DB-backed memory store. The unit-level proof that boost actually shifts
scoring lives in
packages/decision-engine/src/__tests__/twin-learns-from-corrections.test.ts.

Full suite: 70/70 turbo tasks pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197): memory dashboard — show users what their twin remembers

Closes the "show me the goods" gap: until now, all the memory infrastructure
was invisible to users. The dashboard surface makes the value visible.

Ships:

1. **`GET /api/memory-config/dashboard`** — operator + user-facing view:
   - `index`: total pages, embedded pages, pending embedding jobs
   - `episodes.recent[]`: last 10 episodes (summary, action, feedback)
   - `episodes.feedbackCounts`: histogram (approve / reject / undo / pending)
   - `entities.total`, `topByRecency` (last 10), `topByType` (top 5)

   Each query is independently failure-handled via .catch(() => default),
   so a partial DB hiccup degrades gracefully rather than 500ing the
   whole dashboard.

2. **`apps/web/public/js/pages/memory-settings.js`** — new "What your
   twin remembers" card under the existing backend selector:
   - Recent decisions table with timestamps, action, feedback badge,
     and the situation summary.
   - Feedback count strip (✓ approved, ✗ rejected, etc.)
   - Top entities by recency + entity-type histogram.
   - All three dashboard / config / diagnostics endpoints fetched in
     parallel for snappy load.

Tests: 5 new cases in memory-config-routes.test.ts covering:
   - 400 on invalid userId
   - empty-state shape
   - feedback counts aggregated correctly
   - top entities sorted by recency, type histogram by count
   - partial DB failure → graceful degraded response

Full suite: 70/70 turbo tasks pass; api 541 / 558 (added 5).

This makes the gbrain memory layer's value legible to the user — they
can see entities accumulating, episodes recording approve/reject signals,
embeddings backfilling. The "twin learns" loop is now visible end-to-end.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(#197 post-/review): address Copilot findings + real MemPalaceMemoryPort

Addresses every finding from Copilot's PR #250 review plus the merge
conflict with main.

Blockers fixed:

1. **Migration PK types** — brain_pages/brain_entities/brain_triples/
   brain_episodes/brain_signals all had `id UUID PRIMARY KEY`. Production
   signal IDs are not UUIDs (e.g. `sig_gmail_abc123`); they're
   connector-assigned opaque strings. Forcing UUID would 500 every
   recordSignal in prod. Changed to `id STRING PRIMARY KEY DEFAULT
   gen_random_uuid()::STRING`. brain_settings.user_id stays UUID (real FK
   to users) and brain_embedding_jobs.id stays UUID (internal-only).

2. **StubMempalacePort replaced** — selecting `mempalace` (or relying
   on hybrid secondary) used to drop all legacy mempalace data on the
   floor. Now wires a real `MemPalaceMemoryPort` with a proper
   `MemPalaceRepos` adapter against `mempalaceRepository`. Covers
   knowledgeGraph (upsertEntity/getEntities/findEntity/addTriple/
   queryTriples/invalidateTriple) and episode (createEpisode/getEpisodes/
   getEpisodeByDecision/updateEpisode/searchEpisodes). Palace / closet /
   entityCode methods throw (they're never reached via MemoryPort, but
   throwing makes any future regression loud).

Other bugs Copilot flagged:

3. **`pendingEmbeddingJobs` per-user** — the dashboard was showing the
   global queue depth instead of the user's. Added optional `userId`
   parameter; defaults to global for the worker drain telemetry but
   the API route now passes userId so the dashboard reports the right
   number in multi-tenant installs.

4. **`candidatePoolSize` computed per-query** — docstring promised
   `max(k*4, 40)` but constructor hard-coded 40, truncating recall on
   large-K queries. Store the user override as a sentinel and apply the
   max-based default in `searchInternal`.

5. **In-memory `embeddingModel` parity** — when `embed()` rejected in the
   in-memory path, we still set `embeddingModel: this.embedding.model`,
   leaving pages with non-null model + null embedding. The CRDB path
   conditionally sets only when embedding succeeded. Matched both paths.

6. **`event.target.closest` guard** — memory-settings click delegator
   could throw on text-node clicks. Guard with
   `instanceof Element` per CLAUDE.md frontend event-handling discipline.

7. **`getEntitiesByType` routed through `resolveReadPort`** — was
   hard-wired to secondary, sending entity reads to the secondary even
   when the primary (gbrain) could serve them. Added a routing rule
   defaulting to primary; fallback still kicks in when capability is
   absent.

8. **docs/memory-swap.md capability table** — claimed `mempalace` had no
   semantic_search; the real `MemPalaceMemoryPort` declares it (backed
   by ILIKE). Updated to show `ILIKE` in the cell + a note explaining
   when to prefer each backend.

Plus a rebase onto main (#248 first-run dashboard merged in between).
The conflict was in CHANGELOG.md — both entries are now stacked under
unreleased.

Full suite: 70/70 turbo tasks pass; api 542, decision-engine 109,
memory-gbrain 86.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(#197): /document-release sync — arch-philosophy, cockroach-architecture, technical-spec

Post-ship documentation update for the gbrain memory backend ship.

- docs/architecture-philosophy.md: memory port row updated to reflect
  gbrain (default, CRDB-native) + mempalace (selectable fallback). The
  "interim" framing was obsolete — gbrain is the default.
- docs/cockroach-architecture.md: added the 7 brain_* tables to the
  schema reference. Documented the STRING-PK choice (production signal
  ids aren't UUIDs; the table reflects that contract).
- docs/technical-spec.md: package layout shows the 5 new memory-* packages.
  Build dependency chain updated to include them in topological order.

Full suite: 70/70 turbo tasks pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(#197 polish): SSE live memory dashboard + Ollama recipe + CRDB harness

- SSE: events.ts emits `memory:page-indexed` after a successful
  `recordSignalToMemory`; approvals.ts emits `memory:episode-recorded`
  after `mempalaceRepository.createEpisode`. Web sse-client.js subscribes
  and dispatches `sse:memory:*` CustomEvents; memory-settings.js wires a
  module-singleton listener (1s debounce) that re-renders the dashboard
  without polling.
- Ollama recipe in docs/memory-swap.md — zero-cloud local embeddings via
  the OpenAI-compatible /v1/embeddings endpoint (nomic-embed-text default).
- CRDB integration harness: packages/memory-gbrain-crdb-adapter ships
  `scripts/run-crdb-integration.sh` (Docker-based) and a `test:crdb`
  package script. Spins a hermetic CRDB, applies migration 040, seeds a
  test user, and runs the 6 DB-gated integration tests.
- Tests: feedback-loop.test.ts now mocks `createEpisode` resolved value
  so the SSE emit path is reachable, plus an assertion that
  `memory:episode-recorded` is emitted. gbrain-write-on-events.test.ts
  gains a parallel assertion for `memory:page-indexed`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(#197): align memory-swap prose with corrected capability table

The bullet on line 24-25 still said mempalace "declares no semantic_search
capability" — that was true at the start of #197 but
MemPalaceMemoryPort.capabilities() now returns 'semantic_search'
(ILIKE-backed). The table below already reflected this; the prose did not.
Tightens the wording to match.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(#197 post-/review): harden CRDB integration harness

Three issues found by the /review pass on commits aa78f13..HEAD:

1. Migration was applied twice. The first apply ran against `skytwin_test`
   *before* the inlined `users` table existed there, so the brain_* FK
   references failed silently (psql -f exits 0 on per-statement errors
   without ON_ERROR_STOP). The second apply then worked because the
   tables already partially existed. Reordered to create-db →
   create-users-in-test-db → apply-migration-once.
2. Added `-v ON_ERROR_STOP=on` to every psql invocation so any future
   schema regression fails the harness loudly instead of being masked
   by `>/dev/null`.
3. The cockroach-ready wait loop completed silently after 30s even on
   total startup failure; now sets a `ready` flag and bails with the
   container's last 20 log lines if the DB never accepts connections.
   Also tightened TEST_USER_ID parsing: `-A` unaligned output + tr against
   `[:space:]` instead of just ` \n`, plus an empty-result check.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(#197 post-/document-release): restore Embedded LLM round-3 CHANGELOG entry

The "Embedded LLM downloader: round-3 review fixes (#187 AC#2 follow-up)"
entry landed on main via PR #249 (commit c6e93de) after this branch
last rebased. The branch did not pull it in, so squash-merging would
have deleted it from main as a side effect.

Restored verbatim from origin/main:CHANGELOG.md so the squash diff is
purely additive. Caught by the /document-release cross-doc consistency
pass — exactly the kind of silent regression that motivated adding the
pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(#197 post-/review r2): address Copilot round-2 findings

Five findings from Copilot's re-review on commit 50f297c:

1. **brain_embedding_jobs.page_id FK type mismatch (HIGH)** — column was
   declared `UUID` but `brain_pages.id` is `STRING`. CRDB would reject
   the FK at apply time, or accept it and reject any insert with a
   non-UUID page id (which is most signal-derived pages —
   `sig_gmail_abc123` etc.). Fixed in migration 040 to `page_id STRING`
   matching `brain_pages.id` exactly.

2. **assistant.ts dedupe comment was wrong** — outer comment said
   "dedupe by (summary, occurredAt)" but the implementation uses just
   summary. Updated the comment to reflect the actual logic and
   explain WHY occurredAt can't be in the key (gbrain hits never carry
   one; including it would defeat cross-source dedupe entirely).

3. **hybrid-port.ts resolveReadPort docstring drift** — claimed a
   3-step priority (override → routing table → capability fallback)
   but the implementation collapses steps 1+2 (the override IS the
   routing table) and step 3 is the same as the capability fallback
   inside step 2. Rewrote the docstring to match the actual logic.

4. **.claude/scheduled_tasks.lock leaked into the PR** — runtime
   session lock metadata (sessionId / pid / ts) was getting committed
   on every session. `git rm --cached` to untrack, added to
   .gitignore so future sessions don't re-add it. This is technically
   a removal-from-main but is the right long-term shape.

5. **BrainPageRow.embedding type / parsePageRow runtime mismatch** —
   types.ts declared `number[] | null` but parsePageRow defensively
   checks `typeof === 'string'` for the pg array-literal case, which
   strict mode flagged as always-false. Introduced a `RawBrainPageRow`
   type with `embedding: number[] | string | null` for the raw DB
   shape, and parsePageRow narrows it to `BrainPageRow` (with
   `number[] | null`) for downstream consumers. No behaviour change.

Verified: pnpm --filter @skytwin/api test → 544 pass; memory-* tests
→ 155 pass. No regressions.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants