fix(automations): filter the timeline before validating it (#914)

[sapersky]

Fixes #914 — automations on issue.labeled / issue.unlabeled failed on every poll against a real repository.

The bug

GithubPoller.timeline() validated the whole /timeline response with timelineEventSchema, which admits only labeled and unlabeled entries. The timeline is a heterogeneous, additive GitHub surface, so the first commented (or cross-referenced, assigned, renamed, …) entry rejected the array parse — and the throw propagated out of poll(), so no candidate on the page was evaluated, not even from issues whose own history is nothing but label events.

Since any issue anyone has commented on carries such an entry, the two label events were unusable in practice. pull_request.opened and issue.opened never reach timeline() and were unaffected.

The fix

Exactly what the issue suggests — filter before validating, via safeParse per entry:

return z.array(z.unknown()).parse(JSON.parse(raw)).flatMap((entry) => {
  const parsed = timelineEventSchema.safeParse(entry);
  return parsed.success ? [parsed.data] : [];
});

timelineEventSchema stays strict for the entries that matter — a labeled entry with a malformed label is still dropped rather than crashing the poll — and reconstructLabelEvents is untouched. Widening the z.enum to z.string() and filtering afterwards would also work, but it would cost the schema its role as the definition of "an entry we can reconstruct from".

Silently discarding unparseable entries is the right default here: the timeline is open-ended, and a new GitHub event type must never be able to stop an automation.

Tests

Two regression tests in github-poller.test.ts, both of which fail on the parent commit:

• keeps polling when the timeline carries entries other than labeled/unlabeled — mixes commented and cross-referenced entries between two labeled ones, asserts the poll still yields the label candidate.
• drops a labeled entry whose label is malformed instead of failing the poll — guards the half that a widened schema would have lost.

without the fix:  Tests  2 failed | 6 passed (8)
with the fix:     Tests  8 passed (8)

Also verified end to end against a live repository: the poll raised a ZodError before the change and returned candidates after it.

The full validation gate is clean on this branch: npm run typecheck, npm run test:unit, npm run build (including check:pack), and npm run test:package all pass.

npm test reports 6154 passed, 6 failed (6160), none of them in this code path: four (git-changes.test.ts ×2, directional-usage.test.tsx, agents-section.test.tsx) fail identically on the parent commit, and the other two (automations-gate.test.ts, run-isolation.test.ts) are load-sensitive — they pass on parent and on this branch alike when run on their own.

One note on running the suite outside a scratch directory

Several server tests fail when TMPDIR points inside a git work tree — mkdtempSync(join(tmpdir(), …)) then lands in a repository, so assertions like expect(await getRepoInfo(bare)).toBeNull() see the enclosing repo. TMPDIR=/tmp clears most of them. Not part of this PR, just what tripped me up first.

[CLAassistant]

All committers have signed the CLA.

[pat-lewczuk]

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-08-28T21:23:39Z. Other auto-skills will skip this PR until the lock is released.

[pat-lewczuk]

🔍 Code Review: fix(automations): filter the timeline before validating it (#914)

🎯 Summary

This PR fixes the bug reported in #914: GithubPoller.timeline() validated the entire /timeline response with timelineEventSchema, a schema that only admits labeled and unlabeled entries. Because /timeline is a heterogeneous, additive GitHub surface, the first commented or cross-referenced entry rejected the whole array parse, and the resulting ZodError propagated out of poll() — so no candidate on the page was evaluated, not even from issues whose own history is nothing but label events. Since any issue anyone has ever commented on carries such an entry, issue.labeled and issue.unlabeled automations were unusable in practice.

The fix (packages/cezar/src/automations/github-poller.ts:200-217) is four lines: parse the response as z.array(z.unknown()), then flatMap each entry through timelineEventSchema.safeParse, keeping the successes and dropping the rest. I reviewed both changed files — github-poller.ts and github-poller.test.ts — against the repository's CODE_REVIEW.md and BACKWARD_COMPATIBILITY.md.

This is a good change, and I want to be specific about why rather than just waving it through. It is the minimal correct fix rather than the tempting one: widening event to z.string() and filtering afterwards would also have worked, but it would have cost timelineEventSchema its role as the definition of "an entry we can reconstruct from," and it would have silently accepted a labeled entry with a broken label object. Filtering with safeParse keeps the schema strict exactly where strictness earns its keep. It also matches the house convention already stated in CODE_REVIEW.md — "parse failure degrades to a sane default, never a crash," and "readers skip unparseable lines" — so the poller now behaves like the rest of the codebase's boundary parsing rather than as an exception to it. reconstructLabelEvents is untouched, the change is confined to one private method, and the explanatory comment cites the issue as the repo asks.

I independently verified the regression claim rather than taking the PR description's word for it: with github-poller.ts reverted to its parent state (185c68a7) and the new test file kept, the suite reports 2 failed | 6 passed; with the fix restored, 8 passed. Both new tests are genuine regression guards, not decoration.

I also re-ran the full gate and reached a slightly better result than the PR description reports — see the Validation Gate section: with TMPDIR pointed outside a git work tree, all 6160 tests pass, including the two the description characterises as load-sensitive.

Verdict

✅ approve — the change is correct, minimal, faithful to the diagnosis in #914, covered by two regression tests that I confirmed fail without it, and green across the entire validation gate. It touches no contract surface and cannot affect the pull_request.opened / issue.opened paths, which never reach timeline(). The findings below are two minors and a nit; none of them blocks the merge, and none of them needs to be addressed in this PR.

🧪 Validation Gate

Command	Status	Notes
npm run typecheck	✅ PASS	Clean across contract, client, server and web projects.
npm test	✅ PASS	326 files, 6160 passed / 0 failed, when TMPDIR points outside a git work tree. Run with the default TMPDIR of this environment — which happens to live inside the cezar checkout — six tests fail (git-changes.test.ts ×2, directional-usage.test.tsx, agents-section.test.tsx, health-forge.test.ts, projects-api.test.ts). Every one of those failures is the mkdtempSync(join(tmpdir(), …)) artifact the PR description calls out: the "temp" directory lands inside a repository, so assertions like expect(await getRepoInfo(bare)).toBeNull() correctly see the enclosing repo. This is an environment artifact, not a property of the branch. Vitest also emitted 4 EnvironmentTeardownError unhandled rejections from runs/store.test.ts teardown; they are worker-shutdown noise, unrelated to this diff, and did not fail any test.
npm run test:unit	✅ PASS	node:test core-module suite, clean.
npm run build	✅ PASS	build:server, build:web and the check:pack tarball gate all succeeded.
npm run test:package	✅ PASS	Packaged CLI E2E, 15 tests, 0 failures.

The gate is fully green on this branch. Worth recording for the author: the two tests you flagged as load-sensitive (automations-gate.test.ts, run-isolation.test.ts) also pass in a full-suite run once TMPDIR is clean, so the TMPDIR explanation appears to cover all six failures rather than four of them.

Findings

🔹 Minor

packages/cezar/src/automations/github-poller.ts:157 — the timeline() call site is still unguarded, so the same blast radius remains for transport failures. This fix closes the schema half of #914, but the structural problem the issue's root-cause section also names is untouched: const timeline = await this.timeline(owner, repo, item.number) sits inside the per-item loop of poll() with no try/catch, so any non-schema failure from that one gh api call — a 404 on a transferred or deleted issue, a secondary rate limit, the 30-second GITHUB_COMMAND_TIMEOUT_MS expiring, a malformed non-array body — still throws straight out of poll() and discards every candidate on the page, exactly the failure mode #914 was filed about. scheduler.ts:99 then records the error and recordFailure escalates the backoff toward its six-hour cap, so a single unlucky issue can silence the whole automation. The repository's own CODE_REVIEW.md puts this under "Graceful degradation": GitHub reads are supposed to degrade rather than throw. The fix is small and local — wrap the call so a failed timeline fetch skips that item instead of the page:

let timeline;
try {
  timeline = await this.timeline(owner, repo, item.number);
} catch {
  continue; // one issue's timeline is unreachable; the rest of the page is still valid (#914)
}

I am flagging this as minor rather than major deliberately: it is pre-existing behaviour that this diff does not introduce or worsen, and the PR is right to keep its scope to what #914 asked for. It is a good candidate for a follow-up issue, and I am happy to file one.

packages/cezar/src/automations/github-poller.ts:213-216 — the filter cannot distinguish "an entry we do not care about" from "an entry we should have understood but could not," and silence is only correct for the first. Dropping a commented or cross-referenced entry is expected and uninteresting; dropping a labeled entry whose label object is malformed is a signal that something is wrong, and it currently vanishes without a trace. That second case is not merely invisible — it makes reconstructLabelEvents produce wrong data rather than incomplete data, because its backward walk at github-poller.ts:270-277 starts from the issue's current label set and undoes each transition in turn, which assumes it has been handed every transition. A dropped labeled entry therefore corrupts the reconstructed labels array on every older event in that issue's timeline, and nothing anywhere reports it. The issue itself anticipated this ("If observability matters, a debug counter of dropped entries would be preferable to a throw"). A cheap version that captures the distinction: count only the entries whose raw event field is labeled or unlabeled yet still failed safeParse, and surface that count through the automation log. In practice GitHub always sends label on a labeled event, so this is a robustness and diagnosability point rather than a live bug — hence minor.

💅 Nit

packages/cezar/src/automations/github-poller.ts:205-212 — the comment is eight lines explaining four. It is accurate, well written, and correctly cites #914 as CODE_REVIEW.md requires, so this is purely the author's call. But most of it restates the commit message, and the repository explicitly values modules that read in one sitting. Something like "Filter before validating: /timeline is heterogeneous and additive, so parsing the whole array against timelineEventSchema let the first commented entry kill the poll (#914). The schema stays strict for the entries we do reconstruct from." carries the same information in a third of the space, with the full story remaining one git blame away.

💥 Breaking Changes

• No exported/public symbol removed or renamed without a deprecation path. timeline() is a private method; the exported GithubPoller, buildSearchQuery, reconstructLabelEvents and matchesFilters are untouched.
• No function signature changed in a breaking way. timeline() still returns z.infer<typeof timelineEventSchema>[], which is exactly what reconstructLabelEvents(…, timeline) at github-poller.ts:267 expects, so its signature did not need to move either.
• No required type field removed or narrowed. timelineEventSchema is byte-for-byte unchanged; only where it is applied changed.
• No HTTP route URL removed or renamed; no method changed for an existing operation. No route in server.ts is touched.
• No field removed or retyped in an existing response shape. GithubCandidate and GithubPollResult are unchanged, so /api/v1/automations/:id/check and GET /api/v1/automation-checks/:checkId keep their exact shapes.
• No event or message name renamed or removed; no payload field removed. The AutomationEvent vocabulary is untouched — issue.labeled and issue.unlabeled simply start working.
• No CLI command or flag renamed or removed; no machine-parsed output format changed.
• No database table or column renamed or removed; no column type narrowed. Not applicable — no schema in this project.
• No config key renamed and no default changed silently. CEZ_AUTOMATIONS=1 gating is untouched.
• Where a contract had to change: not applicable, no contract changed.

Checked against BACKWARD_COMPATIBILITY.md section 2 and the "GitHub automations — opt-in gating (#801)" section, which requires the automations routes to "behave exactly as before once the flag is on." This change strictly widens what parses successfully — every input that produced candidates before still produces the same candidates — so it moves toward that guarantee rather than away from it. Persisted formats (automations.json, receipts, the NDJSON execution log, frozen high-watermarks) are all untouched. No protected surface is affected and there is nothing here to warn about.

🧪 Test Coverage

Coverage for this fix is genuinely good, and I verified it rather than inferring it. github-poller.test.ts:90-120 stubs the injected run so /timeline returns a labeled, a commented, a cross-referenced and a second labeled entry, then polls an issue.labeled definition filtered to changedLabels: ['triage'] and asserts one candidate comes back with changedLabel: 'triage' at the right timestamp. That test does double duty: it proves the non-label entries no longer abort the poll, and — because the first labeled entry carries the label other and is correctly filtered out rather than dropped — it proves the surviving entries still flow through reconstructLabelEvents and matchesFilters intact. github-poller.test.ts:122-143 covers the half a widened z.enum would have lost: a labeled entry with no label object is dropped while its well-formed sibling still yields its candidate.

I confirmed both are real regression guards by reverting github-poller.ts to its parent state and re-running the file: 2 failed | 6 passed, with the failures landing on exactly these two tests at github-poller.ts:157. Restoring the fix gives 8 passed. The existing test at github-poller.test.ts:34-88 continues to pass unchanged, so the ordering, cursor and tie-breaker behaviour did not regress.

One optional gap, worth a sentence rather than a finding: nothing asserts what happens when /timeline returns a non-array — a {"message": "Not Found"} error body, say. The outer z.array(z.unknown()).parse will still throw there, which is defensible behaviour, but it is currently untested and therefore unpinned. If you address the first minor above, that try/catch would become the natural place for such a test (expect(result.candidates).toEqual([]) when one item's timeline returns an error body), and I would suggest adding it there rather than in this PR.

---

Reviewed by om-auto-review-pr. Autofix: skipped (not my PR — re-run with --autofix to fix it here). No autofix was needed regardless: the verdict is approve.

[pat-lewczuk]

🤖 om-auto-review-pr — 🏷️ label rationale

• 🚀 merge-queue — the code review passed with no blockers and no majors, and the full validation gate (npm run typecheck, npm test, npm run test:unit, npm run build, npm run test:package) is green on this branch, so the PR is cleared for merge.
• 🐛 bug — this is a bug fix: it restores issue.labeled / issue.unlabeled automations, which failed on every poll against a real repository (Fix: automations on issue.labeled/unlabeled fail on every poll — the /timeline parse rejects any non-label entry #914).
• ⏭️ skip-qa — the change is a four-line parsing fix inside a private method with no user-facing surface to click through; it is fully exercised by two regression tests that I confirmed fail on the parent commit, and reaching the code path manually requires CEZ_AUTOMATIONS=1 plus a label automation created through the API, which the author already verified end to end against a live repository.
• 🔹 priority-medium — an ordinary bug rather than an outage or a security incident: the automations surface is gated behind CEZ_AUTOMATIONS=1 and off by default, and the cockpit editor cannot even express a label-event automation today (Implement: finish GitHub Automations editor and show Test filter matches #886), so the blast radius is limited to users who opted in and created one through the API.
• 🟢 risk-low — an isolated change to one private method that strictly widens what parses successfully; every input that produced candidates before still produces the same candidates, reconstructLabelEvents is untouched, the pull_request.opened / issue.opened paths never reach timeline(), and no contract surface in BACKWARD_COMPATIBILITY.md is affected.

[pat-lewczuk]

🤖 om-auto-review-pr completed: APPROVED. Lock released.

The full om-code-review pass is posted as the review above. No blockers and no majors: the fix is the minimal correct one, it is confined to a private method, it touches no protected surface in BACKWARD_COMPATIBILITY.md, and the two new tests are real regression guards — I reverted github-poller.ts to its parent state (185c68a7) and confirmed 2 failed | 6 passed, then 8 passed with the fix restored.

The whole validation gate is green on this branch: npm run typecheck, npm test (6160 passed / 0 failed), npm run test:unit, npm run build including check:pack, and npm run test:package (15/15). The six npm test failures the PR description reports reproduce only when TMPDIR points inside a git work tree; with TMPDIR outside one, all six pass — including the two the description characterises as load-sensitive, so the TMPDIR diagnosis appears to explain all of them.

The review leaves two minors and a nit, none of which needs to be addressed here: the timeline() call site at github-poller.ts:157 is still unguarded against transport failures (the same blast radius as #914, for a different cause), the safeParse filter cannot distinguish an entry type we do not care about from a labeled entry we should have understood, and the eight-line comment could be trimmed. The first two are good follow-up-issue material.

Labels: merge-queue, bug, skip-qa, priority-medium, risk-low — rationale in the comment above. Assignment handed to @sapersky so the PR shows its owner rather than the review lock. CI: the only configured check (license/cla) passes, so there is nothing pending and no CI follow-up is scheduled. Note that GitHub still reports mergeStateStatus: BLOCKED even with reviewDecision: APPROVED — most plausibly the require_extra_approval_for_unattributed_changes rule on main, which would need a second maintainer approval before the merge button unlocks.

Autofix: skipped (not my PR — re-run with --autofix to fix it here). None was needed regardless; the verdict is approve.

[sapersky]

🤖 om-auto-review-pr started by @sapersky at 2026-09-15T09:43:05.105193+00:00. Investigating the failed nightly-version package test and addressing required changes for PR #916. Other auto-skills will skip this PR until the lock is released.

[sapersky]

🔍 Code Review: PR #916 — CI follow-up

🎯 Summary

The existing review approves the timeline-filter fix and explicitly says its two minor suggestions and comment-length nit do not need to be addressed in this PR. There are no inline review threads and the PR is mergeable without conflicts. The current failed CI run fails in the nightly snapshot package test, not the timeline parser.

The failure reproduces on the PR head with GITHUB_RUN_ATTEMPT=3: the test expects 0.9.9-nightly.20260813.12, but the script correctly generates 0.9.9-nightly.20260813.12.3. The test helper inherits the workflow's run attempt from process.env.

Upstream already fixed this in #911, commit feb856667e489c0b4d9973e5b23eaa9aee0fa924. I cherry-picked that exact change, preserving attribution, into local commit b70a659b6a2ccff3d6acd8d487094a847d20480b. It pins the fixture's default attempt and includes an explicit rerun test that preserves the collision-avoidance behavior. Only packages/cezar/test/e2e/release-snapshot.test.ts changes in this follow-up.

Verdict

The identified CI defect is fixed and pushed to the existing PR as b70a659b6a2ccff3d6acd8d487094a847d20480b, following the author's explicit approval. Local validation passes. The new GitHub Actions run has status action_required and no jobs; remote CI has not executed. This is validation evidence, not a self-approval or permission to merge.

🧪 Validation Gate

Command	Status	Notes
npm run typecheck	✅ PASS	All four workspace checks passed.
npm test -- --maxWorkers=2	✅ PASS	Full repeat: 326 files, 6160 tests passed, no unhandled errors.
npm run test:unit	✅ PASS	35 passed, 1 skipped, 0 failed.
npm run build	✅ PASS	Server, cockpit and check:pack passed; the tarball contains 475 files.
GITHUB_RUN_ATTEMPT=3 npm run test:package	✅ PASS	All 16 package tests passed under the CI rerun environment.

Validation used TMPDIR=/tmp outside the git worktree. The first full Vitest run reported 6159 passed, one scheduler-start assertion failure, and five asynchronous cleanup errors. The scheduler test uses a fixed 50 ms wait. A full repeat with two workers was run to check the effect of load; its result is recorded above.

Findings

⛔ Blocker

packages/cezar/test/e2e/release-snapshot.test.ts:87 — ambient GITHUB_RUN_ATTEMPT makes the nightly fixture nondeterministic on CI reruns. Fixed and pushed in b70a659b by the upstream #911 patch. The new remote CI run requires maintainer action before its jobs can run.

🔍 Inherited reviewer feedback

The existing review approves the change. Its transport-failure isolation and malformed-label observability suggestions remain follow-up work, as that review explicitly recommends. Adding transport skipping requires a separate cursor/retry analysis; it is outside this CI repair. The optional comment shortening is declined because the current explanation is accurate and does not block approval. No required review ask is outstanding.

💥 Breaking Changes

• No exported/public symbol is removed or renamed.
• No public function signature is changed.
• No required type field is removed or narrowed.
• No HTTP route or method is changed.
• No response field is removed or retyped.
• No event or payload is changed.
• No CLI command, flag or machine-readable output is changed.
• No database or persisted state format is changed.
• No application config or runtime default is changed.
• No contract migration is needed; this follow-up only isolates the test environment.

🧪 Test Coverage

The original nightly test fails without this patch under GITHUB_RUN_ATTEMPT=3 (4 passed, 1 failed). With the patch, the snapshot suite passes all 6 tests under the same environment. The additional case explicitly requests attempt 2 and asserts the version and dependency suffix .2, so environment isolation does not disable rerun collision avoidance. All 16 packaged CLI tests pass as well. UI verification is not applicable to this test-only follow-up; skip-qa remains appropriate.

Handoff

The existing merge-queue, bug, skip-qa, priority-medium, and risk-low labels were not changed. GitHub denied the attempt to add in-progress due to insufficient label permissions; this run's claim therefore consisted of the existing author assignment and its start comment. No PR was merged and no replacement PR was opened.

The author approved the push. git push origin HEAD:fix/914-timeline-filter succeeded, and GitHub confirms that PR #916 now points at b70a659b.

🤖 om-auto-review-pr — CI result

The new CI run targets b70a659b and concluded action_required with zero jobs. The fork workflow requires maintainer action before validation can execute. The authenticated account has read-only access to open-mercato/cezar (push, maintain, and admin are false), so this agent cannot provide repository approval. The CLA check passes. A maintainer must approve the workflow run; no further CI follow-up will come from this agent until that external approval is supplied. Local results above do not substitute for remote CI.

🤖 om-auto-review-pr completed: CI fix pushed and locally validated; remote CI awaits maintainer action. Lock released.