feat: add field descriptions, JSON Schema output, and review fixes

[hbraswelrh]

• This PR includes content assisted by an LLM (Claude Opus 4.6)
• This PR was reviewed by a human prior to submission

Summary

Extends the asyncapi-gen tool with field descriptions, standalone JSON Schema output, CI validation, versioning policy, and review-driven quality improvements.

Supersedes PR #8 — this PR includes PR #8's commits.

Changes

• Field descriptions: Add asyncapi-field struct tag for carrying field-level descriptions into generated AsyncAPI and JSON Schema output
• JSON Schema generation: Add standalone JSON Schema files (Draft 2020-12) to api/events/schemas/ for downstream consumer validation (envelope + data schemas)
• CI validation: Add ci_asyncapi.yml workflow validating the generated AsyncAPI spec via @asyncapi/cli on push and PR to main. Actions pinned by SHA, Node.js and Task versions pinned for reproducibility. The proposed arduino/setup-task action introduces an unnecessary dependency that expands to a single npx command. The action call is replaced by the npx command and can be adjusted if necessary based on discussion.
• Versioning policy: Add docs/versioning.md defining the four-layer event versioning strategy (NATS subject, CloudEvents type, AsyncAPI info.version, CloudEvents specversion) with decision rules for additive vs breaking changes
• Testable CLI pattern: Extract run(opts Options, stdout, stderr io.Writer) error from main() per AP-002/AP-003 conventions. Add main_test.go with 6 test cases covering all error paths (CRAP score reduced from 132 to testable).
• Complexity reduction: Refactor parseAsyncAPITag validation from switch-in-loop to struct-slice iteration (complexity ~10 vs 22)
• Test coverage improvements: Add table-driven tests for goTypeToJSONSchema (11 cases, all branches), buildServers edge cases (valid/malformed/empty URL), channelName, humanTitle, upperFirst helpers, malformed param error path, and error content assertions
• Resilience: Add os.MkdirAll to WriteYAML for fresh checkout safety, matching WriteJSONSchemas pattern
• README update: Fix source-of-truth direction — Go types are authoritative, spec is generated. Add Development section with generation workflow docs, "Adding a new event type" and "Evolving an event contract" sections.
• CLI pin: Pin @asyncapi/cli@2.16.1 in Taskfile (latest has broken npm dependency)

Related Issues

• Supersedes PR feat: automated AsyncAPI generation from Go structs #8
• Addresses complytime/nunya#430
• Closes https://github.com/complytime/nunya/issues/430?issue=complytime%7Cnunya%7C460
• Closes https://github.com/complytime/nunya/issues/430?issue=complytime%7Cnunya%7C461

Review Hints

• All 42 tests pass (go test ./...)
• golangci-lint run — 0 issues
• Generated files in api/events/schemas/ are deterministic — drift detection integration tests verify byte-identical output
• Review council (8 Divisor agents) reviewed full branch diff — all HIGH findings addressed
• Acceptance criteria for nunya#430 verified unaffected by review council refactors (generated artifacts unchanged)
• Commit 951376e updates the channel and data descriptions for readability based on the existing functionality in PR 8.

[hbraswelrh]

@jpower432 this PR depends on merge of PR #8. There were only a couple updates made so the diff between this PR and #8 is relatively small. The JSON Schemas for the envelope and the data are included in this PR (mentioned on Issue #430 here).

[trevor-vaughan-ai]

🔴 Review Council: REQUEST CHANGES

Automated LLM review, not a human sign-off. Findings are machine-generated, may contain errors, and are advisory input to human judgment.

Models used:

• claude-opus-5[1m]

Reviewed at commit 951376e (standard effort).

TL;DR: Solid, well-tested generator work; 25 verified issues remain, mostly silent generator failures, duplicated sources of truth, and shallow tests. Changes requested.

Findings: 🔴 0 Critical, 🟠 0 High, 🟡 17 Medium, 🔵 8 Low

Reviewer	Verdict	Findings
🛡️ Adversary (code)	✅ Approve	2 MEDIUM
📚 Curator (code)	✅ Approve	3 MEDIUM, 2 LOW
🧭 Guard (code)	✅ Approve	4 MEDIUM, 2 LOW
⚙️ Operator (code)	✅ Approve	3 MEDIUM, 1 LOW
🧪 Tester (code)	❌ Changes	5 MEDIUM, 3 LOW

🟡 MEDIUM (17)

• 🛡️ asyncapi tag parser silently drops comma-split fragments and lets them rebind other keys (cmd/asyncapi-gen/parser.go:200)

  		idx := strings.IndexByte(pair, ':')
  		if idx < 0 {
  			continue
  		}
  

  💡 Recommendation: Return an error from parseAsyncAPITag for any comma-split segment that contains no ':' (and for any duplicate binding of a non-param key) rather than continue-ing, so a malformed tag fails generation instead of silently emitting a wrong contract. Alternatively adopt a value format that can carry commas (e.g. quoted values or a per-key struct tag).
  Constraint: reviewer-protocol.md Engineering Discipline - no silenced errors; malformed input at a parse boundary must be rejected, not discarded (CWE-20).

  💬 Full reviewer analysis

  parseAsyncAPITag splits the entire asyncapi tag on ',' and then discards any resulting segment that contains no ':' via a bare continue, with no error and no diagnostic. Two consequences follow. First, a comma inside any human-readable value (send:, receive:, description:) silently truncates that value at the comma and drops the remainder from the published contract. Second - and worse - if the dropped remainder happens to contain a colon and a recognised key, it is not dropped but silently rebinds that key: a tag such as send:Accepted, type:wrong sets CEType to "wrong", so the generated type const in api/events/asyncapi.yaml and api/events/schemas/EvidenceIngestedCloudEvent.schema.json would disagree with TypeEvidenceIngested in events/events.go with no error raised. The only validation is the trailing "missing required keys" check (parser.go:238-246), which passes because every key is non-empty. The drift integration test cannot catch this because it regenerates from the same corrupted tag. That comma-bearing descriptions are realistic here is visible in the diff: the previous hand-written channel description in api/events/asyncapi.yaml was "Published when evidence is ingested, before sealing." - a string this tag format cannot represent. The doc comment at parser.go:190 acknowledges "Values may contain spaces but not commas" but nothing enforces it.

