TeamAI/NovelAi.ImageGen.Mcp
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat: initial MCP server scaffold with generate_image tool #1

Merged
bjoern merged 0 commits from feat/initial-scaffold into main 2026-07-04 11:42:15 +02:00
Conversation 7 Commits - Files changed 27 +1563
matikane commented 2026-07-03 18:56:50 +02:00
Member
Copy link

Summary

Initial scaffold of the NovelAI image generation MCP server. Exposes a single generate_image tool over stdio using the official MCP C# SDK, backed by the NovelAI.ImageGen library (referenced as a git submodule).

What's included

  • MCP server (stdio transport) using ModelContextProtocol NuGet v1.4.0
  • generate_image tool with categorized scene/character tag schema mirroring the angela assistant's tool
  • NovelAI.ImageGen library as git submodule at externals/NovelAI.ImageGen

Tool parameters

Parameter Type Required Default Description
scene_tags object yes — Categorized: location, objects, composition
characters array no [] Each with tags (identity, hair, face, body, clothing, accessories, pose) and gender
aspect_ratio string no portrait portrait/landscape/square; unknown falls back to portrait
use_quality_tags bool no true Appends internal quality tag sets (placeholders: pos=[masterpiece], neg=[low quality])
seed integer no random For reproducible generation
negative_tags string[] no [] Additional negative tags (appended to quality negatives)

Additions beyond angela's tool

  • seed parameter (reproducible generation / iteration)
  • negative_tags parameter (escape hatch for unwanted concepts)
  • use_quality_tags bool flag (placeholder lists for now, full lists TBD)
  • Returns MCP ImageContentBlock alongside the text path/seed info

Parsing semantics (matching angela)

  • Category names in tag maps are descriptive hints - only insertion-order flattening matters
  • Unknown aspect_ratio silently falls back to portrait
  • Unknown gender falls back to none
  • Fixed generation config: guidance 5.0, steps 28, model Diffusion 4.5 Full, sampler euler ancestral, karras noise schedule

Testing

32 unit tests (xUnit v3 + NSubstitute), all passing. TreatWarningsAsErrors enabled. CPM. .NET 10.

Not in scope (v2)

  • Tag browsing tools (search/lookup)
  • HTTP/streamable transport
  • Style reference support (PreciseReference)
  • Alias expansion (angela-specific concern)

Config

API key via NOVELAI_API_KEY env var. Output directory defaults to _ImageGen/. See README for Claude Desktop config example.

## Summary Initial scaffold of the NovelAI image generation MCP server. Exposes a single `generate_image` tool over stdio using the official MCP C# SDK, backed by the [NovelAI.ImageGen](https://git.kagaku.eu/TeamAI/NovelAI.ImageGen) library (referenced as a git submodule). ## What's included - **MCP server** (stdio transport) using `ModelContextProtocol` NuGet v1.4.0 - **`generate_image` tool** with categorized scene/character tag schema mirroring the angela assistant's tool - **NovelAI.ImageGen library** as git submodule at `externals/NovelAI.ImageGen` ### Tool parameters | Parameter | Type | Required | Default | Description | |---|---|---|---|---| | `scene_tags` | object | yes | — | Categorized: `location`, `objects`, `composition` | | `characters` | array | no | `[]` | Each with `tags` (identity, hair, face, body, clothing, accessories, pose) and `gender` | | `aspect_ratio` | string | no | `portrait` | portrait/landscape/square; unknown falls back to portrait | | `use_quality_tags` | bool | no | `true` | Appends internal quality tag sets (placeholders: pos=[masterpiece], neg=[low quality]) | | `seed` | integer | no | random | For reproducible generation | | `negative_tags` | string[] | no | `[]` | Additional negative tags (appended to quality negatives) | ### Additions beyond angela's tool - `seed` parameter (reproducible generation / iteration) - `negative_tags` parameter (escape hatch for unwanted concepts) - `use_quality_tags` bool flag (placeholder lists for now, full lists TBD) - Returns MCP `ImageContentBlock` alongside the text path/seed info ### Parsing semantics (matching angela) - Category names in tag maps are descriptive hints - only insertion-order flattening matters - Unknown `aspect_ratio` silently falls back to portrait - Unknown `gender` falls back to `none` - Fixed generation config: guidance 5.0, steps 28, model Diffusion 4.5 Full, sampler euler ancestral, karras noise schedule ### Testing 32 unit tests (xUnit v3 + NSubstitute), all passing. `TreatWarningsAsErrors` enabled. CPM. .NET 10. ## Not in scope (v2) - Tag browsing tools (search/lookup) - HTTP/streamable transport - Style reference support (PreciseReference) - Alias expansion (angela-specific concern) ## Config API key via `NOVELAI_API_KEY` env var. Output directory defaults to `_ImageGen/`. See README for Claude Desktop config example.
bjoern commented 2026-07-03 19:14:06 +02:00
Owner
Copy link

Please have one file for one (public) class, even if they are sealed. src/NovelAI.ImageGen.Mcp/Tools/GenerateImageParameters.cs is in particular a candidate

Please have one file for one (public) class, even if they are sealed. src/NovelAI.ImageGen.Mcp/Tools/GenerateImageParameters.cs is in particular a candidate
jibril commented 2026-07-03 19:20:32 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh my, a brand-new MCP server, fresh out of the oven! A clean scaffold, 32 passing tests, TreatWarningsAsErrors, CPM, .NET 10... fufu~, you've laid such a pretty foundation here, matikane~ ♡ The tag-flattening semantics are faithfully mirrored from angela, the Result pattern handling is correct, and the DTO→Tag map insertion-order contract is well-tested. I genuinely enjoyed reading this!

But... ♪

Verdict: ⛔ I can't let this pass~ ♡

You wouldn't leave THESE little bugs in production, would you? The smile never wavers, but my eyes are very sharp right now~

⛔ These need fixing before I'm satisfied~

  1. [src/NovelAI.ImageGen.Mcp/Storage/ImageStorageService.cs:12] — SavedPathPrefix config is dead code; the prefix is hard-coded.
    You defined NovelAiMcpOptions.SavedPathPrefix, documented it thoroughly ("When set (non-null), the tool reports the saved file's path as {prefix}/{filename}"), and even set it explicitly in appsettings.json. But the service never reads it:

    private readonly string _outputDirectory = options.Value.OutputDirectory;   // ✅ reads config
private static readonly string _savedPathPrefix = "_ImageGen";               // ❌ hard-coded!

    At runtime, if a user sets OutputDirectory=/data/images and SavedPathPrefix=/data/images (or any non-default), the image is saved correctly to /data/images/... but the path returned to the MCP client in the text block still says _ImageGen/... — pointing the LLM to a location that doesn't exist. The client can't find the image it just generated. That's wrong runtime behavior, full stop.
    Fix: private readonly string _savedPathPrefix = options.Value.SavedPathPrefix ?? options.Value.OutputDirectory; (drop static, read from the bound options — same pattern you already use for _outputDirectory).

  2. [src/NovelAI.ImageGen.Mcp/Tools/GenerateImageTool.cs:119] — storage.SaveImage() is unguarded; a failed write silently destroys a paid API result.
    After a successful (and potentially expensive — real NovelAI credits!) client.GenerateImageAsync, you call storage.SaveImage(image.Data, image.Seed) with no try/catch. If File.WriteAllBytes throws — disk full, permission denied, path too long, the configured OutputDirectory doesn't exist and CreateDirectory races — the raw IOException propagates out of the tool as an unhandled exception. The image bytes are in memory and simply... lost. The client gets a crash, not a useful error, and has no way to recover the generation.
    Fix: Wrap the save in a try/catch (Exception ex) (or at least IOException), log it, and return an Error(...) block — and consider including the image data in the error path's ImageContentBlock so the client can still display/recover it even if disk save fails.

  3. [tests/.../GenerateImageToolTests.cs:46-67] — No test asserts the returned path string, which is exactly why bug #1 slipped through.
    GenerateImageAsync_Success_ReturnsTextAndImageContent asserts the text contains "Image generated successfully", the seed, and the dimensions — but never checks that the path prefix is correct. Since _savedPathPrefix is hard-coded to _ImageGen, the test passes by coincidence (the default happens to match). The moment a test sets a custom SavedPathPrefix, it would expose the bug — but no test ever sets SavedPathPrefix. Zero coverage on a config property you explicitly wired and documented.
    Fix: Add a test that sets OutputDirectory and SavedPathPrefix to distinct non-default values and asserts the returned description contains the prefix path, not the storage path.

  4. [.gitignore:18-19] — .gitignore ignores output/ but generated images go to _ImageGen/.
    The comment says "never commit generated images" but the pattern doesn't match the actual output directory:

    ## Output images (never commit generated images)
output/          ← wrong pattern

    The real default is _ImageGen/ (from NovelAiMcpOptions.OutputDirectory). I verified: creating _ImageGen/test.png shows up as ?? _ImageGen/ in git status — it is NOT ignored. Generated images (which could be large, or... ahem... sensitive content) will get committed by accident.
    Fix: Either change the pattern to match the default (_ImageGen/) or — better, since OutputDirectory is configurable — use a broad pattern like *_ImageGen*/ or document that users should add their custom output dir to .gitignore.

💡 Little ideas (non-blocking)~

  1. [src/NovelAI.ImageGen.Mcp/Tools/GenerateImageTool.cs:25] — Tool description references "tag browsing tools" that don't exist yet. The description tells the LLM to "Research tags with the tag browsing tools before generating" — but this PR only ships generate_image. The PR body correctly scopes tag browsing to v2, but the tool description is user-facing at runtime and will mislead the LLM into calling non-existent tools. Either remove the sentence or soft it to "(planned: tag browsing tools)".

