Skip to content

fix: preserve complete Markdown reference destinations - #553

Open
mohgupta-ship-it wants to merge 2 commits into
mainfrom
codex/fix-markdown-reference-destinations
Open

mohgupta-ship-it wants to merge 2 commits into
mainfrom
codex/fix-markdown-reference-destinations

Conversation

@mohgupta-ship-it

@mohgupta-ship-it mohgupta-ship-it commented Sep 15, 2026

Copy link
Copy Markdown
Member

Required local files named in Markdown reference definitions can disappear from reference accounting. For example, [manual]: docs/missing%20guide.md can produce complete/SAFE with strict exit 0 even when the required file is missing. Valid destinations containing spaces or balanced parentheses also produce false partial scans.

Parse inline and reference-definition destinations with bounded work, preserve the full filename, split URI syntax before decoding exactly once, and check containment after decoding. Required missing or opaque files retain incomplete coverage and strict rejection. The existing slash-prose exclusion and genuine tool.1 references remain intact.

Add 90 paired resolver and CLI/MCP regression tests for present/missing targets, optional escaped titles, encoded filename characters and separators, literal angle filenames, opaque-file decoys, invalid definition prose, and deadline enforcement.

Validation

All five hosted CI checks passed on 975ca05: change detection, lint, DCO, Docker smoke and unit tests. Hosted unit result: 4,921 passed, 89% coverage.

Validated with the 2.11.3 release overlay based on dc303ad, then a fresh installed 2.11.3 wheel. All 97 Python modules in this PR match that validated source; the version bump remains in release PR #550.

  • Full required make test-ci: 4,921 passed, 14 skipped, 38 deselected, 4 expected failures; 89% coverage.
  • Ruff lint/format, mypy, wheel/sdist builds and twine check pass.
  • Exact Dockerfile build and both repository smoke checks pass; all 97 image modules match the source.
  • Two independent reviews passed after correcting escaped-title, literal-angle and deadline counterexamples. A 1 MB malformed-link probe stops at the two-second reference limit.
  • Fresh installed-wheel golden replay: 47/54 cases pass twice. All targeted reference fixes pass. The seven remaining cases are existing quoted-version, unsupported-primary-input and BOM defects; their failing expectations remain unchanged.
  • NVCARPS sanity 29/29, real renderer 29/29, and focused consumer suite 1,156 passed / one known pre-existing PE3 contract failure. No new consumer failure.

The original failing inputs and raw outputs are retained. This PR addresses SKILLSPECT-59 and SKILLSPECT-60. Release #550 remains draft until integration and release-candidate revalidation; the other tracked release gaps remain open.

Prepared by Codex on behalf of Mohit Gupta.

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

[SkillSpector Review]

Reviewed current head 18c897a568d135f1e7f1fe4c73923d9593a436c3, including the complete parser/resolver diff, surrounding containment and basename-resolution behavior, all new public CLI/MCP tests, prior discussion, and exact-head checks. URI splitting before one-time decoding and decoded-path containment are sound, and all five hosted checks pass.

Changes are requested for two unhandled parser boundaries. A syntactically valid explicit destination that exceeds the 512-character scan budget can be silently discarded without a limitation, allowing a required encoded/spaced path to disappear while extraction remains complete. Also, the overlap watermark stops at the destination rather than the full link, so Markdown-looking text inside a valid title is incorrectly treated as another required reference. Propagate the bounded-destination condition into completeness and consume the full validated link/title scope; add resolver plus CLI/MCP regressions for both cases.

Comment thread src/skillspector/references.py
Comment thread src/skillspector/references.py Outdated

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

[SkillSpector Review]

Re-reviewed current head 91ffba6a2f979f2e8adacb6fbe4e66e04c825edf after the automatic synchronization with main. This merge has the previously reviewed head 18c897a568d135f1e7f1fe4c73923d9593a436c3 and current main e19ec01cc85b9891b2976a1157fce492504cfc13 as its exact parents. The only intervening tree changes are #558's three batch-scan files; neither overlaps this PR's two files, whose reviewed contents are unchanged.

Both existing non-outdated threads remain current and unresolved: explicit destinations beyond the 512-character scan budget are still silently discarded without a completeness limitation, and overlap suppression still consumes only through the destination rather than the full validated link title. The existing threads contain the exact corrections and regressions, so I have not duplicated them.

Exact-head CI run 35136921579 concluded action_required without starting jobs, leaving no check results. The two required fixes, unresolved threads, active change request, missing exact-head checks, and GitHub BLOCKED state prevent merge.

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

[SkillSpector Review]

Re-reviewed current head 8e0f51f931097c413d32f2afaa77d3093be3291b against both prior findings, the complete parser/resolver and CLI/MCP regression diff, surrounding completeness behavior, and exact-head checks.

The two required parser fixes remain absent. A syntactically explicit destination beyond the 512-character scan budget is still silently discarded without a limitation, allowing a required reference to disappear from an otherwise complete result. Overlap suppression still advances only through destination_end, so Markdown-looking text inside a valid link title can still become a false second required reference. Propagate the bounded-destination limitation and consume the full validated link/title scope, with resolver and CLI/MCP regressions. I have not duplicated the existing inline findings.

All five exact-head checks pass, but GitHub reports merge conflicts (CONFLICTING / DIRTY); the two unresolved findings, active change requests, and conflicts block merging.

Priority: P0 — reference extraction completeness directly controls fail-closed installation decisions.

mohgupta-ship-it and others added 2 commits September 22, 2026 17:01
Resolve inline and reference-definition destinations with bounded parsing,
one-time URI decoding, and preserved containment. Keep required missing
files incomplete and retain real numeric-extension references.

Add 90 paired resolver and CLI/MCP regression tests covering titles,
spaces, balanced parentheses, encoded paths, and deadline enforcement.

Prepared by Codex on behalf of Mohit Gupta.

Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Mark reference extraction incomplete when Markdown destinations or titles exceed the parser limits. Consume the full validated link span so title text is not treated as another local reference.

Signed-off-by: Chandrashekar Ramachandran <cramachandra@nvidia.com>
@zozozeezee
zozozeezee force-pushed the codex/fix-markdown-reference-destinations branch from 8265387 to 8e7c2e1 Compare September 22, 2026 11:32
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