feat: Unified repository servers - #82
Merged
Merged
Conversation
…support Rename DesktopRepositoryServer to RepositoryServer with unified namespace OpenShock.RepositoryServer. Reorganize controllers into V1 (backwards-compat desktop endpoints at /v1/) and V2 (desktop at /v2/desktop/, firmware at /v2/firmware/). Add full firmware repository with chips, boards, versions, artifacts, and release notes backed by PostgreSQL enums and EF Core entities. Includes public OTA endpoints and admin CRUD with token auth. Update Docker, CI/CD workflows, and generate AddFirmwareTables migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add PUT /v2/firmware/admin/versions/{version}/boards/{board}/upload
endpoint that accepts multipart binary uploads, computes SHA256,
uploads to BunnyCDN via HTTP API, and stores artifact metadata
- Add CdnStorageService wrapping BunnyCDN Storage HTTP API
- Extend FirmwareConfig with CdnStorageUrl and CdnStorageApiKey
- Update LatestController to return all artifact types (not just Merged)
and add optional ?board= query filter
- Unify FirmwareLatestResponse to use List<FirmwareArtifactDto> matching
FirmwareVersionResponse shape
- Make artifacts optional in CreateFirmwareVersionRequest
- Extract shared GetArtifactFileName into FirmwareArtifactFileNames util
- Delete unused FirmwareBoardArtifact model
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…penshock/RepositoryServers into feature/unified-repository-server
Restores RepoServerDb/Module.cs, RepoServerDb/Version.cs and V1/CiCdController.cs to their pre-firmware-rewrite state. The firmware work should not be modifying desktop module code.
- Test fixture now injects config via UseSetting so values reach Program.cs before ApiConfig validation (ConfigureAppConfiguration overlays only apply at Build() in minimal hosting) - Make RepoServerContext.MapEnums the single source of enum mappings with pinned DB names; drop the duplicate HasPostgresEnum block and map enums in the migration-tool path too - Register only the pooled DbContext factory and derive scoped contexts from it; the AddDbContextPool + AddPooledDbContextFactory combo applied MapEnums twice and broke type mapping on first model use - Order advisories by CLR severity rank in memory since MapEnum creates PG enum labels alphabetically - Opt into the Microsoft.Testing.Platform dotnet test runner via global.json and update the documented test command
…erable transitives - Generate the firmware/USB/repositories schema migration (enums, 14 new tables, unique constraints); desktop tables intentionally untouched - Declare the usb_serial_filters unique (vid, pid) NULLS NOT DISTINCT index fluently via AreNullsDistinct(false) instead of the never-written raw-SQL migration the model comment referenced - Test fixture now applies real migrations through MigrationOpenShockContext instead of EnsureCreated, so the suite validates the migration chain - Enable CPM transitive pinning and pin Microsoft.OpenApi 2.7.5 (GHSA-v5pm-xwqc-g5wc) and OpenTelemetry.Api 1.15.3 (GHSA-g94r-2vxg-569j)
- Replace deprecated HasCount(n) assertions with Count().IsEqualTo(n) - Pass the Postgres image via the PostgreSqlBuilder constructor instead of the obsolete parameterless constructor + WithImage
C# mirrors of the ReleaseData/ChangeEntry/NoticeEntry/ReleaseNoteEntry/ Repository structs emitted by the OpenShock release-tool (schema_version 1). Not consumed yet — ingestion wiring lands separately once the integration point is decided.
…pository-server # Conflicts: # .github/workflows/ci-build.yml # Directory.Packages.props # RepositoryServer/Controllers/V1/RepoController.cs # RepositoryServer/ExceptionHandler/ExceptionHandler.cs # RepositoryServer/appsettings.json
Hubs can only know their board as a name — OPENSHOCK_FW_BOARD is the
PlatformIO env, compiled in at build time — but the single-board endpoints
were constrained to {boardId:guid}. A hub had no way to obtain that UUID, so
the route constraint rejected its request before the handler ever ran.
Make the name the public identifier everywhere: path segment, the boardId
field in responses, the boards map key, and the board segment of artifact
URLs and storage keys. UUIDs remain the primary key and FK target, and are
still accepted on public paths for admin tooling that holds one.
CI/CD ingestion follows the same rule, so workflows never need a name->UUID
lookup pass: InitReleaseRequest.Boards takes names, the upload route takes
{board}, and the release-incomplete / missing-artifact problems name the
board instead of emitting a UUID into CI logs.
Resolution goes through one chokepoint, FirmwareBoardLookup, which returns
the canonical stored name regardless of what the caller sent. That is where
board aliases plug in later: alternate names can map onto a canonical board
without touching storage layout, response shapes, or hubs. Renaming a board
in place is not supported and is documented as such — published versions are
immutable and their artifacts live under the name in force at publish time.
Board names are now validated as URL-safe, since they are interpolated into
storage keys unescaped; this notably rejects '/', which would otherwise let a
board name reshape the storage path.
Also fold the five copies of the CDN path interpolation into
FirmwareArtifactFileNames.BuildStoragePath/BuildUrl, and resolve the board
before the up-to-date check in LatestController so an unknown board reports
404 rather than being masked as 204 "no update needed".
Spec: correct §9.2, which told hubs to flash the 'merged' artifact. That is a
full-flash esptool image (bootloader at 0x1000, partition table at 0x8000,
app at 0x10000); writing it into an OTA app slot would put a bootloader image
where the app belongs. Hubs flash app + staticfs per-partition.
hhvrc
marked this pull request as ready for review
August 13, 2026 11:12
|
Important Review skippedToo many files! This PR contains 184 files, which is 84 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (184)
You can disable this status message by setting the 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 |
…eness Three defects, all downstream of making the board name do more work than a label should. Storage keys used the canonical board name, so renaming a board silently 404'd every artifact ever published for it. Nothing records the name a blob was written under, so it was not recoverable by query either — and UpdateBoard had no guard, while DeleteBoard directly below it did. Storage keys now use the board UUID, which never changes, so a rename is a pure metadata edit. The name remains the public identifier everywhere it acts as a label: routes, response boardId, the boards map key, ingestion and error messages. Consumers already treat `url` as opaque, so nothing outside the server observes the difference. Name uniqueness was case-sensitive while resolution was case-insensitive, so ESP32-Core and esp32-core could coexist and both resolve to whichever sorted first. The loser could never receive an upload, and a hub compiled with its spelling would be served the other board's firmware — potentially built for a different chip. A unique index on lower(name) makes that unrepresentable and lets the lookups use an index instead of a sequential scan. Expression indexes cannot be declared in the EF model, so the model-level index is relaxed to non-unique and the real constraint is created in the migration; the migration fails loudly if duplicates already exist, since auto-merging would orphan artifacts. Chips now identify by name on the public surface. Chip names are unique and externally pinned — they must match esptool-js identifiers exactly, because the flashtool passes them straight through — and no chip ever appears in a storage path, so a surrogate key buys nothing there. /boards filters by ?chip= name. Also fix the changelog parser, which dropped section prose whenever the section also had bullets, and dropped indented list items entirely. The spec's own §5.3 worked example documents 8 notes and exercises both constructs; the golden test passed only because its fixture omitted the two lines that would have exposed this, and a second test asserted the dropped note stays dropped. The fixture is now the spec's example verbatim, and the spec's expected output is corrected to match its own concatenation rule.
…wnership The ingestion path authenticated callers but never authorized them. The OIDC handler inserted a repositories row for whatever repository_owner and repository claims arrived, so the allowlist populated itself from whoever showed up. A GitHub OIDC token proves only that some workflow somewhere on GitHub requested one: the issuer is shared by every repository, and the audience is a plain string any workflow can request by name and which ships in plaintext in appsettings.json. Neither identifies the caller. Any GitHub repository could therefore obtain a publishing principal. Registration is now a lookup that fails closed, and onboarding is an explicit admin action via PUT /v2/firmware/admin/repositories — an endpoint the spec has always specified in §5.1 and §6.1, but which was never built. Removes the orphaned CiCd.RepositoryOwner config key, which nothing read. Authentication identifies a repository, not which release it may touch, and every registered repository presents an equally valid principal. RepositoryId was recorded at init and never consulted again, so upload, publish and abort all resolved by releaseId alone: any authorized repository could inject binaries into another's in-flight release, publish it under that repository's identity and commit hash, or delete it. All three now re-check ownership and return 403. Desktop module publishing had the same hole and no ownership model at all — modules gain an owning repository_id, and one with no owner assigned is closed to every publisher rather than open to any. Republishing an existing module version is also rejected: the upsert is keyed on (module, version), so it previously replaced a published zip URL and hash in place, after which integrity checks would pass against the replacement. Firmware likewise rejects initialising a release for an already-published version, which otherwise let a second release overwrite live artifacts at the same storage keys before any publish call. Test coverage for all of the above, which previously had none: ReleasesController, the desktop CI/CD path, and the TTL cleanup service. The cleanup pass is now internal rather than private so tests can drive it deterministically — StartAsync returns at the first await inside ExecuteAsync, so start-then-stop races the very tick it means to observe. A test auth handler substitutes the JWT-validation half of the OIDC scheme; the allowlist lookup itself is not covered by it, and is documented as verified by construction instead. Replaces the test asserting the onboarding endpoint is absent.
H5: every "latest" and list query sorted only by release_date, which is
client-supplied and not unique — the spec's own example uses midnight. Ties
were broken arbitrarily by the database, so /manifest, /latest/{channel} and
/latest/{channel}/{board} could disagree with each other and flip between
requests, which a hub experiences as firmware flapping. Offset pagination had
the same problem and could duplicate and drop rows across pages. Ordering now
falls back to version, the primary key, giving a total order.
I3: channels now cascade, stable into beta into develop. CI advances all three
pointers when it ships a stable build, so a stable release is the newest thing
a beta subscriber should receive; strict per-channel equality pinned beta hubs
to the last explicit release candidate and offered them a downgrade.
H4: artifact upload deleted the board's prior staged rows and then hashed and
uploaded each file in turn, reporting mismatches at the end. One good file
alongside one bad one therefore left the good blob on the CDN with no row
referencing it — unreachable forever, since both abort and the TTL job
enumerate staged rows — while the board's previously valid staged state was
already gone. Everything is now verified before anything is deleted or
written, and the delete/upload/insert sequence runs in one transaction.
H2: the cleanup job read expired releases and then wrote Aborted
unconditionally, so a release hitting its TTL exactly as CI published it ended
up aborted in the database, live in the public API, and deleted from the CDN.
It now claims each release with a conditional update narrowed to the statuses
it decided on, and only deletes artifacts if the claim won. This also means
the tracked entity keeps its pre-abort status, so the log can finally say
which TTL fired.
H3: releaseDate is coerced to UTC on ingest. Npgsql rejects a non-zero offset
for `timestamp with time zone`, so any CI runner outside UTC got a bare 500.
H7: adds a partial unique index on version where status is staging or editing.
The controller's read-then-insert guard let two concurrent jobs for the same
tag both pass, after which they staged into the same CDN keys and raced each
other's uploads — a permanent hash mismatch for every OTA client, surfaced
nowhere. Both paths now return the same 409.
Board and chip name collisions return 409 rather than escaping as a 500, now
that lower(name) uniqueness is enforced by the database.
Adds scripts/seed-catalog.sh, which bootstraps the 13 PlatformIO boards, their
4 chips, and optionally the publish allowlist. A fresh database has no catalog
and InitRelease rejects unknown boards, so the first ingestion would otherwise
404.
Squashes the four migrations this branch adds into a single UnifiedRepositoryServer migration, via `dotnet ef migrations remove` down to the Initial migration that develop already has, then a fresh `dotnet ef migrations add`. The expression and partial indexes are re-declared in raw SQL because the EF model cannot express them, so the tooling could not regenerate them: lower(name) uniqueness on boards and chips, and the partial unique index on version for open releases. Also removes whitespace-only churn against develop: - ci-build.yml and ci-tag.yml carried trailing-whitespace edits on blank lines, which is where most of their diff came from. Rebuilt from develop's file with only the substantive renames applied, so their diffs are now entirely image/path changes. - global.json had the same, rebuilt the same way. - ApiConfig.cs, AdminController.cs and RepoController.cs had each gained a trailing newline that develop's copies do not have. Note that `git diff -w` overstates this: it also collapses genuine structural edits whose lines differ only by braces and indentation, which is why Program.cs and appsettings.json appeared to carry churn and do not. Verified with --ignore-space-at-eol and --ignore-blank-lines, which cannot be fooled that way, and the rebuilt files are byte-identical to their previous content under `git diff -w`.
…ublish
Uploads wrote straight to the published CDN key, so a version's artifacts were
readable at their final URL before the version existed, and a release that
failed midway left a half-populated version directory. It also meant abort and
the TTL job reconstructed live keys to delete, which is what made an abandoned
re-run able to remove artifacts a published version was serving.
Uploads now land under _staging/{releaseId}/, and PublishRelease copies each
object to its published key inside the publish path. A published key is
written exactly once; abort and TTL cleanup delete the release's own staging
prefix wholesale and cannot name a live key at all.
Storage is not transactional, so the promotion is ordered deliberately: copy
first, then commit. A database failure after copying leaves unreferenced
objects at published keys — invisible, since no version row points at them,
and overwritten by a later publish of the same version — and they are unwound
best-effort anyway. Committing first would instead publish a version whose
artifacts are not all present, which hubs would see as 404s mid-update.
Adds CopyFileAsync to IStorageService: server-side CopyObject on S3, File.Copy
locally, and a stream-through on Bunny, whose storage API has no copy.
Both files carry a BOM on develop and lost it during the project move, which showed up as a spurious change to their first line. The repository is inconsistent about BOMs generally — most carried-over files have one, most new files do not — but that is pre-existing and not this PR's to settle; these two are the only files whose BOM state actually changed.
Also corrects the artifact URL convention, which still described the board name as the storage path segment.
…config Firmware release ingestion and desktop module publishing share one authentication scheme, and nothing distinguished them. Registering a repository so it could publish desktop modules therefore also let it initialise and publish firmware releases — the per-release ownership checks only stop it hijacking someone else's in-flight release, not starting its own. Registrations now carry scopes (publish_firmware, publish_modules). The OIDC handler already loads the repository row, so it attaches them as claims, and each ingestion controller sits behind an authorization policy requiring the scope its grant was issued for. A repository registered with no scopes is authenticated but cannot publish anything, which is the safe default for a new row. Scopes are entirely server-side: a publishing workflow neither sends nor sees them, so adopting this costs the repositories nothing. Provider, owner and repo are now matched case-insensitively, enforced by a unique index on (provider, lower(owner), lower(repo)). GitHub treats owner and repo names case-insensitively and the casing in a token's claims follows whatever the repository is currently named, so an exact match would let a cosmetic rename silently de-authorize a repository — or let onboarding create a second row that the handler's lookup would never reach. The audience moves from Firmware:CiCd:Audience to a top-level CiCd:Audience. It was nested under firmware config while also governing desktop publishing, and appsettings.json already had a top-level CiCd block that the binding ignored. It remains a single value: the audience is what makes an OIDC token non-transferable between services, but rotating it is rare enough that accepting a list was not worth the extra surface. Also stops the DEBUG configuration dump printing secrets. GetDebugView() emitted every config value and the process environment — admin token, database password, storage credentials, Discord webhook URLs — to stdout on any Debug build. It now prints resolved keys with credential-looking values masked, which is what is actually useful for diagnosing binding.
Webhook targets were a flat list in configuration and every notification fanned out to all of them, so maintainer alerts could not be routed away from a public releases channel, and adding a channel meant a redeploy. Targets now live in discord_webhooks, each subscribing to specific events, and are managed through /v2/admin/discord-webhooks. The route sits outside /firmware/admin because desktop module publishing notifies through the same webhooks. A webhook URL is a credential — possession of it is authorization to post to that channel — so it is write-only. Responses carry a masked form that keeps the Discord webhook id, which is what identifies the channel in Discord's own UI, and drops the token. Delivery failures log the event rather than the URL. Also fixes the delivery path, which was firing Task.Run with the triggering request's CancellationToken and a typed HttpClient disposed with the request scope. Notifications were therefore lost non-deterministically, including the "needs editing" alert that is the only signal a release is stuck. The service is now a singleton resolving its own HttpClient and DbContext, with its own delivery timeout, since this work deliberately outlives the request.
The shipped desktop app hardcodes https://repo.openshock.org/1 as a built-in module repository (Desktop/Constants.cs, BuiltInModuleRepositories), and develop's controllers route on "/{version:apiVersion}/". This branch had changed them to "/v{version:apiVersion}/", so deploying it would have returned 404 to every installed desktop client for the module manifest and for module version downloads. firmware-api-spec.md documents the unprefixed form throughout ("/2/firmware/..."), so the implementation disagreed with both the deployed clients and its own contract. Routes now match: /1/ for the desktop endpoints and /2/firmware/... for the firmware API. Updates the two consumers written against the prefixed form in this branch — the frontend client and the firmware OTA client, neither of which has shipped — along with the CI ingestion action, the seeding script and the tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.