Skip to content

fix: preserve heading auto-numbering in im-markdown and warn for plain markdown - #2038

Closed
godququ5-code wants to merge 2 commits into
larksuite:mainfrom
godququ5-code:fix/docs-markdown-heading-seq
Closed

fix: preserve heading auto-numbering in im-markdown and warn for plain markdown#2038
godququ5-code wants to merge 2 commits into
larksuite:mainfrom
godququ5-code:fix/docs-markdown-heading-seq

Conversation

@godququ5-code

@godququ5-code godququ5-code commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Why

Heading auto-numbering was silently lost in im-markdown output, and plain markdown has no way to carry it. Callers (including AI agents) need either correct numbering or an explicit, machine-readable signal to re-fetch.

Root cause

  • --doc-format markdown: the server returns literal markdown and drops seq/seq-level server-side; the CLI never receives the data, so it cannot be recovered locally. → handled via a warning (Option B).
  • --doc-format im-markdown: the server returns HTML5/DocxXML that does contain seq="auto" seq-level="auto". The renderer already preserved seq for list items, but handleIMMarkdownHeading ignored its attrs argument and discarded the numbering. → fixed in-CLI (Option A).

Test plan

  • Added TestConvertToIMMarkdownHeadingSeq: auto-numbered h1/h2 with seq="auto" seq-level="auto" render as # 1. Section A / ## 1.1. Section A.1 (and nesting resets deeper counters: 1., 1.1., 1.2., 2., 2.1.). Explicit seq and empty-heading regressions covered.
  • Updated TestDocsFetchMarkdownDetailDowngradeWarnsInOutput to expect the new heading-seq warning alongside the existing detail-downgrade warning.
  • go test ./shortcuts/doc/... passes; go vet ./shortcuts/..., gofmt -l ., and go mod tidy are clean.

Changed files

  • shortcuts/doc/docs_fetch_im_markdown.go (heading seq state + handleIMMarkdownHeading/resolveIMMarkdownHeadingSeq)
  • shortcuts/doc/docs_fetch_im_markdown_test.go (TestConvertToIMMarkdownHeadingSeq)
  • shortcuts/doc/docs_fetch_v2.go (addFetchMarkdownHeadingSeqWarning + call site)
  • shortcuts/doc/docs_fetch_v2_test.go (expect new warning)

Related Issues

Summary by CodeRabbit

  • New Features

    • Added automatic hierarchical numbering for headings in converted IM Markdown.
    • Added support for explicit heading sequence values, including proper normalization and per-heading sequencing overrides (via seq-level).
  • Bug Fixes

    • Heading counters now maintain and reset correctly across heading levels during a single conversion.
  • Documentation

    • Added a CLI warning for plain Markdown output noting that automatic heading numbering may be lost.
  • Tests

    • Added coverage for heading sequencing behavior and seq-level precedence.

…n markdown

Option A: handleIMMarkdownHeading now reads the seq/seq-level attrs emitted by the server for --doc-format im-markdown. seq="auto" resolves against a per-level running counter that resets deeper levels (1., 1.1., 1.2., 2., ...); an explicit seq value is used verbatim, mirroring the existing ordered-list-item logic. Capped at 6 levels like before.

Option B: for --doc-format markdown the server returns literal markdown and drops seq/seq-level server-side, so the data is unrecoverable locally. addFetchMarkdownHeadingSeqWarning appends a warnings entry (reusing appendDocWarning, same precedent as addFetchDetailDowngradeWarning) telling callers to re-fetch with --doc-format xml or im-markdown.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 42441951-2230-4f73-bbd8-ae5a153e1d1f

📥 Commits

Reviewing files that changed from the base of the PR and between 98db11c and 043cc2f.

📒 Files selected for processing (2)
  • shortcuts/doc/docs_fetch_im_markdown.go
  • shortcuts/doc/docs_fetch_im_markdown_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • shortcuts/doc/docs_fetch_im_markdown_test.go
  • shortcuts/doc/docs_fetch_im_markdown.go

📝 Walkthrough

Walkthrough

Adds hierarchical heading numbering to IM Markdown conversion and warns when plain Markdown fetches lose seq/seq-level metadata. Tests cover automatic, explicit, absent, and empty heading sequences plus combined warnings.

