Skip to content

fix(chat): keep pull-request state in sync across sidebar and detail panel - #443

Merged
iamwhatever merged 1 commit into
mainfrom
fix/pr-state-sync
Jul 27, 2026
Merged

fix(chat): keep pull-request state in sync across sidebar and detail panel#443
iamwhatever merged 1 commit into
mainfrom
fix/pr-state-sync

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

The sidebar session chips and the Changes-strip detail panel read two independent caches that nothing invalidated on an agent turn, so they could render different lifecycles for the same PR indefinitely:

  • The full-payload cache (_CACHE, 30s TTL) and the chip cache (_check_cache, 60s TTL) never cross-populated, so each could be individually "fresh" and still disagree.
  • The chip sweep (_refresh_check_loop) is TTL-paced and admits at most CHECK_STATUS_PENDING_MAX (16) URLs per round, backing the rest off a full TTL — so with more PR-linked slots than that cap, a given chip lagged by ⌈N/16⌉ minutes.
  • The detail query is staleTime: Infinity with refetchOnWindowFocus/refetchOnReconnect off and no interval, so after mount it only ever updated when the user hit Refresh.

Net effect: a PR that merged, went red, or gained review comments mid-session stayed visibly wrong on at least one of the two surfaces.

Change

1. Unified cache (both directions). A completed full fetch projects state/draft/checks onto the chip cache (status_from_full_payloadrecord_full_payload_status), so the sidebar cannot render an older lifecycle than the panel it was just fetched for. Conversely, a chip refresh that observes a changed status drops the now-stale full payload, so the panel's next read can't serve a lifecycle the chip has already passed. An unchanged status leaves the payload alone (so this doesn't defeat the 30s cache).

2. Turn-boundary refresh. On the idle transition in _run_chat, DashboardState.refresh_slot_source_status re-reads that slot's serialized chip URLs through request_check_refresh_now, which bypasses the chip TTL. An agent turn that ran `gh pr create`, pushed a revision, or drove a review round is the moment the remote state most likely moved. Bounded by:

  • owner-gated — status is credential-backed and only owner windows render it, so a headless or non-owner gateway spawns no provider subprocess;
  • slot-scoped — only the session that finished, not every slot;
  • rate-floored — one forced read per URL per _CHECK_FORCE_MIN_INTERVAL_SECS (10s); URLs inside the floor fall back to plain TTL pacing rather than being dropped;
  • the pre-existing pending cap and _CHECK_CONCURRENCY semaphore still apply, so a forced round can never outgrow a paced one;
  • best-effort — a failure is logged at debug and can never break turn completion.

3. Status deltas push instead of polling. When a URL's cached {ci, state} changes, the owner-only source_status WS event carries {url, origin, ci?, state?} (DashboardState.push_source_status, registered once as a delta sink at app wiring; _send_ws_owners means it never reaches non-owner or app-token clients).

The client invalidates the mounted pull-request queries (['pull-request-source', url] / ['pull-request-checks', url]) for every changed delta, regardless of origin, and patches its status batch. This is required for cross-window convergence: a detail-origin delta is produced by one window's full HTTP fetch, so only that window received the fresh payload — other owner windows (whose detail queries are staleTime: Infinity) must still invalidate to converge. This does not loop, because record_full_payload_status runs only in the uncached fetch path: the initiating window's redundant refetch hits the warm 30s payload cache and emits no new delta.

origin ("chip" vs "detail") is therefore diagnostic only — it records which path produced the delta and is retained on the wire for future requester-aware routing, but no client behavior branches on it today.

  • origin: "chip" — a lightweight chip refresh learned something the full payload didn't know (and the gateway already dropped that payload).
  • origin: "detail" — a full detail fetch's write-through produced the change.

The client also invalidates the mounted pull-request queries on chat_done for the active slot, because lifecycle/CI deltas don't cover review comments or mergeability and the detail query would otherwise never refetch after mount. Only mounted queries refetch; the rest are just marked stale. Polling remains the safety net for a missed event.

Files

File What
src/kiro_crew/dashboard/handlers/source_providers.py projection + write-through, reverse invalidation, force= / request_check_refresh_now, delta sink registry, force-ledger bounding
src/kiro_crew/dashboard/state.py source_link_urls_for_slot, push_source_status, refresh_slot_source_status
src/kiro_crew/dashboard/chat_runner.py turn-boundary hook on the idle chat_done (not the mid-turn /compact flush)
src/kiro_crew/dashboard/server.py one-time delta-sink registration
website/src/utils/pullRequestStatusDelta.ts new: parseStatusDelta (validates the wire payload as untrusted) + applyStatusDelta
website/src/hooks/useWebSocket.ts source_status case + turn-boundary invalidation
docs/system-specs/modules/learn-cron-dashboard.md spec paragraph, WS event catalog, header

Tests (27 new)

  • Backend (8, test_source_providers.py) — projection vocabulary across GitHub/GitLab spellings and failure dominance; full fetch → chip write-through with detail origin; chip change → full-payload drop with chip origin; unchanged status preserves the payload; TTL bypass then force-floor suppression; refresh resumes after the floor; sink dedup + one broken sink not starving others; force ledger bounded by _trim_check_cache.
  • Backend (5, test_dashboard_state_ws.py) — per-slot URL scoping and cap; owner-gated force refresh with the broadcast callback; no-op without an owner window; failure swallowed; delta serialized only to owner sockets.
  • Frontend (14) — delta parsing rejects unknown vocabulary and url-less payloads; merge is identity-stable on no-op, clears refreshing, records unseen URLs; WS patches the batch and invalidates the detail queries on every changed delta regardless of origin (chip and detail) so other owner windows converge, ignores malformed deltas, refetches on active-slot chat_done but not for a background slot.

Verification

All local gates green: pytest 17,161 passed, isort / flake8 / mypy (473 files) clean, tsc -b clean, vitest 4,493 passed across 389 files.

4 backend failures are pre-existing host-environment issues, not from this change — the 3 test_dashboard_origin.py::TestParseDashboardUrlMalformed cases fail identically when run against unmodified origin/main source (a local config default port leaks into parse_dashboard_url), and test_skills.py::test_flat_copy_untouched_when_nested_missing is the known locally-modified-skills-dir failure.

Screenshots

Captured by website/scripts/capture-pr-state-sync.mjs — it drives the real built SPA (website/dist) in Playwright with every /api/** call and the /api/ws socket answered from fixtures. No gateway, no dashboard token, no provider calls: only the network is stubbed, so the chips, the strip, and the delta handler are the production code paths. The cache states are staged deliberately (a naturally-caught desync isn't reproducible on demand) — stating that here rather than implying these were caught in the wild.

The bug — one PR, two answers. Sidebar chip #443 still shows open + CI green while the detail panel beside it already knows the PR is Merged. Both caches were inside their own TTL; they simply never told each other.

Desynced chip and panel

After the write-through. The full fetch projects onto the chip cache, so the chip carries the same merge glyph the panel shows — and CI correctly drops off the chip once merged.

Chip and panel agreeing

Delta pushes instead of polling. With the page already rendered and no poll due, one source_status frame for #409 (origin: "chip") is pushed into the websocket. The strip tab goes from open + checks-running to merged and the CI glyph disappears — previously this waited out a poll interval.

Before the frame After the frame
Strip before delta Strip after delta

Light theme parity for the synced state:

Light theme

Full frame for context (dark, synced):

Full dashboard

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/store/dashboardSlice.ts:151 -- "!== undefined" preserves stale CI/state when an authoritative snapshot omits that field -> Fix: assign both fields unconditionally.
FINDING -- src/kiro_crew/dashboard/state.py:2577 -- function-local "from ... import" violates the top-level-imports rule -> Fix: move the import to the module import block.
[CODEX-REVIEWED] 8c35ba9

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf: <one-sentence reason>

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound fix for a real desync, but the shape is a two-cache coherence protocol fragile enough to need its own runtime loop-breaker.

Watch

  • The design keeps two independent projections coherent by convention (shared helpers, "do not inline a second copy") and then ships _note_check_flap because the acknowledged failure mode of that convention is "a provider subprocess per URL per cycle indefinitely, silently." A heuristic damper guarding against your own protocol's divergence is a signal the invariant is not structural: any future contributor who touches GitHub/GitLab vocabulary in one path re-arms the loop, and the damper degrades it to a silently stale glyph rather than surfacing it. Consider making the chip projection derivable from exactly one function over one raw value per provider (it's close — _gitlab_aggregate_ci already does this for GitLab CI) so drift is impossible rather than damped.
  • source_providers.py now carries eight coordinated module-global structures (_check_cache, _check_generations, _check_inflight, _check_forced_at, _check_force_pending, _check_flap, _check_flap_damped, _status_delta_sinks) whose race-freedom rests on ordering comments (the post-await latest is not previous re-read, force-stamp-only-if-started). It works, but the next feature on this file inherits an implicit state machine.
  • Diff→description: user-visible GitLab CI semantics changed beyond sync — manual gate now renders running, the glyph moves from job-bucket rollup to pipeline aggregate (allow_failure folds in), a synthetic "Pipeline" check row is fabricated, and a full job page flags checks partial. These are defensible reconciliations, but they're undocumented behavior changes riding a sync PR; note them in the description or split them.

Suggestions

  • Fold _check_forced_at/_check_force_pending/_check_flap* into a single per-URL cache-entry record so _trim_check_cache's four separate bounding sweeps collapse into eviction of one structure.

[DESIGN-REVIEWED] 8c35ba9

@kyleseaman
kyleseaman force-pushed the fix/pr-state-sync branch 2 times, most recently from d4c4260 to f649bc1 Compare July 25, 2026 16:38
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Opus 5 Review — ✅ no blocking findings

Reviewed 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf — this comment is updated in place on each push.

No findings.

Verdict recorded via the action's structured output for commit 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf: <one-sentence reason>

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Windows shard failure: inherited from a stale base, fixed by rebase.

Backend Tests (Windows) (2) failed on exactly one test — test/test_history.py::TestRecentFromSource::test_recent_from_source_sorted_by_ts (['first', 'third', 'second'] != ['first', 'second', 'third']). That test appends three messages in rapid succession and sorts by a ts stamped from datetime.now(); on Windows's ~15ms clock tick the appends collide on an identical timestamp, the sort becomes ambiguous, and the underlying merge order leaks through.

Nothing in this PR touches history.py or that test — the diff is confined to the source-provider status caches, DashboardState, the chat_done hook, and the websocket client. The failure was inherited: this branch was cut before 067a7e1c ("green up Windows Backend Tests", #450) landed the fix, which injects a strictly-increasing clock into the test so the asserted order is actually encoded in the timestamps.

Rebased onto current origin/main (was 5 commits behind, clean rebase, still one commit) and force-pushed as 5a450fef. Screenshot URLs in the description re-pinned to the new SHA.

Local gates on the rebased tree: pytest 17,165 passed, isort / flake8 / mypy (473 files) clean, tsc -b clean, vitest 4,493 across 389 files. Remaining local failures are host-environment only and not from this change: three test_dashboard_origin.py::TestParseDashboardUrlMalformed cases (a local config default port leaks into parse_dashboard_url — fails identically on unmodified main), plus two load-sensitive tests that each passed in isolation and on a repeat run (test_token_auth.py::test_start_paths_warm_auth_singletons_off_loop, test_terminal_handler.py::...test_ws_ctrl_c_delivers_sigint).

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Review round — disposition (head 5a450fef)

Addressed the advisory MEDIUM/LOW findings raised by GPT 5.6 and Fable 5 (Claude) even though all four checks were passing. All local gates green (pytest 17168 passed, isort/flake8/mypy clean, tsc + 4512 vitest). Rebased onto current origin/main.

1. Sink leak on dashboard restart (GPT MEDIUM — server.py)
register_status_delta_sink(state.push_source_status) had no matching cleanup, so restarting a dashboard in one process retained every old DashboardState and dispatched to dead states. Fixed: added an app.on_cleanup hook (_status_sink_shutdown) that calls unregister_status_delta_sink.

2. Forced turn-boundary refresh made the chip staler under a full pending cap (GPT MEDIUM + Claude MEDIUM #1source_providers.py)
request_check_refresh_now recorded _check_forced_at[url] before admission, and schedule_check_refresh's over-cap branch renewed the cache timestamp even for forced calls. A URL the pending cap rejected was thus both marked "just forced" (10s floor) and had its TTL renewed, so the next turn boundary and the periodic sweep skipped it. Fixed: burn the force floor only for URLs actually admitted (returned by schedule_check_refresh), and skip the cache-timestamp renewal when force=True so a deferred forced URL stays immediately eligible. New test test_forced_refresh_over_cap_stays_eligible.

3. record_full_payload_status could erase a known CI chip on a partial payload (Claude MEDIUM #2source_providers.py)
When a provider's secondary pipelines/jobs call fails, the full payload returns checks: [] with checks in partialSections; the projection then omits ci, and the write-through blanked the sidebar's failed-CI glyph — the exact divergence this PR fixes. Fixed by mirroring _refresh_check_status's keep-known-status rule: carry the previous ci over only when checks is explicitly partial (a genuinely empty checks section still clears the glyph). New tests test_record_full_payload_preserves_ci_when_checks_partial and ..._clears_ci_when_checks_genuinely_empty.

4. Client wiped a populated chip on an all-stripped delta (Claude MEDIUM #3pullRequestStatusDelta.ts)
On version skew, parseStatusDelta strips unknown state/ci, and applyStatusDelta then wrote the empty result over a populated {state, ci} entry. Fixed: bail (return the same batch) when a delta carries neither field. New test in pullRequestStatusDelta.test.ts.

5. Detail-origin delta left other owner windows stale (GPT MEDIUM pass 2 — useWebSocket.ts)
A detail-origin delta is produced by one window's full fetch; only that window received the fresh HTTP payload, so other owner windows (with staleTime: Infinity detail queries) skipped invalidation and kept rendering the pre-change lifecycle — the cross-window echo of this bug. Fixed: invalidate the detail query for every changed delta regardless of origin. Verified this cannot loop — record_full_payload_status runs only in the uncached fetch path, so the initiator's redundant refetch hits the warm 30s cache and emits no new delta. Updated the does-not-refetch-on-detail test to assert convergence instead.

LOW — chat_done refetch can still serve the 30s full-payload cache (Claude LOW): deferred. Forcing refresh: true on every turn boundary would defeat the cache and hammer providers; turns typically exceed the 30s TTL so the payload is usually already dropped, and the retained poll heals the rare miss. Tightened the surrounding comments so they no longer overstate the guarantee rather than change behavior.

No code changed for false positives — all five were legitimate.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Review round — disposition (head 5a450fef, Design Review)

Addressing the two advisory Design Review (Fable 5) CONCERNS. Both are non-blocking; no code behavior changed this round.

1. Description ↔ diff mismatch on the origin mechanism (fidelity) — FIXED (description).
The PR body still described the pre-rebase design where a detail-origin delta made the client "patch chips only" (and listed a test asserting it "does not refetch on detail"). That is stale: disposition item #5 on this head changed the client to invalidate the detail queries for every changed delta regardless of origin, so other owner windows converge (only the initiating window received the fresh HTTP payload), and the test was renamed to assert convergence. Updated the description's section 3 and the Tests list to match the shipped code, and restated that origin is now diagnostic only — retained on the wire for potential future requester-aware routing, with no client behavior branching on it today. No consumer distinguishes the field, but it is cheap, validated as untrusted, and its retention rationale is now stated rather than implied, so I kept it rather than dropping wire surface.

2. Two stores kept in step by convention (design) — acknowledged, deferred to follow-up as suggested.
Fable explicitly framed this as "worth considering as a follow-up, not a rework of this PR." The two fetch paths (lightweight chip call vs. full payload) have genuinely different costs, so collapsing them into a single status-derivation function / single URL-keyed store with the chip status as a projection is the right longer-term shape but out of scope here. The vocabulary-drift risk is currently held by status_from_full_payload's "keep the two in step" docstring plus the projection-vocabulary tests. Filing as a follow-up.

GPT 5.6 is now LGTM (pass 3) and Claude's advisory MEDIUM/LOWs were all fixed in the prior round; this round is description-only.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Design finding #2 follow-up filed as #463.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf.

Second-order review for 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf; this comment is updated in place on each push.

Review details

I've read both files. The sub-threshold findings are: two GPT 5.6 line-level items (a frontend stale-field assignment and a function-local import), and the design reviewer's CONCERNS (a two-cache coherence protocol needing a runtime loop-breaker, eight coordinated module-globals, and undocumented GitLab CI semantic changes). Claude found nothing.

Judging each against the narrow one-way-door / concrete-harm bar:

  • dashboardSlice.ts:151 (!== undefined preserves stale CI/state) is a client-side render-freshness bug — no persisted schema, wire format, or data loss; fully reversible in a later frontend change.
  • state.py:2577 (function-local import) is a style/convention nit, explicitly excluded from blocking.
  • The two-cache coherence protocol / eight module-globals is architectural-erosion / maintainability — explicitly routed to follow-ups. The unbounded-polling failure mode the design reviewer names is already mitigated by this diff via _note_check_flap (caps the blast radius to a stale glyph + one loud log), so no unbounded-resource-growth harm survives.
  • Undocumented GitLab CI semantic changes is a documentation concern, reversible.

None of these lock in an expensive-to-reverse contract or trigger a security/data-loss/availability regression that this diff introduces.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Considered but not escalated

  • dashboardSlice.ts:151 stale-field preservation (GPT 5.6) — a client-render freshness bug (the exact class this PR targets), but purely in-memory UI state, reversible in a later frontend fix; not a one-way door or production harm.
  • state.py:2577 function-local import (GPT 5.6) — style/convention only; explicitly outside the blocking bar.
  • Two-cache coherence protocol + _note_check_flap damper (Design) — the unbounded provider-polling loop it guards against is already contained by the damper shipped in this diff, so no availability/resource harm survives; the remaining concern is maintainability, which is reversible.
  • Eight coordinated module-globals / implicit state machine (Design) — architectural erosion; real tech-debt but reversible in a later refactor.
  • Undocumented GitLab CI semantic changes (Design) — documentation gap, not a behavior lock-in.

Suggested follow-ups (open as issues — non-blocking)

  • Assign CI/state unconditionally in dashboardSlice (website/src/store/dashboardSlice.ts:151): the !== undefined guard can retain a stale field when an authoritative snapshot omits it, undercutting this PR's coherence goal — fix in a follow-up frontend change.
  • Fold the per-URL cache-entry bookkeeping into one record (source_providers.py): collapse _check_forced_at / _check_force_pending / _check_flap* into a single per-URL entry so _trim_check_cache's separate bounding sweeps become one eviction, reducing the implicit state machine's surface area (matches the design reviewer's own suggestion).
  • Consider deriving the chip projection from one function per provider so drift is structurally impossible rather than damped, retiring the need for _note_check_flap over time.
  • Document the GitLab CI semantic changes (manual→running, job-bucket rollup→pipeline aggregate, synthetic "Pipeline" row, full-job-page partial flag) in the PR description or a spec note, since they are behavior changes riding a sync PR.

[ARBITER-REVIEWED] 8c35ba9

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter 8c35ba9a80b9df41d994776b644b9eeb22d5d6cf: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Review round — disposition (head 6bb8210f, Arbiter)

Addressed all three items the Arbiter escalated to BLOCK on 5a450fef. Each was a real code/spec gap — no false positives. Rebased onto current origin/main (was 1 behind). All local gates green: pytest 17,185 passed (only the 3 pre-existing host-only test_dashboard_origin.py::TestParseDashboardUrlMalformed failures, which pass in CI), isort/flake8/mypy (473 files) clean, tsc -b clean, vitest 4,512 passed.

1. GitLab CI projection mismatch → sustained cache ping-pong (Claude MEDIUM #1) — FIXED (source_providers.py).
The chip path (_fetch_check_status) mapped a GitLab pipeline aggregate status of manual/skipped to ci: "running", while the full-payload path (status_from_full_payload over _gitlab_check job buckets) maps those jobs to the "skipped" bucket → ci: "passed". With this PR's new mutual invalidation, any MR sitting on a manual gate (a common steady state) would ping-pong every 60s: chip sees running ≠ cached passed, drops the full payload, client refetches, the full fetch re-projects passed, next chip cycle repeats. Fixed on the chip side (Claude's preferred option — GitHub-safe, since GitHub derives both caches from identical _github_check buckets and is untouched): _fetch_check_status's GitLab branch now maps manual/skippedpassed, matching the job-bucket projection. Added test_fetch_check_status_gitlab_manual_pipeline_matches_projection (asserts chip and projection agree) plus manual/skipped bucket cases in test_status_from_full_payload_projects_lifecycle_and_ci.

2. _refresh_check_status pre-await snapshot race (Claude MEDIUM #2 + GPT pass 1) — FIXED (source_providers.py).
previous was captured before the provider await, then used for the changed comparison and cache write. The turn-boundary design makes a concurrent full fetch the common case (on chat_done the client's detail invalidation and the forced chip refresh fire for the same URL together). If the full fetch resolved first and wrote a fresh projection via record_full_payload_status, the chip refresh would clobber it with a possibly-older read, spuriously judge changed, drop the just-stored full payload, and emit a redundant delta. Fixed: after the await, re-read _check_cache.get(url); since _check_inflight dedups concurrent chip refreshes, the only other writer is record_full_payload_status, so if the cache entry identity changed during the await we defer to it entirely (no overwrite, no invalidation, no delta) rather than compare against the stale snapshot. Transient-failure fallback still anchors on previous. Added test_chip_refresh_defers_to_concurrent_full_payload_write.

3. Spec + backend docstring documented a non-existent origin-based refetch-suppression mechanism (GPT pass 2 + Design finding 1) — FIXED (learn-cron-dashboard.md, source_providers.py).
The prior round updated the PR description, but the authoritative spec (docs/system-specs/modules/learn-cron-dashboard.md, both the line-3 summary and the detailed §Pull-request sources) and _emit_status_delta's docstring still claimed a detail-origin delta makes the client "patch chips only." The shipped client invalidates the detail payload for every changed delta regardless of origin (a detail delta is emitted by the single window whose full fetch ran; the others must still converge). Both spec locations and the docstring now state that origin is diagnostic only, both origins trigger detail-query invalidation, and the initiating window's refetch cannot loop (it hits the warm 30s cache). origin is retained on the wire for diagnostics / possible future requester-aware routing, with that rationale now stated rather than implied.

The two "considered but not escalated" items (Design finding 2 structural refactor — filed as #463; orchestrator chat_done path — a completeness gap, not a regression) remain out of scope as the Arbiter itself scoped them.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

CI note — two shard failures are unrelated flakes, re-run (head 38fdf99d)

Two backend shards went red on this run; both are in subsystems this PR does not touch and are cross-test state-leak flakes, not regressions:

  1. Backend Tests (3.10, 4)test_telegram_config_handlers.py::test_clear_also_removes_legacy_config_token (assert 'bot_token' not in saved['telegram']). Passes cleanly in isolation locally (1 passed); the leaked bot_token: '' is a config-clear residue from a neighbor test in the shard. This PR touches no telegram code (diff confined to source-provider caches, state.py, the chat_done hook, and the WS client); the test file was last modified by feat(telegram): expose forum per-thread config in settings panel #327, days before this branch.
  2. Backend Tests (Windows) (2)test_dashboard_chat.py::TestEmptyResponseRetry::test_first_empty_response_requeues_message (assert 5 == 1). A requeue-count accumulation across the shard — classic shared-state leak under parallel xdist. This PR adds no retry/requeue behavior.

Branch is 0 commits behind origin/main (current). Re-ran the failed jobs only (gh run rerun --failed); the in-progress Claude AI Review, Arbiter, and Coverage Gate on 38fdf99d are left to complete. No code changed — the tests were not weakened.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Review round — disposition (head 9053d50f, Arbiter BLOCK)

Addressed both items the Arbiter escalated to BLOCK on 38fdf99d. Both are legitimate — no false positives. Rebased state unchanged (0 behind origin/main). Local gates green on the worktree venv: pytest 17,189 passed (only the 3 pre-existing host-only test_dashboard_origin.py::TestParseDashboardUrlMalformed cases fail locally — they pass in CI), isort / flake8 / mypy (473 files) clean, tsc -b clean, vitest 4,512 passed.

1. GitLab closed-draft / locked MRs diverged between the two projections (Claude MEDIUM #1 → Arbiter item 1) — FIXED (source_providers.py).
The chip path (_fetch_check_status) applied the GitLab draft mapping unconditionally (if details.get("draft") or details.get("work_in_progress")) and had no locked branch, while status_from_full_payload maps both closed and locked"closed". GitLab keeps draft: true on an MR closed while still in draft (a common way to abandon one), so for that MR the chip projected "draft" while the full payload projected "closed" — and this PR's mutual invalidation would ping-pong that divergence every 60s chip TTL forever (chip sees draft ≠ cached closed, drops the full payload, client refetches, full re-projects closed, repeat). locked diverged the same way (chip omitted state; full said closed). Fixed on the chip side (Claude's preferred, GitHub-safe option — GitHub derives both caches from identical _github_check buckets and is untouched): gated the draft mapping on raw_state in {"opened", "open"} and folded locked into the closed branch, so both paths now agree for every GitLab lifecycle. Added test_fetch_check_status_gitlab_closed_draft_matches_projection (asserts chip == projection for closed-draft and locked) plus a closed-draft case in test_status_from_full_payload_projects_lifecycle_and_ci.

2. Projection coherence rested on a comment-enforced convention with no structural loop-breaker (Design CONCERN #1 → Arbiter item 2) — FIXED (source_providers.py).
Implemented the design reviewer's suggested structural backstop rather than deferring, since it is a small bounded addition that caps the blast radius of item-1-class bugs permanently. _refresh_check_status now records each URL's changed transition; when a URL repeats the identical (previous → new) transition past _CHECK_FLAP_DAMP_THRESHOLD (3) consecutive refreshes — the signature of a chip↔full vocabulary divergence, where the chip re-projects value A while the client's full refetch keeps resetting the cache to B — the loop-breaker stops invalidating the full payload and emitting deltas for that URL and logs loudly once. The divergence then degrades to a stale glyph instead of an unbounded provider-polling loop. A genuinely changing PR produces distinct transitions, which resets the counter and clears the damp, so real lifecycle changes are never suppressed. The chip cache is still updated and the sidebar still re-serializes (glyph stays live); only the loop-driving invalidation + delta are withheld. The flap maps are bounded by _trim_check_cache (evicted URLs pruned). Added test_chip_refresh_damps_projection_flap (asserts invalidation/delta fire for the first N transitions, then stop once damped, that the cache still tracks the latest projection, and that a different transition clears the damp).

The two items the Arbiter explicitly "considered but not escalated" (Claude MEDIUM #2 sidebar-chip race → follow-up; Design CONCERN #2 turn-boundary proportionality → follow-up) remain out of scope as the Arbiter itself scoped them. GPT 5.6 is LGTM (pass 3); Claude and Design are advisory-clear on the prior head and re-run on this one.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Rebase + conflict resolution (head 81b33ce6)

The PR went CONFLICTING after main advanced 12 commits. Rebased onto current origin/main; two files conflicted and both were merged rather than taken from one side:

1. source_providers.py — real regression surfaced by the merge, fixed.
main (via the auto-merge/ready mutation work, #442) changed _invalidate_pull_request_cache to also clear the lightweight chip cache (_invalidate_check_status, which pops _check_cache[url] and bumps its generation). This branch's _refresh_check_status calls that helper after a changed chip refresh solely to drop the now-stale full payload behind the detail panel — when this branch was written, that helper touched only the full cache. Post-merge it also nuked the chip entry the refresh had just written, so:

  • get_cached_check_status returned None right after a successful refresh, and
  • the next refresh saw previous is None, re-judged "changed", re-invalidated, and re-queued the broadcast — i.e. the exact self-sustaining invalidation ping-pong this PR exists to prevent, plus a doubled _queue_check_update.

Caught by test_refresh_check_status_queues_broadcast_only_when_status_changes (main's test, auto-merged in) failing Called 2 times. Fix: split _invalidate_full_payload_cache (full-source cache + in-flight only) out of _invalidate_pull_request_cache (which now = full-payload invalidation + _invalidate_check_status, unchanged for the mutation paths). The changed-chip-refresh path now calls _invalidate_full_payload_cache, so it drops only the stale panel payload and preserves the chip entry it just produced (no generation bump, no self-loop). The six mutation callers (resolve/auto-merge/ready) still invalidate both caches. Updated the two chip-refresh tests' mock targets to _invalidate_full_payload_cache accordingly (assertions/counts unchanged — no test weakened).

2. docs/system-specs/modules/learn-cron-dashboard.md — merged both sides.
main had added the auto-merge/ready mutations (three mutations, six endpoints, both-cache invalidation) to the Pull-request-sources section; this branch had added the state-sync narrative. Kept main's mutation text and folded in the "One truth for two surfaces" / structural loop-breaker / "Turn-boundary refresh" / "Status deltas push instead of polling" paragraphs.

Gates on the rebased+fixed tree (worktree .venv): pytest 17,266 passed (only the 3 pre-existing host-only test_dashboard_origin.py::TestParseDashboardUrlMalformed cases fail locally — they pass in CI), isort / flake8 / mypy (473 files) clean, tsc -b clean, vitest 4,522 passed. Screenshot URLs re-pinned to 81b33ce6. Single commit on top of origin/main.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Review round — disposition (head 8419fa69, Arbiter BLOCK)

Addressed both items the Arbiter escalated to BLOCK on 81b33ce6. Both are legitimate — no false positives. Rebase state unchanged (0 behind origin/main, still one commit). Local gates green on the worktree .venv: pytest 17,268 passed (only the 3 pre-existing host-only test_dashboard_origin.py::TestParseDashboardUrlMalformed cases fail locally — they pass in CI; one test_apps_registry.py::...reaps_clone_tree_on_timeout timeout flake passed in isolation, subprocess-reap timing under the parallel load, and this PR touches no apps code), isort / flake8 / mypy (473 files) clean, tsc -b clean, vitest 4,522 passed.

1. GitLab allow_failure jobs made the two projections disagree in a common steady state (Claude MEDIUM 1 → Arbiter item 1) — FIXED (source_providers.py).
_gitlab_check bucketed a failed job as "failed" without consulting allow_failure. GitLab folds an allowed-failure job into a success pipeline aggregate, so the chip path (reads the aggregate) projected ci: "passed" while the full-payload job rollup projected ci: "failed" — the exact vocabulary divergence this PR forbids, and one that would ping-pong every 60s chip TTL for every project using allow_failure (lint/optional jobs — extremely common). Fixed by carrying allow_failure into the shared bucket mapping: an allowed failure now buckets as "skipped", so the job-level rollup matches the aggregate the chip reads. New test test_fetch_check_status_gitlab_allow_failure_matches_projection asserts _gitlab_check buckets an allowed failure as skipped / a hard failure as failed, and that the chip and full-payload projections agree for the mixed (green + allowed-failure) case.

2. Coherence rested on two hand-mirrored copies of the provider-vocabulary mapping (Design CONCERN 1 → Arbiter item 2) — FIXED structurally (source_providers.py).
Implemented the design reviewer's/Arbiter's preferred structural fix rather than leaning on the flap damper as the primary guarantee. Extracted the raw-provider-value → chip-projection vocabulary into three shared functions both paths now call, so drift is structurally impossible instead of comment-enforced:

  • _rollup_ci(buckets) — the failed > pending(running) > passed CI rollup (was inlined identically in _fetch_check_status's GitHub branch and status_from_full_payload).
  • _project_state(raw_state, draft=…) — the OPEN/MERGED/CLOSED(+locked) + draft-gated-on-open lifecycle mapping (was hand-duplicated in both paths with GitHub-uppercase / GitLab-lowercase variants; now normalized once).
  • _gitlab_status_bucket(status, allow_failure=…) — the single GitLab status→bucket vocabulary, now called by both _gitlab_check (per job) and the chip path (aggregate → single-element rollup), so the aggregate and job-bucket paths can no longer diverge on manual/skipped/canceled/allow_failure.

The flap damper (_note_check_flap) is retained as defense-in-depth only. Updated status_from_full_payload's docstring to state the shared-helper invariant rather than the old "keep the two in step" convention.

Folded in the empty-jobs case the Arbiter grouped under item 2 (GPT pass-2 checks: [] clearing a known glyph): _fetch_gitlab now distinguishes a pipeline-less MR (keeps checks: [] → both paths project no CI) from a pipeline whose jobs have not materialized yet (falls back to the pipeline aggregate via _gitlab_check({**pipeline, "name": "Pipeline"}), the same convention _fetch_gitlab_checks already uses), so the full-payload projection matches the chip aggregate instead of clearing the glyph. Two new tests: test_fetch_gitlab_full_payload_synthesizes_pipeline_when_jobs_empty and test_fetch_gitlab_full_payload_no_pipeline_keeps_checks_empty. Also added manual/allow_failure bucket cases to the existing projection tests.

The Arbiter's "considered but not escalated" items and the explicit follow-ups (source_status origin unstable-marker, chat_done warm-cache bypass, sidebar re-broadcast, capture-harness theme) remain out of scope / filed as follow-ups (#463) as the Arbiter itself scoped them. Screenshot URLs re-pinned to 8419fa69. No code changed for false positives — both items were legitimate.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Review round — disposition (head 6bb8210f6f671f09, GPT 5.6 BLOCK)

Addressed every finding GPT 5.6 raised across its three passes on 8419fa69 (two HIGH + several MEDIUM). All legitimate — no false positives. Rebase state unchanged (still one commit on fix/pr-state-sync; not CONFLICTING/BEHIND, so not re-rebased). Local gates green on the worktree .venv: pytest 17,277 passed (only the 3 pre-existing host-only test_dashboard_origin.py::TestParseDashboardUrlMalformed cases fail locally — they pass in CI), isort / flake8 / mypy (473 files) clean, tsc -b clean, vitest 4,523 passed.

The backend findings all pointed at one root cause: the prior rounds bought chip↔panel coherence by making BOTH projections lossy (allow_failure→skipped, aggregate manual→passed, locked→closed, job-rollup as the CI source). That hid real signal. The fix decouples the two concerns structurally:

HIGH 1 — source_status updated only React Query, not the Redux sidebar chips (useWebSocket.ts:685) — FIXED.
The sidebar renders PR chips from the Redux slots payload (source_links[].state/ci), not react-query, so a delta only patched the Changes strip + detail panel and left the sidebar glyph stale — the exact chip↔panel divergence this PR removes, recreated on the sidebar. Added a patchSlotSourceLinks(url, state?, ci?) reducer and dispatch it from the source_status handler, so both caches update on every delta. New test asserts the react-query batch AND the Redux source_links entry both move.

HIGH 2 — production wiring lacked regression coverage (chat_runner.py, server.py) — FIXED.
Tests called refresh_slot_source_status / register_status_delta_sink directly, so deleting the wiring would stay green. Added test_idle_turn_boundary_refreshes_source_status (drives real _run_chat to the idle branch, asserts refresh_slot_source_status(slot.key) fires) and extracted _wire_status_delta_sink(app, state) (called from start_dashboard) with test_wire_status_delta_sink_registers_and_cleans_up (registers the owner-scoped sink AND unregisters it on app shutdown).

MEDIUM — GitLab CI glyph now derives from the pipeline AGGREGATE on both paths; per-job buckets stay faithful (source_providers.py) — covers allow_failure (:640), aggregate manual (:2454), and truncated jobs (:1097).
Split the vocabulary: _gitlab_status_bucket is now the FAITHFUL per-job display mapping (a failed allow_failure job shows failed in the Checks list, no more false "all checks passed"), and _gitlab_aggregate_ci is the authoritative, lossless glyph projection called by BOTH the chip path (reads the aggregate) and the full-payload path (via a new ciStatus stamped in _fetch_gitlab). Because both sides project the CI glyph from the same aggregate, they cannot diverge — regardless of the mapping. Consequences: allow_failure folds into success→passed (glyph) while the job stays red (list); a blocking manual aggregate → running (not passed); a full jobs page is marked partial and never poisons the glyph (the aggregate stays authoritative). New tests: test_gitlab_aggregate_ci_vocabulary, test_gitlab_check_bucket_is_faithful, test_fetch_gitlab_full_jobs_page_keeps_aggregate_authoritative, plus updated projection/allow_failure/manual coherence tests.

MEDIUM — locked mapped to terminal closed (:619) — FIXED.
locked is GitLab's transient mid-merge state. _project_state now returns no lifecycle for it (both paths agree on "no change") instead of painting a false "closed" glyph; the next read resolves to merged/closed once GitLab settles.

MEDIUM — flap tracker miscounted across full-payload writes (:2079) — FIXED.
record_full_payload_status now clears the URL's flap tracker when it changes the cached status (_clear_check_flap), so an interleaved authoritative full write is not mistaken for a repeating chip transition and cannot falsely damp three legitimate CI re-runs.

MEDIUM — background-slot detail queries left permanently fresh (useWebSocket.ts:585) — FIXED.
The turn-boundary chat_done handler now invalidates PR detail/status queries for background slots with refetchType: 'none' (mark stale, refetch on next mount) and the active slot with an immediate refetch, so a staleTime: Infinity detail query no longer renders pre-turn data when the user later switches to that session.

MEDIUM — in-flight status poll could clobber the pushed delta (pass 3, useWebSocket.ts:685) — FIXED.
The source_status handler now cancelQueries(['pull-request-statuses']) before setQueriesData, so an older in-flight poll cannot resolve afterward and overwrite the authoritative delta for a TTL.

MEDIUM — forced turn-boundary refresh could be satisfied by a pre-turn in-flight fetch (:2333) — FIXED.
request_check_refresh_now now burns the force floor ONLY for URLs it actually started; a URL whose (possibly pre-turn) fetch was already in flight is not floor-stamped and is added to _check_force_pending, so _refresh_check_status issues exactly one follow-up forced read on completion instead of pinning stale data for the floor interval. New tests test_forced_refresh_inflight_not_floored_and_requeues and test_refresh_check_status_issues_pending_follow_up_force.

Screenshot URLs re-pinned to 6f671f09. The structural refactor (single URL-keyed store, Design finding #2) remains filed as #463 as previously scoped.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round: rebase to unblock Claude AI Review (no code change).

Every mechanical check and GPT/Design were green; the only red was Claude AI Review, which cancelled at the 40-minute wall clock on four consecutive attempts against head 6f671f09. The Arbiter sits pending behind it, waiting for all reviewers to post.

Diagnosis: the branch had drifted 39 commits behind main, inflating the two-dot diff a reviewer fetches to 207 files against this PR's actual 20. That is the same failure mode fixed on #464 (188 → 27 files) and #383 — the reviewer exhausts its budget re-scanning unrelated churn rather than the change under review.

Rebased onto current main (clean, no conflicts, still one commit) and force-pushed as e23f0d91. The two-dot diff is now 20 files, matching the PR. No source changes in this round; screenshot URLs re-pinned to the new SHA.

Gates on the rebased tree, all green: pytest 17,378 passed / 0 failed, isort, flake8, mypy (474 files) clean, tsc -b clean, vitest 4,584 passed across 397 files.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Jul 27, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Jul 27, 2026
…panel

The sidebar chips and the Changes-strip detail panel read two independent
caches that nothing invalidated on an agent turn, so they could render
different lifecycles for the same PR indefinitely:

- the full-payload cache (30s TTL) and the chip cache (60s TTL) never
  cross-populated, so each could be "fresh" and still disagree;
- the chip sweep is TTL-paced and admits at most CHECK_STATUS_PENDING_MAX
  URLs per round, so with more PR-linked slots than that cap a given chip
  lagged by minutes;
- the detail query is staleTime: Infinity with no refetch triggers, so
  after mount it only ever updated on a manual Refresh.

Unify the caches: a completed full fetch projects state/draft/checks onto
the chip cache (status_from_full_payload / record_full_payload_status),
and a chip refresh that observes a CHANGED status drops the now-stale
full payload so the panel cannot serve a lifecycle the chip has passed.
An unchanged status leaves the payload alone.

Refresh at turn boundaries: on the idle transition, DashboardState.
refresh_slot_source_status re-reads the finishing slot's chip URLs via
request_check_refresh_now, which bypasses the chip TTL. Gated on an owner
websocket being open (status is credential-backed and nobody else can
render it, so a headless gateway spawns no provider work), scoped to the
one slot, floored to one forced read per URL per 10s, and best-effort so
it can never break turn completion.

Push instead of poll: a changed status emits the owner-only source_status
WS event {url, origin, ci?, state?}. origin="chip" means the lightweight
path knows something the payload does not, so the client patches its
status batch AND invalidates the detail queries; origin="detail" means a
full fetch produced it, so the client patches chips only -- otherwise a
detail fetch's own echo would refetch the query that emitted it. The
client additionally invalidates the mounted pull-request queries on
chat_done for the active slot, since lifecycle/CI deltas do not cover
review comments or mergeability. Polling remains the safety net.

Tests: 8 backend cases for the projection, both write-through directions,
TTL bypass + force floor, sink dedup/failure isolation and ledger
bounding; 5 for per-slot scoping, owner gating, failure swallowing and
owner-only delta delivery; 14 frontend for delta parsing/merging and the
WS wiring.
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Jul 27, 2026
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Conflict resolved by rebase — the hook moved with main's refactor.

Branch is single-author, so rebase (not merge). It was 14 commits behind and CONFLICTING in src/kiro_crew/dashboard/chat_runner.py.

The collision was structural, not textual: main extracted the idle-transition finalization block (slot.append("done")push_slots_updatebroadcast_ws("chat_done")push_refresh("history") → auto-title) out of the inline queue-drain path and into a new _finish_queue_cycle() helper, reached via _start_next_queued_turn. This PR's turn-boundary hook was patched onto the old inline site, which no longer exists.

Resolution: took main's refactored call site verbatim, and re-homed state.refresh_slot_source_status(slot.key) inside _finish_queue_cycle() immediately after the chat_done broadcast. Semantics are unchanged — that helper is the idle transition now, so the hook still fires exactly once per completed turn.

Checked the other two chat_done sites main has and deliberately left both alone, consistent with the original scope: the Kiro-readiness error abort (~line 2006) goes idle without a turn having run, so no PR can have moved; the /compact flush (~line 4165) is mid-turn, not a boundary.

Pushed as 8c35ba9a with --force-with-lease (lease held). Screenshot URLs re-pinned. MERGEABLE again.

Gates on the resolved tree: pytest 17,924 passed, isort / flake8 / mypy (481 files) clean, tsc -b clean, vitest 4,666 passed across 404 files. One failure, test_apps_registry.py::test_fetch_app_manifest_reaps_clone_tree_on_timeout, is a load-sensitive timeout unrelated to this diff (passes in isolation; this PR touches nothing under the apps registry).

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Jul 27, 2026
@iamwhatever
iamwhatever merged commit 5fa0a2b into main Jul 27, 2026
44 of 46 checks passed
@iamwhatever
iamwhatever deleted the fix/pr-state-sync branch July 27, 2026 14:57
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Jul 27, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…panel (kirodotdev#443)

The sidebar chips and the Changes-strip detail panel read two independent
caches that nothing invalidated on an agent turn, so they could render
different lifecycles for the same PR indefinitely:

- the full-payload cache (30s TTL) and the chip cache (60s TTL) never
  cross-populated, so each could be "fresh" and still disagree;
- the chip sweep is TTL-paced and admits at most CHECK_STATUS_PENDING_MAX
  URLs per round, so with more PR-linked slots than that cap a given chip
  lagged by minutes;
- the detail query is staleTime: Infinity with no refetch triggers, so
  after mount it only ever updated on a manual Refresh.

Unify the caches: a completed full fetch projects state/draft/checks onto
the chip cache (status_from_full_payload / record_full_payload_status),
and a chip refresh that observes a CHANGED status drops the now-stale
full payload so the panel cannot serve a lifecycle the chip has passed.
An unchanged status leaves the payload alone.

Refresh at turn boundaries: on the idle transition, DashboardState.
refresh_slot_source_status re-reads the finishing slot's chip URLs via
request_check_refresh_now, which bypasses the chip TTL. Gated on an owner
websocket being open (status is credential-backed and nobody else can
render it, so a headless gateway spawns no provider work), scoped to the
one slot, floored to one forced read per URL per 10s, and best-effort so
it can never break turn completion.

Push instead of poll: a changed status emits the owner-only source_status
WS event {url, origin, ci?, state?}. origin="chip" means the lightweight
path knows something the payload does not, so the client patches its
status batch AND invalidates the detail queries; origin="detail" means a
full fetch produced it, so the client patches chips only -- otherwise a
detail fetch's own echo would refetch the query that emitted it. The
client additionally invalidates the mounted pull-request queries on
chat_done for the active slot, since lifecycle/CI deltas do not cover
review comments or mergeability. Polling remains the safety net.

Tests: 8 backend cases for the projection, both write-through directions,
TTL bypass + force floor, sink dedup/failure isolation and ledger
bounding; 5 for per-slot scoping, owner gating, failure swallowing and
owner-only delta delivery; 14 frontend for delta parsing/merging and the
WS wiring.

Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants