Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions .shipyard/ISSUES.md
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,55 @@ Phase 12 closeout commit added `Path.GetFullPath(value)` canonicalization at CLI

---

### ID-30: Validator HttpClient.BaseAddress + Timeout configured in keyed-singleton factory rather than `AddHttpClient` builder

**Source:** CodeRabbit review on PR #43 (2026-05-06, nitpick #4)
**Severity:** Low / Style (no operator-visible impact, no security implication)
**Status:** Open

**Description:**
All three validator-plugin registrars (CPAI, Roboflow, DOODS2) mutate `HttpClient.BaseAddress` + `Timeout` on the resolved `HttpClient` inside their `AddKeyedSingleton<IValidationPlugin>` factory:

```csharp
context.Services.AddKeyedSingleton<IValidationPlugin>(instanceKey, (sp, key) =>
{
var name = (string)key!;
var opts = sp.GetRequiredService<IOptionsMonitor<TOptions>>().Get(name);
var http = sp.GetRequiredService<IHttpClientFactory>().CreateClient($"<Type>:{name}");
http.BaseAddress = new Uri(opts.BaseUrl); // ← mutated here
http.Timeout = opts.Timeout; // ← mutated here
// ...
});
```

The standard `IHttpClientFactory` pattern is to configure the client itself at registration time via `AddHttpClient(name, (sp, client) => ...)` so the factory hands back a fully-prepared client:

```csharp
context.Services
.AddHttpClient(clientName, (sp, client) =>
{
var opts = sp.GetRequiredService<IOptionsMonitor<TOptions>>().Get(instanceKey);
client.BaseAddress = new Uri(opts.BaseUrl);
client.Timeout = opts.Timeout;
})
.ConfigurePrimaryHttpMessageHandler(sp => /* ...unchanged... */);
```

It works today because the keyed-singleton factory runs once per instance, so the mutation only happens once. But it splits client configuration across two registration points and is fragile if anyone later adds `ConfigureHttpClient` upstream of the keyed singleton (which would silently lose the per-instance overrides).

**Affected files:**
- `src/FrigateRelay.Plugins.CodeProjectAi/PluginRegistrar.cs` (~lines 75-84)
- `src/FrigateRelay.Plugins.Roboflow/PluginRegistrar.cs` (~lines 75-84)
- `src/FrigateRelay.Plugins.Doods2/PluginRegistrar.cs` (~lines 75-84)

