Skip to content

fix: remove maturity from synthesis file frontmatter, seed sidecar instead#458

Merged
danhdoan merged 2 commits intoproj/runtime-signalfrom
fix/synthesize-maturity-sidecar
Apr 19, 2026
Merged

fix: remove maturity from synthesis file frontmatter, seed sidecar instead#458
danhdoan merged 2 commits intoproj/runtime-signalfrom
fix/synthesize-maturity-sidecar

Conversation

@RyanNg1403
Copy link
Copy Markdown
Collaborator

Summary

  • synthesize.ts was writing maturity: 'draft' into markdown frontmatter of newly created synthesis files — missed during the runtime-signals migration (ENG-2160)
  • Removed maturity from synthesis frontmatter, added sidecar seeding via createDefaultRuntimeSignals() after file write (best-effort, fail-open)
  • Threaded runtimeSignalStore through SynthesizeDeps and DreamExecutor.runOperations

Type of change

  • Bug fix (synthesis files dirtied brv vc status with ranking field in markdown)

Scope

  • Server / Daemon — synthesize.ts, dream-executor.ts

Root cause

synthesize.ts:246 had maturity: 'draft' hardcoded in the frontmatter object. Every other write path (curate ADD/UPDATE/MERGE, consolidate, prune/archive) was migrated to write ranking signals to the sidecar only. Synthesize was missed.

Test plan

  • Updated existing test: expect(content).to.include('maturity: draft')expect(content).to.not.include('maturity:')
  • New test: synthesis file has no ranking fields in markdown (importance, recency, maturity, accessCount, updateCount)
  • New test: sidecar seeded with default signals after write
  • New test: multi-candidate run seeds sidecar for each file
  • New test: succeeds without runtimeSignalStore (graceful no-op)
  • Interactive verification: brv dream --force created 4 synthesis files, all with clean frontmatter (no maturity)

Evidence

Synthesis file frontmatter before fix:

---
confidence: 0.9
maturity: draft          # <-- leaked ranking field
sources: [...]
synthesized_at: '...'
type: synthesis
---

Synthesis file frontmatter after fix:

---
confidence: 0.9
sources: [...]
synthesized_at: '...'
type: synthesis
---

Full suite: 6505 passing / 0 failing. Lint: 0 errors. Types: clean.

Checklist

  • Tests added or updated and passing (npm test)
  • Lint passes (npm run lint)
  • Type check passes (npm run typecheck)
  • Build succeeds (npm run build)
  • Commits follow Conventional Commits format
  • No breaking changes
  • Branch is up to date with proj/runtime-signal

…stead

synthesize.ts was writing `maturity: 'draft'` into markdown frontmatter
of newly created synthesis files. This was missed during the runtime-signals
migration (ENG-2160) which moved all five ranking fields to the sidecar.

- Remove `maturity` from the synthesis frontmatter object
- Add optional `runtimeSignalStore` to `SynthesizeDeps`
- Seed sidecar with `createDefaultRuntimeSignals()` after file write
- Thread `runtimeSignalStore` from `DreamExecutor.runOperations` to synthesize
- Update existing test assertion from `include('maturity: draft')` to `not.include`
- Add 4 new sidecar-specific tests (no ranking in markdown, sidecar seeded,
  multi-candidate seeding, graceful no-store)
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 19, 2026

Claude finished @RyanNg1403's task in 2m 21s —— View job


PR Review: fix/synthesize-maturity-sidecar

  • Gather context and read changed files
  • Review synthesize.ts changes
  • Review dream-executor.ts changes
  • Review synthesize.test.ts changes
  • Post inline comments for specific issues
  • Post overall summary with score

Summary

Score: 4 / 5

The fix is correct and minimal. Removing maturity: 'draft' from synthesis file frontmatter and seeding the sidecar after write is exactly the right migration — consistent with every other write path that was already updated (curate ADD/UPDATE/MERGE, consolidate, prune/archive). dream-executor.ts threads the dependency cleanly. The path traversal guard is untouched, and the fail-open try/catch around runtimeSignalStore.set() is the right pattern.