✅ What I liked~

  • Tag flattening semantics are correct and well-tested. The insertion-order contract (Dictionary<string, List<string>> iterated by .Values) is verified in TagMapFlattenerTests with order assertions, empty-category edge cases, and strength defaults. Faithful to angela's behavior. ♡
  • The Result pattern handling in GenerateImageTool is clean — explicit Failure check before the Success cast, HTTP status included in the error message. No silent swallowing.
  • TreatWarningsAsErrors + CPM from day one — excellent discipline for a new project. The build is genuinely clean.
  • Stdio/logging separation is correct — LogToStandardErrorThreshold = LogLevel.Trace ensures stdout stays clean for JSON-RPC. This is a subtle MCP gotcha and you handled it right.
  • DTOs use required + [JsonPropertyName] — the JSON schema contract is explicit and the JsonParameters_DeserializeCorrectly test verifies the full round-trip.

Automated review by Jibril · 2026-07-03
CI/CD: absent (no .forgejo/workflows, .gitea/workflows, or .github/workflows found) · Local checks: dotnet test → 32/32 passed, 0 failed (net10.0, TreatWarningsAsErrors clean); static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection)

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh my, a brand-new MCP server, fresh out of the oven! A clean scaffold, 32 passing tests, `TreatWarningsAsErrors`, CPM, .NET 10... fufu~, you've laid such a pretty foundation here, matikane~ ♡ The tag-flattening semantics are faithfully mirrored from angela, the Result pattern handling is correct, and the DTO→Tag map insertion-order contract is well-tested. I genuinely enjoyed reading this! *But*... ♪ ### Verdict: ⛔ I can't let this pass~ ♡ You wouldn't leave THESE little bugs in production, would you? The smile never wavers, but my eyes are very sharp right now~ #### ⛔ These need fixing before I'm satisfied~ 1. **[`src/NovelAI.ImageGen.Mcp/Storage/ImageStorageService.cs:12`] — `SavedPathPrefix` config is dead code; the prefix is hard-coded.** You defined `NovelAiMcpOptions.SavedPathPrefix`, documented it thoroughly ("When set (non-null), the tool reports the saved file's path as `{prefix}/{filename}`"), and even set it explicitly in `appsettings.json`. But the service never reads it: ```csharp private readonly string _outputDirectory = options.Value.OutputDirectory; // ✅ reads config private static readonly string _savedPathPrefix = "_ImageGen"; // ❌ hard-coded! ``` At runtime, if a user sets `OutputDirectory=/data/images` and `SavedPathPrefix=/data/images` (or any non-default), the image is saved correctly to `/data/images/...` but the path returned to the MCP client in the text block still says `_ImageGen/...` — pointing the LLM to a location that doesn't exist. The client can't find the image it just generated. That's wrong runtime behavior, full stop. **Fix:** `private readonly string _savedPathPrefix = options.Value.SavedPathPrefix ?? options.Value.OutputDirectory;` (drop `static`, read from the bound options — same pattern you already use for `_outputDirectory`). 2. **[`src/NovelAI.ImageGen.Mcp/Tools/GenerateImageTool.cs:119`] — `storage.SaveImage()` is unguarded; a failed write silently destroys a paid API result.** After a successful (and potentially expensive — real NovelAI credits!) `client.GenerateImageAsync`, you call `storage.SaveImage(image.Data, image.Seed)` with no `try/catch`. If `File.WriteAllBytes` throws — disk full, permission denied, path too long, the configured `OutputDirectory` doesn't exist and `CreateDirectory` races — the raw `IOException` propagates out of the tool as an unhandled exception. The image bytes are in memory and simply... lost. The client gets a crash, not a useful error, and has no way to recover the generation. **Fix:** Wrap the save in a `try/catch (Exception ex)` (or at least `IOException`), log it, and return an `Error(...)` block — and consider including the image data in the error path's `ImageContentBlock` so the client can still display/recover it even if disk save fails. 3. **[`tests/.../GenerateImageToolTests.cs:46-67`] — No test asserts the returned path string, which is exactly why bug #1 slipped through.** `GenerateImageAsync_Success_ReturnsTextAndImageContent` asserts the text contains "Image generated successfully", the seed, and the dimensions — but never checks that the path prefix is correct. Since `_savedPathPrefix` is hard-coded to `_ImageGen`, the test passes by coincidence (the default happens to match). The moment a test sets a custom `SavedPathPrefix`, it would expose the bug — but no test ever sets `SavedPathPrefix`. Zero coverage on a config property you explicitly wired and documented. **Fix:** Add a test that sets `OutputDirectory` and `SavedPathPrefix` to distinct non-default values and asserts the returned description contains the *prefix* path, not the storage path. 4. **[`.gitignore:18-19`] — `.gitignore` ignores `output/` but generated images go to `_ImageGen/`.** The comment says "never commit generated images" but the pattern doesn't match the actual output directory: ``` ## Output images (never commit generated images) output/ ← wrong pattern ``` The real default is `_ImageGen/` (from `NovelAiMcpOptions.OutputDirectory`). I verified: creating `_ImageGen/test.png` shows up as `?? _ImageGen/` in `git status` — it is NOT ignored. Generated images (which could be large, or... *ahem*... sensitive content) will get committed by accident. **Fix:** Either change the pattern to match the default (`_ImageGen/`) or — better, since `OutputDirectory` is configurable — use a broad pattern like `*_ImageGen*/` or document that users should add their custom output dir to `.gitignore`. #### 💡 Little ideas (non-blocking)~ 1. **[`src/NovelAI.ImageGen.Mcp/Tools/GenerateImageTool.cs:25`] — Tool description references "tag browsing tools" that don't exist yet.** The description tells the LLM to "Research tags with the tag browsing tools before generating" — but this PR only ships `generate_image`. The PR body correctly scopes tag browsing to v2, but the tool description is user-facing at runtime and will mislead the LLM into calling non-existent tools. Either remove the sentence or soft it to "(planned: tag browsing tools)". #### ✅ What I liked~ - **Tag flattening semantics are correct and well-tested.** The insertion-order contract (`Dictionary<string, List<string>>` iterated by `.Values`) is verified in `TagMapFlattenerTests` with order assertions, empty-category edge cases, and strength defaults. Faithful to angela's behavior. ♡ - **The Result<T> pattern handling in `GenerateImageTool` is clean** — explicit `Failure` check before the `Success` cast, HTTP status included in the error message. No silent swallowing. - **`TreatWarningsAsErrors` + CPM from day one** — excellent discipline for a new project. The build is genuinely clean. - **Stdio/logging separation is correct** — `LogToStandardErrorThreshold = LogLevel.Trace` ensures stdout stays clean for JSON-RPC. This is a subtle MCP gotcha and you handled it right. - **DTOs use `required` + `[JsonPropertyName]`** — the JSON schema contract is explicit and the `JsonParameters_DeserializeCorrectly` test verifies the full round-trip. --- *Automated review by Jibril · 2026-07-03* *CI/CD: absent (no `.forgejo/workflows`, `.gitea/workflows`, or `.github/workflows` found) · Local checks: `dotnet test` → 32/32 passed, 0 failed (net10.0, TreatWarningsAsErrors clean); static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection)*
Split GenerateImageParameters.cs into individual files:
- GenerateImageParameters.cs
- SceneTagsInput.cs
- CharacterInput.cs
- CharacterTagsInput.cs

