[PR #162] [MERGED] feat(jobs): surface the job timestamps, progress and queue position on Job #161

New issue

Closed

opened 2026-10-31 22:05:12 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:05:12 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/162
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15898-job-timestamp-accessors

📝 Commits (6)

ab6eac7 feat(jobs): surface the job timestamps, progress and queue position on Job
ba133aa Merge remote-tracking branch 'origin/main' into matt/be-15898-job-timestamp-accessors
7ac3186 fix(jobs): read an omitted wire field as none, and de-duplicate the changelog heading
ba790da fix(jobs): read an unusable wire field as none, and announce a missing required one
2354f64 Merge origin/main into matt/be-15898-job-timestamp-accessors
c8445e7 fix(jobs): return the events Progress from job.progress, matching the Python SDK

📊 Changes

7 files changed (+427 additions, -11 deletions)

View changed files

📝 CHANGELOG.md (+19 -0)
📝 README.md (+34 -0)
📝 src/sdk/events.test.ts (+10 -1)
📝 src/sdk/events.ts (+8 -1)
📝 src/sdk/jobs.test.ts (+200 -1)
📝 src/sdk/jobs.ts (+142 -3)
📝 test/support/stub-server.ts (+14 -5)

📄 Description

ELI-5

A Job knew far more about itself than it would tell you. The server sends back created/started/completed/expires timestamps, a progress snapshot, a queue position, run metrics and the job's own links — and the handle kept all of it in a private field while exposing only id, status, outputs and error. So the most obvious question you have after a run — how long did that take? — had no answer on the public type, and callers were reaching past private with a cast to get at it. This adds the missing getters, so the answer is job.completedAt minus job.startedAt.

What changed

Eight read-only accessors on Job, in the same one-line-getter style as id/status:

Accessor	Type
`createdAt`, `expiresAt`	`Date`
`startedAt`, `completedAt`	`Date \| null` (both are nullable on the wire)
`progress`	the generated `Progress \| null`
`queuePosition`	`number \| null`
`metrics`	`Record<string, number \| null> \| undefined`
`urls`	the generated `JobUrls`

createdAt, expiresAt and urls are non-nullable because the spec's required list makes those fields required and non-nullable; see the contract note below for what happens if a server breaks that.

model stays private — these are views onto it, and they read whatever state the handle currently holds, the same contract the existing getters document. Nothing re-fetches implicitly.

Timestamps are returned as Date rather than the raw ISO string or an epoch number, so the type says what the value is. The object-valued three (progress, metrics, urls) hand back a snapshot copy: refresh, cancel, events and getLogs all navigate by model.urls.*, so returning the live object from a getter documented as read-only would have let a caller rewrite the address the handle polls and cancels through. There is a test that fails if the copy is removed.

metrics distinguishes absent from empty — it is optional on the wire, and a surface that measures nothing sends no key at all, which reads as undefined rather than {}.

Off-contract values are split the way the spec's required list splits the fields. started_at, completed_at, queue_position and progress are required-but-nullable, so "none" is a real answer and absent-or-unusable reads as none however the server spells it: the generated type declares exactly one absent-value per field, but testing for only that one let the other fall through to the value branch — toDate(undefined) is an Invalid Date, which compares false against everything instead of announcing itself, queue_position returned undefined through a declared number | null so a caller's !== null guard passed and the value entered arithmetic as NaN, and spreading an absent progress yielded a {} typed as a Progress with no value or nodes_total. toDate also rejects a non-string or an unparseable string, since the transport does JSON.parse(text) as T with no runtime validation and an Invalid Date is truthy — it would slip past the if (job.startedAt && job.completedAt) guard the README documents. This matches the defensive ?? null the SSE side already applies to these same fields in events.ts.

created_at, expires_at and urls are required and non-nullable, so there is no "none" for them to mean and softening them would invent one callers must then check forever. Reading one the server did not send raises ComfyError with code: "unexpected_response", naming the field — the same way the models routes already report a response that breaks its contract. That beats the alternatives: new Date(undefined) compares false against everything, new Date(null) is the Unix epoch (a retention check reading a live job as expired in 1970), and { ...undefined } is a {} typed as a full JobUrls handing back undefined for a self the type promises is a string. A spec-conformant response is unaffected either way.

The stub server grows one knob, jobFieldOverrides, shallow-merged over the GET /jobs/{id} body only (not POST /jobs, not the cancel response) so a test can prove a handle adopted this state on refresh() rather than having held it since submit.

Docs: a new What a job carries section in the README lists every accessor with its type, shows the duration snippet with no cast, and flags the one real trap — job.progress is the generated snake_case wire model, not the camelCase Progress event of the same name that job.events() yields. Changelog entry under Unreleased.

Verification of the report

The report is about the published package, so I checked the published package rather than only my own diff: npm pack @comfyorg/sdk@0.4.0 and reading package/dist/sdk/jobs.d.ts shows private model; plus exactly four getters (id, status, outputs, error) — the cast the report describes is the only way to reach started_at on that artifact, and it targets the real private field name. After this change, dist/sdk/jobs.d.ts carries all eight new getters. A repo-wide sweep for the private model cast pattern found 0 remaining occurrences in this repo (the reported cast lives in consumer code, which is outside this repo and unswept).

Provenance

Authored by: agent-work loop
Verified:
- Merged result with origin/main at 9d2fa64 (job labels, #200, and credits-used, #157): pnpm test 699 passed, 5 skipped, 0 failed
- pnpm lint, pnpm format:check and pnpm typecheck are clean, and pnpm check:spec-drift passes both of its checks
- node scripts/check-public-repo-hygiene.mjs: OK
- Earlier round: a mutation check (restoring the previous src/sdk/jobs.ts) reddened exactly the 7 absent/unusable/required-field tests
- node scripts/sync-python-surface.mjs fails with the same RouterRunResult ExtractionError on a clean main, see Residual
Deviations:
- None against the accessor list
- Review rounds added the absent-value handling and the split between required and nullable fields described above, and collapsed a duplicate ### Added changelog heading
- Merging main resolved conflicts in jobs.ts, the stub server, README and CHANGELOG by keeping both sides. job.metadata from #200 now sits alongside the new accessors and is listed in the README's "What a job carries" table, and the stub's jobFieldOverrides now merges over the metadata-aware job body
- The cross-SDK half is one-sided, see Residual

Residual

The Python sibling has not landed, so this is a one-sided addition. The Python SDK's Job needs the same fields under the same names (created_at, started_at, completed_at, expires_at, progress, queue_position, metrics, urls) before the two surfaces match. I checked what that costs today: the parity harness in src/sdk/surface-parity.test.ts compares the models method names, the router error class names, their error_type mapping, the status→bucket fallback table and the retry-policy fields — it does not introspect Job accessors at all, and parity/python-surface.json carries no Job entry. So pnpm test stays green on this branch and no INTENTIONAL_ASYMMETRIES or *AheadOfPython entry was needed. The gap is therefore invisible to CI, which is the part worth tracking: a human should land the Python twin, and whoever does should consider extending the parity extractor to cover Job so the next one-sided addition is caught.

The sdk-parity CI job is already red on main, and this branch does not fix it. node scripts/sync-python-surface.mjs fails with ExtractionError: src/comfy_sdk/models.py: unrecognized public class(es) RouterRunResult. I reproduced it on a clean main checkout with no local changes, so it is pre-existing Python-side drift, unrelated to this diff, and the fix is the one AGENTS.md §6 describes (teach PYTHON_MODELS_CLASSES in scripts/python-surface.mjs about the new class, then re-run pnpm sync:python-surface). Deliberately left alone here to keep this diff scoped — and #167 (fix(parity): track the Python SDK's refactors) already carries that fix and is approved, so this branch's sdk-parity should go green once #167 lands.

job.progress on Comfy Cloud is documented from a report, not from a measurement. The vendored contract (spec/openapi.yaml) says the poll returns the latest snapshot, "same data the SSE stream pushes"; the report is that Comfy Cloud sends null there even for a running job. I could not falsify or confirm that: there are no Comfy credentials in this environment, and the only way to observe a running job's poll response is to submit a real paid generation, which is a mutating and billable action I did not take. The accessor ships regardless and denies nothing — it returns whatever the server sends — and both the README and the JSDoc attribute the Cloud behaviour rather than asserting it, and point at job.events(), which is a working path for live progress on every surface. If the server gap is fixed, nothing here needs to change. If someone with Cloud access confirms it, the attribution can become a statement.

Retention timestamps are now typed two different ways across the SDK. Job.expiresAt returns a Date (what was asked for, and what Output.getDownloadUrl()/Asset.getDownloadUrl() already return for the signed-URL expiry), while Asset.expiresAt still returns the raw string | undefined. Both are retention deadlines and a reader will reasonably expect them to match. I did not change Asset — it is a separate public surface, changing its return type is a breaking change, and it is outside what this asked for — but the inconsistency is real and someone should decide which way it goes.

Summary by CodeRabbit

New Features
- Added access to job timestamps, progress, queue position, metrics, and URLs.
- Progress details now include the current node’s class.
- Object-valued job details are returned as snapshots, and refreshed job data is reflected by accessors.
- Missing or unusable required job fields now produce a clear response error.
Documentation
- Added guidance on job details, refreshing state, progress snapshots, and calculating run duration.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/162 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15898-job-timestamp-accessors` --- ### 📝 Commits (6) - [`ab6eac7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/ab6eac7afea2a9e5360a1ae380fc9bbd7230df01) feat(jobs): surface the job timestamps, progress and queue position on Job - [`ba133aa`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/ba133aad0471dc0d8af4083c39a4fde6d39cf130) Merge remote-tracking branch 'origin/main' into matt/be-15898-job-timestamp-accessors - [`7ac3186`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7ac31865a8e19b713c372ceef36166b1b407fc30) fix(jobs): read an omitted wire field as none, and de-duplicate the changelog heading - [`ba790da`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/ba790da6aa655aba81b89175050c9cb570f2b64a) fix(jobs): read an unusable wire field as none, and announce a missing required one - [`2354f64`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/2354f644bdd8ccb87bd87d33d4de1b2a657fe9c6) Merge origin/main into matt/be-15898-job-timestamp-accessors - [`c8445e7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/c8445e7e254081e470a8483ceed87ee5725a2f5d) fix(jobs): return the events Progress from job.progress, matching the Python SDK ### 📊 Changes **7 files changed** (+427 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+19 -0) 📝 `README.md` (+34 -0) 📝 `src/sdk/events.test.ts` (+10 -1) 📝 `src/sdk/events.ts` (+8 -1) 📝 `src/sdk/jobs.test.ts` (+200 -1) 📝 `src/sdk/jobs.ts` (+142 -3) 📝 `test/support/stub-server.ts` (+14 -5) </details> ### 📄 Description ## ELI-5 A `Job` knew far more about itself than it would tell you. The server sends back created/started/completed/expires timestamps, a progress snapshot, a queue position, run metrics and the job's own links — and the handle kept all of it in a private field while exposing only `id`, `status`, `outputs` and `error`. So the most obvious question you have after a run — *how long did that take?* — had no answer on the public type, and callers were reaching past `private` with a cast to get at it. This adds the missing getters, so the answer is `job.completedAt` minus `job.startedAt`. ## What changed Eight read-only accessors on `Job`, in the same one-line-getter style as `id`/`status`: | Accessor | Type | | --- | --- | | `createdAt`, `expiresAt` | `Date` | | `startedAt`, `completedAt` | `Date \| null` (both are nullable on the wire) | | `progress` | the generated `Progress \| null` | | `queuePosition` | `number \| null` | | `metrics` | `Record<string, number \| null> \| undefined` | | `urls` | the generated `JobUrls` | `createdAt`, `expiresAt` and `urls` are non-nullable because the spec's `required` list makes those fields required and non-nullable; see the contract note below for what happens if a server breaks that. `model` stays private — these are views onto it, and they read whatever state the handle currently holds, the same contract the existing getters document. Nothing re-fetches implicitly. Timestamps are returned as `Date` rather than the raw ISO string or an epoch number, so the type says what the value is. The object-valued three (`progress`, `metrics`, `urls`) hand back a **snapshot copy**: `refresh`, `cancel`, `events` and `getLogs` all navigate by `model.urls.*`, so returning the live object from a getter documented as read-only would have let a caller rewrite the address the handle polls and cancels through. There is a test that fails if the copy is removed. `metrics` distinguishes absent from empty — it is optional on the wire, and a surface that measures nothing sends no key at all, which reads as `undefined` rather than `{}`. Off-contract values are split the way the spec's `required` list splits the fields. `started_at`, `completed_at`, `queue_position` and `progress` are required-but-**nullable**, so "none" is a real answer and absent-or-unusable reads as none however the server spells it: the generated type declares exactly one absent-value per field, but testing for only that one let the other fall through to the value branch — `toDate(undefined)` is an `Invalid Date`, which compares false against everything instead of announcing itself, `queue_position` returned `undefined` through a declared `number | null` so a caller's `!== null` guard passed and the value entered arithmetic as `NaN`, and spreading an absent `progress` yielded a `{}` typed as a `Progress` with no `value` or `nodes_total`. `toDate` also rejects a non-string or an unparseable string, since the transport does `JSON.parse(text) as T` with no runtime validation and an `Invalid Date` is truthy — it would slip past the `if (job.startedAt && job.completedAt)` guard the README documents. This matches the defensive `?? null` the SSE side already applies to these same fields in `events.ts`. `created_at`, `expires_at` and `urls` are required **and non-nullable**, so there is no "none" for them to mean and softening them would invent one callers must then check forever. Reading one the server did not send raises `ComfyError` with `code: "unexpected_response"`, naming the field — the same way the `models` routes already report a response that breaks its contract. That beats the alternatives: `new Date(undefined)` compares false against everything, `new Date(null)` is the Unix epoch (a retention check reading a live job as expired in 1970), and `{ ...undefined }` is a `{}` typed as a full `JobUrls` handing back `undefined` for a `self` the type promises is a `string`. A spec-conformant response is unaffected either way. The stub server grows one knob, `jobFieldOverrides`, shallow-merged over the `GET /jobs/{id}` body **only** (not `POST /jobs`, not the cancel response) so a test can prove a handle *adopted* this state on `refresh()` rather than having held it since submit. Docs: a new **What a job carries** section in the README lists every accessor with its type, shows the duration snippet with no cast, and flags the one real trap — `job.progress` is the generated snake_case wire model, not the camelCase `Progress` *event* of the same name that `job.events()` yields. Changelog entry under Unreleased. ## Verification of the report The report is about the published package, so I checked the published package rather than only my own diff: `npm pack @comfyorg/sdk@0.4.0` and reading `package/dist/sdk/jobs.d.ts` shows `private model;` plus exactly four getters (`id`, `status`, `outputs`, `error`) — the cast the report describes is the only way to reach `started_at` on that artifact, and it targets the real private field name. After this change, `dist/sdk/jobs.d.ts` carries all eight new getters. A repo-wide sweep for the `private model` cast pattern found **0** remaining occurrences in this repo (the reported cast lives in consumer code, which is outside this repo and unswept). ## Provenance - **Authored by:** agent-work loop - **Verified:** - Merged result with `origin/main` at `9d2fa64` (job labels, #200, and credits-used, #157): `pnpm test` 699 passed, 5 skipped, 0 failed - `pnpm lint`, `pnpm format:check` and `pnpm typecheck` are clean, and `pnpm check:spec-drift` passes both of its checks - `node scripts/check-public-repo-hygiene.mjs`: OK - Earlier round: a mutation check (restoring the previous `src/sdk/jobs.ts`) reddened exactly the 7 absent/unusable/required-field tests - `node scripts/sync-python-surface.mjs` fails with the same `RouterRunResult` ExtractionError on a clean `main`, see Residual - **Deviations:** - None against the accessor list - Review rounds added the absent-value handling and the split between required and nullable fields described above, and collapsed a duplicate `### Added` changelog heading - Merging `main` resolved conflicts in `jobs.ts`, the stub server, README and CHANGELOG by keeping both sides. `job.metadata` from #200 now sits alongside the new accessors and is listed in the README's "What a job carries" table, and the stub's `jobFieldOverrides` now merges over the metadata-aware job body - The cross-SDK half is one-sided, see Residual ## Residual **The Python sibling has not landed, so this is a one-sided addition.** The Python SDK's `Job` needs the same fields under the same names (`created_at`, `started_at`, `completed_at`, `expires_at`, `progress`, `queue_position`, `metrics`, `urls`) before the two surfaces match. I checked what that costs today: the parity harness in `src/sdk/surface-parity.test.ts` compares the `models` method names, the router error class names, their `error_type` mapping, the status→bucket fallback table and the retry-policy fields — it does **not** introspect `Job` accessors at all, and `parity/python-surface.json` carries no `Job` entry. So `pnpm test` stays green on this branch and no `INTENTIONAL_ASYMMETRIES` or `*AheadOfPython` entry was needed. The gap is therefore invisible to CI, which is the part worth tracking: a human should land the Python twin, and whoever does should consider extending the parity extractor to cover `Job` so the next one-sided addition is caught. **The `sdk-parity` CI job is already red on `main`, and this branch does not fix it.** `node scripts/sync-python-surface.mjs` fails with `ExtractionError: src/comfy_sdk/models.py: unrecognized public class(es) RouterRunResult`. I reproduced it on a clean `main` checkout with no local changes, so it is pre-existing Python-side drift, unrelated to this diff, and the fix is the one `AGENTS.md` §6 describes (teach `PYTHON_MODELS_CLASSES` in `scripts/python-surface.mjs` about the new class, then re-run `pnpm sync:python-surface`). Deliberately left alone here to keep this diff scoped — and #167 (`fix(parity): track the Python SDK's refactors`) already carries that fix and is approved, so this branch's `sdk-parity` should go green once #167 lands. **`job.progress` on Comfy Cloud is documented from a report, not from a measurement.** The vendored contract (`spec/openapi.yaml`) says the poll returns the latest snapshot, "same data the SSE stream pushes"; the report is that Comfy Cloud sends `null` there even for a running job. I could not falsify or confirm that: there are no Comfy credentials in this environment, and the only way to observe a *running* job's poll response is to submit a real paid generation, which is a mutating and billable action I did not take. The accessor ships regardless and denies nothing — it returns whatever the server sends — and both the README and the JSDoc attribute the Cloud behaviour rather than asserting it, and point at `job.events()`, which is a working path for live progress on every surface. If the server gap is fixed, nothing here needs to change. If someone with Cloud access confirms it, the attribution can become a statement. **Retention timestamps are now typed two different ways across the SDK.** `Job.expiresAt` returns a `Date` (what was asked for, and what `Output.getDownloadUrl()`/`Asset.getDownloadUrl()` already return for the signed-URL expiry), while `Asset.expiresAt` still returns the raw `string | undefined`. Both are retention deadlines and a reader will reasonably expect them to match. I did not change `Asset` — it is a separate public surface, changing its return type is a breaking change, and it is outside what this asked for — but the inconsistency is real and someone should decide which way it goes.  ## Summary by CodeRabbit - **New Features** - Added access to job timestamps, progress, queue position, metrics, and URLs. - Progress details now include the current node’s class. - Object-valued job details are returned as snapshots, and refreshed job data is reflected by accessors. - Missing or unusable required job fields now produce a clear response error. - **Documentation** - Added guidance on job details, refreshing state, progress snapshots, and calculating run duration.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:05:12 +00:00