Skip to content

fix(discover): detect extensionless scripts by shebang - #1203

Open
ibaldr89 wants to merge 1 commit into
DeusData:mainfrom
ibaldr89:fix/extensionless-shebang-language
Open

fix(discover): detect extensionless scripts by shebang#1203
ibaldr89 wants to merge 1 commit into
DeusData:mainfrom
ibaldr89:fix/extensionless-shebang-language

Conversation

@ibaldr89

@ibaldr89 ibaldr89 commented Jul 21, 2026

Copy link
Copy Markdown

Summary

  • fall back to a bounded first-line shebang probe when filename/special-filename/extension detection returns unknown
  • recognize direct interpreter paths plus /usr/bin/env and env -S for common supported scripting languages
  • preserve filename/extension precedence and fail closed on malformed, NUL-containing, truncated, or unreadable first lines
  • add production-discovery regressions for valid, conflicting, malformed, binary, env-assignment, and boundary cases

Fixes #1199

Why

Executable source scripts commonly omit a filename extension. Before this change, files such as:

#!/usr/bin/env python3

def extensionless_probe():
    return 42

were silently absent from the graph even though Python is supported. Full indexing did not change that because language detection was filename-only.

Behavior

Existing detection remains authoritative. Shebang detection runs only when cbm_language_for_filename() returns CBM_LANG_COUNT.

Supported interpreter basenames:

  • Python: python, python2, python3, dotted numeric versions such as python3.12
  • shell: sh, bash, dash, ksh, zsh
  • JavaScript: node, nodejs
  • ruby, perl, php, lua

The probe reads at most 255 bytes plus one EOF/truncation probe byte, rejects embedded NUL, read errors, overlong first lines, unsupported env option/assignment shapes, and arbitrary prefix matches such as python-wrapper.

Related work checked

No existing issue or PR found in the checked open/closed history implements shebang-based language fallback for extensionless source files.

Testing

  • New tests added
  • Focused discovery suite: 107 passed under ASan/UBSan
  • CI lint stack: clang-format 20 + cppcheck 2.20 + NOLINT policy passed
  • GitHub Actions full matrix: 20/20 checks passed, including Linux x86/ARM, macOS, Windows, TSan, CodeQL, DCO, and lint
  • Real-repository regression: 7/7 extensionless entrypoints indexed versus 0/7 before; expected function and call graph became queryable
  • Independent exact-tree review: approved for commit

Full suite comparison in the same isolated Docker environment:

  • candidate: 6198 passed, 0 failed
  • clean main: 6180 passed, 0 failed
  • both runs exit non-zero on the same pre-existing 61-byte LeakSanitizer finding in tests/test_httpd.c:358; no candidate-only failure was introduced

Signed-off-by: ibaldr89 <163388887+ibaldr89@users.noreply.github.com>
@ibaldr89
ibaldr89 requested a review from DeusData as a code owner July 21, 2026 23:42
@DeusData DeusData added the bug Something isn't working label Jul 22, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 22, 2026
@DeusData DeusData added parsing/quality Graph extraction bugs, false positives, missing edges ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 22, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks — acknowledged here too, and I have replied in detail on #1199 confirming the underlying bug on current main.

Short version of that confirmation: detect_file_language() returns at src/discover/discover.c:653-655 before any content inspection, and every content-based path in that function keys off an extension, so an extensionless file can never reach one. Your diagnosis was exactly right.

Queued for review. The branch is CONFLICTING against main — a rebase before I read it would be ideal.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — the implementation here is careful, and I want to say what I checked before raising anything: the probe is bounded and read-only, it uses cbm_fopen correctly, it fails closed on error, NUL bytes and truncation, the interpreter list is a fixed allowlist, nothing is executed, and CRLF is handled. The env/-S/assignment/option rejections are conservative in the right direction — false negatives rather than false positives. The 18 tests include negative and vacuity-guarded cases and they bind.

Two things before it can land.

1. It needs a rebase — it currently conflicts with main.

2. One behavioural question I would like your view on. The description says extension detection stays authoritative, but the fallback fires whenever cbm_language_for_filename() returns unknown — which includes files with an unknown extension, not only extensionless names. So data.conf beginning with a bash shebang would now index as Bash. That may well be what you intended, but it is broader than the stated contract.

