[shounakjoshi88-a11y]

Closes #83 (part of #76).

Changed behavior

By Go convention pkg/testdata/** is read by the tests in pkg: the go tool runs a package's tests with the package's directory as the working directory. DiffCI treated every testdata/ edit as a full-validation fallback, so internal/parser/testdata/case1.json reran the whole suite instead of just go test ./internal/parser.

Changes under <dir>/testdata/ of a package that reported _test.go files now select that package's tests (new GO_TESTDATA impact reason) instead of falling back — the same selection the worked-out convention implies.

The route is refused (and the previous full-validation fallback stands) when any of these hold:

Refusal	Reason
No discovered package owns the parent directory, or it has no _test.go files	byOwner is built from profile.goTestPackages; legacy cached profiles without that evidence fail closed
A _-prefixed or .-prefixed directory lies on the way to testdata or below it	keeps the discovery-excluded fallback (_data, .data, _hidden, …)
Another package's _test.go file names the changed path or its testdata directory by reference	the issue's "../parser/testdata" literal example; checked through the existing reference machinery (contentNamesPath), so ../p/data/... never matches p/testdata/...
The file is a go:embed input	embedding makes it a production runtime input, not only test input — the embed route and fallback win
A listed _test.go file's contents cannot be read	an unclearable sibling reader blocks the selective route (fail closed)

One supporting fix: declared inputs on a GO repository never resolved any tests (profile.tests is a JS-runner notion and is empty for Go profiles), so any declaration naming a Go path was always "stale" and forced the very fallback it was meant to override. The go adapter's reported _test.go files now join the declared-test candidates, behind globs the repository names — so the inputs workaround the issue's last Done-when bullet depends on actually overrides the fallback, and its declared tests union with the package-owned selection.

Fixture

tests/repo/go-runtime-inputs.test.ts (now 51 cases) exercises the real graph → impact → planner pipeline with synthetic go list metadata, no Go toolchain needed:

• p/testdata/settings.json selects ./p (p/value_test.go), plans SELECTIVE, skips service and unrelated
• a sibling package's test naming ../p/testdata/settings.json or ../p/testdata (the issue's example form) keeps full validation
• ownerless orphan/testdata/…, _scratch/p/testdata/… and p/testdata/_hidden/… fall back
• a legacy cached profile without goTestPackages fails closed (both via deletion and via the GraphCache round-trip)
• embedded testdata keeps full validation even with repository contents wired exactly like src/analyze.ts does (the pre-existing embedded-testdata canary keeps passing)
• declared inputs union with the package-owned tests (DECLARED_INPUT + GO_TESTDATA in one SELECTIVE plan)

Validation results (Node 24.12, Windows 11)

• npm ci — 0 vulnerabilities
• npm run typecheck — clean
• npm test — 791 passed / 8 failed / 0 skipped (799). Every failure is in tests/action-verify.test.ts, which fails the same 8/8 on the unmodified tree on this machine (verified by stashing this change and re-running the file) — they are spawn/exit-code assertions and are unrelated to this change; CI on Linux runs the same file green on main.
• npm run build — clean
• npm run audit:boundary — "Release boundary passed: 331 reviewed files"

Unsupported cases (still full validation)

• testdata claimed by a sibling _test.go file whose contents cannot be read
• testdata below a package without _test.go files, and profiles cached before this change
• embedded testdata (production runtime input)
• reads that are not literal references: dynamic path construction (filepath.Join(dir, name)), and directories read through non-literal aliases — the reference check is deliberately literal-only, per the issue's scope

Signed-off-by: Shounak Joshi shounakjoshi88@gmail.com

[adityankale190895]

Thanks for this — the scope matches #83 closely, the refusal table is clear, and the declared-inputs fix for Go profiles is a real bug fix. Typecheck is clean and all 51 cases in tests/repo/go-runtime-inputs.test.ts pass locally.

One blocker, though: the cross-package check can let a test be skipped when it should run, which is the failure DiffCI must not have.

1. Non-test Go readers are not checked (blocker, reproduced)

goPackageTestDataOwnerFor only scans other packages' _test.go files. A shared helper in an ordinary .go file (the common testutil/golden.go pattern) can read another package's testdata on behalf of its callers' tests. Reproduction, appended to go-runtime-inputs.test.ts:

test("non-test Go helper naming testdata", async () => {
  const files = {
    "p/testdata/settings.json": "expected",
    "service/service.go": 'package service\nimport "os"\nfunc Read(string) ([]byte, error) { return os.ReadFile("../p/testdata/settings.json") }\n',
  };
  await fixture(files, (graph, root) => {
    const { impact, plan } = planChange(graph, { path: "p/testdata/settings.json", changeType: "modified" }, { repoPath: root, fileContents: files });
    // actual on this branch: SELECTIVE ["p/value_test.go"], fallbackRequired false
    // service/service_test.go calls Read() and is skipped; on main this is FULL
  });
});

Suggestion: scan every Go file of other packages (e.g. profile.goDependencyPaths.sources plus the test files), not only _test.go, and add this case as a fixture.

2. Non-Go readers bypass reference resolution (likely, not reproduced)

The GO_TESTDATA branch in processChange (src/repo/impact.ts, around the new early return) runs before the general non-code reference resolution. Previously the Go fallback made that moot; now a JS/Python test in a mixed repo, or a Go test in an excluded nested module (goExcludedModuleRoots), that names p/testdata/... would no longer be found. Either let the normal reference resolution run as well (union rather than return), or fall back when any reader outside the reported Go packages names the path or its testdata directory.

3. Repeated reads (performance)

For each changed testdata path the loop re-reads every other package's test files through an uncached readRepositoryFile. A golden-file refresh touching hundreds of files in a large Go repo becomes changed-paths × test-files reads. Reading each candidate reader once per analysis and reusing the contents would keep this linear.

4. Minor

The 8 action-verify.test.ts failures you saw are Windows-local and also fail on main; we'll confirm with the Linux CI run.

Happy to re-review once 1 and 2 are addressed — 1 with the fixture above, 2 with at least one non-Go reader fixture.

[shounakjoshi88-a11y]

Pushed becdd40 addressing all three review points (branch fix/go-package-testdata-selection):

1. Ordinary non-test Go sources as cross-package readers (fixed): the route now scans every package's full source list, not only its _test.go files. The current adapter puts a package's test files into sourcePaths, so a package's members are exactly its goDependencyPaths.sources; the test-file grouping remains only as the stand-in for legacy cached profiles that lack source metadata. Disqualification reads memory via the same contentNamesPath boundary checks as before, with one addition: Go import clauses are excluded from the scanned text. An import "example.com/project/p" names the package, not the directories inside it — without that exclusion the suffix-name rule (unknown working directory) turned every sibling package's import into a false disqualifier.

2. Readers outside every go-reported package (fixed): files no package reported (nested-module tests, other-language tests, ordinary .go files excluded from package discovery) are collected into an outside-pool at map time — repository inventory minus all go-reported sources and tests, filtered to .go/test-matcher files — and any reference from that pool disqualifies the selective route: those tests run with a different working directory, so the package-owned convention cannot hold. The pool explicitly excludes the owners' own reported sources so the owner package's internal references cannot defeat its own route.

3. Read amplification (bounded): all requested reads stay on readRepositoryFile, which memoizes one read per file per analysis; the outside pool only contains files ending .go or matching the profile's test matcher, so unrelated docs/config files are never opened for route disqualification. The added surface is O(outside-reader count) content checks per changed testdata path, on top of the pre-existing per-package scan.

New fixtures in tests/repo/go-runtime-inputs.test.ts, both extending the existing synthetic-metadata harness:

• non-test Go helper naming testdata — service/service.go (ordinary source, not a _test.go) reads ../p/testdata/settings.json for its callers; route falls back to full validation.
• a test outside every go-reported package naming the testdata keeps full validation — legacy/tools/manual_test.go in a nested module under diffci.json {"go":{"scope":"root-module"}} reads ../../p/testdata/settings.json; route falls back to full validation.

Local run: affected suite 53/53 green (51 pre-existing + 2 new), typecheck clean. I also noticed tests/action-verify.test.ts fails 8 of 8 cases in my Windows sandbox with the process spawns coming back status 1, identically on the unmodified branch — looks environmental, and separate from this change; happy to dig into it as a follow-up if it reproduces for you.

[adityankale190895]

Thanks a lot for the quick and careful follow-up, @shounakjoshi88-a11y. becdd40 addresses most of the review well:

• Point 1: ordinary .go files in other packages are now scanned, and the service/service.go fixture covers it.
• Point 2: readers outside the go-reported packages now block the selective route, and the nested-module fixture covers it.
• Point 3: you're right, and I was wrong: readRepositoryFile already reads each file once per analysis. Thanks for pointing it out.
• Leaving out import clauses is a good catch. Without it, every package that imports p would block p's route.

A few things are still left. I found these by reading the code, not by running it.

1. Packages without tests are never scanned (blocker)

goPackageTestDataOwnerFor loops over byOwner, which only holds packages that report _test.go files. A shared helper package with no tests of its own, the usual testutil/golden.go shape, is never in that map. It is also left out of goOutsideReaders, because its files are in goDependencyPaths.sources (knownSources). So this case still plans SELECTIVE ["p/value_test.go"] and skips the callers' tests:

testutil/golden.go      package testutil; reads "../p/testdata/settings.json"  (no _test.go files)
service/service_test.go calls testutil.Golden(...)
change: p/testdata/settings.json

The new service/service.go fixture passes only because service has its own service_test.go. Suggestion: loop over every package in goSourceFilesByOwner (all reported packages) rather than byOwner, and add a fixture where the helper package has no tests.

2. Non-Go, non-test helpers (smaller, can be a follow-up)

The outside pool only takes .go files and files that match the test matcher. In a mixed repo, a non-test JS or Python module that reads p/testdata/... for the tests that import it is not checked. The early return in processChange also skips the general reference resolution that used to follow such indirect readers. Running that resolution as well, and taking the union with the GO_TESTDATA selection, would cover this.

3. The bare "testdata" literal blocks the route in most real repos (not a safety issue)

referenceTokens("p/testdata") includes the bare testdata token. With ancestors: true, contentNamesPath returns true for a word with no slash. So any other package that uses the common filepath.Join("testdata", name) or os.ReadDir("testdata") blocks p's route. In a repo where two or more packages use their own testdata, the new route would rarely be used. That fails safe, but it limits how much of #83 this delivers. A bare testdata in package q refers to q/testdata, so it could resolve against the reader's own directory and not count as a reference to p/testdata.

4. Minor

• Every changed file under the same testdata directory rescans the same readers. Caching the result per dataDir would keep a large golden-file refresh cheap.
• The branch is behind main. GitHub still reports it as mergeable.
• CI hasn't run yet because the PR is from a fork. We'll approve the workflow run so the Linux results confirm the action-verify failures are Windows-only.

Happy to re-review once 1 is fixed with a fixture. 2 and 3 can go into this PR or a follow-up, whichever you prefer.

[shounakjoshi88-a11y]

Pushed a single commit on fix/go-package-testdata-selection, points 1 and 2 both:

1. Helper packages without tests (blocker) — fixed, with the fixture. The cross-package scan now runs over the union of every go-reported package's members and its listed tests (goReadersByPackage, built once per analysis in mapGoPackageTestData), so testutil/golden.go contributes to ./testutil's entry even though the package reports no _test.go files. Legacy cached profiles without source metadata degrade to the listed tests exactly as before. I confirmed your reading empirically: on the pre-fix tree the fixture plans SELECTIVE ["p/value_test.go"] (fallback not required, empty reasons) while service/service_test.go — which calls testutil.Golden() — is skipped; with the fix the route is refused and full validation holds.

2. Non-Go, non-test helpers — in this PR, via the outside-readers pool rather than resolution. Took you up on the second shape: the pool now takes every modeled source module (isSourceFilePath) plus the python adapter's modeled sources. In a modeled go + python repository, a non-test python helper reading ../p/testdata/settings.json for the pytest that imports it now disqualifies the route and falls back; the pre-fix tree reproduces the miss. I preferred this over running general reference resolution under the early return because addAffectedTest dedupes reasons but appends evidence rows, so a file reached by two paths would grow duplicate evidence rows and break the reason/evidence assertions downstream — the pool keeps the reason set exactly the GO_TESTDATA-refusal shape. Another reason the fixture is python rather than JS: a Go adapter plus any .js file is itself the "Mixed Go/JavaScript repositories require explicit cross-language dependencies" global blocker (src/repo/graph.ts), so a JS helper can only exist in a graph that is already fully validating. The mixed-repository fixture stubs the python adapter's parse results the same way the go go list descriptors are stubbed, so no interpreter is needed for node --test.

3. Agreed — deferring. The bare "testdata" token needs the reader-relative resolution you describe (filepath.Join("testdata", name) inside package q is about q/testdata, not the target), and that change deserves its own pass with fixtures for the resolution cases; happy to take it as the follow-up.

4. On the per-dataDir rescan: readRepositoryFile already reads each file once per analysis, so the repeating cost is the reference-pattern pass over cached content, linear in readers per changed file under the same directory. I'll fold a per-dataDir memo into the point-3 follow-up so it's measured together. Locally: 803 tests — 795 pass, and the 8 failures are the action-verify Windows-env set that pre-dates this branch (round 1 already scoped them); typecheck and build are clean. The branch is behind main and still mergeable per GitHub — say the word if you'd like it merged or rebased before re-review.

[adityankale190895]

Thanks again, @shounakjoshi88-a11y. 1408f03 resolves the remaining blocker and point 2 as well. I reviewed it by reading the code:

• Point 1 (blocker): goReadersByPackage now covers every go-reported package, not just the ones with _test.go files. The go adapter reports a package's GoFiles even when it has no tests, so ./testutil gets its own entry, and the package keys use the same ./dir form as goTestPackages. The testutil/golden.go fixture covers exactly the case from the last review.
• Point 2: the outside pool now includes every modeled source module plus pythonSources. The import-clause stripping applies only to Go readers, so Python content is scanned in full, and an unreadable reader still refuses the route. Your reason for using the pool instead of general resolution (duplicate evidence rows from addAffectedTest) makes sense, and so does using Python rather than JS for the fixture, given the mixed Go/JS blocker.
• Points 3 and 4: fine as a follow-up together. Please open an issue for the bare "testdata" resolution and the per-dataDir memo so they're tracked.

No rebase needed: a trial merge against current main is clean.

I've approved the Core CI and CodeQL runs for this commit. Approving now, and we'll merge once Linux CI is green, which should also confirm the 8 action-verify failures are Windows-only.

[shounakjoshi88-a11y]

Thanks for the quick re-review and approval! Opened #90 to track the bare testdata token resolution and the per-dataDir memo as suggested. Happy to take either in a separate PR if useful — otherwise we'll follow in the issue.

[adityankale190895]

Merged in 6e20260 — thank you, @shounakjoshi88-a11y! Three careful rounds, each with fixtures that reproduced the miss before fixing it, made this an easy one to land. Package-owned testdata changes now select the owning package's tests instead of always falling back to full validation, which is a real win for Go repos.

CI also passed on windows-latest, so the 8 action-verify failures you saw look specific to your local setup, nothing to chase here.

Two small asks, only if you feel like it:

• If DiffCI looks useful to you, a ⭐ on the repo helps other people find the project.
• We'd love to hear how DiffCI does on a repo of your own, especially a Go one. Run diffci check (or the GitHub Action in observer mode) on a few real PRs and post what it selected and what it skipped, here or in a new issue. Wrong selections and confusing output are the most useful things to report.

And thanks for already filing #90 for the bare "testdata" token and the per-dataDir memo. We'll pick it up there.

[shounakjoshi88-a11y]

Thanks for the review and the merge! Follow-ups for the bare testdata token and the per-dataDir memo are tracked in #90.