Chore/curation sync main 3 10 1#600
Conversation
…ivation (#549) * fix: [ENG-2484] validate openai-compatible URL and defer provider activation Removes the `llama3` placeholder default that silently corrupted config when users couldn't reach the model picker, validates the user-provided base URL against `/models` before persisting any state, and stops flipping the active provider during connect when no active model would be set — that intermediate state was unmounting the slash command's flow via the welcome view, hanging the REPL after the user picked a model. * refactor: [ENG-2484] address review-agent feedback on openai-compatible setup Falls back to stored baseUrl/apiKey on partial reconfigure so an endpoint that requires auth no longer 401s when the user keeps the same key. Rejects openai-compatible connects that have no effective baseUrl (request + stored) instead of silently storing a useless config entry. Short-circuits the existingActiveModel lookup when the registry already supplies a defaultModel. Tightens the TUI auto-route to openai-compatible specifically, clears selectedProvider on connect failure for symmetry with sibling error paths, and adds a model:setActive test pinning the activate-on-pick chain that the deferred activation design depends on. * test: [ENG-2484] pin activeModel field on provider:list DTO The TUI provider-flow now branches on `provider.activeModel` to decide whether to route a connected openai-compatible into the model picker. Add a list-handler test that pins the DTO carries `activeModel: undefined` with `isConnected: true` for that half-configured shape, so a future serialization or DTO-shape change cannot silently re-break the routing. --------- Co-authored-by: bao-byterover <bao@byterover.dev>
* feat: [ENG-1593] add brv review --disable / --enable toggle Adds a project-scoped HITL review toggle. When disabled: - `brv curate` no longer prints the "X operations require review" prompt - `--detach` curate-log entries omit the per-op review marker - `brv dream` no longer surfaces its own needsReview ops as pending reviews - `brv review pending` lists nothing new until re-enabled - Review backups (the rollback substrate) are not written Single source of truth: the daemon snapshots `reviewDisabled` once at the task-create boundary and forwards it through TaskInfo (daemon-side, used by CurateLogHandler) and TaskExecuteSchema (wire format, opened as an AsyncLocalStorage scope around the agent-process task body so any descendant async callsite — direct curate-tool invocation OR sandbox `tools.curate(...)` via CurateService — observes the same value even if the user toggles mid-task). Same propagation pattern as CurateResultCollector. Mid-task toggle race coverage: - Started disabled, toggle enable → daemon snapshot held disabled → no orphan backup, no pending entry - Started enabled, toggle disable → daemon snapshot held enabled → backup created, pending entry created - Verified across 24 curates + 6 dreams in interactive testing. Top-level `brv review` command (and the new `--enable` / `--disable` flags) routes through the daemon transport via two new events (`review:getDisabled`, `review:setDisabled`) — same wiring pattern as `brv review pending` / `approve` / `reject`. Existing review subcommands are untouched. * refactor: [ENG-1593] address review-agent feedback on idle-dream stamping and curate-tool style Fix #1 (blocker — brv-server.ts): the idle-trigger dream-dispatch path called `agentPool.submitTask(...)` directly, bypassing `TaskRouter.handleTaskCreate` and therefore the daemon's reviewDisabled snapshot. Without the stamp, `task.reviewDisabled` arrived undefined in the agent process, the AsyncLocalStorage scope was never opened, and `DreamExecutor` defaulted to review-enabled — so idle dreams ignored the project's `reviewDisabled` setting regardless of `brv review --disable`. Hoisted the `ProjectConfigStore` declaration up the file and added a shared `resolveReviewDisabled` resolver that both the idle-dream dispatch and the existing `TransportHandlers.isReviewDisabled` reuse. The two paths now stamp identical values, so review semantics are consistent across CLI dispatch and idle-trigger dispatch. Fix #2 (curate-tool.ts): replaced `JSON.parse(raw) as {reviewDisabled?: unknown}` in `isReviewDisabledForBrvDir` with a proper `hasReviewDisabledField` type guard. CLAUDE.md forbids `as Type` assertions; the new guard narrows safely without an escape-hatch cast. Fix #3 (curate-tool.ts): dropped redundant braces around the single-expression arrow body in the `onAfterWrite` definition. The union-type ordering nit (`undefined | WriteCallback` vs reverse) was left as-is — `perfectionist/sort-union-types` requires the existing order, so the reviewer's suggested swap would actually have introduced a lint error. * refactor: [ENG-1593] address review-agent feedback on snapshot fail-open P2 fix (task-router.ts): `snapshotReviewDisabled` returned `undefined` when the resolver threw, which re-introduced the exact daemon/agent divergence the snapshot is meant to prevent — daemon stamps no field → `CurateLogHandler` treats it as enabled (`?? false`), while the agent process opens no ALS scope and may observe a different value from `.brv/config.json` in the curate-tool fallback. Now stamps an explicit `false` (fail-open: review enabled) on resolver throw, so both sides observe a single concrete value. Aligns with the agent-side `isReviewDisabledForBrvDir` which also fails open. `undefined` is still returned when no resolver is wired or no projectPath was resolved — those are legitimate "not configured" cases that downstream consumers handle correctly. Adds a regression test pinning the new behavior in `reviewDisabled stamping` describe block. * refactor: [ENG-1593] address review-agent feedback on redundant === true comparison Drop `=== true` on `flags.disable` in `brv review`'s run handler. `Flags.boolean` with `default: false` already guarantees `flags.disable` is a `boolean`, so the explicit comparison was a readability nit with no functional impact. * fix: [ENG-1593] suppress dream "flagged for review" line when review disabled When `reviewDisabled=true`, dream still computed `flaggedForReview` from LLM `op.needsReview` tags and printed `"N operations flagged for review"` even though the dual-write at `createReviewEntries` correctly dropped those ops. Users would see the message and find nothing in `brv review pending`. Thread `reviewDisabled` into `formatResult` and gate the line on it. Functional behavior unchanged; queue is still source of truth. --------- Co-authored-by: Danh Doan <danhdoancv@gmail.com>
* fix: [ENG-2513] bump byterover-packages to 1.0.2 * feat: [ENG-2513] bundle package to avoid duplication * feat: [ENG-2513] bundle package to avoid duplication
* fix: [ENG-2516] unify statusMatrix tuple classification across vc consumers
* test: [ENG-2516] add tuple-reachability fuzz + pull error format-lock
* fix: [ENG-2516] derive classifier from encoding rules + extract overwrite messages
Replace the whitelist tuple enumeration in classifyTuple with rule-based
stagedDiff/unstagedDiff/untracked helpers that are total over the
statusMatrix encoding's value range. Out-of-range columns still throw
GitError so a real encoding shift surfaces loudly.
Surfaces two silent-drops the previous whitelist missed:
- [1,1,3]: now reports both staged + unstaged modifications, matching
native git (was reporting only the staged side after the prior fix)
- [1,0,2]: now reports both staged-modified + unstaged-deleted (was
dropping the staged side entirely; same bug class as [1,1,3])
Extract formatOverwriteMessage into git-error-messages.ts, used by all
five overwrite paths (checkout / merge / pull, with and without file
lists). Drop the "stash them" trailer because brv has no stash command;
trailer now reads "commit or discard". The "would be overwritten" anchor
is preserved so vc-handler's UNCOMMITTED_CHANGES mapping still works.
Cross-property invariants in the unit suite now run exhaustively over
the full (h, w, s) cartesian product (24 tuples x 6 invariants = 144
assertions) so any future algorithmic regression is caught structurally.
Drops the brittle source-scan format-lock test in favor of helper unit
tests against the extracted formatter.
* fix: [ENG-2516] address PR review feedback on tuple classifier tests
- Switch cross-property invariant test to classifyTuple().dirty so it
tracks pull()'s live filter instead of the removed raw formula
- Add expectedTuple identity assertions to every named-tuple scenario
in the reachability fuzz, locking scenario names to actual matrix rows
- Fix [1,2,1] and [1,2,3] recipes that silently produced [1,1,1] and
[1,2,2] due to stat-cache aliasing on same-size payloads (utimes bump,
same workaround the existing [1,1,3] scenario uses)
- Remove misnamed [1,0,2] scenario; the tuple is unreachable by the
encoding (w=0 forces s in {0,3}, never 2) and the recipe duplicated [1,0,3]
- Add comments noting the (s=2,w=0) and [0,0,0] unreachable edges in
status-row-classifier
…it (#582) * feat: [ENG-2522] add PostWorkRegistry for detached post-curate work Foundation for moving Phase 4 (summary regeneration, manifest rebuild, background drain) off the synchronous task-completion path. Empirical measurement showed Phase 4 dominates 98% of post-curate latency (~18s of an 18.2s silent wait); detaching it is the largest user- visible win. The registry is the plumbing — actual detachment lands in subsequent commits. Provides three guarantees: - per-project mutex so two concurrent curates' Phase 4 cannot race on snapshot pre-state or `_index.md` writes; - cross-project concurrency so unrelated workspaces are not serialised; - bounded drain(timeoutMs) for the daemon shutdown handler — gives in-flight thunks a grace window before exit so propagateStaleness cannot truncate a partially-written `_index.md` on SIGTERM. Errors inside thunks are isolated and do not block subsequent work, matching the existing fail-open behaviour of post-curate steps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: [ENG-2522] split curate-executor into runAgentBody + finalize Prepare for detached Phase 4. Adds runAgentBody returning the agent response immediately along with a finalize thunk covering snapshot diff, summary regeneration, manifest rebuild, dream counter, background drain, and task-session cleanup. Backwards compatibility: executeWithAgent now delegates to runAgentBody and awaits finalize before returning, so existing callers (and the ENG-2100 regression tests) see unchanged synchronous behaviour. agent-process will switch to the detached path next. If the agent body throws, the task session is cleaned up before the error propagates and no finalize is returned — eliminates the leaky- session class of bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: [ENG-2522] split folder-pack-executor into runAgentBody + finalize Same shape as the curate-executor split. runAgentBody returns the agent response immediately along with a finalize thunk covering snapshot diff, summary regeneration, manifest rebuild, and background drain. executeWithAgent stays as a backwards-compat wrapper that awaits finalize before returning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: [ENG-2522] detach post-curate Phase 4 from task:completed Wire the PostWorkRegistry into agent-process. For curate and curate-folder, the agent body now returns the response immediately, task:completed fires off that response, and the Phase 4 finalize thunk (snapshot diff, summary regeneration, manifest rebuild, dream counter, background drain, task-session cleanup) is queued to the registry to run asynchronously. Effect on user-visible latency: command time drops from phase3 + phase4 (≈11.4 s + 18.2 s ≈ 29.6 s in the measured curate) to roughly phase3 alone (≈11.4 s). Phase 4 keeps running in the daemon's agent-process for ~20 s after the user sees the result. Hardening: - per-project mutex inside the registry serialises concurrent curates' Phase 4, preventing snapshot pre-state races and concurrent writes to `_index.md`; - shutdown drain (30 s) gives in-flight Phase 4 a bounded grace window before the agent-process exits, so SIGTERM cannot truncate a partially-written summary; - registry errors are logged via the existing agentLog channel. Other task types (query, dream, search) are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: [ENG-2522] coordinate detached Phase 4 with DreamLockService Detaching curate Phase 4 made an idle-triggered dream reachable while curate is still mid-summary-regen. Both writers touch _index.md and _manifest.json, so without coordination they corrupt each other. CurateExecutor.propagateSummariesIfChanged and FolderPackExecutor's post-work now acquire the existing DreamLockService PID-lock around propagateStaleness + buildManifest. If the lock is held (dream is running), propagation is skipped — dream's own Phase 5 covers the same diff and the next curate catches any residual change. On success the lock is released; on failure rollback restores the prior mtime so dream's staleness check is unaffected. DreamLockService gets a class-level comment noting its expanded role; naming preserved (dream.lock + DreamLockService) since dream remains the dominant caller and the semantics are identical. Also: - Phase 4 query-latency window (Fix 2 from cross-service review) needs no code change. query-executor.ts already has readManifestIfFresh + buildManifest fallback (L198-200). buildManifest is filesystem-only (readdir/stat/readFile, no LLM), so the fallback rebuild absorbs the post-curate stale-manifest window cheaply. Tests: - 3 new ENG-2522 tests: lock acquired-released on success, skip when held, rollback when propagation throws. - Default DreamLockService stubs added to both executor test files so Phase 4 tests don't write real /<projectRoot>/.brv/dream.lock files to disk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: [ENG-2522] gate hot-swap and dispatch on detached Phase 4 Address review findings on the detach. `task:completed` and `activeTaskCount === 0` no longer imply "agent + tree free", so two consumers needed updating: 1. Deferred hot-swap (agent-process.ts): rebuilding SessionManager mid-Phase 4 would silently fail any in-flight `propagateStaleness` LLM call. Wait on `postWorkRegistry.awaitAll()` before swapping so the rebuild only runs once every project's tail drains. 2. Context-tree task dispatch: `await postWorkRegistry.awaitProject(projectPath)` at the top of curate / curate-folder / dream so a freshly-dispatched dream cannot interleave with a prior curate's still-running Phase 4 on `_index.md` / `_manifest.json`. Bounded by Phase 4 (~18s worst case). Stacked on the DreamLockService coordination from bfe5d57 — the await is the in-process belt; the lock remains the cross-process suspenders for ad-hoc CLI dream while the daemon is mid-Phase 4. Polish (review #3, #4, #5): - PostWorkRegistry: JSDocs reattached to the right fields, `interface` → `type` for the options bag. - folder-pack-executor.test.ts: mirror the curate `runAgentBody / finalize split` describe (returns response before Phase 4; backwards-compat wrapper still inlines Phase 4). - 3 new awaitAll tests (no work / multi-project blocking / snapshot-at-call-time). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: [ENG-2522] trim comments and drop in-source ticket references Per CLAUDE.md: comments should explain WHY when non-obvious; don't restate WHAT the code does, and don't anchor to the ticket — that context belongs in the PR description, not the source. Net -98 lines across 8 files (5 src + 3 test). No behavior change. * fix: [ENG-2522] address PR review findings - Hot-swap race: clear providerConfigDirty BEFORE awaitAll chain so a task arriving during the wait sees a clean flag and skips its inline swap. Re-arm only on unexpected throw from the chain. - Test structure: re-nest 'summary propagation' describe as a sibling of 'instructionsVar naming' (was misnested under it). 'workspace path resolution' and 'runAgentBody / finalize split' now also nest properly inside the outer 'FolderPackExecutor' describe. - Extract shared propagateSummariesUnderLock helper into context-tree/propagate-summaries.ts. Curate and folder-pack post-work now call it instead of duplicating the dream-lock + diff + propagate + manifest block. - Document the folder-pack Phase 4 task-session timing (session is already deleted by executeIterative; folder-pack has no per-task sandbox state to preserve through Phase 4). - Document the query/search exclusion from the awaitProject dispatch gate (manifest staleness is absorbed by readManifestIfFresh fallback).
* chore: Bump veversion to 3.10.1 * chore: update CHANGELOG.md * chore: update CLAUDE.md
|
Claude finished @danhdoan's task in 8m 12s —— View job PR Review: Chore/curation sync main 3.10.1Tasks
SummaryThis PR integrates five feature branches from main 3.10.1 into the curation-enhancement line. The dominant themes are prefix-cache poisoning prevention, batched abstract generation, and deferred stale-summary propagation. The code is well-structured, well-documented, and accompanied by solid test coverage. Below is a feature-by-feature breakdown. 1. Prefix Caching —
|
| File | Location | Severity | Issue |
|---|---|---|---|
abstract-queue.ts |
L213–220 | Nitpick | Empty strings written to .abstract.md/.overview.md when model produces no output — creates blank artifact files |
Overall Score: 4 / 5
The design decisions are sound, well-justified in comments, and backed by comprehensive tests. The A∪B merge for stale summary paths and the atomic drain are the correct solutions to the concurrency problem. The main nit is the unconditional empty-string file write. Everything else is production-grade.
ncnthien
left a comment
There was a problem hiding this comment.
not really good but I think we need to approve now
Summary
Test plan
User-visible changes
None
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
None