Changes

Markdown heading sequence support

Layer / File(s) Summary
Heading sequence conversion
shortcuts/doc/docs_fetch_im_markdown.go, shortcuts/doc/docs_fetch_im_markdown_test.go
Tracks heading-depth counters, resolves heading levels, renders automatic or explicit prefixes, resets deeper levels, and validates conversion behavior.
Fetch warning integration
shortcuts/doc/docs_fetch_v2.go, shortcuts/doc/docs_fetch_v2_test.go
Adds and prints a warning for heading-numbering loss in plain Markdown output, with assertions for both fetch warnings.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant FetchCommand
  participant executeFetchV2
  participant convertToIMMarkdown
  participant MarkdownOutput
  FetchCommand->>executeFetchV2: request Markdown document
  executeFetchV2->>convertToIMMarkdown: convert document headings
  convertToIMMarkdown-->>MarkdownOutput: emit numbered headings
  executeFetchV2-->>FetchCommand: append and print heading-sequence warning
Loading

Possibly related PRs

  • larksuite/cli#1466 — Updates related warning emission and assertions in the document fetch flow.
  • larksuite/cli#1550 — Modifies the IM Markdown heading conversion path used by this change.

Suggested reviewers: fangshuyu-768

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preserving heading auto-numbering in im-markdown and warning for plain markdown.
Description check ✅ Passed The description covers summary, motivation, test plan, and related issue; the template's Changes section is implied but not explicitly named.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/M Single-domain feat or fix with limited business impact labels Jul 23, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shortcuts/doc/docs_fetch_im_markdown_test.go`:
- Around line 247-260: Add a focused test case in the existing document-fetch
Markdown tests that uses server-provided seq-level values to determine numbering
prefixes and reset behavior, rather than relying only on h1/h2 nesting. Ensure
the expected Markdown verifies seq-level is honored, including a level
transition or reset that would fail if seq-level were ignored.

In `@shortcuts/doc/docs_fetch_im_markdown.go`:
- Around line 293-318: The automatic numbering in resolveIMMarkdownHeadingSeq
currently uses only the heading level; update it to parse and honor the
server-provided seq-level attribute for sequence depth, including counter
incrementing and deeper-level resets, while preserving the existing
heading-level fallback when seq-level is absent or invalid. In
shortcuts/doc/docs_fetch_im_markdown_test.go lines 247-260, add a fixture where
seq-level differs from the heading tag and assert the resulting numbering
reflects seq-level.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 166a459c-77ee-407a-b04e-ad080bb806ff

📥 Commits

Reviewing files that changed from the base of the PR and between ebc0c53 and 98db11c.

📒 Files selected for processing (4)
  • shortcuts/doc/docs_fetch_im_markdown.go
  • shortcuts/doc/docs_fetch_im_markdown_test.go
  • shortcuts/doc/docs_fetch_v2.go
  • shortcuts/doc/docs_fetch_v2_test.go

Comment thread shortcuts/doc/docs_fetch_im_markdown_test.go
Comment thread shortcuts/doc/docs_fetch_im_markdown.go
Read the seq-level attribute to determine a heading's depth (markdown
'#' count and auto-numbering counter), falling back to the <hN> tag
level when seq-level is absent or 'auto'. This preserves the
seq/seq-level metadata the server attaches to headings (see larksuite#1781)
and addresses a CodeRabbit review suggestion on the PR.

Co-Authored-By: WorkBuddy <noreply@workbuddy.ai>
@zhaojiaxing-coding

Copy link
Copy Markdown
Contributor

Thanks for working on this.

After reviewing the proposal, we decided not to change the Markdown export behavior for issue #1781. This is considered an expected limitation of the Markdown format: Markdown is content-oriented and does not guarantee lossless preservation of Feishu-specific heading-numbering configuration or other Docx metadata. XML should be used when the complete document semantics are required.

Rendering heading numbers as literal Markdown text is also not a reliable solution because it breaks round-tripping. For example, # 1. Section A may be written back as a heading whose text contains 1., rather than being restored as structured Feishu heading numbering, and can result in duplicated numbering.

Since issue #1781 has been closed as not planned, this PR is no longer needed and will not be merged. Closing the PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants