Skip to content

[TECH DEBT] Let the soothfast bot land SDK regeneration instead of make sdk #482

Description

@Verdenroz

Summary

make sdk is a manual pre-PR step only because our own workflow gates the generated specs and SDKs with --check at PR time. soothfast already supports regenerating and bot-landing exactly these paths, and we already configure it to — the two mechanisms just sit side by side, and the --check is the one that wins. This is our CI's limitation, not soothfast's.

Current state

The same four artifacts are handled two different ways in the same workflow.

Already wired for bot-landing — .github/workflows/soothfast.yml:106-115, the gate job, which runs on push to master:

regen-run: |
  "$SOOTHFAST" spec gen -p finance-query-server --target soothfast-routes
  "$SOOTHFAST" sdk gen  -p finance-query-server --target soothfast-routes
  "$SOOTHFAST" spec gen -p finance-query-mcp    --target soothfast-routes --features backtesting
regen-paths: server/openapi.yaml server/asyncapi.yaml sdks finance-query-mcp/mcp-tools.json

That regen-run block is make sdk (Makefile:123-127) verbatim, over the same paths.

Also gated with --check at PR time, which is what forces the manual run:

Job Step Line
Spec & SDK (server) spec gen -p finance-query-server --target soothfast-routes --check soothfast.yml:249
Spec & SDK (server) sdk gen -p finance-query-server --target soothfast-routes --check soothfast.yml:251
Spec (MCP) spec gen -p finance-query-mcp --target soothfast-routes --check --features backtesting soothfast.yml:301

Compare how the other generated artifacts are treated. docs/reference/**, docs/perf/**, docs/server/**, docs/coverage.md and llms.txt are generated by make docs-pages, and .claude/rules/workflow.md marks them CI, bot-landed after merge. Never locally. Same class of artifact, opposite policy, no stated reason for the split.

What the manual step costs

  • Two rules files (CLAUDE.md, .claude/rules/workflow.md) carry a "you run make sdk before the PR" instruction, and a PreToolUse hook denies direct edits to these paths — all scaffolding that exists to enforce a step CI could do itself.
  • It requires the contributor to have the exact pinned cargo-soothfast installed. A stale CLI emits plausible, wrong output — 0.2.0 silently dropped every struct field list from llms.txt. That is a documented hazard we push onto every contributor.
  • make sdk needs a full build of finance-query-server and finance-query-mcp against the soothfast-routes target. On a shared machine that competes with whatever else is compiling.
  • It is easy to forget, and the failure mode is a red check discovered after the PR is open rather than a thing that just happens.

Proposed change

Stop gating the generated specs and SDKs with --check at PR time and let the existing regen-run/regen-paths bot-land them after merge, exactly as make docs-pages output is handled today. make sdk becomes optional-for-convenience, the workflow.md table row moves from You to CI, and the hook and rules around it get simpler.

Three things to get right, none of them blockers but all of them easy to break:

  1. Keep spec check at PR time. soothfast.yml:246 and :299 reconcile routes against the handlers and the hand-authored manifests. That is a genuinely different check from spec gen --check, and it is the one that catches a route missing from server/spec/routes.rs — which would otherwise be absent from openapi.yaml and both SDKs with CI green. Do not remove it along with the freshness checks.
  2. Decide what spec gate compares. soothfast.yml:253-261 runs spec gate --base "$BASE_SHA" --from-committed for API-compat against the merge base. If the spec is no longer regenerated at PR time, --from-committed compares stale committed output against the base. It probably needs to gate freshly generated output instead.
  3. Accept that reviewers stop seeing the SDK diff. The generated client change would land after merge rather than appearing in the PR. For a published SDK that is a real tradeoff — a breaking client change becomes invisible during review. Worth deciding deliberately rather than discovering later.

Also note this adds to the bot-commit volume tracked in #479, which is about collapsing the two regeneration commits into one. These two want solving together rather than separately.

Acceptance

  • A PR that changes a route or tool without running make sdk passes CI.
  • The regenerated specs, mcp-tools.json and sdks/** land on master after merge via the existing soothfast-bot path.
  • spec check still fails a PR that adds a route without registering it in the spec manifest.
  • CLAUDE.md and .claude/rules/workflow.md are updated so the table and the regeneration policy match what CI actually does.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

refactorInternal improvement, cleanup, or architectural change

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions