Skip to content

Update comment-memory rendering to use six-backtick code regions#28115

Merged
pelikhan merged 7 commits intomainfrom
copilot/update-content-memory-rendering
Apr 23, 2026
Merged

Update comment-memory rendering to use six-backtick code regions#28115
pelikhan merged 7 commits intomainfrom
copilot/update-content-memory-rendering

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 23, 2026

Summary

  • wrap managed comment-memory content in six-backtick markdown code regions when rendering comment bodies
  • strip those fence markers when parsing managed comment-memory entries back into local memory files
  • add/adjust JS tests for fenced parsing behavior, legacy unfenced compatibility, and malformed fence edge cases

Validation

  • npx vitest run comment_memory.test.cjs comment_memory_helpers.test.cjs setup_comment_memory_files.test.cjs (from actions/setup/js)
  • make fmt-cjs
  • make agent-finish (currently fails due to pre-existing workflow package test failures: TestCopilotDetectionDefaultModel and TestWasmGolden_CompileFixtures in pkg/workflow)

Copilot AI and others added 7 commits April 23, 2026 14:48
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c58aea4c-9179-42ff-b29b-9bbb205e5f9b

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c58aea4c-9179-42ff-b29b-9bbb205e5f9b

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan April 23, 2026 15:13
@pelikhan pelikhan marked this pull request as ready for review April 23, 2026 15:46
Copilot AI review requested due to automatic review settings April 23, 2026 15:46
@pelikhan pelikhan merged commit f8701b1 into main Apr 23, 2026
23 of 25 checks passed
@pelikhan pelikhan deleted the copilot/update-content-memory-rendering branch April 23, 2026 15:46
@github-actions github-actions Bot mentioned this pull request Apr 23, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 97/100

Excellent test quality

Metric Value
New/modified tests analyzed 11 (primary new tests in comment_memory_helpers.test.cjs)
✅ Design tests (behavioral contracts) 11 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 9 (82%)
Duplicate test clusters 0
Test inflation detected No (36 test lines / 20 production lines = 1.8:1)
🚨 Coding-guideline violations None

Test Classification Details

View All Test Classifications (11 tests)
Test File Classification Issues Detected
extracts managed memory entries comment_memory_helpers.test.cjs ✅ Design None
supports legacy memory entries without code fence markers comment_memory_helpers.test.cjs ✅ Design None
keeps fenced text unchanged when trailing content exists after closing fence comment_memory_helpers.test.cjs ✅ Design Edge case
keeps fenced text unchanged when closing fence is missing comment_memory_helpers.test.cjs ✅ Design Edge case
keeps malformed fenced text unchanged comment_memory_helpers.test.cjs ✅ Design Edge case
strips valid fenced text with extra newlines before content comment_memory_helpers.test.cjs ✅ Design None
strips valid fenced text when content contains six-backtick lines comment_memory_helpers.test.cjs ✅ Design Edge case (key PR invariant)
keeps fenced text unchanged when closing fence has no leading newline comment_memory_helpers.test.cjs ✅ Design Edge case
rejects unsafe memory IDs comment_memory_helpers.test.cjs ✅ Design Error path
allows memory IDs up to 128 characters comment_memory_helpers.test.cjs ✅ Design Boundary value
TestParseRepoSpec (Go, table-driven) pkg/cli/spec_test.go ✅ Design Error paths + edge cases

Analysis Notes

This PR adds a stripCommentMemoryCodeFence helper and updates comment-memory rendering to use six-backtick code regions. The tests are well-aligned with the change:

  • comment_memory_helpers.test.cjs: Thoroughly covers stripCommentMemoryCodeFence with 8 behavioral scenarios — happy path, edge cases (trailing content, missing closing fence, malformed fence, no leading newline), and the critical invariant that six-backtick content within the fenced region is handled correctly (a potential quoting issue that could cause data loss).
  • comment_memory.test.cjs (+2 lines): Added expect(body).toContain("``````") assertions to existing tests to verify the new six-backtick rendering.
  • setup_comment_memory_files.test.cjs (+5/-5 lines): Updated test fixtures to use the new six-backtick format, keeping behavioral coverage intact.
  • pkg/cli/spec_test.go (+5/-5 lines): Minor refactor; existing table-driven tests with proper build tag and descriptive assertion messages.

