Skip to content

feat(issue-radar): record an empty queue without an issue number - #7655

Merged
iamwhatever merged 1 commit into
mainfrom
fix/radar-worker-ledger-5905
Sep 2, 2026
Merged

feat(issue-radar): record an empty queue without an issue number#7655
iamwhatever merged 1 commit into
mainfrom
fix/radar-worker-ledger-5905

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

An Issue Radar crew that swept its queue and took nothing had no way to record
that it had looked.

PUT /crew/work required an issue number, and the issue_radar_crew_record MCP
tool declared number as its one required field. The requirement exists for a
real reason -- the number becomes the work item's filename
(crews/<crew_id>/<n>.json) -- but it also meant a crew reporting a
no-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_repo returns an empty result
silently 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:

  • A crew could not distinguish "I checked this repository and there was nothing
    to take" from "I have not checked yet", which is exactly the question its next
    cycle needs answered.
  • Recording the cycle against an arbitrary issue puts a progress line on an issue
    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_KINDS gains one crew-level kind, sweep, named through a
    singular CREW_LEVEL_EVENT_KIND constant rather than a set -- there is exactly
    one such kind, and a one-member set would invite ad-hoc membership checks for a
    vocabulary that has none.
  • append_event still REQUIRES a number, typed int. The numberless line is
    written only by record_crew_checkpoint, so there is one path to that shape
    rather than two. The stored line OMITS the number key rather than writing a
    null or coercing to 0: a 0 would be indistinguishable from a real issue
    number 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_checkpoint is a new one-write store function returning the same
    {item, event, skip} envelope with item and skip as None, so the route
    answers 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 exists
    to make three durable writes all-or-nothing, and a single line has no
    transaction to reconcile.
  • PUT /crew/work treats a missing number as the selector for the crew-level
    path, 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.
  • The pairing is enforced in BOTH directions, in the route and in the store: a
    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.
  • number is no longer required on the MCP tool or in its validator schema, but
    keeps its bound for when it IS sent. The tool description tells the agent to
    send event_kind: sweep with no number and never to invent one.
  • 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 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.
  • Consecutive sweeps coalesce. "Checked, took nothing" is a recurring
    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 stretch
    BEGAN. This is the same record-the-transition discipline commit_work_progress
    already applies to phase. The tail check and the append share ONE hold of the
    events lock, since two holds would let two crew turns waking together both
    append.
  • One writer shapes a ledger line. _event_entry is the single place a line is
    validated 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.number field becomes optional and the Issue column
renders an em dash instead of a bare # for a crew-level line. The WHEN column
states 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 after
Design 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 enabled
true, 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. sweep is added to the kind vocabulary and
label 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: the
    number 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_checkpoint
    writes one line and creates no work item.
  • test/test_issue_radar_crew_routes.py -- new TestACrewCanRecordAnEmptyQueue
    driving 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; a
    sweep 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 -- new TestACrewCanReportAnEmptyQueue
    plus 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/main by reverting src/ to base and re-running them (14 failed, 3
passed -- 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 files
in 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 sweep line at the top, beside the numbered
rows it has to be distinguishable from. The Issue column renders an em dash
rather than a bare #, and the OUTCOME badge carries the new Swept label in
the same muted register as the other did-not-act kinds.

Crew work log showing a Swept row with an em dash in the Issue column above three numbered rows

Captured from the real view, not a mock-up: website/capture/crew-work-log-sweep.tsx
mounts CrewPageView inside the real IssueRadarProvider with only
GET /api/apps/issue-radar/crew stubbed, so the frame uses the shipped
component, classes, theme tokens and catalog. The capture script asserts the
sweep row's Issue cell IS an em dash and the numbered sibling IS #2251 before
it 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, so
the new capture harness needs no exemption); and npm run i18n:check, which
reports 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.tsx gains two assertions against the
real view -- a crew-level row renders an em dash and contains no # while its
numbered sibling still renders #2251, and the new kind is translated through
the 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-XA pseudolocale entry was
hand-written first, and [pseudolocale] is a hard zero that failed on it. It is
now produced by node scripts/gen-pseudolocale.mjs, which is the only correct
source, 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 skips
the line, and it reads with require_phase=True while a sweep carries no phase,
so it is excluded twice over.

Related Issues

Refs #5905

Deliberately a Refs trailer. This change implements one acceptance criterion
from 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.

@chenmingwei23
chenmingwei23 requested a review from a team September 1, 2026 15:18
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 1, 2026 15:18
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 2903b88f3306ea6def6dd3baa96c2b47753715fb — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 2903b88f3306ea6def6dd3baa96c2b47753715fb — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 2903b88f3306ea6def6dd3baa96c2b47753715fb — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified: read_events(crew_id=, limit=) pre-existed, no crew-level last-checked mechanism exists, the capture/screenshot convention is established repo practice (375 capture scripts, 593 committed screenshots), and _WORK_PATCH_FIELDS covers the item fields the stray-check names. The one first-principles tension: the change's own docstring calls the datum "a recurring LATEST-VALUE fact, not an event," yet stores it in an append-only event log and then builds coalescing machinery to compensate — while a crew-record field would have answered the motivating question with less surface. That's a smaller-alternative concern, not a blocker, since the ledger line does remove the named public-misattribution harm and the description discloses its limits honestly.

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 ships

