[FIX] Seed challenge_llm from the exported tool's resolved value so challenge-enabled deploys pass#2209
Open
hari-kuriakose wants to merge 1 commit into
Open
Conversation
…hallenge-enabled deploys pass
A tool exported with LLMChallenge enabled failed at the final `deployment run`
with `422 Unprocessable Entity: Tool validation failed`, even though export had
resolved a valid challenge LLM.
The chain:
1. Export resolves a real `challenge_llm` - falling back to the default
profile's LLM when the project set none - and stores it under
`tool_metadata[tool_settings]`.
2. Tool-instance creation seeds metadata from `get_default_settings`, which
walks the spec: a string property with no `default` is seeded "".
`challenge_llm` has no spec default, so the instance stores "", discarding
the value export resolved.
3. Because `challenge_llm` declares `adapterType: "LLM"`, an `enum` of real
adapter IDs is injected into the tool-instance schema, and "" is not in it.
4. Deployment validation rejects "" - an enum violation.
A sibling change made the challenge-*off* case pass by omitting the enum when
the feature is disabled. With challenge *on* the enum is present and required,
so the seeded "" still fails. This is the root-cause fix for that remaining
case.
The resolved value was not reachable at the seed site: `get_tool_by_prompt_registry_id`
builds `Tool` from `tool_spec`/`tool_property` and drops `tool_metadata`. This
adds `PromptStudioRegistryHelper.get_resolved_settings`, which reads
`tool_metadata[tool_settings]`, and has tool-instance creation overlay those
values onto the spec-seeded defaults.
Scope guards:
- Only spec-declared keys are overlaid. The export's `tool_settings` is a
superset (llm, vector-db, preamble, ...); overlaying all of it would inject
non-spec keys into instance metadata. The loop iterates the spec-seeded keys
and pulls a resolved value only when one exists.
- `get_resolved_settings` returns {} for a missing registry row, so a
non-Prompt-Studio tool (regular registry tool, or agentic tool in cloud)
resolves to {} and the overlay is a no-op.
Not in scope: the instance still seeds `enable_challenge` from the spec default
(False) rather than the tool's real setting. That affects whether challenge
*runs*, not whether deploy *validates*, and is a separate change. Also, a real
`challenge_llm` id only validates if the deploying user has access to that
adapter (the enum is built per-user); cross-user deploy is a pre-existing
residual, not introduced here.
Verified by mutation: neutering the overlay loop fails the seed and
deploy-validation tests, while the scope-guard and no-op tests stay green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughPrompt-studio registry settings are now retrieved from registry metadata and overlaid onto tool-instance defaults during creation. Regression tests cover resolved adapter seeding, schema validation, declared-key filtering, empty settings, and missing registry metadata. ChangesResolved Tool Settings Overlay
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ToolInstanceSerializer
participant PromptStudioRegistryHelper
participant PromptStudioRegistry
ToolInstanceSerializer->>PromptStudioRegistryHelper: Request resolved settings
PromptStudioRegistryHelper->>PromptStudioRegistry: Fetch registry metadata
PromptStudioRegistry-->>PromptStudioRegistryHelper: Return tool settings
PromptStudioRegistryHelper-->>ToolInstanceSerializer: Return resolved settings or {}
ToolInstanceSerializer->>ToolInstanceSerializer: Overlay declared settings onto defaults
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Contributor
|
| Filename | Overview |
|---|---|
| backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py | Adds a tolerant lookup for the resolved tool-settings block stored in Prompt Studio registry metadata. |
| backend/tool_instance_v2/serializers.py | Overlays resolved Prompt Studio values onto spec-derived defaults while excluding undeclared metadata keys. |
| backend/tool_instance_v2/tests/test_challenge_llm_seed_overlay.py | Adds focused regression tests covering the resolved-value overlay and challenge-enabled schema validation. |
| backend/tool_instance_v2/tests/init.py | Marks the tool-instance test directory as a Python package. |
Reviews (1): Last reviewed commit: "[FIX] Seed challenge_llm from the export..." | Re-trigger Greptile
Contributor
Unstract test resultsPer-group results
Critical paths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py`:
- Around line 146-170: Update get_resolved_settings to validate
prompt_registry_id as a UUID before calling PromptStudioRegistry.objects.get or
logging; return an empty dict immediately for invalid identifiers so
non-Prompt-Studio tool instances remain a silent no-op, while preserving the
existing database lookup behavior for valid UUIDs.
In `@backend/tool_instance_v2/serializers.py`:
- Around line 176-195: Update the settings overlay in the tool-instance
serializer to map the resolved export field
JsonSchemaKey.ENABLE_SINGLE_PASS_EXTRACTION to the spec property
single_pass_extraction_mode before writing tool_settings. Preserve the existing
direct-key overlay for other settings and ensure enabled single-pass extraction
is not replaced by the default False.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ccec1d0-efc9-456f-908a-f71b05ee1c94
📒 Files selected for processing (4)
backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.pybackend/tool_instance_v2/serializers.pybackend/tool_instance_v2/tests/__init__.pybackend/tool_instance_v2/tests/test_challenge_llm_seed_overlay.py
Comment on lines
+146
to
+170
| @staticmethod | ||
| def get_resolved_settings(prompt_registry_id: str) -> dict[str, Any]: | ||
| """Return the settings export already resolved for this exported tool. | ||
|
|
||
| ``frame_export_json`` resolves adapter-valued settings at export time - | ||
| notably ``challenge_llm``, which falls back to the default profile's LLM | ||
| when the project set none - and stores them under | ||
| ``tool_metadata[tool_settings]``. ``Tool`` (built from ``tool_spec`` / | ||
| ``tool_property``) does not carry them, so callers that only have a | ||
| ``Tool`` cannot see the resolved values. | ||
|
|
||
| Returns an empty dict when the registry row is missing or carries no | ||
| settings, so callers can treat "no resolved settings" as a no-op. | ||
| """ | ||
| try: | ||
| prompt_registry_tool = PromptStudioRegistry.objects.get(pk=prompt_registry_id) | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"Error while fetching resolved settings for prompt registry ID " | ||
| f"{prompt_registry_id}: {e}" | ||
| ) | ||
| return {} | ||
| metadata = prompt_registry_tool.tool_metadata or {} | ||
| return metadata.get(JsonSchemaKey.TOOL_SETTINGS, {}) or {} | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
New helper logs a WARNING on every non-Prompt-Studio tool-instance creation.
get_resolved_settings is called unconditionally from ToolInstanceSerializer.create() for every tool type via str(tool_uid). prompt_registry_id is a UUID primary key, so any non-Prompt-Studio tool_uid (the common case — built-in/registry tools) will fail PromptStudioRegistry.objects.get(pk=...) and hit the except Exception branch, logging a warning on every single such creation. The docstring frames "missing settings" as an expected no-op, but the implementation logs it as a warning regardless, which will flood production logs and mask genuine errors.
Short-circuit on an invalid UUID before touching the DB/logger:
🔇 Proposed fix
+import uuid
+
`@staticmethod`
def get_resolved_settings(prompt_registry_id: str) -> dict[str, Any]:
...
+ try:
+ uuid.UUID(str(prompt_registry_id))
+ except (ValueError, AttributeError, TypeError):
+ # Not a Prompt Studio registry ID (e.g. a built-in tool's uid);
+ # nothing to resolve, and not worth a warning.
+ return {}
try:
prompt_registry_tool = PromptStudioRegistry.objects.get(pk=prompt_registry_id)
except Exception as e:
logger.warning(
f"Error while fetching resolved settings for prompt registry ID "
f"{prompt_registry_id}: {e}"
)
return {}Note: the existing test double for this function (test_get_resolved_settings_reads_the_tool_settings_block) would still pass unchanged since it only exercises the DB-lookup branch.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 162-162: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py`
around lines 146 - 170, Update get_resolved_settings to validate
prompt_registry_id as a UUID before calling PromptStudioRegistry.objects.get or
logging; return an empty dict immediately for invalid identifiers so
non-Prompt-Studio tool instances remain a silent no-op, while preserving the
existing database lookup behavior for valid UUIDs.
Source: Linters/SAST tools
Comment on lines
+176
to
+195
| tool_settings = ToolProcessor.get_default_settings(tool) | ||
| # `get_default_settings` seeds an adapter-valued property with "" when | ||
| # the spec carries no default (e.g. challenge_llm). The exported tool | ||
| # already resolved a real value for it, so prefer that - otherwise the | ||
| # instance stores "", which fails deployment validation against the | ||
| # adapter enum. Only spec-declared keys are overlaid; the export's | ||
| # settings are a superset (llm, vector-db, ...) and the rest are not | ||
| # part of the instance schema. Non-Prompt-Studio tools resolve to {} | ||
| # here, making this a no-op for them. | ||
| resolved_settings = PromptStudioRegistryHelper.get_resolved_settings( | ||
| prompt_registry_id=str(tool_uid) | ||
| ) | ||
| for key in tool_settings: | ||
| if key in resolved_settings: | ||
| tool_settings[key] = resolved_settings[key] | ||
| validated_data[TIKey.METADATA] = { | ||
| # TODO: Review and remove tool instance ID | ||
| WorkflowKey.WF_TOOL_INSTANCE_ID: str(validated_data[TIKey.PK]), | ||
| PromptStudioRegistryKeys.PROMPT_REGISTRY_ID: str(tool_uid), | ||
| **ToolProcessor.get_default_settings(tool), | ||
| **tool_settings, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'single_pass_extraction_mode|ENABLE_SINGLE_PASS_EXTRACTION' backend/prompt_studio/prompt_studio_registry_v2/Repository: Zipstack/unstract
Length of output: 702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ToolInstanceV2 serializer relevant section =="
rg -n "get_resolved_settings|get_default_settings|tool_settings|single_pass_extraction_mode|ENABLE_SINGLE_PASS_EXTRACTION|METADATA|TIKey" backend/tool_instance_v2/serializers.py
echo
echo "== prompt_studio_registry_helper relevant exports =="
sed -n '280,325p' backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py
sed -n '50,90p' backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py
echo
echo "== constants =="
rg -n "class JsonSchemaKey|ENABLE_SINGLE_PASS_EXTRACTION|SINGLE_PASS|single_pass_extraction_mode|ENABLE_HIGHLIGHT|ENABLE_WORD_CONFIDENCE|CHALLENGE_LLM|ENABLE_CHALLENGE|SUMMARIZE_AS_SOURCE" backend/prompt_studio/prompt_studio_registry_v2/constants.pyRepository: Zipstack/unstract
Length of output: 5802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tool_instance serializer diff/stat =="
git diff -- backend/tool_instance_v2/serializers.py | sed -n '1,120p' || true
git diff -- backend/tool_instance_v2/ --stat || true
echo
echo "== relevant tests mentioning single_pass_extraction_mode / ENABLE_SINGLE_PASS_EXTRACTION =="
rg -n "single_pass_extraction_mode|ENABLE_SINGLE_PASS_EXTRACTION|enable_single_pass_extraction" backend tests | sed -n '1,160p'
echo
echo "== read-only structural/behavioral verifier for overlay keys =="
python3 - <<'PY'
from pathlib import Path
import ast, re
spec_file = Path("backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py")
serializer_file = Path("backend/tool_instance_v2/serializers.py")
const_file = Path("backend/prompt_studio/prompt_studio_registry_v2/constants.py")
tree = ast.parse(spec_file.read_text())
export_props_found = []
default_props_found = []
for node in ast.walk(tree):
if isinstance(node, ast.Dict):
# find dicts whose keys include ENABLE_SINGLE_PASS_EXTRACTION or single_pass_extraction_mode
keys = [v.value if isinstance(v, ast.Constant) else None for v in [k for k in node.keys]]
if "enable_single_pass_extraction" in keys or "single_pass_extraction_mode" in keys:
if node.lineno < 150 and any(isinstance(k, ast.Constant) and k.value in ("single_pass_extraction_mode", "enable_challenge") for k in node.keys):
export_props_found = keys[:10]
elif node.lineno > 250 and node.lineno < 350:
export_props_found = keys[:25]
if node.col_offset == 0 and node.lineno < 120:
default_props_found = [v.value if isinstance(v, ast.Constant) else None for v in [k for k in node.keys]]
serializer_text = serializer_file.read_text()
const_tree = ast.parse(const_file.read_text())
const_dict = {}
for node in ast.walk(const_tree):
if isinstance(node, ast.Assign):
for t in node.targets:
if isinstance(t, ast.Name) and t.id in (
"CHALLENGE_LLM",
"ENABLE_CHALLENGE",
"SUMMARIZE_AS_SOURCE",
"ENABLE_SINGLE_PASS_EXTRACTION",
"ENABLE_HIGHLIGHT",
"ENABLE_WORD_CONFIDENCE",
):
const_dict[t.id] = node.value.value if isinstance(node.value, ast.Constant) else None
export_alias = const_dict["ENABLE_SINGLE_PASS_EXTRACTION"]
spec_key = "single_pass_extraction_mode"
overlay_match = bool(re.search(r"for key in tool_settings:(\s*\n\s*if key in resolved_settings:|.\n\s*if export_key in resolved_settings:)", serializer_text))
print("export_setting_key=", export_alias)
print("spec_property_key=", spec_key)
print("export_setting_matches_spec_property=", export_alias == spec_key)
print("overlay_checks_resolved_settings_key=", overlay_match)
print("overlapping_props=", [k for k in sorted(export_props_found or []) if k in sorted(default_props_found or [])])
PYRepository: Zipstack/unstract
Length of output: 2922
Overlay silently drops single_pass_extraction_mode.
tool_settings uses the spec property name (single_pass_extraction_mode), but get_resolved_settings() exports the model field under JsonSchemaKey.ENABLE_SINGLE_PASS_EXTRACTION (enable_single_pass_extraction). Since the overlay only matches key in resolved_settings, a Prompt-Studio tool exported with single-pass extraction enabled will seed the new instance with single_pass_extraction_mode: False, preserving the same silent-setting-drop shape as the challenge_llm bug this PR fixes.
🔧 One way to close the gap
+ # frame_export_json exports a couple of settings under a different
+ # key than the spec property name.
+ _EXPORT_KEY_ALIASES = {"single_pass_extraction_mode": "enable_single_pass_extraction"}
for key in tool_settings:
- if key in resolved_settings:
- tool_settings[key] = resolved_settings[key]
+ export_key = _EXPORT_KEY_ALIASES.get(key, key)
+ if export_key in resolved_settings:
+ tool_settings[key] = resolved_settings[export_key]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tool_settings = ToolProcessor.get_default_settings(tool) | |
| # `get_default_settings` seeds an adapter-valued property with "" when | |
| # the spec carries no default (e.g. challenge_llm). The exported tool | |
| # already resolved a real value for it, so prefer that - otherwise the | |
| # instance stores "", which fails deployment validation against the | |
| # adapter enum. Only spec-declared keys are overlaid; the export's | |
| # settings are a superset (llm, vector-db, ...) and the rest are not | |
| # part of the instance schema. Non-Prompt-Studio tools resolve to {} | |
| # here, making this a no-op for them. | |
| resolved_settings = PromptStudioRegistryHelper.get_resolved_settings( | |
| prompt_registry_id=str(tool_uid) | |
| ) | |
| for key in tool_settings: | |
| if key in resolved_settings: | |
| tool_settings[key] = resolved_settings[key] | |
| validated_data[TIKey.METADATA] = { | |
| # TODO: Review and remove tool instance ID | |
| WorkflowKey.WF_TOOL_INSTANCE_ID: str(validated_data[TIKey.PK]), | |
| PromptStudioRegistryKeys.PROMPT_REGISTRY_ID: str(tool_uid), | |
| **ToolProcessor.get_default_settings(tool), | |
| **tool_settings, | |
| tool_settings = ToolProcessor.get_default_settings(tool) | |
| # `get_default_settings` seeds an adapter-valued property with "" when | |
| # the spec carries no default (e.g. challenge_llm). The exported tool | |
| # already resolved a real value for it, so prefer that - otherwise the | |
| # instance stores "", which fails deployment validation against the | |
| # adapter enum. Only spec-declared keys are overlaid; the export's | |
| # settings are a superset (llm, vector-db, ...) and the rest are not | |
| # part of the instance schema. Non-Prompt-Studio tools resolve to {} | |
| # here, making this a no-op for them. | |
| resolved_settings = PromptStudioRegistryHelper.get_resolved_settings( | |
| prompt_registry_id=str(tool_uid) | |
| ) | |
| # frame_export_json exports a couple of settings under a different | |
| # key than the spec property name. | |
| _EXPORT_KEY_ALIASES = {"single_pass_extraction_mode": "enable_single_pass_extraction"} | |
| for key in tool_settings: | |
| export_key = _EXPORT_KEY_ALIASES.get(key, key) | |
| if export_key in resolved_settings: | |
| tool_settings[key] = resolved_settings[export_key] | |
| validated_data[TIKey.METADATA] = { | |
| # TODO: Review and remove tool instance ID | |
| WorkflowKey.WF_TOOL_INSTANCE_ID: str(validated_data[TIKey.PK]), | |
| PromptStudioRegistryKeys.PROMPT_REGISTRY_ID: str(tool_uid), | |
| **tool_settings, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tool_instance_v2/serializers.py` around lines 176 - 195, Update the
settings overlay in the tool-instance serializer to map the resolved export
field JsonSchemaKey.ENABLE_SINGLE_PASS_EXTRACTION to the spec property
single_pass_extraction_mode before writing tool_settings. Preserve the existing
direct-key overlay for other settings and ensure enabled single-pass extraction
is not replaced by the default False.
hari-kuriakose
requested review from
Deepak-Kesavan,
chandrasekharan-zipstack,
harini-venkataraman and
jaseemjaskp
July 25, 2026 12:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
A tool exported with LLMChallenge enabled fails at the final
deployment runwith422 Unprocessable Entity: Tool validation failed, even though export has already resolved a valid challenge LLM. This fixes the seeding at its root.This is the challenge-enabled counterpart to #2203, which unblocked the challenge-off case by omitting the adapter enum when the feature is disabled. With challenge on the enum is present and required, so the seeding bug still bites — that is what this PR addresses.
The chain
challenge_llm— falling back to the default profile's LLM when the project set none — and stores it undertool_metadata[tool_settings](prompt_studio_registry_helper.py:285).get_default_settings, which walks the spec: a"type": "string"property with nodefaultis seeded"".challenge_llmhas no spec default, so the instance stores""— discarding the resolved value.challenge_llmdeclaresadapterType: "LLM",_update_schema_for_adapter_typeinjectsenum: [<real adapter ids>]into the tool-instance schema.""— an enum violation.Why a simple overlay wasn't already possible
The resolved value is not reachable at the seed site.
get_tool_by_prompt_registry_idbuilds theToolfromtool_spec/tool_propertyand dropstool_metadata— where the resolved settings live. So the plumbing had to be added first.The fix
PromptStudioRegistryHelper.get_resolved_settings(prompt_registry_id)readstool_metadata[tool_settings], returning{}when the row or the block is absent.create()overlays those resolved values onto the spec-seeded defaults, sochallenge_llmcarries its real adapter id.Scope guards
tool_settingsis a superset —llm,vector-db,embedding,preamble,postamble,grammar,chunk-size, … Overlaying all of it would inject non-spec keys into instance metadata. The loop iterates the spec-seeded keys and pulls a resolved value only where one exists.get_resolved_settingsreturns{}for a missing registry row, so a regular registry tool (or an agentic tool in cloud) resolves to{}and the overlay does nothing.Explicitly not in scope
enable_challengefrom the spec default (False) rather than the tool's real setting. That affects whether challenge runs, not whether deploy validates — a separate latent issue, noted so it isn't assumed fixed.challenge_llmid only validates if the deploying user has access to that adapter (the enum is built per-user). Cross-user deploy is a pre-existing residual, not introduced by this change.Tests
tool_instance_v2/tests/test_challenge_llm_seed_overlay.py— 6 tests in the unit tier (no DB; function bodies are extracted and executed against stubs, matching the existingtest_build_index_payload.pyapproach, so they run in CI rather than skipping).create(), not a reimplementation, so a break in the shipped loop breaks these tests.""is asserted to still fail (enum violation), so the tests discriminate.get_resolved_settingsreads thetool_settingsblock specifically, nottool_metadatatop-level.Verified by mutation: neutering the overlay loop fails the seed and deploy-validation tests while the scope-guard and no-op tests stay green.
ruffandruff-formatpass at the version pinned in.pre-commit-config.yaml(v0.3.4).Related
🤖 Generated with Claude Code