diff --git a/.gitignore b/.gitignore index e8b46d0..dbe4f51 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,5 @@ /bin /obj +**/bin/ +**/obj/ config.json \ No newline at end of file diff --git a/AuthorAgent.cs b/AuthorAgent.cs index 5c35afd..2c9a056 100644 --- a/AuthorAgent.cs +++ b/AuthorAgent.cs @@ -45,7 +45,7 @@ public AuthorAgent(IChatClient llm, ChatOptions chatOptions, ILogger InvokeAsync(ResearchState state) + public async Task InvokeAsync(ResearchState state, CancellationToken cancellationToken = default) { using Activity? activity = s_activitySource.StartActivity("Author.Invoke"); activity?.SetTag("blog.revision", state.RevisionNumber); @@ -72,7 +72,7 @@ public async Task InvokeAsync(ResearchState state) { MaxOutputTokens = _maxOutputTokens, }); - AgentResponse response = await _agent.RunAsync(message, options: runOptions); + AgentResponse response = await _agent.RunAsync(message, options: runOptions, cancellationToken: cancellationToken); string content = response.Text; return !string.IsNullOrEmpty(content) ? content : "Draft in progress..."; } @@ -83,18 +83,18 @@ public async Task InvokeAsync(ResearchState state) } catch (Exception e) { - Console.WriteLine($"Author error: {e.Message}"); + _logger.LogError(e, "Author agent failed to generate content."); return "Error generating draft. Please try again."; } } /// Author node that creates or revises draft. - public async Task AuthorNodeAsync(ResearchState state) + public async Task AuthorNodeAsync(ResearchState state, CancellationToken cancellationToken = default) { - Console.WriteLine("\n>>>Author"); + _logger.LogInformation("Author stage started."); - string draft = await InvokeAsync(state); - Console.WriteLine($"Draft created: {draft.Length} characters"); + string draft = await InvokeAsync(state, cancellationToken); + _logger.LogInformation("Draft created: {Length} characters", draft.Length); state.Draft = draft; state.RevisionNumber += 1; diff --git a/BlogWorkflow.cs b/BlogWorkflow.cs index 5f5e8ac..b4e6b58 100644 --- a/BlogWorkflow.cs +++ b/BlogWorkflow.cs @@ -1,5 +1,6 @@ using System.Diagnostics; using Microsoft.Agents.AI.Workflows; +using Microsoft.Extensions.Logging; namespace BlogWriter; @@ -15,13 +16,14 @@ public class BlogWorkflow( IBloggerAgent blogger, IResearcherAgent researcher, IAuthorAgent author, - IReviewerAgent reviewer) : IBlogWorkflow + IReviewerAgent reviewer, + ILogger logger) : IBlogWorkflow { // Emits the root span for a workflow run. Activated by the ActivityListener // registered in Program.cs (or an OpenTelemetry TracerProvider). private static readonly ActivitySource s_activitySource = new("BlogWriter.Workflow"); - public async Task RunAsync(ResearchState state) + public async Task RunAsync(ResearchState state, CancellationToken cancellationToken = default) { using Activity? activity = s_activitySource.StartActivity("Workflow.Run"); activity?.SetTag("blog.topic", state.MainTask); @@ -45,27 +47,26 @@ public async Task RunAsync(ResearchState state) // Stream execution instead of running to completion in one shot. The // topology is identical to before (proven terminating, MAF-Doctor grade A); // streaming simply surfaces each executor's lifecycle as it happens, giving - // live progress and replacing the scattered Console.WriteLine tracing that - // previously lived inside the node classes. The final ResearchState is - // captured from the WorkflowOutputEvent emitted by the reviewer. - StreamingRun run = await InProcessExecution.RunStreamingAsync(workflow, state); + // live progress. The final ResearchState is captured from the + // WorkflowOutputEvent emitted by the reviewer. + StreamingRun run = await InProcessExecution.RunStreamingAsync(workflow, state, cancellationToken: cancellationToken); ResearchState? result = null; - await foreach (WorkflowEvent evt in run.WatchStreamAsync()) + await foreach (WorkflowEvent evt in run.WatchStreamAsync().WithCancellation(cancellationToken)) { switch (evt) { case ExecutorInvokedEvent invoked: - Console.WriteLine($"[workflow] → {invoked.ExecutorId} started"); + logger.LogInformation("[workflow] -> {ExecutorId} started", invoked.ExecutorId); break; case ExecutorCompletedEvent completed: - Console.WriteLine($"[workflow] ✓ {completed.ExecutorId} completed"); + logger.LogInformation("[workflow] {ExecutorId} completed", completed.ExecutorId); break; case ExecutorFailedEvent failed: - Console.WriteLine($"[workflow] ✗ {failed.ExecutorId} failed: {(failed.Data as Exception)?.Message}"); + logger.LogError(failed.Data as Exception, "[workflow] {ExecutorId} failed", failed.ExecutorId); // A token-cap breach must abort the whole run, not just the // node. Re-throw it so it unwinds to the application entry point. diff --git a/BlogWriter.Tests/BlogWriter.Tests.csproj b/BlogWriter.Tests/BlogWriter.Tests.csproj new file mode 100644 index 0000000..59def42 --- /dev/null +++ b/BlogWriter.Tests/BlogWriter.Tests.csproj @@ -0,0 +1,20 @@ + + + + net10.0 + enable + enable + false + + + + + + + + + + + + + diff --git a/BlogWriter.Tests/BloggerAgentRoutingTests.cs b/BlogWriter.Tests/BloggerAgentRoutingTests.cs new file mode 100644 index 0000000..59b8298 --- /dev/null +++ b/BlogWriter.Tests/BloggerAgentRoutingTests.cs @@ -0,0 +1,98 @@ +using Microsoft.Extensions.AI; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace BlogWriter.Tests; + +/// +/// Verifies the Blogger's deterministic C# routing rules never reach the model +/// (a fails the test if the LLM path is hit). +/// +public class BloggerAgentRoutingTests +{ + private static BloggerAgent CreateAgent() => + new(new ThrowingChatClient(), new ChatOptions { Temperature = 0, MaxOutputTokens = 100 }, NullLogger.Instance); + + [Fact] + public async Task NoResearch_RoutesToResearcher() + { + BloggerDecision decision = await CreateAgent().InvokeAsync(new ResearchState { MainTask = "topic" }); + + Assert.Equal("researcher", decision.NextStep); + } + + [Fact] + public async Task ResearchButNoDraft_RoutesToAuthor() + { + var state = new ResearchState { MainTask = "topic", ResearchFindings = ["some findings"] }; + + BloggerDecision decision = await CreateAgent().InvokeAsync(state); + + Assert.Equal("author", decision.NextStep); + } + + [Fact] + public async Task DraftWithNoReview_RoutesToReviewer() + { + var state = new ResearchState + { + MainTask = "topic", + ResearchFindings = ["some findings"], + Draft = "a draft", + }; + + BloggerDecision decision = await CreateAgent().InvokeAsync(state); + + Assert.Equal("reviewer", decision.NextStep); + } + + [Fact] + public async Task DraftWithRevisionFeedback_RoutesBackToAuthor() + { + var state = new ResearchState + { + MainTask = "topic", + ResearchFindings = ["some findings"], + Draft = "a draft", + ReviewNotes = "Please tighten the intro.", + RevisionNumber = 1, + }; + + BloggerDecision decision = await CreateAgent().InvokeAsync(state); + + Assert.Equal("author", decision.NextStep); + } + + [Fact] + public async Task ApprovedDraft_Ends() + { + var state = new ResearchState + { + MainTask = "topic", + ResearchFindings = ["some findings"], + Draft = "a draft", + ReviewNotes = ResearchState.ApprovedMarker, + }; + + BloggerDecision decision = await CreateAgent().InvokeAsync(state); + + Assert.Equal("END", decision.NextStep); + } + + [Fact] + public async Task RevisionCapReached_Ends() + { + var state = new ResearchState + { + MainTask = "topic", + ResearchFindings = ["some findings"], + Draft = "a draft", + ReviewNotes = "Still not good enough.", + RevisionNumber = ResearchState.MaxRevisions, + }; + + BloggerDecision decision = await CreateAgent().InvokeAsync(state); + + Assert.Equal("END", decision.NextStep); + } +} diff --git a/BlogWriter.Tests/ResearchStateTests.cs b/BlogWriter.Tests/ResearchStateTests.cs new file mode 100644 index 0000000..769d486 --- /dev/null +++ b/BlogWriter.Tests/ResearchStateTests.cs @@ -0,0 +1,53 @@ +using BlogWriter; +using Xunit; + +namespace BlogWriter.Tests; + +public class ResearchStateTests +{ + [Theory] + [InlineData(null, false)] + [InlineData("", false)] + [InlineData("Needs more detail on X.", false)] + [InlineData("APPROVED", true)] + [InlineData("approved - nice work", true)] + [InlineData("APPROVED - Maximum revisions reached.", true)] + public void IsApproved_DetectsMarkerCaseInsensitively(string? reviewNotes, bool expected) + { + Assert.Equal(expected, ResearchState.IsApproved(reviewNotes)); + } + + [Fact] + public void NeedsRevision_TrueWhenNotApprovedAndUnderCap() + { + var state = new ResearchState { ReviewNotes = "Please revise the intro.", RevisionNumber = 1 }; + + Assert.True(state.NeedsRevision); + } + + [Fact] + public void NeedsRevision_FalseWhenApproved() + { + var state = new ResearchState { ReviewNotes = ResearchState.ApprovedMarker, RevisionNumber = 1 }; + + Assert.False(state.NeedsRevision); + } + + [Fact] + public void NeedsRevision_FalseWhenRevisionCapReached() + { + var state = new ResearchState { ReviewNotes = "Still needs work.", RevisionNumber = ResearchState.MaxRevisions }; + + Assert.False(state.NeedsRevision); + } + + [Fact] + public void NeedsRevision_FalseOneStepBelowCap_TrueWhenBelow() + { + var belowCap = new ResearchState { ReviewNotes = "revise", RevisionNumber = ResearchState.MaxRevisions - 1 }; + var atCap = new ResearchState { ReviewNotes = "revise", RevisionNumber = ResearchState.MaxRevisions }; + + Assert.True(belowCap.NeedsRevision); + Assert.False(atCap.NeedsRevision); + } +} diff --git a/BlogWriter.Tests/TestChatClients.cs b/BlogWriter.Tests/TestChatClients.cs new file mode 100644 index 0000000..9d3c50e --- /dev/null +++ b/BlogWriter.Tests/TestChatClients.cs @@ -0,0 +1,58 @@ +using Microsoft.Extensions.AI; + +namespace BlogWriter.Tests; + +/// Minimal test double returning a canned response with fixed usage. +internal sealed class FakeChatClient : IChatClient +{ + private readonly Func _usageFactory; + + public FakeChatClient(Func usageFactory) => _usageFactory = usageFactory; + + public FakeChatClient(long totalTokens) : this(() => new UsageDetails { TotalTokenCount = totalTokens }) + { + } + + public Task GetResponseAsync( + IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) + { + var response = new ChatResponse(new ChatMessage(ChatRole.Assistant, "ok")) + { + Usage = _usageFactory(), + }; + return Task.FromResult(response); + } + + public async IAsyncEnumerable GetStreamingResponseAsync( + IEnumerable messages, + ChatOptions? options = null, + [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken = default) + { + await Task.Yield(); + yield return new ChatResponseUpdate(ChatRole.Assistant, "ok"); + } + + public object? GetService(Type serviceType, object? serviceKey = null) => null; + + public void Dispose() + { + } +} + +/// An that fails the test if it is ever invoked. +internal sealed class ThrowingChatClient : IChatClient +{ + public Task GetResponseAsync( + IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) => + throw new InvalidOperationException("The model should not have been called for this deterministic routing path."); + + public IAsyncEnumerable GetStreamingResponseAsync( + IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) => + throw new InvalidOperationException("The model should not have been called for this deterministic routing path."); + + public object? GetService(Type serviceType, object? serviceKey = null) => null; + + public void Dispose() + { + } +} diff --git a/BlogWriter.Tests/TokenCapChatClientTests.cs b/BlogWriter.Tests/TokenCapChatClientTests.cs new file mode 100644 index 0000000..fd8c865 --- /dev/null +++ b/BlogWriter.Tests/TokenCapChatClientTests.cs @@ -0,0 +1,41 @@ +using Microsoft.Extensions.AI; +using Xunit; + +namespace BlogWriter.Tests; + +public class TokenCapChatClientTests +{ + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void Constructor_ThrowsForNonPositiveCap(long invalidCap) + { + using var inner = new FakeChatClient(totalTokens: 0); + + Assert.Throws(() => new TokenCapChatClient(inner, invalidCap)); + } + + [Fact] + public async Task GetResponseAsync_AllowsCallsUnderTheCap() + { + using var inner = new FakeChatClient(totalTokens: 40); + using var client = new TokenCapChatClient(inner, maxTotalTokens: 100); + + await client.GetResponseAsync([new ChatMessage(ChatRole.User, "hi")]); + await client.GetResponseAsync([new ChatMessage(ChatRole.User, "hi again")]); + + // 80 tokens consumed of a 100 cap — no exception expected. + } + + [Fact] + public async Task GetResponseAsync_ThrowsOnceCumulativeUsageExceedsCap() + { + using var inner = new FakeChatClient(totalTokens: 60); + using var client = new TokenCapChatClient(inner, maxTotalTokens: 100); + + await client.GetResponseAsync([new ChatMessage(ChatRole.User, "hi")]); + + await Assert.ThrowsAsync( + () => client.GetResponseAsync([new ChatMessage(ChatRole.User, "hi again")])); + } +} diff --git a/BlogWriter.csproj b/BlogWriter.csproj index ba67b23..62c5a4a 100644 --- a/BlogWriter.csproj +++ b/BlogWriter.csproj @@ -10,6 +10,11 @@ 162971f9-9e16-430b-9879-eef4676fe8c8 + + + + + diff --git a/BloggerAgent.cs b/BloggerAgent.cs index f64b65d..1d362b5 100644 --- a/BloggerAgent.cs +++ b/BloggerAgent.cs @@ -54,7 +54,7 @@ public BloggerAgent(IChatClient llm, ChatOptions chatOptions, ILogger InvokeAsync(ResearchState state) + public async Task InvokeAsync(ResearchState state, CancellationToken cancellationToken = default) { using Activity? activity = s_activitySource.StartActivity("Blogger.Invoke"); activity?.SetTag("blog.revision", state.RevisionNumber); @@ -66,40 +66,40 @@ public async Task InvokeAsync(ResearchState state) bool hasDraft = !string.IsNullOrWhiteSpace(state.Draft); string review = state.ReviewNotes; - if (review.ToUpperInvariant().Contains("APPROVED") && hasDraft) + if (ResearchState.IsApproved(review) && hasDraft) { - Console.WriteLine("Blogger: Draft approved, ending workflow"); + _logger.LogInformation("Blogger: Draft approved, ending workflow"); return new BloggerDecision("END", "Report approved and complete"); } if (!hasResearch) { - Console.WriteLine("Blogger: No research yet, directing to researcher"); + _logger.LogInformation("Blogger: No research yet, directing to researcher"); return new BloggerDecision("researcher", $"Research the topic: {state.MainTask}"); } if (hasResearch && !hasDraft) { - Console.WriteLine("Blogger: Have research, creating first draft"); + _logger.LogInformation("Blogger: Have research, creating first draft"); return new BloggerDecision("author", "Write the first draft based on research findings"); } if (hasDraft && string.IsNullOrEmpty(review)) { - Console.WriteLine("Blogger: Have draft, sending to reviewer"); + _logger.LogInformation("Blogger: Have draft, sending to reviewer"); return new BloggerDecision("reviewer", "Prepare draft for review"); } - if (!string.IsNullOrEmpty(review) && !review.ToUpperInvariant().Contains("APPROVED") && revision < ResearchState.MaxRevisions) + if (!string.IsNullOrEmpty(review) && !ResearchState.IsApproved(review) && revision < ResearchState.MaxRevisions) { - Console.WriteLine($"Blogger: Revision {revision}, sending back to author"); + _logger.LogInformation("Blogger: Revision {Revision}, sending back to author", revision); return new BloggerDecision("author", "Revise the draft based on review feedback"); } // Max revisions reached if (revision >= ResearchState.MaxRevisions) { - Console.WriteLine("Blogger: Max revisions reached! Ending"); + _logger.LogInformation("Blogger: Max revisions reached! Ending"); return new BloggerDecision("END", "Maximum revisions reached! Finalizing report"); } @@ -123,7 +123,7 @@ public async Task InvokeAsync(ResearchState state) MaxOutputTokens = _maxOutputTokens, }); AgentResponse response = - await _agent.RunAsync(stateSummary, options: runOptions, serializerOptions: _jsonOptions); + await _agent.RunAsync(stateSummary, options: runOptions, serializerOptions: _jsonOptions, cancellationToken: cancellationToken); BloggerDecision decision = response.Result; if (decision is not null && !string.IsNullOrEmpty(decision.NextStep)) @@ -138,26 +138,23 @@ public async Task InvokeAsync(ResearchState state) } catch (Exception e) { - Console.WriteLine($"LLM decision error: {e.Message}"); + _logger.LogError(e, "Blogger LLM decision failed."); } // Final fallback - continue with author - Console.WriteLine("Blogger: Using final fallback - continuing with author"); + _logger.LogInformation("Blogger: Using final fallback - continuing with author"); return new BloggerDecision("author", "Continue with draft creation"); } /// Blogger decides the next step. - public async Task BloggerNodeAsync(ResearchState state) + public async Task BloggerNodeAsync(ResearchState state, CancellationToken cancellationToken = default) { - Console.WriteLine("\n>>>Blogger"); - - BloggerDecision decision = await InvokeAsync(state); + BloggerDecision decision = await InvokeAsync(state, cancellationToken); string nextStep = string.IsNullOrEmpty(decision.NextStep) ? "researcher" : decision.NextStep; string taskDesc = string.IsNullOrEmpty(decision.TaskDescription) ? "Continue work" : decision.TaskDescription; - Console.WriteLine($"Decision: {nextStep}"); - Console.WriteLine($"Task: {taskDesc}"); + _logger.LogInformation("Blogger decision: {NextStep}, Task: {TaskDescription}", nextStep, taskDesc); state.NextStep = nextStep; state.CurrentSubTask = taskDesc; diff --git a/IAuthorAgent.cs b/IAuthorAgent.cs index f434957..73e9bf8 100644 --- a/IAuthorAgent.cs +++ b/IAuthorAgent.cs @@ -2,7 +2,7 @@ namespace BlogWriter; public interface IAuthorAgent { - Task InvokeAsync(ResearchState state); + Task InvokeAsync(ResearchState state, CancellationToken cancellationToken = default); - Task AuthorNodeAsync(ResearchState state); + Task AuthorNodeAsync(ResearchState state, CancellationToken cancellationToken = default); } diff --git a/IBlogWorkflow.cs b/IBlogWorkflow.cs index 71a93e0..c8433ac 100644 --- a/IBlogWorkflow.cs +++ b/IBlogWorkflow.cs @@ -2,5 +2,5 @@ namespace BlogWriter; public interface IBlogWorkflow { - Task RunAsync(ResearchState state); + Task RunAsync(ResearchState state, CancellationToken cancellationToken = default); } diff --git a/IBloggerAgent.cs b/IBloggerAgent.cs index 8ca673f..ab5a240 100644 --- a/IBloggerAgent.cs +++ b/IBloggerAgent.cs @@ -2,7 +2,7 @@ namespace BlogWriter; public interface IBloggerAgent { - Task InvokeAsync(ResearchState state); + Task InvokeAsync(ResearchState state, CancellationToken cancellationToken = default); - Task BloggerNodeAsync(ResearchState state); + Task BloggerNodeAsync(ResearchState state, CancellationToken cancellationToken = default); } diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md new file mode 100644 index 0000000..9259ab1 --- /dev/null +++ b/IMPROVEMENTS.md @@ -0,0 +1,74 @@ +# BlogWriter — MAF Health & Improvement Report +_Generated 2026-08-17_ + +## 1. Tooling status + +| Item | Value | +|---|---| +| maf-doctor tool version (installed) | `1.14.0` | +| maf-doctor latest available | `1.14.0` | +| Status | **Up to date** — no update needed | +| Workspace init | Current, no re-run of `maf-doctor init` required | + +## 2. MAF health grade: **B** + +Full scan (`MafDoctor --full`) results: + +| Metric | Count | +|---|---:| +| Anti-pattern errors | 0 | +| Anti-pattern warnings | 0 | +| Anti-pattern info notes | 0 | +| Silent-starvation risks (fan-out) | 0 | +| Prompt-lint errors (injection, etc.) | 0 | +| Prompt-lint warnings | 0 | +| `[MessageHandler]` methods inspected | 4 | +| Agent call sites flagged "no `MaxOutputTokens` cap" | 4 (all false positives — see below) | + +**The only findings are 4 heuristic `COST-001` notes** (one per agent: `AuthorAgent.cs:75`, `BloggerAgent.cs:126`, `ResearcherAgent.cs:88`, `ReviewerAgent.cs:77`) claiming the `RunAsync` call has no output-token cap. I verified this against the actual code: **it's a false positive.** Every agent already: +- caps `MaxOutputTokens` on the agent's own `ChatOptions` at construction, and +- re-applies `_maxOutputTokens` on a per-call `ChatClientAgentRunOptions` on every `RunAsync`. + +The scanner is a text/name heuristic and doesn't trace the `options: runOptions` variable back to its construction, so it can't see the cap is actually there. No action needed — this repo has no real, unmitigated cost-runaway risk. + +There is a genuinely good, second layer of cost protection on top of that: `TokenCapChatClient` enforces a **hard cumulative 10,000-token budget for the whole process**, across every model round-trip (including tool-invocation turns), and fails the run loudly (`TokenCapExceededException`) rather than silently overspending. + +## 3. What's good — in plain language + +- **Clean separation of concerns.** Each pipeline stage (Blogger, Researcher, Author, Reviewer) has its own interface, agent implementation, and workflow executor. The workflow graph (`BlogWorkflow.cs`) is a thin orchestration layer over that — easy to read, easy to test in isolation. +- **The workflow is provably terminating.** The reviewer→author loop is gated by `ResearchState.MaxRevisions` (a hard cap of 4), so there's no way for the graph to spin forever even if the model keeps asking for revisions. +- **Cost is controlled at two independent layers**: per-call `MaxOutputTokens` and a process-wide cumulative token budget (`TokenCapChatClient`). This is a genuinely strong pattern — most sample MAF apps only do one or the other. +- **Structured decision-making done right.** `BloggerAgent` uses `RunAsync` to get a typed result straight from the model instead of hand-rolling JSON extraction/fenced-code-block stripping — this is the current MAF idiom and avoids a whole class of brittle parsing bugs. +- **Deterministic routing before LLM routing.** `BloggerAgent.InvokeAsync` short-circuits with plain C# rules (no research → researcher, has draft + approved → END, etc.) and only falls back to an LLM call when the state is genuinely ambiguous. This saves tokens and makes the common paths 100% predictable. +- **Secrets hygiene.** API keys come from `dotnet user-secrets` locally and environment variables in CI/prod, never hardcoded, with a clear fail-fast message (`GetRequired`) if a key is missing. +- **Observability is wired in, not bolted on.** Every agent and the workflow itself emits `ActivitySource` spans, and the `IChatClient` pipeline emits GenAI spans via `UseOpenTelemetry`. Swapping the console `ActivityListener` for a real `TracerProvider` is a one-line change. +- **Token-cap failure handling is correct.** A budget breach is deliberately *not* caught and swallowed — it's re-thrown through the workflow's `ExecutorFailedEvent` handling and all the way to `Program.cs`, so the process exits cleanly instead of continuing to spend money after the budget is blown. + +## 4. What's wrong / weak — in plain language + +None of these are MAF anti-patterns (the scanner is clean); they're general .NET/production-readiness gaps that matter if this app grows beyond a demo/CLI. + +1. **`Console.WriteLine` instead of `ILogger` almost everywhere.** Every agent takes an `ILogger` in its constructor but only uses it once (`"...initialized."`). All the actually interesting events — errors, decisions, review outcomes, revision counts — go to `Console.WriteLine`, which means they're unstructured, can't be filtered/routed by log level, and won't show up if this ever runs somewhere without a console (e.g. a hosted service or Azure Function). +2. **Broad `catch (Exception e)` blocks that swallow the real error.** `AuthorAgent`, `ResearcherAgent`, `ReviewerAgent`, and `BloggerAgent` all catch every exception from `RunAsync` and replace it with a generic fallback string ("Error generating draft...", "Research completed on: ..."). This means a genuine auth failure, rate-limit, network timeout, or bad-request error looks identical to the workflow as "the model didn't have much to say" — nothing distinguishes a transient/retryable failure from a permanent one, and the real exception is only ever printed to the console, never logged with the `ILogger` that's already injected. +3. **No `CancellationToken` anywhere in the agent/workflow APIs.** `InvokeAsync`, `*NodeAsync`, `RunAsync` (on `IBlogWorkflow`) — none of them accept or forward a `CancellationToken`. There's no way to cancel an in-flight run (Ctrl+C, a timeout, a hosting shutdown signal); the process can only be stopped by the token-cap exception or letting it run to completion. +4. **No resilience policy around outbound calls.** Neither the OpenAI chat client pipeline nor the raw `tavilyHttpClient` has retry/backoff/timeout policies. A single transient network blip on a Tavily call or a chat completion call falls straight into the generic `catch` and produces a silent, low-quality fallback rather than retrying once or twice. +5. **`tavilyHttpClient` is a manually constructed, unbounded-lifetime `HttpClient`.** It's fine for a short-lived console run, but it's not using `IHttpClientFactory`/`AddHttpClient`, has no request timeout configured (defaults to 100s), and would be a socket-exhaustion risk if this code were ever lifted into a long-running service. +6. **Duplicated "is approved?" string check.** `review.ToUpperInvariant().Contains("APPROVED")` is duplicated across `BloggerAgent` and `ReviewerAgent`. It's a magic string with no single source of truth — a typo in one place (e.g. "Approved" vs the exact literal used elsewhere) silently breaks the loop-exit condition. +7. **Model name and token budget are hardcoded in `Program.cs`** (`"gpt-4o-mini"`, `MaxOutputTokens = 4096`, cumulative cap `10000`). Fine for a demo; brittle if you want to swap models/budgets without recompiling. +8. **No automated tests.** There's no test project in the repo. The workflow's termination guarantee, the Blogger's deterministic routing rules, and the token-cap logic are all excellent candidates for fast, no-LLM-required unit tests (they're pure C# logic), but none exist today. +9. **Minor nit:** `TokenCapChatClient.Track` throws `InvalidOperationException` for a non-positive `maxTotalTokens` *inside the per-response hot path* rather than validating it once in the constructor — it should fail fast at construction instead of on the first token update. + +## 5. Recommended action plan (priority order) + +| # | Action | Effort | Why first | +|---|---|---|---| +| 1 | Replace `Console.WriteLine` calls in agents/workflow with `_logger.LogInformation/LogWarning/LogError`, including the caught exception object (`_logger.LogError(e, ...)`) instead of `e.Message` only. | Small | Everything downstream (diagnostics, prod-readiness) depends on this; `ILogger` is already injected everywhere. | +| 2 | In each agent's `catch (Exception e)` block, log the real exception via `ILogger` before returning the fallback string, and consider narrowing the catch (e.g. distinguish `ClientResultException`/HTTP errors from unexpected bugs). | Small–Medium | Currently a real outage looks identical to "model had nothing to say." | +| 3 | Thread a `CancellationToken` through `IBlogWorkflow.RunAsync` → node methods → `RunAsync` calls, and pass `Console.CancelKeyPress`/a timeout token from `Program.cs`. | Medium | Lets the app be stopped cleanly and bounds worst-case run time independent of the token cap. | +| 4 | Add a small resilience layer (e.g. `Microsoft.Extensions.Http.Resilience` for `tavilyHttpClient`, or a `.Use(...)` retry middleware on the `IChatClient` pipeline) for transient failures, with a sane per-call timeout. | Medium | Reduces "silent low-quality fallback" outcomes caused by one-off network blips. | +| 5 | Centralize the "approved" check as a single helper/constant (e.g. `ResearchState.IsApproved(string reviewNotes)` or a `const string ApprovedMarker = "APPROVED"`) used by both `BloggerAgent` and `ReviewerAgent`. | Small | Removes a duplicated magic string that both loop-exit paths depend on. | +| 6 | Move `modelName`, per-call `MaxOutputTokens`, and the cumulative token cap into configuration (`IConfiguration`/env vars) instead of literals in `Program.cs`. | Small | Lets you tune cost/model without recompiling. | +| 7 | Add a test project covering: `ResearchState.NeedsRevision` boundary conditions, `BloggerAgent`'s deterministic routing rules, and `TokenCapChatClient`'s cap-exceeded behavior. All three are pure logic — no live model calls required. | Medium | Cheapest tests to write, and they protect the two correctness guarantees (termination, budget) that matter most. | +| 8 | Fix the `TokenCapChatClient` constructor to validate `maxTotalTokens > 0` eagerly (throw in the constructor, not in `Track`). | Trivial | Fail-fast instead of failing on the first real response. | + +Nothing above is required to keep the MAF grade at **B** — the scanner is already clean. These are general production-hardening items for when this moves beyond a demo CLI. diff --git a/IResearcherAgent.cs b/IResearcherAgent.cs index 589f5a6..96a2a47 100644 --- a/IResearcherAgent.cs +++ b/IResearcherAgent.cs @@ -2,7 +2,7 @@ namespace BlogWriter; public interface IResearcherAgent { - Task InvokeAsync(string query); + Task InvokeAsync(string query, CancellationToken cancellationToken = default); - Task ResearchNodeAsync(ResearchState state); + Task ResearchNodeAsync(ResearchState state, CancellationToken cancellationToken = default); } diff --git a/IReviewerAgent.cs b/IReviewerAgent.cs index 32d9742..81d1215 100644 --- a/IReviewerAgent.cs +++ b/IReviewerAgent.cs @@ -2,7 +2,7 @@ namespace BlogWriter; public interface IReviewerAgent { - Task InvokeAsync(ResearchState state); + Task InvokeAsync(ResearchState state, CancellationToken cancellationToken = default); - Task ReviewerNodeAsync(ResearchState state); + Task ReviewerNodeAsync(ResearchState state, CancellationToken cancellationToken = default); } diff --git a/Program.cs b/Program.cs index 4005ab3..e6923fb 100644 --- a/Program.cs +++ b/Program.cs @@ -23,13 +23,19 @@ string GetRequired(string key) => string openAiApiBase = GetRequired("OPENAI_API_BASE"); string tavilyApiKey = GetRequired("TAVILY_API_KEY"); -string modelName = "gpt-4o-mini"; +// Overridable via user-secrets/env vars; these defaults match the original behaviour. +string modelName = config["MODEL_NAME"] ?? "gpt-4o-mini"; +int maxOutputTokens = int.TryParse(config["MAX_OUTPUT_TOKENS"], out int configuredMaxOutputTokens) ? configuredMaxOutputTokens : 4096; +long maxTotalTokens = long.TryParse(config["MAX_TOTAL_TOKENS"], out long configuredMaxTotalTokens) ? configuredMaxTotalTokens : 10000; var openAIClient = new OpenAIClient( new ApiKeyCredential(openAiApiKey), new OpenAIClientOptions { - Endpoint = new Uri(openAiApiBase) + Endpoint = new Uri(openAiApiBase), + // The SDK's default RetryPolicy still applies on top of this; this only + // bounds how long a single network attempt can hang before it retries/fails. + NetworkTimeout = TimeSpan.FromSeconds(60), }); // Build the IChatClient pipeline once and share it across all agents. @@ -46,28 +52,48 @@ string GetRequired(string key) => // TokenCapChatClient is registered *after* function invocation, which makes it // the innermost wrapper around the raw client — so it observes every individual // model round-trip (including the extra calls tool invocation triggers) and -// enforces a hard 10,000-token budget for the whole process. +// enforces a hard cumulative-token budget for the whole process. IChatClient llm = openAIClient .GetChatClient(modelName) .AsIChatClient() .AsBuilder() .UseFunctionInvocation() .UseOpenTelemetry(sourceName: "BlogWriter.ChatClient") - .Use(inner => new TokenCapChatClient(inner, maxTotalTokens: 10000)) + .Use(inner => new TokenCapChatClient(inner, maxTotalTokens)) .Build(); var chatOptions = new ChatOptions { Temperature = 0, - MaxOutputTokens = 4096 + MaxOutputTokens = maxOutputTokens }; -var tavilyHttpClient = new HttpClient { BaseAddress = new Uri("https://api.tavily.com/") }; +var tavilyHttpClient = new HttpClient { BaseAddress = new Uri("https://api.tavily.com/"), Timeout = TimeSpan.FromSeconds(20) }; tavilyHttpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", tavilyApiKey); +// Small manual retry: transient network errors/timeouts get up to 2 retries +// with exponential backoff before the failure surfaces to the calling agent. +async Task PostWithRetryAsync(string requestUri, object body, CancellationToken cancellationToken) +{ + const int maxAttempts = 3; + for (int attempt = 1; ; attempt++) + { + try + { + HttpResponseMessage response = await tavilyHttpClient.PostAsJsonAsync(requestUri, body, cancellationToken); + response.EnsureSuccessStatusCode(); + return response; + } + catch (Exception ex) when (attempt < maxAttempts && ex is HttpRequestException or TaskCanceledException) + { + await Task.Delay(TimeSpan.FromSeconds(Math.Pow(2, attempt - 1)), cancellationToken); + } + } +} + AIFunction tavilyTool = AIFunctionFactory.Create( - async (string query) => + async (string query, CancellationToken cancellationToken) => { var request = new { @@ -79,9 +105,8 @@ string GetRequired(string key) => search_depth = "basic" }; - using HttpResponseMessage response = await tavilyHttpClient.PostAsJsonAsync("search", request); - response.EnsureSuccessStatusCode(); - return await response.Content.ReadAsStringAsync(); + using HttpResponseMessage response = await PostWithRetryAsync("search", request, cancellationToken); + return await response.Content.ReadAsStringAsync(cancellationToken); }, name: "tavily_search", description: "A search engine optimized for comprehensive, accurate, and trusted results."); @@ -93,7 +118,7 @@ string GetRequired(string key) => var researcherAgent = new ResearcherAgent(llm, chatOptions, tavilyTool, loggerFactory.CreateLogger()); var authorAgent = new AuthorAgent(llm, chatOptions, loggerFactory.CreateLogger()); var reviewerAgent = new ReviewerAgent(llm, chatOptions, loggerFactory.CreateLogger()); -var app = new BlogWorkflow(bloggerAgent, researcherAgent, authorAgent, reviewerAgent); +var app = new BlogWorkflow(bloggerAgent, researcherAgent, authorAgent, reviewerAgent, loggerFactory.CreateLogger()); // Distributed tracing: an ActivityListener activates every "BlogWriter.*" // ActivitySource in the app (agents, workflow, and the IChatClient's @@ -120,12 +145,21 @@ string GetRequired(string key) => MainTask = topic }; +// Ctrl+C requests a graceful cancellation of the in-flight run instead of an +// abrupt process kill. +using var cts = new CancellationTokenSource(); +Console.CancelKeyPress += (_, e) => +{ + e.Cancel = true; + cts.Cancel(); +}; + ResearchState result; try { using Activity? runActivity = appActivitySource.StartActivity("BlogWriter.Run"); runActivity?.SetTag("blog.topic", topic); - result = await app.RunAsync(initialState); + result = await app.RunAsync(initialState, cts.Token); } catch (TokenCapExceededException ex) { @@ -135,6 +169,12 @@ string GetRequired(string key) => Environment.ExitCode = 1; return; } +catch (OperationCanceledException) +{ + Console.Error.WriteLine("Run cancelled. Exiting application."); + Environment.ExitCode = 1; + return; +} Console.WriteLine("\n========== RESULTS =========="); Console.WriteLine($"Task: {result.MainTask}"); diff --git a/ResearchState.cs b/ResearchState.cs index d10bd41..c2a1812 100644 --- a/ResearchState.cs +++ b/ResearchState.cs @@ -6,6 +6,9 @@ public class ResearchState /// Hard upper bound on author/review revision cycles. Guarantees the workflow terminates. public const int MaxRevisions = 4; + /// The single source-of-truth marker written to on approval. + public const string ApprovedMarker = "APPROVED"; + public string MainTask { get; set; } = ""; public List ResearchFindings { get; set; } = []; public string Draft { get; set; } = ""; @@ -14,11 +17,13 @@ public class ResearchState public string NextStep { get; set; } = ""; public string CurrentSubTask { get; set; } = ""; + /// True when the given review text contains the approval marker (case-insensitive). + public static bool IsApproved(string? reviewNotes) => + !string.IsNullOrEmpty(reviewNotes) && reviewNotes.Contains(ApprovedMarker, StringComparison.OrdinalIgnoreCase); + /// /// True while the draft is not yet approved AND the revision cap has not been /// reached. Drives the bounded review loop; when false the workflow terminates. /// - public bool NeedsRevision => - !string.Equals(ReviewNotes, "APPROVED", StringComparison.OrdinalIgnoreCase) - && RevisionNumber < MaxRevisions; + public bool NeedsRevision => !IsApproved(ReviewNotes) && RevisionNumber < MaxRevisions; } diff --git a/ResearcherAgent.cs b/ResearcherAgent.cs index bc86bf7..3b84e87 100644 --- a/ResearcherAgent.cs +++ b/ResearcherAgent.cs @@ -71,7 +71,7 @@ public ResearcherAgent(IChatClient llm, ChatOptions chatOptions, AIFunction tavi } /// Execute research by letting the agent search and summarise. - public async Task InvokeAsync(string query) + public async Task InvokeAsync(string query, CancellationToken cancellationToken = default) { using Activity? activity = s_activitySource.StartActivity("Researcher.Invoke"); activity?.SetTag("blog.query", query); @@ -85,7 +85,7 @@ public async Task InvokeAsync(string query) { MaxOutputTokens = _maxOutputTokens, }); - AgentResponse response = await _agent.RunAsync(query, options: runOptions); + AgentResponse response = await _agent.RunAsync(query, options: runOptions, cancellationToken: cancellationToken); string summary = response.Text; return !string.IsNullOrEmpty(summary) @@ -99,25 +99,23 @@ public async Task InvokeAsync(string query) } catch (Exception e) { - Console.WriteLine($"Research error: {e.Message}"); + _logger.LogError(e, "Research failed for query '{Query}'.", query); return $"Research completed on: {query}. Key information has been gathered from web sources."; } } /// Research node that gathers information. - public async Task ResearchNodeAsync(ResearchState state) + public async Task ResearchNodeAsync(ResearchState state, CancellationToken cancellationToken = default) { - Console.WriteLine("\n>>>RESEARCHER"); - string subTask = !string.IsNullOrEmpty(state.CurrentSubTask) ? state.CurrentSubTask : state.MainTask; - Console.WriteLine($"Researching: {subTask}"); + _logger.LogInformation("Researching: {SubTask}", subTask); string findings; try { - findings = await InvokeAsync(subTask); + findings = await InvokeAsync(subTask, cancellationToken); string preview = findings.Length > 100 ? findings[..100] : findings; - Console.WriteLine($"Found: {preview}..."); + _logger.LogInformation("Found: {Preview}...", preview); } catch (TokenCapExceededException) { @@ -126,7 +124,7 @@ public async Task ResearchNodeAsync(ResearchState state) } catch (Exception e) { - Console.WriteLine($"Research error: {e.Message}"); + _logger.LogError(e, "Research failed for sub-task '{SubTask}'.", subTask); findings = $"Research on {subTask} - information gathered"; } diff --git a/ReviewerAgent.cs b/ReviewerAgent.cs index bd6eec6..fa02546 100644 --- a/ReviewerAgent.cs +++ b/ReviewerAgent.cs @@ -46,7 +46,7 @@ public ReviewerAgent(IChatClient llm, ChatOptions chatOptions, ILogger InvokeAsync(ResearchState state) + public async Task InvokeAsync(ResearchState state, CancellationToken cancellationToken = default) { using Activity? activity = s_activitySource.StartActivity("Reviewer.Invoke"); activity?.SetTag("blog.revision", state.RevisionNumber); @@ -74,7 +74,7 @@ public async Task InvokeAsync(ResearchState state) { MaxOutputTokens = _maxOutputTokens, }); - AgentResponse response = await _agent.RunAsync(message, options: runOptions); + AgentResponse response = await _agent.RunAsync(message, options: runOptions, cancellationToken: cancellationToken); string content = response.Text; return !string.IsNullOrEmpty(content) ? content : ManageError("No review content returned from the agent."); } @@ -85,39 +85,45 @@ public async Task InvokeAsync(ResearchState state) } catch (Exception e) { - return ManageError(e.Message); + return ManageError(e.Message, e); } } - private string ManageError(string errorMessage) + private string ManageError(string reason, Exception? exception = null) { // Do NOT approve on failure — that would ship an unreviewed draft. // Returning feedback (not "APPROVED") routes back to the author for // another attempt; the revision cap still guarantees termination. - Console.WriteLine($"Review error: {errorMessage}"); + if (exception is not null) + { + _logger.LogError(exception, "Review failed: {Reason}", reason); + } + else + { + _logger.LogWarning("Review could not be completed: {Reason}", reason); + } + return "Review could not be completed due to a transient error. Please revise and resubmit the draft."; } /// Node that reviews the draft. - public async Task ReviewerNodeAsync(ResearchState state) + public async Task ReviewerNodeAsync(ResearchState state, CancellationToken cancellationToken = default) { - Console.WriteLine("\n>>REVIEWER"); - - string review = await InvokeAsync(state); + string review = await InvokeAsync(state, cancellationToken); string preview = review.Length > 100 ? review[..100] : review; - Console.WriteLine($"Review: {preview}..."); + _logger.LogInformation("Review: {Preview}...", preview); - bool isApproved = review.ToUpperInvariant().Contains("APPROVED"); + bool isApproved = ResearchState.IsApproved(review); if (isApproved) { - Console.WriteLine("\u2713 Draft APPROVED"); - state.ReviewNotes = "APPROVED"; + _logger.LogInformation("Draft APPROVED"); + state.ReviewNotes = ResearchState.ApprovedMarker; state.NextStep = "END"; } else { - Console.WriteLine("\u2717 Revisions needed"); + _logger.LogInformation("Revisions needed"); state.ReviewNotes = review; state.NextStep = "author"; } diff --git a/TokenCapChatClient.cs b/TokenCapChatClient.cs index 716dd81..b70a609 100644 --- a/TokenCapChatClient.cs +++ b/TokenCapChatClient.cs @@ -10,11 +10,18 @@ namespace BlogWriter; /// application is terminated with an explanatory message rather than continuing /// to spend tokens. /// -public sealed class TokenCapChatClient(IChatClient innerClient, long maxTotalTokens) - : DelegatingChatClient(innerClient) +public sealed class TokenCapChatClient : DelegatingChatClient { + private readonly long _maxTotalTokens; private long _totalTokens; + public TokenCapChatClient(IChatClient innerClient, long maxTotalTokens) : base(innerClient) + { + _maxTotalTokens = maxTotalTokens > 0 + ? maxTotalTokens + : throw new ArgumentOutOfRangeException(nameof(maxTotalTokens), maxTotalTokens, "Token cap must be a positive number."); + } + public override async Task GetResponseAsync( IEnumerable messages, ChatOptions? options = null, @@ -53,15 +60,10 @@ private void Track(UsageDetails? usage) return; } - if (maxTotalTokens <= 0) - { - throw new InvalidOperationException("Token cap must be a positive number."); - } - long total = Interlocked.Add(ref _totalTokens, used); - if (total > maxTotalTokens) + if (total > _maxTotalTokens) { - throw new TokenCapExceededException(total, maxTotalTokens); + throw new TokenCapExceededException(total, _maxTotalTokens); } } } diff --git a/Workflows/BlogExecutors.cs b/Workflows/BlogExecutors.cs index c8ceb20..7f91764 100644 --- a/Workflows/BlogExecutors.cs +++ b/Workflows/BlogExecutors.cs @@ -10,24 +10,24 @@ namespace BlogWriter; internal sealed partial class BloggerExecutor(IBloggerAgent blogger) : Executor("Blogger") { [MessageHandler] - private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context) - => await blogger.BloggerNodeAsync(state); + private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context, CancellationToken cancellationToken) + => await blogger.BloggerNodeAsync(state, cancellationToken); } /// Gathers research findings. internal sealed partial class ResearcherExecutor(IResearcherAgent researcher) : Executor("Researcher") { [MessageHandler] - private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context) - => await researcher.ResearchNodeAsync(state); + private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context, CancellationToken cancellationToken) + => await researcher.ResearchNodeAsync(state, cancellationToken); } /// Writes or revises the draft (increments the revision counter). internal sealed partial class AuthorExecutor(IAuthorAgent author) : Executor("Author") { [MessageHandler] - private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context) - => await author.AuthorNodeAsync(state); + private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context, CancellationToken cancellationToken) + => await author.AuthorNodeAsync(state, cancellationToken); } /// @@ -37,9 +37,9 @@ private async ValueTask HandleAsync(ResearchState state, IWorkflo internal sealed partial class ReviewerExecutor(IReviewerAgent reviewer) : Executor("Reviewer") { [MessageHandler] - private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context) + private async ValueTask HandleAsync(ResearchState state, IWorkflowContext context, CancellationToken cancellationToken) { - state = await reviewer.ReviewerNodeAsync(state); + state = await reviewer.ReviewerNodeAsync(state, cancellationToken); if (!state.NeedsRevision) {