Intent: let a crew record "checked the queue, took nothing" without inventing an issue number — an ADDITION, framed as one (feat), motivated by a triage-identified gap.

  1. A crew can record an empty-queue cycle (sweep, no number) — justified; no existing mechanism (grepped last_checked|heartbeat|last_swept: none crew-level)
  2. number optional on the MCP tool and route — justified; entailed by 1
  3. Sweep/number pairing refused in both directions, three new error codes — justified; blocks re-fabrication
  4. Consecutive sweeps coalesce; response gains coalesced — justified in-shape (see Watch); 1 real consumer (mcp_tools/apps.py summary)
  5. Work-item fields refused on the numberless path — justified; silent drop would fake a stored phase
  6. Crew page renders em dash + translated Swept badge in 13 catalogs — entailed by 1; i18n gate mandates catalogs
  7. Store refactor: _event_entry single builder, record_crew_checkpoint, lock-hold split — mechanism-level, justified
  8. Capture harness + committed screenshot — follows established convention (375 sibling capture scripts)
  9. Ledger spec + crew brief updated same-commit — mandated by AGENTS.md
  10. Liveness/"checking since" explicitly deferred — honest; level stated

Watch

  • The docstring concedes the premise: "'I checked and took nothing' is a recurring LATEST-VALUE fact, not an event." The description also notes "the crew record has no heartbeat or last-checked field" — names that gap, then doesn't weigh filling it. A last_swept_at field on the crew record would remove the same harms (resumed crew knows it checked; nothing is misattributed publicly) with no new event kind, no pairing enforcement, no coalescing, no id-formula extension, and no 13-catalog entry — and would preserve the recency the fold discards: the surviving stretch-begin timestamp cannot answer "how recently did I check", which the description itself names as "exactly the question its next cycle needs answered". The field is also where the deferred last-seen work will land, so this surface may be partially superseded by its own follow-up.
  • Disclosed in-code and real: every idle nudge now reads the entire repo-wide events file under the exclusive events lock ("a compaction story is still owed") — a cost the field alternative would not pay.

[FIRST-PRINCIPLES-REVIEWED] 2903b88

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2903b88f3306ea6def6dd3baa96c2b47753715fb and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2903b88

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2903b88f3306ea6def6dd3baa96c2b47753715fb — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 2903b88

Verdict parsed from the review's SHA-scoped output markers for commit 2903b88f3306ea6def6dd3baa96c2b47753715fb.

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

@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch 3 times, most recently from 369f244 to 14445ac Compare September 1, 2026 15:59
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Review dispositions -- head 14445acefe7afca8762a61c3b2ea131beb14e7f4

Both First Principles items are taken. Neither was a false positive.

FIXED -- crew_ledger_spec.md contradicted the shipped API

The finding is correct and this was a real miss: AGENTS.md requires the spec
updated in the same commit as the behavior it documents, and I changed the API
without touching its co-located spec. Two places were wrong, and a third was
incomplete:

  • :399 read number required int. Now optional int -- omit ONLY with event_kind: sweep, followed by a paragraph stating why nothing is
    unconditionally required and where the replacement coupling is enforced (the
    route and the store, because it is a relation between two fields that a
    per-field schema cannot express).
  • :417 the event_kind enum lacked sweep. Added.
  • The Event log section documented a line shape that always carried number. It
    now states that a crew-level line OMITS the key -- absent rather than null or
    0, since a 0 is indistinguishable from a real issue number -- that an
    absent number renders as the empty string in the id so the numbered formula is
    unchanged, and that a crew-level line reaches only the crew page and not a
    claim comment, because it has no issue to comment on.

FIXED -- CREW_LEVEL_EVENT_KINDS was a one-member set

Also correct, including the reasoning: the only justification its own comment
offered was a second member that does not exist, and the frontend already tests
kind === 'sweep' by equality. Collapsed to a single named constant
CREW_LEVEL_EVENT_KIND = "sweep" and the four membership tests to equality
(crew_store.py x2, crew_routes.py x2). The route's error text no longer joins
a sorted set to name one value. The constant's comment now records what would
make a collection correct -- a second real crew-level kind turns three equality
tests into a membership test then, against a set with two members.

Design Review -- transient, re-run

Its comment says "could not complete (the review step completed but returned no
verdict header)". That is a model error with no verdict to dispose of rather than
a finding, so I have re-run that workflow on this head rather than treating it as
advisory feedback.

Clean lanes, recorded so silence is not mistaken for an oversight

UX Review PASS. GPT 5.6: no findings. Opus 4.8: no findings, and it independently
reached the same conclusion I had put in the PR body about the one consumer that
could have mattered -- the fold at crew_store.py filters on require_phase=True
(a sweep carries no phase) and additionally guards isinstance(num, int), so a
numberless line cannot reach a number-keyed join.

Verification after both fixes: 319 passed across the three targeted crew test
files with -n 2; flake8, isort --check-only and mypy src/kiro_crew/ (1237
files) clean; the baselined black gate passes with 28 files in scope.

@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from 14445ac to 73163f9 Compare September 1, 2026 16:23
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Design Review disposition -- head 73163f933414ca6ccdc84c991b053e5784e4b1db

FIXED -- an idle crew would have buried its own work log

Design Review's CONCERNS on 14445acef is correct, and it found the one real
steady-state defect in this change:

A recurring latest-value fact ("last checked, nothing to take") is stored as
unbounded append-only ledger lines, so idle crews will slowly fill capped reads
with sweeps.

I had not accounted for the write rate. A crew is nudged on a timer, so an idle
one would have appended one sweep per cycle forever, and every ledger read is
capped and discards the OLDEST line first (_DEFAULT_EVENTS is 200). A crew
idling a couple of hundred cycles would therefore have pushed its entire real work
history out of the work log -- the feature would have buried exactly what the log
exists to show. That is worse than the problem it set out to fix, so it is fixed
here rather than deferred.

Consecutive sweeps now coalesce. 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: true. Three consequences worth stating:

  • The surviving line is the FIRST, so its timestamp marks when the idle stretch
    BEGAN. "Nothing to take since 09:12" is the more useful reading than "nothing to
    take as of a minute ago", and how long a quiet crew has been quiet is the
    question a human opening it is actually asking.
  • This is the same record-the-transition discipline commit_work_progress already
    applies to phase, which is stamped only when an item is created or actually
    moves -- and for the same reason, so a no-move write cannot reset what the log
    is read for. I took the existing pattern rather than inventing one.
  • The tail check and the append happen under ONE hold of the events lock. Two
    holds would be a check-then-act: two crew turns waking together would both see
    no trailing sweep and both append, which is the exact run of duplicates this
    prevents. _write_event_line and _latest_crew_event exist so the read and the
    write can share that hold without re-entering append_event and blocking on the
    lock this frame already holds.

