Pin the renewal scan to a fixed UTC hour, fail fast on blank secrets, drop scaffolding (#89-#91) - #94
Merged
Conversation
…crets, drop scaffolding (#89-#91) Pin the renewal scan to a fixed UTC hour (#89) - The job was never nightly: a flat 24h delay from process start meant the scan time was whatever the last deploy happened to be, and a restart slightly under 24h after the previous one could skip a calendar day - a skipped day being a missed alert on the app's primary promise. - DelayUntilNextRun(now, utcHour) is pure and public so the schedule is testable without waiting on real time. Configurable via RenewalScan:UtcHour, default 2. A mistyped or out-of-range value falls back to the default rather than taking the API down. - The scan on startup is kept deliberately. It is the catch-up for exactly the skipped-day case above, and notifications_log makes a repeat scan for a day already scanned a no-op. - No scheduling library for one daily job. Fail fast on blank secrets (#90) - Steps 1 and 2 were already satisfied: appsettings.json ships ConnectionStrings and Jwt:Secret as empty strings, and `git log --all -p` over both appsettings files shows no non-empty value was ever committed. Nothing to rotate. - The actual hole was the reading side. All four required-config reads used `?? throw`, which only catches null - so the empty string that ships in appsettings.json sailed straight through and the API booted with an empty JWT signing key. IConfiguration.GetRequired now rejects null-or-whitespace, and all four call sites route through it. - appsettings.Docker.json does not exist, contrary to the issue: the api service in docker-compose.yml got no connection string and no signing key at all, so `docker compose up` could not have worked. It now takes the compose-local connection string inline (same throwaway credentials as the db service beside it) and the signing secret from ${SUBVORA_JWT_SECRET}, which compose refuses to start without. The secret is never written to a tracked file. - README documents the hook setup, user secrets, the Docker env vars, and the per-project test commands. Delete template scaffolding and correct the docs (#91) - Removed UnitTest1.cs and the three TestRunnerSmokeTest.cs files. They proved the runner worked while CI was being stood up; all four projects have real suites now. - CLAUDE.md no longer claims "no application code exists yet", lists the real five-project layout, and records the actual build/test/migration commands including why the solution is not built as a whole. - Did not move the requirement docs. The issue describes three root technical_requirements*.md files as documentation sprawl. Only one exists, and .gitignore:66 ignores it deliberately alongside prd.md and issues.md - moving it into docs/ would commit a file the repo is explicitly keeping out of source control, which is the owner's call. Instead: README's link to it is removed (it was a dead link for anyone cloning), the four code comments citing it and a non-existent technical_requirements.backend-hardening.md now point at real references, and CLAUDE.md explains what the file is so the next reader is not hunting for a missing doc. Closes #89 Closes #90 Closes #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 2026
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
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.
The last three unblocked issues under #76.
?? throw, which only catches null — the empty string shipped inappsettings.jsonbooted the API with an empty JWT signing key#90 — the hole was somewhere other than where the issue looked
Steps 1 and 2 were already satisfied.
appsettings.jsonships both values blank, andgit log --all -pover both appsettings files shows no non-empty value was ever committed — so no rotation is needed (step 5).What was actually broken:
Program.cs×3,JwtTokenService) read required config with?? throw."Jwt:Secret": ""is non-null, so nothing threw and the API ran with an empty signing key — precisely the "far worse than a failed boot" case step 4 names. Fixed once inIConfiguration.GetRequired, which all four now route through, rather than four separate guards.appsettings.Docker.jsondoes not exist. The issue assumes it carries the compose-local connection string. Theapiservice indocker-compose.ymlhadASPNETCORE_ENVIRONMENT: Dockerand nothing else — no connection string, no signing key — sodocker compose upcould not have worked. It now gets the connection string inline (the same throwaway credentials as thedbservice directly above it) and the signing secret from${SUBVORA_JWT_SECRET:?...}, which compose refuses to start without. The secret never lands in a tracked file.#89 — startup behaviour, stated deliberately (step 5)
The scan on startup is kept. It is the catch-up for exactly the failure this issue describes: a restart that would otherwise skip a calendar day.
notifications_logmakes a repeat scan for an already-scanned day a no-op, so the cost of keeping it is zero and the cost of dropping it is a missed alert after an unlucky deploy.RenewalScan:UtcHourdefaults to 2 — late enough that "renews tomorrow" still holds for most of the world, quiet enough not to compete with daytime traffic. An absent or out-of-range value falls back to the default rather than failing startup; a mistyped scan hour should not take the API down.Sibling job, not fixed here (as the issue asks):
FxRateRefreshBackgroundServicehas the identicalTimeSpan.FromHours(24)-from-start drift. It matters much less — a stale FX rate skews a dashboard total slightly, where a skipped renewal scan means a missed alert — but it's the same bug and worth its own issue.#91 — one part not done, on purpose
The issue describes three root
technical_requirements*.mdfiles as documentation sprawl to be moved intodocs/. Only one exists, and.gitignore:66ignores it deliberately, alongsideprd.mdandissues.md. Moving it intodocs/would commit a file the repo is explicitly keeping out of source control — that's the owner's call, not a mechanical cleanup, so I left it where it is.The real damage from that arrangement is fixed instead:
README.mdlinked to it in the documentation table — a dead link for anyone cloning. Removed.technical_requirements.backend-hardening.mdthat doesn't exist anywhere. Repointed at real references (docs/Design.md, the migration by name) or reworded.CLAUDE.mdnow explains what the file is, so the next reader isn't hunting for a missing doc.Everything else in #91 is done:
UnitTest1.csand all threeTestRunnerSmokeTest.csare gone, and CLAUDE.md carries the real five-project layout plus the actual build, test and migration commands — including why the solution isn't built as a whole (mobile's Windows-only TFM on Linux, and the Android SDK requirement even on Windows).Verification
SubVora.Mobile.TestsUnitTest1)SubVora.Api.Tests/SubVora.Application.Tests/SubVora.Infrastructure.TestsWindows Smart App Control on the dev machine blocks freshly-built unsigned test assemblies (
FileLoadException … An Application Control policy has blocked this file. (0x800711C7)); it began mid-session and now catches every rebuilt test host. Disabling it is a one-way system change, so it was left alone. Everything compiles (dotnet build src/SubVora.Api/SubVora.Api.csproj -c Releaseclean), and CI is the authority for those three suites — the runners are unaffected.This PR deletes the smoke tests whose whole job was proving the runner works, so a green CI run is the required check rather than a nicety: it is what confirms all four projects still discover and run their real tests.
New coverage: six
DelayUntilNextRuncases (before/after/exactly-at the hour, date boundary, month end, midnight) and fiveGetRequiredcases including the blank-string one that is the actual bug.Closes #89
Closes #90
Closes #91