test: strengthen taxonomy API contracts (ENG-1202)

[BhagyaAmarasinghe]

Summary

• centralize public and internal taxonomy route registration so production and integration tests cannot drift
• verify the public route inventory, operation IDs, and response statuses bidirectionally against openapi.yaml while keeping internal routes private
• extend taxonomy HTTP contracts for RFC 9457 errors, heartbeat, service availability, missing/invalid runs, invalid transitions, and tenant isolation

Linear: ENG-1202

Validation

• go test -run "^TestTaxonomyAPI" -count=1 ./tests
• go test ./tests/... -count=1 -timeout 120s against a migrated pgvector Postgres database
• make test-unit
• make build
• make lint-new
• make lint-openapi
• downstream taxonomy client: uv run pytest tests/test_hub_client.py tests/test_start_run.py tests/test_taxonomy_domain.py -q (34 passed)

[coderabbitai]

Walkthrough

The change centralizes public and internal taxonomy route registration in shared functions. The API application and taxonomy test server now use these registries. Route tests compare registered public routes with the OpenAPI document and exclude internal routes from it. API tests add structured RFC 9457 error checks, tenant isolation coverage, service-availability checks, internal endpoint contracts, terminal-run conflict cases, and reusable result fixtures.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description covers the main objectives, validation steps, and references the issue. However, the 'How should this be tested?' section lacks detail on how reviewers can reproduce the tests, and several required checklist items are not checked off.	Add specific instructions in 'How should this be tested?' to enable reviewer reproduction. Check off all completed required checklist items to confirm validation steps were performed.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: strengthening taxonomy API contracts through expanded testing, which directly reflects the core objective of the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/taxonomy_api_test.go`:
- Around line 1086-1111: Replace the identical inline taxonomy payload in the
“complete run stores artifacts and activates” subtest with a call to
validTaxonomyResult(feedbackRecordID). Leave the lifecycle test payload
unchanged because its cluster size and confidence differ.
- Around line 473-482: The test hard-codes the expected error message as "found
1" in the assertTaxonomyInvalidParam call, but this value should be derived from
the actual seeded count which is taxonomyMinEmbeddedRecords-1. Update the
assertTaxonomyInvalidParam call to calculate the expected count dynamically from
taxonomyMinEmbeddedRecords-1 and use string formatting to construct the expected
message string instead of the literal "found 1", ensuring the fmt package is
imported.
- Around line 71-79: In the shared problem-response assertion helper, add an
assertion that problem.RequestID is not empty, alongside the existing
assert.NotEmpty(t, problem.Title) check, so all error paths verify the RFC 9457
contract includes a request_id.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d5194e03-8678-4d61-829b-e91ddfa97bc8

📥 Commits

Reviewing files that changed from the base of the PR and between 4993dfb and be26f24.

📒 Files selected for processing (5)

• cmd/api/app.go
• internal/api/routes/taxonomy.go
• internal/api/routes/taxonomy_test.go
• tests/taxonomy_api_test.go
• tests/taxonomy_support_test.go

[xernobyl]

The premise checks out — the drift was real: the old harness registered only 4 of the 5 internal taxonomy routes (heartbeat was missing entirely, so it had zero coverage), and the shared registry closes that. Both new layers also actually run in CI (test-unit covers ./internal/..., integration runs on pg16/17/18).

Four things worth fixing, in severity order. Two are about the abstraction this PR introduces; one is pre-existing but becomes load-bearing here, because the PR builds a whole RFC 9457 assertion layer on top of it.

I haven't re-raised the three open CodeRabbit threads — all three are valid — but note the interaction flagged in the taxonomy_support_test.go comment below: its suggestion #1 (assert.NotEmpty(t, problem.RequestID)) turns CI red if applied before the middleware fix.

[xernobyl]

Approving — verified at aa5cc83, not just on the resolved threads.

All four of my findings and CodeRabbit's three are genuinely fixed. The method-expression route table drops both enums, both switches and both panic branches, and puts the path next to its handler again; I dumped the actual bindings by reflection and all 15 resolve correctly and are mutually distinct. The harness now wraps middleware.RequestID(middleware.ProblemErrors(mux)), which is what makes the request_id assertion meaningful and the new 404/405 cases reachable at all. The OpenAPI decode tolerates path-item metadata, with a fixture test pinning it.

Ran locally: make build, go vet ./..., make test-unit (0 failures), the full ./tests/... suite against a migrated pgvector database, make lint-new against current main, and make lint-openapi — all clean, matching CI.

Worth noting that the contract test paid for itself immediately: ENG-1887 landed on main and added 404 to list-taxonomy-node-records, so the table had to be synced. That's the drift mechanism catching a real collision on its first outing rather than a hypothetical one. Nice.

Three non-blocking things, all optional:

1. The new node records 404 for another tenant case uses requestTaxonomyJSON (status + Content-Type only), while its comment says "Same contract as the run, tree, rename and remove cases above" — and those four all use requestTaxonomyProblem, which also checks code, type, instance and request_id. I confirmed the stronger helper passes here, so it's a two-line upgrade if you want the comment to be literally true:

requestTaxonomyProblem(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil,
	http.StatusNotFound, response.CodeNotFound, response.ProblemTypeNotFound)

2. writeProblem mirrors request_id into the X-Request-ID header specifically so the two stay consistent "even if upstream middleware ordering changes" — asserting problem.RequestID == resp.Header.Get("X-Request-ID") in the shared helper would lock that down now that the middleware is in the harness.

3. TestTaxonomyAPI_RoutingProblems is the only test function in the file without a doc comment.

One thing to sort before merge: the description still has ## Summary / ## Validation rather than the repo template's ## What does this PR do? / ## How should this be tested? plus the checklist. The Validation section does cover the commands, so it's mostly a copy-paste of the reproduction steps into the right sections and ticking the boxes.