The MCP tool now tells the crew which happened, because a crew that believed each
idle cycle added a line would misread its own log's length as its cycle count.

Coverage added, all against the real store and the registered route:
consecutive sweeps coalesce and the second call returns the first line's id; a
sweep AFTER real work IS written (so the transition back into idle is not
swallowed); one crew's sweep does not coalesce another's, since the ledger file is
repo-wide; a non-crew-level kind is refused by the checkpoint writer; and at the
route level, two idle cycles leave exactly one line. 365 passed across five crew
test files with -n 2; flake8, isort, mypy (1237 files) and the baselined
black gate clean. crew_ledger_spec.md records the coalescing rule in the same
commit as the behavior, per AGENTS.md.

Lanes clean on the previous head, for the record

UX Review PASS. First Principles Review PASS (it was CONCERNS; both of its items
were fixed and dispositioned above). Opus 4.8: no findings. GPT 5.6's marker still
named 369f2444d when I read it, so its verdict for this head is still pending and
I am not treating the stale marker as a result either way.

@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from 73163f9 to 1e9e966 Compare September 1, 2026 16:59
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Review dispositions -- head 1e9e96674e4c245945fe09eae70ea3aa2b79191d

Design Review flipped to PASS and Opus 4.8 is clean on the previous head. The two
new CONCERNS are both taken -- each is a real consequence of the coalescing added
to answer Design Review's earlier finding, which is the right kind of finding to
get and I would rather fix than defer.

FIXED -- UX: a coalesced sweep read as stale

A coalesced sweep row renders "idle since" data with "happened at" wording -- an
active crew reads as stale.

Correct, and it is the defect coalescing introduced. Once consecutive sweeps
collapse, the surviving timestamp marks when the idle stretch BEGAN and the crew
keeps checking after it -- so the bare "3h ago" every other kind uses made an
actively-working crew look like one that had done nothing for three hours. The
column said "happened at" about data that means "since".

The WHEN cell for a sweep now reads checking since {{when}}, a new catalog key
translated in all 13 locales (en-XA regenerated with
scripts/gen-pseudolocale.mjs, not hand-written). Every other kind is a discrete
act and keeps the plain past-instant wording, so the qualifier marks the ongoing
kind rather than decorating every row -- isOngoing() names that distinction in
one place instead of testing the kind at both render sites. The screenshot in the
PR body is re-captured and shows the contrast directly: checking since 24m ago
on the sweep row above 1h ago / 3h ago / 6h ago on the numbered ones.

Covered by a new assertion in IssueRadarCrewPage.test.tsx that the sweep row's
WHEN cell is the qualified string and the numbered sibling's is not. It builds the
expected relative string from the real formatter rather than pinning "3 hours ago"
-- that wording is locale data, so a literal would assert the formatter's current
output instead of this row's qualifier.

FIXED -- First Principles, both halves

Consecutive-sweep coalescing ships undeclared, and the store grows two writers
for the same numberless line -- one of which no production code calls.

Both correct.

Undeclared: coalescing was described only in a review comment and the spec.
The PR body now declares it under What changed, with the read-cap reasoning, the
begins-not-observed timestamp semantics, and the single-lock-hold requirement.

Two writers: accurate, and it was a direct consequence of how coalescing had
to be built. The crew-level path must read the crew's tail and decide whether to
append at all, under one lock hold, so it could not delegate to append_event --
which left append_event(number=None) reachable only from tests. Fixed by
extracting _event_entry, the single place a line is validated and shaped: the
issue-line writer now types number as int and the pairing check refuses a
crew-level kind through it, so the numberless case is unreachable there by type as
well as by check. A test asserts exactly that, and that the one legitimate writer
still produces the builder's shape.

The two shape tests that used to call append_event(..., None, ...) now target
_event_entry and record_crew_checkpoint, which is where the production shape
lives -- they were asserting a path that should not have existed.

Verification

351 passed across four crew test files with -n 2; 14 passed in
IssueRadarCrewPage.test.tsx. flake8, isort --check-only, mypy src/kiro_crew/
(1237 files) and the baselined black gate clean. Frontend: npx tsc -b clean,
npx eslint src/ --max-warnings 603 0 errors at the ceiling, npx jscpd . 0
clones, and npm run i18n:check 19 checks PASS.

One note on that last one, since it is the kind of thing worth stating rather than
quietly passing: run locally against origin/main the i18n gate reported
[changed-passthrough] FAIL 1 naming 11 untranslated apps.designCritique values.
None of them are in this diff -- origin/main has advanced past this branch's
point, so main's own newer strings were being charged here. That is precisely the
moving-base hazard ci.yml documents when it explains why the workflow uses
base.sha rather than origin/<base.ref>. Re-run against the real merge base
1ee69f225 it is 19 checks PASS.

@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from 1e9e966 to c93c99e Compare September 1, 2026 17:46
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

UX Review disposition -- head c93c99ebb23a841390d355bd2ad0a439231662c5

Design Review PASS, First Principles PASS, Opus 4.8 no findings on 1e9e96674.
UX's remaining CONCERNS is taken.

FIXED -- the qualifier was keyed on kind, so a closed stretch kept claiming to be open

"checking since..." is keyed on kind, not recency -- a finished idle stretch keeps
claiming to be ongoing, contradicted by the rows above it.

