Ask for the boot config in a drawer, and commit the answers - #261
Conversation
The backend half took answers; nothing could supply them. This is the panel that does: the popover becomes a drawer, a checklist says what inference settled and what it could not, and the footer posts the difference as a commit on the setup PR. Answers carry two kinds of silence. A key the maintainer never touched is omitted, so inference keeps it; a key they cleared sends "none", which the merge writes as empty. Confusing the two wipes a working config, so the form keeps them apart and a test pins both directions. Servers can only be edited while a setup PR is open. The panel is told the branch's servers by name alone, so any answer built from that view would replace the list rather than edit it — a one-row answer would have dropped the rest. It says so rather than offering an edit that loses them. A form that builds what the endpoint refuses is the whole feature failing in the maintainer's hands, and nothing crossed that seam before: a check now runs the real answersFromForm into the real validateSetupAnswers over eleven filled-in forms, cleared keys included. 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
No issues found
✅ Merge score: 100/100
11 of 11 acceptance criteria met.
The change adds a pure setup-form module (verify-setup-form.ts) with an extensive node --test suite, rewires the verify panel into a modal drawer, and threads answers through requestSetupPr.
Tests: 11 passed, 0 unverifiable · 4 UI criteria checked without a browser (set up) — see the "Tests by DevAsign" comment.
Security: 1 pre-existing security finding touches files in this PR (not introduced by it) — view on the Security page.
Tests by DevAsign✅ 11 of 11 criteria verified by tests. Each verdict below links to its evidence. 4 UI criteria were checked without a browser because their browser tests could not run — see setup 1 — A key the maintainer never touched is omitted from the built SetupAnswers so inference keeps it, while a key the maintainer cleared is sent as "none" so the merge writes it empty; both directions are exercised. (pass)Verdict: pass Component test confirms an untouched key is omitted while a cleared key is sent as none, exercising both directions. Test: 2 — Clearing servers to "none" merges to an empty server list ({"servers":[]}) and clearing login to "none" merges to an empty login ({"login":{}}). (pass)Verdict: pass Both subtests confirm clearing servers merges to an empty list and clearing login merges to an empty login. Test: 3 — The output of the real answersFromForm passes the real validateSetupAnswers for the covered filled-in forms, including every cleared-key case, so the form never builds answers the validator rejects. (pass)Verdict: pass Twelve round-trip subtests confirm real answersFromForm output passes the real validator across all covered cases. Test: 4 — Because the panel receives the branch's servers by name alone, the drawer does not offer a server edit that would replace the whole list; it states this limitation instead. (pass)Verdict: pass Component test confirms name-only branch servers lock the row with a stated limitation instead of offering a server edit. Test: 5 — An app-start port answer is checked against servers the maintainer left untouched, so a duplicate port between them is detected. (pass)Verdict: pass Component test confirms an app-start port colliding with an untouched server's port is refused. Test: 6 — Clearing a field to "none" does not remove from view the rows the refusal named that the maintainer was told to clear. (pass)Verdict: pass Component test confirms the servers row stays visible before and after clearing to none; the e2e timeout at the repo-select precondition never reached the claim. Test: 7 — A field-scoped refusal is rendered on-screen so that submitting a form with such a refusal is not indistinguishable from a no-op. (pass)Verdict: pass Component and passing e2e test both confirm an invalid-port refusal renders in its own checklist row rather than a silent no-op. Test: 8 — After submission the answers are not reverted; the UI indicates the job was queued rather than silently reverting after 2.5s. (pass)Verdict: pass Unit test confirms requestSetupPr resolves queued so the panel does not revert; the e2e timeout at the repo-select precondition never reached the claim. Test: 9 — The "Re-check setup PR" action appears for a follow-up setup PR on an already-onboarded repo, rather than keying off onboarding.state. (pass)Verdict: pass Component test confirms the re-check action opens off the follow-up flag rather than onboarding state; the e2e runs timed out at the repo-select precondition. Test: 10 — For a merged config that does sign in, the Login and Services sections do not read "None". (pass)Verdict: pass Component test confirms a merged signed-in config reads neither Login nor Services as None; the e2e runs timed out at the repo-select precondition. Test: 11 — The "This repo has no Actions secrets" message is not printed when the list is only the expected-and-confirmed secret names. (pass)Verdict: pass Component and passing e2e test both confirm the no-secrets message is suppressed when the list is only expected-and-confirmed names. Test: |
The servers row said "None — the app boots on its own" for a repo nothing had been read for, sitting under three rows that honestly said "Not checked yet". It is the same claim the login and services rows were just corrected for, in the one row the review did not list. The boot row then printed its own sentence twice: the evidence line resolves to the same bootOkText the checklist row already showed, and only the probe line was guarded against repeating it. The links it carries stay. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
DevAsign Code Review
🧭 Intent (1) · ❌ Tests failing (1)
✅ Merge score: 84/100
13 of 13 acceptance criteria met.
This cumulative PR adds the setup drawer, its pure form/checklist half, and a thorough node --test suite. Criteria 1-11 were satisfied by earlier commits and remain intact in the current diff.
Prompt to fix all issues
You are helping fix PR "Ask for the boot config in a drawer, and commit the 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
A right-side drawer lets a maintainer review inferred setup config, answer or clear each key, and post the differences as a commit on the setup PR, keeping untouched keys (omitted) distinct from cleared keys (sent as "none").
## Review findings
### 1. [New-commit review · Warn] `frontend/src/workflow-header.tsx` — The suppressed evidence line still renders an empty wrapping <div>, and when saidByRow(line) is true the first link's leading separator is correctly dropped, but the outer container may render an empty element with mute/t-warn class. Minor cosmetic risk only.
Fix: Avoid rendering an empty evidence container when the line is suppressed
File: frontend/src/workflow-header.tsx
Symbol: VerifySetupDrawer
Issue:
The commit 040d95a suppresses the duplicated boot evidence line via !saidByRow(bootCheck.evidence.line), but the wrapping <div> with its class still renders even when both the line text is empty and there are no links, potentially leaving an empty styled element.
Expected behavior:
When the evidence line is suppressed and no links exist, no empty div should render; when links exist, they render without a leading separator.
Suggested approach:
Guard the outer div render on (!saidByRow(evidence.line) || evidence.links.length) so an empty container is never emitted; keep the per-link separator logic as-is.
Relevant diff:
```diff
{bootCheck.evidence && (
<div className={bootCheck.evidence.tone === "warn" ? "t-warn" : "mute"}>
- {bootCheck.evidence.line}
- {bootCheck.evidence.links.map((l) => (
+ {!saidByRow(bootCheck.evidence.line) && bootCheck.evidence.line}
+ {bootCheck.evidence.links.map((l, i) => (
<React.Fragment key={l.href}>
- {" · "}
+ {(i > 0 || !saidByRow(bootCheck.evidence.line)) && " · "}
<a className="wf-verify-link" href={l.href} target="_blank" rel="noreferrer">{l.label} <Icon name="external" size={10} /></a>
</React.Fragment>
))}
```
## 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: 1 pre-existing security finding touches files in this PR (not introduced by it) — view on the Security page.
There was a problem hiding this comment.
🧭 Intent — The suppressed evidence line still renders an empty wrapping <div>, and when saidByRow(line) is tru…
The suppressed evidence line still renders an empty wrapping
Prompt to fix with AI
Fix: Avoid rendering an empty evidence container when the line is suppressed
File: frontend/src/workflow-header.tsx
Symbol: VerifySetupDrawer
Issue:
The commit 040d95a suppresses the duplicated boot evidence line via !saidByRow(bootCheck.evidence.line), but the wrapping <div> with its class still renders even when both the line text is empty and there are no links, potentially leaving an empty styled element.
Expected behavior:
When the evidence line is suppressed and no links exist, no empty div should render; when links exist, they render without a leading separator.
Suggested approach:
Guard the outer div render on (!saidByRow(evidence.line) || evidence.links.length) so an empty container is never emitted; keep the per-link separator logic as-is.
Relevant diff:
```diff
{bootCheck.evidence && (
<div className={bootCheck.evidence.tone === "warn" ? "t-warn" : "mute"}>
- {bootCheck.evidence.line}
- {bootCheck.evidence.links.map((l) => (
+ {!saidByRow(bootCheck.evidence.line) && bootCheck.evidence.line}
+ {bootCheck.evidence.links.map((l, i) => (
<React.Fragment key={l.href}>
- {" · "}
+ {(i > 0 || !saidByRow(bootCheck.evidence.line)) && " · "}
<a className="wf-verify-link" href={l.href} target="_blank" rel="noreferrer">{l.label} <Icon name="external" size={10} /></a>
</React.Fragment>
))}
```
Tests by DevAsign✅ 11 of 13 criteria verified by tests, 1 failed, 1 unverifiable. Each verdict below links to its evidence. 4 UI criteria were checked without a browser because their browser tests could not run — see setup 1 — A key the maintainer never touched is omitted from the built SetupAnswers so inference keeps it, while a key the maintainer cleared is sent as "none" so the merge writes it empty; both directions are exercised. (pass)Verdict: pass An untouched key is omitted (inference kept) while a cleared key is sent as 'none' (merge empties it); both directions asserted and passed. Test: 2 — Clearing servers to "none" merges to an empty server list ({"servers":[]}) and clearing login to "none" merges to an empty login ({"login":{}}). (FAIL)Verdict: FAIL The fixture prefills a real server row and login script, ticks none, and expects servers:[] and login:{}, but the merge produced the string 'none' instead of the empty shapes. Test: 3 — The output of the real answersFromForm passes the real validateSetupAnswers for the covered filled-in forms, including every cleared-key case, so the form never builds answers the validator rejects. (pass)Verdict: pass answersFromForm output passes the real validateSetupAnswers across filled and cleared-key cases, including servers=none, login=none, and both together. Test: 4 — Because the panel receives the branch's servers by name alone, the drawer does not offer a server edit that would replace the whole list; it states this limitation instead. (pass)Verdict: pass Servers known only by name lock the row with a limitation message; setup PRs carrying commands are not locked. Test: 5 — An app-start port answer is checked against servers the maintainer left untouched, so a duplicate port between them is detected. (pass)Verdict: pass An app-start port matching an untouched server row is refused as a duplicate naming the start field; moving the port away is accepted. Test: 6 — Clearing a field to "none" does not remove from view the rows the refusal named that the maintainer was told to clear. (unverifiable)Verdict: unverifiable The failing e2e renders a self-authored about:blank HTML/JS mock and asserts a string it sets itself, not the real component, so it does not exercise the criterion's DOM-visibility claim. Test: 7 — A field-scoped refusal is rendered on-screen so that submitting a form with such a refusal is not indistinguishable from a no-op. (pass)Verdict: pass The component test shows an out-of-range port is refused with a renderable error on a real checklist field rather than a silent no-op; the e2e only timed out on boot. Test: 8 — After submission the answers are not reverted; the UI indicates the job was queued rather than silently reverting after 2.5s. (pass)Verdict: pass The component test shows submission moves to a queued state and keeps answers past the 2.5s window with no silent revert; the e2e was skipped, not contradicting. Test: 9 — The "Re-check setup PR" action appears for a follow-up setup PR on an already-onboarded repo, rather than keying off onboarding.state. (pass)Verdict: pass The component test shows a follow-up setup PR on an onboarded repo reads as open via the flag not onboarding.state; the e2e only timed out on boot. Test: 10 — For a merged config that does sign in, the Login and Services sections do not read "None". (pass)Verdict: pass The component test confirms a merged config that signs in does not report Login or Services as None; the e2e only timed out on the repo picker. Test: 11 — The "This repo has no Actions secrets" message is not printed when the list is only the expected-and-confirmed secret names. (pass)Verdict: pass Both the component and the passing e2e confirm the no-secrets message is never printed for a full, confirmed secret list. Test: 12 — A repo whose packages have not been read (no packages seen) renders the servers row as "Not checked yet" with tone "mute", not "None — the app boots on its own"; a repo whose packages have been read and run no server still renders "None — the app boots on its own". (pass)Verdict: pass No packages seen renders 'Not checked yet' with tone mute, while packages read with no server still renders 'None — the app boots on its own'. Test: 13 — When the boot evidence line resolves to the same text already shown by a checklist row (saidByRow), the evidence line text is suppressed and its leading " · " separator is not printed, while the evidence links still render. (pass)Verdict: pass When evidence text matches the row's own text it is suppressed and its ' · ' separator dropped while links still render; differing text is shown with its separator. Test: Prompt to fix all failing tests |
Phase 5 of the browser-tests plan, frontend half. #260 taught the backend to take answers; nothing could supply them. This is the panel that does.
The popover becomes a right-side drawer: a checklist that says what inference settled and what it couldn't, pickers over the real candidate packages, and a footer that posts the difference as a commit on the setup PR.
The distinction the whole thing turns on
Answers carry two kinds of silence:
"none"— the merge writes it empty.Confusing them wipes a working config. The form keeps them apart and a test pins both directions.
A seam nothing tested before
The form builds a
SetupAnswers; the backend validator refuses anything malformed. A mismatch shows up as the maintainer filling in the form, clicking the button, and getting an opaque 400 — and no test in this repo crossed that boundary. There is one now: the realanswersFromForminto the realvalidateSetupAnswers, over eleven filled-in forms including every cleared-key case.One capability deliberately withheld
Servers can only be edited while a setup PR is open. The panel is told the branch's servers by name alone (
defaultYml.servers: Array<{name}>), so any answer built from that view would replace the list rather than edit it — adding one server would have silently dropped the rest. The drawer says so instead of offering an edit that loses them.Lifting this is a backend change: return the full server rows in
browserTests.defaultYml, and the editor can prefill honestly.Nine findings, and the honest reason there were nine
The backend half produced three confirmed findings out of twelve. This one produced nine. The cause is structural, not incidental:
Pre-existing, on this file at HEAD and on 13 files in the app. So a 565-line change to the drawer got no type checking at all, and
node --testcan't render components — the only real coverage is whatever lives in plain.tsmodules. That's exactly why the plan puts the logic inverify-setup-form.ts, and it's why the findings clustered in the component.The confirmed nine, all fixed with tests:
onboarding.state, so it never appeared for a follow-up setup PR on an already-onboarded repo;Three more were raised and refuted on evidence.
I verified four of the fixes myself by executing the real module rather than trusting the report — including the servers case, where my first fixture was wrong and I had to correct it against the declared
defaultYmlshape.Not verified
I did not render the drawer in a browser; it needs a signed-in session and a repo with an open setup PR. Verification is at the logic layer.
Follow-up worth taking
Removing
// @ts-nocheckfromworkflow-header.tsxis 3 errors away — I measured it: two are theanswersFromFormunion failing to narrow under this repo'sstrict: false, one is a<select>value widening tostring. None are runtime defects. That file is now the largest untypechecked surface in the app, and it's the one this PR grew most.frontend 294 / 294,
tsc -b && vite buildclean, backend validator 19 / 19.🤖 Generated with Claude Code