Repository navigation
Add TypeScript extraction, change, and history support - #12
Conversation
|
Thank you for this stack. I reviewed #12 as the unit, because it contains the commits from #10 and #11. Summary: the design is right, but the TypeScript adapter is not ready to merge. The adapter boundary in Below: what I tested, what holds up, the changes needed in priority order, and how I suggest handling the stack. What I tested, and what I did notI tested head
I did not run What holds up
Changes neededThese are in priority order. Items 1 to 4 each break a requirement from #9: "Unsupported or ambiguous TypeScript syntax is reported as unknown, not silently treated as absent." 1. Record a module that does not parse, instead of stopping the extract
The rejected files are valid TypeScript. In hono, the failure is at <Key extends keyof E['Variables']>(key: Key): E['Variables'][Key]The pinned I suggest keeping the module in the facts with a marker that says it could not be read. 2. Cover the export forms that produce no name, or report them as unknown
One form produces a wrong name. The next table answers how often this happens in real code. It counts top-level
The rate depends on style. taxonomy is a Next.js app that declares components and then writes 3. Read
|
| repository | modules | time |
|---|---|---|
| hono | 179 | 10.7 s |
| zod | 305 | 22.0 s |
| date-fns | 1704 | 378.6 s |
date-fns has 5.6 times as many modules as zod and took 17 times as long. Under cProfile on zod, 49.7 of 52.5 seconds were spent in that dictionary comprehension. The profiler's overhead is why the total is higher than 22 s.
Building the dictionary once in build and passing it in should fix this. The cost also repeats in history, which runs an extraction for each commit it samples.
6. Entry points: map built paths back to source, or say none were found
All five repositories produced 0 entry points. bin and exports in package.json usually point at built output, such as ./dist/index.js, which never matches a .ts source path.
This is a lower priority than 1 to 5, because #9 lists framework entry points as follow-up work. But bin is the case the PR claims to support. compilerOptions.outDir and rootDir give the mapping from dist/index.js back to src/index.ts. Where no mapping exists, a line saying so is better than an empty list.
7. Pass the commit-time gates
AGENTS.md requires uv run pre-commit run --all-files to pass. But the complexity and length hooks compare the working tree against HEAD. On a tree that is already committed, they compare the code with itself and pass. Staged against main, three hooks fail:
- cognitive complexity (complexipy):
change.computegoes from 100 to 105,config.loadfrom 30 to 33, andextract.buildfrom 36 to 40. The hook fails any function a commit makes worse. - cyclomatic complexity:
config.loadgoes from 14 to 15, and two new functions are over the limit of 10:TypeScriptLanguage.entry_pointsat 16 andparse_surfaceat 12. - file length:
config.pygoes from 800 to 847 lines. The limit is 800.
To see these failures, stage the whole diff against this repository's main and run the hooks. Do it in a separate worktree, so your branch does not move. upstream below is whatever your remote for 0xfauzi/systemap is called:
git fetch upstream main
git worktree add --detach ../gate-check HEAD
cd ../gate-check
git reset --soft upstream/main
uv run pre-commit runMoving the TypeScript config helpers (_package_json, _typescript_name, discover_typescript_roots) into their own module would fix the length failure. The new language branches in load could move there too.
8. State the acceptance number before the next run
AGENTS.md asks for the number a feature must reach to be written down before the experiment runs, so the number cannot be chosen after the result to make it pass. I have asked for this on #9, because it applies to the whole feature rather than to this PR.
Smaller notes
- Module name collisions: the new check in
buildalso applies to Python. A Python repository with bothfoo.pyandfoo/__init__.pyused to have one file overwrite the other silently. Now the extract stops. I think the new behaviour is better, but it is a change to Python output, and the PR should say so. - Repositories without
src/: ky, zod and date-fns have nosrc/directory, so the whole repository becomes the source root. Scripts, fixtures and, in ky's case, test files become modules. Monorepos such as zod and date-fns probably need[package_roots]set by hand. The README should say so.
How I suggest handling the stack
I suggest closing #10 and #11 and continuing in this PR. The layers change each other's behaviour. For example, #11 makes tsconfig.json required for detection, which #10 did not. So merging #10 on its own would ship a state that #11 replaces. One PR also gives one CI run and one review for the whole change.
When items 1 to 5 and 7 are done, I would re-run the same five repositories and compare the numbers against the acceptance number agreed on #9.
Generated by Claude Code
0xfauzi
left a comment
There was a problem hiding this comment.
Changes are requested before this stack can merge. This review records the decision. The findings and measurements are in the review comment on this PR.
The design of the adapter boundary is accepted. The blocking changes are items 1 to 5 and 7 of that comment:
- Record a module that does not parse as unknown, instead of stopping the extract. Today, extraction fails on hono, zod and date-fns.
- Cover the export forms that produce no name, or report them as unknown. Between 2% and 24% of top-level exports are missing, depending on the repository.
- Read
tsconfig.jsonwith comments, and followextends. Today, a comment drops every path alias without a message. - Find tests under the repository's own naming, and make
extractanddeltause one rule. - Resolve file paths once per build. date-fns took 378.6 s.
- Pass the complexity and file-length gates when the stack is staged against
main.
Item 6 (entry points) and item 8 (the acceptance number, asked for on #9) are needed before TypeScript support is described as complete, but they do not block this review on their own.
When the changes are pushed, I will re-run the same five repositories against the acceptance number agreed on #9.
Generated by Claude Code
0xfauzi
left a comment
There was a problem hiding this comment.
Thank you for the fast turnaround. ab67c4f fixes every item the first review asked for, and the numbers below show it. But it is not ready to merge yet: the process crashes on two of the seven repositories, check can never pass on a real TypeScript repository, and the change figure crashes on a common case. Each has a reproduction below.
So the round-trips stay short, I ran the whole set of commands this time, on the five pinned repositories plus two the adapter had not seen. Everything below is from this head, in one container: Linux x86_64, Python 3.11.15, tree-sitter 0.26.0 from the PR's uv.lock, tree-sitter-typescript 0.23.2.
What is fixed
- Gates: with the diff staged against
main, every pre-commit hook passes, including the three that failed before. - Tests: 394 pass.
- Python output: facts for this repository are byte-identical to
main's. - Unparseable files stay in the map with an unknown entry.
- Export forms: 100% of top-level exports on all seven repositories produce a name or an unknown.
tsconfig.jsonwith comments andextendsreads correctly.- Speed: date-fns builds in 3.5 s, down from 378.6 s.
deltaworks on a TypeScript change. A test added undertests/that imports through the@/alias was credited to the module it tests.
Results against the pass numbers
The pass numbers are the ones you posted on #9. I added two repositories that were not named when the fix was written, so they show how the adapter does on code it was not written against: nestjs/nest at b58554ea58 (1917 files, *.spec.ts tests, decorators) and excalidraw/excalidraw at 5db42c3ddb (664 files, mostly .tsx). Both are now part of the pinned set on #9.
| pass number | ky | taxonomy | hono | zod | date-fns | nest | excalidraw |
|---|---|---|---|---|---|---|---|
| facts produced | only with node_modules present |
yes | yes | yes | systemap extract crashes |
yes | yes |
| exports accounted for (100%) | 132/132 | 160/160 | 713/713 | 1374/1374 | 1713/1713 | 2499/2499 | 2570/2570 |
| test files attributed (90% or more) | 28/30, 93% | N/A | 134/137, 98% | 57/202, 28% | 259/265, 98% | 467/504, 93% | 134/137, 98% |
| date-fns build time (60 s or less) | 3.5 s |
How the counts were made:
- Exports: every top-level
exportstatement for whichparse_surfacerecorded neither a name nor an unknown entry counts as missing. A file the parser rejects is not counted, because the whole module carries an unknown. - Test files: counted by each repository's own convention (
test/*.tsfor ky,*.spec.tsfor nest,*.test.tsand*.test.tsxfor the rest,test.tsnext to the code for date-fns). ky'stest/helpers/is left out, because ava leaves it out too. - zod: its tests import
zod/v4by package name, and with the repository root as the source root nothing resolves. With"packages/zod/src" = "zod"under[package_roots], zod reaches 190/202 (94%). Per-repository settings are allowed in the benchmark from now on, provided they are written in the PR before the run. That rule is on #9. - The export census and the hono and excalidraw numbers were produced on
tree-sitter0.25.2, because on 0.26.0 the process crashes partway through (item 1 below). The extraction numbers are the same on both versions where both complete.
Blocking
1. The process crashes on tree-sitter 0.26.0, which uv.lock selects
systemap extract on date-fns exits with a segmentation fault, 3 of 3 runs. The trace ends in json.dumps inside write_facts, which is pure Python, so memory was corrupted earlier. The same happens partway through excalidraw.
The smallest reproduction I found is two files from hono, in this order:
from pathlib import Path
from systemap.typescript_surface import parse_surface
for f in ("components.test.tsx", "components.ts"):
p = Path("src/jsx/intrinsic-element") / f
parse_surface(p.read_text(), p.as_posix())What I measured:
- crashes on
tree-sitter0.26.0 withtree-sitter-typescript0.23.2 - passes on
tree-sitter0.25.2 with the same grammar package - passes on the previous head
fcdc768with 0.26.0, so the newtypescript_surface.pyreaches something the old code did not - either file alone passes; plain tree-sitter parsing the same two files passes; keeping the
Treeobjects alive does not help
tree-sitter-typescript 0.23.2 is the newest on PyPI, so there is no grammar upgrade to try. The safe fix is to pin tree-sitter>=0.25,<0.26 and re-lock, and to say in the PR why. If you find the actual cause in 0.26, better still.
2. check fails on every unknown line, and [judgement] answered does not clear it
I tested this on the PR's own fixture. Adding two consecutive generic call signatures to service.ts, the form hono's context.ts uses, makes check exit 1 with unknown surface: 1 finding. Answering it under [judgement] answered with kind = "unknown surface" clears it from judgement but not from check, because Result.ok in check.py requires unknown_surface to be empty and never reads the answers.
On real repositories, with every module mapped and an entry set, check fails only on unknown lines, and there are always some:
| repository | unknown lines | main cause |
|---|---|---|
| ky | 2 | a default export expression |
| taxonomy | 2 | default export expressions |
| hono | 178 | 150 package.json export targets that did not map |
| zod | 34 | default exports, local exports |
| date-fns | 23 | mixed |
| nest | 4 | parser failures |
| excalidraw | 97 | 69 default export expressions such as export default memo(...) |
Since refresh runs check, refresh fails too, and the CI workflow that init writes would be red on every one of these repositories with no way to make it green.
The requested change: take unknown surface out of check's failure set. Keep it as a judgement line, so an answer under [judgement] answered clears it and judgement --strict enforces that every line is either fixed or answered. check can still print the count as information. That matches how the other judgement kinds already work.
3. The change figure crashes when a changed module carries an unknown entry
surface_delta now returns None when either side has any unknown entry. compute puts that module in unparsed, then reads deltas[m] for it anyway:
systemap figure --mode change --base main
KeyError: 'taxonomy.middleware'
Reproduction: on taxonomy, add one exported function to middleware.ts (its default export is already an unknown), commit, and run the command above. systemap delta itself survives, because it does not go through compute.
The deltas[m] read exists on main too, but there it is reachable only for a Python file that fails to parse. This PR makes it common: 2 of 125 taxonomy modules and 79 of 515 excalidraw modules carry an unknown. Two changes are needed: a module that parsed but has unknown entries should still get a delta (with the unknowns carried along), and compute should not read deltas for a module in unparsed.
4. An extends that names an npm package stops the extract when node_modules is absent
ky's tsconfig.json has "extends": "@sindresorhus/tsconfig". Without node_modules, load_typescript_config raises ConfigError: could not resolve tsconfig extends '@sindresorhus/tsconfig' and nothing is produced. The CI workflow init writes does not run npm install, so ky would fail in CI.
The requested change: record it as an unknown line and carry on with the aliases that could be read. Base configs from packages rarely define paths, so the facts are usually still right.
5. .d.ts files are modules again
fcdc768 excluded them; source_paths in this head does not. hono gets hono.adapter.deno.deno.d and taxonomy gets taxonomy.types.index.d and taxonomy.types.next_auth.d as modules. module_for_path still excludes them, so extract and delta disagree about whether they exist.
Not blocking
package.jsonexport targets. hono's 150 unknown lines come from one cause: itsoutDirandrootDirare intsconfig.build.json, whichtsconfig.jsondoes not extend, sodist/index.jsnever maps tosrc/index.ts. Two things would help. First, when the target's first path segment is not a source root and the rest exists undersrc/, map it. Second, report one line per package rather than one per target: 150 lines for one package is more noise than information.- Parser failures are a grammar limit. The generic call signatures in hono and zod fail on the newest grammar package, so an unknown entry is the right handling. Say so in the README's limits section.
- Entry points are 0 on all seven repositories. With the mapping fix above, hono and ky would get theirs.
Next steps
Items 1 to 5, then re-run the seven repositories against the pass numbers on #9 and put the results in the PR description, including any miss. I will re-run them on the same commits when you push.
Generated by Claude Code
0xfauzi
left a comment
There was a problem hiding this comment.
Approved. 0d35111 fixes all five blockers from the second review, and the seven-repository run in the PR description reproduces on a second host. Thank you for the care in this: the committed runner, the declared settings, and the corrected census note are what make the numbers trustworthy.
What I checked on 0d35111
Everything below ran in one container: Linux x86_64, Python 3.11.15, tree-sitter 0.25.2 from the new lockfile, tree-sitter-typescript 0.23.2.
| second-review item | result |
|---|---|
1. crash on tree-sitter 0.26.0 |
The two-file reproduction passes. systemap extract on date-fns and on excalidraw: 3 of 3 runs each, exit 0. |
2. check fails on unknown lines |
On the fixture with an unparseable construct, check exits 0 and prints the finding as information. judgement --strict lists it, and an answer under [judgement] answered clears it. |
3. change figure KeyError |
figure --mode change on a changed module that carries an unknown writes the figure. |
4. extends naming an npm package |
ky without node_modules extracts, with one config issue line. |
5. .d.ts as modules |
No .d modules in hono or taxonomy. |
Gates: 398 tests pass; every pre-commit hook passes with the diff staged against main; mypy is clean; Python facts for this repository are byte-identical to main. The four steps the CI workflow runs (extract --check, check, judgement --strict, render --check) all pass on the PR tree.
The benchmark, reproduced
I ran bench/typescript_adapter.py on my checkouts at the seven pinned commits, one process per repository, with the settings the PR declares. Modules, exports accounted, tests attributed, unknown lines and unparsed modules match the PR's table exactly on all seven. Only the times differ, by host:
| repository | extract time here | extract time in the PR |
|---|---|---|
| ky | 3.19 s | 0.33 s |
| taxonomy | 0.33 s | 0.20 s |
| hono | 2.26 s | 1.28 s |
| zod | 2.24 s | 1.31 s |
| date-fns | 4.24 s | 2.11 s |
| nest | 7.67 s | 2.97 s |
| excalidraw | 5.81 s | 2.61 s |
All are under the 60 s limit. Entry points went from 0 to 126 on hono and 7 on ky.
Follow-up, not blocking
${configDir} in inherited tsconfig.json path options is not expanded. On ky, out_dir resolves to node_modules/@sindresorhus/tsconfig/${configDir}/distribution, and only the new distribution/ to source/ fallback keeps the entry points correct. Filed as #13, with acceptance criteria written there before the fix.
Before merging
The systemap workflow has not yet run on this PR; only GitGuardian has. It passes locally, but the merge waits for a real run.
Generated by Claude Code
A shared base tsconfig cannot know where the project that extends it
lives, so TypeScript lets a path option start with ${configDir}, the
folder of the top-level tsconfig.json. The reader kept the text as
written, so @sindresorhus/tsconfig left ky's outDir pointing inside
node_modules at a folder that does not exist, and a compiled package
entry could only be mapped back to its source by guessing src/ or
source/.
The rule comes from tsc --showConfig (7.0.2), not from memory: the
variable counts only at the start of a value, in every file of the
extends chain, and always means the top-level folder. A value such as
cache/${configDir}/x is left literal, and a plain relative value still
means the folder of the file that declares it.
The test states its acceptance before the fix and fails on the previous
head. bench/typescript_adapter.py gives the same counts as the run
recorded on #12 for all seven repositories.
Fixes #13.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzLdCRXKdAP2swLysEfacu
0xfauzi
left a comment
There was a problem hiding this comment.
084122e is README only, so the approval on 0d35111 stands for the code. I ran the new example as written: the three files, systemap init, the systemap.toml shown, then extract, facts --names example.service and facts --module example.service. The facts name greet and attribute the test to example.service, as the text says.
Two small fixes before merge, both in the new section:
- The heading. Every README heading is a question the section answers (the rule is recorded in
CHANGELOG.mdunder Unreleased), and### TypeScript exampleis a topic name. Something like### What does a TypeScript project look like?fits the section. initwithouttsconfig.json. The example's file list has notsconfig.json, andinitdetects TypeScript only when one exists. On that treeinitwriteslanguage = "python", so the reader has to changelanguageas well as the source root, while the text says only to set the root. Either addtsconfig.jsonto the file list, soinitdetects the language itself, or say that both keys need setting. The first is closer to how a real project looks.
Generated by Claude Code
Summary
Consolidates the TypeScript extraction, change detection, and history work from former PRs #10 and #11.
.d.tsdeclarations from modules and change analysis.binandexportstargets through compiler paths or a unique matchingsrc/orsource/file; groups unresolved package exports into one issue per package.Review follow-up
The second review's blockers are addressed:
tree-sitter>=0.25,<0.26withtree-sitter-typescript0.23.2 because the locked 0.26.0 combination crashed on real repositories.checkas information.judgement --strictremains the gate for fixing or answering them.extendsis unavailable, recording a config unknown..d.tsfiles consistently from extraction and delta.The README also documents the current grammar limit around some valid generic call signatures.
Verification
uv run pytest -q: 398 passed.uv run pre-commit run --all-files: passed with the whole PR diff staged againstmain, including the complexity, cyclomatic, and file-length gates.uv run systemap refresh,extract --check,render --check,check, andjudgement --strict: passed; all 40 modules in this repository are mapped.uv run mypy src/systemapandgit diff --check: passed.Seven-repository acceptance run
Thresholds were recorded on issue #9 before measurement: the original five plus the two additional pinned repositories must all extract; every top-level export must yield a name or unknown; each repository with tests must attribute at least 90% of test files; date-fns must extract within 60 seconds.
Settings were declared in this PR before the run: all seven use
language = "typescript"and default test recognition. Ky, taxonomy, hono, date-fns, nest, and excalidraw use default source-root discovery. Zod uses:The exact seven commit SHAs and measurement method are in the committed runner. Results below are from PR head
0d35111, measured sequentially on macOS 27 arm64 with Python 3.11.13,tree-sitter0.25.2, andtree-sitter-typescript0.23.2. Time is one unprofiledextract.buildplus facts serialization per repository.All declared thresholds pass on this host. The export census includes test files and excludes modules the grammar cannot parse, matching the review's denominator. An initial local census omitted exports in test files and five date-fns
.test.tsfiles; the corrected runner above was used for this committed-head result. Unknown lines remain visible forjudgement; they are not treated as missing facts.