Added spec properties property and changed definition to description #841
Conversation
WalkthroughSpec "type" and exported SpecType enum were replaced by a discriminated union Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant API
participant Validator
participant DB
Note over WebUI,API: Create / Update spec (properties-based)
User->>WebUI: submit form (name, description, properties:{ spec_type, ... })
WebUI->>API: POST/PATCH /spec { name, description, properties, eval_id, tags }
API->>Validator: parse SpecUpsertRequest (Field discriminator="spec_type")
Validator->>Validator: validate properties shape for spec_type
alt valid
Validator-->>API: validated Spec object
API->>DB: save Spec (properties stored with spec_type)
DB-->>API: 201/200 OK
API-->>WebUI: Spec response (includes properties)
WebUI-->>User: success UI
else invalid
Validator-->>API: 422 with field-specific errors
API-->>WebUI: 422 response
WebUI-->>User: show validation errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 92% Diff: origin/sfierro/specs...HEAD
Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte (1)
92-95: Consider simplifying the title construction.The conditional logic is correct but verbose. For improved readability, consider refactoring:
- title="{spec?.type ? `${formatSpecType(spec.type)}: ` : ''}{spec?.name - ? `${spec.name}` - : ''}" + title={[ + spec?.type ? formatSpecType(spec.type) : null, + spec?.name + ].filter(Boolean).join(': ')} subtitle={spec?.description}libs/core/kiln_ai/datamodel/spec_properties.py (1)
1-19: Consider adding docstrings and explicit exports.To improve maintainability and discoverability:
- Add a module docstring explaining the purpose of these property types
- Add class docstrings to describe when each property type should be used
- Consider adding
__all__to explicitly declare the public APIExample:
"""TypedDict definitions for spec properties supporting structured tool-use and undesired-behavior specifications.""" from typing import Literal from typing_extensions import TypedDict class AppropriateToolUseProperties(TypedDict, total=True): """Properties for specs that define appropriate tool usage guidelines.""" spec_type: Literal["appropriate_tool_use"] tool_id: str appropriate_tool_use_guidelines: str inappropriate_tool_use_guidelines: str | None class UndesiredBehaviourProperties(TypedDict, total=True): """Properties for specs that define undesired behavior patterns.""" spec_type: Literal["undesired_behaviour"] undesired_behaviour_guidelines: str examples: str SpecProperties = AppropriateToolUseProperties | UndesiredBehaviourProperties __all__ = [ "AppropriateToolUseProperties", "UndesiredBehaviourProperties", "SpecProperties", ]libs/core/kiln_ai/datamodel/test_spec.py (2)
405-470: Negative properties validation scenarios are good; consider adding tests for empty-but-present required fields
test_spec_properties_validation_missing_required_fieldscorrectly checks that omitting required keys from thepropertiesdict results in aValidationErrorfrom Pydantic’s TypedDict handling.test_spec_properties_validation_wrong_spec_typeensures incorrectspec_typeliterals on both property variants also raiseValidationError, exercising the Literal-based constraints.test_spec_rejects_empty_dict_propertiesverifies that{}is rejected forproperties, which is important given the validator assumesspec_typeis present.test_spec_with_properties_and_descriptionnicely confirms thatdescriptionand a validAppropriateToolUsePropertiespayload coexist and that the description is preserved.One possible enhancement (optional) would be to add tests where the required fields exist but are empty strings (e.g.
tool_id="",appropriate_tool_use_guidelines="",undesired_behaviour_guidelines="",examples=""). Those cases are handled bySpec.validate_properties(raisingValueError), but currently aren’t covered in tests. That would fully exercise the semantic validation in addition to the structural TypedDict validation.Also applies to: 472-503
18-52: Error message assertions are currently correct but tests could be more robust to future Pydantic upgradesThese tests correctly assert on
"Field required"and"String should have at least 1 character"for Pydantic v2.9.x and newer—these messages are stable through the current versions. However, to future-proof against potential changes in newer Pydantic versions, consider asserting on error types or locations viae.errors()instead of string matching. This is optional and can be deferred.Applies to: test_spec_valid_creation, test_spec_with_custom_values, and related required-field tests (lines 18-52, 55-83, 85-107)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/web_ui/src/lib/api_schema.d.ts(4 hunks)app/web_ui/src/lib/types.ts(1 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelte(4 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte(2 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte(3 hunks)libs/core/kiln_ai/datamodel/spec.py(3 hunks)libs/core/kiln_ai/datamodel/spec_properties.py(1 hunks)libs/core/kiln_ai/datamodel/test_spec.py(14 hunks)libs/core/kiln_ai/datamodel/test_task.py(3 hunks)libs/server/kiln_server/spec_api.py(4 hunks)libs/server/kiln_server/test_spec_api.py(44 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-04T06:27:58.852Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 487
File: app/web_ui/src/lib/api_schema.d.ts:3315-3320
Timestamp: 2025-09-04T06:27:58.852Z
Learning: The file app/web_ui/src/lib/api_schema.d.ts is auto-generated by openapi-typescript from the backend API schema and should never be manually edited. Changes to API types should be made in the backend, which will then be reflected in the regenerated TypeScript definitions.
Applied to files:
app/web_ui/src/lib/types.tsapp/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-10-29T06:59:55.635Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 736
File: app/web_ui/src/lib/api_schema.d.ts:8143-8180
Timestamp: 2025-10-29T06:59:55.635Z
Learning: app/web_ui/src/lib/api_schema.d.ts is generated by openapi-typescript; do not propose manual edits. Schema changes should be made in the FastAPI backend (e.g., app/desktop/studio_server/data_gen_api.py or libs/server/kiln_server/*), then re-generate the TS types.
Applied to files:
app/web_ui/src/lib/types.tsapp/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-10-29T11:19:31.059Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 759
File: app/web_ui/src/lib/api_schema.d.ts:2485-2490
Timestamp: 2025-10-29T11:19:31.059Z
Learning: In libs/server/kiln_server/document_api.py and the generated app/web_ui/src/lib/api_schema.d.ts, CreateVectorStoreConfigRequest.properties is now a union of three distinct public schemas: LanceDBConfigFTSPropertiesPublic, LanceDBConfigVectorPropertiesPublic, and LanceDBConfigHybridPropertiesPublic. These are intentionally split to allow independent evolution; do not suggest merging them.
Applied to files:
app/web_ui/src/lib/types.tsapp/web_ui/src/lib/api_schema.d.tslibs/server/kiln_server/spec_api.py
📚 Learning: 2025-09-10T08:30:56.973Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 546
File: app/web_ui/src/lib/api_schema.d.ts:4173-4218
Timestamp: 2025-09-10T08:30:56.973Z
Learning: leonardmq correctly identified that app/web_ui/src/lib/api_schema.d.ts is auto-generated and should not be manually edited. The proper fix for type inconsistencies between TypeScript API schema and backend models is to update the backend Python models and regenerate the TypeScript definitions.
Applied to files:
app/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-09-10T08:27:47.227Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 546
File: app/web_ui/src/lib/api_schema.d.ts:0-0
Timestamp: 2025-09-10T08:27:47.227Z
Learning: leonardmq correctly identifies auto-generated files and prefers not to manually edit them. When suggesting changes to generated TypeScript API schema files, the fix should be made in the source OpenAPI specification in the backend, not in the generated TypeScript file.
Applied to files:
app/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-10-01T21:48:25.854Z
Learnt from: sfierro
Repo: Kiln-AI/Kiln PR: 674
File: libs/core/kiln_ai/tools/kiln_task_tool.py:30-34
Timestamp: 2025-10-01T21:48:25.854Z
Learning: In libs/core/kiln_ai/datamodel/external_tool_server.py, KilnTaskServerProperties is a TypedDict, not a Pydantic model. At runtime it's a regular Python dict, so .get() method works correctly and no .model_dump() conversion is needed.
Applied to files:
libs/server/kiln_server/spec_api.pylibs/core/kiln_ai/datamodel/spec_properties.pylibs/core/kiln_ai/datamodel/test_spec.py
🧬 Code graph analysis (5)
libs/core/kiln_ai/datamodel/test_task.py (1)
libs/core/kiln_ai/datamodel/task.py (1)
specs(177-178)
libs/server/kiln_server/spec_api.py (1)
app/web_ui/src/lib/types.ts (1)
SpecProperties(113-115)
libs/core/kiln_ai/datamodel/spec_properties.py (1)
app/web_ui/src/lib/types.ts (1)
SpecProperties(113-115)
libs/server/kiln_server/test_spec_api.py (2)
libs/core/kiln_ai/datamodel/spec_properties.py (1)
UndesiredBehaviourProperties(13-16)libs/core/kiln_ai/datamodel/spec.py (3)
SpecType(12-44)SpecStatus(47-53)Spec(56-137)
libs/core/kiln_ai/datamodel/test_spec.py (3)
libs/core/kiln_ai/datamodel/spec_properties.py (2)
AppropriateToolUseProperties(6-10)UndesiredBehaviourProperties(13-16)libs/core/kiln_ai/datamodel/spec.py (3)
SpecType(12-44)Spec(56-137)SpecStatus(47-53)libs/core/kiln_ai/datamodel/test_task.py (1)
sample_task(54-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Web UI Code Format, Lint, Typecheck, Test, and Build
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (macos-15-intel)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Web UI Code Format, Lint, Typecheck, Test, and Build
🔇 Additional comments (20)
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte (1)
70-71: LGTM! Payload correctly updated for the new schema.The migration from
definitiontodescriptionand the addition of thepropertiesfield align with the PR's objective. Using|| nullis consistent with the existing pattern in this file (line 76).libs/core/kiln_ai/datamodel/spec_properties.py (2)
13-16: LGTM! Clean TypedDict definition.The structure is well-defined with clear required fields and proper use of the discriminator pattern via
spec_type.
19-19: LGTM! Proper discriminated union definition.The union correctly combines both property types, and the
spec_typeliterals enable type discrimination at runtime when used with Pydantic validators.app/web_ui/src/lib/api_schema.d.ts (4)
2176-2189: AppropriateToolUseProperties schema looks coherent and well-discriminatedThe new
AppropriateToolUsePropertiesshape (with literalspec_type: "appropriate_tool_use",tool_id, and guideline fields) is consistent withSpecTypeand should work well as a discriminated union member ofSpec.properties. No issues from the TS/OpenAPI perspective. As this file is auto-generated, the main requirement is that the backend/OpenAPI schema stays in sync, which this appears to reflect.
Based on learnings
5196-5205: Spec.description + Spec.properties changes are consistent; ensure data/backfill is handledSwitching from a
definitionfield to a requireddescription: stringonSpecaligns with naming in the rest of the API surface, and the new optionalproperties?: AppropriateToolUseProperties | UndesiredBehaviourProperties | nullcleanly models typed spec metadata for relevant spec types while remaining nullable/omittable for others. The types themselves look correct; just make sure existing persisted specs are migrated/backfilled so that requireddescriptionis always present at load time.
5248-5251: SpecUpsertRequest now requires description/properties; semantics look intentional
SpecUpsertRequestnow requires a non-nullabledescription: stringand a required but nullableproperties: AppropriateToolUseProperties | UndesiredBehaviourProperties | null. This is compatible with theSpecmodel (wherepropertiesis optional) as long as the client always sendsproperties(e.g.spec.properties ?? null) on create and update, which the UI changes reportedly do. If you ever want partial/patch semantics for specs, that would be the point to relax these in the backend/OpenAPI schema rather than here in the generated file.
5751-5762: UndesiredBehaviourProperties union member is consistent with SpecTypeThe
UndesiredBehaviourPropertiesschema (literalspec_type: "undesired_behaviour", plusundesired_behaviour_guidelinesandexamples) cleanly complementsAppropriateToolUsePropertiesas a second discriminated member forSpec.properties. The required fields and literal discriminator all look correct and in line with the described spec model.
Based on learningsapp/web_ui/src/lib/types.ts (1)
113-115: SpecProperties alias correctly mirrors backend/public schemasThe
SpecPropertiesunion cleanly matches the two spec property schemas exposed viacomponents["schemas"], giving the UI a stable type without touching the generatedapi_schema.d.ts. This is the right place to introduce the alias.
Based on learningsapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte (1)
19-20: Create-spec form now aligned with description/properties APIUsing
descriptionin the POST body and wiring the new “Spec Description” textarea into the unload warning keeps the UI consistent with the updated Spec model; explicitly sendingproperties: nullis correct while there’s no properties editor yet.Also applies to: 42-45, 88-89, 94-101
libs/core/kiln_ai/datamodel/test_task.py (1)
347-360: Task–specs relationship tests updated correctly fordescriptionThe specs relationship tests now construct and assert against
descriptioninstead ofdefinition, keeping coverage in place for the on-disk roundtrip without changing behavior.Also applies to: 370-384
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelte (1)
388-396: Spec list PATCH payloads and table now preserve description/propertiesIncluding
description,properties: spec.properties ?? null, andeval_id: spec.eval_id ?? nullin both tag-edit and archive PATCHes prevents losing structured properties or eval bindings when updating specs. The table header/cell swap toDescription/spec.descriptionmatches the new model.Also applies to: 468-475, 598-599, 683-684
libs/server/kiln_server/spec_api.py (1)
7-8: SpecUpsertRequest and handlers correctly mirror new Spec fieldsSwitching from
definitiontodescriptionand addingproperties: SpecProperties | NoneonSpecUpsertRequest, then passing those through in create/update, keeps the API aligned with the Spec model and the new properties validation without altering read behavior.
Based on learningsAlso applies to: 25-33, 42-52, 71-78
libs/server/kiln_server/test_spec_api.py (4)
49-118: API spec tests consistently migrated todescriptionandpropertiesAcross create/get/update and archived-status tests, payloads and assertions now use
descriptionand include apropertiesfield (usuallyNone), matching the new Spec/SpecUpsertRequest contract while still asserting on persisted Spec instances. The patterns look consistent and should keep regression coverage intact.Also applies to: 137-152, 185-212, 230-276, 281-319, 346-373, 418-447
375-416:test_create_spec_with_propertiesexercises end‑to‑end properties roundtripUsing
UndesiredBehaviourPropertiesto build the request and then asserting on both the JSON response andtask.specs()[0].propertiesverifies that structured spec properties survive the entire create pipeline and are stored on the Spec object as intended.
Based on learnings
545-571: 422 validation tests cover required fields, enums, and tag constraintsThe 422 tests for missing fields, invalid enum values, and invalid/empty/space‑containing tags give good coverage of the validation surface, checking
source_errorswith focused predicates rather than fragile full‑message matches; this should make failures informative but resilient to minor error formatting changes.Also applies to: 573-727, 772-883
1056-1189: New SpecProperties validation tests align with Spec model_validator logicThe added tests for empty
tool_id, emptyappropriate_tool_use_guidelines, emptyundesired_behaviour_guidelines, and emptyexamplesverify that the SpecProperties validator rejects structurally correct but semantically empty data, and that the resulting 422s expose clear messages viasource_errors. This directly exercises the newvalidate_propertieslogic.libs/core/kiln_ai/datamodel/spec.py (1)
9-10: Spec model’s description/properties fields and validator enforce consistent specsAdding
description(required, min_length=1) andproperties: SpecProperties | Noneplus thevalidate_propertiespost‑validator tightly couplestypewithproperties["spec_type"]and enforces non‑empty tool IDs/guidelines/examples for the supported spec types, matching the new API/tests and existing TypedDict‑based patterns in the codebase.
Based on learningsAlso applies to: 59-64, 85-127
libs/core/kiln_ai/datamodel/test_spec.py (3)
6-9: Imports for spec properties look correct and aligned with model typesBringing in
AppropriateToolUsePropertiesandUndesiredBehaviourPropertieshere matches theSpec.propertiesunion and keeps the tests close to the actual TypedDict definitions. No issues.
109-179: Enum coverage for type/priority/status and tag validation remains solid
- The parameterized tests over all
SpecType,Priority, andSpecStatusvalues correctly switched todescriptionand still provide good regression coverage that every enum value is constructible.- The tag validation tests (
empty_string,spaces, and valid tags) continue to exercise thevalidate_tagsmodel validator properly; switching todescriptionthere is consistent with the rest of the suite.No functional issues spotted here.
Also applies to: 181-237
240-366: Happy-path tests for properties and None handling look correct and aligned with TypedDict definitions
test_spec_with_none_propertiesverifies thatproperties=Noneis accepted and bypasses thevalidate_propertieslogic, which matches the model’sproperties: SpecProperties | None.- The three positive-path tests for
AppropriateToolUsePropertiesandUndesiredBehaviourProperties(including the optionalinappropriate_tool_use_guidelinesbeingNone) correctly mirror the TypedDict shapes and check thatSpecpreserves them as plain dicts.- The
*_type_matches_properties_*tests validate that whentypeandproperties["spec_type"]agree, construction succeeds and fields are wired as expected.These tests look accurate and give good confidence that the new
propertiesfield interoperates correctly withSpecType.
libs/server/kiln_server/spec_api.py
Outdated
| name: FilenameString | ||
| definition: str | ||
| description: str | ||
| properties: SpecProperties | None |
There was a problem hiding this comment.
You could also do discriminated union here (would only need to add the Field(discriminator="spec_type") in here) - I think that will make your request in the frontend also get typechecked (i.e. not allow passing in the wrong properties given the spec_type in properties)
In the chunker API, I did this - don't really remember if that is any good or why, but probably ok pattern:
Kiln/libs/server/kiln_server/document_api.py
Line 360 in 4cdddfc
Essentially just having a getter that returns the union of possible types (I think it is because the typechecker in Python does not consider valid to try to shove an object of type A into an A | B, it wants an explicitly A | B union type; or something along these lines)
There was a problem hiding this comment.
don't think I need the get_properties_for_chunker_type as the all properties will be defined, it looks like you had that to put some default values in for the client. but did change this to the discriminator one
|
Closing for now as the spec model object is in flux we might not do properties at all |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte (1)
68-77: Remove the obsoletetypefield and handlepropertiescorrectly.Two issues in the PATCH payload:
Line 72 sends
type: spec.type, but theSpecUpsertRequestmodel no longer includes atypefield (it's now withinproperties.spec_type). This may cause a validation error or be silently ignored.Line 71 uses
spec.properties || null, butpropertiesis a required field inSpecUpsertRequestwith no default. Ifspec.propertiesis falsy, sendingnullwill cause a 422 validation error.body: { name: spec.name, description: spec.description, - properties: spec.properties || null, - type: spec.type, + properties: spec.properties, priority: spec.priority, status: spec.status, tags: tags, eval_id: spec.eval_id || null, },app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte (1)
19-27:SpecTypemust be imported as a runtime value, not just a type.The issue is confirmed. Line 6 imports
SpecTypewith thetypekeyword (import type { SpecType }), which means it's only available at compile time. However, line 21 attempts to useSpecType.desired_behaviouras if it were a runtime enum or object with properties, which will result in a runtime error: "Cannot read properties of undefined (reading 'desired_behaviour')".In other files like
template_select/+page.svelte, the correct pattern is used:spec_type: "desired_behaviour"as a string literal. Fix line 21 to use the string literal instead:let spec_type: SpecType = "desired_behaviour" as SpecTypeAlternatively, if
SpecTypeshould be a runtime enum, it needs to be exported as a const enum or const object in$lib/types.ts, not just as a type alias.
♻️ Duplicate comments (1)
libs/core/kiln_ai/datamodel/spec_properties.py (1)
111-139: Aligninappropriate_tool_use_guidelinestyping with “optional if provided” semanticsRight now the field is:
- Declared as
str | Nonewithtotal=True, so the key is required but may beNone.- Validated as “if provided cannot be empty”, which conceptually treats it as optional.
If callers omit the key entirely, strict TypedDict / Pydantic validation may reject the payload despite the doc intent that this field is optional. Making the key truly optional and adjusting the validator to use
.get()would better match the intended behavior.You could apply something like:
-from typing_extensions import TypedDict +from typing_extensions import NotRequired, TypedDict @@ class AppropriateToolUseProperties(TypedDict, total=True): spec_type: Literal[SpecType.appropriate_tool_use] tool_id: str appropriate_tool_use_guidelines: str - inappropriate_tool_use_guidelines: str | None + inappropriate_tool_use_guidelines: NotRequired[str] @@ - # inappropriate_tool_use_guidelines - inappropriate_tool_use_guidelines = properties["inappropriate_tool_use_guidelines"] - if ( - inappropriate_tool_use_guidelines is not None - and not inappropriate_tool_use_guidelines.strip() - ): + # inappropriate_tool_use_guidelines + inappropriate_tool_use_guidelines = properties.get( + "inappropriate_tool_use_guidelines" + ) + if ( + inappropriate_tool_use_guidelines is not None + and not inappropriate_tool_use_guidelines.strip() + ): raise ValueError( "inappropriate_tool_use_guidelines if provided cannot be empty" )
🧹 Nitpick comments (3)
libs/server/kiln_server/spec_api.py (1)
25-34: Consider addingmin_length=1todescriptionfor early validation.The
Specmodel requiresdescriptionto havemin_length=1, butSpecUpsertRequestdoesn't enforce this. WhileSpecconstruction will catch empty descriptions, adding the constraint here provides clearer error messages at the API layer.class SpecUpsertRequest(BaseModel): name: FilenameString - description: str + description: str = Field(min_length=1) properties: SpecProperties = Field( discriminator="spec_type", )libs/core/kiln_ai/datamodel/test_spec.py (1)
143-179: Consider adding dedicated tests for skipped property types.The parameterized test skips
undesired_behaviourandappropriate_tool_usebecause they require additional fields. While there are dedicated tests for these types later in the file (lines 296-365), this skip pattern could be cleaner by removing these entries from the parameterized list since they're covered elsewhere.@pytest.mark.parametrize( "spec_type,properties_class", [ (SpecType.desired_behaviour, DesiredBehaviourProperties), - (SpecType.undesired_behaviour, None), - (SpecType.appropriate_tool_use, None), (SpecType.intermediate_reasoning, IntermediateReasoningProperties), # ... rest of the list ], ) def test_spec_all_types(sample_task, spec_type, properties_class): """Test that all spec types can be created.""" - if properties_class is None: - pytest.skip(f"Skipping {spec_type} - requires additional fields") - properties = properties_class(spec_type=spec_type)libs/core/kiln_ai/datamodel/spec_properties.py (1)
166-174: SimplifyAfterValidatorusage by passing the functions directlyThe lambdas here just forward to
validate_appropriate_tool_use_propertiesandvalidate_undesired_behaviour_properties. Passing the functions directly is clearer and keeps tracebacks simpler.AppropriateToolUsePropertiesValidator = Annotated[ - AppropriateToolUseProperties, - AfterValidator(lambda v: validate_appropriate_tool_use_properties(v)), + AppropriateToolUseProperties, + AfterValidator(validate_appropriate_tool_use_properties), ] @@ UndesiredBehaviourPropertiesValidator = Annotated[ - UndesiredBehaviourProperties, - AfterValidator(lambda v: validate_undesired_behaviour_properties(v)), + UndesiredBehaviourProperties, + AfterValidator(validate_undesired_behaviour_properties), ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/web_ui/src/lib/api_schema.d.ts(18 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte(2 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte(3 hunks)libs/core/kiln_ai/datamodel/spec.py(2 hunks)libs/core/kiln_ai/datamodel/spec_properties.py(1 hunks)libs/core/kiln_ai/datamodel/test_spec.py(8 hunks)libs/core/kiln_ai/datamodel/test_task.py(4 hunks)libs/server/kiln_server/spec_api.py(4 hunks)libs/server/kiln_server/test_spec_api.py(39 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/core/kiln_ai/datamodel/test_task.py
- libs/core/kiln_ai/datamodel/spec.py
🧰 Additional context used
📓 Path-based instructions (11)
app/web_ui/**/*.svelte
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
app/web_ui/**/*.svelte: Use Svelte v4 (not v5) for the web frontend UI
Use DaisyUI for UI components in the web frontendUse Svelte v4 (not v5) for the web UI framework
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte
app/web_ui/**/*.{svelte,ts}
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Tailwind CSS for styling in the web frontend
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelteapp/web_ui/src/lib/api_schema.d.ts
app/web_ui/**/*.{svelte,css,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Tailwind and DaisyUI for styling the frontend
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelteapp/web_ui/src/lib/api_schema.d.ts
app/web_ui/**/*.{svelte,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Make FastAPI backend calls from the Svelte web app to communicate with backend servers
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelteapp/web_ui/src/lib/api_schema.d.ts
libs/server/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use FastAPI for creating REST servers
libs/server/**/*.py: Use FastAPI for REST server implementations
Use asyncio for asynchronous operations in Python
Use FastAPI with asyncio for the REST server wrapping the core library
Files:
libs/server/kiln_server/spec_api.pylibs/server/kiln_server/test_spec_api.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
**/*.py: Use asyncio for asynchronous operations in Python
Use Pydantic v2 (not v1) for data validation
Files:
libs/server/kiln_server/spec_api.pylibs/core/kiln_ai/datamodel/test_spec.pylibs/server/kiln_server/test_spec_api.pylibs/core/kiln_ai/datamodel/spec_properties.py
libs/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Pydantic v2 (not v1) for data validation and serialization
Files:
libs/server/kiln_server/spec_api.pylibs/core/kiln_ai/datamodel/test_spec.pylibs/server/kiln_server/test_spec_api.pylibs/core/kiln_ai/datamodel/spec_properties.py
libs/core/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Python 3.10+ for the core library (libs/core) and Python 3.13 for the desktop app
Use python 3.10+ for the core library (libs/core) and python 3.13 for the desktop app
Files:
libs/core/kiln_ai/datamodel/test_spec.pylibs/core/kiln_ai/datamodel/spec_properties.py
**/*test*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use pytest for testing Python code
Use pytest for testing in python projects
Files:
libs/core/kiln_ai/datamodel/test_spec.pylibs/server/kiln_server/test_spec_api.py
app/web_ui/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use TypeScript for frontend type safety
Files:
app/web_ui/src/lib/api_schema.d.ts
app/web_ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for frontend development
Files:
app/web_ui/src/lib/api_schema.d.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 759
File: app/web_ui/src/lib/api_schema.d.ts:2485-2490
Timestamp: 2025-10-29T11:19:31.059Z
Learning: In libs/server/kiln_server/document_api.py and the generated app/web_ui/src/lib/api_schema.d.ts, CreateVectorStoreConfigRequest.properties is now a union of three distinct public schemas: LanceDBConfigFTSPropertiesPublic, LanceDBConfigVectorPropertiesPublic, and LanceDBConfigHybridPropertiesPublic. These are intentionally split to allow independent evolution; do not suggest merging them.
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 736
File: app/web_ui/src/lib/api_schema.d.ts:8143-8180
Timestamp: 2025-10-29T06:59:55.635Z
Learning: app/web_ui/src/lib/api_schema.d.ts is generated by openapi-typescript; do not propose manual edits. Schema changes should be made in the FastAPI backend (e.g., app/desktop/studio_server/data_gen_api.py or libs/server/kiln_server/*), then re-generate the TS types.
📚 Learning: 2025-10-29T11:19:31.059Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 759
File: app/web_ui/src/lib/api_schema.d.ts:2485-2490
Timestamp: 2025-10-29T11:19:31.059Z
Learning: In libs/server/kiln_server/document_api.py and the generated app/web_ui/src/lib/api_schema.d.ts, CreateVectorStoreConfigRequest.properties is now a union of three distinct public schemas: LanceDBConfigFTSPropertiesPublic, LanceDBConfigVectorPropertiesPublic, and LanceDBConfigHybridPropertiesPublic. These are intentionally split to allow independent evolution; do not suggest merging them.
Applied to files:
libs/server/kiln_server/spec_api.pylibs/server/kiln_server/test_spec_api.pyapp/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-10-27T09:38:25.562Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 753
File: libs/server/kiln_server/document_api.py:279-299
Timestamp: 2025-10-27T09:38:25.562Z
Learning: In the Kiln codebase, leonardmq intentionally overrides (rather than using setdefault) certain internal config fields like include_metadata and include_prev_next_rel in semantic chunker properties, even if the frontend sends them. These fields are too granular for user control and are set to enforce config stability.
Applied to files:
libs/server/kiln_server/spec_api.py
📚 Learning: 2025-10-29T06:59:55.635Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 736
File: app/web_ui/src/lib/api_schema.d.ts:8143-8180
Timestamp: 2025-10-29T06:59:55.635Z
Learning: app/web_ui/src/lib/api_schema.d.ts is generated by openapi-typescript; do not propose manual edits. Schema changes should be made in the FastAPI backend (e.g., app/desktop/studio_server/data_gen_api.py or libs/server/kiln_server/*), then re-generate the TS types.
Applied to files:
app/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-09-04T06:27:58.852Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 487
File: app/web_ui/src/lib/api_schema.d.ts:3315-3320
Timestamp: 2025-09-04T06:27:58.852Z
Learning: The file app/web_ui/src/lib/api_schema.d.ts is auto-generated by openapi-typescript from the backend API schema and should never be manually edited. Changes to API types should be made in the backend, which will then be reflected in the regenerated TypeScript definitions.
Applied to files:
app/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-09-10T08:30:56.973Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 546
File: app/web_ui/src/lib/api_schema.d.ts:4173-4218
Timestamp: 2025-09-10T08:30:56.973Z
Learning: leonardmq correctly identified that app/web_ui/src/lib/api_schema.d.ts is auto-generated and should not be manually edited. The proper fix for type inconsistencies between TypeScript API schema and backend models is to update the backend Python models and regenerate the TypeScript definitions.
Applied to files:
app/web_ui/src/lib/api_schema.d.ts
📚 Learning: 2025-09-10T08:27:47.227Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 546
File: app/web_ui/src/lib/api_schema.d.ts:0-0
Timestamp: 2025-09-10T08:27:47.227Z
Learning: leonardmq correctly identifies auto-generated files and prefers not to manually edit them. When suggesting changes to generated TypeScript API schema files, the fix should be made in the source OpenAPI specification in the backend, not in the generated TypeScript file.
Applied to files:
app/web_ui/src/lib/api_schema.d.ts
🧬 Code graph analysis (4)
libs/server/kiln_server/spec_api.py (2)
libs/core/kiln_ai/datamodel/spec.py (2)
Spec(21-55)SpecStatus(12-18)app/web_ui/src/lib/types.ts (3)
Spec(109-109)SpecStatus(111-111)SpecProperties(113-115)
libs/core/kiln_ai/datamodel/test_spec.py (3)
libs/core/kiln_ai/datamodel/spec.py (2)
Spec(21-55)SpecStatus(12-18)libs/core/kiln_ai/datamodel/spec_properties.py (16)
AppropriateToolUseProperties(111-115)BiasProperties(87-88)CompletenessProperties(63-64)ConsistencyProperties(67-68)DesiredBehaviourProperties(43-44)FactualCorrectnessProperties(55-56)FormattingProperties(75-76)HallucinationsProperties(59-60)JailbreakProperties(103-104)LocalizationProperties(79-80)MaliciousnessProperties(91-92)NsfwProperties(95-96)PromptLeakageProperties(107-108)ReferenceAnswerAccuracyProperties(51-52)SpecType(8-40)UndesiredBehaviourProperties(144-147)libs/core/kiln_ai/datamodel/test_task.py (1)
sample_task(59-60)
libs/server/kiln_server/test_spec_api.py (2)
libs/core/kiln_ai/datamodel/spec.py (2)
Spec(21-55)SpecStatus(12-18)libs/core/kiln_ai/datamodel/spec_properties.py (5)
DesiredBehaviourProperties(43-44)HallucinationsProperties(59-60)SpecType(8-40)ToxicityProperties(83-84)UndesiredBehaviourProperties(144-147)
libs/core/kiln_ai/datamodel/spec_properties.py (1)
app/web_ui/src/lib/types.ts (2)
SpecType(110-110)SpecProperties(113-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Generate Coverage Report
- GitHub Check: Generate Coverage Report
🔇 Additional comments (14)
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte (1)
42-48: LGTM on the payload structure.The request body correctly uses
name,description, andproperties: { spec_type: spec_type }which aligns with the newSpecUpsertRequestmodel in the backend that expects a discriminated union for properties.app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte (1)
92-95: LGTM on the title rendering usingproperties.spec_type.The optional chaining (
spec?.properties?.spec_type) correctly handles cases where spec or properties might be undefined during loading.libs/server/kiln_server/spec_api.py (2)
37-54: LGTM oncreate_specendpoint.The endpoint correctly constructs a
Specwith the newdescriptionandpropertiesfields, and the discriminated union ensures type-safe property handling.
65-80: LGTM onupdate_specendpoint.The PATCH endpoint correctly updates both
descriptionandpropertiesfields on the existing spec before saving.libs/server/kiln_server/test_spec_api.py (3)
8-15: LGTM on the updated imports.The imports correctly reference
SpecTypeand property classes fromkiln_ai.datamodel.spec_properties, aligning with the refactored module structure.
374-414: LGTM on thetest_create_spec_with_propertiestest.Good coverage for creating specs with
UndesiredBehaviourProperties, verifying both the API response and persisted data include the correct property values.
1034-1163: LGTM on the validation tests for empty property fields.The tests comprehensively verify that empty
tool_id,appropriate_tool_use_guidelines,undesired_behaviour_guidelines, andexamplesare properly rejected with 422 responses and appropriate error messages.libs/core/kiln_ai/datamodel/test_spec.py (3)
1-27: LGTM on the comprehensive imports.All required property classes and
SpecTypeare correctly imported fromkiln_ai.datamodel.spec_properties, andValidationErroris imported from Pydantic for proper exception testing.
368-394: LGTM on validation tests for missing required property fields.These tests correctly verify that
ValidationErroris raised when required fields likeappropriate_tool_use_guidelinesorexamplesare missing from the properties dict.
397-424: LGTM on wrong spec_type literal validation tests.The tests correctly verify that passing an invalid
spec_typeliteral (like"wrong_type") raises aValidationError, ensuring the discriminated union validation works properly.app/web_ui/src/lib/api_schema.d.ts (2)
2193-2206: Spec property schemas form a consistent discriminated unionAll the new
*Propertiesschemas use a uniformspec_typestring literal discriminator, and the shapes (including the extra fields onAppropriateToolUsePropertiesandUndesiredBehaviourProperties) look internally consistent. This is a good fit for narrowing onproperties.spec_typein the client, and is appropriate for a generated OpenAPI → TS layer. No changes requested here.As per retrieved learnings, any further tweaks should go via the backend models/OpenAPI spec and then re‑generate this file, not by hand-editing it.
Also applies to: 2254-2261, 2551-2566, 3098-3110, 3793-3804, 4084-4091, 4119-4126, 4147-4154, 4156-4163, 4405-4412, 4427-4434, 4497-4503, 4721-4728, 4987-4993, 5397-5404, 5825-5832, 5889-5896, 5898-5909
5327-5336: Spec/SpecUpsertRequest description + properties union look alignedSwitching from
definitionto a requireddescription: stringand adding thepropertiesunion that mirrors all the new spec property schemas keepsSpecandSpecUpsertRequestin sync and matches the discriminator design above. From the frontend typing perspective this is coherent and should work cleanly with Svelte consumers usingspec.properties.spec_typeto branch. No issues from the TS/schema side.Given this file is generated, ensure any future changes are made in the FastAPI/OpenAPI schema and then re-generated, rather than editing this declaration file directly (based on retrieved learnings).
Also applies to: 5372-5375
libs/core/kiln_ai/datamodel/spec_properties.py (2)
8-109: Discriminated union design forSpecTypeand property TypedDicts looks goodEnum values and
spec_type: Literal[...]fields line up cleanly for a Pydantic-discriminated union, and the property groups are well-factored for future extension. No changes needed here.
176-196:SpecPropertiesunion composition is consistent and ready for discriminated useThe union correctly mixes the two validated variants with the plain TypedDict variants, all sharing a
spec_typeLiteral discriminator. This should work well withField(discriminator="spec_type")in theSpecmodel.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte (1)
75-75: Minor: Inconsistent nullish coalescing operator.This uses
|| nullwhile the list page (+page.svelteat lines 394, 472) uses?? null. For consistency and correctness (to avoid coercing empty strings or0tonull), consider using?? nullhere as well.- eval_id: spec.eval_id || null, + eval_id: spec.eval_id ?? null,app/web_ui/src/lib/types.ts (1)
112-130: Remove duplicateAppropriateToolUsePropertiesfrom union.
AppropriateToolUsePropertiesappears twice in theSpecPropertiesunion (lines 113 and 118). While TypeScript unions automatically deduplicate identical members, the duplicate indicates a copy-paste error and reduces code clarity.Apply this diff to remove the duplicate:
export type SpecProperties = | components["schemas"]["AppropriateToolUseProperties"] | components["schemas"]["BehaviourProperties"] | components["schemas"]["ToneProperties"] | components["schemas"]["FormattingProperties"] | components["schemas"]["LocalizationProperties"] - | components["schemas"]["AppropriateToolUseProperties"] | components["schemas"]["ReferenceAnswerAccuracyProperties"] | components["schemas"]["FactualCorrectnessProperties"] | components["schemas"]["HallucinationsProperties"] | components["schemas"]["CompletenessProperties"] | components["schemas"]["ToxicityProperties"] | components["schemas"]["BiasProperties"] | components["schemas"]["MaliciousnessProperties"] | components["schemas"]["NsfwProperties"] | components["schemas"]["TabooProperties"] | components["schemas"]["JailbreakProperties"] | components["schemas"]["PromptLeakageProperties"]libs/core/kiln_ai/datamodel/spec_properties.py (3)
40-68: Clarify assumptions and typing invalidate_string_properties.
validate_string_propertiesassumes that allrequired_fieldsandoptional_fieldsmap tostrvalues (because of.strip()), but the signature is fully generic overTand doesn’t constrainpropertiesto a mapping ofstrkeys to stringy values. To make misuse harder and static typing clearer, consider tightening the types, e.g. by constrainingTto aMapping[str, Any](or similar) and/or assertingisinstance(value, str)before calling.strip(). This keeps the runtime behavior the same under Pydantic (where fields are already parsed asstr) but makes the helper safer for any future non‑Pydantic callers.
71-427: Revisit optional field semantics (str | None+total=True).All the
*_examplesand similar “optional” fields are declared asstr | NoneonTypedDict(..., total=True). That models them as required keys that may benull, rather than truly optional keys. Your validators also allowNone(they only reject empty/whitespace strings), so at the type level there’s currently no way to express “key may be absent, but if present must be a non‑empty string”.If the API contract is that these example fields are genuinely optional (and not always present as
null), it may be clearer to flip them toNotRequired[str]instead ofstr | Noneand let the validators enforce non‑empty strings when present. If the wire format always includes these keys and usesnullto mean “no examples”, then the current typing is fine as‑is, but it would be worth confirming that intent and keeping it consistent across the schemas.
10-37: Discriminated union design forSpecPropertieslooks solid.The
SpecTypeenum, per‑typespec_type: Literal[...]fields, and theSpecPropertiesunion of*PropertiesValidatoraliases form a clean discriminated union that should work well with Pydantic v2’sField(discriminator="spec_type")on theSpecmodel. Coverage of all enum variants is complete and the per‑type validators correctly delegate to the shared string validator with appropriate required/optional fields. No issues from a modeling or Pydantic‑usage perspective.As per coding guidelines, if you’d like, I can sketch the corresponding
Specmodel field definition using this union (with proper discriminator wiring) to double‑check end‑to‑end validation.Also applies to: 71-77, 95-452
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/web_ui/src/lib/api_schema.d.ts(17 hunks)app/web_ui/src/lib/types.ts(1 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelte(4 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte(3 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte(3 hunks)app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/template_select/+page.svelte(2 hunks)libs/core/kiln_ai/datamodel/spec.py(2 hunks)libs/core/kiln_ai/datamodel/spec_properties.py(1 hunks)libs/core/kiln_ai/datamodel/test_spec.py(9 hunks)libs/core/kiln_ai/datamodel/test_spec_properties.py(1 hunks)libs/core/kiln_ai/datamodel/test_task.py(4 hunks)libs/server/kiln_server/spec_api.py(4 hunks)libs/server/kiln_server/test_spec_api.py(41 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/+page.svelte
- libs/server/kiln_server/spec_api.py
- libs/core/kiln_ai/datamodel/test_task.py
🧰 Additional context used
📓 Path-based instructions (11)
app/web_ui/**/*.svelte
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
app/web_ui/**/*.svelte: Use Svelte v4 (not v5) for the web frontend UI
Use DaisyUI for UI components in the web frontendUse Svelte v4 (not v5) for the web UI framework
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/template_select/+page.svelte
app/web_ui/**/*.{svelte,ts}
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Tailwind CSS for styling in the web frontend
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/template_select/+page.svelteapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
app/web_ui/**/*.{svelte,css,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Tailwind and DaisyUI for styling the frontend
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/template_select/+page.svelteapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
app/web_ui/**/*.{svelte,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Make FastAPI backend calls from the Svelte web app to communicate with backend servers
Files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/template_select/+page.svelteapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
libs/core/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use Python 3.10+ for the core library (libs/core) and Python 3.13 for the desktop app
Use python 3.10+ for the core library (libs/core) and python 3.13 for the desktop app
Files:
libs/core/kiln_ai/datamodel/test_spec_properties.pylibs/core/kiln_ai/datamodel/test_spec.pylibs/core/kiln_ai/datamodel/spec.pylibs/core/kiln_ai/datamodel/spec_properties.py
**/*test*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use pytest for testing Python code
Use pytest for testing in python projects
Files:
libs/core/kiln_ai/datamodel/test_spec_properties.pylibs/server/kiln_server/test_spec_api.pylibs/core/kiln_ai/datamodel/test_spec.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
**/*.py: Use asyncio for asynchronous operations in Python
Use Pydantic v2 (not v1) for data validation
Files:
libs/core/kiln_ai/datamodel/test_spec_properties.pylibs/server/kiln_server/test_spec_api.pylibs/core/kiln_ai/datamodel/test_spec.pylibs/core/kiln_ai/datamodel/spec.pylibs/core/kiln_ai/datamodel/spec_properties.py
libs/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Pydantic v2 (not v1) for data validation and serialization
Files:
libs/core/kiln_ai/datamodel/test_spec_properties.pylibs/server/kiln_server/test_spec_api.pylibs/core/kiln_ai/datamodel/test_spec.pylibs/core/kiln_ai/datamodel/spec.pylibs/core/kiln_ai/datamodel/spec_properties.py
libs/server/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use FastAPI for creating REST servers
libs/server/**/*.py: Use FastAPI for REST server implementations
Use asyncio for asynchronous operations in Python
Use FastAPI with asyncio for the REST server wrapping the core library
Files:
libs/server/kiln_server/test_spec_api.py
app/web_ui/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use TypeScript for frontend type safety
Files:
app/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
app/web_ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for frontend development
Files:
app/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 759
File: app/web_ui/src/lib/api_schema.d.ts:2485-2490
Timestamp: 2025-10-29T11:19:31.059Z
Learning: In libs/server/kiln_server/document_api.py and the generated app/web_ui/src/lib/api_schema.d.ts, CreateVectorStoreConfigRequest.properties is now a union of three distinct public schemas: LanceDBConfigFTSPropertiesPublic, LanceDBConfigVectorPropertiesPublic, and LanceDBConfigHybridPropertiesPublic. These are intentionally split to allow independent evolution; do not suggest merging them.
📚 Learning: 2025-09-25T07:20:11.459Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 650
File: app/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelte:19-19
Timestamp: 2025-09-25T07:20:11.459Z
Learning: When analyzing relative import paths in SvelteKit route structures, be more careful to verify the actual directory structure exists before suggesting corrections. The import path ../../../../generate/[project_id]/[task_id]/table_button.svelte from app/web_ui/src/routes/(app)/docs/library/[project_id]/[document_id]/+page.svelte correctly resolves to the existing file at app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/table_button.svelte.
Applied to files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelte
📚 Learning: 2025-10-29T06:59:55.635Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 736
File: app/web_ui/src/lib/api_schema.d.ts:8143-8180
Timestamp: 2025-10-29T06:59:55.635Z
Learning: app/web_ui/src/lib/api_schema.d.ts is generated by openapi-typescript; do not propose manual edits. Schema changes should be made in the FastAPI backend (e.g., app/desktop/studio_server/data_gen_api.py or libs/server/kiln_server/*), then re-generate the TS types.
Applied to files:
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelteapp/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
📚 Learning: 2025-10-07T11:59:47.217Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 704
File: libs/core/kiln_ai/datamodel/chunk.py:170-182
Timestamp: 2025-10-07T11:59:47.217Z
Learning: In the Kiln AI datamodel (libs/core/kiln_ai/datamodel), validation functions should not set defaults. The datamodel requires everything to be explicit, and all required fields must be provided by the caller rather than being populated by validation logic.
Applied to files:
libs/core/kiln_ai/datamodel/test_spec_properties.py
📚 Learning: 2025-10-29T11:19:31.059Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 759
File: app/web_ui/src/lib/api_schema.d.ts:2485-2490
Timestamp: 2025-10-29T11:19:31.059Z
Learning: In libs/server/kiln_server/document_api.py and the generated app/web_ui/src/lib/api_schema.d.ts, CreateVectorStoreConfigRequest.properties is now a union of three distinct public schemas: LanceDBConfigFTSPropertiesPublic, LanceDBConfigVectorPropertiesPublic, and LanceDBConfigHybridPropertiesPublic. These are intentionally split to allow independent evolution; do not suggest merging them.
Applied to files:
app/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
📚 Learning: 2025-09-04T06:27:58.852Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 487
File: app/web_ui/src/lib/api_schema.d.ts:3315-3320
Timestamp: 2025-09-04T06:27:58.852Z
Learning: The file app/web_ui/src/lib/api_schema.d.ts is auto-generated by openapi-typescript from the backend API schema and should never be manually edited. Changes to API types should be made in the backend, which will then be reflected in the regenerated TypeScript definitions.
Applied to files:
app/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
📚 Learning: 2025-09-10T08:30:56.973Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 546
File: app/web_ui/src/lib/api_schema.d.ts:4173-4218
Timestamp: 2025-09-10T08:30:56.973Z
Learning: leonardmq correctly identified that app/web_ui/src/lib/api_schema.d.ts is auto-generated and should not be manually edited. The proper fix for type inconsistencies between TypeScript API schema and backend models is to update the backend Python models and regenerate the TypeScript definitions.
Applied to files:
app/web_ui/src/lib/api_schema.d.tsapp/web_ui/src/lib/types.ts
📚 Learning: 2025-10-28T05:25:09.283Z
Learnt from: leonardmq
Repo: Kiln-AI/Kiln PR: 757
File: libs/core/kiln_ai/adapters/reranker_list.py:39-42
Timestamp: 2025-10-28T05:25:09.283Z
Learning: In the Kiln codebase (Python), model family and model name fields in reranker and similar model definition classes (like KilnRerankerModel in libs/core/kiln_ai/adapters/reranker_list.py) should use plain str types rather than Enum types. This is because users receive over-the-air (OTA) updates for new models, and using Enums would break when clients with older builds encounter new model families their Enum definitions don't include. Strings provide forward compatibility for the OTA update mechanism.
Applied to files:
libs/core/kiln_ai/datamodel/spec.py
📚 Learning: 2025-11-24T19:13:16.956Z
Learnt from: CR
Repo: Kiln-AI/Kiln PR: 0
File: .cursor/rules/project.mdc:0-0
Timestamp: 2025-11-24T19:13:16.956Z
Learning: Applies to app/web_ui/**/*.ts : Use TypeScript for frontend type safety
Applied to files:
app/web_ui/src/lib/types.ts
📚 Learning: 2025-11-24T19:13:27.852Z
Learnt from: CR
Repo: Kiln-AI/Kiln PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:13:27.852Z
Learning: Applies to app/web_ui/**/*.{ts,tsx} : Use TypeScript for frontend development
Applied to files:
app/web_ui/src/lib/types.ts
🧬 Code graph analysis (6)
libs/core/kiln_ai/datamodel/test_spec_properties.py (1)
libs/core/kiln_ai/datamodel/spec_properties.py (1)
validate_string_properties(40-68)
libs/server/kiln_server/test_spec_api.py (3)
libs/core/kiln_ai/datamodel/spec.py (2)
Spec(21-57)SpecStatus(12-18)libs/core/kiln_ai/datamodel/spec_properties.py (5)
BehaviourProperties(71-76)HallucinationsProperties(242-245)SpecType(10-37)ToneProperties(95-100)ToxicityProperties(287-290)libs/core/kiln_ai/datamodel/test_spec.py (1)
sample_tone_properties(34-41)
libs/core/kiln_ai/datamodel/test_spec.py (4)
app/web_ui/src/lib/types.ts (4)
Spec(109-109)SpecStatus(110-110)SpecType(130-130)Priority(111-111)libs/core/kiln_ai/datamodel/spec.py (2)
Spec(21-57)SpecStatus(12-18)libs/core/kiln_ai/datamodel/spec_properties.py (4)
AppropriateToolUseProperties(164-170)BehaviourProperties(71-76)SpecType(10-37)ToneProperties(95-100)libs/core/kiln_ai/datamodel/test_task.py (1)
sample_task(59-60)
libs/core/kiln_ai/datamodel/spec.py (1)
app/web_ui/src/lib/types.ts (1)
SpecProperties(112-129)
libs/core/kiln_ai/datamodel/spec_properties.py (2)
app/web_ui/src/lib/types.ts (2)
SpecType(130-130)SpecProperties(112-129)libs/core/kiln_ai/utils/filesystem_cache.py (1)
get(24-35)
app/web_ui/src/lib/types.ts (2)
app/web_ui/src/lib/api_schema.d.ts (1)
components(2157-6077)libs/core/kiln_ai/datamodel/spec_properties.py (1)
SpecType(10-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Generate Coverage Report
- GitHub Check: Build, Typecheck, and Test Python (3.13)
- GitHub Check: Build, Typecheck, and Test Python (3.11)
- GitHub Check: Build, Typecheck, and Test Python (3.12)
- GitHub Check: Build, Typecheck, and Test Python (3.10)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (macos-15-intel)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
🔇 Additional comments (21)
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/+page.svelte (4)
186-187: LGTM on sort logic update.The sorting logic correctly references
spec.properties.spec_typeto align with the new properties-based model.
390-394: Payload structure looks correct.The PATCH body properly includes
properties: spec.propertiesand uses?? nullforeval_idto handle nullish values correctly.
468-472: Consistent with other PATCH payloads.The archive flow uses the same payload structure with
propertiesandeval_id ?? null.
683-683: Type display updated correctly.The table cell now displays the formatted spec type from
spec.properties.spec_type.libs/core/kiln_ai/datamodel/test_spec_properties.py (1)
1-164: Comprehensive test coverage forvalidate_string_properties.The tests thoroughly cover required and optional field validation scenarios including edge cases for
None, empty strings, whitespace-only values, and missing fields. Good use of descriptive docstrings.app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/[spec_id]/+page.svelte (2)
91-93: Title logic correctly updated for properties-based model.The title now dynamically prefixes the spec name with the formatted spec type from
spec.properties.spec_type.
71-71: Properties field correctly used in payload and display.The PATCH payload and Properties panel display both correctly reference
spec.propertiesandspec.properties.spec_type.Also applies to: 125-125
app/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/create_spec/template_select/+page.svelte (2)
35-38: Template consolidation looks good.Consolidating "desired_behaviour" and "undesired_behaviour" into a single "behaviour" type with the description "follow or avoid" is a cleaner approach.
131-140:existing_spec_typesset updated correctly.The set now includes
"behaviour"and reflects the currently implemented spec types.libs/core/kiln_ai/datamodel/spec.py (2)
28-31: Discriminator pattern correctly implemented.The
propertiesfield withdiscriminator="spec_type"enables Pydantic to automatically validate and type-discriminate based on the spec type. This follows the recommendation from past reviews.
25-27: Clarify:definitionfield retention vs. PR title.The PR title mentions "changed definition to description", but the
definitionfield is retained here. Given the PR author's comment that "the spec model object is in flux", please confirm whether:
- This field should remain as
definition, or- This should be renamed to
descriptionas part of this PR.libs/server/kiln_server/test_spec_api.py (3)
52-81: LGTM! Well-structured property helper functions.The property dict helper functions are well-designed with clear names, documentation, and proper enum value serialization for API payloads.
83-125: LGTM! Well-structured fixtures for property objects.The fixtures properly use TypedDict classes and provide good reusability across tests.
1129-1264: LGTM! Comprehensive validation tests for property fields.The new validation tests provide excellent coverage for empty string validation across different property types (tool use, behaviour). The assertions properly check for 422 status codes and field-specific error messages.
libs/core/kiln_ai/datamodel/test_spec.py (5)
147-262: LGTM! Comprehensive helper for all spec property types.The
create_sample_propertieshelper provides excellent coverage for all 16 spec types with proper TypedDict usage and clear error handling. This is a well-designed test utility.
393-440: LGTM! Thorough tests for AppropriateToolUseProperties.The tests properly validate both required and optional fields of
AppropriateToolUseProperties, including proper type checks and field value assertions.
442-470: LGTM! Good test coverage for BehaviourProperties.The test properly validates the
BehaviourPropertiesstructure including behavior description and example fields.
472-534: LGTM! Comprehensive property validation tests.The validation tests provide excellent coverage for missing required fields and invalid spec_type values, properly using Pydantic's
ValidationErrorand pytest assertions.
536-567: LGTM! Good tests for edge cases and field coexistence.The tests properly validate that empty property dicts are rejected and that the
definitionfield works correctly alongsideproperties. Note that thedefinitionfield is still present in the code, which appears inconsistent with the PR summary that mentions changing "definition to description".app/web_ui/src/lib/api_schema.d.ts (2)
2193-2210: New Spec*Properties schemas are consistent and discriminator-friendlyAll of the new
*Propertiesschemas share a literalspec_typediscriminator plus a consistentbase_instruction+ optional examples structure, which should work well as a discriminated union forSpec.propertiesandSpecUpsertRequest.propertiesand give good narrowing on the frontend. No structural or typing issues stand out here.Based on learnings, this file is generated by openapi-typescript and should not be edited manually; any future shape changes should be driven from the backend models and the types regenerated.
Also applies to: 2258-2285, 2575-2588, 3811-3822, 4102-4117, 4145-4156, 4178-4188, 4431-4444, 4460-4470, 4533-4543, 4762-4772, 5031-5045, 5450-5460
5357-5433: Verify Spec/SpecUpsertRequestdefinitionvsdescriptionand requiredproperties
SpecandSpecUpsertRequestnow expose a requiredpropertiesunion, but the API schema still usesdefinition: stringand does not expose adescriptionfield. Given the PR description mentions changing “definition” to “description”, please double‑check:
- Is the external API intentionally staying on
definitionfor compatibility while internal models move todescription, or is the rename meant to be reflected in the OpenAPI schema too?- Should
propertiesbe required onSpecUpsertRequest, or do you expect existing callers that only send a definition/description to remain valid?If the current shape is not intentional, update the FastAPI/Pydantic models in the backend and regenerate this file rather than editing it directly.
Based on learnings,
app/web_ui/src/lib/api_schema.d.tsis auto-generated; aligning it should be done via backend schema changes and re-generation.
Summary by CodeRabbit
New Features
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.