Skip to content

ci: the job schema can express ordering - #1762

Merged
sergey-shandar merged 2 commits into
mainfrom
claude/private-ts-todo-partial-bydyc9
Aug 28, 2026
Merged

ci: the job schema can express ordering#1762
sergey-shandar merged 2 commits into
mainfrom
claude/private-ts-todo-partial-bydyc9

Conversation

@sergey-shandar

Copy link
Copy Markdown
Contributor

First step of Stage 2's packed-artifact check, recorded in #1757. It is the prerequisite the design names, and nothing else in the stage can start without it.

Why this is a prerequisite, not a detail

A job that consumes an artifact must not start before the job that uploads it. The generator could not say so: jobSchema named only runs-on and steps.

Emitting a bare needs: key past the schema is not an option, and the reason is specific to this repository: jobSchema is deliberately closed, and it is the same schema parseGitHubAction reads the generated workflow back through in fjs/ci/proof.f.mjs. An unmodelled key is generator drift by construction, so the round-trip proof would reject it.

Without this, the packed-artifact check could only fail at download-artifact — a required check red for a reason unrelated to what it tests, which is the one failure mode that trains people to hit re-run instead of reading the log.

The change

needs: or(option, array(string)) on jobSchema, matching the optional-field idiom already used by stepSchema. Job widens automatically through Ts<typeof jobSchema>, so fjs/ci/common/types.ts needed no edit.

The generated workflow is unchanged. Nothing orders itself yet, so the field stays absent and npm run ci-update reproduces .github/workflows/ci.yml byte-identically. This lands dormant.

What the proof covers

Not that the field exists — what it is for:

  • an ordered pair round-trips with its dependency intact, which is the property the consuming job depends on;
  • independent jobs still parse with the field absent, so optionality is real;
  • a bare scalar needs: pack is rejected. GitHub accepts that spelling, but this generator emits the list form only, so a scalar is drift rather than an alternative — the same reasoning that makes these schemas closed;
  • no generated job carries the field today, so the first one to do so is a deliberate change rather than an accident.

Ownership

Owned by fjs/ci/todo/ci-integration-tests.md rather than by either consumer. That issue's own two-stage build/integration split needs the same edge, so putting it at the artifact hand-off stops whichever consumer lands first from implementing another issue's task.

Checks

npx tsc clean; full suite 3528/3528; coverage 100%; npm run ci-update produces no diff.

No changelog entry: behavior is unchanged and fjs/ci is this repository's own generator, so this is internal — the entry rules scope release notes to users of the package.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n


Generated by Claude Code

A job that consumes an artifact must not start before the job that uploads it.
The generator could not say so: jobSchema named only runs-on and steps, and it
is deliberately closed, so a `needs:` key emitted past it would fail the
round-trip that parseGitHubAction performs in fjs/ci/proof.f.mjs. Without this,
the packed-artifact check in fjs/ci/todo/f-mjs-package-support.md could only
fail at download-artifact — a required check red for a reason unrelated to what
it tests, which is the one failure mode that trains people to re-run instead of
read.

Adds `needs: or(option, array(string))`, which widens Job through
Ts<typeof jobSchema>. The generated ci.yml is unchanged: nothing orders itself
yet, so the field stays absent.

The proof covers what the field is for rather than that it exists: an ordered
pair round-trips with its dependency intact; independent jobs still parse with
it absent; a bare scalar `needs: pack` is rejected, since GitHub accepts that
spelling but this generator emits the list form only, so a scalar is drift; and
no generated job carries the field today, so the first one to do so is a
deliberate change.

Prerequisite for both fjs/ci/todo/ci-integration-tests.md's two-stage split and
the Stage 2 packed-artifact check, which is why it is owned there rather than by
either consumer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
functionalscript f9cab0e Commit Preview URL

Branch Preview URL
Aug 28 2026, 07:40 PM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cbca4511bf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread fjs/ci/proof.f.mjs Outdated
fjs/AGENTS.md requires a const with a literal initializer to pin its type; the
fixture array relied on tsc's default widening, which drops readonly and the
literal types — exactly what a schema-oriented proof depends on.

The other two consts added here are exempt by the same rule: an arrow function
and a call are not literal initializers and already carry non-widening types.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

needs: or(option, array(string)) is genuinely optional — a job without it validates, one with it validates, and tsc catches the mutant that makes it required, before the runtime proofs even run. jobNeeds also pins optionality independently by unwrapping the needs-less parse, so the guarantee is not only a type-level accident.

Rejecting the bare scalar spelling is a real narrowing against GitHub's string | string[], and the right one here: the generator only ever emits the list form, so a scalar would be drift, not an alternative. Worth having the proof say so, which it does.

ci.yml correctly does not move — the capability is dormant, and the new proof asserts that on the actually-generated workflow rather than assuming it. Regeneration leaves git diff empty. Gates: tsc 0, npm test 3528/3528, zero leaves vanished and exactly one added. No changelog needed, matching five prior fjs/ci-only PRs.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 28, 2026
Merged via the queue into main with commit 2b24a66 Aug 28, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the claude/private-ts-todo-partial-bydyc9 branch August 28, 2026 20:18
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