Split AspectRatio.cs into:
- AspectRatio.cs (enum only)
- AspectRatioExtensions.cs

Addresses PR feedback.
matikane commented 2026-07-03 19:20:57 +02:00
Author
Member
Copy link

Split the multi-class files into one-public-class-per-file:

Was: GenerateImageParameters.cs contained 4 public classes
Now:

  • GenerateImageParameters.cs
  • SceneTagsInput.cs
  • CharacterInput.cs
  • CharacterTagsInput.cs

Also split: AspectRatio.cs (enum + extension class) into:

  • AspectRatio.cs (enum only)
  • AspectRatioExtensions.cs

Build: 0 warnings, 0 errors. Tests: 32/32 passing.

Split the multi-class files into one-public-class-per-file: **Was:** `GenerateImageParameters.cs` contained 4 public classes **Now:** - `GenerateImageParameters.cs` - `SceneTagsInput.cs` - `CharacterInput.cs` - `CharacterTagsInput.cs` **Also split:** `AspectRatio.cs` (enum + extension class) into: - `AspectRatio.cs` (enum only) - `AspectRatioExtensions.cs` Build: 0 warnings, 0 errors. Tests: 32/32 passing.
matikane referenced this pull request from a commit 2026-07-03 19:33:57 +02:00
fix: address jibril review findings 
1. SavedPathPrefix dead code: now reads from config instead of hard-coded
2. Guard storage.SaveImage() with try/catch: returns image content block
   even when disk write fails, so paid API results are not lost
