Release v0.3.1: eighteen fixes from reviews run to a clean pass - #35
Merged
Conversation
The guidance claimed a Result discarded as the body of a void-returning lambda escapes the analyzer. It does not: Roslyn lowers the lambda body to an expression statement, the exact operation the analyzer registers for. Confirmed by running the analyzer over both lambda shapes — each reports LOOM0001 — and over an unawaited Task<Result>, which stays silent and is now the one documented blind spot. Both cases are pinned as tests, which is how the claim should have been checked before it was written down.
The archetype guidance called result.ToExitCode() as though a package shipped it. None does — the roadmap concedes it twice — and the core rule that category-to-transport mapping comes from a Loom package made the guidance unsatisfiable as written. The snippet now carries a callout naming the gap and forbidding an inline bridge.
Both templates' registration tests discovered only IHandler<,>, so a void handler whose AddHandler call was forgotten resolved nothing and failed no test — the exact silence the test's own remarks claim to prevent, and the ordinary command shape in the sample. The predicate now covers both arities, as the sample's already did; the enforcement rule names them both.
LoomVersion still said 0.2.0 with 0.3.0 published, so CI compiled every scaffold against an API one release behind the packages a released template names — on a 0.x line where a minor may break. The pin lags one release by design, because at tag time the tagged packages are not on nuget.org yet; the defect was that nothing moved it afterwards and nothing said to. Bumped to 0.3.0 — a scaffold restores, builds and passes against it — the after-release step is now in section 8, and a test holds the two archetypes to the same pin, which the byte-for-byte shared-file check cannot cover because their package sets legitimately differ. Verified the test fails when one pin is moved alone.
…e leans on WidgetErrors.NotFound was copied from loom-api, where a get-by-id slice returns it; the worker has no such slice and nothing referenced the property. The pass's NotFound branch is unaffected — it acts on the category and is exercised by a stub. Retire()'s AlreadyRetired branch is the invariant the slice's comment leans on — a widget retired concurrently reports Conflict and the loop keeps going — and no domain test covered it. Two now do. Scaffolded worker: 25 tests pass.
A --name containing /, & or backslash was spliced into the sed expression, where each is syntax: a slash fails loudly but & and backslash rewrite the output silently. The name is now validated against the shapes the template engine accepts and substituted with bash's own literal expansion, which also removes the GNU-only sed -i. Byte-identical output for every clean name, including the three CI stress-tests; the four hostile shapes are refused with the reason.
The escape-hatch test read back its own assignment, which cannot fail; it now pins that the mapped status and code extension stand after mutation. Entities_Are_Sealed asserted every public domain class, so it is named for what it does.
release.yml scopes its token explicitly; CI ran at the repository default, which can include write. Nothing in CI pushes, comments or uploads through the API, so contents: read is the whole requirement.
AddProblemDetails registered the service but nothing invoked it outside ToHttpResult(): an unhandled exception returned a bodyless 500 and an unmatched route a bodyless 404, against the rule that every non-2xx response is ProblemDetails. UseExceptionHandler and UseStatusCodePages pick the registered service up, in the template and the sample both. Pinned through HTTP with a route no slice owns: application/problem+json with the middleware, bodyless without — verified both ways against a scaffolded solution.
Both fixtures migrate the container schema, then reset with a Respawner that also truncated __EFMigrationsHistory. Nothing at HEAD re-migrates after a reset, so no test failed — but the first consumer addition that does, such as a pending-migrations startup check, replays every migration into tables that already exist. The sample already ignored the table; the templates now match it.
The examples read namespace MyApp.Features.Orders.CreateOrder while every real slice, both archetypes and the sample, uses the host-qualified root — and the architecture test watches <HostAssembly>.Features. only. A slice written from the doc's literal example landed outside the enforcement that section calls mechanical.
Program.cs says the AppHost supplies the connection string and refuses to start without one; the committed localhost fallback made that refusal unreachable and put credentials in an appsettings file against the section 8 rule. The signing key stays: its deviation is documented in the README and on the options type, and startup refuses it outside Development. Tests inject their own string and still pass.
The line predated the matrix: two archetypes by three names, not one solution.
Error<TMetadata> compares metadata with the default comparer — by reference for a dictionary — so two ValidationErrors from identical inputs are unequal, and nothing said so. The remark names the surprise and points at Code, the documented discriminator; the test makes the claim checkable.
The comment said the pass counts Conflict as information; the pass dead-letters it. The absorption is the slice's per-widget loop.
The ToExitCode callout sat a section below the snippet that calls it, so a reader met the code before the caveat; the snippet now says it is illustrative where it starts. The 404 pin asserted the media type, which is a header — it now parses the body as ProblemDetails and asserts the status inside it.
Fix sixteen findings from four review rounds run to a clean pass
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR updates result diagnostics and equality documentation, adds ProblemDetails handling to API templates and samples, expands worker and template tests, synchronizes template versions, and revises CI, architecture guidance, CLI guidance, and agent-generation validation. ChangesTemplate and Result Infrastructure Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request as ASP.NET Core request
participant ExceptionMiddleware as Exception handling middleware
participant StatusMiddleware as Status-code middleware
participant ProblemDetails as ProblemDetails service
Request->>ExceptionMiddleware: Execute request pipeline
ExceptionMiddleware->>StatusMiddleware: Continue for non-exception responses
StatusMiddleware->>ProblemDetails: Format unmatched or non-success response
ProblemDetails-->>Request: Return ProblemDetails response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/Loom.Templates/templates/loom-api/AGENTS.md`:
- Line 179: Update the namespace examples in the documentation fragments so
host-rooted namespaces are consistent: in
src/Loom.Templates/templates/loom-api/AGENTS.md lines 179-179, change the later
example to MyApp.Api.Features..., and in
src/Loom.Templates/templates/loom-worker/AGENTS.md lines 179-179, change it to
MyApp.Worker.Features.... Regenerate the template AGENTS.md files with
scripts/new-agents-md.sh, then run scripts/check-docs.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b692b36d-b035-4d67-a1d1-01a589307711
📒 Files selected for processing (27)
.github/workflows/ci.ymlAGENTS.mddocs/ROADMAP.mddocs/agents/00-core.mddocs/agents/10-cli.mdsamples/src/Ordering.Api/Program.cssamples/src/Ordering.Api/appsettings.Development.jsonscripts/new-agents-md.shsrc/Loom.Results/Error.cssrc/Loom.Templates/templates/loom-api/AGENTS.mdsrc/Loom.Templates/templates/loom-api/Directory.Packages.propssrc/Loom.Templates/templates/loom-api/src/MyApp.Api/Program.cssrc/Loom.Templates/templates/loom-api/tests/MyApp.Api.Tests/RegistrationTests.cssrc/Loom.Templates/templates/loom-api/tests/MyApp.Api.Tests/TestApp.cssrc/Loom.Templates/templates/loom-api/tests/MyApp.Api.Tests/WidgetSliceTests.cssrc/Loom.Templates/templates/loom-api/tests/MyApp.ArchitectureTests/DomainPurityTests.cssrc/Loom.Templates/templates/loom-worker/AGENTS.mdsrc/Loom.Templates/templates/loom-worker/Directory.Packages.propssrc/Loom.Templates/templates/loom-worker/src/MyApp.Domain/Widgets/WidgetErrors.cssrc/Loom.Templates/templates/loom-worker/tests/MyApp.ArchitectureTests/DomainPurityTests.cssrc/Loom.Templates/templates/loom-worker/tests/MyApp.Domain.Tests/WidgetTests.cssrc/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests/RegistrationTests.cssrc/Loom.Templates/templates/loom-worker/tests/MyApp.Worker.Tests/WorkerHost.cstests/Loom.Results.Analyzers.Tests/DiscardedResultAnalyzerTests.cstests/Loom.Results.AspNetCore.Tests/ResultHttpExtensionsTests.cstests/Loom.Results.Tests/ResultTests.cstests/Loom.Templates.Tests/TemplateManifestTests.cs
💤 Files with no reviewable changes (2)
- samples/src/Ordering.Api/appsettings.Development.json
- src/Loom.Templates/templates/loom-worker/src/MyApp.Domain/Widgets/WidgetErrors.cs
Section 12's example was API-rooted, and both templates bake the same fragment — so a worker's own guidance illustrated the rule with an Api namespace. Both roots now shown, the same way section 4 already does. Per-archetype examples are not possible without diverging the baked files from regeneration, which check-docs refuses.
Show both archetype roots in the add-a-slice example
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four two-axis review rounds run in a loop until a full round returned no findings, plus the
follow-up review of the resulting PR. Eighteen fixes; no change to any package's public surface.
fix
Resultdiscarded asa void lambda's body escapes
LOOM0001; running the analyzer disproved it — Roslyn lowers thelambda body to the exact operation the analyzer registers for. Both lambda shapes and the one real
blind spot, an unawaited
Task<Result>, are pinned as tests. The false claim shipped in bothtemplates' baked guidance.
IHandler<,>was discovered, so a voidhandler with a forgotten
AddHandlerresolved nothing and failed no test. Both arities nowdiscovered.
AddProblemDetails()was registered but nothing invoked itoutside
ToHttpResult(): unhandled exceptions returned empty 500s, unmatched routes empty 404s.UseExceptionHandler+UseStatusCodePagesin template and sample; pinned by parsing the problembody of a route no slice owns, verified failing without the middleware.
__EFMigrationsHistory, so the first consumer addition that re-touches migrations would replaythem into a populated schema. Now ignored, as the sample already did.
new-agents-md.shspliced--nameinto a sed script, where&and\corrupt outputsilently. Names validated, substitution literal, GNU-only
sed -igone.step now runs under a catch that disposes and rethrows, with the catch path mutation-verified.
build / ci
LoomVersion0.2.0 with 0.3.0 published, so CI compiled scaffolds one releasebehind. Bumped, the after-release step recorded in §8, and a test holds both archetypes to one pin.
contents: read, matching release.yml.docs
MyApp.Features…while the architecture test watches thehost-qualified root — a slice written from the literal example escaped enforcement. Corrected.
Password=postgresinappsettings.Development.jsonagainst the secretsrule, and the fallback made the documented startup refusal unreachable. Removed.
10-cli.mdcalledresult.ToExitCode()as though shipped; the snippet is now marked illustrativewith a callout naming the gap and forbidding an inline bridge.
Error<TMetadata>compares dictionary metadata by reference — stated in the remarks and pinned,since two
ValidationErrors from identical inputs are unequal and nothing said so.AggregateRootandToPageremarks corrected in 0.3.0 get their follow-ups here: the worker'sdead
WidgetErrors.NotFoundremoved,Retire()'s Conflict branch tested, the ROADMAP's CIdescription matches the actual matrix.
test
mutation.
Entities_Are_Sealedrenamed to what it asserts.PageRequest.Firstand the analyzer'slambda/task behaviour pinned. Where a fix added a test, the test was mutation-checked — verified to
fail with the defect reintroduced.
Verification
check-docs.sh,dotnet format --verify-no-changes, build with warnings as errors, 232 tests, thesample's 46, and both archetypes packed, scaffolded and tested against the published 0.3.0 packages
across CI's full 2×3 name matrix.
Summary by CodeRabbit
New Features
Documentation
Maintenance