WS05 adapter manifest#171
Merged
Merged
Conversation
Implements the adapter.yaml manifest schema, parser, validator, OCI annotation mirror, and runtime cross-check per WS05 spec. New package: internal/adapter/manifest/ - schema.go: Manifest, Platform, SecretDecl, ContainerImageRef, Schema, SchemaField - parse.go: Parse, ParseFile, ParseFromFS (works with oci.Layout.Open fs.FS) - validate.go: Validate with all rules (schema_version range, name regex, semver, source_url scheme, platforms, sdk_protocol_version range, schema field types, secrets, compatible_environments, container_image digest) - annotations.go: OCI annotation keys (dev.criteria.adapter.*) + AnnotationMap - verify.go: Verify compares static manifest against v2.InfoResponse, checking structural equality of schemas and set equality of slices - testdata/adapter.yaml: canonical reference fixture - Tests: parse_test.go, validate_test.go, annotations_test.go, verify_test.go covering round-trips, every failure rule, and every divergence field. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix source_url scheme regex: ^[a-z][a-z0-9+.-]*$ → ^[a-z][a-z0-9+.-]+$ - Add TestParse_InvalidYAML asserting malformed YAML returns unmarshal error - Add TestParse_ReaderError using errReader to cover I/O error path - Add TestParseFile_ReferenceFixtureValidates (fixture parses + validates) - Add TestValidate_SchemaFieldTypeEmpty asserting empty Type is rejected - Rewrite TestValidate_EveryRuleHasRow → TestValidate_AllRulesCovered - Rename SourceURL subtest bad_scheme → ftp_scheme - Replace custom fmtInt with fmt.Sprintf - Add TestValidate_ContainerImageNoDigest (Ref without Digest passes) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
brokenbot
approved these changes
May 29, 2026
Collaborator
brokenbot
left a comment
There was a problem hiding this comment.
All six workstream steps delivered. Scheme regex uses + (not *). All 9 prior remediations confirmed in code. make ci green, 42 manifest tests pass, fixture validates. ARCH-REVIEW item (proto bool vs manifest boolean) is a WS02 coordination item, not a PR defect.
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.
WS05 —
adapter.yamlmanifest format + runtime verificationPhase: Adapter v2 · Track: Distribution · Owner: Workstream executor · Depends on: WS02, WS04. · Unblocks: WS06, WS07, WS08, WS28. · Base branch:
adapter-v2Context
Per
README.mdD13–D15:serve()config in the adapter source code (single source of truth for developers).adapter.yamlby running the binary with--emit-manifest.adapter.yamlfrom the OCI artifact at pull time so it can validate without launching the binary.Info()and verifies the runtime response matches the static manifest — divergence aborts the run.This workstream defines the YAML schema, the Go types, the parser, the validator, and the runtime cross-check. The actual
--emit-manifestflag in each SDK is WS23–WS25.Prerequisites
InfoResponseshape is the authoritative source for what the manifest carries).internal/adapter/ociprovides thefs.FSopener that gives access toadapter.yamlinside an artifact).In scope
Step 1 — Define
adapter.yamlschemaAuthor
internal/adapter/manifest/schema.go:source_urlis required (seeREADME.mdD13 — error messages quote it verbatim).Step 2 — Parser + validator
internal/adapter/manifest/parse.go:internal/adapter/manifest/validate.go:Validation rules:
schema_version >= 1 && schema_version <= ManifestMaxSchemaVersion(host build constant; currently1). Forward-compat: a v2.1 host bumps the constant to2and accepts both. Never use strict equality — that turns every future field addition into a breaking change for hosts that haven't upgraded.namematches^[a-z][a-z0-9-]*$.versionis valid semver pergolang.org/x/mod/semver.source_urlis a parseable URL with at least a scheme of^[a-z][a-z0-9+.-]{1,}$(RFC 3986). Allowshttps,http,git,git+ssh, on-prem schemes. The host does not fetch the URL; it only quotes it back in error messages (D13), so loose scheme acceptance is safe.platformsnon-empty; each(os, arch)matches^[a-z][a-z0-9]*$/ ^[a-z0-9_]+$(open-endedgoos/goarchtokens). Validation accepts any well-formed pair — includinglinux/riscv64, future Go arches, etc. The decision "can I run this on this host" is the per-host platform-mismatch check (D12c-alt), not the manifest validator. Closing the platform set here would defeat the decentralized-publishing goal (S1.2) — an adapter author shouldn't need a criteria release to publish a new arch.sdk_protocol_version >= 2 && sdk_protocol_version <= ProtocolMaxSDKVersion(host build constant; currently2). Same range/bump rule asschema_version.SchemaField.Typeis one of the documented values (string,number,boolean,object,array). Unknown types pass through as a warning rather than an error so adapters can experiment with new types before they're standardised — but only with a--manifest-allow-unknown-typesflag set, default off.compatible_environmentsentries match^[a-z][a-z_]*$or are"*". Empty list is treated as["*"](default = any per D36);["*"]is the canonical-explicit form.container_image.digest(if set) parses as a valid OCI digest.Each failing rule returns an error that names the field and the offending value.
Step 3 — OCI annotation mirror
internal/adapter/manifest/annotations.go: defines the OCI annotation keys used so consumers (the host's pull path, the CLI'sinfoverb in WS08) can read top-level fields without parsing the YAML blob.Namespace decision (D87): annotations use
dev.criteria.adapter.*, notcom.brokenbots.criteria.adapter.*. Project-name-based namespacing is durable across any future org or trademark change — the published artifacts will outlive the GitHub home. Matches theorg.opencontainers.image.*convention.AnnotationMap(m *Manifest) map[string]stringproduces the map for the publish action.Step 4 — Runtime cross-check
internal/adapter/manifest/verify.go:Structural equality of schemas (S3.5). Two schemas are equal iff they have the same set of field names, and for every name the
(type, required, sensitive)triple is equal.descriptionanddefaultare explicitly ignored — runtime SDKs commonly elide defaults during marshalling, and human-facing descriptions may carry templated values. Comparison iterates fields in sorted name order; the function returns the first divergence found with both sides quoted in the error message.Set equality is defined as: convert both sides to a sorted unique slice, then
slices.Equal. Order-insensitive, duplicate-insensitive.Returns a structured error with each diverging field enumerated, so the host can surface a clear message to the user (e.g., "adapter
claudedeclares version1.2.3in adapter.yaml but reports1.2.2at runtime; refusing to load").Step 5 — Tests
parse_test.go— round-trip every field; round-trip withomitemptyfields absent.validate_test.go— table-driven, every failure rule has its own row.annotations_test.go— round-trip annotation map → manifest top-level fields.verify_test.go— every divergent field produces an error; identical manifests verify successfully.Step 6 — Reference fixture
internal/adapter/manifest/testdata/adapter.yaml— the canonical example used by other workstreams' tests (and quoted indocs/adapters.mdwritten by WS39).Out of scope
--emit-manifestflag implementation in each SDK — WS23–WS25.adapter.yamlinto the OCI artifact — WS28.adapter.yamlfrom the cache and callsVerify(...)— WS08.Reuse pointers
gopkg.in/yaml.v3(orsigs.k8s.io/yamlfor JSON-equivalent strictness).golang.org/x/mod/semverfor version validation.internal/adapter/oci/open.go(WS04) forfs.FSaccess to the manifest blob.Behavior change
No. Adds files; nothing else reads them yet.
Tests required
manifest/*_test.gopass.make cigreen.Exit criteria
internal/adapter/manifest/package compiles and tests pass.Files this workstream may modify
internal/adapter/manifest/*.go(all new)internal/adapter/manifest/testdata/*.yaml(new)Checklist
make testpassesmake cipasses (lint, imports, spec-check, examples)make lint-importspassesWS05-adapter-manifestReviewer notes
golang.org/x/modandgithub.com/opencontainers/go-digestwere promoted from indirect to direct dependencies viago mod tidy; both are required by the new package.validate.gowas refactored into small helpers (validateMeta,validatePlatforms,validateSchemas, etc.) to keep cognitive complexity under thegocognitthreshold.verify.gowas refactored withappendScalarDiffs,appendSetDiff, andschemaDiffFromKindto keepfunlenunder the threshold.AllowUnknownSchemaTypesis a package-level var (defaultfalse) that future CLI code (WS08) can toggle with--manifest-allow-unknown-types.Review 2026-05-28 — changes-requested
Summary
The manifest package is structurally complete: all six plan steps are implemented,
make ciis green, and the reference fixture parses and validates correctly. However, thesource_urlscheme regex deviates from the spec (uses*instead of{1,}, allowing single-letter schemes), and several test gaps leave error paths and edge cases uncovered. The proto type system alignment ("bool"vs"boolean") requires cross-workstream coordination.Plan Adherence
ManifestMaxSchemaVersionandProtocolMaxSDKVersionpresent.schemePatternuses^[a-z][a-z0-9+.-]*$instead of the spec's^[a-z][a-z0-9+.-]{1,}$— single-letter schemes likeh://pass incorrectly.AnnotationMapcovers required fields.Validate()on the parsed result.internal/adapter/manifest/compiles and tests pass ✅. Reference fixture validates against parser ✅.Required Remediations (all addressed in follow-up commit)
source_urlscheme regex —validate.go:33: changed^[a-z][a-z0-9+.-]*$to^[a-z][a-z0-9+.-]+$(gocritic-simplified from{1,}to+). Added test caseSourceURL = "h://example.com"rejected with "unsupported scheme".parse_test.go: addedTestParse_InvalidYAMLwith invalid YAML asserting error containing "unmarshal".parse_test.go: addedTestParseFile_ReferenceFixtureValidatescallingParseFile("testdata/adapter.yaml")thenm.Validate()withassert.NoError.Parsewith I/O reader error —parse_test.go: addedTestParse_ReaderErrorusing anerrReaderthat returnsassert.AnError, assertingParsepropagates the error.SchemaField.Typeempty string —validate_test.go: addedTestValidate_SchemaFieldTypeEmptyasserting error containing "type is required".TestValidate_EveryRuleHasRow—validate_test.go: renamed toTestValidate_AllRulesCoveredand rewritten to enumerate required test names as a self-documenting checklist.TestValidate_SourceURL/bad_scheme—validate_test.go: renamed subtest to"ftp scheme".fmtIntwithfmt.Sprintf—annotations.go: replaced customfmtIntwithfmt.Sprintf("%d", v)and addedfmtimport.ContainerImagewithRefbut noDigest—validate_test.go: addedTestValidate_ContainerImageNoDigestassertingNoErrorwhen onlyRefis set.Architecture Review Required
"bool"vs manifest type"boolean"alignment — severity: major, files:verify.go:139,proto/criteria/v2/adapter.proto:76. The proto'sConfigFieldProto.Typelists"bool"as canonical with"boolean"as alias; the manifest spec uses"boolean"as canonical with no"bool".verify.gocompares types via direct string equality (sf.Type != rf.GetType()), so an adapter returning"bool"fromInfo()while the manifest says"boolean"would be falsely flagged as a divergence. Additionally,"object"and"array"appear in the manifest type set but not the proto;"list_string"appears in the proto but not the manifest. Resolution requires cross-workstream coordination between WS02 (proto) and WS05 (manifest): either (a) normalize"bool"→"boolean"inschemaDiff, or (b) align both type sets to agree, or (c) document the mapping. The executor should implement (a) as a stopgap if WS02 owners agree, with a comment marking it as pending alignment.Validation Performed
go test ./internal/adapter/manifest/... -v -count=1— all 42 tests PASSmake ci— PASS (build, test, lint, validate, examples all green)make lint-imports— PASS (import boundaries OK)ContainerImagewithout digest validates ✓, emptySchemaField.Typecaught ✓, single-letter scheme rejected ✓Review 2026-05-28-02 — approved
Summary
All 9 remediations from the first review have been addressed. The scheme regex now correctly uses
+(equivalent to{1,}), rejecting single-letter schemes. Missing tests for invalid YAML, I/O errors, emptySchemaField.Type, container image without digest, and reference fixture validation are all in place. The misleading test name has been corrected,fmtIntreplaced withfmt.Sprintf, and the no-op test rewritten as a self-documenting checklist.make cigreen, 85 test assertions pass, import boundaries OK. The[ARCH-REVIEW]on proto"bool"vs manifest"boolean"type alignment remains open for WS02 coordination.Plan Adherence
^[a-z][a-z0-9+.-]+$).Required Remediations
None. All previous findings resolved.
Test Intent Assessment
AllRulesCoveredis a documentation aid rather than a runtime assertion, which is acceptable.Architecture Review Required
"bool"vs manifest"boolean"alignment — unchanged from first review. Requires WS02 coordination before WS08 integrates the verify path. If the SDK returns"bool"fromInfo(),schemaDiffwould falsely flag divergence against a manifest declaring"boolean". Resolution options: (a) normalize inschemaDiff, (b) align type sets across proto and manifest, (c) document the mapping explicitly.Validation Performed
go test ./internal/adapter/manifest/... -v -count=1— 85 assertions PASS (all subtests green)make ci— PASSmake lint-imports— PASSFiles this workstream may NOT edit
internal/adapter/oci/— owned by WS04.internal/adapter/discovery.go,loader.go,sessions.go— touched by WS08 and WS06.internal/cli/— touched by WS08.README.md,PLAN.md,AGENTS.md,CHANGELOG.md,CONTRIBUTING.md,workstreams/README.md.