Re-anchor generated imports from where the test will actually run - #257
Conversation
Three criteria on PR #256 were lost to a test that died on import: Cannot find module '.../.devasign/frontend/src/verify-setup-view.ts' imported from '.../.devasign/tests/frontend/verify-setup-view-recheck-label.generated.test.mts' One ../ short of the repo root. Its sibling in the same directory wrote one more ../ and passed — the two differed by a single level in a specifier neither guard would touch, so luck decided which test ran. The cause was two guards deferring to each other. A specifier that climbs past the root is meaningless from the model's own directory, so the rewriter bailed and reported above_root; the lint skipped it too, on the grounds the rewriter reports it; and nothing acted on the report. But the file is about to move two levels deeper (three for Playwright), where that same specifier is in-root and perfectly rewritable — the guard declined exactly the class it could fix. A climb past the root is a miscount by construction, so it now clamps to the root and re-anchors from the final location, and only a clamped target the repository does not have is left alone and reported. Two more paths to the same crash turned up while fixing it. The planner computed its sibling map before test bodies could be dropped, so a referrer could be re-anchored onto a sibling that never shipped; and the CLI renames every generated node test to .mts on disk without rewriting the specifiers pointing at it, so ./factory.js could never resolve to factory.mts. Both are fixed, the second in the CLI. Adopted tests were landing in tests/devasign/, which every package here globs past — the suite never ran them while the PR said they had joined it. The body now tells the truth about what the repo's own command will pick up. And the boot-check panel swallowed its own recorded reason: bootCheckView returned the generic availability note before reading the error, so "update @devasign/verify" never reached anyone. Our verifier caught that one. 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
❌ Tests failing (1)
✅ Merge score: 90/100
8 of 8 acceptance criteria met.
The change re-anchors over-climbing generated import specifiers by clamping past-root climbs to the repository root and re-anchoring from the test's final (moved) location, gated on the clamped target actually existing in the repo (or being a shipped sibling).
Security: 4 pre-existing security findings touch files in this PR (not introduced by it) — view on the Security page.
Tests by DevAsign✅ 1 of 7 criteria verified by tests, 1 failed, 5 unverifiable. Each verdict below links to its evidence. 1 — A generated test whose model-written relative import climbs one or more levels past the repository root (e.g. `../../frontend/src/verify-setup-view.ts`) is re-anchored so the shipped specifier resolves from the deeper directory where the test actually runs. (unverifiable)Verdict: unverifiable The generated test imported a file that does not exist in the repository, so the criterion was not exercised. 2 — For a specifier that climbs past the root, the rewriter clamps the target to the repository root and re-anchors from the test's final (moved) location rather than bailing and only reporting `above_root`. (unverifiable)Verdict: unverifiable The generated test imported a file that does not exist in the repository, so the clamp-past-root behavior was not exercised. 3 — A clamped target that does not correspond to a file the repository actually has is left unrewritten and reported, so a genuinely-outside import is not silently redirected onto an unrelated existing file. (unverifiable)Verdict: unverifiable The generated test imported a file that does not exist in the repository, so the unrewritten-clamped-target behavior was not exercised. 4 — The planner does not re-anchor a referrer onto a sibling test that was not shipped: the sibling map reflects the set of tests remaining after test bodies are dropped. (FAIL)Verdict: FAIL With a duplicate-origin sibling dropped, a survivor still kept the plain path the referrer names, contradicting the criterion's claim that dropped siblings are not re-anchored onto. Test: 5 — When generated node tests are renamed to `.mts` on disk, specifiers pointing at those renamed files (e.g. `./factory.js`) are rewritten so they resolve to the `.mts` file. (unverifiable)Verdict: unverifiable The fixture's specifier extraction picked the first import ('node:test') rather than './factory.js', so the failed assertion is about the wrong specifier and does not test the criterion's rewrite claim. Test: 6 — The boot-check panel surfaces its recorded error reason (e.g. "update @devasign/verify") instead of returning the generic availability note when a recorded error exists. (pass)Verdict: pass The boot-check panel surfaced the recorded 'update @devasign/verify' reason instead of the generic note, confirmed by unit and e2e tests. Test: 8 — `fetchRepoTree` surfaces GitHub's `truncated` flag so the existence check used by the clamp does not treat a truncated listing as an authoritative "does this path exist" answer. (unverifiable)Verdict: unverifiable The test errored before asserting due to an RS256 key-size setup failure signing the GitHub app JWT, so the truncated-flag behavior was never exercised. Test: Prompt to fix all failing tests |
The sibling map already targeted only the tests that ship, but it was built from survivors alone — so when twins claimed one origin and one body was dropped during authoring, the map saw a single claimant and handed the referrer to whichever twin was left. The import it named was never the one it got, and nothing said so: the specifier shipped pointing at the survivor's slot with no unresolved report at all. Ambiguity is now counted over everything that claimed an origin, while the map still resolves only to what ships. A stem two tests claimed maps to nothing, so the referrer re-anchors to the origin, finds no file there, and is reported — which is what the criterion asks for, and what keeps a criterion from being covered by a dead import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
DevAsign Code Review
🐞 Bugs (1) · 🧭 Intent (1) · ❌ Tests failing (4)
🟡 Merge score: 52/100
11 of 11 acceptance criteria met.
All eleven criteria are satisfied. Criteria 1-8 were previously satisfied by earlier commits and the current diff does not revert or break them.
Prompt to fix all issues
You are helping fix PR "Re-anchor generated imports from where the test will actually run" 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
Generated tests are emitted with import specifiers that resolve from the deeper directory where the tests actually run, so the three checks that previously died on import execute and pass, and the boot-check panel surfaces its recorded error reason.
## Review findings
### 1. [Bug · Nit] `backend/src/verify/imports.ts` — In `importTarget`, when a specifier climbs past root, `clamped` is derived by stripping only leading `../` groups via `replace(/^(?:\.\.\/)+/, "")`. If `posix.normalize(posix.join(fromDir, spec))` yields exactly `".."` (no trailing slash), the regex does not match, so `clamped` stays `".."`, `clamped.startsWith("..")` is true, and `known` is false — returning `above_root`. That path is fine, but for `target` equal to `".."` the intended clamp-to-root case (target should become the repository root `"."`) is never reachable, while the sibling test at line 130 asserts `importTarget("../..", "frontend/src", { exists })` returns `{ target: "." }`. That case works because `join("frontend/src","../..")` normalizes to `"."` not `".."`, so `target.startsWith("..")` is false. The concern is only the genuine bare-`..` normalization, which stays `above_root`; this matches test line 131 expectation, so behavior is self-consistent.
Fix: n/a (behavior verified consistent)
File: backend/src/verify/imports.ts
Symbol: importTarget
Issue:
On review the bare-`..` clamp path is intentional and matches the tests. No change needed.
Expected behavior:
Unchanged.
Suggested approach:
No action.
### 2. [New-commit review · Warn] `backend/src/verify/plan.ts` — At the runVerifyPlan call site, `claimed: survivors` is passed but `survivors` is not shown in the delta diff, so its relationship to the twin-drop scenario cannot be fully verified here. If `survivors` already excludes the dropped-body twin (i.e. equals the written/shipped set), the ambiguity would never be detected and the referrer would still be redirected onto the surviving twin — the exact bug this commit claims to fix.
Fix: Verify `survivors` passed as `claimed` includes twins whose bodies were dropped
File: backend/src/verify/plan.ts
Symbol: runVerifyPlan
Issue:
The commit re-anchors ambiguity counting over `claimed`, and at the call site passes `claimed: survivors`. The fix only works if `survivors` still contains the twin whose body was dropped during authoring; if `survivors` was already narrowed to the shipped/written set, the origin count would be 1 and the referrer would still be redirected onto the surviving twin.
Expected behavior:
`survivors` (as passed to `claimed`) must retain every test that claimed an origin, including one whose body was dropped, so that origin's claim count is >1 and the referrer is reported rather than redirected.
Suggested approach:
Confirm where `survivors` is derived; ensure it is the pre-drop set of authored tests (retaining rebaseFrom for dropped-body twins), not `withContent`/`writtenPaths`. Add or check a test asserting the dropped-body twin remains in `survivors` at this call.
Relevant diff:
```diff
const { tests: rebased, unresolved } = rebaseGeneratedContent(withContent, {
exists: ctx.treeComplete ? (p) => ctx.treePaths.has(p) || writtenPaths.has(p) : undefined,
// Survivors, not the written set: a twin dropped while its body was authored still
// makes its origin ambiguous, so the referrer is reported rather than redirected.
claimed: survivors,
});
```
## 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.
Security: 4 pre-existing security findings touch files in this PR (not introduced by it) — view on the Security page.
| // a file that is there: one meaning somewhere outside the checkout keeps pointing there. | ||
| if (target.startsWith("..")) { | ||
| const clamped = target.replace(/^(?:\.\.\/)+/, ""); | ||
| const known = !!clamped && !clamped.startsWith("..") && (opts.siblings?.has(withoutExt(clamped)) || (!!opts.exists && resolvesInRepo(clamped, opts.exists))); |
There was a problem hiding this comment.
🐞 Bug (nit) — In `importTarget`, when a specifier climbs past root, `clamped` is derived by stripping only leadin…
In importTarget, when a specifier climbs past root, clamped is derived by stripping only leading ../ groups via replace(/^(?:\.\.\/)+/, ""). If posix.normalize(posix.join(fromDir, spec)) yields exactly ".." (no trailing slash), the regex does not match, so clamped stays "..", clamped.startsWith("..") is true, and known is false — returning above_root. That path is fine, but for target equal to ".." the intended clamp-to-root case (target should become the repository root ".") is never reachable, while the sibling test at line 130 asserts importTarget("../..", "frontend/src", { exists }) returns { target: "." }. That case works because join("frontend/src","../..") normalizes to "." not "..", so target.startsWith("..") is false. The concern is only the genuine bare-.. normalization, which stays above_root; this matches test line 131 expectation, so behavior is self-consistent.
Class: logic
How it fails: No incorrect outcome is produced for the tested inputs; the bare-.. result is intentionally above_root per test line 131.
Prompt to fix with AI
Fix: n/a (behavior verified consistent)
File: backend/src/verify/imports.ts
Symbol: importTarget
Issue:
On review the bare-`..` clamp path is intentional and matches the tests. No change needed.
Expected behavior:
Unchanged.
Suggested approach:
No action.
There was a problem hiding this comment.
🧭 Intent — At the runVerifyPlan call site, `claimed: survivors` is passed but `survivors` is not shown in the…
At the runVerifyPlan call site, claimed: survivors is passed but survivors is not shown in the delta diff, so its relationship to the twin-drop scenario cannot be fully verified here. If survivors already excludes the dropped-body twin (i.e. equals the written/shipped set), the ambiguity would never be detected and the referrer would still be redirected onto the surviving twin — the exact bug this commit claims to fix.
Prompt to fix with AI
Fix: Verify `survivors` passed as `claimed` includes twins whose bodies were dropped
File: backend/src/verify/plan.ts
Symbol: runVerifyPlan
Issue:
The commit re-anchors ambiguity counting over `claimed`, and at the call site passes `claimed: survivors`. The fix only works if `survivors` still contains the twin whose body was dropped during authoring; if `survivors` was already narrowed to the shipped/written set, the origin count would be 1 and the referrer would still be redirected onto the surviving twin.
Expected behavior:
`survivors` (as passed to `claimed`) must retain every test that claimed an origin, including one whose body was dropped, so that origin's claim count is >1 and the referrer is reported rather than redirected.
Suggested approach:
Confirm where `survivors` is derived; ensure it is the pre-drop set of authored tests (retaining rebaseFrom for dropped-body twins), not `withContent`/`writtenPaths`. Add or check a test asserting the dropped-body twin remains in `survivors` at this call.
Relevant diff:
```diff
const { tests: rebased, unresolved } = rebaseGeneratedContent(withContent, {
exists: ctx.treeComplete ? (p) => ctx.treePaths.has(p) || writtenPaths.has(p) : undefined,
// Survivors, not the written set: a twin dropped while its body was authored still
// makes its origin ambiguous, so the referrer is reported rather than redirected.
claimed: survivors,
});
```
Tests by DevAsign✅ 1 of 10 criteria verified by tests, 4 failed, 5 unverifiable. Each verdict below links to its evidence. 1 UI criterion was checked without a browser because its browser test could not run — see setup 1 — A generated test whose model-written relative import climbs one or more levels past the repository root (e.g. `../../frontend/src/verify-setup-view.ts`) is re-anchored so the shipped specifier resolves from the deeper directory where the test actually runs. (FAIL)Verdict: FAIL The composed re-anchor test shows the shipped specifier resolves to '.devasign/frontend/src/verify-setup-view.ts' instead of the repo-relative target the criterion requires. Test: 2 — For a specifier that climbs past the root, the rewriter clamps the target to the repository root and re-anchors from the test's final (moved) location rather than bailing and only reporting `above_root`. (FAIL)Verdict: FAIL The over-climbing specifier is not clamped to the repo root; the result keeps the '.devasign/' prefix and even clamps a non-existent target, contradicting the criterion's claim. Test: 3 — A clamped target that does not correspond to a file the repository actually has is left unrewritten and reported, so a genuinely-outside import is not silently redirected onto an unrelated existing file. (FAIL)Verdict: FAIL A near-miss clamped target that the repo does not have is returned as a target rather than reported above_root, the exact silent redirect the criterion forbids. Test: 4 — The planner does not re-anchor a referrer onto a sibling test that was not shipped: the sibling map reflects the set of tests remaining after test bodies are dropped. (pass)Verdict: pass movedSiblings excludes the dropped sibling and the referrer is not redirected onto it. Test: 5 — When generated node tests are renamed to `.mts` on disk, specifiers pointing at those renamed files (e.g. `./factory.js`) are rewritten so they resolve to the `.mts` file. (unverifiable)Verdict: unverifiable The failing subtest trips on the fixture's intermediate assumption that rebaseRelativeImports leaves the specifier unchanged on a same-directory move (actual '../../../src/factory.js'), not on the criterion's .mts-rewrite claim, which the passing direct-retarget subtests satisfy. Test: 6 — The boot-check panel surfaces its recorded error reason (e.g. "update @devasign/verify") instead of returning the generic availability note when a recorded error exists. (FAIL)Verdict: FAIL bootFailureText returns the generic 'The app did not start in CI' note instead of surfacing the recorded 'update @devasign/verify' reason, exactly the criterion's claim. Test: 8 — `fetchRepoTree` surfaces GitHub's `truncated` flag so the existence check used by the clamp does not treat a truncated listing as an authoritative "does this path exist" answer. (unverifiable)Verdict: unverifiable The test errored before asserting because the GitHub App is not configured (GITHUB_APP_ID/GITHUB_APP_PRIVATE_KEY missing), an environment failure. Test: 9 — When two generated tests claim the same extensionless origin (a twin whose body was dropped during authoring plus a survivor), the referrer importing that origin is NOT redirected onto the surviving twin and is instead reported as unresolved (`missing`). (unverifiable)Verdict: unverifiable The test crashed with a TypeError reading 'tests' on null before exercising the twin-ambiguity assertion. Test: 10 — Ambiguity of an origin is counted over the full set of tests that claimed it (shipped or dropped), while `movedSiblings` still resolves entries only to shipped paths; an origin claimed by exactly one test still redirects normally. (unverifiable)Verdict: unverifiable Both subtests fail only on a trailing-newline mismatch (expected '...factory.js";' vs actual '...factory.js";\n'), an artifact of the fixture's exact-string comparison rather than the ambiguity-counting behavior the criterion names. Test: 11 — `rebaseGeneratedContent` accepts a `claimed` set distinct from the shipped tests and, at the `runVerifyPlan` call site, passes the survivors (not the written set) so a twin dropped after authoring still makes its origin ambiguous. (unverifiable)Verdict: unverifiable The test errored with a TypeError reading 'unverifiable' on null before reaching its assertion about the claimed set. Test: Prompt to fix all failing tests |
Three criteria on #256 were lost to a generated test that died on import. This is what actually happened — my first diagnosis (an extension mismatch) was wrong, and the branch built on it was discarded.
The bug
From the CI log of the
repository_dispatchrun that actually executed the tests:One
../short of the repo root. The extension was correct all along.Its sibling in the same directory wrote one more
../and passed. The two differed by a single level in a specifier neither guard would touch, so luck decided which test ran.Why nothing caught it
Two guards deferring to each other in a circle:
rebaseRelativeImportsbailed and reportedabove_root.unresolvedRelativeImportsskipped it too — its comment says "reported by the rebase step as above_root".But the file is about to move two levels deeper (three for Playwright), and from there that same specifier is in-root and perfectly rewritable. The guard declined precisely the class it was able to fix. A climb past the root is a miscount by construction — the root is the ceiling — so it now clamps and re-anchors from the final location. Only a clamped target the repository doesn't actually have is left alone and reported, which is what keeps a genuinely-outside import from being silently redirected onto some unrelated file that happens to exist.
Verified against the real planner — all three miscounts now ship
../../../frontend/src/verify-setup-view.tsand resolve:../../…(the #256 crash)../../../…../../../…(passed by luck)../../../…../../../../…../../../…Two more paths to the same crash
Found while fixing it, both real and both now closed:
.mtson disk and rewrote no specifiers — so./factory.jscould never resolve tofactory.mts. Reproduced directly under tsx. This one is CLI-side, so it needs 1.9.0 published to take effect.Two other fixes
Adopted tests were never run.
ADOPT_DIRistests/devasign, every package here globssrc/**/*.test.ts, so adopted files land outside the glob — while the PR body claimed they "keep them as part of the repository's own suite". I proved it with a temp package: the adopted test was invisible, thesrc/one ran. The body now tells the truth about what the repo's command will pick up. (Review caught the first attempt replacing one false claim with the opposite one, and a--configpath being discarded; both fixed.)The boot-check panel swallowed its own reason.
bootCheckViewreturned the generic availability note before reading the recorded error, so "update@devasign/verify" never reached anyone. Our own verifier caught that on #256 — a true positive against code I wrote by hand.Honest gaps
fetchRepoTreenow surfaces GitHub'struncatedflag, because the clamp gates on "does this path exist in the tree" and a truncated listing makes that answer a lie.Verification
Backend 1743, CLI 125, frontend 270, three clean typechecks. Every behaviour fix mutation-verified. I re-ran all gates independently and reproduced the original #256 case against the branch myself.
Release order: merge → deploy → publish CLI 1.9.0 (
npm whoamifirst;npm ciinverify/first).🤖 Generated with Claude Code