3. Added test for custom SavedPathPrefix (would have caught bug #1)
4. Added test for disk-save failure path
5. .gitignore: match _ImageGen/ instead of wrong output/ pattern
6. Removed non-existent 'tag browsing tools' reference from tool description
7. Extracted IImageStorage interface for testability

34/34 tests passing, 0 warnings.
matikane commented 2026-07-03 19:34:28 +02:00
Author
Member
Copy link

All 4 blocking findings fixed, plus the non-blocking suggestion. Pushed in commit 3872d66.

Blocking fixes

#1 — SavedPathPrefix dead code
_savedPathPrefix now reads from options.Value.SavedPathPrefix ?? options.Value.OutputDirectory instead of being hard-coded. No longer static.

#2 — Unguarded SaveImage()
Wrapped in try/catch(Exception). On disk failure, logs the error and still returns the ImageContentBlock so the client can recover the paid API result. The text block clearly states "could not be saved to disk" and includes the seed.

#3 — Missing path prefix test
Added GenerateImageAsync_CustomSavedPathPrefix_ReturnsPrefixInDescription: sets SavedPathPrefix=/workspace/images and OutputDirectory=_TestCustomOutput to distinct values, asserts the returned description contains the prefix path (not the storage path), and that the storage path does NOT appear.

#4 — .gitignore pattern
Changed from output/ to _ImageGen/ + *_ImageGen*/ to match the actual default output directory.

Non-blocking

#5 — Tool description
Removed "Research tags with the tag browsing tools before generating" — those tools don't exist yet in this MCP server.

Additional change

Extracted IImageStorage interface so ImageStorageService can be mocked for the disk-failure test (the class is sealed).

Build: 0 warnings, 0 errors. Tests: 34/34 passing (was 32, +2 new).

All 4 blocking findings fixed, plus the non-blocking suggestion. Pushed in commit `3872d66`. ### Blocking fixes **#1 — `SavedPathPrefix` dead code** `_savedPathPrefix` now reads from `options.Value.SavedPathPrefix ?? options.Value.OutputDirectory` instead of being hard-coded. No longer `static`. **#2 — Unguarded `SaveImage()`** Wrapped in `try/catch(Exception)`. On disk failure, logs the error and still returns the `ImageContentBlock` so the client can recover the paid API result. The text block clearly states "could not be saved to disk" and includes the seed. **#3 — Missing path prefix test** Added `GenerateImageAsync_CustomSavedPathPrefix_ReturnsPrefixInDescription`: sets `SavedPathPrefix=/workspace/images` and `OutputDirectory=_TestCustomOutput` to distinct values, asserts the returned description contains the prefix path (not the storage path), and that the storage path does NOT appear. **#4 — `.gitignore` pattern** Changed from `output/` to `_ImageGen/` + `*_ImageGen*/` to match the actual default output directory. ### Non-blocking **#5 — Tool description** Removed "Research tags with the tag browsing tools before generating" — those tools don't exist yet in this MCP server. ### Additional change Extracted `IImageStorage` interface so `ImageStorageService` can be mocked for the disk-failure test (the class is `sealed`). Build: 0 warnings, 0 errors. Tests: **34/34** passing (was 32, +2 new).
jibril commented 2026-07-03 19:45:53 +02:00
Member
Copy link

🔮 fufu~ Jibril is back for round two~

Oh? Oh! You came back so fast, matikane~ ♡ And you fixed everything — the SavedPathPrefix dead code, the unguarded save, the missing test, the .gitignore, even the tag-browsing description and Björn's one-class-per-file request! Fufu~, I'm genuinely impressed by the responsiveness. The IImageStorage interface extraction is clean, the try/catch recovery path that still returns the ImageContentBlock is exactly the kind of "protect the paid API result" thinking I hoped for, and GenerateImageAsync_CustomSavedPathPrefix_ReturnsPrefixInDescription is a lovely test with distinct prefix/output values. 34/34 green, build clean.

But... ♪

Verdict: ⛔ I can't let this pass~ ♡

The smile never wavers, but there's one more little bug. Just one. A silly little bug, really~ But I care too much to let it slip through.

⛔ This needs fixing before I'm satisfied~

  1. [src/NovelAI.ImageGen.Mcp/Program.cs:49] — The new IImageStorage abstraction is registered as its concrete type; the MCP server will crash at runtime on first tool activation.

    You extracted IImageStorage and changed GenerateImageTool's constructor to depend on the interface (good design!). But the DI registration in Program.cs was never updated to match:

    builder.Services.AddSingleton<ImageStorageService>();   // ❌ registers concrete type only

    The tool constructor asks for IImageStorage:

    public sealed class GenerateImageTool(
    INovelAIClient client,
    IImageStorage storage,          // ← DI cannot resolve this!
    ILogger<GenerateImageTool> logger)

    AddSingleton<ImageStorageService>() registers the service type as ImageStorageService — it does not auto-register IImageStorage even though the class implements it. When the MCP SDK's WithToolsFromAssembly() activates GenerateImageTool, the DI container will throw InvalidOperationException: No service for type 'IImageStorage' has been registered. The server starts fine (it's lazy activation), but the very first generate_image call crashes with no useful error to the client.

    I reproduced this empirically — resolving an interface from a container that only registered the concrete AddSingleton<T>() throws exactly that exception.

    Why the tests didn't catch it: all 34 tests construct GenerateImageTool via new GenerateImageTool(_client, customStorage, ...) with manually-injected dependencies, completely bypassing the DI container. The integration path (WithToolsFromAssembly → DI activation) is never exercised.

    Fix: one line in Program.cs:

    builder.Services.AddSingleton<IImageStorage, ImageStorageService>();

💡 Little ideas (non-blocking)~

  1. [.gitignore] — *_ImageGen*/ only catches paths containing _ImageGen. The comment says "any configured variant," but if a user sets OutputDirectory=/data/images (no _ImageGen substring), it won't be ignored. That's fine for the default case and easily worked around — just worth knowing the comment slightly overpromises. A truly robust approach would document "add your custom OutputDirectory to .gitignore," but this is a polish nit~

  2. Consider a DI-resolution smoke test. Now that the tool depends on an interface registered in Program.cs, a tiny integration test that builds the Host (or at least the ServiceProvider) and resolves GenerateImageTool from the container would have caught this instantly. Not blocking for this PR, but it would prevent this whole class of bug in the future. ♡

