fix: preserve complete Markdown reference destinations - #553
mohgupta-ship-it wants to merge 2 commits into
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[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.
rng1995
left a comment
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
[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.
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>
8265387 to
8e7c2e1
Compare
Required local files named in Markdown reference definitions can disappear from reference accounting. For example,
[manual]: docs/missing%20guide.mdcan 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.1references 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.make test-ci: 4,921 passed, 14 skipped, 38 deselected, 4 expected failures; 89% coverage.twine checkpass.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.