feat: [ENG-2157] add RuntimeSignalStore with atomic update primitive (2/6) by danhdoan · Pull Request #448 · campfirein/byterover-cli

danhdoan · 2026-04-18T12:47:54Z

Summary

Problem: Per-machine ranking signals (importance, recency, maturity, accessCount, updateCount) are currently stored in context-tree markdown frontmatter. Every brv query bumps them and rewrites the markdown, dirtying brv vc status and causing merge conflicts when teammates share context trees.
Why it matters: The fix is a 6-commit series. This PR is step 2 of 6 — it introduces the sidecar store as reachable infrastructure so commits 3-6 can redirect writes and reads onto it. On its own, nothing changes for the user.
What changed: New IRuntimeSignalStore interface, thin RuntimeSignalStore facade over the existing IKeyStorage (composite keys ["signals", relPath]), 26 unit tests, wiring in service-initializer.ts, runtimeSignalStore added to CipherAgentServices, SemanticFrontmatter type added to the schema file.
What did NOT change (scope boundary): No consumer calls the store. No mutation site redirected. No read path redirected. No YAML emitter change. No cloud sync exclusion. All of those land in commits 3-6.

Type of change

Scope (select all touched areas)

Linked issues

Closes #
Related: runtime-signals sidecar series — ENG-2133 (commit 1 of 6 landed in PR feat: [ENG-2133] add RuntimeSignals Zod schema and types (1/6) #447)

Root cause (bug fixes only, otherwise write `N/A`)

Root cause: N/A
Why this was not caught earlier: N/A

Test plan

Coverage added:
- Unit test
- Integration test
- Manual verification only
Test file(s):
- test/unit/agent/knowledge/runtime-signal-store.test.ts — 26 tests
Key scenario(s) covered:
- CRUD round-trip via set/get
- get returns defaults for missing path
- get returns defaults and logs a warning for corrupt stored data
- get fills partial records with per-field defaults (forward-compat)
- update seeds defaults when no record exists and applies the updater
- update rejects updater output that violates the schema
- 20 parallel update calls on the same path all land with no lost bumps (atomic read-modify-write)
- batchUpdate applies all updaters; serializes concurrent bumps on shared paths across batches
- getMany returns an entry for every requested path, missing ones default; does not read entries outside the requested set
- list returns all entries, falls back to defaults for corrupt, ignores keys outside the signals namespace
- delete removes an entry so subsequent get returns defaults; no-op for missing paths
- Path encoding edge cases: leading, trailing, and consecutive slashes all normalize to the same entry
- Deeply-nested paths round-trip correctly

User-visible changes

None. This commit adds a module reachable via CipherAgentServices.runtimeSignalStore but no consumer calls it yet.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording

Concurrent-atomicity test output:

Full store test output: 26 passing (14ms)

Full repo suite: 6459 passing, 0 failing (no regression — +5 tests from commit 1's 6454).

Checklist

Tests added or updated and passing (npm test)
Lint passes (npm run lint) — 0 errors, 201 pre-existing warnings (none new)
Type check passes (npm run typecheck)
Build succeeds (npm run build)
Commits follow Conventional Commits format
Documentation updated (if applicable) — N/A, internal change
No breaking changes
Branch is up to date with main

Risks and mitigations

Risk: Cross-process atomicity is not provided. The daemon (flushAccessHits) and CLI (curate) can race on the same signal record; IKeyStorage.update's RWLock is in-process only. Under concurrent writes, one bump may be lost.
- Mitigation: Acceptable for ranking signals — losing one access-hit bump has no correctness impact, only a small ranking drift that the next session self-heals. Documented explicitly in the interface doc and implementation comments. Not suitable for data where strict consistency is required; noted in the backlog for future upgrade to file-level flock if drift becomes measurable.
Risk: Store layer does not enforce the importance ↔ maturity hysteresis invariant. Callers bumping importance must recompute maturity via determineTier themselves; the store accepts any schema-valid combination.
- Mitigation: TODO inlined in the implementation file. Audit will happen as part of commit 3 when the first real callers are wired.
Risk: Orphan entries accumulate. When a markdown file is deleted outside curate (manual rm), its sidecar entry remains forever.
- Mitigation: No production impact yet — list() is only called for admin/diagnostic use, and getMany is used for ranking. Pruning mechanism captured in the backlog with a clear trigger.
Risk: Mock store in test/helpers/mock-factories.ts skips Zod validation — consumer tests using the mock won't catch bugs that rely on validation behaviour.
- Mitigation: The mock is a convenience; consumer tests needing real behaviour can instantiate RuntimeSignalStore with FileKeyStorage({inMemory: true}). Captured in the backlog.

(2/6) Second task in the series that moves per-machine ranking signals out of context-tree markdown frontmatter into a sidecar store. Adds IRuntimeSignalStore interface and RuntimeSignalStore implementation, a thin facade over IKeyStorage using composite keys ["signals", ...segments]. Atomicity within a single process comes from IKeyStorage.update's per-key RWLock — no external mutex needed. Cross-process writes have a narrow lost-update window, acceptable for ranking signals (documented in the interface). The store exposes six methods: - get / getMany / list — bulk and single reads - set — full replace (used for seeding, merge, restore) - update — atomic read-modify-write via updater callback - delete — remove an entry - batchUpdate — parallel update across many paths Wired into service-initializer.ts and exposed on CipherAgentServices. No consumer calls the store yet — dual-write lands in commit 3. Mock factory updated for test compatibility. Design decisions worth noting: - Updater callback (not delta-merge) supports both SET and INCREMENT semantics safely inside the atomic critical section. - Path encoding splits relPath on '/' so FileKeyStorage's segment validation accepts it. Path normalization variants (leading/trailing/ consecutive slashes) collapse to the same key. - Zod validation on both read and write; corrupt stored records fall back to defaults with a logged warning. - SemanticFrontmatter type introduced here (deferred from 1/6). Tests: 26 store tests including concurrent-atomicity (20 parallel updates on same path), corrupt-data handling, namespace isolation, path-encoding edge cases, and forward-compat with partial records. Full suite: 6459 passing, 0 failing.

github-actions · 2026-04-18T12:48:09Z

Claude finished @danhdoan's task in 3m 15s —— View job

PR Review: feat: [ENG-2157] RuntimeSignalStore (2/6)

Tasks

Gather context and understand the request
Read changed files
Analyze for correctness, security, and code quality
Post inline comments and overall review

Overall Assessment: 4 / 5

This is clean, well-scoped infrastructure work. The design is sound, the concurrency story is honest and well-documented, scope is correctly constrained, and the 26-test suite covers the important scenarios including the parallel-update atomicity case. The few items below are all minor.

What's good

Interface design — IRuntimeSignalStore is minimal and consumer-driven. The split between set (full replace), update (atomic RMW), and batchUpdate (parallel flush) maps exactly to the three real call sites expected in commits 3–6.
validateOrDefault pattern — silently defaults on undefined, warns on corrupt, never throws. The two-path distinction gives the right diagnostics without burdening callers.
Zod validation at both write boundaries — set and update both re-validate before storage, so a buggy caller can't land out-of-range values on disk.
Concurrency docs — the interface-level comment on cross-process limits is exactly the right level of honesty: it says what the lock covers, what it doesn't, and why losing a bump is acceptable here but wouldn't be for other data.
Key encoding — signalKey filtering empty segments makes path normalization transparent and the tests cover the edge cases (leading slash, trailing slash, consecutive slashes).
Mock implementation — createMockRuntimeSignalStore is a real in-memory store rather than a stub bag. This gives consumer tests meaningful behavior without Zod overhead.

Issues / suggestions

#	Severity	Location	Summary
1	nitpick	`runtime-signal-store.ts:82`	`keyStorage.update<RuntimeSignals>` generic type is misleading — disk value can be corrupt. `<unknown>` would be honest; existing comment explains the handling correctly.
2	question	`runtime-signals-schema.ts`	`DEFAULT_RECENCY = 1` starts new files at max recency. Correct if recency decays from 1, but worth a doc note confirming that "new = maximally fresh" is intentional.
3	nitpick	`service-initializer.ts:309–312`	The `runtimeSignalStore` init block lacks a step number (every other block has `// N.`), and the JSDoc steps list at the top doesn't include it.
4	nitpick	`runtime-signal-store.test.ts:1`	Test is filed under `test/unit/agent/knowledge/` but the module lives in `src/server/infra/context-tree/`. Doesn't affect correctness; `test/unit/server/context-tree/` would be more discoverable.
5	nitpick	`runtime-signal-store.test.ts:23`	`expectRejected` hand-rolls try/catch — `assert.rejects(promise)` (Node built-in) or `chai-as-promised` avoids the manual boolean dance.

Security

No concerns. The store doesn't touch the filesystem directly (delegates to IKeyStorage), user input never reaches signalKey without being a relative path that the existing pipeline has already validated, and the Zod schema enforces range bounds at every write.

danhdoan requested review from RyanNg1403, bao-byterover, cuongdo-byterover, leehpham and ngduyanhece as code owners April 18, 2026 12:47