Skip to content

plan 89: address Copilot review on PR #147 - #148

Merged
jeduden merged 3 commits into
mainfrom
claude/plan-89-copilot-fixes
Apr 20, 2026
Merged

plan 89: address Copilot review on PR #147#148
jeduden merged 3 commits into
mainfrom
claude/plan-89-copilot-fixes

Conversation

@jeduden

@jeduden jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to PR #147 (already merged). Addresses six Copilot review threads on the plan 89 document that were not resolved before merge.

Changes

  • Renumber MDS036MDS037 for the toc rule. Plan 51 (PR plan 51: add MDS036 section-size-limits rule #143) merged first and took MDS036 for max-section-length. MDS034 remains reserved for plan 86.
  • Reuse existing slug logic. Instead of a new hand-rolled algorithm, require moving slugify and the duplicate-anchor counter out of internal/rules/crossfilereferenceintegrity/ into a shared helper (internal/mdtext/) that both rules call. Drops the incorrect goldmark-meta reference.
  • Drop <?listindent?> — it is not a real directive. Describe list indentation in terms of MDS016 list-indent.spaces (default 2).
  • Correct MDS015 interaction note. MDS015 enforces blank lines around fenced code blocks only; padding around <?toc?> is MDS035 Fix's responsibility.
  • Unify the canonical empty-body form as <?toc?>\n<?/toc?> everywhere (design, tasks, acceptance criteria).
  • Add a prerequisite task to move the slug helper to the shared package and renumber downstream tasks.
  • Grammar pass: "GitHub-style" consistently.

Test plan

  • mdsmith check . passes
  • Plan doc re-reads consistently (rule ID, empty-body format, references)

Copilot AI review requested due to automatic review settings April 19, 2026 20:58
@codecov

codecov Bot commented Apr 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.76%. Comparing base (7958dcd) to head (6a6db67).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #148   +/-   ##
=======================================
  Coverage   86.76%   86.76%           
=======================================
  Files          96       96           
  Lines       10446    10446           
=======================================
  Hits         9063     9063           
  Misses        900      900           
  Partials      483      483           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates plan 89’s specification for the upcoming <?toc?>...<?/toc?> directive and related MDS035 fix behavior, aligning it with current rule numbering and existing anchor/slug logic used elsewhere in mdsmith.

Changes:

  • Renumbers the planned TOC rule from MDS036 → MDS037 and documents the numbering rationale.
  • Specifies reusing (and extracting) existing heading-anchor slug/disambiguation logic from crossfilereferenceintegrity into a shared helper under internal/mdtext/.
  • Removes/clarifies incorrect directive and rule-interaction details (no <?listindent?>, MDS015 scope, canonical empty <?toc?> block form).

Comment thread plan/89_toc-generator-directive.md Outdated
- Renumber MDS036 → MDS037 for the toc rule. Plan 51 shipped first
  and took MDS036 for max-section-length; MDS034 stays reserved for
  plan 86.
- Replace the hand-rolled slug algorithm with a directive to move
  the existing slugify / duplicate-disambiguation logic out of
  crossfilereferenceintegrity into a shared helper
  (internal/mdtext) that both rules call. Drops incorrect
  goldmark-meta reference.
- Drop fictitious <?listindent?> directive reference; describe
  indentation as MDS016 list-indent.spaces (default 2).
- Correct MDS015 interaction note: MDS015 only covers fenced code
  blocks, so blank-line padding around <?toc?> is MDS035 Fix's
  responsibility.
- Unify the canonical empty-body form as <?toc?>\n<?/toc?>
  throughout design, tasks, and acceptance criteria.
- Add task 1 (move slug helper to shared package) and renumber
  downstream tasks.
- Grammar pass: "GitHub-style" consistently.

Follow-up to PR #147 (already merged).
@jeduden
jeduden requested a review from Copilot April 20, 2026 13:27
@jeduden
jeduden force-pushed the claude/plan-89-copilot-fixes branch from 7b998e9 to fe1437e Compare April 20, 2026 13:27
Copilot noted that `list-indent.spaces` implies a flat key that
doesn't exist in .mdsmith.yml; the actual config is nested under
list-indent with a 'spaces' field. Reword to match.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread plan/89_toc-generator-directive.md Outdated
Line numbers drift as the file evolves. Replace 'around lines 349 and
389' with a reference to the collectHeadingAnchors / slugify helpers.
Copilot AI review requested due to automatic review settings April 20, 2026 15:46
@jeduden
jeduden force-pushed the claude/plan-89-copilot-fixes branch from c293646 to 6a6db67 Compare April 20, 2026 15:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Apr 20, 2026
@jeduden

jeduden commented Apr 20, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Apr 20, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-148-1776700679. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Apr 20, 2026
@jeduden
jeduden merged commit 3291efc into main Apr 20, 2026
16 checks passed
@jeduden

jeduden commented Apr 20, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit 3291efc. CI run that validated the merge.

Next: Done — nothing more to do here.

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