Skip to content

feat: add DOODS2 validator (HTTP-only) (#14)#43

Merged
blehnen merged 12 commits into
mainfrom
feature/14-doods2-validator
May 6, 2026
Merged

feat: add DOODS2 validator (HTTP-only) (#14)#43
blehnen merged 12 commits into
mainfrom
feature/14-doods2-validator

Conversation

@blehnen

@blehnen blehnen commented May 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • New FrigateRelay.Plugins.Doods2 validator plugin — HTTP-based IValidationPlugin against a self-hosted DOODS2 v2 server (http://<host>:10200). Closes feat: add DOODS2 validator (TFLite / TF / YOLOv5 detector hub) #14.
  • Per-instance DetectorName (e.g. default TFLite / tensorflow Faster R-CNN / pytorch YOLOv5s), MinConfidence, AllowedLabels, OnError (FailClosed/FailOpen), Timeout, AllowInvalidCertificates.
  • API shape verified against a live DOODS2 v2 instance (Python rewrite, MIT-licensed). POST /detect with {detector_name, data: <base64>, detect: {"*": <0-100 threshold>}} request, {id, detections: [{top, left, bottom, right, label, confidence}]} response. Confidence is 0-100 scale on the wire; the validator normalizes to 0-1 internally before comparing to operator-facing MinConfidence.
  • 9 WireMock-driven unit tests covering every validator path: allow-after-normalization / reject-low-confidence / reject-bad-label / no-snapshot / timeout-FailClosed/Open / unavailable-FailClosed / unavailable-FailOpen / cancellation propagation. Test count rises 258 → 267.

Why HTTP-only (gRPC scope reverted mid-PR)

The original Phase 14 / CONTEXT-14 D4 decision called for both HTTP and gRPC transports. During this PR's build, the orchestrator probed the live DOODS2 v2 server and verified upstream's README:

"DOODS2 drops support for gRPC as I doubt very much anyone used it anyways."
snowzach/doods2 README

gRPC was a feature of the legacy Go-based snowzach/doods server only. The Python snowzach/doods2 rewrite is HTTP-only by design. Maintaining a gRPC code path that targets a hypothetical legacy Go server adds a new dep family (Grpc.Net.Client, Google.Protobuf, Grpc.Tools), ~300 LOC, and ~5 tests for a feature with zero real-world users.

Reversal commit (97fcb0e) drops:

  • Grpc.Net.Client, Google.Protobuf, Grpc.Tools package references
  • Vendored Protos/odrpc.proto (was sourced from snowzach/doods, MIT)
  • Doods2Options.Transport property + Doods2Transport enum
  • gRPC code path in Doods2Validator + RpcException catches
  • gRPC client construction in PluginRegistrar

Net diff vs the dual-transport scope: -283 LOC. No Grpc.* packages anywhere in the repo (git grep + transitive checks both empty).

CONTEXT-14 D4 amended; PLAN-2.3 marked REMOVED with reversal record. PLAN-2.2 absorbed PLAN-2.3's residual scope (CHANGELOG bullet + cancellation test).

Pre-emptively addressed REVIEW findings

The local reviewer caught two Important issues that were fixed inline before opening this PR:

  1. DOODS2 detect scale mismatchDoods2Validator was sending operator's MinConfidence (0-1 scale) as the detect threshold to DOODS2 (which expects 0-100). Masked by the plugin's own post-filter, but silently mis-encoded operator intent on the wire. Fixed: _opts.MinConfidence * 100.0 plus a regression-locking assertion in Test 1 (body.Should().Contain("\"*\":50")).
  2. Implicit cancellation guardDoods2Validator.ValidateAsync now has an explicit ct.ThrowIfCancellationRequested() at the top of the try block, removing reliance on HttpClient pre-cancel internals to ensure a pre-cancelled token never reaches the wire.

Plus the Roboflow-pattern lessons from PR #42's REVIEW-1.2 are pre-applied here:

Wave 2 of 3 in Phase 14 / v1.2

This is the second of three sequential PRs against main:

  1. ✅ Roboflow validator — PR feat: add Roboflow Inference validator (#13) #42, merged
  2. This PR — DOODS2 HTTP-only validator (feat: add DOODS2 validator (TFLite / TF / YOLOv5 detector hub) #14)
  3. Next — Per-action ParallelValidators opt-in (feat(validators): optional parallel execution with all-pass aggregation #23)

CHANGELOG [Unreleased] accumulates bullets across all three; promotes to [1.2.0] after the third merges.

Test plan

  • dotnet build FrigateRelay.sln -c Release — 0 warnings, 0 errors
  • bash .github/scripts/run-tests.sh --skip-integration — 267/267 passing
  • Architectural invariants clean: no Grpc.* source or transitive, no App.Metrics/OpenTracing/Jaeger.*, no .Result/.Wait, no ServicePointManager (except doc-comments saying "never use it"), no hard-coded IPs in source
  • Operator validation against the live DOODS2 at http://192.168.0.2:10200 once merged + deployed locally

Review trail

  • .shipyard/phases/14/results/SUMMARY-2.1.md — initial scaffold (4 commits) + reversal addendum
  • .shipyard/phases/14/results/SUMMARY-2.2.md — tests + CHANGELOG
  • .shipyard/phases/14/results/REVIEW-2.2.md — PASS after one revision cycle (2 Important findings addressed inline, including the scale-mismatch silent bug)
  • .shipyard/phases/14/plans/PLAN-2.3.md — REMOVED (gRPC scope reverted)

Carry-over for follow-up

RoboflowValidator on main has the same implicit-cancellation pattern; should get the explicit ct.ThrowIfCancellationRequested() guard in a v1.2.x cleanup commit. Not in this PR's scope.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added DOODS2 validator plugin supporting HTTP-based object detection with configurable detection thresholds, label filtering, and timeout handling.
    • Introduced per-action parallel validation mode enabling concurrent validator execution with strict AND decision logic for enhanced detection accuracy.
  • Bug Fixes

    • Fixed dispatcher eviction callback logging to correctly report the action identifier in event logs.

blehnen added 10 commits May 6, 2026 15:14
…PC codegen

Upstream: snowzach/doods2 commit 34f4f07 (2022-01-02, MIT).
Cleaned for Grpc.Tools: removed Go extensions, added csharp_namespace.
Grpc.Tools PrivateAssets=all keeps gRPC deps plugin-local.
…ransport clients

PluginRegistrar enumerates Validators:* config, filters by Type=="Doods2",
and registers per-instance:
- Named options (`AddOptions<Doods2Options>(key).ValidateOnStart`)
- Named HttpClient with optional per-handler TLS bypass (no resilience handler)
- Singleton `GrpcChannel` per validator instance (HTTP/2-multiplexed)
- AddKeyedSingleton<IValidationPlugin> with both transport clients injected

Both HTTP and gRPC clients are always built per instance — the validator
branches on Doods2Options.Transport at call time, keeping the registrar
branch-free and the validator constructor testable without conditional DI.

HostBootstrap.cs:135 adds the registrar inside the existing Validators-section
gate alongside CPAI (#13) and Roboflow (PR #42).

Architectural-invariant note: source-level gRPC coupling stays plugin-local
(`git grep -nE 'Grpc\.' src/FrigateRelay.Host src/FrigateRelay.Abstractions`
returns empty ✓). Runtime transitive Grpc.* deps DO flow into Host's package
graph because Host ProjectReferences Doods2 — this is unavoidable with
build-time DI plugin loading (CONTEXT-1 decision A). The meaningful invariant
is source coupling, not transitive package list. Documented in SUMMARY-2.1.
…ly since v2

User flagged that the live DOODS2 server at 192.168.0.2:10200 is the v2 Python
rewrite (snowzach/doods2), which deliberately dropped gRPC support. Confirmed
via upstream README:

  "DOODS2 drops support for gRPC as I doubt very much anyone used it anyways."

gRPC was a feature of the legacy Go-based snowzach/doods server only.
Maintaining a gRPC code path in this plugin would target zero real users.

This commit reverts the gRPC scope CONTEXT-14 D4 originally locked in:

Source code:
- Doods2Options.Transport property + Doods2Transport enum: removed.
- Doods2Validator constructor reduced from 5 params to 4 (no odrpcClient).
- gRPC code path (DetectGrpcAsync), RpcException catch blocks, gRPC imports:
  removed.
- PluginRegistrar: no longer constructs GrpcChannel or registers a gRPC
  client.
- FrigateRelay.Plugins.Doods2.csproj: dropped Grpc.Net.Client, Google.Protobuf,
  Grpc.Tools PackageReferences and the <Protobuf> MSBuild item.
- src/FrigateRelay.Plugins.Doods2/Protos/odrpc.proto + Protos/ directory:
  deleted.
- Validator now also catches JsonException (mirrors the Roboflow PR #42
  review-feedback fix) so a non-JSON proxy response routes through OnError.

Plans:
- PLAN-2.3.md: replaced with a REMOVED notice + reversal record.
- PLAN-2.2.md: expanded from 7 tests to 9 (absorbed PLAN-2.3's deferred
  cancellation test + a FailOpen-on-HTTP-error test mirroring PR #42's
  REVIEW-1.2 fix). Now owns the DOODS2 CHANGELOG bullet.
- PLAN-2.1.md: header notice added that the original scope was reverted.

Project meta-docs:
- CONTEXT-14.md D4: amended with explicit reversal note + rationale.
- PROJECT.md "v1.2 scope" / #14: updated to HTTP-only.
- ROADMAP.md Phase 14: goal + risk + deliverables updated. #14 risk
  dropped from Medium to Low.
- RESEARCH.md: header notice added that §4 (gRPC integration plan) and
  §7.3 (DOODS2 gRPC API) are stale historical content.
- SUMMARY-2.1.md: Reversal Addendum at the end records what changed.

Architectural-invariant tension RESOLVED: both source-grep and transitive-
package checks for `Grpc.*` now return empty. The "tension" surfaced in
SUMMARY-2.1's original "Issues Encountered" is gone.

Build clean (0 warnings); 258/258 tests still passing; PLAN-2.2 will add 9
new for a 267 final target.
…2 PASS

Two source/test fixes from REVIEW-2.2:

1. **Doods2Validator.cs:75** sent operator-facing 0-1 MinConfidence as the
   threshold in the DOODS2 `detect` dict, which expects 0-100 scale. Operator
   `MinConfidence=0.5` was sent as 0.5 (= 0.5% confidence) instead of 50.0
   (= 50%). Masked by the plugin's own EvaluateDetections post-filter, but
   silently mis-encoded operator intent. Fixed: `_opts.MinConfidence * 100.0`
   plus a regression-locking assertion in Test 1
   (`body.Should().Contain("\"*\":50")`).

2. **Doods2Validator.ValidateAsync** now has an explicit
   `ct.ThrowIfCancellationRequested()` as the first line in the try block.
   Previously Test 9's `LogEntries.Should().BeEmpty()` relied on HttpClient
   pre-cancel behavior; now self-documenting. Same fix should follow for
   RoboflowValidator on main in a v1.2.x cleanup commit (out of scope here).

Plus one Suggestion: comment on Test 5's inline stub explaining why error-path
tests don't use StubDetectEndpoint body matchers (they exercise validator
behavior, not request-contract — Test 1 covers that).

REVIEW-2.2.md committed with PASS verdict.

Build clean (0 warnings); 267/267 tests still passing.
@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@blehnen has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 34 minutes and 36 seconds before requesting another review.

To continue reviewing without waiting, purchase usage credits in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 250be3cc-acad-48fa-aa92-80f6a615a298

📥 Commits

Reviewing files that changed from the base of the PR and between dc14f26 and d35c86e.

📒 Files selected for processing (3)
  • .shipyard/ISSUES.md
  • .shipyard/phases/14/RESEARCH.md
  • tests/FrigateRelay.Plugins.Doods2.Tests/Doods2ValidatorTests.cs
📝 Walkthrough

Walkthrough

This PR adds a new DOODS2 HTTP-only validator plugin to Phase 14, including per-instance configuration options, an HTTP POST-based detection validator, plugin registration wiring, comprehensive test coverage via WireMock, and updated planning/documentation to reflect the HTTP-only transport decision and reversal of previously planned gRPC support.

Changes

DOODS2 HTTP Validator Plugin

Layer / File(s) Summary
Configuration & Data Models
src/FrigateRelay.Plugins.Doods2/Doods2Options.cs, Doods2Response.cs
Introduces Doods2Options with per-instance settings (BaseUrl, DetectorName, MinConfidence, AllowedLabels, OnError, Timeout, AllowInvalidCertificates) and enum Doods2ValidatorErrorMode (FailClosed/FailOpen); adds internal DTOs for HTTP request/response shapes with JSON property mappings.
Core Validator Logic
src/FrigateRelay.Plugins.Doods2/Doods2Validator.cs
Implements IValidationPlugin to POST snapshots as base64 JSON to DOODS2's /detect endpoint, normalizes 0–100 confidence to 0–1, filters detections by MinConfidence and AllowedLabels, and routes errors (timeout, HTTP failure, JSON parse) through OnError policy; includes logging for unavailability and timeout events.
Plugin Registration & DI
src/FrigateRelay.Plugins.Doods2/PluginRegistrar.cs, HostBootstrap.cs, FrigateRelay.Host.csproj, FrigateRelay.sln
Registers PluginRegistrar to scan Validators config, bind per-instance named Doods2Options, create per-instance HttpClient with TLS handler, and register keyed singleton IValidationPlugin; integrates into solution and host project references; updates HostBootstrap.ConfigureServices to invoke registrar.
Project Scaffolding
src/FrigateRelay.Plugins.Doods2/FrigateRelay.Plugins.Doods2.csproj
New plugin project with SDK, RootNamespace, AssemblyName, doc generation, references to Abstractions and Microsoft.Extensions packages, and InternalsVisibleTo for tests.
Tests & Test Infrastructure
tests/FrigateRelay.Plugins.Doods2.Tests/*
New test project (csproj, Usings.cs) and Doods2ValidatorTests.cs with nine test cases covering normalization (0–100 to 0–1), threshold/label filtering, missing snapshot, HTTP timeout (FailClosed/FailOpen), server errors (FailClosed/FailOpen), and cancellation propagation; uses WireMock stubs and CapturingLogger for EventId assertions.
Documentation & Planning
.shipyard/*, CHANGELOG.md
Updated roadmap and context documents to reflect HTTP-only DOODS2 transport (PLAN-2.3 removed, gRPC scope reverted); added Phase 14 research, plans (PLAN-2.1, PLAN-2.2), review (REVIEW-2.2), and summary documents; CHANGELOG records new DOODS2 validator with HTTP-only scope, per-instance config knobs, and 9 test count; also fixes ChannelActionDispatcher eviction-callback to log correct Action.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Possibly related issues

  • feat: add DOODS2 validator (TFLite / TF / YOLOv5 detector hub) #14 — This PR fully implements the acceptance criteria for the DOODS2 validator feature: new plugin project following existing layout, per-instance HTTP configuration, WireMock-based integration tests covering pass/fail/timeout/error scenarios, wiring into HostBootstrap registrar list, and CHANGELOG documentation for the HTTP-only validator.

Possibly related PRs

  • blehnen/FrigateRelay#42 — Both add HTTP-based validator plugins (Roboflow vs. DOODS2) following the same registrar/Options/Validator pattern and modify HostBootstrap.ConfigureServices to register the plugin.
  • blehnen/FrigateRelay#38 — Both touch the ChannelActionDispatcher eviction-callback fix to read the evicted DispatchItem instead of the iteration variable, indicating direct overlap in the same method.

Poem

🐰 A DOODS2 validator hops in,
With HTTP and JSON to begin,
Base64 snapshots post away,
Nine tests guard the happy-path day,
No gRPC—just clean, light, and thin! 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add DOODS2 validator (HTTP-only) (#14)' is concise, specific, and directly summarizes the main change—adding a new HTTP-only validator plugin for DOODS2 detection servers.
Description check ✅ Passed The PR description comprehensively covers the change with summary, API details, configuration options, test coverage, rationale for HTTP-only reversal, pre-emptive review fixes, phase context, and test plan results.
Linked Issues check ✅ Passed The PR fully addresses issue #14 requirements: new FrigateRelay.Plugins.Doods2 plugin implementing IValidationPlugin, DoodsOptions with all required fields, validator with confidence/label gating, host registration following patterns, 9 unit tests covering pass/fail/timeout/error scenarios, and configuration documentation in PR description.
Out of Scope Changes check ✅ Passed All changes are directly in-scope: DOODS2 validator plugin implementation, test scaffolding, solution/project wiring, CHANGELOG updates, and Phase 14 roadmap documentation reflecting the HTTP-only scope reversal.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
.shipyard/phases/14/RESEARCH.md (2)

220-229: 💤 Low value

Consider updating the config example to reflect HTTP-only implementation.

Line 222 includes "Transport": "Http" in the DOODS2 config example, but the stale-content notice states DOODS2 v2 is HTTP-only. If there's only one transport option, this discriminator field should not exist in the config shape.

Suggest either removing this field from the example or adding a stale marker to §3.4 similar to the notice for §4 and §7.3.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.shipyard/phases/14/RESEARCH.md around lines 220 - 229, The example config
for "doods_persons" includes a redundant "Transport": "Http" discriminator even
though DOODS2 v2 is HTTP-only; update the example by removing the "Transport"
field from the "doods_persons" block (remove the "Transport" key/value) so the
config shape no longer implies multiple transports, or alternatively add a
stale-content marker to §3.4 (like the existing notice in §4 and §7.3)
clarifying DOODS2 v2 is HTTP-only; target the "doods_persons" example and §3.4
in the same document when making this change.

467-472: 💤 Low value

Update test count breakdown to reflect HTTP-only implementation.

Line 470 shows PR-2 adding "12" tests with a breakdown including "gRPC path: 5 covering..." tests, but §7.3 (DOODS2 gRPC API) is marked stale and gRPC scope was reverted. The PR summary indicates 258 → 267 (+9 tests), which is closer to the HTTP-only count.

Suggest updating this table to reflect the actual HTTP-only test count or marking this projection as superseded by the implementation reality.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.shipyard/phases/14/RESEARCH.md around lines 467 - 472, Update the table row
for PR-2 (DOODS2) to reflect the HTTP-only implementation: change the "New tests
minimum" count from 12 to 9, adjust the breakdown text to show only the HTTP
path tests (e.g., HTTP path: 7 including the transport-config-validation and the
6 from PR-1 list) and remove the "gRPC path" bullet, and then recalculate the
cumulative totals (PR-1 250 → PR-2 259 → PR-3 266) so the table and descriptive
text match the implemented HTTP-only scope and the stale §7.3 gRPC note.
tests/FrigateRelay.Plugins.Doods2.Tests/Doods2ValidatorTests.cs (1)

38-41: 💤 Low value

Tighten the threshold-encoding assertion to avoid accidental future drift.

body!.Should().Contain("\"*\":50") is robust today because STJ writes the double 50.0 as 50 with no decimal, but the substring also matches "*":500, "*":50.5, etc. If a future change ever computes a different scaled value (e.g. operator changes MinConfidence and the test data drifts), the regression guard described in the comment above could silently keep passing. A delimiter-anchored check is a low-cost upgrade.

♻️ Suggested tightening
-        body!.Should().Contain("\"*\":50");
+        // Anchor on a JSON delimiter so "*":500 / "*":50.5 cannot accidentally satisfy this guard.
+        body!.Should().MatchRegex("\"\\*\"\\s*:\\s*50(?=[,}\\s])");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/FrigateRelay.Plugins.Doods2.Tests/Doods2ValidatorTests.cs` around lines
38 - 41, The current test in Doods2ValidatorTests uses
body!.Should().Contain("\"*\":50") which can match substrings like "500" or
"50.5"; update the assertion on stub.LogEntries[0].RequestMessage?.Body to
ensure the encoded threshold is a standalone token by checking a
delimiter-anchored form (e.g. assert the body contains "\"*\":50," or
"\"*\":50}" or use a regex like "\"\\*\":\\s*50(\\D)" to enforce a non-digit
delimiter). Modify the assertion around stub.LogEntries / RequestMessage.Body
accordingly so the test fails if the numeric token changes or is part of a
larger number.
src/FrigateRelay.Plugins.Doods2/PluginRegistrar.cs (1)

55-84: 💤 Low value

Optional: centralize HttpClient configuration on the AddHttpClient builder.

BaseAddress and Timeout are mutated on the resolved HttpClient inside the keyed singleton factory. It works (the singleton factory runs once), but it splits client configuration across two registration points and is fragile if anyone later adds ConfigureHttpClient upstream. The standard IHttpClientFactory pattern is to configure the client itself in AddHttpClient(name, (sp, client) => ...) so the factory hands back a fully-prepared client.

♻️ Suggested centralization
             var clientName = $"Doods2:{instanceKey}";
             context.Services
-                .AddHttpClient(clientName)
+                .AddHttpClient(clientName, (sp, client) =>
+                {
+                    var opts = sp.GetRequiredService<IOptionsMonitor<Doods2Options>>().Get(instanceKey);
+                    client.BaseAddress = new Uri(opts.BaseUrl);
+                    client.Timeout = opts.Timeout;
+                })
                 .ConfigurePrimaryHttpMessageHandler(sp =>
                 {
                     // ... unchanged
                 });

             context.Services.AddKeyedSingleton<IValidationPlugin>(instanceKey, (sp, key) =>
             {
                 var name = (string)key!;
                 var opts = sp.GetRequiredService<IOptionsMonitor<Doods2Options>>().Get(name);
                 var http = sp.GetRequiredService<IHttpClientFactory>().CreateClient($"Doods2:{name}");
-                http.BaseAddress = new Uri(opts.BaseUrl);
-                http.Timeout = opts.Timeout;
                 var logger = sp.GetRequiredService<ILogger<Doods2Validator>>();
                 return new Doods2Validator(name, opts, http, logger);
             });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/FrigateRelay.Plugins.Doods2/PluginRegistrar.cs` around lines 55 - 84, The
HttpClient is partially configured in the AddKeyedSingleton factory (setting
BaseAddress and Timeout on the resolved client) which spreads client setup; move
that configuration into the AddHttpClient(clientName) registration by calling
the overload AddHttpClient(clientName, (sp, client) => { ... }) and inside that
ConfigureHttpClient use the same
IOptionsMonitor<Doods2Options>.Get(instanceKey/name) to set client.BaseAddress
and client.Timeout so CreateClient($"Doods2:{name}") in the AddKeyedSingleton
factory returns a fully configured HttpClient for Doods2Validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In @.shipyard/phases/14/RESEARCH.md:
- Around line 220-229: The example config for "doods_persons" includes a
redundant "Transport": "Http" discriminator even though DOODS2 v2 is HTTP-only;
update the example by removing the "Transport" field from the "doods_persons"
block (remove the "Transport" key/value) so the config shape no longer implies
multiple transports, or alternatively add a stale-content marker to §3.4 (like
the existing notice in §4 and §7.3) clarifying DOODS2 v2 is HTTP-only; target
the "doods_persons" example and §3.4 in the same document when making this
change.
- Around line 467-472: Update the table row for PR-2 (DOODS2) to reflect the
HTTP-only implementation: change the "New tests minimum" count from 12 to 9,
adjust the breakdown text to show only the HTTP path tests (e.g., HTTP path: 7
including the transport-config-validation and the 6 from PR-1 list) and remove
the "gRPC path" bullet, and then recalculate the cumulative totals (PR-1 250 →
PR-2 259 → PR-3 266) so the table and descriptive text match the implemented
HTTP-only scope and the stale §7.3 gRPC note.

In `@src/FrigateRelay.Plugins.Doods2/PluginRegistrar.cs`:
- Around line 55-84: The HttpClient is partially configured in the
AddKeyedSingleton factory (setting BaseAddress and Timeout on the resolved
client) which spreads client setup; move that configuration into the
AddHttpClient(clientName) registration by calling the overload
AddHttpClient(clientName, (sp, client) => { ... }) and inside that
ConfigureHttpClient use the same
IOptionsMonitor<Doods2Options>.Get(instanceKey/name) to set client.BaseAddress
and client.Timeout so CreateClient($"Doods2:{name}") in the AddKeyedSingleton
factory returns a fully configured HttpClient for Doods2Validator.

In `@tests/FrigateRelay.Plugins.Doods2.Tests/Doods2ValidatorTests.cs`:
- Around line 38-41: The current test in Doods2ValidatorTests uses
body!.Should().Contain("\"*\":50") which can match substrings like "500" or
"50.5"; update the assertion on stub.LogEntries[0].RequestMessage?.Body to
ensure the encoded threshold is a standalone token by checking a
delimiter-anchored form (e.g. assert the body contains "\"*\":50," or
"\"*\":50}" or use a regex like "\"\\*\":\\s*50(\\D)" to enforce a non-digit
delimiter). Modify the assertion around stub.LogEntries / RequestMessage.Body
accordingly so the test fails if the numeric token changes or is part of a
larger number.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 527e086c-62fa-4f6e-ad41-d5e423e1e579

📥 Commits

Reviewing files that changed from the base of the PR and between 59dd4f2 and dc14f26.

📒 Files selected for processing (22)
  • .shipyard/PROJECT.md
  • .shipyard/ROADMAP.md
  • .shipyard/phases/14/CONTEXT-14.md
  • .shipyard/phases/14/RESEARCH.md
  • .shipyard/phases/14/plans/PLAN-2.1.md
  • .shipyard/phases/14/plans/PLAN-2.2.md
  • .shipyard/phases/14/plans/PLAN-2.3.md
  • .shipyard/phases/14/results/REVIEW-2.2.md
  • .shipyard/phases/14/results/SUMMARY-2.1.md
  • .shipyard/phases/14/results/SUMMARY-2.2.md
  • CHANGELOG.md
  • FrigateRelay.sln
  • src/FrigateRelay.Host/FrigateRelay.Host.csproj
  • src/FrigateRelay.Host/HostBootstrap.cs
  • src/FrigateRelay.Plugins.Doods2/Doods2Options.cs
  • src/FrigateRelay.Plugins.Doods2/Doods2Response.cs
  • src/FrigateRelay.Plugins.Doods2/Doods2Validator.cs
  • src/FrigateRelay.Plugins.Doods2/FrigateRelay.Plugins.Doods2.csproj
  • src/FrigateRelay.Plugins.Doods2/PluginRegistrar.cs
  • tests/FrigateRelay.Plugins.Doods2.Tests/Doods2ValidatorTests.cs
  • tests/FrigateRelay.Plugins.Doods2.Tests/FrigateRelay.Plugins.Doods2.Tests.csproj
  • tests/FrigateRelay.Plugins.Doods2.Tests/Usings.cs

blehnen added 2 commits May 6, 2026 17:06
3 of 4 nitpicks fixed (all marked "💤 Low value" / informational; not blocking):

1. **Doods2ValidatorTests.cs Test 1** — tighten the threshold-encoding
   regression assertion. `body.Should().Contain("\"*\":50")` could
   accidentally false-match on `"*":500` or `"*":50.5`. Anchored to a JSON
   delimiter via regex: `MatchRegex("\"\\*\"\\s*:\\s*50(?=[,}\\s])")`.

2. **RESEARCH.md §3.4 DOODS2 config example** — removed the redundant
   `"Transport": "Http"` field (DOODS2 v2 is HTTP-only post-reversal). Also
   bumped the example BaseUrl port from 8080 → 10200 to match the canonical
   DOODS2 v2 default.

3. **RESEARCH.md §8 test-count table** — updated PR-2 row from "12 (HTTP +
   gRPC)" to "9 HTTP-only" with reversal pointer to PLAN-2.3.md. Adjusted
   cumulative totals to match the gRPC-reverted reality.

Punted #4 (centralize HttpClient config in `AddHttpClient(name, (sp, client) =>
...)` rather than mutating BaseAddress/Timeout in the keyed-singleton factory):
the same critique applies to CPAI + Roboflow registrars on main. Apply
atomically to all three in a v1.2.x cleanup commit, not just DOODS2.

Build clean (0 warnings); 267/267 tests still passing.
… factory)

CodeRabbit nitpick #4 from PR #43: HttpClient.BaseAddress + Timeout are
mutated inside the AddKeyedSingleton factory in all three validator
registrars (CPAI, Roboflow, DOODS2) rather than configured at AddHttpClient
registration time. Works today, but fragile if anyone later adds
ConfigureHttpClient upstream.

Deferred to a v1.2.x cleanup commit so all three registrars get the fix
atomically — applying to DOODS2 only would create inconsistency.

No code changes; just an ISSUES.md entry so the architect picks it up at
next plan time.
@blehnen
blehnen merged commit 9672726 into main May 6, 2026
13 checks passed
@blehnen
blehnen deleted the feature/14-doods2-validator branch May 6, 2026 22:14
blehnen added a commit that referenced this pull request May 7, 2026
PR #42 (#13 Roboflow), PR #43 (#14 DOODS2), PR #44 (#23 ParallelValidators
+ StopAsync hardening) all merged to main. 291/291 tests passing across
the suite (49 net-new tests for the phase: 242 → 291). CHANGELOG
[Unreleased] is fully populated and ready for promotion to [1.2.0] per
RELEASING.md. Next step is operator-cut `git tag v1.2.0`, which triggers
release.yml's smoke + push-multiarch GHCR pipeline.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
blehnen added a commit that referenced this pull request May 7, 2026
Mirrors the Roboflow PluginRegistrar test pattern that closed the same
patch-coverage gap on PR #42. The DOODS2 PluginRegistrar was at 9.76%
(4/41 lines covered) on PR #43 because the validator unit tests only
exercise Doods2Validator directly — they don't touch the DI registration
path. Five tests now exercise:

- Single-entry registration (keyed validator + named options + named HttpClient).
- Two-entry independence (per-instance keyed singletons with distinct DetectorName).
- Type-discriminator filtering (DOODS2 registrar skips CodeProjectAi entries).
- Missing Validators section (clean return, no throw).
- AllowInvalidCertificates=true (lazy SocketsHttpHandler TLS-bypass branch).

5/5 passing in 326 ms. Pushed directly to main per operator authorization
(test-only, brings DOODS2 patch coverage above codecov's threshold for the
v1.2.0 release window).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add DOODS2 validator (TFLite / TF / YOLOv5 detector hub)

1 participant