• 🛡️ Unmapped Go types silently become JSON Schema "object", including slices the parser explicitly emits (cmd/asyncapi-gen/schema.go:258)

  func goTypeToJSONSchema(goType string) string {
  	base := strings.TrimPrefix(goType, "*")
  	switch base {
  	case "string":
  		return "string"
  	case "int", "int32", "int64":
  		return "integer"
  	case "float32", "float64":
  		return "number"
  	case "bool":
  		return "boolean"
  	default:
  		return "object"
  	}
  }
  

  💡 Recommendation: Handle the [] prefix explicitly by emitting {"type": "array", "items": {"type": <element type>}}, and return an error (propagated through BuildDoc/BuildDataJSONSchema) for any Go type the generator cannot represent, so an unsupported field fails go generate instead of producing a silently incorrect published schema.

  💬 Full reviewer analysis

  The default branch converts every type it does not recognise into "object" with no error. This is not only a theoretical gap: fieldGoType at cmd/asyncapi-gen/parser.go:166 deliberately produces "[]" + fieldGoType(t.Elt) for slice fields, and goTypeToJSONSchema has no case matching a [] prefix, so a field declared Tags []string yields "type": "object" in both api/events/asyncapi.yaml and the published api/events/schemas/*.schema.json. The same applies to qualified types such as time.Time, which fieldGoType maps to "interface{}" (parser.go:168-169) and which then also becomes "object". Because these schemas are published for downstream consumer validation (README.md "Development" section and PR body: "standalone JSON Schema files (Draft 2020-12) ... for downstream consumer validation"), a consumer enforcing them would reject structurally valid events. No current event struct has a slice field, so nothing is broken today, but README.md documents "Adding a new event type" as the expected workflow and the generator will emit a wrong contract on the first such field rather than failing. Note schema_test.go asserts {"SomeStruct", "object"}, so the fallback is intentional for nested structs - the defect is that it silently swallows slices and qualified types too. Grepped cmd/asyncapi-gen/schema.go for [] handling in the mapper: the only [] occurrences are Go slice declarations (lines 107, 216, 233, 291), none is an array branch.

• 📚 Breaking-change procedure omits the asyncapi struct tag, so the published contract keeps advertising the old type (docs/versioning.md:102)

  1. Bump the CloudEvents `type` with a version suffix
     (`dev.complytime.evidence.ingested` → `dev.complytime.evidence.ingested.v2`).
     Update the `Type…` constant in [`events/events.go`](../events/events.go).
  

  💡 Recommendation: Amend step 1 of the Breaking section to require updating both the Type… constant and the type: key inside the asyncapi sentinel tag on the corresponding *Data struct, and state that the generated const in api/events/asyncapi.yaml and api/events/schemas/*.schema.json comes from the tag, not the constant. No Docs repo is configured for this council run (no Review Council Configuration block in the invoking repository), so no tracking issue is proposed - fix the doc in place.
  Constraint: Documentation gap detection - a documented procedure for a user-facing contract change must name every artifact the change requires.

  💬 Full reviewer analysis

  This PR makes events/events.go the source of truth and adds docs/versioning.md as the authoritative procedure for evolving a contract. The CloudEvents type now lives in two places in that file: the exported constant at line 18 (const TypeEvidenceIngested = "dev.complytime.evidence.ingested", consumed by e.SetType at line 45) and the type: key inside the sentinel asyncapi struct tag at line 24, which is what parseAsyncAPITag reads into EventSpec.CEType and what buildEnvelopeSchema emits as the const for the type property in the generated spec and JSON Schemas. The breaking-change step quoted above names only the constant. I grepped the repo for both occurrences (grep -rn "TypeEvidenceIngested\|type:dev.complytime" --include='*.go' --include='*.md' .): the struct-tag copy is referenced nowhere in docs/versioning.md or README.md. A maintainer who follows the documented steps exactly bumps only the constant, runs go generate, and ships a spec whose type const still says the v1 value while the producer emits v2. TestIntegration_GeneratedMatchesCommitted regenerates from the same struct tag, so it stays green and the mismatch reaches consumers.

• 📚 Version-bump procedure omits the version hardcoded in the drift-detection test (docs/versioning.md:90)

  1. Bump `info.version` minor (`0.1.0` → `0.2.0`) in the `//go:generate`
     directive.
  2. Leave `type`, `specversion`, and the subject unchanged.
  3. Run `go generate ./events/...` and commit the regenerated artifacts.
  

  💡 Recommendation: Either add a step to both procedures telling the maintainer to update the title/version/description arguments in cmd/asyncapi-gen/integration_test.go, or remove the duplication by having the test shell out to go generate (or read the flags from the //go:generate directive) so the doc's three steps stay complete.
  Constraint: Documentation gap detection - the documented workflow must cover every file a change touches.

  💬 Full reviewer analysis

  Both the additive procedure (lines 90-93) and the breaking procedure (line 105, Bump info.version major) tell the maintainer to change the version only in the //go:generate directive. I grepped the repo for the current version string (grep -rn "0\.1\.0" --include='*.go' --include='*.md' --include='*.yaml' --include='*.yml' .): besides events/events.go:7 and the generated api/events/asyncapi.yaml:5, it is also hardcoded at cmd/asyncapi-gen/integration_test.go:26 - doc := BuildDoc(specs, "ComplyTime API Events", "0.1.0", - together with a verbatim copy of the -description text on line 27. Following the documented steps therefore leaves the drift test comparing a doc built at 0.1.0 against a committed file at 0.2.0, and the failure message it prints (Run + backtick + go generate ./events/... + backtick + to update it.) instructs the maintainer to redo the step they already did. The same trap applies to any future edit of the description text.

• 📚 "Adding a new event type" omits the asyncapi tag's grammar, its description key, and its no-comma constraint (README.md:76)

  1. Define a new `*Data` struct in `events/events.go` with a sentinel blank
     field carrying the `asyncapi` tag (channel, params, stream, type, send,
     receive metadata).
  2. Add `asyncapi-field:"description:..."` tags on each struct field for
     schema descriptions.
  3. Run `task generate` to regenerate all derived artifacts.
  

  💡 Recommendation: Extend step 1 with the full key list (channel, param:<name>=<desc>, stream, type, send, receive, description), state that values must not contain commas, and show one filled-in sentinel-field example rather than describing it.
  Constraint: Documentation gap detection - a change that introduces a new authoring convention must document that convention completely.

  💬 Full reviewer analysis

  This PR introduces a struct-tag authoring convention and this README section is the only prose that teaches it. Two properties of the format that parseAsyncAPITag enforces are absent from it. First, the description key (channel description, es.ChannelDescription) is not listed among the recognised keys, even though events/events.go:24 uses it and it is what produces the channel description in the generated spec; I searched README.md and docs/versioning.md for description: used as an asyncapi tag key and found only the separate asyncapi-field form. Second, parseAsyncAPITag splits the tag on , and silently continues on any segment lacking a :, so a value containing a comma is truncated with no error - a maintainer writing send:Published when a widget is created, and stored gets Published when a widget is created in the published contract and no diagnostic. The section also gives no worked example of the tag, so the only reference is events/events.go itself.

• 🧭 Generated spec silently drops the field examples the hand-written contract carried (api/events/asyncapi.yaml:105)

                  contentDigest:
                      type: string
                      description: SHA-256 digest of the evidence artifact
  

  💡 Recommendation: Add an example: key to the asyncapi-field tag vocabulary (parallel to description:) and emit it as examples in both buildDataSchema and BuildDataJSONSchema, restoring the four example values the previous spec published. If examples are deliberately being retired, say so in the PR description so downstream consumers of the contract are not surprised.
  Constraint: Cross-Component

  💬 Full reviewer analysis

  The pre-change asyncapi.yaml carried examples: values on four schema properties (contentDigest: sha256:abc123..., artifactType: application/vnd.gemara.evaluation-log+json, subjectId: my-app-v1, and source: complytime-gateway). Regenerating the spec from Go types drops all of them, and the generator has no way to express them: grep -rni "example" cmd/asyncapi-gen/ events/ returns only the dev.example.widget.created string inside cmd/asyncapi-gen/testdata/fixture.go, and grep -rn "examples" api/ returns nothing. The struct tag vocabulary defined in parseAsyncAPITag (cmd/asyncapi-gen/parser.go) and extractFieldDescription supports only description:, so this is a permanent capability gap, not a one-time omission. The PR description enumerates "Field descriptions" as an addition and "Generated files ... are deterministic" but nowhere discloses that example values present in the published contract are removed; the closest passage, "Commit 951376e updates the channel and data descriptions for readability", does not cover deletion of examples.

• 🧭 Published contract changed shape but info.version stays 0.1.0, contradicting the versioning policy added in this PR (api/events/asyncapi.yaml:5)

      title: ComplyTime API Events
      version: 0.1.0
  

  💡 Recommendation: Bump -version in the //go:generate directive in events/events.go and regenerate, so the first spec produced under the new pipeline carries a version distinct from the hand-written 0.1.0 - or state explicitly in docs/versioning.md that pre-1.0 contracts may change in place, so the policy and the artifact agree.
  Constraint: Intent Drift

  💬 Full reviewer analysis

  docs/versioning.md, added by this changeset, states at line 28 that info.version versions "The whole published contract" for "Humans, codegen tooling", and at lines 70-74 that "Consumers integrate from it and pin codegen to info.version". This changeset materially changes that published contract while leaving info.version: 0.1.0 unchanged: the message component key was renamed from evidenceIngested to EvidenceIngested (so #/components/messages/evidenceIngested no longer resolves), a servers: block and a NATS bindings block were added, the channel description was rewritten, and property examples were removed. Under the document's own decision rule (line 99: "Removing or renaming a field ... Bump info.version major"), a consumer that pinned codegen to 0.1.0 now gets a different document under the same version string. Mitigating: git tag in the clone returns no tags, so 0.1.0 appears never to have been released, which is why this is MEDIUM rather than a live contract break.

• 🧭 Drift test hardcodes doc metadata, so the documented version-bump procedure breaks CI with a misleading message (cmd/asyncapi-gen/integration_test.go:26)

  	doc := BuildDoc(specs, "ComplyTime API Events", "0.1.0",
  

  💡 Recommendation: Have the test drive the real generator (e.g. go generate ./events/... into a temp output, or parse the flags out of the //go:generate line) instead of restating the metadata, so document metadata has one authoring location.
  Constraint: Structural Coherence

  💬 Full reviewer analysis

  The single source of truth for document metadata is the //go:generate directive at events/events.go:7 (-title, -version, -description, -license, -contact-name, -contact-url, -server). This test re-specifies all seven values as literals rather than invoking the generator, so the pair must be edited in lockstep. grep -rn "0\.1\.0" over the repo confirms the version literal lives in exactly two authoring locations - events/events.go:7 and this line - plus the generated artifact. docs/versioning.md:90 instructs a maintainer to "Bump info.version minor (0.1.0 -> 0.2.0) in the //go:generate directive" and then "Run go generate ./events/... and commit the regenerated artifacts"; following that procedure exactly leaves this test comparing a locally built 0.1.0 document against a committed 0.2.0 one, and the failure message it prints - "Run go generate ./events/... to update it" - points at a step that was already performed and will not fix the failure. The drift detector then reports drift that does not exist.

• 🧭 CloudEvents envelope definition duplicated between the YAML and JSON Schema generators (cmd/asyncapi-gen/jsonschema.go:72)

  		"required":    []string{"specversion", "id", "type", "source", "subject", "time", "datacontenttype", "data"},
  

  💡 Recommendation: Derive both envelopes from one declaration - for example a shared envelopeFields() returning name/type/const/format/description tuples that schema.go renders into Schema and jsonschema.go renders into JSONSchema - so the contract is stated once.
  Constraint: Structural Coherence

  💬 Full reviewer analysis

  BuildEnvelopeJSONSchema (jsonschema.go:52-75) and buildEnvelopeSchema (schema.go:212-228) independently encode the same envelope contract: the identical eight-entry required list, the same eight properties, the same const values ("1.0", "application/json"), and the same literal descriptions ("URI identifying the producing service", "The compliance subject identifier"). The two outputs are published side by side as one contract (api/events/asyncapi.yaml and api/events/schemas/), so adding a CloudEvents attribute such as dataschema or making subject optional requires editing both. Nothing enforces agreement: the integration tests compare each generator against its own committed output, not against each other, so the YAML and JSON Schema envelopes can drift apart while all tests stay green. This is business logic duplicated across modules, not incidental similarity.

• ⚙️ Generated schema directory is never pruned and orphaned files are never detected (cmd/asyncapi-gen/jsonschema.go:81)

  if err := os.MkdirAll(dir, 0o755); err != nil {
  

  💡 Recommendation: Have WriteJSONSchemas remove pre-existing *.schema.json files in the target directory before writing (or write to a temp dir and swap), and extend TestIntegration_JSONSchemasMatchCommitted to read the committed directory listing and fail on any file not produced by the current specs. Consider adding a schema-validation step covering api/events/schemas/ alongside the existing asyncapi validation.
  Constraint: Generated asset synchronization - checked-in generated files consistent with source definitions

  💬 Full reviewer analysis

  WriteJSONSchemas creates the output directory and writes one data schema plus one envelope schema per spec (loop at line 85), but never removes files that no longer correspond to any spec. The paired drift test, TestIntegration_JSONSchemasMatchCommitted, iterates for _, spec := range specs and then for _, name := range files (integration_test.go:74), checking only the filenames it expects to exist - it never lists the committed directory to find extras. I verified api/events/schemas/ currently holds exactly the two expected files. Concretely: rename EvidenceIngestedData to EvidenceAcceptedData in events/events.go and run go generate; the two new schema files appear, EvidenceIngestedData.schema.json and EvidenceIngestedCloudEvent.schema.json remain on disk and get committed, and CI stays green while consumers see a stale published schema for an event that no longer exists. Separately, nothing in the pipeline validates these JSON Schema files at all - task asyncapi-lint and .github/workflows/ci_asyncapi.yml:30 both target only api/events/asyncapi.yaml - so the relative $ref from the envelope schema to the data schema is never resolution-checked either.

• ⚙️ Published event contract hardcodes localhost as the NATS server (events/events.go:7)

  -server nats://localhost:4222
  

  💡 Recommendation: Either point -server at the real ecosystem broker address, or keep the local address and populate the server Description (e.g. 'Local development broker; override per environment') so consumers and codegen users are not silently handed a loopback endpoint. Wiring the value through an environment variable or a checked-in config read by the directive would also let non-dev environments regenerate without editing source.
  Constraint: Hardcoded paths, hostnames, or environment-specific values that should be parameterized

  💬 Full reviewer analysis

  The //go:generate directive passes a developer-workstation address as the server URL, and buildServers in cmd/asyncapi-gen/schema.go turns it into servers:\n nats:\n host: localhost:4222\n protocol: nats in the committed api/events/asyncapi.yaml (I read the generated block). README.md line 8 presents that YAML as the contract downstream consumers integrate against, and docs/versioning.md calls it 'the public, machine-readable contract' that 'Consumers integrate from'. AsyncAPI codegen tools read the servers block to seed client connection defaults, so any consumer generating a client from the published spec gets localhost:4222. The generator's Server struct (schema.go) carries a Description field that BuildDoc never populates, so the emitted entry has no text marking it as a local-development placeholder either.

• ⚙️ Required PR check resolves an unpinned npm dependency tree at run time (.github/workflows/ci_asyncapi.yml:30)

  run: npx --yes @asyncapi/cli@2.16.1 validate api/events/asyncapi.yaml
  

  💡 Recommendation: Commit a package.json plus package-lock.json declaring @asyncapi/cli@2.16.1 and run npm ci && npx asyncapi validate api/events/asyncapi.yaml, which makes the tool tree byte-reproducible and lets Dependabot/Renovate track it. Add timeout-minutes to the job so a registry hang fails fast rather than consuming the default 6-hour budget.
  Constraint: Release pipeline integrity - all dependencies pinned to specific versions; builds reproducible from same inputs

  💬 Full reviewer analysis

  The workflow pins the CLI itself to 2.16.1 but there is no package.json or package-lock.json anywhere in the repo (I listed the repo root and confirmed only commitlint.config.js exists as npm-adjacent config), so npx resolves @asyncapi/cli's entire transitive tree fresh from the npm registry on every run. That is exactly the failure class this project has already hit: Taskfile.yml:28 records the reason for the pin as 'latest has broken npm dependency (@asyncapi/studio-ui@0.5.0 404)'. Pinning the top-level version does not prevent a recurrence one level down, because published packages carry semver ranges rather than exact dependency versions. The job runs on every push and PR to main with no timeout-minutes bound, so an upstream unpublish or registry incident turns every PR red - and blocks merges - with no change to this repository. The Taskfile's asyncapi-lint task has the identical exposure.

• 🧪 TestRun_WriteError depends on the process lacking write access to / and writes outside the test sandbox when it has it (cmd/asyncapi-gen/main_test.go:129)

  		Input:   "testdata/fixture.go",
  		Output:  "/nonexistent/deeply/nested/dir/out.yaml",
  

  💡 Recommendation: Produce the write error inside the sandbox: create a file (not a directory) inside t.TempDir() and use filepath.Join(thatFile, "out.yaml") as the output path, which fails with ENOTDIR regardless of privilege level and leaves nothing outside the temp dir.
  Constraint: Test isolation: tests must not depend on ambient filesystem permissions or write outside t.TempDir()

  💬 Full reviewer analysis

  The test asserts an error by pointing the output at an absolute path under /. WriteYAML (cmd/asyncapi-gen/writer.go:24) now calls os.MkdirAll(filepath.Dir(path), 0o755) before writing, so when the test process can write to / - root in a container, the common local dev setup for this repo - MkdirAll succeeds, WriteFile succeeds, run returns nil, and the test fails at t.Fatal("expected error for invalid output path") while having created /nonexistent/deeply/nested/dir/ on the host and left it behind. Every other test in this package correctly confines itself to t.TempDir(). The failure is environment-dependent, not code-dependent, so it will read as a mystery break to anyone not running as an unprivileged user.

• 🧪 The schema write error branch of run() has no test, contradicting the PR's "all error paths" claim (cmd/asyncapi-gen/main.go:76)

  	if opts.SchemasDir != "" {
  		if err := WriteJSONSchemas(specs, opts.SchemasDir); err != nil {
  			return fmt.Errorf("schema write error: %w", err)
  		}
  

  💡 Recommendation: Add a TestRun_SchemaWriteError case that sets SchemasDir to a path under a regular file inside t.TempDir() (so os.MkdirAll in WriteJSONSchemas fails deterministically) and asserts the error mentions schema write error.
  Constraint: Coverage completeness: every error return in a changed function needs a test (lang-go testing_conventions)

  💬 Full reviewer analysis

  The PR description states "Add main_test.go with 6 test cases covering all error paths". Three of run's four error returns are covered (required flags missing, parse error, write error); this one is not. I ran grep -rn "schema write error" --include='*.go' . - the string occurs only at cmd/asyncapi-gen/main.go:76 and in no test file. The branch is reachable whenever -schemas-dir points somewhere unwritable, which is precisely the case the branch exists for, and it is the only error path whose wrapping message could regress unnoticed.

• 🧪 CLI happy-path tests assert only stdout substrings and never check that any artifact was produced (cmd/asyncapi-gen/main_test.go:116)

  	output := stdout.String()
  	if !strings.Contains(output, "wrote JSON schemas") {
  		t.Errorf("stdout = %q, want mention of wrote JSON schemas", output)
  	}
  	if !strings.Contains(output, "wrote") {
  		t.Errorf("stdout = %q, want mention of wrote", output)
  	}
  

  💡 Recommendation: After run returns, stat/read dir + "/asyncapi.yaml" and assert it is non-empty and contains the fixture channel address; in the schemas test assert both WidgetCreatedData.schema.json and WidgetCreatedCloudEvent.schema.json exist under SchemasDir. Replace the redundant "wrote" check with an assertion on the distinct "wrote <path> (1 event(s))" line.
  Constraint: Assertion quality: assertions must verify the produced result, not only a log line (severity.md: shallow assertions)

  💬 Full reviewer analysis

  TestRun_HappyPath (line 82) and TestRun_HappyPathWithSchemas (line 101) are the only tests of the whole CLI success path, and neither reads opts.Output or opts.SchemasDir back off disk. If run printed its progress lines but wrote an empty file, wrote to the wrong path, or emitted zero schema files, both tests would still pass. The second assertion quoted here is additionally tautological: "wrote JSON schemas" contains "wrote", so once the first check passes the second can never fail - it adds no signal. stderr is captured in both tests and never asserted, matching the fact that run never writes to the stderr writer it accepts.

• 🧪 Slice-typed event fields are an implemented parser branch with no test in either the parser or the type-mapping table (cmd/asyncapi-gen/parser.go:166)

  	case *ast.ArrayType:
  		return "[]" + fieldGoType(t.Elt)
  

  💡 Recommendation: Add a slice field and an untagged / json:"-" field to testdata/fixture.go with parser assertions for each, and add {"[]string", ...} and {"interface{}", ...} rows to the goTypeToJSONSchema table once the intended mapping (array) is decided.
  Constraint: Coverage completeness: implemented branches of core generator logic require tests

  💬 Full reviewer analysis

  fieldGoType explicitly handles slice fields, producing e.g. "[]string", which goTypeToJSONSchema (cmd/asyncapi-gen/schema.go:258) then falls through to default and maps to "object" - a wrong JSON Schema type for an array. Nothing exercises it: cmd/asyncapi-gen/testdata/fixture.go declares only string and *string fields, and the TestGoTypeToJSONSchema table (cmd/asyncapi-gen/schema_test.go:236-252) has no slice row despite the PR describing it as "11 cases, all branches" - grep -rn '\[\]string\|ArrayType\|interface{}' cmd/asyncapi-gen/*_test.go returns no matches. The same gap covers the default: return "interface{}" branch (parser.go:168) and the field-skipping branches at parser.go:121 (untagged field) and parser.go:127 (json:"-"), none of which appear in any fixture or temp-file test. The first event type that adds a slice field will silently ship type: object to downstream consumers with a green test suite.

• 🧪 No test validates a real event payload against the shipped JSON Schemas (cmd/asyncapi-gen/jsonschema_test.go:131)

  	if dataSchema["$schema"] != "https://json-schema.org/draft/2020-12/schema" {
  		t.Error("data schema missing $schema")
  	}
  

  💡 Recommendation: Add a contract test in the events package that builds an event via NewEvidenceIngestedEvent, marshals it, and validates the result against api/events/schemas/EvidenceIngestedCloudEvent.schema.json (a Draft 2020-12 validator such as santhosh-tekuri/jsonschema, or at minimum an assertion that every required name in the schema is present in the marshaled JSON and that no schema property is absent from the struct).
  Constraint: Missing contract test for a published API contract (severity.md, Tester/MEDIUM)

  💬 Full reviewer analysis

  api/events/schemas/*.schema.json are described in the PR as "standalone JSON Schema files (Draft 2020-12) ... for downstream consumer validation", yet no test anywhere asserts that a message produced by events.NewEvidenceIngestedEvent satisfies them. The written-file test quoted here checks only that the $schema key round-trips; the integration test compares bytes of generator output against generator output; events/events_test.go marshals the struct but never sees the schemas. I ran grep -rn 'schema.json\|jsonschema\|Validate' --include='*.go' . - the only references to the schema files are the generator and the byte-comparison drift test. The result is that the envelope schema's required list (specversion, id, type, source, subject, time, datacontenttype, data) and the data schema's required set are never checked against what the library actually emits, so a divergence - e.g. a required field the constructor does not set, or shardId typed string while the Go field is *string and may serialize null - would ship uncaught. CI validates only the YAML (.github/workflows/ci_asyncapi.yml:30 runs validate api/events/asyncapi.yaml); the JSON Schemas are validated by nothing.

🔵 LOW (8)

• 📚 task generate description names only asyncapi.yaml but the target also writes the JSON Schemas (Taskfile.yml:21)

    generate:
      desc: Regenerate derived artifacts (asyncapi.yaml)
  

  💡 Recommendation: Change the description to something like Regenerate derived artifacts (asyncapi.yaml and JSON Schemas) so task --list matches the README.
  Constraint: Cross-reference consistency - task descriptions are user-facing help text and must match what the task does.

  💬 Full reviewer analysis

  desc is what task --list prints, so it is user-facing documentation. The //go:generate directive this target runs passes -schemas-dir ../api/events/schemas, and README.md lines 59-60 correctly say the command "rebuilds api/events/asyncapi.yaml and the JSON Schema files in api/events/schemas/". The task description added in this changeset lists only asyncapi.yaml, so the CLI help understates the target's effect relative to the README written in the same PR.

• 📚 Generated JSON Schemas carry no do-not-edit provenance marker (api/events/schemas/EvidenceIngestedData.schema.json:2)

    "$id": "EvidenceIngestedData.schema.json",
    "$schema": "https://json-schema.org/draft/2020-12/schema",
  

  💡 Recommendation: Add a $comment key in BuildDataJSONSchema and BuildEnvelopeJSONSchema (e.g. "Generated by cmd/asyncapi-gen from events/events.go - do not edit manually; run 'go generate ./events/...'") and regenerate, so every generated artifact states its provenance.
  Constraint: Documentation convention compliance - generated artifacts in this repo announce that they are generated.

  💬 Full reviewer analysis

  The other artifact this generator emits, api/events/asyncapi.yaml, is self-describing: its info.description (line 9 of that file) ends with "Do not edit manually - run 'go generate ./events/...' to regenerate.", and it carries the SPDX header prepended by WriteYAML. Both new schema files under api/events/schemas/ are emitted by writeJSON with no equivalent marker; I read both files and grepped them for generate, generated and SPDX with no hits. The warning exists only in README.md line 10, which a downstream consumer or a maintainer opening the schema file directly will not see. JSON Schema defines $comment for exactly this purpose, so the marker can be added without breaking validators.

• 🧭 Envelope schema name computed inline instead of via the existing envelopeSchemaName helper (cmd/asyncapi-gen/schema.go:143)

  		envSchemaKey := strings.TrimSuffix(spec.StructName, "Data") + "CloudEvent"
  

  💡 Recommendation: Replace the inline expression with envSchemaKey := envelopeSchemaName(spec.StructName).
  Constraint: Zero-Waste

  💬 Full reviewer analysis

  envelopeSchemaName (cmd/asyncapi-gen/jsonschema.go:102-104) already performs exactly this derivation via messageKey(structName) + "CloudEvent", and it is used in jsonschema.go:91 and integration_test.go:72. Both files are in package main, so the helper is directly callable here. The inline copy means the naming rule for envelope schemas exists in two places, and the AsyncAPI component key and the JSON Schema filename could diverge if only one is changed.

• 🧭 Channel description loses the lifecycle detail "before sealing" (api/events/asyncapi.yaml:25)

          description: Evidence ingestion pipeline for compliance artifacts
  

  💡 Recommendation: Restore the ordering detail in the description: value of the asyncapi struct tag in events/events.go and regenerate, e.g. "Evidence ingestion pipeline for compliance artifacts; published before sealing".
  Constraint: Cross-Component

  💬 Full reviewer analysis

  The previous channel description read "Published when evidence is ingested, before sealing." The replacement text describes what the channel is rather than when the event fires, dropping the "before sealing" ordering guarantee - the one fact in that sentence a consumer could act on when deciding whether the referenced artifact is final. The PR description covers this only as "Commit 951376e updates the channel and data descriptions for readability based on the existing functionality in PR 8", which characterises the edit as stylistic; removing a stated lifecycle ordering is a semantic change to the published contract, not a readability one.

• ⚙️ New Development section omits the Node.js and network prerequisites it introduces (README.md:62)

  To validate the generated AsyncAPI spec:
  
  ```bash
  task asyncapi-lint
  ```
  

  💡 Recommendation: Add a one-line prerequisites note to the Development section listing Go (per go.mod), Task, golangci-lint, and Node.js (for the AsyncAPI CLI), and mention that asyncapi-lint requires network access on first run.
  Constraint: Operational documentation - environment prerequisites explicit

  💬 Full reviewer analysis

  The Development section added by this change documents task generate, task asyncapi-lint and task check but never states what a contributor needs installed to run them. task asyncapi-lint requires Task, Node.js with npx, and outbound network access to the npm registry (Taskfile.yml:29), and Taskfile.yml:37 folds asyncapi-lint into check - so task check, which README.md line 68 presents as the run-everything entry point, now silently fails on any machine without Node or without registry access, where it previously needed only Go and golangci-lint. I searched the repo for a CONTRIBUTING file and found none, so the README is the only place a contributor would learn this.

• 🧪 Two-level test helper wrapper around os.WriteFile with a single caller (cmd/asyncapi-gen/main_test.go:143)

  func writeTestFile(path, content string) error {
  	return writeFileForTest(path, []byte(content))
  }
  

  💡 Recommendation: Inline os.WriteFile(input, []byte(content), 0o644) at the single call site, matching parser_test.go, and drop both helpers.
  Constraint: Test architecture: assertions and setup should be direct, not hidden behind abstraction layers

  💬 Full reviewer analysis

  writeTestFile has exactly one caller (line 61) and does nothing but convert a string to bytes and delegate to writeFileForTest, which in turn does nothing but call os.WriteFile. Two indirection layers for one call site obscure where the fixture is written and where the //nolint:gosec suppression applies. parser_test.go (lines 115, 134) writes its temp fixtures with a direct os.WriteFile call, so this package already has an inconsistent pattern for the same operation.

• 🧪 Comment in TestChannelName table describes the wrong row (cmd/asyncapi-gen/schema_test.go:314)

  		{"Data", "Data"},
  		// edge: name becomes empty after trim, returns original
  		{"SimpleData", "simple"},
  

  💡 Recommendation: Move the comment directly above the {"Data", "Data"} row.
  Constraint: Test clarity: comments must describe the case they annotate

  💬 Full reviewer analysis

  The comment sits above {"SimpleData", "simple"} but describes the preceding row: channelName("Data") trims the Data suffix to the empty string, hits the if len(name) == 0 guard at cmd/asyncapi-gen/schema.go:277 and returns the original - that is the edge case. SimpleData trims to Simple and lowercases normally; nothing about it is empty-after-trim. A future reader deleting the "redundant-looking" {"Data", "Data"} row would remove the only coverage of that guard while believing the comment says it is still covered.

• 🧪 humanTitle table omits the trim-to-empty case its sibling channelName table covers (cmd/asyncapi-gen/schema_test.go:331)

  		{"EvidenceIngestedData", "Evidence Ingested"},
  		{"WidgetCreatedData", "Widget Created"},
  		{"SimpleData", "Simple"},
  

  💡 Recommendation: Add a {"Data", ...} row to the humanTitle table asserting the intended output, matching the boundary row already present in TestChannelName.
  Constraint: Coverage consistency: parallel helpers should be exercised on the same boundary inputs

  💬 Full reviewer analysis

  channelName and humanTitle (cmd/asyncapi-gen/schema.go:275 and :289) both begin with strings.TrimSuffix(structName, "Data"), but only channelName guards the empty result and only its table (line 313) tests the "Data" input. humanTitle("Data") reaches the loop with an empty name and returns "", which would surface as an empty title: on the generated AsyncAPI message - untested and unasserted either way, so the intended behaviour for that input is undefined by the suite.

---

Produced by Review Council, an open-source multi-persona code reviewer. Spot a wrong call or want the source? File feedback or browse the repository.

[hbraswelrh]

🔴 Review Council: REQUEST CHANGES

Automated LLM review, not a human sign-off. Findings are machine-generated, may contain errors, and are advisory input to human judgment.
Models used:

• claude-opus-5[1m]

Reviewed at commit 951376e (standard effort).

TL;DR: Solid, well-tested generator work; 25 verified issues remain, mostly silent generator failures, duplicated sources of truth, and shallow tests. Changes requested.

Findings: 🔴 0 Critical, 🟠 0 High, 🟡 17 Medium, 🔵 8 Low

Reviewer Verdict Findings
🛡️ Adversary (code) ✅ Approve 2 MEDIUM
📚 Curator (code) ✅ Approve 3 MEDIUM, 2 LOW
🧭 Guard (code) ✅ Approve 4 MEDIUM, 2 LOW
⚙️ Operator (code) ✅ Approve 3 MEDIUM, 1 LOW
🧪 Tester (code) ❌ Changes 5 MEDIUM, 3 LOW
🟡 MEDIUM (17)

• 🛡️ asyncapi tag parser silently drops comma-split fragments and lets them rebind other keys (cmd/asyncapi-gen/parser.go:200)

  		idx := strings.IndexByte(pair, ':')
  		if idx < 0 {
  			continue
  		}
  

  💡 Recommendation: Return an error from parseAsyncAPITag for any comma-split segment that contains no ':' (and for any duplicate binding of a non-param key) rather than continue-ing, so a malformed tag fails generation instead of silently emitting a wrong contract. Alternatively adopt a value format that can carry commas (e.g. quoted values or a per-key struct tag).
  Constraint: reviewer-protocol.md Engineering Discipline - no silenced errors; malformed input at a parse boundary must be rejected, not discarded (CWE-20).
  💬 Full reviewer analysis
  parseAsyncAPITag splits the entire asyncapi tag on ',' and then discards any resulting segment that contains no ':' via a bare continue, with no error and no diagnostic. Two consequences follow. First, a comma inside any human-readable value (send:, receive:, description:) silently truncates that value at the comma and drops the remainder from the published contract. Second - and worse - if the dropped remainder happens to contain a colon and a recognised key, it is not dropped but silently rebinds that key: a tag such as send:Accepted, type:wrong sets CEType to "wrong", so the generated type const in api/events/asyncapi.yaml and api/events/schemas/EvidenceIngestedCloudEvent.schema.json would disagree with TypeEvidenceIngested in events/events.go with no error raised. The only validation is the trailing "missing required keys" check (parser.go:238-246), which passes because every key is non-empty. The drift integration test cannot catch this because it regenerates from the same corrupted tag. That comma-bearing descriptions are realistic here is visible in the diff: the previous hand-written channel description in api/events/asyncapi.yaml was "Published when evidence is ingested, before sealing." - a string this tag format cannot represent. The doc comment at parser.go:190 acknowledges "Values may contain spaces but not commas" but nothing enforces it.

• 🛡️ Unmapped Go types silently become JSON Schema "object", including slices the parser explicitly emits (cmd/asyncapi-gen/schema.go:258)

  func goTypeToJSONSchema(goType string) string {
  	base := strings.TrimPrefix(goType, "*")
  	switch base {
  	case "string":
  		return "string"
  	case "int", "int32", "int64":
  		return "integer"
  	case "float32", "float64":
  		return "number"
  	case "bool":
  		return "boolean"
  	default:
  		return "object"
  	}
  }
  

  💡 Recommendation: Handle the [] prefix explicitly by emitting {"type": "array", "items": {"type": <element type>}}, and return an error (propagated through BuildDoc/BuildDataJSONSchema) for any Go type the generator cannot represent, so an unsupported field fails go generate instead of producing a silently incorrect published schema.
  💬 Full reviewer analysis
  The default branch converts every type it does not recognise into "object" with no error. This is not only a theoretical gap: fieldGoType at cmd/asyncapi-gen/parser.go:166 deliberately produces "[]" + fieldGoType(t.Elt) for slice fields, and goTypeToJSONSchema has no case matching a [] prefix, so a field declared Tags []string yields "type": "object" in both api/events/asyncapi.yaml and the published api/events/schemas/*.schema.json. The same applies to qualified types such as time.Time, which fieldGoType maps to "interface{}" (parser.go:168-169) and which then also becomes "object". Because these schemas are published for downstream consumer validation (README.md "Development" section and PR body: "standalone JSON Schema files (Draft 2020-12) ... for downstream consumer validation"), a consumer enforcing them would reject structurally valid events. No current event struct has a slice field, so nothing is broken today, but README.md documents "Adding a new event type" as the expected workflow and the generator will emit a wrong contract on the first such field rather than failing. Note schema_test.go asserts {"SomeStruct", "object"}, so the fallback is intentional for nested structs - the defect is that it silently swallows slices and qualified types too. Grepped cmd/asyncapi-gen/schema.go for [] handling in the mapper: the only [] occurrences are Go slice declarations (lines 107, 216, 233, 291), none is an array branch.

• 📚 Breaking-change procedure omits the asyncapi struct tag, so the published contract keeps advertising the old type (docs/versioning.md:102)

  1. Bump the CloudEvents `type` with a version suffix
     (`dev.complytime.evidence.ingested` → `dev.complytime.evidence.ingested.v2`).
     Update the `Type…` constant in [`events/events.go`](../events/events.go).
  

  💡 Recommendation: Amend step 1 of the Breaking section to require updating both the Type… constant and the type: key inside the asyncapi sentinel tag on the corresponding *Data struct, and state that the generated const in api/events/asyncapi.yaml and api/events/schemas/*.schema.json comes from the tag, not the constant. No Docs repo is configured for this council run (no Review Council Configuration block in the invoking repository), so no tracking issue is proposed - fix the doc in place.
  Constraint: Documentation gap detection - a documented procedure for a user-facing contract change must name every artifact the change requires.
  💬 Full reviewer analysis
  This PR makes events/events.go the source of truth and adds docs/versioning.md as the authoritative procedure for evolving a contract. The CloudEvents type now lives in two places in that file: the exported constant at line 18 (const TypeEvidenceIngested = "dev.complytime.evidence.ingested", consumed by e.SetType at line 45) and the type: key inside the sentinel asyncapi struct tag at line 24, which is what parseAsyncAPITag reads into EventSpec.CEType and what buildEnvelopeSchema emits as the const for the type property in the generated spec and JSON Schemas. The breaking-change step quoted above names only the constant. I grepped the repo for both occurrences (grep -rn "TypeEvidenceIngested\|type:dev.complytime" --include='*.go' --include='*.md' .): the struct-tag copy is referenced nowhere in docs/versioning.md or README.md. A maintainer who follows the documented steps exactly bumps only the constant, runs go generate, and ships a spec whose type const still says the v1 value while the producer emits v2. TestIntegration_GeneratedMatchesCommitted regenerates from the same struct tag, so it stays green and the mismatch reaches consumers.

• 📚 Version-bump procedure omits the version hardcoded in the drift-detection test (docs/versioning.md:90)

  1. Bump `info.version` minor (`0.1.0` → `0.2.0`) in the `//go:generate`
     directive.
  2. Leave `type`, `specversion`, and the subject unchanged.
  3. Run `go generate ./events/...` and commit the regenerated artifacts.
  

  💡 Recommendation: Either add a step to both procedures telling the maintainer to update the title/version/description arguments in cmd/asyncapi-gen/integration_test.go, or remove the duplication by having the test shell out to go generate (or read the flags from the //go:generate directive) so the doc's three steps stay complete.
  Constraint: Documentation gap detection - the documented workflow must cover every file a change touches.
  💬 Full reviewer analysis
  Both the additive procedure (lines 90-93) and the breaking procedure (line 105, Bump info.version major) tell the maintainer to change the version only in the //go:generate directive. I grepped the repo for the current version string (grep -rn "0\.1\.0" --include='*.go' --include='*.md' --include='*.yaml' --include='*.yml' .): besides events/events.go:7 and the generated api/events/asyncapi.yaml:5, it is also hardcoded at cmd/asyncapi-gen/integration_test.go:26 - doc := BuildDoc(specs, "ComplyTime API Events", "0.1.0", - together with a verbatim copy of the -description text on line 27. Following the documented steps therefore leaves the drift test comparing a doc built at 0.1.0 against a committed file at 0.2.0, and the failure message it prints (Run + backtick + go generate ./events/... + backtick + to update it.) instructs the maintainer to redo the step they already did. The same trap applies to any future edit of the description text.

• 📚 "Adding a new event type" omits the asyncapi tag's grammar, its description key, and its no-comma constraint (README.md:76)

  1. Define a new `*Data` struct in `events/events.go` with a sentinel blank
     field carrying the `asyncapi` tag (channel, params, stream, type, send,
     receive metadata).
  2. Add `asyncapi-field:"description:..."` tags on each struct field for
     schema descriptions.
  3. Run `task generate` to regenerate all derived artifacts.
  

  💡 Recommendation: Extend step 1 with the full key list (channel, param:<name>=<desc>, stream, type, send, receive, description), state that values must not contain commas, and show one filled-in sentinel-field example rather than describing it.
  Constraint: Documentation gap detection - a change that introduces a new authoring convention must document that convention completely.
  💬 Full reviewer analysis
  This PR introduces a struct-tag authoring convention and this README section is the only prose that teaches it. Two properties of the format that parseAsyncAPITag enforces are absent from it. First, the description key (channel description, es.ChannelDescription) is not listed among the recognised keys, even though events/events.go:24 uses it and it is what produces the channel description in the generated spec; I searched README.md and docs/versioning.md for description: used as an asyncapi tag key and found only the separate asyncapi-field form. Second, parseAsyncAPITag splits the tag on , and silently continues on any segment lacking a :, so a value containing a comma is truncated with no error - a maintainer writing send:Published when a widget is created, and stored gets Published when a widget is created in the published contract and no diagnostic. The section also gives no worked example of the tag, so the only reference is events/events.go itself.

• 🧭 Generated spec silently drops the field examples the hand-written contract carried (api/events/asyncapi.yaml:105)

                  contentDigest:
                      type: string
                      description: SHA-256 digest of the evidence artifact
  

  💡 Recommendation: Add an example: key to the asyncapi-field tag vocabulary (parallel to description:) and emit it as examples in both buildDataSchema and BuildDataJSONSchema, restoring the four example values the previous spec published. If examples are deliberately being retired, say so in the PR description so downstream consumers of the contract are not surprised.
  Constraint: Cross-Component
  💬 Full reviewer analysis
  The pre-change asyncapi.yaml carried examples: values on four schema properties (contentDigest: sha256:abc123..., artifactType: application/vnd.gemara.evaluation-log+json, subjectId: my-app-v1, and source: complytime-gateway). Regenerating the spec from Go types drops all of them, and the generator has no way to express them: grep -rni "example" cmd/asyncapi-gen/ events/ returns only the dev.example.widget.created string inside cmd/asyncapi-gen/testdata/fixture.go, and grep -rn "examples" api/ returns nothing. The struct tag vocabulary defined in parseAsyncAPITag (cmd/asyncapi-gen/parser.go) and extractFieldDescription supports only description:, so this is a permanent capability gap, not a one-time omission. The PR description enumerates "Field descriptions" as an addition and "Generated files ... are deterministic" but nowhere discloses that example values present in the published contract are removed; the closest passage, "Commit 951376e updates the channel and data descriptions for readability", does not cover deletion of examples.

• 🧭 Published contract changed shape but info.version stays 0.1.0, contradicting the versioning policy added in this PR (api/events/asyncapi.yaml:5)

      title: ComplyTime API Events
      version: 0.1.0
  

  💡 Recommendation: Bump -version in the //go:generate directive in events/events.go and regenerate, so the first spec produced under the new pipeline carries a version distinct from the hand-written 0.1.0 - or state explicitly in docs/versioning.md that pre-1.0 contracts may change in place, so the policy and the artifact agree.
  Constraint: Intent Drift
  💬 Full reviewer analysis
  docs/versioning.md, added by this changeset, states at line 28 that info.version versions "The whole published contract" for "Humans, codegen tooling", and at lines 70-74 that "Consumers integrate from it and pin codegen to info.version". This changeset materially changes that published contract while leaving info.version: 0.1.0 unchanged: the message component key was renamed from evidenceIngested to EvidenceIngested (so #/components/messages/evidenceIngested no longer resolves), a servers: block and a NATS bindings block were added, the channel description was rewritten, and property examples were removed. Under the document's own decision rule (line 99: "Removing or renaming a field ... Bump info.version major"), a consumer that pinned codegen to 0.1.0 now gets a different document under the same version string. Mitigating: git tag in the clone returns no tags, so 0.1.0 appears never to have been released, which is why this is MEDIUM rather than a live contract break.

• 🧭 Drift test hardcodes doc metadata, so the documented version-bump procedure breaks CI with a misleading message (cmd/asyncapi-gen/integration_test.go:26)

  	doc := BuildDoc(specs, "ComplyTime API Events", "0.1.0",
  

  💡 Recommendation: Have the test drive the real generator (e.g. go generate ./events/... into a temp output, or parse the flags out of the //go:generate line) instead of restating the metadata, so document metadata has one authoring location.
  Constraint: Structural Coherence
  💬 Full reviewer analysis
  The single source of truth for document metadata is the //go:generate directive at events/events.go:7 (-title, -version, -description, -license, -contact-name, -contact-url, -server). This test re-specifies all seven values as literals rather than invoking the generator, so the pair must be edited in lockstep. grep -rn "0\.1\.0" over the repo confirms the version literal lives in exactly two authoring locations - events/events.go:7 and this line - plus the generated artifact. docs/versioning.md:90 instructs a maintainer to "Bump info.version minor (0.1.0 -> 0.2.0) in the //go:generate directive" and then "Run go generate ./events/... and commit the regenerated artifacts"; following that procedure exactly leaves this test comparing a locally built 0.1.0 document against a committed 0.2.0 one, and the failure message it prints - "Run go generate ./events/... to update it" - points at a step that was already performed and will not fix the failure. The drift detector then reports drift that does not exist.

• 🧭 CloudEvents envelope definition duplicated between the YAML and JSON Schema generators (cmd/asyncapi-gen/jsonschema.go:72)

  		"required":    []string{"specversion", "id", "type", "source", "subject", "time", "datacontenttype", "data"},
  

  💡 Recommendation: Derive both envelopes from one declaration - for example a shared envelopeFields() returning name/type/const/format/description tuples that schema.go renders into Schema and jsonschema.go renders into JSONSchema - so the contract is stated once.
  Constraint: Structural Coherence
  💬 Full reviewer analysis
  BuildEnvelopeJSONSchema (jsonschema.go:52-75) and buildEnvelopeSchema (schema.go:212-228) independently encode the same envelope contract: the identical eight-entry required list, the same eight properties, the same const values ("1.0", "application/json"), and the same literal descriptions ("URI identifying the producing service", "The compliance subject identifier"). The two outputs are published side by side as one contract (api/events/asyncapi.yaml and api/events/schemas/), so adding a CloudEvents attribute such as dataschema or making subject optional requires editing both. Nothing enforces agreement: the integration tests compare each generator against its own committed output, not against each other, so the YAML and JSON Schema envelopes can drift apart while all tests stay green. This is business logic duplicated across modules, not incidental similarity.

• ⚙️ Generated schema directory is never pruned and orphaned files are never detected (cmd/asyncapi-gen/jsonschema.go:81)

  if err := os.MkdirAll(dir, 0o755); err != nil {
  

  💡 Recommendation: Have WriteJSONSchemas remove pre-existing *.schema.json files in the target directory before writing (or write to a temp dir and swap), and extend TestIntegration_JSONSchemasMatchCommitted to read the committed directory listing and fail on any file not produced by the current specs. Consider adding a schema-validation step covering api/events/schemas/ alongside the existing asyncapi validation.
  Constraint: Generated asset synchronization - checked-in generated files consistent with source definitions
  💬 Full reviewer analysis
  WriteJSONSchemas creates the output directory and writes one data schema plus one envelope schema per spec (loop at line 85), but never removes files that no longer correspond to any spec. The paired drift test, TestIntegration_JSONSchemasMatchCommitted, iterates for _, spec := range specs and then for _, name := range files (integration_test.go:74), checking only the filenames it expects to exist - it never lists the committed directory to find extras. I verified api/events/schemas/ currently holds exactly the two expected files. Concretely: rename EvidenceIngestedData to EvidenceAcceptedData in events/events.go and run go generate; the two new schema files appear, EvidenceIngestedData.schema.json and EvidenceIngestedCloudEvent.schema.json remain on disk and get committed, and CI stays green while consumers see a stale published schema for an event that no longer exists. Separately, nothing in the pipeline validates these JSON Schema files at all - task asyncapi-lint and .github/workflows/ci_asyncapi.yml:30 both target only api/events/asyncapi.yaml - so the relative $ref from the envelope schema to the data schema is never resolution-checked either.

• ⚙️ Published event contract hardcodes localhost as the NATS server (events/events.go:7)

  -server nats://localhost:4222
  

  💡 Recommendation: Either point -server at the real ecosystem broker address, or keep the local address and populate the server Description (e.g. 'Local development broker; override per environment') so consumers and codegen users are not silently handed a loopback endpoint. Wiring the value through an environment variable or a checked-in config read by the directive would also let non-dev environments regenerate without editing source.
  Constraint: Hardcoded paths, hostnames, or environment-specific values that should be parameterized
  💬 Full reviewer analysis
  The //go:generate directive passes a developer-workstation address as the server URL, and buildServers in cmd/asyncapi-gen/schema.go turns it into servers:\n nats:\n host: localhost:4222\n protocol: nats in the committed api/events/asyncapi.yaml (I read the generated block). README.md line 8 presents that YAML as the contract downstream consumers integrate against, and docs/versioning.md calls it 'the public, machine-readable contract' that 'Consumers integrate from'. AsyncAPI codegen tools read the servers block to seed client connection defaults, so any consumer generating a client from the published spec gets localhost:4222. The generator's Server struct (schema.go) carries a Description field that BuildDoc never populates, so the emitted entry has no text marking it as a local-development placeholder either.

• ⚙️ Required PR check resolves an unpinned npm dependency tree at run time (.github/workflows/ci_asyncapi.yml:30)

  run: npx --yes @asyncapi/cli@2.16.1 validate api/events/asyncapi.yaml
  

  💡 Recommendation: Commit a package.json plus package-lock.json declaring @asyncapi/cli@2.16.1 and run npm ci && npx asyncapi validate api/events/asyncapi.yaml, which makes the tool tree byte-reproducible and lets Dependabot/Renovate track it. Add timeout-minutes to the job so a registry hang fails fast rather than consuming the default 6-hour budget.
  Constraint: Release pipeline integrity - all dependencies pinned to specific versions; builds reproducible from same inputs
  💬 Full reviewer analysis
  The workflow pins the CLI itself to 2.16.1 but there is no package.json or package-lock.json anywhere in the repo (I listed the repo root and confirmed only commitlint.config.js exists as npm-adjacent config), so npx resolves @asyncapi/cli's entire transitive tree fresh from the npm registry on every run. That is exactly the failure class this project has already hit: Taskfile.yml:28 records the reason for the pin as 'latest has broken npm dependency (@asyncapi/studio-ui@0.5.0 404)'. Pinning the top-level version does not prevent a recurrence one level down, because published packages carry semver ranges rather than exact dependency versions. The job runs on every push and PR to main with no timeout-minutes bound, so an upstream unpublish or registry incident turns every PR red - and blocks merges - with no change to this repository. The Taskfile's asyncapi-lint task has the identical exposure.

• 🧪 TestRun_WriteError depends on the process lacking write access to / and writes outside the test sandbox when it has it (cmd/asyncapi-gen/main_test.go:129)

  		Input:   "testdata/fixture.go",
  		Output:  "/nonexistent/deeply/nested/dir/out.yaml",
  

  💡 Recommendation: Produce the write error inside the sandbox: create a file (not a directory) inside t.TempDir() and use filepath.Join(thatFile, "out.yaml") as the output path, which fails with ENOTDIR regardless of privilege level and leaves nothing outside the temp dir.
  Constraint: Test isolation: tests must not depend on ambient filesystem permissions or write outside t.TempDir()
  💬 Full reviewer analysis
  The test asserts an error by pointing the output at an absolute path under /. WriteYAML (cmd/asyncapi-gen/writer.go:24) now calls os.MkdirAll(filepath.Dir(path), 0o755) before writing, so when the test process can write to / - root in a container, the common local dev setup for this repo - MkdirAll succeeds, WriteFile succeeds, run returns nil, and the test fails at t.Fatal("expected error for invalid output path") while having created /nonexistent/deeply/nested/dir/ on the host and left it behind. Every other test in this package correctly confines itself to t.TempDir(). The failure is environment-dependent, not code-dependent, so it will read as a mystery break to anyone not running as an unprivileged user.

• 🧪 The schema write error branch of run() has no test, contradicting the PR's "all error paths" claim (cmd/asyncapi-gen/main.go:76)

  	if opts.SchemasDir != "" {
  		if err := WriteJSONSchemas(specs, opts.SchemasDir); err != nil {
  			return fmt.Errorf("schema write error: %w", err)
  		}
  

  💡 Recommendation: Add a TestRun_SchemaWriteError case that sets SchemasDir to a path under a regular file inside t.TempDir() (so os.MkdirAll in WriteJSONSchemas fails deterministically) and asserts the error mentions schema write error.
  Constraint: Coverage completeness: every error return in a changed function needs a test (lang-go testing_conventions)
  💬 Full reviewer analysis
  The PR description states "Add main_test.go with 6 test cases covering all error paths". Three of run's four error returns are covered (required flags missing, parse error, write error); this one is not. I ran grep -rn "schema write error" --include='*.go' . - the string occurs only at cmd/asyncapi-gen/main.go:76 and in no test file. The branch is reachable whenever -schemas-dir points somewhere unwritable, which is precisely the case the branch exists for, and it is the only error path whose wrapping message could regress unnoticed.

• 🧪 CLI happy-path tests assert only stdout substrings and never check that any artifact was produced (cmd/asyncapi-gen/main_test.go:116)

  	output := stdout.String()
  	if !strings.Contains(output, "wrote JSON schemas") {
  		t.Errorf("stdout = %q, want mention of wrote JSON schemas", output)
  	}
  	if !strings.Contains(output, "wrote") {
  		t.Errorf("stdout = %q, want mention of wrote", output)
  	}
  

  💡 Recommendation: After run returns, stat/read dir + "/asyncapi.yaml" and assert it is non-empty and contains the fixture channel address; in the schemas test assert both WidgetCreatedData.schema.json and WidgetCreatedCloudEvent.schema.json exist under SchemasDir. Replace the redundant "wrote" check with an assertion on the distinct "wrote <path> (1 event(s))" line.
  Constraint: Assertion quality: assertions must verify the produced result, not only a log line (severity.md: shallow assertions)
  💬 Full reviewer analysis
  TestRun_HappyPath (line 82) and TestRun_HappyPathWithSchemas (line 101) are the only tests of the whole CLI success path, and neither reads opts.Output or opts.SchemasDir back off disk. If run printed its progress lines but wrote an empty file, wrote to the wrong path, or emitted zero schema files, both tests would still pass. The second assertion quoted here is additionally tautological: "wrote JSON schemas" contains "wrote", so once the first check passes the second can never fail - it adds no signal. stderr is captured in both tests and never asserted, matching the fact that run never writes to the stderr writer it accepts.

• 🧪 Slice-typed event fields are an implemented parser branch with no test in either the parser or the type-mapping table (cmd/asyncapi-gen/parser.go:166)

  	case *ast.ArrayType:
  		return "[]" + fieldGoType(t.Elt)
  

  💡 Recommendation: Add a slice field and an untagged / json:"-" field to testdata/fixture.go with parser assertions for each, and add {"[]string", ...} and {"interface{}", ...} rows to the goTypeToJSONSchema table once the intended mapping (array) is decided.
  Constraint: Coverage completeness: implemented branches of core generator logic require tests
  💬 Full reviewer analysis
  fieldGoType explicitly handles slice fields, producing e.g. "[]string", which goTypeToJSONSchema (cmd/asyncapi-gen/schema.go:258) then falls through to default and maps to "object" - a wrong JSON Schema type for an array. Nothing exercises it: cmd/asyncapi-gen/testdata/fixture.go declares only string and *string fields, and the TestGoTypeToJSONSchema table (cmd/asyncapi-gen/schema_test.go:236-252) has no slice row despite the PR describing it as "11 cases, all branches" - grep -rn '\[\]string\|ArrayType\|interface{}' cmd/asyncapi-gen/*_test.go returns no matches. The same gap covers the default: return "interface{}" branch (parser.go:168) and the field-skipping branches at parser.go:121 (untagged field) and parser.go:127 (json:"-"), none of which appear in any fixture or temp-file test. The first event type that adds a slice field will silently ship type: object to downstream consumers with a green test suite.

• 🧪 No test validates a real event payload against the shipped JSON Schemas (cmd/asyncapi-gen/jsonschema_test.go:131)

  	if dataSchema["$schema"] != "https://json-schema.org/draft/2020-12/schema" {
  		t.Error("data schema missing $schema")
  	}
  

  💡 Recommendation: Add a contract test in the events package that builds an event via NewEvidenceIngestedEvent, marshals it, and validates the result against api/events/schemas/EvidenceIngestedCloudEvent.schema.json (a Draft 2020-12 validator such as santhosh-tekuri/jsonschema, or at minimum an assertion that every required name in the schema is present in the marshaled JSON and that no schema property is absent from the struct).
  Constraint: Missing contract test for a published API contract (severity.md, Tester/MEDIUM)
  💬 Full reviewer analysis
  api/events/schemas/*.schema.json are described in the PR as "standalone JSON Schema files (Draft 2020-12) ... for downstream consumer validation", yet no test anywhere asserts that a message produced by events.NewEvidenceIngestedEvent satisfies them. The written-file test quoted here checks only that the $schema key round-trips; the integration test compares bytes of generator output against generator output; events/events_test.go marshals the struct but never sees the schemas. I ran grep -rn 'schema.json\|jsonschema\|Validate' --include='*.go' . - the only references to the schema files are the generator and the byte-comparison drift test. The result is that the envelope schema's required list (specversion, id, type, source, subject, time, datacontenttype, data) and the data schema's required set are never checked against what the library actually emits, so a divergence - e.g. a required field the constructor does not set, or shardId typed string while the Go field is *string and may serialize null - would ship uncaught. CI validates only the YAML (.github/workflows/ci_asyncapi.yml:30 runs validate api/events/asyncapi.yaml); the JSON Schemas are validated by nothing.

🔵 LOW (8)

• 📚 task generate description names only asyncapi.yaml but the target also writes the JSON Schemas (Taskfile.yml:21)

    generate:
      desc: Regenerate derived artifacts (asyncapi.yaml)
  

  💡 Recommendation: Change the description to something like Regenerate derived artifacts (asyncapi.yaml and JSON Schemas) so task --list matches the README.
  Constraint: Cross-reference consistency - task descriptions are user-facing help text and must match what the task does.
  💬 Full reviewer analysis
  desc is what task --list prints, so it is user-facing documentation. The //go:generate directive this target runs passes -schemas-dir ../api/events/schemas, and README.md lines 59-60 correctly say the command "rebuilds api/events/asyncapi.yaml and the JSON Schema files in api/events/schemas/". The task description added in this changeset lists only asyncapi.yaml, so the CLI help understates the target's effect relative to the README written in the same PR.

• 📚 Generated JSON Schemas carry no do-not-edit provenance marker (api/events/schemas/EvidenceIngestedData.schema.json:2)

    "$id": "EvidenceIngestedData.schema.json",
    "$schema": "https://json-schema.org/draft/2020-12/schema",
  

  💡 Recommendation: Add a $comment key in BuildDataJSONSchema and BuildEnvelopeJSONSchema (e.g. "Generated by cmd/asyncapi-gen from events/events.go - do not edit manually; run 'go generate ./events/...'") and regenerate, so every generated artifact states its provenance.
  Constraint: Documentation convention compliance - generated artifacts in this repo announce that they are generated.
  💬 Full reviewer analysis
  The other artifact this generator emits, api/events/asyncapi.yaml, is self-describing: its info.description (line 9 of that file) ends with "Do not edit manually - run 'go generate ./events/...' to regenerate.", and it carries the SPDX header prepended by WriteYAML. Both new schema files under api/events/schemas/ are emitted by writeJSON with no equivalent marker; I read both files and grepped them for generate, generated and SPDX with no hits. The warning exists only in README.md line 10, which a downstream consumer or a maintainer opening the schema file directly will not see. JSON Schema defines $comment for exactly this purpose, so the marker can be added without breaking validators.

• 🧭 Envelope schema name computed inline instead of via the existing envelopeSchemaName helper (cmd/asyncapi-gen/schema.go:143)

  		envSchemaKey := strings.TrimSuffix(spec.StructName, "Data") + "CloudEvent"
  

  💡 Recommendation: Replace the inline expression with envSchemaKey := envelopeSchemaName(spec.StructName).
  Constraint: Zero-Waste
  💬 Full reviewer analysis
  envelopeSchemaName (cmd/asyncapi-gen/jsonschema.go:102-104) already performs exactly this derivation via messageKey(structName) + "CloudEvent", and it is used in jsonschema.go:91 and integration_test.go:72. Both files are in package main, so the helper is directly callable here. The inline copy means the naming rule for envelope schemas exists in two places, and the AsyncAPI component key and the JSON Schema filename could diverge if only one is changed.

• 🧭 Channel description loses the lifecycle detail "before sealing" (api/events/asyncapi.yaml:25)

          description: Evidence ingestion pipeline for compliance artifacts
  

  💡 Recommendation: Restore the ordering detail in the description: value of the asyncapi struct tag in events/events.go and regenerate, e.g. "Evidence ingestion pipeline for compliance artifacts; published before sealing".
  Constraint: Cross-Component
  💬 Full reviewer analysis
  The previous channel description read "Published when evidence is ingested, before sealing." The replacement text describes what the channel is rather than when the event fires, dropping the "before sealing" ordering guarantee - the one fact in that sentence a consumer could act on when deciding whether the referenced artifact is final. The PR description covers this only as "Commit 951376e updates the channel and data descriptions for readability based on the existing functionality in PR 8", which characterises the edit as stylistic; removing a stated lifecycle ordering is a semantic change to the published contract, not a readability one.

• ⚙️ New Development section omits the Node.js and network prerequisites it introduces (README.md:62)

  To validate the generated AsyncAPI spec:
  
  ```bash
  task asyncapi-lint
  

  
  
      
        
      
  
        
      
  
      
    
  💡 **Recommendation:** Add a one-line prerequisites note to the Development section listing Go (per go.mod), Task, golangci-lint, and Node.js (for the AsyncAPI CLI), and mention that asyncapi-lint requires network access on first run.
  **Constraint:** Operational documentation - environment prerequisites explicit
  💬 Full reviewer analysis
  The Development section added by this change documents `task generate`, `task asyncapi-lint` and `task check` but never states what a contributor needs installed to run them. `task asyncapi-lint` requires Task, Node.js with npx, and outbound network access to the npm registry (Taskfile.yml:29), and Taskfile.yml:37 folds asyncapi-lint into `check` - so `task check`, which README.md line 68 presents as the run-everything entry point, now silently fails on any machine without Node or without registry access, where it previously needed only Go and golangci-lint. I searched the repo for a CONTRIBUTING file and found none, so the README is the only place a contributor would learn this.
  

• 🧪 Two-level test helper wrapper around os.WriteFile with a single caller (cmd/asyncapi-gen/main_test.go:143)

  func writeTestFile(path, content string) error {
  	return writeFileForTest(path, []byte(content))
  }
  

  💡 Recommendation: Inline os.WriteFile(input, []byte(content), 0o644) at the single call site, matching parser_test.go, and drop both helpers.
  Constraint: Test architecture: assertions and setup should be direct, not hidden behind abstraction layers
  💬 Full reviewer analysis
  writeTestFile has exactly one caller (line 61) and does nothing but convert a string to bytes and delegate to writeFileForTest, which in turn does nothing but call os.WriteFile. Two indirection layers for one call site obscure where the fixture is written and where the //nolint:gosec suppression applies. parser_test.go (lines 115, 134) writes its temp fixtures with a direct os.WriteFile call, so this package already has an inconsistent pattern for the same operation.

• 🧪 Comment in TestChannelName table describes the wrong row (cmd/asyncapi-gen/schema_test.go:314)

  		{"Data", "Data"},
  		// edge: name becomes empty after trim, returns original
  		{"SimpleData", "simple"},
  

  💡 Recommendation: Move the comment directly above the {"Data", "Data"} row.
  Constraint: Test clarity: comments must describe the case they annotate
  💬 Full reviewer analysis
  The comment sits above {"SimpleData", "simple"} but describes the preceding row: channelName("Data") trims the Data suffix to the empty string, hits the if len(name) == 0 guard at cmd/asyncapi-gen/schema.go:277 and returns the original - that is the edge case. SimpleData trims to Simple and lowercases normally; nothing about it is empty-after-trim. A future reader deleting the "redundant-looking" {"Data", "Data"} row would remove the only coverage of that guard while believing the comment says it is still covered.

• 🧪 humanTitle table omits the trim-to-empty case its sibling channelName table covers (cmd/asyncapi-gen/schema_test.go:331)

  		{"EvidenceIngestedData", "Evidence Ingested"},
  		{"WidgetCreatedData", "Widget Created"},
  		{"SimpleData", "Simple"},
  

  💡 Recommendation: Add a {"Data", ...} row to the humanTitle table asserting the intended output, matching the boundary row already present in TestChannelName.
  Constraint: Coverage consistency: parallel helpers should be exercised on the same boundary inputs
  💬 Full reviewer analysis
  channelName and humanTitle (cmd/asyncapi-gen/schema.go:275 and :289) both begin with strings.TrimSuffix(structName, "Data"), but only channelName guards the empty result and only its table (line 313) tests the "Data" input. humanTitle("Data") reaches the loop with an empty name and returns "", which would surface as an empty title: on the generated AsyncAPI message - untested and unasserted either way, so the intended behaviour for that input is undefined by the suite.

Produced by Review Council, an open-source multi-persona code reviewer. Spot a wrong call or want the source? File feedback or browse the repository.

@trevor-vaughan updated with review feedback in latest commits.