Correct, and it is a defect in my own previous fix rather than in the original
change. I keyed isOngoing on the event kind, so EVERY sweep row rendered
"checking since". That is only true of a stretch that is still running. Once the
crew takes work again, the old sweep's stretch has ended -- and the row went on
asserting an open stretch while the newer rows sitting directly above it in the
same log said otherwise. A claim on screen contradicted by its own neighbours is
worse than the bare timestamp it replaced.

isOngoing is now keyed on RECENCY. The log is already sorted newest-first, so
the memo derives one ongoingId -- the newest line's id, and only when that newest
line is a sweep -- and a row is ongoing iff it holds that id. Consequences:

  • At most ONE row in the log can ever carry the qualifier.
  • A sweep with any newer line above it renders as a plain past instant, exactly
    like every other completed act.
  • The distinction lives in one place (the memo that already owns the ordering)
    rather than being re-derived at the two render sites.

Three assertions cover it in IssueRadarCrewPage.test.tsx: the newest sweep reads
as ongoing; a sweep with newer work above it reads as a plain past instant; and
with two sweeps separated by real work, exactly one row -- the newer -- carries the
qualifier. 16 tests pass in that file.

The screenshot is re-captured to show both cases in ONE frame, because a frame
showing only the open stretch could not distinguish the two: the top row reads
checking since 24m ago and the bottom row, also Swept, reads a plain 9h ago.
The capture script now asserts that difference before it shoots, so a regression
that made both rows read alike cannot produce a green screenshot.

On Opus's assessment of the same code

Opus reached the opposite conclusion on this candidate, scoring it below threshold
because "a superseded sweep row appears below the newer work rows ... so the
accurate newer activity is visible above it" and calling the outcome a
product-intent judgment rather than a code-grounded defect. I have gone with UX
here. Its reasoning is not that the reader lacks the information to work out the
truth -- it is that the row states something false, and requiring a reader to
reconcile it against neighbouring rows is the cost. That is a wrong outcome, not a
preference, and the fix is small enough that the "may be acceptable" framing does
not need to be tested on users.

Verification

325 passed across the three targeted crew test files with -n 2 (no backend change
this round); 16 passed in IssueRadarCrewPage.test.tsx. npx tsc -b clean,
npx eslint src/ --max-warnings 603 0 errors at the ceiling, npx jscpd . 0
clones, i18n 19 checks PASS against the merge base, and the baselined black
gate clean.

@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from c93c99e to f76d65d Compare September 1, 2026 18:15
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Review dispositions -- head f76d65d4247433e54ddb416714689ece88345aae

Design Review PASS and Opus 4.8 no findings on c93c99ebb. One item fixed, one
escalated rather than patched.

FIXED -- UX: a paused crew still claimed to be checking

A paused crew's newest sweep row still asserts "checking since..." -- a
present-tense activity claim the page's own paused badge contradicts.

Correct. isOngoing gated on recency but not on whether the crew is running, so a
paused crew's newest sweep kept asserting present-tense activity while the paused
badge a few hundred pixels above said the opposite. Same class as the previous
finding -- a claim contradicted by adjacent UI -- and this closes the last state
that can contradict it, since paused is the only way a crew stops checking.

isOngoing now takes running and returns false when the crew is paused: its
newest sweep becomes a closed stretch like any other and renders as a past
instant. Covered by a new assertion (a paused crew's newest sweep renders the
plain relative time). 17 tests pass in IssueRadarCrewPage.test.tsx.

ESCALATED, not patched -- First Principles: event vs crew-record field

The sweep is stored as an event, then coalescing un-events it -- the author's own
docstring calls it "a latest-value fact, not an event," which is the case for a
crew-record field never weighed.

This is a fair hit and I am not going to paper over it. The docstring does say
exactly that, and the observation that the alternative was never weighed IN WRITING
is accurate -- I decided it while implementing coalescing and only documented the
choice. Worse, the evidence has accumulated since: three of the five advisories on
this PR (stale wording, superseded stretch, paused crew) exist ONLY because a
latest-value fact lives in an append-only log and then has to be rendered as a
state. A crew-record field would have produced none of them.

So here is the weighing, done properly.

Keep the event line (what is implemented). The interleaving is the point: a log
reading sweep -> claimed #2251 -> sweep tells a human the crew went quiet, took
work, and went quiet again, with the boundaries visible. A scalar
last_swept_at cannot express that a stretch began at T and ENDED at T2 -- it only
ever holds "most recently". The issue's sibling acceptance criteria are all about
the ledger, so a ledger line is the reading that matches them. Growth is bounded by
coalescing, which Design Review assessed as proportionate and has now PASSed twice.

Move it to a crew-record field. Strictly simpler: no coalescing, no
read-tail-under-one-lock, no coalesced flag, no open-vs-closed rendering
distinction, and none of the three UX rounds above. It is also honest about what
the datum is. The cost is that the crew page loses the interleaved history and
gains a header value, and the work log stops showing that the crew was awake at
all during a quiet stretch.

I lean toward keeping the event line, because the interleaved boundaries are
information a scalar structurally cannot hold -- but I hold that view weakly, and
the finding is right that it should be a stated decision rather than an implementation
accident. This is a data-model fork that would discard a twice-PASSed
implementation, so it belongs to a maintainer rather than to a sixth patch from me.
Recorded here, deferred, and I will act on whichever way it is ruled.

Nothing about it blocks this PR: First Principles is advisory, the shipped behavior
is correct under the design it has, and the alternative is a clean follow-up rather
than a fix to something broken.

Verification

325 passed across the three targeted crew test files with -n 2 (no backend change
this round); 17 in IssueRadarCrewPage.test.tsx. npx tsc -b clean, eslint 0
errors at the 603 ceiling, npx jscpd . 0 clones, i18n 19 checks PASS against
the merge base, black baseline clean.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from f76d65d to aaf3145 Compare September 1, 2026 22:23
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition: GPT 5.6 blocking finding -- FIXED in aaf314588

Finding. crew_store.py built the ledger entry, and therefore stamped ts, before taking the events lock. Confirmed real and reachable, so it is fixed rather than rebutted.