There is a cost attached to that breadth: in FULL mode every unknown file gets an open and a read. ALWAYS_IGNORED_SUFFIXES keeps binaries out, but README, LICENSE, .txt, .zip and lockfiles would all be probed, and this project routinely indexes repositories in the millions-of-files range where that is measurable.

Either resolution is fine by me: gate the probe to genuinely extensionless basenames, or keep the breadth and say explicitly in the description why unknown extensions should participate and why the discovery I/O is acceptable at scale. I would just rather that be a decision than a side effect.

Worth noting for transparency: #1199 was filed by you a day before this PR, so the direction has not been through maintainer triage — but the fit is good. Finding code the graph currently misses is exactly the kind of coverage gap we want closed.

@DeusData

Copy link
Copy Markdown
Owner

This is the best-engineered contribution I have reviewed in this backlog sweep, and I want to be specific about why rather than just saying so. It needs one rebase and then I would like to merge it.

I measured the cost concern empirically rather than estimating it, because "open and read every candidate file" is exactly the kind of change that quietly costs real time at scale. On the actual 94,851-file Linux kernel checkout:

  • ~5,066 files get probed — 5.3% of the repo. Everything else is filtered first: Makefile (×3,193) and Kconfig (×1,828) are filename-mapped and never touched, and gitignore, cbmignore, skip-lists and the size cap all run before the probe.
  • ~15,200 extra syscalls, three per candidate.
  • Well under half a second warm-cache against a ~260 s index — under 0.2%. Even the pathological case (every file unknown) lands at a second or two.

And the over-inclusion question came out equally clean: 301 candidates have a shebang, ~294 newly indexed — 196 sh, 67 bash, 18 python3, 13 perl, all genuine scripts in scripts/ and tools/ that were previously invisible to the graph. No junk. LICENSE and README have no shebang so they are never touched; vendor/vendored are always-skip so blobs are never probed; compiled artefacts do not start with #!. That is +0.31% files, all of them real source.

The fail-closed discipline throughout is exemplary. Every ambiguous input declines rather than guessing:

  • a bounded 255-byte read plus a one-byte EOF probe to distinguish "short file" from "truncated first line" — a genuinely clever detail;
  • embedded NUL before the newline → treated as binary, declined;
  • env NAME=value prog and option tokens rejected, since env would set a variable rather than run that token;
  • strict per-component digit matching on Python version suffixes, so python3. and python3..12 are rejected and python-wrapper does not match by prefix;
  • UTF-8 BOM before #! declines — which is correct, because the kernel refuses BOM'd shebangs too. Matching exec semantics rather than being liberal is the right instinct.

You used cbm_fopen throughout — including in the test helper — without being told. That rule exists because raw fopen breaks non-ASCII paths on Windows, and it is one people usually have to be reminded of. "rb" is also the right mode, with \r handled explicitly.

The tests are what this repo actually asks for. Eighteen of them, running end-to-end through the production cbm_discover walk rather than the unit function, red without the fix. Ten negative cases assert the file stays unindexed. Precedence is pinned both ways (weird.py with a bash shebang stays Python; Makefile with a python shebang stays Makefile). And the shebang_probe helper asserts setup success separately so a negative expectation cannot pass vacuously — that is precisely the discipline we care about, and most contributions miss it.

What is needed: a rebase. The branch conflicts with main, but the conflict is mechanical — one call-site line plus a self-contained function, and the early-return branch it hooks into is unchanged. The test_discover.c suite-list churn is mechanical too. Your PR body's "20/20 checks passed" predates the conflict, so CI needs re-establishing on the rebased head.

One thing to look at after rebasing, which is new since you branched: main gained cbm_discover_count_bounded, a deadline-bounded count path that also calls detect_file_language — so the probe cost now lands inside the coverage-count budget too. Counts staying consistent with real discovery is correct behaviour, and at these numbers the impact should be small, but it is worth one check on a very large repo.

One optional follow-up, not for this PR: the interpreter table is conservative and misses several languages we already support — awk/gawk (four real Linux files decline today), Rscript, tclsh, fish, julia, elixir, groovy, pwsh, deno/bun. Under-inclusion only, so it is safe as a v1, and a natural second PR.

Rebase and ping me; I will re-diff and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(discover): extensionless scripts with valid shebangs are silently skipped

2 participants