Skip to content

feat(cli): report unsubstituted template placeholders in repair-summaries - #1017

Open
AliceLJY wants to merge 1 commit into
masterfrom
feat/repair-summaries-placeholders-1015
Open

AliceLJY wants to merge 1 commit into
masterfrom
feat/repair-summaries-placeholders-1015

Conversation

@AliceLJY

@AliceLJY AliceLJY commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1015

What problem this solves

memory-pro repair-summaries flagged 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:

Form Names
{name} l0, l1, l2, l0_abstract, l1_overview, l2_content
{{name}} same six names

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:

  • braces inside real text (Release notes use a {version} token, Mentions {l0} inline)
  • other brace words ({version}, {abstract}, {name}, {l3}, {})
  • near misses ({ l1 }, {L2}, {l0} pending)
  • short or punctuated summaries (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. The Found N repairable entries summary line is unchanged.

Repair (--apply only; report-only is still the default). In the shared judgeSummaryLevels verdict, 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: transformMetadata with its locked re-judge on the fresh row, the --scope filter, and the isCurrentReflectionMemory exclusion. Whole-row replacement is now keyed on a kind field 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-pro group. #923: the scope filter is passed to both the scan and the locked apply.

The CLI help text and CHANGELOG.md are updated, and dist/ is rebuilt. No README or docs page describes repair-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 real createMemoryCLI with Commander, feed rows through a mock store list(), and use the production-shaped transformMetadata double. They cover:

  • the issue's four cases in one report-only run: genuine distinct layers not flagged; missing, identical, and {l0}/{l1}/{l2} flagged with their own reasons; the mutation callback is never called
  • placeholder rows reported under their own heading, separate from the other two categories
  • double-brace, field-name and whitespace-padded tokens in a single layer are flagged
  • braces in real text, other brace words, near misses and short summaries are not flagged, even with --apply
  • --apply fills only the placeholder layers and keeps a genuine L0
  • a row healed between scan and apply is skipped by the locked recheck
  • --scope reaches both list() and transformMetadata
  • reflection rows with placeholder-shaped metadata stay excluded

Red check: I restored master's cli.ts and ran node --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 pass
  • npm test: exit 0, 1260 node:test cases pass, 0 fail, 0 skipped (plus the standalone script tests)
  • npm run build (tsc): clean
  • CI groups test:cli-smoke, test:core-regression, test:storage-and-schema, test:llm-clients-and-auth, test:packaging-and-workflow: all exit 0

Open questions (resolved)

  • Case-insensitive ({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.
  • No separate --apply opt-in for placeholder rows beyond the dedicated report heading, per the maintainer's decision.

Generated by Claude Code

…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

AliceLJY commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

The assign-prs check (Auto Assign workflow) is red, but the failure is not caused by this PR.

pozil/auto-assign-issue@v1 stops with Error: Couldn't find issue info in current context, because this action only reads issue events. It also warns that numOfAssignee is not a valid input. The step never touches the code, and the previous non-fork PR (fix(llm-client): keep response text out of JSON er…) failed the same check the same way. The fix belongs in .github/workflows/auto-assign.yml (an assign action that handles pull_request events), not in this PR. No such fix exists on master yet.

All test checks for this PR are separate from that one.


Generated by Claude Code

AliceLJY commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

The claude-review check (Claude Code Review workflow) is red, but the failure is not caused by this PR.

The review step exits after about 2 seconds with total_cost_usd: 0, a single turn, and result is_error:true, before reading any of the diff. The other open non-fork PR (fix(llm-client): keep response text out of JSON er…) failed the same check at the same time with the same signature. I re-ran the job once (job 127133383975) and it failed the same way.

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

@AliceLJY
AliceLJY marked this pull request as ready for review November 5, 2026 09:10
@AliceLJY
AliceLJY requested a review from rwmjhb November 5, 2026 09:10
@AliceLJY

AliceLJY commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

On the two open questions: keeping the strict set for now (lowercase, no inner spaces, no separate opt-in for --apply beyond the dedicated report heading). We can widen it later if real placeholder variants show up in the wild.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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.

[Feature]: Detect distinct template placeholders in repair-summaries

3 participants