Repository navigation
Conversation
| 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
left a comment
There was a problem hiding this comment.
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:
- The title is one slice ahead of the code. "give an iterate job's goal one line" is what ships —
preflight()is called only fromkind === 'iterate'(src/jobs/service.ts:1112), anddocs/architecture.mdsays "Iterate jobs … add the temporal delta line". The owning card says "every job"; slice 1 does not, and the PR title should not either. src/jobs/service.ts:937— the window is 20 rows.listJobsForRepo(repoId, 20).find(...)saysSince: no previous runwhen the 20 newest rows are unfinished or predatebase_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.- Body and docs say "two bounded reads"; the code makes three requests per iterate enqueue —
compareCommits(:941),getCommitMeta(:947) andlistMergedPullRequestsSince(: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 oknode scripts/pr-lint.mjs --pr 842— ok (#842, T3, 7 files)node scripts/changelog.mjs check— 66 fragments oknode scripts/version.mjs check— 0.3.6 oknode scripts/secret-scan.mjs— no suspected value in the tracked filesnpx 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.
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.tsis 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.Since: no previous runand makes neither read.Since 4f2c1a9: 3 commits ahead, 2 merged PRs (#812, #815); numbers are deduplicated, sorted and capped at five with an ellipsis.delta unavailable (partial).merged PRs partialand 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.sinceShais thebase_shaof the newest finished job for the same repository, and only when one exists; then two bounded reads run —compareCommits(sinceSha, baseSha)and, throughgetCommitMeta(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_jsonstoressinceSha,status,aheadBy,mergedPrsandcomplete. The column is additive: every row written before it reads asNULL, and the job page renders nothing for aNULLor unparsable value instead of failing.Risk and rollback
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.temporal_jsonis 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/architecture.mddocuments the line, the two reads, the partial rule and the column. Changelog fragmentchangelog.d/366-temporal-delta.added.mdcovers the user-visible line.Red half
test/temporal.test.tsat the unmodified tip9f2ef3aa9b0cb00446f6cd0a25325d2efc2fe3bffails 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 fortemporal_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 oknode scripts/risk-gate.mjs --base 9f2ef3aa9b0cb00446f6cd0a25325d2efc2fe3bf --head HEAD --no-fetch— T3, basissrc/db.tsnode scripts/changelog.mjs check— 66 fragments oknode scripts/version.mjs check— 0.3.6 oknode scripts/secret-scan.mjs— no suspected value in the tracked filesnpx tsx --test test/temporal.test.ts— 12/12 at this tip, and red at9f2ef3aaas aboveNot run: Docker, a live GitHub compare against a real repository (the two reads are bounded GETs that
src/repos/github.tsalready covers), and any production path.The delta drives nothing: no decision, refusal, priority or merge reads it in this slice.