feat(issue-radar): record an empty queue without an issue number - #7655
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS A numberless sweep line renders honestly — em dash for the absent issue, muted "Swept" badge, translated in all locales — and the screenshot confirms it reads as background against real work. [UX-REVIEWED] 2903b88 |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A real ledger gap closed at its root — the API shape — with the bidirectional kind/number pairing, id stability, and coalescing all keeping it from becoming a second dishonest write path. The two residual risks are already named and deferred with rationale in the diff itself (the coalesce read holding the exclusive lock across the whole uncompacted events file, and the surviving timestamp carrying no liveness meaning), so neither changes what the author would build here. [DESIGN-REVIEWED] 2903b88 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All claims verified: First-Principles-Verdict: CONCERNS A "latest-value fact, not an event" stored in an event log, then coalescing built to undo the mismatch — the crew-record field alternative is never weighed. What this change shipsIntent: let a crew record "checked the queue, took nothing" without inventing an issue number — an ADDITION, framed as one (
Watch
[FIRST-PRINCIPLES-REVIEWED] 2903b88 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
369f244 to
14445ac
Compare
Review dispositions -- head
|
14445ac to
73163f9
Compare
Design Review disposition -- head
|
73163f9 to
1e9e966
Compare
Review dispositions -- head
|
1e9e966 to
c93c99e
Compare
UX Review disposition -- head
|
c93c99e to
f76d65d
Compare
Review dispositions -- head
|
f76d65d to
aaf3145
Compare
Disposition: GPT 5.6 blocking finding -- FIXED in
|
Status on
|
de190ab to
19cab90
Compare
Disposition: Design and First Principles CONCERNS on
|
19cab90 to
529d635
Compare
Disposition: GPT 5.6 blocking finding on
|
Disposition: Design + First Principles on
|
529d635 to
09a25e1
Compare
Decision on the escalation: option 2 taken -- the ongoing wording is removed, and
|
A crew that swept its queue and took nothing had no way to say so. The write path required an issue number because that number becomes the work item's filename, so the only way to report the cycle was to attribute it to an issue the crew never acted on -- the ledger either lied or stayed silent, and a resumed crew could not tell those apart. Adds one crew-level event kind, `sweep`, and makes `number` optional on the write path. A numberless write appends one ledger line and nothing else: no work item, no skip-index entry, no work-item fields accepted. The pairing is enforced in both directions -- a missing number is valid only with `sweep`, and `sweep` is invalid with one -- so neither shape can drift into the other's meaning. The event id formula is unchanged for a numbered line: it is content-addressed and drives merge-on-read dedupe, so a new formula would have given every existing line a fresh id. Refs #5905
09a25e1 to
2903b88
Compare
Disposition: First Principles CONCERNS on
|
Disposition: First Principles CONCERNS on
|
buluoray
left a comment
There was a problem hiding this comment.
Verdict: 0 blocking, 2 non-blocking. Reviewed at head 2903b88f3306ea6def6dd3baa96c2b47753715fb.
This relaxes a required identifier to optional, so I traced every reader of a ledger line's number to confirm the absent key never crashes, formats as None/#undefined, or collides with a real record. It does not.
What I verified
- Every consumer of an event line's
numbertolerates absence.crew_store.read_eventsreads with.get, dedupes by id, tolerates a missing key; it never touchesnumber, so asweepline passes through cleanly (crew_store.py).- The fabric fold (
crew_store.py,by_key.setdefault((cid, num), ...)) guards withisinstance(num, int)AND reads withrequire_phase=True; asweepcarries no phase, so it is excluded twice. _fabric_title_hintsand the work-item fold both guardisinstance(num, int)before usingnumas a dict key.- The skip index is untouched: a
sweepwrites no skip entry (record_crew_checkpointreturnsskip=None), soread_skips/skipped_numbersnever see it. - Backend hands raw event dicts straight to the frontend (
crew_routes.py:590), so the only UI consumer is the renderer:CrewPageView.tsxissueCell(number)returns an em dash whennumber === undefinedand#${number}otherwise. Both render sites (recent + history tables) were switched from the old#{e.number}, so no path prints#undefined.CrewEvent.numberisnumber?(optional, not nullable) inapi.ts, matching the backend omitting the key.
- Storage round-trips and old records still load. The line is written by
json.dumpswith the key simply absent and read back byread_events, which requires nonumber; a pre-change numbered line still loads unchanged. - The numbered path is byte-identical.
_event_idrendersint(number)for a numbered line (unchanged),_event_entrypreserves the original key order (id, ts, crew_id, number, kind, text, [phase]),append_eventstill typesnumberas requiredint, and the route's numbered branch still parses via_pr_number_field. A present-but-invalid number is still a 400 (crew_routes.py), not reinterpreted as "no issue". - Pairing is enforced both directions in the route (
number_required/unexpected_number,crew_routes.py:~925-940) and in the store (_event_entry), and work-item fields on the numberless path are refused, not dropped (item_fields_without_number). The identity gate still 403s a non-crew session on the new path (route test). - Coalescing is correct.
record_crew_checkpointreads the crew's newest line and appends only if it is not already asweep, both under one exclusive events-lock hold (crew_store.py:1236);coalescedis returned to the caller._latest_crew_eventusesread_events(..., limit=1)which is newest-first, so[0]is genuinely the newest line. - Tests pin the behavior, not just non-crash.
test_the_number_key_is_omitted_from_the_requestasserts"number" not in body; the route test assertsassertNotIn("number", ledger[0])andcoalescedisFalse;test_a_numbered_call_still_sends_and_reports_its_numberassertsbody["number"] == 12. These fail if the change is reverted (consistent with the PR's reported 14 RED onorigin/main). - Schema/docs match the contract.
validation.pykeeps themin_val=1/max_valbound and only dropsrequired=True. The MCP tool description (mcp_tools/apps.py) tells the agent to sendevent_kind: sweepwith no number and no work-item fields, and the summary path branches on"number" in argsso bothargs["number"]reads are guarded (no KeyError). Ledger spec and crew brief are updated in the same commit.
Findings (non-blocking)
-
crew_store.py:1226(record_crew_checkpoint) — Consequence: every idle nudge reads the entire repo-wide events file under the exclusive events lock, and nothing compacts that file today, so the lock hold grows with total ledger volume. Coalescing bounds it to one append per idle stretch, and the cost is disclosed in-code. Suggestion: none required for this PR; the owed compaction story can be a follow-up. Not blocking — no user-facing defect on the changed path. -
crew_store.py:123/:1156— Design-plane observation, matching the First Principles CONCERNS: a "latest-value fact" (checked, took nothing) is stored in an append-only event log with coalescing to compensate, where alast_swept_atfield on the crew record could answer the same question with less surface. This is a design choice the author has already dispositioned (decided twice, last-seen datum deferred as separate scope). Not blocking — the shipped line removes the named misattribution harm and discloses its limits.
What I could not verify
- I did not run the test suite or mutation-test the new guards locally (read-only shared checkout); I confirmed the assertions pin the absent-key behavior by reading them, and CI Backend Tests are green on this head.
- The rendered screenshot/i18n gates I judged only via the green CI checks and the diff, not by rendering the page myself.
Problem / Motivation
An Issue Radar crew that swept its queue and took nothing had no way to record
that it had looked.
PUT /crew/workrequired an issue number, and theissue_radar_crew_recordMCPtool declared
numberas its one required field. The requirement exists for areal reason -- the number becomes the work item's filename
(
crews/<crew_id>/<n>.json) -- but it also meant a crew reporting ano-work cycle had to name an issue it never acted on. So the ledger either
carried a work item and a progress line attributing the cycle to an unrelated
issue, or the cycle was not recorded at all. A resumed crew reading its own
ledger could not tell those two apart.
Nothing filled the gap elsewhere. The crew record has no heartbeat or
last-checked field, and
crew_runtime.sweep_reporeturns an empty resultsilently when nothing is due, so a sweep that took nothing left no trace on
either plane.
Why it matters
The ledger is what a crew reads to re-establish state after a compaction or a
restart, so a line it cannot write is state it cannot recover. Two concrete
consequences:
to take" from "I have not checked yet", which is exactly the question its next
cycle needs answered.
the crew never touched. Those lines are also rendered publicly, so the
workaround made the crew page and the forge misreport what happened.
This was identified as a real API-shape gap during triage on #5905, independently
of that issue's other claims.
What changed (motivation -> approach -> change)
The write path now accepts a step that belongs to no issue.
crew_store.EVENT_KINDSgains one crew-level kind,sweep, named through asingular
CREW_LEVEL_EVENT_KINDconstant rather than a set -- there is exactlyone such kind, and a one-member set would invite ad-hoc membership checks for a
vocabulary that has none.
append_eventstill REQUIRES a number, typedint. The numberless line iswritten only by
record_crew_checkpoint, so there is one path to that shaperather than two. The stored line OMITS the
numberkey rather than writing anull or coercing to
0: a0would be indistinguishable from a real issuenumber in every filter and join that keys on the field, while an absent key is
what tells a reader the line is about no issue.
record_crew_checkpointis a new one-write store function returning the same{item, event, skip}envelope withitemandskipasNone, so the routeanswers one shape and no caller branches on which kind of write it made. It is
deliberately not a branch inside
commit_work_progress: that function existsto make three durable writes all-or-nothing, and a single line has no
transaction to reconcile.
PUT /crew/worktreats a missingnumberas the selector for the crew-levelpath, and refuses work-item fields (
phase,ci_state,skip_scope, ...)there rather than dropping them -- honouring the line while discarding the
patch would report a phase move that was never stored.
missing number is valid only with a crew-level kind, and a crew-level kind is
invalid with one. Accepting the first would only move the fabricated number
into the store; accepting the second files a sweep under an issue it never
touched, which is the same false attribution written the other way round.
numberis no longer required on the MCP tool or in its validator schema, butkeeps its bound for when it IS sent. The tool description tells the agent to
send
event_kind: sweepwith no number and never to invent one.and drives merge-on-read dedupe, so a new formula would have given every
existing line a fresh id and silently defeated dedupe for the whole ledger.
The numberless variant renders the number as the empty string, which no real
number produces, so the two families cannot collide.
latest-value fact and a crew is nudged on a timer, so one line per idle cycle
would be an unbounded run -- and because every ledger read is capped and drops
the OLDEST line first, a crew idling a couple of hundred cycles would push its
real work history out of the log this change exists to make honest. The first
sweep after real work is written; a sweep whose crew already has one as its
newest line is answered with that existing line, and the response carries
coalesced. The surviving timestamp therefore marks when the idle stretchBEGAN. This is the same record-the-transition discipline
commit_work_progressalready applies to
phase. The tail check and the append share ONE hold of theevents lock, since two holds would let two crew turns waking together both
append.
_event_entryis the single place a line isvalidated and built; the issue-line writer requires a number and the crew-level
writer is the only numberless path, so there is no second way to write the same
shape.
Frontend: the
CrewEvent.numberfield becomes optional and the Issue columnrenders an em dash instead of a bare
#for a crew-level line. The WHEN columnstates when the line was written and nothing more. An earlier revision of this PR
qualified the newest sweep as
checking since ..., and that was removed afterDesign Review pointed out it is a present-tense activity claim backed by no
evidence of life: a crew that crashes or loses its nudge timer keeps
enabledtrue, so the row would have asserted ongoing checking for a crew that had stopped
-- masking the one failure an operator opens this page to notice, and reading worse
than the bare timestamp it replaced. Closing that honestly needs a real last-seen
datum, which is a separate change.
sweepis added to the kind vocabulary andlabel map, and its label is translated in all 13 catalogs.
A present-but-invalid number is still a 400 and is NOT reinterpreted as "no
issue", so a typo cannot be silently recorded as a queue sweep.
Tests
Targeted files only, run with a bounded
-n 2.test/test_issue_radar_crew_store.py-- new section for crew-level lines: thenumber key is omitted rather than zeroed; an item kind with no number is
refused; a crew-level kind with a number is refused; the numbered id formula is
unchanged and the two id families cannot collide;
record_crew_checkpointwrites one line and creates no work item.
test/test_issue_radar_crew_routes.py-- newTestACrewCanRecordAnEmptyQueuedriving the registered handler: a sweep needs no number and writes no work
item; both halves of the pairing are refused; a present-but-invalid number
stays
invalid_number; work-item fields are refused rather than dropped; asweep still needs a reason; a sweep from a non-crew session is still 403, so
the new path is not a way around the identity gate.
test/test_issue_radar_crew_mcp_tools.py-- newTestACrewCanReportAnEmptyQueueplus updated registration assertions: the tool validates without a number, the
key is omitted from the request, the summary names the crew instead of printing
a bare
#, and a numbered call still sends and reports its number.Results: 507 passed across the nine crew/identity test files; 688 passed across
the validation suites. The new behavioural tests were confirmed RED on
origin/mainby revertingsrc/to base and re-running them (14 failed, 3passed -- the 3 are the regression guards that are meant to hold on base).
Gates run locally and clean:
flake8,isort --check-only,mypy src/kiro_crew/(1237 files, no issues), and
scripts/check_black_formatting.py(22 changed filesin scope, nothing unformatted outside the baseline). All 13 edited catalogs were
re-parsed as JSON.
Screenshots / video
The crew work log with a crew-level
sweepline at the top, beside the numberedrows it has to be distinguishable from. The Issue column renders an em dash
rather than a bare
#, and the OUTCOME badge carries the newSweptlabel inthe same muted register as the other did-not-act kinds.
Captured from the real view, not a mock-up:
website/capture/crew-work-log-sweep.tsxmounts
CrewPageViewinside the realIssueRadarProviderwith onlyGET /api/apps/issue-radar/crewstubbed, so the frame uses the shippedcomponent, classes, theme tokens and catalog. The capture script asserts the
sweep row's Issue cell IS an em dash and the numbered sibling IS
#2251beforeit shoots, so a harness that stopped rendering the row cannot produce a green
screenshot.
Manual verification
The frontend toolchain was installed and every gate this diff can reach was run
locally, all clean:
npx tsc -b;npx eslint src/ --max-warnings 603(0 errors,exactly 603 warnings, so the ratchet is untouched);
npx jscpd .(0 clones, sothe new capture harness needs no exemption); and
npm run i18n:check, whichreports 19 checks PASS including the three hard zeros
[pseudolocale],[dnt]and[manifest-sync]and the zero-tolerance[added-lines],[source-strings]and[changed-values].website/src/test/IssueRadarCrewPage.test.tsxgains two assertions against thereal view -- a crew-level row renders an em dash and contains no
#while itsnumbered sibling still renders
#2251, and the new kind is translated throughthe catalog rather than shown as a raw token. 13 tests pass in that file.
One earlier iteration is worth recording because it is the kind of thing a
reviewer would otherwise have to catch: the
en-XApseudolocale entry washand-written first, and
[pseudolocale]is a hard zero that failed on it. It isnow produced by
node scripts/gen-pseudolocale.mjs, which is the only correctsource, and the diff to that catalog is the single generated line.
Backend consumers of a ledger line's number were checked for tolerance of the
absent key: the fabric fold already guards with
isinstance(num, int)and skipsthe line, and it reads with
require_phase=Truewhile a sweep carries no phase,so it is excluded twice over.
Related Issues
Refs #5905
Deliberately a
Refstrailer. This change implements one acceptance criterionfrom that issue -- recording an empty queue without a fabricated issue number --
and stands on its own. The issue's headline claim about session identity is a
separate matter still under discussion there, so the issue should stay open.
Pattern harvest
The reusable shape here is pairing a vocabulary with a payload shape in both
directions. Making a required field optional is usually written as one relaxed
check, which then lets the two shapes drift into each other's meaning: a missing
number with an item kind, or a crew-level kind carrying a number, are the same
false attribution written two ways. Enforcing both halves -- and at the two
layers that can see the relation, since a per-field schema validates one field at
a time and cannot express it -- keeps the relaxation from becoming a second way
to write a dishonest record.
Second, smaller: when a content-addressed id gains a nullable component, render
the null as a value the non-null domain cannot produce and leave the existing
formula byte-identical. Changing the formula for existing inputs would have
re-issued every id in an append-only log and silently disabled its dedupe.