Fix three bugs, and raise coverage from 72% to 88% - #1
Merged
Conversation
Bugs found by putting the untested paths under test. 1. A rejected relay upload deadlocked the caller. `attachAsset` is rejected before it subscribes to the chunk stream for a duplicate name, an unknown release or an unsafe filename. Nothing was then listening to that stream, so the upload session's `close()` could never complete and `commit` hung until the caller's RPC timeout — 30 seconds to deliver a ConflictException that was decided immediately. The session also kept buffering chunks into the dead controller, so a multi-gigabyte artifact was held in memory on its way to an error already settled. The session now records the rejection, fails the next chunk fast, and stops awaiting a close that cannot happen. 2. S3 never recorded the SHA-256, so `head` always reported null. `_checksumHeaderOf` documented reading the digest back from `x-amz-meta-sha256`, but nothing ever wrote it — while GCS did record it via a metadata patch, so the two backends disagreed. The digest is now attached whenever it is known before the request is sent: always for a buffered body, and for a streamed one when the caller declared it. S3 headers precede the body and S3 has no metadata-only update, so the remaining case is documented on `head` rather than paid for with a server-side copy per upload. 3. The server and node banners bypassed the injected output sinks. Both wrote to `stdout` directly, so `--quiet` could not suppress them and an embedding tool could not capture them — despite the CLI being documented as driveable in-process with injected sinks. Both now take the sink the runner already has. Coverage: 72.4% -> 88.2%, 413 -> 707 tests. - A fake S3 and a fake GCS endpoint speaking the real wire protocol — signed requests, XML and JSON responses, ranges, pagination — so the shared ObjectStorage contract runs against both cloud backends as well as memory and local disk, in six configurations. This is what surfaced bug 2; a stub returning canned bytes would only have proved the methods exist. - An OmnyStoreApi conformance suite running the same 35 scenarios against all four implementations: embedded, hub over a local provider, hub over a remote node across the RPC protocol, and the client over a real socket. This tests the central design claim directly, and surfaced bug 1. - Real-socket coverage for `omnystore server` and `omnystore node`, which surfaced bug 3. - Full coverage of the auth providers and the API error rendering. - Multi-architecture tests: one release carrying five os-arch builds, an updater always resolving its own, and never being handed another's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
`omnystore download` required `--platform` on every invocation: without it, a release with more than one artifact failed asking you to choose. Downloading an artifact almost always means "the one I can run", so requiring the flag every time invited fetching a build for the wrong architecture — and there was no way to ask what the current architecture even was. Adds `Platforms`, which reports this process's `os-arch` token by reading the Dart VM's own target triple (the only place the architecture is exposed), and normalises the spellings other toolchains use — `darwin-aarch64`, `osx-x86_64`, `win32-amd64` all resolve. `download` and `check-update` now default to it, with `--platform any` to opt out. Matching stays exact. An x64 build *can* run on Apple Silicon under Rosetta, but choosing that silently would ship the slower binary to every Apple Silicon user forever and hide a missing native build from whoever publishes it. A publisher who wants a fallback ships an artifact with no platform, which the resolver already prefers second. Also fixes `omnystore download` against a `--data` registry, which could never work: it asked for a download URL a local store has none of, and failed. The bytes are already on the machine, so they are copied out and verified against the digest on the asset record — the same guarantee the HTTP path gives, including deleting the file if it does not match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The S3 and GCS backends had each grown their own copy of the same small
helpers. Two of those copies mattered: `_canonicalQuery` and the RFC 3986
encoder are the same construction in AWS SigV4 and GCS V4 signing, so a fix
found while debugging one backend would have silently left the other wrong,
and the symptom is a 403 whose message names no character.
- `Rfc3986` (new) — encodeComponent / encodePath / canonicalQuery
- `Hex` (new) — replaces three private `_hex`
- `CloudStorageSupport` — totalSizeOf / normalizePrefix / trimSlashes /
truncateBody
- `Names.sanitizeForHeader` — replaces the three content-disposition
sanitisers, which had already drifted: only one
of them stripped DEL
Also splits `StoreApiService.build()` (490 lines) into one registration
function per resource, mapping 1:1 to the endpoint table in its doc comment,
and adds `_Handler`/`_Route` typedefs so a group cannot register a handler
without the error-rendering and write-guard wrapper.
`StoreRpcServer._dispatch` is deliberately left as one flat switch: splitting
it would replace an exhaustive dispatch whose default rejects unknown actions
with null-sentinel plumbing between groups.
Adds direct tests for the shared helpers — they were covered only
transitively, where a drift shows up as a signature mismatch rather than as a
failure naming the helper.
751 tests, 88.5% line coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--platform` took one value, so pulling a release for more than one target
meant one invocation per target and no way to say "everything in this
release" at all. It is now a multi-option:
omnystore download --package omnyagent # this machine
omnystore download --package omnyagent --platform linux-x64
omnystore download --package omnyagent --platform linux-x64,macos-arm64
omnystore download --package omnyagent --platform all -o dist/
omnystore download --package omnyagent --platform any --asset agent.tar.gz
`all` takes every artifact in the release — the mirror, signing and
GitHub-release case, as opposed to the updater case a single platform serves.
`all` and `any` select on their own; combining either with a specific platform
is a usage error rather than a flag that is silently dropped.
With more than one artifact selected, `-o` names a directory and is created if
absent, since `--platform all -o dist/` is the natural spelling of a release
bundle. Platforms that resolve to the same artifact through an alias or the
portable fallback download it once.
Every platform is resolved before the first byte is fetched, so a typo in the
third of four platforms fails immediately instead of leaving a partial bundle
on disk, and each artifact is checksum-verified individually.
Defaulting to the current machine, alias matching and the single-platform
error message are unchanged.
759 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`kind` was settable on upload and read by the update service, but there was no
way to ask for one on the way out — and `download` did not apply the update
service's kind rules at all, so picking the artifact for a platform could hand
back that platform's `.sha256` file if it happened to sort first.
omnystore download --package omnyagent --kind installer
omnystore download --package omnyagent --kind installer,archive
omnystore download --package omnyagent --platform all --kind checksums
omnystore download --package omnyagent --kind all
Without `--kind`, a single-artifact download now excludes auxiliary artifacts
and prefers an installer over an archive — the same order `check-update` uses,
so the two can no longer disagree about which artifact *is* the release.
`--platform all` deliberately keeps the auxiliary files: a mirror or a
GitHub-release step wants the digests too, and "all" means all.
The kind rules moved out of UpdateResolver's privates into `AssetKinds`, which
is now the one place that says what a kind means. It merges the two paths the
old code had — an explicit tag, and the filename convention for untagged
artifacts — into `AssetKinds.of`, and is exported so callers can apply the same
vocabulary. Only the auxiliary kinds are inferred from a filename; guessing
`installer` from `.dmg` would change which artifact clients are offered.
`release publish --asset` gained the third field: `path`, `path:platform` and
`path:platform:kind` are all valid. The spec is parsed by peeling at most two
trailing `:token` suffixes, so `C:\build\agent.exe` is still a path.
785 tests, 88.6% line coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test count predated the coverage work, and the download selection and AssetKinds vocabulary were missing entirely. This text becomes the release notes, so it should describe what 1.0.0 actually ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Three bugs, found by putting the untested paths under test — and the tests that found them.
Bugs fixed
1. A rejected relay upload deadlocked the caller
attachAssetis rejected before it subscribes to the chunk stream for a duplicate name, an unknown release, or an unsafe filename. Nothing was then listening to that stream, so the upload session'sclose()could never complete andcommithung until the caller's RPC timeout — 30 seconds to deliver aConflictExceptionthat was decided immediately.Worse, the session kept accepting chunks into the dead controller, so a multi-gigabyte artifact was buffered in memory on its way to an error already settled.
The session now records the rejection, fails the next chunk fast, and stops awaiting a close that cannot happen.
2. S3 never recorded the SHA-256, so
headalways reported null_checksumHeaderOfdocumented reading the digest back fromx-amz-meta-sha256, but nothing ever wrote it — while GCS did record it via a metadata patch. Two backends, same operation, different results.The digest is now attached whenever it is known before the request is sent: always for a buffered body, and for a streamed one when the caller declared it. S3 headers precede the body and S3 has no metadata-only update, so the one remaining case is documented on
headrather than paid for with a server-side COPY on every upload.3. The server and node banners bypassed the injected output sinks
Both wrote to
stdoutdirectly, so--quietcould not suppress them and an embedding tool could not capture them — despite the CLI being documented as driveable in-process with injected sinks. Both now take the sink the runner already has.Coverage: 72.4% → 88.2% (413 → 707 tests)
ObjectStoragecontract now runs against both cloud backends as well as memory and local disk — six configurations. Signed requests, XML/JSON responses, ranges, pagination. This surfaced bug 2; a stub returning canned bytes would only have proved the methods exist.OmnyStoreApiconformance suiteomnystore server/nodetestsos-archbuilds; an updater always resolving its own, and never being handed another's.Lowest-covered files went from
s3_object_storage.dartat 18.8% andserver_commands.dartat 16.8% to 88%+ and 80% respectively.Also
Documented the multi-architecture model in
doc/workflows.md— one release, manyos-archartifacts, exact matching, and whatisInstallablemeans when a matrix runner drops a build.Verification
dart format,dart analyze --fatal-infos --fatal-warnings, 707 tests,dart pub publish --dry-run(0 warnings) — all green.