Skip to content

feat: follow openapi.yaml, and sync it daily by pull request - #51

Merged
yiichou merged 1 commit into
mainfrom
yiichou/feat-qunli-t17509
Oct 16, 2026
Merged

yiichou merged 1 commit into
mainfrom
yiichou/feat-qunli-t17509

Conversation

@yiichou

@yiichou yiichou commented Oct 15, 2026 •

Copy link
Copy Markdown

Why

The CLI wrote down every path, parameter and allowed value by hand, a second copy of what openapi.yaml already says. The copy had fallen behind: view create --type form and form create --scene form|quickpay were refused before reaching the API, and the form edit help example sent an exam_setting key the API rejects. Nothing told anyone when the spec moved.

This adds an Action that notices openapi.yaml changes and brings them to the CLI by pull request. Which commands exist stays the CLI's decision; what a request may carry — paths, parameters and their types, the values a flag offers — follows the spec.

What changed

  • spec/openapi.yaml: the published spec, verbatim. npm run spec:update fetches it; npm run generate writes src/generated/operations.ts (every operation's method, path, parameters and request-side schemas, prose stripped). A schema keyword the generator does not know stops it rather than being dropped.
  • src/spec.ts: call('listEntries', { path, query, body }) is how every command makes a request now. It checks path and query against the operation — an undeclared or missing parameter, a value not of its type — and spells a list the way the spec says (explode: false → comma-joined). onContainer picks the form or table operation by --form / --table; choices / bodyChoices feed the flags' allowed values from the spec. A list the spec stops giving leaves its flag open rather than crashing every command at load; a test fails on it instead.
  • A body goes as given. The API validates every request against the openapi.yaml it runs and answers a precise 400; a local copy would make every installed CLI refuse what the API learns after it ships.
  • The tests hold every request the suite sends, and every example in --help, to the spec — body included — with src/testing/conformance.ts, which is not shipped. That is how the form edit example was found.
  • .github/workflows/spec-sync.yml: daily at 09:17 Asia/Shanghai. Fetches, regenerates, runs check and test, and opens a PR on chore/sync-openapi listing the operations and schemas that moved and which the CLI calls (scripts/spec-diff.mjs; an operation counts as changed when any schema it refers to changed). The title comes from the same comparison: feat: follow openapi.yaml changes to <operations> when an operation the CLI calls changed, a chore otherwise. A PR is opened even when the CLI no longer passes, and the run fails so it is noticed.
  • CI fails if src/generated differs from what npm run generate produces.
  • ARCHITECTURE.md gains a "The spec" section; README's Development section lists the two new scripts.

Behaviour changes

  • view create --type takes form; form create --scene takes form and quickpay; table create --help lists RelationField.
  • transaction list|aggregate --status a --status b sends paid_status=a,b (spec: explode: false) instead of paid_status[]=.
  • Refused before sending, exit 2: entry list --view with --limit (a view is read 50 a page), entry count of several containers with --keyword, a non-numeric entry serial.
  • form edit example now uses exam_setting.limited_time; total_score is not in ExamSettingInput.

Setup

The sync PR is opened with RELEASE_PLEASE_TOKEN so CI runs on it. Without that secret it falls back to GITHUB_TOKEN, which needs "Allow GitHub Actions to create and approve pull requests" enabled and shows no checks on the PR. workflow_dispatch only works from the default branch, so the workflow can first be run by hand right after merge.

Testing

  • npm run check and npm test (277 tests) pass.
  • Renaming an operation the CLI calls in the spec and regenerating makes tsc fail at the call sites.
  • spec-diff.mjs and the PR-description step were run locally against an older copy of the spec; the workflow itself has not run on GitHub yet.

Refs: #T17509

🤖 Generated with Claude Code

@kba977

kba977 commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Nice direction overall: one source of truth for paths, parameters and allowed values, plus a sync that turns spec drift into a reviewable PR. I checked that the backend runs the same request check on every v1 endpoint (multipart skipped) and that paid_status is read both as paid_status[]= and comma-joined, so those parts hold. A few things before merging:

1. An installed CLI now refuses what the server already accepts (main concern)

The one way it can be wrong is by being older than the server, which the daily sync keeps short.

The daily sync keeps this repo close to the server. It does nothing for a CLI that is already installed: an npm install, or a version pinned somewhere downstream, stays on its bundled spec until someone upgrades. That lag is often days.

  • Before: when the API learns a new field type, enum value or body key, an older CLI just sends it and the server accepts it.
  • After: the older CLI refuses it locally (exit 5) and the request never leaves. That is the same class of bug this PR fixes for view create --type form / form create --scene, now widened from a few flags to every JSON body.
  • The CLI also contradicts itself: jinshuju field types asks the server and lists the new type, but form create with that type is refused by the bundled spec.
  • It also blocks pointing the CLI at a test environment whose API is ahead of the published spec, which is how backend changes get tried from the CLI before release.

Suggestion: add an escape hatch (an env var such as JINSHUJU_NO_SPEC_CHECK=1 and/or --no-validate). Alternatively, refuse locally only what is certainly wrong (type, required) and let the server judge vocabulary (enum, discriminator, additionalProperties).

2. Internal details in a public repo

  • The PR description, both commit bodies and a comment in src/spec.ts (the call() doc comment) name the internal backend repository and its validation module. This repo is public and these end up in the commit history, so please describe it generically, e.g. "the way API v1 validates requests against the same openapi.yaml".
  • The commit footers read Refs: T17509; the convention here is Refs: #T17509. The card number in the description body ("The goal (T17509) …") can go, too.

3. Smaller points

  • Validate both requests before sending either. form create / form edit with exam_setting / evaluation_setting send two requests, and the second is only validated by call() right before it is sent. If the settings block is invalid locally, the form has already been created (or half-edited) and the user gets the "first half landed" error. Since validation is now local and free, both bodies can be checked before the first request, which removes that partial state for a whole class of mistakes.
  • Sync PR title. Any change to src/generated produces feat: sync with openapi.yaml, which means a minor release each time and a CHANGELOG full of identical lines. Consider building the title from spec-diff.mjs's output (e.g. the operations the CLI uses that changed), or at least expect a human to retitle before squash.
  • Spec read at import time. bodyChoices('createForm', 'scene'), choices(...) in trade.ts / entry.ts and similar run at module load, so a spec that drops one of those enums crashes every command, --help included. The sync PR's tests would catch it, so low risk; just noting it.
  • spec-sync.yml has not run on GitHub yet. Worth a manual workflow_dispatch right after merge to confirm the token, the label and the no-change path.

@warmwind

warmwind commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

I agree with the direction. Calling by operationId, failing the generator on an unknown keyword, the CI drift check, holding every request in the suite to the spec, and reading flag choices from the spec are all good. They also fix real bugs: view --type form, --scene quickpay, and the paid_status wire format.

One part needs to change before merge: the local JSON body check (validate in call()).

Problem: when the API accepts more, installed CLIs start to fail

Every released CLI version freezes the spec it was built with. The spec has 159 additionalProperties: false and a closed enum of field types. When the API adds a field type, a key or an enum value, every older CLI refuses the request locally (exit 5). The request never leaves the machine, and the user has no way around it.

Reproduced with a build of this PR (dummy token, no network request):

$ jinshuju form create --json '{"name":"x","fields":[{"type":"TextField","label":"a","future_option":true}]}'
the request body is not one POST /api/v1/forms takes (createForm in openapi.yaml): /fields/0/future_option is not a key this takes
exit=5

So "never stricter than the server" is true only while both sides have the same spec. Over time, an old client is always stricter than the API.

The daily sync shortens how far the repo lags behind the API. It does not shorten how long users take to upgrade. A new capability works only after the whole chain is done:

API change → sync PR → merge → release → npm publish → every client upgrades

Our own clients can upgrade quickly. External users cannot follow our schedule. They need a CLI that keeps working, not one that starts to fail the day the API ships a change. The error also says "openapi.yaml does not take this", which misleads both people and models.

Why the local body check does not pay for itself

The API already validates every request against the same openapi.yaml, and its 400 has the same shape as the one here (error_description + errors[{pointer, message, code}]). The local check saves one round trip. In exchange, it ties every client's release schedule to the API's. The only spec that is never out of date is the one the API runs, and the API already checks against it.

Suggestions

  1. Remove the local JSON body check and let the API decide. An old CLI then sends --json as is, and new types and keys work as soon as the API ships them.
  2. Keep the path/query checks and the flag choices. They cover only the flags the CLI itself offers. A new flag needs a CLI release anyway, so these checks never make an old version stricter.
  3. With the body check gone, most of the ~300-line validator in spec.ts (validate / objectIssues / discriminated / closest) can go too. The scalar parameter check that remains is small. operationOf is used only by tests; move it to a test helper. The conform() hook in the tests can stay, to check the requests the CLI builds itself.

Smaller items:

  • For a wrong field type, the error lists 12 of the 52 types and then says "and 40 more". Suggestion 1 removes this message. If the body check stays, add a pointer to jinshuju field types.
  • upload.ts checks the local file path as file against the multipart schema, so that pass means nothing. A TS literal type for image_type is enough.
  • The second commit removes the required-keys check that the first commit added to payload.ts. Fold that hunk back into the first commit.

@yiichou
yiichou force-pushed the yiichou/feat-qunli-t17509 branch from ac03c79 to e7330eb Compare October 15, 2026 09:23
@yiichou

yiichou commented Oct 15, 2026

Copy link
Copy Markdown
Author

Thanks — all taken. Pushed as a new commit on top, with the two earlier commits reworded (force-pushed; the code in them is unchanged).

1. An installed CLI refusing what the server accepts. Went with your second option, so no escape hatch is needed: only what no version of the API would take is refused locally — a wrong type, a missing required key. A value outside an enum, a key additionalProperties does not list, an unknown discriminator tag, or a value past a maximum / maxLength / maxItems is sent for the server to judge. A oneOf / anyOf miss counts as certain only when every branch refuses for certain (a field with no label). The same rule now applies to path and query, so folder list --limit 500 goes to the server too. A test environment ahead of the published spec and field types listing a new type both work again.

Flag choices read from the spec (--scene, --type, --status…) still refuse values the bundled spec does not list, as the hand-written lists did before this PR. Happy to make those advisory too if you'd rather.

2. Internal details. The doc comment, ARCHITECTURE.md, the PR description and both earlier commit bodies now describe it as "the way API v1 validates requests against the same openapi.yaml". Footers are Refs: #T17509, and the card number is out of the description text.

3. Smaller points.

  • Both requests are built and checked before either is sent in form create / form edit with a settings block.
  • The sync PR title comes from spec-diff.mjs --title: feat: follow openapi.yaml changes to createForm, updateForm and 2 more when an operation the CLI calls changed, a chore (no release) otherwise. While at it, an operation now counts as changed when any schema it refers to changed, not only its own definition.
  • choices / bodyChoices no longer throw at load: a missing list leaves the flag open and is recorded, and a test fails on it, so the sync PR is where it shows up.
  • spec-sync.yml: agreed, I'll run it by hand once this is merged (workflow_dispatch only works from the default branch).

spec/openapi.yaml is a verbatim copy of the published API v1 spec, and
`npm run generate` turns it into src/generated/operations.ts: every
operation's method, path, parameters and request-side schemas, prose
stripped. Commands name an operation instead of a path, through call(),
which checks path and query against the operation before sending (an
undeclared or missing parameter, a value not of its type) and spells a
list the way the spec says. The values a flag offers are read from the
spec too; a list the spec stops giving leaves the flag open instead of
crashing every command, and a test fails on it.

A body goes as given: the API validates it against the openapi.yaml it
runs, and a local copy would make every installed CLI refuse what the
API learns after it ships. The tests hold every request the suite sends,
and every example in the help, to the spec instead, body included, with
a check that is not shipped.

A daily workflow fetches the spec, regenerates, runs check and test, and
opens a pull request listing the operations that moved and which the
CLI calls; it is a feat naming those, or a chore when none is. CI fails
if src/generated drifts from the spec. Which commands exist is still the
CLI's decision.

Following the spec changed what some commands accept:
- view create --type takes form; form create --scene takes form and
  quickpay; table create lists RelationField
- transaction list/aggregate send paid_status comma-joined
- refused before sending: entry list --view with --limit, entry count of
  several containers with --keyword, a serial number that is not one
- the form edit example used exam_setting.total_score, which the API
  does not take; it now uses limited_time

Refs: #T17509

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@yiichou
yiichou force-pushed the yiichou/feat-qunli-t17509 branch from e7330eb to 8b40b9f Compare October 15, 2026 09:31
@yiichou

yiichou commented Oct 15, 2026

Copy link
Copy Markdown
Author

Agreed on all of it — done, and squashed into one commit (force-pushed).

  1. The local JSON body check is gone. call() sends --json as given; your future_option repro is now a test that the request leaves. InvalidInputError went with it, as did the "definite" rule and the both-requests pre-check from the previous round, which only existed to serve the body check.
  2. Path/query checks and flag choices stay. The parameter check is now just: declared, required, and of its type — no ranges or enums, so --limit 500 and a new status value go to the server like a body does.
  3. The validator moved out of spec.ts into src/testing/conformance.ts with operationOf, and dist/testing is excluded from the package. The conform() hook and the help-example test use it, which is what still catches CLI-built bodies such as the old total_score example.

Smaller items:

  • The "and 40 more" message went with the body check.
  • upload.ts no longer checks multipart; image_type is a TS literal type (FORM_IMAGE_TYPES), and a test holds that list to the spec's enum.
  • payload.ts is back to what main has; there is no add-then-remove in the history any more.

@kba977 this supersedes item 1 of my previous reply: rather than refusing only certain errors, the body is not checked locally at all. The sync PR title, the non-fatal choices lookups and the transitive change detection are still in.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants