Skip to content

feat: give an iterate job's goal one line of temporal context (#366) - #842

Open
HugoXK wants to merge 1 commit into
mainfrom
dev1/366-temporal-contract
Open

HugoXK wants to merge 1 commit into
mainfrom
dev1/366-temporal-contract

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 15, 2026

Copy link
Copy Markdown
Owner

Written-by: HugoXK

Fixes #366

Slice 1 of #366: an iterate job records what changed since the previous run and says it in one line. Nothing reads the delta to make a decision yet — this slice only records and displays it.

What changed

src/lib/temporal.ts is a new pure module: summariseDelta() turns a delta into one line, so the wording is decided by inputs rather than by whatever the reads returned.

  • A first run on a repository says Since: no previous run and makes neither read.
  • A normal delta reads Since 4f2c1a9: 3 commits ahead, 2 merged PRs (#812, #815); numbers are deduplicated, sorted and capped at five with an ellipsis.
  • A compare that failed says delta unavailable (partial).
  • A merged-PR walk that hit its page cap says merged PRs partial and prints no count at all, rather than the smaller number that fit inside the pages it did read. That is the rule the issue asked for: an understated delta is worse than one that admits it is incomplete.

preflight() (src/jobs/service.ts) now returns the delta too. sinceSha is the base_sha of the newest finished job for the same repository, and only when one exists; then two bounded reads run — compareCommits(sinceSha, baseSha) and, through getCommitMeta(sinceSha).committedAt, listMergedPullRequestsSince(). Both were added as the read half of #760 and were unused from the dispatch path until now.

The goal gains exactly one data line, and jobs.temporal_json stores sinceSha, status, aheadBy, mergedPrs and complete. The column is additive: every row written before it reads as NULL, and the job page renders nothing for a NULL or unparsable value instead of failing.

Risk and rollback

  • Tier: T3, basis src/db.ts (node scripts/risk-gate.mjs --base 9f2ef3aa --head HEAD --no-fetch → risk-tier: T3; basis: src/db.ts), 7 files +278/−7.
  • Rollback: revert this commit. temporal_json is additive and unread by any other path, and the goal line disappears with the code that writes it, so a revert needs no data migration.
  • Docs: docs/architecture.md documents the line, the two reads, the partial rule and the column. Changelog fragment changelog.d/366-temporal-delta.added.md covers the user-visible line.

Red half

test/temporal.test.ts at the unmodified tip 9f2ef3aa9b0cb00446f6cd0a25325d2efc2fe3bf fails before any implementation: ERR_MODULE_NOT_FOUND: Cannot find module '.../src/lib/temporal.js' imported from test/temporal.test.ts. The same file passes at this tip. The suite also pins the wording, the singular/plural forms, the partial rule (and that a capped walk prints no count), the sort/deduplicate/ellipsis behaviour, a store round trip for temporal_json, and that the job page renders nothing for a missing or broken value.

Ran:

  • npm run check — exit 0, 2204/2204 tests, lint 11 baseline warnings, typecheck clean, build ok
  • node scripts/risk-gate.mjs --base 9f2ef3aa9b0cb00446f6cd0a25325d2efc2fe3bf --head HEAD --no-fetch — T3, basis src/db.ts
  • node scripts/changelog.mjs check — 66 fragments ok
  • node scripts/version.mjs check — 0.3.6 ok
  • node scripts/secret-scan.mjs — no suspected value in the tracked files
  • npx tsx --test test/temporal.test.ts — 12/12 at this tip, and red at 9f2ef3aa as above

Not run: Docker, a live GitHub compare against a real repository (the two reads are bounded GETs that src/repos/github.ts already covers), and any production path.

The delta drives nothing: no decision, refusal, priority or merge reads it in this slice.

Comment thread src/jobs/service.ts
const since = this.store.listJobsForRepo(repoId, 20).find((job) => job.finished_at !== null && typeof job.base_sha === 'string' && job.base_sha !== '')
const sinceSha = since?.base_sha ?? null
if (sinceSha === null) return { sinceSha: null, status: 'ahead', aheadBy: 0, mergedPrs: [], complete: true }
let status: TemporalStatus = 'unknown'

@HugoXK-bot HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES at 07bf4d2
Written-by: HugoXK-bot
Posted-by: HugoXK-bot

Read the tip 07bf4d25bb0f741790f09c0f39abf67524b209bb (7 files, +278/−7; tree 18c6d268223fb2d1a7f365aab75fe608ad88b480, all 716 blobs hashed against the API tree). The implementation is careful and the pure half is genuinely tested — but the card's own red test has two halves, and only one of them ships.

Blocking: the dispatcher half of #366's red test is missing. The issue's Red test section names it explicitly: "plus a dispatcher case in test/jobs.test.ts with the existing fake client" — (a) a repository whose previous job recorded base_sha produces a job whose temporal_json.sinceSha equals that value and whose goal contains the line, and (b) a first job produces Since: no previous run and the fake client records zero compare calls. Neither assertion exists: grep -rn temporal test/ | grep -v temporal.test.ts is empty, and the whole new suite is test/temporal.test.ts — 12 cases over summariseDelta, the store round trip and jobPage. That leaves the only non-pure code in this slice untested: the sinceSha selection at src/jobs/service.ts:934-937, the goal append at :1131-1133, the temporal_json write at :1235-1238, and the first-run rule that makes no read at all. A listJobsForRepo ordering or window mistake, or the column never being written on the dispatch path, passes the current suite. The harness to fix it already exists: test/jobs.test.ts:1898, :1934, :1971 and :1996 build Jobs with a { name: 'flybot' } runner (needed, because preflight() returns early for any other runner at src/jobs/service.ts:900), and :196 already stands up a fake GitHub server with config.github.apiBase — serve /compare/…, /commits/… and the closed-PR walk there and count the /compare/ requests.

Three non-blocking findings, all worth one line each while the test is being added:

  1. The title is one slice ahead of the code. "give an iterate job's goal one line" is what ships — preflight() is called only from kind === 'iterate' (src/jobs/service.ts:1112), and docs/architecture.md says "Iterate jobs … add the temporal delta line". The owning card says "every job"; slice 1 does not, and the PR title should not either.
  2. src/jobs/service.ts:937 — the window is 20 rows. listJobsForRepo(repoId, 20).find(...) says Since: no previous run when the 20 newest rows are unfinished or predate base_sha, even though an older finished job exists. That is the same window the rest of the console uses, so it may be acceptable — but the card says "the newest finished job" with no limit, and the difference is a false "no previous run" on a busy repository. Name the limit in the comment, or pick the row with a query that has no window.
  3. Body and docs say "two bounded reads"; the code makes three requests per iterate enqueue — compareCommits (:941), getCommitMeta (:947) and listMergedPullRequestsSince (:949). Say "two reads plus the commit lookup", since the budget line in slice 4 depends on the number being right.

Two smaller notes: temporalLine() (src/views/pages.ts:2995) only checks that status is a string, so a parsable but malformed row renders undefined commits ahead instead of nothing — the body's claim is only true for unparsable values; and the line is appended before .slice(0, 4000) (src/jobs/service.ts:1133), so a goal near the cap silently loses it.

Ran (exact tip tree; github.com:443 is unreachable from this box, so the tree came from the API tarball and every blob was hash-checked):

  • npm run check — lint 11 baseline warnings, typecheck clean, 2204/2204 tests, build ok
  • node scripts/pr-lint.mjs --pr 842 — ok (#842, T3, 7 files)
  • node scripts/changelog.mjs check — 66 fragments ok
  • node scripts/version.mjs check — 0.3.6 ok
  • node scripts/secret-scan.mjs — no suspected value in the tracked files
  • npx tsx --test test/temporal.test.ts — 12/12 pass (the pure half is real)

Not run: Docker, a live compare against a real repository, the release-readiness callers that already use these reads.

Conclusion: REQUEST_CHANGES — the design and the pure half are right, but the card's dispatcher test is the part that proves the delta is actually built from the previous run and stored, and it is absent. Add it on the same branch and I judge the new tip.

@HugoXK-bot HugoXK-bot added the review:changes Issue needs changes before it can be claimed label Nov 15, 2026

This branch has not been deployed

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

Labels

review:changes Issue needs changes before it can be claimed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: give every job a temporal repository contract, including what changed since the last run

3 participants