fix: preserve complete Markdown reference destinations - #553
mohgupta-ship-it wants to merge 10 commits into
Conversation
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>
rng1995
left a comment
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.
| start += 1 | ||
| if start >= len(line): | ||
| continue | ||
| limit = min(len(line), start + _MAX_MARKDOWN_DESTINATION_CHARS + 2) |
There was a problem hiding this comment.
[P1] Fail closed when an explicit destination exceeds the budget
The 512-character limit silently continues in both the angle and bare branches. A valid long destination containing encoded spaces (so the plain-path fallback cannot recover it) then produces no reference record and no extraction limitation; a missing required file can still report complete/SAFE. Preserve bounded work, but return an explicit limitation/partial result when a syntactically explicit destination crosses this cap, and cover the resulting CLI/MCP rejection.
| # A prose line such as "[status]: All checks passed." is not a | ||
| # reference definition: a destination can only be followed by a title. | ||
| continue | ||
| consumed_until = destination_end |
There was a problem hiding this comment.
[P2] Consume the full link title before finding another opening
consumed_until stops at destination_end, even though _MARKDOWN_INLINE_END may have validated a quoted title and the link's closing parenthesis. Consequently [guide](docs/guide.md "see [sample](missing.md)") yields missing.md as a second required reference even though that text is only the outer link title. Retain the full end position from the successful inline-end match for overlap suppression (while keeping the candidate's destination span unchanged), and add a title regression.
rng1995
left a comment
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
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.
2 participants
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.