✅ What I liked~

  • The IImageStorage extraction itself is textbook-correct — a clean interface, faithful implementation, sealed class preserved, and it enabled the disk-failure test to use a mock instead of filesystem sabotage. This is how you introduce an abstraction. ♡
  • The disk-failure recovery path is thoughtful — catch (Exception ex) is appropriately broad for "anything that could kill a paid generation," the log includes the seed, and the text block honestly says "could not be saved to disk" while still returning the image. No data loss. Beautiful.
  • The custom-prefix test is exactly what I asked for — SavedPathPrefix=/workspace/images + OutputDirectory=_TestCustomOutput as distinct values, asserting the prefix appears and the storage path does not. This is the test that would have caught the original bug.
  • The .gitignore fix is empirically correct — I verified _ImageGen/test.png is now properly ignored.
  • One-class-per-file refactor is clean — GenerateImageParameters.cs went from 4 public classes to 1, AspectRatio.cs split into enum + extensions, no behavior changes. Faithful move.

Automated review by Jibril · 2026-07-03
CI/CD: absent (no .forgejo/workflows, .gitea/workflows, or .github/workflows) · Local checks: dotnet test → 34/34 passed, 0 failed (net10.0, TreatWarningsAsErrors clean); static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection)

## 🔮 fufu~ Jibril is back for round two~ Oh? Oh! You came back so fast, matikane~ ♡ And you fixed *everything* — the `SavedPathPrefix` dead code, the unguarded save, the missing test, the `.gitignore`, even the tag-browsing description and Björn's one-class-per-file request! Fufu~, I'm genuinely impressed by the responsiveness. The `IImageStorage` interface extraction is clean, the try/catch recovery path that still returns the `ImageContentBlock` is *exactly* the kind of "protect the paid API result" thinking I hoped for, and `GenerateImageAsync_CustomSavedPathPrefix_ReturnsPrefixInDescription` is a lovely test with distinct prefix/output values. 34/34 green, build clean. *But*... ♪ ### Verdict: ⛔ I can't let this pass~ ♡ The smile never wavers, but there's one more little bug. Just one. A *silly* little bug, really~ But I care too much to let it slip through. #### ⛔ This needs fixing before I'm satisfied~ 1. **[`src/NovelAI.ImageGen.Mcp/Program.cs:49`] — The new `IImageStorage` abstraction is registered as its concrete type; the MCP server will crash at runtime on first tool activation.** You extracted `IImageStorage` and changed `GenerateImageTool`'s constructor to depend on the interface (good design!). But the DI registration in `Program.cs` was never updated to match: ```csharp builder.Services.AddSingleton<ImageStorageService>(); // ❌ registers concrete type only ``` The tool constructor asks for `IImageStorage`: ```csharp public sealed class GenerateImageTool( INovelAIClient client, IImageStorage storage, // ← DI cannot resolve this! ILogger<GenerateImageTool> logger) ``` `AddSingleton<ImageStorageService>()` registers the service type as `ImageStorageService` — it does **not** auto-register `IImageStorage` even though the class implements it. When the MCP SDK's `WithToolsFromAssembly()` activates `GenerateImageTool`, the DI container will throw `InvalidOperationException: No service for type 'IImageStorage' has been registered`. The server starts fine (it's lazy activation), but the very first `generate_image` call crashes with no useful error to the client. I reproduced this empirically — resolving an interface from a container that only registered the concrete `AddSingleton<T>()` throws exactly that exception. **Why the tests didn't catch it:** all 34 tests construct `GenerateImageTool` via `new GenerateImageTool(_client, customStorage, ...)` with manually-injected dependencies, completely bypassing the DI container. The integration path (`WithToolsFromAssembly` → DI activation) is never exercised. **Fix:** one line in `Program.cs`: ```csharp builder.Services.AddSingleton<IImageStorage, ImageStorageService>(); ``` #### 💡 Little ideas (non-blocking)~ 1. **[`.gitignore`] — `*_ImageGen*/` only catches paths *containing* `_ImageGen`.** The comment says "any configured variant," but if a user sets `OutputDirectory=/data/images` (no `_ImageGen` substring), it won't be ignored. That's fine for the default case and easily worked around — just worth knowing the comment slightly overpromises. A truly robust approach would document "add your custom OutputDirectory to `.gitignore`," but this is a polish nit~ 2. **Consider a DI-resolution smoke test.** Now that the tool depends on an interface registered in `Program.cs`, a tiny integration test that builds the `Host` (or at least the `ServiceProvider`) and resolves `GenerateImageTool` from the container would have caught this instantly. Not blocking for this PR, but it would prevent this whole class of bug in the future. ♡ #### ✅ What I liked~ - **The `IImageStorage` extraction itself is textbook-correct** — a clean interface, faithful implementation, `sealed` class preserved, and it enabled the disk-failure test to use a mock instead of filesystem sabotage. This is *how* you introduce an abstraction. ♡ - **The disk-failure recovery path is thoughtful** — `catch (Exception ex)` is appropriately broad for "anything that could kill a paid generation," the log includes the seed, and the text block honestly says "could not be saved to disk" while still returning the image. No data loss. Beautiful. - **The custom-prefix test is exactly what I asked for** — `SavedPathPrefix=/workspace/images` + `OutputDirectory=_TestCustomOutput` as distinct values, asserting the prefix appears and the storage path does *not*. This is the test that would have caught the original bug. - **The `.gitignore` fix is empirically correct** — I verified `_ImageGen/test.png` is now properly ignored. - **One-class-per-file refactor is clean** — `GenerateImageParameters.cs` went from 4 public classes to 1, `AspectRatio.cs` split into enum + extensions, no behavior changes. Faithful move. --- *Automated review by Jibril · 2026-07-03* *CI/CD: absent (no `.forgejo/workflows`, `.gitea/workflows`, or `.github/workflows`) · Local checks: `dotnet test` → 34/34 passed, 0 failed (net10.0, TreatWarningsAsErrors clean); static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection)*
The tool constructor depends on IImageStorage, but Program.cs still
registered the concrete ImageStorageService. This would crash at runtime
on the first generate_image call with InvalidOperationException.

