[eb-v2 review] Git import: bind the trust prompt to the repo it was granted for — sign-off requested - #1654
[eb-v2 review] Git import: bind the trust prompt to the repo it was granted for — sign-off requested#1654chiang-daniel wants to merge 1 commit into
Conversation
…, and surface code-tool input validation errors Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThe PR adds validation feedback to the code-tool test panel. It also updates Git import state handling so repository changes clear stale selections, same-repository trust persists, and local imports require a selected path. ChangesCode-tool input validation
Git import state management
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // A required field is missing or an object param is invalid JSON. Keep | ||
| // the dialog open and surface the error so the typed inputs aren't | ||
| // discarded and Run Test can't post stale values. | ||
| input_error = createKilnError(e) |
There was a problem hiding this comment.
The edit-inputs dialog built its param values in a try with an empty catch: when build_params threw (a missing required field, or an object param that isn't valid JSON) the error was swallowed, the dialog closed as if it succeeded, and the just-typed inputs were dropped — Run Test then posted the previous, stale values. Now a build failure keeps the dialog open, returns false, and renders the error inline, so bad input is fixed instead of silently discarded.
| // which the store-only validate_step_requirements can't see. A remount or | ||
| // deep-link to #local-trust starts with no path, so send the user back to | ||
| // pick a file rather than letting Trust Project import an empty path. | ||
| if (step === "local_trust_confirm" && !import_project_path) { |
There was a problem hiding this comment.
The local-trust step's only real input — the selected file path — lives in component state, which the store-only step validator can't see. A remount or a deep-link straight to #local-trust arrives with no path, and the old code let the user reach "Trust Project" and POST an empty path into a server error. This sends them back to the file picker instead. It's paired with the guard in on_local_trust_confirmed so an empty path can't be imported by either route.
| // Seed the url step from the deep-link param. Route through adopt_git_url | ||
| // so a param pointing at a different repo than the persisted session | ||
| // clears its stale downstream state (and its trust skip). | ||
| adopt_git_url(url_param) |
There was a problem hiding this comment.
Deep links carry a ?url= param. Previously this wrote git_url directly and left any persisted session state untouched, which is the same trust-skip hole as the Back-nav path: land on the wizard with a stale clone_path for repo A, deep-link to repo B, and B inherits A's passed trust. Seeding through adopt_git_url means a deep link to a different repo clears that state like any other URL change.
| // it can never be mistaken for the current repo — in particular so the | ||
| // clone_path-based trust skip below only fires for the same repo the trust | ||
| // gate was passed for. | ||
| function adopt_git_url( |
There was a problem hiding this comment.
The trust gate was granted per-repo, but nothing tied it to the repo it was granted for. clone_path, selected_branch and the selected project all described whichever repo was entered earlier; changing the URL and coming back (e.g. via browser Back) left that state in place, so a different repo could sail past the trust prompt on the strength of the previous repo's clone. This routes every URL entry through one choke point that drops the downstream state whenever the URL actually changes, so the clone_path-based trust-skip below can only ever fire for the repo currently being imported.
| extra_fields: Partial<typeof $git_import_wizard_store> = {}, | ||
| ) { | ||
| const url_changed = url !== $git_import_wizard_store.git_url | ||
| update_store({ |
There was a problem hiding this comment.
Personal review requested: This makes the trust prompt fail-safe-annoying — re-entering the same repo URL still skips the prompt, but any different URL re-prompts, including a differently-spelled but equivalent one (trailing slash, .git suffix, http vs https, ssh vs https for the same repo). Is re-prompting on equivalent spellings the right trade for your trust model, or do you want URL normalization before the comparison?
| ...extra_fields, | ||
| ...(url_changed | ||
| ? { | ||
| clone_path: "", |
There was a problem hiding this comment.
Personal review requested: A URL change clears clone_path, branch and project selection, but deliberately keeps the stored credentials/auth — the credentials step re-verifies them against the new repo before any clone. So a token entered for repo A survives a switch to repo B and is re-checked rather than re-entered. Confirm that carrying credentials across a URL change matches your trust intent (the alternative is clearing them too and forcing re-entry).
| // If clone_path is set, the user already passed the trust gate and the | ||
| // branch step redirected back here for credentials — skip trust and | ||
| // return directly to branch. | ||
| // A clone_path here means this repo already reached the branch step, which |
There was a problem hiding this comment.
This is the trust-skip itself: a set clone_path is taken as proof the repo already passed trust and the branch step merely bounced back for credentials. That inference is only sound once clone_path is guaranteed to belong to the current repo, which the adopt_git_url change now enforces. The comment is rewritten to state that dependency so the skip isn't later "simplified" back into a hole.
📊 Coverage ReportOverall Coverage: 93% Diff: origin/review/eb-v2/base...HEADNo lines with coverage information in this diff.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/web_ui/src/lib/components/import/import_project.test.ts (1)
513-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover URL-query initialization.
Line 515 should not assert that
onMountis unavailable because this setup uses@testing-library/svelteon jsdom with Svelte 4. Add a test that seeds?url=before rendering and checks that a different repository clearsclone_path,selected_branch, and project selection state throughadopt_git_url.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web_ui/src/lib/components/import/import_project.test.ts` around lines 513 - 524, Update the ImportProject tests around renderAtUrlStep to cover URL-query initialization under jsdom/Svelte 4 rather than assuming onMount is unavailable. Seed the browser URL with a ?url= value before rendering, then verify that adopting a different repository through adopt_git_url clears clone_path, selected_branch, and the project selection state.app/web_ui/src/lib/components/code_tools/code_tool_test_panel.test.ts (1)
90-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest error recovery in the same component instance.
The success test mounts a new panel, so it cannot detect a missing
input_error = nullreset. Extend the invalid-input flow: provide the required value, invoke Done again, and assert that the action returnstrueand the warning disappears.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/web_ui/src/lib/components/code_tools/code_tool_test_panel.test.ts` around lines 90 - 103, Extend the invalid-input test for CodeToolTestPanel so it reuses the same rendered component after correcting the required field, invokes the Done action again, and verifies it returns true with the “Required property not set” warning removed. Do not rely on the separate “saves and closes when all required fields are provided” test; the same-instance flow must validate input_error reset behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/web_ui/src/lib/components/code_tools/code_tool_test_panel.test.ts`:
- Around line 90-103: Extend the invalid-input test for CodeToolTestPanel so it
reuses the same rendered component after correcting the required field, invokes
the Done action again, and verifies it returns true with the “Required property
not set” warning removed. Do not rely on the separate “saves and closes when all
required fields are provided” test; the same-instance flow must validate
input_error reset behavior.
In `@app/web_ui/src/lib/components/import/import_project.test.ts`:
- Around line 513-524: Update the ImportProject tests around renderAtUrlStep to
cover URL-query initialization under jsdom/Svelte 4 rather than assuming onMount
is unavailable. Seed the browser URL with a ?url= value before rendering, then
verify that adopting a different repository through adopt_git_url clears
clone_path, selected_branch, and the project selection state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b67068c6-7b56-4b79-bd6f-51b56b308e28
📒 Files selected for processing (8)
app/web_ui/src/lib/components/code_tools/code_tool_test_panel.svelteapp/web_ui/src/lib/components/code_tools/code_tool_test_panel.test.tsapp/web_ui/src/lib/components/import/__tests__/step_branch_stub.svelteapp/web_ui/src/lib/components/import/__tests__/step_credentials_stub.svelteapp/web_ui/src/lib/components/import/__tests__/step_url_stub.svelteapp/web_ui/src/lib/components/import/import_project.svelteapp/web_ui/src/lib/components/import/import_project.test.tsapp/web_ui/src/lib/stores/git_import_wizard_store.ts
1 participant
Surface: the Git-import project wizard and the Code Tool test panel. One commit,
e9c4b294c (landed as ccf082f54)— bind git-import trust to the repo URL it was granted for (any URL change clears the stale clone/branch/project state that the trust-skip relies on), refuse a local-trust import with no selected path, and stop the code-tool test panel from silently swallowing input-validation errors. Roughly half the diff is the wizard-stub harness and tests. The trust-model trade-offs are called out in two Personal review requested comments below — those are the ones I'd like your explicit sign-off on.🤖 Generated with Claude Code
CI note: the "Check API Schema Bindings" failure here is an artifact of this review PR's pinned snapshot — the canonical schema verification lives on
dchiang/eb-v2-merge. No action needed from reviewers.