copilot connect UI polish#1052
Conversation
WalkthroughThis PR introduces a comprehensive Prompt Optimization workflow with entitlements checking. Key additions include a new Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Web UI)
participant Server as Server
participant KilnAI as Kiln AI API
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over Client,DB: Check Entitlements Flow
Client->>Server: GET /api/check_entitlements?feature_codes=prompt-optimization
Server->>KilnAI: GET /v1/check_entitlements (with API key)
KilnAI->>KilnAI: Validate feature codes
KilnAI-->>Server: CheckEntitlementsResponse {prompt-optimization: true/false}
Server-->>Client: {prompt-optimization: true/false}
end
rect rgba(150, 200, 100, 0.5)
Note over Client,DB: Prompt Optimization Job Flow
Client->>Server: POST /api/projects/{id}/tasks/{id}/prompt_optimization_jobs/start
Server->>Server: Validate run config & evals
Server->>KilnAI: POST /v1/jobs/prompt_optimization_job/start (with ZIP)
KilnAI->>KilnAI: Queue job
KilnAI-->>Server: {job_id: "...", status: "pending"}
Server->>DB: Save PromptOptimizationJob
Server-->>Client: {job_id, status}
Client->>Server: GET /api/prompt_optimization_jobs/{job_id}/status (polling)
Server->>KilnAI: GET /v1/jobs/prompt_optimization_job/{job_id}/status
KilnAI-->>Server: {status: "running"/"succeeded"}
Server-->>Client: {status: "running"/"succeeded"}
alt Job Succeeded
Server->>KilnAI: GET /v1/jobs/prompt_optimization_job/{job_id}/result
KilnAI-->>Server: {optimized_prompt: "..."}
Server->>Server: Create Prompt from optimized text
Server->>Server: Create RunConfig from optimized Prompt
Server->>DB: Save Prompt & RunConfig
Server->>DB: Update PromptOptimizationJob with artifact IDs
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
📊 Coverage ReportOverall Coverage: 91% Diff: origin/main...HEAD
Summary
Line-by-lineView line-by-line diff coverageapp/desktop/studio_server/copilot_api.pyLines 205-213 205 client=client,
206 body=questioner_input,
207 )
208 )
! 209 result = unwrap_response(
210 detailed_result,
211 none_detail="Failed to generate clarifying questions for spec. Please try again.",
212 )Lines 231-239 231 detailed_result = await refine_spec_with_answers_v1_copilot_refine_spec_with_answers_post.asyncio_detailed(
232 client=client,
233 body=submit_input,
234 )
! 235 result = unwrap_response(
236 detailed_result,
237 none_detail="Failed to refine spec with question answers. Please try again.",
238 )app/desktop/studio_server/eval_api.pyLines 490-498 490 status_code=400,
491 detail="Cannot update this run config.",
492 )
493 if request.name is not None:
! 494 run_config.name = request.name
495 if request.starred is not None:
496 run_config.starred = request.starred
497 if request.prompt_name is not None:
498 if run_config.prompt is None:app/desktop/studio_server/prompt_optimization_job_api.pyLines 146-162 146 Raises exceptions on failure.
147 """
148 parent_project = task.parent_project()
149 if parent_project is None:
! 150 raise HTTPException(status_code=500, detail="Task has no parent project")
151
152 if not parent_project.id or not task.id:
! 153 raise HTTPException(
154 status_code=500, detail="Task has no parent project or task"
155 )
156
157 if not prompt.id:
! 158 raise HTTPException(status_code=500, detail="Prompt has no ID")
159
160 # get the original target run config that we optimized for in the job
161 target_run_config = task_run_config_from_id(
162 parent_project.id, task.id, prompt_optimization_job.target_run_config_idLines 218-226 218 or not parent_project.id
219 or not task.id
220 or not prompt_optimization_job.id
221 ):
! 222 raise ValueError("Cannot reload Prompt Optimization job: missing required IDs")
223
224 # reload the job in case artifacts were created by another request while waiting for the lock
225 reloaded_job = prompt_optimization_job_from_id(
226 parent_project.id,Lines 229-242 229 )
230
231 # check if artifacts already exist
232 if reloaded_job.created_prompt_id:
! 233 prompt_optimization_job.created_prompt_id = reloaded_job.created_prompt_id
! 234 prompt_optimization_job.created_run_config_id = (
235 reloaded_job.created_run_config_id
236 )
! 237 prompt_optimization_job.optimized_prompt = reloaded_job.optimized_prompt
! 238 return
239
240 detailed_response = await get_prompt_optimization_job_result_v1_jobs_prompt_optimization_job_job_id_result_get.asyncio_detailed(
241 job_id=prompt_optimization_job.job_id,
242 client=server_client,Lines 357-365 357 return CheckRunConfigResponse(is_supported=False)
358
359 server_client = get_authenticated_client(_get_api_key())
360 if not isinstance(server_client, AuthenticatedClient):
! 361 raise HTTPException(
362 status_code=500, detail="Server client not authenticated"
363 )
364
365 detailed_response = await check_prompt_optimization_model_supported_v1_jobs_prompt_optimization_job_check_model_supported_get.asyncio_detailed(Lines 368-376 368 model_provider_name=model_provider.value,
369 )
370 response = unwrap_response(detailed_response)
371
! 372 return CheckRunConfigResponse(is_supported=response.is_model_supported)
373
374 except HTTPException:
375 raise
376 except Exception as e:Lines 424-432 424 )
425
426 server_client = get_authenticated_client(_get_api_key())
427 if not isinstance(server_client, AuthenticatedClient):
! 428 raise HTTPException(
429 status_code=500, detail="Server client not authenticated"
430 )
431
432 # EvalConfig.model_provider is already a string, no need for .valueLines 593-602 593 for job in batch
594 ],
595 return_exceptions=True,
596 )
! 597 except Exception as e:
! 598 logger.error(
599 f"Error updating Prompt Optimization job statuses: {e}",
600 exc_info=True,
601 )Lines 629-638 629 await update_prompt_optimization_job_and_create_artifacts(
630 prompt_optimization_job, server_client
631 )
632 )
! 633 except Exception as e:
! 634 logger.error(
635 f"Error updating Prompt Optimization job status: {e}", exc_info=True
636 )
637
638 return prompt_optimization_jobLines 646-654 646 """
647 try:
648 server_client = get_authenticated_client(_get_api_key())
649 if not isinstance(server_client, AuthenticatedClient):
! 650 raise HTTPException(
651 status_code=500, detail="Server client not authenticated"
652 )
653
654 detailed_response = await get_job_status_v1_jobs_job_type_job_id_status_get.asyncio_detailed(Lines 666-678 666 )
667
668 except HTTPException:
669 raise
! 670 except Exception as e:
! 671 logger.error(
672 f"Error getting prompt optimization job status: {e}", exc_info=True
673 )
! 674 raise HTTPException(
675 status_code=500,
676 detail=f"Failed to get Prompt Optimization job status: {str(e)}",
677 )Lines 685-693 685 """
686 try:
687 server_client = get_authenticated_client(_get_api_key())
688 if not isinstance(server_client, AuthenticatedClient):
! 689 raise HTTPException(
690 status_code=500, detail="Server client not authenticated"
691 )
692
693 detailed_response = await get_prompt_optimization_job_result_v1_jobs_prompt_optimization_job_job_id_result_get.asyncio_detailed(app/desktop/studio_server/utils/copilot_utils.pyLines 90-98 90 client=client,
91 body=generate_input,
92 )
93 )
! 94 result = unwrap_response(
95 detailed_result,
96 none_detail="Failed to generate synthetic data for spec. Please try again.",
97 )libs/core/kiln_ai/cli/commands/package_project.pyLines 441-449 441 task_prompts: dict[str, Prompt] = {}
442
443 for task in tasks:
444 if task.id is None:
! 445 raise ValueError(f"Task '{task.name}' ID is not set")
446
447 run_config = run_configs[task.id]
448 prompt_id = run_config.run_config_properties.prompt_idLines 813-821 813 Returns:
814 List of eval IDs that were actually exported (intersection of eval_ids and task's evals)
815 """
816 if exported_task.path is None:
! 817 raise ValueError("Exported task path is not set")
818
819 exported_eval_ids: list[str] = []
820 evals = task.evals(readonly=True)
821 eval_ids_set = set(eval_ids)Lines 825-833 825 for eval_obj in evals:
826 if eval_obj.id not in eval_ids_set:
827 continue
828 if eval_obj.path is None:
! 829 raise ValueError(f"Eval '{eval_obj.name}' path is not set")
830
831 eval_dir = eval_obj.path.parent
832 dest_dir = exported_task.path.parent / "evals" / eval_dir.nameLines 844-854 844 project: The source project
845 exported_project: The exported project to copy documents into
846 """
847 if project.path is None:
! 848 raise ValueError("Project path is not set")
849 if exported_project.path is None:
! 850 raise ValueError("Exported project path is not set")
851
852 source_docs_dir = project.path.parent / "documents"
853 if not source_docs_dir.exists() or not source_docs_dir.is_dir():
854 returnLines 864-874 864 task: The source task containing the runs
865 exported_task: The exported task to copy runs into
866 """
867 if task.path is None:
! 868 raise ValueError(f"Task '{task.name}' path is not set")
869 if exported_task.path is None:
! 870 raise ValueError("Exported task path is not set")
871
872 source_runs_dir = task.path.parent / "runs"
873 if not source_runs_dir.exists() or not source_runs_dir.is_dir():
874 returnLines 885-893 885 """
886 run_configs = task.run_configs()
887 run_config = next((rc for rc in run_configs if rc.id == run_config_id), None)
888 if not run_config:
! 889 raise ValueError(
890 f"Run config '{run_config_id}' not found for task '{task.name}' (ID: {task.id})"
891 )
892 return run_configlibs/core/kiln_ai/datamodel/eval.pyLines 439-447 439 Generates a tag-based filter ID from the eval name following the convention
440 used by spec-based evals (e.g., "train_{name_slug}").
441 """
442 if self.id is None:
! 443 return self
444
445 if not self._loaded_from_file:
446 return selflibs/core/kiln_ai/datamodel/prompt_optimization_job.pyLines 4-12 4
5 from kiln_ai.datamodel.basemodel import FilenameString, KilnParentedModel
6
7 if TYPE_CHECKING:
! 8 from kiln_ai.datamodel.task import Task
9
10
11 class PromptOptimizationJob(KilnParentedModel):
12 """libs/core/kiln_ai/datamodel/task.pyLines 188-196 188
189 def prompt_optimization_jobs(
190 self, readonly: bool = False
191 ) -> list[PromptOptimizationJob]:
! 192 return super().prompt_optimization_jobs(readonly=readonly) # type: ignore
193
194 # Workaround to return typed parent without importing Task
195 def parent_project(self) -> Union["Project", None]:
196 if self.parent is None or self.parent.__class__.__name__ != "Project":
|
Summary of ChangesHello @sfierro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: automatic prompt optimization. Users can now leverage Kiln's Copilot capabilities to refine their prompts based on evaluation results and training data, aiming to maximize prompt quality. Alongside this, the PR includes extensive UI/UX polish, particularly in the newly structured 'Optimize' section, the models browsing experience, and prompt creation workflows. Backend changes involve new API endpoints for managing optimization jobs, updated data models, and a refactored approach to API response error handling, ensuring a more robust and user-friendly experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request polishes the UI for the Copilot connect flow by enhancing the Intro component to support markdown, which improves the descriptive text in copilot_required_card. It also adds helpful 'Learn More' links and clarifies text in the entitlement_required_card. My feedback points out a hardcoded URL in a generic component that could affect reusability. Overall, these are positive UI improvements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/desktop/studio_server/api_client/kiln_ai_server_client/api/jobs/get_sample_job_result_v1_jobs_sample_job_job_id_result_get.py (1)
14-25:⚠️ Potential issue | 🟡 MinorRemove manual edit to auto-generated file.
This file is explicitly marked as auto-generated with a warning: "Important: This folder is automatically generated. Do not make any edits, they will be replaced on next generation." The blank line added at line 17 is a manual edit that will be silently lost on the next code generation run. Remove the blank line and avoid hand-editing files in
api_client/kiln_ai_server_client/. If the API definition needs updating, regenerate the client from the source OpenAPI spec using the generation script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/api_client/kiln_ai_server_client/api/jobs/get_sample_job_result_v1_jobs_sample_job_job_id_result_get.py` around lines 14 - 25, The file contains a manual blank-line edit inside the auto-generated function _get_kwargs; remove the added blank line so the function matches the generator output exactly and do not hand-edit files under the api_client/kiln_ai_server_client folder; if you need changes to the endpoint URL or signature, update the OpenAPI spec and re-run the client generation script instead of modifying _get_kwargs or other generated functions.app/web_ui/src/lib/ui/run_config_component/create_new_run_config_dialog.svelte (1)
74-80:⚠️ Potential issue | 🟡 Minor
submit_labelshould reflect the current mode.The submit button reads "Create" even in clone mode, which is misleading for users. Since
dialog_titleis already made reactive for the title, the same pattern should apply to the submit label.🛠️ Proposed fix
+ $: submit_label = mode === "create" ? "Create" : "Clone"<FormContainer submit_visible={true} - submit_label="Create" + {submit_label} on:submit={create_new_run_config}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/run_config_component/create_new_run_config_dialog.svelte` around lines 74 - 80, Replace the hardcoded submit_label="Create" with a reactive submit label tied to the dialog's mode (e.g. use submit_label={submit_label} or submit_label={dialog_submit_label}) and add a reactive statement that derives submit_label from the existing dialog_title (or the mode variable) so the button reads "Clone" in clone mode and "Create" (or another appropriate verb) in create mode; keep on:submit={create_new_run_config} and other props unchanged.app/desktop/studio_server/test_eval_api.py (1)
1644-1740:⚠️ Potential issue | 🟡 MinorAdd assertion for the new
train_dataset_sizefield intest_get_eval_progress.The
get_eval_progressendpoint now returnstrain_dataset_size(line 693 of eval_api.py), but the test does not verify this field. Addassert result["train_dataset_size"] == 0(sincemock_eval.train_set_filter_idis None, no train dataset is fetched).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/test_eval_api.py` around lines 1644 - 1740, The test test_get_eval_progress is missing an assertion for the new train_dataset_size field returned by the get_eval_progress endpoint; update the test_get_eval_progress function to assert result["train_dataset_size"] == 0 (mock_eval.train_set_filter_id is None so no train dataset is fetched) after the existing response checks, keeping the rest of the mocks and assertions intact.
🧹 Nitpick comments (38)
app/web_ui/src/lib/ui/icons/star_icon.svelte (1)
6-17: LGTM — consider addingaria-hidden="true"for decorative usage.The
filledprop toggle and fill/stroke logic are correct. As this SVG is purely decorative, addingaria-hidden="true"prevents screen readers from announcing it as an unlabelled image. If the icon is ever used in a meaningful, interactive context, a wrapping<button>witharia-labelwould be the appropriate place for the label instead.♿ Suggested accessibility improvement
<svg class="w-full h-full" viewBox="0 0 24 24" fill={filled ? "currentColor" : "none"} xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/icons/star_icon.svelte` around lines 6 - 17, The SVG in star_icon.svelte is decorative; add aria-hidden="true" to the <svg> element (the element using the filled prop and fill/stroke logic) so screen readers ignore it, and ensure any interactive wrapper (e.g., a <button> that uses this icon) provides an accessible name via aria-label or similar when the icon becomes meaningful.app/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/add_data/+page.svelte (1)
118-118: Redundantreason &&guard —reasonis always truthy.
reasonis typed asReason("generic" | "eval" | "fine_tune") and is assigned via a reactive statement (lines 35–39) that always falls back to"generic"— it is nevernull,undefined, or an empty string. Thereason &&sub-expression is therefore a no-op and only thereason !== "generic"clause does real work.♻️ Simplify both guards
- if (reason && reason !== "generic") params.set("reason", reason) + if (reason !== "generic") params.set("reason", reason)- if (reason && reason !== "generic") params.set("reason", reason) + if (reason !== "generic") params.set("reason", reason)Also applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/dataset/[project_id]/[task_id]/add_data/+page.svelte at line 118, The guard "reason &&" is redundant because reason is always a valid Reason ("generic" | "eval" | "fine_tune"); remove the "reason &&" portion and only check reason !== "generic" before calling params.set("reason", reason). Update both occurrences that call params.set("reason", reason) so they use if (reason !== "generic") params.set("reason", reason) (leaving all other logic untouched).app/web_ui/src/lib/utils/link_builder.test.ts (1)
80-107: Missing coverage for plain (no-::) prompt IDs after removing thegenerator_detailsfallback.The
"saved prompts (ID style)"group only testsprompt_idvalues containing::. The core behavioral change — that prompts without::(previously routed togenerator_details) now also resolve to thesavedpath — has no test. Adding a case like"my-custom-prompt"→/prompts/.../saved/my-custom-promptwould explicitly document and guard this new behaviour.✅ Proposed additional test case
describe("saved prompts (ID style)", () => { + it("routes plain (non-colon) prompt IDs to the saved path", () => { + const promptId = "my-custom-prompt" + const result = prompt_link(mockProjectId, mockTaskId, promptId) + + expect(result).toBe( + `/prompts/${mockProjectId}/${mockTaskId}/saved/my-custom-prompt`, + ) + }) + it("links to saved prompts for prompts with double colons", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/utils/link_builder.test.ts` around lines 80 - 107, Add a unit test to cover plain saved prompt IDs without any "::" so the new behavior (routing plain IDs to the saved path instead of generator_details) is asserted; locate the "saved prompts (ID style)" describe block in link_builder.test.ts and add a test that uses prompt_link(mockProjectId, mockTaskId, "my-custom-prompt") and expects `/prompts/${mockProjectId}/${mockTaskId}/saved/my-custom-prompt`; ensure the test name clearly states it handles plain (no-"::") prompt IDs and mirrors the existing style of the other cases.app/desktop/studio_server/api_client/kiln_ai_server_client/models/specification_input_spec_field_current_values.py (1)
1-16: Auto-generatedattrs-based model — Pydantic guideline doesn't apply here.This file follows the
openapi-python-clientgeneration pattern (@_attrs_define,TypeVar-boundfrom_dict/to_dict,additional_propertiesbag). The project guideline to use Pydantic v2 targets hand-authored data-validation code, not generated client stubs, so no action is required. Worth documenting this exclusion in the project guidelines if it isn't already.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/api_client/kiln_ai_server_client/models/specification_input_spec_field_current_values.py` around lines 1 - 16, This autogenerated attrs-based model (class SpecificationInputSpecFieldCurrentValues with additional_properties) is intentionally not migrated to Pydantic; add a short inline comment at the top of this file explaining it's generated by openapi-python-client and is excluded from the project's Pydantic-v2 guideline (or add that exclusion to the project guidelines doc), so reviewers know this pattern is intentional and should not be changed.libs/core/kiln_ai/cli/commands/package_project.py (2)
786-796:_ignore_eval_config_runsrelies on directory naming convention — consider adding a comment or assertion.The callback assumes the on-disk hierarchy is exactly
evals/{eval_id}/configs/{config_id}/runs/.... If a directory happens to be namedconfigsat a different level, it would incorrectly strip itsrunschild. This is fine given the known Kiln directory structure, but a brief inline comment noting the expected path pattern would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/kiln_ai/cli/commands/package_project.py` around lines 786 - 796, The helper _ignore_eval_config_runs assumes the directory path follows evals/{eval_id}/configs/{config_id}/runs and thus only checks parent.name == "configs"; add a short inline comment stating the exact expected on-disk pattern and add a defensive assertion or path check in _ignore_eval_config_runs (e.g., assert "evals" in dir_path.parents or verify parent.parent.name looks like an eval id) to make the assumption explicit and fail-fast if the directory structure differs; reference the function name _ignore_eval_config_runs and the dir_path/parent.parent logic when adding the comment and assertion.
779-783: Consider using PydanticBaseModelfor consistency with the rest of the codebase.The rest of the Kiln data model uses Pydantic. While a plain
@dataclassworks fine for this simple config, usingBaseModelwould maintain consistency and enable validation if fields grow in the future. As per coding guidelines, "Use Pydantic v2 (not v1) for data validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/kiln_ai/cli/commands/package_project.py` around lines 779 - 783, Replace the dataclass with a Pydantic v2 model for consistency: remove the `@dataclass` decorator and make PackageForTrainingConfig inherit from pydantic.BaseModel (import BaseModel from pydantic), keep the same typed fields and default values (include_documents, exclude_task_runs, exclude_eval_config_runs), and adjust any usages if they relied on dataclass-specific behavior; you may add a model_config dict if you need custom validation/serialization options later.libs/core/kiln_ai/cli/commands/test_package_project.py (2)
1560-1560: Remove theOUTPUT_TO_CWDdebug flag.This module-level constant appears to be a developer convenience for writing exported zips to CWD during local debugging (used on line 1737). It shouldn't be committed — if someone accidentally sets it to
True, the test will write files outsidetmp_path, polluting the working directory and potentially causing non-deterministic test behavior.Proposed fix
-OUTPUT_TO_CWD = False - - class TestPackageProjectForTraining: def test_full_package_with_evals_and_docs( self, temp_project_with_evals, tmp_path: Path ): source = temp_project_with_evals - output_path = ( - Path("./exported_training.zip") - if OUTPUT_TO_CWD - else tmp_path / "output" / "training.zip" - ) + output_path = tmp_path / "output" / "training.zip"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/kiln_ai/cli/commands/test_package_project.py` at line 1560, Remove the module-level debug flag OUTPUT_TO_CWD and any branch that writes exported zips to the current working directory; instead always write test outputs to the provided tmp_path. Locate uses of OUTPUT_TO_CWD (and the conditional logic that writes to CWD, referenced where exported zips are created) and replace them so the output path is derived only from the test's tmp_path fixture (or a local variable defaulting to tmp_path), ensuring no code path writes to os.getcwd() or CWD during tests.
1672-1704: Missing temp directory cleanup inTestExportEvals.
create_export_directorycreates atempfile.mkdtemp-based directory. Existing tests (e.g.,TestExportTask,TestSavePromptToTask) wrap this intry/finallywithshutil.rmtree. These tests don't, relying ontmp_pathcleanup — butcreate_export_directorywrites to its owntempfile.mkdtemp, not undertmp_path.Example fix for `test_exports_selected_evals`
def test_exports_selected_evals(self, temp_project_with_evals, tmp_path: Path): """Test that only selected eval IDs are exported with full subtree.""" + import shutil source = temp_project_with_evals - _, exported_project = create_export_directory(source["project"]) - exported_task, _ = export_task( - source["task"], source["run_config"], exported_project - ) + temp_dir, exported_project = create_export_directory(source["project"]) + try: + exported_task, _ = export_task( + source["task"], source["run_config"], exported_project + ) - exported_ids = export_evals(source["task"], [source["eval1"].id], exported_task) + exported_ids = export_evals(source["task"], [source["eval1"].id], exported_task) - assert exported_ids == [source["eval1"].id] - exported_evals = exported_task.evals(readonly=True) - assert len(exported_evals) == 1 - assert exported_evals[0].id == source["eval1"].id + assert exported_ids == [source["eval1"].id] + exported_evals = exported_task.evals(readonly=True) + assert len(exported_evals) == 1 + assert exported_evals[0].id == source["eval1"].id - configs = exported_evals[0].configs(readonly=True) - assert len(configs) == 1 - assert configs[0].id == source["eval1_config"].id + configs = exported_evals[0].configs(readonly=True) + assert len(configs) == 1 + assert configs[0].id == source["eval1_config"].id + finally: + shutil.rmtree(temp_dir, ignore_errors=True)Same applies to
test_exports_no_evals_when_ids_dont_match,TestExportDocuments, andTestExportTaskRuns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/kiln_ai/cli/commands/test_package_project.py` around lines 1672 - 1704, The tests in TestExportEvals (methods test_exports_selected_evals and test_exports_no_evals_when_ids_dont_match) call create_export_directory which returns a tempfile.mkdtemp-created directory that is not under tmp_path and must be explicitly removed; update each test to wrap the export-directory creation and subsequent assertions in a try/finally (or ensure cleanup) and call shutil.rmtree on the directory returned by create_export_directory in the finally block (referencing the local var holding the exported_project/export_dir), similar to existing patterns in TestExportTask and TestSavePromptToTask; do the same cleanup change for other tests mentioned (TestExportDocuments, TestExportTaskRuns) that also use create_export_directory.app/web_ui/src/lib/ui/float.svelte (1)
23-55: Prop changes toplacement,offset_px,shift_padding, andstrategydon't trigger repositioning.
autoUpdateadds listeners that automatically call an update function when the DOM layout changes (scroll, resize, ResizeObserver) — but not when Svelte props change. If a parent togglesplacementat runtime, the float silently stays in its old position.Additionally, changing
strategyfromfixedtoabsoluteafter mount updates the Tailwind class correctly but leavescleanupAutoUpdatein place with stale listener configuration; the guard on Line 43 blocks a clean restart.Add a reactive statement and a helper to restart autoUpdate on strategy changes:
♻️ Proposed fix
+ $: placement, strategy, offset_px, shift_padding, updatePosition() + + $: if (strategy && contentElement) { + stopAutoUpdate() + startAutoUpdate() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/float.svelte` around lines 23 - 55, The component currently never restarts autoUpdate when props like placement, offset_px, shift_padding, or strategy change; add a small helper restartAutoUpdate that calls stopAutoUpdate() then startAutoUpdate(), adjust startAutoUpdate so it doesn't bail out when cleanupAutoUpdate already exists (or simply call stopAutoUpdate before creating a new autoUpdate), and add a reactive statement (e.g., $: restartAutoUpdate() or an equivalent Svelte reactive block) that watches placement, offset_px, shift_padding, and strategy and calls restartAutoUpdate so computePosition listeners are re-created with up-to-date middleware and strategy; keep references to updatePosition, startAutoUpdate, stopAutoUpdate, cleanupAutoUpdate, and autoUpdate when making the changes.app/web_ui/src/lib/ui/feature_carousel.svelte (1)
69-69: Hardcodedtext-gray-500/bg-gray-200break DaisyUI themingThese fixed Tailwind palette values won't respond to DaisyUI dark-mode or custom themes. Replacing them with DaisyUI semantic tokens keeps the component theme-aware.
♻️ Proposed fix
- <div class="text-xs text-gray-500 font-medium mt-1"> + <div class="text-xs text-base-content/50 font-medium mt-1">- <span class="text-gray-500 w-24 text-xs">{label}</span> + <span class="text-base-content/50 w-24 text-xs">{label}</span>- class="h-3 rounded-full flex-1 {i < value + class="h-3 rounded-full flex-1 {i < value ? 'bg-secondary' - : 'bg-gray-200'}" + : 'bg-base-300'}"As per coding guidelines, DaisyUI should be used for UI components in the web frontend; semantic DaisyUI tokens (
base-content,base-300) are the appropriate replacements for raw Tailwind palette grays.Also applies to: 82-82, 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/feature_carousel.svelte` at line 69, Replace hardcoded Tailwind gray utilities in the FeatureCarousel component with DaisyUI semantic tokens so themes/dark mode work: update the class "text-xs text-gray-500 font-medium mt-1" to use "text-xs text-base-content font-medium mt-1" and replace any "bg-gray-200" instances (and the other occurrences noted around lines 82 and 88) with "bg-base-300" (or the appropriate DaisyUI semantic bg token) in the feature_carousel.svelte markup to make the component theme-aware.libs/server/kiln_server/prompt_api.py (1)
103-111: Guard against duplicatecreated_atkey ifBasePromptgains the field.Currently
BasePromptdoes not havecreated_at, sotask_run_config.prompt.model_dump(exclude={"id"})is safe. However, ifBasePromptis ever extended withcreated_at, unpacking**propertiesalongside the explicitcreated_at=task_run_config.created_aton line 108 would raiseTypeError: multiple values for keyword argument 'created_at'.A defensive fix would exclude
created_atfrom the dump:🛡️ Defensive fix
- properties = task_run_config.prompt.model_dump(exclude={"id"}) + properties = task_run_config.prompt.model_dump(exclude={"id", "created_at"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/server/kiln_server/prompt_api.py` around lines 103 - 111, The ApiPrompt construction may get duplicate created_at if BasePrompt later adds that field; update the call that builds properties from task_run_config.prompt.model_dump to exclude "created_at" (in addition to "id") so that when creating ApiPrompt with created_at=task_run_config.created_at you won't pass duplicate keyword args; modify the model_dump(exclude=...) invocation used before ApiPrompt(...) to include "created_at" in the exclusion.libs/core/kiln_ai/datamodel/test_task.py (1)
408-408: DeferredPromptOptimizationJobimport inconsistent with rest of file
PromptOptimizationJobis imported inside both test functions, while all other types in this file (Spec,Task,RunConfigProperties, etc.) are imported at the module top. SincePromptOptimizationJobis now exported fromkiln_ai.datamodel, there's no circular-import reason to defer it.♻️ Suggested fix — move import to top of file
from kiln_ai.datamodel.task import RunConfigProperties, Task, TaskRunConfig from kiln_ai.datamodel.task_output import normalize_rating +from kiln_ai.datamodel import PromptOptimizationJobThen remove both inline
from kiln_ai.datamodel import PromptOptimizationJoblines inside the test bodies.Also applies to: 432-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/kiln_ai/datamodel/test_task.py` at line 408, Move the deferred import of PromptOptimizationJob to the module top alongside the other model imports (i.e., add "from kiln_ai.datamodel import PromptOptimizationJob" with the existing top-level imports) and remove the two inline "from kiln_ai.datamodel import PromptOptimizationJob" lines inside the test functions (the inline imports currently present around lines referenced by the review). This keeps imports consistent with Spec/Task/RunConfigProperties and avoids duplicate imports in the test bodies.libs/core/kiln_ai/datamodel/prompt_optimization_job.py (1)
25-28:latest_statusaccepts any string — consider constraining to known valuesThe docstring documents five legal values (
pending,running,succeeded,failed,cancelled) but the field is typed as plainstr, allowing arbitrary values to be stored silently.Literalwould make typos and unexpected statuses visible at the type-checker level without adding runtime cost.♻️ Suggested change
+from typing import Literal, TYPE_CHECKING ... - latest_status: str = Field( + latest_status: Literal["pending", "running", "succeeded", "failed", "cancelled"] = Field( default="pending", description="The latest known status of this prompt optimization job (pending, running, succeeded, failed, cancelled). Not updated in real time.", )If the remote server can return statuses not in this list, keep
strbut add a note explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/kiln_ai/datamodel/prompt_optimization_job.py` around lines 25 - 28, The latest_status Field currently typed as str allows arbitrary values; change its annotation to use typing.Literal with the five allowed values (Literal["pending", "running", "succeeded", "failed", "cancelled"]) so type-checkers will catch invalid statuses, and keep the Field(...) call but remove any conflicting runtime validation; alternatively, if the remote API can return other statuses, leave latest_status as str but add a clarifying comment/docstring on the field explaining why it remains unconstrained; refer to the latest_status declaration in prompt_optimization_job.py when making this change.app/web_ui/src/lib/utils/name_generator.ts (1)
1-52: Optional: useas constfor immutable vocabulary arrays.The
string[]annotation is redundant (TypeScript infers it) and allows accidental runtime mutation of the arrays.as constwould give readonly tuples and prevent mutation at zero cost.♻️ Proposed refactor
-const ADJECTIVES: string[] = [ +const ADJECTIVES = [ "Curious", ... "Imperial", -] +] as const -const NOUNS: string[] = [ +const NOUNS = [ "Penguin", ... "Ivy", -] +] as constThe index expressions
Math.floor(Math.random() * ADJECTIVES.length)work identically on readonly tuples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/utils/name_generator.ts` around lines 1 - 52, Replace the mutable typed arrays (e.g., the ADJECTIVES declaration) with readonly tuple literals using "as const" instead of "string[]" to prevent runtime mutation and remove the redundant explicit type; for example change the ADJECTIVES declaration to a const initialized array with "as const" (and do the same for any sibling vocabulary arrays in this file), leaving existing index usages like Math.floor(Math.random() * ADJECTIVES.length) unchanged since readonly tuples support the same operations.app/web_ui/src/lib/ui/markdown_block.svelte (1)
71-73: Use<div>instead of<p>for the blank-line spacer.An empty
<p>is semantically a paragraph and may accumulate browser defaultmargin-top/margin-bottomthat conflicts with theh-2height. A<div class="h-2"></div>avoids this and better represents a pure visual spacer.♻️ Proposed fix
- <p class="h-2"></p> + <div class="h-2"></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/markdown_block.svelte` around lines 71 - 73, Replace the empty paragraph used as a spacer in the {:else} branch of markdown_block.svelte with a neutral block element: change the <p class="h-2"></p> spacer to a <div class="h-2"></div> so it no longer implies paragraph semantics or inherits paragraph margins; locate the {:else} branch in the template and update that element accordingly.app/web_ui/src/lib/ui/run_config_component/run_config_summary.svelte (1)
26-37: Consider using a native<button>instead of<div role="button">.A native
<button>provides keyboard interaction (Enter/Space), focus ring, and correct semantics without the manualtabindex,role, andon:keydownwiring. This also eliminates the need for the explicite.preventDefault()on Space.♻️ Proposed refactor
-<div - class="cursor-pointer hover:bg-base-200 rounded-lg p-4" - tabindex="0" - aria-label="Open Run Configuration" - role="button" - on:click={openRunConfig} - on:keydown={(e) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault() - openRunConfig() - } - }} -> +<button + class="cursor-pointer hover:bg-base-200 rounded-lg p-4 w-full text-left" + aria-label="Open Run Configuration" + on:click={openRunConfig} +>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/run_config_component/run_config_summary.svelte` around lines 26 - 37, Replace the interactive div in run_config_summary.svelte with a native <button type="button"> element: keep the class="cursor-pointer hover:bg-base-200 rounded-lg p-4", aria-label="Open Run Configuration", and the on:click={openRunConfig} binding (openRunConfig is the click handler), but remove tabindex, role, and the on:keydown handler and its e.preventDefault() logic since the native button provides correct keyboard behavior and focus styling automatically; ensure the button element retains any styling and accessibility attributes currently on the div.app/web_ui/src/lib/ui/run_config_component/create_new_run_config_dialog.svelte (1)
25-25: Minor: initialrun_config_namegeneration at module load is immediately overridden.
generate_memorable_name()at line 25 runs once on module instantiation but is overridden on everyshow()/showClone()call before the dialog is shown. Consider initializing lazily:♻️ Proposed refactor
- let run_config_name: string = generate_memorable_name() + let run_config_name: string = ""The name is unconditionally assigned inside both
show()andshowClone()beforedialog?.show(), so the initial value is never used.Also applies to: 33-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/lib/ui/run_config_component/create_new_run_config_dialog.svelte` at line 25, Initial memorable name is generated at module load but always overwritten by show() / showClone(); change run_config_name initialization to lazy (e.g., empty string or undefined) and call generate_memorable_name() only inside show() and showClone() when a name isn't provided. Update references to run_config_name in create_new_run_config_dialog.svelte so show() and showClone() set run_config_name = generate_memorable_name() only if it's falsy (or when cloning behavior requires it), removing the needless module-level generate_memorable_name() call.app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelte (1)
44-45: Consider decouplingsettings_loadingfrom the primary page loading state.
settings_loading(the copilot check) is included in$: loading, causing the full page spinner to block until the copilot check resolves. Sincehas_kiln_copilotis only needed for the "Create Spec" action, specs and evals could render independently while the copilot check happens in the background.♻️ Proposed refactor
- $: loading = specs_loading || evals_loading || settings_loading + $: loading = specs_loading || evals_loadingThe
has_kiln_copilotflag can stayfalse(its default) until the async check resolves; the "Create Spec" button will simply use the fallback branch until then, with no visible breakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/specs/[project_id]/[task_id]/+page.svelte around lines 44 - 45, The page-level reactive `loading` currently includes `settings_loading` and blocks the full-page spinner until the copilot check finishes; remove `settings_loading` from the page loading aggregation (change the reactive `$: loading = specs_loading || evals_loading`) so specs/evals can render independently, and keep `settings_loading` (and `has_kiln_copilot`) only for the Create Spec UI (use `settings_loading` to disable/show spinner on the Create Spec button or to fall back to the default branch while the async check completes), ensuring `has_kiln_copilot` remains false by default until the async check resolves.app/web_ui/src/routes/(app)/prompt_optimization/[project_id]/[task_id]/prompt_optimization_job/[job_id]/+page.svelte (2)
27-54: Minor:polling_timertype and variable name suggestsetTimeoutbutsetIntervalis used.The variable is named
polling_timerand typed asReturnType<typeof setTimeout>, but lines 44 and 51 usesetInterval/clearInterval. While the return type is identical at runtime (both arenumberin browsers), consider typing it asReturnType<typeof setInterval>for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/prompt_optimization_job/[job_id]/+page.svelte around lines 27 - 54, The polling_timer is typed and named as if for setTimeout but start_polling() uses setInterval/clearInterval; update the declaration of polling_timer to use ReturnType<typeof setInterval> (and consider renaming to pollingInterval or similar) and ensure start_polling() and stop_polling() keep using setInterval/clearInterval consistently so the type and name match the actual usage in start_polling and stop_polling.
183-259:build_propertiesis called imperatively—properties won't reactively update ifrun_configsorevalschange independently.
build_properties()is only invoked fromget_prompt_optimization_job(line 94). If the run configs store updates outside that flow, the properties list will be stale. Consider makingpropertiesa reactive$:derivation, or at minimum documenting this is intentional. This is acceptable for a detail page but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/prompt_optimization_job/[job_id]/+page.svelte around lines 183 - 259, build_properties currently runs only when get_prompt_optimization_job is called so `properties` can become stale if `run_configs` or `eval_ids` change later; make `properties` reactive instead by replacing the imperative call pattern with a Svelte reactive statement that recomputes properties whenever any of prompt_optimization_job, run_configs, or prompt_optimization_job.eval_ids change (or alternatively call build_properties inside a reactive $: block that depends on those values), referencing the existing build_properties logic (or moving its body into the reactive block) so UI updates automatically without relying solely on get_prompt_optimization_job.app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/prompt_generators/prompt_generators.ts (1)
102-116: Consider returning a more informative fallback whengenerator_idis provided but unrecognized.When
generator_idis truthy but not found inprompt_generator_categories, the function silently falls through to the"id::"/"Unknown"checks. If agenerator_idwas explicitly supplied, the caller likely expects a generator-based label. This is a minor edge case—current behavior is safe—but a fallback likegenerator_iditself or"Custom Generator"might be more informative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/prompt_generators/prompt_generators.ts around lines 102 - 116, getPromptType currently falls through to other checks when a truthy generator_id isn't found in prompt_generator_categories; change its fallback so if generator_id is provided but no matching template is found, it returns a more informative label (e.g., the generator_id value or a fixed string like "Custom Generator") instead of silently continuing to the prompt_id checks; update the logic around the generator lookup in getPromptType to return that informative fallback when generator_id is truthy and .find(...) yields no result so callers can see the intended generator context.app/web_ui/src/routes/(app)/models/price.ts (2)
26-61: Failed fetches are permanently cached — no retry on transient errors.Once
pricingLoadPromiseis set, subsequent calls return the same (possiblynull) promise. If the first call fails due to a transient network issue or timeout, pricing data will remainnullfor the entire session with no way to retry.Consider clearing
pricingLoadPromiseon failure so the next caller triggers a fresh attempt:Proposed fix
pricingLoadPromise = (async () => { try { const controller = new AbortController() const timeoutId = setTimeout(() => controller.abort(), 5000) // ...fetch logic... } catch { + pricingLoadPromise = null // allow retry on next call return null } })()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/models/price.ts around lines 26 - 61, The current fetchPricingData implementation caches a failing result in pricingLoadPromise so transient errors are never retried; update fetchPricingData to clear/reset pricingLoadPromise and leave pricingData unchanged when a fetch fails or returns invalid data (i.e., on non-ok response, invalid JSON shape, or in the catch block) so subsequent calls will create a new promise; keep the successful path setting pricingData and resolving pricingLoadPromise, and ensure any cleanup (clearTimeout on timeoutId) still runs before resetting the cached promise.
48-52: Weak runtime validation before theas PricingDatacast.The check
typeof data === "object" && data !== nullaccepts any object shape (e.g.,[],{foo: 42}). Since the external API is third-party and the shape could change, consider validating a bit more defensively — e.g., confirming the top-level values contain amodelskey — to avoid silent misbehavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/models/price.ts around lines 48 - 52, The runtime guard is too weak before casting to PricingData; strengthen validation of the fetched `data` (used to set `pricingData`) by asserting expected top-level shape—e.g., check `data` is an object, has a `models` property and that `models` is an array (or object) with expected items, and optionally validate other top-level keys your PricingData requires—before doing `pricingData = data as PricingData` in this module (look for the `pricingData`, `PricingData`, and `data` symbols). If validation fails, handle the error path (throw or return a safe default) instead of casting blindly.app/web_ui/src/routes/(app)/models/+page.svelte (1)
598-598: Pricing fetch blocks the entire page render.
pricingLoadingis included in the loading gate (loading || pricingLoading), so the spinner stays until the externalmodels.devfetch completes (or times out after 5s). Since pricing is supplementary, consider loading the page immediately and letting prices fill in asynchronously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/models/+page.svelte at line 598, The page's main loading gate uses pricingLoading so the spinner waits for the external models.dev pricing fetch; change the render condition to only depend on the primary page loader (replace or remove pricingLoading from the {`#if` loading || pricingLoading} check), and move pricingLoading handling into the pricing display component/section so prices are fetched and rendered asynchronously with their own placeholder or loading indicator; update references to pricingLoading and the pricing renderer in +page.svelte so the page renders immediately while prices fill in when ready.app/web_ui/src/routes/(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte (2)
565-575: Reactive async call can race whentarget_run_config_idchanges rapidly.If the user toggles run configs quickly, multiple
check_run_config_validation()calls can be in-flight concurrently, and the last one to resolve (not the last one started) writes the final state. Consider guarding with an incrementing request counter orAbortControllerto ignore stale results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte around lines 565 - 575, The reactive block that calls check_run_config_validation() can produce stale state when target_run_config_id changes rapidly; update either the reactive block or the async function check_run_config_validation to ignore outdated responses by adding a cancellation/serial-id guard: introduce a module-scoped counter (e.g., runConfigValidationRequestId) or an AbortController that you increment/create before each call, pass the id/controller into check_run_config_validation, and inside that function only commit to run_config_validation_status, run_config_validation_message, and run_config_blocking_reason if the request id matches the latest or the controller is not aborted; ensure you also abort or ignore previous in-flight requests when starting a new one.
590-657:refresh_evaluatorsonly updates existing evals — new server-side evals won't appear.The refresh maps over the current
evals_with_configslist, so any evals created since the page loaded are silently excluded. If that's intentional, a brief comment would help; otherwise, consider merging the freshevals_datalist with the existing one to surface new evals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/create_prompt_optimization_job/+page.svelte around lines 590 - 657, refresh_evaluators currently only remaps the existing evals_with_configs so any evals returned in evals_data that weren't present in evals_with_configs are dropped; change it to merge fresh evals_data with the existing evals_with_configs so new server-side evals appear. Concretely: in refresh_evaluators use evals_data to drive the resulting list (iterate evals_data), look up any existing entry in evals_with_configs by id to reuse configs/current_config when present, and for evals without local configs fetch their configs (same GET to /eval/{eval_id}/eval_configs) and build new items with validation_status reset and errors null; finally set evals_with_configs to that merged list and still call check_eval_validation for each index. Ensure you update usages of evals_with_configs, evals_data, and check_eval_validation accordingly.app/desktop/studio_server/prompt_optimization_job_api.py (4)
183-203: Dead code:artifact_idon line 196 is assigned but never used after successful deletion.The variable is only used in the
exceptblock (line 198) for logging. In the success path (line 196), it's unused.Proposed fix
try: artifact.delete() - artifact_id = getattr(artifact, "id", "unknown") except Exception as cleanup_error: artifact_id = getattr(artifact, "id", "unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/prompt_optimization_job_api.py` around lines 183 - 203, In _cleanup_artifact, the artifact_id is assigned after a successful delete but never used; move the artifact_id = getattr(artifact, "id", "unknown") line to before the try block (so both the success and exception paths can reference it) and remove the redundant assignment inside the try; keep artifact.delete() in the try and retain the logger.error call in the except using artifact_id and exc_info=True.
330-381:check_run_configandcheck_evalendpoints expose internal exception messages to clients.Lines 379 and 451 include
str(e)in the HTTP error detail. Internal exception messages may leak implementation details (stack traces, file paths, credentials in URLs). Consider returning a generic message or sanitizing the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/prompt_optimization_job_api.py` around lines 330 - 381, The catch-all exception handlers in check_run_config and check_eval currently return HTTPException details containing str(e), which can leak internals; change both except Exception blocks to log the full error with logger.error(..., exc_info=True) but raise a generic HTTPException (e.g., status_code=500, detail="Internal server error while checking run config" for check_run_config and "Internal server error while checking eval" for check_eval) instead of including str(e); keep the internal logging so diagnostics remain available but avoid exposing exception messages to clients.
279-327: Status update errors are silently swallowed — job returned with potentially stale status.
update_prompt_optimization_job_and_create_artifactscatches all exceptions at line 322 and returns the job without re-raising. This is intentional for the batch-update use case (line 588), but for the single-jobget_prompt_optimization_jobendpoint (line 628), the caller also swallows exceptions. The user gets a 200 with a stale status and no indication of failure. Consider at least logging atwarninglevel or returning a flag indicating the status might be stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/prompt_optimization_job_api.py` around lines 279 - 327, The function update_prompt_optimization_job_and_create_artifacts currently swallows all exceptions and returns possibly stale PromptOptimizationJob; change it to mark failures explicitly and increase visibility: inside the except block of update_prompt_optimization_job_and_create_artifacts, log a warning (logger.warning) that includes the exception and job_id, and set a clear flag on the returned object (e.g., prompt_optimization_job.status_maybe_stale = True or prompt_optimization_job.stale = True) so callers like get_prompt_optimization_job can detect and surface stale status to users; keep the batch-use behavior (do not re-raise) but ensure the new flag is persisted via prompt_optimization_job.save_to_file() so callers can read it.
491-528: ZIPFileobject is created inside thewithblock but consumed outside it — works here, but the indentation boundary is easy to misread.
project_zip_filewraps an in-memoryBytesIO(not a reference to the temp file), so using it after theTemporaryDirectoryexits is safe. However,bodyconstruction and the API call at lines 518–528 are outside thewithblock while relying onproject_zip_file. Consider moving the API call inside thewithblock to make the lifecycle relationship clearer, or adding a brief comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/desktop/studio_server/prompt_optimization_job_api.py` around lines 491 - 528, The File wrapper project_zip_file is created from in-memory bytes inside the TemporaryDirectory context but is then used after that with BodyStartPromptOptimizationJobV1JobsPromptOptimizationJobStartPost and in the call to start_prompt_optimization_job_v1_jobs_prompt_optimization_job_start_post.asyncio_detailed; to make the lifecycle explicit either move the body construction and the asyncio_detailed API call into the same with tempfile.TemporaryDirectory(...) block so the creation and use are colocated, or keep them outside but add a concise comment stating that project_zip_file holds in-memory BytesIO and is safe after the temp dir exits; update references to project_zip_file, BodyStartPromptOptimizationJobV1JobsPromptOptimizationJobStartPost, and start_prompt_optimization_job_v1_jobs_prompt_optimization_job_start_post.asyncio_detailed accordingly.app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/+page.svelte (2)
58-83: Sorting recomputesgetPromptTypeon every reactive update.
sorted_promptscallsgetPromptType(a.id, a.generator_id)for every comparison during sort. IfgetPromptTypeis non-trivial, this could be inefficient for large prompt lists since comparisons happen O(n log n) times. Consider pre-computing the type values before sorting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/+page.svelte around lines 58 - 83, The sort recomputes getPromptType repeatedly inside the sorted_prompts reactive block causing O(n log n) calls; precompute type strings once before sorting (inside the same reactive block) by mapping prompts to a temporary array or a Map keyed by prompt id (use getPromptType(a.id, a.generator_id).toLowerCase() once per prompt), then use those cached values in the comparator for the "type" case; update the comparator to read from the cache and perform comparisons using the precomputed lowercase strings so sorting no longer calls getPromptType for each comparison.
85-88: Use a query parameter instead ofsessionStoragefor passing the prompt text.The
sessionStorageapproach with a magic string key is inconsistent with the established pattern in this codebase, which uses query parameters extensively for inter-page data passing (e.g.,goto(...?${params.toString()}). While cleanup does occur in the consuming page, using a query parameter would be more maintainable, type-safe, and aligned with project conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/+page.svelte around lines 85 - 88, Replace the sessionStorage-based handoff in handleSetBasePrompt with a query parameter: stop writing to sessionStorage using the "pending_base_prompt" key and instead build a URLSearchParams (e.g., params.set('base_prompt', prompt_text)) and call goto(`/prompts/${project_id}/${task_id}/edit_base_prompt?${params.toString()}`); update the consuming page (+page.svelte for edit_base_prompt) to read and validate the base_prompt query param rather than expecting sessionStorage, and remove the sessionStorage key usage from handleSetBasePrompt.app/web_ui/src/routes/(app)/+layout.svelte (2)
234-325: Optimize parent link won't show active state when a child route is active.When the user navigates to
/prompts/...,/models,/fine_tune/..., or/docs/..., the respective child gets theactiveclass, but the parent "Optimize"<a>(line 237) only activates on/optimize/.... This means the parent nav item won't visually indicate that the user is within its subtree. Consider adding a check like:[Section.Optimize, Section.Prompts, Section.Models, Section.FineTune, Section.Documents].includes(section)for the parent's active class, so it highlights when any child is active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/+layout.svelte around lines 234 - 325, The parent "Optimize" link only checks section == Section.Optimize, so it won't show active when any child route is active; update the class binding on the Optimize <a> (the element using section and Section) to check whether section is one of the subtree values (e.g. Section.Optimize, Section.Prompts, Section.Models, Section.FineTune, Section.Documents) by using an includes-style test or boolean ORs so the parent receives "active" whenever any child section is active.
396-402: Global CSS selectors may have unintended side effects.
:global(ul > li.menu-nested)and:global(.menu li > ul)apply broadly to all matching elements across the app. If other pages introduce similar DaisyUI menu structures, these overrides could cause unexpected styling. Consider scoping these rules more narrowly if feasible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/+layout.svelte around lines 396 - 402, The global CSS selectors :global(ul > li.menu-nested) and :global(.menu li > ul) are too broad; scope them to the component by adding/using a unique wrapper class (e.g., .app-menu, .sidebar or .chat-menu) on the parent container and update the selectors to target that wrapper (for example :global(.app-menu ul > li.menu-nested) and :global(.app-menu .menu li > ul)). Locate the style block in +layout.svelte and replace the two global selectors with scoped variants that include the chosen wrapper class so the rules only affect this component's menu.app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte (1)
24-28:getPromptTypecalled with empty string whenprompt_model?.idis undefined.If
prompt_modelis undefined (loading or not found),getPromptType("", null)is called during the reactive computation. This won't cause a runtime error since the result is filtered out by the.filter()on line 31, but it's a minor inefficiency. Not a blocker given the filter handles it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte around lines 24 - 28, Avoid calling getPromptType with an empty string when prompt_model is undefined by making the Type field conditional: only call getPromptType when prompt_model?.id exists (e.g., use a guarded expression like prompt_model?.id ? getPromptType(prompt_model.id, prompt_model.generator_id ?? null) : undefined). Update the object that sets Description/Type (referencing prompt_model and getPromptType) so Type becomes undefined when prompt_model is missing, preventing the unnecessary getPromptType("") call.app/web_ui/src/routes/(app)/optimize/[project_id]/[task_id]/+page.svelte (2)
87-108:load_available_toolsis fire-and-forget (not awaited).Line 89 calls
load_available_tools(project_id)withoutawaitand outside thePromise.all. If it fails, the error is silently swallowed. ThegetToolsDisplayfunction gracefully degrades to"Loading...", so this isn't a blocker, but an unhandled rejection could still surface in the console. Consider either awaiting it insidePromise.allor adding a.catch().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/optimize/[project_id]/[task_id]/+page.svelte around lines 87 - 108, The call to load_available_tools in the onMount block is fire-and-forget and can produce unhandled rejections; update the onMount async logic to either include load_available_tools(project_id) in the Promise.all with the other async calls (so it is awaited) or append a .catch(...) to handle errors explicitly, ensuring any thrown error is converted to a Kiln error (createKilnError) or logged via existing error handling; locate the onMount function and the Promise.all invocation and modify the call to load_available_tools accordingly.
298-326: The "Compare" navigation button is visible outside theselect_modeconditional.The
{#ifselected_run_configs.size > 0}block (line 321) is a sibling to the{#ifselect_mode}block, not nested inside it. Currently this is safe becausecancelSelectionclears the set when exiting select mode. However, the intent would be clearer if the navigation Compare button were nested inside theselect_modebranch to avoid any edge-case where selections leak.♻️ Suggested restructure
{`#if` select_mode} <div class="font-light text-sm"> {selected_run_configs.size} selected{`#if` selected_run_configs.size >= MAX_SELECTIONS} <span class="text-gray-400">{` (max)`}</span> {/if} </div> <button class="btn btn-mid" on:click={cancelSelection}> Cancel Selection </button> + {`#if` selected_run_configs.size > 0} + <button class="btn btn-primary btn-mid" on:click={handleCompare}> + Compare + </button> + {/if} {:else} <button class="btn btn-mid" on:click={() => (select_mode = true)}> Compare </button> <button class="btn btn-mid" on:click={() => create_run_config_dialog?.show()} > Create Run Config </button> {/if} - {`#if` selected_run_configs.size > 0} - <button class="btn btn-primary btn-mid" on:click={handleCompare}> - Compare - </button> - {/if}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/optimize/[project_id]/[task_id]/+page.svelte around lines 298 - 326, The Compare button rendered by the {`#if` selected_run_configs.size > 0} block should be moved inside the {`#if` select_mode} branch so it only appears when select_mode is active; update the template so the selected count, Cancel Selection button and the Compare navigation button (which calls handleCompare) are all inside the select_mode branch, leaving the non-select-mode buttons (Compare to enter select_mode and Create Run Config which calls create_run_config_dialog?.show()) in the {:else} branch; this prevents leakage from selected_run_configs and keeps behavior tied to the select_mode state (references: select_mode, selected_run_configs, cancelSelection, handleCompare, create_run_config_dialog).app/web_ui/src/routes/(app)/prompt_optimization/[project_id]/[task_id]/+page.svelte (1)
81-88: In-place.sort()mutates the response array directly.
prompt_optimization_jobs_response.sort(...)mutates the API response array in place. While functionally fine here since the result is immediately assigned, using[...prompt_optimization_jobs_response].sort(...)would be more defensive and consistent with the pattern in the optimize page'ssortRunConfigs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompt_optimization/[project_id]/[task_id]/+page.svelte around lines 81 - 88, The code currently calls prompt_optimization_jobs_response.sort(...) which mutates the API response array; change this to sort a shallow copy instead (e.g., use a spread copy before calling sort) so sorted_prompt_optimization_jobs is derived from a new array and the original prompt_optimization_jobs_response remains unchanged—apply this change where sorted_prompt_optimization_jobs is constructed (same pattern as sortRunConfigs) and then assign the result to prompt_optimization_jobs.
|
CI failing due to |
2 participants
No description provided.