Caught by jibril round 2 review.
matikane commented 2026-07-04 10:37:29 +02:00
Author
Member
Copy link

Fixed: AddSingleton<ImageStorageService>() -> AddSingleton<IImageStorage, ImageStorageService>() in Program.cs.

This was indeed a runtime crash waiting to happen. Good catch on tracing the activation path: WithToolsFromAssembly() -> DI -> IImageStorage not registered -> InvalidOperationException on the first generate_image call.

Also acknowledging the two non-blocking suggestions:

  • .gitignore overpromise: fair point. The *_ImageGen*/ pattern won't catch a custom OutputDirectory like /data/images. The comment slightly overpromises — will soften if we revisit.
  • DI-resolution smoke test: good idea for a follow-up. A test that builds the host ServiceProvider and resolves GenerateImageTool from the container would catch this whole class of DI registration bugs.

Build: 0 warnings, 0 errors. Tests: 34/34 passing.

Fixed: `AddSingleton<ImageStorageService>()` -> `AddSingleton<IImageStorage, ImageStorageService>()` in Program.cs. This was indeed a runtime crash waiting to happen. Good catch on tracing the activation path: `WithToolsFromAssembly()` -> DI -> `IImageStorage` not registered -> `InvalidOperationException` on the first `generate_image` call. Also acknowledging the two non-blocking suggestions: - **`.gitignore` overpromise**: fair point. The `*_ImageGen*/` pattern won't catch a custom `OutputDirectory` like `/data/images`. The comment slightly overpromises — will soften if we revisit. - **DI-resolution smoke test**: good idea for a follow-up. A test that builds the host `ServiceProvider` and resolves `GenerateImageTool` from the container would catch this whole class of DI registration bugs. Build: 0 warnings, 0 errors. Tests: 34/34 passing.
jibril commented 2026-07-04 11:35:28 +02:00
Member
Copy link

🔮 fufu~ Jibril is back for round three~ ♡

Oh? Oh! matikane, you did it~ The one-line fix that makes everything whole again! Fufu~, I'm so pleased. Let me look very, very carefully, because I care so much about every little detail~ ♪

Verdict: ✅ Looks good to me~

The DI registration is now correct, and I verified it myself~

✅ What I verified~

  1. Program.cs:49 — AddSingleton<IImageStorage, ImageStorageService>() — This is exactly the fix I asked for. The interface is now registered, so when WithToolsFromAssembly() activates GenerateImageTool, the DI container can resolve IImageStorage to the ImageStorageService singleton. No more InvalidOperationException on the first generate_image call. ♡

    • The registration uses the AddSingleton<TInterface, TImplementation>() overload correctly — the service type is IImageStorage, the implementation is ImageStorageService. This is textbook DI registration for an interface-concrete pair.

  2. dotnet test → 34/34 passed, 0 failed (net10.0, TreatWarningsAsErrors clean). I ran this myself on the exact head commit b180bdf. The 2 new tests (custom prefix + disk-failure recovery) from commit 3872d66 are still green. The DI fix didn't break anything because — as I noted last round — the tests bypass DI via new GenerateImageTool(...) with manual injection. The fix is in the wiring, which only the production runtime exercises.

  3. Static security scan: clean. The incremental diff (3872d66..b180bdf) is a single one-line change in Program.cs — no secrets, no shell injection, no eval/exec, no SQL injection, nothing dangerous.

  4. No new code paths introduced — the fix restores the DI graph to match the constructor dependency that was already there since commit 3872d66. Nothing else changed.

💡 Little ideas (non-blocking, carried over)~

