Resolve UseExisting string instead of leaking it to output#155
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3777599b32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/colin/llm/prompts/extract.md
Outdated
| {% if previous_output %} | ||
| Provide the extracted information. If previous output is still valid, respond with UseExisting instead. |
There was a problem hiding this comment.
The new {% if previous_output %} condition is false when the prior extraction output is the empty string, so the prompt omits both the previous output section and the UseExisting instruction even though a previous result exists. In that scenario (empty-string output with unchanged content), the LLM is no longer prompted to return UseExisting, so caching/stability can regress compared to prior behavior. Consider testing explicitly for previous_output is not none instead of truthiness so empty but valid outputs still enable the reuse path.
Useful? React with 👍 / 👎.
tests/providers/test_llm_provider.py
Outdated
|
|
||
| provider = LLMProvider(model=FunctionModel(return_use_existing)) | ||
|
|
||
| from colin.compiler.cache import _serialize_value |
There was a problem hiding this comment.
The repo’s AGENTS.md (Colin Development Guidelines) mandates that all imports be at module scope; the local from colin.compiler.cache import _serialize_value inside the test violates this rule and is likely to fail the required lint step (uv run prek run --all-files). Please move it to the module-level imports.
Useful? React with 👍 / 👎.
… to module level
1 participant
When the LLM returns the literal text "UseExisting" to signal that previous output is still valid,
_extractand_completepass it through as template output. Users see "UseExisting" rendered in their documents instead of the actual previous content.The fix detects the "UseExisting" response and substitutes the previous output. The resolved value (not the sentinel string) is what gets recorded in the manifest, so downstream caching works correctly. The extract prompt template also now only mentions UseExisting when previous output actually exists, avoiding confusing the LLM when there's nothing to reuse.
Fixes #152