Feat/architecture map - #23
Merged
Merged
Conversation
The architecture map needs to know which files reach which other files, and the scan currently has no dependency data at all: fetchRepoTree returns paths only. This adds the extraction layer. Regex-based on purpose. An import statement cannot hide its target, so a full parser per accepted extension would cost far more than the answer is worth. Covers the JS/TS family, Python, Go and Ruby; an extension it does not recognise yields nothing rather than guessing. Specifiers resolve against a path index built from every trailing segment run of every repository path, which is what lets a project alias or a Go module prefix resolve without the scan knowing the alias config or the module name. Ambiguous suffixes prefer the shortest repository path. Anything that does not land inside the repository is dropped, so package imports never become nodes.
Pins the behaviour the architecture map depends on: every JavaScript import shape, Python dotted and dot-prefixed relative forms, grouped Go imports, and the extension fallthrough. Two cases guard against silent wrongness rather than breakage. A bare side-effect import must not let the lazy run reach the next quoted string further down the file, and an ambiguous path suffix must resolve to the shortest match so a vendored copy cannot capture edges belonging to the real module.
The import graph wants to see as much of the repository as it can, but reading a repository is not free: every file is a GitHub request. So the tree is ranked rather than truncated, and a hard cap of 200 files decides what a scan costs. Nothing outside the returned list is ever fetched. Application source ranks first, supporting material last. The penalties deliberately outweigh the source-root bonus, because tests and generated files usually live in a source root and their edges are the ones that do the most damage: a test imports half the codebase and a generated route tree imports every route, so either becomes a false hub in the middle of the map. Ties break on path, so two scans of an unchanged repository read the same files and draw the same map.
Asserts the ordering intent rather than the raw scores, so the bands can be retuned without rewriting the suite: source above config, module above its own test, hand-written above generated. Also pins the two properties the map depends on for trust — the cap is honoured, and the selection is stable under reordering of the input so it never inherits GitHub tree order.
The dedupe key joined the two paths with a literal NUL character, which git and grep read as a binary file marker: the module stopped showing up in content searches and diffs rendered as Binary files differ. Replaced with a nested map keyed by source path then target. No separator means no escaping question and no possible collision, which matters because a repository path may legally contain any character except NUL itself.
Provides the two things the scan needs from dependency data. rankFilesByFanIn orders files by how many others import them, which is the blast radius of a bug and a far better reason to spend the 20-file budget than the tree order used today. buildModuleGraph turns the same edges into the directory-level graph the architecture map draws. Nothing in the output is a model opinion: boxes are directories, arrows are resolved imports, colour is the most severe finding rolled up per module. A wrong label is a shrug, but a wrong arrow is a false dependency diagram someone pastes into a design doc, so structure stays derived from code. Over the node cap, the deepest module folds into its parent instead of being dropped, so a collapse costs a level of detail rather than a file, and edges re-point at the surviving ancestor. Dropping only happens when a repository has more top-level directories than the map can show, and that count is reported as omittedModules rather than left implicit. Findings whose path does not match the tree are counted too, so model drift surfaces instead of vanishing.
Covers the arithmetic the map depends on: per-pair edge weights, intra-module edges dropped, directory-targeted edges from package-style import systems, and severity rolled up to the most severe per module. The collapse cases are the ones worth having. A collapse must preserve the total file count, must re-point both ends of an edge at the surviving ancestor, and when modules genuinely cannot fit, kept plus omitted must still equal the real total so the UI can never imply it drew everything.
An installation token is valid for an hour, but every call minted a fresh one, so any operation reading more than one file paid two requests per file. The codebase scan is about to read hundreds for the import graph, which makes this the difference between practical and not. Cached per installation and retired a minute before GitHub expires it, so a token cannot lapse mid-request. In-flight mints are shared through a promise map as well: without that, a burst of concurrent file reads would each race to mint its own token and the cache would never be warm when it mattered. A failed mint clears that entry so it cannot pin later callers to the same rejection. The exported name stays as it was to avoid churning every caller, and now documents that it returns a usable token rather than always minting one.
Pins the arithmetic most likely to be wrong. Too generous a margin and a long scan reuses a token that expires mid-request, which surfaces as an unexplained GitHub 401 rather than as a cache bug, so the boundary case is asserted explicitly.
The import graph needs many more files than the scan prompt does, and reading them one at a time would make the graph pass the slowest part of a run. fetchFiles takes the paths it is allowed to read and works through them with a small concurrency ceiling. It reads an explicit list and nothing else, so what a scan touches is always something that was decided on rather than everything that happened to be in the repository. maxChars trims each file as it lands, which is all the graph needs since import statements sit at the top. Results keep the order of the requested paths regardless of which read finishes first, so an unchanged repository yields the same input every time. A file that cannot be read is skipped instead of failing the run: one unreadable file should cost its own edges, not the whole map.
…them The architecture map needs a caption per module. This is the single extra LLM call the feature costs, and its scope is deliberately narrow: boxes, arrows and colours are all derived from code, so the model never gets to assert a dependency. A bad label is a confusing caption; a bad arrow would be a false dependency diagram someone treats as fact. The prompt sees directory paths and file names only, never file contents, and is told to fall back to a plain reading of the directory name rather than guess at behaviour the names do not support. Parsing keeps only ids that were actually asked about, so a hallucinated module cannot add a box to the diagram, and labels are trimmed and length-capped to what the diagram has room for. Unparseable output yields no labels rather than throwing.
collectFiles walked the tree in order and broke at 20 files, so a scan only ever read whichever files git happened to sort first. On a large repository that meant the findings — and any heat map built from them — were a function of path position rather than risk. The old ordering was also what made a hotspot overlay misleading enough not to ship. The run now reads the ranked candidate set for the import graph first, then spends the 20-file budget on the files with the highest fan-in, because a defect in a module twenty files depend on has a larger blast radius than one in a leaf nothing imports. The character budget is applied after ranking, so the truncation lands on the least-depended-upon file rather than an arbitrary one. The same graph then produces the architecture map stored on the summary, and the labelling call is wrapped so a failure costs the labels rather than the scan. Its tokens are added to the run usage, since it is real spend against this scan.
The map rides along on the same opaque summary JSON as the findings, so it gets the same treatment: nothing about the stored shape is trusted on the way back out. Counts are rebuilt severity by severity, an unrecognised severity reads as none, and numbers are clamped to non-negative integers. A scan that ran before the map existed has no architecture key and reads back as null rather than as an empty diagram, so old scans keep rendering exactly as they did. Edges are dropped unless both endpoints are modules that are actually on the map. Without that, a stored edge pointing at a module the node cap collapsed away would render as an arrow into empty space.
The read path is the one place a malformed summary could break the scan page, so the cases are about what happens when the stored JSON is wrong rather than when it is right: no architecture key, a non-object, no usable modules, missing counts, an unknown severity, and a negative edge weight. The dangling-edge case is the important one. An arrow whose endpoint is missing would draw into empty space, so both ends must resolve to a module the map is showing.
Adds architecture to CodebaseScanDetail so the scan page can draw the map, coerced through summaryToArchitecture rather than read raw off the summary. Null for a scan from before the feature or one whose repository had no modules to draw, which the UI treats as simply having no map to show.
Pure geometry, kept React-free so the arithmetic can be tested without rendering. Modules are pushed one column right of everything that imports them, so a reader follows dependencies in one direction. Cycle-closing edges are found by depth-first search and excluded from the column maths. This is not a nicety: rendering a realistic sample showed a single import cycle dragging six modules into one column with arrows looping back across the whole diagram. Those edges are still drawn, they just do not get a say in where the boxes go. The relaxation is still bounded by the module count as a guard. Columns are centred against the tallest one, and modules keep their sorted order within a column, so the same scan always draws the same picture.
The cycle cases earn their place: before back-edge detection, a three-module cycle collapsed the diagram into one column, and the tests now pin the column each module lands in rather than merely asserting the layout terminates. A fully cyclic graph still has a termination test as a backstop. The rest covers canvas sizing, column centring, edges anchored to the box edges they connect, and that laying out the same map twice gives an identical result.
Boxes are directories, arrows are resolved imports with thickness standing for how many, and colour is the most severe finding in the module. The label under each name is the only part a model wrote. The caption states how many files the arrows were actually derived from against the repository total, and names the number of modules left off the map. Without that the diagram would read as a complete picture of a repository when it is a complete picture of the modules and a bounded sample of the dependencies.
The map is the part of a scan most likely to be screenshotted, so the tests are mostly about it not overstating itself: the caption has to carry the graphed-versus-total file counts, it has to admit omitted modules when there are any, and say nothing when there are none.
JSON.parse returns null for the literal null and a primitive for a bare string or number, neither of which has a modules property. The optional chain read as safe but was applied after a cast that told TypeScript the value could not be nullish, so it was doing nothing. Checked explicitly instead.
The cap test declared its own paths over the shared fixture of the same name, which reads as though it were using the outer one.
Renders between the severity chips and the findings list, so the structural read comes before the itemised one. Omitted entirely when a scan has no map, which covers both scans that ran before the feature and repositories with no modules to draw.
collapseToFit rescanned and re-sorted every module to choose each single victim, which is quadratic in the directory count. Measured on synthetic repositories: 600 directories took 1.9s, 1200 took 8.7s, and 2400 took 36.5s of blocking synchronous CPU inside the scan run. On serverless that last one is a timeout, and because the call sits inside the run try block it would fail the whole scan rather than just the map. Modules are now bucketed by depth once and each bucket sorted once, walking deepest-first, so the pass is O(n log n). A parent that did not previously exist as a module joins its own bucket so a later shallower pass can fold it further. The same benchmark now runs in 8ms, 21ms and 66ms, and 20000 files across 5000 directories takes 228ms. Behaviour is unchanged: the merge order is still least-interesting-first within the deepest level, and the file-count invariant was verified to hold exactly at every size tested.
The budget was spent strictly in rank order, so a single file larger than the 50k character budget consumed all of it and the other nineteen went unread. The arithmetic predates the architecture map, but fan-in ranking made it far more likely to bite: the file now in first position is the most-imported one, which is exactly the kind of file that runs long. Every file now gets an equal share first, and only the unspent remainder is handed out, highest-ranked first. A small file still stays whole and its leftover flows to the leader. Moved here from run-scan because this module already owns what gets read and how much of it; that makes the export part of its job rather than something added just to be testable.
The starvation case is the one worth pinning: four files against a budget of 1000 where the first is 5000 characters long used to return one file, and now returns all four. Also covers the invariant that matters for cost — the total never exceeds the budget — plus small files staying whole with their remainder flowing to the ranked leader, and the empty-input and zero-budget edges.
The map container only constrained itself horizontally. A repository whose modules barely depend on each other lays out as a single column, and 24 modules that way measured 2138px tall — a card that pushes the findings list off the page. Now capped at 70vh and scrollable in both directions, matching the pattern already used by the admin tables.
Counting every rect and path inside the card asserted the markup rather than the behaviour, and it had already broken once because the heading icon contributes shapes of its own. The remaining assertion — that every module is named — is renamed to say what it actually checks. Trade-off worth recording: nothing now asserts the arrows render at all. Edge geometry is still covered by the layout tests, so a broken arrow would have to come from the JSX rather than the maths.
Four cases for a one-line comparison was heavy. Kept the fresh case and the boundary, which together pin both directions and the exact margin; the other two were weaker restatements of the boundary.
Adds ARCHITECTURE_MAP_LOGINS to the optional key union so the flag can be read through getOptionalEnv rather than reaching into process.env directly.
The map is still being proven out, so it ships to two accounts first. The allowlist is a comma-separated environment variable defaulting to those accounts, which means widening or revoking access is a config change rather than a migration or a deploy. Comparison is case-insensitive because GitHub logins are. A wildcard opens the flag to everyone, so shipping broadly later does not mean listing logins forever, and an empty list disables it outright. A caller with no login is always denied. Not stored per workspace in the database on purpose: this gates unproven work, and the set of people who should see it changes far more often than a schema should.
Covers the cases that decide who sees an unreleased feature: exact membership, case-insensitive matching, spacing and empty entries in the list, and a signed-out caller being denied. The rollout controls get their own cases — the wildcard opening access to everyone, and an empty list switching the feature off without a deploy. One test pins that setting the variable fully replaces the default rather than merging with it, so rolling the trial forward cannot silently leave the original accounts enabled.
Adds architectureMap to the run input and resolves it where a scan is requested. When it is off, the module graph and its labelling call are skipped entirely, so a scan for someone without the flag costs no extra LLM spend. The import graph is still built either way, because it decides which files get the 20-file budget — that is a scan-quality improvement everyone gets, not part of the gated feature. The flag is resolved in the route rather than inside the engine because it is about who asked for the scan, and the engine only ever knows the repository. This is the only caller of runScan, so there is no path around the gate. Also drops the local character-budget helper in favour of the shared one in candidates, which no longer lets a single oversized file consume the whole prompt budget. Kept in one commit with the route change because the input field is required: split apart, neither half typechecks on its own.
Gated on read as well as on write. Write-side gating alone would leave the map visible on scans that already stored one, so taking an account off the allowlist would not actually revoke access. Returning null makes the flag authoritative for display, and the page already renders nothing when there is no map.
Records the three states that matter when rolling the feature out: unset falls back to the trial accounts, a wildcard opens it to everyone, and empty disables it. Notes that the flag gates both building the map and returning it.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![jargons-ai[bot]](https://avatars.githubusercontent.com/in/4369042?s=60&v=4){kind=small}
There was a problem hiding this comment.
Jargons review
Found 2 issues worth a look:
🟡 Medium — Potential for inconsistent token caching due to Date.now()
src/server/github-app/client.ts:100
The tokenIsFresh function uses Date.now() to check token freshness, but Date.now() is also used in mintInstallationAccessToken to set expiresAt if expires_at is missing. If these two calls to Date.now() are not perfectly synchronized (e.g., due to event loop delays or system clock changes), it could lead to a token being considered fresh when it's actually expired, or vice-versa, causing intermittent authentication issues.
Suggestion: Pass the now timestamp as an argument to mintInstallationAccessToken when expires_at is missing, ensuring consistency with the tokenIsFresh check. Alternatively, ensure that Date.now() is called only once per token freshness check and minting operation, and that this single timestamp is used throughout.
🔵 Low — Inconsistent handling of maxChars in fetchFiles
src/server/scan-engine/github.ts:150
The fetchFiles function applies maxChars to content.slice(0, maxChars) only if maxChars is truthy. However, the withinCharBudget function expects content to be already truncated to a certain length. If maxChars is not provided to fetchFiles, withinCharBudget will receive full file contents, potentially exceeding its internal budget calculations and leading to unexpected behavior or higher memory usage than intended.
Suggestion: Ensure that maxChars is always applied consistently. If maxChars is intended to be optional, withinCharBudget should be robust enough to handle full file contents and perform its own truncation, or fetchFiles should always apply a default maxChars if none is provided.
Note: the diff was large and reviewed in part.
🔧 Suggested fixes: open the fix PR →
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture map from a codebase scan
Adds a module dependency diagram to the scan detail page — and, to make it
possible, changes how a scan chooses which files to read.
Gated behind a feature flag to
devtofunmiandxt42io, so this is safe tomerge and deploy before the feature is ready for everyone.
Why this is two changes, not one
The map needs to know what depends on what, and the scan had no dependency data
at all —
fetchRepoTreereturns paths only. Building that graph turned out tofix a bigger problem than it created.
collectFileswalked the repo tree in order and stopped at 20 files, so a scanonly ever read whichever files git happened to sort first. On a large repository
the findings were a function of path position rather than risk. Now the import
graph is built first, and the 20-file budget goes to the files with the highest
fan-in — a defect in a module twenty files depend on has a far larger blast
radius than one in a leaf nothing imports.
That improvement is not behind the flag. It makes every scan better and is
independent of whether you can see the diagram.
How the map is built
candidates.tsscores the tree and reads at most 200files. Application source first; tests and generated files score down hard
enough that living in
src/doesn't rescue them, because a test imports halfthe codebase and a generated route tree imports every route, so either one
becomes a false hub.
imports.ts, regex per language (JS/TS, Python, Go,Ruby), resolved against a suffix index so project aliases and Go module
prefixes work without knowing the project's config. Anything landing outside
the repository is dropped.
graph.tscollapses file edges to directory modules, r