Fix #258#266
Open
Carl-Johnsons wants to merge 14 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/utils/setup-mise.py (1)
213-215:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
resolve_powershell_profile()returns on empty output and skips valid output.The condition is inverted at Line 213. With a valid profile path, the function doesn’t return and may end by raising
PowerShell is not found, breaking activation flow on Windows.Suggested patch
- if not profile_path: - return Path(profile_path) + if profile_path: + return Path(profile_path)As per coding guidelines, runtime order/branch correctness issues that break execution paths must be fixed before release.
🤖 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 `@scripts/utils/setup-mise.py` around lines 213 - 215, The branch in resolve_powershell_profile is inverted causing valid profile output to be ignored: change the logic so that when profile_path is non-empty you immediately return Path(profile_path) (i.e., if profile_path: return Path(profile_path)), and only proceed to the "PowerShell is not found" or error path when profile_path is empty or None; update the return/conditional around profile_path in resolve_powershell_profile accordingly to restore correct Windows activation flow.
🤖 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 `@app/server/Contract/Contract/Utilities/EnvUtility.cs`:
- Line 57: EnvUtility.GetConnectionString currently appends "SSL
Mode=Require;Trust Server Certificate=true;" unconditionally; change it so the
"Trust Server Certificate=true" flag is only added when running in local
development (e.g., check an environment/config flag like ASPNETCORE_ENVIRONMENT
== "Development" or a dedicated EnvUtility.IsDevelopment), otherwise omit that
directive so production/staging use strict TLS validation; update
GetConnectionString to conditionally include the Trust Server Certificate
portion and ensure callers like the IdentityServer AddOperationalStore still use
the secure connection string.
In
`@app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs`:
- Around line 31-40: The AddOperationalStore ConfigureDbContext calls (in
AddMinimalInfrastructureServices() within DependencyInjection.cs and the
AddOperationalStore extension in DuendeIdentityServer/HostingExtensions.cs) use
UseNpgsql(...) without a retry policy; update those UseNpgsql invocations to
configure resilient retries by passing a Npgsql-specific SQL options lambda that
calls EnableRetryOnFailure (e.g., sql => {
sql.MigrationsAssembly("IdentityService.Infrastructure");
sql.EnableRetryOnFailure(); }) so persisted-grant/session DB operations use
transient-fault retry; ensure the same pattern is applied in both
AddOperationalStore usages and keep the MigrationsAssembly setting intact.
In `@mise.toml`:
- Around line 9-10: Replace the non-deterministic version pin in the tools table
by changing the version field for [tools."github:PhenChua29/maestro-runner"]
from "latest" to a specific release tag (for example "v1.x.y" or the exact
released tag you want to lock to); update the version string in the mise.toml
entry so CI and local bootstrapping use the same immutable maestro-runner
version.
In `@scripts/ci/build/derive-service-state.py`:
- Line 46: The exclude pattern for the mobile Service is only excluding the
directory path itself; update the exclude_paths for the mobile Service (the
mobile variable/Service(ServiceName.MOBILE, ...)) to use a recursive glob such
as "app/client/mobile/.maestro/**" (or an equivalent pattern) so all files under
that directory are excluded rather than just the directory entry.
In `@scripts/local/apply-all-migrations.sh`:
- Around line 42-47: The script currently only echoes "$env_path file not
found." when an expected env file is missing, which allows subsequent dotnet run
to proceed with wrong environment; modify the checks in apply-all-migrations.sh
(the blocks that reference env_path and the similar block at the later
occurrence) to fail fast by exiting with a non-zero code after logging the
error: replace the echo-only behavior with a processLogger-style error/echo then
call exit 1 so the script stops immediately when a required env file (env_path)
is absent; update both occurrences around the env_path check (the first block
that exports env variables and the later block at the 62-66 equivalent) to use
this exit-on-missing-file behavior.
- Line 28: The current use of export $(grep -v '^#' $ENV_FILE | xargs) in
apply-all-migrations.sh is unsafe; replace both occurrences with a safe
line-by-line parser that reads $ENV_FILE with while IFS= read -r line; skips
empty lines and comments, splits only on the first '=' to produce KEY and VALUE,
strips surrounding quotes without performing command substitution, and then
export the KEY with the literal VALUE (or use declare -x) so keys with
spaces/special chars are preserved and no unintended code executes; locate the
occurrences referencing ENV_FILE in the script and implement this pattern for
both places.
In `@scripts/utils/setup-mise.py`:
- Line 1: The script setup-mise.py uses a shebang of "#!/usr/bin/env python"
which fails on systems that only have python3; update the shebang to
"#!/usr/bin/env python3" so the script is executable with python3, or
alternatively modify the caller in scripts.json to invoke "python3
./scripts/utils/setup-mise.py" instead; make the change in setup-mise.py's top
line (the shebang) to ensure the existing installation/activation logic runs
when only python3 is installed.
---
Outside diff comments:
In `@scripts/utils/setup-mise.py`:
- Around line 213-215: The branch in resolve_powershell_profile is inverted
causing valid profile output to be ignored: change the logic so that when
profile_path is non-empty you immediately return Path(profile_path) (i.e., if
profile_path: return Path(profile_path)), and only proceed to the "PowerShell is
not found" or error path when profile_path is empty or None; update the
return/conditional around profile_path in resolve_powershell_profile accordingly
to restore correct Windows activation flow.
🪄 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: f18fa4ca-b61f-4ecd-a3fe-e437b7679abf
📒 Files selected for processing (22)
.infisical.jsonJenkinsfileapp/server/Contract/Contract/Utilities/EnvUtility.csapp/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.csapp/server/IdentityService/src/DuendeIdentityServer/Controllers/AdminController.csapp/server/IdentityService/src/DuendeIdentityServer/DTOs/AdminGetAccountStatistic.dto.csapp/server/IdentityService/src/DuendeIdentityServer/DuendeIdentityServer.csprojapp/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.csapp/server/IdentityService/src/IdentityService.Application/Account/Commands/RegisterAccountCommandHandler.csapp/server/IdentityService/src/IdentityService.Application/Account/Queries/AdminGetNumberOfAccountStatisticQueryHandler.csapp/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.csapp/server/IdentityService/src/IdentityService.Infrastructure/IdentityService.Infrastructure.csprojapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.csapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/20260611045123_InitialIdentityServerPersistedGrantDbMigration.Designer.csapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/20260611045123_InitialIdentityServerPersistedGrantDbMigration.csapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/PersistedGrantDbContextModelSnapshot.csdocker-compose.ymlmise.tomlnginx.confscripts/ci/build/derive-service-state.pyscripts/local/apply-all-migrations.shscripts/utils/setup-mise.py
💤 Files with no reviewable changes (4)
- .infisical.json
- nginx.conf
- docker-compose.yml
- Jenkinsfile
Comment on lines
31
to
+40
| services.AddDbContext<IApplicationDbContext, ApplicationDbContext>(options => | ||
| options.UseNpgsql(Contract.Utilities.EnvUtility.GetConnectionString())); | ||
| services.AddIdentityServer() | ||
| .AddOperationalStore(options => | ||
| { | ||
| options.ConfigureDbContext = b => | ||
| b.UseNpgsql( | ||
| Contract.Utilities.EnvUtility.GetConnectionString(), | ||
| sql => sql.MigrationsAssembly("IdentityService.Infrastructure")); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Npgsql registrations and retry-policy presence across related contexts.
rg -n --type=cs -C3 'UseNpgsql\(' \
app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs \
app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs \
app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.cs
rg -n --type=cs -C3 'EnableRetryOnFailure' \
app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs \
app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs \
app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.csRepository: Carl-Johnsons/tastopia
Length of output: 4597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant code blocks for operational-store wiring
sed -n '1,120p' app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs
echo "----"
sed -n '1,140p' app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs
echo "----"
sed -n '1,120p' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.cs
echo "----"
# Trace whether operational-store uses ApplicationDbContext or a different Duende context
rg -n --type=cs 'OperationalStore|AddOperationalStore|ConfigureDbContext|ApplicationDbContext|IApplicationDbContext' \
app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs \
app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs \
app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.csRepository: Carl-Johnsons/tastopia
Length of output: 14374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=cs 'AddOperationalStore\(' app/server/IdentityService/src
rg -n --type=cs 'EnableRetryOnFailure' app/server/IdentityService/srcRepository: Carl-Johnsons/tastopia
Length of output: 464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=cs 'PersistedGrant|OperationalStore|DeviceFlow|ConfigurationDbContext|PersistedGrantDbContext|Granular?DbContext' app/server/IdentityService/srcRepository: Carl-Johnsons/tastopia
Length of output: 7543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the actual DbContext classes used by Duende operational store
rg -n --type=cs 'class\s+PersistedGrantDbContext\b|DbContext\s*:\s*' \
app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer
# Show PersistedGrantDbContext and any other operational-store DbContext implementations
fd -a 'PersistedGrantDbContext*.cs' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer || true
fd -a '*.cs' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer | wc -l
# Print the relevant DbContext class bodies (small files/sections only)
for f in $(fd -a 'PersistedGrantDbContext*.cs' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer); do
echo "---- $f ----"
sed -n '1,160p' "$f"
done
# Also check DeviceFlow (if present)
for f in $(rg -l --type=cs 'class\s+\w*DeviceFlow\w*DbContext|DeviceFlow.*DbContext' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer); do
echo "---- $f ----"
sed -n '1,200p' "$f"
done
# Confirm how AppCommandHandler migrates persisted grant DbContext
sed -n '1,120p' app/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.csRepository: Carl-Johnsons/tastopia
Length of output: 48
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify operational-store DbContext classes in our repo
# (we expect ones like PersistedGrantDbContext, DeviceFlowCodesDbContext, etc.)
mapfile -t ctx_files < <(
fd -a '*.cs' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer \
| head -n 200
)
echo "Context candidate files: ${`#ctx_files`[@]} (showing up to 200)"
for f in "${ctx_files[@]}"; do
rg -n --type=cs 'class\s+\w*DbContext\b|:?\s*DbContext\b|\[DbContext' "$f" || true
done
# Specifically look for PersistedGrantDbContext definition and any DbContext base with retry
mapfile -t target_files < <(
(fd -a 'PersistedGrantDbContext*.cs' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer || true)
)
echo "PersistedGrantDbContext files: ${`#target_files`[@]}"
for f in "${target_files[@]}"; do
echo "---- $f ----"
sed -n '1,220p' "$f"
done
# Also inspect the persisted grant model snapshot file where retry might be set/expected via configuration
sed -n '1,140p' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/PersistedGrantDbContextModelSnapshot.cs
# Inspect AppCommandHandler migration wiring
sed -n '1,120p' app/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.csRepository: Carl-Johnsons/tastopia
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
base="app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer"
echo "Listing DbContext-related files under $base (first 200):"
fd -a '*.cs' "$base" | head -n 200
echo "---- PersistedGrantDbContext* ----"
fd -a 'PersistedGrantDbContext*.cs' "$base" || true
echo "---- Files containing 'class' + 'DbContext' under $base ----"
rg -n --type=cs 'class\s+\w*DbContext\b' "$base" || true
rg -n --type=cs '\[DbContext\(' "$base" || true
echo "---- AppCommandHandler migration wiring ----"
sed -n '1,160p' app/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.cs
echo "---- HostingExtensions AddOperationalStore block ----"
sed -n '50,120p' app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs
echo "---- DependencyInjection AddMinimalInfrastructureServices operational store block ----"
sed -n '1,120p' app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.csRepository: Carl-Johnsons/tastopia
Length of output: 508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show top of files to see using directives for TimeSpan (and EF Core types)
sed -n '1,80p' app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs
echo "----"
sed -n '1,110p' app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs
echo "----"
sed -n '1,120p' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.cs
echo "----"
base="app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer"
echo "Listing ${base} (few levels):"
find "$base" -maxdepth 4 -type f -name '*.cs' | head -n 200
echo "----"
rg -n --type=cs 'PersistedGrantDbContext|DeviceFlowCodes|ConfigurationDbContext|AddOperationalStore|ConfigureDbContext' "$base" app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.csRepository: Carl-Johnsons/tastopia
Length of output: 13683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for global usings that might provide TimeSpan
rg -n --type=cs 'global using\s+System;' . || true
rg -n --type=cs 'global using\s+System\.Collections|global using\s+System\.Threading|global using\s+System\.' . || true
# Check if DependencyInjection.cs or nearby files use TimeSpan already (without an explicit using)
rg -n --type=cs 'TimeSpan\.FromSeconds' app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs || trueRepository: Carl-Johnsons/tastopia
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find any global usings / using System in IdentityService project
rg -n --type=cs 'global using System\b|using System;' app/server/IdentityService || true
rg -n --type=cs 'TimeSpan\.From' app/server/IdentityService || true
# 2) Find where PersistedGrantDbContext is implemented and whether it sets retries
rg -n --type=cs 'class\s+PersistedGrantDbContext\b|PersistedGrantDbContext\s*:\s*DbContext' app/server/IdentityService/src || true
rg -n --type=cs 'EnableRetryOnFailure' app/server/IdentityService/src || true
rg -n --type=cs 'OnConfiguring\s*\(' app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer || trueRepository: Carl-Johnsons/tastopia
Length of output: 4792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any definition of PersistedGrantDbContext in the repo
rg -n --type=cs 'class\s+PersistedGrantDbContext\b' app/server/IdentityService/src || true
rg -n --type=cs 'PersistedGrantDbContext\b' app/server/IdentityService/src/DuendeIdentityServer app/server/IdentityService/src/IdentityService.Infrastructure || true
# Inspect AppCommandHandler where persisted grant DbContext is migrated
sed -n '1,140p' app/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.csRepository: Carl-Johnsons/tastopia
Length of output: 2031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find IdentityService csproj(s)
fd -a -e csproj 'IdentityService*.csproj' . || true
fd -a -e csproj '*IdentityService*.csproj' app/server/IdentityService || true
# If exactly one, print relevant property lines around ImplicitUsings / TargetFramework
for proj in $(fd -a -e csproj '*IdentityService*.csproj' app/server/IdentityService || true); do
echo "---- $proj ----"
sed -n '1,200p' "$proj" | rg -n 'ImplicitUsings|TargetFramework|LangVersion' || true
done
# Also check for repo-wide Directory.Build.props that could set ImplicitUsings
for f in $(fd -a 'Directory.Build.props' . || true); do
echo "---- $f (ImplicitUsings) ----"
rg -n 'ImplicitUsings' "$f" || true
doneRepository: Carl-Johnsons/tastopia
Length of output: 674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find IdentityService csproj(s) using glob (fd defaults to regex)
projs=$(fd -a --glob '*IdentityService*.csproj' app/server || true)
echo "$projs"
for proj in $projs; do
echo "---- $proj ----"
sed -n '1,200p' "$proj" | rg -n 'ImplicitUsings|TargetFramework|LangVersion' || true
done
# Check for repo-wide Directory.Build.props / Directory.Build.targets
for f in $(fd -a --glob 'Directory.Build.props' . || true); do
echo "---- $f ----"
rg -n 'ImplicitUsings|TargetFramework|LangVersion' "$f" || true
done
for f in $(fd -a --glob 'Directory.Build.targets' . || true); do
echo "---- $f ----"
rg -n 'ImplicitUsings|TargetFramework|LangVersion' "$f" || true
doneRepository: Carl-Johnsons/tastopia
Length of output: 1036
Add Npgsql EnableRetryOnFailure to Duende IdentityServer operational-store DbContext configuration.
IdentityService.Infrastructure/DependencyInjection.cs’s AddMinimalInfrastructureServices() configures AddOperationalStore(...).ConfigureDbContext with UseNpgsql(...) but no retry policy, weakening resilience for persisted grants/session cleanup during transient DB faults. Same omission exists in DuendeIdentityServer/HostingExtensions.cs’s AddOperationalStore(...).
Suggested patch
services.AddDbContext<IApplicationDbContext, ApplicationDbContext>(options =>
- options.UseNpgsql(Contract.Utilities.EnvUtility.GetConnectionString()));
+ options.UseNpgsql(
+ Contract.Utilities.EnvUtility.GetConnectionString(),
+ sql => sql.EnableRetryOnFailure(
+ maxRetryCount: 10,
+ maxRetryDelay: TimeSpan.FromSeconds(15),
+ errorCodesToAdd: null)));
services.AddIdentityServer()
.AddOperationalStore(options =>
{
options.ConfigureDbContext = b =>
b.UseNpgsql(
Contract.Utilities.EnvUtility.GetConnectionString(),
- sql => sql.MigrationsAssembly("IdentityService.Infrastructure"));
+ sql =>
+ {
+ sql.MigrationsAssembly("IdentityService.Infrastructure");
+ sql.EnableRetryOnFailure(
+ maxRetryCount: 10,
+ maxRetryDelay: TimeSpan.FromSeconds(15),
+ errorCodesToAdd: null);
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| services.AddDbContext<IApplicationDbContext, ApplicationDbContext>(options => | |
| options.UseNpgsql(Contract.Utilities.EnvUtility.GetConnectionString())); | |
| services.AddIdentityServer() | |
| .AddOperationalStore(options => | |
| { | |
| options.ConfigureDbContext = b => | |
| b.UseNpgsql( | |
| Contract.Utilities.EnvUtility.GetConnectionString(), | |
| sql => sql.MigrationsAssembly("IdentityService.Infrastructure")); | |
| }); | |
| services.AddDbContext<IApplicationDbContext, ApplicationDbContext>(options => | |
| options.UseNpgsql( | |
| Contract.Utilities.EnvUtility.GetConnectionString(), | |
| sql => sql.EnableRetryOnFailure( | |
| maxRetryCount: 10, | |
| maxRetryDelay: TimeSpan.FromSeconds(15), | |
| errorCodesToAdd: null))); | |
| services.AddIdentityServer() | |
| .AddOperationalStore(options => | |
| { | |
| options.ConfigureDbContext = b => | |
| b.UseNpgsql( | |
| Contract.Utilities.EnvUtility.GetConnectionString(), | |
| sql => | |
| { | |
| sql.MigrationsAssembly("IdentityService.Infrastructure"); | |
| sql.EnableRetryOnFailure( | |
| maxRetryCount: 10, | |
| maxRetryDelay: TimeSpan.FromSeconds(15), | |
| errorCodesToAdd: null); | |
| }); | |
| }); |
🤖 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
`@app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs`
around lines 31 - 40, The AddOperationalStore ConfigureDbContext calls (in
AddMinimalInfrastructureServices() within DependencyInjection.cs and the
AddOperationalStore extension in DuendeIdentityServer/HostingExtensions.cs) use
UseNpgsql(...) without a retry policy; update those UseNpgsql invocations to
configure resilient retries by passing a Npgsql-specific SQL options lambda that
calls EnableRetryOnFailure (e.g., sql => {
sql.MigrationsAssembly("IdentityService.Infrastructure");
sql.EnableRetryOnFailure(); }) so persisted-grant/session DB operations use
transient-fault retry; ensure the same pattern is applied in both
AddOperationalStore usages and keep the MigrationsAssembly setting intact.
Comment on lines
+9
to
+10
| [tools."github:PhenChua29/maestro-runner"] | ||
| version = "latest" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify latest stable release, then pin it in mise.toml
curl -fsSL https://api.github.com/repos/PhenChua29/maestro-runner/releases/latest | jq -r '.tag_name'Repository: Carl-Johnsons/tastopia
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant snippet around lines 9-10 in mise.toml
if [ -f mise.toml ]; then
echo "=== mise.toml (lines 1-30) ==="
nl -ba mise.toml | sed -n '1,30p'
else
echo "mise.toml not found at repo root" >&2
fi
# Extract any maestro-runner-related entries from mise.toml
echo "=== maestro-runner entries ==="
rg -n 'maestro-runner' mise.toml || true
# Show all tool tables in mise.toml for context
echo "=== tool tables ==="
rg -n '^\[tools\.' mise.toml || trueRepository: Carl-Johnsons/tastopia
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate mise.toml
echo "=== locating mise.toml ==="
fd -t f -i 'mise.toml' . || true
# Read the first matching mise.toml (if multiple, show all short previews)
echo "=== printing mise.toml contents (with line numbers) ==="
paths=$(fd -t f -i 'mise.toml' . || true)
if [ -z "${paths}" ]; then
echo "No mise.toml found" >&2
exit 0
fi
echo "$paths" | while read -r p; do
echo "--- $p (first 80 lines) ---"
python3 - <<PY
import pathlib
p = pathlib.Path("$p")
lines = p.read_text(errors="replace").splitlines()
for i, line in enumerate(lines[:80], start=1):
print(f"{i:4d}: {line}")
PY
done
# Search for maestro-runner mentions across any mise.toml files
echo "=== searching for maestro-runner in mise.toml files ==="
echo "$paths" | while read -r p; do
echo "--- in $p ---"
rg -n 'maestro-runner' "$p" || true
done
# Also search the repo for the tool table header just in case
echo "=== searching repo for [tools.\"github:PhenChua29/maestro-runner\"] ==="
rg -n '\[tools\."github:PhenChua29/maestro-runner"\]' . || trueRepository: Carl-Johnsons/tastopia
Length of output: 925
🌐 Web query:
mise.toml tools \"github:PhenChua29/maestro-runner\" version latest behavior reproducible deterministic
💡 Result:
In mise.toml, using version latest for a GitHub-hosted tool like "github:PhenChua29/maestro-runner" leverages mise's integration with the GitHub API to identify the most recent stable release [1]. Determinism and Reproducibility While using latest provides the most up-to-date version, it is inherently non-deterministic because the version resolved will change over time as new releases are published [1]. For reproducible environments, such as CI/CD pipelines, it is standard practice to pin tools to a specific version (e.g., "1.1.13") rather than using latest [2][3]. How it works in mise: 1. GitHub Backend Resolution: Mise resolves GitHub-hosted tools by querying the GitHub API for releases [1]. As of recent updates, mise optimizes this by hitting the releases/latest endpoint directly [1]. 2. Stability: Explicitly requesting @latest ensures you receive the stable release identified by the repository's metadata [4][1]. 3. Reproducibility Best Practices: To ensure that every developer and CI runner uses the exact same binary, avoid dynamic tags like latest. Pinning to a specific version (e.g., maestro-runner = "1.1.13") prevents unexpected behavior changes caused by upstream updates [3][1]. Context on "maestro-runner" The tool "github:PhenChua29/maestro-runner" (often associated with the devicelab-dev organization in public repositories) is a mobile UI test automation tool designed to be a faster, Go-based alternative to Maestro [5][2][6]. When managing such tools, consistency across environments is critical; relying on floating versions like latest can introduce flakiness if different environments resolve to different binary versions [7][3].
Citations:
- 1: https://github.com/jdx/mise/releases/tag/v2026.3.9
- 2: https://github.com/devicelab-dev/maestro-runner/releases/tag/v1.1.13
- 3: https://assrt.ai/t/mobile-app-ci-testing-without-device-farm
- 4: fix(backend): route explicit latest through stable lookup jdx/mise#9228
- 5: https://github.com/devicelab-dev/maestro-runner
- 6: https://www.reddit.com/r/iOSProgramming/comments/1rqs6w0/we_opensourced_a_faster_alternative_to_maestro/
- 7: https://medium.com/getir/maestro-at-scale-architecting-a-deterministic-mobile-test-platform-0298b012f7a8
Pin maestro-runner to an explicit version in mise.toml (avoid latest).
mise.toml sets version = "latest" for [tools."github:PhenChua29/maestro-runner"], which makes tool resolution non-deterministic across time (and thus across CI vs local runs). Pin to a specific release tag (e.g., v1.x.y) to keep bootstrapping reproducible.
🤖 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 `@mise.toml` around lines 9 - 10, Replace the non-deterministic version pin in
the tools table by changing the version field for
[tools."github:PhenChua29/maestro-runner"] from "latest" to a specific release
tag (for example "v1.x.y" or the exact released tag you want to lock to); update
the version string in the mise.toml entry so CI and local bootstrapping use the
same immutable maestro-runner version.
| "app/client/website/cypress.config.ts", | ||
| ), | ||
| ) | ||
| mobile = Service(ServiceName.MOBILE, ("app/client/mobile",), exclude_paths=("app/client/mobile/.maestro",)) |
There was a problem hiding this comment.
Fix mobile exclusion glob to actually exclude directory contents.
Line 46 excludes app/client/mobile/.maestro as a single path, but the Git glob exclude should typically be app/client/mobile/.maestro/** to exclude files under that directory.
Suggested patch
-mobile = Service(ServiceName.MOBILE, ("app/client/mobile",), exclude_paths=("app/client/mobile/.maestro",))
+mobile = Service(
+ ServiceName.MOBILE,
+ ("app/client/mobile",),
+ exclude_paths=("app/client/mobile/.maestro/**",),
+)As per coding guidelines, deterministic branch/path logic should correctly handle boundary/path patterns to avoid wrong behavior in CI selection.
🤖 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 `@scripts/ci/build/derive-service-state.py` at line 46, The exclude pattern for
the mobile Service is only excluding the directory path itself; update the
exclude_paths for the mobile Service (the mobile
variable/Service(ServiceName.MOBILE, ...)) to use a recursive glob such as
"app/client/mobile/.maestro/**" (or an equivalent pattern) so all files under
that directory are excluded rather than just the directory entry.
| exit | ||
| fi | ||
|
|
||
| export $(grep -v '^#' $ENV_FILE | xargs) |
There was a problem hiding this comment.
Avoid export $(grep ... | xargs) for .env loading.
Lines 28 and 44 are vulnerable to word-splitting and command-substitution side effects, and they break on common .env values (spaces/special chars). Use a safe line-by-line parser for KEY=VALUE.
Suggested patch
-export $(grep -v '^#' $ENV_FILE | xargs)
+while IFS= read -r line; do
+ [[ "$line" =~ ^[A-Za-z_][A-Za-z0-9_]*= ]] || continue
+ export "$line"
+done < "$ENV_FILE"- export $(grep -v '^#' $env_path | xargs)
+ while IFS= read -r line; do
+ [[ "$line" =~ ^[A-Za-z_][A-Za-z0-9_]*= ]] || continue
+ export "$line"
+ done < "$env_path"As per coding guidelines, input handling in scripts must avoid runtime/security pitfalls that can execute unintended behavior.
Also applies to: 44-44
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[info] 28-28: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@scripts/local/apply-all-migrations.sh` at line 28, The current use of export
$(grep -v '^#' $ENV_FILE | xargs) in apply-all-migrations.sh is unsafe; replace
both occurrences with a safe line-by-line parser that reads $ENV_FILE with while
IFS= read -r line; skips empty lines and comments, splits only on the first '='
to produce KEY and VALUE, strips surrounding quotes without performing command
substitution, and then export the KEY with the literal VALUE (or use declare -x)
so keys with spaces/special chars are preserved and no unintended code executes;
locate the occurrences referencing ENV_FILE in the script and implement this
pattern for both places.
Source: Linters/SAST tools
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env python3 | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm direct runner entrypoints and shebang expectations
sed -n '1,30p' scripts.json
sed -n '1,5p' scripts/utils/setup-mise.pyRepository: Carl-Johnsons/tastopia
Length of output: 1107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Shebang occurrences =="
rg -n --hidden -S "#!/usr/bin/env python(\s|$)" .
echo "== python3 vs python usage in scripts =="
rg -n --hidden -S "(^|\s)python3(\s|$)" scripts .
rg -n --hidden -S "(^|\s)python(\s|$)" scripts .
echo "== Related setup-mise references =="
rg -n --hidden -S "setup-mise\.py" .
echo "== Tooling/config files that may pin python =="
ls -la | sed -n '1,200p'
fd -a .mise.toml -t f 2>/dev/null || true
fd -a tool-versions -t f 2>/dev/null || true
fd -a .tool-versions -t f 2>/dev/null || true
fd -a asdf -t f 2>/dev/null || true
fd -a pyproject.toml -t f 2>/dev/null || trueRepository: Carl-Johnsons/tastopia
Length of output: 4406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant script header and any docs/config hints inside setup-mise.py
sed -n '1,120p' scripts/utils/setup-mise.pyRepository: Carl-Johnsons/tastopia
Length of output: 2758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mise.toml =="
cat mise.toml
echo "== setup-mise.py python-related handling (first 60 matches) =="
rg -n --hidden -S "(\bpython\b|\bpython3\b|mise\s|shim|shims|alias)" scripts/utils/setup-mise.py | head -n 60
echo "== Any docs about shebang/runtime entrypoint? =="
rg -n --hidden -S "setup-mise\.py|#!/usr/bin/env python|python3" README.md scripts -g'*.md' || trueRepository: Carl-Johnsons/tastopia
Length of output: 1963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sections around mise install/activation and potential re-exec logic
sed -n '150,260p' scripts/utils/setup-mise.py
sed -n '260,460p' scripts/utils/setup-mise.py
echo "== runScripts.sh =="
sed -n '1,200p' runScripts.shRepository: Carl-Johnsons/tastopia
Length of output: 9677
Make scripts/utils/setup-mise.py runnable when only python3 is installed.
scripts.json runs ./scripts/utils/setup-mise.py directly, but the shebang is #!/usr/bin/env python; if python isn’t available (common on some systems where only python3 exists), the script will fail before any mise install/activation logic can run. Prefer #!/usr/bin/env python3 (or update scripts.json to invoke it with python3).
🤖 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 `@scripts/utils/setup-mise.py` at line 1, The script setup-mise.py uses a
shebang of "#!/usr/bin/env python" which fails on systems that only have
python3; update the shebang to "#!/usr/bin/env python3" so the script is
executable with python3, or alternatively modify the caller in scripts.json to
invoke "python3 ./scripts/utils/setup-mise.py" instead; make the change in
setup-mise.py's top line (the shebang) to ensure the existing
installation/activation logic runs when only python3 is installed.
PhenChua29
force-pushed
the
pipeline
branch
2 times, most recently
from
ad267d4 to
1fcb235
Compare
PhenChua29
approved these changes
Jun 12, 2026
PhenChua29
force-pushed
the
pipeline
branch
2 times, most recently
from
6b2e480 to
7e883a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/server/IdentityService/src/DuendeIdentityServer/Pages/ExternalLogin/Callback.cshtml.cs (1)
85-91:⚠️ Potential issue | 🟡 MinorFix
AccessTokennullability contract in external login callback
Callback.cshtml.csassignsAccessToken = accessToken!even thoughaccessTokencomes fromGetTokenValue("access_token")(nullable). InLoginWithGoogleCommandHandler.Handle,request.AccessTokenis never used, so this won’t currently cause a runtime exception in this handler—but the non-nullableLoginWithGoogleCommand.AccessToken(string) with a null-forgiving assignment is an inconsistent contract. MakeLoginWithGoogleCommand.AccessTokenstring?(and handle accordingly) or only set it when present / remove it if unused.🤖 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 `@app/server/IdentityService/src/DuendeIdentityServer/Pages/ExternalLogin/Callback.cshtml.cs` around lines 85 - 91, Callback assigns AccessToken using the null-forgiving operator (AccessToken = accessToken!) even though GetTokenValue("access_token") can return null, creating an inconsistent non-nullable contract for LoginWithGoogleCommand; change the LoginWithGoogleCommand.AccessToken property to string? and remove the null-forgiving assignment in Callback.cshtml.cs (assign AccessToken only when accessToken != null or pass the nullable value directly), and update any consumers (e.g., LoginWithGoogleCommandHandler.Handle) to accept the nullable AccessToken (no-op if unused) so the contract matches actual nullability.app/server/UserService/src/UserService.API/Configs/UserProfile.cs (1)
23-25:⚠️ Potential issue | 🟠 MajorGuard the reverse
Dobmapping against an unset protobufTimestampIn
app/server/UserService/src/UserService.API/Configs/UserProfile.cs, the reverse mapping usesopt.MapFrom(src => src.Dob.ToDateTime())unconditionally, butGrpcUserDetailDTO’sDobis agoogle.protobuf.Timestamp(UserService.proto, fieldDob = 5) and is set tonullwhenuser.Dobis not present—so reverse mapping can throw.Suggested fix
.ReverseMap() .ForMember(dest => dest.Dob, - opt => opt.MapFrom(src => src.Dob.ToDateTime())) + opt => opt.MapFrom(src => src.Dob == null + ? (DateTime?)null + : src.Dob.ToDateTime())) .ForMember(dest => dest.AccountId, opt => opt.MapFrom(src => Guid.Parse(src.AccountId)));🤖 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 `@app/server/UserService/src/UserService.API/Configs/UserProfile.cs` around lines 23 - 25, The reverse mapping unconditionally calls src.Dob.ToDateTime() which will NRE when GrpcUserDetailDTO.Dob (google.protobuf.Timestamp) is null; update the AutoMapper configuration (the ReverseMap ForMember for dest.Dob) to guard: map using src => src.Dob != null ? src.Dob.ToDateTime() : (DateTime?)null (or the appropriate default if dest.Dob is non-nullable), i.e., check src.Dob for null before calling ToDateTime so the reverse map from GrpcUserDetailDTO handles unset Timestamp safely.
🧹 Nitpick comments (2)
app/server/RecipeService/src/RecipeService.Application/Configs/RecipeProfile.cs (1)
34-43: ⚡ Quick winConsider simplifying the Users dictionary transformation.
The mapping creates a new dictionary from
src.Userswith the same keys (user.Key), which is redundant. If the goal is only to transform the value type, consider usingSelector confirming whether this pattern is required by AutoMapper for dictionary-to-MapField conversions in gRPC contexts.🤖 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 `@app/server/RecipeService/src/RecipeService.Application/Configs/RecipeProfile.cs` around lines 34 - 43, The Users dictionary mapping in CreateMap<GetSimpleUsersDTO, GrpcGetSimpleUsersDTO>() is rebuilding a dictionary with identical keys which is redundant; simplify the MapFrom to transform only the values (e.g., use src.Users.ToDictionary(kv => kv.Key, kv => /* map kv.Value to CommonProto.GrpcSimpleUser */) or use src.Users.Select/ToDictionary to project values) or, if AutoMapper/gRPC supports it, map the dictionary values directly (map User value -> CommonProto.GrpcSimpleUser) so you avoid recreating the same key twice; update the .ForMember(dest => dest.Users, opt => opt.MapFrom(...)) logic that constructs new CommonProto.GrpcSimpleUser instances accordingly.app/server/NotificationService/src/NotificationService.Application/Configs/NotificationProfile.cs (1)
17-26: ⚡ Quick winConsider simplifying the Users dictionary transformation.
The mapping creates a new dictionary from
src.Userswith the same keys (user.Key), which is redundant. If the goal is only to transform the value type fromSimpleUserDTOtoGrpcSimpleUser, consider usingSelector confirming whether this pattern is required by AutoMapper for dictionary-to-MapField conversions in gRPC contexts.🤖 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 `@app/server/NotificationService/src/NotificationService.Application/Configs/NotificationProfile.cs` around lines 17 - 26, The Users dictionary mapping is creating a redundant key projection; update the MapFrom in CreateMap<GetSimpleUsersDTO, GrpcGetSimpleUsersDTO>() to either pass src.Users directly (letting AutoMapper map Dictionary<string,SimpleUserDTO> => MapField<string,GrpcSimpleUser> automatically) or simplify the transformation to only change values (e.g., MapFrom(src => src.Users.ToDictionary(kv => kv.Key, kv => new CommonProto.GrpcSimpleUser { AccountId = kv.Value.AccountId.ToString(), AvtUrl = kv.Value.AvtUrl, DisplayName = kv.Value.DisplayName }))). Modify the MapFrom so it does not recreate identical keys (refer to GetSimpleUsersDTO.Users and GrpcGetSimpleUsersDTO.Users / CommonProto.GrpcSimpleUser).
🤖 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 `@app/server/APIGateway/src/APIGateway/DependenciesInjection.cs`:
- Around line 142-146: The current early-return treats an empty
AuthenticationProviderKeys array as unauthenticated and skips claim checks;
change the condition in the block that references
ctx.Items.DownstreamRoute().AuthenticationOptions.AuthenticationProviderKeys so
it only returns true when AuthenticationProviderKeys is null (no auth
configured), not when its Length == 0, ensuring RouteClaimsRequirement is still
evaluated for routes that explicitly supply an empty provider list.
In `@app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs`:
- Around line 204-208: Replace the Console.WriteLine call in the inline
middleware (the app.Use(...) block in HostingExtensions.cs) with structured
logging via an ILogger at Debug level: obtain an ILogger (e.g., via app.Logger
or app.ApplicationServices.GetRequiredService<ILogger<HostingExtensions>>()),
check logger.IsEnabled(LogLevel.Debug) before reading/formatting
context.Connection.RemoteIpAddress, and log the IP using logger.LogDebug with a
structured message (avoid writing raw PII to stdout and only emit when debug
logging is enabled).
In
`@app/server/IdentityService/src/DuendeIdentityServer/Pages/Account/ForgotPassword/Index.cshtml.cs`:
- Around line 129-131: In the catch block in Index.cshtml.cs (the
forgot-password handler, e.g., OnPostAsync), stop echoing ex.Message to the
user: replace ModelState.AddModelError(string.Empty, $"Send OTP failed! Please
try again ({ex.Message})") with a generic message such as
ModelState.AddModelError(string.Empty, "Send OTP failed! Please try again.");
and instead log the exception server-side via the class logger (e.g.,
_logger.LogError(ex, "Failed to send OTP for ForgotPassword")); ensure no
exception details are returned to the unauthenticated UI.
In
`@app/server/IdentityService/src/DuendeIdentityServer/Pages/ExternalLogin/Callback.cshtml.cs`:
- Line 74: The InvalidOperationException message contains a typo: update the
thrown exception text used when resolving provider (the expression assigning var
provider in Callback.cshtml.cs) to replace "authentiation" with "authentication"
so the message reads something like "Null scheme in authentication properties".
In
`@app/server/NotificationService/src/NotificationService.Application/DependencyInjection.cs`:
- Around line 14-19: The current AddAutoMapper lambda unconditionally assigns
cfg.LicenseKey to the literal "Not Found" via DotNetEnv.Env.GetString, which
causes AutoMapper startup validation to fail; update the AddAutoMapper
registration (the cfg => { ... } lambda used in services.AddAutoMapper) to first
retrieve the env var into a local (e.g., var license =
DotNetEnv.Env.GetString("LUCKYPENNYSOFTWARE_LICENSE_KEY");) and then either (a)
if the license is required, throw a clear exception when
string.IsNullOrWhiteSpace(license) so startup fails fast, or (b) if optional,
only set cfg.LicenseKey when !string.IsNullOrWhiteSpace(license) (omit
assignment when missing) so the free tier works; apply the same change to all
four DependencyInjection files that currently set cfg.LicenseKey = "Not Found".
In `@app/server/RecipeService/src/RecipeService.API/Configs/RecipeProfile.cs`:
- Around line 70-87: In RecipeProfile where CreateMap<TagDTO, Tag>() maps
dest.Status and dest.Category using Enum.Parse, validate the input strings
before parsing (same approach as the gRPC mapping): replace direct Enum.Parse
calls with a safe conversion that uses Enum.TryParse (with ignoreCase as
appropriate) and either map to the parsed enum or throw/return a controlled
validation exception when parsing fails; update the ForMember mappings for dest
=> dest.Status and dest => dest.Category to perform this TryParse-based
validation so invalid strings no longer cause unhandled ArgumentExceptions.
- Around line 32-33: Duplicate CreateMap<RecipeDetailsDTO, Recipe>() mapping is
defined twice causing AutoMapper startup failure; remove the redundant mapping
call (the CreateMap<RecipeDetailsDTO, Recipe>().ForMember(...).ReverseMap() at
the shown diff) or consolidate it into the existing mapping block that defines
members (the earlier RecipeDetailsDTO → Recipe map) so there is only one mapping
entry for RecipeDetailsDTO to Recipe.
- Around line 46-63: The mapping from GrpcTagDTO to Tag uses Enum.Parse for
Status and Category which will throw on invalid input; update the
CreateMap<GrpcTagDTO, Tag>() mapping for dest.Status and dest.Category to use
Enum.TryParse (or a small helper like TryParseOrDefault) that checks for
null/empty and returns a safe default (e.g., TagStatus.Unknown /
TagCategory.Unknown) on failure, and keep the ReverseMap() string conversions
as-is; ensure both Status and Category MapFrom expressions are replaced with the
TryParse-based logic so malformed or unexpected src.Status/src.Category values
don't throw.
In
`@app/server/RecipeService/src/RecipeService.Application/Configs/RecipeProfile.cs`:
- Around line 25-28: The reverse mapping using Enum.Parse in CreateMap<Tag,
AdminTagResponse>().ReverseMap() is unsafe because Enum.Parse(typeof(TagStatus),
src.Status) will throw if src.Status is invalid; replace it with a safe parse
using Enum.TryParse (or a helper) to validate src.Status before mapping to
TagStatus and handle invalid values explicitly (e.g., map to a default TagStatus
or throw a clear ArgumentException), ensuring the MapFrom for dest =>
dest.Status uses the TryParse result for TagStatus instead of Enum.Parse.
In
`@app/server/TrackingService/src/TrackingService.Application/UserViewRecipeDetails/Queries/SearchUserViewRecipeDetaiQueryHandler.cs`:
- Around line 47-52: The current code builds viewsMap of latest
UserViewRecipeDetail per RecipeId but then paginates using response.Recipes
iteration order, causing inconsistent history ordering; update the handler
(SearchUserViewRecipeDetaiQueryHandler) to reorder response.Recipes by the
corresponding viewsMap entry's UpdatedAt (descending) before applying
pagination: filter recipes to those present in viewsMap, sort them using
viewsMap[recipe.Id.ToString()].UpdatedAt descending, then proceed with paging
and mapping so page boundaries reflect newest views; apply the same change for
the other occurrence referenced around the response.Recipes usage (lines
~89-91).
In `@app/server/UserService/src/UserService.Application/Configs/UserProfile.cs`:
- Around line 14-16: Remove the duplicate AutoMapper type map by deleting the
CreateMap<UserDetailsDTO, GetUserDetailsResponse>().ReverseMap() declaration
from one of the UserProfile classes (either the UserService.Application
UserProfile or the UserService.API UserProfile) so the mapping is defined only
once; alternatively, fix the registration to only add AutoMapper once by
adjusting DependencyInjection (the methods that call services.AddAutoMapper(...)
in UserService.Application.DependencyInjection and
UserService.API.DependenciesInjection) so
AppDomain.CurrentDomain.GetAssemblies() does not cause both profiles to be
scanned. Ensure you update either the profile (UserProfile) to remove the
duplicated CreateMap symbol or the dependency injection registration to prevent
double registration.
In `@scripts/local/apply-all-migrations.sh`:
- Line 17: The printf call in scripts/local/apply-all-migrations.sh currently
interpolates $TARGET_ENV directly into the format string, creating a
format-string vulnerability; change the printf invocation to use an explicit
"%s" format token and pass TARGET_ENV as a separate argument (i.e., printf "%s"
... $TARGET_ENV) so user content is not interpreted as format specifiers —
locate the printf line that mentions "${DANGER}Invalid argument:
'$TARGET_ENV'..." and replace the inline interpolation with a format string and
variable argument.
---
Outside diff comments:
In
`@app/server/IdentityService/src/DuendeIdentityServer/Pages/ExternalLogin/Callback.cshtml.cs`:
- Around line 85-91: Callback assigns AccessToken using the null-forgiving
operator (AccessToken = accessToken!) even though GetTokenValue("access_token")
can return null, creating an inconsistent non-nullable contract for
LoginWithGoogleCommand; change the LoginWithGoogleCommand.AccessToken property
to string? and remove the null-forgiving assignment in Callback.cshtml.cs
(assign AccessToken only when accessToken != null or pass the nullable value
directly), and update any consumers (e.g., LoginWithGoogleCommandHandler.Handle)
to accept the nullable AccessToken (no-op if unused) so the contract matches
actual nullability.
In `@app/server/UserService/src/UserService.API/Configs/UserProfile.cs`:
- Around line 23-25: The reverse mapping unconditionally calls
src.Dob.ToDateTime() which will NRE when GrpcUserDetailDTO.Dob
(google.protobuf.Timestamp) is null; update the AutoMapper configuration (the
ReverseMap ForMember for dest.Dob) to guard: map using src => src.Dob != null ?
src.Dob.ToDateTime() : (DateTime?)null (or the appropriate default if dest.Dob
is non-nullable), i.e., check src.Dob for null before calling ToDateTime so the
reverse map from GrpcUserDetailDTO handles unset Timestamp safely.
---
Nitpick comments:
In
`@app/server/NotificationService/src/NotificationService.Application/Configs/NotificationProfile.cs`:
- Around line 17-26: The Users dictionary mapping is creating a redundant key
projection; update the MapFrom in CreateMap<GetSimpleUsersDTO,
GrpcGetSimpleUsersDTO>() to either pass src.Users directly (letting AutoMapper
map Dictionary<string,SimpleUserDTO> => MapField<string,GrpcSimpleUser>
automatically) or simplify the transformation to only change values (e.g.,
MapFrom(src => src.Users.ToDictionary(kv => kv.Key, kv => new
CommonProto.GrpcSimpleUser { AccountId = kv.Value.AccountId.ToString(), AvtUrl =
kv.Value.AvtUrl, DisplayName = kv.Value.DisplayName }))). Modify the MapFrom so
it does not recreate identical keys (refer to GetSimpleUsersDTO.Users and
GrpcGetSimpleUsersDTO.Users / CommonProto.GrpcSimpleUser).
In
`@app/server/RecipeService/src/RecipeService.Application/Configs/RecipeProfile.cs`:
- Around line 34-43: The Users dictionary mapping in
CreateMap<GetSimpleUsersDTO, GrpcGetSimpleUsersDTO>() is rebuilding a dictionary
with identical keys which is redundant; simplify the MapFrom to transform only
the values (e.g., use src.Users.ToDictionary(kv => kv.Key, kv => /* map kv.Value
to CommonProto.GrpcSimpleUser */) or use src.Users.Select/ToDictionary to
project values) or, if AutoMapper/gRPC supports it, map the dictionary values
directly (map User value -> CommonProto.GrpcSimpleUser) so you avoid recreating
the same key twice; update the .ForMember(dest => dest.Users, opt =>
opt.MapFrom(...)) logic that constructs new CommonProto.GrpcSimpleUser instances
accordingly.
🪄 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: e1591b31-48d6-49f5-b194-9f791b45ccff
📒 Files selected for processing (65)
app/server/APIGateway/src/APIGateway/DependenciesInjection.csapp/server/Contract/Contract/Event/UserEvent/UpdateUserTotalRecipeEvent.csapp/server/Contract/Contract/Protos/TrackingService.protoapp/server/Contract/Contract/Utilities/EnvUtility.csapp/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.csapp/server/IdentityService/src/DuendeIdentityServer/DTOs/IdentityProfile.csapp/server/IdentityService/src/DuendeIdentityServer/DTOs/MappingConfig.csapp/server/IdentityService/src/DuendeIdentityServer/DuendeIdentityServer.csprojapp/server/IdentityService/src/DuendeIdentityServer/Extensions/ReinforcedTypingsExtension.csapp/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.csapp/server/IdentityService/src/DuendeIdentityServer/Pages/Account/ChangePassword/Index.cshtml.csapp/server/IdentityService/src/DuendeIdentityServer/Pages/Account/ChangePasswordSuccess/Index.cshtml.csapp/server/IdentityService/src/DuendeIdentityServer/Pages/Account/ForgotPassword/Index.cshtml.csapp/server/IdentityService/src/DuendeIdentityServer/Pages/Account/VerifyForgotPassword/Index.cshtml.csapp/server/IdentityService/src/DuendeIdentityServer/Pages/ExternalLogin/Callback.cshtml.csapp/server/IdentityService/src/DuendeIdentityServer/Program.csapp/server/IdentityService/src/IdentityService.Application/Configs/IdentityProfile.csapp/server/IdentityService/src/IdentityService.Application/Configs/MappingConfig.csapp/server/IdentityService/src/IdentityService.Application/DependencyInjection.csapp/server/IdentityService/src/IdentityService.Application/IdentityService.Application.csprojapp/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.csapp/server/IdentityService/src/IdentityService.Infrastructure/IdentityService.Infrastructure.csprojapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.csapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/20260611045123_InitialIdentityServerPersistedGrantDbMigration.Designer.csapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/20260611045123_InitialIdentityServerPersistedGrantDbMigration.csapp/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/PersistedGrantDbContextModelSnapshot.csapp/server/NotificationService/src/EmailWorker/EmailWorker.csprojapp/server/NotificationService/src/NotificationService.API/AppCommandHandler.csapp/server/NotificationService/src/NotificationService.Application/Configs/MappingConfig.csapp/server/NotificationService/src/NotificationService.Application/Configs/NotificationProfile.csapp/server/NotificationService/src/NotificationService.Application/DependencyInjection.csapp/server/NotificationService/src/NotificationService.Application/NotificationService.Application.csprojapp/server/RecipeService/src/RecipeService.API/AppCommandHandler.csapp/server/RecipeService/src/RecipeService.API/Configs/MappingConfig.csapp/server/RecipeService/src/RecipeService.API/Configs/RecipeProfile.csapp/server/RecipeService/src/RecipeService.API/DependenciesInjection.csapp/server/RecipeService/src/RecipeService.API/Extensions/ReinforcedTypingsExtension.csapp/server/RecipeService/src/RecipeService.Application/Configs/MappingConfig.csapp/server/RecipeService/src/RecipeService.Application/Configs/RecipeProfile.csapp/server/RecipeService/src/RecipeService.Application/DependencyInjection.csapp/server/RecipeService/src/RecipeService.Application/RecipeService.Application.csprojapp/server/RecipeService/src/RecipeService.Application/ReportReasons/Queries/GetReportReasonsQueryHandler.csapp/server/RecipeService/src/RecipeWorker/Worker.csapp/server/TrackingService/src/TrackingService.API/AppCommandHandler.csapp/server/TrackingService/src/TrackingService.API/Configs/MappingConfig.csapp/server/TrackingService/src/TrackingService.API/Controllers/TrackingController.csapp/server/TrackingService/src/TrackingService.API/DependenciesInjection.csapp/server/TrackingService/src/TrackingService.Application/Configs/MappingConfig.csapp/server/TrackingService/src/TrackingService.Application/Configs/TrackingProfile.csapp/server/TrackingService/src/TrackingService.Application/DependencyInjection.csapp/server/TrackingService/src/TrackingService.Application/TrackingService.Application.csprojapp/server/TrackingService/src/TrackingService.Application/UserViewRecipeDetails/Queries/SearchUserViewRecipeDetaiQueryHandler.csapp/server/UploadFileService/src/UploadFileService.API/Configs/MappingConfig.csapp/server/UploadFileService/src/UploadFileService.API/Configs/UploadProfile.csapp/server/UploadFileService/src/UploadFileService.API/DependenciesInjection.csapp/server/UploadFileService/src/UploadFileService.API/UploadFileService.API.csprojapp/server/UserService/src/UserService.API/AppCommandHandler.csapp/server/UserService/src/UserService.API/Configs/UserProfile.csapp/server/UserService/src/UserService.API/DependenciesInjection.csapp/server/UserService/src/UserService.API/Extensions/ReinforcedTypingsExtension.csapp/server/UserService/src/UserService.Application/Configs/MappingConfig.csapp/server/UserService/src/UserService.Application/Configs/UserProfile.csapp/server/UserService/src/UserService.Application/DependencyInjection.csapp/server/UserService/src/UserService.Application/UserService.Application.csprojscripts/local/apply-all-migrations.sh
💤 Files with no reviewable changes (11)
- app/server/IdentityService/src/DuendeIdentityServer/DTOs/MappingConfig.cs
- app/server/UserService/src/UserService.Application/Configs/MappingConfig.cs
- app/server/TrackingService/src/TrackingService.API/Configs/MappingConfig.cs
- app/server/Contract/Contract/Protos/TrackingService.proto
- app/server/NotificationService/src/NotificationService.Application/Configs/MappingConfig.cs
- app/server/IdentityService/src/IdentityService.Application/Configs/MappingConfig.cs
- app/server/Contract/Contract/Event/UserEvent/UpdateUserTotalRecipeEvent.cs
- app/server/UploadFileService/src/UploadFileService.API/Configs/MappingConfig.cs
- app/server/RecipeService/src/RecipeService.Application/Configs/MappingConfig.cs
- app/server/TrackingService/src/TrackingService.Application/Configs/MappingConfig.cs
- app/server/RecipeService/src/RecipeService.API/Configs/MappingConfig.cs
✅ Files skipped from review due to trivial changes (10)
- app/server/NotificationService/src/EmailWorker/EmailWorker.csproj
- app/server/IdentityService/src/IdentityService.Application/IdentityService.Application.csproj
- app/server/RecipeService/src/RecipeService.Application/RecipeService.Application.csproj
- app/server/NotificationService/src/NotificationService.Application/NotificationService.Application.csproj
- app/server/UploadFileService/src/UploadFileService.API/UploadFileService.API.csproj
- app/server/TrackingService/src/TrackingService.Application/Configs/TrackingProfile.cs
- app/server/TrackingService/src/TrackingService.Application/TrackingService.Application.csproj
- app/server/IdentityService/src/DuendeIdentityServer/Program.cs
- app/server/UserService/src/UserService.Application/UserService.Application.csproj
- app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/PersistedGrantDbContextModelSnapshot.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/ApplicationDbContext.cs
- app/server/Contract/Contract/Utilities/EnvUtility.cs
- app/server/IdentityService/src/IdentityService.Infrastructure/IdentityService.Infrastructure.csproj
- app/server/IdentityService/src/IdentityService.Infrastructure/DependencyInjection.cs
- app/server/IdentityService/src/DuendeIdentityServer/AppCommandHandler.cs
- app/server/IdentityService/src/DuendeIdentityServer/DuendeIdentityServer.csproj
- app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/20260611045123_InitialIdentityServerPersistedGrantDbMigration.Designer.cs
- app/server/IdentityService/src/IdentityService.Infrastructure/Persistence/IdentityServer/PersistedGrant/20260611045123_InitialIdentityServerPersistedGrantDbMigration.cs
Comment on lines
+142
to
+146
| if (ctx.Items.DownstreamRoute() | ||
| .AuthenticationOptions | ||
| .AuthenticationProviderKeys?.Length == 0) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
Don't short-circuit role checks on an empty provider list.
This now returns true before RouteClaimsRequirement is evaluated. If a route is configured with AuthenticationProviderKeys: [] but still carries a role requirement, this method will authorize it anyway and skip the claim check entirely.
Suggested fix
- if (ctx.Items.DownstreamRoute()
- .AuthenticationOptions
- .AuthenticationProviderKeys?.Length == 0)
+ var downstreamRoute = ctx.Items.DownstreamRoute();
+ var providerKeys = downstreamRoute.AuthenticationOptions.AuthenticationProviderKeys;
+ var required = downstreamRoute.RouteClaimsRequirement;
+
+ if ((providerKeys == null || providerKeys.Length == 0) &&
+ (required == null || required.Count == 0))
{
return true;
}
- else
- {
- bool auth = false;
- Claim[] claims = ctx.User.Claims.ToArray<Claim>();
- Dictionary<string, string> required = ctx.Items.DownstreamRoute().RouteClaimsRequirement;
+ else
+ {
+ bool auth = false;
+ Claim[] claims = ctx.User.Claims.ToArray<Claim>();🤖 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 `@app/server/APIGateway/src/APIGateway/DependenciesInjection.cs` around lines
142 - 146, The current early-return treats an empty AuthenticationProviderKeys
array as unauthenticated and skips claim checks; change the condition in the
block that references
ctx.Items.DownstreamRoute().AuthenticationOptions.AuthenticationProviderKeys so
it only returns true when AuthenticationProviderKeys is null (no auth
configured), not when its Length == 0, ensuring RouteClaimsRequirement is still
evaluated for routes that explicitly supply an empty provider list.
Comment on lines
204
to
+208
| app.Use(async (context, next) => | ||
| { | ||
| Console.WriteLine($"RemoteIp: {context.Connection.RemoteIpAddress}"); | ||
| await next(); | ||
| }); | ||
| { | ||
| Console.WriteLine($"RemoteIp: {context.Connection.RemoteIpAddress}"); | ||
| await next(); | ||
| }); |
There was a problem hiding this comment.
Avoid logging raw client IPs to stdout on every request.
This writes PII from every auth request into unstructured logs. If you still need this for debugging, gate it behind ILogger and a debug-level setting instead of Console.WriteLine.
Suggested change
- app.Use(async (context, next) =>
- {
- Console.WriteLine($"RemoteIp: {context.Connection.RemoteIpAddress}");
- await next();
- });
+ app.Use(async (context, next) =>
+ {
+ await next();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.Use(async (context, next) => | |
| { | |
| Console.WriteLine($"RemoteIp: {context.Connection.RemoteIpAddress}"); | |
| await next(); | |
| }); | |
| { | |
| Console.WriteLine($"RemoteIp: {context.Connection.RemoteIpAddress}"); | |
| await next(); | |
| }); | |
| app.Use(async (context, next) => | |
| { | |
| await next(); | |
| }); |
🤖 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 `@app/server/IdentityService/src/DuendeIdentityServer/HostingExtensions.cs`
around lines 204 - 208, Replace the Console.WriteLine call in the inline
middleware (the app.Use(...) block in HostingExtensions.cs) with structured
logging via an ILogger at Debug level: obtain an ILogger (e.g., via app.Logger
or app.ApplicationServices.GetRequiredService<ILogger<HostingExtensions>>()),
check logger.IsEnabled(LogLevel.Debug) before reading/formatting
context.Connection.RemoteIpAddress, and log the IP using logger.LogDebug with a
structured message (avoid writing raw PII to stdout and only emit when debug
logging is enabled).
Comment on lines
129
to
+131
| catch (Exception ex) | ||
| { | ||
| ModelState.AddModelError(string.Empty, "Send OTP failed! Please try again"); | ||
| ModelState.AddModelError(string.Empty, $"Send OTP failed! Please try again ({ex.Message})"); |
There was a problem hiding this comment.
Don't echo exception details back to the forgot-password UI.
ex.Message is now rendered to the end user on an unauthenticated recovery flow. That can leak internal provider state or validation details; keep the generic message here and log the exception server-side instead.
Suggested change
catch (Exception ex)
{
- ModelState.AddModelError(string.Empty, $"Send OTP failed! Please try again ({ex.Message})");
+ ModelState.AddModelError(string.Empty, "Send OTP failed! Please try again");
+ // Log `ex` with the server-side logger instead of exposing it in the UI.
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) | |
| { | |
| ModelState.AddModelError(string.Empty, "Send OTP failed! Please try again"); | |
| ModelState.AddModelError(string.Empty, $"Send OTP failed! Please try again ({ex.Message})"); | |
| catch (Exception ex) | |
| { | |
| ModelState.AddModelError(string.Empty, "Send OTP failed! Please try again"); | |
| // Log `ex` with the server-side logger instead of exposing it in the UI. | |
| } |
🤖 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
`@app/server/IdentityService/src/DuendeIdentityServer/Pages/Account/ForgotPassword/Index.cshtml.cs`
around lines 129 - 131, In the catch block in Index.cshtml.cs (the
forgot-password handler, e.g., OnPostAsync), stop echoing ex.Message to the
user: replace ModelState.AddModelError(string.Empty, $"Send OTP failed! Please
try again ({ex.Message})") with a generic message such as
ModelState.AddModelError(string.Empty, "Send OTP failed! Please try again.");
and instead log the exception server-side via the class logger (e.g.,
_logger.LogError(ex, "Failed to send OTP for ForgotPassword")); ensure no
exception details are returned to the unauthenticated UI.
| throw new InvalidOperationException("Unknown userid"); | ||
|
|
||
| var provider = result.Properties.Items["scheme"] ?? throw new InvalidOperationException("Null scheme in authentiation properties"); | ||
| var provider = result.Properties?.Items["scheme"] ?? throw new InvalidOperationException("Null scheme in authentiation properties"); |
There was a problem hiding this comment.
Fix typo in exception message.
The exception message contains a typo: "authentiation" should be "authentication".
📝 Proposed fix
- var provider = result.Properties?.Items["scheme"] ?? throw new InvalidOperationException("Null scheme in authentiation properties");
+ var provider = result.Properties?.Items["scheme"] ?? throw new InvalidOperationException("Null scheme in authentication properties");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var provider = result.Properties?.Items["scheme"] ?? throw new InvalidOperationException("Null scheme in authentiation properties"); | |
| var provider = result.Properties?.Items["scheme"] ?? throw new InvalidOperationException("Null scheme in authentication properties"); |
🤖 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
`@app/server/IdentityService/src/DuendeIdentityServer/Pages/ExternalLogin/Callback.cshtml.cs`
at line 74, The InvalidOperationException message contains a typo: update the
thrown exception text used when resolving provider (the expression assigning var
provider in Callback.cshtml.cs) to replace "authentiation" with "authentication"
so the message reads something like "Null scheme in authentication properties".
Comment on lines
+14
to
+19
| services.AddAutoMapper( | ||
| cfg => | ||
| { | ||
| cfg.LicenseKey = DotNetEnv.Env.GetString("LUCKYPENNYSOFTWARE_LICENSE_KEY", "Not Found"); | ||
| }, | ||
| AppDomain.CurrentDomain.GetAssemblies()); |
There was a problem hiding this comment.
Invalid license key fallback will cause AutoMapper initialization failure across all services.
All four dependency injection files (NotificationService.Application/DependencyInjection.cs, TrackingService.API/DependenciesInjection.cs, TrackingService.Application/DependencyInjection.cs, and RecipeService.API/DependenciesInjection.cs) set cfg.LicenseKey = "Not Found" when the LUCKYPENNYSOFTWARE_LICENSE_KEY environment variable is missing. AutoMapper will fail validation at startup with this invalid license key. Either throw an exception when the key is missing (if required), or omit the license key configuration entirely (if using the free tier).
🤖 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
`@app/server/NotificationService/src/NotificationService.Application/DependencyInjection.cs`
around lines 14 - 19, The current AddAutoMapper lambda unconditionally assigns
cfg.LicenseKey to the literal "Not Found" via DotNetEnv.Env.GetString, which
causes AutoMapper startup validation to fail; update the AddAutoMapper
registration (the cfg => { ... } lambda used in services.AddAutoMapper) to first
retrieve the env var into a local (e.g., var license =
DotNetEnv.Env.GetString("LUCKYPENNYSOFTWARE_LICENSE_KEY");) and then either (a)
if the license is required, throw a clear exception when
string.IsNullOrWhiteSpace(license) so startup fails fast, or (b) if optional,
only set cfg.LicenseKey when !string.IsNullOrWhiteSpace(license) (omit
assignment when missing) so the free tier works; apply the same change to all
four DependencyInjection files that currently set cfg.LicenseKey = "Not Found".
Comment on lines
+70
to
+87
| CreateMap<TagDTO, Tag>() | ||
| .ForMember( | ||
| dest => dest.Status, | ||
| opt => opt.MapFrom(src => Enum.Parse<TagStatus>(src.Status)) | ||
| ) | ||
| .ForMember( | ||
| dest => dest.Category, | ||
| opt => opt.MapFrom(src => Enum.Parse<TagCategory>(src.Category)) | ||
| ) | ||
| .ReverseMap() | ||
| .ForMember( | ||
| dest => dest.Status, | ||
| opt => opt.MapFrom(src => src.Status.ToString()) | ||
| ) | ||
| .ForMember( | ||
| dest => dest.Category, | ||
| opt => opt.MapFrom(src => src.Category.ToString()) | ||
| ); |
There was a problem hiding this comment.
Add validation before parsing enum values.
Enum.Parse<TagStatus>(src.Status) and Enum.Parse<TagCategory>(src.Category) will throw ArgumentException if the input strings don't match valid enum values. Apply the same validation as recommended for the gRPC mapping above.
🤖 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 `@app/server/RecipeService/src/RecipeService.API/Configs/RecipeProfile.cs`
around lines 70 - 87, In RecipeProfile where CreateMap<TagDTO, Tag>() maps
dest.Status and dest.Category using Enum.Parse, validate the input strings
before parsing (same approach as the gRPC mapping): replace direct Enum.Parse
calls with a safe conversion that uses Enum.TryParse (with ignoreCase as
appropriate) and either map to the parsed enum or throw/return a controlled
validation exception when parsing fails; update the ForMember mappings for dest
=> dest.Status and dest => dest.Category to perform this TryParse-based
validation so invalid strings no longer cause unhandled ArgumentExceptions.
Comment on lines
+25
to
+28
| CreateMap<Tag, AdminTagResponse>() | ||
| .ForMember(dest => dest.Status, opt => opt.MapFrom(src => src.Status.ToString())) | ||
| .ReverseMap() | ||
| .ForMember(dest => dest.Status, opt => opt.MapFrom(src => Enum.Parse(typeof(TagStatus), src.Status))); |
There was a problem hiding this comment.
Add validation before parsing enum value.
Enum.Parse(typeof(TagStatus), src.Status) in the reverse mapping will throw ArgumentException if src.Status doesn't match a valid TagStatus enum value. This can cause runtime failures when mapping admin responses.
🛡️ Use Enum.TryParse with validation
CreateMap<Tag, AdminTagResponse>()
.ForMember(dest => dest.Status, opt => opt.MapFrom(src => src.Status.ToString()))
.ReverseMap()
- .ForMember(dest => dest.Status, opt => opt.MapFrom(src => Enum.Parse(typeof(TagStatus), src.Status)));
+ .ForMember(dest => dest.Status, opt => opt.MapFrom(src =>
+ Enum.TryParse<TagStatus>(src.Status, out var status)
+ ? status
+ : throw new ArgumentException($"Invalid TagStatus: {src.Status}")));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CreateMap<Tag, AdminTagResponse>() | |
| .ForMember(dest => dest.Status, opt => opt.MapFrom(src => src.Status.ToString())) | |
| .ReverseMap() | |
| .ForMember(dest => dest.Status, opt => opt.MapFrom(src => Enum.Parse(typeof(TagStatus), src.Status))); | |
| CreateMap<Tag, AdminTagResponse>() | |
| .ForMember(dest => dest.Status, opt => opt.MapFrom(src => src.Status.ToString())) | |
| .ReverseMap() | |
| .ForMember(dest => dest.Status, opt => opt.MapFrom(src => | |
| Enum.TryParse<TagStatus>(src.Status, out var status) | |
| ? status | |
| : throw new ArgumentException($"Invalid TagStatus: {src.Status}"))); |
🤖 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
`@app/server/RecipeService/src/RecipeService.Application/Configs/RecipeProfile.cs`
around lines 25 - 28, The reverse mapping using Enum.Parse in CreateMap<Tag,
AdminTagResponse>().ReverseMap() is unsafe because Enum.Parse(typeof(TagStatus),
src.Status) will throw if src.Status is invalid; replace it with a safe parse
using Enum.TryParse (or a helper) to validate src.Status before mapping to
TagStatus and handle invalid values explicitly (e.g., map to a default TagStatus
or throw a clear ArgumentException), ensuring the MapFrom for dest =>
dest.Status uses the TryParse result for TagStatus instead of Enum.Parse.
Comment on lines
+47
to
+52
| var views = await viewsQuery.ToListAsync(cancellationToken); | ||
| var viewsMap = views | ||
| .GroupBy(v => v.RecipeId) | ||
| .ToDictionary( | ||
| g => g.Key.ToString(), | ||
| g => g.OrderByDescending(x => x.UpdatedAt).First()); |
There was a problem hiding this comment.
Preserve history order before paginating.
This selects the newest UserViewRecipeDetail per RecipeId, but the page still comes from response.Recipes iteration order rather than viewsMap[*].UpdatedAt. As a result, this "history" endpoint can paginate in a different order than the user's latest views, so older items can appear first and page boundaries can shift between calls.
Suggested fix
- var mapRecipe = response.Recipes
- .Skip(UserViewRecipeDetailConstant.USER_VIEW_RECIPE_DETAIL_LIMIT * skip)
- .Take(UserViewRecipeDetailConstant.USER_VIEW_RECIPE_DETAIL_LIMIT).ToDictionary();
+ var orderedRecipes = response.Recipes
+ .OrderByDescending(entry => viewsMap[entry.Value.Id].UpdatedAt);
+
+ var mapRecipe = orderedRecipes
+ .Skip(UserViewRecipeDetailConstant.USER_VIEW_RECIPE_DETAIL_LIMIT * skip)
+ .Take(UserViewRecipeDetailConstant.USER_VIEW_RECIPE_DETAIL_LIMIT)
+ .ToDictionary(entry => entry.Key, entry => entry.Value);Also applies to: 89-91
🤖 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
`@app/server/TrackingService/src/TrackingService.Application/UserViewRecipeDetails/Queries/SearchUserViewRecipeDetaiQueryHandler.cs`
around lines 47 - 52, The current code builds viewsMap of latest
UserViewRecipeDetail per RecipeId but then paginates using response.Recipes
iteration order, causing inconsistent history ordering; update the handler
(SearchUserViewRecipeDetaiQueryHandler) to reorder response.Recipes by the
corresponding viewsMap entry's UpdatedAt (descending) before applying
pagination: filter recipes to those present in viewsMap, sort them using
viewsMap[recipe.Id.ToString()].UpdatedAt descending, then proceed with paging
and mapping so page boundaries reflect newest views; apply the same change for
the other occurrence referenced around the response.Recipes usage (lines
~89-91).
Comment on lines
+14
to
+16
| CreateMap<User, GetUserDetailsResponse>().ReverseMap(); | ||
| CreateMap<User, AdminGetUserDetailResponse>().ReverseMap(); | ||
| CreateMap<UserDetailsDTO, GetUserDetailsResponse>().ReverseMap(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the duplicate map declarations and the two AutoMapper registrations
# that scan the full AppDomain in the same UserService host.
rg -n 'CreateMap<UserDetailsDTO,\s*GetUserDetailsResponse>' \
app/server/UserService/src/UserService.API/Configs/UserProfile.cs \
app/server/UserService/src/UserService.Application/Configs/UserProfile.cs
rg -n 'AddAutoMapper\(' \
app/server/UserService/src/UserService.API/DependenciesInjection.cs \
app/server/UserService/src/UserService.Application/DependencyInjection.csRepository: Carl-Johnsons/tastopia
Length of output: 572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== API UserProfile.cs (around line 14) ==="
sed -n '1,80p' app/server/UserService/src/UserService.API/Configs/UserProfile.cs | nl -ba | sed -n '1,120p'
echo "=== Application UserProfile.cs (around line 14-16) ==="
sed -n '1,80p' app/server/UserService/src/UserService.Application/Configs/UserProfile.cs | nl -ba | sed -n '1,120p'
echo "=== API DependenciesInjection.cs (around AddAutoMapper) ==="
sed -n '1,120p' app/server/UserService/src/UserService.API/DependenciesInjection.cs | nl -ba | sed -n '1,160p'
echo "=== Application DependencyInjection.cs (around AddAutoMapper) ==="
sed -n '1,120p' app/server/UserService/src/UserService.Application/DependencyInjection.cs | nl -ba | sed -n '1,160p'Repository: Carl-Johnsons/tastopia
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== API UserProfile.cs (around line 14) ==="
awk 'NR>=5 && NR<=30 {print NR ":" $0}' app/server/UserService/src/UserService.API/Configs/UserProfile.cs
echo "=== Application UserProfile.cs (around line 14-16) ==="
awk 'NR>=5 && NR<=40 {print NR ":" $0}' app/server/UserService/src/UserService.Application/Configs/UserProfile.cs
echo "=== API DependenciesInjection.cs (around AddAutoMapper) ==="
awk 'NR>=1 && NR<=80 {print NR ":" $0}' app/server/UserService/src/UserService.API/DependenciesInjection.cs
echo "=== Application DependencyInjection.cs (around AddAutoMapper) ==="
awk 'NR>=1 && NR<=90 {print NR ":" $0}' app/server/UserService/src/UserService.Application/DependencyInjection.csRepository: Carl-Johnsons/tastopia
Length of output: 5893
Remove the duplicate UserDetailsDTO ↔ GetUserDetailsResponse map to avoid double AutoMapper configuration.
CreateMap<UserDetailsDTO, GetUserDetailsResponse>().ReverseMap();is declared in both:app/server/UserService/src/UserService.Application/Configs/UserProfile.cs(line 16)app/server/UserService/src/UserService.API/Configs/UserProfile.cs(line 14)
- AutoMapper is registered twice in the same API host with
AppDomain.CurrentDomain.GetAssemblies():app/server/UserService/src/UserService.API/DependenciesInjection.cs(lines 22-27)app/server/UserService/src/UserService.Application/DependencyInjection.cs(lines 17-23)
- This makes AutoMapper discover both profiles and configure the same type map twice, which can lead to configuration/startup failures.
Remove the duplicated CreateMap from one profile (or register AutoMapper only once / restrict the scanned assemblies).
🤖 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 `@app/server/UserService/src/UserService.Application/Configs/UserProfile.cs`
around lines 14 - 16, Remove the duplicate AutoMapper type map by deleting the
CreateMap<UserDetailsDTO, GetUserDetailsResponse>().ReverseMap() declaration
from one of the UserProfile classes (either the UserService.Application
UserProfile or the UserService.API UserProfile) so the mapping is defined only
once; alternatively, fix the registration to only add AutoMapper once by
adjusting DependencyInjection (the methods that call services.AddAutoMapper(...)
in UserService.Application.DependencyInjection and
UserService.API.DependenciesInjection) so
AppDomain.CurrentDomain.GetAssemblies() does not cause both profiles to be
scanned. Ensure you update either the profile (UserProfile) to remove the
duplicated CreateMap symbol or the dependency injection registration to prevent
double registration.
| ;; | ||
| *) | ||
| # Invalid argument | ||
| printf "${DANGER}Invalid argument: '$TARGET_ENV'. Usage: $0 [-psh] [dev|staging|production]${NC}\n" |
There was a problem hiding this comment.
Fix format-string vulnerability in error message.
Line 17 passes $TARGET_ENV directly into the printf format string, which allows format-string injection if the argument contains format specifiers like %s or %n. Use %s in the format and pass the variable as an argument.
🔒 Proposed fix
- printf "${DANGER}Invalid argument: '$TARGET_ENV'. Usage: $0 [-psh] [dev|staging|production]${NC}\n"
+ printf "${DANGER}Invalid argument: '%s'. Usage: %s [dev|staging|production]${NC}\n" "$TARGET_ENV" "$0"🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 17-17: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
(SC2059)
🤖 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 `@scripts/local/apply-all-migrations.sh` at line 17, The printf call in
scripts/local/apply-all-migrations.sh currently interpolates $TARGET_ENV
directly into the format string, creating a format-string vulnerability; change
the printf invocation to use an explicit "%s" format token and pass TARGET_ENV
as a separate argument (i.e., printf "%s" ... $TARGET_ENV) so user content is
not interpreted as format specifiers — locate the printf line that mentions
"${DANGER}Invalid argument: '$TARGET_ENV'..." and replace the inline
interpolation with a format string and variable argument.
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
2 participants
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.
Summary by CodeRabbit
New Features
Improvements
Dependencies