**Remediation:**
Apply the centralization pattern to all three registrars atomically in a single cleanup commit on a v1.2.x branch. Tests should not need changes — behavior is identical, only registration shape differs. Verify via existing `*PluginRegistrarTests` (Roboflow has 5 of these from PR #42; CPAI + DOODS2 do not yet — adding them as part of the cleanup would be valuable.

**Why deferred:**
Surfaced during PR #43 (DOODS2) CodeRabbit review. Applying to DOODS2 only would create stylistic inconsistency across the three registrars. Better to land all three together post-Phase-14 ship.

---

## Closed Issues

### ID-2: `IActionDispatcher`/`DispatcherOptions` should be `internal` *[CLOSED 2026-04-27]*
Expand Down
13 changes: 6 additions & 7 deletions .shipyard/PROJECT.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,9 @@ v1.1.0 shipped 2026-05-04 with full observability tagging and the `BlueIrisUrlTe
- Transport: HTTP via `HttpClient` (typed client per `IPluginRegistrar`); WireMock-driven unit tests; Testcontainers integration test if `roboflow/inference` image is available, otherwise WireMock-only with a documented manual-smoke recipe.

- **#14 — DOODS2 validator** (`FrigateRelay.Plugins.Doods2`).
- Operator-selectable transport: `Transport: "Http" | "Grpc"`. Both must be implemented and tested in v1.2 — gRPC is the perf-first path for high-throughput hosts (sub-millisecond serialisation, persistent HTTP/2 streams); HTTP matches the rest of the plugin family for simplicity. Operators choose per validator instance.
- **HTTP only** (CONTEXT-14 D4 reversed 2026-05-06). DOODS2 v2 (the Python rewrite at `snowzach/doods2`) deliberately dropped gRPC support per upstream README; the original-Go-server gRPC scope was reverted during PLAN-2.1 build. Operators on the legacy Go `snowzach/doods` server should use that project's gRPC client; this plugin targets v2.
- HTTP path: `POST /detect` with base64-encoded image + JSON detections back. WireMock-driven unit tests.
- gRPC path: vendored DOODS2 `.proto` file compiled in-project via `Grpc.Tools`; `Grpc.Net.Client` + `Google.Protobuf` added as deps to **this plugin only** — not to `FrigateRelay.Abstractions`, not to `FrigateRelay.Host`. In-process gRPC test server for unit tests.
- Same `MinConfidence` / `AllowedLabels` / `OnError` / `Timeout` knobs as CPAI/Roboflow.
- Same `MinConfidence` / `AllowedLabels` / `OnError` / `Timeout` knobs as CPAI/Roboflow. Plus `DetectorName` (default/tensorflow/pytorch) per CONTEXT-14.

- **#23 — Per-action `ParallelValidators: true`** in `ActionEntry`.
- Default `false`; sequential behavior unchanged for existing config.
Expand All @@ -239,20 +238,20 @@ v1.1.0 shipped 2026-05-04 with full observability tagging and the `BlueIrisUrlTe
Three sequential PRs against `main`:

1. **#13 first** — Roboflow Inference plugin. Smaller surface than #14 (HTTP-only); establishes the second-validator pattern that #14 follows.
2. **#14 second** — DOODS2 plugin (HTTP + gRPC). Builds on #13's pattern; adds the gRPC dep contained to this plugin only.
2. **#14 second** — DOODS2 plugin (HTTP only — gRPC scope reverted; see #14 description above). Builds on #13's pattern; no new dep families.
3. **#23 last** — parallel-AND opt-in. Lands after both #13 and #14 are in so its integration tests can exercise three validator types (CPAI + Roboflow + DOODS2) in a single AND chain — proves the design holds beyond the toy CPAI-only case.

CHANGELOG classifies all three as additive (semver minor). #23's `ParallelValidators` defaults to `false`, so existing `ActionEntry` configs are unaffected.

### v1.2 verification gates

- `dotnet build FrigateRelay.sln -c Release` zero warnings (warnings-as-errors, both Linux and Windows). New plugin projects compile clean.
- gRPC dep containment: `dotnet list <abstractions|host>.csproj package --include-transitive` shows no `Grpc.*` transitive on either project. The dep lives in `FrigateRelay.Plugins.Doods2` only.
- No gRPC anywhere (D4 reversed): `git grep -nE 'Grpc\.' src/` returns empty across the entire repo; no csproj declares Grpc.* packages.
- All existing tests pass.
- New unit tests per validator: at minimum allow / reject / timeout / OnError-FailClosed / OnError-FailOpen / cancellation, driven by WireMock for HTTP and an in-process gRPC server for DOODS2's gRPC path.
- New unit tests per validator: at minimum allow / reject / timeout / OnError-FailClosed / OnError-FailOpen / cancellation, driven by WireMock (HTTP-only — DOODS2 gRPC scope reverted).
- New integration test demonstrating ≥ 2 validators running in parallel under a single `ActionEntry` with `ParallelValidators: true`. WireMock or Testcontainers as available; the CPAI + Roboflow combination is the smallest meaningful coverage.
- `git grep -nE 'App\.Metrics|OpenTracing|Jaeger\.' src/` still empty (architectural invariant unchanged).
- `git grep -nE 'Grpc\.' src/FrigateRelay.Host src/FrigateRelay.Abstractions` returns empty (host + abstractions stay gRPC-free; gRPC is plugin-local).
- `git grep -nE 'Grpc\.' src/` returns empty across the entire repo (D4 reversed; no gRPC anywhere).

### v1.2 success criteria

Expand Down
13 changes: 6 additions & 7 deletions .shipyard/ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -416,36 +416,35 @@ Phases 1 and 2 can execute in parallel once the `.sln` exists (Phase 2 needs a s

**Status.** Not started. Phase 14 is the implementation phase for the v1.2 scope captured in `PROJECT.md` "Post-v1.1 Scope — v1.2 (more inference engines + parallel-AND validation)" (PROJECT.md:200–261). Three sequential PRs cover issues #13 (Roboflow Inference validator), #14 (DOODS2 validator with HTTP + gRPC transports), and #23 (per-action `ParallelValidators: true` opt-in). Decision rationale and the in-/out-of-scope boundary live in PROJECT.md; this section captures only the verifiable execution shape.

**Goal.** Ship v1.2.0 with (a) two new self-hosted `IValidationPlugin` implementations — Roboflow Inference (`FrigateRelay.Plugins.Roboflow`) and DOODS2 (`FrigateRelay.Plugins.Doods2`, HTTP + gRPC transports operator-selectable per validator instance); (b) a per-`ActionEntry` `ParallelValidators: true` opt-in that runs the action's validators concurrently under a strict-AND aggregation, with default-false preserving today's sequential behavior; (c) at least one integration test that exercises ≥ 2 validators in parallel under a single `ActionEntry` so the multi-engine story is operationally proven, not hypothetical.
**Goal.** Ship v1.2.0 with (a) two new self-hosted `IValidationPlugin` implementations — Roboflow Inference (`FrigateRelay.Plugins.Roboflow`) and DOODS2 (`FrigateRelay.Plugins.Doods2`, HTTP only — gRPC scope reverted post-PLAN-2.1 because DOODS2 v2 is HTTP-only upstream; CONTEXT-14 D4 amended); (b) a per-`ActionEntry` `ParallelValidators: true` opt-in that runs the action's validators concurrently under a strict-AND aggregation, with default-false preserving today's sequential behavior; (c) at least one integration test that exercises ≥ 2 validators in parallel under a single `ActionEntry` so the multi-engine story is operationally proven, not hypothetical.

**Dependencies.** Phase 13 (v1.1.0 GA on `main`; tagged counters and observability stack in place so any new counters introduced by #23 inherit the v1.1 tag matrix). The `[Unreleased]` ID-29 hotfix (eviction-callback log staleness) will likely roll out as v1.1.1 before or alongside Phase 14 — Phase 14 is **not** gated on it; the hotfix and v1.2 scope are independent.

**Risk.** **Low–Medium**. Composite breakdown:
- **#13 — Low.** HTTP-only validator that mirrors the established CPAI pattern (typed `HttpClient` per `IPluginRegistrar`, WireMock unit tests). No new dep families. Smaller surface than #14.
- **#14 — Medium.** Highest risk in this phase. Vendored DOODS2 `.proto` + `Grpc.Tools` codegen + `Grpc.Net.Client` + `Google.Protobuf` are a new dep family for this codebase. The architectural invariant is that gRPC stays plugin-local — never reaches abstractions or host. In-process gRPC test server is straightforward but new ground for FrigateRelay's test infrastructure.
- **#14 — Low** (was Medium pre-reversal). HTTP-only validator mirroring CPAI/Roboflow patterns. The original gRPC scope (vendored proto + Grpc.Tools + new dep family) was reverted during PLAN-2.1 build because DOODS2 v2 dropped gRPC upstream — see CONTEXT-14 D4 reversal note and PLAN-2.3.md.
- **#23 — Low–Medium.** Touches the validator execution loop in the host (the per-action chain that today runs sequentially). Feature-flagged with `ParallelValidators: false` as the default, so existing configs are unaffected by a regression here. Failure surface is bounded to actions that opt in.

**Estimate.** 6–9 hours of active work across the three PRs. #14 dominates (proto vendoring, gRPC client wiring, transport-selection plumbing); #13 and #23 are smaller.

**PR sequencing (decided).** Three sequential PRs against `main`, in the order #13 → #14 → #23 (PROJECT.md:237–245):
1. **#13 first.** Smaller surface (HTTP-only); establishes the second-validator pattern that #14 reuses for its HTTP transport.
2. **#14 second.** Builds on #13's pattern; adds the gRPC dep contained to this plugin only.
2. **#14 second.** Builds on #13's pattern; HTTP-only after the PLAN-2.1 reversal (see CONTEXT-14 D4 reversal note + PLAN-2.3.md).
3. **#23 last.** Lands after both #13 and #14 are merged so its integration test can exercise three validator types (CPAI + Roboflow + DOODS2) in a single AND chain — proves the parallel design holds beyond a CPAI-only toy case.

CHANGELOG classifies all three as additive (semver minor). #23's `ParallelValidators` defaults to `false`, so existing `ActionEntry` configs are unaffected on upgrade.

**Deliverables.**

- **#13 — Roboflow Inference validator.** New `src/FrigateRelay.Plugins.Roboflow/` project: `RoboflowValidator : IValidationPlugin`, `RoboflowOptions` (`BaseUrl`, `ModelId`, `MinConfidence`, `AllowedLabels`, `OnError`, `Timeout`), `Roboflow.PluginRegistrar` registering a typed `HttpClient`. Self-hosted Roboflow Inference only — no Roboflow Hosted Cloud API in v1.2 (PROJECT.md:213–214). Per-instance `ModelId` so operators declare multiple validator instances if they need different models per camera. New `tests/FrigateRelay.Plugins.Roboflow.Tests/` with WireMock-driven coverage of allow/reject/timeout/OnError-FailClosed/OnError-FailOpen/cancellation. Optional Testcontainers integration test if `roboflow/inference` exists on a public registry with acceptable boot time; otherwise WireMock-only with a documented manual-smoke recipe.
- **#14 — DOODS2 validator (HTTP + gRPC).** New `src/FrigateRelay.Plugins.Doods2/` project: `Doods2Validator : IValidationPlugin`, `Doods2Options` (`Transport: "Http" | "Grpc"`, `BaseUrl`, `MinConfidence`, `AllowedLabels`, `OnError`, `Timeout`), `Doods2.PluginRegistrar`. HTTP path: `POST /detect` with base64-encoded image + JSON detections back, WireMock-driven unit tests. gRPC path: vendored DOODS2 `.proto` compiled in-project via `Grpc.Tools`; `Grpc.Net.Client` + `Google.Protobuf` added as deps to **this plugin only** — not abstractions, not host. In-process gRPC test server for unit tests. Operator chooses transport per validator instance — both paths must ship and be tested in v1.2.
- **#14 — DOODS2 validator (HTTP only).** New `src/FrigateRelay.Plugins.Doods2/` project: `Doods2Validator : IValidationPlugin`, `Doods2Options` (`BaseUrl`, `DetectorName`, `MinConfidence`, `AllowedLabels`, `OnError`, `Timeout`, `AllowInvalidCertificates`), `Doods2.PluginRegistrar`. `POST /detect` with base64-encoded image + JSON detections back. DOODS2 returns confidence in 0-100 scale; validator normalizes to 0-1 internally. WireMock-driven unit tests. **gRPC scope reverted 2026-05-06** during PLAN-2.1 build — DOODS2 v2 (Python rewrite) is HTTP-only upstream per the project README ("DOODS2 drops support for gRPC as I doubt very much anyone used it anyways"). PLAN-2.3 was REMOVED accordingly. See CONTEXT-14 D4 reversal note for full rationale.
- **#23 — Per-action `ParallelValidators: true`.** New boolean field on `ActionEntry` (default `false`). When `true`, the host's per-action validator chain runs concurrently via `Task.WhenAll`; each validator's own `Timeout` applies; aggregate fails closed if any validator times out (matches existing per-validator `OnError: FailClosed` semantics — "parallel" changes scheduling, not failure semantics). Aggregation: strict AND. First-validator-rejects does **not** short-circuit other in-flight validators — operators get full per-validator visibility on every dispatch (PROJECT.md:228). Each rejecting validator still emits its own `validators.rejected` counter (no behavioral change to the v1.1 counter tag matrix). Affected files: `ActionEntry` (host-internal, declared in `src/FrigateRelay.Host/Configuration/ActionEntry.cs` per RESEARCH §2.5 — NOT in `FrigateRelay.Abstractions`), the validator-execution path in `src/FrigateRelay.Host/Dispatch/ChannelActionDispatcher.cs:200-246`, plus a new integration test exercising ≥ 2 validators in parallel under a single `ActionEntry`.

**Verification (gates reproduced from PROJECT.md:248–256).**
- `dotnet build FrigateRelay.sln -c Release` zero warnings on both Linux and Windows (warnings-as-errors invariant unchanged). New plugin projects compile clean.
- **gRPC dep containment.** `dotnet list src/FrigateRelay.Abstractions/FrigateRelay.Abstractions.csproj package --include-transitive` and `dotnet list src/FrigateRelay.Host/FrigateRelay.Host.csproj package --include-transitive` both show **no** `Grpc.*` transitive entries. The dep lives in `FrigateRelay.Plugins.Doods2` only.
- `git grep -nE 'Grpc\.' src/FrigateRelay.Host src/FrigateRelay.Abstractions` returns empty (host + abstractions stay gRPC-free; gRPC is plugin-local).
- **No gRPC anywhere** (D4 reversed 2026-05-06). `git grep -nE 'Grpc\.' src/` returns empty across the entire repo; no csproj declares Grpc.* packages.
- All existing tests pass. **Test-count gate:** 242 baseline (post-Phase 13) → 242+N expected; architect to determine N during Phase 14 PLAN dispatch (per-PR new-test minimums will be set at PR-1 / PR-2 / PR-3 planning briefs).
- New unit tests per validator, at minimum: allow / reject / timeout / OnError-FailClosed / OnError-FailOpen / cancellation, driven by WireMock for HTTP transports and an in-process gRPC server for DOODS2's gRPC path.
- New unit tests per validator, at minimum: allow / reject / timeout / OnError-FailClosed / OnError-FailOpen / cancellation, all WireMock-driven (DOODS2 gRPC scope reverted; no in-process gRPC server harness needed).
- New integration test demonstrating ≥ 2 validators running in parallel under a single `ActionEntry` with `ParallelValidators: true`. WireMock or Testcontainers as available; the CPAI + Roboflow combination is the smallest meaningful coverage, the CPAI + Roboflow + DOODS2 trio is the target if PR-3's test infrastructure allows it.
- `git grep -nE 'App\.Metrics|OpenTracing|Jaeger\.' src/` still empty (architectural invariant unchanged).
- Three merged PRs on `main` (one per issue), then `v1.2.0` tag — operator-cut per the CONTEXT-12 D7 manual tag-cut policy; release.yml auto-builds + pushes multi-arch GHCR images on the tag push.
Expand Down
Loading
Loading