Show a spec author the URLs its app answers - #259
Conversation
Nine of one run's eighteen spec timeouts sat on "/" waiting for a screen
mounted at another path, because the author was never told the path existed.
Measured against this repo's own frontend, appSourceFor crawled breadth first
from the entry and nine large screens — most of them nothing to do with the
test — spent all 140K characters between them. The crawl stopped there, before
the pass that collects the plain modules, so src/routes.ts never reached the
author even though app.tsx did. And app.tsx holds no URLs: it renders
`<Route path={ROUTE_PATHS.workflow}>`, so the shell alone is 22 identifiers.
So the source is now emitted by relevance rather than by breadth — targets, the
URL table, the screens between the entry and a target, the modules they read,
then whatever screens are left — and the two halves are joined for the prompt:
app-routes.ts resolves each `<Route>` through the constants that name it and
lists the result, "/ — redirects to /agent" included. A route table sitting at
the foot of a shell too long to show whole is kept by window rather than lost to
head-first truncation, and its characters are held back from the targets, which
can otherwise spend the budget on their own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
DevAsign Code Review
🐞 Bugs (2) · ❌ Tests failing (3)
🟡 Merge score: 58/100
9 of 9 acceptance criteria met.
This PR adds route-URL resolution (app-routes.ts) and reworks app-source.
Prompt to fix all issues
You are helping fix PR "Show a spec author the URLs its app answers" in devasignhq/agent. Automated review surfaced the items below — failed acceptance criteria and review findings. Each item states what was required, what's wrong with the current diff, and how to fix it; the embedded fix blocks include the expected behavior and the relevant diff hunk. Apply each fix so the item is resolved. Items tagged **Blocker** gate approval; the rest are advisory but worth addressing. Don't introduce changes beyond what's listed.
## End goal
When the spec-authoring assistant is given a repo's frontend source, the prompt includes the app's real route table resolved to URL literals (e.g. `/agent — renders AgentPage`, `/ — redirects to /agent`), so the author writes specs that navigate to the paths where screens are actually mounted.
## Review findings
### 1. [Bug · Warn] `backend/src/verify/app-source.ts` — The emit loop breaks with `else if (!owed) break;` — it stops as soon as a non-pinned file fails to fit AND there are no pinned files still owed. But if a pinned route module is still owed (`owed > 0`), the loop continues past a file that could not be added, iterating every remaining ranked file. Each iteration calls `add`, which returns false when `out.length >= lim.files` or `room <= 0`; that is harmless but the loop no longer terminates early, so it scans the full `ranks.flat()` list looking for the pinned file. That is intended (find the pinned file), but note that if a pinned file also fails `add` (because `out.length >= lim.files`), `owed` never reaches 0 and the loop runs to completion doing nothing — merely wasteful, not incorrect.
Fix: pinned route module can still be dropped when the file-count cap is hit first
File: backend/src/verify/app-source.ts
Symbol: appSourceFor
Issue:
The budget reservation (`held`) protects the route module's characters, but nothing protects its slot against `lim.files`. If enough higher-ranked files are emitted to reach `lim.files` before the pinned route module is reached in rank order, `add` returns false for it and the URL table is dropped despite being reserved.
Expected behavior:
A pinned route module reserved for the URL map should be emitted even when the file-count cap would otherwise exclude it.
Suggested approach:
Emit pinned route modules first (or reserve a file slot for them) before spending the file-count budget on ranked non-pinned files, so the reservation covers both characters and the slot.
Relevant diff:
```diff
+ let owed = pinned.size;
+ for (const p of ranks.flat()) {
+ const c = content.get(p)!;
+ const shown = routeModules.has(p) ? routeWindow(c, room(pinned.has(p))) : c;
+ if (add({ path: p, content: shown, partial: shown.length < c.length }, pinned.has(p))) {
+ if (pinned.has(p)) owed--;
+ } else if (!owed) break;
+ }
```
### 2. [Bug · Warn] `backend/src/verify/app-routes.ts` — In `routeWindow`, `from = Math.min(raws[best].at, content.length - room)` then `start = content.lastIndexOf("\n", from) + 1` and the slice is `content.slice(start, start + room)`. When `from` is clamped to `content.length - room`, snapping the start back to the previous line start makes `start < content.length - room`, so `start + room < content.length` and the tail of the file (which may hold the last routes when the best window is the final window) is cut. The comment claims the snap is "never forward" to keep the anchored route, but clamping-then-snap-back can push the window start earlier than needed and drop trailing routes that were inside the pre-snap window.
Fix: line-start snap after clamping can drop trailing routes in routeWindow
File: backend/src/verify/app-routes.ts
Symbol: routeWindow
Issue:
When `from` is clamped to `content.length - room` and then snapped back to the previous line start, the window start moves earlier and `start + room` no longer reaches the end of the file, so a route table sitting at the very foot can lose its last line(s).
Expected behavior:
When the best window is the final region of the file, the retained window should extend to the end of the content so trailing routes survive.
Suggested approach:
After computing `start`, if the intended window covers the file tail, slice to the end: e.g. `return content.slice(Math.max(0, content.length - room))` when `start + room >= content.length`, or take the max of the line-snapped start and `content.length - room` only when it does not lose the anchored route.
Relevant diff:
```diff
+ const from = Math.min(raws[best].at, content.length - room);
+ const start = content.lastIndexOf("\n", from) + 1;
+ return content.slice(start, start + room);
```
## Your task
Work through every item above — the failed acceptance criteria and each review finding. For each one: understand the gap from "What's wrong now", implement the change so the Required behavior holds (each fix block's `Expected behavior` describes the target state), and use the `Relevant diff` hunks as the anchor for where to edit. After each change, re-verify it resolves the item. Treat **Blocker**-tagged items as required (they block approval); address the rest too.
Tests by DevAsign✅ 6 of 9 criteria verified by tests, 3 failed. Each verdict below links to its evidence. 1 — For a frontend where the entry (e.g. app.tsx) renders `` tags using named constants and the URL literals live in a separate module (e.g. routes.ts), the source shown to the spec author includes that module defining the route literals, rather than stopping before it because breadth-first screen collection exhausted the character budget. (pass)Verdict: pass The route-defining module is included even when large screens alone would exhaust the budget under old breadth-first collection. Test: 2 — Route resolution renders each `` into a URL line by joining the JSX/route entries with the constants that name their paths, producing real URL literals rather than the identifier names (e.g. resolving `ROUTE_PATHS.workflow` to `/workflow — renders WorkflowPage`). (pass)Verdict: pass path={ROUTE_PATHS.workflow} resolves to the literal /workflow rather than the identifier name. Test: 3 — A route that only redirects is rendered as a redirect line naming its target (e.g. `/ — redirects to /agent`, `/security/gate — redirects to /security/config?tab=gate`), not as a route that renders a screen. (pass)Verdict: pass Navigate/Redirect routes are reported as redirectsTo targets, including nested query strings, distinct from rendering routes. Test: 4 — Route resolution handles JSX routes, object routes in either field order, React Router 6.4 `lazy`/`loader` data routes, and file-system routing, and indicates when a map was guessed from filenames rather than read from a route table. (FAIL)Verdict: FAIL isRouteModule fails to recognize a data-route-only file (createBrowserRouter with lazy) as a route table, which the criterion claims should be handled. Test: 5 — Source discovery is separated from budget spending so that exhausting the character budget on large screen files no longer stops the crawl before the cheap plain modules (including the route-defining module) are collected. (pass)Verdict: pass Discovery proceeds past a budget-exhausting screen to collect the cheap plain modules beneath it. Test: 6 — Files are emitted to the spec author in relevance/rank order — the test's targets, the module naming the URLs, the entry and screens on an import path to a target, the labels and sample data they read, then remaining screens — rather than in breadth-first order. (FAIL)Verdict: FAIL Emitted order places labels.ts before WorkflowChild.tsx, whereas the criterion requires on-path screens ahead of the plain data they read. Test: 7 — When a route table sits at the foot of a shell file longer than the per-file character cap, the retained window covers the region containing the most routes rather than truncating head-first and losing the table, so the route table is still resolved to URLs. (FAIL)Verdict: FAIL routeWindow retains the sparse head filler rather than the dense route table at the foot of the over-cap shell, losing the table. Test: 8 — The characters of the route-defining module are held back from the targets' budget so that a set of large changed screens spending the whole budget does not cause the URL map to silently drop to zero lines. (pass)Verdict: pass Route-module characters are reserved so large targets spending the whole budget do not erase the URL map. Test: 9 — The Playwright prompt splices in the resolved URL map plus standing guidance to navigate to the URL that renders the flow and to assert only on data the spec creates or the source shows is built in. (pass)Verdict: pass Playwright prompt splices in the resolved URL map and standing guidance; non-playwright runners omit both. Test: Prompt to fix all failing tests |
The file carrying the <Route> table is usually the app shell itself, and ranking it as a route table took it out of the seed for the plain modules below — so the labels it imports fell past every screen to the foot of the list, which is where they were before this branch started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
DevAsign Code Review
❌ Tests failing (3) · ✅ Fixed since last review (2)
🟡 Merge score: 70/100
10 of 10 acceptance criteria met.
This PR adds route-URL resolution (app-routes.ts), separates source discovery from budget spending in app-source.
Tests by DevAsign✅ 7 of 10 criteria verified by tests, 3 failed. Each verdict below links to its evidence. 1 — For a frontend where the entry (e.g. app.tsx) renders `` tags using named constants and the URL literals live in a separate module (e.g. routes.ts), the source shown to the spec author includes that module defining the route literals, rather than stopping before it because breadth-first screen collection exhausted the character budget. (FAIL)Verdict: FAIL Fixture builds the named case (entry with named-constant Routes, literals in routes.ts, screens exhausting the budget) and routes.ts was dropped while screens filled the budget. Test: 2 — Route resolution renders each `` into a URL line by joining the JSX/route entries with the constants that name their paths, producing real URL literals rather than the identifier names (e.g. resolving `ROUTE_PATHS.workflow` to `/workflow — renders WorkflowPage`). (pass)Verdict: pass path={ROUTE_PATHS.workflow} resolves through routes.ts to the literal URL, not the identifier. Test: 3 — A route that only redirects is rendered as a redirect line naming its target (e.g. `/ — redirects to /agent`, `/security/gate — redirects to /security/config?tab=gate`), not as a route that renders a screen. (pass)Verdict: pass Redirect-only routes are named as redirects to their target across Navigate, Redirect, query-string, and object-route shapes. Test: 4 — Route resolution handles JSX routes, object routes in either field order, React Router 6.4 `lazy`/`loader` data routes, and file-system routing, and indicates when a map was guessed from filenames rather than read from a route table. (pass)Verdict: pass JSX, object, 6.4 lazy/loader, and file-system routing all resolve, and guessed-from-filenames vs read-from-table is distinguished. Test: 5 — Source discovery is separated from budget spending so that exhausting the character budget on large screen files no longer stops the crawl before the cheap plain modules (including the route-defining module) are collected. (pass)Verdict: pass A route module survives discovery even when large screens exhaust the character budget, at two budget levels. Test: 6 — Files are emitted to the spec author in relevance/rank order — the test's targets, the module naming the URLs, the entry and screens on an import path to a target, the labels and sample data they read, then remaining screens — rather than in breadth-first order. (FAIL)Verdict: FAIL Fixture builds the rank-order case and not every crawled file was emitted, so the relevance ordering the criterion names is not produced. Test: 7 — When a route table sits at the foot of a shell file longer than the per-file character cap, the retained window covers the region containing the most routes rather than truncating head-first and losing the table, so the route table is still resolved to URLs. (FAIL)Verdict: FAIL Fixture puts a route table at the foot of an oversized shell and the retained window did not include its last row, so the table is not resolved. Test: 8 — The characters of the route-defining module are held back from the targets' budget so that a set of large changed screens spending the whole budget does not cause the URL map to silently drop to zero lines. (pass)Verdict: pass Large changed screens spending the whole budget still leave the route module and its routes in the output. Test: 9 — The Playwright prompt splices in the resolved URL map plus standing guidance to navigate to the URL that renders the flow and to assert only on data the spec creates or the source shows is built in. (pass)Verdict: pass The prompt splices in the resolved URL map plus the navigate-to-URL and assert-only-on-known-data guidance. Test: 10 — When the file holding the `` table is also the app shell, the plain modules it imports (e.g. a labels module) keep their rank near the shell rather than falling past every screen to the foot of the emitted list. (pass)Verdict: pass Labels imported by a combined shell/route file rank near the shell, not after unrelated screens. Test: Prompt to fix all failing tests |
Nine of one run's eighteen spec timeouts sat on
/waiting for a screen mounted at another path. The specs were not badly written — the author was never told the path existed.What was measured
Running the real
appSourceForagainst this repo's frontend, with the target file from #256/#257:Nine large
.tsxscreens —screen-fund-bounty,screen-bounties,screens-onboardingamong them, none of any use to the test — each hit the 12,000-char cap and together spent exactly 100% of the budget while only 14 of 60 file slots were used. The crawl therefore stopped before the second pass that collects the plain modules, andsrc/routes.tsnever reached the author.app.tsxdid reach it, and contains no URLs at all:22 identifiers. The literals live only in
routes.ts, andDEFAULT_ROUTE = ROUTE_PATHS.agentis exactly whygoto("/")lands on/agentand the spec waits out its timeout.What changed
app-source.ts— emit by relevance, not by breadth. Discovery is separated from spending, so running out of budget no longer stops the crawl before the cheap decisive modules. Files are then emitted in rank order: the test's targets, the module naming the URLs, the entry and the screens on an import path to a target, the labels and sample data those read, then whatever screens are left.app-routes.ts(new) — join the two halves.appRoutesresolves each<Route>through the constants that name it;routeLinesrenders the result into the prompt. It handles JSX routes, object routes either field order, 6.4lazy/loaderdata routes, and file-system routing, and says so when a map was guessed from filenames rather than read from a table.plan.tssplices the URL map into the Playwright prompt and adds two lines of standing guidance: navigate to the URL that renders the flow, and assert only on data the spec creates or the source shows is built in.Result on the same instrument
21 of 21 resolved to real URL literals. The last line is the 9 timeouts.
Counterfactual:
routeLinesover the old 14-file set returns nothing, and overapp.tsxalone returns nothing — even though all 22 of its<Route>tags are inside the window. It is specificallyroutes.tsreaching the author that turns 22 identifiers into 21 URLs.Two defects found by review, both fixed
fileCharswas cut off by head-first truncation — on this repo's ownapp.tsx(45,176 chars, table at 39,721) the feature was inert, returning zero routes.routeWindownow keeps the window covering the most routes.routes.tsfell off the end — the URL map went from 21 lines to none, silently and non-monotonically. Its characters are now held back from the targets.Both are covered by tests that fail without the fix.
What this does not fix
Cause 3 — specs asserting on invented seeded data — is not mechanically closed. The seeded repo name comes from the ephemeral backend seed, not from frontend source, so no amount of source ranking can put it in front of the author. That one rests on the new prompt rule alone, which is a weaker guarantee than the URL map.
Backend suite 1768/1768,
tsc --noEmitclean.🤖 Generated with Claude Code