Skip to content

Bugfix for 0 4 0 release#208

Open
bmerkle wants to merge 37 commits intomicrosoft:mainfrom
bmerkle:bugfix-for-0-4-0-release
Open

Bugfix for 0 4 0 release#208
bmerkle wants to merge 37 commits intomicrosoft:mainfrom
bmerkle:bugfix-for-0-4-0-release

Conversation

@bmerkle
Copy link
Contributor

@bmerkle bmerkle commented Feb 27, 2026

based on #200 I have added some bugfixes for the upcoming release 0-4-0

commits from me are all Commits on Feb 27, 2026
each commit covers one potential improvement or bug, also new testcases are added in this step
changelog should explain the reason

gvanrossum-ms and others added 30 commits February 17, 2026 09:02
reduce lookup to cache the ordinals_of_subset
Total Complexity reduces from O(n*k) to O(n+k)
e.g. didn't match &api-version=... in multi-parameter URLs. Fixed to [?&,].
There was a mutable default argument in collections.py — SemanticRefAccumulator.__init__ used set() as default parameter, shared across all instances.

The code has been rewritten to still track which search terms produced hits in the accumulator but in a way to avoid mutable default arguments.
Now:
__init__ always creates a fresh set() — no parameter

Add a with_term_matches factory that creates a new accumulator with a copy of term matches (makes the copy-vs-share explicit)

Update group_matches_by_type to use a copy instead of sharing the same set object
Update get_matches_in_scope and WhereSemanticRefExpr to use the factory

2nd change:
hit_count=1 for non-exact matches in collections.py MatchAccumulator.add — inflated hit counts for related-only matches. Fixed to hit_count=0.

added comments to the else branches

added additional tests
… the object — including inherited methods, dunder methods (__init__, __eq__, …), and class-level descriptors.

The fix switches to vars(self), which returns only the instance's __dict__ — i.e. the actual dataclass field values.

added testcases

removed unused method from class
— produced dataclass repr strings instead of actual facet values.

Off-by-one in get_enclosing_date_range_for_text_range
— used exclusive end ordinal directly, potentially indexing past the last message.
Fixed to use message_ordinal - 1.

added testcases
…ding a full chunk), the merged result would begin with a spurious separator like "\n\n". The fix adds a guard

added testcases
The coverage package is a dev/test dependency.

"user" role correctly but defaulted everything else to "assistant" — including "system" prompts.
MCP's SamplingMessage only accepts "user" or "assistant" roles.
A "system" prompt section from TypeChat would silently be sent as "assistant", which changes LLM behavior.

added testcases
dunder methods (__init__, __eq__, __hash__, …), and descriptors.

The fix switches to vars(self), which returns only the instance's __dict__ — the actual dataclass field values.
Combined with the not key.startswith("_") and is not None filters, the repr is now clean.

added testcases
…buteError.

The fix introduces a local ordinal = 0 counter incremented on each iteration,
which works regardless of the message implementation.

added message.timestamp null guard

fix related to the changes in SemanticRefAccumulator
use with_term_matches classmethod factory, which copies the search-term provenance set,
so mutations to the filtered accumulator's set don't affect the source.

added testcases
Copilot AI review requested due to automatic review settings February 27, 2026 15:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request contains bugfixes and improvements for the upcoming 0.4.0 release. The most significant change is the migration from direct OpenAI SDK usage to a provider-agnostic architecture using pydantic_ai, which enables support for 25+ AI providers (OpenAI, Anthropic, Google, Azure, etc.) through a unified interface. Additionally, the PR fixes numerous bugs across the codebase including string formatting errors, logic bugs, and API design issues.

Changes:

  • Introduced provider-agnostic model adapters using pydantic_ai for chat and embedding models
  • Fixed multiple critical bugs including provenance copying, facet serialization, and timestamp handling
  • Added comprehensive test coverage for new functionality and bug fixes
  • Moved pydantic-ai-slim from dev dependencies to runtime dependencies

Reviewed changes

