Skip to content

Chore/curation sync main#602

Merged
danhdoan merged 2 commits intoproj/curation-enhancementfrom
chore/curation-sync-main
May 4, 2026
Merged

Chore/curation sync main#602
danhdoan merged 2 commits intoproj/curation-enhancementfrom
chore/curation-sync-main

Conversation

@danhdoan
Copy link
Copy Markdown
Collaborator

@danhdoan danhdoan commented May 4, 2026

Summary

  • Problem: sync with main (commit 31dc9795)

Test plan

  • Coverage added:
    • Unit test
    • Integration test

User-visible changes

None

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
  • Documentation updated (if applicable)
  • No breaking changes (or clearly documented above)
  • Branch is up to date with main

Risks and mitigations

None

bao-byterover and others added 2 commits May 4, 2026 10:05
…595)

* feat: [ENG-2544] improve byterover provider auth-gate error message

Replace the dead-end "Run /login or brv login to sign in" with a
multi-line message that names every onboarding path: interactive
shell (brv login), and headless / SSH / CI (signup URL, api-key URL,
brv login --api-key). Extract the wording to a module-level constant
in provider-handler.ts so the connect and setActive paths cannot drift.

* fix: [ENG-2544] address PR review comments

- Single space after colon in 'Interactive shell:' for consistency
  with the other bullet line.
- Extract STUB_BYTEROVER_AUTH_ERROR to test/helpers/provider-fixtures.ts
  to remove duplication between connect.test.ts and switch.test.ts.
- Reword fixture comment to clarify it is a representative stub, not a
  mirror of the daemon's full message.
- Add missing 'brv login --api-key' assertion to the SET_ACTIVE test in
  connect.test.ts for parity with the sibling CONNECT test.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

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


PR Review

Tasks

  • Gather context and understand the PR
  • Review source file changes
  • Review test file changes
  • Post inline comments for specific issues
  • Post overall summary with score

Summary

Score: 4 / 5

This is a well-engineered sync that lands three distinct optimizations with strong rationale and test coverage. The changes are coherent, the comments explain the why (not just the what), and the test suite pins the central correctness invariants precisely.


What's good

Prompt caching strategy (LLM layer)
Moving dateTime from the system prompt into the iter-0 user message is the correct Anthropic caching pattern. The byte-stable system prefix now caches cleanly across iterations. The buildDateTimePrefix() helper is well-isolated and tested. The prependCachedSystemMessage() approach (passing the system prompt through the messages[] array with cache_control: ephemeral) is the right workaround for the AI SDK's lack of providerOptions support on the top-level system: string. Adding cache_control to the last tool in toAiSdkTools to cache the entire tool block is also correct.

Batched abstract generation (abstract-queue.ts / abstract-generator.ts)
The 10× LLM call reduction (2N → 2 per batch cycle) is solid. CDATA wrapping for file content injection prevention is a thoughtful security measure — XML/HTML/JSX docs with literal </document> or </file> markers would have silently broken the prompt envelope without it. The parser anchored on <file path="..."> openers (not </file> closers) correctly handles model overviews that mention closing tags in prose. The drainRequested flag elegantly solves the partial-batch buffering problem at curate-end.

Deferred summary cascade / atomic drain (dream layer)
The A ∪ B merge (drained queue + snapshot diff, deduped) in DreamExecutor is clean. The atomic drain (RMW clears the queue in the same transaction that reads it) correctly avoids the race where a concurrent enqueue during propagation would be lost. The re-enqueue on propagation failure is the right recovery path. DreamTrigger's activity gate bypass when staleSummaryPaths.length > 0 correctly unblocks low-activity projects (ENG-2485).


Issues raised (inline)

Severity File Description
suggestion abstract-queue.ts:221 Model-omitted files get empty strings written to disk and counted as processed — existing abstract content silently overwritten with no retry. Consider skipping writeFile when content is empty.
issue abstract-generator.ts:260 Return array maps over the original (possibly duplicate-path) items, not the deduped set. Intentional for queue alignment — worth documenting in the JSDoc signature.
nitpick ai-sdk-message-converter.ts:85 The cache-control landing point depends on Object.entries() insertion order. The invariant ("last tool by registration order") is invisible to callers. Consider a comment spelling out the stability guarantee.
nitpick curate-executor.ts:399 DreamStateService is instantiated directly in propagateAndRebuild rather than injected — diverges from the DreamExecutorDeps DI pattern. Low risk given prototype stubbing in tests covers it.

Minor observations

  • The removal of the three dream-lock tests in curate-executor.test.ts is correct — the old lock-around-propagation contract no longer applies since propagation is deferred to dream. The new ENG-2485 invariant tests (propagateStaleness must NOT be called on the curate path) are the right replacements.
  • StaleSummaryEntrySchema with z.string().min(1) correctly rejects empty paths at the schema boundary. The comment explaining why ("malformed diff entry would resolve to an empty parent dir") is the right level of documentation.
  • readOrInit now returns {...EMPTY_DREAM_STATE} instead of {...EMPTY_DREAM_STATE, pendingMerges: []} — correct cleanup since EMPTY_DREAM_STATE already includes pendingMerges: [] and now staleSummaryPaths: [].

Comment thread src/agent/infra/map/abstract-queue.ts
Comment thread src/agent/infra/map/abstract-generator.ts
Comment thread src/agent/infra/llm/generators/ai-sdk-message-converter.ts
Comment thread src/server/infra/executor/curate-executor.ts
ncnthien
ncnthien previously approved these changes May 4, 2026
Copy link
Copy Markdown
Collaborator

@ncnthien ncnthien left a comment

Choose a reason for hiding this comment

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

good

RyanNg1403
RyanNg1403 previously approved these changes May 4, 2026
@danhdoan danhdoan changed the base branch from main to proj/curation-enhancement May 4, 2026 04:15
@danhdoan danhdoan dismissed stale reviews from RyanNg1403 and ncnthien May 4, 2026 04:15

The base branch was changed.

@danhdoan danhdoan merged commit eea396b into proj/curation-enhancement May 4, 2026
6 checks passed
@danhdoan danhdoan deleted the chore/curation-sync-main branch May 4, 2026 04:16
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.

4 participants