Skip to content

fix(providers): scope restored chains to subtask - #420

Open
SherryOvo wants to merge 1 commit into
vxcontrol:mainfrom
SherryOvo:fix/msgchain-subtask-isolation
Open

SherryOvo wants to merge 1 commit into
vxcontrol:mainfrom
SherryOvo:fix/msgchain-subtask-isolation

Conversation

@SherryOvo

Copy link
Copy Markdown

Summary

  • add a flow/task/subtask/type-scoped lookup for message-chain restoration
  • make restoreChain use the scoped lookup so it cannot inherit a same-type chain from another subtask
  • keep the existing task-level lookup unchanged for generator/refiner and task summary callers
  • regenerate the sqlc output with the project-documented sqlc v1.27.0 and add lookup parameter tests

Problem

restoreChain currently selects the latest chain by flow, task, and type. When multiple subtasks use the same specialist type, restoring one subtask can therefore load another subtask’s chain and append the new prompt to unrelated tool history. Besides mixing execution context, that history can grow increasingly expensive to normalize and summarize.

The new query uses PostgreSQL IS NOT DISTINCT FROM for both nullable identifiers, so assistant (NULL/NULL), task-only, and subtask-scoped chains each match their exact scope.

Validation

  • go test ./pkg/providers ./pkg/database/... -count=1
  • go build ./cmd/pentagi
  • git diff --check

Copilot AI lite review requested due to automatic review settings September 15, 2026 06:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Add a focused restoration test covering same-type subtask chains and NULL scopes.

Pull request overview

This PR scopes restored message-chain lookups to the exact flow, task, subtask, and type.

Changes:

  • Added a nullable-safe scoped lookup.
  • Updated restoreChain to use it.
  • Regenerated sqlc code and added parameter tests.
File summaries
File Summary
backend/sqlc/models/msgchains.sql Adds the scoped lookup query.
backend/pkg/providers/helpers.go Uses scoped restoration parameters.
backend/pkg/providers/helpers_test.go Tests scope parameter mapping.
backend/pkg/database/querier.go Exposes the generated query method.
backend/pkg/database/msgchains.sql.go Implements the generated query.
Review details

Files not reviewed (2)

  • backend/pkg/database/msgchains.sql.go: Generated file
  • backend/pkg/database/querier.go: Generated file

Suppressed comments (1)

backend/pkg/providers/helpers.go:472

  • The added test only verifies conversion to sql.NullInt64; it would still pass if restoreChain called the old task/type lookup or if the SQL predicate returned another subtask's row. Please add a focused restore/database test with two same-type chains in one task and assert that restoring each subtask selects its own chain (including the NULL/NULL scope).
	msgChain, err := fp.db.GetFlowTaskSubtaskTypeLastMsgChain(ctx, scopedMsgChainLookupParams(
		fp.flowID, taskID, subtaskID, msgChainType,
	))
  • Files reviewed: 3/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

2 participants