Copilot reviewed 65 out of 66 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
uv.lock Moved pydantic-ai-slim[openai] from dev to runtime dependencies
pyproject.toml Updated dependency specification for pydantic-ai-slim
src/typeagent/aitools/model_adapters.py New module providing provider-agnostic chat and embedding model adapters
src/typeagent/aitools/embeddings.py Refactored to protocol-based interfaces (IEmbedder, IEmbeddingModel)
src/typeagent/aitools/vectorbase.py Updated to support dynamic embedding size discovery and improved error handling
src/typeagent/aitools/utils.py Fixed regex for parsing Azure endpoints with ampersand-separated query params
src/typeagent/knowpro/query.py Fixed manual ordinal counting and timestamp None guards
src/typeagent/knowpro/searchlang.py Removed duplicate methods, fixed repr, added missing action_group append
src/typeagent/knowpro/search.py Fixed repr to use vars() instead of dir()
src/typeagent/knowpro/collections.py Fixed hit_count initialization for non-exact matches and provenance copying
src/typeagent/knowpro/answers.py Fixed facet value stringification and off-by-one error in date range lookup
src/typeagent/knowpro/convknowledge.py Removed deprecated create_typechat_model function
src/typeagent/knowpro/convsettings.py Updated to use new model adapter interfaces
src/typeagent/knowpro/conversation_base.py Updated to use new chat model creation pattern
src/typeagent/knowpro/fuzzyindex.py Fixed deserialization logic
src/typeagent/knowpro/knowledge.py Removed max_retries parameter
src/typeagent/knowpro/serialization.py Added embedding size metadata to file headers
src/typeagent/knowpro/interfaces_storage.py Removed embedding_size field from ConversationMetadata
src/typeagent/knowpro/interfaces_search.py Removed duplicate all definition
src/typeagent/emails/email_import.py Fixed separator prepending in chunk merging
src/typeagent/emails/email_memory.py Updated to use new chat model creation
src/typeagent/mcp/server.py Added coverage import guard and match statement default case
src/typeagent/storage/sqlite/provider.py Removed embedding_size consistency checks, now checks size consistency between indexes
src/typeagent/storage/sqlite/messageindex.py Added empty chunks guard
src/typeagent/transcripts/transcript.py Fixed f-string formatting and reads embedding size from file metadata
src/typeagent/podcasts/podcast.py Same f-string and metadata reading fixes as transcripts
tools/query.py Updated imports to use new model adapters
tools/ingest_vtt.py Updated to use new embedding model creation
tests/test_*.py Comprehensive test updates to use new interfaces and added extensive new test coverage
AGENTS.md Added deprecation guideline
Comments suppressed due to low confidence (1)

src/typeagent/knowpro/searchlang.py:626

  • The duplicate method add_search_term_to_groupadd_entity_name_to_group with the typo in its name has been removed. This method name appears to be a copy-paste error combining two method names, and its implementation is identical to add_entity_name_to_group above it.
    def add_property_term_to_group(
        self,
        property_name: str,
        property_value: str,
        term_group: SearchTermGroup,
        exact_match_value=False,
    ) -> None:
        if not self.is_searchable_string(property_name):
            return
        if not self.is_searchable_string(property_value):
            return
        if self.is_noise_term(property_value):
            return
        # Dedupe any terms already added to the group earlier.
        if not self.dedupe or not self.entity_terms_added.has(

Comment on lines +118 to +126
async def get_embedding_nocache(self, input: str) -> NormalizedEmbedding:
result = await self._embedder.embed_documents([input])
embedding: NDArray[np.float32] = np.array(
result.embeddings[0], dtype=np.float32
)
norm = float(np.linalg.norm(embedding))
if norm > 0:
embedding = (embedding / norm).astype(np.float32)
return embedding
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_embedding_nocache method doesn't validate that the input string is non-empty before calling the embedding API. According to the test test_get_embedding_nocache_empty_input, empty strings should raise a ValueError with "Empty input text". Consider adding validation: if not input: raise ValueError("Empty input text").

Copilot uses AI. Check for mistakes.
if not azure_endpoint:
raise RuntimeError(f"Environment variable {endpoint_envvar} not found")

m = re.search(r"[?,]api-version=([\d-]+(?:preview)?)", azure_endpoint)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the comma here -- I just had the URL format wrong.

"""api-version preceded by comma (alternate separator)."""
monkeypatch.setenv(
"TEST_ENDPOINT",
"https://myhost.openai.azure.com/openai/deployments/gpt-4?foo=bar,api-version=2024-06-01",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't even a valid URL.

Comment on lines +270 to 278
@classmethod
def with_term_matches(
cls, source: "SemanticRefAccumulator"
) -> "SemanticRefAccumulator":
"""Create a new accumulator inheriting a copy of *source*'s term-match provenance."""
acc = cls()
acc.search_term_matches = set(source.search_term_matches)
return acc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd write this as a clone() method on an instance.

def clone(self):
    acc = self.__class__()
    acc.search_term_matches = set(self.search_term_matches))
    return acc

@@ -516,12 +537,13 @@ def add_ranges(self, text_ranges: "list[TextRange] | TextRangeCollection") -> No
def is_in_range(self, inner_range: TextRange) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that is_in_range is a terrible name. It should be contains_range or maybe __contains__.


class SemanticRefAccumulator(MatchAccumulator[SemanticRefOrdinal]):
def __init__(self, search_term_matches: set[str] = set()):
"""Accumulates scored semantic reference matches.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will take me a bit more time to review this particular commit more carefully. It seems you're on to something, but I think I don't recall how it originally worked or how it's supposed to work.

Comment on lines +372 to +373
if use_or_max and action_group.terms:
term_group.terms.append(action_group)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition wasn't mentioned in the commit description.

return None
end_timestamp = (
(await messages.get_item(range.end.message_ordinal)).timestamp
(await messages.get_item(range.end.message_ordinal - 1)).timestamp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything here is supposed to be half-open intervals, and it really should be the (start) timestamp of the message following the range. But this can raise IndexError if the range ends at the end of the messages array. Maybe we could check for that edge case and use either None or the timestamp of the final message plus 1 second or something like that. But I think this is just wrong.

@@ -0,0 +1,167 @@
# Copyright (c) Microsoft Corporation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this file's name doesn't have to end in '_unit'.

if range_start_ordinal >= 0:
# We have a range, so break.
break
ordinal += 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use enumerate() in the for loop.

Comment on lines -107 to -108
range_start_ordinal = message.ordinal
range_end_ordinal = message.ordinal
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. I don't think messages ever have an ordinal field... Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants