fix: ground claims whose source tags are written adjacently (2.5.37) - #686
Conversation
apackeer
left a comment
There was a problem hiding this comment.
I checked out this head (11e9157) on Linux and ran the full tier you asked for: bash tests/run-tests.sh --smoke --unit completes with 180 files, 4458 assertions, 0 failures. bun run check is clean (543 files), bun scripts/package.ts --check reports byte parity across all five dists, t247 passes 34/34, and the 2.5.31 version trio agrees (t68 green). The slot arithmetic checks out too: 2.5.27 through 2.5.30 are indeed claimed by the open PRs you named.
The definition-aware approach is the right call, and the resolution logic holds up beyond the pinned cases. Three probes I ran that the tests do not cover, all behaving correctly:
[desc][Q1]with only[desc]: <url>defined stays literal: a full reference resolves against its second label, and the code gets that right.![Q1]with no definition now grounds the claim (the old sensor silently consumed the tag).- A definition separated from surrounding hidden HTML by blank lines is registered, and a shortcut use of it correctly grounds nothing.
That said, I would hold merge for two findings. Both reproduce on this head, both are confirmed against markdown-it in commonmark mode, and both land in the one direction this sensor cannot afford: a false pass. The bug this PR fixes was a false fail (loud, visible, self-correcting: a conductor rewrites an already-compliant file). These two let unsourced or invisible-tag content through silently.
1. A definition-shaped line with an invalid destination exempts its whole block from inspection
## Target Customer
- The sole user is the author. [Q1]
[evidence]: this is an unsupported assertion[evidence]: this is an unsupported assertion is not a link reference definition: the destination is invalid, so CommonMark renders the whole line as visible prose (markdown-it emits <p>[evidence]: this is an unsupported assertion</p>). But referenceDefinitionLabel (core/tools/aidlc-sensor-claim-sources.ts:730) recognises definitions by line shape only, and the new isReferenceDefinitionBlock skip (defined at 792, applied at 896) then exempts the block from claim inspection entirely. Result here: pass: true, zero findings. On v2 this fixture fails with claim block has no source tag, which is the correct answer for a reader-visible unsourced sentence.
This is the part I would not take as a follow-up. The PR body flags the permissive definition reading as a residual that is "conservative in the direction that matters", and that was true while the shape test only stripped syntax: over-recognising a definition could at worst consume brackets and produce a false fail. The block-skip changes the sign. Once a shape-matched line can exempt a block from inspection, the permissive reading becomes a bypass: any assertion written in [label]: text shape escapes the sensor.
2. Definitions inside containers are not registered, so rendered links still count as tags
## Target Customer
- The sole user is the author. [desc][Q1]
> [Q1]: https://example.invalidCommonMark accepts link reference definitions inside block quotes and list items, so [desc][Q1] renders as a link here (markdown-it confirms). referenceDefinitionLabel only matches a definition starting within three columns at the top level, misses this one, keeps both tags visible, and returns pass: true on a claim whose tags the reader never sees. The list-item form reproduces the same way:
- The sole user is the author. [desc][Q1]
- [Q1]: https://example.invalidv2 happens to fail both fixtures correctly.
Shared root and a contained fix
Both findings are one defect: definition detection diverges from CommonMark's definition grammar in both directions at once (too loose on what a definition line is, too strict on where one may sit). A fix that stays inside this PR's architecture:
- Gate
isReferenceDefinitionBlock, and ideally label registration too, on a strictly valid definition: well-formed destination plus optional valid title. A shape-matched line that fails that test is prose, and its block gets inspected. - Make label collection container-aware: strip leading block-quote markers and list markers before testing for the definition shape.
The invariant worth holding the sensor to, and maybe worth one line in the manifest: wherever the sensor cannot afford full CommonMark, the divergence must land as a false fail, never a false pass.
Minor, not blocking
A definition-shaped line inside an HTML block with no blank-line separation (a <div hidden> directly above it) is not a definition per CommonMark, but it registers a label and can over-consume references elsewhere in the document. That lands as a false fail, so it is an acceptable residual if documented; it is the same root cause as the two blockers.
Also noting for scope: markup-only lines such as <div hidden> / </div> inside a deliverable emit duplicate claim block has no source tag findings. That reproduces on v2 as well, so it is pre-existing and not this PR's problem.
The report-vs-mirror analysis and the reproduction discipline in this PR are genuinely good work, and the two blockers have small fixes that live exactly where the PR already works. Happy to re-run the full Linux tier on the next push.
11e9157 to
128d0b0
Compare
|
Both blockers reproduce, both are fixed. New head: Thank you for running the Linux tier I could not run, and for the three extra probes. Knowing that You are right about the sign, and that is the part I got wrongI wrote in the PR body that the permissive definition reading was "conservative in the direction that matters". That claim was true of the change I had made when I wrote it — a shape-matched line could at worst consume brackets and produce a false fail. Then I added the block-skip and did not go back and re-test the claim against it. Once a shape-matched line can exempt a block from inspection, the same permissiveness stops being conservative and becomes a bypass: any assertion written as I reproduced both of your fixtures as failing tests at What changedDefinition detection now requires a well-formed destination, plus an optional valid title on the same line. Label collection is container-aware. Block-quote and list markers come off before the shape test, so Both live in One decision worth naming, since it is the residual you would otherwise have to find: CommonMark allows the destination to sit on the line after the label, and I do not treat that as a definition. That is deliberate under your invariant — reading it as prose costs a false fail on a form these deliverables do not use, while reading it as a definition would let a block skip inspection. The comment says so at the site. The invariantAdded to the manifest, near enough to your wording that I hope it reads as yours:
That is the rule I failed to apply to my own change, and it is more useful in the manifest than in this thread. Your minor — a definition-shaped line inside an HTML block with no blank-line separation — has the same root and now lands as a false fail by construction, which the invariant covers as an acceptable residual. The duplicate findings from markup-only lines I have left alone, since you confirmed they reproduce on Gates
The full Linux tier is the one thing I still cannot verify from here — I would appreciate the re-run you offered. |
v2 shipped 2.5.30 while this branch still declared 2.5.27. Scanned the open PRs by their actual AIDLC_VERSION diff rather than their titles: 2.5.31 (awslabs#535, awslabs#661, awslabs#686), 2.5.32 (awslabs#660, awslabs#687) and 2.5.33 (awslabs#689) are claimed, so this takes 2.5.34. The CHANGELOG entry was rebuilt from v2's file with this branch's block reinserted, so no upstream heading is lost. Its sensor-cache bullet now describes the engine-path match rather than the leaf-name one, and a new bullet covers the clean-filter binding. Coverage registry regenerated with the tool, not hand-edited.
|
I checked the current head ( 1. Malformed single-token destinations still bypass inspection
[evidence]: <brokenCommonMark renders this as visible prose ( 2. Nested container stripping remains order-dependent
- > [Q1]: https://example.invalid
[desc][Q1]CommonMark resolves The HTML-block residual still lands as the accepted false failure, and leaving the pre-existing duplicate markup findings unchanged matches the review. I would keep the PR in changes-requested state until destination validation rejects malformed forms and container markers are consumed iteratively (or equivalent regression coverage proves the invariant). |
128d0b0 to
01b03a9
Compare
|
Both variants reproduce and are fixed. New head: Your two findings are the reason this round exists, but chasing them the way I chased the last two would have earned a fourth round. So I stopped patching cases and checked the whole grammar against a reference implementation instead. That turned up four more forms of the same defect that neither of us had named — including one that is a false pass in exactly the way you describe. The two you reportedI reproduced both as failing tests against
What the sweep found beyond themI generated every container nesting to depth three, crossed with the destination and title forms CommonMark's grammar distinguishes, and diffed the sensor's definition detection against markdown-it in 105 429 cases.
The last two are the ones I want to flag, because both are false passes and neither is a destination or a container marker:
After the change: 105 429 / 105 429, zero diverging forms. The change
All of it still runs through One decision beyond what you asked forYou asked for destination validation and iterative container stripping. The title grammar is neither, and I fixed it anyway — it is a false pass under the invariant this PR added, it shares the root cause and the code path, and shipping a round that fixed two thirds of one defect seemed worse than the alternative. Both title regexes are a one-character change ( The residuals you accepted are untouched: the definition-shaped line inside an HTML block still lands as a false failure, and the duplicate findings from markup-only lines are still the pre-existing Gates
|
v2 shipped 2.5.30 while this branch still declared 2.5.27. Scanned the open PRs by their actual AIDLC_VERSION diff rather than their titles: 2.5.31 (awslabs#535, awslabs#661, awslabs#686), 2.5.32 (awslabs#660, awslabs#687) and 2.5.33 (awslabs#689) are claimed, so this takes 2.5.34. The CHANGELOG entry was rebuilt from v2's file with this branch's block reinserted, so no upstream heading is lost. Its sensor-cache bullet now describes the engine-path match rather than the leaf-name one, and a new bullet covers the clean-filter binding. Coverage registry regenerated with the tool, not hand-edited.
apackeer
left a comment
There was a problem hiding this comment.
I checked the current head (01b03a95). The three exact variants from the
previous round are fixed, but the broader definition parser still violates the
new false-failure invariant. The first three cases below regress from v2
reporting claim block has no source tag to this head returning pass: true.
1. Invalid reference labels still exempt visible prose
referenceDefinitionLabel accepts any balanced bracket content and returns it
without validating CommonMark's label grammar
(core/tools/aidlc-sensor-claim-sources.ts:793-808). Consequently each of
these reader-visible lines is removed as a definition, and
isReferenceDefinitionBlock skips its block:
[]: /url
[ ]: /url
[a[b]]: /urlmarkdown-it in CommonMark mode renders all three as paragraphs. Focused
t247-style probes return pass: true, while current v2 fails all three.
Require a non-whitespace label, reject unescaped brackets, and cover the label
grammar rather than only destination/title grammar.
2. Definition context can swallow a second visible assertion
Both label collection and definition removal set mayContinueDefinition = true
after every accepted definition, even when that definition already has an
inline title (core/tools/aidlc-sensor-claim-sources.ts:828-836,
:841-854):
[evidence]: /url "title"
"This is an unsupported assertion."CommonMark renders the second line as a paragraph. This head removes both
lines, treats the block as definition-only, and returns pass: true; v2
reports the missing tag. Continuation eligibility needs to retain whether an
inline title was already consumed (and, for container definitions, the
container context).
3. Column and destination-boundary checks are still bypassable
claimBlocks trims block text before definition detection (:552-560,
:594-600), so a top-level indented code line loses the indentation that
referenceDefinitionLabel relies on at :795-796:
[evidence]: /urlThere is also no whitespace check between an angle-bracket destination and an
optional title at :800-806, so this visible paragraph is accepted:
[evidence]: <url>"title"markdown-it renders the first as an indented code block and the second as a
paragraph.
Finally, the bare-destination scanner stops only at code points <= 0x20
(:770-783), omitting the U+007F ASCII control. A line containing
[evidence]: foo<DEL>bar is therefore accepted even though CommonMark renders
it as a paragraph. All three probes return pass: true here and fail correctly
on v2. Preserve source indentation through the block skip, require
title-separating whitespace, and reject DEL.
4. The deliberately unsupported multiline destination flips to a false pass
The comment at core/tools/aidlc-sensor-claim-sources.ts:786-792 says treating
a destination deferred to the next line as prose is conservative. It is
conservative for removing the definition, but not for resolving shortcut
references:
The sole user is the author. [desc]
## Review
[desc]:
/urlCommonMark renders [desc] as a link, so its brackets are invisible. Because
referenceLabels misses this valid definition, visibleMarkdownLinkText
treats [desc] as literal source text (:819-838, :914-918); Review content
is excluded from claim inspection. The actual sensor returns pass: true.
Collect multiline definitions for document-wide resolution so the conservative
fallback does not invert in the reference-resolution path.
The PR also currently conflicts with v2 at 2.5.36; the next revision needs
to retain changelog entries 2.5.34 through 2.5.36 while rebasing the
2.5.37 trio.
Verification
STAMP: tests/logs/2026-08-03T20-14-18Z
TRACES: /tmp/pr686-latest/tests/logs/2026-08-03T20-14-18Z/*.log (183 files)
SUMMARY: tests/logs/2026-08-03T20-14-18Z/summary.txt + failures.txt - Result: PASS; Failed files: 0
RESULT: smoke+unit . 184 pass/0 fail . reds: none . live vars set: none . invariant grep hits: 0 (path-excluded)
The clean Linux slice completed with 4,541 assertions, including all 45 current
t247 cases. bun run check is clean (550 files), and
bun scripts/package.ts --check reports byte parity across all five harness
trees. Those gates do not cover the false-pass probes above, so I am keeping
this in changes-requested state.
01b03a9 to
e9c1b73
Compare
|
Latest review addressed on I reproduced the review as eight failing What changed
The sensor manifest and 2.5.37 changelog entry now state these boundaries explicitly. All five generated harness trees were regenerated. Gates
Windows disclosure for the last gate: nested |
|
Thanks, Iury. The latest head closes the cases from the previous review, and the definition-aware direction remains the right fix. The exact-head sensor slice passes all 10 focused files (262 assertions), package parity is clean across all five harnesses, and typecheck passes. Four CommonMark boundary probes still return
These are all in the same parser boundary this PR now owns. Could you add regression coverage and close those four false-pass paths? There is no need to rebase or renumber the branch. Current |
|
Four additional false-pass variants reproduce on the same head ( 1.
|
|
Thank you for the detailed follow-up and for providing concrete end-to-end fixtures. I reproduced all eight boundary probes against the reviewed head before changing the parser, then addressed them in The follow-up covers:
I also ran the exact fixtures through the real system path, rather than relying only on unit assertions:
Independent boundary checks also found two variants of the same false-pass path at HTML-block and asterisk-thematic-break interruptions. Both now fail closed and have focused regression coverage. Validation on the published head:
This follow-up commit changes only the authored sensor, its five generated projections, and This should now be ready for Leandro's second pass when convenient. Thank you again for the precise review. |
apackeer
left a comment
There was a problem hiding this comment.
Approved on exact head 48ec04da. The follow-up closes the eight CommonMark false-pass cases from the latest review; I found no new code blocker. Typecheck, lint, and generated-tree parity are clean.
STAMP: tests/logs/2026-08-06T01-21-02Z
TRACES: /home/ubuntu/src/aidlc-workflows/.claude/worktrees/pr-686-verify/tests/logs/2026-08-06T01-21-02Z/*.log (7 files)
SUMMARY: tests/logs/2026-08-06T01-21-02Z/summary.txt + failures.txt - Result: PASS; Failed files: 0
RESULT: smoke+unit related slice . 7 pass/0 fail . reds: none . live vars set: none . invariant grep hits: 0 (path-excluded)
The remaining DIRTY state, release-slot/changelog reconciliation, and distribution regeneration are integration mechanics for the rebase onto current v2.
|
LGTM. @leandrodamascena can you take a look also? |
The claim-sources sensor reported `claim block has no source tag` for claims that carry two tags with no separator, such as `[Q1][Q2]`, while the same claim passed with a space between them. `visibleMarkdownLinkText` treated any `[a][b]` as a Markdown reference link and consumed the pair before `SOURCE_TAG_RE` could see it. In CommonMark a reference link resolves only against a link reference definition the document carries; with no definition for the label, both bracket pairs render as literal text and stay visible to the reader. Reference resolution now consults the labels the document actually defines, which settles the three forms at once: - full `[a][b]` and collapsed `[a][]` pairs are text unless defined, so adjacent tags ground the claim again; - a shortcut `[Q1]` in a document that defines `[Q1]: <url>` is a link and grounds nothing, which is the mirror the sensor previously missed; - a paragraph holding only link reference definitions renders as nothing and is no longer treated as a claim block owing a tag. Labels are collected per document because that is a reference definition's scope, then handed to the per-block and per-line callers.
Two ways the sensor's definition detection disagreed with CommonMark, both letting content through that a reader can see is unsourced. A line only shaped like a definition, such as `[evidence]: some prose`, has no valid destination, so CommonMark renders it as the visible sentence it is. The sensor recognised it by shape alone and then skipped its whole block from inspection, turning a permissive read into a bypass: any assertion written in `[label]: text` shape escaped the sensor. CommonMark also accepts definitions inside block quotes and list items. The sensor only matched them at the top level, so `> [Q1]: <url>` left `[desc][Q1]` counted as two tags when the reader sees a link. Definition detection now requires a well-formed destination plus an optional valid title, and strips block-quote and list markers before testing the shape. Where the reading still cannot afford full CommonMark it now errs toward inspecting more: a destination deferred to the next line reads as prose rather than as a definition that could skip a block. The manifest states the invariant, so the next change to this code knows which way to fail.
A definition-shaped line with a malformed destination still registered a label and exempted its block from inspection, and container markers came off one quote and one list deep, so a definition nested the other way was missed. Both land as false passes, which the sensor's stated invariant forbids. Destination parsing now follows the grammar: an angle-bracket run has to close on its own line and hold no unescaped `<`, and a bare run ends at the first space or control character with its parentheses balanced. A parenthesised title may no longer hold an unescaped `(`. Container markers come off until the line stops changing, and a run of five or more spaces after a list marker is an indented code block rather than content. Checked against a reference CommonMark implementation over a generated sweep of every container nesting to depth three crossed with the destination and title forms the grammar distinguishes: 105429 cases, full agreement, where the previous reading diverged on four destination forms, four nestings, the title form and the indented-code-block form.
48ec04d to
377ffee
Compare
Closes #665.
The report
claim-sourcesreportedclaim block has no source tagfor claims that carry two tags with no separator —[Q1][Q2]— while the same claim passed with a space between them. @wowzoo's write-up already named the mechanism, and it holds:visibleMarkdownLinkTexttreats any[a][b]as a reference link and consumes the pair beforeSOURCE_TAG_REever sees it.I reproduced it against the shipped sensor before changing anything, using
tests/fixtures/intent-grounding/passingand a one-token mutation:[desc] [Q1](baseline)pass=true, 0 findingspass=true, 0 findings[desc][Q1]pass=false, claim block has no source tagpass=true, 0 findingsTwo things the report does not cover
Probing around the case turned up that the bug is parity-dependent, and that it has a mirror.
[desc][Q1][Q2]— three adjacent tags — passes today. Only the first pair is consumed, so the third tag survives and grounds the claim. The failure needs an even run; a document can carry adjacent tags in one paragraph and be fine, then fail in the next. That is worth a pinned test either way, so this PR adds one.The mirror is the more interesting half. A shortcut
[Q1]in a document that also defines[Q1]: <url>really is a link — the reader sees link text, not a tag — and the sensor accepts it as grounding. So the sensor was consuming brackets that CommonMark leaves as text, and keeping brackets that CommonMark turns into a link. Both are the same missing question: is this label defined?I checked the contract against a reference CommonMark implementation rather than reasoning from the spec alone (markdown-it in
commonmarkmode):[Q1][Q2], no definition[Q1][Q2]+[Q2]: url[Q1][], no definition[Q1], no definition[Q1]+[Q1]: urlThe change
Reference resolution now consults the labels the document actually defines. Full
[a][b], collapsed[a][], and shortcut[a]are each a link only when the named label is defined; otherwise the brackets stay literal and the tags remain visible. Labels are collected per document, because that is a reference definition's scope, and handed to the per-block and per-line callers — a block cannot see a definition that sits three paragraphs below it.One consequence fell out of the same rule and is included: a paragraph holding only link reference definitions renders as nothing, so it is no longer treated as a claim block owing a tag. Before this, adding a normal reference-style link to a deliverable produced a finding against the definition line itself.
Why not the fix the issue suggests
@wowzoo proposed exempting bracket groups whose content matches
SOURCE_TAG_REfrom reference-link handling, on the reasoning that a source tag is never a link label. That disambiguates the reported case correctly, and I want to be clear that it would work for it. I went a different way for two reasons.The premise does not hold in general: in a document that defines
[Q1]: <url>,[Q1]is a link label, and the reader sees a link. A content-based exemption would keep that reading a tag — the mirror described above — so the sensor would still disagree with the rendered document, just in the other direction. Resolving against the definitions the document carries answers both halves with the same rule, and it is CommonMark's own rule rather than a sensor-local exception list.The issue's other suggestion — collect tags before link-syntax stripping — would drop protections
t247currently pins: tags hidden in a link destination, in image alt metadata, in a reference title, or in an HTML attribute must not ground a claim, and each of those depends on the stripping running first.On the third option you raised — declaring adjacent tags invalid and saying so in the stage prose — I took the opposite branch, because CommonMark already renders
[Q1][Q2]as two visible tags and nothing in the prose asks the conductor to separate them. Pinning citation style would make the author responsible for a distinction the rendered document does not make. Your measurement carried weight here: 5 of 8 findings false in a singlepocrun, invisible to an author looking at a file that plainly shows the tags.Decisions I made, and would happily revisit
Scope beyond the reported case. I fixed the shortcut mirror rather than only the reported pair. The two are one rule and one code path, and leaving the mirror in place would mean the sensor keeps accepting a tag that renders as a link. If you would rather keep this PR to exactly the reported symptom, removing the shortcut branch is a two-line change and I will do it.
What counts as a definition. The sensor already recognises a definition by line shape (
withoutReferenceDefinitions+REFERENCE_TITLE_RE), which is more permissive than CommonMark — CommonMark requires the definition to start its own block, so a[label]: urlsitting on the second line of a paragraph is a lazy continuation and not a definition. I deliberately reused the sensor's existing notion instead of introducing a stricter one, so that label collection and definition removal cannot disagree with each other, and because tightening definition detection is a second behaviour change with its own risk. The residual gap is conservative in the direction that matters: the sensor may consider a label defined where CommonMark would not, and consume a pair it could have kept.t247's existing Markdown reference metadata case depends on this reading, and still passes. Happy to take the stricter rule as a follow-up if you want it.Version slot. Now 2.5.37: 2.5.31 merged while this sat (#671), and 2.5.34–2.5.36 are claimed by open PRs. I read each open PR's actual
AIDLC_VERSIONdiff rather than its title, since several titles trail their own bump. Glad to move it if you are sequencing differently.Checklist
If your change doesn't seem to apply, please leave them unchecked.
Test Plan
t247grew 26 → 34 tests at the time this was opened (45 today, after two review rounds): four cases pinning that undefined adjacent, triple-adjacent, collapsed, and mixed-run tags all ground a claim; three pinning that a defined full, collapsed, or shortcut reference grounds nothing; one pinning that a definition-only paragraph is not a claim block. Four of the eight fail onv2before the change.bun scripts/package.ts+bun scripts/package.ts --check→ green for all 5 harnesses.bun run check(typecheck + biome) → clean, 543 files.t68(version/CHANGELOG/badge sync at 2.5.37),gen-coverage-registry, andt247→ 74 tests, 0 failures. The coverage registry was regenerated withbun tests/gen-coverage-registry.ts, not hand-edited.t247,t227,t230,t234,t86,t68,gen-coverage-registry→ 171 tests, 1742 assertions, 0 failures;t92andt93(integration) → 60 tests, 223 assertions, 0 failures.tests/unit/tier does not complete on my Windows machine — I stopped it after ~30 minutes with no output, which matches the platform-specific failures I have hit on this repo before and is not specific to this change. The targeted runs above are the evidence I actually have; a Linux run of the full tier would be worth having before merge.Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.