Repository navigation
Conversation
…ries (#1015) repair-summaries flagged only missing layers and all-three-identical layers, so rows whose L0/L1/L2 were distinct but unsubstituted tokens ({l0}, {l1}, {l2}) were reported healthy. A layer is now flagged when its entire trimmed value is one of a bounded token set: the layer names l0/l1/l2 or the stored field names l0_abstract/l1_overview/l2_content, in single- or double-brace form. These rows are reported under their own category so operators can review them before repair. Under --apply only the placeholder layers are filled from the source text, through the existing locked recheck, scope filter and reflection exclusion; genuine layers are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019whQZwWU3s3n9aHQMJrGMX
|
The
All test checks for this PR are separate from that one. Generated by Claude Code |
|
The The review step exits after about 2 seconds with The cause looks like the review action's setup in this repo (likely its API credential or quota), not this code. Fixing it needs maintainer access to the workflow secrets, so this PR can't fix it, and no fix exists on master yet. The test checks for this PR are separate from this one. Generated by Claude Code |
|
On the two open questions: keeping the strict set for now (lowercase, no inner spaces, no separate opt-in for |
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed 062726d through the orchestrator. Approved: exact whole-layer matching of the bounded placeholder list is conservative, and the shared scan/apply verdict retains report-only defaults, scope filtering, and apply-time rechecking. The automated CLI smoke test passed; the review also reports passing focused command tests and real-store selective-repair coverage. Automated build/full-suite verification was skipped.
Non-blocking reporting follow-up: when a row contains both a placeholder and a missing layer, placeholder classification wins and its reason/category count omits the missing-layer problem. The displayed layers expose it and apply repairs both, so this affects reporting accuracy.
rwmjhb
left a comment
There was a problem hiding this comment.
GitHub now reports this previously approved PR as conflicting with the latest base branch (mergeable=false, mergeable_state=dirty), following recent merges into the base.
Please rebase onto the latest base branch, resolve the conflicts, and push the updated branch. Regenerate any affected dist artifacts and rerun the relevant tests. The earlier approval applies to the previously reviewed commit; we will check the conflict resolution and any additional changes after the update.
Closes #1015
What problem this solves
memory-pro repair-summariesflagged a row only when a summary layer was missing or all three layers were identical. A row whose L0/L1/L2 were distinct but never substituted ({l0}/{l1}/{l2}) was reported as healthy, even though none of its layers summarize the source.What changed
A layer is flagged when its entire trimmed value exactly matches one of these 12 tokens:
{name}l0,l1,l2,l0_abstract,l1_overview,l2_content{{name}}Why the set stops there. No prompt template in this repo uses brace placeholders. The extraction prompts produce JSON keys (
abstract/overview/content), and the code builds strings with JS template literals. That leaves two kinds of name a template could have meant for a summary layer: the short layer names from the issue, and the metadata field names the layers are stored under. Double braces are included because they are the other common templating syntax (Mustache/Handlebars) for the same tokens. I left out{abstract},{overview}and{content}: nothing in the repo emits them, and they are ordinary words that real content could contain. The match is lowercase-only, allows no spaces inside the braces, and the token must fill the whole layer, so these are not flagged:Release notes use a {version} token,Mentions {l0} inline){version},{abstract},{name},{l3},{}){ l1 },{L2},{l0} pending)OK.,- Yes!)Reporting. Repairable rows are now grouped under three headings: "Missing summary levels", "Degenerate identical levels", and "Unsubstituted template placeholders (review before repair)". Placeholder rows also print the offending layer values (
layers: L0="{l0}" L1="{l1}" L2="{l2}"), so operators can review them before applying. TheFound N repairable entriessummary line is unchanged.Repair (
--applyonly; report-only is still the default). In the sharedjudgeSummaryLevelsverdict, placeholder layers come back blank. The existing repair then fills them from the source text the same way it fills missing layers, and genuine layers on the same row are kept. The path is otherwise the same as for other repairable rows:transformMetadatawith its locked re-judge on the fresh row, the--scopefilter, and theisCurrentReflectionMemoryexclusion. Whole-row replacement is now keyed on akindfield instead of comparing the reason string.The fixes from earlier issues are unchanged. #655: still no text-vs-L0 prefix comparison, so a healthy generated L0 is left alone. #944: the command stays under the
memory-progroup. #923: the scope filter is passed to both the scan and the locked apply.The CLI help text and
CHANGELOG.mdare updated, anddist/is rebuilt. No README or docs page describesrepair-summaries, so there was nothing else to update.Tests
The new cases are in
test/cli-subcommand-attachment.test.mjs. They use the existing harness: they register the realcreateMemoryCLIwith Commander, feed rows through a mock storelist(), and use the production-shapedtransformMetadatadouble. They cover:{l0}/{l1}/{l2}flagged with their own reasons; the mutation callback is never called--apply--applyfills only the placeholder layers and keeps a genuine L0--scopereaches bothlist()andtransformMetadataRed check: I restored master's
cli.tsand rannode --test --test-name-pattern="1015|placeholder" test/cli-subcommand-attachment.test.mjs: 6 of the 8 new tests failed. The 2 that passed are the negative guards (braces in real text, reflection exclusion), which master also satisfies. With the change restored, all 8 pass.Commands run:
node --test test/cli-subcommand-attachment.test.mjs: 30 tests, 30 passnpm test: exit 0, 1260node:testcases pass, 0 fail, 0 skipped (plus the standalone script tests)npm run build(tsc): cleantest:cli-smoke,test:core-regression,test:storage-and-schema,test:llm-clients-and-auth,test:packaging-and-workflow: all exit 0Open questions (resolved)
{L0}) or space-padded ({ l0 }) tokens are not matched. The strict set stays, per the maintainer's decision, and can be widened later if real variants show up.--applyopt-in for placeholder rows beyond the dedicated report heading, per the maintainer's decision.Generated by Claude Code