Skip to content

fix: ground claims whose source tags are written adjacently (2.5.37) - #686

Merged
apackeer merged 5 commits into
awslabs:v2from
iuryeng:fix/665-adjacent-source-tags
Aug 7, 2026
Merged

fix: ground claims whose source tags are written adjacently (2.5.37)#686
apackeer merged 5 commits into
awslabs:v2from
iuryeng:fix/665-adjacent-source-tags

Conversation

@iuryeng

@iuryeng iuryeng commented Jul 30, 2026

Copy link
Copy Markdown

This description covers the change as opened. Two review rounds have landed since, both
reported here in the thread: definition detection now follows CommonMark's destination, title
and container grammar, checked against a reference implementation over a generated sweep.

Closes #665.

The report

claim-sources reported claim block has no source tag for 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: visibleMarkdownLinkText treats any [a][b] as a reference link and consumes the pair before SOURCE_TAG_RE ever sees it.

I reproduced it against the shipped sensor before changing anything, using tests/fixtures/intent-grounding/passing and a one-token mutation:

claim text before after
[desc] [Q1] (baseline) pass=true, 0 findings pass=true, 0 findings
[desc][Q1] pass=false, claim block has no source tag pass=true, 0 findings

Two 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 commonmark mode):

input CommonMark sensor before
[Q1][Q2], no definition literal text consumed
[Q1][Q2] + [Q2]: url link consumed ✓
[Q1][], no definition literal text consumed
[Q1], no definition literal text kept ✓
[Q1] + [Q1]: url link kept

The 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_RE from 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 t247 currently 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 single poc run, 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]: url sitting 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_VERSION diff 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.

  • I have reviewed the contributing guidelines
  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented

Test Plan

  • t247 grew 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 on v2 before 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, and t247 → 74 tests, 0 failures. The coverage registry was regenerated with bun tests/gen-coverage-registry.ts, not hand-edited.
  • Regression across everything that reaches the sensor: t247, t227, t230, t234, t86, t68, gen-coverage-registry → 171 tests, 1742 assertions, 0 failures; t92 and t93 (integration) → 60 tests, 223 assertions, 0 failures.
  • What I could not run: the full 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.
  • The sensor's manifest documents the rule the tests pin, so a reader hitting this does not have to infer it from behaviour.

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.

@apackeer apackeer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CommonMark 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.invalid

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

@iuryeng

iuryeng commented Jul 31, 2026

Copy link
Copy Markdown
Author

Both blockers reproduce, both are fixed. New head: 128d0b0a, rebased onto v2 at 2.5.30 — the slot stays 2.5.31 and the PR is MERGEABLE again.

Thank you for running the Linux tier I could not run, and for the three extra probes. Knowing that ![Q1] now grounds where the old sensor consumed it silently is exactly the kind of confirmation I could not get from here.

You are right about the sign, and that is the part I got wrong

I 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 [label]: text walks past the sensor. That is a false pass introduced by this PR, in a sensor whose whole job is to refuse them, and it should not have shipped behind a sentence I had stopped checking.

I reproduced both of your fixtures as failing tests at 11e9157 before touching the code — three tests, three failures, exactly the behaviour you describe.

What changed

Definition detection now requires a well-formed destination, plus an optional valid title on the same line. [evidence]: this is an unsupported assertion has a destination of this followed by prose that is not a title, so it is not a definition: it neither registers a label nor exempts its block, and the block is inspected like any other claim.

Label collection is container-aware. Block-quote and list markers come off before the shape test, so > [Q1]: <url> and - [Q1]: <url> register, and [desc][Q1] correctly grounds nothing in those documents.

Both live in referenceDefinitionLabel, which is the single place withoutReferenceDefinitions, isReferenceDefinitionBlock, and referenceLabels all consult — so the two directions cannot drift apart again.

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 invariant

Added to the manifest, near enough to your wording that I hope it reads as yours:

Where this reading cannot afford full CommonMark, the divergence must land as a false failure and never as a false pass: the sensor may ask for a citation the document did not owe, but it must not let unsourced or invisible-tag content through.

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

