fix(extraction): treat trailing blanks at EOF as an absent newline, not a miss (#1746)

[rarepops]

Fixes #1746

Root cause

#1610 suppressed the phantom parse_partial produced by a missing final newline, on the grounds that the grammar's MISSING terminator at EOF is zero-width: the parser consumed no source for it, so by construction nothing was dropped.

That suppression tested end == source_len exactly. Trailing blanks are extras owned by no node, so the terminator parks one byte short and the check misses it.

Dumping the parse tree for the reporter's three byte-exact controls confirms the mechanism:

fixture	source_len	MISSING node	flagged
ENTRYPOINT ["a"] + EOF	30	bytes[29,29) zero-width	yes (29 != 30)
ENTRYPOINT ["a"] + EOF	29	bytes[29,29) zero-width	no (29 == 29)
ENTRYPOINT ["a"] \n	31	none	no

This is exactly why neither a trailing blank nor an absent newline flagged alone: only the pair did.

Fix

Treat "at EOF" as EOF modulo a trailing blank run.

Every blank except newline qualifies, deliberately not a hand-picked subset. An earlier cut of this patch allowed only space/tab/CR and left \f and \v still flagged, which recreated in miniature the exact arbitrariness #1610 set out to kill: a form feed is no more content than a space. Newline stays excluded because a terminated final line produces no MISSING terminator at all.

The rule remains zero-width only. A width-bearing MISSING/ERROR at EOF is a genuine loss and is still flagged (a Makefile's unterminated final recipe really does vanish), as is any failure earlier in the file.

Tests

Added to tests/test_parse_coverage.c, mirroring the existing #1610 structure:

• dockerfile_trailing_blank_at_eof_not_flagged_issue1746 : the report's repro plus tab, vertical tab, form feed, blank-run and CR variants
• dockerfile_trailing_blank_then_newline_still_clean_issue1746 : pins the reporter's clean control from the other side
• real_error_before_eof_still_flagged_with_trailing_blank_issue1746 : guard, a genuine mid-file failure must not be swallowed just because the file ends in blanks
• width_bearing_error_at_eof_still_flagged_with_trailing_blank_issue1746 : guard, a width-bearing loss at EOF stays honest

Both guards fail if the suppression is widened too far.

Verification

End to end, using the issue's own command:

printf 'FROM scratch\nENTRYPOINT ["a"] ' > Dockerfile
codebase-memory-mcp cli index_repository --repo_path=. --mode=fast --name=repro

parse_partial_count goes 1 -> 0, with nodes: 8 unchanged, so the phantom flag is gone without anything dropping out of the graph. Controls stay at 0.

Gates run locally (Ubuntu 24.04 + gcc, ASan + UBSan, mirroring CI):

• Full suite: 7570 passed
• parse_coverage: 18/18, including all five pre-existing Dockerfile grammar flags stock .NET Dockerfiles as parse_partial #1610 tests
• clang-format-20 (CI's pin): clean
• cppcheck 2.20.0 (CI's pin): clean
• lint-no-suppress: clean

One caveat on the full-suite run: five cli install/activation tests failed locally with ancestor_directory_world_writable (mode 0777, uid 0). That is a Docker-on-Windows bind mount presenting mode 0777 and tripping the install activation guard, not a regression: re-running that suite from a container-native directory gives 282/282. Mentioning it in case it helps anyone else developing on a Windows host.

I have not been able to exercise the Windows CI leg locally. The change is byte arithmetic over the source buffer with no platform-dependent behaviour, so I would expect parity, but flagging it rather than assuming.

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[rarepops]

The test / test-windows-guards failure on this PR does not come from this change. Evidence below in case it saves someone a bisect.

What failed

error: the daemon did not accept the UI configuration; browser was not opened
RED (tests/windows/test_daemon_stability.py): section_crash_recovery failed
REGRESSION: 1 green guard(s) went red: tests\windows\test_daemon_stability.py

alongside three setup failures in the same run:

SETUP FAIL: ASCII baseline did not index: {... 'nodes': None, 'edges': None, 'definition_nodes': 5}
SETUP FAIL: index did not run:
SETUP FAIL: ASCII control did not index via CLI:
Guards skipped (precondition): 4

That is daemon crash-recovery and UI-config acceptance. This PR only changes zero-width MISSING-node handling in cbm_is_eof_terminator_miss / cbm_collect_error_regions in internal/cbm/cbm.c, plus tests/test_parse_coverage.c. It touches no daemon, CLI, UI or indexing-orchestration code.

The same failure on two unrelated branches

Identical signature, line for line, including the same three SETUP FAIL lines and the same Guards skipped (precondition): 4:

branch / PR	guards job	finished
#1768 test(hook-augment): pin worktree payload-cwd resolution	96507743615	2026-08-20 17:24Z
feat/index-stale-warning	96663906691	2026-08-21 04:42Z
this PR	96701005651	2026-08-21 07:43Z

Neither of the other two shares any code with this one.

This PR's content did not change between a pass and a fail

The branch was rebased onto successive bases. The patch is byte-identical in both runs (same per-file diff, internal/cbm/cbm.c +30/-8 and tests/test_parse_coverage.c +89/-0); only the parent differs:

run	commit	base	guards
32412481536	cbb456af	31819c9c	pass
32457481602	2888d3e3	65c7cf10	fail

I first suspected something in 31819c9c..65c7cf10, since 4a29f0da fix(cli): wait out a held startup transition instead of failing at 10s sits in exactly the startup-coordination area the guard exercises. That turns out to be wrong: #1768 hit the same failure about seven hours before 65c7cf10 landed, so it predates that range.

Across the last 11 completed pull_request runs that include this job, I count 9 pass and 1 fail on distinct branches, so it looks intermittent rather than base-dependent (PR #1777 also passed it on the new base at 07:01Z).

Local verification of the change itself

For what it is worth, on Ubuntu 24.04 + gcc with the ASan/UBSan build:

• parse_coverage: 18 passed, 0 failed (the five pre-existing Dockerfile grammar flags stock .NET Dockerfiles as parse_partial #1610 tests plus the new cases and two guards)
• extraction, grammar_regression, index_resilience: 327 passed, 0 failed
• lint-format (clang-format-20) and lint-cppcheck (cppcheck 2.20.0): clean

Happy to open a separate issue for the flaky guard if that is useful. I did not file one unprompted because blank issues are disabled and neither template really fits a CI-infrastructure flake, so I would rather follow whatever process you prefer.

[DeusData]

Thank you for the careful cross-branch attribution. Your evidence is useful and the parse-coverage diff is isolated from the failing daemon and UI guard.

Please do not open another issue for that historical signature right now. The startup-lock failure was addressed by #1772, and the remaining UI-configuration result is already tracked by #1796. Keeping those records together avoids a duplicate while this PR stays focused on the Dockerfile EOF false positive.

I also checked current main: cbm_is_eof_terminator_miss() only suppresses a zero-width missing node when its end byte equals the raw source length, so trailing blanks do expose the exact gap you described. Thank you for the binding controls and for not bundling the unrelated guard work.

[DeusData]

I owe you an apology before anything else.

You opened this on 20 August. It sat without a single comment, and on 29 August I merged #1853 — my own fix for the same issue, #1746 — without reviewing yours first and without crediting you. You were nine days ahead of me and I did not look. That is my mistake, not a process quirk, and I am sorry.

It is worse than a duplicate, because your version is the better one on the point that still matters.

What landed, and where it is still wrong

main now has (internal/cbm/cbm.c:823):

if (!source || strcmp(ts_node_type(n), "\n") != 0) {
    return false;
}
for (uint32_t i = end; i < (uint32_t)source_len; i++) {
    if (source[i] != ' ' && source[i] != '\t') {
        return false;
    }
}

Space and tab only. Which means the exact objection you wrote into your PR description still applies to the code that shipped:

An earlier cut of this patch allowed only space/tab/CR and left \f and \v still flagged, which recreated in miniature the exact arbitrariness #1610 set out to kill: a form feed is no more content than a space.

You were right, and main shipped the version you had already argued against. Reading that loop, three of your six fixtures should still flag today:

• ENTRYPOINT ["a"]\v
• ENTRYPOINT ["a"]\f
• ENTRYPOINT ["a"] \r — a CRLF file truncated to a bare CR

main's four #1746 tests cover space, space+LF, no-space, and CRLF on the first line. None of them reaches a \v, \f or trailing \r, which is exactly why the gap survived. Your cbm_is_blank_not_newline is isspace() minus \n — principled, complete, and not a hand-picked subset.

Your two guards are also additive: main has width_bearing_error_at_eof_still_flagged_issue1610, but neither guard in the with a trailing blank variant, which is the combination that actually distinguishes a correct widening from an over-eager one.

The one thing #1853 has that your patch drops

main gates on strcmp(ts_node_type(n), "\n") != 0, so only a MISSING newline token is ever suppressed. Your version drops that, so any zero-width MISSING at EOF followed by blanks qualifies — a missing } or ) landing there would be suppressed too. #1610 had the same looseness, so this is not a regression you introduced, but it is a real tightening worth keeping.

What I would like to ask of you

Rebase onto main and reduce this to the delta:

1. Keep main's ts_node_type(n) == "\n" guard.
2. Replace the ' '/'\t' test with your cbm_is_blank_not_newline.
3. Bring all six of your fixtures and both _with_trailing_blank guards.

That is a much smaller diff than what you have now, and the tests are the proof: rebased against main, your \v, \f and \r cases should go red before your one-line change and green after. That is a cleaner demonstration of the residual bug than anything I could write here.

Both files conflict with main as it stands, so a rebase is needed regardless.

If you would rather not spend more time on a PR that got treated this way — entirely fair — say so and I will land the delta myself with Co-authored-by: credit to you. But it is your finding and your argument, and I would prefer it landed as yours.

Thank you for this. The parse-tree dump with byte offsets for all three controls is the reason the mechanism was unambiguous, and I should have read it three weeks ago.

[rarepops]

Rebased onto current main and force-updated the branch at 572725e6.

The residual diff now follows the requested shape:

• retains main's ts_node_type(n) == \n guard;
• replaces the space/tab-only check with cbm_is_blank_not_newline for space, tab, vertical tab, form feed, and carriage return;
• brings the six trailing-blank fixtures plus both _with_trailing_blank safety guards.

Validation completed in the repository test image with GCC ASan/UBSan:

./build/c/test-runner parse_coverage
23 passed

git diff --check upstream/main...HEAD is also clean.

[DeusData]

That is exactly the delta — thank you for turning it round so quickly.

You kept main's ts_node_type(n) == "\n" guard and the exact-EOF fast path, and changed only the thing that was actually wrong:

-        if (source[i] != ' ' && source[i] != '\t') {
+        if (!cbm_is_blank_not_newline(source[i])) {

with the helper covering the full set — space, tab, vertical tab, form feed, CR. So \v, \f and a trailing CR stop producing a phantom parse_partial, and the tightening main gained in the meantime is preserved rather than reverted. +82/-4 against the +119/-8 you started with, which is the right direction for a diff that now expresses one idea.

CI is queued — our Actions pool is badly backlogged today (roughly one job at a time against 38 queued runs), so expect a wait that has nothing to do with this PR. I will merge on green.

And to say it once more plainly: you were right about the blank set from the start, in August, and main shipped the narrower version because nobody read your PR. Four of your changes have landed today. Thank you for staying with it.

[DeusData]

Your test-unix (ubuntu-24.04-arm) red is not yours — I checked it properly rather than waving it off, because at first glance it looked like it could be.

The failing assertion was real:

FAIL tests/test_pipeline.c:5550: cbm_store_count_edges_by_type(s, project, "HTTP_CALLS") == 1, expected 0

pipeline_local_fetch_shadow_not_classified_as_http — a locally-defined fetch() shadowing the global must not produce an HTTP_CALLS edge. And there is a mechanism by which your change could plausibly cause that: suppressing a phantom parse_partial means fewer ERROR regions, which could let extraction proceed on content it previously skipped, and produce an extra edge.

So I tested it instead of reasoning about it. Built the actual merge — current main plus this branch — and ran pipeline and parse_coverage: clean build, 287 passed, 0 failed, and pipeline_local_fetch_shadow_not_classified_as_http among the passes.

Two other things point the same way: that test's fixture (src/local_fetch.ts) ends with a trailing newline, so your EOF-blank suppression cannot alter how it parses at all; and the same CI log carries ERROR: repository build/ ancestor must be absent or a real directory, a runner-environment preflight fault we have seen kill entire boards this week — while also summarising 3121 passed, 0 failed, 1 skipped, which contradicts the FAIL line it printed.

Nothing for you to do. The change itself is exactly the delta I asked for and I am happy with it — main's ts_node_type(n) == "\n" guard kept, only the blank set widened via cbm_is_blank_not_newline, and the fixtures brought across. I will merge once a clean run lands.

[DeusData]

Merged as 6cca7688. Thank you — and thank you for turning the delta round the same day after the wait you had.

To restate the record plainly, because it matters more than the diff: you opened this on 20 August, and on 27 August I merged my own #1853 for the same issue — uncredited, and narrower. main shipped space-and-tab only, which is exactly the hand-picked subset your description had already argued against. You were right first, and the weaker version landed only because nobody read your PR. I have noted that on #1746 as well, so the history is not just in this thread.

The final shape is better than either original: main's ts_node_type(n) == "\n" guard kept, so only a MISSING newline token is ever suppressed, with your cbm_is_blank_not_newline widening the blank set to every blank except newline. +82/-4 against the +119/-8 you started with.

On the red you saw

Your test-unix (ubuntu-24.04-arm) failure was real-looking and I nearly could not rule it out from reading alone — suppressing a phantom parse_partial means fewer ERROR regions, which could plausibly let extraction reach content it had skipped and mint an extra HTTP_CALLS edge. So I built the actual merge locally and ran pipeline and parse_coverage: 287 passed, 0 failed, with the failing test among the passes. The re-run has since come back clean too.

That leg was also carrying a known runner fault, and it reported 3121 passed, 0 failed in the same log that printed the FAIL — which is how I learned that fault can leave a leg producing misleading results rather than simply dying.

Four of your changes have landed today: #1779, #1799, #1802 and this. Thank you for staying with it.