Minor observation: The vi.fn() mock for the warning callback in rejects unsafe memory IDs is legitimate — it's testing the callback interface (a function parameter), not mocking an internal implementation detail.


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 1 file (pkg/cli/spec_test.go) — unit (//go:build !integration) ✅
  • 🟨 JavaScript (*.test.cjs): 3 files, 11+ tests (vitest) ✅

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). Tests directly enforce the behavioral contract of the new six-backtick code region rendering, including the critical edge case where fenced content itself contains six-backtick lines.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §24844716189

🧪 Test quality analysis by Test Quality Sentinel · ● 622.6K ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

✅ Test Quality Sentinel: 97/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Tests thoroughly cover the behavioral contract of the new six-backtick code region rendering, including critical edge cases.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates managed comment-memory formatting to wrap stored memory content in six-backtick Markdown code fences, and adds parsing support to strip those fences (while remaining compatible with legacy unfenced entries).

Changes:

  • Render managed comment-memory bodies with six-backtick fenced regions.
  • Strip six-backtick fences when extracting managed memory entries back into local files, with edge-case handling.
  • Expand/adjust Vitest coverage for fenced, legacy unfenced, and malformed fence scenarios.
Show a summary per file
File Description
pkg/cli/spec_test.go Formatting-only alignment change in a table-driven test struct.
actions/setup/js/comment_memory.cjs Wraps managed memory content with the exported six-backtick fence marker during rendering.
actions/setup/js/comment_memory_helpers.cjs Adds fence constants + stripCommentMemoryCodeFence() and uses it in extraction.
actions/setup/js/comment_memory_helpers.test.cjs Adds unit tests for fence stripping and malformed/legacy cases.
actions/setup/js/comment_memory.test.cjs Asserts rendered comment bodies include the new fence marker.
actions/setup/js/setup_comment_memory_files.test.cjs Updates tests to use the new fenced managed comment format.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 5/6 changed files
  • Comments generated: 2

Comment on lines 45 to 49
const openingTag = `<${COMMENT_MEMORY_TAG} id="${memoryID}">`;
const closingTag = `</${COMMENT_MEMORY_TAG}>`;
core.info(`comment_memory: building managed body for memory_id='${memoryID}'`);
let body = `${MANAGED_COMMENT_HEADER}\n\n${openingTag}\n${sanitizeContent(rawBody)}\n${closingTag}`;
let body = `${MANAGED_COMMENT_HEADER}\n\n${openingTag}\n${COMMENT_MEMORY_CODE_FENCE}\n${sanitizeContent(rawBody)}\n${COMMENT_MEMORY_CODE_FENCE}\n${closingTag}`;

Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Wrapping memory content with a fixed six-backtick fence will break Markdown rendering if the memory itself contains a line equal to the same six-backtick sequence (it will prematurely close the outer fence and corrupt the rest of the comment). Consider selecting a fence length that is longer than any backtick-run present in the sanitized memory content (minimum 6), and update the stripping/parser logic to accept that variable fence length so round-tripping remains reliable.

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +26
function stripCommentMemoryCodeFence(content) {
const trimmed = typeof content === "string" ? content.trim() : "";
if (trimmed.length === 0) {
return "";
}
if (!trimmed.startsWith(COMMENT_MEMORY_CODE_FENCE)) {
return trimmed;
}
const match = trimmed.match(new RegExp(`^${ESCAPED_COMMENT_MEMORY_CODE_FENCE}[^\\n]*\\n([\\s\\S]*)\\n${ESCAPED_COMMENT_MEMORY_CODE_FENCE}$`));
if (!match) {
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

stripCommentMemoryCodeFence() constructs a new RegExp on every call. This helper runs in a loop while scanning many comments/pages, so repeatedly compiling the same regex is unnecessary overhead. Consider precompiling the fence-stripping regex once at module scope and reusing it inside the function.

Copilot uses AI. Check for mistakes.
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.

3 participants