Gates

  • t247 34 → 37 tests, all green; the three new ones fail on 11e9157.
  • bun run check clean (544 files), bun scripts/package.ts --check byte parity across all five dists, t68 + gen-coverage-registry green at 2.5.31 → 77 tests, 0 failures locally.
  • Rebase: v2 moved to 2.5.30 while this sat, so the CHANGELOG block was repositioned above it and the 2.5.31 trio re-verified. I rebuilt the CHANGELOG from v2 plus the extracted block rather than resolving the conflict in place, and checked that no v2 heading was lost.

The full Linux tier is the one thing I still cannot verify from here — I would appreciate the re-run you offered.

iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Jul 31, 2026
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

Copy link
Copy Markdown
Contributor

I checked the current head (128d0b0a). The three exact review fixtures are fixed and covered, and the false-failure invariant is now documented. Two variants still violate that invariant, so I do not think the underlying review findings are fully closed yet.

1. Malformed single-token destinations still bypass inspection

referenceDefinitionLabel treats any non-whitespace token as a valid destination (core/tools/aidlc-sensor-claim-sources.ts:755-759). For example:

[evidence]: <broken

CommonMark renders this as visible prose (<p>[evidence]: &lt;broken</p>), but the sensor accepts it as a definition, removes it, and returns pass: true. A focused t247 regression expecting claim block has no source tag fails with Expected: false, Received: true. An unbalanced destination such as [evidence]: /foo(bar has the same issue. The new test at tests/unit/t247-claim-sources-sensor.test.ts:547-560 covers the reviewed multi-word sentence, but not destination grammar itself.

2. Nested container stripping remains order-dependent

withoutContainerMarkers strips block quotes once and then list markers once (core/tools/aidlc-sensor-claim-sources.ts:735-738). Thus > - [Q1]: ... works, while this valid inverse nesting does not:

- > [Q1]: https://example.invalid

[desc][Q1]

CommonMark resolves [desc][Q1] as a link, but the sensor misses the definition and returns pass: true. A focused t247 regression reproduces that false pass. The current tests cover unnested quote and list forms only (tests/unit/t247-claim-sources-sensor.test.ts:563-587).

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

@iuryeng
iuryeng force-pushed the fix/665-adjacent-source-tags branch from 128d0b0 to 01b03a9 Compare August 2, 2026 22:33
@iuryeng iuryeng changed the title fix: ground claims whose source tags are written adjacently (2.5.31) fix: ground claims whose source tags are written adjacently (2.5.37) Aug 2, 2026
@iuryeng

iuryeng commented Aug 2, 2026

Copy link
Copy Markdown
Author

Both variants reproduce and are fixed. New head: 01b03a95, rebased onto v2 at 2.5.33.

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 reported

I reproduced both as failing tests against 128d0b0a before touching the code — three reds, matching your description exactly.

fixture 128d0b0a 01b03a95
[evidence]: <broken pass=true, block skipped pass=false, claim block has no source tag
[evidence]: /foo(bar pass=true, block skipped pass=false, claim block has no source tag
- > [Q1]: … + [desc][Q1] pass=true pass=false, claim block has no source tag

What the sweep found beyond them

I 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 commonmark mode — asking of each case the only question that matters here: does [desc][Q1] come out as a link?

105 429 cases. 128d0b0a agreed on 73 784 of them, across 117 diverging forms. Grouped by root cause:

cause example direction in your report
destination grammar [Q1]: <broken, [Q1]: /foo(bar false pass yes
destination grammar [Q1]: /foo)bar, [Q1]: <a<b> false pass no
destination grammar [Q1]: <a b> false fail no
container nesting - > [Q1]: … false pass yes
container nesting - - , > - > , - 1. false pass no
title grammar [Q1]: /url (ti(tle) false pass no
list indentation - [Q1]: … (five spaces) false pass no

The last two are the ones I want to flag, because both are false passes and neither is a destination or a container marker:

  • A parenthesised title may not hold an unescaped (. REFERENCE_TITLE_RE allowed it, so [evidence]: /url (ti(tle) registered a label and exempted its block. That regex predates this PR, but it only became a bypass when I added the block skip — the same sign inversion you caught last round, in the one place I had not looked.
  • Five or more spaces after a list marker start an indented code block inside the item, not content. withoutContainerMarkers — which I introduced last round — took the marker off with a greedy [ \t]+ and read the code block as a definition.

After the change: 105 429 / 105 429, zero diverging forms.

The change

referenceDestinationEnd implements the grammar: an angle-bracket destination has to close on its own line and hold no unescaped <; a bare destination ends at the first space or control character with its parentheses balanced. The title regexes reject an unescaped ( inside (...). withoutContainerMarkers strips until the line stops changing, so quote and list nest in either order and to any depth, and the list marker only comes off for a run of one to four spaces.

All of it still runs through referenceDefinitionLabel, so label collection, definition removal and the block skip cannot drift apart.

One decision beyond what you asked for

You 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 ([^)\\][^()\\]); happy to pull it out into its own PR if you would rather review it separately.

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 v2 behaviour.

Gates

iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Aug 3, 2026
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 apackeer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]]: /url

markdown-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]: /url

There 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]:
/url

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

@iuryeng
iuryeng force-pushed the fix/665-adjacent-source-tags branch from 01b03a9 to e9c1b73 Compare August 3, 2026 21:26
@iuryeng

iuryeng commented Aug 3, 2026

Copy link
Copy Markdown
Author

Latest review addressed on e9c1b732, rebased onto current v2 (046a9a6). The PR is mergeable again, retains the 2.5.34–2.5.36 changelog entries, and keeps this change in the 2.5.37 slot.

I reproduced the review as eight failing t247 cases before changing the parser: the three invalid labels, missing title whitespace, DEL in a bare destination, top-level indented code, the post-inline-title assertion, and the multiline destination used for document-wide resolution. All eight now pass.

What changed

  • Label grammar: definitions now require a non-whitespace label, reject unescaped brackets, and enforce CommonMark's 999-character limit.
  • Title continuation: parsing retains whether an inline title was already consumed, so the following quoted assertion cannot be swallowed. Continuation is also restricted to the same stripped container context.
  • Columns and destination boundaries: claim blocks preserve source indentation through the definition-only skip; optional inline titles require separating spaces/tabs; bare destinations stop at U+007F as well as U+0000–U+0020.
  • Multiline destinations: document-wide label collection recognizes a destination on the following line, so shortcut/full references resolve correctly. The definition-only block skip remains deliberately line-based: an unsupported multiline shape may false-fail, but cannot exempt visible prose.

The sensor manifest and 2.5.37 changelog entry now state these boundaries explicitly. All five generated harness trees were regenerated.

Gates

  • t247: 53/53, 226 assertions.
  • t247 + t68 + t86 + t227 + t230 + t234 + smoke t86: 168/168, 832 assertions.
  • t92 + t93: 60/60, 223 assertions.
  • bun run typecheck: green.
  • bun run lint: 556 files, no findings.
  • bun tests/gen-coverage-registry.ts --check: fresh, guards green, ratchet held.
  • package.ts --check: byte parity across all five harnesses.

Windows disclosure for the last gate: nested spawnSync(bun, ...) returned a status-only failure with no stderr on this shell. I reran package generation/check using process.execPath for that nested invocation, then reverted the local workaround; scripts/package.ts is unchanged in the PR. I have not rerun the full smoke+unit tier on Linux after this revision, so I would appreciate the same clean Linux slice on the new head.

@apackeer

apackeer commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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 pass: true with zero findings in the prohibited false-pass direction:

  1. A multiline destination inside a list item, for example - [evidence]: followed by an indented /url, resolves the reference in CommonMark but loses its list context in referenceLabels.
  2. A reference title spanning physical lines resolves in CommonMark, but the parser requires the title to close on its first line.
  3. A valid 500-code-point astral label is rejected because label.length counts 1,000 UTF-16 code units; the rendered link is then mistaken for a visible [Q1] source tag.
  4. A bare destination nested 33 parenthesis levels is not a CommonMark definition, but the parser accepts it without the 32-level limit and the definition-only block skip removes the visible unsourced line.

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 v2 now owns 2.5.37; maintainers can preserve that changelog entry, select the release slot, regenerate the harness trees, and leave out the unrelated README tool-count change during integration. Once the parser cases are fixed, this should be ready for Leandro's second pass.

@apackeer

apackeer commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Four additional false-pass variants reproduce on the same head (e9c1b732) beyond the four in my preceding comment. I checked each end to end against the sensor, current v2, and markdown-it's CommonMark rendering. In every case this head returns pass: true with zero findings, v2 reports claim block has no source tag, and CommonMark renders the alleged source tags as link syntax or leaves the unsupported assertion visible.

1. ) ordered-list items let title continuation cross item boundaries

