Feature/con 118 campaign assistant assets - #69
Merged
Conversation
CON-112: Campaign Assistant — per-campaign chat over content_plan + enrich_brief
Post.UsedAssetIDs was set from the model's self-reported AssetRefs, which can hallucinate or omit ids. Bind each generated post to the assets actually retrieved into the generation context instead (both the full-plan and targeted paths), and surface that set as ContentPlanResponse.UsedAssets (id+title) so the campaign assistant can report provenance. AssetRefs stays in the model output schema (it still nudges the prose to cite sources) but no longer drives persistence. Add assetIDsOf/assetRefsOf helpers (dedupe, order-preserving, skip empty) with unit tests; the persisted-binding assertion is covered by the integration suite.
runContentPlan and generatePosts now read ContentPlanResponse.UsedAssets, emit a new assets_used SSE event when non-empty, carry the assets on their tool result and request-state result, and the prompt instructs the planner to name the used assets by title in its explanation (and stay silent when none were used).
New read tool that answers questions grounded in a campaign's attached assets: resolve the campaign's ready assets (AssetIDs, or all ready, excluding failed/partial; UseAssets is not required for an explicit Q&A), embed the query, and search their chunks via pgvector, returning the top excerpts (title + page) for the planner to answer from. Degrades cleanly (available:false / a note) when the embedder is unavailable, no assets are ready, or nothing matches — it never fails the turn. Wire Embedder + Assets/Chunks repos through the flow config and server, register the tool, and document it in the prompt. Unit-test the page-citation helper.
Add campaign_assistant integration specs (require ANTHROPIC_API_KEY): - asset-grounded content plan: seed a ready asset, attach it to the campaign, generate a plan, then assert every persisted post's UsedAssetIDs is bound to the retrieved asset and an assets_used event names it — validating the grounded binding + provenance via content_plan's creation-order fallback (no embedder needed). - asset-question degradation: with no embedder wired, an asset question still completes the turn cleanly (askCampaignAssets degrades to unavailable). Wire the asset + chunk repos into the assistant harness and set MaxContextAssets so the creation-order fallback includes the attached asset.
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughCampaign Assistant now answers questions from attached campaign assets, reports assets used in generated content, persists grounded asset IDs, emits asset usage events, and wires asset repositories and embedding support through the server runtime. ChangesCampaign asset grounding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CampaignAssistant
participant askCampaignAssets
participant Embedder
participant AssetChunksRepository
User->>CampaignAssistant: ask question about attached assets
CampaignAssistant->>askCampaignAssets: invoke read-only asset search
askCampaignAssets->>Embedder: embed query
askCampaignAssets->>AssetChunksRepository: search ready asset chunks
AssetChunksRepository-->>askCampaignAssets: return matching excerpts
askCampaignAssets-->>CampaignAssistant: return excerpts with page citations
CampaignAssistant-->>User: provide grounded explanation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/integration/campaign_assistant_test.go (1)
444-454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
askCampaignAssetsactually runs.This passes if the model answers directly or hallucinates. Capture
SSEEventToolCalland require the tool name to beaskCampaignAssets.Proposed test
It("handles an asset question without failing the turn", func() { + var called bool + onEvent := campaign_assistant.OnEventFunc(func(name campaign_assistant.SSEEventKind, data any) { + if name == campaign_assistant.SSEEventToolCall { + if p, ok := data.(campaign_assistant.ToolCallEventPayload); ok { + called = called || p.Name == "askCampaignAssets" + } + } + }) resp, err := callback(ctx, campaign_assistant.CampaignAssistantRequest{ CampaignID: campaignID, Instruction: "What do the attached assets say about concurrency benchmarks?", - }, nil) + }, onEvent) Expect(err).NotTo(HaveOccurred()) Expect(resp).NotTo(BeNil()) Expect(resp.Explanation).NotTo(BeEmpty()) + Expect(called).To(BeTrue()) })🤖 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 `@src/integration/campaign_assistant_test.go` around lines 444 - 454, Strengthen the asset-question test around callback by capturing the emitted SSEEventToolCall event and asserting that its tool name is askCampaignAssets. Keep the existing successful-response assertions, but ensure the test fails when the model answers directly without invoking the asset tool.
🤖 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 `@src/genkit/flows/campaign_assistant/tools.go`:
- Around line 584-615: Update readyCampaignAssetIDs to resolve only assets
attached to the campaign and whose status is explicitly ready. When AssetIDs is
empty, do not call assets.List or include tenant-wide assets; use the campaign’s
attached asset relationship and filter by models.AssetStatusReady, while
preserving exclusion of failed, partial, pending, and unknown statuses.
---
Nitpick comments:
In `@src/integration/campaign_assistant_test.go`:
- Around line 444-454: Strengthen the asset-question test around callback by
capturing the emitted SSEEventToolCall event and asserting that its tool name is
askCampaignAssets. Keep the existing successful-response assertions, but ensure
the test fails when the model answers directly without invoking the asset tool.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d86ff836-ce4e-43e1-8766-9e807189a136
📒 Files selected for processing (14)
src/genkit/flows/campaign_assistant/prompts/campaign_assistant.tmplsrc/genkit/flows/campaign_assistant/run.gosrc/genkit/flows/campaign_assistant/tools.gosrc/genkit/flows/campaign_assistant/tools_test.gosrc/genkit/flows/campaign_assistant/types.gosrc/genkit/flows/content_plan/assets.gosrc/genkit/flows/content_plan/assets_test.gosrc/genkit/flows/content_plan/flow.gosrc/genkit/flows/content_plan/generate.gosrc/genkit/flows/content_plan/types.gosrc/integration/campaign_assistant_test.gosrc/server/campaign_assistant.gosrc/server/genkit_runtime.gosrc/server/server.go
The core capability: the assistant now generates content from the campaign's attached assets even when UseAssets is off. Before runContentPlan / generatePosts, if the campaign has ready attached assets, enable UseAssets and persist it. content_plan already injects attached assets into the generation prompt whenever UseAssets is true, so flipping (and persisting) the flag is all that's needed — no content_plan change. Best-effort: a persist failure falls back to asset-free generation. This reverses the earlier "respect campaign settings only" scoping call, which left asset-sourced generation un-triggerable from the chat. Integration spec updated: attach an asset with UseAssets off, generate, and assert the assistant enables + persists UseAssets and grounds the posts' UsedAssetIDs on the retrieved asset.
Each post created via campaign_assistant -> content_plan recorded an identical UsedAssetIDs containing every attached asset, since persistOne stamped all posts with the run-wide grounded set. Bind each post instead to the model's self-reported AssetRefs, filtered to the ids actually retrieved into context so a hallucinated id never persists. A post that cited nothing now records an empty list. The plan-level UsedAssets (assets_used SSE event / tool output) stays the retrieved superset — "assets offered to the model for this plan".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/genkit/flows/content_plan/assets.go (1)
148-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix unintended asset truncation.
If
cfg.MaxContextAssetsis uninitialized (0), this truncatescandidateIDsto empty, excluding all fallback assets. Bypass the check if 0 means unlimited, or set a default.🐛 Proposed fix
- if len(candidateIDs) > cfg.MaxContextAssets { + if cfg.MaxContextAssets > 0 && len(candidateIDs) > cfg.MaxContextAssets {🤖 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 `@src/genkit/flows/content_plan/assets.go` around lines 148 - 151, Update the asset truncation logic around candidateIDs and cfg.MaxContextAssets so a zero MaxContextAssets value means unlimited and does not slice candidateIDs to empty. Only append the exclusion warning and truncate when the configured limit is positive and candidateIDs exceeds it.
🧹 Nitpick comments (1)
src/genkit/flows/content_plan/assets.go (1)
46-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove premature abstraction.
As per path instructions, avoid premature abstractions; three similar lines is better than a speculative helper. Inline this map construction at the call site.
🤖 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 `@src/genkit/flows/content_plan/assets.go` around lines 46 - 54, Remove the idSet helper and inline its map construction at the call site that builds the retrieved-context grounding set. Preserve the existing capacity and ID-to-empty-struct entries, and update the caller to use the inline map directly.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/genkit/flows/content_plan/assets.go`:
- Around line 148-151: Update the asset truncation logic around candidateIDs and
cfg.MaxContextAssets so a zero MaxContextAssets value means unlimited and does
not slice candidateIDs to empty. Only append the exclusion warning and truncate
when the configured limit is positive and candidateIDs exceeds it.
---
Nitpick comments:
In `@src/genkit/flows/content_plan/assets.go`:
- Around line 46-54: Remove the idSet helper and inline its map construction at
the call site that builds the retrieved-context grounding set. Preserve the
existing capacity and ID-to-empty-struct entries, and update the caller to use
the inline map directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 380ac12c-f7de-479e-9b0e-aad95bfede65
📒 Files selected for processing (6)
src/genkit/flows/campaign_assistant/tools.gosrc/genkit/flows/content_plan/assets.gosrc/genkit/flows/content_plan/assets_test.gosrc/genkit/flows/content_plan/generate.gosrc/genkit/flows/content_plan/types.gosrc/integration/campaign_assistant_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- src/genkit/flows/content_plan/generate.go
- src/genkit/flows/content_plan/types.go
- src/integration/campaign_assistant_test.go
- src/genkit/flows/campaign_assistant/tools.go
4 tasks
Merged
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.
Summary
Lets the Campaign Assistant use a campaign's attached assets (PDFs, markdown, etc.) as a source for content generation, records which assets informed each post, and answers questions grounded in them (sub-issue of CON-112).
content_planalready injects each asset's excerpt into the generation prompt whenUseAssetsis on; the assistant now enables and persistsUseAssetswhenever the campaign has attached assets, so asset-sourced generation is triggerable straight from the chat (previously it only happened ifUseAssetshad been set elsewhere).Post.UsedAssetIDsis set from the assets actually retrieved into context, not the model's self-reported (hallucination-prone)AssetRefs.runContentPlan/generatePostsreport which assets informed the posts via a newassets_usedSSE event and a line in the reply.askCampaignAssetsanswers questions grounded in the attached assets via pgvector chunk search.What changed
campaign_assistantensureCampaignAssetUse: before generating, if the campaign has ready attached assets andUseAssetsis off, turn it on and persist it (best-effort; falls back to asset-free generation on failure).assets_usedSSE event;UsedAssetson tool results/outputs; prompt names used assets by title.askCampaignAssetstool (resolve ready assets → embed query → chunk search → titled/paged excerpts), degrading cleanly (never fails the turn).FlowConfiggainsEmbedder;CampaignAssistantReposgainsAssets+Chunks; wired throughserver/.content_planUsedAssetIDsgrounded on the retrieved asset set (assetIDsOf);AssetRefsno longer persisted (kept as a prose-citation hint).ContentPlanResponse.UsedAssets(id + title) surfaced from the retrieved pieces.Decisions
UseAssetson first use — the assistant drives asset use from the chat (revises the earlier "respect campaign settings only" call).UseAssets— an explicit request to consult the assets.Testing
go build ./...,go vet, unit tests (assetIDsOf/assetRefsOf/pageRef) — green.src/integration/campaign_assistant_tAPI_KEY): attach an asset withUseAssetsoff →generate → assert the assistant enabled + persistedUseAssets, every post'sUsedAssetIDsis bound to the retrieved asset, andassets_usednames it; plus an asset-question turn embedder.askCampaignAssetsRAG answer path (needs a Gemini embedder) — only its degradation is covered.Commits
CON-118: ground content-plan asset→post binding on retrieved assetsCON-118: add askCampaignAssets Q&A tool over attached assetsCON-118: auto-generate content from attached campaign assetsCloses CON-118.
Summary by CodeRabbit
Summary by CodeRabbit
assets_usedstreaming event.