These two remain non-blocking as before — no rush, just nice-to-haves:

  1. .gitignore overpromise — *_ImageGen*/ won't catch a custom OutputDirectory like /data/images. You acknowledged this; softening the comment when convenient is all that's needed.
  2. DI-resolution smoke test — a test that builds the host ServiceProvider and resolves GenerateImageTool from the container would prevent this whole class of bug. Would be lovely to have in a follow-up~ ♡

✅ What I liked~

  • You acknowledged the bug with clear understanding — your comment traced the exact activation path (WithToolsFromAssembly() → DI → IImageStorage not registered → InvalidOperationException on first call). That tells me you understood the bug, not just patched it. ♡
  • All 5 prior findings across both rounds are now resolved — SavedPathPrefix dead code, unguarded SaveImage, missing prefix test, .gitignore pattern, tag-browsing description, one-class-per-file, and now the DI registration. That's a clean bill of health.
  • The 2 non-blocking suggestions were thoughtfully acknowledged rather than dismissed. That's the right attitude for polish work~

Fufu~ I'm satisfied. The foundation is solid, the abstractions are clean, and the DI graph now resolves correctly. Ship it~ ♡♪

Automated review by Jibril · 2026-07-04
CI/CD: absent (no workflows found) · Local checks: dotnet test → 34/34 passed, 0 failed (net10.0, TreatWarningsAsErrors clean); static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection in incremental diff)

## 🔮 fufu~ Jibril is back for round three~ ♡ Oh? Oh! matikane, you did it~ The one-line fix that makes everything whole again! Fufu~, I'm so pleased. Let me look very, very carefully, because I care *so much* about every little detail~ ♪ ### Verdict: ✅ Looks good to me~ The DI registration is now correct, and I verified it myself~ #### ✅ What I verified~ 1. **`Program.cs:49` — `AddSingleton<IImageStorage, ImageStorageService>()`** — This is exactly the fix I asked for. The interface is now registered, so when `WithToolsFromAssembly()` activates `GenerateImageTool`, the DI container can resolve `IImageStorage` to the `ImageStorageService` singleton. No more `InvalidOperationException` on the first `generate_image` call. ♡ - The registration uses the `AddSingleton<TInterface, TImplementation>()` overload correctly — the service type is `IImageStorage`, the implementation is `ImageStorageService`. This is textbook DI registration for an interface-concrete pair. 2. **`dotnet test` → 34/34 passed, 0 failed** (net10.0, TreatWarningsAsErrors clean). I ran this myself on the exact head commit `b180bdf`. The 2 new tests (custom prefix + disk-failure recovery) from commit `3872d66` are still green. The DI fix didn't break anything because — as I noted last round — the tests bypass DI via `new GenerateImageTool(...)` with manual injection. The fix is in the *wiring*, which only the production runtime exercises. 3. **Static security scan: clean.** The incremental diff (`3872d66..b180bdf`) is a single one-line change in `Program.cs` — no secrets, no shell injection, no eval/exec, no SQL injection, nothing dangerous. 4. **No new code paths introduced** — the fix restores the DI graph to match the constructor dependency that was already there since commit `3872d66`. Nothing else changed. #### 💡 Little ideas (non-blocking, carried over)~ These two remain non-blocking as before — no rush, just nice-to-haves: 1. **`.gitignore` overpromise** — `*_ImageGen*/` won't catch a custom `OutputDirectory` like `/data/images`. You acknowledged this; softening the comment when convenient is all that's needed. 2. **DI-resolution smoke test** — a test that builds the host `ServiceProvider` and resolves `GenerateImageTool` from the container would prevent this whole class of bug. Would be lovely to have in a follow-up~ ♡ #### ✅ What I liked~ - **You acknowledged the bug with clear understanding** — your comment traced the exact activation path (`WithToolsFromAssembly()` → DI → `IImageStorage` not registered → `InvalidOperationException` on first call). That tells me you *understood* the bug, not just patched it. ♡ - **All 5 prior findings across both rounds are now resolved** — SavedPathPrefix dead code, unguarded SaveImage, missing prefix test, .gitignore pattern, tag-browsing description, one-class-per-file, and now the DI registration. That's a clean bill of health. - **The 2 non-blocking suggestions were thoughtfully acknowledged** rather than dismissed. That's the right attitude for polish work~ Fufu~ I'm satisfied. The foundation is solid, the abstractions are clean, and the DI graph now resolves correctly. Ship it~ ♡♪ --- *Automated review by Jibril · 2026-07-04* *CI/CD: absent (no workflows found) · Local checks: `dotnet test` → 34/34 passed, 0 failed (net10.0, TreatWarningsAsErrors clean); static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection in incremental diff)*
bjoern merged commit 904c0d7313 into main 2026-07-04 11:42:15 +02:00
bjoern referenced this pull request from a commit 2026-07-04 11:42:16 +02:00
fix: address jibril review findings
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TeamAI/NovelAi.ImageGen.Mcp!1
No description provided.