Skip to content

feat(query): add temporal_relation and evidence_request plan steps #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sidra-009 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(query): add temporal_relation and evidence_request plan steps #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98c6069
feat(query): add temporal_relation and evidence_request plan steps
Sidra-009 Sep 5, 2026
4556772
Merge branch 'main' into feature/llm-query-planning
tulayha Sep 14, 2026
60b4eba
fix(query): reject unsupported occurrence modes and validate temporal…
Sidra-009 Sep 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion src/vidxp/application_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,18 +1127,48 @@ def _unique_modalities(
return values


class OccurrenceMode(StrEnum):
first = "first"
best = "best"
all = "all"


class SearchMomentsPlanStep(ApplicationModel):
kind: Literal["search_moments"] = "search_moments"
modality: Identifier
query: SearchQuery
occurrence_mode: OccurrenceMode = OccurrenceMode.best

@tulayha tulayha Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

first and all pass plan validation, but execution ignores this field and performs the usual ranked search. Please implement the selected mode or reject unsupported modes so the existing fallback is used. Add coverage for this behavior.

@Sidra-009 Sidra-009 Sep 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed first/all now reject at plan validation and fall back to the existing ranked search, best stays supported. added test coverage for both the rejection and fallback paths..........



class ActorOverviewPlanStep(ApplicationModel):
kind: Literal["actor_overview"] = "actor_overview"


class TemporalRelation(StrEnum):
before = "before"
after = "after"
during = "during"


class TemporalRelationPlanStep(ApplicationModel):
kind: Literal["temporal_relation"] = "temporal_relation"
relation: TemporalRelation
reference_query: SearchQuery
target_modality: Identifier
target_query: SearchQuery


class EvidenceRequestPlanStep(ApplicationModel):
kind: Literal["evidence_request"] = "evidence_request"
delivery_mode: EvidenceDeliveryMode
include_board: bool = False


QueryPlanStep = Annotated[
SearchMomentsPlanStep | ActorOverviewPlanStep,
SearchMomentsPlanStep
| ActorOverviewPlanStep
| TemporalRelationPlanStep
| EvidenceRequestPlanStep,
Field(discriminator="kind"),
]

Expand All @@ -1151,6 +1181,8 @@ class QueryPlanningRequest(ApplicationModel):
question: SearchQuery
allowed_modalities: tuple[Identifier, ...]
actor_overview_allowed: bool = False
temporal_relations_allowed: bool = False
evidence_requests_allowed: bool = False


class QueryModelIdentity(ApplicationModel):
Expand Down
38 changes: 37 additions & 1 deletion src/vidxp/query_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@

from vidxp.application_models import (
ActorEvidence,
ActorOverviewPlanStep,
DraftAnswer,
Evidence,
EvidenceRequestPlanStep,
FusedSearchResult,
GroundedClaim,
IndexSnapshotReference,
Expand All @@ -18,7 +20,7 @@
QuerySynthesisRequest,
QueryVideoCommand,
SearchMomentsPlanStep,
ActorOverviewPlanStep,
TemporalRelationPlanStep,
)
from vidxp.capabilities.actor.schemas import ActorClusterSummary
from vidxp.ports import QueryModelPort, QueryProviderError
Expand All @@ -39,6 +41,8 @@ def _default_plan(
*,
search_modalities: tuple[str, ...],
actor_overview: bool,
temporal_relations: bool = False,
evidence_requests: bool = False,
) -> QueryPlan:
steps = [
SearchMomentsPlanStep(
Expand All @@ -57,6 +61,8 @@ def _valid_plan(
*,
search_modalities: tuple[str, ...],
actor_overview: bool,
temporal_relations: bool = False,
evidence_requests: bool = False,
) -> bool:
searches = [
step.modality
Expand All @@ -66,6 +72,28 @@ def _valid_plan(
actor_steps = sum(
isinstance(step, ActorOverviewPlanStep) for step in plan.steps
)
temporal_steps = sum(
isinstance(step, TemporalRelationPlanStep) for step in plan.steps
)
evidence_steps = sum(
isinstance(step, EvidenceRequestPlanStep) for step in plan.steps
)
if any(
step.occurrence_mode.value != "best"
for step in plan.steps
if isinstance(step, SearchMomentsPlanStep)
):
return False
if not temporal_relations and temporal_steps > 0:

@tulayha tulayha Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This checks whether temporal steps are enabled, but does not validate their target modality. A plan with target_modality="nonexistent" passes when temporal planning is enabled. Please check it against search_modalities and cover rejection and fallback.

@Sidra-009 Sidra-009 Sep 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed......... target_modality on TemporalRelationPlanStep now gets checked against search_modalities, unknown modality gets rejected using the same fallback pattern as the rest of the file. added tests for rejection + fallback here too

return False
if not evidence_requests and evidence_steps > 0:
return False
if any(
step.target_modality not in search_modalities
for step in plan.steps
if isinstance(step, TemporalRelationPlanStep)
):
return False
return (
len(searches) == len(set(searches))
and set(searches) == set(search_modalities)
Expand All @@ -85,11 +113,15 @@ def plan(
*,
search_modalities: tuple[str, ...],
actor_overview: bool,
temporal_relations: bool = False,
evidence_requests: bool = False,
) -> tuple[QueryPlan, str | None]:
fallback = _default_plan(
command,
search_modalities=search_modalities,
actor_overview=actor_overview,
temporal_relations=temporal_relations,
evidence_requests=evidence_requests,
)
if self.model is None:
return fallback, "query_model_not_configured"
Expand All @@ -99,6 +131,8 @@ def plan(
question=command.question,
allowed_modalities=search_modalities,
actor_overview_allowed=actor_overview,
temporal_relations_allowed=temporal_relations,
evidence_requests_allowed=evidence_requests,
)
)
except QueryProviderError:
Expand All @@ -107,6 +141,8 @@ def plan(
proposed,
search_modalities=search_modalities,
actor_overview=actor_overview,
temporal_relations=temporal_relations,
evidence_requests=evidence_requests,
):
return fallback, "query_plan_rejected"
return proposed, None
Expand Down
77 changes: 77 additions & 0 deletions tests/test_query_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
SearchHit,
SearchMomentsPlanStep,
SearchResult,
TemporalRelationPlanStep,
)
from vidxp.ports import QueryProviderError
from vidxp.query_service import GroundedQueryService
Expand Down Expand Up @@ -121,6 +122,82 @@ def test_invalid_model_plan_falls_back_to_complete_closed_plan(self):
["scene", "speech"],
)

def test_best_occurrence_mode_is_supported(self):
model = FakeQueryModel(
QueryPlan(
steps=(
SearchMomentsPlanStep(
modality="scene",
query="taxi",
occurrence_mode="best",
),
)
)
)
service = GroundedQueryService(model)

plan, reason = service.plan(
self.command,
search_modalities=("scene",),
actor_overview=False,
)

self.assertIsNone(reason)
self.assertEqual(plan.steps[0].occurrence_mode.value, "best")

def test_unsupported_occurrence_mode_falls_back_to_ranked_search(self):
for occurrence_mode in ("first", "all"):
model = FakeQueryModel(
QueryPlan(
steps=(
SearchMomentsPlanStep(
modality="scene",
query="taxi",
occurrence_mode=occurrence_mode,
),
)
)
)
service = GroundedQueryService(model)

plan, reason = service.plan(
self.command,
search_modalities=("scene",),
actor_overview=False,
)

self.assertEqual(reason, "query_plan_rejected")
self.assertEqual(plan.steps[0].occurrence_mode.value, "best")

def test_unknown_temporal_target_modality_falls_back(self):
model = FakeQueryModel(
QueryPlan(
steps=(
SearchMomentsPlanStep(
modality="scene",
query="taxi",
),
TemporalRelationPlanStep(
relation="before",
reference_query="taxi",
target_modality="nonexistent",
target_query="car",
),
)
)
)
service = GroundedQueryService(model)

plan, reason = service.plan(
self.command,
search_modalities=("scene",),
actor_overview=False,
temporal_relations=True,
)

self.assertEqual(reason, "query_plan_rejected")
self.assertEqual([step.modality for step in plan.steps], ["scene"])
Comment on lines +172 to +199

@greptile-apps greptile-apps Bot Oct 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 New plan paths lack tests

The new tests never accept a valid temporal or evidence plan, or check that either step is rejected without its opt-in flag. FakeQueryModel.plan also ignores the request, so broken flag forwarding would go unnoticed. Add tests for both step types with their flags enabled and disabled, and assert the flags received by the model.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/test_query_service.py
Line: 172-199

Comment:
**New plan paths lack tests**

The new tests never accept a valid temporal or evidence plan, or check that either step is rejected without its opt-in flag. `FakeQueryModel.plan` also ignores the request, so broken flag forwarding would go unnoticed. Add tests for both step types with their flags enabled and disabled, and assert the flags received by the model.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex


def test_provider_failure_uses_deterministic_retrieval_plan(self):
service = GroundedQueryService(
FakeQueryModel(QueryProviderError("offline"))
Expand Down
48 changes: 24 additions & 24 deletions uv.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.