The chain, verified in code. A sweep stamps ts at T0 and then blocks on the lock. A numbered append_event stamps T1 (later) but wins the lock and appends first. The sweep then appends with the earlier stamp, so it is the file tail while carrying a timestamp that precedes the line above it. The two readers disagree from there:

  • _latest_crew_event reads through read_events, which walks reversed(lines), so it sees the sweep as newest and keeps coalescing onto it. No further sweep line is ever written.
  • CrewPageView.tsx sorts by timestamp ([...events].sort((a, b) => stampMs(b.ts) - stampMs(a.ts))) and takes sorted[0] as the ongoing anchor, so the numbered line wins and the sweep is never the anchor.

Net effect is the one the finding names: the idle stretch stays hidden for as long as the crew keeps idling, which defeats the checking since wording added earlier in this PR for the UX lane.

Fix. Entry construction moved inside the lock hold in both append paths, so the stamp is taken in the same order the lines are appended and file order and timestamp order agree by construction. In record_crew_checkpoint the build now sits after the coalesce check, so a coalesced call no longer builds an entry it discards. _event_entry remains the single place a line is validated and built, per the earlier First Principles finding; its docstring now records that it must be called under the lock and why, so the call cannot be hoisted back out without contradicting the stated contract.

Evidence. New test test_a_sweep_blocked_on_the_lock_is_stamped_after_the_line_it_follows reproduces the race with real contention: it holds the events lock, starts a sweep that blocks on it, appends a numbered line under the hold, then asserts the stamps are ordered to match file order. Mutation-verified against the pre-fix ordering, where it fails with '2026-09-01T22:22:10.526821Z' >= '2026-09-01T22:22:10.576933Z' -- the inversion itself, measured.

326 tests pass across the three affected files. flake8, isort, the baselined black gate and mypy (1237 files) are all clean.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Status on aaf314588: all five review lanes clean, remaining reds are main-owned

Every review lane passed on this head and each marker names it: Design PASS, First Principles PASS, UX PASS, Opus 4.8 no findings, GPT 5.6 no findings. GPT's blocking finding from the previous head was fixed in aaf314588 and dispositioned above.

The reds that remain are not this branch's. Both were reproduced on current origin/main in a detached worktree containing none of this work.

1. Backend Tests shard 3 -- slack/gateway.py census drift

test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor
AssertionError: ... slack/gateway.py: 7 sites, census says 6.

