Skip to content

fix(doc): warn on markdown heading numbering loss - #1819

Open
xu91102 wants to merge 1 commit into
larksuite:mainfrom
xu91102:fix/docs-markdown-heading-seq-warning
Open

fix(doc): warn on markdown heading numbering loss#1819
xu91102 wants to merge 1 commit into
larksuite:mainfrom
xu91102:fix/docs-markdown-heading-seq-warning

Conversation

@xu91102

@xu91102 xu91102 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Warn callers when docs +fetch --doc-format markdown or im-markdown cannot preserve heading seq / seq-level metadata, so agents know to fetch XML when numbering fidelity matters.

Changes

  • Add fetch response warnings for Markdown formats that drop heading numbering metadata.
  • Cover the warning path in docs fetch tests.
  • Update the lark-doc fetch reference with XML guidance for heading numbering metadata.

Test Plan

  • GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./shortcuts/doc -count=1
  • GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go build -o ./lark-cli .
  • LARK_CLI_BIN=$PWD/lark-cli GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./tests/cli_e2e/docs -run 'TestDocs_DryRunDefaultsToV2OpenAPI' -count=1
  • git diff --check
  • conflict marker scan

Related Issues

Summary by CodeRabbit

  • New Features
    • Added support for showing multiple warnings during document fetches.
    • Markdown fetches now warn when heading auto-numbering metadata may be lost, with guidance to use XML when needed.
  • Bug Fixes
    • Improved warning output so users can see all relevant notices instead of only one.
  • Documentation
    • Updated fetch guidance to clarify Markdown and im-markdown format limitations.

@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 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a new warning emitted by docs +fetch (v2) when --doc-format is markdown or im-markdown, informing users that heading auto-numbering metadata (seq/seq-level) is not preserved and that --doc-format xml should be used instead. Warning emission is refactored to support multiple warnings simultaneously, with corresponding test updates and documentation changes.

Changes

Markdown heading seq/seq-level loss warning

Layer / File(s) Summary
Warning aggregation and new markdown heading warning
shortcuts/doc/docs_fetch_v2.go
executeFetchV2 now prints each warning returned by a new addFetchWarnings helper (instead of a single warning); a new addFetchMarkdownHeadingSeqWarning function appends a warning when --doc-format is markdown or im-markdown.
Test updates and documentation
shortcuts/doc/docs_fetch_v2_test.go, skills/lark-doc/references/lark-doc-fetch.md
Existing test now asserts two warnings (downgrade + seq/seq-level); a new test validates the standalone heading-seq-loss warning; documentation notes the markdown/im-markdown limitation and recommends --doc-format xml.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • larksuite/cli#1466: Both modify executeFetchV2's warning generation in docs_fetch_v2.go, extending it from a single detail downgrade warning to multiple aggregated warnings.
  • larksuite/cli#1550: Both touch the v2 fetch path's handling of im-markdown vs markdown formats, complementing the new heading seq/seq-level warning.
  • larksuite/cli#1109: Both modify --doc-format handling logic within docs_fetch_v2.go.

Suggested reviewers: fangshuyu-768, SunPeiYang996

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: adding a warning for markdown heading numbering loss.
Description check ✅ Passed The description follows the template and includes summary, changes, test plan, and related issue.
Linked Issues check ✅ Passed The PR meets #1781 by surfacing a fetch warning when markdown drops seq/seq-level metadata.
Out of Scope Changes check ✅ Passed The code changes stay focused on warnings, tests, and docs for markdown heading numbering loss.
✨ 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.

@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.

🧹 Nitpick comments (1)
shortcuts/doc/docs_fetch_v2_test.go (1)

709-753: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a pretty-mode assertion for the heading seq warning.

The new test covers JSON output, but the pretty-mode stderr path for the heading seq warning is not explicitly asserted anywhere. The existing TestDocsFetchMarkdownDetailDowngradeWarnsInPrettyOutput (line 756) exercises --doc-format markdown --format pretty and now receives both warnings in stderr, but only checks for the downgrade warning strings. Per coding guidelines, every behavior change needs a test alongside the change — consider adding an assertion for the heading seq warning in that test or adding a dedicated pretty-mode test.

🤖 Prompt for 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.

In `@shortcuts/doc/docs_fetch_v2_test.go` around lines 709 - 753, Add pretty-mode
coverage for the heading sequence warning in the docs fetch tests. Update
TestDocsFetchMarkdownDetailDowngradeWarnsInPrettyOutput, or add a dedicated
pretty-output test, so it asserts stderr includes the new heading seq warning
emitted by DocsFetch/mountAndRunDocs when --doc-format markdown --format pretty
is used. Keep the existing downgrade-warning checks and extend them to validate
the warning text from the heading sequence loss path.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@shortcuts/doc/docs_fetch_v2_test.go`:
- Around line 709-753: Add pretty-mode coverage for the heading sequence warning
in the docs fetch tests. Update
TestDocsFetchMarkdownDetailDowngradeWarnsInPrettyOutput, or add a dedicated
pretty-output test, so it asserts stderr includes the new heading seq warning
emitted by DocsFetch/mountAndRunDocs when --doc-format markdown --format pretty
is used. Keep the existing downgrade-warning checks and extend them to validate
the warning text from the heading sequence loss path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 373f40cc-4421-4c63-878b-ad50891b3c13

📥 Commits

Reviewing files that changed from the base of the PR and between 74d8458 and 54e7f62.

📒 Files selected for processing (3)
  • shortcuts/doc/docs_fetch_v2.go
  • shortcuts/doc/docs_fetch_v2_test.go
  • skills/lark-doc/references/lark-doc-fetch.md

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.

docs +fetch --doc-format markdown silently drops heading seq/seq-level (auto-numbering) attribute

1 participant