Skip to content

Fix/swarm memory resolve comment#408

Merged
danhdoan merged 2 commits intoproj/swarm-memoryfrom
fix/swarm-memory-resolve-comment
Apr 14, 2026
Merged

Fix/swarm memory resolve comment#408
danhdoan merged 2 commits intoproj/swarm-memoryfrom
fix/swarm-memory-resolve-comment

Conversation

@cuongdo-byterover
Copy link
Copy Markdown
Collaborator

Summary

  • Problem:
  • Why it matters:
  • What changed:
  • What did NOT change (scope boundary):

Type of change

  • Bug fix
  • New feature
  • Refactor (no behavior change)
  • Documentation
  • Test
  • Chore (build, dependencies, CI)

Scope (select all touched areas)

  • TUI / REPL
  • Agent / Tools
  • LLM Providers
  • Server / Daemon
  • Shared (constants, types, transport events)
  • CLI Commands (oclif)
  • Hub / Connectors
  • Cloud Sync
  • CI/CD / Infra

Linked issues

  • Closes #
  • Related #

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

  • Root cause:
  • Why this was not caught earlier:

Test plan

  • Coverage added:
    • Unit test
    • Integration test
    • Manual verification only
  • Test file(s):
  • Key scenario(s) covered:

User-visible changes

List user-visible changes (including defaults, config, or CLI output).
If none, write None.

Evidence

Attach at least one:

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

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

List real risks for this PR. If none, write None.

  • Risk:
    • Mitigation:

cuongdo-byterover and others added 2 commits April 14, 2026 17:07
Blocking (5 fixes):
- #1: Bound resolveUniqueFilename loop to MAX_SUFFIX=10000 with
  timestamp fallback (local-markdown-adapter)
- #2: Make GBrain bin resolution lazy — execFileSync no longer runs
  in constructor, deferred to first executor use
- #3: Replace execFileSync with async execFileAsync in config-validator
  (validateGBrain is now async)
- #4: Add MAX_ARG_LENGTH guard (200KB) in execBrvCurate to prevent
  E2BIG on large content
- #5: Bound memory-wiki store() filename loop with same pattern

Medium (6 fixes):
- #6: Remove unreachable 'creative' from QueryType union and
  SELECTION_MATRIX
- #8: Replace this.index! non-null assertions with explicit null
  guards in obsidian and local-markdown adapters
- #9: Fix byterover writeSupported: true → false in
  createDefaultCapabilities (matches adapter)
- #10: Replace 'as' type assertion with instanceof narrowing in
  execBrvCurate error handler
- #11: Replace 'as Error' assertions with instanceof checks in
  swarm curate and query CLI commands
- Stale JSDoc: remove 'creative' from router comment

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

Prevent YAML title injection by using JSON.stringify instead of raw
interpolation. Add clarifying comments for null guard and sync probe.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 14, 2026

Claude finished @cuongdo-byterover's task in 3m 10s —— View job


PR Review

  • Gather context and read changed files
  • Analyze types, adapters, coordinator, router, validator changes
  • Review test coverage
  • Post inline comments and summary

Summary

Score: 3.5 / 5

This PR resolves several legitimate issues from a prior review: lazy executor init in GBrainAdapter, infinite-loop protection in filename uniqueness logic, YAML injection in memory-wiki frontmatter, sync→async in the config validator, and as Error casts in the CLI commands. The direction is correct throughout. Three issues prevent a higher score.


What's good

  • YAML injection fix (memory-wiki-adapter.ts:228): JSON.stringify(title) correctly handles ", newlines, and colons in titles that would previously corrupt the YAML frontmatter. This is the most important fix in the PR.
  • Lazy executor init (gbrain-adapter.ts:164): The private get executor() pattern elegantly defers the synchronous execFileSync PATH probe to first-use. The added comment on line 60 documents the trade-off clearly.
  • MAX_SUFFIX guards in both local-markdown-adapter.ts and memory-wiki-adapter.ts: The timestamp fallback is a safe unbounded-loop escape hatch.
  • async validateGBrain in config-validator.ts: Removing execFileSync from the validator is correct — it was blocking the event loop during config startup.
  • instanceof Error checks in curate.ts and query.ts: Replaces unsafe as Error casts with proper narrowing.

Issues

1. Import order in config-validator.ts (see inline comment, line 5)
const execFileAsync = promisify(execFile) is placed between import declarations. All imports should come first; the const should follow after.

2. Remaining as assertion in swarm-coordinator.ts (see inline comment, line 41)
(error as NodeJS.ErrnoException & {stderr?: string}).stderr uses as, which the project style guide prohibits (except lazy-init singletons). A simple {message: string; stderr?: string} cast or a 'stderr' in error guard would be idiomatic.

3. Missing tests for new behavior (see inline comments on swarm-coordinator.ts:29 and memory-wiki-adapter.ts:226)
Per the project's strict TDD policy, three new behaviors lack coverage:

  • execBrvCurate should throw when content.length > 200_000.
  • MemoryWikiAdapter.store() should produce valid YAML when the title contains ".
  • MemoryWikiAdapter.store() filename collision fallback (suffix loop + timestamp).

These are correctness/security fixes — they should be verifiable by a test, not just by inspection.


Minor nits

  • Redundant this.repoPath in gbrain-adapter.ts (inline, line 154): Now duplicates this.options.repoPath since options is stored as a field.
  • Duplicate JSDoc in swarm-router.ts (inline, line 27): The old comment block for SELECTION_MATRIX was not removed when the new one was added.
  • creative type removal: The routing matrix removal in swarm-router.ts is consistent and correct given the type narrowing in types.ts. classifyQuery never returned 'creative' anyway since there were no regex signals for it.

Comment thread src/agent/infra/swarm/validation/config-validator.ts
Comment thread src/agent/infra/swarm/swarm-coordinator.ts
Comment thread src/agent/infra/swarm/adapters/gbrain-adapter.ts
Comment thread src/agent/infra/swarm/swarm-router.ts
Comment thread src/agent/infra/swarm/swarm-coordinator.ts
Comment thread src/agent/infra/swarm/adapters/memory-wiki-adapter.ts
Comment thread src/agent/infra/swarm/adapters/memory-wiki-adapter.ts
Comment thread src/agent/infra/swarm/adapters/gbrain-adapter.ts
Copy link
Copy Markdown
Collaborator

@danhdoan danhdoan left a comment

Choose a reason for hiding this comment

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

Lgtm

@danhdoan danhdoan merged commit 9170fdf into proj/swarm-memory Apr 14, 2026
6 checks passed
@bao-byterover bao-byterover mentioned this pull request Apr 14, 2026
29 tasks
@cuongdo-byterover cuongdo-byterover deleted the fix/swarm-memory-resolve-comment branch April 14, 2026 14:18
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