Two issues worth addressing in the tests:

1. False-positive sidecar-seeding tests (issue)
The "seeds sidecar with default signals" and "seeds sidecar for each file" tests assert signals.importance === 50 and signals.maturity === 'draft', but createMockRuntimeSignalStore().get() returns createDefaultRuntimeSignals() for any missing path — meaning these assertions pass even if set() was never called. A regression that removed the if (runtimeSignalStore) block would go undetected. Spy on set with .callThrough() to make the assertions load-bearing. See inline comment at line 529.

2. Missing fail-open test (suggestion)
The try/catch around runtimeSignalStore.set() is the core behavioral guarantee of the sidecar-seed path — a store failure must not fail the synthesis write. That guarantee currently has no test. Adding a stub that rejects on set and asserting results.length === 1 would lock in this contract. See inline comment at line 565.

Minor nit: warnSidecarFailure(undefined, ...) silently no-ops — the synthesize path gets zero observability when sidecar seeding fails, unlike curate/prune which pass real loggers. Low-priority, but worth considering if operators need to monitor sidecar health. See inline comment at line 283.

The implementation changes themselves are clean and correct — happy to approve once the test reliability issue is addressed.

Comment thread test/unit/infra/dream/operations/synthesize.test.ts
Comment thread src/server/infra/dream/operations/synthesize.ts Outdated
Comment thread test/unit/infra/dream/operations/synthesize.test.ts
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7deea768e4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/server/infra/dream/operations/synthesize.ts Outdated
…eading

- Spy on signalStore.set with callThrough to prove set() was called (not
  just asserting defaults that match missing-path fallback)
- Add fail-open test: store.set() throws, synthesis file still created
- Thread ILogger through SynthesizeDeps so warnSidecarFailure produces
  observable output instead of silently no-opping
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 19, 2026

Claude finished @RyanNg1403's task in 1m 34s —— View job


PR Review: fix/synthesize-maturity-sidecar (re-review)

  • Gather context and read changed files
  • Review updated synthesize.ts changes
  • Review updated synthesize.test.ts changes
  • Post summary with score

Summary

Score: 5 / 5

All three issues from the previous review have been addressed cleanly.

Test reliability (issue → resolved)
Both sidecar-seeding tests now spy on set with .callThrough() before calling synthesize, then assert setSpy.calledOnce / setSpy.calledTwice and verify the exact path argument. The assertions are no longer trivially satisfied by the mock's get() ?? createDefaultRuntimeSignals() fallback — a regression that skips the set() call will now fail.

Fail-open test (suggestion → resolved)
The new 'creates file even when sidecar store.set throws (fail-open)' test stubs brokenStore.set to reject, then asserts both results.length === 1 and that the file content contains type: synthesis. The dual assertion is important: it would catch the regression where atomicWrite drifts inside the try-block (file wouldn't be written), not just the case where the result array is empty.

Logger threading (nit → resolved as designed)
logger?: ILogger was added to SynthesizeDeps, making sidecar-failure observability available when callers supply one. The executor call site doesn't pass a logger — but that's consistent with how prune is also called from dream-executor.ts (no logger threaded there either). The nit was about consistency, and synthesize is now on the same footing as the rest of the dream pipeline. Making the full executor→logger wiring would require adding logger to DreamExecutorDeps, which is a separate concern beyond the scope of this fix.

Implementation remains correct and minimal: maturity: 'draft' is removed from frontmatter, sidecar seeding is best-effort with a proper fail-open guard, runtimeSignalStore is threaded through SynthesizeDeps and the executor, and the JSDoc comments on the new fields are clear.

@danhdoan danhdoan merged commit 0a9566d into proj/runtime-signal Apr 19, 2026
6 checks passed
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