CON-112: Campaign Assistant — per-campaign chat over content_plan + enrich_brief - #68
Merged
Conversation
…nrich_brief Adds a conversational, tenant-scoped Campaign Assistant that mirrors the Post Assistant architecture (shared-service + tool + REST + SSE). A cheap planning model (new RolePlanning / PLANNING_MODEL_ID, Haiku) drives intent-routing while the existing content_plan and enrich_brief flows are invoked as tools for the heavy prose. enrichBrief auto-applies the four brief fields to the campaign. - migration + TenantScoped model + repository for campaign_assistant_messages - genkit/flows/campaign_assistant: types, scanner, context, flow, tools, run, prompt - llm.RolePlanning + config.PlanningModelID; NewProvider gains a planning model - server wiring (genkit_runtime + initCampaignAssistant) behind the Anthropic gate - POST /api/campaigns/:id/assistant (SSE) + GET /api/campaigns/:id/messages, tenant-wide (no owner guard), diverging intentionally from the owner-only standalone generate-draft/enrich-brief endpoints - planner usage recorded under campaign_assistant; sub-flows keep their own - updated NewCampaignsHandler call sites across tests
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a tenant-scoped Campaign Assistant with streamed Genkit interactions, targeted post generation, consistency reviews, campaign overview data, conversation persistence, HTTP endpoints, model routing, diagnostics, and deterministic campaign date redistribution. ChangesCampaign Assistant
Prototype cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 2
🧹 Nitpick comments (4)
src/vendors/llm/llm_test.go (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuality and planning use the same model string — arg-order swap bugs would go undetected.
Use a distinct literal for the planning model so a constructor arg-order regression (quality ↔ planning) actually fails the test.
🧪 Proposed fix
- p := llm.NewProvider("claude-sonnet-4-5-20250929", "claude-haiku-4-5-20251001", "claude-haiku-4-5-20251001") + p := llm.NewProvider("claude-sonnet-4-5-20250929", "claude-haiku-4-5-20251001", "claude-haiku-3-5-20241022") if got := p.Ref(llm.RoleGeneration); got != "anthropic/claude-sonnet-4-5-20250929" { t.Errorf("generation Ref = %q", got) } if got := p.Model(llm.RoleQuality); got != "claude-haiku-4-5-20251001" { t.Errorf("quality Model = %q", got) } - if got := p.Ref(llm.RolePlanning); got != "anthropic/claude-haiku-4-5-20251001" { + if got := p.Ref(llm.RolePlanning); got != "anthropic/claude-haiku-3-5-20241022" { t.Errorf("planning Ref = %q", got) }🤖 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/vendors/llm/llm_test.go` around lines 99 - 109, Update TestProviderRef to pass a distinct model literal for the planning argument in llm.NewProvider, then update the RolePlanning expectation to that distinct value while keeping the quality model assertion unchanged. This must make quality/planning constructor argument swaps fail the test.src/genkit/flows/campaign_assistant/scanner.go (1)
253-263: 🎯 Functional Correctness | 🔵 TrivialNested-value skip doesn't track string state — a brace inside a nested string can end the skip early.
stInNestedcounts{/[/}/]unconditionally. A quoted string inside a skipped nested value containing a literal}/](e.g.{"note": "a } inside"}) decrementsnestedDepthprematurely, exiting the skip mid-value and letting the trailing text be mis-parsed as top-level content.Not triggered by the current envelope (no nested keys), but the type's docstring claims general tolerance for arbitrary/malformed model output, and this is the shared parser other future response shapes might rely on.
♻️ Sketch: track quote state while skipping nested structures
case stInNested: + if s.nestedInString { + if s.esc == escBackslash { + s.esc = escNone + } else if c == '\\' { + s.esc = escBackslash + } else if c == '"' { + s.nestedInString = false + } + return + } switch c { + case '"': + s.nestedInString = true case '{', '[': s.nestedDepth++ case '}', ']':🤖 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/campaign_assistant/scanner.go` around lines 253 - 263, Update the stInNested handling in the scanner state machine to track whether it is inside a quoted string, ignoring braces and brackets while quoted and respecting escaped quotes. Only adjust nestedDepth and return to stTop when structural delimiters are encountered outside strings, preserving existing resetKey behavior.src/genkit/flows/campaign_assistant/tools.go (1)
157-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid manual struct literal when underlying types match.
EnrichBriefInputandEnrichBriefStartedEventPayloadshare identical fields. Cast directly.♻️ Proposed refactor
- emit(st.onEvent, SSEEventEnrichBriefStarted, EnrichBriefStartedEventPayload{Instruction: in.Instruction}) + emit(st.onEvent, SSEEventEnrichBriefStarted, EnrichBriefStartedEventPayload(in))🤖 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/campaign_assistant/tools.go` at line 157, Update the emit call in the EnrichBrief flow to cast the existing EnrichBriefInput value directly to EnrichBriefStartedEventPayload instead of constructing a manual struct literal, preserving the current event payload contents.Source: Linters/SAST tools
src/handlers/campaigns_test.go (1)
792-975: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the new
POST /:id/assistantandGET /:id/messagesendpoints.Every other campaign endpoint (including
EnrichBriefright above) has a dedicatedDescribeblock covering the 503-unwired, 404-unknown-campaign, success-stream, and error-stream paths. The new Assistant/ListMessages endpoints have none in this file — thebuildBriefApp/parseSSEhelpers here are directly reusable as a template.🤖 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/handlers/campaigns_test.go` around lines 792 - 975, Add dedicated test coverage in campaigns_test.go for POST /:id/assistant and GET /:id/messages, following the existing EnrichBrief Describe block and reusing buildBriefApp, seedCookie, createCampaignOn, and parseSSE where applicable. Cover unwired 503 behavior, unknown-campaign 404 responses, successful assistant/message streaming or responses, and error-stream handling consistent with each endpoint’s contract.
🤖 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/run.go`:
- Around line 237-260: Update the conversation persistence flow around
persistTurn so a history-write failure is logged and does not abort the
assistant turn or suppress subsequent completion events. Within persistTurn,
wrap both repos.Messages.Create calls in a single transaction so the user and
model messages commit or roll back together.
In `@src/handlers/campaigns.go`:
- Around line 584-593: Normalize the msgs result in
CampaignsHandler.ListMessages before calling c.JSON so a nil or empty
ListRecentByCampaignID result is returned as an empty
[]models.CampaignAssistantMessage array, preserving the endpoint’s array
response contract.
---
Nitpick comments:
In `@src/genkit/flows/campaign_assistant/scanner.go`:
- Around line 253-263: Update the stInNested handling in the scanner state
machine to track whether it is inside a quoted string, ignoring braces and
brackets while quoted and respecting escaped quotes. Only adjust nestedDepth and
return to stTop when structural delimiters are encountered outside strings,
preserving existing resetKey behavior.
In `@src/genkit/flows/campaign_assistant/tools.go`:
- Line 157: Update the emit call in the EnrichBrief flow to cast the existing
EnrichBriefInput value directly to EnrichBriefStartedEventPayload instead of
constructing a manual struct literal, preserving the current event payload
contents.
In `@src/handlers/campaigns_test.go`:
- Around line 792-975: Add dedicated test coverage in campaigns_test.go for POST
/:id/assistant and GET /:id/messages, following the existing EnrichBrief
Describe block and reusing buildBriefApp, seedCookie, createCampaignOn, and
parseSSE where applicable. Cover unwired 503 behavior, unknown-campaign 404
responses, successful assistant/message streaming or responses, and error-stream
handling consistent with each endpoint’s contract.
In `@src/vendors/llm/llm_test.go`:
- Around line 99-109: Update TestProviderRef to pass a distinct model literal
for the planning argument in llm.NewProvider, then update the RolePlanning
expectation to that distinct value while keeping the quality model assertion
unchanged. This must make quality/planning constructor argument swaps fail the
test.
🪄 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: 7ffa07ab-09bb-4aa9-ae6b-0b2ab849d7ad
📒 Files selected for processing (28)
src/config/config.gosrc/database/migrations/20260714000001_campaign_assistant_messages.down.sqlsrc/database/migrations/20260714000001_campaign_assistant_messages.up.sqlsrc/genkit/flows/campaign_assistant/context.gosrc/genkit/flows/campaign_assistant/flow.gosrc/genkit/flows/campaign_assistant/prompts/campaign_assistant.tmplsrc/genkit/flows/campaign_assistant/run.gosrc/genkit/flows/campaign_assistant/scanner.gosrc/genkit/flows/campaign_assistant/tools.gosrc/genkit/flows/campaign_assistant/types.gosrc/handlers/analytics_test.gosrc/handlers/campaigns.gosrc/handlers/campaigns_test.gosrc/handlers/multi_tenant_test.gosrc/handlers/post_attachments_test.gosrc/handlers/posts_test.gosrc/integration/post_analytics_test.gosrc/integration/post_attachments_test.gosrc/integration/post_clone_test.gosrc/integration/post_restore_test.gosrc/integration/post_schedule_test.gosrc/models/campaign_assistant_message.gosrc/repository/campaign_assistant_messages.gosrc/server/campaign_assistant.gosrc/server/genkit_runtime.gosrc/server/server.gosrc/vendors/llm/llm_test.gosrc/vendors/llm/provider.go
- repository/campaign_assistant_messages_test.go: ordering, limit, tenant isolation - handlers/campaigns_test.go: assistant SSE (401/503/400/deltas+complete/tool-forward/ error 502) and messages (401/empty/chronological), 9 specs - integration/campaign_assistant_test.go: real-flow suite (integration tag) wiring all three flows — Q&A (no mutation), enrich (persisted), content plan (persisted), history - http-client/campaigns/campaigns.http: campaign assistant + messages requests
- run.go: a persistTurn failure is now logged and no longer aborts the turn or suppresses the completion events — tool side effects (posts/brief) have already committed, so the turn is still reported successful. - persistTurn writes the user + model messages via a new CampaignAssistantMessageRepository.CreateBatch, which runs both inserts in one transaction (all-or-nothing) so a turn never persists half-written.
ListRecentByCampaignID scans into a nil slice on an empty result set, and c.JSON(nil) serializes to null, breaking the array response contract. Normalize to an empty slice before encoding, matching the messageRepo==nil branch. Tighten the empty-history test to assert the raw body is [] (decoding null into a slice also yields empty, so the old assertion was blind to this).
Mirror the campaigns fix: ListRecentByPostID scans into a nil slice on an empty result set, and c.JSON(nil) serializes to null, breaking the array contract. Normalize to an empty slice before encoding.
New campaignoverview shared service (pure buildOverview aggregation + repo-backed Overview) reused by two surfaces: - getCampaignOverview assistant read tool (campaign_assistant flow) — phases + per-phase post counts + distribution by status/platform/content-type - GET /api/campaigns/:id/overview REST endpoint (JSON; 404 on not-found), wired via CampaignsHandler.SetOverviewService (setter, no constructor churn) Phases are also added to the assistant's always-on context block (free — already hydrated on the campaign), so phase questions need no tool round-trip; the per-turn posts query for distribution stays on-demand via the tool/endpoint. Tests: pure buildOverview unit tests (buckets, unassigned/none, reconciling totals, empty/no-type); handler endpoint specs (200 shape / 401 / 404 / 503); integration spec for the tool. http-client entry added.
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/campaign_assistant/scanner_test.go (1)
1-415: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the scanner test copies identical. This file matches
enrich_briefandpost_assistant, but it drifts fromcontent_plan; update all four copies together, with only thepackageline allowed to differ.🤖 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/campaign_assistant/scanner_test.go` around lines 1 - 415, Synchronize the complete scanner test contents across the campaign_assistant, enrich_brief, post_assistant, and content_plan copies so they are identical, allowing only each package declaration to differ. Apply any future test changes consistently to all four files and verify no other differences remain.Source: Learnings
🤖 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/campaign_assistant/scanner_test.go`:
- Around line 1-415: Synchronize the complete scanner test contents across the
campaign_assistant, enrich_brief, post_assistant, and content_plan copies so
they are identical, allowing only each package declaration to differ. Apply any
future test changes consistently to all four files and verify no other
differences remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74ec94fe-75e2-49b7-a857-bd7fd8967440
📒 Files selected for processing (19)
http-client/campaigns/campaigns.httpsrc/campaignoverview/overview.gosrc/campaignoverview/service.gosrc/campaignoverview/service_test.gosrc/genkit/flows/campaign_assistant/context.gosrc/genkit/flows/campaign_assistant/prompts/campaign_assistant.tmplsrc/genkit/flows/campaign_assistant/run.gosrc/genkit/flows/campaign_assistant/scanner_test.gosrc/genkit/flows/campaign_assistant/tools.gosrc/genkit/flows/campaign_assistant/types.gosrc/handlers/campaigns.gosrc/handlers/campaigns_test.gosrc/handlers/posts.gosrc/integration/campaign_assistant_test.gosrc/repository/campaign_assistant_messages.gosrc/repository/campaign_assistant_messages_test.gosrc/server/campaign_assistant.gosrc/server/genkit_runtime.gosrc/server/server.go
🚧 Files skipped from review as they are similar to previous changes (9)
- src/server/campaign_assistant.go
- src/genkit/flows/campaign_assistant/prompts/campaign_assistant.tmpl
- src/genkit/flows/campaign_assistant/context.go
- src/server/genkit_runtime.go
- src/genkit/flows/campaign_assistant/tools.go
- src/server/server.go
- src/genkit/flows/campaign_assistant/run.go
- src/genkit/flows/campaign_assistant/types.go
- src/handlers/campaigns.go
Mirror the post_actions/{clone,restore,schedule} layout: relocate the shared
service to src/campaign_actions/overview and rename the package
campaignoverview -> overview. Pure move + rename; no behaviour change.
post_assistant, enrich_brief, and campaign_assistant each carried a byte-identical 471-line scanner.go (+ identical scanner_test.go). Extract the one copy to a shared src/genkit/jsonstream package (type JSONStringScanner -> jsonstream.Scanner, NewJSONStringScanner -> jsonstream.New) and delete the two duplicates; the three run.go call sites now use jsonstream.New. content_plan's separate jsonPostScanner is unaffected. Pure de-duplication, no behaviour change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/anthropic_tool_order.go`:
- Around line 56-57: Update the request handling around readAnthropicReqBody to
clone req before any mutation, and perform subsequent body, ContentLength, or
GetBody changes on the cloned request. Keep the original request untouched while
preserving the existing request-processing behavior.
🪄 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: 827560ed-83db-41a6-b7bc-42df07cae2f1
📒 Files selected for processing (9)
src/config/config.gosrc/genkit/flows/campaign_assistant/flow.gosrc/genkit/flows/campaign_assistant/prewarm.gosrc/genkit/flows/campaign_assistant/types.gosrc/server/anthropic_http_debug.gosrc/server/anthropic_tool_order.gosrc/server/anthropic_tool_order_test.gosrc/server/campaign_assistant.gosrc/server/genkit_runtime.go
🚧 Files skipped from review as they are similar to previous changes (6)
- src/genkit/flows/campaign_assistant/flow.go
- src/server/campaign_assistant.go
- src/server/anthropic_http_debug.go
- src/config/config.go
- src/server/genkit_runtime.go
- src/genkit/flows/campaign_assistant/types.go
post_assistant sends 7 strict tools, so it has the same cold-compile cost as campaign_assistant: the global tool-order stabilizer keeps its cache warm after the first request, but that first request still paid the ~50s grammar compile. Mirror the campaign_assistant pre-warm: a PrewarmTools flag on the flow config (set by the server from AnthropicStableToolOrder) fires one throwaway max_tokens:1 generation carrying the full 7-tool set in the background at flow init, under the generation model the flow actually uses. WithMaxTurns(1) + max_tokens:1 means no tool executes during warm-up.
… skill Capture the hard-won lesson so the next tool-using flow doesn't re-suffer it: - New Gotcha #15 — root cause (Genkit map-ordered tools + forced strict:true → Anthropic recompiles the constrained-decoding grammar per random tool-set, ~50s/request), the global stabilizer fix, the per-flow pre-warm pattern, and the diagnosis technique (ANTHROPIC_DEBUG_HTTP server_ms vs conn_ms; diff the outgoing bytes — never trust wall-clock alone). - A warning callout in Step 5 (Tools) pointing to Gotcha #15. - A checklist item for wiring the cold-start pre-warm on new tool flows.
toolSetCampaignDates persisted the new dates before counting out-of-range posts, and swallowed a Posts.ListByCampaign error (err == nil guard) — a failed lookup silently reported PostsOutsideRange: 0 while the dates were already saved. Load posts before Campaigns.Update, return the repository error instead of treating a failed lookup as zero, and compute PostsOutsideRange from the loaded posts before persisting. A lookup failure now aborts cleanly with no partial side effect; the count is unchanged on the happy path since Update doesn't touch posts' ScheduledAt.
The posts-consistency limit used a positive req.Max directly, so a model-supplied max bypassed cfg.MaxPosts — the per-call cap that bounds review cost. Compute an effective cap (cfg.MaxPosts when positive, else defaultMaxPosts), start the limit at that cap, and lower it only when a positive req.Max is smaller. A caller may request fewer posts but can never exceed the cap; the defaultMaxPosts fallback is preserved.
UpdateScheduledAtBatch blind-wrote scheduled_at by PK+tenant with no status guard, so a post concurrently published or scheduled between the redistribute read and this write would have its schedule clobbered. Restrict each update to status IN (draft, ready_for_publish) — the reschedule eligibility set — and return an error unless exactly one row is affected. A post that was redistributed or is no longer eligible matches zero rows and fails the whole transaction, rolling back rather than applying a stale plan.
The pass-through test left the RoundTrip response body unclosed (a bodyclose omission). Close it after the successful call, before the assertions; behavior is unchanged since the body is a NopCloser.
anthropicToolOrderTransport.RoundTrip rewrote the sorted body directly on the caller's request (setAnthropicReqBody plus readAnthropicReqBody's body-consuming fallback), violating the http.RoundTripper contract that RoundTrip must not modify the request. Clone the request and apply the Body/ContentLength/GetBody changes to the clone, forwarding the clone and leaving the original untouched. readAnthropicReqBody now reads a copy via GetBody (which net/http sets for the SDK's in-memory payload) without consuming or resetting the original body; it forwards untouched when GetBody is unavailable. Add a RoundTrip test asserting the base receives a sorted clone while the original request's GetBody still yields the original tool order.
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
resolveWindow only applied the default window when both windowStart and windowEnd were empty, so when the planner resolved a vague timeframe like "upcoming weeks" into just a start (no end), it tried to parse the empty end and hard-failed with "windowEnd must be an ISO date" — aborting the whole campaign_assistant turn. Derive the missing bound instead: start-only -> end = start + 14 days; end-only -> start = today. The both-empty default, past-start clamp, and end-before-start guard are unchanged, and a non-empty malformed date is still rejected. Extend TestResolveWindow with the single-bound cases.
This was referenced Jul 17, 2026
This was referenced Aug 6, 2026
This was referenced Aug 15, 2026
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.
Adds a conversational, tenant-scoped Campaign Assistant that mirrors the Post Assistant architecture (shared-service + tool + REST + SSE). A cheap planning model (new RolePlanning / PLANNING_MODEL_ID, Haiku) drives intent-routing while the existing content_plan and enrich_brief flows are invoked as tools for the heavy prose. enrichBrief auto-applies the four brief fields to the campaign.
Summary by CodeRabbit