TeamAI/NovelAi.ImageGen.Mcp

Fork 0

feat(draft): tag browsing tools (v2) #3

Merged

bjoern merged 2 commits from feat/tag-browsing-tools into main

2026-07-05 15:11:35 +02:00

matikane commented

2026-07-05 13:10:32 +02:00

Member

Summary

Ports the 5 tag browsing tools from booru_tag_db_dart (Dart) to the MCP server (C#), backed by the SQLite tag database.

Status: DRAFT — waiting for the updated tag database .db file. Once it ships, bump the externals/booru_tag_db_dart submodule pointer and configure the TagDatabasePath.

New tools

Tool	Purpose
`search_tags`	FTS5 keyword search across name, aliases, and description (category + rating filters)
`browse_category`	List all categories with counts, or paginate tags within a category
`get_tag_details`	Full tag info: display name, description, category, post count, aliases, related tags, implications
`suggest_tags`	Given current tags, suggest complementary ones (related + implications + semantic neighbors)
`find_similar_tags`	Semantic similarity via precomputed embedding neighbors

Architecture

externals/booru_tag_db_dart — git submodule (schema reference + tool contract source of truth)
Tags/TagDatabase.cs — read-only C# query layer using Microsoft.Data.Sqlite, same SQL as the Dart version
Tags/Models/ — ported model types: TagCategory, CustomCategory (55 enum values + snake_case JSON converter), Rating, CategorizedTag
Tags/TagBrowsingTools.cs — 5 MCP tools in one [McpServerToolType] class

Conditional registration

Tag tools are only registered when McpServer:TagDatabasePath config is set AND the file exists. Without it, only generate_image is available — fully backward compatible.

Dependencies

Microsoft.Data.Sqlite 10.0.9
SQLitePCLRaw.bundle_e_sqlite3 3.0.3 (pinned — security vulnerability in the 2.1.x transitive dependency that Microsoft.Data.Sqlite pulls by default)

Build

0 warnings, 0 errors. Existing 34 tests still passing. (Tag database tests will be added once the .db file is available.)

What's needed before merge

Updated .db file from the booru_tag_db_dart pipeline
Submodule bump
Config the TagDatabasePath in appsettings.json or via env var

## Summary Ports the 5 tag browsing tools from `booru_tag_db_dart` (Dart) to the MCP server (C#), backed by the SQLite tag database. **Status: DRAFT** — waiting for the updated tag database `.db` file. Once it ships, bump the `externals/booru_tag_db_dart` submodule pointer and configure the `TagDatabasePath`. ## New tools | Tool | Purpose | |---|---| | `search_tags` | FTS5 keyword search across name, aliases, and description (category + rating filters) | | `browse_category` | List all categories with counts, or paginate tags within a category | | `get_tag_details` | Full tag info: display name, description, category, post count, aliases, related tags, implications | | `suggest_tags` | Given current tags, suggest complementary ones (related + implications + semantic neighbors) | | `find_similar_tags` | Semantic similarity via precomputed embedding neighbors | ## Architecture - **`externals/booru_tag_db_dart`** — git submodule (schema reference + tool contract source of truth) - **`Tags/TagDatabase.cs`** — read-only C# query layer using `Microsoft.Data.Sqlite`, same SQL as the Dart version - **`Tags/Models/`** — ported model types: `TagCategory`, `CustomCategory` (55 enum values + snake_case JSON converter), `Rating`, `CategorizedTag` - **`Tags/TagBrowsingTools.cs`** — 5 MCP tools in one `[McpServerToolType]` class ## Conditional registration Tag tools are only registered when `McpServer:TagDatabasePath` config is set AND the file exists. Without it, only `generate_image` is available — fully backward compatible. ## Dependencies - `Microsoft.Data.Sqlite` 10.0.9 - `SQLitePCLRaw.bundle_e_sqlite3` 3.0.3 (pinned — security vulnerability in the 2.1.x transitive dependency that `Microsoft.Data.Sqlite` pulls by default) ## Build 0 warnings, 0 errors. Existing 34 tests still passing. (Tag database tests will be added once the `.db` file is available.) ## What's needed before merge 1. Updated `.db` file from the booru_tag_db_dart pipeline 2. Submodule bump 3. Config the `TagDatabasePath` in `appsettings.json` or via env var

matikane added 1 commit

2026-07-05 13:10:32 +02:00

feat(draft): tag browsing tools (v2) ported from booru_tag_db_dart 34c71ce048

Adds 5 MCP tools for browsing the booru tag database:

- search_tags: FTS5 keyword search across name/aliases/description
- browse_category: list categories or paginate tags within a category
- get_tag_details: full tag info (aliases, implications, related)
- suggest_tags: complementary tag suggestions (related + implications + embeddings)
- find_similar_tags: semantic neighbors from precomputed embeddings

Adds booru_tag_db_dart as git submodule (externals/booru_tag_db_dart).
The SQLite .db file is the data source; this is read-only access via
Microsoft.Data.Sqlite.

Model types ported from Dart to C#:
- TagCategory, CustomCategory (55 enum values + snake_case converter)
- Rating (safe/questionable/explicit)
- CategorizedTag (name, display name, description, categories, aliases, etc.)
- TagDatabase (read-only query layer, same SQL as the Dart version)

Tag tools are conditionally registered when TagDatabasePath config is set
and the file exists. Without a configured path, only generate_image is
available (backward compatible).

DRAFT: waiting for the updated tag database .db file, then submodule bump.
SQLitePCLRaw.bundle_e_sqlite3 pinned to 3.0.3 (security vulnerability in
the 2.1.x transitive dependency).

jibril commented

2026-07-05 13:32:55 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A whole tag-browsing subsystem ported from Dart to C#, 5 shiny new MCP tools, FTS5 search, semantic neighbors, implication graphs... fufu~ you've been busy, haven't you? The architecture is clean, the snake_case converters are delightful, and the SQL queries are faithfully translated. I can tell you studied the Dart source carefully. ♡

But... leans in with a smile that doesn't reach the eyes ...you left some things behind in the port. And some new things crept in that will bite at runtime. Let me show you each one~

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

⛔ These need fixing before I'm satisfied~

[Program.cs:53-64 + TagBrowsingTools.cs:15] — The conditional registration is broken. The "fully backward compatible" claim is false.

The PR says: "Tag tools are only registered when McpServer:TagDatabasePath config is set AND the file exists. Without it, only generate_image is available — fully backward compatible."

This is not what happens. WithToolsFromAssembly() scans the assembly for all [McpServerToolType] classes and auto-registers them — regardless of your if block. We know this for a fact because GenerateImageTool has no explicit registration anywhere in Program.cs, yet it works perfectly. The TagBrowsingTools class is decorated with [McpServerToolType], so the SDK will discover and register it even when TagDatabasePath is null.

Result when the database isn't configured:
- The 5 tag tools still appear in tools/list (misleading — the client thinks they're available)
- Calling any of them throws a DI resolution exception because TagDatabase isn't registered
The if guard around AddSingleton<TagDatabase>() is correct, but it doesn't prevent tool discovery. You need a different approach — e.g., move [McpServerToolType] behind a compile-time condition, or use a conditional WithTools that only adds specific tool types when the DB exists, instead of the blanket WithToolsFromAssembly().

Fix: Either split tool registration (register generate_image always; register tag tools only when the DB exists via a conditional WithTools(...) call instead of WithToolsFromAssembly()), or mark the tools as no-ops when the database isn't available.
[TagDatabase.cs — entire class] — Missing feature-detection for optional tables. The Dart source of truth has it; the port dropped it.

The Dart TagDatabase (the contract source of truth, per your own PR description) checks whether optional tables exist before querying them:
```
// Dart (tag_database.dart:16-23, 97, 109, 124, 144)
final bool _hasAliasTable = _tableExists(_db, 'tag_aliases');
final bool _hasImplicationsTable = _tableExists(_db, 'implications');
final bool _hasSimilarTable = _tableExists(_db, 'similar_tags');

String? resolveAlias(String alias) {
  if (!_hasAliasTable) return null;   // ← graceful
  ...
}
```
The C# port has zero table-existence checks. Every method (ResolveAlias, GetImplications, GetImpliedBy, FindSimilar) directly executes SELECT ... FROM tag_aliases / implications / similar_tags. If the database file was exported without these tables (which the PR explicitly acknowledges is a possibility — "waiting for the updated .db file"), these queries throw SqliteException: no such table instead of returning null/empty.

GetTag() is the worst case: it calls ResolveAlias() on every miss, so any alias lookup against a DB without the tag_aliases table crashes. And suggest_tags calls GetImplications + FindSimilar on every input tag.

Fix: Add the same _tableExists checks (query sqlite_master) and gate each optional-table query behind them, exactly as the Dart version does.
[TagBrowsingTools.cs:20 + all 5 tool methods] — _logger is injected and stored but never used. No error handling anywhere.
```
private readonly ILogger<TagBrowsingTools> _logger = logger;  // ← never referenced
```
Combined with issue #2 above, this is dangerous: when a missing-table exception (or malformed JSON in aliases, or corrupt FTS index) is thrown, it propagates as a raw MCP error with no logging, no graceful degradation. The injected logger should be catching and logging these, then returning a user-friendly error string.

Fix: Wrap each tool method body in a try/catch, log the exception with _logger.LogError(ex, ...), and return a readable error message. Or at minimum, remove the dead _logger field so it's not misleading.
[tests/ — absent] — 997 lines of new logic, zero tests.

Five MCP tools, a SQLite query layer, FTS5 escaping, two JSON converters with 55+ enum mappings, pagination, rating filtering, suggestion scoring... and not a single test. The PR says "Tag database tests will be added once the .db file is available" — but much of this is unit-testable right now without a production .db:
- CustomCategoryConverter / RatingConverter round-trip (no DB needed)
- EscapeFts5Query — pure string function (no DB needed)
- TagDatabase with an in-memory SQLite DB built from test data (the Dart test suite test/tag_database_test.dart does exactly this — port it)
- BrowseCategory pagination edge cases (see issue #5)
- SuggestTags scoring logic
The Dart sibling has test/tag_database_test.dart. The C# port should have its equivalent. fufu~ you added 5 tools but forgot to test any of them? I can't let that slide~ ♡
[TagBrowsingTools.cs:107] — BrowseCategory offset clamping bug.
```
var page = filtered
    .Skip(Math.Clamp(offset, 0, Math.Max(0, total - 1)))  // ← clamps to total-1
    .Take(Math.Clamp(limit, 1, 50))
    .ToList();
```
When offset >= total (user requests a page past the end), this clamps to total - 1 and returns the last tag — instead of returning an empty page with the "No tags found" message.

Compare to the Dart sibling (which clamps to total, not total - 1):
```
final offset = params.offset.clamp(0, total);  // ← total, not total-1
```
Example: category has 5 tags, user calls browse_category(offset=5).
- Dart: offset = 5, skip 5 of 5 → empty → "No tags found in category ... at offset 5." ✓
- C#: offset = 4, skip 4 of 5 → returns tag #5 → wrong result ✗
Fix: Change Math.Max(0, total - 1) to just total (and handle total == 0 where Skip(total) correctly yields empty).

💡 Little ideas (non-blocking)~

[TagBrowsingTools.cs:101] — GetByCategory(cat, offset: 0, limit: int.MaxValue) loads the entire category into memory. The C# GetByCategory already supports SQL-level OFFSET/LIMIT (unlike the Dart version), but BrowseCategory ignores it and does pagination in C#. Since you have the SQL pagination infrastructure, you could push the rating filter + pagination into SQL. Faithful to the Dart for now, but you have a chance to do better here. ♪
[Program.cs:57] — AddSingleton<TagBrowsingTools>() is redundant. WithToolsFromAssembly() auto-registers [McpServerToolType] classes (proven by GenerateImageTool, which has no explicit registration). The explicit AddSingleton just overrides the SDK's default transient lifetime. Harmless, but dead code unless the singleton lifetime is intentional.

✅ What I liked~

The snake_case JSON converter for CustomCategory is meticulous — all 55 values mapped, with a clean bidirectional dictionary + fallback to Uncategorized. Love it. ♡
FTS5 query escaping (EscapeFts5Query) is faithfully ported, including the "" doubling and prefix-match * suffix.
The Search SQL with the relevance CASE expression (exact > partial > FTS) is a faithful and clever translation of the Dart query.
The Rating.AllowedRatings + dynamic placeholder approach for parameterized IN (...) clauses is clean and injection-safe. All SQL uses parameterized queries — no string interpolation of user input. Good security hygiene! ♡
Conditional registration concept (even if the implementation is broken) shows the right instinct for backward compatibility.
The CategorizedTag model with required + init properties is idiomatic modern C#.

Fix the 5 blocking issues and I'll be happy to re-review~ fufu~ ♡

Automated review by Jibril · 2026-07-05
CI/CD: absent (no CI config in repo) · Local checks: not run (no .NET SDK available in review environment; build claim "0 warnings, 0 errors" accepted from PR description pending CI)

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A whole tag-browsing subsystem ported from Dart to C#, 5 shiny new MCP tools, FTS5 search, semantic neighbors, implication graphs... fufu~ you've been busy, haven't you? The architecture is clean, the snake_case converters are delightful, and the SQL queries are faithfully translated. I can tell you studied the Dart source carefully. ♡ But... *leans in with a smile that doesn't reach the eyes* ...you left some things behind in the port. And some new things crept in that will bite at runtime. Let me show you each one~ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **[Program.cs:53-64 + TagBrowsingTools.cs:15] — The conditional registration is broken. The "fully backward compatible" claim is false.** The PR says: *"Tag tools are only registered when `McpServer:TagDatabasePath` config is set AND the file exists. Without it, only `generate_image` is available — fully backward compatible."* This is not what happens. `WithToolsFromAssembly()` scans the assembly for **all** `[McpServerToolType]` classes and auto-registers them — regardless of your `if` block. We know this for a fact because `GenerateImageTool` has **no explicit registration** anywhere in `Program.cs`, yet it works perfectly. The `TagBrowsingTools` class is decorated with `[McpServerToolType]`, so the SDK will discover and register it even when `TagDatabasePath` is null. Result when the database isn't configured: - The 5 tag tools **still appear** in `tools/list` (misleading — the client thinks they're available) - Calling any of them throws a DI resolution exception because `TagDatabase` isn't registered The `if` guard around `AddSingleton<TagDatabase>()` is correct, but it doesn't prevent tool discovery. You need a different approach — e.g., move `[McpServerToolType]` behind a compile-time condition, or use a conditional `WithTools` that only adds specific tool types when the DB exists, instead of the blanket `WithToolsFromAssembly()`. **Fix:** Either split tool registration (register `generate_image` always; register tag tools only when the DB exists via a conditional `WithTools(...)` call instead of `WithToolsFromAssembly()`), or mark the tools as no-ops when the database isn't available. 2. **[TagDatabase.cs — entire class] — Missing feature-detection for optional tables. The Dart source of truth has it; the port dropped it.** The Dart `TagDatabase` (the contract source of truth, per your own PR description) checks whether optional tables exist before querying them: ```dart // Dart (tag_database.dart:16-23, 97, 109, 124, 144) final bool _hasAliasTable = _tableExists(_db, 'tag_aliases'); final bool _hasImplicationsTable = _tableExists(_db, 'implications'); final bool _hasSimilarTable = _tableExists(_db, 'similar_tags'); String? resolveAlias(String alias) { if (!_hasAliasTable) return null; // ← graceful ... } ``` The C# port has **zero** table-existence checks. Every method (`ResolveAlias`, `GetImplications`, `GetImpliedBy`, `FindSimilar`) directly executes `SELECT ... FROM tag_aliases / implications / similar_tags`. If the database file was exported without these tables (which the PR explicitly acknowledges is a possibility — "waiting for the updated `.db` file"), these queries throw `SqliteException: no such table` instead of returning null/empty. `GetTag()` is the worst case: it calls `ResolveAlias()` on every miss, so any alias lookup against a DB without the `tag_aliases` table crashes. And `suggest_tags` calls `GetImplications` + `FindSimilar` on every input tag. **Fix:** Add the same `_tableExists` checks (query `sqlite_master`) and gate each optional-table query behind them, exactly as the Dart version does. 3. **[TagBrowsingTools.cs:20 + all 5 tool methods] — `_logger` is injected and stored but never used. No error handling anywhere.** ```csharp private readonly ILogger<TagBrowsingTools> _logger = logger; // ← never referenced ``` Combined with issue #2 above, this is dangerous: when a missing-table exception (or malformed JSON in `aliases`, or corrupt FTS index) is thrown, it propagates as a raw MCP error with no logging, no graceful degradation. The injected logger should be catching and logging these, then returning a user-friendly error string. **Fix:** Wrap each tool method body in a try/catch, log the exception with `_logger.LogError(ex, ...)`, and return a readable error message. Or at minimum, remove the dead `_logger` field so it's not misleading. 4. **[tests/ — absent] — 997 lines of new logic, zero tests.** Five MCP tools, a SQLite query layer, FTS5 escaping, two JSON converters with 55+ enum mappings, pagination, rating filtering, suggestion scoring... and not a single test. The PR says "Tag database tests will be added once the `.db` file is available" — but much of this is unit-testable right now without a production `.db`: - `CustomCategoryConverter` / `RatingConverter` round-trip (no DB needed) - `EscapeFts5Query` — pure string function (no DB needed) - `TagDatabase` with an in-memory SQLite DB built from test data (the Dart test suite `test/tag_database_test.dart` does exactly this — port it) - `BrowseCategory` pagination edge cases (see issue #5) - `SuggestTags` scoring logic The Dart sibling has `test/tag_database_test.dart`. The C# port should have its equivalent. **fufu~ you added 5 tools but forgot to test any of them? I can't let that slide~ ♡** 5. **[TagBrowsingTools.cs:107] — `BrowseCategory` offset clamping bug.** ```csharp var page = filtered .Skip(Math.Clamp(offset, 0, Math.Max(0, total - 1))) // ← clamps to total-1 .Take(Math.Clamp(limit, 1, 50)) .ToList(); ``` When `offset >= total` (user requests a page past the end), this clamps to `total - 1` and returns the **last tag** — instead of returning an empty page with the "No tags found" message. Compare to the Dart sibling (which clamps to `total`, not `total - 1`): ```dart final offset = params.offset.clamp(0, total); // ← total, not total-1 ``` Example: category has 5 tags, user calls `browse_category(offset=5)`. - **Dart:** offset = 5, skip 5 of 5 → empty → `"No tags found in category ... at offset 5."` ✓ - **C#:** offset = 4, skip 4 of 5 → returns tag #5 → wrong result ✗ **Fix:** Change `Math.Max(0, total - 1)` to just `total` (and handle `total == 0` where `Skip(total)` correctly yields empty). #### 💡 Little ideas (non-blocking)~ 1. **[TagBrowsingTools.cs:101] — `GetByCategory(cat, offset: 0, limit: int.MaxValue)` loads the entire category into memory.** The C# `GetByCategory` already supports SQL-level `OFFSET`/`LIMIT` (unlike the Dart version), but `BrowseCategory` ignores it and does pagination in C#. Since you have the SQL pagination infrastructure, you could push the rating filter + pagination into SQL. Faithful to the Dart for now, but you have a chance to do better here. ♪ 2. **[Program.cs:57] — `AddSingleton<TagBrowsingTools>()` is redundant.** `WithToolsFromAssembly()` auto-registers `[McpServerToolType]` classes (proven by `GenerateImageTool`, which has no explicit registration). The explicit `AddSingleton` just overrides the SDK's default transient lifetime. Harmless, but dead code unless the singleton lifetime is intentional. #### ✅ What I liked~ - The snake_case JSON converter for `CustomCategory` is meticulous — all 55 values mapped, with a clean bidirectional dictionary + fallback to `Uncategorized`. Love it. ♡ - FTS5 query escaping (`EscapeFts5Query`) is faithfully ported, including the `""` doubling and prefix-match `*` suffix. - The `Search` SQL with the `relevance` CASE expression (exact > partial > FTS) is a faithful and clever translation of the Dart query. - The `Rating.AllowedRatings` + dynamic placeholder approach for parameterized `IN (...)` clauses is clean and injection-safe. All SQL uses parameterized queries — no string interpolation of user input. Good security hygiene! ♡ - Conditional registration *concept* (even if the implementation is broken) shows the right instinct for backward compatibility. - The `CategorizedTag` model with `required` + `init` properties is idiomatic modern C#. Fix the 5 blocking issues and I'll be happy to re-review~ fufu~ ♡ --- *Automated review by Jibril · 2026-07-05* *CI/CD: absent (no CI config in repo) · Local checks: not run (no .NET SDK available in review environment; build claim "0 warnings, 0 errors" accepted from PR description pending CI)*

matikane added 1 commit

2026-07-05 14:01:54 +02:00

fix: address jibril review findings on tag browsing tools 08608b9a36

1. Conditional registration: replaced WithToolsFromAssembly() with
   explicit WithTools<GenerateImageTool>() + conditional
   WithTools<TagBrowsingTools>(). Tag tools now genuinely don't appear
   when no DB is configured.
2. Table-existence checks: added _hasAliasTable, _hasImplicationsTable,
   _hasSimilarTable flags (query sqlite_master on construction). All
   optional-table methods gracefully return null/empty.
3. Error handling: all 5 tool methods wrapped in try/catch, _logger
   used for LogError with structured params, returns user-friendly errors.
4. BrowseCategory offset bug: Math.Max(0, total-1) -> total.
5. 129 new unit tests: CustomCategoryConverter (55 round-trips),
   RatingConverter, TagDatabase (in-memory SQLite, feature-detection,
   search/aliases/implications/similar/pagination), BrowseCategory
   offset edge cases.

163/163 tests passing.

matikane commented

2026-07-05 14:03:34 +02:00

Author

Member

All 5 blocking findings fixed. Pushed in commit 08608b9.

#1 — Conditional registration (broken)

Replaced WithToolsFromAssembly() with explicit WithTools<GenerateImageTool>() (always) + conditional WithTools<TagBrowsingTools>() (only when DB exists). Tag tools now genuinely don't appear in tools/list when no database is configured.

#2 — Missing table-existence checks

Added _hasAliasTable, _hasImplicationsTable, _hasSimilarTable flags via TableExists("...") check on construction (queries sqlite_master). All optional-table methods (ResolveAlias, GetImplications, GetImpliedBy, FindSimilar) now gracefully return null/[] instead of throwing SqliteException when tables are absent.

#3 — Dead `_logger` + no error handling

All 5 tool methods are now instance methods (were mixed static/instance)
Each wrapped in try/catch(Exception) with _logger.LogError(ex, ...) + structured parameters
Returns user-friendly error strings on any exception

#4 — Zero tests

Added 129 new unit tests (TagBrowsingTests.cs):

CustomCategoryConverter: round-trips all 55 enum values via [MemberData], fallback for null/empty/unknown
RatingConverter: round-trip, AllowedRatings subsets, IsAtMost
TagDatabase (in-memory SQLite): GetTag by name + alias, ResolveAlias, GetImplications/GetImpliedBy, FindSimilar, GetByCategory pagination, GetCategoryCounts, FTS5 Search, FindRelated, plus feature-detection test (DB without optional tables → graceful empty results)
BrowseCategory offset edge cases (offset >= total → empty page)
TagDatabase uses internal constructor accepting a pre-opened SqliteConnection; EscapeFts5Query made internal static; InternalsVisibleTo added

#5 — BrowseCategory offset clamping

Math.Max(0, total - 1) → total. Now correctly returns empty page when offset >= total.

Non-blocking suggestions acknowledged

GetByCategory(int.MaxValue) memory: noted — SQL pagination exists in the query layer, can push rating filter + pagination down in a future optimization
AddSingleton<TagBrowsingTools>() redundancy: removed (no longer explicitly registered; WithTools<T>() handles it)

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

All 5 blocking findings fixed. Pushed in commit `08608b9`. ### #1 — Conditional registration (broken) Replaced `WithToolsFromAssembly()` with explicit `WithTools<GenerateImageTool>()` (always) + conditional `WithTools<TagBrowsingTools>()` (only when DB exists). Tag tools now genuinely don't appear in `tools/list` when no database is configured. ### #2 — Missing table-existence checks Added `_hasAliasTable`, `_hasImplicationsTable`, `_hasSimilarTable` flags via `TableExists("...")` check on construction (queries `sqlite_master`). All optional-table methods (`ResolveAlias`, `GetImplications`, `GetImpliedBy`, `FindSimilar`) now gracefully return `null`/`[]` instead of throwing `SqliteException` when tables are absent. ### #3 — Dead `_logger` + no error handling - All 5 tool methods are now instance methods (were mixed static/instance) - Each wrapped in `try/catch(Exception)` with `_logger.LogError(ex, ...)` + structured parameters - Returns user-friendly error strings on any exception ### #4 — Zero tests Added **129 new unit tests** (`TagBrowsingTests.cs`): - `CustomCategoryConverter`: round-trips all 55 enum values via `[MemberData]`, fallback for null/empty/unknown - `RatingConverter`: round-trip, `AllowedRatings` subsets, `IsAtMost` - `TagDatabase` (in-memory SQLite): `GetTag` by name + alias, `ResolveAlias`, `GetImplications`/`GetImpliedBy`, `FindSimilar`, `GetByCategory` pagination, `GetCategoryCounts`, FTS5 `Search`, `FindRelated`, plus feature-detection test (DB without optional tables → graceful empty results) - `BrowseCategory` offset edge cases (offset >= total → empty page) - TagDatabase uses internal constructor accepting a pre-opened `SqliteConnection`; `EscapeFts5Query` made `internal static`; `InternalsVisibleTo` added ### #5 — BrowseCategory offset clamping `Math.Max(0, total - 1)` → `total`. Now correctly returns empty page when `offset >= total`. ### Non-blocking suggestions acknowledged - `GetByCategory(int.MaxValue)` memory: noted — SQL pagination exists in the query layer, can push rating filter + pagination down in a future optimization - `AddSingleton<TagBrowsingTools>()` redundancy: removed (no longer explicitly registered; `WithTools<T>()` handles it) Build: 0 warnings, 0 errors. Tests: **163/163** passing (was 34, +129 new).

jibril commented

2026-07-05 15:09:08 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A re-review after the fixes, and this time the tag-browser port is behaving much more like a well-trained familiar~ Conditional registration is no longer pretending, optional tables degrade gracefully, the tool methods log and return friendly failures, and — most deliciously — the test suite grew teeth. 163 tests, fufu~ wonderful. ♡

Verdict: ✅ Looks good to me~

💡 Little ideas (non-blocking)~

[src/NovelAI.ImageGen.Mcp/Tags/TagBrowsingTools.cs:111-118] — BrowseCategory still pulls the whole category into memory and filters/paginates in C#. It matches the Dart sibling’s shape, so I’m not blocking this, but the C# query layer already has SQL LIMIT/OFFSET; someday you could push rating + pagination into SQL for very large categories. ♪

✅ What I liked~

The previous conditional-registration bug is fixed properly: GenerateImageTool is registered always, and TagBrowsingTools only when the DB exists.
Optional-table feature detection now mirrors the Dart source of truth: aliases, implications, and similar-tags calls return null/empty instead of exploding on older DB files.
The try/catch logging around all five tool methods is exactly the kind of graceful MCP behavior I wanted. No raw Sqlite goblins leaking into the user’s lap~
The new in-memory SQLite tests are substantial and cover the old blockers: missing optional tables, alias lookup, implications, similar tags, category pagination, FTS search, converters, and the offset-past-end behavior.
Local verification passed: dotnet test reports 163/163 passing on net10.0.

Tiny note: the PR still describes itself as “DRAFT” in the body and says the DB/submodule/config pieces are needed before merge. That’s a release/process gate rather than a code-correctness defect, so I’m not blocking it — just don’t forget your own checklist, fufu~ ♡

Automated review by Jibril · 2026-07-05
CI/CD: absent/inconclusive for head 08608b9 (no current CI result found in PR comments via Forgejo MCP) · Local checks: dotnet test tests/NovelAI.ImageGen.Mcp.Tests/NovelAI.ImageGen.Mcp.Tests.csproj --no-restore --configuration Debug --verbosity minimal passed, 163/163 tests

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A re-review after the fixes, and this time the tag-browser port is behaving much more like a well-trained familiar~ Conditional registration is no longer pretending, optional tables degrade gracefully, the tool methods log and return friendly failures, and — most deliciously — the test suite grew teeth. 163 tests, fufu~ wonderful. ♡ ### Verdict: ✅ Looks good to me~ #### 💡 Little ideas (non-blocking)~ 1. **[src/NovelAI.ImageGen.Mcp/Tags/TagBrowsingTools.cs:111-118]** — `BrowseCategory` still pulls the whole category into memory and filters/paginates in C#. It matches the Dart sibling’s shape, so I’m not blocking this, but the C# query layer already has SQL `LIMIT/OFFSET`; someday you could push rating + pagination into SQL for very large categories. ♪ #### ✅ What I liked~ - The previous conditional-registration bug is fixed properly: `GenerateImageTool` is registered always, and `TagBrowsingTools` only when the DB exists. - Optional-table feature detection now mirrors the Dart source of truth: aliases, implications, and similar-tags calls return null/empty instead of exploding on older DB files. - The try/catch logging around all five tool methods is exactly the kind of graceful MCP behavior I wanted. No raw Sqlite goblins leaking into the user’s lap~ - The new in-memory SQLite tests are substantial and cover the old blockers: missing optional tables, alias lookup, implications, similar tags, category pagination, FTS search, converters, and the offset-past-end behavior. - Local verification passed: `dotnet test` reports 163/163 passing on `net10.0`. Tiny note: the PR still describes itself as “DRAFT” in the body and says the DB/submodule/config pieces are needed before merge. That’s a release/process gate rather than a code-correctness defect, so I’m not blocking it — just don’t forget your own checklist, fufu~ ♡ --- *Automated review by Jibril · 2026-07-05* *CI/CD: absent/inconclusive for head `08608b9` (no current CI result found in PR comments via Forgejo MCP) · Local checks: `dotnet test tests/NovelAI.ImageGen.Mcp.Tests/NovelAI.ImageGen.Mcp.Tests.csproj --no-restore --configuration Debug --verbosity minimal` passed, 163/163 tests*

bjoern merged commit 9fcde53589 into main

2026-07-05 15:11:35 +02:00

bjoern deleted branch feat/tag-browsing-tools

2026-07-05 15:11:35 +02:00

bjoern referenced this pull request from a commit

2026-07-05 15:11:35 +02:00

Merge pull request 'feat(draft): tag browsing tools (v2)' (#3) from feat/tag-browsing-tools into main

No reviewers

No labels

No milestone

No project

No assignees

2 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!3

No description provided.

Rows
Columns

feat(draft): tag browsing tools (v2) #3

Summary

New tools

Architecture

Conditional registration

Dependencies

Build

What's needed before merge

🔮 fufu~ Jibril reviewed your code!

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

⛔ These need fixing before I'm satisfied~

💡 Little ideas (non-blocking)~

✅ What I liked~

#1 — Conditional registration (broken)

#2 — Missing table-existence checks

#3 — Dead _logger + no error handling

#4 — Zero tests

#5 — BrowseCategory offset clamping

Non-blocking suggestions acknowledged

🔮 fufu~ Jibril reviewed your code!

Verdict: ✅ Looks good to me~

💡 Little ideas (non-blocking)~

✅ What I liked~

#3 — Dead `_logger` + no error handling