fix: [ENG-2676] strip derived-artifact paths from related: frontmatter#618
fix: [ENG-2676] strip derived-artifact paths from related: frontmatter#618cuongdo-byterover wants to merge 3 commits intomainfrom
Conversation
`*.abstract.md` and `*.overview.md` (and other derived artifacts) are excluded from the push payload via isExcludedFromSync(). When they landed in a doc's `related:` field, the parent doc shipped to remote with dangling references to siblings that were never sent. Two write paths could produce them; both now filter: - consolidate.ts `addRelatedLinks` (dream's CROSS_REFERENCE) filters incoming AND pre-existing entries, so each touch opportunistically cleans up legacy dangling refs. - curate-tool.ts `filterValidFiles` drops excluded paths from `relations` before MarkdownWriter writes them — covers both ADD and UPDATE since both call sites share the helper. Pre-existing dangling refs in untouched docs are out of scope; a separate migration can scrub them.
cuongdo-byterover
requested review from
RyanNg1403,
bao-byterover,
danhdoan,
leehpham and
ngduyanhece
as code owners
|
Claude finished @cuongdo-byterover's task in 3m 45s —— View job PR Review — fix: [ENG-2676] strip derived-artifact paths from related: frontmatterScore: 3/5 — solid, well-scoped fix on the dream/CROSS_REFERENCE side, with one meaningful gap on the curate side that contradicts the PR description.
What's good
What's missing or worth tightening
Out of scope (correctly noted in PR body)Pre-existing dangling refs in untouched docs — agreed, separate migration. The current PR's opportunistic cleanup on dream's CROSS_REFERENCE is a nice middle ground. · branch |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… UPDATE PR #618 review flagged that the curate UPDATE path could still leak derived-artifact entries into `related:` because `existingParsed.relations` flowed unfiltered into both `detectStructuralLoss` (false-positive `lostRelations`, bumping impact to high) and `resolveStructuralLoss` (union-merging the legacy bad data back into the final write). Filter `existingParsed.relations` through isExcludedFromSync immediately after parse so all downstream conflict logic sees a clean view. Adds an UPDATE-path test that pre-seeds a stale `.abstract.md` entry and asserts it is stripped. Also fixes the reviewer nit on consolidate.ts:681 — early-return when `incoming` is empty and there's no existing frontmatter, so we don't wrap an otherwise-clean file in `---\nrelated: []\n---\n`.
|
Claude finished @cuongdo-byterover's task in 3m 8s —— View job PR Review — fix: [ENG-2676] strip derived-artifact paths from related: frontmatterScore: 4/5 — solid follow-up from 3/5. The blocking UPDATE structural-loss leak is properly fixed; new test locks in the regression-prevention. Remaining items are non-blocking refinements.
What's good
What's worth tightening (all non-blocking)
Out of scope (correctly noted in PR body)Pre-existing dangling refs in untouched docs — agreed, separate migration. The opportunistic-cleanup-on-touch behavior is the right middle ground. Verification not done
· branch |
| if (existingParsed?.relations) { | ||
| // Drop legacy dangling refs before conflict-detection; otherwise resolver unions them back. | ||
| existingParsed.relations = existingParsed.relations.filter((r) => !isExcludedFromSync(r)) | ||
| } |
There was a problem hiding this comment.
praise: This addresses the blocking issue from the prior review cleanly. Filtering existingParsed.relations here (before detectStructuralLoss at line 1023 and resolveStructuralLoss at line 1026, which union-merges existing.relations with proposed.relations per conflict-resolver.ts:130) ensures legacy dangling refs get stripped on UPDATE rather than re-injected via structural-loss recovery. As a bonus, this also prevents false-positive lostRelations bumps in conflict-detector.ts:75 that would have elevated impact and unnecessarily triggered review.
Mutating existingParsed.relations in place is safe — MarkdownWriter.parseContent (line 875) returns a fresh ContextData object, and existingParsed is local to this function.
nitpick (non-blocking): if (existingParsed?.relations) matches both empty and populated arrays. Could tighten to if (existingParsed?.relations?.length) to skip the no-op filter when the array is already empty. Pure cosmetic.
| try { | ||
| const parsed = yamlLoad(yamlBlock) as null | Record<string, unknown> | ||
| if (parsed && typeof parsed === 'object') { | ||
| const existing = Array.isArray(parsed.related) ? (parsed.related as string[]) : [] | ||
| parsed.related = [...new Set([...existing, ...relatedPaths])] | ||
| const existing = (Array.isArray(parsed.related) ? (parsed.related as string[]) : []) | ||
| .filter((p) => !isExcludedFromSync(p)) | ||
| parsed.related = [...new Set([...existing, ...incoming])] | ||
| const newYaml = yamlDump(parsed, {flowLevel: 1, lineWidth: -1, sortKeys: false}).trimEnd() | ||
| await atomicWrite(filePath, `---\n${newYaml}\n---\n${body}`) | ||
| return |
There was a problem hiding this comment.
nitpick (non-blocking): This handles the no-frontmatter empty-result case nicely (early return at line 681), but if the file has frontmatter without a related key and incoming ends up empty after filtering, we still set parsed.related = [] and rewrite the file — adding an empty related: [] field that wasn't there before. Edge case, cosmetic.
Could short-circuit when nothing meaningful changed:
const existingArr = Array.isArray(parsed.related) ? (parsed.related as string[]) : []
const existing = existingArr.filter((p) => !isExcludedFromSync(p))
if (existing.length === existingArr.length && incoming.length === 0) return
parsed.related = [...new Set([...existing, ...incoming])]Feel free to defer.
| }) | ||
|
|
||
| it('strips legacy derived-artifact entries from existing related: on UPDATE (conflict-resolver path)', async () => { | ||
| // Pre-seed a file whose related: already contains a stale .abstract.md | ||
| // entry (legacy data from before the fix). UPDATE must not union it | ||
| // back through resolveStructuralLoss. | ||
| const tool = createCurateTool() | ||
| const targetDir = join(basePath, 'operations/cafe') | ||
| await fs.mkdir(targetDir, {recursive: true}) | ||
| const seedPath = join(targetDir, 'menu_notes.md') | ||
| const seed = [ | ||
| '---', | ||
| 'title: Menu Notes', | ||
| 'summary: original notes', | ||
| 'tags: []', | ||
| 'related: [operations/cafe/sunrise_cafe_menu.md, operations/cafe/sunrise_cafe_menu.abstract.md]', | ||
| 'keywords: []', | ||
| "createdAt: '2026-04-01T00:00:00.000Z'", | ||
| "updatedAt: '2026-04-10T00:00:00.000Z'", | ||
| '---', | ||
| '## Reason\nseed', | ||
| '## Raw Concept\n**Task:** seed', | ||
| ].join('\n') | ||
| await fs.writeFile(seedPath, seed, 'utf8') | ||
|
|
||
| const result = (await tool.execute({ | ||
| basePath, | ||
| operations: [ | ||
| { | ||
| confidence: 'high', | ||
| content: { | ||
| keywords: [], | ||
| relations: ['operations/cafe/ingredient_sourcing.md'], | ||
| snippets: ['updated notes'], | ||
| tags: [], | ||
| }, | ||
| impact: 'low', | ||
| path: 'operations/cafe', | ||
| reason: 'testing UPDATE relations filter', | ||
| title: 'Menu Notes', | ||
| type: 'UPDATE', | ||
| }, | ||
| ], | ||
| })) as CurateOutput | ||
|
|
||
| expect(result.applied[0].status).to.equal('success') | ||
| const written = await fs.readFile(seedPath, 'utf8') | ||
| expect(written).to.include('operations/cafe/sunrise_cafe_menu.md') | ||
| expect(written).to.include('operations/cafe/ingredient_sourcing.md') | ||
| expect(written).to.not.include('sunrise_cafe_menu.abstract.md') | ||
| }) |
There was a problem hiding this comment.
praise: The UPDATE test (strips legacy derived-artifact entries from existing related: on UPDATE (conflict-resolver path)) is exactly what was missing — pre-seeds a file with a stale .abstract.md reference, runs UPDATE with a fresh proposal, and asserts the legacy entry is stripped while a real existing relation (sunrise_cafe_menu.md) survives the structural-loss merge. This is the regression-prevention coverage that matters; without it, fix could silently regress.
suggestion (non-blocking): Consider adding one more assertion against result.applied[0].impact (or whichever field surfaces structural-loss elevation) to lock in the secondary effect: that filtering the existing relations also avoids a false-positive lostRelations count, so impact stays at 'low' rather than getting bumped to 'high'. That's the silent failure mode this fix also closes — currently the test only verifies file content, not the review/impact behavior.
|
|
||
| async function addRelatedLinks(filePath: string, relatedPaths: string[]): Promise<void> { | ||
| // Skip paths that won't be pushed — they'd be dangling refs on remote. | ||
| const incoming = relatedPaths.filter((p) => !isExcludedFromSync(p)) |
There was a problem hiding this comment.
suggestion (defensive, non-blocking): The new filter cleans the payload (paths written into the target's related: frontmatter) and the existing entries in the target's frontmatter, but the target file itself (the filePath argument) is whatever the LLM put in action.files at consolidate.ts:628. If the LLM ever includes a .abstract.md / .overview.md / archive stub there, we'd write related: frontmatter into a derived artifact (owned by the abstract pipeline, excluded from sync — so the writes get overwritten on the next dream run anyway, but it's wasted I/O and confusing for anyone reading those files locally).
A one-liner inside the executeCrossReference map would harden this:
action.files.map((file) => {
if (isExcludedFromSync(file)) return Promise.resolve()
const otherFiles = action.files.filter((f) => f !== file)
return addRelatedLinks(join(contextTreeDir, file), otherFiles)
}),Defer if you prefer to keep this PR narrowly scoped.
Round 2 of review-driven cleanup. None blocking, all small. - Filter executeCrossReference target files: skip derived-artifact paths in `action.files` so addRelatedLinks never writes `related:` onto a `.abstract.md` / `.overview.md` if the LLM ever proposes one. - addRelatedLinks: don't introduce `related: []` into a file whose frontmatter never had the key, when the merged set is empty. - Tighten existingParsed.relations guard from truthy to length-aware so the no-op filter is actually skipped on empty arrays. - UPDATE relations test: assert `impact === 'low'` to lock in the no-false-positive structural-loss-elevation invariant. Also retains the legitimate sibling in proposed so the only "lost" entry would be the filtered derived one — the assertion now genuinely exercises the filter's effect on conflict-detector counting.
|
Claude finished @cuongdo-byterover's task in 2m 32s —— View job PR Review — fix: [ENG-2676] strip derived-artifact paths from related: frontmatterScore: 5/5 — Final round addresses all 4 non-blocking items from the prior review cleanly. Up from 4/5 → ship-ready.
What's good in this round
Minor observations (defer-able, won't block)
Verification not done
SummaryThis is the right shape of follow-up: surgical, scoped, and each change traceable to a specific review item. The blocking issue from review #1 stays fixed (verified |
RyanNg1403
left a comment
RyanNg1403
left a comment
There was a problem hiding this comment.
E2E verified on a paid-tier project: brv curate writes clean related: despite *.abstract.md / *.overview.md siblings on disk, and brv dream CROSS_REFERENCE strips both incoming AND pre-existing derived-artifact entries on every touched doc — fix works.
2 participants
*.abstract.mdand*.overview.md(and other derived artifacts) are excluded from the push payload via isExcludedFromSync(). When they landed in a doc'srelated:field, the parent doc shipped to remote with dangling references to siblings that were never sent.Two write paths could produce them; both now filter:
addRelatedLinks(dream's CROSS_REFERENCE) filters incoming AND pre-existing entries, so each touch opportunistically cleans up legacy dangling refs.filterValidFilesdrops excluded paths fromrelationsbefore MarkdownWriter writes them — covers both ADD and UPDATE since both call sites share the helper.Pre-existing dangling refs in untouched docs are out of scope; a separate migration can scrub them.
Summary
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
User-visible changes
List user-visible changes (including defaults, config, or CLI output).
If none, write
None.Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
List real risks for this PR. If none, write
None.