Conversation
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Test Results 57 files + 40 57 suites +40 34m 19s ⏱️ + 23m 18s Results for commit c10e47d. ± Comparison against base commit 7a9b9bd. This pull request removes 5 and adds 26 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
# Conflicts: # Directory.Packages.props
Review Summary by QodoReplace Spyglass reflection with compile-time code generation
WalkthroughsDescription• Replace reflection-based Spyglass API with compile-time code generation • Implement incremental Roslyn source generator for aggregate discovery • Support standalone state types for functional command services • Refactor sample apps to use generated registration and simplified patterns Diagramflowchart LR
A["Aggregates & States"] -->|Roslyn Generator| B["SpyglassGenerator"]
B -->|Discovers at compile-time| C["Module Initializer"]
C -->|Auto-registers| D["SpyglassRegistry"]
D -->|Serves| E["Spyglass API"]
F["InsidePeek<br/>Reflection"] -.->|Removed| G["No Runtime Reflection"]
File Changes1. src/Experimental/gen/Eventuous.Spyglass.Generators/SpyglassGenerator.cs
|
Code Review by Qodo
1. key logged in SpyglassApi
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if (!builder.ServiceProvider.GetRequiredService<IWebHostEnvironment>().IsDevelopment() && key == null) { | ||
| logger.LogWarning("Insecure Spyglass API is only available in development environment"); | ||
| key = Guid.NewGuid().ToString("N"); | ||
| logger.LogInformation("Using generated key: {Key}", key); | ||
| } |
There was a problem hiding this comment.
1. key logged in spyglassapi 📘 Rule violation ⛨ Security
The Spyglass API logs the generated access key at Information level, which can leak a secret into centralized logs. Anyone with log access could reuse the key to access Spyglass endpoints and potentially view internal state/events.
Agent Prompt
## Issue description
The Spyglass API logs a generated access key value (`{Key}`), which is a secret and should never be written to application logs.
## Issue Context
This key gates access to Spyglass endpoints; logging it makes it retrievable from log aggregation systems and enables unauthorized use.
## Fix Focus Areas
- src/Experimental/src/Eventuous.Spyglass/SpyglassApi.cs[28-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
It logs the secret deliberately, otherwise the user won't know what secret to use. It only happens in development.
There was a problem hiding this comment.
Question:
It logs the secret deliberately, otherwise the user won't know what secret to use. It only happens in development.
Answer:
You can reduce (or eliminate) this kind of “dev-only secret logging” finding by guiding Qodo Merge with targeted extra_instructions, and (optionally) by disabling the security subsection/labeling if it’s too noisy for your repo.
Recommended (keep security checks, but allow explicit dev-only exceptions):
[pr_reviewer]
extra_instructions = """\
Security findings:
- Treat secrets in logs as a finding ONLY if they can occur outside Development/local runs.
- If the code is clearly guarded to Development-only (e.g., environment checks / debug-only compilation flags) and cannot run in production, do not flag it as a security violation; instead add a short note recommending masking/redaction and adding an explicit warning comment.
"""
[pr_code_suggestions]
extra_instructions = """\
When suggesting fixes for secrets-in-logs:
- First verify whether the logging happens only in Development/local runs.
- If it is dev-only, do not suggest removing it unconditionally; prefer suggesting safer alternatives (masking, one-time display, explicit warning text, or printing only a prefix/suffix).
"""If you want to suppress security findings entirely in /review (more aggressive):
[pr_reviewer]
require_security_review = false
# alternatively/also:
enable_review_labels_security = falseIf you share whether this finding came from /review or /improve (or /compliance), I can narrow the TOML to just the relevant tool section(s).
Relevant Sources:
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/review#extra-instructions
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/review#configuration-options
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/improve#extra-instructions-and-best-practices
- https://docs.qodo.ai/qodo-documentation/qodo-merge/tools/improve#configuration-options
| sb.AppendLine(" var events = await eventStore.ReadStream(streamName, StreamReadPosition.Start, true, default);"); | ||
| sb.AppendLine(" var selected = version == -1 ? events : events.Take(version + 1).ToArray();"); | ||
| sb.AppendLine(" aggregate.Load(selected.Length > 0 ? selected[^1].Revision : -1, selected.Select(x => x.Payload));"); | ||
| sb.AppendLine(" return new SpyglassLoadResult("); | ||
| sb.AppendLine(" aggregate.State,"); | ||
| sb.AppendLine(" events.Select(x => new SpyglassEventInfo(x.Payload!.GetType().Name, x.Payload)).ToArray());"); | ||
| sb.AppendLine(" }"); |
There was a problem hiding this comment.
2. Versioned load inconsistent 🐞 Bug ✓ Correctness
Generated load delegates rehydrate state using a version-truncated event list but return the full (untruncated) event list to the API response. This can mislead callers because the returned events won’t match the returned state when version != -1.
Agent Prompt
### Issue description
The generated Spyglass load delegates apply `version` to state rehydration but still return *all* events from the stream, causing state/events mismatch.
### Issue Context
Spyglass is used for debugging/introspection; returning inconsistent data makes the API misleading.
### Fix Focus Areas
- src/Experimental/gen/Eventuous.Spyglass.Generators/SpyglassGenerator.cs[177-203]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| sb.AppendLine(" var events = await eventStore.ReadStream(streamName, StreamReadPosition.Start, true, default);"); | ||
| sb.AppendLine(" var selected = version == -1 ? events : events.Take(version + 1).ToArray();"); |
There was a problem hiding this comment.
3. Missing stream yields 500 🐞 Bug ⛯ Reliability
The generated load delegates call ReadStream(..., failIfNotFound: true) and Spyglass API doesn’t handle StreamNotFound, so loading an unknown entity likely becomes an unhandled exception (500) instead of a 404/empty result.
Agent Prompt
### Issue description
Loading a non-existent stream is likely to throw `StreamNotFound` and surface as a 500 because SpyglassApi doesn’t catch it.
### Issue Context
`StoreFunctions.ReadStream` only swallows `StreamNotFound` when `failIfNotFound` is false.
### Fix Focus Areas
- src/Experimental/gen/Eventuous.Spyglass.Generators/SpyglassGenerator.cs[177-203]
- src/Experimental/src/Eventuous.Spyglass/SpyglassApi.cs[60-72]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| sb.AppendLine(" events.Select(x => new SpyglassEventInfo(x.Payload!.GetType().Name, x.Payload)).ToArray());"); | ||
| sb.AppendLine(" }"); | ||
| sb.AppendLine(" ));"); | ||
| } | ||
| } | ||
|
|
||
| if (!states.IsDefaultOrEmpty) { | ||
| foreach (var s in states.Distinct()) { | ||
| sb.AppendLine(" SpyglassRegistry.Register(new SpyglassAggregateInfo("); | ||
| sb.AppendLine(" null,"); | ||
| sb.AppendLine($" \"{Escape(s.StateSimpleName)}\","); | ||
| sb.AppendLine(" System.Array.Empty<string>(),"); | ||
| sb.AppendLine($" new {s.StateFqn}().GetRegisteredEventTypes().Select(t => t.Name).ToArray(),"); | ||
| sb.AppendLine($" static (_, entityId) => StreamName.ForState<{s.StateFqn}>(entityId),"); | ||
| sb.AppendLine(" static async (eventStore, streamName, version) => {"); | ||
| sb.AppendLine(" var events = await eventStore.ReadStream(streamName, StreamReadPosition.Start, true, default);"); | ||
| sb.AppendLine(" var selected = version == -1 ? events : events.Take(version + 1).ToArray();"); | ||
| sb.AppendLine($" var state = selected.Select(x => x.Payload!).Aggregate(new {s.StateFqn}(), (s, e) => s.When(e));"); | ||
| sb.AppendLine(" return new SpyglassLoadResult("); | ||
| sb.AppendLine(" state,"); | ||
| sb.AppendLine(" events.Select(x => new SpyglassEventInfo(x.Payload!.GetType().Name, x.Payload)).ToArray());"); |
There was a problem hiding this comment.
4. Null payload crashes load 🐞 Bug ⛯ Reliability
Generated code null-forces StreamEvent.Payload when folding state and when computing event type names. Since StreamEvent.Payload is nullable, a null payload can cause NullReferenceException and break /spyglass/load.
Agent Prompt
### Issue description
The generated Spyglass load code uses `Payload!` but `StreamEvent.Payload` is nullable. Null payloads can crash the endpoint.
### Issue Context
Domain aggregate load already defensively filters null events.
### Fix Focus Areas
- src/Experimental/gen/Eventuous.Spyglass.Generators/SpyglassGenerator.cs[177-203]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Avoid using reflections for Spyglass API.
Breaking change: current version of Spyglass won't be able to load streams with this version of the API.