1) [evidence]: /url
2) "This is an unsupported assertion."

CommonMark renders two list items; the second sentence is visible prose. isListItem recognizes ordered markers ending only in . (core/tools/aidlc-sensor-claim-sources.ts:526-527), while containerLine recognizes both . and ) (:758). The two items therefore remain one claim block, the second item has the same synthetic list context and matches REFERENCE_TITLE_RE (:923-935), and the definition-only skip removes the entire block (:1058-1059).

2. A same-line definition indented within a parent list item is missed

- [desc]

    [Q1]: /url

This is an unsupported assertion. [desc][Q1]

CommonMark hides the indented definition inside the list item and resolves [desc][Q1] as a link whose visible text is only desc. containerLine tracks markers only on the current physical line (:748-767), so the definition retains four leading spaces and the start > 3 check rejects it (:891-895). The unresolved-reference fallback then treats [desc] as a visible source tag and passes the unsupported assertion.

This is distinct from the multiline-destination-in-a-list case above: the destination and label are on one line, but the definition itself is continuation content of an existing list item.

3. Multiline reference labels are not collected

## Review
[foo
bar]: /url

## Probe
This is an unsupported assertion. [desc][foo bar]

CommonMark permits the line ending in the label and normalizes it to a space, so the full reference resolves and the reader sees no [desc] tag. referenceLabelEnd is line-local (:816-826), so document-wide label collection misses the definition. With the definition in excluded Review content, the visible Probe assertion is the only inspected block and incorrectly passes.

