Files
novelly/docs/plans/api/claude_md_compliance_output.md
T
James Wampler e598c18d67 Add users, roles, and per-novel permissions
Introduces accounts (ASP.NET Identity + cookie auth), four global
roles (Admin/Writer/Editor/Reviewer), per-novel ownership and grants
via ProjectMember, and a service-API-key principal for the MCP server
and background import jobs. Enforcement lives in the application
services (not endpoint filters) so the embedded agent and MCP tools,
which call the same services directly, can't bypass it. Web client
gets a login page, session-aware routing, and a People section for
managing per-novel access.

Also includes prior in-flight changes from this branch (CLAUDE.md
compliance pass, dev-deploy docker-compose setup) that were
uncommitted when this feature work started.
2026-08-15 22:29:33 -07:00

7.2 KiB

CLAUDE.md compliance — implementation summary

Plan: docs/plans/transient-baking-clock.md (four chunks). All four completed.

Chunk 1 — Comments stripped

314 C# comment lines and 53 web comment lines removed across ~40 files (see plan for the file list). Exemptions honored: Novelly.ServiceDefaults/Extensions.cs (Aspire scaffold), vite-env.d.ts (required TS directive). One file the original audit missed and this pass caught: src/Novelly.Web/vite.config.ts had a two-line comment on the dev proxy — removed.

Added scripts/ci/check-no-comments.sh, wired into scripts/ci/prepush.sh before the build. Grep-based trip-wire (excludes Data/Migrations/, the two exempted files, node_modules/dist/bin/obj, and skips https:// matches) — verified it catches real violations and passes on the cleaned tree.

Chunk 2 — else removed, INovelDbContext folded

  • Added AddRequiredTextErrors / AddOptionalTextErrors / AddUnclearableTextErrors to ValidationResultExtensions and pointed all *Contracts.cs validators at them, collapsing ~18 if/else if blocks into one-line calls.
  • Converted the 7 flagged service-level else sites to early return, ternary, or logger.Log(level, ...) with a ternary LogLevel argument (used for the several "two log calls, different level" sites — a value pick, not a branch).
  • INovelDbContext moved to the bottom of NovelDbContext.cs; INovelDbContext.cs deleted.

Chunk 3 — Logging: Warning on rejection/not-found paths

  • ~20 rejection/not-found sites flipped from LogInformation to LogWarning across ProjectService, ChapterService, BeatService, CharacterService, CharacterArcService, TagService, OpenQuestionService, NovelAgentService.
  • Agent tool outcomes (NovelAgentService, ImportAgentService) now log at Warning when outcome.IsError, Information otherwise.
  • ToolNotFound / ImportToolNotFound records changed from a single pre-formatted Message string to (Entity, Id), with Message as a computed property — the tool-not-found log now carries {Entity}/{EntityId} as separate structured properties instead of an interpolated string.
  • ThrowIfInvalid gained an ILogger overload that logs Warning with the failing field names before throwing; every service's Validate(...).ThrowIfInvalid() call now passes logger, so a validation rejection reaching a service directly (agent tools, MCP, tests) is no longer silent.
  • Added missing Debug end-logs: CharacterArcService.EnsureChapterIsInSameProjectAsync / NextSortOrderAsync, BeatService.ResolveCharactersAsync / NextSortOrderAsync, OpenQuestionService.ValidateAssociationsAsync, NovelAgentService.AppendMessageAsync / StartConversation / FindConversationAsync, ImportAgentService.RunOneTurnAsync.
  • Added the missing entry Information log to ImportAgentService.RunAsync.
  • NovelApiClient (MCP) now takes ILogger<NovelApiClient> via DI; the swallowed HttpRequestException now logs Error, and the two swallowed JsonException catches (Prettify, TryReadProblemDetail, both changed from static to instance methods) now log Warning instead of silently falling back.
  • ImportPaths.ResolveRoot no longer drops the original exception — it's passed as innerException on the rethrown ArgumentException. (Left un-logged: ImportPaths is a static utility with no ILogger; threading one through would be a bigger change than this pass's scope, and the exception is still visible to whichever caller has a logger.)
  • Program.cs's exception-handler else collapsed into one logger.Log(level, ...) call driven by the 500-vs-handled ternary.
  • LoggingTests.cs: the one test that asserted the old (rule-violating) behavior — "missing chapter logs at Information, not Warning" — renamed and rewritten to assert the corrected Warning-level behavior. All other tests were unaffected.

Chunk 4 — Coverage config, gate, and two new test files

  • Added [ExcludeFromCodeCoverage] to Program and NovellyServiceRegistration (composition roots, not unit-testable).
  • Added coverlet.msbuild alongside the existing coverlet.collector package reference. Correction to the plan: a .runsettings file with <Threshold> under the XPlat Code Coverage collector does not actually fail the run — verified directly (dotnet test --collect:"XPlat Code Coverage" --settings .runsettings exited 0 at 54.8% coverage, well under the configured 70% floor). Deleted the .runsettings file rather than ship a threshold that silently does nothing. coverlet.msbuild's /p:Threshold does enforce (verified: exits 1 below the threshold, 0 at or above it), so prepush.sh now gates through MSBuild properties instead.
  • Added ChapterServiceTests.cs and CharacterServiceTests.cs (previously the two largest services with no owning test file). Both use the existing ServiceTestFixture / TestDatabase pattern, BDD-named, covering create/patch-semantics/delete, the cross-project relationship rejection, and cascade behavior (chapter delete takes its beats; character delete detaches from beats without deleting them).
  • Grouped the two genuinely-adjacent bare Assert.That pairs in ImportServiceTests.cs (:96-97, :108-109) into Assert.Multiple. Left the other flagged sites alone — each interleaves an act step between asserts, so grouping them would obscure ordering rather than clarify it.

Coverage floor set to 60, not 70. Real Novelly.Api line coverage after the two new test files is 60.53%, not 70% — the gap is concentrated in code the plan explicitly put out of scope: every *Endpoints.cs file and the two endpoint filters (RequestLoggingEndpointFilter, ValidationEndpointFilter) have zero tests, since there's no WebApplicationFactory/HttpClient integration-test setup in the project yet. Setting the gate to 70 today would fail every push until that follow-up work lands, which is a worse outcome than a lower floor that (a) still catches regressions and (b) is honest about where the codebase actually stands. Endpoint/integration tests remain the deliberately deferred next chunk — building the WebApplicationFactory harness and covering the five endpoint groups would very likely clear 70 on its own.

Verification

  • dotnet build Novelly.slnx -c Debug — succeeds (pre-existing nullable warnings only, unrelated to this change).
  • dotnet test tests/Novelly.Api.Tests/Novelly.Api.Tests.csproj -c Debug — 114/114 pass (100 existing + 14 new, one existing test rewritten as noted above).
  • npm --prefix src/Novelly.Web run build — succeeds.
  • scripts/ci/check-no-comments.sh — passes on the cleaned tree, was verified to fail before the vite.config.ts fix.
  • bash scripts/ci/prepush.sh — full run end to end, exits 0 (build, comment guard, coverage-gated tests, web build).

Not run: the app itself (Aspire host / API / MCP stdio) was not exercised live in this pass — the changes here are logging levels, validation-message plumbing, and test additions, not endpoint or protocol-shape changes, so build+test coverage was judged sufficient. Worth a live smoke pass before merging if the reviewer wants to see the corrected log levels in the Aspire dashboard.