Main commit 7d05970c7 (#7424) added a seventh gate-side baseline-redactor log site in slack/gateway.py without raising _BASELINE_LOG_SITE_CENSUS. Reproduces on origin/main alone with a byte-identical assertion, and passes at this branch's merge-base. This branch touches zero slack files.

It is platform-independent despite first surfacing on Windows: the assertion is a static census of source files, and shard 3 is simply where the test is scheduled. Both Backend Tests (Windows) (3) and Backend Tests (3.10, 3) now show it on this head, so Backend Tests (3.12, 3) is expected to as well.

Fix is up as a separate one-file PR: #7762. It converts the site to redact_log_via_context rather than raising the census, because slack/gateway.py calls boot_platform itself and so is not the no-composition process that redact_log_via_context documents the exemption for.

2. Frontend Lint & Type Check -- eslint ratchet breached on main

Current origin/main alone measures 604 problems (0 errors, 604 warnings) against its own --max-warnings 603 ceiling. The surplus is an unused eslint-disable directive for no-eval. This branch's diff contains no eslint-relevant files. Already covered by a fleet fix in flight, so nothing is duplicated here.

What happens next

Neither red can be cleared by re-running: a rerun replays the original merge ref and never absorbs a base-branch fix. Both clear when the two fixes land and this branch is rebased onto settled main -- one rebase absorbs both. Holding until then rather than pushing, so the review lanes that are currently clean are not re-rolled for nothing.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@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 Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from de190ab to 19cab90 Compare September 2, 2026 01:44
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition: Design and First Principles CONCERNS on de190abe2 -- two fixed, one rebutted, one accepted

Both lanes were advisory (no BLOCK), and all five lanes were clean on this head. Addressed in 19cab903d. Note both lanes returned PASS on aaf314588, whose tree is byte-identical to de190abe2 -- so these arrived on a re-roll, not on new code. Judged on merit rather than on that, since two of them are right.

1. FIXED -- adoption hangs on tool-schema prose alone (First Principles)

Verified before fixing: crew_brief.md had zero occurrences of sweep. The finding is correct and it was the most consequential one here -- the brief is what scripts a crew's cycle, so a capability it never mentions is a capability that may never be invoked. Shipping the API without it risks shipping dead code.

Step 5 of the per-turn protocol already required a ledger write on turns where nothing moved, and that is exactly the instruction a crew with no open work item could not comply with, because every write needed an issue number. It now names the numberless call directly, says when it applies (nothing in scope, or every candidate already claimed or skipped), and states that consecutive sweeps coalesce so an idle crew can call it every turn without growing the log.

2. FIXED -- growth bound at the lock hold (Design, Suggestions)

Also correct, and worse than "note it" implied, so the note says the real shape. read_events does path.read_text().splitlines() and then walks backwards to the first match, so the tail read is a WHOLE-FILE read of the repo-wide ledger, taken under the exclusive write lock. Nothing compacts that file today, so the hold grows with total ledger volume across all crews, not with the calling crew's share. Documented where the lock hold is justified, including that coalescing is what keeps an idle crew from being the thing that grows it, and that a compaction story is still owed.

3. REBUTTED -- store the datum as a last_sweep field on the crew record (First Principles)

This fork was already put to the maintainer and ruled: keep the event line. Re-raising it does not reopen it, so no code change.

The lane's reasoning is fair -- the docstring does call this a latest-value fact, and the list of machinery the log shape costs is accurate. What the ruling weighed against it is that the event line is what makes the work-log row and the cold-resume read work without a second read path, and that a mutable field would need its own history to answer "how long has this crew been idle", which is the question the page exists to answer. Both designs are defensible; one was chosen.

4. ACCEPTED AND DEFERRED -- a dead crew still reads as "checking since" (Design, Watch)

Real, correctly reasoned, and the sharpest item here: it is the same class of false-activity claim this PR exists to remove, resurfacing at the display layer. A crew that dies mid-idle -- agent crash, lost nudge timer -- is not paused, so isOngoing keeps the qualifier, and because coalescing writes nothing the timestamp never advances. It would read "checking since 3d ago" indefinitely.

Not fixed here because there is no liveness signal to fix it with, and I checked rather than assumed. Crew carries no heartbeat or last-seen field; paused is derived from !crew.enabled, which is an operator's intent, not evidence of life. last_progress_at exists only on WorkItem, and an idle crew by definition has no item advancing, so nothing on the wire distinguishes idling from dead.

That leaves three honest options, and each is a decision rather than a detail: add a real per-crew heartbeat (a new primitive, and the last_sweep-style field the ruling above declined for this PR); advance the timestamp on a coalesced sweep (abandons the "stretch began at X" semantic the coalescing rule is built on, and re-opens the runaway-log problem it solved); or weaken the wording to something like "nothing taken since X", which asserts only what the log knows but walks back the ongoing-stretch wording the UX lane asked for and passed. Inventing a liveness signal inside this PR is how the scope stops being reviewable, so this is recorded rather than guessed at.

Residual, stated plainly so it is not discovered later: on this page a crew that stops running without being paused looks like a crew that is idling. Worth noting that is not unique to sweeps -- the same crew's in-flight work items also keep rendering their last phase -- so the correct fix is a liveness signal the page can trust, not a patch to this one string.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from 19cab90 to 529d635 Compare September 2, 2026 02:06
@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 Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition: GPT 5.6 blocking finding on 19cab903d -- FINDING FIXED in 529d63594, REMEDY DECLINED

The finding is real. Verified rather than assumed: set_crew_paused patches only enabled and paused_reason and writes no ledger line, so nothing marked the boundary. Sweep, pause for a day, resume, sweep therefore returned the pre-pause line, and because the timestamp is read as "idle since" the page reported one unbroken "checking since" run across a period the crew was deliberately switched off. That is the same false-activity claim this PR exists to remove, so it is fixed rather than argued with.

The prescribed remedy is declined. "Remove this coalescing return so the post-resume sweep appends a fresh checkpoint" would delete coalescing outright, for every crew and every cycle -- not just across a pause. Coalescing exists because Design Review found the opposite defect earlier in this PR: a crew nudged on a timer appends one line per idle wake, ledger reads are capped and drop the OLDEST line first, so a few hundred idle cycles push a crew's real work history out of its own work log. Removing it trades a rare wrong timestamp for a guaranteed loss of history, which is a worse instance of the same anchor the finding is filed under. So the boundary is marked instead of the folding being abandoned.

What changed. A pause now ends the stretch:

  • The crew record gains paused_at, stamped when a crew is paused. It deliberately SURVIVES a resume, which is the one place it differs from paused_reason -- it is not part of the current state, it is the record of when the last stop happened, and clearing it on resume would erase exactly the fact that makes the boundary decidable.
  • record_crew_checkpoint folds only when paused_at is no newer than the line it would fold into. The comparison is a plain string compare, valid because _now_iso is fixed-width to microseconds and always Z, so ISO-8601 sorts lexicographically.
  • The crew record is read BEFORE the events lock is taken, not inside it. Reading a crew record takes the crew lock, and acquiring these two in that order under the other would invert an ordering another path already relies on.
  • paused_at is listed in _validated_crew_patch for the same reason paused_reason is. Worth stating plainly: forging it cannot fabricate work, its only effect is whether ONE idle line folds, and enabled -- the field that actually stops a crew -- is already patchable.
  • The spec records the rule and its residual alongside the coalescing rule it modifies.

Tests. Three added, and the first is mutation-verified: with the paused_at comparison removed it fails with assert True is False, and with it restored it passes. The other two are the guard against over-correcting -- sweeps still coalesce when the crew was never paused, and a pause OLDER than the stretch does not break it, so the fix cannot quietly become an unconditional append and re-open the log-burial defect.

The first version of the fix was silently a no-op and the test caught it: _validated_crew_patch is an allowlist, so paused_at was dropped on write and the stretch still folded. Worth recording because the failure mode was invisible -- the store returned success and the field simply was not there.

329 tests pass across the three crew suites, and 506 across the suites that touch the crew record (routes, runtime, onboarding import). flake8, isort, the baselined black gate and mypy over 1237 files are clean; 17 frontend tests still pass.

Known residual, unchanged by this. A crew that stops WITHOUT being paused -- a crash, a lost nudge timer -- still cannot be told apart from one that is idling, because nothing on the crew record evidences liveness. That is the Design Review Watch item dispositioned in the previous comment, and it needs a liveness primitive rather than another special case here.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition: Design + First Principles on 529d63594 -- ESCALATED, both are right and they meet at the same choice

Both advisory, nothing blocking, and all five lanes are otherwise clean on this head (GPT and Opus no findings, UX PASS). Not answering these with the earlier residual note at 5503092420, because each adds a fact that note did not weigh.

Design's new fact, and it is the one that matters. Before this change an idle crew's newest row simply AGED -- "3h ago" -- which claims nothing. With the ongoing qualifier the same row now reads "checking since 3h ago", a present-tense assertion of activity backed by no evidence of life. So for the crash and lost-timer modes this is not merely an unfixed gap, as I characterised it earlier: it is a REGRESSION against the status quo, and it lands on precisely the failure mode an operator opens this page to notice. That framing is correct and I withdraw the "residual, unchanged" characterisation.

First Principles' subtraction is coherent too. paused_at closes 1 of 3 stop modes (operator pause) and leaves crash and lost timer open. Its only consumer is the one read in record_crew_checkpoint, and it costs a permanent record field plus the resume-survival exception in set_crew_paused and an entry in _validated_crew_patch. If the qualifier goes, paused_at buys nothing and should go with it.

Why I am not choosing. The two lanes agree on the diagnosis and imply different scopes, and every route out reverses a decision someone else already made -- the ongoing wording exists because the UX lane asked for it and passed on it. Deciding that from here would either flip a UX-requested behaviour unilaterally or grow this PR a liveness primitive it was not scoped for. Design says it directly: humans should decide whether the heartbeat lands before or after this ships. So this is recorded and referred rather than guessed at.

The three coherent options, with what each actually costs:

  1. Ship as-is. The residual is disclosed in the spec and here. Accepts that a crashed crew reads as idling, which is worse than the pre-change display for that one mode.
  2. Drop the ongoing qualifier; keep everything else. Verified available and clean: the non-ongoing branch is already plain relative time, so this deletes isOngoing and its two call sites, one key across 13 catalogs plus the generated pseudolocale, and then paused_at and its three touch points go too -- First Principles' subtraction, executed. The feature's actual purpose, recording an empty cycle without inventing an issue number, is untouched. Cost: reverses the wording the UX lane asked for, so that lane may re-raise it.
  3. Add a real last-seen datum before this ships. Closes all three stop modes honestly and makes the qualifier true. Largest scope, and it is the crew-record-field shape a maintainer already ruled against for the sweep datum -- so it needs that ruling revisited, not worked around.

No code changed for these two items. The paused_at mechanism from 5503258962 stays in place meanwhile, since it is correct for the mode it covers and option 1 or 3 keeps it.

@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from 529d635 to 09a25e1 Compare September 2, 2026 02:36
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Decision on the escalation: option 2 taken -- the ongoing wording is removed, and paused_at with it

Following up on 5503380802, where I laid out three options and referred the choice. The maintainer decision is option 2: drop the "checking since" qualifier and the paused_at mechanism, implemented in 09a25e1ec.

Why, in the reviewers' own terms.

Design Review's regression argument is the deciding one. A present-tense activity claim with no evidence of life is worse than a bare timestamp, and it lands on precisely the failure mode this page exists to surface: a crew that crashes or loses its nudge timer keeps enabled true, so the row asserted ongoing checking for a crew that had stopped. Before this PR the row merely aged visibly. Shipping the qualifier would have made this surface actively mask the thing an operator opens it to notice.

First Principles' matching subtraction follows directly. Once the qualifier is gone, paused_at protects a reading that no longer exists -- it covered 1 of the 3 stop modes (operator pause) and left crash and lost timer open, at the price of a permanent record field with a single consumer. So it is removed rather than kept for partial honesty.

Option 3, a real last-seen datum, is the honest fix for all three modes, but it is new-feature scope rather than a bug fix and belongs in its own change. Option 1 would have shipped a known regression.

What changed.

  • Removed isOngoing, its ongoingId derivation, and both call sites. The WHEN column now renders fmtRelative / shortDate for every kind, sweeps included.
  • Removed the work_log_checking_since key from all 13 catalogs and regenerated the pseudolocale. kind_sweep stays.
  • Removed paused_at from the crew record default, from _validated_crew_patch, from set_crew_paused (restored to its original two-field pairing), and the pre-lock crew read plus the fold condition in record_crew_checkpoint.
  • Rewrote the four frontend tests that asserted the qualifier down to two that assert the contract now in force: a sweep renders as a past instant, and it renders identically whether the crew is running or paused -- the second one exists specifically to stop a future qualifier being gated on !paused, which would fix only the operator-pause mode and still misreport a crashed crew.
  • Replaced the three paused_at store tests with one that pins the simpler fold and asserts the field is gone rather than merely unused.
  • The store docstring and the spec now state what the timestamp does NOT mean, and name the residual: the ledger cannot tell a crew still checking from one that died mid-stretch.
  • Re-captured the committed screenshot. Both sweep rows now read as bare timestamps ("24m ago", "9h ago"); the capture script asserts neither carries a since qualifier, so the evidence cannot silently drift back.

What did not change: the feature. A crew can still record an empty-queue cycle without inventing an issue number -- the numberless PUT /crew/work, the optional number on the MCP tool, the sweep kind, coalescing, the pairing refusals and the em-dash Issue cell are all untouched. What went is one display qualifier and the field that existed to prop it up.

Verification. 719 backend tests pass across the five affected suites; 15 frontend tests. flake8, isort, the baselined black gate and mypy over 1237 files clean. Frontend: tsc -b clean, eslint 603 against the 603 ceiling, jscpd 0 clones, i18n [changed-passthrough] 0 with the base pinned to the merge-base.

Note for the UX lane, which asked for the ongoing wording and passed on it: this reverses that on the strength of Design's regression finding, and the re-run on this head is the arbitration. If UX still considers the qualifier correct, say so on this head and it goes to a human rather than being flipped again from here.

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
@chenmingwei23
chenmingwei23 force-pushed the fix/radar-worker-ledger-5905 branch from 09a25e1 to 2903b88 Compare September 2, 2026 03:11
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition: First Principles CONCERNS on 09a25e1ec -- two fixed, one settled

Advisory only; UX and Design both returned PASS on that head and GPT and Opus found nothing. Addressed in 2903b88f3.

1. FIXED -- the description misstated the shipped shape

Correct catch, and it was my error. The description claimed the diff adds a CREW_LEVEL_EVENT_KINDS set; the diff ships the singular CREW_LEVEL_EVENT_KIND, which is the collapse this lane asked for earlier in this same PR. Rewritten to describe the constant and why a one-member set was rejected.

While fixing it I found the neighbouring bullet was stale in the same way and corrected that too: it claimed append_event accepts number=None, but append_event requires an int and record_crew_checkpoint is the only numberless writer -- which is the whole point of the single-builder change from an earlier round. Two sentences describing a shape the code had moved past.

2. FIXED -- event_kind on record_crew_checkpoint was dead generality

Verified before removing: the route 400s a non-crew-level kind on the numberless path (number_required), so by the time the store is reached the argument provably holds CREW_LEVEL_EVENT_KIND. It existed to be re-checked and rejected, not chosen -- the same shape as the one-member set.

The parameter is gone and the constant is written directly at the build site, which also removes the CrewStoreError guard that policed it: a wrong kind is now unrepresentable rather than refused at runtime. The number/kind pairing is untouched and still enforced in _event_entry, covered in both directions by the tests above it.

The test that asserted the removed guard is replaced rather than dropped, by one that pins the narrowed signature and the resulting shape -- so a future revision cannot quietly widen the parameter back without the reason being restated.

3. SETTLED -- the event-plane versus a latest-value field on the crew record

No code change. This is the third round for this fork and it has now been decided twice by a human: once when the event line was chosen, and again in the ruling recorded at 5503493585, which took the subtraction option and explicitly put a real last-seen datum outside this PR as new-feature scope rather than a bug fix.

The lane's framing is fair -- the docstring does call the datum a latest-value fact, the machinery listed is real, and the lane is right that the plane is a human's call. It has had one. Reopening it here would re-litigate a decision rather than surface a defect, so this is answered by pointing at it.

Verification. 450 tests pass across the four affected suites. flake8, isort, the baselined black gate and mypy over 1237 source files are clean. The parameter removal touched 13 call sites; one was missed on the first pass and the suite caught it immediately, which is the argument for running the full affected set rather than the changed file alone.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition: First Principles CONCERNS on 2903b88f3 -- both items already answered, no code change

Advisory only. UX and Design are PASS on this head, GPT and Opus found nothing, and the earlier Subtraction from this lane (the dead event_kind parameter) is fixed in this commit.

1. A last_sweep field on the crew record instead of the event line. Fourth round for this fork, and it has been decided twice by a human: once when the event line was chosen, and again in the ruling at 5503493585, which took the subtraction option and put a real last-seen datum outside this PR as new-feature scope. The lane's reasoning is not wrong -- the docstring does concede the datum is a latest-value fact -- but the plane is a decision, it has been made, and re-litigating it here would not surface a defect. Full reasoning at 5503380802 and 5503754882.

2. Every idle nudge reads the whole repo-wide events file under the exclusive lock. Correct, and this lane is reading a note this PR added on purpose: Design raised the cost, and the fix was to state it honestly where the lock hold is justified rather than to hide it -- including that nothing compacts that file today and that a compaction story is still owed. Coalescing is what keeps an idle crew from being the thing that grows it: one append per stretch, not one per wake. Disposition at 5503092420.

Nothing here is new, so nothing changes in the diff. Recording it because silence is not a disposition.

For the record on this head: 68 checks green, zero red, zero pending, and PR Readiness reports success. Not merging -- that is a maintainer's call.

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 number tolerates absence.
    • crew_store.read_events reads with .get, dedupes by id, tolerates a missing key; it never touches number, so a sweep line passes through cleanly (crew_store.py).
    • The fabric fold (crew_store.py, by_key.setdefault((cid, num), ...)) guards with isinstance(num, int) AND reads with require_phase=True; a sweep carries no phase, so it is excluded twice.
    • _fabric_title_hints and the work-item fold both guard isinstance(num, int) before using num as a dict key.
    • The skip index is untouched: a sweep writes no skip entry (record_crew_checkpoint returns skip=None), so read_skips/skipped_numbers never see it.
    • Backend hands raw event dicts straight to the frontend (crew_routes.py:590), so the only UI consumer is the renderer: CrewPageView.tsx issueCell(number) returns an em dash when number === undefined and #${number} otherwise. Both render sites (recent + history tables) were switched from the old #{e.number}, so no path prints #undefined. CrewEvent.number is number? (optional, not nullable) in api.ts, matching the backend omitting the key.
  • Storage round-trips and old records still load. The line is written by json.dumps with the key simply absent and read back by read_events, which requires no number; a pre-change numbered line still loads unchanged.
  • The numbered path is byte-identical. _event_id renders int(number) for a numbered line (unchanged), _event_entry preserves the original key order (id, ts, crew_id, number, kind, text, [phase]), append_event still types number as required int, 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_checkpoint reads the crew's newest line and appends only if it is not already a sweep, both under one exclusive events-lock hold (crew_store.py:1236); coalesced is returned to the caller. _latest_crew_event uses read_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_request asserts "number" not in body; the route test asserts assertNotIn("number", ledger[0]) and coalesced is False; test_a_numbered_call_still_sends_and_reports_its_number asserts body["number"] == 12. These fail if the change is reverted (consistent with the PR's reported 14 RED on origin/main).
  • Schema/docs match the contract. validation.py keeps the min_val=1/max_val bound and only drops required=True. The MCP tool description (mcp_tools/apps.py) tells the agent to send event_kind: sweep with no number and no work-item fields, and the summary path branches on "number" in args so both args["number"] reads are guarded (no KeyError). Ledger spec and crew brief are updated in the same commit.

Findings (non-blocking)

  1. 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.

  2. 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 a last_swept_at field 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.

@bolichen97
bolichen97 enabled auto-merge September 2, 2026 18:20
@iamwhatever
iamwhatever disabled auto-merge September 2, 2026 20:56
@iamwhatever
iamwhatever merged commit 3402ba9 into main Sep 2, 2026
110 of 119 checks passed
@iamwhatever
iamwhatever deleted the fix/radar-worker-ledger-5905 branch September 2, 2026 20:57
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 2, 2026
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.

3 participants