4. Label normalization needs Unicode full case folding

[ss]: /url

This is an unsupported assertion. [desc][]

CommonMark resolves this reference because label matching uses Unicode case folding ( folds to ss). normalizedReferenceLabel uses JavaScript toLowerCase() (:857-860), which produces ß, so the sensor misses the match and counts [desc] as literal evidence.

The last case is separate from the 500-code-point astral-label length bug: one is label length in UTF-16 code units, the other is label equivalence.

These four also violate the manifest's false-failure-only invariant. I would add them to the same regression set before the next review pass.

@iuryeng

iuryeng commented Aug 6, 2026

Copy link
Copy Markdown
Author

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 48ec04da.

The follow-up covers:

  • multiline destinations inside list items;
  • reference titles spanning physical lines;
  • the 500-code-point astral-label limit;
  • the 32-level bare-destination parenthesis limit;
  • ordered-list siblings using ) markers;
  • definitions indented as continuation content of a parent list item;
  • multiline reference labels and whitespace normalization;
  • Unicode full case folding for reference-label equivalence.

I also ran the exact fixtures through the real system path, rather than relying only on unit assertions:

  • reviewed head e9c1b732: 8/8 returned pass: true with zero findings;
  • markdown-it 15 CommonMark rendering: the unsupported content remained visible in 8/8;
  • updated sensor: 8/8 returned pass: false, each with one claim block has no source tag finding;
  • real aidlc-sensor fire claim-sources dispatcher: 8/8 emitted SENSOR_FIRED followed by SENSOR_FAILED, with no SENSOR_PASSED event.

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:

  • focused t247: 79/79 tests, 346 assertions;
  • related 10-file unit/smoke/integration slice: 288/288 tests, 2,223 assertions;
  • typecheck: clean;
  • lint: 556 files, no findings;
  • coverage registry: fresh, guards green, ratchet held;
  • package.ts --check: byte parity across all five harness distributions;
  • git diff --check: clean.

This follow-up commit changes only the authored sensor, its five generated projections, and t247. I intentionally did not rebase or renumber the branch, and did not alter README, CHANGELOG, version, or manifest files in this follow-up, following your integration guidance.

This should now be ready for Leandro's second pass when convenient. Thank you again for the precise review.

@apackeer apackeer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@apackeer

apackeer commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

LGTM. @leandrodamascena can you take a look also?

iuryeng added 5 commits August 7, 2026 22:00
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.
@apackeer
apackeer force-pushed the fix/665-adjacent-source-tags branch from 48ec04d to 377ffee Compare August 7, 2026 23:21
@apackeer
apackeer merged commit 8c60e1a into awslabs:v2 Aug 7, 2026
3 checks passed
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.

2 participants