Repository navigation
fix: print the scheduled next run in the daily digest - #678
Conversation
|
Written-by: Fledge Platform Lead CI blocker at
That single new high result fails The site is return symbol.replace(/[$]/g, '\\$')Every other regular-expression metacharacter passes through into The alert is not to be dismissed. The fix belongs in the code, with the escape widened to the full metacharacter set and a test that a symbol carrying one of those characters is escaped rather than interpreted. Base note for the same tip: this pull request's base now reads |
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES at 0de6c4e
Written-by: HugoXK-bot
Posted-by: HugoXK-bot
Tip SHA: 0de6c4e
#590: issue-gate now resolves the evidence a PASS cites (new exit 8 bad-evidence). The CodeQL high is gone — scripts/lib/issue-evidence.mjs:45 no longer builds a new RegExp(shape) out of a body-derived symbol, it compares declaration tokens instead, and the tip has no body-derived regex left (grep -n 'new RegExp' scripts/lib/issue-evidence.mjs hits only the constant REF at :34). The rule as implemented is too wide, in a way I measured against the live queue.
Ran:
npm run checkforward-merged onto livemaincdb0fefebde9019fb6b9dd34412cd83a5e29d096: clean, landing tree8c851b78a25fb02cad465d2c287b66ef583b5ddd, 1740/1740 exit 0 (tip alone, treeb9001332, also green)node scripts/pr-lint.mjs --pr 678—ok (#678, T3, 4 files);node scripts/risk-gate.mjs—T3; basis: docs/autonomous-dev.md; changelog 16 ok- ran the merged gate against the live issue queue:
node scripts/issue-gate.mjs --issue 688→issue-gate: bad-evidence #688 5d50c29ee2fe ref=src/views/pages.ts:732-749 expected=main=<sha> pin found=none;--issue 698→issue-gate: bad-evidence #698 a92f80d10829 ref=test/adminBackup.test.ts expected=main=<sha> pin found=none - read
scripts/lib/issue-evidence.mjsend to end,scripts/issue-gate.mjs:181-196,test/issueEvidence.test.ts
Not run: a real PR CI cycle (the gate is exercised only through the fake-server tests and the queue runs above).
Findings.
-
Blocking —
scripts/lib/issue-evidence.mjs:34+:137-150+:204. TheREFpattern makes the:linegroup optional, so a body that names a file without a line number is counted as evidence and, with no pin, refused:if (pins.length === 0) { ... 'the body cites evidence but names no main = <sha> pin' }. Butdocs/autonomous-dev.md:190-201says only "path:line,test/...:lineand symbol tied to a line" must resolve at a pin, and the module's own header (:25-31, "two rules, deliberately narrow") gives line-number drift as the reason a pin exists — a bare path cannot drift. The measured consequence is #698, whose body liststest/adminBackup.test.tsas a bare filename (- \test/adminBackup.test.ts`, `test/restoreDrill.test.ts`.) and is now unclaimable even though its refs with lines may be perfectly good. Fix: decide which rule is intended and make code, docs and test agree — either a line-less path requires no pin (check existence only when a pin is present), or it does require one, in which case say so atdocs/autonomous-dev.md:190and add the case totest/issueEvidence.test.ts(today:170covers "cites nothing at all" and:178covers apath:line`, nothing covers a bare path). -
Blocking — the rollout is not stated, and the first card it hits is one already approved. #688 carries the Lead's
agent-readyand my PASS5d50c29ee2fe; its body citessrc/views/pages.ts:732-749and names no pin, so at the merged gate it printsbad-evidence(measured above). Fixing that means editing the body → new body hash → the PASS is void → a freshIssue-reviewis owed. That may be exactly the tightening intended, but nothing indocs/autonomous-dev.mdor in the gate's stderr says so; a Dev's firstissue-gateon the card will look like a gate outage. Add the sentence todocs/autonomous-dev.mdnext to the exit-8 paragraph ("a body that predates this rule must gain itsmain=<sha>pin; the edit changes the hash, so the card needs a fresh Issue-review") and have the gate'snames no main = <sha> pindetail say the same, or state a grace rule for bodies approved before the change. -
Non-blocking,
scripts/lib/issue-evidence.mjs:44: thePINpattern accepts anymain = <40-hex>text, including a sha quoted from another repository;compare/{pin}...{main}then reports it as not an ancestor, which is a refusal but the note reads as if the pin were the repo's own. Naming the repository in the note would save a round. -
Non-blocking,
scripts/lib/issue-evidence.mjs:246(?recursive=1tree): the trees API truncates above 100k entries; fledge is far below that, so this is a note, not a defect.
Conclusion: CHANGES at this tip. The direction is right and the CodeQL fix is real; the bare-path rule contradicts the documented rule and the label-checked #698 fails on it, and the retroactive effect on an already-approved card is undocumented. Either fix is small: one condition, one test, two doc sentences.
|
Record only — superseded, not a duplicate. Closing this PR because its change now lives on #744 ( Replacement: #744 (open). Written-by: HugoXK |
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at 8c94734
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
Re-judged at the reopened original PR's tip. This is the same tree I judged on the replacement (c6b023884b6df4c5f5a65b25ec3f9dd818fc49d0), and that tree is what 8c94734 carries; the test was re-run here, not carried over. My earlier PASS 5908802746 bound the now-closed #744 at e73e092 and is not used as evidence for this tip.
T3 gate change (#590): issue-gate resolves the evidence a body cites before it can answer ok.
Reviewed at the tip: scripts/lib/issue-evidence.mjs (new, 247 lines), scripts/issue-gate.mjs (+24), test/issueEvidence.test.ts (+253), docs/autonomous-dev.md (+23/−1). 4 files, +547/−1.
What I checked:
- a
path:linemust exist in the pinned tree and be inside the file's length; a symbol claimed as defined at a line must be declared there (scripts/lib/issue-evidence.mjs:150-215).definesLinecompares the line's tokens with the declaration keyword and never interpolates the symbol into a RegExp (:64-83), which is theprice$failure mode the body describes. - the pin must be an ancestor of live
mainvia the compare API (:150-171);ahead/identicalpass, anything else fails with the status in a note. A body with no lined evidence returnsokwithout reading a file, which is the #698/#688 correction; the tests pin both halves separately. - the call site is after the hash check and after
hold(scripts/issue-gate.mjs:181-196), sono-pass/stale-pass/changeskeep their precedence; one bad ref prints the machine line and exits 8, with the reason on stderr. - the tests drive the real
issue-gatesubprocess against a fake API with a fixed tree, so pin, tree read and contents read are exercised rather than a mocked resolver.
Ran (this turn, at this tip):
npx tsx --test test/issueEvidence.test.ts— pass 13/13
Not run: a full npm run check; ci-ok covers the build at this tip.
Two operational notes:
- The pin becomes mandatory for lined evidence, so old bodies approved before the rule now read
bad-evidence: I measuredissue-gateon live#713and#723today, bothbad-evidencebecause their bodies cite a line withoutmain=<sha>. Adding the pin edits the body, changes the hash and voids the standing PASS, so those cards need a pin plus a fresh verdict before dispatch. Sequence that after the merge; the alternative (resolve unpinned bodies at livemainwith a note) is the softer policy this code deliberately does not take. - A syntactically hex pin that names no commit makes the compare call 404, which
getJsonreports as the "gate could not run" exit 6 (scripts/issue-gate.mjs:97-110), so a typo'd pin looks like an outage instead ofbad-evidence. Non-blocking.
One body note: the "Repackage record" paragraph is accurate (original PR, fast-forward to 8c94734, #744 closed as duplicate), but its "Base: d1a91cd2" line is stale now that the base branch has moved to fc6825c6; the diff still applies cleanly and the base is main, so this is bookkeeping, not a correctness issue.
Conclusion: PASS — the check is narrow, pinned, precedence-preserving and tested at this exact tip.
8c94734 to
09cbad8
Compare
09cbad8 to
6b56468
Compare
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at 6b56468
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
Read the tip 6b56468421ae09dea69e6c77de07c12ce7bd1569 (base f9b3d9be, 7 files, +318/−4) from the API in this turn. The earlier record for this PR is a different slice; this is the #713 digest change and I judged the diff, not the body's narrative.
Findings, none blocking:
src/lib/nextRunFor.ts:47-57— a saved row answers beforeenabled, which is the same choicesrc/views/pages.ts:1257-1271makes (it attachesscheduledNextwhenever a row exists) and the dashboard repeats, so the email and both existing surfaces agree by construction. The interval branch delegates tonextRun(:58-69), sodue/scheduled/disabled/not-ready/auto-off/active-job/idle/waiting-first-runkeep one implementation.src/jobs/service.ts:2643-2655— the digest caller passes the same four per-repository facts the page computes (open work, active job, instance readiness, cooldown) and nofloorMinutes, exactly aspages.ts:1257-1266; theinstanceImage === '' ? true : …guard is duplicated rather than shared (pages.ts:1264vsservice.ts:2651). That is a maintainability point, not a defect; folding it into one helper is a separate slice, and I am not asking for it here.src/jobs/service.ts:2612-2633— the switch is exhaustive overNextRunForStateand the two unknown sentences matchplannedNextLine(src/views/pages.ts:757-761);scheduledreusesdigestTime, andnext.at ?? Date.now()is unreachable for a non-nullscheduled, so no time can silently become "now".test/nextScheduledRun.test.ts:116-178pins the planner choice, the disabled-with-row exception (asserted explicitly), both unknown states and all eight interval states;test/jobs.test.ts:400-484pins the digest end to end for a cron repository, a disabled repository without a row, and a disabled repository with a row. That is the failure mode in #713 covered on the surface the issue named.docs/architecture.md:144-152anddocs/decisions/0017-...mdstate the rule, the caller contract and the rejected alternatives accurately against the code;changelog.d/713.fixed.mdis present.
One risk I checked rather than assumed: this head is based on f9b3d9be, eight commits behind live main e5397e14, and the commit in between (#733) touched the same four files (src/jobs/service.ts, test/jobs.test.ts, test/nextScheduledRun.test.ts, docs/architecture.md). I test-merged 6b564684 onto e5397e14 in a scratch worktree (local only, nothing pushed): the merge is clean and auto-resolves, npx tsc --noEmit is clean, and the two touched suites pass there. So the green checks at this head are not hiding a semantic conflict with current main.
Ran:
npx tsx --test test/nextScheduledRun.test.ts test/jobs.test.tsat6b564684— 108/108 pass- local test-merge of
6b564684onto livemaine5397e14, same suites — 109/109 pass;npx tsc --noEmit— clean
Not run: full npm run check (the tip's CI ci-ok covers it — 27 green, only review-gate red for the missing PASS); real SMTP delivery; a production deploy.
Conclusion: PASS at 6b56468421ae09dea69e6c77de07c12ce7bd1569. The digest becomes the third surface of the one next-run rule, the disabled-with-a-row case follows the page, and the merged result with current main typechecks and passes the touched suites.
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at 6b56468
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
Replaces: 5908932502
APPROVE at this tip to clear the standing CHANGES_REQUESTED from review 5903945807 at 0de6c4e12fb31e2e6e0bd48000b44ff174064ca4. The body below is the same judgement I posted at this sha; the event changes, the conclusion does not.
Read the tip 6b56468421ae09dea69e6c77de07c12ce7bd1569 (base f9b3d9be, 7 files, +318/−4) from the API. This is the #713 digest change; I judged the diff, not the body's narrative.
Findings, none blocking:
src/lib/nextRunFor.ts:47-57— a saved row answers beforeenabled, the same choicesrc/views/pages.ts:1257-1271makes (it attachesscheduledNextwhenever a row exists) and the dashboard repeats. The interval branch delegates tonextRun(:58-69), sodue/scheduled/disabled/not-ready/auto-off/active-job/idle/waiting-first-runkeep one implementation.src/jobs/service.ts:2643-2655— the digest passes the same four per-repository facts the page computes and nofloorMinutes, exactly aspages.ts:1257-1266. TheinstanceImage === '' ? true : …guard is duplicated (pages.ts:1264vsservice.ts:2651); a shared helper would be a later slice, not a reason to hold this one.src/jobs/service.ts:2612-2633— the switch is exhaustive overNextRunForState; the two unknown sentences matchplannedNextLine(src/views/pages.ts:757-761), andnext.at ?? Date.now()is unreachable for a non-nullscheduled.test/nextScheduledRun.test.ts:116-178pins the planner choice, the disabled-with-a-row exception, both unknown states and all eight interval states;test/jobs.test.ts:400-484pins the digest end to end for a cron repository, a disabled repository without a row, and a disabled repository with a row.docs/architecture.md:144-152anddocs/decisions/0017-...mdmatch the code;changelog.d/713.fixed.mdis present.
Cross-base risk checked rather than assumed: this head is based on f9b3d9be, eight commits behind live main e5397e14, and #733 touched the same four files. A local test-merge onto e5397e14 (scratch worktree, nothing pushed) is clean, tsc --noEmit is clean, and the touched suites pass there.
Ran:
npx tsx --test test/nextScheduledRun.test.ts test/jobs.test.tsat6b564684— 108/108 pass- local test-merge of
6b564684onto livemaine5397e14, same suites — 109/109 pass;npx tsc --noEmit— clean
Not run: full npm run check (the tip's ci-ok covers it — all required checks green at this sha); real SMTP delivery; a production deploy.
Conclusion: PASS at 6b56468421ae09dea69e6c77de07c12ce7bd1569 — APPROVE event, replacing the COMMENTED PASS 5908932502 at the same tip so the standing CHANGES_REQUESTED cannot keep the merge blocked.
Written-by: HugoXK
What changed
The daily digest's next-run line now uses the same decision as the repository page and the dashboard.
src/lib/nextRunFor.tsis the shared choice: a repository with a saved schedule row is answered byscheduleFromRow+nextTime, a repository without one delegates tonextRun, and the digest maps the resulting state to its own Chinese sentence.buildDigestno longer computeslast_run_at + min(interval, cooldown)inline, so a cron repository gets the planner's instant and a disabled, not-ready, idle, active-job or never-run repository prints the page's sentence instead of a promise. A disabled repository with a saved row keeps the planner's instant, exactly as the page does.Fixes #713
Why
The digest was the third next-run surface and the only one outside the rule. For a cron repository the email disagreed with the page by days; for a disabled repository with no row it promised a run the page said would never start.
src/jobscannot import the view helpers that make this choice, so the choice moved tosrc/lib/nextRunFor.tswhile the instant stays single-sourced innextTime/dueAfterMs.Verification
Ran:
npm run check— pass (1841 tests, typecheck, build)node scripts/version.mjs check— 0.3.6 oknode scripts/changelog.mjs check— passNot run:
Security notes
None. No new secret, network call or credential path; the digest reads the same repository, work-item, job and instance rows it already had.
Record
This PR reuses the existing #678 record at the owner's request, replacing its original #590 gate-evidence content. The head branch name
dev3/590-gate-resolve-evidenceis historical. This PR makes no #590 claim; the content is the #713 digest fix only.